From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A567CC4332F for ; Wed, 19 Oct 2022 20:07:43 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A52EF10E00A; Wed, 19 Oct 2022 20:07:42 +0000 (UTC) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id E93DC10E00A; Wed, 19 Oct 2022 20:07:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1666210060; x=1697746060; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=nKzsJm4pqQVlZfNpCa5TDJAV/i7ukl/c1Mi8LLg7k3g=; b=LNEY8ySj0lMuLhwNqcsQtCesvinb1IemqZBSo6XqvTd0E5dtzsGbfVCA 27nKCAUlA7m1D4kn2JZhy0HAMlKiiZxj1U/69E5HXbEFgzca9ZBsdj/DD oHH7LtAUdalFEC/uDZxbPKMLr4B3PjqCeaoXJieXsDthh43H9GM26xmnO 6vXyhpDZnb2wYWN95GKt2fNuBdoKZ63NZ8ijH0UjWPtxoM+acjDKl21zN ruK9wYLWeHtqZj2CiqN1GDgGWaexiCcELxCp/ubEeB0/os72EfsxI/9dT cQRsDJY6Rp482j9XCechi1pFtUniXuGwu9wQqy1Jc3A8Lx8e9Yhp/hZjp w==; X-IronPort-AV: E=McAfee;i="6500,9779,10505"; a="305253271" X-IronPort-AV: E=Sophos;i="5.95,196,1661842800"; d="scan'208";a="305253271" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Oct 2022 13:07:40 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10505"; a="754742153" X-IronPort-AV: E=Sophos;i="5.95,196,1661842800"; d="scan'208";a="754742153" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.191]) by orsmga004.jf.intel.com with SMTP; 19 Oct 2022 13:07:37 -0700 Received: by stinkbox (sSMTP sendmail emulation); Wed, 19 Oct 2022 23:07:36 +0300 Date: Wed, 19 Oct 2022 23:07:36 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Jani Nikula Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Patchwork-Hint: comment Subject: Re: [Intel-gfx] [PATCH 09/15] drm/edid: detach debugfs EDID override from EDID property update X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" 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 > --- > 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_ 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A878DC433FE for ; Wed, 19 Oct 2022 20:07:48 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 037EE10E07A; Wed, 19 Oct 2022 20:07:45 +0000 (UTC) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id E93DC10E00A; Wed, 19 Oct 2022 20:07:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1666210060; x=1697746060; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=nKzsJm4pqQVlZfNpCa5TDJAV/i7ukl/c1Mi8LLg7k3g=; b=LNEY8ySj0lMuLhwNqcsQtCesvinb1IemqZBSo6XqvTd0E5dtzsGbfVCA 27nKCAUlA7m1D4kn2JZhy0HAMlKiiZxj1U/69E5HXbEFgzca9ZBsdj/DD oHH7LtAUdalFEC/uDZxbPKMLr4B3PjqCeaoXJieXsDthh43H9GM26xmnO 6vXyhpDZnb2wYWN95GKt2fNuBdoKZ63NZ8ijH0UjWPtxoM+acjDKl21zN ruK9wYLWeHtqZj2CiqN1GDgGWaexiCcELxCp/ubEeB0/os72EfsxI/9dT cQRsDJY6Rp482j9XCechi1pFtUniXuGwu9wQqy1Jc3A8Lx8e9Yhp/hZjp w==; X-IronPort-AV: E=McAfee;i="6500,9779,10505"; a="305253271" X-IronPort-AV: E=Sophos;i="5.95,196,1661842800"; d="scan'208";a="305253271" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Oct 2022 13:07:40 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10505"; a="754742153" X-IronPort-AV: E=Sophos;i="5.95,196,1661842800"; d="scan'208";a="754742153" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.191]) by orsmga004.jf.intel.com with SMTP; 19 Oct 2022 13:07:37 -0700 Received: by stinkbox (sSMTP sendmail emulation); Wed, 19 Oct 2022 23:07:36 +0300 Date: Wed, 19 Oct 2022 23:07:36 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Jani Nikula Subject: Re: [PATCH 09/15] drm/edid: detach debugfs EDID override from EDID property update Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Patchwork-Hint: comment X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 > --- > 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_ 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