| Message ID | 20211217071356.12517-2-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adding writeback support for i915 | expand |
On Fri, 17 Dec 2021, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: > From: suraj kandpal <suraj.kandpal@intel.com> > > Adding drm_writeback_connector to drm_connector so that > writeback_connector can be fetched from drm_connector This needs some explaining, since drm_connector_to_writeback() does exactly that. > Adding drm_connector and drm_encoder pointers in drm_writeback_connector Why? We can all read the code, the commit message should mostly be about the *why*. Also, drm changes should Cc: dri-devel mailing list. BR, Jani. > > Signed-off-by: suraj kandpal <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 379746d3266f..ec049b1bc4bb 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -42,6 +42,7 @@ struct drm_property_blob; > struct drm_printer; > struct edid; > struct i2c_adapter; > +struct drm_writeback_connector; > > enum drm_connector_force { > DRM_FORCE_UNSPECIFIED, > @@ -1483,6 +1484,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,
> > Adding drm_connector and drm_encoder pointers in > > drm_writeback_connector > > Why? 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, one of them should be a pointer, else both will be pointing to 2 separate 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. > > We can all read the code, the commit message should mostly be about the > *why*. > > Also, drm changes should Cc: dri-devel mailing list. Sure we will add this in Cc in the next series of patches Thanks, Suraj Kandpal
On Thu, 06 Jan 2022, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: >> > Adding drm_connector and drm_encoder pointers in >> > drm_writeback_connector >> >> Why? > > 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, one of them should be a pointer, else both will be pointing to 2 separate 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. Okay, I can see the problem, but the solution you present breaks at least komeda, mali, rcard-du, vc4, and vkms drivers. Either you need to change all of them, or figure out another way. I don't know off the top of my head what the correct answer should be, but you can't break other drivers. Again, maybe you'll get better ideas with a bigger audience than just intel-gfx. BR, Jani. >> >> We can all read the code, the commit message should mostly be about the >> *why*. >> >> Also, drm changes should Cc: dri-devel mailing list. > Sure we will add this in Cc in the next series of patches > > Thanks, > Suraj Kandpal
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 379746d3266f..ec049b1bc4bb 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -42,6 +42,7 @@ struct drm_property_blob; struct drm_printer; struct edid; struct i2c_adapter; +struct drm_writeback_connector; enum drm_connector_force { DRM_FORCE_UNSPECIFIED, @@ -1483,6 +1484,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,