[PATCH] drm/vmwgfx: Add Fake EDID

[PATCH] drm/vmwgfx: Add Fake EDID

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

Signed-off-by: Ian Forbes <ian.forbes@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 172 ++++++++++++++++++++++++++-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |   3 +
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |   2 +
 3 files changed, 176 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 5a1192496d49..9d9d0b8539dd 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -37,7 +37,6 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_rect.h>
 #include <drm/drm_sysfs.h>
-#include <drm/drm_edid.h>
 
 void vmw_du_init(struct vmw_display_unit *du)
 {
@@ -2662,6 +2661,175 @@ enum drm_mode_status vmw_connector_mode_valid(struct drm_connector *connector,
 	return MODE_OK;
 }
 
+/*
+ * Average pixels per millimeter and centimeter for a circa 2020 display
+ */
+#define VMW_FAKE_PPMM 4
+#define VMW_FAKE_PPCM 40
+
+static void vmw_mode_to_timing(const struct drm_display_mode *mode,
+			       struct detailed_timing *timing)
+{
+	struct detailed_pixel_timing *dpt = &timing->data.pixel_data;
+
+	const u8 hblank = (mode->htotal - mode->hdisplay);
+	const u8 hfront = (mode->hsync_start - mode->hdisplay);
+	const u8 hsync  = (mode->hsync_end - mode->hsync_start);
+
+	const u8 vblank = (mode->vtotal - mode->vdisplay);
+	const u8 vfront = (mode->vsync_start - mode->vdisplay);
+	const u8 vsync  = (mode->vsync_end - mode->vsync_start);
+
+	const unsigned int wmm = mode->hdisplay / VMW_FAKE_PPMM;
+	const unsigned int hmm = mode->vdisplay / VMW_FAKE_PPMM;
+
+	timing->pixel_clock = mode->clock / 10;
+	memset(dpt, 0, sizeof(*dpt));
+
+	// horizontal
+	dpt->hactive_lo = mode->hdisplay & 0xFF;
+	dpt->hblank_lo = hblank & 0xFF;
+
+	dpt->hactive_hblank_hi |= (mode->hdisplay >> 4) & 0xF0;
+	dpt->hactive_hblank_hi |= (hblank >> 8) & 0x0F;
+
+	dpt->hsync_offset_lo      = hfront & 0xFF;
+	dpt->hsync_pulse_width_lo =  hsync & 0xFF;
+
+	dpt->hsync_vsync_offset_pulse_width_hi |= (hfront >> 2) & 0xC0;
+	dpt->hsync_vsync_offset_pulse_width_hi |= (hsync  >> 4) & 0x30;
+
+	// vertical
+	dpt->vactive_lo = mode->vdisplay & 0xFF;
+	dpt->vactive_vblank_hi |= (mode->vdisplay >> 4) & 0xF0;
+
+	dpt->vblank_lo = vblank & 0xFF;
+	dpt->vactive_vblank_hi |= (vblank >> 8) & 0x0F;
+
+	dpt->vsync_offset_pulse_width_lo |= (vfront & 0x0F) << 4;
+	dpt->vsync_offset_pulse_width_lo |= (vsync  & 0x0F) << 0;
+
+	dpt->hsync_vsync_offset_pulse_width_hi |= (vfront >> 4) & 0x0C;
+	dpt->hsync_vsync_offset_pulse_width_hi |= (vsync  >> 8) & 0x03;
+
+	// physical sizes
+	dpt->width_mm_lo  = wmm & 0xFF;
+	dpt->height_mm_lo = hmm & 0xFF;
+	dpt->width_height_mm_hi |= (wmm >> 4) & 0xF0;
+	dpt->width_height_mm_hi |= (hmm >> 8) & 0x0F;
+
+	dpt->hborder = 0;
+	dpt->vborder = 0;
+	dpt->misc |= 0x18;
+	dpt->misc |= (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0x2 : 0;
+	dpt->misc |= (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0x4 : 0;
+}
+
+/* Our encoded Plug & Play ID
+ * See: https://uefi.org/PNP_ACPI_Registry and https://uefi.org/PNP_ID_List
+ */
+static inline __be16 vmw_pnp_id(void)
+{
+	return cpu_to_be16((('V' - '@') << 10) |
+			   (('M' - '@') <<  5) |
+			   (('W' - '@') <<  0));
+}
+
+/*
+ * Fills in the fake EDID using the preferred mode.
+ * See 'Vesa Enhanced EDID Standard Release A Revision 2' and
+ * 'VESA DMT Standard Version 1.0 Revision 13'.
+ */
+static void vmw_fill_fake_edid(const struct drm_display_mode *pref_mode,
+			       unsigned int unit, struct edid *e)
+{
+	int checksum = 0;
+
+	/*
+	 * Bump the manufacture week ever time we fill the edid so that
+	 * it will change on every hotplug. This way even if the screen
+	 * size does not change userspace will rescan the connector. A
+	 * hotplug with no changes in resolution is likely a multi-mon change
+	 * and the suggested_X/Y will have changed and we want userspace to
+	 * pick up this. Use this field to only fill the constant data once
+	 * by checking for zero.
+	 */
+	if (e->mfg_week++ == 0) {
+		memset(e->header + 1, 0xFF, 6);
+
+		e->product_id.manufacturer_name = vmw_pnp_id();
+		e->mfg_year = 32; // 2022
+
+		e->prod_code[0] = 'V';
+		e->prod_code[1] = 'M';
+		e->serial = 0xABC0 | unit;
+
+		e->version = 1;
+		e->revision = 4;
+
+		e->input = 0xA0;
+		e->gamma = 120; // 2.20
+		e->features = 0x26;
+
+		// Standard sRGB color space
+		e->red_green_lo = 0xEE;
+		e->blue_white_lo = 0x91;
+		e->red_x = 0xA3;
+		e->red_y = 0x54;
+		e->green_x = 0x4C;
+		e->green_y = 0x99;
+		e->blue_x  = 0x26;
+		e->blue_y  = 0x0F;
+		e->white_x = 0x50;
+		e->white_y = 0x54;
+
+		e->established_timings.t1 = 0x21;
+		e->established_timings.t2 = 0x08;
+
+		e->standard_timings[0].hsize = 0x81;
+		e->standard_timings[0].vfreq_aspect =  0xC0; // 720p60
+
+		e->standard_timings[1].hsize = 0xD1;
+		e->standard_timings[1].vfreq_aspect =  0xC0; // 1080p60
+
+		e->standard_timings[2].hsize = 0x81;
+		e->standard_timings[2].vfreq_aspect =  0x80; // 1280x1024@60
+
+		e->standard_timings[3].hsize = 0xD1;
+		e->standard_timings[3].vfreq_aspect =  0x40; // 1920x1440@60
+
+		e->standard_timings[4].hsize = 0xE1;
+		e->standard_timings[4].vfreq_aspect =  0xC0; // 2048x1152@60
+
+		e->standard_timings[5].hsize = 0xA9;
+		e->standard_timings[5].vfreq_aspect =  0x40; // 1600x1200@60
+
+		e->standard_timings[6].hsize = 0xB3;
+		e->standard_timings[6].vfreq_aspect =  0x00; // 2048x1152@60
+
+		e->standard_timings[7].hsize = 0x95;
+		e->standard_timings[7].vfreq_aspect =  0x00; // 1440x900@60
+
+		e->detailed_timings[1].data.other_data.type = EDID_DETAIL_MONITOR_NAME;
+		memcpy(e->detailed_timings[1].data.other_data.data.str.str,
+		       "VMware Screen", 13);
+
+		e->detailed_timings[2].data.other_data.type = 0x10;
+		e->detailed_timings[3].data.other_data.type = 0x10;
+
+		e->extensions = 0;
+	}
+	e->width_cm = DIV_ROUND_CLOSEST(pref_mode->hdisplay, VMW_FAKE_PPCM);
+	e->height_cm = DIV_ROUND_CLOSEST(pref_mode->vdisplay, VMW_FAKE_PPCM);
+	// This is the preferred/native mode
+	vmw_mode_to_timing(pref_mode, &e->detailed_timings[0]);
+
+	for (int i = 0; i < sizeof(struct edid) - 1; i++)
+		checksum += ((u8 *)e)[i];
+
+	e->checksum = 0x100 - checksum;
+}
+
 /*
  * Common modes not present in the VESA DMT standard or assigned a VIC.
  */
@@ -2699,6 +2867,8 @@ int vmw_connector_get_modes(struct drm_connector *connector)
 	drm_mode_probed_add(connector, mode);
 	num_modes++;
 	drm_dbg_kms(dev, "preferred mode " DRM_MODE_FMT "\n", DRM_MODE_ARG(mode));
+	vmw_fill_fake_edid(mode, du->unit, &du->fake_edid);
+	drm_connector_update_edid_property(connector, &du->fake_edid);
 
 	/* Probe connector for all modes not exceeding our geom limits */
 	max_width  = dev_priv->texture_max_width;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 6a8bb60c507d..56c739d78582 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -32,6 +32,7 @@
 #include <drm/drm_encoder.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_edid.h>
 
 #include "vmwgfx_drv.h"
 
@@ -363,6 +364,8 @@ struct vmw_display_unit {
 	unsigned pref_height;
 	bool pref_active;
 
+	struct edid fake_edid;
+
 	/*
 	 * Gui positioning
 	 */
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 82d18b88f4a7..3defdf00a975 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -1631,6 +1631,8 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.suggested_y_property, 0);
 
+	drm_connector_attach_edid_property(connector);
+
 	vmw_du_init(&stdu->base);
 
 	return 0;
-- 
2.34.1

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

[Thomas Zimmermann]

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

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.11.8/source/include/drm/drm_connector.h#L1994



>
> Signed-off-by: Ian Forbes <ian.forbes@broadcom.com>
> ---
>   drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 172 ++++++++++++++++++++++++++-
>   drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |   3 +
>   drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |   2 +
>   3 files changed, 176 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 5a1192496d49..9d9d0b8539dd 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -37,7 +37,6 @@
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_rect.h>
>   #include <drm/drm_sysfs.h>
> -#include <drm/drm_edid.h>
>   
>   void vmw_du_init(struct vmw_display_unit *du)
>   {
> @@ -2662,6 +2661,175 @@ enum drm_mode_status vmw_connector_mode_valid(struct drm_connector *connector,
>   	return MODE_OK;
>   }
>   
> +/*
> + * Average pixels per millimeter and centimeter for a circa 2020 display
> + */
> +#define VMW_FAKE_PPMM 4
> +#define VMW_FAKE_PPCM 40
> +
> +static void vmw_mode_to_timing(const struct drm_display_mode *mode,
> +			       struct detailed_timing *timing)
> +{
> +	struct detailed_pixel_timing *dpt = &timing->data.pixel_data;
> +
> +	const u8 hblank = (mode->htotal - mode->hdisplay);
> +	const u8 hfront = (mode->hsync_start - mode->hdisplay);
> +	const u8 hsync  = (mode->hsync_end - mode->hsync_start);
> +
> +	const u8 vblank = (mode->vtotal - mode->vdisplay);
> +	const u8 vfront = (mode->vsync_start - mode->vdisplay);
> +	const u8 vsync  = (mode->vsync_end - mode->vsync_start);
> +
> +	const unsigned int wmm = mode->hdisplay / VMW_FAKE_PPMM;
> +	const unsigned int hmm = mode->vdisplay / VMW_FAKE_PPMM;
> +
> +	timing->pixel_clock = mode->clock / 10;
> +	memset(dpt, 0, sizeof(*dpt));
> +
> +	// horizontal
> +	dpt->hactive_lo = mode->hdisplay & 0xFF;
> +	dpt->hblank_lo = hblank & 0xFF;
> +
> +	dpt->hactive_hblank_hi |= (mode->hdisplay >> 4) & 0xF0;
> +	dpt->hactive_hblank_hi |= (hblank >> 8) & 0x0F;
> +
> +	dpt->hsync_offset_lo      = hfront & 0xFF;
> +	dpt->hsync_pulse_width_lo =  hsync & 0xFF;
> +
> +	dpt->hsync_vsync_offset_pulse_width_hi |= (hfront >> 2) & 0xC0;
> +	dpt->hsync_vsync_offset_pulse_width_hi |= (hsync  >> 4) & 0x30;
> +
> +	// vertical
> +	dpt->vactive_lo = mode->vdisplay & 0xFF;
> +	dpt->vactive_vblank_hi |= (mode->vdisplay >> 4) & 0xF0;
> +
> +	dpt->vblank_lo = vblank & 0xFF;
> +	dpt->vactive_vblank_hi |= (vblank >> 8) & 0x0F;
> +
> +	dpt->vsync_offset_pulse_width_lo |= (vfront & 0x0F) << 4;
> +	dpt->vsync_offset_pulse_width_lo |= (vsync  & 0x0F) << 0;
> +
> +	dpt->hsync_vsync_offset_pulse_width_hi |= (vfront >> 4) & 0x0C;
> +	dpt->hsync_vsync_offset_pulse_width_hi |= (vsync  >> 8) & 0x03;
> +
> +	// physical sizes
> +	dpt->width_mm_lo  = wmm & 0xFF;
> +	dpt->height_mm_lo = hmm & 0xFF;
> +	dpt->width_height_mm_hi |= (wmm >> 4) & 0xF0;
> +	dpt->width_height_mm_hi |= (hmm >> 8) & 0x0F;
> +
> +	dpt->hborder = 0;
> +	dpt->vborder = 0;
> +	dpt->misc |= 0x18;
> +	dpt->misc |= (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0x2 : 0;
> +	dpt->misc |= (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0x4 : 0;
> +}
> +
> +/* Our encoded Plug & Play ID
> + * See: https://uefi.org/PNP_ACPI_Registry and https://uefi.org/PNP_ID_List
> + */
> +static inline __be16 vmw_pnp_id(void)
> +{
> +	return cpu_to_be16((('V' - '@') << 10) |
> +			   (('M' - '@') <<  5) |
> +			   (('W' - '@') <<  0));
> +}
> +
> +/*
> + * Fills in the fake EDID using the preferred mode.
> + * See 'Vesa Enhanced EDID Standard Release A Revision 2' and
> + * 'VESA DMT Standard Version 1.0 Revision 13'.
> + */
> +static void vmw_fill_fake_edid(const struct drm_display_mode *pref_mode,
> +			       unsigned int unit, struct edid *e)
> +{
> +	int checksum = 0;
> +
> +	/*
> +	 * Bump the manufacture week ever time we fill the edid so that
> +	 * it will change on every hotplug. This way even if the screen
> +	 * size does not change userspace will rescan the connector. A
> +	 * hotplug with no changes in resolution is likely a multi-mon change
> +	 * and the suggested_X/Y will have changed and we want userspace to
> +	 * pick up this. Use this field to only fill the constant data once
> +	 * by checking for zero.
> +	 */
> +	if (e->mfg_week++ == 0) {
> +		memset(e->header + 1, 0xFF, 6);
> +
> +		e->product_id.manufacturer_name = vmw_pnp_id();
> +		e->mfg_year = 32; // 2022
> +
> +		e->prod_code[0] = 'V';
> +		e->prod_code[1] = 'M';
> +		e->serial = 0xABC0 | unit;
> +
> +		e->version = 1;
> +		e->revision = 4;
> +
> +		e->input = 0xA0;
> +		e->gamma = 120; // 2.20
> +		e->features = 0x26;
> +
> +		// Standard sRGB color space
> +		e->red_green_lo = 0xEE;
> +		e->blue_white_lo = 0x91;
> +		e->red_x = 0xA3;
> +		e->red_y = 0x54;
> +		e->green_x = 0x4C;
> +		e->green_y = 0x99;
> +		e->blue_x  = 0x26;
> +		e->blue_y  = 0x0F;
> +		e->white_x = 0x50;
> +		e->white_y = 0x54;
> +
> +		e->established_timings.t1 = 0x21;
> +		e->established_timings.t2 = 0x08;
> +
> +		e->standard_timings[0].hsize = 0x81;
> +		e->standard_timings[0].vfreq_aspect =  0xC0; // 720p60
> +
> +		e->standard_timings[1].hsize = 0xD1;
> +		e->standard_timings[1].vfreq_aspect =  0xC0; // 1080p60
> +
> +		e->standard_timings[2].hsize = 0x81;
> +		e->standard_timings[2].vfreq_aspect =  0x80; // 1280x1024@60
> +
> +		e->standard_timings[3].hsize = 0xD1;
> +		e->standard_timings[3].vfreq_aspect =  0x40; // 1920x1440@60
> +
> +		e->standard_timings[4].hsize = 0xE1;
> +		e->standard_timings[4].vfreq_aspect =  0xC0; // 2048x1152@60
> +
> +		e->standard_timings[5].hsize = 0xA9;
> +		e->standard_timings[5].vfreq_aspect =  0x40; // 1600x1200@60
> +
> +		e->standard_timings[6].hsize = 0xB3;
> +		e->standard_timings[6].vfreq_aspect =  0x00; // 2048x1152@60
> +
> +		e->standard_timings[7].hsize = 0x95;
> +		e->standard_timings[7].vfreq_aspect =  0x00; // 1440x900@60
> +
> +		e->detailed_timings[1].data.other_data.type = EDID_DETAIL_MONITOR_NAME;
> +		memcpy(e->detailed_timings[1].data.other_data.data.str.str,
> +		       "VMware Screen", 13);
> +
> +		e->detailed_timings[2].data.other_data.type = 0x10;
> +		e->detailed_timings[3].data.other_data.type = 0x10;
> +
> +		e->extensions = 0;
> +	}
> +	e->width_cm = DIV_ROUND_CLOSEST(pref_mode->hdisplay, VMW_FAKE_PPCM);
> +	e->height_cm = DIV_ROUND_CLOSEST(pref_mode->vdisplay, VMW_FAKE_PPCM);
> +	// This is the preferred/native mode
> +	vmw_mode_to_timing(pref_mode, &e->detailed_timings[0]);
> +
> +	for (int i = 0; i < sizeof(struct edid) - 1; i++)
> +		checksum += ((u8 *)e)[i];
> +
> +	e->checksum = 0x100 - checksum;
> +}
> +
>   /*
>    * Common modes not present in the VESA DMT standard or assigned a VIC.
>    */
> @@ -2699,6 +2867,8 @@ int vmw_connector_get_modes(struct drm_connector *connector)
>   	drm_mode_probed_add(connector, mode);
>   	num_modes++;
>   	drm_dbg_kms(dev, "preferred mode " DRM_MODE_FMT "\n", DRM_MODE_ARG(mode));
> +	vmw_fill_fake_edid(mode, du->unit, &du->fake_edid);
> +	drm_connector_update_edid_property(connector, &du->fake_edid);
>   
>   	/* Probe connector for all modes not exceeding our geom limits */
>   	max_width  = dev_priv->texture_max_width;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
> index 6a8bb60c507d..56c739d78582 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
> @@ -32,6 +32,7 @@
>   #include <drm/drm_encoder.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_probe_helper.h>
> +#include <drm/drm_edid.h>
>   
>   #include "vmwgfx_drv.h"
>   
> @@ -363,6 +364,8 @@ struct vmw_display_unit {
>   	unsigned pref_height;
>   	bool pref_active;
>   
> +	struct edid fake_edid;
> +
>   	/*
>   	 * Gui positioning
>   	 */
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 82d18b88f4a7..3defdf00a975 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -1631,6 +1631,8 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
>   	drm_object_attach_property(&connector->base,
>   				   dev->mode_config.suggested_y_property, 0);
>   
> +	drm_connector_attach_edid_property(connector);
> +
>   	vmw_du_init(&stdu->base);
>   
>   	return 0;

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

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

[Jani Nikula]

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?

And preferrably let the EDID functions handle epoch counter updates
instead of doing it yourself, if at all possible.

BR,
Jani.


>
> Best regards
> Thomas
>
> [1] 
> https://elixir.bootlin.com/linux/v6.11.8/source/include/drm/drm_connector.h#L1994
>
>
>
>>
>> Signed-off-by: Ian Forbes <ian.forbes@broadcom.com>
>> ---
>>   drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 172 ++++++++++++++++++++++++++-
>>   drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |   3 +
>>   drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |   2 +
>>   3 files changed, 176 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> index 5a1192496d49..9d9d0b8539dd 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> @@ -37,7 +37,6 @@
>>   #include <drm/drm_fourcc.h>
>>   #include <drm/drm_rect.h>
>>   #include <drm/drm_sysfs.h>
>> -#include <drm/drm_edid.h>
>>   
>>   void vmw_du_init(struct vmw_display_unit *du)
>>   {
>> @@ -2662,6 +2661,175 @@ enum drm_mode_status vmw_connector_mode_valid(struct drm_connector *connector,
>>   	return MODE_OK;
>>   }
>>   
>> +/*
>> + * Average pixels per millimeter and centimeter for a circa 2020 display
>> + */
>> +#define VMW_FAKE_PPMM 4
>> +#define VMW_FAKE_PPCM 40
>> +
>> +static void vmw_mode_to_timing(const struct drm_display_mode *mode,
>> +			       struct detailed_timing *timing)
>> +{
>> +	struct detailed_pixel_timing *dpt = &timing->data.pixel_data;
>> +
>> +	const u8 hblank = (mode->htotal - mode->hdisplay);
>> +	const u8 hfront = (mode->hsync_start - mode->hdisplay);
>> +	const u8 hsync  = (mode->hsync_end - mode->hsync_start);
>> +
>> +	const u8 vblank = (mode->vtotal - mode->vdisplay);
>> +	const u8 vfront = (mode->vsync_start - mode->vdisplay);
>> +	const u8 vsync  = (mode->vsync_end - mode->vsync_start);
>> +
>> +	const unsigned int wmm = mode->hdisplay / VMW_FAKE_PPMM;
>> +	const unsigned int hmm = mode->vdisplay / VMW_FAKE_PPMM;
>> +
>> +	timing->pixel_clock = mode->clock / 10;
>> +	memset(dpt, 0, sizeof(*dpt));
>> +
>> +	// horizontal
>> +	dpt->hactive_lo = mode->hdisplay & 0xFF;
>> +	dpt->hblank_lo = hblank & 0xFF;
>> +
>> +	dpt->hactive_hblank_hi |= (mode->hdisplay >> 4) & 0xF0;
>> +	dpt->hactive_hblank_hi |= (hblank >> 8) & 0x0F;
>> +
>> +	dpt->hsync_offset_lo      = hfront & 0xFF;
>> +	dpt->hsync_pulse_width_lo =  hsync & 0xFF;
>> +
>> +	dpt->hsync_vsync_offset_pulse_width_hi |= (hfront >> 2) & 0xC0;
>> +	dpt->hsync_vsync_offset_pulse_width_hi |= (hsync  >> 4) & 0x30;
>> +
>> +	// vertical
>> +	dpt->vactive_lo = mode->vdisplay & 0xFF;
>> +	dpt->vactive_vblank_hi |= (mode->vdisplay >> 4) & 0xF0;
>> +
>> +	dpt->vblank_lo = vblank & 0xFF;
>> +	dpt->vactive_vblank_hi |= (vblank >> 8) & 0x0F;
>> +
>> +	dpt->vsync_offset_pulse_width_lo |= (vfront & 0x0F) << 4;
>> +	dpt->vsync_offset_pulse_width_lo |= (vsync  & 0x0F) << 0;
>> +
>> +	dpt->hsync_vsync_offset_pulse_width_hi |= (vfront >> 4) & 0x0C;
>> +	dpt->hsync_vsync_offset_pulse_width_hi |= (vsync  >> 8) & 0x03;
>> +
>> +	// physical sizes
>> +	dpt->width_mm_lo  = wmm & 0xFF;
>> +	dpt->height_mm_lo = hmm & 0xFF;
>> +	dpt->width_height_mm_hi |= (wmm >> 4) & 0xF0;
>> +	dpt->width_height_mm_hi |= (hmm >> 8) & 0x0F;
>> +
>> +	dpt->hborder = 0;
>> +	dpt->vborder = 0;
>> +	dpt->misc |= 0x18;
>> +	dpt->misc |= (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0x2 : 0;
>> +	dpt->misc |= (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0x4 : 0;
>> +}
>> +
>> +/* Our encoded Plug & Play ID
>> + * See: https://uefi.org/PNP_ACPI_Registry and https://uefi.org/PNP_ID_List
>> + */
>> +static inline __be16 vmw_pnp_id(void)
>> +{
>> +	return cpu_to_be16((('V' - '@') << 10) |
>> +			   (('M' - '@') <<  5) |
>> +			   (('W' - '@') <<  0));
>> +}
>> +
>> +/*
>> + * Fills in the fake EDID using the preferred mode.
>> + * See 'Vesa Enhanced EDID Standard Release A Revision 2' and
>> + * 'VESA DMT Standard Version 1.0 Revision 13'.
>> + */
>> +static void vmw_fill_fake_edid(const struct drm_display_mode *pref_mode,
>> +			       unsigned int unit, struct edid *e)
>> +{
>> +	int checksum = 0;
>> +
>> +	/*
>> +	 * Bump the manufacture week ever time we fill the edid so that
>> +	 * it will change on every hotplug. This way even if the screen
>> +	 * size does not change userspace will rescan the connector. A
>> +	 * hotplug with no changes in resolution is likely a multi-mon change
>> +	 * and the suggested_X/Y will have changed and we want userspace to
>> +	 * pick up this. Use this field to only fill the constant data once
>> +	 * by checking for zero.
>> +	 */
>> +	if (e->mfg_week++ == 0) {
>> +		memset(e->header + 1, 0xFF, 6);
>> +
>> +		e->product_id.manufacturer_name = vmw_pnp_id();
>> +		e->mfg_year = 32; // 2022
>> +
>> +		e->prod_code[0] = 'V';
>> +		e->prod_code[1] = 'M';
>> +		e->serial = 0xABC0 | unit;
>> +
>> +		e->version = 1;
>> +		e->revision = 4;
>> +
>> +		e->input = 0xA0;
>> +		e->gamma = 120; // 2.20
>> +		e->features = 0x26;
>> +
>> +		// Standard sRGB color space
>> +		e->red_green_lo = 0xEE;
>> +		e->blue_white_lo = 0x91;
>> +		e->red_x = 0xA3;
>> +		e->red_y = 0x54;
>> +		e->green_x = 0x4C;
>> +		e->green_y = 0x99;
>> +		e->blue_x  = 0x26;
>> +		e->blue_y  = 0x0F;
>> +		e->white_x = 0x50;
>> +		e->white_y = 0x54;
>> +
>> +		e->established_timings.t1 = 0x21;
>> +		e->established_timings.t2 = 0x08;
>> +
>> +		e->standard_timings[0].hsize = 0x81;
>> +		e->standard_timings[0].vfreq_aspect =  0xC0; // 720p60
>> +
>> +		e->standard_timings[1].hsize = 0xD1;
>> +		e->standard_timings[1].vfreq_aspect =  0xC0; // 1080p60
>> +
>> +		e->standard_timings[2].hsize = 0x81;
>> +		e->standard_timings[2].vfreq_aspect =  0x80; // 1280x1024@60
>> +
>> +		e->standard_timings[3].hsize = 0xD1;
>> +		e->standard_timings[3].vfreq_aspect =  0x40; // 1920x1440@60
>> +
>> +		e->standard_timings[4].hsize = 0xE1;
>> +		e->standard_timings[4].vfreq_aspect =  0xC0; // 2048x1152@60
>> +
>> +		e->standard_timings[5].hsize = 0xA9;
>> +		e->standard_timings[5].vfreq_aspect =  0x40; // 1600x1200@60
>> +
>> +		e->standard_timings[6].hsize = 0xB3;
>> +		e->standard_timings[6].vfreq_aspect =  0x00; // 2048x1152@60
>> +
>> +		e->standard_timings[7].hsize = 0x95;
>> +		e->standard_timings[7].vfreq_aspect =  0x00; // 1440x900@60
>> +
>> +		e->detailed_timings[1].data.other_data.type = EDID_DETAIL_MONITOR_NAME;
>> +		memcpy(e->detailed_timings[1].data.other_data.data.str.str,
>> +		       "VMware Screen", 13);
>> +
>> +		e->detailed_timings[2].data.other_data.type = 0x10;
>> +		e->detailed_timings[3].data.other_data.type = 0x10;
>> +
>> +		e->extensions = 0;
>> +	}
>> +	e->width_cm = DIV_ROUND_CLOSEST(pref_mode->hdisplay, VMW_FAKE_PPCM);
>> +	e->height_cm = DIV_ROUND_CLOSEST(pref_mode->vdisplay, VMW_FAKE_PPCM);
>> +	// This is the preferred/native mode
>> +	vmw_mode_to_timing(pref_mode, &e->detailed_timings[0]);
>> +
>> +	for (int i = 0; i < sizeof(struct edid) - 1; i++)
>> +		checksum += ((u8 *)e)[i];
>> +
>> +	e->checksum = 0x100 - checksum;
>> +}
>> +
>>   /*
>>    * Common modes not present in the VESA DMT standard or assigned a VIC.
>>    */
>> @@ -2699,6 +2867,8 @@ int vmw_connector_get_modes(struct drm_connector *connector)
>>   	drm_mode_probed_add(connector, mode);
>>   	num_modes++;
>>   	drm_dbg_kms(dev, "preferred mode " DRM_MODE_FMT "\n", DRM_MODE_ARG(mode));
>> +	vmw_fill_fake_edid(mode, du->unit, &du->fake_edid);
>> +	drm_connector_update_edid_property(connector, &du->fake_edid);
>>   
>>   	/* Probe connector for all modes not exceeding our geom limits */
>>   	max_width  = dev_priv->texture_max_width;
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
>> index 6a8bb60c507d..56c739d78582 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
>> @@ -32,6 +32,7 @@
>>   #include <drm/drm_encoder.h>
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_probe_helper.h>
>> +#include <drm/drm_edid.h>
>>   
>>   #include "vmwgfx_drv.h"
>>   
>> @@ -363,6 +364,8 @@ struct vmw_display_unit {
>>   	unsigned pref_height;
>>   	bool pref_active;
>>   
>> +	struct edid fake_edid;
>> +
>>   	/*
>>   	 * Gui positioning
>>   	 */
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
>> index 82d18b88f4a7..3defdf00a975 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
>> @@ -1631,6 +1631,8 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
>>   	drm_object_attach_property(&connector->base,
>>   				   dev->mode_config.suggested_y_property, 0);
>>   
>> +	drm_connector_attach_edid_property(connector);
>> +
>>   	vmw_du_init(&stdu->base);
>>   
>>   	return 0;

-- 
Jani Nikula, Intel

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

[Zack Rusin]

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.

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

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

[Ian Forbes]

[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]

On Wed, Nov 20, 2024 at 4:22 AM Jani Nikula <jani.nikula@linux.intel.com>
wrote:
>
> 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.
>

drm_edid isn't exported so we can't use it. I know you've read the EDID
spec so complaining about the magic numbers is silly.
I didn't choose to make the whole spec based on bizarrely packed 10 and 12
bit numbers, they are unavoidable.

>
> > 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?
>
> And preferrably let the EDID functions handle epoch counter updates
> instead of doing it yourself, if at all possible.
>
> BR,
> Jani.
>

You're both missing the fact that virtual devices never disconnect active
displays. We don't have to do a disconnect/connect cycle like a physical
monitor and we wouldn't want to because it would be poor user experience.
The issue is not sending the hotplug event, it's that userspace will ignore
hotplug events on connectors that were previously connected because they
assume a disconnect/connect cycle must occur for changes to occur. The
whole point of hotplug_mode_update was as a hint to userspace that the
display can be "re-plugged" without a disconnect first and to always rescan
the connector for changes on hotplug.

Currently compositors are taking an arbitrary set of connector properties
that they've organically collected over the years and doing a diff to
trigger a refresh in the modes/display layout. EDID is the only property
that they universally agree should trigger a display layout change. The
fact that Autofit works on any compsitors using vmwgfx, qxl, or virtio is
completely by accident.

EDID is also a standardized connector property so it's not really fair to
say that all devices should export this property and then when we fix our
deficiency to block it. Now that it's standardized it is part of the uapi
and if userspace does weird things with it it's not really our concern. The
fact that it's standardized is also likely the reason it is being used in
such a fashion.

Ian,

[-- Attachment #2: Type: text/html, Size: 3593 bytes --]

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

[Jani Nikula]

On Wed, 20 Nov 2024, Ian Forbes <ian.forbes@broadcom.com> wrote:
> On Wed, Nov 20, 2024 at 4:22 AM Jani Nikula <jani.nikula@linux.intel.com>
> wrote:
>>
>> 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.
>>
>
> drm_edid isn't exported so we can't use it. I know you've read the EDID
> spec so complaining about the magic numbers is silly.
> I didn't choose to make the whole spec based on bizarrely packed 10 and 12
> bit numbers, they are unavoidable.

I'm complaining about adding those details in drivers. That is, if we
really have to have EDID generation.

It's hard to get all the EDID details right. Drivers would invariably
get them wrong, in different ways. The main reason struct drm_edid is
opaque is to prevent drivers from attempting to parse it
themselves. (Yes, drm_edid.c gets stuff wrong too, but at least it's the
same for everyone and can be fixed in one place.)

>>
>> > 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?
>>
>> And preferrably let the EDID functions handle epoch counter updates
>> instead of doing it yourself, if at all possible.
>>
>> BR,
>> Jani.
>>
>
> You're both missing the fact that virtual devices never disconnect active
> displays. We don't have to do a disconnect/connect cycle like a physical
> monitor and we wouldn't want to because it would be poor user experience.
> The issue is not sending the hotplug event, it's that userspace will ignore
> hotplug events on connectors that were previously connected because they
> assume a disconnect/connect cycle must occur for changes to occur. The
> whole point of hotplug_mode_update was as a hint to userspace that the
> display can be "re-plugged" without a disconnect first and to always rescan
> the connector for changes on hotplug.
>
> Currently compositors are taking an arbitrary set of connector properties
> that they've organically collected over the years and doing a diff to
> trigger a refresh in the modes/display layout. EDID is the only property
> that they universally agree should trigger a display layout change. The
> fact that Autofit works on any compsitors using vmwgfx, qxl, or virtio is
> completely by accident.
>
> EDID is also a standardized connector property so it's not really fair to
> say that all devices should export this property and then when we fix our
> deficiency to block it. Now that it's standardized it is part of the uapi
> and if userspace does weird things with it it's not really our concern. The
> fact that it's standardized is also likely the reason it is being used in
> such a fashion.

If you want standardized, you should probably consider using
drm_edid_read_custom() to fetch the generated EDID the same way all the
other EDIDs get fetched, with support for firmware/debugfs EDID, and
validity checks. And you get to use drm_edid based interfaces. And you
get connector->display_info filled with the parsed data, so it aligns
with everyone else.

Arguably it's also backwards to first generate modes, update them using
drm_mode_probed_add() and drm_add_modes_noedid(), then generate an EDID
based on them and update the property with that. Userspace should not
even look at the modes from the EDID directly, only the modes provided
via KMS API, and nobody would even know if they differ. So if we go the
EDID generation route, IMO this should go all the way. Generate EDID
based on modes, and then use drm_edid_connector_add_modes() to add the
modes.

BR,
Jani.


-- 
Jani Nikula, Intel

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

[Jonas Ådahl]

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

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

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 5181 bytes --]

On Tue, Dec 3, 2024 at 10:57 AM Jonas Ådahl <jadahl@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@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.

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 #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5427 bytes --]

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

[Jonas Ådahl]

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

So I'm still unsure what the problem you are seeing is. If any mode
changed in any way, after having freshly queried drm for all connector
associated state, we'd do the monitor reconfiguration.


Jonas

> 
> z

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

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 6379 bytes --]

On Tue, Dec 3, 2024 at 11:32 AM Jonas Ådahl <jadahl@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@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@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.
> >
> > 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.

z

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5427 bytes --]

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

[Thomas Zimmermann]

Hi


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

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

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 6991 bytes --]

On Tue, Dec 3, 2024 at 11:42 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
>
> Am 03.12.24 um 17:39 schrieb Zack Rusin:
> > On Tue, Dec 3, 2024 at 11:32 AM Jonas Ådahl <jadahl@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@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@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.
> >>> 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.

I don't think anyone is particularly excited about putting fake edid
into virtualized drivers. It's either that or we ourselves have to
maintain the special paths that are required to make virtualized
drivers work in userspace. We just don't have enough resources to
maintain every piece of userspace, so having virtualized drivers look
more like real gpu's is in the long term a much more sustainable path.

z

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5427 bytes --]

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

[Jonas Ådahl]

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

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

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 9135 bytes --]

On Tue, Dec 3, 2024 at 1:21 PM Jonas Ådahl <jadahl@gmail.com> wrote:
>
> 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@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@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@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.
> > > >
> > > > 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.

Yea, I think Ian was planning to update docs for it at some point.
It's a confusing one because it's really just a flag. For most
properties it's the fact that they changed between the old and the
current state that implies that a rescan is needed, for
hotplug_mode_update it's the presence i.e. if the property is present
a full-rescan is needed and when it's absent the regular paths can be
taken (i.e. just comparing edid's would be enough).

Plus a lot has changed since that property was introduced e.g.
userspace often performs more checks than just comparing edid's which
means that using the property incorrectly or not using it at all
doesn't necessarily mean that anything is visibly broken.

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

It's closer. In Mutter there's no reason to compare it at all, e.g. instead of
if (state->hotplug_mode_update != new_state->hotplug_mode_update)
    return META_KMS_RESOURCE_CHANGE_FULL;
it should probably be:
if (state->hotplug_mode_update)
    return META_KMS_RESOURCE_CHANGE_FULL;

z

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5427 bytes --]

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

[Jonas Ådahl]

On Tue, Dec 03, 2024 at 02:46:10PM -0500, Zack Rusin wrote:
> On Tue, Dec 3, 2024 at 1:21 PM Jonas Ådahl <jadahl@gmail.com> wrote:
> >
> > 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@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@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@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.
> > > > >
> > > > > 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.
> 
> Yea, I think Ian was planning to update docs for it at some point.
> It's a confusing one because it's really just a flag. For most
> properties it's the fact that they changed between the old and the
> current state that implies that a rescan is needed, for
> hotplug_mode_update it's the presence i.e. if the property is present
> a full-rescan is needed and when it's absent the regular paths can be
> taken (i.e. just comparing edid's would be enough).

It would be great if it could be documented, so usperspace can stop
guessing what the correct behavior is :)

> 
> Plus a lot has changed since that property was introduced e.g.
> userspace often performs more checks than just comparing edid's which
> means that using the property incorrectly or not using it at all
> doesn't necessarily mean that anything is visibly broken.
> 
> > 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;
> 
> It's closer. In Mutter there's no reason to compare it at all, e.g. instead of
> if (state->hotplug_mode_update != new_state->hotplug_mode_update)
>     return META_KMS_RESOURCE_CHANGE_FULL;
> it should probably be:
> if (state->hotplug_mode_update)
>     return META_KMS_RESOURCE_CHANGE_FULL;

I suspect not, since we already did a full rescan of all things KMS
prior to ending up here. The purpose of the function here is to
"optimize" away changes we don't care about, and if e.g. there were no
relevant changes, might as well not do anything. FWIW, it was added as
a necessity due to how hotplugs and the privacy screen kernel uAPI is
designed.

The check here, if the above diff would be applied, would treat the
property going away or appearing as "lets reconfigure everything from
scratch again".

But anyhow, perhaps a bit off topic to this thread.


Jonas

> 
> z