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

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

On Tue, Oct 11, 2022 at 04:49:43PM +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.
> 
> 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.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 56 ++++++++++++-------------------------
>  include/drm/drm_connector.h | 11 +++++---
>  2 files changed, 25 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c3cf942186b7..0f2898badd51 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2207,8 +2207,8 @@ 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);
> +	if (connector->edid_override)
> +		override = drm_edid_duplicate(connector->edid_override->edid);
>  
>  	if (!override)
>  		override = drm_load_edid_firmware(connector);
> @@ -2223,10 +2223,10 @@ 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 = connector->edid_override;
>  
> -	if (connector->override_edid && edid)
> -		seq_write(m, edid->data, edid->length);
> +	if (drm_edid)
> +		seq_write(m, drm_edid->edid, drm_edid->size);
>  
>  	return 0;
>  }
> @@ -2235,32 +2235,33 @@ 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;
> +	drm_edid_free(connector->edid_override);
>  
> -	if (size < EDID_LENGTH || edid_size(edid) > size)
> +	connector->edid_override = drm_edid_alloc(edid, size);
> +	if (!drm_edid_valid(connector->edid_override)) {
> +		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override invalid\n",
> +			    connector->base.id, connector->name);
> +		drm_edid_free(connector->edid_override);
> +		connector->edid_override = NULL;
>  		return -EINVAL;

Hmm. Should we perhaps invest in some locking around these parts?

> -
> -	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;
> -
> -	return ret;
> +	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);
> +	drm_edid_free(connector->edid_override);
> +	connector->edid_override = NULL;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -6634,23 +6635,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 +6649,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..09a7d7f23e4a 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1550,12 +1550,15 @@ struct drm_connector {
>  	struct drm_cmdline_mode cmdline_mode;
>  	/** @force: a DRM_FORCE_<foo> state for forced mode sets */
>  	enum drm_connector_force force;
> +
>  	/**
> -	 * @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: Override EDID set via debugfs.
> +	 *
> +	 * Do not modify or access outside of the drm_edid_override_* family of
> +	 * functions.
>  	 */
> -	bool override_edid;
> +	const struct drm_edid *edid_override;
> +
>  	/** @epoch_counter: used to detect any other changes in connector, besides status */
>  	u64 epoch_counter;
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel