Hi Jani,
On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote:
> On Mon, 28 Feb 2022, Laurent Pinchart wrote:
> > 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@xxxxxxxxx>
> >> > >> >> ---
> >> > >> >> 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.
>
> Laurent, as you accuse us of not doing our homework, I'll point out that
> we've been embedding drm crtc, encoder and connector ever since
> modesetting support was added to i915 in 2008, since before *any* of the
> things you now use as a rationale for asking us to do a massive rewrite
> of the driver existed.
>
> It's been ok to embed those structures for well over ten years. It's a
> common pattern, basically throughout the kernel. Other drivers do it
> too, not just i915. There hasn't been the slightest hint this should not
> be done until this very conversation.
>
> > 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.
>
> I think the onus is on you to first convince everyone that embedding the
> drm core kms structures is an antipattern that all drivers, not just
> i915, should stop using. In OO terms, you're saying they are classes
> that should be final and not extended.
>
> And even then, to be totally honest, refactoring the structures is not
> going to be anywhere near the top of our list of things to do, for a
> very long time.
I may have not expressed myself correctly. There's nothing wrong as such
in embedded those structures in driver-specific structures (a.k.a. C
inheritance). That doesn't need to change (albeit for drm_encoder I
think we should move away from that pattern, but that's an entirely
different issue, and nothing that needs to be addressed soonà.
The issue here is assuming that every drm_connector instance can be
up-casted to an i915-specific structure.
> >> > 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
