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