On Tue, Oct 14, 2025 at 06:48:19PM +0300, Dmitry Baryshkov wrote: > On Tue, Oct 14, 2025 at 02:59:43PM +0200, Maxime Ripard wrote: > > On Fri, Oct 03, 2025 at 06:54:47PM +0300, Dmitry Baryshkov wrote: > > > On Fri, Oct 03, 2025 at 03:22:23PM +0200, Maxime Ripard wrote: > > > > On Tue, Sep 30, 2025 at 10:02:28AM +0300, Dmitry Baryshkov wrote: > > > > > On Mon, Sep 29, 2025 at 03:00:04PM +0200, Maxime Ripard wrote: > > > > > > On Thu, Sep 25, 2025 at 05:16:07PM +0300, Dmitry Baryshkov wrote: > > > > > > > On Thu, Sep 25, 2025 at 03:13:47PM +0200, Maxime Ripard wrote: > > > > > > > > On Wed, Sep 10, 2025 at 06:26:56PM +0300, Dmitry Baryshkov > > > > > > > > wrote: > > > > > > > > > On Wed, Sep 10, 2025 at 09:30:19AM +0200, Maxime Ripard wrote: > > > > > > > > > > On Wed, Sep 03, 2025 at 03:03:43AM +0300, Dmitry Baryshkov > > > > > > > > > > wrote: > > > > > > > > > > > On Tue, Sep 02, 2025 at 08:06:54PM +0200, Maxime Ripard > > > > > > > > > > > wrote: > > > > > > > > > > > > On Tue, Sep 02, 2025 at 06:45:44AM +0300, Dmitry > > > > > > > > > > > > Baryshkov wrote: > > > > > > > > > > > > > On Mon, Sep 01, 2025 at 09:07:02AM +0200, Maxime > > > > > > > > > > > > > Ripard wrote: > > > > > > > > > > > > > > On Sun, Aug 31, 2025 at 01:29:13AM +0300, Dmitry > > > > > > > > > > > > > > Baryshkov wrote: > > > > > > > > > > > > > > > On Sat, Aug 30, 2025 at 09:30:01AM +0200, Daniel > > > > > > > > > > > > > > > Stone wrote: > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sat, 30 Aug 2025 at 02:23, Dmitry Baryshkov > > > > > > > > > > > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > > > > It's not uncommon for the particular device > > > > > > > > > > > > > > > > > to support only a subset of > > > > > > > > > > > > > > > > > HDMI InfoFrames. It's not a big problem for > > > > > > > > > > > > > > > > > the kernel, since we adopted > > > > > > > > > > > > > > > > > a model of ignoring the unsupported > > > > > > > > > > > > > > > > > Infoframes, but it's a bigger > > > > > > > > > > > > > > > > > problem for the userspace: we end up having > > > > > > > > > > > > > > > > > files in debugfs which do > > > > > > > > > > > > > > > > > mot match what is being sent on the wire. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Sort that out, making sure that all > > > > > > > > > > > > > > > > > interfaces are consistent. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the series, it's a really good > > > > > > > > > > > > > > > > cleanup. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I know that dw-hdmi-qp can support _any_ > > > > > > > > > > > > > > > > infoframe, by manually > > > > > > > > > > > > > > > > packing it into the two GHDMI banks. So the > > > > > > > > > > > > > > > > supported set there is > > > > > > > > > > > > > > > > 'all of the currently well-known ones, plus any > > > > > > > > > > > > > > > > two others, but only > > > > > > > > > > > > > > > > two and not more'. I wonder if that has any > > > > > > > > > > > > > > > > effect on the interface > > > > > > > > > > > > > > > > you were thinking about for userspace? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I was mostly concerned with the existing debugfs > > > > > > > > > > > > > > > interface (as it is > > > > > > > > > > > > > > > also used e.g. for edid-decode, etc). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It seems "everything + 2 spare" is more or less > > > > > > > > > > > > > > > common (ADV7511, MSM > > > > > > > > > > > > > > > HDMI also have those. I don't have at hand the > > > > > > > > > > > > > > > proper datasheet for > > > > > > > > > > > > > > > LT9611 (non-UXC one), but I think its InfoFrames > > > > > > > > > > > > > > > are also more or less > > > > > > > > > > > > > > > generic). Maybe we should change debugfs > > > > > > > > > > > > > > > integration to register the > > > > > > > > > > > > > > > file when the frame is being enabled and removing > > > > > > > > > > > > > > > it when it gets unset. > > > > > > > > > > > > > > > > > > > > > > > > > > > > But, like, for what benefit? > > > > > > > > > > > > > > > > > > > > > > > > > > > > It's a debugfs interface for userspace to consume. > > > > > > > > > > > > > > The current setup > > > > > > > > > > > > > > works fine with edid-decode already. Why should we > > > > > > > > > > > > > > complicate the design > > > > > > > > > > > > > > that much and create fun races like "I'm running > > > > > > > > > > > > > > edid-decode in parallel > > > > > > > > > > > > > > to a modeset that would remove the file I just > > > > > > > > > > > > > > opened, what is the file > > > > > > > > > > > > > > now?". > > > > > > > > > > > > > > > > > > > > > > > > > > Aren't we trading that with the 'I'm running > > > > > > > > > > > > > edid-decode in paralle with > > > > > > > > > > > > > to a modeset and the file suddenly becomes empty'? > > > > > > > > > > > > > > > > > > > > > > > > In that case, you know what the file is going to be: > > > > > > > > > > > > empty. And you went > > > > > > > > > > > > from a racy, straightforward, design to a racy, > > > > > > > > > > > > complicated, design. > > > > > > > > > > > > > > > > > > > > > > > > It was my question before, but I still don't really see > > > > > > > > > > > > what benefits it > > > > > > > > > > > > would have, and why we need to care about it in the > > > > > > > > > > > > core, when it could > > > > > > > > > > > > be dealt with in the drivers just fine on a case by > > > > > > > > > > > > case basis. > > > > > > > > > > > > > > > > > > > > > > Actually it can not: debugfs files are registered from > > > > > > > > > > > the core, not > > > > > > > > > > > from the drivers. That's why I needed all the > > > > > > > > > > > supported_infoframes > > > > > > > > > > > (which later became software_infoframes). > > > > > > > > > > > > > > > > > > > > That's one thing we can change then. > > > > > > > > > > > > > > > > > > > > > Anyway, I'm fine with having empty files there. > > > > > > > > > > > > > > > > > > > > > > > > > > Then in the long run we can add 'slots' and > > > > > > > > > > > > > > > allocate some of the frames > > > > > > > > > > > > > > > to the slots. E.g. ADV7511 would get 'software > > > > > > > > > > > > > > > AVI', 'software SPD', > > > > > > > > > > > > > > > 'auto AUDIO' + 2 generic slots (and MPEG > > > > > > > > > > > > > > > InfoFrame which can probably be > > > > > > > > > > > > > > > salvaged as another generic one)). MSM HDMI would > > > > > > > > > > > > > > > get 'software AVI', > > > > > > > > > > > > > > > 'software AUDIO' + 2 generic slots (+MPEG + > > > > > > > > > > > > > > > obsucre HDMI which I don't > > > > > > > > > > > > > > > want to use). Then the framework might be able to > > > > > > > > > > > > > > > prioritize whether to > > > > > > > > > > > > > > > use generic slots for important data (as DRM HDR, > > > > > > > > > > > > > > > HDMI) or less important > > > > > > > > > > > > > > > (SPD). > > > > > > > > > > > > > > > > > > > > > > > > > > > > Why is it something for the framework to deal with? > > > > > > > > > > > > > > If you want to have > > > > > > > > > > > > > > extra infoframes in there, just go ahead and create > > > > > > > > > > > > > > additional debugfs > > > > > > > > > > > > > > files in your driver. > > > > > > > > > > > > > > > > > > > > > > > > > > > > If you want to have the slot mechanism, check in > > > > > > > > > > > > > > your atomic_check that > > > > > > > > > > > > > > only $NUM_SLOT at most infoframes are set. > > > > > > > > > > > > > > > > > > > > > > > > > > The driver can only decide that 'we have VSI, SPD and > > > > > > > > > > > > > DRM InfoFrames > > > > > > > > > > > > > which is -ETOOMUCH for 2 generic slots'. The > > > > > > > > > > > > > framework should be able to > > > > > > > > > > > > > decide 'the device has 2 generic slots, we have HDR > > > > > > > > > > > > > data, use VSI and > > > > > > > > > > > > > DRM InfoFrames and disable SPD for now'. > > > > > > > > > > > > > > > > > > > > > > > > I mean... the spec does? The spec says when a > > > > > > > > > > > > particular feature > > > > > > > > > > > > requires to send a particular infoframe. If your device > > > > > > > > > > > > cannot support > > > > > > > > > > > > to have more than two "features" enabled at the same > > > > > > > > > > > > time, so be it. It > > > > > > > > > > > > something that should be checked in that driver > > > > > > > > > > > > atomic_check. > > > > > > > > > > > > > > > > > > > > > > Sounds good to me. Let's have those checks in the drivers > > > > > > > > > > > until we > > > > > > > > > > > actually have seveal drivers performing generic frame > > > > > > > > > > > allocation. > > > > > > > > > > > > > > > > > > > > > > > Or just don't register the SPD debugfs file, ignore it, > > > > > > > > > > > > put a comment > > > > > > > > > > > > there, and we're done too. > > > > > > > > > > > > > > > > > > > > > > It's generic code. > > > > > > > > > > > > > > > > > > > > > > > > But... We are not there yet and I don't have clear > > > > > > > > > > > > > usecase (we support > > > > > > > > > > > > > HDR neither on ADV7511 nor on MSM HDMI, after > > > > > > > > > > > > > carefully reading the > > > > > > > > > > > > > guide I realised that ADV7511 has normal audio > > > > > > > > > > > > > infoframes). Maybe I > > > > > > > > > > > > > should drop all the 'auto' features, simplifying this > > > > > > > > > > > > > series and land > > > > > > > > > > > > > [1] for LT9611UXC as I wanted origianlly. > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > https://lore.kernel.org/dri-devel/[email protected]/ > > > > > > > > > > > > > > > > > > > > > > > > Looking back at that series, I think it still has value > > > > > > > > > > > > to rely on the > > > > > > > > > > > > HDMI infrastructure at the very least for the > > > > > > > > > > > > atomic_check sanitization. > > > > > > > > > > > > > > > > > > > > > > > > But since you wouldn't use the generated infoframes, > > > > > > > > > > > > just skip the > > > > > > > > > > > > debugfs files registration. You're not lying to > > > > > > > > > > > > userspace anymore, and > > > > > > > > > > > > you get the benefits of the HDMI framework. > > > > > > > > > > > > > > > > > > > > > > We create all infoframe files for all HDMI connectors. > > > > > > > > > > > > > > > > > > > > Then we can provide a debugfs_init helper to register all > > > > > > > > > > of them, or > > > > > > > > > > only some of them, and let the drivers figure it out. > > > > > > > > > > > > > > > > > > > > Worst case scenario, debugfs files will not get created, > > > > > > > > > > which is a much > > > > > > > > > > better outcome than having to put boilerplate in every > > > > > > > > > > driver that will > > > > > > > > > > get inconsistent over time. > > > > > > > > > > > > > > > > > > debugfs_init() for each infoframe or taking some kind of > > > > > > > > > bitmask? > > > > > > > > > > > > > > > > I meant turning hdmi_debugfs_add and > > > > > > > > create_hdmi_*_infoframe_file into > > > > > > > > public helpers. That way, drivers that don't care can use the > > > > > > > > (renamed) > > > > > > > > hdmi_debugfs_add, and drivers with different constraints can > > > > > > > > register > > > > > > > > the relevant infoframes directly. > > > > > > > > > > > > > > Doesn't that mean more boilerplate? > > > > > > > > > > > > I don't think it would? In the general case, it wouldn't change > > > > > > anything, and in special cases, then it's probably going to be > > > > > > different > > > > > > from one driver to the next so there's not much we can do. > > > > > > > > > > > > > In the end, LT9611UXC is a special case, for which I'm totally > > > > > > > fine > > > > > > > not to use HDMI helpers at this point: we don't control infoframes > > > > > > > (hopefully that can change), we don't care about the TMDS clock, > > > > > > > no > > > > > > > CEC, etc. > > > > > > > > > > > > Not using the helpers sound pretty reasonable here too. > > > > > > > > > > > > > For all other usecases I'm fine with having atomic_check() unset > > > > > > > all > > > > > > > unsupported infoframes and having empty files in debugfs. Then we > > > > > > > can > > > > > > > evolve over the time, once we see a pattern. We had several > > > > > > > drivers > > > > > > > which had very limited infoframes support, but I think this now > > > > > > > gets > > > > > > > sorted over the time. > > > > > > > > > > > > I never talked about atomic_check()? You were initially concerned > > > > > > that > > > > > > the framework would expose data in debugfs that it's not using. Not > > > > > > registering anything in debugfs solves that, but I'm not sure we > > > > > > need to > > > > > > special case atomic_check. > > > > > > > > > > Well... I ended up with [1], handling infoframes in the atomic_check() > > > > > rather than registering fewer infoframe debugfs files. This way device > > > > > state is consistent, we don't have enabled instances, etc. However it > > > > > results in repetetive code in atomic_check(). > > > > > > > > > > [1] > > > > > https://lore.kernel.org/dri-devel/[email protected]/ > > > > > > > > I guess we can continue the discussion there, but I'm not sure we want > > > > to have more boilerplate in drivers, and especially in the atomic_check > > > > part. If drivers are inconsistent or wrong in the debugfs path, there's > > > > no major issue. If they are wrong in the atomic_check path, it will lead > > > > to regressions, possibly in paths that are pretty hard to test. > > > > > > You've responded there and I can drop the extra handling for HDR DRM and > > > audio infoframes in the atomic_check(). What is your opinion about the > > > atomic_check() unsetting the infoframe->set for SPD and HDMI infoframes? > > > > HDMI infoframes are mandatory, so that's a big no-no. > > Nevertheless... There are drivers (sun4i, inno_hdmi, rk3066, dw_hdmi_qp) > which don't (yet) implement VSI support.
We should really differentiate drivers that didn't because they were allowed to, and drivers that can't. For the bridge you mentioned earlier in the discussion, it makes sense not to expose the debugfs file because we simply don't have access to the actual content. That's fine. For the drivers you listed there, I'm pretty confident that it's because nobody really tried. That's definitely not fine, and we should complain as loudly as possible for that particular case, and not give them a free pass. Checking sun4i, I'm pretty sure it can be implemented. Looking at the incomplete RK3066 TRM, it can be implemented too. And inno_hdmi looks really similar. So it's not really impossible, you just need some hardware and a day's worth of work. There's no reason these should get a pass, it's breaking the spec for no reason. > > For SPD, It's really not clear to me why atomic_check should do that in > > the first place. Your initial concern was about exposing infoframes in > > debugfs that wouldn't be used by the driver. > > > > If the driver doesn't register a debugfs file for SPD, and ignores > > whatever is in the atomic state, what's should we force drivers to do > > that? > > I really don't think that drivers should mess up with debugfs on their > own. Making atomic_check() disable the unsupported InfoFrames makes the > picture perfect: the DRM no longer tries to program them to the > hardware, DebugFS files stay empty, so the whole state becomes > consistent. In the "bridge has no access to infoframes" case, there's really no infoframe. An empty file is "the infoframe can be there but isn't used", not "we don't have access to it and can't report them". Only drivers have those infos. If we do split up write_infoframe into multiple functions though, I guess we could create the debugfs file only if the function pointer is set, which removes drivers' involvement if you don't like that. Maxime
signature.asc
Description: PGP signature
