[PATCH] drm/vmwgfx: Add Fake EDID

Tue Dec 3 16:42:39 UTC 2024

Hi

Am 03.12.24 um 17:39 schrieb Zack Rusin:
> 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.

Just FTR, I still this patch is re-enforcing wrong behavior in 
compositors instead of fixing them. It's not the driver's job to work 
around compositor issues.

Best regards
Thomas

>
> z

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)