[PATCH 0/4] drm/dp: Limit the DPCD probe quirk to the affected monitor

[PATCH 0/4] drm/dp: Limit the DPCD probe quirk to the affected monitor

[Imre Deak]

This patchset fixes a known issue where the DPCD probe quirk leads to
link training failures (patch 1) and limits the quirk to the monitor
which requires it, to avoid similar failures in the future.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>

Imre Deak (4):
  drm/dp: Change AUX DPCD probe address from DPCD_REV to LANE0_1_STATUS
  drm/edid: Add support for quirks visible to DRM core and drivers
  drm/dp: Add an EDID quirk for the DPCD register access probe
  drm/i915/dp: Disable the AUX DPCD probe quirk if it's not required

 drivers/gpu/drm/display/drm_dp_helper.c      | 17 +++++++--
 drivers/gpu/drm/drm_edid.c                   | 36 ++++++++++++++++----
 drivers/gpu/drm/i915/display/intel_dp.c      | 11 ++++--
 drivers/gpu/drm/i915/display/intel_dp_aux.c  |  2 ++
 drivers/gpu/drm/i915/display/intel_hotplug.c | 10 ++++++
 include/drm/display/drm_dp_helper.h          |  6 ++++
 include/drm/drm_connector.h                  |  5 +++
 include/drm/drm_edid.h                       |  6 ++++
 8 files changed, 82 insertions(+), 11 deletions(-)

-- 
2.44.2

[PATCH 1/4] drm/dp: Change AUX DPCD probe address from DPCD_REV to LANE0_1_STATUS

[Imre Deak]

Reading DPCD registers has side-effects in general. In particular
accessing registers outside of the link training register range
(0x102-0x106, 0x202-0x207, 0x200c-0x200f, 0x2216) is explicitly
forbidden by the DP v2.1 Standard, see

3.6.5.1 DPTX AUX Transaction Handling Mandates
3.6.7.4 128b/132b DP Link Layer LTTPR Link Training Mandates

Based on my tests, accessing the DPCD_REV register during the link
training of an UHBR TBT DP tunnel sink leads to link training failures.

Solve the above by using the DP_LANE0_1_STATUS (0x202) register for the
DPCD register access quirk.

Cc: <stable@vger.kernel.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index f2a6559a27100..dc622c78db9d4 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -725,7 +725,7 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 	 * monitor doesn't power down exactly after the throw away read.
 	 */
 	if (!aux->is_remote) {
-		ret = drm_dp_dpcd_probe(aux, DP_DPCD_REV);
+		ret = drm_dp_dpcd_probe(aux, DP_LANE0_1_STATUS);
 		if (ret < 0)
 			return ret;
 	}
-- 
2.44.2

Re: [PATCH 1/4] drm/dp: Change AUX DPCD probe address from DPCD_REV to LANE0_1_STATUS

[Jani Nikula]

On Tue, 03 Jun 2025, Imre Deak <imre.deak@intel.com> wrote:
> Reading DPCD registers has side-effects in general. In particular
> accessing registers outside of the link training register range
> (0x102-0x106, 0x202-0x207, 0x200c-0x200f, 0x2216) is explicitly
> forbidden by the DP v2.1 Standard, see
>
> 3.6.5.1 DPTX AUX Transaction Handling Mandates
> 3.6.7.4 128b/132b DP Link Layer LTTPR Link Training Mandates
>
> Based on my tests, accessing the DPCD_REV register during the link
> training of an UHBR TBT DP tunnel sink leads to link training failures.
>
> Solve the above by using the DP_LANE0_1_STATUS (0x202) register for the
> DPCD register access quirk.
>
> Cc: <stable@vger.kernel.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Acked-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/display/drm_dp_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index f2a6559a27100..dc622c78db9d4 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -725,7 +725,7 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>  	 * monitor doesn't power down exactly after the throw away read.
>  	 */
>  	if (!aux->is_remote) {
> -		ret = drm_dp_dpcd_probe(aux, DP_DPCD_REV);
> +		ret = drm_dp_dpcd_probe(aux, DP_LANE0_1_STATUS);
>  		if (ret < 0)
>  			return ret;
>  	}

-- 
Jani Nikula, Intel

[PATCH 2/4] drm/edid: Add support for quirks visible to DRM core and drivers

[Imre Deak]

Add support for EDID based quirks which can be queried outside of the
EDID parser iteself by DRM core and drivers. There at least two such
quirks applicable to all drivers: the DPCD register access probe quirk
and the 128b/132b DPRX Lane Count Conversion quirk (see 3.5.2.16.3 in
the v2.1a DP Standard). The latter quirk applies to panels with specific
EDID panel names, accordingly add support for defining quirks based on
the EDID panel name.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 33 ++++++++++++++++++++++++++-------
 include/drm/drm_connector.h |  5 +++++
 include/drm/drm_edid.h      |  5 +++++
 3 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 74e77742b2bd4..e867315253493 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -114,15 +114,21 @@ struct drm_edid_match_closure {
 #define LEVEL_GTF2	2
 #define LEVEL_CVT	3
 
-#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
+#define PANEL_NAME_ANY	NULL
+
+#define DRM_EDID_QUIRK(_panel_id, _panel_name, _quirks) \
 { \
 	.ident = { \
-		.panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, \
-						     vend_chr_2, product_id), \
+		.panel_id = _panel_id, \
+		.name = _panel_name, \
 	}, \
 	.quirks = _quirks \
 }
 
+#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
+	DRM_EDID_QUIRK(drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, product_id), \
+		       PANEL_NAME_ANY, _quirks)
+
 static const struct edid_quirk {
 	const struct drm_edid_ident ident;
 	u32 quirks;
@@ -248,6 +254,9 @@ static const struct edid_quirk {
 	EDID_QUIRK('A', 'U', 'O', 0x1111, EDID_QUIRK_NON_DESKTOP),
 };
 
+static const struct edid_quirk global_edid_quirk_list[] = {
+};
+
 /*
  * Autogenerated from the DMT spec.
  * This table is copied from xfree86/modes/xf86EdidModes.c.
@@ -2937,13 +2946,14 @@ EXPORT_SYMBOL(drm_edid_duplicate);
  *
  * Return: A u32 represents the quirks to apply.
  */
-static u32 edid_get_quirks(const struct drm_edid *drm_edid)
+static u32 edid_get_quirks(const struct drm_edid *drm_edid,
+			   const struct edid_quirk *quirk_list, int quirk_list_size)
 {
 	const struct edid_quirk *quirk;
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
-		quirk = &edid_quirk_list[i];
+	for (i = 0; i < quirk_list_size; i++) {
+		quirk = &quirk_list[i];
 		if (drm_edid_match(drm_edid, &quirk->ident))
 			return quirk->quirks;
 	}
@@ -6660,7 +6670,10 @@ static void update_display_info(struct drm_connector *connector,
 
 	edid = drm_edid->edid;
 
-	info->quirks = edid_get_quirks(drm_edid);
+	info->quirks = edid_get_quirks(drm_edid, edid_quirk_list,
+				       ARRAY_SIZE(edid_quirk_list));
+	info->global_quirks = edid_get_quirks(drm_edid, global_edid_quirk_list,
+					      ARRAY_SIZE(global_edid_quirk_list));
 
 	info->width_mm = edid->width_cm * 10;
 	info->height_mm = edid->height_cm * 10;
@@ -7566,3 +7579,9 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid)
 		drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
 }
 EXPORT_SYMBOL(drm_edid_is_digital);
+
+bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk)
+{
+	return connector->display_info.global_quirks & BIT(quirk);
+}
+EXPORT_SYMBOL(drm_edid_has_quirk);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 73903c3c842f3..996ecf229f8c7 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -847,6 +847,11 @@ struct drm_display_info {
 	 */
 	u32 quirks;
 
+	/**
+	 * @global_quirks: EDID based quirks. Can be queried by DRM core and drivers.
+	 */
+	u32 global_quirks;
+
 	/**
 	 * @source_physical_address: Source Physical Address from HDMI
 	 * Vendor-Specific Data Block, for CEC usage.
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b38409670868d..3d8e168521c82 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -109,6 +109,10 @@ struct detailed_data_string {
 #define DRM_EDID_CVT_FLAGS_STANDARD_BLANKING (1 << 3)
 #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
 
+enum drm_edid_quirk {
+	DRM_EDID_QUIRK_NONE,
+};
+
 struct detailed_data_monitor_range {
 	u8 min_vfreq;
 	u8 max_vfreq;
@@ -476,5 +480,6 @@ void drm_edid_print_product_id(struct drm_printer *p,
 u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
 bool drm_edid_match(const struct drm_edid *drm_edid,
 		    const struct drm_edid_ident *ident);
+bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk);
 
 #endif /* __DRM_EDID_H__ */
-- 
2.44.2

[PATCH v2 2/4] drm/edid: Add support for quirks visible to DRM core and drivers

[Imre Deak]

Add support for EDID based quirks which can be queried outside of the
EDID parser iteself by DRM core and drivers. There are at least two such
quirks applicable to all drivers: the DPCD register access probe quirk
and the 128b/132b DPRX Lane Count Conversion quirk (see 3.5.2.16.3 in
the v2.1a DP Standard). The latter quirk applies to panels with specific
EDID panel names, accordingly add support for defining quirks based on
the EDID panel name.

v2: Reset global_quirks in drm_reset_display_info().

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 34 +++++++++++++++++++++++++++-------
 include/drm/drm_connector.h |  5 +++++
 include/drm/drm_edid.h      |  5 +++++
 3 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 74e77742b2bd4..5d3a25cbc4d36 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -114,15 +114,21 @@ struct drm_edid_match_closure {
 #define LEVEL_GTF2	2
 #define LEVEL_CVT	3
 
-#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
+#define PANEL_NAME_ANY	NULL
+
+#define DRM_EDID_QUIRK(_panel_id, _panel_name, _quirks) \
 { \
 	.ident = { \
-		.panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, \
-						     vend_chr_2, product_id), \
+		.panel_id = _panel_id, \
+		.name = _panel_name, \
 	}, \
 	.quirks = _quirks \
 }
 
+#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
+	DRM_EDID_QUIRK(drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, product_id), \
+		       PANEL_NAME_ANY, _quirks)
+
 static const struct edid_quirk {
 	const struct drm_edid_ident ident;
 	u32 quirks;
@@ -248,6 +254,9 @@ static const struct edid_quirk {
 	EDID_QUIRK('A', 'U', 'O', 0x1111, EDID_QUIRK_NON_DESKTOP),
 };
 
+static const struct edid_quirk global_edid_quirk_list[] = {
+};
+
 /*
  * Autogenerated from the DMT spec.
  * This table is copied from xfree86/modes/xf86EdidModes.c.
@@ -2937,13 +2946,14 @@ EXPORT_SYMBOL(drm_edid_duplicate);
  *
  * Return: A u32 represents the quirks to apply.
  */
-static u32 edid_get_quirks(const struct drm_edid *drm_edid)
+static u32 edid_get_quirks(const struct drm_edid *drm_edid,
+			   const struct edid_quirk *quirk_list, int quirk_list_size)
 {
 	const struct edid_quirk *quirk;
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
-		quirk = &edid_quirk_list[i];
+	for (i = 0; i < quirk_list_size; i++) {
+		quirk = &quirk_list[i];
 		if (drm_edid_match(drm_edid, &quirk->ident))
 			return quirk->quirks;
 	}
@@ -6614,6 +6624,7 @@ static void drm_reset_display_info(struct drm_connector *connector)
 	info->vics_len = 0;
 
 	info->quirks = 0;
+	info->global_quirks = 0;
 
 	info->source_physical_address = CEC_PHYS_ADDR_INVALID;
 }
@@ -6660,7 +6671,10 @@ static void update_display_info(struct drm_connector *connector,
 
 	edid = drm_edid->edid;
 
-	info->quirks = edid_get_quirks(drm_edid);
+	info->quirks = edid_get_quirks(drm_edid, edid_quirk_list,
+				       ARRAY_SIZE(edid_quirk_list));
+	info->global_quirks = edid_get_quirks(drm_edid, global_edid_quirk_list,
+					      ARRAY_SIZE(global_edid_quirk_list));
 
 	info->width_mm = edid->width_cm * 10;
 	info->height_mm = edid->height_cm * 10;
@@ -7566,3 +7580,9 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid)
 		drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
 }
 EXPORT_SYMBOL(drm_edid_is_digital);
+
+bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk)
+{
+	return connector->display_info.global_quirks & BIT(quirk);
+}
+EXPORT_SYMBOL(drm_edid_has_quirk);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 73903c3c842f3..996ecf229f8c7 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -847,6 +847,11 @@ struct drm_display_info {
 	 */
 	u32 quirks;
 
+	/**
+	 * @global_quirks: EDID based quirks. Can be queried by DRM core and drivers.
+	 */
+	u32 global_quirks;
+
 	/**
 	 * @source_physical_address: Source Physical Address from HDMI
 	 * Vendor-Specific Data Block, for CEC usage.
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b38409670868d..3d8e168521c82 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -109,6 +109,10 @@ struct detailed_data_string {
 #define DRM_EDID_CVT_FLAGS_STANDARD_BLANKING (1 << 3)
 #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
 
+enum drm_edid_quirk {
+	DRM_EDID_QUIRK_NONE,
+};
+
 struct detailed_data_monitor_range {
 	u8 min_vfreq;
 	u8 max_vfreq;
@@ -476,5 +480,6 @@ void drm_edid_print_product_id(struct drm_printer *p,
 u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
 bool drm_edid_match(const struct drm_edid *drm_edid,
 		    const struct drm_edid_ident *ident);
+bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk);
 
 #endif /* __DRM_EDID_H__ */
-- 
2.44.2

Re: [PATCH v2 2/4] drm/edid: Add support for quirks visible to DRM core and drivers

[Jani Nikula]

On Wed, 04 Jun 2025, Imre Deak <imre.deak@intel.com> wrote:
> Add support for EDID based quirks which can be queried outside of the
> EDID parser iteself by DRM core and drivers. There are at least two such
> quirks applicable to all drivers: the DPCD register access probe quirk
> and the 128b/132b DPRX Lane Count Conversion quirk (see 3.5.2.16.3 in
> the v2.1a DP Standard). The latter quirk applies to panels with specific
> EDID panel names, accordingly add support for defining quirks based on
> the EDID panel name.
>
> v2: Reset global_quirks in drm_reset_display_info().
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 34 +++++++++++++++++++++++++++-------
>  include/drm/drm_connector.h |  5 +++++
>  include/drm/drm_edid.h      |  5 +++++
>  3 files changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 74e77742b2bd4..5d3a25cbc4d36 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -114,15 +114,21 @@ struct drm_edid_match_closure {
>  #define LEVEL_GTF2	2
>  #define LEVEL_CVT	3
>  
> -#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
> +#define PANEL_NAME_ANY	NULL
> +
> +#define DRM_EDID_QUIRK(_panel_id, _panel_name, _quirks) \
>  { \
>  	.ident = { \
> -		.panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, \
> -						     vend_chr_2, product_id), \
> +		.panel_id = _panel_id, \
> +		.name = _panel_name, \
>  	}, \
>  	.quirks = _quirks \
>  }
>  
> +#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
> +	DRM_EDID_QUIRK(drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, product_id), \
> +		       PANEL_NAME_ANY, _quirks)
> +

I'm not sure why this has to change. It's not explained in the commit
message.

>  static const struct edid_quirk {
>  	const struct drm_edid_ident ident;
>  	u32 quirks;
> @@ -248,6 +254,9 @@ static const struct edid_quirk {
>  	EDID_QUIRK('A', 'U', 'O', 0x1111, EDID_QUIRK_NON_DESKTOP),

Don't we want the other quirk list also be this concise?

>  };
>  
> +static const struct edid_quirk global_edid_quirk_list[] = {
> +};
> +
>  /*
>   * Autogenerated from the DMT spec.
>   * This table is copied from xfree86/modes/xf86EdidModes.c.
> @@ -2937,13 +2946,14 @@ EXPORT_SYMBOL(drm_edid_duplicate);
>   *
>   * Return: A u32 represents the quirks to apply.
>   */
> -static u32 edid_get_quirks(const struct drm_edid *drm_edid)
> +static u32 edid_get_quirks(const struct drm_edid *drm_edid,
> +			   const struct edid_quirk *quirk_list, int quirk_list_size)
>  {
>  	const struct edid_quirk *quirk;
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
> -		quirk = &edid_quirk_list[i];
> +	for (i = 0; i < quirk_list_size; i++) {
> +		quirk = &quirk_list[i];
>  		if (drm_edid_match(drm_edid, &quirk->ident))
>  			return quirk->quirks;
>  	}
> @@ -6614,6 +6624,7 @@ static void drm_reset_display_info(struct drm_connector *connector)
>  	info->vics_len = 0;
>  
>  	info->quirks = 0;
> +	info->global_quirks = 0;

So I am not sure if we really need or want two lists.

I think we could have

drm_edid.h:

enum drm_edid_quirk {
	/* ... */
	DRM_EDID_QUIRK_MAX,
};

drm_edid.c:

enum drm_edid_internal_quirk {
	FOO_QUIRK = DRM_EDID_QUIRK_MAX,
        /* etc ... */
};

And just make info->quirks big enough. I think it feels simpler overall.

>  
>  	info->source_physical_address = CEC_PHYS_ADDR_INVALID;
>  }
> @@ -6660,7 +6671,10 @@ static void update_display_info(struct drm_connector *connector,
>  
>  	edid = drm_edid->edid;
>  
> -	info->quirks = edid_get_quirks(drm_edid);
> +	info->quirks = edid_get_quirks(drm_edid, edid_quirk_list,
> +				       ARRAY_SIZE(edid_quirk_list));
> +	info->global_quirks = edid_get_quirks(drm_edid, global_edid_quirk_list,
> +					      ARRAY_SIZE(global_edid_quirk_list));
>  
>  	info->width_mm = edid->width_cm * 10;
>  	info->height_mm = edid->height_cm * 10;
> @@ -7566,3 +7580,9 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid)
>  		drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
>  }
>  EXPORT_SYMBOL(drm_edid_is_digital);
> +
> +bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk)
> +{
> +	return connector->display_info.global_quirks & BIT(quirk);
> +}
> +EXPORT_SYMBOL(drm_edid_has_quirk);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 73903c3c842f3..996ecf229f8c7 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -847,6 +847,11 @@ struct drm_display_info {
>  	 */
>  	u32 quirks;
>  
> +	/**
> +	 * @global_quirks: EDID based quirks. Can be queried by DRM core and drivers.

Might mention that you should not access directly, but using
drm_edid_has_quirk().

> +	 */
> +	u32 global_quirks;
> +
>  	/**
>  	 * @source_physical_address: Source Physical Address from HDMI
>  	 * Vendor-Specific Data Block, for CEC usage.
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index b38409670868d..3d8e168521c82 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -109,6 +109,10 @@ struct detailed_data_string {
>  #define DRM_EDID_CVT_FLAGS_STANDARD_BLANKING (1 << 3)
>  #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
>  
> +enum drm_edid_quirk {
> +	DRM_EDID_QUIRK_NONE,
> +};
> +
>  struct detailed_data_monitor_range {
>  	u8 min_vfreq;
>  	u8 max_vfreq;
> @@ -476,5 +480,6 @@ void drm_edid_print_product_id(struct drm_printer *p,
>  u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
>  bool drm_edid_match(const struct drm_edid *drm_edid,
>  		    const struct drm_edid_ident *ident);
> +bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk);
>  
>  #endif /* __DRM_EDID_H__ */

-- 
Jani Nikula, Intel

Re: [PATCH v2 2/4] drm/edid: Add support for quirks visible to DRM core and drivers

[Imre Deak]

On Wed, Jun 04, 2025 at 01:02:57PM +0300, Jani Nikula wrote:
> On Wed, 04 Jun 2025, Imre Deak <imre.deak@intel.com> wrote:
> > Add support for EDID based quirks which can be queried outside of the
> > EDID parser iteself by DRM core and drivers. There are at least two such
> > quirks applicable to all drivers: the DPCD register access probe quirk
> > and the 128b/132b DPRX Lane Count Conversion quirk (see 3.5.2.16.3 in
> > the v2.1a DP Standard). The latter quirk applies to panels with specific
> > EDID panel names, accordingly add support for defining quirks based on
> > the EDID panel name.
> >
> > v2: Reset global_quirks in drm_reset_display_info().
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 34 +++++++++++++++++++++++++++-------
> >  include/drm/drm_connector.h |  5 +++++
> >  include/drm/drm_edid.h      |  5 +++++
> >  3 files changed, 37 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 74e77742b2bd4..5d3a25cbc4d36 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -114,15 +114,21 @@ struct drm_edid_match_closure {
> >  #define LEVEL_GTF2	2
> >  #define LEVEL_CVT	3
> >  
> > -#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
> > +#define PANEL_NAME_ANY	NULL
> > +
> > +#define DRM_EDID_QUIRK(_panel_id, _panel_name, _quirks) \
> >  { \
> >  	.ident = { \
> > -		.panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, \
> > -						     vend_chr_2, product_id), \
> > +		.panel_id = _panel_id, \
> > +		.name = _panel_name, \
> >  	}, \
> >  	.quirks = _quirks \
> >  }
> >  
> > +#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
> > +	DRM_EDID_QUIRK(drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, product_id), \
> > +		       PANEL_NAME_ANY, _quirks)
> > +
> 
> I'm not sure why this has to change. It's not explained in the commit
> message.

The reason for the change is to add a way to define quirks based on the
panel name, also referred to in the commit message. I can drop it from
this patchset, adding it only later.

> 
> >  static const struct edid_quirk {
> >  	const struct drm_edid_ident ident;
> >  	u32 quirks;
> > @@ -248,6 +254,9 @@ static const struct edid_quirk {
> >  	EDID_QUIRK('A', 'U', 'O', 0x1111, EDID_QUIRK_NON_DESKTOP),
> 
> Don't we want the other quirk list also be this concise?

The quirk defined by the standard is panel name based. Imo the new macro
to add a quirk should allow for matching any panel ID _or_ any panel
name and I came up with the way above. But I'll drop it from this
patchset and use only EDID_QUIRK adding the DPCD probe quirk needed for
now.

> >  };
> >  
> > +static const struct edid_quirk global_edid_quirk_list[] = {
> > +};
> > +
> >  /*
> >   * Autogenerated from the DMT spec.
> >   * This table is copied from xfree86/modes/xf86EdidModes.c.
> > @@ -2937,13 +2946,14 @@ EXPORT_SYMBOL(drm_edid_duplicate);
> >   *
> >   * Return: A u32 represents the quirks to apply.
> >   */
> > -static u32 edid_get_quirks(const struct drm_edid *drm_edid)
> > +static u32 edid_get_quirks(const struct drm_edid *drm_edid,
> > +			   const struct edid_quirk *quirk_list, int quirk_list_size)
> >  {
> >  	const struct edid_quirk *quirk;
> >  	int i;
> >  
> > -	for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
> > -		quirk = &edid_quirk_list[i];
> > +	for (i = 0; i < quirk_list_size; i++) {
> > +		quirk = &quirk_list[i];
> >  		if (drm_edid_match(drm_edid, &quirk->ident))
> >  			return quirk->quirks;
> >  	}
> > @@ -6614,6 +6624,7 @@ static void drm_reset_display_info(struct drm_connector *connector)
> >  	info->vics_len = 0;
> >  
> >  	info->quirks = 0;
> > +	info->global_quirks = 0;
> 
> So I am not sure if we really need or want two lists.
> 
> I think we could have
> 
> drm_edid.h:
> 
> enum drm_edid_quirk {
> 	/* ... */
> 	DRM_EDID_QUIRK_MAX,
> };
> 
> drm_edid.c:
> 
> enum drm_edid_internal_quirk {
> 	FOO_QUIRK = DRM_EDID_QUIRK_MAX,
>         /* etc ... */
> };
> 
> And just make info->quirks big enough. I think it feels simpler overall.

The internal quirks are not listed in an enum. That conversion is a
bigger change, but I agree it's a better way to define quirks. Did that
now, will follow up with it, this also allows using a single quirk list
as you suggested.

> >  
> >  	info->source_physical_address = CEC_PHYS_ADDR_INVALID;
> >  }
> > @@ -6660,7 +6671,10 @@ static void update_display_info(struct drm_connector *connector,
> >  
> >  	edid = drm_edid->edid;
> >  
> > -	info->quirks = edid_get_quirks(drm_edid);
> > +	info->quirks = edid_get_quirks(drm_edid, edid_quirk_list,
> > +				       ARRAY_SIZE(edid_quirk_list));
> > +	info->global_quirks = edid_get_quirks(drm_edid, global_edid_quirk_list,
> > +					      ARRAY_SIZE(global_edid_quirk_list));
> >  
> >  	info->width_mm = edid->width_cm * 10;
> >  	info->height_mm = edid->height_cm * 10;
> > @@ -7566,3 +7580,9 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid)
> >  		drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
> >  }
> >  EXPORT_SYMBOL(drm_edid_is_digital);
> > +
> > +bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk)
> > +{
> > +	return connector->display_info.global_quirks & BIT(quirk);
> > +}
> > +EXPORT_SYMBOL(drm_edid_has_quirk);
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 73903c3c842f3..996ecf229f8c7 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -847,6 +847,11 @@ struct drm_display_info {
> >  	 */
> >  	u32 quirks;
> >  
> > +	/**
> > +	 * @global_quirks: EDID based quirks. Can be queried by DRM core and drivers.
> 
> Might mention that you should not access directly, but using
> drm_edid_has_quirk().

Ok, will add a comment to quirks about this.

> > +	 */
> > +	u32 global_quirks;
> > +
> >  	/**
> >  	 * @source_physical_address: Source Physical Address from HDMI
> >  	 * Vendor-Specific Data Block, for CEC usage.
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index b38409670868d..3d8e168521c82 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -109,6 +109,10 @@ struct detailed_data_string {
> >  #define DRM_EDID_CVT_FLAGS_STANDARD_BLANKING (1 << 3)
> >  #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
> >  
> > +enum drm_edid_quirk {
> > +	DRM_EDID_QUIRK_NONE,
> > +};
> > +
> >  struct detailed_data_monitor_range {
> >  	u8 min_vfreq;
> >  	u8 max_vfreq;
> > @@ -476,5 +480,6 @@ void drm_edid_print_product_id(struct drm_printer *p,
> >  u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
> >  bool drm_edid_match(const struct drm_edid *drm_edid,
> >  		    const struct drm_edid_ident *ident);
> > +bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk);
> >  
> >  #endif /* __DRM_EDID_H__ */
> 
> -- 
> Jani Nikula, Intel

[PATCH 3/4] drm/dp: Add an EDID quirk for the DPCD register access probe

[Imre Deak]

Reading DPCD registers has side-effects and some of these can cause a
problem for instance during link training. Based on this it's better to
avoid the probing quirk done before each DPCD register read, limiting
this to the monitor which requires it. Add an EDID quirk for this. Leave
the quirk enabled by default, allowing it to be disabled after the
monitor is detected.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c | 15 ++++++++++++++-
 drivers/gpu/drm/drm_edid.c              |  3 +++
 include/drm/display/drm_dp_helper.h     |  6 ++++++
 include/drm/drm_edid.h                  |  3 ++-
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index dc622c78db9d4..4f2e919c6f7a6 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -691,6 +691,19 @@ void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered)
 }
 EXPORT_SYMBOL(drm_dp_dpcd_set_powered);
 
+/**
+ * drm_dp_dpcd_set_dpcd_probe_quirk() - Set whether a probing before DPCD access is done
+ * @aux: DisplayPort AUX channel
+ * @enable: Enable the probing if required
+ */
+void drm_dp_dpcd_set_probe_quirk(struct drm_dp_aux *aux, bool enable)
+{
+	mutex_lock(&aux->hw_mutex);
+	aux->dpcd_probe_disabled = !enable;
+	mutex_unlock(&aux->hw_mutex);
+}
+EXPORT_SYMBOL(drm_dp_dpcd_set_probe_quirk);
+
 /**
  * drm_dp_dpcd_read() - read a series of bytes from the DPCD
  * @aux: DisplayPort AUX channel (SST or MST)
@@ -724,7 +737,7 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 	 * We just have to do it before any DPCD access and hope that the
 	 * monitor doesn't power down exactly after the throw away read.
 	 */
-	if (!aux->is_remote) {
+	if (!aux->is_remote && !aux->dpcd_probe_disabled) {
 		ret = drm_dp_dpcd_probe(aux, DP_LANE0_1_STATUS);
 		if (ret < 0)
 			return ret;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e867315253493..9250b425ae230 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -255,6 +255,9 @@ static const struct edid_quirk {
 };
 
 static const struct edid_quirk global_edid_quirk_list[] = {
+	/* HP ZR24w DP AUX DPCD access requires probing to prevent corruption. */
+	DRM_EDID_QUIRK(drm_edid_encode_panel_id('H', 'W', 'P', 0x2869), PANEL_NAME_ANY,
+		       BIT(DRM_EDID_QUIRK_DP_DPCD_PROBE)),
 };
 
 /*
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index e4ca35143ff96..75fa9548aefa0 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -523,10 +523,16 @@ struct drm_dp_aux {
 	 * @no_zero_sized: If the hw can't use zero sized transfers (NVIDIA)
 	 */
 	bool no_zero_sized;
+
+	/**
+	 * @dpcd_probe_disabled: If probing before a DPCD access is disabled.
+	 */
+	bool dpcd_probe_disabled;
 };
 
 int drm_dp_dpcd_probe(struct drm_dp_aux *aux, unsigned int offset);
 void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered);
+void drm_dp_dpcd_set_probe_quirk(struct drm_dp_aux *aux, bool enable);
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 			 void *buffer, size_t size);
 ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 3d8e168521c82..a878805c81d97 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -110,7 +110,8 @@ struct detailed_data_string {
 #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
 
 enum drm_edid_quirk {
-	DRM_EDID_QUIRK_NONE,
+	/* Do a dummy read before DPCD accesses, to prevent corruption. */
+	DRM_EDID_QUIRK_DP_DPCD_PROBE,
 };
 
 struct detailed_data_monitor_range {
-- 
2.44.2

[PATCH v2 3/4] drm/dp: Add an EDID quirk for the DPCD register access probe

[Imre Deak]

Reading DPCD registers has side-effects and some of these can cause a
problem for instance during link training. Based on this it's better to
avoid the probing quirk done before each DPCD register read, limiting
this to the monitor which requires it. Add an EDID quirk for this. Leave
the quirk enabled by default, allowing it to be disabled after the
monitor is detected.

v2: Fix lockdep wrt. drm_dp_aux::hw_mutex when calling
    drm_dp_dpcd_set_probe_quirk() with a dependent lock already held.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c | 13 ++++++++++++-
 drivers/gpu/drm/drm_edid.c              |  3 +++
 include/drm/display/drm_dp_helper.h     |  6 ++++++
 include/drm/drm_edid.h                  |  3 ++-
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index dc622c78db9d4..4dad677ac6ebe 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -691,6 +691,17 @@ void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered)
 }
 EXPORT_SYMBOL(drm_dp_dpcd_set_powered);
 
+/**
+ * drm_dp_dpcd_set_dpcd_probe_quirk() - Set whether a probing before DPCD access is done
+ * @aux: DisplayPort AUX channel
+ * @enable: Enable the probing if required
+ */
+void drm_dp_dpcd_set_probe_quirk(struct drm_dp_aux *aux, bool enable)
+{
+	WRITE_ONCE(aux->dpcd_probe_disabled, !enable);
+}
+EXPORT_SYMBOL(drm_dp_dpcd_set_probe_quirk);
+
 /**
  * drm_dp_dpcd_read() - read a series of bytes from the DPCD
  * @aux: DisplayPort AUX channel (SST or MST)
@@ -724,7 +735,7 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 	 * We just have to do it before any DPCD access and hope that the
 	 * monitor doesn't power down exactly after the throw away read.
 	 */
-	if (!aux->is_remote) {
+	if (!aux->is_remote && !READ_ONCE(aux->dpcd_probe_disabled)) {
 		ret = drm_dp_dpcd_probe(aux, DP_LANE0_1_STATUS);
 		if (ret < 0)
 			return ret;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e867315253493..9250b425ae230 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -255,6 +255,9 @@ static const struct edid_quirk {
 };
 
 static const struct edid_quirk global_edid_quirk_list[] = {
+	/* HP ZR24w DP AUX DPCD access requires probing to prevent corruption. */
+	DRM_EDID_QUIRK(drm_edid_encode_panel_id('H', 'W', 'P', 0x2869), PANEL_NAME_ANY,
+		       BIT(DRM_EDID_QUIRK_DP_DPCD_PROBE)),
 };
 
 /*
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index e4ca35143ff96..75fa9548aefa0 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -523,10 +523,16 @@ struct drm_dp_aux {
 	 * @no_zero_sized: If the hw can't use zero sized transfers (NVIDIA)
 	 */
 	bool no_zero_sized;
+
+	/**
+	 * @dpcd_probe_disabled: If probing before a DPCD access is disabled.
+	 */
+	bool dpcd_probe_disabled;
 };
 
 int drm_dp_dpcd_probe(struct drm_dp_aux *aux, unsigned int offset);
 void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered);
+void drm_dp_dpcd_set_probe_quirk(struct drm_dp_aux *aux, bool enable);
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 			 void *buffer, size_t size);
 ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 3d8e168521c82..a878805c81d97 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -110,7 +110,8 @@ struct detailed_data_string {
 #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
 
 enum drm_edid_quirk {
-	DRM_EDID_QUIRK_NONE,
+	/* Do a dummy read before DPCD accesses, to prevent corruption. */
+	DRM_EDID_QUIRK_DP_DPCD_PROBE,
 };
 
 struct detailed_data_monitor_range {
-- 
2.44.2

Re: [PATCH v2 3/4] drm/dp: Add an EDID quirk for the DPCD register access probe

[Jani Nikula]

On Tue, 03 Jun 2025, Imre Deak <imre.deak@intel.com> wrote:
> Reading DPCD registers has side-effects and some of these can cause a
> problem for instance during link training. Based on this it's better to
> avoid the probing quirk done before each DPCD register read, limiting
> this to the monitor which requires it. Add an EDID quirk for this. Leave
> the quirk enabled by default, allowing it to be disabled after the
> monitor is detected.
>
> v2: Fix lockdep wrt. drm_dp_aux::hw_mutex when calling
>     drm_dp_dpcd_set_probe_quirk() with a dependent lock already held.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c | 13 ++++++++++++-
>  drivers/gpu/drm/drm_edid.c              |  3 +++
>  include/drm/display/drm_dp_helper.h     |  6 ++++++
>  include/drm/drm_edid.h                  |  3 ++-
>  4 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index dc622c78db9d4..4dad677ac6ebe 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -691,6 +691,17 @@ void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered)
>  }
>  EXPORT_SYMBOL(drm_dp_dpcd_set_powered);
>  
> +/**
> + * drm_dp_dpcd_set_dpcd_probe_quirk() - Set whether a probing before DPCD access is done
> + * @aux: DisplayPort AUX channel
> + * @enable: Enable the probing if required
> + */
> +void drm_dp_dpcd_set_probe_quirk(struct drm_dp_aux *aux, bool enable)
> +{
> +	WRITE_ONCE(aux->dpcd_probe_disabled, !enable);
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_set_probe_quirk);

We don't use this yet, which feels a bit odd.

> +
>  /**
>   * drm_dp_dpcd_read() - read a series of bytes from the DPCD
>   * @aux: DisplayPort AUX channel (SST or MST)
> @@ -724,7 +735,7 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>  	 * We just have to do it before any DPCD access and hope that the
>  	 * monitor doesn't power down exactly after the throw away read.
>  	 */
> -	if (!aux->is_remote) {
> +	if (!aux->is_remote && !READ_ONCE(aux->dpcd_probe_disabled)) {

I think it would be worth it to add a small helper to decide whether to
do a dpcd probe. It would include the ->is_remote check as well, and the
big comment could be moved there, simplifying drm_dp_dpcd_read().

>  		ret = drm_dp_dpcd_probe(aux, DP_LANE0_1_STATUS);
>  		if (ret < 0)
>  			return ret;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e867315253493..9250b425ae230 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -255,6 +255,9 @@ static const struct edid_quirk {
>  };
>  
>  static const struct edid_quirk global_edid_quirk_list[] = {
> +	/* HP ZR24w DP AUX DPCD access requires probing to prevent corruption. */
> +	DRM_EDID_QUIRK(drm_edid_encode_panel_id('H', 'W', 'P', 0x2869), PANEL_NAME_ANY,
> +		       BIT(DRM_EDID_QUIRK_DP_DPCD_PROBE)),

So I think we should keep using EDID_QUIRK(), and maybe add
EDID_QUIRK_NAME() companion to also assign a name != NULL, so we don't
have to pass PANEL_NAME_ANY to all uses. Less verbose that way I think.

>  };
>  
>  /*
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index e4ca35143ff96..75fa9548aefa0 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -523,10 +523,16 @@ struct drm_dp_aux {
>  	 * @no_zero_sized: If the hw can't use zero sized transfers (NVIDIA)
>  	 */
>  	bool no_zero_sized;
> +
> +	/**
> +	 * @dpcd_probe_disabled: If probing before a DPCD access is disabled.
> +	 */
> +	bool dpcd_probe_disabled;

Is this a negative just so it's false by default?

>  };
>  
>  int drm_dp_dpcd_probe(struct drm_dp_aux *aux, unsigned int offset);
>  void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered);
> +void drm_dp_dpcd_set_probe_quirk(struct drm_dp_aux *aux, bool enable);
>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>  			 void *buffer, size_t size);
>  ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 3d8e168521c82..a878805c81d97 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -110,7 +110,8 @@ struct detailed_data_string {
>  #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
>  
>  enum drm_edid_quirk {
> -	DRM_EDID_QUIRK_NONE,
> +	/* Do a dummy read before DPCD accesses, to prevent corruption. */
> +	DRM_EDID_QUIRK_DP_DPCD_PROBE,
>  };
>  
>  struct detailed_data_monitor_range {

-- 
Jani Nikula, Intel

Re: [PATCH v2 3/4] drm/dp: Add an EDID quirk for the DPCD register access probe

[Imre Deak]

On Wed, Jun 04, 2025 at 01:11:08PM +0300, Jani Nikula wrote:
> On Tue, 03 Jun 2025, Imre Deak <imre.deak@intel.com> wrote:
> > Reading DPCD registers has side-effects and some of these can cause a
> > problem for instance during link training. Based on this it's better to
> > avoid the probing quirk done before each DPCD register read, limiting
> > this to the monitor which requires it. Add an EDID quirk for this. Leave
> > the quirk enabled by default, allowing it to be disabled after the
> > monitor is detected.
> >
> > v2: Fix lockdep wrt. drm_dp_aux::hw_mutex when calling
> >     drm_dp_dpcd_set_probe_quirk() with a dependent lock already held.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_helper.c | 13 ++++++++++++-
> >  drivers/gpu/drm/drm_edid.c              |  3 +++
> >  include/drm/display/drm_dp_helper.h     |  6 ++++++
> >  include/drm/drm_edid.h                  |  3 ++-
> >  4 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> > index dc622c78db9d4..4dad677ac6ebe 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -691,6 +691,17 @@ void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered)
> >  }
> >  EXPORT_SYMBOL(drm_dp_dpcd_set_powered);
> >  
> > +/**
> > + * drm_dp_dpcd_set_dpcd_probe_quirk() - Set whether a probing before DPCD access is done
> > + * @aux: DisplayPort AUX channel
> > + * @enable: Enable the probing if required
> > + */
> > +void drm_dp_dpcd_set_probe_quirk(struct drm_dp_aux *aux, bool enable)
> > +{
> > +	WRITE_ONCE(aux->dpcd_probe_disabled, !enable);
> > +}
> > +EXPORT_SYMBOL(drm_dp_dpcd_set_probe_quirk);
> 
> We don't use this yet, which feels a bit odd.

It's used in the next driver patch and I wanted to keep the DRM core and
driver changes separate.

> > +
> >  /**
> >   * drm_dp_dpcd_read() - read a series of bytes from the DPCD
> >   * @aux: DisplayPort AUX channel (SST or MST)
> > @@ -724,7 +735,7 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> >  	 * We just have to do it before any DPCD access and hope that the
> >  	 * monitor doesn't power down exactly after the throw away read.
> >  	 */
> > -	if (!aux->is_remote) {
> > +	if (!aux->is_remote && !READ_ONCE(aux->dpcd_probe_disabled)) {
> 
> I think it would be worth it to add a small helper to decide whether to
> do a dpcd probe. It would include the ->is_remote check as well, and the
> big comment could be moved there, simplifying drm_dp_dpcd_read().

Ok, can do this.

> 
> >  		ret = drm_dp_dpcd_probe(aux, DP_LANE0_1_STATUS);
> >  		if (ret < 0)
> >  			return ret;
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index e867315253493..9250b425ae230 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -255,6 +255,9 @@ static const struct edid_quirk {
> >  };
> >  
> >  static const struct edid_quirk global_edid_quirk_list[] = {
> > +	/* HP ZR24w DP AUX DPCD access requires probing to prevent corruption. */
> > +	DRM_EDID_QUIRK(drm_edid_encode_panel_id('H', 'W', 'P', 0x2869), PANEL_NAME_ANY,
> > +		       BIT(DRM_EDID_QUIRK_DP_DPCD_PROBE)),
> 
> So I think we should keep using EDID_QUIRK(), and maybe add
> EDID_QUIRK_NAME() companion to also assign a name != NULL, so we don't
> have to pass PANEL_NAME_ANY to all uses. Less verbose that way I think.

Imo one macro should accept a quirk matching any panel ID or any panel
name; but for now I can drop the change adding a way for that and use
EDID_QUIRK() here.

> 
> >  };
> >  
> >  /*
> > diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> > index e4ca35143ff96..75fa9548aefa0 100644
> > --- a/include/drm/display/drm_dp_helper.h
> > +++ b/include/drm/display/drm_dp_helper.h
> > @@ -523,10 +523,16 @@ struct drm_dp_aux {
> >  	 * @no_zero_sized: If the hw can't use zero sized transfers (NVIDIA)
> >  	 */
> >  	bool no_zero_sized;
> > +
> > +	/**
> > +	 * @dpcd_probe_disabled: If probing before a DPCD access is disabled.
> > +	 */
> > +	bool dpcd_probe_disabled;
> 
> Is this a negative just so it's false by default?

Yes.

> 
> >  };
> >  
> >  int drm_dp_dpcd_probe(struct drm_dp_aux *aux, unsigned int offset);
> >  void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered);
> > +void drm_dp_dpcd_set_probe_quirk(struct drm_dp_aux *aux, bool enable);
> >  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> >  			 void *buffer, size_t size);
> >  ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 3d8e168521c82..a878805c81d97 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -110,7 +110,8 @@ struct detailed_data_string {
> >  #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
> >  
> >  enum drm_edid_quirk {
> > -	DRM_EDID_QUIRK_NONE,
> > +	/* Do a dummy read before DPCD accesses, to prevent corruption. */
> > +	DRM_EDID_QUIRK_DP_DPCD_PROBE,
> >  };
> >  
> >  struct detailed_data_monitor_range {
> 
> -- 
> Jani Nikula, Intel

[PATCH 4/4] drm/i915/dp: Disable the AUX DPCD probe quirk if it's not required

[Imre Deak]

Reading DPCD registers has side-effects and some of these can cause a
problem for instance during link training. Based on this it's better to
avoid the probing quirk done before each DPCD register read, limiting
this to the monitor which requires it. The only known problematic
monitor is an external SST sink, so keep the quirk disabled always for
eDP and MST sinks. Reenable the quirk after a hotplug event and after
resuming from a power state without hotplug support, until the
subsequent EDID based detection.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c      | 11 +++++++++--
 drivers/gpu/drm/i915/display/intel_dp_aux.c  |  2 ++
 drivers/gpu/drm/i915/display/intel_hotplug.c | 10 ++++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 208a953b04a2f..d65a18fc1aeb9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5747,6 +5747,11 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 	/* Below we depend on display info having been updated */
 	drm_edid_connector_update(&connector->base, drm_edid);
 
+	if (!intel_dp_is_edp(intel_dp))
+		drm_dp_dpcd_set_probe_quirk(&intel_dp->aux,
+					    drm_edid_has_quirk(&connector->base,
+							       DRM_EDID_QUIRK_DP_DPCD_PROBE));
+
 	vrr_capable = intel_vrr_is_capable(connector);
 	drm_dbg_kms(display->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n",
 		    connector->base.base.id, connector->base.name, str_yes_no(vrr_capable));
@@ -5881,6 +5886,7 @@ intel_dp_detect(struct drm_connector *_connector,
 	intel_dp_print_rates(intel_dp);
 
 	if (intel_dp->is_mst) {
+		drm_dp_dpcd_set_probe_quirk(&intel_dp->aux, false);
 		/*
 		 * If we are in MST mode then this connector
 		 * won't appear connected or have anything
@@ -6321,10 +6327,11 @@ intel_dp_hpd_pulse(struct intel_digital_port *dig_port, bool long_hpd)
 	 * complete the DP tunnel BW request for the latter connector/encoder
 	 * waiting for this encoder's DPRX read, perform a dummy read here.
 	 */
-	if (long_hpd)
+	if (long_hpd) {
+		drm_dp_dpcd_set_probe_quirk(&intel_dp->aux, true);
+
 		intel_dp_read_dprx_caps(intel_dp, dpcd);
 
-	if (long_hpd) {
 		intel_dp->reset_link_params = true;
 		intel_dp_invalidate_source_oui(intel_dp);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
index bf8e8e0cc19c9..410252ba6fd16 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
@@ -835,6 +835,8 @@ void intel_dp_aux_init(struct intel_dp *intel_dp)
 
 	intel_dp->aux.transfer = intel_dp_aux_transfer;
 	cpu_latency_qos_add_request(&intel_dp->pm_qos, PM_QOS_DEFAULT_VALUE);
+
+	drm_dp_dpcd_set_probe_quirk(&intel_dp->aux, !intel_dp_is_edp(intel_dp));
 }
 
 static enum aux_ch default_aux_ch(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
index 74fe398663d63..1093c6c3714c0 100644
--- a/drivers/gpu/drm/i915/display/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
@@ -33,6 +33,7 @@
 #include "intel_display_core.h"
 #include "intel_display_rpm.h"
 #include "intel_display_types.h"
+#include "intel_dp.h"
 #include "intel_hdcp.h"
 #include "intel_hotplug.h"
 #include "intel_hotplug_irq.h"
@@ -906,9 +907,18 @@ void intel_hpd_poll_enable(struct intel_display *display)
  */
 void intel_hpd_poll_disable(struct intel_display *display)
 {
+	struct intel_encoder *encoder;
+
 	if (!HAS_DISPLAY(display))
 		return;
 
+	for_each_intel_dp(display->drm, encoder) {
+		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+
+		if (!intel_dp_is_edp(intel_dp))
+			drm_dp_dpcd_set_probe_quirk(&intel_dp->aux, true);
+	}
+
 	WRITE_ONCE(display->hotplug.poll_enabled, false);
 
 	spin_lock_irq(&display->irq.lock);
-- 
2.44.2

Re: [PATCH 4/4] drm/i915/dp: Disable the AUX DPCD probe quirk if it's not required

[Jani Nikula]

On Tue, 03 Jun 2025, Imre Deak <imre.deak@intel.com> wrote:
> Reading DPCD registers has side-effects and some of these can cause a
> problem for instance during link training. Based on this it's better to
> avoid the probing quirk done before each DPCD register read, limiting
> this to the monitor which requires it. The only known problematic
> monitor is an external SST sink, so keep the quirk disabled always for
> eDP and MST sinks. Reenable the quirk after a hotplug event and after
> resuming from a power state without hotplug support, until the
> subsequent EDID based detection.

It's a bummer we have to do this in so many places. :(


>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c      | 11 +++++++++--
>  drivers/gpu/drm/i915/display/intel_dp_aux.c  |  2 ++
>  drivers/gpu/drm/i915/display/intel_hotplug.c | 10 ++++++++++
>  3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 208a953b04a2f..d65a18fc1aeb9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5747,6 +5747,11 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>  	/* Below we depend on display info having been updated */
>  	drm_edid_connector_update(&connector->base, drm_edid);
>  
> +	if (!intel_dp_is_edp(intel_dp))
> +		drm_dp_dpcd_set_probe_quirk(&intel_dp->aux,
> +					    drm_edid_has_quirk(&connector->base,
> +							       DRM_EDID_QUIRK_DP_DPCD_PROBE));
> +
>  	vrr_capable = intel_vrr_is_capable(connector);
>  	drm_dbg_kms(display->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n",
>  		    connector->base.base.id, connector->base.name, str_yes_no(vrr_capable));
> @@ -5881,6 +5886,7 @@ intel_dp_detect(struct drm_connector *_connector,
>  	intel_dp_print_rates(intel_dp);
>  
>  	if (intel_dp->is_mst) {
> +		drm_dp_dpcd_set_probe_quirk(&intel_dp->aux, false);
>  		/*
>  		 * If we are in MST mode then this connector
>  		 * won't appear connected or have anything
> @@ -6321,10 +6327,11 @@ intel_dp_hpd_pulse(struct intel_digital_port *dig_port, bool long_hpd)
>  	 * complete the DP tunnel BW request for the latter connector/encoder
>  	 * waiting for this encoder's DPRX read, perform a dummy read here.
>  	 */
> -	if (long_hpd)
> +	if (long_hpd) {
> +		drm_dp_dpcd_set_probe_quirk(&intel_dp->aux, true);
> +
>  		intel_dp_read_dprx_caps(intel_dp, dpcd);
>  
> -	if (long_hpd) {
>  		intel_dp->reset_link_params = true;
>  		intel_dp_invalidate_source_oui(intel_dp);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> index bf8e8e0cc19c9..410252ba6fd16 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -835,6 +835,8 @@ void intel_dp_aux_init(struct intel_dp *intel_dp)
>  
>  	intel_dp->aux.transfer = intel_dp_aux_transfer;
>  	cpu_latency_qos_add_request(&intel_dp->pm_qos, PM_QOS_DEFAULT_VALUE);
> +
> +	drm_dp_dpcd_set_probe_quirk(&intel_dp->aux, !intel_dp_is_edp(intel_dp));
>  }
>  
>  static enum aux_ch default_aux_ch(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
> index 74fe398663d63..1093c6c3714c0 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> @@ -33,6 +33,7 @@
>  #include "intel_display_core.h"
>  #include "intel_display_rpm.h"
>  #include "intel_display_types.h"
> +#include "intel_dp.h"
>  #include "intel_hdcp.h"
>  #include "intel_hotplug.h"
>  #include "intel_hotplug_irq.h"
> @@ -906,9 +907,18 @@ void intel_hpd_poll_enable(struct intel_display *display)
>   */
>  void intel_hpd_poll_disable(struct intel_display *display)
>  {
> +	struct intel_encoder *encoder;
> +
>  	if (!HAS_DISPLAY(display))
>  		return;
>  
> +	for_each_intel_dp(display->drm, encoder) {
> +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> +		if (!intel_dp_is_edp(intel_dp))
> +			drm_dp_dpcd_set_probe_quirk(&intel_dp->aux, true);
> +	}
> +
>  	WRITE_ONCE(display->hotplug.poll_enabled, false);
>  
>  	spin_lock_irq(&display->irq.lock);

-- 
Jani Nikula, Intel