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

[Maxime Ripard]

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.

> 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

Attachment: signature.asc
Description: PGP signature