[PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Kandpal, Suraj]

> Subject: Re: [PATCH 03/28] drm/writeback: Define function to get
> drm_connector from writeback
> 
> > > > >
> > > > > > Now that drm_connector may not always be embedded within
> > > > > > drm_writeback_connector, let's define a function which either
> > > > > > uses the writeback helper hook that returns the drm_connector
> > > > > > associated with the drm_writeback_connector or just return the
> > > > > > drm_connector embedded inside drm_writeback_connector if the
> > > > > > helper hook is not present. Lets use this function and call it
> > > > > > whenever drm_connector is required mostly when connector
> > > > > > helper
> > > private funcs need to be fetched.
> > > > > >
> > > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_writeback.c | 33
> > > > > > ++++++++++++++++++++++++++-------
> > > > > >  1 file changed, 26 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > > > > b/drivers/gpu/drm/drm_writeback.c index
> > > > > > e9f7123270d6..d610cb827975
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > > @@ -120,6 +120,18 @@ drm_connector_to_writeback(struct
> > > > > drm_connector
> > > > > > *connector)  }  EXPORT_SYMBOL(drm_connector_to_writeback);
> > > > > >
> > > > > > +static struct drm_connector *
> > > > > > +drm_connector_from_writeback(struct drm_writeback_connector
> > > > > > +*wb_connector) {
> > > > > > +	const struct drm_writeback_connector_helper_funcs *funcs =
> > > > > > +		wb_connector->helper_private;
> > > > > > +
> > > > > > +	if (funcs && funcs->get_connector_from_writeback)
> > > > > > +		return funcs-
> > > > > >get_connector_from_writeback(wb_connector);
> > > > >
> > > > > The get_connector_from_writeback() and
> > > > > drm_writeback_connector_helper_funcs should be moved to this patch.
> > > >
> > > > Want to keep them separate since they themselves introduce a lot
> > > > of changes on of them has use introducing a new
> > > > writeback_helper_function
> > > structure.
> > >
> > > Let's see how the series will take shape.
> > >
> > > >
> > > >
> > > > >
> > > > > However it feels really awkward to make drm_writeback_connector,
> > > > > which is a wrapper around the drm_connector, to use some
> > > > > external DRM
> > > connector.
> > > > > A quick grepping reveals API (which you missed) that expects
> > > > > drm_writeback_connector.base to be a valid connector. And it
> > > > > would be very hard to catch sunch an API later on.
> > > >
> > > > Also seems like I did miss the fence_get_driver_name one which is
> > > > an easy fix or did you see anything else.
> > > > Really don't see any other problematic areas
> > >
> > > Yes, it was that function. However it is a nice example of how easy
> > > it is to miss a call. Likewise anybody else changing the code might
> > > easily not notice that Intel driver uses drm_writeback_connector in a
> strange way.
> > > >
> > > > >
> > > > > If you want to use DRM framwework, while retaining
> > > > > intel_connector for writeback connectors, I'd suggest following
> > > > > slightly different
> > > > > path: extract common parts of drm_writeback_connector into a
> > > > > common structure, and use it within the DRM core. Then provide
> > > > > functions to fetch drm_connector from that struct or fetch it from
> drm_connector.
> > > >
> > > > Causes a lot of changes in the drm_writeback_connector structure
> > > > causing every other driver Using this to change how they
> > > > essentially call upon drm_writeback_connector. This API was to
> > > > provide more non
> > > invasive way to give everyone another alternative.
> > >
> > > Currently drm_writeback_connector is documented and implemented as
> > > being a wrapper around drm_connector. You are changing that contract
> > > in a non- intuitive way. I think there are several options on how to proceed:
> > >
> > > - Clearly and loudly document that drm_writeback_connector is no longer
> > >   a wrapper around drm_connector. Clearly document how to distinguish
> > >   those two cases. In my opinion this is the worst option as it is
> > >   significantly error-prone
> > >
> >
> > I think this is already done when
> > drm_writeback_connector_init_with_conn is Defined
> 
> No. You also need to update drm_writeback_connector documentation, etc.
> 
> >
> > > - Make sure that the DRM framework can use writeback without
> > >   drm_writeback_connector and them implement all necessary plumbing in
> > >   the Intel driver. This can result in singnificant amount of code
> > >   duplication, so I'd skip this option.
> >
> > Hmm Agreed.
> >
> > >
> > > - Separate writeback parts of drm_writeback_connector into a struct,
> > >   make drm_writeback_connector include drm_connector, new struct and
> > >   most likely drm_encoder. Implement conversion callbacks (like you did
> > >   in your patchset).
> >
> > Again a lot of changes to other drivers which everyone will resist.
> > Something like this was tried previously with both encoder and
> > connector which was not accepted leading the patch series towards
> > creation of the drm_writeback_connector_init_with_encoder.
> >
> > >
> > > - Rework drm_writeback_connector and drm_connector in a similar way,
> but
> > >   use writeback structure as a field inside drm_connector (similar to
> > >   how we got the HDMI data). This saves you from having all conversion
> > >   callbacks and makes it extensible for other drivers too. In fact you
> > >   can use an anonymous union, making sure that HDMI and writeback
> > >   structures take the same space in memory.
> >
> > The idea of not having it inside drm_connector was that it's not a "real
> connector"
> > and we should not be treating it like one which makes me a little
> > doubtful on if the community will go for this.
> 
> Well... It is a "real" connector, otherwise e.g. Intel wouldn't have to wrap it into
> an intel_connector structure. I think this is more of the historical behaviour - to
> wrap the structure instead of adding data to it. HDMI connector showed that
> it's much easier to add data, so I assume it would be a preferred approach.

Sadly the drm_framework does not treat it as such and again as Jani had pointed out this should have used
drm_connector and drm_encoder as pointers within drm_writeback_connector personally I can go ahead with
designing either of the 3 designs available but let us get a consensus here on which design to go for since I had floated
a RFC for this design to get an idea of which design to go for before coming to this design and doing the final work.
Adding some more people here to get the discussion going.

Regards,
Suraj Kandpal

> 
> >
> > >
> > > My preference is shifted towards the last option, it follows current
> > > HDMI subclassing design and it doesn't add unnecessary complexity.
> > >
> > > Yes, this requires reworking of all writeback drivers. Yes, it's a
> > > price of having your own subclass of drm_connector. No, in my
> > > optionion, leaving a semi- broken abstraction is not an option.
> > > Whatever path gets implemented, it should be internally coherent.
> >
> > Well to be honest this has already been done with drm_encoder which is
> > placed Inside drm_writeback_connector with
> > drm_writeback_connector_init_encoder
> > so this is not something very unintuitive. Also I feel messing with
> > all other drivers by changing writeback structure is the more error prone way
> to go about it.
> 
> This is what we frequently have to do: change other drivers and depend on
> developers testing them.
> 
> For the reference, currently only the following drivers implement writeback. I
> think it's a pretty manageable change:
> 
> - AMDGPU
> - ARM/Komeda
> - ARM/Mali
> - MSM/dpu1
> - R-Car
> - VC4
> - VKMS
> 
> Yes, it requires some effort. But I think that it's better than just making
> drm_connector part optional.
> 
> > Also it will be understood that
> > drm_writeback_connector does not contain drm_connector to those using
> > this API as it will be documented. So its not really the semi-broken
> abstraction.
> 
> Thinking in OOP terms, the encoder is just a field in the struct.
> drm_connector is a base class for drm_writeback_connector. By making it
> optional, you are definitely semi-breaking the abstraction.
> 
> 
> 
> --
> With best wishes
> Dmitry