On Fri, Nov 21, 2025 at 04:48:02PM +0100, Maxime Ripard wrote:

On Tue, Oct 14, 2025 at 07:02:03PM +0300, Dmitry Baryshkov wrote:

quoted

On Tue, Oct 14, 2025 at 02:43:58PM +0200, Maxime Ripard wrote:

quoted

On Fri, Oct 03, 2025 at 06:41:58PM +0300, Dmitry Baryshkov wrote:

quoted

On 03/10/2025 17:23, Maxime Ripard wrote:

quoted

On Thu, Sep 25, 2025 at 05:55:06PM +0300, Dmitry Baryshkov wrote:

quoted

quoted

quoted

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.

quoted

quoted

So, no, sorry. That's still a no for me. Please stop sending that patch

Oops :-)

quoted

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?

Well, we discussed that having the write_foo_infoframe in the
drm_connector_hdmi_funcs means that the connector supports that
infoframe (and it can be used to e.g. report errors). However with
drm_bridge_container, we need to set all callbacks in
drm_connector_hdmi_funcs, even if the underlying bridge reports them as
unsupported.

Am I missing something?

-- 
With best wishes
Dmitry