[Intel-gfx] RFC: pipe writeback design for i915
Shankar, Uma
uma.shankar at intel.com
Wed Feb 12 05:36:50 UTC 2020
> -----Original Message-----
> From: Laxminarayan Bharadiya, Pankaj <pankaj.laxminarayan.bharadiya at intel.com>
> Sent: Tuesday, February 4, 2020 1:35 PM
> To: Syrjala, Ville <ville.syrjala at intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>; daniel at ffwll.ch; Deak, Imre
> <imre.deak at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>; intel-
> gfx at lists.freedesktop.org; Shankar, Uma <uma.shankar at intel.com>; Laxminarayan
> Bharadiya, Pankaj <pankaj.laxminarayan.bharadiya at intel.com>
> Subject: Re: [Intel-gfx] RFC: pipe writeback design for i915
>
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of
> > Ville Syrjälä
> > Sent: Friday, January 31, 2020 5:28 PM
> > To: Laxminarayan Bharadiya, Pankaj
> > <pankaj.laxminarayan.bharadiya at intel.com>
> > Cc: intel-gfx at lists.freedesktop.org
> > Subject: Re: [Intel-gfx] RFC: pipe writeback design for i915
> >
> > On Fri, Jan 31, 2020 at 12:00:39PM +0530, Bharadiya,Pankaj wrote:
> > > I am exploring the way of implementing the pipe writeback feature in
> > > i915 and would like to get early feedback on design.
>
> [snip]
>
> > >
> > > 1# Extend the intel_connector to support writeback
> > > --------------------------------------------------
> > >
> > > drm_writeback connector is of drm_connector type and intel_connector
> > > is also of drm_connector type.
> > >
> > > +-----------------------------------------------------------------------------+
> > > | | |
> > > | struct drm_writeback_connector { | struct intel_connector { |
> > > | struct drm_connector base; | struct drm_connector base; |
> > > | . | . |
> > > | . | . |
> > > | . | . |
> > > | }; | }; |
> > > | | |
> > >
> > > +-------------------------------------------------------------------
> > > +--
> > > --------+
> >
> > That's a bit unfortunate. We like to use intel_connector quite a bit
> > in
> > i915 so having two different types is going to be a pita. Ideally I guess the
> writeback connector shouldn't be a drm_connector at all and instead it would just
> provide some kind of thing to embed into the driver's connector struct. But that
> would mean the writeback helpers would need some other way to get at that data
> rather than just container_of().
>
> I am thinking of the following -
>
> - Modify the struct drm_writeback_connector accept drm_connector pointer (*base)
> - Add new member in struct drm_connector to save struct drm_writeback_connector
> pointer so that drm_writeback_connector can be found using given a
> drm_connector.
> - Modify existing drivers (rcar_du, arm/malidp, arm/komeda, vc4) which are
> implementing drm_writeback to adapt to this new change.
>
> Here is the example patch I came with -
>
> ----------------------
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 43d9e3bb3a94..cb4434baa2eb 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 *
> @@ -178,7 +178,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);
>
> @@ -198,6 +198,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> goto fail;
>
> connector->interlace_allowed = 0;
> + connector->wb_connector = wb_connector;
>
> ret = drm_connector_init(dev, connector, con_funcs,
> DRM_MODE_CONNECTOR_WRITEBACK);
> @@ -264,7 +265,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) {
> @@ -316,7 +317,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); @@ -402,7 +403,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
> 2113500b4075..edd153f1815e 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,
> @@ -1315,6 +1316,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
> 777c14c847f0..51a94c6a4ae3 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -16,7 +16,7 @@
> #include <linux/workqueue.h>
>
> struct drm_writeback_connector {
> - struct drm_connector base;
> + struct drm_connector *base;
>
> /**
> * @encoder: Internal encoder used by the connector to fulfill @@ -134,7
> +134,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,
>
> ---------------------
>
>
> With this, we should be able to extend intel_connector to support writeback.
>
> struct intel_connector {
> struct drm_connector base;
> + struct drm_writeback_connector wb_conn;
> .
> .
> .
> }
>
> Example usage:
> struct intel_connector *intel_connector;
> intel_connector = intel_connector_alloc();
>
> intel_connector->wb_conn.base = &intel_connector->base;
>
> /* Initialize writeback connector */
> drm_writeback_connector_init(...,&intel_connector->wb_conn, ...);
>
>
> What do you think?
I feel adding a pointer as base could work. But since it involves a major change in drm core, please
involve the dri-devel also in this discussion.
Changing the write_back_connector and decoupling from drm_connector will involve lot of re-structuring in
all the drm drivers currently using the writeback framework as well helpers needed to be added for the same.
Ville/Jani N: How should we approach this ?
Regards,
Uma Shankar
> Thanks,
> Pankaj
>
> >
> > --
> > Ville Syrjälä
> > Intel
> > ---------------------------------------------------------------------
> > Intel Finland Oy
> > Registered Address: PL 281, 00181 Helsinki Business Identity Code:
> > 0357606 - 4 Domiciled in Helsinki
> >
> > This e-mail and any attachments may contain confidential material for the sole use
> of the intended recipient(s). Any review or distribution by others is strictly prohibited.
> If you are not the intended recipient, please contact the sender and delete all copies.
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list