[PATCH v4 01/10] drm/connector: let drivers declare infoframes as unsupported

[Maxime Ripard]

On Tue, Oct 14, 2025 at 07:02:03PM +0300, Dmitry Baryshkov wrote:
> On Tue, Oct 14, 2025 at 02:43:58PM +0200, Maxime Ripard wrote:
> > On Fri, Oct 03, 2025 at 06:41:58PM +0300, Dmitry Baryshkov wrote:
> > > On 03/10/2025 17:23, Maxime Ripard wrote:
> > > > On Thu, Sep 25, 2025 at 05:55:06PM +0300, Dmitry Baryshkov wrote:
> > > > > > > As we will be getting more and more features, some of the InfoFrames
> > > > > > > or data packets will be 'good to have, but not required'.
> > > > > > 
> > > > > > And drivers would be free to ignore those.
> > > > > > 
> > > > > > > > So, no, sorry. That's still a no for me. Please stop sending that patch
> > > > > > > 
> > > > > > > Oops :-)
> > > > > > > 
> > > > > > > > unless we have a discussion about it and you convince me that it's
> > > > > > > > actually something that we'd need.
> > > > > > > 
> > > > > > > My main concern is that the drivers should not opt-out of the features.
> > > > > > > E.g. if we start supporting ISRC packets or MPEG or NTSC VBI InfoFrames
> > > > > > > (yes, stupid examples), it should not be required to go through all the
> > > > > > > drivers, making sure that they disable those. Instead the DRM framework
> > > > > > > should be able to make decisions like:
> > > > > > > 
> > > > > > > - The driver supports SPD and the VSDB defines SPD, enable this
> > > > > > >    InfoFrame (BTW, this needs to be done anyway, we should not be sending
> > > > > > >    SPD if it's not defined in VSDB, if I read it correctly).
> > > > > > > 
> > > > > > > - The driver hints that the pixel data has only 10 meaninful bits of
> > > > > > >    data per component (e.g. out of 12 for DeepColor 36), the Sink has
> > > > > > >    HF-VSDB, send HF-VSIF.
> > > > > > > 
> > > > > > > - The driver has enabled 3D stereo mode, but it doesn't declare support
> > > > > > >    for HF-VSIF. Send only H14b-VSIF.
> > > > > > > 
> > > > > > > Similarly (no, I don't have these on my TODO list, these are just
> > > > > > > examples):
> > > > > > > - The driver defines support for NTSC VBI, register a VBI device.
> > > > > > > 
> > > > > > > - The driver defines support for ISRC packets, register ISRC-related
> > > > > > >    properties.
> > > > > > > 
> > > > > > > - The driver defines support for MPEG Source InfoFrame, provide a way
> > > > > > >    for media players to report frame type and bit rate.
> > > > > > > 
> > > > > > > - The driver provides limited support for Extended HDR DM InfoFrames,
> > > > > > >    select the correct frame type according to driver capabilities.
> > > > > > > 
> > > > > > > Without the 'supported' information we should change atomic_check()
> > > > > > > functions to set infoframe->set to false for all unsupported InfoFrames
> > > > > > > _and_ go through all the drivers again each time we add support for a
> > > > > > > feature (e.g. after adding HF-VSIF support).
> > > > > > 
> > > > > >  From what you described here, I think we share a similar goal and have
> > > > > > somewhat similar concerns (thanks, btw, it wasn't obvious to me before),
> > > > > > we just disagree on the trade-offs and ideal solution :)
> > > > > > 
> > > > > > I agree that we need to sanity check the drivers, and I don't want to go
> > > > > > back to the situation we had before where drivers could just ignore
> > > > > > infoframes and take the easy way out.
> > > > > > 
> > > > > > It should be hard, and easy to catch during review.
> > > > > > 
> > > > > > I don't think bitflag are a solution because, to me, it kind of fails
> > > > > > both.
> > > > > > 
> > > > > > What if, just like the debugfs discussion, we split write_infoframe into
> > > > > > write_avi_infoframe (mandatory), write_spd_infoframe (optional),
> > > > > > write_audio_infoframe (checked by drm_connector_hdmi_audio_init?) and
> > > > > > write_hdr_infoframe (checked in drmm_connector_hdmi_init if max_bpc > 8)
> > > > > > 
> > > > > > How does that sound?
> > > > > 
> > > > > I'd say, I really like the single function to be called for writing the
> > > > > infoframes. It makes it much harder for drivers to misbehave or to skip
> > > > > something.
> > > > 
> > > >  From a driver PoV, I believe we should still have that single function
> > > > indeed. It would be drm_atomic_helper_connector_hdmi_update_infoframes's
> > > > job to fan out and call the multiple callbacks, not the drivers.
> > > 
> > > I like this idea, however it stops at the drm_bridge_connector abstraction.
> > > The only way to handle this I can foresee is to make individual bridges
> > > provide struct drm_connector_hdmi_funcs implementation (which I'm fine with)
> > > and store void *data or struct drm_bridge *hdmi_bridge somewhere inside
> > > struct drm_connector_hdmi in order to let bridge drivers find their data.
> > 
> > Does it change anything? The last HDMI bridge should implement all the
> > infoframes it supports. I don't think we should take care of one bridge
> > with one infoframe type and some other with another?
> 
> Note: I wrote about the _data_. So far the connector's write_infoframe /
> clear_infoframe callbacks get drm_connector as an arg. The fact that
> there is a drm_bridge which implements a callback is hidden well inside
> drm_bridge_connector (and only it knows the bridge_hdmi pointer).
> Otherwise, the bridge, trying to implement drm_connector_hdmi_funcs has
> no way to go from drm_connector to drm_bridge.
> 
> The only possible solution would be to introduce something like
> drm_connector_hdmi::data (either void* or drm_bridge*) and use it
> internally. But for me this looks like a bit loose abstraction. Though,
> if it looks good from your POV, I agree, it would solve enough of
> issues.

I'm not sure I understand, sorry.

What prevents us from adding ~4 functions to bridge->funcs that take the
bridge, and drm_bridge_connector would get the connector, retrieve the
bridge instance from it, and pass it to the bridge actually implementing
it? Like we do currently for write_infoframe and clear_infoframe
already?

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20251121/562de55c/attachment.sig>