On Fri, Aug 01, 2025 at 05:18:47AM +0000, Kandpal, Suraj wrote:
> > 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.kand...@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.
>
> >
> > 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
