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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 22 Nov 2024, Ian Forbes <ian.forbes@broadcom.com> wrote:
> 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, introduced in commit 4695b03970df
> ("qxl: add a connector property to denote hotplug should rescan modes."),
> 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 consistently. 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>
> ---
> v2:
>  - Use drm_edid_read_custom and struct drm_edid
>  - Use DRM_EDID defines where possible
>  - Add edid property to Legacy DU and Screen Object DU
>  - memset the detailed timings since we no longer control the allocation
>
>
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 192 +++++++++++++++++++++++++++
>  drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c  |   1 +
>  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |   2 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |   2 +-
>  4 files changed, 195 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 5a1192496d49..64135cd2302e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -2662,6 +2662,189 @@ 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));
> +}
> +
> +// For use with drm_edid_read_custom
> +struct vmw_edid_read_custom_ctx {
> +	struct drm_display_mode *pref_mode;
> +	unsigned int unit; // display unit id
> +};
> +
> +/*
> + * Fills in the fake EDID using the preferred mode.
> + * The signature must match the drm_edid_read_custom callback.
> + * Only supports the EDID base block. No extensions.
> + * See 'Vesa Enhanced EDID Standard Release A Revision 2' and
> + * 'VESA DMT Standard Version 1.0 Revision 13'.
> + */
> +static
> +int vmw_fill_fake_edid(void *context, u8 *buf, unsigned int block, size_t len)
> +{
> +	int checksum = 0;
> +	struct edid *e = (struct edid *)buf;
> +	const struct vmw_edid_read_custom_ctx *ctx = context;
> +
> +	if (block != 0)
> +		return -ENOENT;
> +	if (len != EDID_LENGTH)
> +		return -EINVAL;
> +
> +	e->header[0] = 0;
> +	memset(e->header + 1, 0xFF, 6);
> +	e->header[7] = 0;
> +
> +	e->product_id.manufacturer_name = vmw_pnp_id();
> +	e->mfg_year = 32; // 2022
> +	e->mfg_week = 32;
> +
> +	e->prod_code[0] = 'V';
> +	e->prod_code[1] = 'M';
> +	e->serial = 0xABC0 | ctx->unit;

Please consider filling the above via the product_id sub-struct instead
of mixing. Notably serial is little-endian.

> +
> +	e->version = 1;
> +	e->revision = 4;
> +
> +	e->input = DRM_EDID_INPUT_DIGITAL | DRM_EDID_DIGITAL_DEPTH_8;
> +	e->width_cm = DIV_ROUND_CLOSEST(ctx->pref_mode->hdisplay, VMW_FAKE_PPCM);
> +	e->height_cm = DIV_ROUND_CLOSEST(ctx->pref_mode->vdisplay, VMW_FAKE_PPCM);
> +	e->gamma = 120; // 2.20
> +	e->features = DRM_EDID_FEATURE_STANDARD_COLOR |
> +			DRM_EDID_FEATURE_PREFERRED_TIMING |
> +			DRM_EDID_FEATURE_RGB |
> +			DRM_EDID_FEATURE_PM_ACTIVE_OFF;
> +
> +	// 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
> +
> +	// This is the preferred/native mode
> +	vmw_mode_to_timing(ctx->pref_mode, &e->detailed_timings[0]);
> +
> +	memset(&e->detailed_timings[1], 0, 3 * sizeof(struct detailed_timing));
> +
> +	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;
> +
> +	for (int i = 0; i < sizeof(struct edid) - 1; i++)
> +		checksum += ((u8 *)e)[i];
> +
> +	e->checksum = 0x100 - checksum;
> +	return 0;
> +}
> +
>  /*
>   * Common modes not present in the VESA DMT standard or assigned a VIC.
>   */
> @@ -2685,6 +2868,8 @@ int vmw_connector_get_modes(struct drm_connector *connector)
>  	struct drm_device *dev = connector->dev;
>  	struct vmw_private *dev_priv = vmw_priv(dev);
>  	struct drm_display_mode *mode = NULL;
> +	const  struct drm_edid *fake_edid;
> +	struct vmw_edid_read_custom_ctx ctx;
>  	u32 max_width;
>  	u32 max_height;
>  	u32 num_modes = 0;
> @@ -2700,6 +2885,13 @@ int vmw_connector_get_modes(struct drm_connector *connector)
>  	num_modes++;
>  	drm_dbg_kms(dev, "preferred mode " DRM_MODE_FMT "\n", DRM_MODE_ARG(mode));
>  
> +	// update EDID
> +	ctx.pref_mode = mode;
> +	ctx.unit = du->unit;
> +	fake_edid = drm_edid_read_custom(connector, vmw_fill_fake_edid, &ctx);
> +	drm_edid_connector_update(connector, fake_edid);
> +	drm_edid_free(fake_edid);
> +

Now that you have the EDID, I'm wondering if you considered dropping
drm_mode_probed_add() and drm_add_modes_noedid() and using
drm_edid_connector_add_modes() instead?

Not insisting, but would seem more complete. With
drm_edid_read_custom(), this allows using debugfs/firmware EDID, and
consequently any modes through that.

Anyway, thanks for switch to struct drm_edid here. I did not review the
EDID generation details, but I have no objections to merging this. Sure,
maybe the EDID generation could be more generic, but seems easy enough
to iterate from here if more users show up.

BR,
Jani.

>  	/* Probe connector for all modes not exceeding our geom limits */
>  	max_width  = dev_priv->texture_max_width;
>  	max_height = dev_priv->texture_max_height;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> index 39949e0a493f..e6496e372993 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> @@ -516,6 +516,7 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
>  				   dev->mode_config.suggested_x_property, 0);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.suggested_y_property, 0);
> +	drm_connector_attach_edid_property(connector);
>  	if (dev_priv->implicit_placement_property)
>  		drm_object_attach_property
>  			(&connector->base,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index 0f4bfd98480a..50c3d0ca4767 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -905,7 +905,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
>  				   dev->mode_config.suggested_x_property, 0);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.suggested_y_property, 0);
> -
> +	drm_connector_attach_edid_property(connector);
>  	vmw_du_init(&sou->base);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 82d18b88f4a7..f7b2dbe4ef5b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -1630,7 +1630,7 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
>  				   dev->mode_config.suggested_x_property, 0);
>  	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