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

[Kandpal, Suraj]

> Please tune your mail client to insert smaller quotation headers. This is just
> useless.
> 
> > >
> > > > 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

> - 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.

> 
> 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. 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.

Regards,
Suraj Kandpal

> With best wishes
> Dmitry