Re: [Intel-gfx] [PATCH v2 09/16] drm/edid: detach debugfs EDID override from EDID property update

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 24, 2022 at 03:33:37PM +0300, Jani Nikula wrote:
> Having the EDID override debugfs directly update the EDID property is
> problematic. The update is partial only. The driver has no way of
> knowing it's been updated. Mode list is not updated. It's an
> inconsistent state.
> 
> Detach debugfs EDID override from the property update completely. Only
> set and reset a separate override EDID copy from debugfs, and have it
> take effect only at detect (via EDID read). The copy is at
> connector->edid_override, protected by connector->edid_override_mutex.
> 
> This also brings override EDID closer to firmware EDID in behaviour.
> 
> Add validation of the override EDID which we completely lacked.
> 
> Note that IGT already forces a detect whenever tests update the override
> EDID.
> 
> v2: Add locking (Ville)
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_connector.c |  1 +
>  drivers/gpu/drm/drm_edid.c      | 73 ++++++++++++++++-----------------
>  include/drm/drm_connector.h     | 16 ++++++--
>  3 files changed, 49 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 61c29ce74b03..f25674c0d41e 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -274,6 +274,7 @@ static int __drm_connector_init(struct drm_device *dev,
>  	INIT_LIST_HEAD(&connector->probed_modes);
>  	INIT_LIST_HEAD(&connector->modes);
>  	mutex_init(&connector->mutex);
> +	mutex_init(&connector->edid_override_mutex);
>  	connector->edid_blob_ptr = NULL;
>  	connector->epoch_counter = 0;
>  	connector->tile_blob_ptr = NULL;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c3cf942186b7..b8d07e4d6215 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2207,8 +2207,12 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
>  {
>  	struct edid *override = NULL;
>  
> -	if (connector->override_edid)
> -		override = drm_edid_duplicate(connector->edid_blob_ptr->data);
> +	mutex_lock(&connector->edid_override_mutex);
> +
> +	if (connector->edid_override)
> +		override = drm_edid_duplicate(connector->edid_override->edid);
> +
> +	mutex_unlock(&connector->edid_override_mutex);

I was first thinking we'd use connection_mutex or something for this,
but given no one should anymore access this thing directly a dedicated
mutex seems like a fine choice. At least it's clear what it protects.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	if (!override)
>  		override = drm_load_edid_firmware(connector);
> @@ -2223,10 +2227,15 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
>  /* For debugfs edid_override implementation */
>  int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
>  {
> -	struct drm_property_blob *edid = connector->edid_blob_ptr;
> +	const struct drm_edid *drm_edid;
>  
> -	if (connector->override_edid && edid)
> -		seq_write(m, edid->data, edid->length);
> +	mutex_lock(&connector->edid_override_mutex);
> +
> +	drm_edid = connector->edid_override;
> +	if (drm_edid)
> +		seq_write(m, drm_edid->edid, drm_edid->size);
> +
> +	mutex_unlock(&connector->edid_override_mutex);
>  
>  	return 0;
>  }
> @@ -2235,32 +2244,43 @@ int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
>  int drm_edid_override_set(struct drm_connector *connector, const void *edid,
>  			  size_t size)
>  {
> -	int ret;
> +	const struct drm_edid *drm_edid;
>  
> -	if (size < EDID_LENGTH || edid_size(edid) > size)
> +	drm_edid = drm_edid_alloc(edid, size);
> +	if (!drm_edid_valid(drm_edid)) {
> +		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override invalid\n",
> +			    connector->base.id, connector->name);
> +		drm_edid_free(drm_edid);
>  		return -EINVAL;
> -
> -	connector->override_edid = false;
> +	}
>  
>  	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override set\n",
>  		    connector->base.id, connector->name);
>  
> -	ret = drm_connector_update_edid_property(connector, edid);
> -	if (!ret)
> -		connector->override_edid = true;
> +	mutex_lock(&connector->edid_override_mutex);
>  
> -	return ret;
> +	drm_edid_free(connector->edid_override);
> +	connector->edid_override = drm_edid;
> +
> +	mutex_unlock(&connector->edid_override_mutex);
> +
> +	return 0;
>  }
>  
>  /* For debugfs edid_override implementation */
>  int drm_edid_override_reset(struct drm_connector *connector)
>  {
> -	connector->override_edid = false;
> -
>  	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override reset\n",
>  		    connector->base.id, connector->name);
>  
> -	return drm_connector_update_edid_property(connector, NULL);
> +	mutex_lock(&connector->edid_override_mutex);
> +
> +	drm_edid_free(connector->edid_override);
> +	connector->edid_override = NULL;
> +
> +	mutex_unlock(&connector->edid_override_mutex);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -6634,23 +6654,6 @@ int drm_edid_connector_update(struct drm_connector *connector,
>  {
>  	int count;
>  
> -	/*
> -	 * FIXME: Reconcile the differences in override_edid handling between
> -	 * this and drm_connector_update_edid_property().
> -	 *
> -	 * If override_edid is set, and the EDID passed in here originates from
> -	 * drm_edid_read() and friends, it will be the override EDID, and there
> -	 * are no issues. drm_connector_update_edid_property() ignoring requests
> -	 * to set the EDID dates back to a time when override EDID was not
> -	 * handled at the low level EDID read.
> -	 *
> -	 * The only way the EDID passed in here can be different from the
> -	 * override EDID is when a driver passes in an EDID that does *not*
> -	 * originate from drm_edid_read() and friends, or passes in a stale
> -	 * cached version. This, in turn, is a question of when an override EDID
> -	 * set via debugfs should take effect.
> -	 */
> -
>  	count = _drm_edid_connector_update(connector, drm_edid);
>  
>  	_drm_update_tile_info(connector, drm_edid);
> @@ -6665,10 +6668,6 @@ EXPORT_SYMBOL(drm_edid_connector_update);
>  static int _drm_connector_update_edid_property(struct drm_connector *connector,
>  					       const struct drm_edid *drm_edid)
>  {
> -	/* ignore requests to set edid when overridden */
> -	if (connector->override_edid)
> -		return 0;
> -
>  	/*
>  	 * Set the display info, using edid if available, otherwise resetting
>  	 * the values to defaults. This duplicates the work done in
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b1b2df48d42c..e641a4725f99 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1550,12 +1550,20 @@ struct drm_connector {
>  	struct drm_cmdline_mode cmdline_mode;
>  	/** @force: a DRM_FORCE_<foo> state for forced mode sets */
>  	enum drm_connector_force force;
> +
> +	/**
> +	 * @edid_override: Override EDID set via debugfs.
> +	 *
> +	 * Do not modify or access outside of the drm_edid_override_* family of
> +	 * functions.
> +	 */
> +	const struct drm_edid *edid_override;
> +
>  	/**
> -	 * @override_edid: has the EDID been overwritten through debugfs for
> -	 * testing? Do not modify outside of drm_edid_override_set() and
> -	 * drm_edid_override_reset().
> +	 * @edid_override_mutex: Protect access to edid_override.
>  	 */
> -	bool override_edid;
> +	struct mutex edid_override_mutex;
> +
>  	/** @epoch_counter: used to detect any other changes in connector, besides status */
>  	u64 epoch_counter;
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel