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

[Dmitry Baryshkov]

On Sat, Jul 26, 2025 at 04:49:44PM +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Baryshkov <dmitry.baryshkov at oss.qualcomm.com>
> > Sent: Saturday, July 26, 2025 6:04 PM
> > To: Kandpal, Suraj <suraj.kandpal at intel.com>
> > Cc: dri-devel at lists.freedesktop.org; intel-xe at lists.freedesktop.org; intel-
> > gfx at lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>;
> > Murthy, Arun R <arun.r.murthy at intel.com>; Shankar, Uma
> > <uma.shankar at intel.com>
> > Subject: Re: [PATCH 03/28] drm/writeback: Define function to get
> > drm_connector from writeback

Please tune your mail client to insert smaller quotation headers. This
is just useless.

> > 
> > On Fri, Jul 25, 2025 at 10:33:44AM +0530, Suraj Kandpal wrote:
> > > 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

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

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

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

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.

-- 
With best wishes
Dmitry