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