Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Rob,

On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote:
> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote:
> > On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> > >> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> > >> >> Changing rcar_du driver to accomadate the change of
> > >> >> drm_writeback_connector.base and drm_writeback_connector.encoder
> > >> >> to a pointer the reason for which is explained in the
> > >> >> Patch(drm: add writeback pointers to drm_connector).
> > >> >>
> > >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > >> >> ---
> > >> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> > >> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> > >> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> > >> >>
> > >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> >> index 66e8839db708..68f387a04502 100644
> > >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> > >> >>   const char *const *sources;
> > >> >>   unsigned int sources_count;
> > >> >>
> > >> >> + struct drm_connector connector;
> > >> >> + struct drm_encoder encoder;
> > >> >
> > >> > Those fields are, at best, poorly named. Furthermore, there's no need in
> > >> > this driver or in other drivers using drm_writeback_connector to create
> > >> > an encoder or connector manually. Let's not polute all drivers because
> > >> > i915 doesn't have its abstractions right.
> > >>
> > >> i915 uses the quite common model for struct inheritance:
> > >>
> > >>      struct intel_connector {
> > >>              struct drm_connector base;
> > >>              /* ... */
> > >>      }
> > >>
> > >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> > >> radeon, tilcdc, and vboxvideo.
> > >>
> > >> We could argue about the relative merits of that abstraction, but I
> > >> think the bottom line is that it's popular and the drivers using it are
> > >> not going to be persuaded to move away from it.
> > >
> > > Nobody said inheritance is bad.
> > >
> > >> It's no coincidence that the drivers who've implemented writeback so far
> > >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> > >> because the drm_writeback_connector midlayer does, forcing the issue.
> > >
> > > Are you sure it's not a coincidence ? :-)
> > >
> > > The encoder and especially connector created by drm_writeback_connector
> > > are there only because KMS requires a drm_encoder and a drm_connector to
> > > be exposed to userspace (and I could argue that using a connector for
> > > writeback is a hack, but that won't change). The connector is "virtual",
> > > I still fail to see why i915 or any other driver would need to wrap it
> > > into something else. The whole point of the drm_writeback_connector
> > > abstraction is that drivers do not have to manage the writeback
> > > drm_connector manually, they shouldn't touch it at all.
> >
> > The thing is, drm_writeback_connector_init() calling
> > drm_connector_init() on the drm_connector embedded in
> > drm_writeback_connector leads to that connector being added to the
> > drm_device's list of connectors. Ditto for the encoder.
> >
> > All the driver code that handles drm_connectors would need to take into
> > account they might not be embedded in intel_connector. Throughout the
> > driver. Ditto for the encoders.
> 
> The assumption that a connector is embedded in intel_connector doesn't
> really play that well with how bridge and panel connectors work.. so
> in general this seems like a good thing to unwind.
> 
> But as a point of practicality, i915 is a large driver covering a lot
> of generations of hw with a lot of users.  So I can understand
> changing this design isn't something that can happen quickly or
> easily.  IMO we should allow i915 to create it's own connector for
> writeback, and just document clearly that this isn't the approach new
> drivers should take.  I mean, I understand idealism, but sometimes a
> dose of pragmatism is needed. :-)

i915 is big, but so is Intel. It's not fair to treat everybody else as a
second class citizen and let Intel get away without doing its homework.
I want to see this refactoring effort moving forward in i915 (and moving
to drm_bridge would then be a good idea too). If writeback support in
i915 urgent, then we can discuss *temporary* pragmatic stopgap measures,
but not without a real effort to fix the core issue.

> > The point is, you can't initialize a connector or an encoder for a
> > drm_device in isolation of the rest of the driver, even if it were
> > supposed to be hidden away.

-- 
Regards,

Laurent Pinchart