On Tue, Aug 22, 2023 at 05:51:39PM +0300, Jani Nikula wrote: > On Tue, 22 Aug 2023, Maxime Ripard <mripard@xxxxxxxxxx> 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
