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
