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

["Kandpal, Suraj" <suraj.kandpal@intel.com>]

> Subject: Re: [RFC PATCH 1/8] drm: writeback: Refactor
> drm_writeback_connector structure
> 
> Hi,
> 
> On Sat, Aug 16, 2025 at 01:20:53AM +0300, Dmitry Baryshkov wrote:
> > On Thu, Aug 14, 2025 at 05:13:54PM +0100, liviu.dudau@arm.com wrote:
> > > Hi,
> > >
> > > On Wed, Aug 13, 2025 at 10:04:22AM +0000, Kandpal, Suraj wrote:
> > > > > > > };
> > > > > >
> > > > > > 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.
> > > > >
> > > > > >
> > > >
> > > > Seems like this thread has died. We need to get a conclusion on the
> design.
> > > > Laurent do you have any issue with the design given Dmitry's
> > > > explanation as to why this Design is good for drm_writeback_connector.
> > >
> > > I'm with Laurent here. The idea for drm_connector (and a lot of drm
> > > structures) are to be used as base "classes" for extended
> > > structures. I don't know why HDMI connector ended up inside
> > > drm_connector as not all connectors have HDMI functionality, but that's a
> cleanup for another day.
> >
> > Maybe Maxime can better comment on it, but I think it was made exactly
> > for the purpose of not limiting the driver's design. For example, a
> > lot of drivers subclass drm_connector via drm_bridge_connector. If
> > struct drm_connector_hdmi was a wrapper around struct drm_connector,
> > then it would have been impossible to use HDMI helpers for bridge
> > drivers, while current design freely allows any driver to utilize
> > corresponding library code.
> 
> That's exactly why we ended up like this. With that design, we wouldn't have
> been able to "inherit" two connector "classes": bridge_connector is one,
> intel_connector another one.
> 
> See here for the rationale:
> https://lore.kernel.org/dri-devel/ZOTDKHxn2bOg+Xmg@phenom.ffwll.local/
> 
> I don't think the "but we'll bloat drm_connector" makes sense either.
> There's already a *lot* of things that aren't useful to every connector (fwnode,
> display_info, edid in general, scaling, vrr, etc.)
> 
> And it's not like we allocate more than a handful of them during a system's life.

So Are we okay with the approach mentioned here with the changes that have been proposed here like
Having drm_writeback_connector in union with drm_hdmi_connector
Also one more thing I would like to clarify here is how everyone would like the patches
patches where each patch changes both the drm core and all related drivers (ensures buildability but then review is tough for each driver).
Or patches where we have initial drm core changes and then each patch does the all changes in a driver in its own respective patch.

Regards,
Suraj Kandpal

> 
> Maxime