Changing drm_connector and drm_encoder feilds to pointers in drm_writeback_connector as the elements of struct drm_writeback_connector are: struct drm_writeback_connector { struct drm_connector base; struct drm_encoder encoder; Similarly the elements of intel_encoder and intel_connector are: struct intel_encoder { struct drm_encoder base;
struct intel_connector { struct drm_connector base;
The function drm_writeback_connector_init() will initialize the drm_connector and drm_encoder and attach them as well. Since the drm_connector/encoder are both struct in drm_writeback_connector and intel_connector/encoder, we need one of them to be a pointer so we can reference them or else we will be pointing to 2 seprate instances.
Usually the struct defined in drm framework pointing to any struct will be pointer and allocating them and initialization will be done with the users. Like struct drm_connector and drm_encoder are part of drm framework and the users of these such as i915 have included them in their struct intel_connector and intel_encoder. Likewise struct drm_writeback_connector is a special connector and hence is not a user of drm_connector and hence this should be pointers.
Adding drm_writeback_connector to drm_connector so that writeback_connector can be fetched from drm_connector as the previous container_of method won't work due to change in the feilds of drm_connector and drm_encoder in drm_writeback_connector.
Note:The corresponding ripple effect due to the above changes namely in two drivers as I can see it komeda and vkms have been dealt with in the upcoming patches of this series.
Signed-off-by: Kandpal, Suraj suraj.kandpal@intel.com --- drivers/gpu/drm/drm_writeback.c | 19 ++++++++++--------- include/drm/drm_connector.h | 3 +++ include/drm/drm_writeback.h | 6 +++--- 3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504f1bb..47238db42363 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -87,7 +87,7 @@ static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence) struct drm_writeback_connector *wb_connector = fence_to_wb_connector(fence);
- return wb_connector->base.dev->driver->name; + return wb_connector->base->dev->driver->name; }
static const char * @@ -177,7 +177,7 @@ int drm_writeback_connector_init(struct drm_device *dev, const u32 *formats, int n_formats) { struct drm_property_blob *blob; - struct drm_connector *connector = &wb_connector->base; + struct drm_connector *connector = wb_connector->base; struct drm_mode_config *config = &dev->mode_config; int ret = create_writeback_properties(dev);
@@ -189,14 +189,15 @@ int drm_writeback_connector_init(struct drm_device *dev, if (IS_ERR(blob)) return PTR_ERR(blob);
- drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); - ret = drm_encoder_init(dev, &wb_connector->encoder, + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); + ret = drm_encoder_init(dev, wb_connector->encoder, &drm_writeback_encoder_funcs, DRM_MODE_ENCODER_VIRTUAL, NULL); if (ret) goto fail;
connector->interlace_allowed = 0; + connector->wb_connector = wb_connector;
ret = drm_connector_init(dev, connector, con_funcs, DRM_MODE_CONNECTOR_WRITEBACK); @@ -204,7 +205,7 @@ int drm_writeback_connector_init(struct drm_device *dev, goto connector_fail;
ret = drm_connector_attach_encoder(connector, - &wb_connector->encoder); + wb_connector->encoder); if (ret) goto attach_fail;
@@ -233,7 +234,7 @@ int drm_writeback_connector_init(struct drm_device *dev, attach_fail: drm_connector_cleanup(connector); connector_fail: - drm_encoder_cleanup(&wb_connector->encoder); + drm_encoder_cleanup(wb_connector->encoder); fail: drm_property_blob_put(blob); return ret; @@ -263,7 +264,7 @@ int drm_writeback_prepare_job(struct drm_writeback_job *job) { struct drm_writeback_connector *connector = job->connector; const struct drm_connector_helper_funcs *funcs = - connector->base.helper_private; + connector->base->helper_private; int ret;
if (funcs->prepare_writeback_job) { @@ -315,7 +316,7 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) { struct drm_writeback_connector *connector = job->connector; const struct drm_connector_helper_funcs *funcs = - connector->base.helper_private; + connector->base->helper_private;
if (job->prepared && funcs->cleanup_writeback_job) funcs->cleanup_writeback_job(connector, job); @@ -401,7 +402,7 @@ drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector) { struct dma_fence *fence;
- if (WARN_ON(wb_connector->base.connector_type != + if (WARN_ON(wb_connector->base->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)) return NULL;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index b501d0badaea..ec4657cfd7b9 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -44,6 +44,7 @@ struct drm_printer; struct drm_privacy_screen; struct edid; struct i2c_adapter; +struct drm_writeback_connector;
enum drm_connector_force { DRM_FORCE_UNSPECIFIED, @@ -1533,6 +1534,8 @@ struct drm_connector { */ struct drm_encoder *encoder;
+ struct drm_writeback_connector *wb_connector; + #define MAX_ELD_BYTES 128 /** @eld: EDID-like data, if present */ uint8_t eld[MAX_ELD_BYTES]; diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d2714d2a..078c9907219c 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -22,7 +22,7 @@ struct drm_writeback_connector { /** * @base: base drm_connector object */ - struct drm_connector base; + struct drm_connector *base;
/** * @encoder: Internal encoder used by the connector to fulfill @@ -31,7 +31,7 @@ struct drm_writeback_connector { * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function. */ - struct drm_encoder encoder; + struct drm_encoder *encoder;
/** * @pixel_formats_blob_ptr: @@ -143,7 +143,7 @@ struct drm_writeback_job { static inline struct drm_writeback_connector * drm_connector_to_writeback(struct drm_connector *connector) { - return container_of(connector, struct drm_writeback_connector, base); + return connector->wb_connector; }
int drm_writeback_connector_init(struct drm_device *dev,
Making changes to komeda driver because we had to change drm_writeback_connector.base into a pointer the reason for which is expained in the Patch (drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal, Suraj suraj.kandpal@intel.com --- drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 2 +- drivers/gpu/drm/arm/display/komeda/komeda_kms.h | 3 ++- drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 +++++---- 3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c index 59172acb9738..eb37f41c1790 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c @@ -265,7 +265,7 @@ komeda_crtc_do_flush(struct drm_crtc *crtc, if (slave && has_bit(slave->id, kcrtc_st->affected_pipes)) komeda_pipeline_update(slave, old->state);
- conn_st = wb_conn ? wb_conn->base.base.state : NULL; + conn_st = wb_conn ? wb_conn->base.base->state : NULL; if (conn_st && conn_st->writeback_job) drm_writeback_queue_job(&wb_conn->base, conn_st);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h index 456f3c435719..8d83883a1d99 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h @@ -53,6 +53,7 @@ struct komeda_plane_state { * struct komeda_wb_connector */ struct komeda_wb_connector { + struct drm_connector conn; /** @base: &drm_writeback_connector */ struct drm_writeback_connector base;
@@ -136,7 +137,7 @@ struct komeda_kms_dev { static inline bool is_writeback_only(struct drm_crtc_state *st) { struct komeda_wb_connector *wb_conn = to_kcrtc(st->crtc)->wb_conn; - struct drm_connector *conn = wb_conn ? &wb_conn->base.base : NULL; + struct drm_connector *conn = wb_conn ? wb_conn->base.base : NULL;
return conn && (st->connector_mask == BIT(drm_connector_index(conn))); } diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c index e465cc4879c9..0caaf483276d 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c @@ -51,7 +51,7 @@ komeda_wb_encoder_atomic_check(struct drm_encoder *encoder, return -EINVAL; }
- wb_layer = to_kconn(to_wb_conn(conn_st->connector))->wb_layer; + wb_layer = to_kconn(drm_connector_to_writeback(conn_st->connector))->wb_layer;
/* * No need for a full modested when the only connector changed is the @@ -123,7 +123,7 @@ komeda_wb_connector_fill_modes(struct drm_connector *connector, static void komeda_wb_connector_destroy(struct drm_connector *connector) { drm_connector_cleanup(connector); - kfree(to_kconn(to_wb_conn(connector))); + kfree(to_kconn(drm_connector_to_writeback(connector))); }
static const struct drm_connector_funcs komeda_wb_connector_funcs = { @@ -155,6 +155,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms, kwb_conn->wb_layer = kcrtc->master->wb_layer;
wb_conn = &kwb_conn->base; + wb_conn->base = &kwb_conn->conn; wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl, @@ -171,9 +172,9 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms, return err; }
- drm_connector_helper_add(&wb_conn->base, &komeda_wb_conn_helper_funcs); + drm_connector_helper_add(wb_conn->base, &komeda_wb_conn_helper_funcs);
- info = &kwb_conn->base.base.display_info; + info = &kwb_conn->base.base->display_info; info->bpc = __fls(kcrtc->master->improc->supported_color_depths); info->color_formats = kcrtc->master->improc->supported_color_formats;
This makes sense given the other patches in your series, but it seems as yet no one has anything to say about this. I don't have anything specific to comment on other than it seems to make the correct changes to komeda given the rest.
Reviewed-by: Carsten Haitzler carsten.haitzler@arm.com
On 1/11/22 10:18, Kandpal, Suraj wrote:
Making changes to komeda driver because we had to change drm_writeback_connector.base into a pointer the reason for which is expained in the Patch (drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal, Suraj suraj.kandpal@intel.com
drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 2 +- drivers/gpu/drm/arm/display/komeda/komeda_kms.h | 3 ++- drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 +++++---- 3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c index 59172acb9738..eb37f41c1790 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c @@ -265,7 +265,7 @@ komeda_crtc_do_flush(struct drm_crtc *crtc, if (slave && has_bit(slave->id, kcrtc_st->affected_pipes)) komeda_pipeline_update(slave, old->state);
- conn_st = wb_conn ? wb_conn->base.base.state : NULL;
- conn_st = wb_conn ? wb_conn->base.base->state : NULL; if (conn_st && conn_st->writeback_job) drm_writeback_queue_job(&wb_conn->base, conn_st);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h index 456f3c435719..8d83883a1d99 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h @@ -53,6 +53,7 @@ struct komeda_plane_state {
- struct komeda_wb_connector
*/ struct komeda_wb_connector {
- struct drm_connector conn; /** @base: &drm_writeback_connector */ struct drm_writeback_connector base;
@@ -136,7 +137,7 @@ struct komeda_kms_dev { static inline bool is_writeback_only(struct drm_crtc_state *st) { struct komeda_wb_connector *wb_conn = to_kcrtc(st->crtc)->wb_conn;
- struct drm_connector *conn = wb_conn ? &wb_conn->base.base : NULL;
struct drm_connector *conn = wb_conn ? wb_conn->base.base : NULL;
return conn && (st->connector_mask == BIT(drm_connector_index(conn))); }
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c index e465cc4879c9..0caaf483276d 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c @@ -51,7 +51,7 @@ komeda_wb_encoder_atomic_check(struct drm_encoder *encoder, return -EINVAL; }
- wb_layer = to_kconn(to_wb_conn(conn_st->connector))->wb_layer;
wb_layer = to_kconn(drm_connector_to_writeback(conn_st->connector))->wb_layer;
/*
- No need for a full modested when the only connector changed is the
@@ -123,7 +123,7 @@ komeda_wb_connector_fill_modes(struct drm_connector *connector, static void komeda_wb_connector_destroy(struct drm_connector *connector) { drm_connector_cleanup(connector);
- kfree(to_kconn(to_wb_conn(connector)));
kfree(to_kconn(drm_connector_to_writeback(connector))); }
static const struct drm_connector_funcs komeda_wb_connector_funcs = {
@@ -155,6 +155,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms, kwb_conn->wb_layer = kcrtc->master->wb_layer;
wb_conn = &kwb_conn->base;
wb_conn->base = &kwb_conn->conn; wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
@@ -171,9 +172,9 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms, return err; }
- drm_connector_helper_add(&wb_conn->base, &komeda_wb_conn_helper_funcs);
- drm_connector_helper_add(wb_conn->base, &komeda_wb_conn_helper_funcs);
- info = &kwb_conn->base.base.display_info;
- info = &kwb_conn->base.base->display_info; info->bpc = __fls(kcrtc->master->improc->supported_color_depths); info->color_formats = kcrtc->master->improc->supported_color_formats;
Changing vkms driver to accomadate the change of drm_writeback_connector.base to a pointer the reason for which is explained in the Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal, Suraj suraj.kandpal@intel.com --- drivers/gpu/drm/vkms/vkms_writeback.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index 8694227f555f..6de0c4213425 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -114,7 +114,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn, struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev); struct vkms_output *output = &vkmsdev->output; struct drm_writeback_connector *wb_conn = &output->wb_connector; - struct drm_connector_state *conn_state = wb_conn->base.state; + struct drm_connector_state *conn_state = wb_conn->base->state; struct vkms_crtc_state *crtc_state = output->composer_state;
if (!conn_state) @@ -140,8 +140,8 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev) { struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
- vkmsdev->output.wb_connector.encoder.possible_crtcs = 1; - drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs); + vkmsdev->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, &vkms_wb_connector_funcs,
Hi All, Gentle Reminder! Any Review comments?
Changing vkms driver to accomadate the change of drm_writeback_connector.base to a pointer the reason for which is explained in the Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal, Suraj suraj.kandpal@intel.com
drivers/gpu/drm/vkms/vkms_writeback.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index 8694227f555f..6de0c4213425 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -114,7 +114,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn, struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn-
dev);
struct vkms_output *output = &vkmsdev->output; struct drm_writeback_connector *wb_conn = &output-
wb_connector;
- struct drm_connector_state *conn_state = wb_conn->base.state;
struct drm_connector_state *conn_state = wb_conn->base->state; struct vkms_crtc_state *crtc_state = output->composer_state;
if (!conn_state)
@@ -140,8 +140,8 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev) { struct drm_writeback_connector *wb = &vkmsdev-
output.wb_connector;
- vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
- drm_connector_helper_add(&wb->base,
&vkms_wb_conn_helper_funcs);
- vkmsdev->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, &vkms_wb_connector_funcs,
Regards, Suraj Kandpal
+ laurent on this
Hi Suraj
On 1/11/2022 2:17 AM, Kandpal, Suraj wrote:
Changing drm_connector and drm_encoder feilds to pointers in drm_writeback_connector as the elements of struct drm_writeback_connector are: struct drm_writeback_connector { struct drm_connector base; struct drm_encoder encoder; Similarly the elements of intel_encoder and intel_connector are: struct intel_encoder { struct drm_encoder base;
struct intel_connector { struct drm_connector base;
The function drm_writeback_connector_init() will initialize the drm_connector and drm_encoder and attach them as well. Since the drm_connector/encoder are both struct in drm_writeback_connector and intel_connector/encoder, we need one of them to be a pointer so we can reference them or else we will be pointing to 2 seprate instances.
Usually the struct defined in drm framework pointing to any struct will be pointer and allocating them and initialization will be done with the users. Like struct drm_connector and drm_encoder are part of drm framework and the users of these such as i915 have included them in their struct intel_connector and intel_encoder. Likewise struct drm_writeback_connector is a special connector and hence is not a user of drm_connector and hence this should be pointers.
Adding drm_writeback_connector to drm_connector so that writeback_connector can be fetched from drm_connector as the previous container_of method won't work due to change in the feilds of drm_connector and drm_encoder in drm_writeback_connector.
Note:The corresponding ripple effect due to the above changes namely in two drivers as I can see it komeda and vkms have been dealt with in the upcoming patches of this series.
Signed-off-by: Kandpal, Suraj suraj.kandpal@intel.com
Jani pointed me to this thread as i had posted something similar here : https://patchwork.freedesktop.org/patch/470296/ but since this thread was posted earlier, we can discuss further here.
Overall, its similar to what I had posted in the RFC and your commit text also covers my concerns too.
One question I have about your change is since you have changed wb_connector::encoder to be a pointer, i saw the other changes in the series but they do not allocate an encoder. Would this not affect the other drivers which are assuming that the encoder in wb_connector is struct drm_encoder encoder and not struct drm_encoder* encoder.
Your changes fix the compilation issue but wouldnt this crash as encoder wasnt allocated for other drivers.
drivers/gpu/drm/drm_writeback.c | 19 ++++++++++--------- include/drm/drm_connector.h | 3 +++ include/drm/drm_writeback.h | 6 +++--- 3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504f1bb..47238db42363 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -87,7 +87,7 @@ static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence) struct drm_writeback_connector *wb_connector = fence_to_wb_connector(fence);
- return wb_connector->base.dev->driver->name;
return wb_connector->base->dev->driver->name; }
static const char *
@@ -177,7 +177,7 @@ int drm_writeback_connector_init(struct drm_device *dev, const u32 *formats, int n_formats) { struct drm_property_blob *blob;
- struct drm_connector *connector = &wb_connector->base;
- struct drm_connector *connector = wb_connector->base; struct drm_mode_config *config = &dev->mode_config; int ret = create_writeback_properties(dev);
@@ -189,14 +189,15 @@ int drm_writeback_connector_init(struct drm_device *dev, if (IS_ERR(blob)) return PTR_ERR(blob);
- drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
- ret = drm_encoder_init(dev, &wb_connector->encoder,
drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
ret = drm_encoder_init(dev, wb_connector->encoder, &drm_writeback_encoder_funcs, DRM_MODE_ENCODER_VIRTUAL, NULL); if (ret) goto fail;
connector->interlace_allowed = 0;
connector->wb_connector = wb_connector;
ret = drm_connector_init(dev, connector, con_funcs, DRM_MODE_CONNECTOR_WRITEBACK);
@@ -204,7 +205,7 @@ int drm_writeback_connector_init(struct drm_device *dev, goto connector_fail;
ret = drm_connector_attach_encoder(connector,
&wb_connector->encoder);
wb_connector->encoder);@@ -233,7 +234,7 @@ int drm_writeback_connector_init(struct drm_device *dev, attach_fail: drm_connector_cleanup(connector); connector_fail:
- drm_encoder_cleanup(&wb_connector->encoder);
- drm_encoder_cleanup(wb_connector->encoder); fail: drm_property_blob_put(blob); return ret;
@@ -263,7 +264,7 @@ int drm_writeback_prepare_job(struct drm_writeback_job *job) { struct drm_writeback_connector *connector = job->connector; const struct drm_connector_helper_funcs *funcs =
connector->base.helper_private;
connector->base->helper_private;int ret;
if (funcs->prepare_writeback_job) {
@@ -315,7 +316,7 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) { struct drm_writeback_connector *connector = job->connector; const struct drm_connector_helper_funcs *funcs =
connector->base.helper_private;
connector->base->helper_private;if (job->prepared && funcs->cleanup_writeback_job) funcs->cleanup_writeback_job(connector, job);
@@ -401,7 +402,7 @@ drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector) { struct dma_fence *fence;
- if (WARN_ON(wb_connector->base.connector_type !=
- if (WARN_ON(wb_connector->base->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)) return NULL;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index b501d0badaea..ec4657cfd7b9 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -44,6 +44,7 @@ struct drm_printer; struct drm_privacy_screen; struct edid; struct i2c_adapter; +struct drm_writeback_connector;
enum drm_connector_force { DRM_FORCE_UNSPECIFIED, @@ -1533,6 +1534,8 @@ struct drm_connector { */ struct drm_encoder *encoder;
- struct drm_writeback_connector *wb_connector;
- #define MAX_ELD_BYTES 128 /** @eld: EDID-like data, if present */ uint8_t eld[MAX_ELD_BYTES];
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d2714d2a..078c9907219c 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -22,7 +22,7 @@ struct drm_writeback_connector { /** * @base: base drm_connector object */
- struct drm_connector base;
struct drm_connector *base;
/**
- @encoder: Internal encoder used by the connector to fulfill
@@ -31,7 +31,7 @@ struct drm_writeback_connector { * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function. */
- struct drm_encoder encoder;
struct drm_encoder *encoder;
/**
- @pixel_formats_blob_ptr:
@@ -143,7 +143,7 @@ struct drm_writeback_job { static inline struct drm_writeback_connector * drm_connector_to_writeback(struct drm_connector *connector) {
- return container_of(connector, struct drm_writeback_connector, base);
return connector->wb_connector; }
int drm_writeback_connector_init(struct drm_device *dev,
- laurent on this
Hi Suraj Jani pointed me to this thread as i had posted something similar here : https://patchwork.freedesktop.org/patch/470296/ but since this thread was posted earlier, we can discuss further here.
Overall, its similar to what I had posted in the RFC and your commit text also covers my concerns too.
One question I have about your change is since you have changed wb_connector::encoder to be a pointer, i saw the other changes in the series but they do not allocate an encoder. Would this not affect the other drivers which are assuming that the encoder in wb_connector is struct drm_encoder encoder and not struct drm_encoder* encoder.
Your changes fix the compilation issue but wouldnt this crash as encoder wasnt allocated for other drivers.
Hi Abhinav, That shouldn't be an issue as normally drivers tend to have their own output structure which has drm_connector and drm_encoder embedded in it depending on the level of binding they have decided to give the connector and encoder in their respective output and the addresses of these are passed to the drm_connector* and drm_encoder* in drm_writeback_connector structure which then gets initialized in drm_writeback_connector_init().
In our i915 code we have intel_connector structure with drm_connector base field and intel_wd with a intel_encoder base which in turn has drm_encoder field and both intel_connector and intel_wd are initialized not requiring explicit alloc and dealloc for drm_encoder intel_wd = kzalloc(sizeof(*intel_wd), GFP_KERNEL);
intel_connector = intel_connector_alloc(); wb_conn = &intel_connector->wb_conn; wb_conn->base = &intel_connector->base; wb_conn->encoder = &intel_wd->base.base;
Similary for komeda driver has struct komeda_wb_connector { struct drm_connector conn; /** @base: &drm_writeback_connector */ struct drm_writeback_connector base;
/** @wb_layer: represents associated writeback pipeline of komeda */ struct komeda_layer *wb_layer; };
static int komeda_wb_connector_add(struct komeda_kms_dev *kms, struct komeda_crtc *kcrtc) { struct komeda_wb_connector *kwb_conn; struct drm_writeback_connector *wb_conn; kwb_conn = kzalloc(sizeof(*kwb_conn), GFP_KERNEL);
wb_conn = &kwb_conn->base; wb_conn->base = &kwb_conn->conn;
and they do not depend on the encoder binding so changes will work for them Also in vkms driver we have the 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; };
Which gets allocated covering for the drm_encoder alloc and dealloc
Regards, Suraj Kandpal
Hi Suraj
Thanks for the response.
I was not too worried about the intel driver as I am sure you must have validated this change with that :)
My question was more for the other vendor writeback drivers.
Thanks for looking into the others and providing the snippets. After looking at those, yes I also think it should work.
So, if others do not have any concern with this change,
Reviewed-by: Abhinav Kumar quic_abhinavk@quicinc.com
On 1/27/2022 1:33 AM, Kandpal, Suraj wrote:
- laurent on this
Hi Suraj Jani pointed me to this thread as i had posted something similar here : https://patchwork.freedesktop.org/patch/470296/ but since this thread was posted earlier, we can discuss further here.
Overall, its similar to what I had posted in the RFC and your commit text also covers my concerns too.
One question I have about your change is since you have changed wb_connector::encoder to be a pointer, i saw the other changes in the series but they do not allocate an encoder. Would this not affect the other drivers which are assuming that the encoder in wb_connector is struct drm_encoder encoder and not struct drm_encoder* encoder.
Your changes fix the compilation issue but wouldnt this crash as encoder wasnt allocated for other drivers.
Hi Abhinav, That shouldn't be an issue as normally drivers tend to have their own output structure which has drm_connector and drm_encoder embedded in it depending on the level of binding they have decided to give the connector and encoder in their respective output and the addresses of these are passed to the drm_connector* and drm_encoder* in drm_writeback_connector structure which then gets initialized in drm_writeback_connector_init().
In our i915 code we have intel_connector structure with drm_connector base field and intel_wd with a intel_encoder base which in turn has drm_encoder field and both intel_connector and intel_wd are initialized not requiring explicit alloc and dealloc for drm_encoder intel_wd = kzalloc(sizeof(*intel_wd), GFP_KERNEL);
intel_connector = intel_connector_alloc(); wb_conn = &intel_connector->wb_conn; wb_conn->base = &intel_connector->base; wb_conn->encoder = &intel_wd->base.base;
Similary for komeda driver has struct komeda_wb_connector { struct drm_connector conn; /** @base: &drm_writeback_connector */ struct drm_writeback_connector base;
/** @wb_layer: represents associated writeback pipeline of komeda */ struct komeda_layer *wb_layer;};
static int komeda_wb_connector_add(struct komeda_kms_dev *kms, struct komeda_crtc *kcrtc) { struct komeda_wb_connector *kwb_conn; struct drm_writeback_connector *wb_conn;
kwb_conn = kzalloc(sizeof(*kwb_conn), GFP_KERNEL);
wb_conn = &kwb_conn->base; wb_conn->base = &kwb_conn->conn;
and they do not depend on the encoder binding so changes will work for them Also in vkms driver we have the 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;};
Which gets allocated covering for the drm_encoder alloc and dealloc
Regards, Suraj Kandpal
Hi Suraj
I think there are more places affected with this change. I can get below compilation issues while trying to compile my branch:
drivers/gpu/drm/vc4/vc4_txp.c: In function ‘encoder_to_vc4_txp’: ./include/linux/build_bug.h:78:41: error: static assertion failed: "pointer type mismatch in container_of()" #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) ^ ./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’ #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) ^ ./include/linux/container_of.h:19:2: note: in expansion of macro ‘static_assert’ static_assert(__same_type(*(ptr), ((type *)0)->member) || \ ^ drivers/gpu/drm/vc4/vc4_txp.c:162:9: note: in expansion of macro ‘container_of’ return container_of(encoder, struct vc4_txp, connector.encoder); ^ drivers/gpu/drm/vc4/vc4_txp.c: In function ‘connector_to_vc4_txp’: ./include/linux/build_bug.h:78:41: error: static assertion failed: "pointer type mismatch in container_of()" #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) ^ ./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’ #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) ^ ./include/linux/container_of.h:19:2: note: in expansion of macro ‘static_assert’ static_assert(__same_type(*(ptr), ((type *)0)->member) || \ ^ drivers/gpu/drm/vc4/vc4_txp.c:167:9: note: in expansion of macro ‘container_of’ return container_of(conn, struct vc4_txp, connector.base); ^ drivers/gpu/drm/vc4/vc4_txp.c: In function ‘vc4_txp_bind’: drivers/gpu/drm/vc4/vc4_txp.c:495:27: error: passing argument 1 of ‘drm_connector_helper_add’ from incompatible pointer type [-Werror=incompatible-pointer-types] drm_connector_helper_add(&txp->connector.base, ^ In file included from ./include/drm/drm_atomic_helper.h:32:0, from drivers/gpu/drm/vc4/vc4_txp.c:17: ./include/drm/drm_modeset_helper_vtables.h:1153:20: note: expected ‘struct drm_connector *’ but argument is of type ‘struct drm_connector **’ static inline void drm_connector_helper_add(struct drm_connector *connector, ^ drivers/gpu/drm/vc4/vc4_txp.c:509:10: error: assignment from incompatible pointer type [-Werror=incompatible-pointer-types] encoder = &txp->connector.encoder; ^ drivers/gpu/drm/vc4/vc4_txp.c: In function ‘vc4_txp_unbind’: drivers/gpu/drm/vc4/vc4_txp.c:532:28: error: passing argument 1 of ‘vc4_txp_connector_destroy’ from incompatible pointer type [-Werror=incompatible-pointer-types] vc4_txp_connector_destroy(&txp->connector.base); ^ drivers/gpu/drm/vc4/vc4_txp.c:333:13: note: expected ‘struct drm_connector *’ but argument is of type ‘struct drm_connector **’ static void vc4_txp_connector_destroy(struct drm_connector *connector)
Looks like vc4 doesnt have its own encoder so we might have to move it to have its own?
struct vc4_txp { struct vc4_crtc base;
struct platform_device *pdev;
struct drm_writeback_connector connector;
void __iomem *regs; struct debugfs_regset32 regset; };
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder) { return container_of(encoder, struct vc4_txp, connector.encoder); }
static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn) { return container_of(conn, struct vc4_txp, connector.base); }
One more thing, it looks like to me, we need to do one more thing.
Like intel does and what MSM will do, the encoder pointer of the wb connector has to point to the encoder struct .
wb_conn = &intel_connector->wb_conn; wb_conn->base = &intel_connector->base; wb_conn->encoder = &intel_wd->base.base;
Thanks
Abhinav On 1/27/2022 10:17 AM, Abhinav Kumar wrote:
Hi Suraj
Thanks for the response.
I was not too worried about the intel driver as I am sure you must have validated this change with that :)
My question was more for the other vendor writeback drivers.
Thanks for looking into the others and providing the snippets. After looking at those, yes I also think it should work.
So, if others do not have any concern with this change,
Reviewed-by: Abhinav Kumar quic_abhinavk@quicinc.com
On 1/27/2022 1:33 AM, Kandpal, Suraj wrote:
- laurent on this
Hi Suraj Jani pointed me to this thread as i had posted something similar here : https://patchwork.freedesktop.org/patch/470296/ but since this thread was posted earlier, we can discuss further here.
Overall, its similar to what I had posted in the RFC and your commit text also covers my concerns too.
One question I have about your change is since you have changed wb_connector::encoder to be a pointer, i saw the other changes in the series but they do not allocate an encoder. Would this not affect the other drivers which are assuming that the encoder in wb_connector is struct drm_encoder encoder and not struct drm_encoder* encoder.
Your changes fix the compilation issue but wouldnt this crash as encoder wasnt allocated for other drivers.
Hi Abhinav, That shouldn't be an issue as normally drivers tend to have their own output structure which has drm_connector and drm_encoder embedded in it depending on the level of binding they have decided to give the connector and encoder in their respective output and the addresses of these are passed to the drm_connector* and drm_encoder* in drm_writeback_connector structure which then gets initialized in drm_writeback_connector_init().
In our i915 code we have intel_connector structure with drm_connector base field and intel_wd with a intel_encoder base which in turn has drm_encoder field and both intel_connector and intel_wd are initialized not requiring explicit alloc and dealloc for drm_encoder intel_wd = kzalloc(sizeof(*intel_wd), GFP_KERNEL);
intel_connector = intel_connector_alloc(); wb_conn = &intel_connector->wb_conn; wb_conn->base = &intel_connector->base; wb_conn->encoder = &intel_wd->base.base;
Similary for komeda driver has struct komeda_wb_connector { struct drm_connector conn; /** @base: &drm_writeback_connector */ struct drm_writeback_connector base;
/** @wb_layer: represents associated writeback pipeline of komeda */ struct komeda_layer *wb_layer; };
static int komeda_wb_connector_add(struct komeda_kms_dev *kms, struct komeda_crtc *kcrtc) { struct komeda_wb_connector *kwb_conn; struct drm_writeback_connector *wb_conn;
kwb_conn = kzalloc(sizeof(*kwb_conn), GFP_KERNEL);
wb_conn = &kwb_conn->base; wb_conn->base = &kwb_conn->conn; and they do not depend on the encoder binding so changes will work for them Also in vkms driver we have the 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; };
Which gets allocated covering for the drm_encoder alloc and dealloc
Regards, Suraj Kandpal
Hey,
I think there are more places affected with this change. I can get below compilation issues while trying to compile my branch:
drivers/gpu/drm/vc4/vc4_txp.c: In function ‘encoder_to_vc4_txp’: ./include/linux/build_bug.h:78:41: error: static assertion failed: "pointer type mismatch in container_of()" #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) ^ ./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’ #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) ^ ./include/linux/container_of.h:19:2: note: in expansion of macro ‘static_assert’ static_assert(__same_type(*(ptr), ((type *)0)->member) || \ ^ drivers/gpu/drm/vc4/vc4_txp.c:162:9: note: in expansion of macro ‘container_of’ return container_of(encoder, struct vc4_txp, connector.encoder); ^ drivers/gpu/drm/vc4/vc4_txp.c: In function ‘connector_to_vc4_txp’: ./include/linux/build_bug.h:78:41: error: static assertion failed: "pointer type mismatch in container_of()" #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) ^ ./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’ #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) ^ ./include/linux/container_of.h:19:2: note: in expansion of macro ‘static_assert’ static_assert(__same_type(*(ptr), ((type *)0)->member) || \ ^ drivers/gpu/drm/vc4/vc4_txp.c:167:9: note: in expansion of macro ‘container_of’ return container_of(conn, struct vc4_txp, connector.base); ^ drivers/gpu/drm/vc4/vc4_txp.c: In function ‘vc4_txp_bind’: drivers/gpu/drm/vc4/vc4_txp.c:495:27: error: passing argument 1 of ‘drm_connector_helper_add’ from incompatible pointer type [- Werror=incompatible-pointer-types] drm_connector_helper_add(&txp->connector.base, ^ In file included from ./include/drm/drm_atomic_helper.h:32:0, from drivers/gpu/drm/vc4/vc4_txp.c:17: ./include/drm/drm_modeset_helper_vtables.h:1153:20: note: expected ‘struct drm_connector *’ but argument is of type ‘struct drm_connector **’ static inline void drm_connector_helper_add(struct drm_connector *connector, ^ drivers/gpu/drm/vc4/vc4_txp.c:509:10: error: assignment from incompatible pointer type [-Werror=incompatible-pointer-types] encoder = &txp->connector.encoder; ^ drivers/gpu/drm/vc4/vc4_txp.c: In function ‘vc4_txp_unbind’: drivers/gpu/drm/vc4/vc4_txp.c:532:28: error: passing argument 1 of ‘vc4_txp_connector_destroy’ from incompatible pointer type [- Werror=incompatible-pointer-types] vc4_txp_connector_destroy(&txp->connector.base); ^ drivers/gpu/drm/vc4/vc4_txp.c:333:13: note: expected ‘struct drm_connector *’ but argument is of type ‘struct drm_connector **’ static void vc4_txp_connector_destroy(struct drm_connector *connector)
I will have a look at this and try to resolve it
Looks like vc4 doesnt have its own encoder so we might have to move it to have its own?
Right seems like we will have to add a drm_connector and drm_encoder in the below structure
struct vc4_txp { struct vc4_crtc base;
struct platform_device *pdev; struct drm_writeback_connector connector; void __iomem *regs; struct debugfs_regset32 regset;};
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder) { return container_of(encoder, struct vc4_txp, connector.encoder); }
static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn) { return container_of(conn, struct vc4_txp, connector.base); }
Changes will be required here in both connector_of functions to point to The new drm_connector and drm_encoder we add in vc4_txp struct.
One more thing, it looks like to me, we need to do one more thing.
Like intel does and what MSM will do, the encoder pointer of the wb connector has to point to the encoder struct .
wb_conn = &intel_connector->wb_conn; wb_conn->base = &intel_connector->base; wb_conn->encoder = &intel_wd->base.base;
Yes this will need to be done
Thanks Abhinav for pointing Ill implement this and send it in the next version of patches
Regards, Suraj Kandpal
Changing drm_connector and drm_encoder feilds to pointers in drm_writeback_connector as the elements of struct drm_writeback_connector are: struct drm_writeback_connector { struct drm_connector base; struct drm_encoder encoder; Similarly the elements of intel_encoder and intel_connector are: struct intel_encoder { struct drm_encoder base;
struct intel_connector { struct drm_connector base;
The function drm_writeback_connector_init() will initialize the drm_connector and drm_encoder and attach them as well. Since the drm_connector/encoder are both struct in drm_writeback_connector and intel_connector/encoder, we need one of them to be a pointer so we can reference them or else we will be pointing to 2 seprate instances.
Usually the struct defined in drm framework pointing to any struct will be pointer and allocating them and initialization will be done with the users. Like struct drm_connector and drm_encoder are part of drm framework and the users of these such as i915 have included them in their struct intel_connector and intel_encoder. Likewise struct drm_writeback_connector is a special connector and hence is not a user of drm_connector and hence this should be pointers.
Adding drm_writeback_connector to drm_connector so that writeback_connector can be fetched from drm_connector as the previous container_of method won't work due to change in the feilds of drm_connector and drm_encoder in drm_writeback_connector.
Note:The corresponding ripple effect due to the above changes namely in two drivers as I can see it komeda and vkms have been dealt with in the upcoming patches of this series.
Signed-off-by: Kandpal Suraj suraj.kandpal@intel.com --- drivers/gpu/drm/drm_writeback.c | 19 ++++++++++--------- include/drm/drm_connector.h | 3 +++ include/drm/drm_writeback.h | 6 +++--- 3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504f1bb..47238db42363 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -87,7 +87,7 @@ static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence) struct drm_writeback_connector *wb_connector = fence_to_wb_connector(fence);
- return wb_connector->base.dev->driver->name; + return wb_connector->base->dev->driver->name; }
static const char * @@ -177,7 +177,7 @@ int drm_writeback_connector_init(struct drm_device *dev, const u32 *formats, int n_formats) { struct drm_property_blob *blob; - struct drm_connector *connector = &wb_connector->base; + struct drm_connector *connector = wb_connector->base; struct drm_mode_config *config = &dev->mode_config; int ret = create_writeback_properties(dev);
@@ -189,14 +189,15 @@ int drm_writeback_connector_init(struct drm_device *dev, if (IS_ERR(blob)) return PTR_ERR(blob);
- drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); - ret = drm_encoder_init(dev, &wb_connector->encoder, + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); + ret = drm_encoder_init(dev, wb_connector->encoder, &drm_writeback_encoder_funcs, DRM_MODE_ENCODER_VIRTUAL, NULL); if (ret) goto fail;
connector->interlace_allowed = 0; + connector->wb_connector = wb_connector;
ret = drm_connector_init(dev, connector, con_funcs, DRM_MODE_CONNECTOR_WRITEBACK); @@ -204,7 +205,7 @@ int drm_writeback_connector_init(struct drm_device *dev, goto connector_fail;
ret = drm_connector_attach_encoder(connector, - &wb_connector->encoder); + wb_connector->encoder); if (ret) goto attach_fail;
@@ -233,7 +234,7 @@ int drm_writeback_connector_init(struct drm_device *dev, attach_fail: drm_connector_cleanup(connector); connector_fail: - drm_encoder_cleanup(&wb_connector->encoder); + drm_encoder_cleanup(wb_connector->encoder); fail: drm_property_blob_put(blob); return ret; @@ -263,7 +264,7 @@ int drm_writeback_prepare_job(struct drm_writeback_job *job) { struct drm_writeback_connector *connector = job->connector; const struct drm_connector_helper_funcs *funcs = - connector->base.helper_private; + connector->base->helper_private; int ret;
if (funcs->prepare_writeback_job) { @@ -315,7 +316,7 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) { struct drm_writeback_connector *connector = job->connector; const struct drm_connector_helper_funcs *funcs = - connector->base.helper_private; + connector->base->helper_private;
if (job->prepared && funcs->cleanup_writeback_job) funcs->cleanup_writeback_job(connector, job); @@ -401,7 +402,7 @@ drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector) { struct dma_fence *fence;
- if (WARN_ON(wb_connector->base.connector_type != + if (WARN_ON(wb_connector->base->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)) return NULL;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 64cf5f88c05b..fa06faeb7844 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -44,6 +44,7 @@ struct drm_printer; struct drm_privacy_screen; struct edid; struct i2c_adapter; +struct drm_writeback_connector;
enum drm_connector_force { DRM_FORCE_UNSPECIFIED, @@ -1539,6 +1540,8 @@ struct drm_connector { */ struct drm_encoder *encoder;
+ struct drm_writeback_connector *wb_connector; + #define MAX_ELD_BYTES 128 /** @eld: EDID-like data, if present */ uint8_t eld[MAX_ELD_BYTES]; diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d2714d2a..078c9907219c 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -22,7 +22,7 @@ struct drm_writeback_connector { /** * @base: base drm_connector object */ - struct drm_connector base; + struct drm_connector *base;
/** * @encoder: Internal encoder used by the connector to fulfill @@ -31,7 +31,7 @@ struct drm_writeback_connector { * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function. */ - struct drm_encoder encoder; + struct drm_encoder *encoder;
/** * @pixel_formats_blob_ptr: @@ -143,7 +143,7 @@ struct drm_writeback_job { static inline struct drm_writeback_connector * drm_connector_to_writeback(struct drm_connector *connector) { - return container_of(connector, struct drm_writeback_connector, base); + return connector->wb_connector; }
int drm_writeback_connector_init(struct drm_device *dev,
Making changes to komeda driver because we had to change drm_writeback_connector.base into a pointer the reason for which is expained in the Patch (drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj suraj.kandpal@intel.com --- drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 2 +- drivers/gpu/drm/arm/display/komeda/komeda_kms.h | 3 ++- .../gpu/drm/arm/display/komeda/komeda_wb_connector.c | 11 ++++++----- 3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c index 59172acb9738..eb37f41c1790 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c @@ -265,7 +265,7 @@ komeda_crtc_do_flush(struct drm_crtc *crtc, if (slave && has_bit(slave->id, kcrtc_st->affected_pipes)) komeda_pipeline_update(slave, old->state);
- conn_st = wb_conn ? wb_conn->base.base.state : NULL; + conn_st = wb_conn ? wb_conn->base.base->state : NULL; if (conn_st && conn_st->writeback_job) drm_writeback_queue_job(&wb_conn->base, conn_st);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h index 456f3c435719..8d83883a1d99 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h @@ -53,6 +53,7 @@ struct komeda_plane_state { * struct komeda_wb_connector */ struct komeda_wb_connector { + struct drm_connector conn; /** @base: &drm_writeback_connector */ struct drm_writeback_connector base;
@@ -136,7 +137,7 @@ struct komeda_kms_dev { static inline bool is_writeback_only(struct drm_crtc_state *st) { struct komeda_wb_connector *wb_conn = to_kcrtc(st->crtc)->wb_conn; - struct drm_connector *conn = wb_conn ? &wb_conn->base.base : NULL; + struct drm_connector *conn = wb_conn ? wb_conn->base.base : NULL;
return conn && (st->connector_mask == BIT(drm_connector_index(conn))); } diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c index e465cc4879c9..2c3dec59fd88 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c @@ -51,7 +51,7 @@ komeda_wb_encoder_atomic_check(struct drm_encoder *encoder, return -EINVAL; }
- wb_layer = to_kconn(to_wb_conn(conn_st->connector))->wb_layer; + wb_layer = to_kconn(drm_connector_to_writeback(conn_st->connector))->wb_layer;
/* * No need for a full modested when the only connector changed is the @@ -123,7 +123,7 @@ komeda_wb_connector_fill_modes(struct drm_connector *connector, static void komeda_wb_connector_destroy(struct drm_connector *connector) { drm_connector_cleanup(connector); - kfree(to_kconn(to_wb_conn(connector))); + kfree(to_kconn(drm_connector_to_writeback(connector))); }
static const struct drm_connector_funcs komeda_wb_connector_funcs = { @@ -155,7 +155,8 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms, kwb_conn->wb_layer = kcrtc->master->wb_layer;
wb_conn = &kwb_conn->base; - wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base)); + wb_conn->base = &kwb_conn->conn; + wb_conn->encoder->possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl, kwb_conn->wb_layer->layer_type, @@ -171,9 +172,9 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms, return err; }
- drm_connector_helper_add(&wb_conn->base, &komeda_wb_conn_helper_funcs); + drm_connector_helper_add(wb_conn->base, &komeda_wb_conn_helper_funcs);
- info = &kwb_conn->base.base.display_info; + info = &kwb_conn->base.base->display_info; info->bpc = __fls(kcrtc->master->improc->supported_color_depths); info->color_formats = kcrtc->master->improc->supported_color_formats;
Changing vkms driver to accomadate the change of drm_writeback_connector.base to a pointer the reason for which is explained in the Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj suraj.kandpal@intel.com --- drivers/gpu/drm/vkms/vkms_writeback.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index 8694227f555f..374431471f49 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -114,7 +114,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn, struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev); struct vkms_output *output = &vkmsdev->output; struct drm_writeback_connector *wb_conn = &output->wb_connector; - struct drm_connector_state *conn_state = wb_conn->base.state; + struct drm_connector_state *conn_state = wb_conn->base->state; struct vkms_crtc_state *crtc_state = output->composer_state;
if (!conn_state) @@ -139,9 +139,12 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = { int vkms_enable_writeback_connector(struct vkms_device *vkmsdev) { struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector; + struct vkms_output *output = &vkmsdev->output;
- vkmsdev->output.wb_connector.encoder.possible_crtcs = 1; - drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs); + wb->base = &output->connector; + wb->encoder = &output->encoder; + 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, &vkms_wb_connector_funcs,
Changing vc4 driver to accomadate the change of drm_writeback_connector.base and drm_writeback_connector.encoder to a pointer the reason for which is explained in the Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj suraj.kandpal@intel.com --- drivers/gpu/drm/vc4/vc4_txp.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 9809ca3e2945..9882569d147c 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -151,6 +151,10 @@ struct vc4_txp {
struct platform_device *pdev;
+ struct drm_connector drm_conn; + + struct drm_encoder drm_enc; + struct drm_writeback_connector connector;
void __iomem *regs; @@ -159,12 +163,12 @@ struct vc4_txp {
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder) { - return container_of(encoder, struct vc4_txp, connector.encoder); + return container_of(encoder, struct vc4_txp, drm_enc); }
static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn) { - return container_of(conn, struct vc4_txp, connector.base); + return container_of(conn, struct vc4_txp, drm_conn); }
static const struct debugfs_reg32 txp_regs[] = { @@ -467,6 +471,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) struct vc4_txp *txp; struct drm_crtc *crtc; struct drm_encoder *encoder; + struct drm_writeback_connector *wb_conn; int ret, irq;
irq = platform_get_irq(pdev, 0); @@ -491,10 +496,13 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) txp->regset.base = txp->regs; txp->regset.regs = txp_regs; txp->regset.nregs = ARRAY_SIZE(txp_regs); + wb_conn = &txp->connector; + wb_conn->base = &txp->drm_conn; + wb_conn->encoder = &txp->drm_enc;
- drm_connector_helper_add(&txp->connector.base, + drm_connector_helper_add(wb_conn->base, &vc4_txp_connector_helper_funcs); - ret = drm_writeback_connector_init(drm, &txp->connector, + ret = drm_writeback_connector_init(drm, wb_conn, &vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs, drm_fmts, ARRAY_SIZE(drm_fmts)); @@ -506,7 +514,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) if (ret) return ret;
- encoder = &txp->connector.encoder; + encoder = txp->connector.encoder; encoder->possible_crtcs = drm_crtc_mask(crtc);
ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0, @@ -529,7 +537,7 @@ static void vc4_txp_unbind(struct device *dev, struct device *master, struct vc4_dev *vc4 = to_vc4_dev(drm); struct vc4_txp *txp = dev_get_drvdata(dev);
- vc4_txp_connector_destroy(&txp->connector.base); + vc4_txp_connector_destroy(txp->connector.base);
vc4->txp = NULL; }
Changing rcar_du driver to accomadate the change of drm_writeback_connector.base and drm_writeback_connector.encoder to a pointer the reason for which is explained in the Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj suraj.kandpal@intel.com --- drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index 66e8839db708..68f387a04502 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h @@ -72,6 +72,8 @@ struct rcar_du_crtc { const char *const *sources; unsigned int sources_count;
+ struct drm_connector connector; + struct drm_encoder encoder; struct drm_writeback_connector writeback; };
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index c79d1259e49b..5b1e83380c47 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, { struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
- wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); - drm_connector_helper_add(&wb_conn->base, + wb_conn->base = &rcrtc->connector; + wb_conn->encoder = &rcrtc->encoder; + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); + drm_connector_helper_add(wb_conn->base, &rcar_du_wb_conn_helper_funcs);
return drm_writeback_connector_init(&rcdu->ddev, wb_conn, @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, struct drm_framebuffer *fb; unsigned int i;
- state = rcrtc->writeback.base.state; + state = rcrtc->writeback.base->state; if (!state || !state->writeback_job) return;
Changing malidp driver to accomadate the change of drm_writeback_connector.base and drm_writeback_connector.encoder to a pointer the reason for which is explained in the Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj suraj.kandpal@intel.com --- drivers/gpu/drm/arm/malidp_crtc.c | 2 +- drivers/gpu/drm/arm/malidp_drv.h | 2 ++ drivers/gpu/drm/arm/malidp_mw.c | 12 ++++++++---- 3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c index 494075ddbef6..294aacd4beef 100644 --- a/drivers/gpu/drm/arm/malidp_crtc.c +++ b/drivers/gpu/drm/arm/malidp_crtc.c @@ -424,7 +424,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc, u32 new_mask = crtc_state->connector_mask;
if ((old_mask ^ new_mask) == - (1 << drm_connector_index(&malidp->mw_connector.base))) + (1 << drm_connector_index(malidp->mw_connector.base))) crtc_state->connectors_changed = false; }
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h index cdfddfabf2d1..971810a685f1 100644 --- a/drivers/gpu/drm/arm/malidp_drv.h +++ b/drivers/gpu/drm/arm/malidp_drv.h @@ -31,6 +31,8 @@ struct malidp_error_stats { struct malidp_drm { struct malidp_hw_device *dev; struct drm_crtc crtc; + struct drm_connector connector; + struct drm_encoder encoder; struct drm_writeback_connector mw_connector; wait_queue_head_t wq; struct drm_pending_vblank_event *event; diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c index f5847a79dd7e..9bd2e400cd3d 100644 --- a/drivers/gpu/drm/arm/malidp_mw.c +++ b/drivers/gpu/drm/arm/malidp_mw.c @@ -206,21 +206,25 @@ static u32 *get_writeback_formats(struct malidp_drm *malidp, int *n_formats) int malidp_mw_connector_init(struct drm_device *drm) { struct malidp_drm *malidp = drm->dev_private; + struct drm_writeback_connector *wb_conn; u32 *formats; int ret, n_formats;
if (!malidp->dev->hw->enable_memwrite) return 0;
- malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc); - drm_connector_helper_add(&malidp->mw_connector.base, + wb_conn = &malidp->mw_connector; + wb_conn->base = &malidp->connector; + wb_conn->encoder = &malidp->encoder; + malidp->mw_connector.encoder->possible_crtcs = 1 << drm_crtc_index(&malidp->crtc); + drm_connector_helper_add(wb_conn->base, &malidp_mw_connector_helper_funcs);
formats = get_writeback_formats(malidp, &n_formats); if (!formats) return -ENOMEM;
- ret = drm_writeback_connector_init(drm, &malidp->mw_connector, + ret = drm_writeback_connector_init(drm, wb_conn, &malidp_mw_connector_funcs, &malidp_mw_encoder_helper_funcs, formats, n_formats); @@ -236,7 +240,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm, { struct malidp_drm *malidp = drm->dev_private; struct drm_writeback_connector *mw_conn = &malidp->mw_connector; - struct drm_connector_state *conn_state = mw_conn->base.state; + struct drm_connector_state *conn_state = mw_conn->base->state; struct malidp_hw_device *hwdev = malidp->dev; struct malidp_mw_connector_state *mw_state;
dri-devel@lists.freedesktop.org
-
Abhinav Kumar -
Carsten Haitzler -
Kandpal Suraj -
Kandpal, Suraj