Re: [PATCH] drm/vmwgfx: Add Fake EDID

["Jonas Ådahl" <jadahl@gmail.com>]

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@linux.intel.com> wrote:
> >
> > On Wed, 20 Nov 2024, Thomas Zimmermann <tzimmermann@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. A few
lines below it also checks whether the advertised modes changed in any
way, and if they did, trigger a monitor reconfiguration, so it's not
clear what is not working, since it should have done a full mode rescan,
and reconfigured everything if anything changed.


Jonas

> 
> 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