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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
>
> 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
