Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

On Mon, Aug 11, 2025 at 02:15:46PM +0300, Laurent Pinchart wrote:
> On Mon, Aug 11, 2025 at 01:22:30PM +0300, Dmitry Baryshkov wrote:
> > On Mon, Aug 11, 2025 at 12:44:29PM +0300, Laurent Pinchart wrote:
> > > On Mon, Aug 11, 2025 at 02:57:00PM +0530, Suraj Kandpal wrote:
> > > > Some drivers cannot work with the current design where the connector
> > > > is embedded within the drm_writeback_connector such as intel and
> > > > some drivers that can get it working end up adding a lot of checks
> > > > all around the code to check if it's a writeback conenctor or not.
> > > > To solve this we move the drm_writeback_connector within the
> > > > drm_connector and remove the drm_connector base which was in
> > > > drm_writeback_connector. We do all other required
> > > > modifications that come with these changes along with addition
> > > > of new function which returns the drm_connector when
> > > > drm_writeback_connector is present.
> > > > All drivers will be expected to allocate the drm_connector.
> > > > 
> > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_writeback.c | 33 ++++++++++------
> > > >  include/drm/drm_connector.h     | 60 +++++++++++++++++++++++++++++
> > > >  include/drm/drm_writeback.h     | 68 ++++-----------------------------
> > > >  3 files changed, 89 insertions(+), 72 deletions(-)
> > > > 
> > > > @@ -2305,6 +2360,11 @@ struct drm_connector {
> > > >  	 * @cec: CEC-related data.
> > > >  	 */
> > > >  	struct drm_connector_cec cec;
> > > > +
> > > > +	/**
> > > > +	 * @writeback: Writeback related valriables.
> > > > +	 */
> > > > +	struct drm_writeback_connector writeback;
> > > 
> > > No, sorry, that's a bad idea. Most connectors have nothing to do with
> > > writeback, you shouldn't introduce writeback-specific fields here.
> > > drm_writeback_connector happens to be a drm_connector because of
> > > historical reasons (it was decided to reuse the connector API exposed to
> > > userspace instead of exposing a completely separate API in order to
> > > simplify the implementation), but that does not mean that every
> > > connector is related to writeback.
> > > 
> > > I don't know what issues the Intel driver(s) have with
> > > drm_writeback_connector, but you shouldn't make things worse for
> > > everybody due to a driver problem.
> > 
> > Suraj is trying to solve a problem that in Intel code every drm_connector
> > must be an intel_connector too. His previous attempt resulted in a loose
> > abstraction where drm_writeback_connector.base wasn't initialized in
> > some cases (which is a bad idea IMO).
> > 
> > I know the historical reasons for drm_writeback_connector, but I think
> > we can do better now.
> > 
> > So, I think, a proper approach would be:
> > 
> > struct drm_connector {
> >     // other fields
> > 
> >     union {
> >         struct drm_connector_hdmi hdmi; // we already have it
> >         struct drm_connector_wb wb;  // this is new
> >     };
> > 
> >     // rest of the fields.
> > };
> 
> I still don't like that. This really doesn't belong here. If anything,
> the drm_connector for writeback belongs to drm_crtc.

Why? We already have generic HDMI field inside drm_connector. I am
really hoping to be able to land DP parts next to it. In theory we can
have a DVI-specific entry there (e.g. with the subconnector type).
The idea is not to limit how the drivers subclass those structures.

I don't see a good case why WB should deviate from that design.

> If the issue is that some drivers need a custom drm_connector subclass,
> then I'd rather turn the connector field of drm_writeback_connector into
> a pointer.

Having a pointer requires additional ops in order to get drm_connector
from WB code and vice versa. Having drm_connector_wb inside
drm_connector saves us from those ops (which don't manifest for any
other kind of structure). Nor will it take any more space since union
will reuse space already taken up by HDMI part.

> 
> > I plan to add drm_connector_dp in a similar way, covering DP needs
> > (currently WIP).

-- 
With best wishes
Dmitry