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.
--
With best wishes
Dmitry
