Hi Abhinav,
On Wed, Mar 02, 2022 at 10:28:03AM -0800, Abhinav Kumar wrote:
> On 2/28/2022 5:42 AM, Laurent Pinchart wrote:
> > On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote:
> >> 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.
> >
> > Thinking some more about this, I wonder a way forward could be to drop
> > the writeback connectors from the connectors list, or at least make them
> > invisible to drivers. The connectors list is used extensively for two
> > different purposes: tracking all drm_connector instances, and tracking
> > all real connectors. The former is mostly needed by the DRM core for
> > bookkeeping purposes and to expose all drm_connector instances to
> > userspace, while the latter is also used by drivers, in many cases in
> > locations that don't expect writeback connectors. Using a drm_connector
> > to implement writeback isn't something we can revisit, but we could
> > avoid exposing that to drivers by considering "real" connectors and
> > writeback connectors two different types of entities in the APIs the DRM
> > core exposes to drivers. What do you think, would it help for i915 ?
>
> Hi Jani and Suraj
>
> Since atleast there is agreement on changing the drm_encoder to a
> pointer in the drm_writeback_connector, can we re-arrange the series OR
> split it into encoder first and then connector so that atleast those
> bits can go in first? It will benefit both our (i915 & MSM )
> implementations.
>
> Hi Laurent
>
> For the connector part, can you please post a RFC for your proposal?
> Perhaps myself and Suraj can evaluate our implementations on top of that
> and the encoder change.
I'm afraid I won't have time to work on this personally for at least
several weeks, if not more.
> >>>>>> 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
