Hi
Am 03.12.24 um 17:39 schrieb Zack Rusin:
On Tue, Dec 3, 2024 at 11:32 AM Jonas Ådahl <jadahl@xxxxxxxxx> 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@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.
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)
