[PATCH] drm/vmwgfx: Add Fake EDID

Tue Dec 3 18:21:43 UTC 2024

On Tue, Dec 03, 2024 at 11:39:05AM -0500, Zack Rusin wrote:
> On Tue, Dec 3, 2024 at 11:32 AM Jonas Ådahl <jadahl at gmail.com> wrote:
> >
> > On Tue, Dec 03, 2024 at 11:27:52AM -0500, Zack Rusin wrote:
> > > On Tue, Dec 3, 2024 at 10:57 AM Jonas Ådahl <jadahl at gmail.com> 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 at linux.intel.com> wrote:
> > > > > >
> > > > > > On Wed, 20 Nov 2024, Thomas Zimmermann <tzimmermann at suse.de> 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.
> >
> > Well, a full rescan did happen, and the linked code only determines if
> > anything actually did change, including currently advertised modes, that
> > will have any potential effect on the final monitor configuration.
> 
> The point I'm making is that no one is using this property correctly.
> Mutter triggering a full-rescan as a result of other changes doesn't
> change the fact that its usage of that property is broken.
> I think
> you're interpreting my comment that usage of that property is broken
> (or not used at all) everywhere as "Mutter is not refreshing
> correctly" which is not the case. Mutter does resize correctly despite
> the fact that the property check is broken.

Ah, yes, I interpret it as something was broken, but I suppose it is
working as expected.

We're indeed apparently using it incorrectly by reading the property
value. After having seen this thread, I went and checked the commit
history, and it seems the implementation has flip flopped between
reading the property, and checking its existence, for more than a
decade. I suspect a reason for this might have been that the property
itself doesn't seem to be documented anywhere. Or is it? I have not yet
found that it is.

The following would probably make the implementation conform to the
expectations of the property:

```

diff --git a/src/backends/native/meta-kms-connector.c b/src/backends/native/meta-kms-connector.c
index cc6cd89f56..a4b9deb85e 100644
--- a/src/backends/native/meta-kms-connector.c
+++ b/src/backends/native/meta-kms-connector.c
@@ -369,7 +369,7 @@ state_set_properties (MetaKmsConnectorState *state,

   prop = &props[META_KMS_CONNECTOR_PROP_HOTPLUG_MODE_UPDATE];
   if (prop->prop_id)
-    state->hotplug_mode_update = prop->value;
+    state->hotplug_mode_update = TRUE;

   prop = &props[META_KMS_CONNECTOR_PROP_SCALING_MODE];
   if (prop->prop_id)
```

In mutter, it has primarily been interpreted as a hint when implementing
policy to decide when to avoid look up monitor configurations from
persistent storage.


Jonas


> 
> z


