[Intel-gfx] RFC: pipe writeback design for i915

[Intel-gfx] RFC: pipe writeback design for i915

[Bharadiya,Pankaj]

I am exploring the way of implementing the pipe writeback feature in i915 and
would like to get early feedback on design.

We have a Wireless display(WD) transcoder which can be used for capturing
display pipe output to memory. It is generally intended for wireless display,
but can be used for other functions such as in validation automation where crc
based comparison is not feasible.

Bspec: 49275

DRM core provides writeback connectors framework (drm_writeback.c) which can
be used to expose hardware which can write the output from a pipe to a memory
buffer.

Writeback connectors have some additional properties, which userspace can use
to query and control them, For more details, please refer [1]

[1] https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html#writeback-connectors


In order to implement pipe writeback feature in i915 using drm writeback
connector framework, I am exploring below possibilities.

  1. Extend the intel_connector to support writeback
  2. Introduce new intel_writeback_connector type
  3. ?? (any other better way?)

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; |
  |         .                           |            .                          |
  |         .                           |            .                          |
  |         .                           |            .                          |
  | };                                  |    };                                 |
  |                                     |                                       |
  +-----------------------------------------------------------------------------+

I see below issues for extending intel_connector to support
drm_writeback_connector

   - extending intel_connector as drm_writeback connector will introduce 2
     copies of drm_connector.

        struct intel_connector {
        		struct drm_connector base;

			// new addition to handle wirteback
        		struct drm_writeback_connector wb_conn;
        		.
        		.
        		.
        };

   - drm_writeback_connector_init() will initalize wb_conn.base (drm_connector)
     and we extract intel_encoder related functions (update_prepare, pre_enable,
     disable, etc) from intel_connector.base (which will never get initialized
     for writeback connector use case)

     e.g. intel_display.c

          static void intel_encoders_update_prepare(struct intel_atomic_state *state)
          {
          .
          .
          .
           intel_connector = to_intel_connector(connector);
          				encoder = intel_connector_primary_encoder(intel_connector);
          				if (!encoder->update_prepare)
          						continue;

          				crtc = new_conn_state->crtc ?
          						to_intel_crtc(new_conn_state->crtc) : NULL;
          				encoder->update_prepare(state, encoder, crtc);
          }


Extending intel_connector to support drm_writeback_connector does not seem to
be logical to me.
Am I missing anything here? Can someone suggest a better approach?


2. Introduce new intel_writeback_connector connector type
---------------------------------------------------------

I feel introducing the intel_writeback_connector is a logical approach, as it
will follow the standard way of derivation from drm core structs (like
intel_connector -> drm_connector, intel_encoder -> drm_encoder, etc)

        struct intel_writeback_connector {
        		struct drm_writeback_connector base;
        		.
        		.
        		.
        };

And, I am thinking of below design -


         +--------------------------------------------------------------+
         |                          DRM CORE                            |
         |                                                              |
         |   +-------------------------+       +--------------------+   |
         |   | drm writeback connector |------>|   drm connector    |   |
         |   +-------------------------+       +--------------------+   |
         |      ^                                                  ^    |
         +------|--------------------------------------------------|----+
                |                       ^                          |
                |                       |                          |
                |                       v                          |
                |       +--------------------------------+         |
                |       |        intel display           |         |
                |       |      (intel_display.c)         |         |
                |       +--------------------------------+         |
                |              ^                  ^                |
                |              |                  |                |
                |              v                  v                |
     +-----------+       +-----------+      +-----------+       +-----------+
     | intel     |       |           |      |   intel   |       |  intel    |
     | writeback |<------|  intel wd | ...  |   hdmi    |------>|  connector|
     | connector |       |           |      |           |       |           |
     +-----------+       +-----------+      +-----------+       +-----------+
    (intel_writeback     (intel_wd.c)       (intel_hdmi.c)      (intel_connector.c)
     _connector.c)


   - Introduce intel_writeback_connector struct which will be of
     drm_writeback_connector type.
   - intel_writeback_connector.c: Will have intel writeback connector helper
     functions.
   - intel_wd.c: will register the drm_writeback_connector and will have
     WD transcoder implementation

To support this design, we will have to modify the intel_connector
related macros and portion display driver where intel_connector related calls
are made(and may be some other part of code).

Ex:
   - Identify the drm_connector type (drm_connector.connector_type,
     DRM_MODE_CONNECTOR_WRITEBACK) and based on connector type select either
     intel_connector or intel_writeback_connector and identify correct
     intel_encoder to make encoder related calls (update_prepare, pre_enable,
     disable, etc) 

Is this the right approach? Do you see any issues/challenges with this?

I would like to get early feedback before I really dive into code. 
Your thoughts and suggestions are much appreciated.

Thanks,
Pankaj

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] RFC: pipe writeback design for i915

[Jani Nikula]

On Fri, 31 Jan 2020, "Bharadiya,Pankaj" <pankaj.laxminarayan.bharadiya@intel.com> wrote:
> I am exploring the way of implementing the pipe writeback feature in i915 and
> would like to get early feedback on design.
>
> We have a Wireless display(WD) transcoder which can be used for capturing
> display pipe output to memory. It is generally intended for wireless display,
> but can be used for other functions such as in validation automation where crc
> based comparison is not feasible.

I think you should probably explore the use case and driver/igt impact
further before embarking on the implementation.

- How much do you need to modify existing code in kernel and igt to make
  use of writeback connectors?

- What kind of test coverage do you get? Pipe CRC is used in connection
  with the physical encoders. In contrast, you won't have that with WD
  transcoders. (Design wise I think this may mean you'll also need
  "writeback encoders", instead of trying to plug it into existing
  encoders.) So you'll only test the pipe side of things, which roughly
  corresponds to pipe CRC coverage I guess. I guess it could speed up
  that part of testing because you can then skip the physical
  connectors, but you do have to test them also. So it's not a panacea.


BR,
Jani.


>
> Bspec: 49275
>
> DRM core provides writeback connectors framework (drm_writeback.c) which can
> be used to expose hardware which can write the output from a pipe to a memory
> buffer.
>
> Writeback connectors have some additional properties, which userspace can use
> to query and control them, For more details, please refer [1]
>
> [1] https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html#writeback-connectors
>
>
> In order to implement pipe writeback feature in i915 using drm writeback
> connector framework, I am exploring below possibilities.
>
>   1. Extend the intel_connector to support writeback
>   2. Introduce new intel_writeback_connector type
>   3. ?? (any other better way?)
>
> 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; |
>   |         .                           |            .                          |
>   |         .                           |            .                          |
>   |         .                           |            .                          |
>   | };                                  |    };                                 |
>   |                                     |                                       |
>   +-----------------------------------------------------------------------------+
>
> I see below issues for extending intel_connector to support
> drm_writeback_connector
>
>    - extending intel_connector as drm_writeback connector will introduce 2
>      copies of drm_connector.
>
>         struct intel_connector {
>         		struct drm_connector base;
>
> 			// new addition to handle wirteback
>         		struct drm_writeback_connector wb_conn;
>         		.
>         		.
>         		.
>         };
>
>    - drm_writeback_connector_init() will initalize wb_conn.base (drm_connector)
>      and we extract intel_encoder related functions (update_prepare, pre_enable,
>      disable, etc) from intel_connector.base (which will never get initialized
>      for writeback connector use case)
>
>      e.g. intel_display.c
>
>           static void intel_encoders_update_prepare(struct intel_atomic_state *state)
>           {
>           .
>           .
>           .
>            intel_connector = to_intel_connector(connector);
>           				encoder = intel_connector_primary_encoder(intel_connector);
>           				if (!encoder->update_prepare)
>           						continue;
>
>           				crtc = new_conn_state->crtc ?
>           						to_intel_crtc(new_conn_state->crtc) : NULL;
>           				encoder->update_prepare(state, encoder, crtc);
>           }
>
>
> Extending intel_connector to support drm_writeback_connector does not seem to
> be logical to me.
> Am I missing anything here? Can someone suggest a better approach?
>
>
> 2. Introduce new intel_writeback_connector connector type
> ---------------------------------------------------------
>
> I feel introducing the intel_writeback_connector is a logical approach, as it
> will follow the standard way of derivation from drm core structs (like
> intel_connector -> drm_connector, intel_encoder -> drm_encoder, etc)
>
>         struct intel_writeback_connector {
>         		struct drm_writeback_connector base;
>         		.
>         		.
>         		.
>         };
>
> And, I am thinking of below design -
>
>
>          +--------------------------------------------------------------+
>          |                          DRM CORE                            |
>          |                                                              |
>          |   +-------------------------+       +--------------------+   |
>          |   | drm writeback connector |------>|   drm connector    |   |
>          |   +-------------------------+       +--------------------+   |
>          |      ^                                                  ^    |
>          +------|--------------------------------------------------|----+
>                 |                       ^                          |
>                 |                       |                          |
>                 |                       v                          |
>                 |       +--------------------------------+         |
>                 |       |        intel display           |         |
>                 |       |      (intel_display.c)         |         |
>                 |       +--------------------------------+         |
>                 |              ^                  ^                |
>                 |              |                  |                |
>                 |              v                  v                |
>      +-----------+       +-----------+      +-----------+       +-----------+
>      | intel     |       |           |      |   intel   |       |  intel    |
>      | writeback |<------|  intel wd | ...  |   hdmi    |------>|  connector|
>      | connector |       |           |      |           |       |           |
>      +-----------+       +-----------+      +-----------+       +-----------+
>     (intel_writeback     (intel_wd.c)       (intel_hdmi.c)      (intel_connector.c)
>      _connector.c)
>
>
>    - Introduce intel_writeback_connector struct which will be of
>      drm_writeback_connector type.
>    - intel_writeback_connector.c: Will have intel writeback connector helper
>      functions.
>    - intel_wd.c: will register the drm_writeback_connector and will have
>      WD transcoder implementation
>
> To support this design, we will have to modify the intel_connector
> related macros and portion display driver where intel_connector related calls
> are made(and may be some other part of code).
>
> Ex:
>    - Identify the drm_connector type (drm_connector.connector_type,
>      DRM_MODE_CONNECTOR_WRITEBACK) and based on connector type select either
>      intel_connector or intel_writeback_connector and identify correct
>      intel_encoder to make encoder related calls (update_prepare, pre_enable,
>      disable, etc) 
>
> Is this the right approach? Do you see any issues/challenges with this?
>
> I would like to get early feedback before I really dive into code. 
> Your thoughts and suggestions are much appreciated.
>
> Thanks,
> Pankaj
>

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] RFC: pipe writeback design for i915

[Ville Syrjälä]

On Fri, Jan 31, 2020 at 12:55:45PM +0200, Jani Nikula wrote:
> On Fri, 31 Jan 2020, "Bharadiya,Pankaj" <pankaj.laxminarayan.bharadiya@intel.com> wrote:
> > I am exploring the way of implementing the pipe writeback feature in i915 and
> > would like to get early feedback on design.
> >
> > We have a Wireless display(WD) transcoder which can be used for capturing
> > display pipe output to memory. It is generally intended for wireless display,
> > but can be used for other functions such as in validation automation where crc
> > based comparison is not feasible.
> 
> I think you should probably explore the use case and driver/igt impact
> further before embarking on the implementation.
> 
> - How much do you need to modify existing code in kernel and igt to make
>   use of writeback connectors?
> 
> - What kind of test coverage do you get? Pipe CRC is used in connection
>   with the physical encoders. In contrast, you won't have that with WD
>   transcoders. (Design wise I think this may mean you'll also need
>   "writeback encoders", instead of trying to plug it into existing
>   encoders.) So you'll only test the pipe side of things, which roughly
>   corresponds to pipe CRC coverage I guess. I guess it could speed up
>   that part of testing because you can then skip the physical
>   connectors, but you do have to test them also. So it's not a panacea.

The main benefit I'm looking forward to is for reverse engineering.
As in answwering the age old question: "let me see wtf the hw is
actually doing to my pixels?". I want this!

-- 
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] RFC: pipe writeback design for i915

[Ville Syrjälä]

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.
> 
> We have a Wireless display(WD) transcoder which can be used for capturing
> display pipe output to memory. It is generally intended for wireless display,
> but can be used for other functions such as in validation automation where crc
> based comparison is not feasible.
> 
> Bspec: 49275
> 
> DRM core provides writeback connectors framework (drm_writeback.c) which can
> be used to expose hardware which can write the output from a pipe to a memory
> buffer.
> 
> Writeback connectors have some additional properties, which userspace can use
> to query and control them, For more details, please refer [1]
> 
> [1] https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html#writeback-connectors
> 
> 
> In order to implement pipe writeback feature in i915 using drm writeback
> connector framework, I am exploring below possibilities.
> 
>   1. Extend the intel_connector to support writeback
>   2. Introduce new intel_writeback_connector type
>   3. ?? (any other better way?)
> 
> 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().

-- 
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] RFC: pipe writeback design for i915

[Shankar, Uma]

> -----Original Message-----
> From: Syrjala, Ville <ville.syrjala@intel.com>
> Sent: Friday, January 31, 2020 5:22 PM
> To: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Laxminarayan Bharadiya, Pankaj <pankaj.laxminarayan.bharadiya@intel.com>;
> daniel@ffwll.ch; Deak, Imre <imre.deak@intel.com>; Vivi, Rodrigo
> <rodrigo.vivi@intel.com>; intel-gfx@lists.freedesktop.org; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: Re: RFC: pipe writeback design for i915
> 
> On Fri, Jan 31, 2020 at 12:55:45PM +0200, Jani Nikula wrote:
> > On Fri, 31 Jan 2020, "Bharadiya,Pankaj"
> <pankaj.laxminarayan.bharadiya@intel.com> wrote:
> > > I am exploring the way of implementing the pipe writeback feature in
> > > i915 and would like to get early feedback on design.
> > >
> > > We have a Wireless display(WD) transcoder which can be used for
> > > capturing display pipe output to memory. It is generally intended
> > > for wireless display, but can be used for other functions such as in
> > > validation automation where crc based comparison is not feasible.
> >
> > I think you should probably explore the use case and driver/igt impact
> > further before embarking on the implementation.
> >
> > - How much do you need to modify existing code in kernel and igt to make
> >   use of writeback connectors?
> >
> > - What kind of test coverage do you get? Pipe CRC is used in connection
> >   with the physical encoders. In contrast, you won't have that with WD
> >   transcoders. (Design wise I think this may mean you'll also need
> >   "writeback encoders", instead of trying to plug it into existing
> >   encoders.) So you'll only test the pipe side of things, which roughly
> >   corresponds to pipe CRC coverage I guess. I guess it could speed up
> >   that part of testing because you can then skip the physical
> >   connectors, but you do have to test them also. So it's not a panacea.
> 
> The main benefit I'm looking forward to is for reverse engineering.
> As in answwering the age old question: "let me see wtf the hw is actually doing to
> my pixels?". I want this!

This will be useful even for all color validation and various other tests where due to pipe precision
and interpolation we don't have exact matches wrt crc's. Currently Chamelium comes as a savior,
but this limits to boards where we have this external hardware. This will help make things
not depend on availability of Chamelium in CI.
From IGT side, changes will be needed to enable this and migrate the respective tests, make them
use writeback dumps and compare.

Another advantage will be definitely in debugging, root causing the real culprits. If things are going
bad at port or pipe itself. In all this, we do get the real thing working i.e. a wireless display 😊 (rest of the stack
will need to be enabled but display side we will get our stuff enabled)

Regards,
Uma Shankar


> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] RFC: pipe writeback design for i915

[Shankar, Uma]

> -----Original Message-----
> From: Laxminarayan Bharadiya, Pankaj <pankaj.laxminarayan.bharadiya@intel.com>
> Sent: Tuesday, February 4, 2020 1:35 PM
> To: Syrjala, Ville <ville.syrjala@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>; daniel@ffwll.ch; Deak, Imre
> <imre.deak@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; intel-
> gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>; Laxminarayan
> Bharadiya, Pankaj <pankaj.laxminarayan.bharadiya@intel.com>
> Subject: Re: [Intel-gfx] RFC: pipe writeback design for i915
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Ville Syrjälä
> > Sent: Friday, January 31, 2020 5:28 PM
> > To: Laxminarayan Bharadiya, Pankaj
> > <pankaj.laxminarayan.bharadiya@intel.com>
> > Cc: intel-gfx@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] RFC: pipe writeback design for i915

[Bharadiya,Pankaj]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjälä
> Sent: Friday, January 31, 2020 5:28 PM
> To: Laxminarayan Bharadiya, Pankaj <pankaj.laxminarayan.bharadiya@intel.com>
> Cc: intel-gfx@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?

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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx