This change supports multiple CRTCs, encoders, connectors instead of one
of each per card.
Since ConfigFS-based devices will support multiple crtcs, it's useful to
move all of the writeback/composition data from being a per-output thing
to being a per-CRTC thing.
Since there's still only ever one CRTC, this should be a no-op refactor.
Signed-off-by: Jim Shargo <[email protected]>
---
drivers/gpu/drm/vkms/vkms_composer.c | 28 +++----
drivers/gpu/drm/vkms/vkms_crtc.c | 91 +++++++++++----------
drivers/gpu/drm/vkms/vkms_drv.c | 3 +-
drivers/gpu/drm/vkms/vkms_drv.h | 66 ++++++++++------
drivers/gpu/drm/vkms/vkms_output.c | 110 ++++++++++++++++++--------
drivers/gpu/drm/vkms/vkms_plane.c | 39 ++++++---
drivers/gpu/drm/vkms/vkms_writeback.c | 25 +++---
7 files changed, 225 insertions(+), 137 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index c6a1036bf2ea..d27c0831d857 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -221,7 +221,7 @@ void vkms_composer_worker(struct work_struct *work)
struct vkms_crtc_state,
composer_work);
struct drm_crtc *crtc = crtc_state->base.crtc;
- struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
struct vkms_composer *primary_composer = NULL;
struct vkms_plane_state *act_plane = NULL;
bool crc_pending, wb_pending;
@@ -230,7 +230,7 @@ void vkms_composer_worker(struct work_struct *work)
u64 frame_start, frame_end;
int ret;
- spin_lock_irq(&out->composer_lock);
+ spin_lock_irq(&vkms_crtc->composer_lock);
frame_start = crtc_state->frame_start;
frame_end = crtc_state->frame_end;
crc_pending = crtc_state->crc_pending;
@@ -238,7 +238,7 @@ void vkms_composer_worker(struct work_struct *work)
crtc_state->frame_start = 0;
crtc_state->frame_end = 0;
crtc_state->crc_pending = false;
- spin_unlock_irq(&out->composer_lock);
+ spin_unlock_irq(&vkms_crtc->composer_lock);
/*
* We raced with the vblank hrtimer and previous work already computed
@@ -270,10 +270,10 @@ void vkms_composer_worker(struct work_struct *work)
crc32 = compute_crc(vaddr_out, primary_composer);
if (wb_pending) {
- drm_writeback_signal_completion(&out->wb_connector, 0);
- spin_lock_irq(&out->composer_lock);
+ drm_writeback_signal_completion(&vkms_crtc->wb_connector, 0);
+ spin_lock_irq(&vkms_crtc->composer_lock);
crtc_state->wb_pending = false;
- spin_unlock_irq(&out->composer_lock);
+ spin_unlock_irq(&vkms_crtc->composer_lock);
} else {
kfree(vaddr_out);
}
@@ -325,25 +325,25 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
return 0;
}
-void vkms_set_composer(struct vkms_output *out, bool enabled)
+void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled)
{
bool old_enabled;
if (enabled)
- drm_crtc_vblank_get(&out->crtc);
+ drm_crtc_vblank_get(&vkms_crtc->base);
- spin_lock_irq(&out->lock);
- old_enabled = out->composer_enabled;
- out->composer_enabled = enabled;
- spin_unlock_irq(&out->lock);
+ spin_lock_irq(&vkms_crtc->lock);
+ old_enabled = vkms_crtc->composer_enabled;
+ vkms_crtc->composer_enabled = enabled;
+ spin_unlock_irq(&vkms_crtc->lock);
if (old_enabled)
- drm_crtc_vblank_put(&out->crtc);
+ drm_crtc_vblank_put(&vkms_crtc->base);
}
int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
{
- struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+ struct vkms_crtc *out = drm_crtc_to_vkms_crtc(crtc);
bool enabled = false;
int ret = 0;
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index c1b632952532..d93678d984ae 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -11,35 +11,34 @@
static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
{
- struct vkms_output *output = container_of(timer, struct vkms_output,
- vblank_hrtimer);
- struct drm_crtc *crtc = &output->crtc;
+ struct vkms_crtc *vkms_crtc = timer_to_vkms_crtc(timer);
+ struct drm_crtc *crtc = &vkms_crtc->base;
struct vkms_crtc_state *state;
u64 ret_overrun;
bool ret, fence_cookie;
fence_cookie = dma_fence_begin_signalling();
- ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
- output->period_ns);
+ ret_overrun = hrtimer_forward_now(&vkms_crtc->vblank_hrtimer,
+ vkms_crtc->period_ns);
if (ret_overrun != 1)
pr_warn("%s: vblank timer overrun\n", __func__);
- spin_lock(&output->lock);
+ spin_lock(&vkms_crtc->lock);
ret = drm_crtc_handle_vblank(crtc);
if (!ret)
DRM_ERROR("vkms failure on handling vblank");
- state = output->composer_state;
- spin_unlock(&output->lock);
+ state = vkms_crtc->composer_state;
+ spin_unlock(&vkms_crtc->lock);
- if (state && output->composer_enabled) {
+ if (state && vkms_crtc->composer_enabled) {
u64 frame = drm_crtc_accurate_vblank_count(crtc);
/* update frame_start only if a queued vkms_composer_worker()
* has read the data
*/
- spin_lock(&output->composer_lock);
+ spin_lock(&vkms_crtc->composer_lock);
if (!state->crc_pending)
state->frame_start = frame;
else
@@ -47,9 +46,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
state->frame_start, frame);
state->frame_end = frame;
state->crc_pending = true;
- spin_unlock(&output->composer_lock);
+ spin_unlock(&vkms_crtc->composer_lock);
- ret = queue_work(output->composer_workq, &state->composer_work);
+ ret = queue_work(vkms_crtc->composer_workq, &state->composer_work);
if (!ret)
DRM_DEBUG_DRIVER("Composer worker already queued\n");
}
@@ -62,28 +61,24 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
static int vkms_enable_vblank(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
- struct vkms_card *card = drm_device_to_vkms_card(dev);
- struct vkms_output *out = &card->output;
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
unsigned int pipe = drm_crtc_index(crtc);
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
drm_calc_timestamping_constants(crtc, &crtc->mode);
- hrtimer_init(&out->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
- out->vblank_hrtimer.function = &vkms_vblank_simulate;
- out->period_ns = ktime_set(0, vblank->framedur_ns);
- hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_REL);
+ hrtimer_init(&vkms_crtc->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ vkms_crtc->vblank_hrtimer.function = &vkms_vblank_simulate;
+ vkms_crtc->period_ns = ktime_set(0, vblank->framedur_ns);
+ hrtimer_start(&vkms_crtc->vblank_hrtimer, vkms_crtc->period_ns, HRTIMER_MODE_REL);
return 0;
}
static void vkms_disable_vblank(struct drm_crtc *crtc)
{
- struct drm_device *dev = crtc->dev;
- struct vkms_card *card = drm_device_to_vkms_card(dev);
- struct vkms_output *out = &card->output;
-
- hrtimer_cancel(&out->vblank_hrtimer);
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
+ hrtimer_cancel(&vkms_crtc->vblank_hrtimer);
}
static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
@@ -91,8 +86,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
bool in_vblank_irq)
{
struct drm_device *dev = crtc->dev;
- struct vkms_card *card = drm_device_to_vkms_card(dev);
- struct vkms_output *output = &card->output;
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
unsigned int pipe = crtc->index;
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
@@ -101,7 +95,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
return true;
}
- *vblank_time = READ_ONCE(output->vblank_hrtimer.node.expires);
+ *vblank_time = READ_ONCE(vkms_crtc->vblank_hrtimer.node.expires);
if (WARN_ON(*vblank_time == vblank->time))
return true;
@@ -113,7 +107,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
* the vblank core expects. Therefore we need to always correct the
* timestampe by one frame.
*/
- *vblank_time -= output->period_ns;
+ *vblank_time -= vkms_crtc->period_ns;
return true;
}
@@ -240,18 +234,18 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
- struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
/* This lock is held across the atomic commit to block vblank timer
* from scheduling vkms_composer_worker until the composer is updated
*/
- spin_lock_irq(&vkms_output->lock);
+ spin_lock_irq(&vkms_crtc->lock);
}
static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
- struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
if (crtc->state->event) {
spin_lock(&crtc->dev->event_lock);
@@ -266,9 +260,9 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
crtc->state->event = NULL;
}
- vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
+ vkms_crtc->composer_state = to_vkms_crtc_state(crtc->state);
- spin_unlock_irq(&vkms_output->lock);
+ spin_unlock_irq(&vkms_crtc->lock);
}
static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
@@ -279,27 +273,38 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
.atomic_disable = vkms_crtc_atomic_disable,
};
-int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
+struct vkms_crtc *vkms_crtc_init(struct vkms_card *card,
struct drm_plane *primary, struct drm_plane *cursor)
{
- struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
+ struct drm_device *dev = &card->drm;
+ struct vkms_crtc *vkms_crtc;
int ret;
- ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
+ if (card->output.num_crtcs >= VKMS_MAX_OUTPUT_OBJECTS) {
+ return ERR_PTR(ENOMEM);
+ }
+ vkms_crtc = &card->output.crtcs[card->output.num_crtcs++];
+
+ ret = drm_crtc_init_with_planes(dev, &vkms_crtc->base, primary, cursor,
&vkms_crtc_funcs, NULL);
if (ret) {
DRM_ERROR("Failed to init CRTC\n");
- return ret;
+ goto out_error;
}
- drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
+ drm_crtc_helper_add(&vkms_crtc->base, &vkms_crtc_helper_funcs);
- spin_lock_init(&vkms_out->lock);
- spin_lock_init(&vkms_out->composer_lock);
+ spin_lock_init(&vkms_crtc->lock);
+ spin_lock_init(&vkms_crtc->composer_lock);
- vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
- if (!vkms_out->composer_workq)
- return -ENOMEM;
+ vkms_crtc->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
+ if (!vkms_crtc->composer_workq)
+ goto out_error;
+
+ return vkms_crtc;
- return ret;
+out_error:
+ memset(vkms_crtc, 0, sizeof(*vkms_crtc));
+ card->output.num_crtcs -= 1;
+ return ERR_PTR(ret);
}
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 92fbade2199b..266c9f643f70 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -57,7 +57,8 @@ static void vkms_release(struct drm_device *dev)
{
struct vkms_card *card = drm_device_to_vkms_card(dev);
- destroy_workqueue(card->output.composer_workq);
+ for (int i = 0; i < card->output.num_crtcs; i++)
+ destroy_workqueue(card->output.crtcs[i].composer_workq);
}
static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 1a98b81d6f22..30ae06d7af71 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -23,6 +23,8 @@
#define NUM_OVERLAY_PLANES 8
+#define VKMS_MAX_OUTPUT_OBJECTS 16
+
struct vkms_writeback_job {
struct iosys_map map[DRM_FORMAT_MAX_PLANES];
struct iosys_map data[DRM_FORMAT_MAX_PLANES];
@@ -51,6 +53,27 @@ struct vkms_plane {
struct drm_plane base;
};
+struct vkms_output;
+
+struct vkms_crtc {
+ struct drm_crtc base;
+
+ struct drm_writeback_connector wb_connector;
+ struct hrtimer vblank_hrtimer;
+ ktime_t period_ns;
+ struct drm_pending_vblank_event *event;
+ /* ordered wq for composer_work */
+ struct workqueue_struct *composer_workq;
+ /* protects concurrent access to composer */
+ spinlock_t lock;
+
+ /* protected by @lock */
+ bool composer_enabled;
+ struct vkms_crtc_state *composer_state;
+
+ spinlock_t composer_lock;
+};
+
/**
* vkms_crtc_state - Driver specific CRTC state
* @base: base CRTC state
@@ -75,23 +98,14 @@ struct vkms_crtc_state {
};
struct vkms_output {
- struct drm_crtc crtc;
- struct drm_encoder encoder;
- struct drm_connector connector;
- struct drm_writeback_connector wb_connector;
- struct hrtimer vblank_hrtimer;
- ktime_t period_ns;
- struct drm_pending_vblank_event *event;
- /* ordered wq for composer_work */
- struct workqueue_struct *composer_workq;
- /* protects concurrent access to composer */
- spinlock_t lock;
-
- /* protected by @lock */
- bool composer_enabled;
- struct vkms_crtc_state *composer_state;
-
- spinlock_t composer_lock;
+ int num_crtcs;
+ struct vkms_crtc crtcs[VKMS_MAX_OUTPUT_OBJECTS];
+ int num_encoders;
+ struct drm_encoder encoders[VKMS_MAX_OUTPUT_OBJECTS];
+ int num_connectors;
+ struct drm_connector connectors[VKMS_MAX_OUTPUT_OBJECTS];
+ int num_planes;
+ struct vkms_plane planes[VKMS_MAX_OUTPUT_OBJECTS];
};
struct vkms_device;
@@ -120,12 +134,15 @@ struct vkms_device {
struct vkms_config config;
};
-#define drm_crtc_to_vkms_output(target) \
- container_of(target, struct vkms_output, crtc)
+#define drm_crtc_to_vkms_crtc(crtc)\
+ container_of(crtc, struct vkms_crtc, base)
-#define drm_device_to_vkms_card(target) \
+#define drm_device_to_vkms_card(target) \
container_of(target, struct vkms_card, drm)
+#define timer_to_vkms_crtc(timer)\
+ container_of(timer, struct vkms_crtc, vblank_hrtimer)
+
#define to_vkms_crtc_state(target)\
container_of(target, struct vkms_crtc_state, base)
@@ -137,14 +154,13 @@ struct vkms_card *vkms_card_init(const char *name, bool is_default);
void vkms_card_destroy(struct vkms_card *card);
/* CRTC */
-int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
- struct drm_plane *primary, struct drm_plane *cursor);
+struct vkms_crtc *vkms_crtc_init(struct vkms_card *card, struct drm_plane *primary, struct drm_plane *cursor);
int vkms_output_init_default(struct vkms_card *card, int index);
int vkms_output_init(struct vkms_card *card, int index);
struct vkms_plane *vkms_plane_init(struct vkms_card *card,
- enum drm_plane_type type, int index);
+ enum drm_plane_type type);
/* CRC Support */
const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
@@ -155,9 +171,9 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
/* Composer Support */
void vkms_composer_worker(struct work_struct *work);
-void vkms_set_composer(struct vkms_output *out, bool enabled);
+void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled);
/* Writeback */
-int vkms_enable_writeback_connector(struct vkms_card *card);
+int vkms_enable_writeback_connector(struct vkms_card *card, struct vkms_crtc *vkms_crtc);
#endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index dafd47c0a54d..2b72d8e763a8 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -32,19 +32,44 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
.get_modes = vkms_conn_get_modes,
};
-static int vkms_add_overlay_plane(struct vkms_card *card, int index,
- struct drm_crtc *crtc)
+static struct drm_connector *vkms_connector_init(struct vkms_card *card)
{
- struct vkms_plane *overlay;
+ struct drm_connector *connector;
+ int ret;
- overlay = vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY, index);
- if (IS_ERR(overlay))
- return PTR_ERR(overlay);
+ if (card->output.num_connectors >= VKMS_MAX_OUTPUT_OBJECTS)
+ return ERR_PTR(ENOMEM);
- if (!overlay->base.possible_crtcs)
- overlay->base.possible_crtcs = drm_crtc_mask(crtc);
+ connector = &card->output.connectors[card->output.num_connectors++];
+ ret = drm_connector_init(&card->drm, connector, &vkms_connector_funcs,
+ DRM_MODE_CONNECTOR_VIRTUAL);
+ if (ret) {
+ memset(connector, 0, sizeof(*connector));
+ card->output.num_connectors -= 1;
+ return ERR_PTR(ret);
+ }
- return 0;
+ drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
+
+ return connector;
+}
+
+static struct drm_encoder *vkms_encoder_init(struct vkms_card *card)
+{
+ struct drm_encoder *encoder;
+ int ret;
+
+ if (card->output.num_encoders >= VKMS_MAX_OUTPUT_OBJECTS)
+ return ERR_PTR(ENOMEM);
+
+ encoder = &card->output.encoders[card->output.num_encoders++];
+ ret = drm_simple_encoder_init(&card->drm, encoder, DRM_MODE_ENCODER_VIRTUAL);
+ if (ret) {
+ memset(encoder, 0, sizeof(*encoder));
+ card->output.num_encoders -= 1;
+ return ERR_PTR(ret);
+ }
+ return encoder;
}
int vkms_output_init_default(struct vkms_card *card, int index)
@@ -52,51 +77,65 @@ int vkms_output_init_default(struct vkms_card *card, int index)
const struct vkms_config *config = &card->vkms_device->config;
struct vkms_output *output = &card->output;
struct drm_device *dev = &card->drm;
- struct drm_connector *connector = &output->connector;
- struct drm_encoder *encoder = &output->encoder;
- struct drm_crtc *crtc = &output->crtc;
+ struct drm_connector *connector;
+ struct drm_encoder *encoder;
+ struct vkms_crtc *vkms_crtc;
struct vkms_plane *primary, *cursor = NULL;
int ret;
int writeback;
unsigned int n;
- primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY, index);
+ primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY);
if (IS_ERR(primary))
return PTR_ERR(primary);
if (config->overlay) {
for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
- ret = vkms_add_overlay_plane(card, index, crtc);
- if (ret)
- return ret;
+ struct vkms_plane *overlay =
+ vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY);
+ if (IS_ERR(overlay)) {
+ ret = PTR_ERR(overlay);
+ goto err_planes;
+ }
}
}
if (config->cursor) {
- cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR, index);
- if (IS_ERR(cursor))
- return PTR_ERR(cursor);
+ cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR);
+ if (IS_ERR(cursor)) {
+ ret = PTR_ERR(cursor);
+ goto err_planes;
+ }
}
- ret = vkms_crtc_init(dev, crtc, &primary->base, &cursor->base);
- if (ret)
- return ret;
+ vkms_crtc = vkms_crtc_init(card, &primary->base,
+ cursor ? &cursor->base : NULL);
+ if (IS_ERR(vkms_crtc)) {
+ DRM_ERROR("Failed to init crtc\n");
+ ret = PTR_ERR(vkms_crtc);
+ goto err_planes;
+ }
- ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
- DRM_MODE_CONNECTOR_VIRTUAL);
- if (ret) {
+ for (int i = 0; i < card->output.num_planes; i++) {
+ card->output.planes[i].base.possible_crtcs |=
+ drm_crtc_mask(&vkms_crtc->base);
+ }
+
+ connector = vkms_connector_init(card);
+ if (IS_ERR(connector)) {
DRM_ERROR("Failed to init connector\n");
+ ret = PTR_ERR(connector);
goto err_connector;
}
- drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
-
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_VIRTUAL);
- if (ret) {
+ encoder = vkms_encoder_init(card);
+ if (IS_ERR(encoder)) {
DRM_ERROR("Failed to init encoder\n");
+ ret = PTR_ERR(encoder);
goto err_encoder;
}
- encoder->possible_crtcs = 1;
+ encoder->possible_crtcs |= drm_crtc_mask(&vkms_crtc->base);
+
ret = drm_connector_attach_encoder(connector, encoder);
if (ret) {
@@ -105,7 +144,7 @@ int vkms_output_init_default(struct vkms_card *card, int index)
}
if (config->writeback) {
- writeback = vkms_enable_writeback_connector(card);
+ writeback = vkms_enable_writeback_connector(card, vkms_crtc);
if (writeback)
DRM_ERROR("Failed to init writeback connector\n");
}
@@ -121,7 +160,14 @@ int vkms_output_init_default(struct vkms_card *card, int index)
drm_connector_cleanup(connector);
err_connector:
- drm_crtc_cleanup(crtc);
+ drm_crtc_cleanup(&vkms_crtc->base);
+
+err_planes:
+ for (int i = 0; i < output->num_planes; i++) {
+ drm_plane_cleanup(&output->planes[i].base);
+ }
+
+ memset(output, 0, sizeof(struct vkms_output));
return ret;
}
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 28abd61a0bb9..5f49257fe16e 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -7,6 +7,7 @@
#include <drm/drm_fourcc.h>
#include <drm/drm_gem_atomic_helper.h>
#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_plane.h>
#include <drm/drm_plane_helper.h>
#include "vkms_drv.h"
@@ -65,6 +66,19 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
kfree(vkms_state);
}
+static void vkms_plane_destroy(struct drm_plane *plane)
+{
+ struct vkms_plane *vkms_plane =
+ container_of(plane, struct vkms_plane, base);
+
+ if (plane->state) {
+ vkms_plane_destroy_state(plane, plane->state);
+ plane->state = NULL;
+ }
+
+ memset(vkms_plane, 0, sizeof(struct vkms_plane));
+}
+
static void vkms_plane_reset(struct drm_plane *plane)
{
struct vkms_plane_state *vkms_state;
@@ -86,8 +100,9 @@ static void vkms_plane_reset(struct drm_plane *plane)
static const struct drm_plane_funcs vkms_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
- .reset = vkms_plane_reset,
- .atomic_duplicate_state = vkms_plane_duplicate_state,
+ .destroy = vkms_plane_destroy,
+ .reset = vkms_plane_reset,
+ .atomic_duplicate_state = vkms_plane_duplicate_state,
.atomic_destroy_state = vkms_plane_destroy_state,
};
@@ -159,13 +174,20 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
};
struct vkms_plane *vkms_plane_init(struct vkms_card *card,
- enum drm_plane_type type, int index)
+ enum drm_plane_type type)
{
struct drm_device *dev = &card->drm;
+ struct vkms_output *output = &card->output;
const struct drm_plane_helper_funcs *funcs;
struct vkms_plane *plane;
const u32 *formats;
int nformats;
+ int ret;
+
+ if (output->num_planes >= VKMS_MAX_OUTPUT_OBJECTS)
+ return ERR_PTR(ENOMEM);
+
+ plane = &output->planes[output->num_planes++];
switch (type) {
case DRM_PLANE_TYPE_PRIMARY:
@@ -186,12 +208,11 @@ struct vkms_plane *vkms_plane_init(struct vkms_card *card,
break;
}
- plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 << index,
- &vkms_plane_funcs,
- formats, nformats,
- NULL, type, NULL);
- if (IS_ERR(plane))
- return plane;
+ ret = drm_universal_plane_init(dev, &plane->base, 0,
+ &vkms_plane_funcs, formats, nformats,
+ NULL, type, NULL);
+ if (ret)
+ return ERR_PTR(ret);
drm_plane_helper_add(&plane->base, funcs);
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 681e7267d688..68c7a77ab93f 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0+
#include <linux/iosys-map.h>
+#include <linux/kernel.h>
#include <drm/drm_atomic.h>
#include <drm/drm_fourcc.h>
@@ -94,15 +95,14 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
struct drm_writeback_job *job)
{
struct vkms_writeback_job *vkmsjob = job->priv;
- struct vkms_card *card;
+ struct vkms_crtc *vkms_crtc = container_of(connector, struct vkms_crtc, wb_connector);
if (!job->fb)
return;
drm_gem_fb_vunmap(job->fb, vkmsjob->map);
- card = drm_device_to_vkms_card(job->fb->dev);
- vkms_set_composer(&card->output, false);
+ vkms_set_composer(vkms_crtc, false);
kfree(vkmsjob);
}
@@ -111,21 +111,20 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
{
struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
conn);
- struct vkms_card *card = drm_device_to_vkms_card(conn->dev);
- struct vkms_output *output = &card->output;
- struct drm_writeback_connector *wb_conn = &output->wb_connector;
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(connector_state->crtc);
+ struct drm_writeback_connector *wb_conn = &vkms_crtc->wb_connector;
struct drm_connector_state *conn_state = wb_conn->base.state;
- struct vkms_crtc_state *crtc_state = output->composer_state;
+ struct vkms_crtc_state *crtc_state = vkms_crtc->composer_state;
if (!conn_state)
return;
- vkms_set_composer(&card->output, true);
+ vkms_set_composer(vkms_crtc, true);
- spin_lock_irq(&output->composer_lock);
+ spin_lock_irq(&vkms_crtc->composer_lock);
crtc_state->active_writeback = conn_state->writeback_job->priv;
crtc_state->wb_pending = true;
- spin_unlock_irq(&output->composer_lock);
+ spin_unlock_irq(&vkms_crtc->composer_lock);
drm_writeback_queue_job(wb_conn, connector_state);
}
@@ -136,11 +135,11 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
.atomic_commit = vkms_wb_atomic_commit,
};
-int vkms_enable_writeback_connector(struct vkms_card *card)
+int vkms_enable_writeback_connector(struct vkms_card *card, struct vkms_crtc *vkms_crtc)
{
- struct drm_writeback_connector *wb = &card->output.wb_connector;
+ struct drm_writeback_connector *wb = &vkms_crtc->wb_connector;
- card->output.wb_connector.encoder.possible_crtcs = 1;
+ vkms_crtc->wb_connector.encoder.possible_crtcs = 1;
drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
return drm_writeback_connector_init(&card->drm, wb,
--
2.37.1.359.gd136c6c3e2-goog
On Fri, Jul 22, 2022 at 05:32:10PM -0400, Jim Shargo wrote:
> This is a small refactor to make ConfigFS support easier.
>
> We now store the vkms_device statically, and maintain a list of
> "cards", each representing a different virtual DRM driver.
>
> We also make it clear when a card is "default", that is created at
> initialization, and not. This is because, due to limitations on what we
> can do with the configfs API, the default card won't be reflected in
> configfs and is initialized in a unique way.
>
> Since we're only managing a single card, this should be a no-op.
>
> Signed-off-by: Jim Shargo <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 11 +-
> drivers/gpu/drm/vkms/vkms_drv.c | 160 ++++++++++++++++----------
> drivers/gpu/drm/vkms/vkms_drv.h | 32 ++++--
> drivers/gpu/drm/vkms/vkms_output.c | 25 ++--
> drivers/gpu/drm/vkms/vkms_plane.c | 4 +-
> drivers/gpu/drm/vkms/vkms_writeback.c | 20 ++--
> 6 files changed, 158 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 57bbd32e9beb..c1b632952532 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -62,9 +62,10 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> static int vkms_enable_vblank(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *out = &card->output;
> unsigned int pipe = drm_crtc_index(crtc);
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> - struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>
> drm_calc_timestamping_constants(crtc, &crtc->mode);
>
> @@ -78,7 +79,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
>
> static void vkms_disable_vblank(struct drm_crtc *crtc)
> {
> - struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> + struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *out = &card->output;
>
> hrtimer_cancel(&out->vblank_hrtimer);
> }
> @@ -88,9 +91,9 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> bool in_vblank_irq)
> {
> struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *output = &card->output;
> unsigned int pipe = crtc->index;
> - struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> - struct vkms_output *output = &vkmsdev->output;
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>
> if (!READ_ONCE(vblank->enabled)) {
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 81ed9417e511..92fbade2199b 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -9,9 +9,9 @@
> * the GPU in DRM API tests.
> */
>
> +#include <linux/kernel.h>
Unless you're using something directly from this header, it's probably best not to include it as it pulls in a _lot_ of stuff.
> #include <linux/module.h>
> #include <linux/platform_device.h>
> -#include <linux/dma-mapping.h>
>
> #include <drm/drm_gem.h>
> #include <drm/drm_atomic.h>
> @@ -37,7 +37,7 @@
> #define DRIVER_MAJOR 1
> #define DRIVER_MINOR 0
>
> -static struct vkms_device *vkms_device;
> +static struct vkms_device vkms_device;
>
> static bool enable_cursor = true;
> module_param_named(enable_cursor, enable_cursor, bool, 0444);
> @@ -55,9 +55,9 @@ DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>
> static void vkms_release(struct drm_device *dev)
> {
> - struct vkms_device *vkms = drm_device_to_vkms_device(dev);
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
>
> - destroy_workqueue(vkms->output.composer_workq);
> + destroy_workqueue(card->output.composer_workq);
> }
>
> static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> @@ -91,9 +91,9 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
>
> static int vkms_config_show(struct seq_file *m, void *data)
> {
> - seq_printf(m, "writeback=%d\n", vkms_device->config.writeback);
> - seq_printf(m, "cursor=%d\n", vkms_device->config.cursor);
> - seq_printf(m, "overlay=%d\n", vkms_device->config.overlay);
> + seq_printf(m, "writeback=%d\n", vkms_device.config.writeback);
> + seq_printf(m, "cursor=%d\n", vkms_device.config.cursor);
> + seq_printf(m, "overlay=%d\n", vkms_device.config.overlay);
>
> return 0;
> }
> @@ -133,9 +133,9 @@ static const struct drm_mode_config_helper_funcs vkms_mode_config_helpers = {
> .atomic_commit_tail = vkms_atomic_commit_tail,
> };
>
> -static int vkms_modeset_init(struct vkms_device *vkmsdev)
> +static int vkms_modeset_init(struct vkms_card *card)
> {
> - struct drm_device *dev = &vkmsdev->drm;
> + struct drm_device *dev = &card->drm;
>
> drm_mode_config_init(dev);
> dev->mode_config.funcs = &vkms_mode_funcs;
> @@ -151,89 +151,133 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> dev->mode_config.preferred_depth = 0;
> dev->mode_config.helper_private = &vkms_mode_config_helpers;
>
> - return vkms_output_init(vkmsdev, 0);
> + return card->is_default ? vkms_output_init_default(card, 0) : -ENOTSUPP;
AFAICT if card is not default this failure will roll up to vkms_card_init and to vkms_init() which will cause everything to get torn down. So we can probably check at the start of the function and fail early.
Also, minor nit: Ternary operators are usually discouraged.
> }
>
> -static int vkms_create(void)
> +static void vkms_device_destroy(void)
> {
> - int ret;
> + struct vkms_card *card, *n;
> +
> + list_for_each_entry_safe(card, n, &vkms_device.cards, node) {
> + vkms_card_destroy(card);
> + }
Nit: no braces
> +
> + memset(&vkms_device, 0, sizeof(struct vkms_device));
Nit: You could do sizeof(vkms_device) here
That said, this is either unnecessary or not a good idea since it blows away at least the backpointer to platform_device, clears drm_device, and other items which have proper destroy/clean-up functions. So we either don't need this, or we need to clean up everything in vkms_device explicitly.
> +}
> +
> +struct vkms_card *vkms_card_init(const char *name, bool is_default)
> +{
> + char unique_name[64] = { 0 };
> struct platform_device *pdev;
> + struct vkms_card *card;
> + void *grp;
> + int ret;
>
> - pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> + ret = snprintf(unique_name, ARRAY_SIZE(unique_name), "%s-%s",
> + DRIVER_NAME, name);
> + if (ARRAY_SIZE(unique_name) <= ret) {
> + DRM_WARN("Truncated vkms card driver name '%s-%s' to '%s'\n",
Nit: The DRM_* print macros are deprecated in favor of the drm_* print functions. You probably also want to use a categorized message here instead of just warn, perhaps drm_dbg() which defaults to DRIVER category.
You might consider converting the driver to drm_* before this patchset so we're working from a consistent base.
> + DRIVER_NAME, name, unique_name);
> + }
> +
> + pdev = platform_device_register_simple(unique_name, -1, NULL, 0);
> if (IS_ERR(pdev))
> - return PTR_ERR(pdev);
> + return ERR_PTR(PTR_ERR(pdev));
return pdev;
>
> - if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> - ret = -ENOMEM;
> - goto out_unregister;
> + grp = devres_open_group(&pdev->dev, NULL, GFP_KERNEL);
> + if (!grp) {
> + ret = ENOMEM;
> + goto out_platform_device;
> }
>
> - vkms_device = devm_drm_dev_alloc(&pdev->dev, &vkms_driver,
> - struct vkms_device, drm);
> - if (IS_ERR(vkms_device)) {
> - ret = PTR_ERR(vkms_device);
> - goto out_devres;
> + card = devm_drm_dev_alloc(&pdev->dev, &vkms_driver, struct vkms_card,
> + drm);
> + if (IS_ERR(card)) {
> + ret = PTR_ERR(card);
> + goto out_release_group;
> }
> -
> - vkms_device->platform = pdev;
> - vkms_device->config.cursor = enable_cursor;
> - vkms_device->config.writeback = enable_writeback;
> - vkms_device->config.overlay = enable_overlay;
>
> - ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
> - DMA_BIT_MASK(64));
> + strncpy(card->name_buf, unique_name, ARRAY_SIZE(card->name_buf) - 1);
> + card->platform = pdev;
> + card->drm.unique = card->name_buf;
> + card->vkms_device = &vkms_device;
> + card->is_default = is_default;
> + card->is_registered = false;
> + card->resource_group_id = grp;
>
> + ret = dma_coerce_mask_and_coherent(card->drm.dev, DMA_BIT_MASK(64));
> if (ret) {
> DRM_ERROR("Could not initialize DMA support\n");
> - goto out_devres;
> + goto out_release_group;
> }
>
> - ret = drm_vblank_init(&vkms_device->drm, 1);
> + ret = drm_vblank_init(&card->drm, 1);
> if (ret) {
> DRM_ERROR("Failed to vblank\n");
> - goto out_devres;
> + goto out_release_group;
> }
>
> - ret = vkms_modeset_init(vkms_device);
> - if (ret)
> - goto out_devres;
> -
> - ret = drm_dev_register(&vkms_device->drm, 0);
> - if (ret)
> - goto out_devres;
> + ret = vkms_modeset_init(card);
> + if (ret) {
> + DRM_ERROR("Unable to initialize modesetting");
> + goto out_release_group;
> + }
>
> - drm_fbdev_generic_setup(&vkms_device->drm, 0);
> + ret = drm_dev_register(&card->drm, 0);
> + if (ret) {
> + DRM_ERROR("Unable to register card");
> + return ERR_PTR(ret);
No cleanup required if this fails?
> + }
>
> - vkms_device->initialized = true;
> + drm_fbdev_generic_setup(&card->drm, 0);
> + card->is_registered = true;
Re-raising the race concern here.
>
> - return 0;
> + devres_close_group(&pdev->dev, grp);
> + list_add_tail(&card->node, &vkms_device.cards);
I am still unclear why we need to bookkeep the cards ourselves since we're
spawning a new platform_device for each drm_device. It feels like perhaps it
would be easier to spawn a new platform_driver for each new drm device with its
own .probe & .remove function which keeps track of its own instance. This would
avoid the necessity of vkms_card, each platform_driver would have its own
vkms_device containing a drm_device.
I'm assuming that rmmod would invoke .remove on all the sub platform_drivers,
but if not you could keep track of them in the platform device and unregister
each one on unload which would be a lot less statekeeping.
> +
Nit: Trailing whitespace here
> + return card;
>
> -out_devres:
> - devres_release_group(&pdev->dev, NULL);
> -out_unregister:
> +out_release_group:
> + devres_release_group(&pdev->dev, grp);
> +out_platform_device:
> platform_device_unregister(pdev);
> - return ret;
> + return ERR_PTR(ret);
> }
>
> -static int __init vkms_init(void)
> +void vkms_card_destroy(struct vkms_card *card)
> {
> - return vkms_create();
> + list_del(&card->node);
> + if (card->is_registered) {
Nit: You could save yourself some indentation here and exit early.
> + drm_dev_unregister(&card->drm);
> + drm_atomic_helper_shutdown(&card->drm);
> + devres_release_group(&card->platform->dev,
> + card->resource_group_id);
> + platform_device_unregister(card->platform);
> + }
> }
>
> -static void __exit vkms_exit(void)
> +static int __init vkms_init(void)
> {
> - struct platform_device *pdev;
> -
> - if (!vkms_device || !vkms_device->initialized) {
> - return;
> + struct vkms_card *card;
> +
> + vkms_device.config.cursor = enable_cursor;
> + vkms_device.config.writeback = enable_writeback;
> + vkms_device.config.overlay = enable_overlay;
> + INIT_LIST_HEAD(&vkms_device.cards);
> +
> + card = vkms_card_init("default", /* configfs */ NULL);
This passes NULL for the bool is_default argument. Presumably this should be
true, or perhaps you don't need the second arg at all right now.
> + if (IS_ERR(card)) {
> + DRM_ERROR("Unable to init default card");
> + vkms_device_destroy();
> + return PTR_ERR(card);
> }
>
> - pdev = vkms_device->platform;
> + return 0;
> +}
>
> - drm_dev_unregister(&vkms_device->drm);
> - drm_atomic_helper_shutdown(&vkms_device->drm);
> - devres_release_group(&pdev->dev, NULL);
> - platform_device_unregister(pdev);
> +static void __exit vkms_exit(void)
> +{
> + vkms_device_destroy();
> }
>
> module_init(vkms_init);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index c7ebc4ee6b14..1a98b81d6f22 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -4,6 +4,7 @@
> #define _VKMS_DRV_H_
>
> #include <linux/hrtimer.h>
> +#include <linux/kernel.h>
From kernel.h:
* This header has combined a lot of unrelated to each other stuff.
* The process of splitting its content is in progress while keeping
* backward compatibility. That's why it's highly recommended NOT to
* include this header inside another header file, especially under
* generic or architectural include/ directory.
>
> #include <drm/drm.h>
> #include <drm/drm_gem.h>
> @@ -101,19 +102,29 @@ struct vkms_config {
> bool overlay;
> };
>
> -struct vkms_device {
> - struct drm_device drm;
> +struct vkms_card {
> struct platform_device *platform;
> + struct drm_device drm;
> + struct list_head node;
Nit: node is pretty ambiguous
> struct vkms_output output;
> + struct vkms_device *vkms_device;
> + /* storage for the value of drm->unique, giving this dev a unique busid */
I didn't quite grok this comment, mind expanding?
> + char name_buf[64];
> + void *resource_group_id;
> + bool is_default;
> + bool is_registered;
> +};
> +
> +struct vkms_device {
> + struct list_head cards;
> struct vkms_config config;
> - bool initialized;
> };
>
> #define drm_crtc_to_vkms_output(target) \
> container_of(target, struct vkms_output, crtc)
>
> -#define drm_device_to_vkms_device(target) \
> - container_of(target, struct vkms_device, drm)
> +#define drm_device_to_vkms_card(target) \
> + container_of(target, struct vkms_card, drm)
>
> #define to_vkms_crtc_state(target)\
> container_of(target, struct vkms_crtc_state, base)
> @@ -121,13 +132,18 @@ struct vkms_device {
> #define to_vkms_plane_state(target)\
> container_of(target, struct vkms_plane_state, base.base)
>
> +/* Cards */
> +struct vkms_card *vkms_card_init(const char *name, bool is_default);
> +void vkms_card_destroy(struct vkms_card *card);
> +
> /* CRTC */
> int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> struct drm_plane *primary, struct drm_plane *cursor);
>
> -int vkms_output_init(struct vkms_device *vkmsdev, int index);
> +int vkms_output_init_default(struct vkms_card *card, int index);
> +int vkms_output_init(struct vkms_card *card, int index);
>
> -struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> +struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> enum drm_plane_type type, int index);
>
> /* CRC Support */
> @@ -142,6 +158,6 @@ void vkms_composer_worker(struct work_struct *work);
> void vkms_set_composer(struct vkms_output *out, bool enabled);
>
> /* Writeback */
> -int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> +int vkms_enable_writeback_connector(struct vkms_card *card);
>
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index d0061c82003a..dafd47c0a54d 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -32,12 +32,12 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> .get_modes = vkms_conn_get_modes,
> };
>
> -static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
> +static int vkms_add_overlay_plane(struct vkms_card *card, int index,
> struct drm_crtc *crtc)
> {
> struct vkms_plane *overlay;
>
> - overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);
> + overlay = vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY, index);
> if (IS_ERR(overlay))
> return PTR_ERR(overlay);
>
> @@ -47,10 +47,11 @@ static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
> return 0;
> }
>
> -int vkms_output_init(struct vkms_device *vkmsdev, int index)
> +int vkms_output_init_default(struct vkms_card *card, int index)
> {
> - struct vkms_output *output = &vkmsdev->output;
> - struct drm_device *dev = &vkmsdev->drm;
> + const struct vkms_config *config = &card->vkms_device->config;
> + struct vkms_output *output = &card->output;
> + struct drm_device *dev = &card->drm;
> struct drm_connector *connector = &output->connector;
> struct drm_encoder *encoder = &output->encoder;
> struct drm_crtc *crtc = &output->crtc;
> @@ -59,20 +60,20 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> int writeback;
> unsigned int n;
>
> - primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
> + primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY, index);
> if (IS_ERR(primary))
> return PTR_ERR(primary);
>
> - if (vkmsdev->config.overlay) {
> + if (config->overlay) {
> for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> - ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
> + ret = vkms_add_overlay_plane(card, index, crtc);
> if (ret)
> return ret;
> }
> }
>
> - if (vkmsdev->config.cursor) {
> - cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
> + if (config->cursor) {
> + cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR, index);
> if (IS_ERR(cursor))
> return PTR_ERR(cursor);
> }
> @@ -103,8 +104,8 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> goto err_attach;
> }
>
> - if (vkmsdev->config.writeback) {
> - writeback = vkms_enable_writeback_connector(vkmsdev);
> + if (config->writeback) {
> + writeback = vkms_enable_writeback_connector(card);
> if (writeback)
> DRM_ERROR("Failed to init writeback connector\n");
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index d8eb674b49a6..28abd61a0bb9 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -158,10 +158,10 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> };
>
> -struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> +struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> enum drm_plane_type type, int index)
> {
> - struct drm_device *dev = &vkmsdev->drm;
> + struct drm_device *dev = &card->drm;
> const struct drm_plane_helper_funcs *funcs;
> struct vkms_plane *plane;
> const u32 *formats;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index af1604dfbbaf..681e7267d688 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -94,15 +94,15 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> struct drm_writeback_job *job)
> {
> struct vkms_writeback_job *vkmsjob = job->priv;
> - struct vkms_device *vkmsdev;
> + struct vkms_card *card;
>
> if (!job->fb)
> return;
>
> drm_gem_fb_vunmap(job->fb, vkmsjob->map);
>
> - vkmsdev = drm_device_to_vkms_device(job->fb->dev);
> - vkms_set_composer(&vkmsdev->output, false);
> + card = drm_device_to_vkms_card(job->fb->dev);
> + vkms_set_composer(&card->output, false);
> kfree(vkmsjob);
> }
>
> @@ -111,8 +111,8 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> {
> struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
> conn);
> - struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> - struct vkms_output *output = &vkmsdev->output;
> + struct vkms_card *card = drm_device_to_vkms_card(conn->dev);
> + struct vkms_output *output = &card->output;
> struct drm_writeback_connector *wb_conn = &output->wb_connector;
> struct drm_connector_state *conn_state = wb_conn->base.state;
> struct vkms_crtc_state *crtc_state = output->composer_state;
> @@ -120,7 +120,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> if (!conn_state)
> return;
>
> - vkms_set_composer(&vkmsdev->output, true);
> + vkms_set_composer(&card->output, true);
>
> spin_lock_irq(&output->composer_lock);
> crtc_state->active_writeback = conn_state->writeback_job->priv;
> @@ -136,14 +136,14 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> .atomic_commit = vkms_wb_atomic_commit,
> };
>
> -int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
> +int vkms_enable_writeback_connector(struct vkms_card *card)
> {
> - struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> + struct drm_writeback_connector *wb = &card->output.wb_connector;
>
> - vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> + card->output.wb_connector.encoder.possible_crtcs = 1;
> drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
>
> - return drm_writeback_connector_init(&vkmsdev->drm, wb,
> + return drm_writeback_connector_init(&card->drm, wb,
> &vkms_wb_connector_funcs,
> &vkms_wb_encoder_helper_funcs,
> vkms_wb_formats,
> --
> 2.37.1.359.gd136c6c3e2-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On Fri, Aug 05, 2022 at 06:27:08PM +0000, Sean Paul wrote:
> On Fri, Jul 22, 2022 at 05:32:10PM -0400, Jim Shargo wrote:
> > This is a small refactor to make ConfigFS support easier.
> >
> > We now store the vkms_device statically, and maintain a list of
> > "cards", each representing a different virtual DRM driver.
> >
> > We also make it clear when a card is "default", that is created at
> > initialization, and not. This is because, due to limitations on what we
> > can do with the configfs API, the default card won't be reflected in
> > configfs and is initialized in a unique way.
> >
> > Since we're only managing a single card, this should be a no-op.
> >
> > Signed-off-by: Jim Shargo <[email protected]>
/snip
What a mess, I replied to the wrong patch. The review here is targeting PATCH
2/5 despite the title and reply-to.
On Fri, Jul 22, 2022 at 05:32:11PM -0400, Jim Shargo wrote:
> This change supports multiple CRTCs, encoders, connectors instead of one
> of each per card.
>
> Since ConfigFS-based devices will support multiple crtcs, it's useful to
> move all of the writeback/composition data from being a per-output thing
> to being a per-CRTC thing.
>
> Since there's still only ever one CRTC, this should be a no-op refactor.
>
> Signed-off-by: Jim Shargo <[email protected]>
> ---
Actual PATCH 3/5 review below :-)
> drivers/gpu/drm/vkms/vkms_composer.c | 28 +++----
> drivers/gpu/drm/vkms/vkms_crtc.c | 91 +++++++++++----------
> drivers/gpu/drm/vkms/vkms_drv.c | 3 +-
> drivers/gpu/drm/vkms/vkms_drv.h | 66 ++++++++++------
> drivers/gpu/drm/vkms/vkms_output.c | 110 ++++++++++++++++++--------
> drivers/gpu/drm/vkms/vkms_plane.c | 39 ++++++---
> drivers/gpu/drm/vkms/vkms_writeback.c | 25 +++---
> 7 files changed, 225 insertions(+), 137 deletions(-)
>
/snip
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index c1b632952532..d93678d984ae 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -11,35 +11,34 @@
>
> static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> {
> - struct vkms_output *output = container_of(timer, struct vkms_output,
> - vblank_hrtimer);
> - struct drm_crtc *crtc = &output->crtc;
> + struct vkms_crtc *vkms_crtc = timer_to_vkms_crtc(timer);
> + struct drm_crtc *crtc = &vkms_crtc->base;
> struct vkms_crtc_state *state;
> u64 ret_overrun;
> bool ret, fence_cookie;
>
> fence_cookie = dma_fence_begin_signalling();
>
> - ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> - output->period_ns);
> + ret_overrun = hrtimer_forward_now(&vkms_crtc->vblank_hrtimer,
> + vkms_crtc->period_ns);
> if (ret_overrun != 1)
> pr_warn("%s: vblank timer overrun\n", __func__);
>
> - spin_lock(&output->lock);
> + spin_lock(&vkms_crtc->lock);
> ret = drm_crtc_handle_vblank(crtc);
> if (!ret)
> DRM_ERROR("vkms failure on handling vblank");
>
> - state = output->composer_state;
> - spin_unlock(&output->lock);
> + state = vkms_crtc->composer_state;
> + spin_unlock(&vkms_crtc->lock);
>
> - if (state && output->composer_enabled) {
> + if (state && vkms_crtc->composer_enabled) {
> u64 frame = drm_crtc_accurate_vblank_count(crtc);
>
> /* update frame_start only if a queued vkms_composer_worker()
> * has read the data
> */
> - spin_lock(&output->composer_lock);
> + spin_lock(&vkms_crtc->composer_lock);
> if (!state->crc_pending)
> state->frame_start = frame;
> else
> @@ -47,9 +46,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> state->frame_start, frame);
> state->frame_end = frame;
> state->crc_pending = true;
> - spin_unlock(&output->composer_lock);
> + spin_unlock(&vkms_crtc->composer_lock);
>
> - ret = queue_work(output->composer_workq, &state->composer_work);
> + ret = queue_work(vkms_crtc->composer_workq, &state->composer_work);
> if (!ret)
> DRM_DEBUG_DRIVER("Composer worker already queued\n");
> }
> @@ -62,28 +61,24 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> static int vkms_enable_vblank(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> - struct vkms_card *card = drm_device_to_vkms_card(dev);
> - struct vkms_output *out = &card->output;
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
> unsigned int pipe = drm_crtc_index(crtc);
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>
> drm_calc_timestamping_constants(crtc, &crtc->mode);
>
> - hrtimer_init(&out->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> - out->vblank_hrtimer.function = &vkms_vblank_simulate;
> - out->period_ns = ktime_set(0, vblank->framedur_ns);
> - hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_REL);
> + hrtimer_init(&vkms_crtc->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + vkms_crtc->vblank_hrtimer.function = &vkms_vblank_simulate;
> + vkms_crtc->period_ns = ktime_set(0, vblank->framedur_ns);
> + hrtimer_start(&vkms_crtc->vblank_hrtimer, vkms_crtc->period_ns, HRTIMER_MODE_REL);
>
> return 0;
> }
>
> static void vkms_disable_vblank(struct drm_crtc *crtc)
> {
> - struct drm_device *dev = crtc->dev;
> - struct vkms_card *card = drm_device_to_vkms_card(dev);
> - struct vkms_output *out = &card->output;
> -
> - hrtimer_cancel(&out->vblank_hrtimer);
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
Nit: blank line here
> + hrtimer_cancel(&vkms_crtc->vblank_hrtimer);
> }
>
> static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> @@ -91,8 +86,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> bool in_vblank_irq)
> {
> struct drm_device *dev = crtc->dev;
> - struct vkms_card *card = drm_device_to_vkms_card(dev);
> - struct vkms_output *output = &card->output;
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
> unsigned int pipe = crtc->index;
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>
> @@ -101,7 +95,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> return true;
> }
>
> - *vblank_time = READ_ONCE(output->vblank_hrtimer.node.expires);
> + *vblank_time = READ_ONCE(vkms_crtc->vblank_hrtimer.node.expires);
>
> if (WARN_ON(*vblank_time == vblank->time))
> return true;
> @@ -113,7 +107,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> * the vblank core expects. Therefore we need to always correct the
> * timestampe by one frame.
> */
> - *vblank_time -= output->period_ns;
> + *vblank_time -= vkms_crtc->period_ns;
>
> return true;
> }
> @@ -240,18 +234,18 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
> static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> - struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
>
> /* This lock is held across the atomic commit to block vblank timer
> * from scheduling vkms_composer_worker until the composer is updated
> */
> - spin_lock_irq(&vkms_output->lock);
> + spin_lock_irq(&vkms_crtc->lock);
> }
>
> static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> - struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
>
> if (crtc->state->event) {
> spin_lock(&crtc->dev->event_lock);
> @@ -266,9 +260,9 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
> crtc->state->event = NULL;
> }
>
> - vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
> + vkms_crtc->composer_state = to_vkms_crtc_state(crtc->state);
>
> - spin_unlock_irq(&vkms_output->lock);
> + spin_unlock_irq(&vkms_crtc->lock);
> }
>
> static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> @@ -279,27 +273,38 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> .atomic_disable = vkms_crtc_atomic_disable,
> };
>
> -int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> +struct vkms_crtc *vkms_crtc_init(struct vkms_card *card,
> struct drm_plane *primary, struct drm_plane *cursor)
> {
> - struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> + struct drm_device *dev = &card->drm;
> + struct vkms_crtc *vkms_crtc;
> int ret;
>
> - ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
> + if (card->output.num_crtcs >= VKMS_MAX_OUTPUT_OBJECTS) {
> + return ERR_PTR(ENOMEM);
> + }
Nit: Remove braces
> + vkms_crtc = &card->output.crtcs[card->output.num_crtcs++];
Instead of incrementing here and decrementing in the error handling code, just
increment right before the return when we're in the clear.
While I'm here, do you need any locking around the output members?
> +
> + ret = drm_crtc_init_with_planes(dev, &vkms_crtc->base, primary, cursor,
> &vkms_crtc_funcs, NULL);
> if (ret) {
> DRM_ERROR("Failed to init CRTC\n");
> - return ret;
> + goto out_error;
> }
>
> - drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
> + drm_crtc_helper_add(&vkms_crtc->base, &vkms_crtc_helper_funcs);
>
> - spin_lock_init(&vkms_out->lock);
> - spin_lock_init(&vkms_out->composer_lock);
> + spin_lock_init(&vkms_crtc->lock);
> + spin_lock_init(&vkms_crtc->composer_lock);
>
> - vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
> - if (!vkms_out->composer_workq)
> - return -ENOMEM;
> + vkms_crtc->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
> + if (!vkms_crtc->composer_workq)
> + goto out_error;
> +
> + return vkms_crtc;
>
> - return ret;
> +out_error:
> + memset(vkms_crtc, 0, sizeof(*vkms_crtc));
> + card->output.num_crtcs -= 1;
> + return ERR_PTR(ret);
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 92fbade2199b..266c9f643f70 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -57,7 +57,8 @@ static void vkms_release(struct drm_device *dev)
> {
> struct vkms_card *card = drm_device_to_vkms_card(dev);
>
> - destroy_workqueue(card->output.composer_workq);
> + for (int i = 0; i < card->output.num_crtcs; i++)
> + destroy_workqueue(card->output.crtcs[i].composer_workq);
> }
>
> static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 1a98b81d6f22..30ae06d7af71 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -23,6 +23,8 @@
>
> #define NUM_OVERLAY_PLANES 8
>
> +#define VKMS_MAX_OUTPUT_OBJECTS 16
> +
> struct vkms_writeback_job {
> struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> @@ -51,6 +53,27 @@ struct vkms_plane {
> struct drm_plane base;
> };
>
> +struct vkms_output;
> +
> +struct vkms_crtc {
> + struct drm_crtc base;
> +
> + struct drm_writeback_connector wb_connector;
> + struct hrtimer vblank_hrtimer;
> + ktime_t period_ns;
> + struct drm_pending_vblank_event *event;
> + /* ordered wq for composer_work */
> + struct workqueue_struct *composer_workq;
> + /* protects concurrent access to composer */
> + spinlock_t lock;
> +
> + /* protected by @lock */
> + bool composer_enabled;
> + struct vkms_crtc_state *composer_state;
> +
> + spinlock_t composer_lock;
> +};
> +
> /**
> * vkms_crtc_state - Driver specific CRTC state
> * @base: base CRTC state
> @@ -75,23 +98,14 @@ struct vkms_crtc_state {
> };
>
> struct vkms_output {
> - struct drm_crtc crtc;
> - struct drm_encoder encoder;
> - struct drm_connector connector;
> - struct drm_writeback_connector wb_connector;
> - struct hrtimer vblank_hrtimer;
> - ktime_t period_ns;
> - struct drm_pending_vblank_event *event;
> - /* ordered wq for composer_work */
> - struct workqueue_struct *composer_workq;
> - /* protects concurrent access to composer */
> - spinlock_t lock;
> -
> - /* protected by @lock */
> - bool composer_enabled;
> - struct vkms_crtc_state *composer_state;
> -
> - spinlock_t composer_lock;
> + int num_crtcs;
> + struct vkms_crtc crtcs[VKMS_MAX_OUTPUT_OBJECTS];
> + int num_encoders;
> + struct drm_encoder encoders[VKMS_MAX_OUTPUT_OBJECTS];
> + int num_connectors;
> + struct drm_connector connectors[VKMS_MAX_OUTPUT_OBJECTS];
Nit: Trailing whitespace
> + int num_planes;
> + struct vkms_plane planes[VKMS_MAX_OUTPUT_OBJECTS];
We probably want some number more planes than CRTCs, so perhaps make
VKMS_MAX_PLANES a multiple of VKMS_MAX_OUTPUT_OBJECTS?
> };
>
> struct vkms_device;
> @@ -120,12 +134,15 @@ struct vkms_device {
> struct vkms_config config;
> };
>
> -#define drm_crtc_to_vkms_output(target) \
> - container_of(target, struct vkms_output, crtc)
> +#define drm_crtc_to_vkms_crtc(crtc)\
> + container_of(crtc, struct vkms_crtc, base)
>
> -#define drm_device_to_vkms_card(target) \
> +#define drm_device_to_vkms_card(target) \
> container_of(target, struct vkms_card, drm)
>
> +#define timer_to_vkms_crtc(timer)\
> + container_of(timer, struct vkms_crtc, vblank_hrtimer)
> +
> #define to_vkms_crtc_state(target)\
> container_of(target, struct vkms_crtc_state, base)
>
> @@ -137,14 +154,13 @@ struct vkms_card *vkms_card_init(const char *name, bool is_default);
> void vkms_card_destroy(struct vkms_card *card);
>
> /* CRTC */
> -int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> - struct drm_plane *primary, struct drm_plane *cursor);
> +struct vkms_crtc *vkms_crtc_init(struct vkms_card *card, struct drm_plane *primary, struct drm_plane *cursor);
Nit: Please split this line
>
> int vkms_output_init_default(struct vkms_card *card, int index);
> int vkms_output_init(struct vkms_card *card, int index);
>
> struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> - enum drm_plane_type type, int index);
> + enum drm_plane_type type);
>
> /* CRC Support */
> const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
> @@ -155,9 +171,9 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>
> /* Composer Support */
> void vkms_composer_worker(struct work_struct *work);
> -void vkms_set_composer(struct vkms_output *out, bool enabled);
> +void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled);
>
> /* Writeback */
> -int vkms_enable_writeback_connector(struct vkms_card *card);
> +int vkms_enable_writeback_connector(struct vkms_card *card, struct vkms_crtc *vkms_crtc);
>
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index dafd47c0a54d..2b72d8e763a8 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -32,19 +32,44 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> .get_modes = vkms_conn_get_modes,
> };
>
> -static int vkms_add_overlay_plane(struct vkms_card *card, int index,
> - struct drm_crtc *crtc)
> +static struct drm_connector *vkms_connector_init(struct vkms_card *card)
> {
> - struct vkms_plane *overlay;
> + struct drm_connector *connector;
> + int ret;
>
> - overlay = vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY, index);
> - if (IS_ERR(overlay))
> - return PTR_ERR(overlay);
> + if (card->output.num_connectors >= VKMS_MAX_OUTPUT_OBJECTS)
> + return ERR_PTR(ENOMEM);
>
> - if (!overlay->base.possible_crtcs)
> - overlay->base.possible_crtcs = drm_crtc_mask(crtc);
> + connector = &card->output.connectors[card->output.num_connectors++];
> + ret = drm_connector_init(&card->drm, connector, &vkms_connector_funcs,
> + DRM_MODE_CONNECTOR_VIRTUAL);
> + if (ret) {
> + memset(connector, 0, sizeof(*connector));
> + card->output.num_connectors -= 1;
> + return ERR_PTR(ret);
> + }
>
> - return 0;
> + drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
> +
> + return connector;
> +}
> +
> +static struct drm_encoder *vkms_encoder_init(struct vkms_card *card)
> +{
> + struct drm_encoder *encoder;
> + int ret;
> +
> + if (card->output.num_encoders >= VKMS_MAX_OUTPUT_OBJECTS)
> + return ERR_PTR(ENOMEM);
> +
> + encoder = &card->output.encoders[card->output.num_encoders++];
> + ret = drm_simple_encoder_init(&card->drm, encoder, DRM_MODE_ENCODER_VIRTUAL);
> + if (ret) {
> + memset(encoder, 0, sizeof(*encoder));
> + card->output.num_encoders -= 1;
> + return ERR_PTR(ret);
> + }
> + return encoder;
> }
>
> int vkms_output_init_default(struct vkms_card *card, int index)
> @@ -52,51 +77,65 @@ int vkms_output_init_default(struct vkms_card *card, int index)
> const struct vkms_config *config = &card->vkms_device->config;
> struct vkms_output *output = &card->output;
> struct drm_device *dev = &card->drm;
> - struct drm_connector *connector = &output->connector;
> - struct drm_encoder *encoder = &output->encoder;
> - struct drm_crtc *crtc = &output->crtc;
> + struct drm_connector *connector;
> + struct drm_encoder *encoder;
> + struct vkms_crtc *vkms_crtc;
> struct vkms_plane *primary, *cursor = NULL;
> int ret;
> int writeback;
> unsigned int n;
>
> - primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY, index);
> + primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY);
> if (IS_ERR(primary))
> return PTR_ERR(primary);
>
> if (config->overlay) {
> for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> - ret = vkms_add_overlay_plane(card, index, crtc);
> - if (ret)
> - return ret;
> + struct vkms_plane *overlay =
> + vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY);
> + if (IS_ERR(overlay)) {
> + ret = PTR_ERR(overlay);
> + goto err_planes;
> + }
> }
> }
>
> if (config->cursor) {
> - cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR, index);
> - if (IS_ERR(cursor))
> - return PTR_ERR(cursor);
> + cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR);
> + if (IS_ERR(cursor)) {
> + ret = PTR_ERR(cursor);
> + goto err_planes;
> + }
> }
>
> - ret = vkms_crtc_init(dev, crtc, &primary->base, &cursor->base);
> - if (ret)
> - return ret;
> + vkms_crtc = vkms_crtc_init(card, &primary->base,
> + cursor ? &cursor->base : NULL);
Looks like we had a NULL deref previously if there was no cursor.. Whoops!
> + if (IS_ERR(vkms_crtc)) {
> + DRM_ERROR("Failed to init crtc\n");
> + ret = PTR_ERR(vkms_crtc);
> + goto err_planes;
> + }
>
> - ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> - DRM_MODE_CONNECTOR_VIRTUAL);
> - if (ret) {
> + for (int i = 0; i < card->output.num_planes; i++) {
> + card->output.planes[i].base.possible_crtcs |=
> + drm_crtc_mask(&vkms_crtc->base);
> + }
> +
> + connector = vkms_connector_init(card);
> + if (IS_ERR(connector)) {
> DRM_ERROR("Failed to init connector\n");
> + ret = PTR_ERR(connector);
> goto err_connector;
> }
>
> - drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
> -
> - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_VIRTUAL);
> - if (ret) {
> + encoder = vkms_encoder_init(card);
> + if (IS_ERR(encoder)) {
> DRM_ERROR("Failed to init encoder\n");
> + ret = PTR_ERR(encoder);
> goto err_encoder;
> }
> - encoder->possible_crtcs = 1;
> + encoder->possible_crtcs |= drm_crtc_mask(&vkms_crtc->base);
> +
>
> ret = drm_connector_attach_encoder(connector, encoder);
> if (ret) {
> @@ -105,7 +144,7 @@ int vkms_output_init_default(struct vkms_card *card, int index)
> }
>
> if (config->writeback) {
> - writeback = vkms_enable_writeback_connector(card);
> + writeback = vkms_enable_writeback_connector(card, vkms_crtc);
> if (writeback)
> DRM_ERROR("Failed to init writeback connector\n");
> }
> @@ -121,7 +160,14 @@ int vkms_output_init_default(struct vkms_card *card, int index)
> drm_connector_cleanup(connector);
>
> err_connector:
> - drm_crtc_cleanup(crtc);
> + drm_crtc_cleanup(&vkms_crtc->base);
> +
> +err_planes:
> + for (int i = 0; i < output->num_planes; i++) {
> + drm_plane_cleanup(&output->planes[i].base);
> + }
Nit: Remove braces
> +
> + memset(output, 0, sizeof(struct vkms_output));
Is this strictly necessary? I tend to worry about blanket clears like this in
error handling since they could cause future issues if people extend
functionality and forget to update the memset.
IMO it would be better to define vkms_output_destroy() which just does all of
the above, clears out the num_* members explicitly and doesn't worry about the
array contents.
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 28abd61a0bb9..5f49257fe16e 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -7,6 +7,7 @@
> #include <drm/drm_fourcc.h>
> #include <drm/drm_gem_atomic_helper.h>
> #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_plane.h>
> #include <drm/drm_plane_helper.h>
>
> #include "vkms_drv.h"
> @@ -65,6 +66,19 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
> kfree(vkms_state);
> }
>
> +static void vkms_plane_destroy(struct drm_plane *plane)
I think this could be replaced by drm_plane_helper_destroy() instead of rolling
your own destroy function.
> +{
> + struct vkms_plane *vkms_plane =
> + container_of(plane, struct vkms_plane, base);
> +
> + if (plane->state) {
> + vkms_plane_destroy_state(plane, plane->state);
> + plane->state = NULL;
> + }
> +
> + memset(vkms_plane, 0, sizeof(struct vkms_plane));
> +}
> +
> static void vkms_plane_reset(struct drm_plane *plane)
> {
> struct vkms_plane_state *vkms_state;
> @@ -86,8 +100,9 @@ static void vkms_plane_reset(struct drm_plane *plane)
> static const struct drm_plane_funcs vkms_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> .disable_plane = drm_atomic_helper_disable_plane,
> - .reset = vkms_plane_reset,
> - .atomic_duplicate_state = vkms_plane_duplicate_state,
> + .destroy = vkms_plane_destroy,
> + .reset = vkms_plane_reset,
Nit: Remove space before tab after .destroy and .reset. I think they're also not
properly aligned.
> + .atomic_duplicate_state = vkms_plane_duplicate_state,
> .atomic_destroy_state = vkms_plane_destroy_state,
> };
>
> @@ -159,13 +174,20 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> };
>
> struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> - enum drm_plane_type type, int index)
> + enum drm_plane_type type)
> {
> struct drm_device *dev = &card->drm;
> + struct vkms_output *output = &card->output;
> const struct drm_plane_helper_funcs *funcs;
> struct vkms_plane *plane;
> const u32 *formats;
> int nformats;
> + int ret;
> +
> + if (output->num_planes >= VKMS_MAX_OUTPUT_OBJECTS)
> + return ERR_PTR(ENOMEM);
> +
> + plane = &output->planes[output->num_planes++];
Hmm, I'm guessing this is why the destroy() hook is being hand rolled? Perhaps
changing the output arrays to arrays of pointers would allow you to leverage the
helpers and drmm_*
>
> switch (type) {
> case DRM_PLANE_TYPE_PRIMARY:
> @@ -186,12 +208,11 @@ struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> break;
> }
>
> - plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 << index,
> - &vkms_plane_funcs,
> - formats, nformats,
> - NULL, type, NULL);
> - if (IS_ERR(plane))
> - return plane;
> + ret = drm_universal_plane_init(dev, &plane->base, 0,
> + &vkms_plane_funcs, formats, nformats,
> + NULL, type, NULL);
> + if (ret)
> + return ERR_PTR(ret);
>
> drm_plane_helper_add(&plane->base, funcs);
>
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 681e7267d688..68c7a77ab93f 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0+
>
> #include <linux/iosys-map.h>
> +#include <linux/kernel.h>
Same kernel.h nit as before.
>
> #include <drm/drm_atomic.h>
> #include <drm/drm_fourcc.h>
> @@ -94,15 +95,14 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> struct drm_writeback_job *job)
> {
> struct vkms_writeback_job *vkmsjob = job->priv;
> - struct vkms_card *card;
> + struct vkms_crtc *vkms_crtc = container_of(connector, struct vkms_crtc, wb_connector);
>
> if (!job->fb)
> return;
>
> drm_gem_fb_vunmap(job->fb, vkmsjob->map);
>
> - card = drm_device_to_vkms_card(job->fb->dev);
> - vkms_set_composer(&card->output, false);
> + vkms_set_composer(vkms_crtc, false);
> kfree(vkmsjob);
> }
>
> @@ -111,21 +111,20 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> {
> struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
> conn);
> - struct vkms_card *card = drm_device_to_vkms_card(conn->dev);
> - struct vkms_output *output = &card->output;
> - struct drm_writeback_connector *wb_conn = &output->wb_connector;
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(connector_state->crtc);
> + struct drm_writeback_connector *wb_conn = &vkms_crtc->wb_connector;
> struct drm_connector_state *conn_state = wb_conn->base.state;
> - struct vkms_crtc_state *crtc_state = output->composer_state;
> + struct vkms_crtc_state *crtc_state = vkms_crtc->composer_state;
>
> if (!conn_state)
> return;
>
> - vkms_set_composer(&card->output, true);
> + vkms_set_composer(vkms_crtc, true);
>
> - spin_lock_irq(&output->composer_lock);
> + spin_lock_irq(&vkms_crtc->composer_lock);
> crtc_state->active_writeback = conn_state->writeback_job->priv;
> crtc_state->wb_pending = true;
> - spin_unlock_irq(&output->composer_lock);
> + spin_unlock_irq(&vkms_crtc->composer_lock);
> drm_writeback_queue_job(wb_conn, connector_state);
> }
>
> @@ -136,11 +135,11 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> .atomic_commit = vkms_wb_atomic_commit,
> };
>
> -int vkms_enable_writeback_connector(struct vkms_card *card)
> +int vkms_enable_writeback_connector(struct vkms_card *card, struct vkms_crtc *vkms_crtc)
> {
> - struct drm_writeback_connector *wb = &card->output.wb_connector;
> + struct drm_writeback_connector *wb = &vkms_crtc->wb_connector;
>
> - card->output.wb_connector.encoder.possible_crtcs = 1;
> + vkms_crtc->wb_connector.encoder.possible_crtcs = 1;
> drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
>
> return drm_writeback_connector_init(&card->drm, wb,
> --
> 2.37.1.359.gd136c6c3e2-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS