On Wed, Nov 20, 2024 at 5:22 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > > On Wed, 20 Nov 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: > > Hi > > > > > > Am 19.11.24 um 20:40 schrieb Ian Forbes: > >> Most compositors are using a change in EDID as an indicator to > >> refresh their connector information on hotplug regardless of whether the > >> connector was previously connected. Originally the hotplug_mode_update > >> property was supposed to provide a hint to userspace to always refresh > >> connector info on hotplug as virtual devices such as vmwgfx and QXL > >> changed the connector without disconnecting it first. This was done to > >> implement Autofit. Unfortunately hotplug_mode_update was not widely > >> adopted and compositors used other heuristics to determine whether to > >> refresh the connector info. > >> > >> Currently a change in EDID is the one heuristic that seems to be universal. > >> No compositors currently implement hotplug_mode_update correctly or at all. > >> By implementing a fake EDID blob we can ensure that our EDID changes on > >> hotplug and therefore userspace will refresh the connector info so that > >> Autofit will work. This is the approach that virtio takes. > >> > >> This also removes the need to add hotplug_mode_update support for all > >> compositors as traditionally this niche feature has fallen on > >> virtualized driver developers to implement. > > > > Why don't you fix the compositors instead? > > > > I feel like NAK'ing this patch. The code itself is not so much a > > problem, but the commit message. > > Oh, I think the code is problematic too. > > Please avoid all struct edid based interfaces, in this case > drm_connector_update_edid_property(). They will be removed in the > future, and adding more is counter-productive. Everything should be > struct drm_edid based going forward. > > Of course, actually grafting the EDID needs struct edid. And that's kind > of annoying too. Do we really want to spread the EDID details all over > the place? This one combines drm_edid.h structs and magic numbers in a > jumble. I'm kind of hoping we'd get rid of driver usage of struct edid, > though that's a long road. But we've made a lot of progress towards it, > there aren't that many places left that directly look at the guts of > EDID, and most of it is centralized in drm_edid.c. > > Of course, not using the standard drm_edid_read* interfaces also lacks > on features such as providing the EDID via the firmware loader or > debugfs, which can be handy for testing and debugging, but that's a > minor issue. > > > Maybe it resolves problems with > > compositors, but it is a step backwards for the overall ecosystem. If > > the connector changes, your driver should increment the epoch counter. > > [1] That will send a hotplug event to userspace. The EDID alone does not > > say anything about connector status. > > Yeah, unplugging and replugging the same display with the same EDID > isn't a problem for other drivers, and they don't have to do this kind > of stuff to trick userspace. Maybe vmwgfx should handle (or simulate) > hotplugs better? I don't think that's what Ian is trying to fix. There's two different issues: 1) The code using struct edid which is frowned upon. 2) The virtualized drivers not behaving like real GPU's and thus breaking userspace. vmwgfx and qxl do not provide edid at all. It's null. But every time someone resizes a host side window in which the para-virtualized driver is displaying, the preferred mode changes. Userspace kept checking whether the edid changes on each hotplug event to figure out if it got new modes and refresh if it noticed that edid changed. Because on qxl and vmwgfx the edid never changes (it's always null) Dave added hotplug_mode_update property which only qxl and vmwgfx send and its presence indicates that the userspace should refresh modes even if edid didn't change. Because that property is only used by qxl and vmwgfx everyone gets it wrong. The property was specifically added to fix gnome and Ian noticed that currently even gnome is broken: https://gitlab.gnome.org/GNOME/mutter/-/blob/main/src/backends/native/meta-kms-connector.c#L940 hotplug_mode_update doesn't change, it's just a flag that indicates that userspace needs a full mode rescan. virtiogpu deals with it by providing a fake edid hostside and not using hotplug_mode_update. So there are two different arguments to be made with this patch: 1) "You should provide the fake edid hostside like virtiogpu". But that means that we'd still be using the broken hotplug_mode_update on everything that's been released. 2) "You should fix all of userspace". Which is not realistic because vast majority of people are not running on qxl or vmwgfx so basically everyone either doesn't support hotplug_mode_update at all (e.g. kwin, window maker, weston) or they handle it incorrectly (e.g. mutter). It's not terribly realistic to be monitoring every compositor out there for breaking changes. I don't love the code and I'm not excited about putting this in the driver, but also I don't see a better way of fixing the core issue (which is that some virtualized drivers just do not behave like real gpu's). z
