Message-ID: <aP9DU6g_qbEn1lHn@fedora>
Date: Mon, 27 Oct 2025 11:02:59 +0100
From: José Expósito <jose.exposito89@...il.com>
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: Haneen Mohammed <hamohammed.sa@...il.com>,
	Simona Vetter <simona@...ll.ch>,
	Melissa Wen <melissa.srw@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Jonathan Corbet <corbet@....net>,
	victoria@...tem76.com, sebastian.wick@...hat.com,
	thomas.petazzoni@...tlin.com, dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 18/22] drm/vkms: Introduce config for connector EDID

On Sat, Oct 18, 2025 at 04:01:18AM +0200, Louis Chauvet wrote:
> Allows configuration of EDID for each connector.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> ---
>  drivers/gpu/drm/vkms/tests/vkms_config_test.c |  2 +
>  drivers/gpu/drm/vkms/vkms_config.h            | 77 +++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_connector.c         | 48 +++++++++++++++--
>  3 files changed, 123 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> index a89ccd75060d..d1e380da31ff 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> @@ -190,6 +190,8 @@ static void vkms_config_test_default_config(struct kunit *test)
>  		KUNIT_EXPECT_EQ(test,
>  				vkms_config_connector_get_supported_colorspaces(connector_cfg),
>  				0);
> +		KUNIT_EXPECT_EQ(test, vkms_config_connector_get_edid_enabled(connector_cfg),
> +				false);
>  	}
>  
>  	KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index ec614c2d4a30..eaf76a58aab6 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -129,6 +129,8 @@ struct vkms_config_encoder {
>   * @type: Store the type of connector using DRM_MODE_CONNECTOR_* values
>   * @config: The vkms_config this connector belongs to
>   * @status: Status (connected, disconnected...) of the connector

@edid_enabled: If true, @edid and @edid_len are taken into account

(or similar)

> + * @edid: Stores the current EDID
> + * @edid_len: Current EDID length
>   * @possible_encoders: Array of encoders that can be used with this connector
>   * @connector: Internal usage. This pointer should never be considered as valid.
>   *             It can be used to store a temporary reference to a VKMS connector
> @@ -142,6 +144,9 @@ struct vkms_config_connector {
>  	int type;
>  	enum drm_connector_status status;
>  	u32 supported_colorspaces;
> +	bool edid_enabled;
> +	u8 *edid;
> +	unsigned int edid_len;
>  	struct xarray possible_encoders;
>  
>  	/* Internal usage */
> @@ -265,6 +270,78 @@ vkms_config_connector_get_supported_colorspaces(struct vkms_config_connector *co
>  	return connector_cfg->supported_colorspaces;
>  }
>  
> +/**
> + * vkms_config_connector_get_edid_enabled() - Check if EDID is enabled for a connector
> + * @connector_cfg: Connector configuration to check
> + *
> + * Returns:
> + * True if EDID is enabled for this connector, false otherwise.
> + */
> +static inline bool
> +vkms_config_connector_get_edid_enabled(struct vkms_config_connector *connector_cfg)
> +{
> +	return connector_cfg->edid_enabled;
> +}
> +
> +/**
> + * vkms_config_connector_set_edid_enabled() - Enable or disable EDID for a connector
> + * @connector_cfg: Connector configuration to modify
> + * @enabled: Whether to enable EDID for this connector
> + */
> +static inline void
> +vkms_config_connector_set_edid_enabled(struct vkms_config_connector *connector_cfg,
> +				       bool enabled)
> +{
> +	connector_cfg->edid_enabled = enabled;
> +}
> +
> +/**
> + * vkms_config_connector_get_edid() - Get the EDID data for a connector
> + * @connector_cfg: Connector configuration to get the EDID from
> + * @len: Pointer to store the length of the EDID data
> + *
> + * Returns:
> + * Pointer to the EDID data buffer, or NULL if no EDID is set.
> + * The length of the EDID data is stored in @len.
> + */
> +static inline const u8 *
> +vkms_config_connector_get_edid(const struct vkms_config_connector *connector_cfg, int *len)
> +{
> +	*len = connector_cfg->edid_len;
> +	return connector_cfg->edid;
> +}
> +
> +/**
> + * vkms_config_connector_set_edid() - Set the EDID data for a connector
> + * @connector_cfg: Connector configuration to modify
> + * @edid: Pointer to the EDID data buffer
> + * @len: Length of the EDID data
> + *
> + * If @len is 0, the EDID data will be cleared. If memory allocation fails,
> + * the existing EDID data will be preserved.
> + */
> +static inline void
> +vkms_config_connector_set_edid(struct vkms_config_connector *connector_cfg,
> +			       const u8 *edid, unsigned int len)
> +{
> +	if (len) {
> +		void *edid_tmp = krealloc(connector_cfg->edid, len, GFP_KERNEL);
> +
> +		if (edid_tmp) {
> +			connector_cfg->edid = edid_tmp;
> +			memcpy(connector_cfg->edid, edid, len);
> +			connector_cfg->edid_len = len;
> +		} else {
> +			kfree(connector_cfg->edid);
> +			connector_cfg->edid_len = 0;

I wonder if in case of error it makes sense to NULL connector_cfg->edid and
vkms_config_connector_set_edid_enabled(false)?

It'd be also nice to print the EDID in hex format in vkms_config_show().

> +		}
> +	} else {
> +		kfree(connector_cfg->edid);
> +		connector_cfg->edid = NULL;
> +		connector_cfg->edid_len = len;
> +	}
> +}
> +
>  /**
>   * vkms_config_get_device_name() - Return the name of the device
>   * @config: Configuration to get the device name from
> diff --git a/drivers/gpu/drm/vkms/vkms_connector.c b/drivers/gpu/drm/vkms/vkms_connector.c
> index cc59d13c2d22..339d747e729e 100644
> --- a/drivers/gpu/drm/vkms/vkms_connector.c
> +++ b/drivers/gpu/drm/vkms/vkms_connector.c
> @@ -42,13 +42,53 @@ static const struct drm_connector_funcs vkms_connector_funcs = {
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> +static int vkms_connector_read_block(void *context, u8 *buf, unsigned int block, size_t len)
> +{
> +	struct vkms_config_connector *config = context;
> +	unsigned int edid_len;
> +	const u8 *edid = vkms_config_connector_get_edid(config, &edid_len);
> +
> +	if (block * len + len > edid_len)
> +		return 1;
> +	memcpy(buf, &edid[block * len], len);
> +	return 0;
> +}
> +
>  static int vkms_conn_get_modes(struct drm_connector *connector)
>  {
> -	int count;
> +	struct vkms_connector *vkms_connector = drm_connector_to_vkms_connector(connector);
> +	const struct drm_edid *drm_edid = NULL;
> +	int count = 0;
> +	struct vkms_config_connector *context = NULL;
> +	struct drm_device *dev = connector->dev;
> +	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> +	struct vkms_config_connector *connector_cfg;
>  
> -	/* Use the default modes list from DRM */
> -	count = drm_add_modes_noedid(connector, XRES_MAX, YRES_MAX);
> -	drm_set_preferred_mode(connector, XRES_DEF, YRES_DEF);
> +	vkms_config_for_each_connector(vkmsdev->config, connector_cfg) {
> +		if (connector_cfg->connector == vkms_connector)
> +			context = connector_cfg;
> +	}
> +	if (context) {
> +		if (vkms_config_connector_get_edid_enabled(context)) {
> +			drm_edid = drm_edid_read_custom(connector,
> +							vkms_connector_read_block, context);
> +
> +			/*
> +			 * Unconditionally update the connector. If the EDID was read
> +			 * successfully, fill in the connector information derived from the
> +			 * EDID. Otherwise, if the EDID is NULL, clear the connector
> +			 * information.
> +			 */
> +			drm_edid_connector_update(connector, drm_edid);
> +
> +			count = drm_edid_connector_add_modes(connector);
> +
> +			drm_edid_free(drm_edid);
> +		} else {
> +			count = drm_add_modes_noedid(connector, XRES_MAX, YRES_MAX);
> +			drm_set_preferred_mode(connector, XRES_DEF, YRES_DEF);
> +		}
> +	}
>  
>  	return count;
>  }
> 
> -- 
> 2.51.0
> 

Powered by blists - more mailing lists