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
signature.asc
Description: PGP signature
