Re: [PATCH RFC 00/13] drm/connector: Create HDMI Connector infrastructure

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Aug 22, 2023 at 05:51:39PM +0300, Jani Nikula wrote:
> On Tue, 22 Aug 2023, Maxime Ripard <mripard@kernel.org> wrote:
> > Hi,
> >
> > On Tue, Aug 22, 2023 at 04:16:08PM +0200, Daniel Vetter wrote:
> >> On Mon, Aug 14, 2023 at 03:56:12PM +0200, Maxime Ripard wrote:
> >> > Here's a series that creates a subclass of drm_connector specifically
> >> > targeted at HDMI controllers.
> >> > 
> >> > The idea behind this series came from a recent discussion on IRC during
> >> > which we discussed infoframes generation of i915 vs everything else. 
> >> > 
> >> > Infoframes generation code still requires some decent boilerplate, with
> >> > each driver doing some variation of it.
> >> > 
> >> > In parallel, while working on vc4, we ended up converting a lot of i915
> >> > logic (mostly around format / bpc selection, and scrambler setup) to
> >> > apply on top of a driver that relies only on helpers.
> >> > 
> >> > While currently sitting in the vc4 driver, none of that logic actually
> >> > relies on any driver or hardware-specific behaviour.
> >> > 
> >> > The only missing piec to make it shareable are a bunch of extra
> >> > variables stored in a state (current bpc, format, RGB range selection,
> >> > etc.).
> >> > 
> >> > Thus, I decided to create some generic subclass of drm_connector to
> >> > address HDMI connectors, with a bunch of helpers that will take care of
> >> > all the "HDMI Spec" related code. Scrambler setup is missing at the
> >> > moment but can easily be plugged in.
> >> > 
> >> > Last week, Hans Verkuil also expressed interest in retrieving the
> >> > infoframes generated from userspace to create an infoframe-decode tool.
> >> > This series thus leverages the infoframe generation code to expose it
> >> > through debugfs.
> >> > 
> >> > This entire series is only build-tested at the moment. Let me know what
> >> > you think,
> >>
> >> I think the idea overall makes sense, we we probably need it to roll out
> >> actual hdmi support to all the hdmi drivers we have. But there's the
> >> eternal issue of "C sucks at multiple inheritance".
> >> 
> >> Which means if you have a driver that subclasses drm_connector already for
> >> it's driver needs it defacto cannot, or only under some serious pains, use
> >> this.
> >
> > That's what vc4 is doing, and it went fine I think? it was mostly a
> > matter of subclassing drm_hdmi_connector instead of drm_connector, and
> > adjusting the various pointers and accessors here and there.
> >
> > It does create a fairly big diffstat, but nothing too painful.
> 
> The main pain point is not the diffstat per se, but that *all* casts to
> subclass need to check what the connector type is before doing
> so. You'll also get fun NULL conditions that you need to check and
> handle if the type isn't what you'd like it to be.
> 
> Currently i915 can just assume all drm_connectors it encounters are
> intel_connectors that it created, always.
> 
> Basically this has blocked the writeback connector stuff for a few years
> now in i915, because writeback forces a different subclassing, and what
> should be a small change in i915 turns into huge churn.

Yeah after the writeback experience I'm heavily leaning towards "this was
a mistake".

For writeback we could refactor it I think by just moving it all (which I
hope isn't too much churn), and then removing the then empty types (which
is where the big churn kicks in, so maybe just add that to gpu/todo.rst).

Cheers, Sima

> 
> BR,
> Jani.
> 
> 
> >
> >> Which is kinda why in practice we tend to not subclass, but stuff
> >> subclass fields into a name sub-structure. So essentially struct
> >> drm_connector.hdmi and struct drm_connector_state.hdmi instead of
> >> drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to
> >> set it all up would all still be the same roughly. It's less typesafe but
> >> I think the gain in practical use (like you could make i915 use the
> >> helpers probably, which with this approach here is practically
> >> impossible).
> >
> > Ack.
> >
> >> The only other nit is that we probably want to put some of the hdmi
> >> properties into struct drm_mode_config because there's no reason to have
> >> per-connector valid values.
> >
> > What property would you want to move?
> >
> >> Also, it might be really good if you can find a co-conspirator who also
> >> wants to use this in their driver, then with some i915 extracting we'd
> >> have three, which should ensure the helper api is solid.
> >
> > I can convert sunxi (old) HDMI driver if needed. I'm not sure how
> > helpful it would be since it doesn't support bpc > 8, but it could be a
> > nice showcase still for "simple" HDMI controllers.
> >
> > Maxime
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch