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: > > 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?
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
