On Tue, Dec 3, 2024 at 10:57 AM Jonas Ådahl <jadahl@xxxxxxxxx> wrote: > > On Wed, Nov 20, 2024 at 07:52:18AM -0500, Zack Rusin wrote: > > 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. > > The linked line just means the property value itself not changing > doesn't result in a full compositor side monitor reconfiguration. Right, that's exactly the point I'm making :) The property isn't used correctly because the full-rescan is expected when that property is present, not if it changed. z
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
