[PATCH 00/13] drm/msm/dp: Add static HDR support for DP and eDP

[PATCH 00/13] drm/msm/dp: Add static HDR support for DP and eDP

[Xilin Wu]

This series adds standard KMS static HDR metadata and BT.2020 RGB
signalling to MSM DP and eDP, with HDR10 as the intended use case. It uses
HDR_OUTPUT_METADATA, Colorspace and max bpc. Userspace supplies the HDR
pixel content; DSC, dynamic HDR and tone mapping are outside this series.

Patch 1 allows max bpc registration before connector state allocation and
initializes the corresponding state defaults. Patch 2 prepares generic
SDP slots. Patches 3-5 establish PHY power ownership, serialize HPD state
updates and add complete stream enable rollback, including AUX recovery
for a connected external peer. New error returns are introduced together
with their cleanup paths.

Patches 6-8 add deferred link-status notification, capability cache
validity and stream reconstruction for modesets leaving eDP self refresh.
Patches 9-10 add atomic output depth selection and BT.2020 RGB signalling.
The BT.2020 patch includes its own PSR disable/restore handling.

Patch 11 independently fixes the uninitialized debugfs connector pointer.
Patch 12 serializes SDR video-test state changes with mode programming.
Patch 13 adds complete static HDR support: metadata validation and packing,
10 bpc enforcement, Generic2 programming and teardown, stream restoration,
PSR exclusion and arbitration with video-pattern tests. A test request
arriving after atomic check cannot replace the checked HDR output depth.

This series is tested on a Lenovo Yoga Slim 7x using x1e80100.

Signed-off-by: Xilin Wu <sophon@radxa.com>
---
Xilin Wu (13):
      drm/atomic: Handle max bpc properties before connector state allocation
      drm/msm/dp: Support multiple generic SDP slots
      drm/msm/dp: Track PHY power ownership
      drm/msm/dp: Serialize HPD state updates
      drm/msm/dp: Unwind resources when enabling a stream fails
      drm/msm/dp: Report stream enable failures through link status
      drm/msm/dp: Track validity of cached sink link capabilities
      drm/msm/dp: Rebuild the eDP stream on modesets leaving self refresh
      drm/msm/dp: Track output bit depth in bridge atomic state
      drm/msm/dp: Signal BT.2020 RGB on DP and eDP
      drm/msm/dp: Initialize the debugfs connector pointer
      drm/msm/dp: Serialize video test state changes
      drm/msm/dp: Add static HDR metadata support for DP and eDP

 drivers/gpu/drm/drm_atomic_state_helper.c |   5 +
 drivers/gpu/drm/drm_connector.c           |   6 +-
 drivers/gpu/drm/msm/Kconfig               |   1 +
 drivers/gpu/drm/msm/dp/dp_ctrl.c          | 127 +++++++++---
 drivers/gpu/drm/msm/dp/dp_ctrl.h          |   4 +-
 drivers/gpu/drm/msm/dp/dp_debug.c         |  26 +--
 drivers/gpu/drm/msm/dp/dp_debug.h         |   8 +-
 drivers/gpu/drm/msm/dp/dp_display.c       | 319 ++++++++++++++++++++++++++----
 drivers/gpu/drm/msm/dp/dp_display.h       |   8 +-
 drivers/gpu/drm/msm/dp/dp_drm.c           | 118 ++++++++++-
 drivers/gpu/drm/msm/dp/dp_drm.h           |  12 ++
 drivers/gpu/drm/msm/dp/dp_panel.c         | 103 +++++-----
 drivers/gpu/drm/msm/dp/dp_panel.h         |   8 +
 drivers/gpu/drm/msm/dp/dp_reg.h           |   4 +
 drivers/gpu/drm/msm/dp/dp_utils.c         | 121 +++++++++++-
 drivers/gpu/drm/msm/dp/dp_utils.h         |  21 +-
 16 files changed, 738 insertions(+), 153 deletions(-)
---
base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
change-id: 20260930-msm-dp-hdr10-d84e11282319

Best regards,
--  
Xilin Wu <sophon@radxa.com>

[PATCH 01/13] drm/atomic: Handle max bpc properties before connector state allocation

[Xilin Wu]

Bridge connectors can attach the max bpc property before their atomic
state is allocated by drm_mode_config_reset(). Unconditionally updating
connector->state in drm_connector_attach_max_bpc_property() dereferences
a NULL pointer in this case. Adding max bpc support to MSM DP and eDP
exposes this during display initialization, before a framebuffer console
is available.

Only update an existing state when attaching the property. Initialize
max_requested_bpc and max_bpc from the property range when creating a
connector state so that deferred allocation and subsequent resets both
restore the default. Avoid leaving the bit depth at zero after reset,
which would otherwise cause valid display configurations to be rejected.

With MSM DP max bpc support applied, this also restores boot on the Lenovo
Yoga Slim 7x.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 5 +++++
 drivers/gpu/drm/drm_connector.c           | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index a2ef272e9f27..91d60bc7a2b2 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -34,6 +34,7 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_plane.h>
 #include <drm/drm_print.h>
+#include <drm/drm_property.h>
 #include <drm/drm_vblank.h>
 #include <drm/drm_writeback.h>
 
@@ -495,6 +496,10 @@ __drm_atomic_helper_connector_state_init(struct drm_connector_state *conn_state,
 					 struct drm_connector *connector)
 {
 	conn_state->connector = connector;
+	if (connector->max_bpc_property) {
+		conn_state->max_requested_bpc = connector->max_bpc_property->values[1];
+		conn_state->max_bpc = conn_state->max_requested_bpc;
+	}
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_state_init);
 
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 8b4baed060f3..34c30469f405 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2886,8 +2886,10 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 	}
 
 	drm_object_attach_property(&connector->base, prop, max);
-	connector->state->max_requested_bpc = max;
-	connector->state->max_bpc = max;
+	if (connector->state) {
+		connector->state->max_requested_bpc = max;
+		connector->state->max_bpc = max;
+	}
 
 	return 0;
 }

-- 
2.55.0

Re: [PATCH 01/13] drm/atomic: Handle max bpc properties before connector state allocation

[Dmitry Baryshkov]

On Wed, Sep 30, 2026 at 08:41:45PM +0800, Xilin Wu wrote:
> Bridge connectors can attach the max bpc property before their atomic
> state is allocated by drm_mode_config_reset(). Unconditionally updating
> connector->state in drm_connector_attach_max_bpc_property() dereferences
> a NULL pointer in this case. Adding max bpc support to MSM DP and eDP
> exposes this during display initialization, before a framebuffer console
> is available.
> 
> Only update an existing state when attaching the property. Initialize
> max_requested_bpc and max_bpc from the property range when creating a
> connector state so that deferred allocation and subsequent resets both
> restore the default. Avoid leaving the bit depth at zero after reset,
> which would otherwise cause valid display configurations to be rejected.
> 
> With MSM DP max bpc support applied, this also restores boot on the Lenovo
> Yoga Slim 7x.

Hmm?

> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@radxa.com>

Can we rather update drm_bridge_connector_init() to register this
property in non-HDMI cases if max_bpc is more than 8?

> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c | 5 +++++
>  drivers/gpu/drm/drm_connector.c           | 6 ++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH 01/13] drm/atomic: Handle max bpc properties before connector state allocation

[Xilin Wu]

On 9/30/2026 9:26 PM, Dmitry Baryshkov wrote:
> On Wed, Sep 30, 2026 at 08:41:45PM +0800, Xilin Wu wrote:
>> Bridge connectors can attach the max bpc property before their atomic
>> state is allocated by drm_mode_config_reset(). Unconditionally updating
>> connector->state in drm_connector_attach_max_bpc_property() dereferences
>> a NULL pointer in this case. Adding max bpc support to MSM DP and eDP
>> exposes this during display initialization, before a framebuffer console
>> is available.
>>
>> Only update an existing state when attaching the property. Initialize
>> max_requested_bpc and max_bpc from the property range when creating a
>> connector state so that deferred allocation and subsequent resets both
>> restore the default. Avoid leaving the bit depth at zero after reset,
>> which would otherwise cause valid display configurations to be rejected.
>>
>> With MSM DP max bpc support applied, this also restores boot on the Lenovo
>> Yoga Slim 7x.
> 
> Hmm?

The boot failure occurred with the later max bpc patch in this series 
applied. I'll drop the "restores boot" sentence and describe this as 
preparatory support instead.

> 
>>
>> Assisted-by: LLM
>> Signed-off-by: Xilin Wu <sophon@radxa.com>
> 
> Can we rather update drm_bridge_connector_init() to register this
> property in non-HDMI cases if max_bpc is more than 8?
> 

Yes, that makes sense. I'll advertise the MSM DP/eDP capability through 
bridge->max_bpc and let drm_bridge_connector_init() attach the property 
for non-HDMI bridges with max_bpc > 8. I'll also handle state creation 
and bpc defaults there, following the HDMI path.

Thanks for the suggestion.


>> ---
>>   drivers/gpu/drm/drm_atomic_state_helper.c | 5 +++++
>>   drivers/gpu/drm/drm_connector.c           | 6 ++++--
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>
> 


-- 
Best regards,
Xilin Wu <sophon@radxa.com>

Re: [PATCH 01/13] drm/atomic: Handle max bpc properties before connector state allocation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2934 bytes --]

Hi,

On Wed, Sep 30, 2026 at 08:41:45PM +0800, Xilin Wu wrote:
> Bridge connectors can attach the max bpc property before their atomic
> state is allocated by drm_mode_config_reset(). Unconditionally updating
> connector->state in drm_connector_attach_max_bpc_property() dereferences
> a NULL pointer in this case. Adding max bpc support to MSM DP and eDP
> exposes this during display initialization, before a framebuffer console
> is available.
> 
> Only update an existing state when attaching the property. Initialize
> max_requested_bpc and max_bpc from the property range when creating a
> connector state so that deferred allocation and subsequent resets both
> restore the default. Avoid leaving the bit depth at zero after reset,
> which would otherwise cause valid display configurations to be rejected.
> 
> With MSM DP max bpc support applied, this also restores boot on the Lenovo
> Yoga Slim 7x.
> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@radxa.com>
> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c | 5 +++++
>  drivers/gpu/drm/drm_connector.c           | 6 ++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index a2ef272e9f27..91d60bc7a2b2 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -34,6 +34,7 @@
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_plane.h>
>  #include <drm/drm_print.h>
> +#include <drm/drm_property.h>
>  #include <drm/drm_vblank.h>
>  #include <drm/drm_writeback.h>
>  
> @@ -495,6 +496,10 @@ __drm_atomic_helper_connector_state_init(struct drm_connector_state *conn_state,
>  					 struct drm_connector *connector)
>  {
>  	conn_state->connector = connector;
> +	if (connector->max_bpc_property) {
> +		conn_state->max_requested_bpc = connector->max_bpc_property->values[1];
> +		conn_state->max_bpc = conn_state->max_requested_bpc;
> +	}

You should use drm_object_property_get_default_value() here

>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_state_init);
>  
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 8b4baed060f3..34c30469f405 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2886,8 +2886,10 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  	}
>  
>  	drm_object_attach_property(&connector->base, prop, max);
> -	connector->state->max_requested_bpc = max;
> -	connector->state->max_bpc = max;
> +	if (connector->state) {
> +		connector->state->max_requested_bpc = max;
> +		connector->state->max_bpc = max;
> +	}


Doesn't that also remove the new state allocation we have everywhere
(like in drmm_connector_hdmi_init, meson_encoder_hdmi_probe,
dw_hdmi_connector_create, etc.)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[PATCH 02/13] drm/msm/dp: Support multiple generic SDP slots

[Xilin Wu]

The SDP writer currently targets Generic0 for VSC packets. Static HDR
metadata needs a separate slot so that both packets can be sent on the
same stream.

Allow the SDP writer to address a generic packet slot, and define the
Generic2 registers needed for static HDR metadata. Use unaligned
little-endian loads for the payload words and accept a const SDP header.

The existing VSC packet continues to use Generic0 without changing its
layout or enable sequence.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_panel.c | 22 ++++++++++------------
 drivers/gpu/drm/msm/dp/dp_reg.h   |  4 ++++
 drivers/gpu/drm/msm/dp/dp_utils.c |  3 ++-
 drivers/gpu/drm/msm/dp/dp_utils.h |  3 ++-
 4 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index e76dad0f6663..fdf1a3f437ba 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -14,6 +14,7 @@
 
 #include <linux/io.h>
 #include <linux/types.h>
+#include <linux/unaligned.h>
 #include <asm/byteorder.h>
 
 #define DP_INTF_CONFIG_DATABUS_WIDEN     BIT(4)
@@ -442,22 +443,19 @@ void msm_dp_panel_clear_dsc_dto(struct msm_dp_panel *msm_dp_panel)
 	msm_dp_write_p0(panel, MMSS_DP_DSC_DTO, 0x0);
 }
 
-static void msm_dp_panel_send_vsc_sdp(struct msm_dp_panel_private *panel, struct dp_sdp *vsc_sdp)
+static void msm_dp_panel_send_sdp(struct msm_dp_panel_private *panel,
+				  const struct dp_sdp *sdp, u32 base)
 {
 	u32 header[2];
-	u32 val;
 	int i;
 
-	msm_dp_utils_pack_sdp_header(&vsc_sdp->sdp_header, header);
+	msm_dp_utils_pack_sdp_header(&sdp->sdp_header, header);
+	msm_dp_write_link(panel, base, header[0]);
+	msm_dp_write_link(panel, base + 4, header[1]);
 
-	msm_dp_write_link(panel, MMSS_DP_GENERIC0_0, header[0]);
-	msm_dp_write_link(panel, MMSS_DP_GENERIC0_1, header[1]);
-
-	for (i = 0; i < sizeof(vsc_sdp->db); i += 4) {
-		val = ((vsc_sdp->db[i]) | (vsc_sdp->db[i + 1] << 8) | (vsc_sdp->db[i + 2] << 16) |
-		       (vsc_sdp->db[i + 3] << 24));
-		msm_dp_write_link(panel, MMSS_DP_GENERIC0_2 + i, val);
-	}
+	for (i = 0; i < sizeof(sdp->db); i += 4)
+		msm_dp_write_link(panel, base + 8 + i,
+				  get_unaligned_le32(&sdp->db[i]));
 }
 
 static void msm_dp_panel_update_sdp(struct msm_dp_panel_private *panel)
@@ -487,7 +485,7 @@ void msm_dp_panel_enable_vsc_sdp(struct msm_dp_panel *msm_dp_panel, struct dp_sd
 	cfg2 |= GENERIC0_SDPSIZE_VALID;
 	msm_dp_write_link(panel, MMSS_DP_SDP_CFG2, cfg2);
 
-	msm_dp_panel_send_vsc_sdp(panel, vsc_sdp);
+	msm_dp_panel_send_sdp(panel, vsc_sdp, MMSS_DP_GENERIC0_0);
 
 	/* indicates presence of VSC (BIT(6) of MISC1) */
 	misc |= DP_MISC1_VSC_SDP;
diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h b/drivers/gpu/drm/msm/dp/dp_reg.h
index dda4b642c81b..6767a215b662 100644
--- a/drivers/gpu/drm/msm/dp/dp_reg.h
+++ b/drivers/gpu/drm/msm/dp/dp_reg.h
@@ -232,10 +232,12 @@
 #define MMSS_DP_AUDIO_CTRL_RESET		(0x00000214)
 
 #define MMSS_DP_SDP_CFG				(0x00000228)
+#define GEN2_SDP_EN				BIT(19)
 #define GEN0_SDP_EN				(0x00020000)
 #define MMSS_DP_SDP_CFG2			(0x0000022C)
 #define MMSS_DP_AUDIO_TIMESTAMP_0		(0x00000230)
 #define MMSS_DP_AUDIO_TIMESTAMP_1		(0x00000234)
+#define GENERIC2_SDPSIZE_VALID			BIT(20)
 #define GENERIC0_SDPSIZE_VALID			(0x00010000)
 
 #define MMSS_DP_AUDIO_STREAM_0			(0x00000240)
@@ -291,6 +293,8 @@
 #define MMSS_DP_GENERIC1_8			(0x00000348)
 #define MMSS_DP_GENERIC1_9			(0x0000034C)
 
+#define MMSS_DP_GENERIC2_0			(0x000003D8)
+
 #define MMSS_DP_VSCEXT_0			(0x000002D0)
 #define MMSS_DP_VSCEXT_1			(0x000002D4)
 #define MMSS_DP_VSCEXT_2			(0x000002D8)
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c b/drivers/gpu/drm/msm/dp/dp_utils.c
index 4a5ebb0c33b8..226b370ce8fe 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.c
+++ b/drivers/gpu/drm/msm/dp/dp_utils.c
@@ -74,7 +74,8 @@ u8 msm_dp_utils_calculate_parity(u32 data)
 	return parity_byte;
 }
 
-void msm_dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 header_buff[2])
+void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
+				  u32 header_buff[2])
 {
 	header_buff[0] = FIELD_PREP(HEADER_0_MASK, sdp_header->HB0) |
 		FIELD_PREP(PARITY_0_MASK, msm_dp_utils_calculate_parity(sdp_header->HB0)) |
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h
index 2e4f98a863c4..2fdb89efc57c 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.h
+++ b/drivers/gpu/drm/msm/dp/dp_utils.h
@@ -31,6 +31,7 @@
 u8 msm_dp_utils_get_g0_value(u8 data);
 u8 msm_dp_utils_get_g1_value(u8 data);
 u8 msm_dp_utils_calculate_parity(u32 data);
-void msm_dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 header_buff[2]);
+void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
+				  u32 header_buff[2]);
 
 #endif /* _DP_UTILS_H_ */

-- 
2.55.0

Re: [PATCH 02/13] drm/msm/dp: Support multiple generic SDP slots

[Dmitry Baryshkov]

On Wed, Sep 30, 2026 at 08:41:46PM +0800, Xilin Wu wrote:
> The SDP writer currently targets Generic0 for VSC packets. Static HDR
> metadata needs a separate slot so that both packets can be sent on the
> same stream.
> 
> Allow the SDP writer to address a generic packet slot, and define the
> Generic2 registers needed for static HDR metadata. Use unaligned
> little-endian loads for the payload words and accept a const SDP header.
> 
> The existing VSC packet continues to use Generic0 without changing its
> layout or enable sequence.
> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@radxa.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_panel.c | 22 ++++++++++------------
>  drivers/gpu/drm/msm/dp/dp_reg.h   |  4 ++++
>  drivers/gpu/drm/msm/dp/dp_utils.c |  3 ++-
>  drivers/gpu/drm/msm/dp/dp_utils.h |  3 ++-
>  4 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c b/drivers/gpu/drm/msm/dp/dp_utils.c
> index 4a5ebb0c33b8..226b370ce8fe 100644
> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
> @@ -74,7 +74,8 @@ u8 msm_dp_utils_calculate_parity(u32 data)
>  	return parity_byte;
>  }
>  
> -void msm_dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 header_buff[2])
> +void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
> +				  u32 header_buff[2])

Nit: separate commit for this one, please.

Feel free to use for both of them:



Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>



>  {
>  	header_buff[0] = FIELD_PREP(HEADER_0_MASK, sdp_header->HB0) |
>  		FIELD_PREP(PARITY_0_MASK, msm_dp_utils_calculate_parity(sdp_header->HB0)) |
> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h
> index 2e4f98a863c4..2fdb89efc57c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_utils.h
> +++ b/drivers/gpu/drm/msm/dp/dp_utils.h
> @@ -31,6 +31,7 @@
>  u8 msm_dp_utils_get_g0_value(u8 data);
>  u8 msm_dp_utils_get_g1_value(u8 data);
>  u8 msm_dp_utils_calculate_parity(u32 data);
> -void msm_dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 header_buff[2]);
> +void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
> +				  u32 header_buff[2]);
>  
>  #endif /* _DP_UTILS_H_ */
> 
> -- 
> 2.55.0
> 

-- 
With best wishes
Dmitry

[PATCH 03/13] drm/msm/dp: Track PHY power ownership

[Xilin Wu]

Mainlink restart and PHY test paths can power down the PHY before a later
cleanup attempts to release it again. The PHY core counts each power-off
call, so cleanup without an owned reference can underflow that count and
prevent a subsequent enable from calling the hardware power-on operation.

Record successful PHY power acquisition in the controller and route all
power-off paths through an ownership check. Keep the reference recorded
when power-off fails and avoid acquiring a second reference while it is
still held. Also skip releasing link clocks that are already disabled.

This establishes resource ownership for repeated cleanup. Error
propagation is left to the subsequent stream enable rollback change.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 59070f399e2d..f005421630c6 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -136,6 +136,7 @@ struct msm_dp_ctrl_private {
 
 	u32 hw_revision;
 
+	bool phy_powered;
 	bool core_clks_on;
 	bool link_clks_on;
 	bool stream_clks_on;
@@ -1813,6 +1814,9 @@ static void msm_dp_ctrl_link_clk_disable(struct msm_dp_ctrl *msm_dp_ctrl)
 
 	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
 
+	if (!ctrl->link_clks_on)
+		return;
+
 	clk_bulk_disable_unprepare(ctrl->num_link_clks, ctrl->link_clks);
 
 	ctrl->link_clks_on = false;
@@ -1824,6 +1828,23 @@ static void msm_dp_ctrl_link_clk_disable(struct msm_dp_ctrl *msm_dp_ctrl)
 		   str_on_off(ctrl->core_clks_on));
 }
 
+static int msm_dp_ctrl_phy_power_off(struct msm_dp_ctrl_private *ctrl)
+{
+	int ret;
+
+	if (!ctrl->phy_powered)
+		return 0;
+
+	ret = phy_power_off(ctrl->phy);
+	if (ret) {
+		DRM_ERROR("Failed to power off PHY: %d\n", ret);
+		return ret;
+	}
+
+	ctrl->phy_powered = false;
+	return 0;
+}
+
 static int msm_dp_ctrl_enable_mainlink_clocks(struct msm_dp_ctrl_private *ctrl,
 					      struct msm_dp_panel *panel)
 {
@@ -1836,7 +1857,11 @@ static int msm_dp_ctrl_enable_mainlink_clocks(struct msm_dp_ctrl_private *ctrl,
 	ctrl->phy_opts.dp.ssc = drm_dp_max_downspread(dpcd);
 
 	phy_configure(phy, &ctrl->phy_opts);
-	phy_power_on(phy);
+	if (!ctrl->phy_powered) {
+		ret = phy_power_on(phy);
+		if (!ret)
+			ctrl->phy_powered = true;
+	}
 
 	dev_pm_opp_set_rate(ctrl->dev, ctrl->link->link_params.rate * 1000);
 	ret = msm_dp_ctrl_link_clk_enable(&ctrl->msm_dp_ctrl);
@@ -1996,7 +2021,7 @@ static int msm_dp_ctrl_reinitialize_mainlink(struct msm_dp_ctrl_private *ctrl,
 	 */
 	msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl);
 
-	phy_power_off(phy);
+	msm_dp_ctrl_phy_power_off(ctrl);
 	/* hw recommended delay before re-enabling clocks */
 	msleep(20);
 
@@ -2022,7 +2047,7 @@ static int msm_dp_ctrl_deinitialize_mainlink(struct msm_dp_ctrl_private *ctrl,
 
 	msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl);
 
-	phy_power_off(phy);
+	msm_dp_ctrl_phy_power_off(ctrl);
 
 	/* aux channel down, reinit phy */
 	phy_exit(phy);
@@ -2643,10 +2668,8 @@ void msm_dp_ctrl_off_link(struct msm_dp_ctrl *msm_dp_ctrl,
 			  struct msm_dp_panel *panel)
 {
 	struct msm_dp_ctrl_private *ctrl;
-	struct phy *phy;
 
 	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
-	phy = ctrl->phy;
 
 	msm_dp_panel_disable_vsc_sdp(panel);
 
@@ -2656,7 +2679,7 @@ void msm_dp_ctrl_off_link(struct msm_dp_ctrl *msm_dp_ctrl,
 
 	msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl);
 
-	phy_power_off(phy);
+	msm_dp_ctrl_phy_power_off(ctrl);
 }
 
 irqreturn_t msm_dp_ctrl_isr(struct msm_dp_ctrl *msm_dp_ctrl,

-- 
2.55.0

Re: [PATCH 03/13] drm/msm/dp: Track PHY power ownership

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] msm/dp: TOCTOU race in PHY power ownership tracking causes power and clock leak
--

commit c9182f8e77cc8634e408d0bae0ac2751df04598b
Author: Xilin Wu <sophon@radxa.com>

drm/msm/dp: Track PHY power ownership

This commit introduces PHY power ownership tracking to prevent underflow
during repeated cleanup operations. It adds boolean flags to track PHY
power and link clock states and uses them to skip redundant enables or
disables.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 59070f399e2d3..f005421630c63 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
[ ... ]
> @@ -1813,6 +1814,9 @@ static void msm_dp_ctrl_link_clk_disable(struct msm_dp_ctrl *msm_dp_ctrl)
>  
>  	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
>  
> +	if (!ctrl->link_clks_on)
> +		return;
> +

[Severity: High]
Could this lockless check introduce a clock reference leak when executing
concurrently?

If a DP sink device sends an HPD interrupt with a
DP_TEST_LINK_PHY_TEST_PATTERN request at the same time as a userspace
modeset, msm_dp_ctrl_link_clk_enable() might run concurrently from two
threads (one holding plugged_lock, the other holding drm_modeset_lock).

Both could enable the clock and set the flag. Later, the first disable
call un-sets the flag, causing subsequent calls to return here early
and leak the extra clock reference.

>  	clk_bulk_disable_unprepare(ctrl->num_link_clks, ctrl->link_clks);
>  
>  	ctrl->link_clks_on = false;
> @@ -1824,6 +1828,23 @@ static void msm_dp_ctrl_link_clk_disable(struct msm_dp_ctrl *msm_dp_ctrl)
>  		   str_on_off(ctrl->core_clks_on));
>  }
>  
> +static int msm_dp_ctrl_phy_power_off(struct msm_dp_ctrl_private *ctrl)
> +{
> +	int ret;
> +
> +	if (!ctrl->phy_powered)
> +		return 0;
> +

[Severity: High]
Similarly, does this skip release when the flag was unset by a racing
thread?

If phy_power_on() was called twice due to the same race, the second
power-off call will skip releasing the reference here, permanently
leaking the PHY power reference.

> +	ret = phy_power_off(ctrl->phy);
> +	if (ret) {
> +		DRM_ERROR("Failed to power off PHY: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ctrl->phy_powered = false;
> +	return 0;
> +}
> +
>  static int msm_dp_ctrl_enable_mainlink_clocks(struct msm_dp_ctrl_private *ctrl,
>  					      struct msm_dp_panel *panel)
>  {
> @@ -1836,7 +1857,11 @@ static int msm_dp_ctrl_enable_mainlink_clocks(struct msm_dp_ctrl_private *ctrl,
>  	ctrl->phy_opts.dp.ssc = drm_dp_max_downspread(dpcd);
>  
>  	phy_configure(phy, &ctrl->phy_opts);
> -	phy_power_on(phy);
> +	if (!ctrl->phy_powered) {

[Severity: High]
Does this flag need lock protection? The phy_powered state is checked and
modified without locking here.

When msm_dp_irq_hpd_handle() and msm_dp_display_atomic_enable() race,
both threads could see !ctrl->phy_powered as true, calling phy_power_on()
twice and incrementing the PHY core's power_count twice.

> +		ret = phy_power_on(phy);
> +		if (!ret)
> +			ctrl->phy_powered = true;
> +	}
>  
>  	dev_pm_opp_set_rate(ctrl->dev, ctrl->link->link_params.rate * 1000);
>  	ret = msm_dp_ctrl_link_clk_enable(&ctrl->msm_dp_ctrl);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-msm-dp-hdr10-v1-0-dbbf8b2b42bc@radxa.com?part=3

[PATCH 04/13] drm/msm/dp: Serialize HPD state updates

[Xilin Wu]

The plug and detect paths hold plugged_lock while accessing the sink
state, but IRQ-HPD processing updates the same state without that lock.
Unplug also changes the video-test flag and AUX transfer gate before
acquiring the lock.

Cover those updates with plugged_lock so that users of the connection
lock cannot observe partially updated HPD state. This also lets stream
failure cleanup decide whether to preserve AUX while excluding HPD
processing.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 4dcbd9b99d06..3ca039ff57b8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -450,6 +450,8 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
 {
 	struct platform_device *pdev = dp->msm_dp_display.pdev;
 
+	guard(mutex)(&dp->plugged_lock);
+
 	dp->panel->video_test = false;
 
 	msm_dp_aux_enable_xfers(dp->aux, false);
@@ -458,7 +460,6 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
 			dp->msm_dp_display.connector_type,
 			dp->link->sink_count);
 
-	guard(mutex)(&dp->plugged_lock);
 	if (!dp->plugged)
 		return 0;
 
@@ -500,6 +501,8 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
 	u32 sink_request;
 	int rc = 0;
 
+	guard(mutex)(&dp->plugged_lock);
+
 	/* irq_hpd can happen at either connected or disconnected state */
 	drm_dbg_dp(dp->drm_dev, "Before, type=%d, sink_count=%d\n",
 			dp->msm_dp_display.connector_type,

-- 
2.55.0

Re: [PATCH 04/13] drm/msm/dp: Serialize HPD state updates

[Dmitry Baryshkov]

On Wed, Sep 30, 2026 at 08:41:48PM +0800, Xilin Wu wrote:
> The plug and detect paths hold plugged_lock while accessing the sink
> state, but IRQ-HPD processing updates the same state without that lock.
> Unplug also changes the video-test flag and AUX transfer gate before
> acquiring the lock.
> 
> Cover those updates with plugged_lock so that users of the connection
> lock cannot observe partially updated HPD state. This also lets stream
> failure cleanup decide whether to preserve AUX while excluding HPD
> processing.
> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@radxa.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

[PATCH 05/13] drm/msm/dp: Unwind resources when enabling a stream fails

[Xilin Wu]

Stream enable can fail after acquiring a runtime PM reference or
starting the link. Returning directly leaks those resources, while a
later disable can release a reference that was never acquired.

Track the stream PM reference and whether mainlink startup was
attempted. Use one rollback path for prepare, enable and post-enable
failures, and share the bookkeeping with normal disable to avoid
repeated release.

Keep AUX available for a connected external DP peer, including a
dongle without a downstream sink. If mainlink startup was attempted,
restore the AUX PHY after link shutdown, since partial startup can
power down AUX as well. Use plugged_lock to serialize this decision
with HPD processing; leave the connection PM reference for the HPD
path to release.

Only access mainlink and SDP registers while the link clocks are
enabled. Startup or link reinitialization can fail with those clocks
already off; keep the controller reset and remaining PHY cleanup
independent of them.

With ownership tracking and rollback in place, propagate errors from
PHY initialization, configuration, power-on, eDP discovery and stream
retraining. Do not mark failed PHY initialization as successful, and
balance resources in the plug, detect and runtime resume paths as
well.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c    |  45 +++++++++-----
 drivers/gpu/drm/msm/dp/dp_ctrl.h    |   2 +-
 drivers/gpu/drm/msm/dp/dp_display.c | 120 +++++++++++++++++++++++++++++++-----
 3 files changed, 135 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index f005421630c6..16c9165b5f31 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1856,17 +1856,22 @@ static int msm_dp_ctrl_enable_mainlink_clocks(struct msm_dp_ctrl_private *ctrl,
 	ctrl->phy_opts.dp.link_rate = ctrl->link->link_params.rate / 100;
 	ctrl->phy_opts.dp.ssc = drm_dp_max_downspread(dpcd);
 
-	phy_configure(phy, &ctrl->phy_opts);
+	ret = phy_configure(phy, &ctrl->phy_opts);
+	if (ret)
+		return ret;
 	if (!ctrl->phy_powered) {
 		ret = phy_power_on(phy);
-		if (!ret)
-			ctrl->phy_powered = true;
+		if (ret)
+			return ret;
+		ctrl->phy_powered = true;
 	}
 
 	dev_pm_opp_set_rate(ctrl->dev, ctrl->link->link_params.rate * 1000);
 	ret = msm_dp_ctrl_link_clk_enable(&ctrl->msm_dp_ctrl);
-	if (ret)
+	if (ret) {
 		DRM_ERROR("Unable to start link clocks. ret=%d\n", ret);
+		msm_dp_ctrl_phy_power_off(ctrl);
+	}
 
 	drm_dbg_dp(ctrl->drm_dev, "link rate=%d\n", ctrl->link->link_params.rate);
 
@@ -1980,7 +1985,7 @@ static void msm_dp_ctrl_phy_reset(struct msm_dp_ctrl_private *ctrl)
 	msm_dp_write_ahb(ctrl, REG_DP_PHY_CTRL, 0x0);
 }
 
-void msm_dp_ctrl_phy_init(struct msm_dp_ctrl *msm_dp_ctrl)
+int msm_dp_ctrl_phy_init(struct msm_dp_ctrl *msm_dp_ctrl)
 {
 	struct msm_dp_ctrl_private *ctrl;
 	struct phy *phy;
@@ -1989,7 +1994,7 @@ void msm_dp_ctrl_phy_init(struct msm_dp_ctrl *msm_dp_ctrl)
 	phy = ctrl->phy;
 
 	msm_dp_ctrl_phy_reset(ctrl);
-	phy_init(phy);
+	return phy_init(phy);
 }
 
 void msm_dp_ctrl_phy_exit(struct msm_dp_ctrl *msm_dp_ctrl)
@@ -2021,7 +2026,9 @@ static int msm_dp_ctrl_reinitialize_mainlink(struct msm_dp_ctrl_private *ctrl,
 	 */
 	msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl);
 
-	msm_dp_ctrl_phy_power_off(ctrl);
+	ret = msm_dp_ctrl_phy_power_off(ctrl);
+	if (ret)
+		return ret;
 	/* hw recommended delay before re-enabling clocks */
 	msleep(20);
 
@@ -2038,16 +2045,21 @@ static int msm_dp_ctrl_deinitialize_mainlink(struct msm_dp_ctrl_private *ctrl,
 					     struct msm_dp_panel *panel)
 {
 	struct phy *phy;
+	int ret;
 
 	phy = ctrl->phy;
 
-	msm_dp_ctrl_mainlink_disable(ctrl);
+	/* Reinitializing the link may already have disabled its clocks. */
+	if (ctrl->link_clks_on)
+		msm_dp_ctrl_mainlink_disable(ctrl);
 
 	msm_dp_ctrl_reset(&ctrl->msm_dp_ctrl, panel);
 
 	msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl);
 
-	msm_dp_ctrl_phy_power_off(ctrl);
+	ret = msm_dp_ctrl_phy_power_off(ctrl);
+	if (ret)
+		return ret;
 
 	/* aux channel down, reinit phy */
 	phy_exit(phy);
@@ -2583,8 +2595,11 @@ int msm_dp_ctrl_prepare_stream_on(struct msm_dp_ctrl *msm_dp_ctrl,
 		}
 	}
 
-	if (force_link_train || !msm_dp_ctrl_channel_eq_ok(ctrl))
-		msm_dp_ctrl_link_retrain(ctrl, panel);
+	if (force_link_train || !msm_dp_ctrl_channel_eq_ok(ctrl)) {
+		ret = msm_dp_ctrl_link_retrain(ctrl, panel);
+		if (ret)
+			return ret;
+	}
 
 	/* stop txing train pattern to end link training */
 	msm_dp_ctrl_clear_training_pattern(ctrl, panel, DP_PHY_DPRX);
@@ -2671,9 +2686,11 @@ void msm_dp_ctrl_off_link(struct msm_dp_ctrl *msm_dp_ctrl,
 
 	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
 
-	msm_dp_panel_disable_vsc_sdp(panel);
-
-	msm_dp_ctrl_mainlink_disable(ctrl);
+	/* Link startup may have failed before enabling the link clocks. */
+	if (ctrl->link_clks_on) {
+		msm_dp_panel_disable_vsc_sdp(panel);
+		msm_dp_ctrl_mainlink_disable(ctrl);
+	}
 
 	msm_dp_ctrl_reset(&ctrl->msm_dp_ctrl, panel);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 5902cf7e746a..42c5f847cb02 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -39,7 +39,7 @@ struct msm_dp_ctrl *msm_dp_ctrl_get(struct device *dev,
 
 void msm_dp_ctrl_reset(struct msm_dp_ctrl *msm_dp_ctrl,
 		       struct msm_dp_panel *panel);
-void msm_dp_ctrl_phy_init(struct msm_dp_ctrl *msm_dp_ctrl);
+int msm_dp_ctrl_phy_init(struct msm_dp_ctrl *msm_dp_ctrl);
 void msm_dp_ctrl_phy_exit(struct msm_dp_ctrl *msm_dp_ctrl);
 void msm_dp_ctrl_irq_phy_exit(struct msm_dp_ctrl *msm_dp_ctrl);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 3ca039ff57b8..ae967ca652c9 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -52,6 +52,8 @@ struct msm_dp_display_private {
 	bool core_initialized;
 	bool phy_initialized;
 	bool audio_supported;
+	bool stream_pm_active;
+	bool stream_link_attempted;
 
 	struct mutex plugged_lock;
 	bool plugged;
@@ -322,22 +324,26 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
  *
  * Prepare DP PHY for the AUX transactions to succeed.
  *
- * Returns: true if this call has initliazed the PHY and false if the PHY has
- * already been setup beforehand.
+ * Returns: 1 if this call initialized the PHY, 0 if it was already
+ * initialized, or a negative error code on failure.
  */
-static bool msm_dp_display_host_phy_init(struct msm_dp_display_private *dp)
+static int msm_dp_display_host_phy_init(struct msm_dp_display_private *dp)
 {
+	int ret;
+
 	drm_dbg_dp(dp->drm_dev, "type=%d core_init=%d phy_init=%d\n",
 		dp->msm_dp_display.connector_type, dp->core_initialized,
 		dp->phy_initialized);
 
 	if (!dp->phy_initialized) {
-		msm_dp_ctrl_phy_init(dp->ctrl);
+		ret = msm_dp_ctrl_phy_init(dp->ctrl);
+		if (ret)
+			return ret;
 		dp->phy_initialized = true;
-		return true;
+		return 1;
 	}
 
-	return false;
+	return 0;
 }
 
 static void msm_dp_display_host_phy_exit(struct msm_dp_display_private *dp)
@@ -419,7 +425,12 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
 
 	msm_dp_aux_enable_xfers(dp->aux, true);
 
-	msm_dp_display_host_phy_init(dp);
+	ret = msm_dp_display_host_phy_init(dp);
+	if (ret < 0) {
+		msm_dp_aux_enable_xfers(dp->aux, false);
+		pm_runtime_put_sync(&pdev->dev);
+		return ret;
+	}
 
 	ret = msm_dp_display_process_hpd_high(dp);
 
@@ -628,8 +639,11 @@ static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
 
 	drm_dbg_dp(dp->drm_dev, "sink_count=%d\n", dp->link->sink_count);
 
-	if (msm_dp_display->is_edp)
-		msm_dp_hpd_plug_handle(dp);
+	if (msm_dp_display->is_edp) {
+		rc = msm_dp_hpd_plug_handle(dp);
+		if (rc)
+			return rc;
+	}
 
 	rc = pm_runtime_resume_and_get(&msm_dp_display->pdev->dev);
 	if (rc) {
@@ -637,18 +651,22 @@ static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
 		return rc;
 	}
 
+	dp->stream_pm_active = true;
+
 	if (dp->link->sink_count == 0)
 		return -ENOTCONN;
 
 	if (!msm_dp_display->power_on) {
-		msm_dp_display_host_phy_init(dp);
+		rc = msm_dp_display_host_phy_init(dp);
+		if (rc < 0)
+			return rc;
 		force_link_train = true;
 	}
 
+	dp->stream_link_attempted = true;
 	rc = msm_dp_ctrl_on_link(dp->ctrl, dp->panel);
 	if (rc) {
 		DRM_ERROR("Failed link training (rc=%d)\n", rc);
-		// TODO: schedule drm_connector_set_link_status_property()
 		return rc;
 	}
 
@@ -730,6 +748,7 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp,
 		msm_dp_link_psm_config(dp->link, &msm_dp_panel->link_info, true);
 
 	msm_dp_ctrl_off_link(dp->ctrl, msm_dp_panel);
+	dp->stream_link_attempted = false;
 
 	if (dp->link->sink_count == 0)
 		/* re-init the PHY so that we can listen to Dongle disconnect */
@@ -912,7 +931,12 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
 		return status;
 	}
 
-	phy_deinit = msm_dp_display_host_phy_init(priv);
+	ret = msm_dp_display_host_phy_init(priv);
+	if (ret < 0) {
+		pm_runtime_put_sync(&dp->pdev->dev);
+		return status;
+	}
+	phy_deinit = ret;
 
 	msm_dp_aux_enable_xfers(priv->aux, true);
 
@@ -1294,6 +1318,7 @@ static int msm_dp_pm_runtime_suspend(struct device *dev)
 static int msm_dp_pm_runtime_resume(struct device *dev)
 {
 	struct msm_dp_display_private *dp = dev_get_dp_display_private(dev);
+	int ret;
 
 	/*
 	 * for eDP, host cotroller, HPD block and PHY are enabled here
@@ -1306,7 +1331,12 @@ static int msm_dp_pm_runtime_resume(struct device *dev)
 	msm_dp_display_host_init(dp);
 	if (dp->msm_dp_display.is_edp) {
 		msm_dp_aux_hpd_enable(dp->aux);
-		msm_dp_display_host_phy_init(dp);
+		ret = msm_dp_display_host_phy_init(dp);
+		if (ret < 0) {
+			msm_dp_aux_hpd_disable(dp->aux);
+			msm_dp_display_host_deinit(dp);
+			return ret;
+		}
 	}
 
 	enable_irq(dp->irq);
@@ -1431,6 +1461,53 @@ void msm_dp_display_atomic_pre_enable(struct msm_dp *msm_dp_display,
 	msm_dp_display_set_mode(msm_dp_display, &crtc_state->adjusted_mode, dp->panel);
 }
 
+static void msm_dp_display_abort_enable(struct msm_dp_display_private *dp)
+{
+	bool keep_aux;
+	int ret;
+
+	/* Do not access the controller if stream preparation never resumed it. */
+	if (!dp->stream_pm_active)
+		goto unplug;
+
+	scoped_guard(mutex, &dp->plugged_lock) {
+		keep_aux = !dp->msm_dp_display.is_edp && dp->plugged &&
+			msm_dp_aux_is_link_connected(dp->aux);
+
+		if (dp->stream_link_attempted) {
+			/* The AUX peer is the remaining dongle, not its absent sink. */
+			if (keep_aux && !dp->link->sink_count && dp->phy_initialized)
+				msm_dp_link_psm_config(dp->link, &dp->panel->link_info, true);
+
+			msm_dp_ctrl_off_pixel_clk(dp->ctrl);
+			msm_dp_ctrl_off_link(dp->ctrl, dp->panel);
+			dp->stream_link_attempted = false;
+
+			/* Mainlink power-off can also power down the AUX circuitry. */
+			msm_dp_display_host_phy_exit(dp);
+			if (keep_aux) {
+				ret = msm_dp_display_host_phy_init(dp);
+				if (ret < 0) {
+					DRM_ERROR("Failed to restore AUX PHY: %d\n", ret);
+					msm_dp_aux_enable_xfers(dp->aux, false);
+				}
+			}
+		} else if (!keep_aux) {
+			msm_dp_display_host_phy_exit(dp);
+		}
+		dp->msm_dp_display.power_on = false;
+	}
+
+unplug:
+	if (dp->msm_dp_display.is_edp)
+		msm_dp_hpd_unplug_handle(dp);
+
+	if (dp->stream_pm_active) {
+		pm_runtime_put_sync(&dp->msm_dp_display.pdev->dev);
+		dp->stream_pm_active = false;
+	}
+}
+
 void msm_dp_display_atomic_enable(struct msm_dp *msm_dp_display,
 				  struct drm_atomic_commit *state)
 {
@@ -1442,20 +1519,26 @@ void msm_dp_display_atomic_enable(struct msm_dp *msm_dp_display,
 	rc = msm_dp_display_prepare_link(dp);
 	if (rc) {
 		DRM_ERROR("DP display prepare failed, rc=%d\n", rc);
-		return;
+		goto err;
 	}
 
 	rc = msm_dp_display_enable(dp, dp->panel);
-	if (rc)
+	if (rc) {
 		DRM_ERROR("DP display enable failed, rc=%d\n", rc);
+		goto err;
+	}
 
 	rc = msm_dp_display_post_enable(msm_dp_display);
 	if (rc) {
 		DRM_ERROR("DP display post enable failed, rc=%d\n", rc);
-		msm_dp_display_disable(dp, dp->panel);
+		goto err;
 	}
 
 	drm_dbg_dp(msm_dp_display->drm_dev, "type=%d Done\n", msm_dp_display->connector_type);
+	return;
+
+err:
+	msm_dp_display_abort_enable(dp);
 }
 
 void msm_dp_display_atomic_disable(struct msm_dp *dp)
@@ -1485,7 +1568,10 @@ static void msm_dp_display_unprepare(struct msm_dp_display_private *dp)
 {
 	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
 
-	pm_runtime_put_sync(&msm_dp_display->pdev->dev);
+	if (dp->stream_pm_active) {
+		pm_runtime_put_sync(&msm_dp_display->pdev->dev);
+		dp->stream_pm_active = false;
+	}
 
 	drm_dbg_dp(dp->drm_dev, "type=%d Done\n", msm_dp_display->connector_type);
 }

-- 
2.55.0

Re: [PATCH 05/13] drm/msm/dp: Unwind resources when enabling a stream fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm/dp: missing clear of DPCD training pattern on retrain failure
--

commit 9215bc6b24d793ba7f4c076d150d2b4195e8af3e
Author: Xilin Wu <sophon@radxa.com>

drm/msm/dp: Unwind resources when enabling a stream fails

This patch adds tracking for DP stream power references and link startup 
to properly unwind resources on failure. It consolidates the rollback 
path and propagates error codes from PHY initialization, power-on, and 
stream retraining.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
[ ... ]
> @@ -2583,8 +2595,11 @@ int msm_dp_ctrl_prepare_stream_on(struct msm_dp_ctrl *msm_dp_ctrl,
>  		}
>  	}
>  
> -	if (force_link_train || !msm_dp_ctrl_channel_eq_ok(ctrl))
> -		msm_dp_ctrl_link_retrain(ctrl, panel);
> +	if (force_link_train || !msm_dp_ctrl_channel_eq_ok(ctrl)) {
> +		ret = msm_dp_ctrl_link_retrain(ctrl, panel);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/* stop txing train pattern to end link training */
>  	msm_dp_ctrl_clear_training_pattern(ctrl, panel, DP_PHY_DPRX);

[Severity: Medium]
Does this early return skip clearing the training pattern on the DP sink?

When msm_dp_ctrl_link_retrain() fails and returns an error, the function 
now returns early and bypasses the required call to 
msm_dp_ctrl_clear_training_pattern(). 

This leaves the DP sink in an incomplete link training state because the 
DPCD DP_TRAINING_PATTERN_SET register is not explicitly reset to 
DP_TRAINING_PATTERN_DISABLE. 

Could this cause the sink to malfunction, remain blank, or become 
unresponsive on subsequent link initialization attempts? Should the hardware 
training state be cleared before propagating the error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-msm-dp-hdr10-v1-0-dbbf8b2b42bc@radxa.com?part=5

[PATCH 06/13] drm/msm/dp: Report stream enable failures through link status

[Xilin Wu]

Atomic bridge enable callbacks cannot return an error to userspace.
After a failed enable, leaving link-status unchanged gives userspace
no indication that it needs to retry the configuration.

Mark the connector link status bad from a work item after unwinding
the failed enable. Send one connector hotplug notification per failure
episode: fbdev can synchronously retry the modeset from the
notification, so notifying on every failure would create an unbounded
retry loop. Subsequent failures still restore BAD after a retry has
set the property to GOOD.

Allow notifications again after a successful enable or an external
sink connection change. Do not reset the notification latch during
eDP's internal plug and unplug handling, which runs on every retry.
Ignore queued work superseded by recovery or an external unplug.
Serialize the failure state with plugged_lock and update link-status
under the connection mutex before notifying clients with both locks
released.

Initialize the work at probe and cancel it before unbinding the
display.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 63 +++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index ae967ca652c9..1bfa6696d904 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -12,9 +12,12 @@
 #include <linux/phy/phy.h>
 #include <linux/delay.h>
 #include <linux/string_choices.h>
+#include <linux/workqueue.h>
 #include <drm/display/drm_dp_aux_bus.h>
 #include <drm/display/drm_hdmi_audio_helper.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_modeset_lock.h>
+#include <drm/drm_probe_helper.h>
 
 #include "msm_drv.h"
 #include "msm_kms.h"
@@ -54,9 +57,13 @@ struct msm_dp_display_private {
 	bool audio_supported;
 	bool stream_pm_active;
 	bool stream_link_attempted;
+	struct work_struct link_status_work;
 
 	struct mutex plugged_lock;
 	bool plugged;
+	/* Protected by plugged_lock, including accesses from link_status_work. */
+	bool link_failed;
+	bool link_status_notified;
 
 	struct drm_device *drm_dev;
 
@@ -204,6 +211,39 @@ void msm_dp_display_signal_audio_complete(struct msm_dp *msm_dp_display)
 	complete_all(&dp->audio_comp);
 }
 
+static void msm_dp_display_reset_link_status(struct msm_dp_display_private *dp)
+{
+	lockdep_assert_held(&dp->plugged_lock);
+
+	dp->link_failed = false;
+	dp->link_status_notified = false;
+}
+
+static void msm_dp_display_link_status_work(struct work_struct *work)
+{
+	struct msm_dp_display_private *dp = container_of(work,
+			struct msm_dp_display_private, link_status_work);
+	struct drm_connector *connector = dp->msm_dp_display.connector;
+	struct drm_device *dev = connector->dev;
+	bool notify = false;
+
+	/* Match atomic check's connection_mutex -> plugged_lock ordering. */
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	scoped_guard(mutex, &dp->plugged_lock) {
+		/* A successful enable or unplug may have superseded this work. */
+		if (dp->link_failed) {
+			connector->state->link_status = DRM_MODE_LINK_STATUS_BAD;
+			notify = !dp->link_status_notified;
+			dp->link_status_notified = true;
+		}
+	}
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+	/* fbdev can retry the modeset synchronously from this notification. */
+	if (notify)
+		drm_kms_helper_connector_hotplug_event(connector);
+}
+
 static int msm_dp_display_bind(struct device *dev, struct device *master,
 			   void *data)
 {
@@ -234,6 +274,7 @@ static void msm_dp_display_unbind(struct device *dev, struct device *master,
 	struct msm_dp_display_private *dp = dev_get_dp_display_private(dev);
 	struct msm_drm_private *priv = dev_get_drvdata(master);
 
+	cancel_work_sync(&dp->link_status_work);
 	of_dp_aux_depopulate_bus(dp->aux);
 
 	msm_dp_aux_unregister(dp->aux);
@@ -417,6 +458,10 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
 
 	guard(mutex)(&dp->plugged_lock);
 
+	/* eDP calls this for every enable, including automatic retries. */
+	if (!dp->msm_dp_display.is_edp && !dp->plugged)
+		msm_dp_display_reset_link_status(dp);
+
 	ret = pm_runtime_resume_and_get(&pdev->dev);
 	if (ret) {
 		DRM_ERROR("failed to pm_runtime_resume\n");
@@ -463,6 +508,9 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
 
 	guard(mutex)(&dp->plugged_lock);
 
+	if (!dp->msm_dp_display.is_edp)
+		msm_dp_display_reset_link_status(dp);
+
 	dp->panel->video_test = false;
 
 	msm_dp_aux_enable_xfers(dp->aux, false);
@@ -524,10 +572,12 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
 	if (!rc) {
 		sink_request = dp->link->sink_request;
 		drm_dbg_dp(dp->drm_dev, "sink_request=%d\n", sink_request);
-		if (sink_request & DS_PORT_STATUS_CHANGED)
+		if (sink_request & DS_PORT_STATUS_CHANGED) {
+			msm_dp_display_reset_link_status(dp);
 			rc = msm_dp_display_process_hpd_high(dp);
-		else
+		} else {
 			rc = msm_dp_display_handle_irq_hpd(dp);
+		}
 	}
 
 	drm_dbg_dp(dp->drm_dev, "After, type=%d, sink_count=%d\n",
@@ -1246,6 +1296,7 @@ static int msm_dp_display_probe(struct platform_device *pdev)
 	dp->hpd_isr_status = 0;
 
 	mutex_init(&dp->plugged_lock);
+	INIT_WORK(&dp->link_status_work, msm_dp_display_link_status_work);
 
 	rc = msm_dp_display_get_io(dp);
 	if (rc)
@@ -1534,11 +1585,19 @@ void msm_dp_display_atomic_enable(struct msm_dp *msm_dp_display,
 		goto err;
 	}
 
+	scoped_guard(mutex, &dp->plugged_lock)
+		msm_dp_display_reset_link_status(dp);
+
 	drm_dbg_dp(msm_dp_display->drm_dev, "type=%d Done\n", msm_dp_display->connector_type);
 	return;
 
 err:
 	msm_dp_display_abort_enable(dp);
+	scoped_guard(mutex, &dp->plugged_lock) {
+		dp->link_failed = true;
+		/* Keep reporting BAD, but notify only once until recovery. */
+		schedule_work(&dp->link_status_work);
+	}
 }
 
 void msm_dp_display_atomic_disable(struct msm_dp *dp)

-- 
2.55.0

Re: [PATCH 06/13] drm/msm/dp: Report stream enable failures through link status

[Dmitry Baryshkov]

On Wed, Sep 30, 2026 at 08:41:50PM +0800, Xilin Wu wrote:
> Atomic bridge enable callbacks cannot return an error to userspace.
> After a failed enable, leaving link-status unchanged gives userspace
> no indication that it needs to retry the configuration.
> 
> Mark the connector link status bad from a work item after unwinding
> the failed enable. Send one connector hotplug notification per failure
> episode: fbdev can synchronously retry the modeset from the
> notification, so notifying on every failure would create an unbounded
> retry loop. Subsequent failures still restore BAD after a retry has
> set the property to GOOD.
> 
> Allow notifications again after a successful enable or an external
> sink connection change. Do not reset the notification latch during
> eDP's internal plug and unplug handling, which runs on every retry.
> Ignore queued work superseded by recovery or an external unplug.
> Serialize the failure state with plugged_lock and update link-status
> under the connection mutex before notifying clients with both locks
> released.
> 
> Initialize the work at probe and cancel it before unbinding the
> display.
> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@radxa.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 63 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index ae967ca652c9..1bfa6696d904 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -12,9 +12,12 @@
>  #include <linux/phy/phy.h>
>  #include <linux/delay.h>
>  #include <linux/string_choices.h>
> +#include <linux/workqueue.h>
>  #include <drm/display/drm_dp_aux_bus.h>
>  #include <drm/display/drm_hdmi_audio_helper.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_modeset_lock.h>
> +#include <drm/drm_probe_helper.h>
>  
>  #include "msm_drv.h"
>  #include "msm_kms.h"
> @@ -54,9 +57,13 @@ struct msm_dp_display_private {
>  	bool audio_supported;
>  	bool stream_pm_active;
>  	bool stream_link_attempted;
> +	struct work_struct link_status_work;
>  
>  	struct mutex plugged_lock;
>  	bool plugged;
> +	/* Protected by plugged_lock, including accesses from link_status_work. */
> +	bool link_failed;
> +	bool link_status_notified;

Do we need it? I think, it's easier to send several notifications.

>  
>  	struct drm_device *drm_dev;
>  
> @@ -204,6 +211,39 @@ void msm_dp_display_signal_audio_complete(struct msm_dp *msm_dp_display)
>  	complete_all(&dp->audio_comp);
>  }
>  
> +static void msm_dp_display_reset_link_status(struct msm_dp_display_private *dp)
> +{
> +	lockdep_assert_held(&dp->plugged_lock);
> +
> +	dp->link_failed = false;
> +	dp->link_status_notified = false;
> +}
> +
> +static void msm_dp_display_link_status_work(struct work_struct *work)
> +{
> +	struct msm_dp_display_private *dp = container_of(work,
> +			struct msm_dp_display_private, link_status_work);
> +	struct drm_connector *connector = dp->msm_dp_display.connector;
> +	struct drm_device *dev = connector->dev;
> +	bool notify = false;
> +
> +	/* Match atomic check's connection_mutex -> plugged_lock ordering. */
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +	scoped_guard(mutex, &dp->plugged_lock) {

Please use drm_connector_set_link_status_property() here. See
intel_connector_modeset_retry_work_fn(). I think, there is a TODO in the
driver code, you can drop it too with this patch.

> +		/* A successful enable or unplug may have superseded this work. */
> +		if (dp->link_failed) {
> +			connector->state->link_status = DRM_MODE_LINK_STATUS_BAD;
> +			notify = !dp->link_status_notified;
> +			dp->link_status_notified = true;
> +		}
> +	}
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +	/* fbdev can retry the modeset synchronously from this notification. */
> +	if (notify)
> +		drm_kms_helper_connector_hotplug_event(connector);
> +}
> +
>  static int msm_dp_display_bind(struct device *dev, struct device *master,
>  			   void *data)
>  {

-- 
With best wishes
Dmitry

[PATCH 07/13] drm/msm/dp: Track validity of cached sink link capabilities

[Xilin Wu]

An eDP panel may not have been powered up when its mode is checked, and a
disconnected DP sink can leave cached link capabilities behind. The cached
values alone do not distinguish either case from a current read.

Track whether link capability discovery completed successfully. Clear the
flag before reading capabilities and on unplug, and set it only after the
downstream information has been read successfully. Subsequent atomic
checks can then defer checks against unknown capabilities until the sink
is accessible.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 1 +
 drivers/gpu/drm/msm/dp/dp_panel.c   | 3 +++
 drivers/gpu/drm/msm/dp/dp_panel.h   | 1 +
 3 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 1bfa6696d904..9c5b1cd200f8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -519,6 +519,7 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
 			dp->msm_dp_display.connector_type,
 			dp->link->sink_count);
 
+	dp->panel->link_caps_valid = false;
 	if (!dp->plugged)
 		return 0;
 
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index fdf1a3f437ba..f7aac0d25c87 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -247,6 +247,7 @@ int msm_dp_panel_read_link_caps(struct msm_dp_panel *msm_dp_panel,
 
 	panel = container_of(msm_dp_panel, struct msm_dp_panel_private, msm_dp_panel);
 
+	msm_dp_panel->link_caps_valid = false;
 	rc = msm_dp_panel_read_dpcd(msm_dp_panel);
 	if (rc) {
 		DRM_ERROR("read dpcd failed %d\n", rc);
@@ -272,6 +273,8 @@ int msm_dp_panel_read_link_caps(struct msm_dp_panel *msm_dp_panel,
 
 	rc = drm_dp_read_downstream_info(panel->aux, msm_dp_panel->dpcd,
 					 msm_dp_panel->downstream_ports);
+	if (!rc)
+		msm_dp_panel->link_caps_valid = true;
 	return rc;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 4519ac374220..4fe6af8b304e 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -38,6 +38,7 @@ struct msm_dp_panel {
 	struct msm_dp_panel_psr psr_cap;
 	bool video_test;
 	bool vsc_sdp_supported;
+	bool link_caps_valid;
 	u32 hw_revision;
 
 	u32 max_bw_code;

-- 
2.55.0

Re: [PATCH 07/13] drm/msm/dp: Track validity of cached sink link capabilities

[Dmitry Baryshkov]

On Wed, Sep 30, 2026 at 08:41:51PM +0800, Xilin Wu wrote:
> An eDP panel may not have been powered up when its mode is checked, and a
> disconnected DP sink can leave cached link capabilities behind. The cached
> values alone do not distinguish either case from a current read.

Since eDP caps (hopefully) don't change, would it be better to cache
them on the first access?

> 
> Track whether link capability discovery completed successfully. Clear the
> flag before reading capabilities and on unplug, and set it only after the
> downstream information has been read successfully. Subsequent atomic
> checks can then defer checks against unknown capabilities until the sink
> is accessible.
> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@radxa.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 1 +
>  drivers/gpu/drm/msm/dp/dp_panel.c   | 3 +++
>  drivers/gpu/drm/msm/dp/dp_panel.h   | 1 +
>  3 files changed, 5 insertions(+)
> 

-- 
With best wishes
Dmitry

[PATCH 08/13] drm/msm/dp: Rebuild the eDP stream on modesets leaving self refresh

[Xilin Wu]

The atomic helpers skip bridge disable when an active CRTC leaves self
refresh. The eDP enable callback also takes a fast path that only exits
PSR, so a concurrent modeset can leave the retained stream using its old
configuration, including the output depth.

Exit PSR and tear down the retained stream in pre-enable when leaving self
refresh with mode_changed or connectors_changed set. Install the new state
and run the normal stream enable sequence afterwards. Keep the fast path
for ordinary self-refresh exits: active_changed alone does not require
rebuilding the stream.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_drm.c   | 24 +++++++++++++++++++++++-
 drivers/gpu/drm/msm/dp/dp_utils.c |  7 +++++++
 drivers/gpu/drm/msm/dp/dp_utils.h |  4 ++++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index c1e4147bfef1..7332fbfe1644 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -14,6 +14,7 @@
 #include "msm_kms.h"
 #include "dp_audio.h"
 #include "dp_drm.h"
+#include "dp_utils.h"
 
 /**
  * msm_dp_bridge_get_modes - callback to add drm modes via drm_mode_probed_add()
@@ -54,6 +55,26 @@ static void msm_dp_bridge_atomic_pre_enable(struct drm_bridge *drm_bridge,
 {
 	struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
 	struct msm_dp *dp = dp_bridge->msm_dp_display;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+
+	crtc = drm_atomic_get_new_crtc_for_encoder(state, drm_bridge->encoder);
+	if (!crtc)
+		return;
+
+	old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	/*
+	 * The helpers skip bridge disable when waking an active CRTC from
+	 * self refresh, even when its color configuration requires a modeset.
+	 * Tear down the retained stream before installing the new state.
+	 */
+	if (dp->is_edp && dp->power_on && old_crtc_state->self_refresh_active &&
+	    new_crtc_state->active && msm_dp_utils_psr_needs_modeset(new_crtc_state)) {
+		msm_dp_display_set_psr(dp, false);
+		msm_dp_display_atomic_disable(dp);
+		msm_dp_display_atomic_post_disable(dp);
+	}
 
 	msm_dp_display_atomic_pre_enable(dp, state);
 }
@@ -157,7 +178,8 @@ static void msm_edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 
 	old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
 
-	if (old_crtc_state && old_crtc_state->self_refresh_active) {
+	if (dp->power_on && old_crtc_state && old_crtc_state->self_refresh_active &&
+	    !msm_dp_utils_psr_needs_modeset(drm_atomic_get_new_crtc_state(state, crtc))) {
 		msm_dp_display_set_psr(dp, false);
 		return;
 	}
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c b/drivers/gpu/drm/msm/dp/dp_utils.c
index 226b370ce8fe..72bae48a1fe0 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.c
+++ b/drivers/gpu/drm/msm/dp/dp_utils.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/types.h>
+#include <drm/drm_crtc.h>
 
 #include "dp_utils.h"
 
@@ -87,3 +88,9 @@ void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
 		FIELD_PREP(HEADER_3_MASK, sdp_header->HB3) |
 		FIELD_PREP(PARITY_3_MASK, msm_dp_utils_calculate_parity(sdp_header->HB3));
 }
+
+bool msm_dp_utils_psr_needs_modeset(const struct drm_crtc_state *state)
+{
+	/* active_changed alone also describes a normal self-refresh exit. */
+	return state->mode_changed || state->connectors_changed;
+}
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h
index 2fdb89efc57c..5d86c978dffd 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.h
+++ b/drivers/gpu/drm/msm/dp/dp_utils.h
@@ -10,6 +10,8 @@
 #include <linux/bits.h>
 #include <drm/display/drm_dp_helper.h>
 
+struct drm_crtc_state;
+
 #define HEADER_BYTE_0_BIT	 0
 #define PARITY_BYTE_0_BIT	 8
 #define HEADER_BYTE_1_BIT	16
@@ -34,4 +36,6 @@ u8 msm_dp_utils_calculate_parity(u32 data);
 void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
 				  u32 header_buff[2]);
 
+bool msm_dp_utils_psr_needs_modeset(const struct drm_crtc_state *state);
+
 #endif /* _DP_UTILS_H_ */

-- 
2.55.0

[PATCH 09/13] drm/msm/dp: Track output bit depth in bridge atomic state

[Xilin Wu]

Expose max bpc on DP and eDP and select a supported component depth from
the new connector state. Keep the result in a private bridge state so
TEST_ONLY atomic commits do not modify the running stream. Force a modeset
when max bpc changes to apply the new depth to the stream.

Read cached capabilities under the plugged lock and defer the bandwidth
check if they are not yet valid. Recheck the selected depth against the
trained link before enabling video, including the reduced pixel rate of
YUV420. Retain support for 6 bpc SDR panels.

Retain the generic bridge helper's missing-state guard in the custom
duplicate callback. Initial state allocation can fail at bridge attach;
return NULL in that case so atomic state acquisition reports -ENOMEM
instead of copying from a NULL pointer.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 15 +++++++
 drivers/gpu/drm/msm/dp/dp_display.c | 39 ++++++++++++++++--
 drivers/gpu/drm/msm/dp/dp_display.h |  6 +++
 drivers/gpu/drm/msm/dp/dp_drm.c     | 81 +++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/msm/dp/dp_drm.h     |  7 ++++
 drivers/gpu/drm/msm/dp/dp_panel.c   |  5 ---
 drivers/gpu/drm/msm/dp/dp_utils.c   | 20 +++++++++
 drivers/gpu/drm/msm/dp/dp_utils.h   |  4 ++
 8 files changed, 162 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 16c9165b5f31..f41924e75854 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -23,6 +23,7 @@
 
 #include "dp_reg.h"
 #include "dp_ctrl.h"
+#include "dp_utils.h"
 #include "dp_link.h"
 
 #define POLLING_SLEEP_US			1000
@@ -2620,6 +2621,20 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *
 
 	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
 
+	/* Link training may have reduced the available bandwidth. */
+	if (!panel->video_test) {
+		u32 clock = panel->msm_dp_mode.drm_mode.clock;
+
+		if (panel->msm_dp_mode.out_fmt_is_yuv_420)
+			clock /= 2;
+		ret = msm_dp_utils_select_bpp(panel->msm_dp_mode.bpp / 3, 10,
+					      clock, ctrl->link->link_params.rate,
+					      ctrl->link->link_params.num_lanes);
+		if (ret < 0)
+			return ret;
+		panel->msm_dp_mode.bpp = ret;
+	}
+
 	pixel_rate_orig = panel->msm_dp_mode.drm_mode.clock;
 	pixel_rate = pixel_rate_orig;
 
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 9c5b1cd200f8..af6bc179723c 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -28,6 +28,7 @@
 #include "dp_panel.h"
 #include "dp_display.h"
 #include "dp_drm.h"
+#include "dp_utils.h"
 #include "dp_audio.h"
 #include "dp_debug.h"
 
@@ -662,7 +663,8 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
 
 static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
 				   const struct drm_display_mode *adjusted_mode,
-				   struct msm_dp_panel *msm_dp_panel)
+				   struct msm_dp_panel *msm_dp_panel,
+				   const struct msm_dp_bridge_state *state)
 {
 	struct msm_dp_display_private *dp;
 	u32 bpp;
@@ -672,7 +674,7 @@ static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
 	if (msm_dp_display_check_video_test(msm_dp_display))
 		bpp = msm_dp_display_get_test_bpp(msm_dp_display);
 	else
-		bpp = msm_dp_panel->connector->display_info.bpc * 3;
+		bpp = state->bpp;
 
 	msm_dp_panel_init_panel_info(msm_dp_panel, adjusted_mode, bpp ? bpp : 24);
 
@@ -682,6 +684,34 @@ static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
 	return 0;
 }
 
+int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
+				struct msm_dp_bridge_state *state,
+				const struct drm_display_mode *mode,
+				const struct drm_connector_state *conn_state)
+{
+	const struct drm_display_info *info = &conn_state->connector->display_info;
+	struct msm_dp_display_private *dp;
+	unsigned int clock = mode->clock;
+	int bpp;
+
+	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
+
+	if (drm_mode_is_420_only(info, mode))
+		clock /= 2;
+
+	guard(mutex)(&dp->plugged_lock);
+
+	bpp = msm_dp_utils_select_bpp(info->bpc, conn_state->max_requested_bpc,
+				      clock, dp->panel->link_caps_valid ?
+				      dp->panel->link_info.rate : 0,
+				      dp->panel->link_info.num_lanes);
+	if (bpp < 0)
+		return bpp;
+
+	state->bpp = bpp;
+	return 0;
+}
+
 static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
 {
 	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
@@ -1497,6 +1527,7 @@ void msm_dp_display_atomic_pre_enable(struct msm_dp *msm_dp_display,
 	struct msm_dp_display_private *dp;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
+	struct drm_bridge_state *bridge_state;
 
 	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
 
@@ -1504,13 +1535,15 @@ void msm_dp_display_atomic_pre_enable(struct msm_dp *msm_dp_display,
 	if (!crtc)
 		return;
 	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	bridge_state = drm_atomic_get_new_bridge_state(state, msm_dp_display->bridge);
 
 	/*
 	 * The DPU encoder's .atomic_enable() reads the mode's YUV420 / wide bus
 	 * state and runs before the bridge's .atomic_enable(), so the mode must
 	 * be programmed here, in .atomic_pre_enable().
 	 */
-	msm_dp_display_set_mode(msm_dp_display, &crtc_state->adjusted_mode, dp->panel);
+	msm_dp_display_set_mode(msm_dp_display, &crtc_state->adjusted_mode, dp->panel,
+				to_dp_bridge_state(bridge_state));
 }
 
 static void msm_dp_display_abort_enable(struct msm_dp_display_private *dp)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 83117e7619f3..6a77e5952530 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -11,6 +11,8 @@
 
 #define DP_MAX_PIXEL_CLK_KHZ	675000
 
+struct msm_dp_bridge_state;
+
 struct msm_dp {
 	struct drm_device *drm_dev;
 	struct platform_device *pdev;
@@ -43,4 +45,8 @@ enum drm_mode_status msm_dp_display_mode_valid(struct msm_dp *dp,
 					       const struct drm_display_info *info,
 					       const struct drm_display_mode *mode);
 
+int msm_dp_display_atomic_check(struct msm_dp *dp, struct msm_dp_bridge_state *state,
+				const struct drm_display_mode *mode,
+				const struct drm_connector_state *conn_state);
+
 #endif /* _DP_DISPLAY_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 7332fbfe1644..7c18ddcdd19b 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/string_choices.h>
+#include <linux/slab.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_bridge.h>
@@ -116,10 +117,71 @@ static enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *drm_brid
 	return msm_dp_display_mode_valid(dp, info, mode);
 }
 
+static struct drm_bridge_state *msm_dp_bridge_create_state(struct drm_bridge *bridge)
+{
+	struct msm_dp_bridge_state *state;
+
+	state = kzalloc_obj(*state);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	__drm_atomic_helper_bridge_state_init(&state->base, bridge);
+	return &state->base;
+}
+
+static struct drm_bridge_state *msm_dp_bridge_duplicate_state(struct drm_bridge *bridge)
+{
+	struct msm_dp_bridge_state *state;
+	struct drm_bridge_state *old_state;
+
+	if (WARN_ON(!bridge->base.state))
+		return NULL;
+
+	old_state = drm_priv_to_bridge_state(bridge->base.state);
+	state = kmemdup(to_dp_bridge_state(old_state), sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_bridge_duplicate_state(bridge, &state->base);
+	return &state->base;
+}
+
+static void msm_dp_bridge_destroy_state(struct drm_bridge *bridge,
+					struct drm_bridge_state *state)
+{
+	kfree(to_dp_bridge_state(state));
+}
+
+static int msm_dp_bridge_atomic_check(struct drm_bridge *bridge,
+				      struct drm_bridge_state *bridge_state,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_connector_state *conn_state)
+{
+	struct drm_connector_state *old_conn_state;
+	int ret;
+
+	if (!conn_state->crtc)
+		return 0;
+
+	ret = msm_dp_display_atomic_check(to_dp_bridge(bridge)->msm_dp_display,
+					  to_dp_bridge_state(bridge_state),
+					  &crtc_state->adjusted_mode, conn_state);
+	if (ret)
+		return ret;
+
+	old_conn_state = drm_atomic_get_old_connector_state(conn_state->state,
+							    conn_state->connector);
+	if (old_conn_state->max_requested_bpc != conn_state->max_requested_bpc)
+		crtc_state->mode_changed = true;
+
+	return 0;
+}
+
 static const struct drm_bridge_funcs msm_dp_bridge_ops = {
-	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
-	.atomic_destroy_state   = drm_atomic_helper_bridge_destroy_state,
-	.atomic_create_state           = drm_atomic_helper_bridge_create_state,
+	.atomic_check           = msm_dp_bridge_atomic_check,
+	.atomic_duplicate_state = msm_dp_bridge_duplicate_state,
+	.atomic_destroy_state   = msm_dp_bridge_destroy_state,
+	.atomic_create_state           = msm_dp_bridge_create_state,
 	.atomic_pre_enable      = msm_dp_bridge_atomic_pre_enable,
 	.atomic_enable          = msm_dp_bridge_atomic_enable,
 	.atomic_disable         = msm_dp_bridge_atomic_disable,
@@ -154,7 +216,7 @@ static int msm_edp_bridge_atomic_check(struct drm_bridge *drm_bridge,
 	if (crtc_state->self_refresh_active && !dp->psr_supported)
 		return -EINVAL;
 
-	return 0;
+	return msm_dp_bridge_atomic_check(drm_bridge, bridge_state, crtc_state, conn_state);
 }
 
 static void msm_edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
@@ -303,9 +365,9 @@ static const struct drm_bridge_funcs msm_edp_bridge_ops = {
 	.atomic_disable = msm_edp_bridge_atomic_disable,
 	.atomic_post_disable = msm_edp_bridge_atomic_post_disable,
 	.mode_valid = msm_edp_bridge_mode_valid,
-	.atomic_create_state = drm_atomic_helper_bridge_create_state,
-	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_create_state = msm_dp_bridge_create_state,
+	.atomic_duplicate_state = msm_dp_bridge_duplicate_state,
+	.atomic_destroy_state = msm_dp_bridge_destroy_state,
 	.atomic_check = msm_edp_bridge_atomic_check,
 	.debugfs_init = msm_edp_bridge_debugfs_init,
 };
@@ -385,11 +447,16 @@ struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
 					    struct drm_encoder *encoder)
 {
 	struct drm_connector *connector = NULL;
+	int ret;
 
 	connector = drm_bridge_connector_init(msm_dp_display->drm_dev, encoder);
 	if (IS_ERR(connector))
 		return connector;
 
+	ret = drm_connector_attach_max_bpc_property(connector, 6, 10);
+	if (ret)
+		return ERR_PTR(ret);
+
 	if (!msm_dp_display->is_edp)
 		drm_connector_attach_dp_subconnector_property(connector);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
index da412c788503..cd755f7205ef 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.h
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -17,6 +17,13 @@ struct msm_dp_bridge {
 	struct msm_dp *msm_dp_display;
 };
 
+struct msm_dp_bridge_state {
+	struct drm_bridge_state base;
+	u32 bpp;
+};
+
+#define to_dp_bridge_state(x) container_of((x), struct msm_dp_bridge_state, base)
+
 #define to_dp_bridge(x)     container_of((x), struct msm_dp_bridge, bridge)
 
 struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index f7aac0d25c87..f288571798d3 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -690,11 +690,6 @@ int msm_dp_panel_init_panel_info(struct msm_dp_panel *msm_dp_panel,
 				drm_mode->clock);
 	drm_dbg_dp(panel->drm_dev, "bpp = %d\n", msm_dp_panel->msm_dp_mode.bpp);
 
-	msm_dp_panel->msm_dp_mode.bpp = msm_dp_panel_get_mode_bpp(msm_dp_panel, msm_dp_panel->msm_dp_mode.bpp,
-						      msm_dp_panel->msm_dp_mode.drm_mode.clock);
-
-	drm_dbg_dp(panel->drm_dev, "updated bpp = %d\n",
-				msm_dp_panel->msm_dp_mode.bpp);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c b/drivers/gpu/drm/msm/dp/dp_utils.c
index 72bae48a1fe0..3e49854d73fb 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.c
+++ b/drivers/gpu/drm/msm/dp/dp_utils.c
@@ -89,6 +89,26 @@ void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
 		FIELD_PREP(PARITY_3_MASK, msm_dp_utils_calculate_parity(sdp_header->HB3));
 }
 
+/* A zero link rate means that a powered-down eDP panel has not been probed. */
+int msm_dp_utils_select_bpp(unsigned int bpc, unsigned int max_bpc,
+			    unsigned int clock, unsigned int rate,
+			    unsigned int lanes)
+{
+	unsigned int bpp;
+
+	bpc = min3(bpc ? bpc : 8, max_bpc, 10U);
+	bpc = round_down(bpc, 2);
+	if (bpc < 6)
+		return -EINVAL;
+
+	for (bpp = bpc * 3; bpp >= 18; bpp -= 6) {
+		if (!rate || (u64)clock * bpp <= (u64)rate * lanes * 8)
+			return bpp;
+	}
+
+	return -EINVAL;
+}
+
 bool msm_dp_utils_psr_needs_modeset(const struct drm_crtc_state *state)
 {
 	/* active_changed alone also describes a normal self-refresh exit. */
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h
index 5d86c978dffd..a32e313d910d 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.h
+++ b/drivers/gpu/drm/msm/dp/dp_utils.h
@@ -36,6 +36,10 @@ u8 msm_dp_utils_calculate_parity(u32 data);
 void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
 				  u32 header_buff[2]);
 
+int msm_dp_utils_select_bpp(unsigned int bpc, unsigned int max_bpc,
+			    unsigned int clock, unsigned int rate,
+			    unsigned int lanes);
+
 bool msm_dp_utils_psr_needs_modeset(const struct drm_crtc_state *state);
 
 #endif /* _DP_UTILS_H_ */

-- 
2.55.0

Re: [PATCH 09/13] drm/msm/dp: Track output bit depth in bridge atomic state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dp: Atomic semantics violation by deferring bandwidth check
--

commit 90a8ee3f0617fe6a0415caee72e5905ba2709743
Author: Xilin Wu <sophon@radxa.com>
drm/msm/dp: Track output bit depth in bridge atomic state

This commit tracks the output bit depth in the bridge atomic state to expose
max bpc on DP and eDP, and selects a supported component depth. It reads
cached capabilities under a lock and defers bandwidth checks if they are not
yet valid, rechecking the selected depth against the trained link before
enabling video.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 16c9165b5f311..f41924e75854c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -2620,6 +2621,20 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *
>  
>  	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
>  
> +	/* Link training may have reduced the available bandwidth. */
> +	if (!panel->video_test) {
> +		u32 clock = panel->msm_dp_mode.drm_mode.clock;
> +
> +		if (panel->msm_dp_mode.out_fmt_is_yuv_420)
> +			clock /= 2;
> +		ret = msm_dp_utils_select_bpp(panel->msm_dp_mode.bpp / 3, 10,
> +					      clock, ctrl->link->link_params.rate,
> +					      ctrl->link->link_params.num_lanes);
> +		if (ret < 0)
> +			return ret;

[Severity: High]
Does this error path abort the stream setup during the atomic_enable phase?

Userspace can trigger this by requesting a modeset via the atomic API with a
mode or bpp that exceeds the hardware DP link rate while the panel is off.
Because the validation was bypassed in msm_dp_display_atomic_check(),
msm_dp_ctrl_on_stream() now re-evaluates the bandwidth using the actual
trained rate. 

If the requested mode exceeds the trained link bandwidth, this returns
-EINVAL during atomic_enable, which aborts the stream setup. Bypassing
validation in atomic_check and failing during atomic_enable violates the
fundamental guarantee of DRM atomic modesetting, which requires that
atomic_check rejects all unsupported configurations so that atomic_enable is
infallible. This can result in state desynchronization between the DRM core
and the driver, leading to a blank screen and potential vblank timeouts.

> +		panel->msm_dp_mode.bpp = ret;
> +	}
> +
>  	pixel_rate_orig = panel->msm_dp_mode.drm_mode.clock;
>  	pixel_rate = pixel_rate_orig;
>  
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 9c5b1cd200f85..af6bc179723cc 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -682,6 +684,34 @@ static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
>  	return 0;
>  }
>  
> +int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
> +				struct msm_dp_bridge_state *state,
> +				const struct drm_display_mode *mode,
> +				const struct drm_connector_state *conn_state)
> +{
> +	const struct drm_display_info *info = &conn_state->connector->display_info;
> +	struct msm_dp_display_private *dp;
> +	unsigned int clock = mode->clock;
> +	int bpp;
> +
> +	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
> +
> +	if (drm_mode_is_420_only(info, mode))
> +		clock /= 2;
> +
> +	guard(mutex)(&dp->plugged_lock);
> +
> +	bpp = msm_dp_utils_select_bpp(info->bpc, conn_state->max_requested_bpc,
> +				      clock, dp->panel->link_caps_valid ?
> +				      dp->panel->link_info.rate : 0,
> +				      dp->panel->link_info.num_lanes);

[Severity: High]
Can this defer the bandwidth check to the enable phase and violate DRM atomic
semantics?

When an eDP panel is powered down and its link capabilities have not yet been
probed (link_caps_valid is false), this passes 0 for the rate to
msm_dp_utils_select_bpp(). This causes the bandwidth verification to be
skipped, allowing the commit to be accepted by the kernel.

> +	if (bpp < 0)
> +		return bpp;
> +
> +	state->bpp = bpp;
> +	return 0;
> +}
> +
>  static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
>  {
>  	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c b/drivers/gpu/drm/msm/dp/dp_utils.c
> index 72bae48a1fe0b..3e49854d73fb9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
[ ... ]
> @@ -89,6 +89,26 @@ void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
>  		FIELD_PREP(PARITY_3_MASK, msm_dp_utils_calculate_parity(sdp_header->HB3));
>  }
>  
> +/* A zero link rate means that a powered-down eDP panel has not been probed. */
> +int msm_dp_utils_select_bpp(unsigned int bpc, unsigned int max_bpc,
> +			    unsigned int clock, unsigned int rate,
> +			    unsigned int lanes)
> +{
> +	unsigned int bpp;
> +
> +	bpc = min3(bpc ? bpc : 8, max_bpc, 10U);
> +	bpc = round_down(bpc, 2);
> +	if (bpc < 6)
> +		return -EINVAL;
> +
> +	for (bpp = bpc * 3; bpp >= 18; bpp -= 6) {
> +		if (!rate || (u64)clock * bpp <= (u64)rate * lanes * 8)
> +			return bpp;

[Severity: High]
Does this code allow bypassing the bandwidth validation?

When rate is 0, the check automatically bypasses bandwidth validation,
returning success unconditionally.

> +	}
> +
> +	return -EINVAL;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-msm-dp-hdr10-v1-0-dbbf8b2b42bc@radxa.com?part=9

Re: [PATCH 09/13] drm/msm/dp: Track output bit depth in bridge atomic state

[Dmitry Baryshkov]

On Wed, Sep 30, 2026 at 08:41:53PM +0800, Xilin Wu wrote:
> Expose max bpc on DP and eDP and select a supported component depth from
> the new connector state. Keep the result in a private bridge state so
> TEST_ONLY atomic commits do not modify the running stream. Force a modeset
> when max bpc changes to apply the new depth to the stream.
> 
> Read cached capabilities under the plugged lock and defer the bandwidth
> check if they are not yet valid. Recheck the selected depth against the
> trained link before enabling video, including the reduced pixel rate of
> YUV420. Retain support for 6 bpc SDR panels.
> 
> Retain the generic bridge helper's missing-state guard in the custom
> duplicate callback. Initial state allocation can fail at bridge attach;
> return NULL in that case so atomic state acquisition reports -ENOMEM
> instead of copying from a NULL pointer.
> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@radxa.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c    | 15 +++++++
>  drivers/gpu/drm/msm/dp/dp_display.c | 39 ++++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_display.h |  6 +++
>  drivers/gpu/drm/msm/dp/dp_drm.c     | 81 +++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/msm/dp/dp_drm.h     |  7 ++++
>  drivers/gpu/drm/msm/dp/dp_panel.c   |  5 ---
>  drivers/gpu/drm/msm/dp/dp_utils.c   | 20 +++++++++
>  drivers/gpu/drm/msm/dp/dp_utils.h   |  4 ++
>  8 files changed, 162 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 16c9165b5f31..f41924e75854 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -23,6 +23,7 @@
>  
>  #include "dp_reg.h"
>  #include "dp_ctrl.h"
> +#include "dp_utils.h"
>  #include "dp_link.h"
>  
>  #define POLLING_SLEEP_US			1000
> @@ -2620,6 +2621,20 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *
>  
>  	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
>  
> +	/* Link training may have reduced the available bandwidth. */
> +	if (!panel->video_test) {
> +		u32 clock = panel->msm_dp_mode.drm_mode.clock;
> +
> +		if (panel->msm_dp_mode.out_fmt_is_yuv_420)
> +			clock /= 2;
> +		ret = msm_dp_utils_select_bpp(panel->msm_dp_mode.bpp / 3, 10,
> +					      clock, ctrl->link->link_params.rate,
> +					      ctrl->link->link_params.num_lanes);

This doesn't feel correct. Yes, we can lower num_lanes (or rate), but
then it would mean that the caps that we checked in atomic_check() might
no longer match the actual hardware. How do other DP drivers handle the
case? We have a "golden standard" of i915, amdgpu and nouveau, which we
probably should refer to and follow.

> +		if (ret < 0)
> +			return ret;
> +		panel->msm_dp_mode.bpp = ret;
> +	}
> +
>  	pixel_rate_orig = panel->msm_dp_mode.drm_mode.clock;
>  	pixel_rate = pixel_rate_orig;
>  

-- 
With best wishes
Dmitry

[PATCH 10/13] drm/msm/dp: Signal BT.2020 RGB on DP and eDP

[Xilin Wu]

Expose the DP Colorspace property and carry VSC packets in bridge atomic
state. Validate VSC support against cached sink capabilities and check it
again after powering up the sink. Regenerate the packet if link training
changes the SDR bit depth.

Signal full-range RGB, matching the existing pixel path, and retain CTA
range for YUV420. Reject BT2020_RGB with a YUV420-only mode. Clear stale
MSA colorimetry bits and include VSC removal in peripheral flush requests.

Force a modeset when colorspace changes. Disable PSR in the controller and
sink while colorimetry VSC packets are active, since they cannot share the
automatic PSR packet generator. Restore PSR configuration when returning
to a stream that does not need colorimetry VSC packets.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 33 ++++++++++++++++------
 drivers/gpu/drm/msm/dp/dp_ctrl.h    |  2 +-
 drivers/gpu/drm/msm/dp/dp_display.c | 26 +++++++++++++++--
 drivers/gpu/drm/msm/dp/dp_drm.c     | 13 +++++++--
 drivers/gpu/drm/msm/dp/dp_drm.h     |  3 ++
 drivers/gpu/drm/msm/dp/dp_panel.c   | 56 +++++--------------------------------
 drivers/gpu/drm/msm/dp/dp_panel.h   |  4 +++
 drivers/gpu/drm/msm/dp/dp_utils.c   | 38 +++++++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_utils.h   |  7 +++++
 9 files changed, 118 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index f41924e75854..3362d65ee4e2 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -411,7 +411,7 @@ static void msm_dp_ctrl_config_ctrl_streams(struct msm_dp_ctrl_private *ctrl,
 
 	config |= tbd << DP_CONFIGURATION_CTRL_BPC_SHIFT;
 
-	if (msm_dp_panel->psr_cap.version)
+	if (msm_dp_panel->psr_cap.version && !msm_dp_panel->vsc_enabled)
 		config |= DP_CONFIGURATION_CTRL_SEND_VSC;
 
 	drm_dbg_dp(ctrl->drm_dev, "stream DP_CONFIGURATION_CTRL=0x%x\n", config);
@@ -476,7 +476,8 @@ static void msm_dp_ctrl_config_misc1_misc0(struct msm_dp_ctrl_private *ctrl,
 	misc_val = msm_dp_read_link(ctrl, REG_DP_MISC1_MISC0);
 
 	/* clear bpp bits */
-	misc_val &= ~(0x07 << DP_MISC0_TEST_BITS_DEPTH_SHIFT);
+	misc_val &= ~((0x07 << DP_MISC0_TEST_BITS_DEPTH_SHIFT) |
+		      GENMASK(4, 1));
 	misc_val |= colorimetry_cfg << DP_MISC0_COLORIMETRY_CFG_SHIFT;
 	misc_val |= test_bits_depth << DP_MISC0_TEST_BITS_DEPTH_SHIFT;
 	/* Configure clock to synchronous mode */
@@ -1913,7 +1914,7 @@ static void msm_dp_ctrl_psr_exit(struct msm_dp_ctrl_private *ctrl)
 }
 
 void msm_dp_ctrl_config_psr(struct msm_dp_ctrl *msm_dp_ctrl,
-			    struct msm_dp_panel *panel)
+			    struct msm_dp_panel *panel, bool enable)
 {
 	struct msm_dp_ctrl_private *ctrl = container_of(msm_dp_ctrl,
 			struct msm_dp_ctrl_private, msm_dp_ctrl);
@@ -1922,15 +1923,20 @@ void msm_dp_ctrl_config_psr(struct msm_dp_ctrl *msm_dp_ctrl,
 	if (!panel->psr_cap.version)
 		return;
 
-	/* enable PSR1 function */
+	/* Colorimetry VSC packets cannot share the PSR packet generator. */
 	cfg = msm_dp_read_link(ctrl, REG_PSR_CONFIG);
-	cfg |= PSR1_SUPPORTED;
+	if (enable)
+		cfg |= PSR1_SUPPORTED;
+	else
+		cfg &= ~PSR1_SUPPORTED;
 	msm_dp_write_link(ctrl, REG_PSR_CONFIG, cfg);
 
-	msm_dp_ctrl_config_psr_interrupt(ctrl);
-	msm_dp_ctrl_enable_sdp(ctrl);
+	if (enable) {
+		msm_dp_ctrl_config_psr_interrupt(ctrl);
+		msm_dp_ctrl_enable_sdp(ctrl);
+	}
 
-	cfg = DP_PSR_ENABLE;
+	cfg = enable ? DP_PSR_ENABLE : 0;
 	drm_dp_dpcd_write(ctrl->aux, DP_PSR_EN_CFG, &cfg, 1);
 }
 
@@ -2635,6 +2641,17 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *
 		panel->msm_dp_mode.bpp = ret;
 	}
 
+	if (panel->vsc_enabled) {
+		if (!panel->vsc_sdp_supported)
+			return -EINVAL;
+
+		ret = msm_dp_utils_pack_vsc_sdp(&panel->vsc_sdp, panel->colorspace,
+						panel->msm_dp_mode.bpp / 3,
+						panel->msm_dp_mode.out_fmt_is_yuv_420);
+		if (ret)
+			return ret;
+	}
+
 	pixel_rate_orig = panel->msm_dp_mode.drm_mode.clock;
 	pixel_rate = pixel_rate_orig;
 
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 42c5f847cb02..4dd604ef9f1b 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -46,7 +46,7 @@ void msm_dp_ctrl_irq_phy_exit(struct msm_dp_ctrl *msm_dp_ctrl);
 void msm_dp_ctrl_set_psr(struct msm_dp_ctrl *msm_dp_ctrl,
 			 struct msm_dp_panel *panel, bool enable);
 void msm_dp_ctrl_config_psr(struct msm_dp_ctrl *msm_dp_ctrl,
-			    struct msm_dp_panel *panel);
+			    struct msm_dp_panel *panel, bool enable);
 
 int msm_dp_ctrl_core_clk_enable(struct msm_dp_ctrl *msm_dp_ctrl);
 void msm_dp_ctrl_core_clk_disable(struct msm_dp_ctrl *msm_dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index af6bc179723c..65ac35e41ed8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -676,6 +676,10 @@ static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
 	else
 		bpp = state->bpp;
 
+	msm_dp_panel->sdp_needs_flush = msm_dp_panel->vsc_enabled || state->vsc_enabled;
+	msm_dp_panel->colorspace = state->colorspace;
+	msm_dp_panel->vsc_enabled = state->vsc_enabled;
+	msm_dp_panel->vsc_sdp = state->vsc_sdp;
 	msm_dp_panel_init_panel_info(msm_dp_panel, adjusted_mode, bpp ? bpp : 24);
 
 	/* populate wide_bus_support to different layers */
@@ -709,7 +713,17 @@ int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
 		return bpp;
 
 	state->bpp = bpp;
-	return 0;
+	state->colorspace = conn_state->colorspace;
+	state->vsc_enabled = drm_mode_is_420_only(info, mode) ||
+			     state->colorspace != DRM_MODE_COLORIMETRY_DEFAULT;
+	if (!state->vsc_enabled)
+		return 0;
+
+	if (dp->panel->link_caps_valid && !dp->panel->vsc_sdp_supported)
+		return -EINVAL;
+
+	return msm_dp_utils_pack_vsc_sdp(&state->vsc_sdp, state->colorspace,
+					bpp / 3, drm_mode_is_420_only(info, mode));
 }
 
 static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
@@ -734,6 +748,9 @@ static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
 
 	dp->stream_pm_active = true;
 
+	if (dp->panel->vsc_enabled && !dp->panel->vsc_sdp_supported)
+		return -EINVAL;
+
 	if (dp->link->sink_count == 0)
 		return -ENOTCONN;
 
@@ -791,7 +808,7 @@ static int msm_dp_display_post_enable(struct msm_dp *msm_dp_display)
 	msm_dp_display_handle_plugged_change(msm_dp_display, true);
 
 	if (msm_dp_display->psr_supported)
-		msm_dp_ctrl_config_psr(dp->ctrl, dp->panel);
+		msm_dp_ctrl_config_psr(dp->ctrl, dp->panel, !dp->panel->vsc_enabled);
 
 	return 0;
 }
@@ -1461,7 +1478,10 @@ void __exit msm_dp_unregister(void)
 bool msm_dp_needs_periph_flush(const struct msm_dp *msm_dp_display,
 			       const struct drm_display_mode *mode)
 {
-	return drm_mode_is_420_only(&msm_dp_display->connector->display_info, mode);
+	struct msm_dp_display_private *dp = container_of(msm_dp_display,
+					struct msm_dp_display_private, msm_dp_display);
+
+	return dp->panel->sdp_needs_flush;
 }
 
 bool msm_dp_wide_bus_available(const struct msm_dp *msm_dp_display)
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 7c18ddcdd19b..48f2b7e79683 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -171,7 +171,7 @@ static int msm_dp_bridge_atomic_check(struct drm_bridge *bridge,
 
 	old_conn_state = drm_atomic_get_old_connector_state(conn_state->state,
 							    conn_state->connector);
-	if (old_conn_state->max_requested_bpc != conn_state->max_requested_bpc)
+	if (msm_dp_utils_needs_modeset(old_conn_state, conn_state))
 		crtc_state->mode_changed = true;
 
 	return 0;
@@ -208,12 +208,13 @@ static int msm_edp_bridge_atomic_check(struct drm_bridge *drm_bridge,
 	if (WARN_ON(!conn_state))
 		return -ENODEV;
 
-	conn_state->self_refresh_aware = dp->psr_supported;
+	conn_state->self_refresh_aware = dp->psr_supported &&
+		conn_state->colorspace == DRM_MODE_COLORIMETRY_DEFAULT;
 
 	if (!conn_state->crtc || !crtc_state)
 		return 0;
 
-	if (crtc_state->self_refresh_active && !dp->psr_supported)
+	if (crtc_state->self_refresh_active && !conn_state->self_refresh_aware)
 		return -EINVAL;
 
 	return msm_dp_bridge_atomic_check(drm_bridge, bridge_state, crtc_state, conn_state);
@@ -453,6 +454,12 @@ struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
 	if (IS_ERR(connector))
 		return connector;
 
+	ret = drm_mode_create_dp_colorspace_property(connector,
+						     BIT(DRM_MODE_COLORIMETRY_BT2020_RGB));
+	if (ret)
+		return ERR_PTR(ret);
+	drm_connector_attach_colorspace_property(connector);
+
 	ret = drm_connector_attach_max_bpc_property(connector, 6, 10);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
index cd755f7205ef..c9bd79949f79 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.h
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -20,6 +20,9 @@ struct msm_dp_bridge {
 struct msm_dp_bridge_state {
 	struct drm_bridge_state base;
 	u32 bpp;
+	enum drm_colorspace colorspace;
+	bool vsc_enabled;
+	struct dp_sdp vsc_sdp;
 };
 
 #define to_dp_bridge_state(x) container_of((x), struct msm_dp_bridge_state, base)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index f288571798d3..69327333f7f1 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -482,14 +482,14 @@ void msm_dp_panel_enable_vsc_sdp(struct msm_dp_panel *msm_dp_panel, struct dp_sd
 	cfg2 = msm_dp_read_link(panel, MMSS_DP_SDP_CFG2);
 	misc = msm_dp_read_link(panel, REG_DP_MISC1_MISC0);
 
+	msm_dp_panel_send_sdp(panel, vsc_sdp, MMSS_DP_GENERIC0_0);
+
 	cfg |= GEN0_SDP_EN;
 	msm_dp_write_link(panel, MMSS_DP_SDP_CFG, cfg);
 
 	cfg2 |= GENERIC0_SDPSIZE_VALID;
 	msm_dp_write_link(panel, MMSS_DP_SDP_CFG2, cfg2);
 
-	msm_dp_panel_send_sdp(panel, vsc_sdp, MMSS_DP_GENERIC0_0);
-
 	/* indicates presence of VSC (BIT(6) of MISC1) */
 	misc |= DP_MISC1_VSC_SDP;
 
@@ -528,49 +528,6 @@ void msm_dp_panel_disable_vsc_sdp(struct msm_dp_panel *msm_dp_panel)
 	msm_dp_panel_update_sdp(panel);
 }
 
-static int msm_dp_panel_setup_vsc_sdp_yuv_420(struct msm_dp_panel *msm_dp_panel)
-{
-	struct msm_dp_display_mode *msm_dp_mode;
-	struct drm_dp_vsc_sdp vsc_sdp_data;
-	struct dp_sdp vsc_sdp;
-	ssize_t len;
-
-	if (!msm_dp_panel) {
-		DRM_ERROR("invalid input\n");
-		return -EINVAL;
-	}
-
-	msm_dp_mode = &msm_dp_panel->msm_dp_mode;
-
-	memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
-
-	/* VSC SDP header as per table 2-118 of DP 1.4 specification */
-	vsc_sdp_data.sdp_type = DP_SDP_VSC;
-	vsc_sdp_data.revision = 0x05;
-	vsc_sdp_data.length = 0x13;
-
-	/* VSC SDP Payload for DB16 */
-	vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
-	vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
-
-	/* VSC SDP Payload for DB17 */
-	vsc_sdp_data.bpc = msm_dp_mode->bpp / 3;
-	vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
-
-	/* VSC SDP Payload for DB18 */
-	vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
-
-	len = drm_dp_vsc_sdp_pack(&vsc_sdp_data, &vsc_sdp);
-	if (len < 0) {
-		DRM_ERROR("unable to pack vsc sdp\n");
-		return len;
-	}
-
-	msm_dp_panel_enable_vsc_sdp(msm_dp_panel, &vsc_sdp);
-
-	return 0;
-}
-
 int msm_dp_panel_timing_cfg(struct msm_dp_panel *msm_dp_panel, bool wide_bus_en)
 {
 	u32 data, total_ver, total_hor;
@@ -640,8 +597,10 @@ int msm_dp_panel_timing_cfg(struct msm_dp_panel *msm_dp_panel, bool wide_bus_en)
 
 	msm_dp_write_p0(panel, MMSS_DP_INTF_CONFIG, reg);
 
-	if (msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420)
-		msm_dp_panel_setup_vsc_sdp_yuv_420(msm_dp_panel);
+	if (msm_dp_panel->vsc_enabled)
+		msm_dp_panel_enable_vsc_sdp(msm_dp_panel, &msm_dp_panel->vsc_sdp);
+	else
+		msm_dp_panel_disable_vsc_sdp(msm_dp_panel);
 
 	panel->panel_on = true;
 
@@ -664,8 +623,7 @@ int msm_dp_panel_init_panel_info(struct msm_dp_panel *msm_dp_panel,
 	msm_dp_panel->msm_dp_mode.h_active_low =
 		!!(adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC);
 	msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420 =
-		drm_mode_is_420_only(&msm_dp_panel->connector->display_info, adjusted_mode) &&
-		msm_dp_panel->vsc_sdp_supported;
+		drm_mode_is_420_only(&msm_dp_panel->connector->display_info, adjusted_mode);
 
 	drm_mode = &msm_dp_panel->msm_dp_mode.drm_mode;
 
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 4fe6af8b304e..4b3403b7a300 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -39,6 +39,10 @@ struct msm_dp_panel {
 	bool video_test;
 	bool vsc_sdp_supported;
 	bool link_caps_valid;
+	bool vsc_enabled;
+	bool sdp_needs_flush;
+	enum drm_colorspace colorspace;
+	struct dp_sdp vsc_sdp;
 	u32 hw_revision;
 
 	u32 max_bw_code;
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c b/drivers/gpu/drm/msm/dp/dp_utils.c
index 3e49854d73fb..657d376d32da 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.c
+++ b/drivers/gpu/drm/msm/dp/dp_utils.c
@@ -109,6 +109,44 @@ int msm_dp_utils_select_bpp(unsigned int bpc, unsigned int max_bpc,
 	return -EINVAL;
 }
 
+int msm_dp_utils_pack_vsc_sdp(struct dp_sdp *sdp, enum drm_colorspace colorspace,
+			      unsigned int bpc, bool yuv420)
+{
+	struct drm_dp_vsc_sdp vsc = {
+		.sdp_type = DP_SDP_VSC,
+		.revision = 0x05,
+		.length = 0x13,
+		.pixelformat = yuv420 ? DP_PIXELFORMAT_YUV420 : DP_PIXELFORMAT_RGB,
+		.bpc = bpc,
+		.dynamic_range = yuv420 ? DP_DYNAMIC_RANGE_CTA : DP_DYNAMIC_RANGE_VESA,
+		.content_type = DP_CONTENT_TYPE_GRAPHICS,
+	};
+	ssize_t ret;
+
+	switch (colorspace) {
+	case DRM_MODE_COLORIMETRY_DEFAULT:
+		vsc.colorimetry = DP_COLORIMETRY_DEFAULT;
+		break;
+	case DRM_MODE_COLORIMETRY_BT2020_RGB:
+		if (yuv420)
+			return -EINVAL;
+		vsc.colorimetry = DP_COLORIMETRY_BT2020_RGB;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = drm_dp_vsc_sdp_pack(&vsc, sdp);
+	return ret < 0 ? ret : 0;
+}
+
+bool msm_dp_utils_needs_modeset(struct drm_connector_state *old_state,
+				struct drm_connector_state *new_state)
+{
+	return old_state->max_requested_bpc != new_state->max_requested_bpc ||
+	       old_state->colorspace != new_state->colorspace;
+}
+
 bool msm_dp_utils_psr_needs_modeset(const struct drm_crtc_state *state)
 {
 	/* active_changed alone also describes a normal self-refresh exit. */
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h
index a32e313d910d..146c652e995e 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.h
+++ b/drivers/gpu/drm/msm/dp/dp_utils.h
@@ -9,6 +9,7 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <drm/display/drm_dp_helper.h>
+#include <drm/drm_connector.h>
 
 struct drm_crtc_state;
 
@@ -40,6 +41,12 @@ int msm_dp_utils_select_bpp(unsigned int bpc, unsigned int max_bpc,
 			    unsigned int clock, unsigned int rate,
 			    unsigned int lanes);
 
+int msm_dp_utils_pack_vsc_sdp(struct dp_sdp *sdp, enum drm_colorspace colorspace,
+			      unsigned int bpc, bool yuv420);
+
+bool msm_dp_utils_needs_modeset(struct drm_connector_state *old_state,
+				struct drm_connector_state *new_state);
+
 bool msm_dp_utils_psr_needs_modeset(const struct drm_crtc_state *state);
 
 #endif /* _DP_UTILS_H_ */

-- 
2.55.0

Re: [PATCH 10/13] drm/msm/dp: Signal BT.2020 RGB on DP and eDP

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dp: PSR state mismatch causes blank screen for YUV420 modes
--

commit 1a1587a85fbd9d8b79a76366fd11f806251ab09b
Author: Xilin Wu <sophon@radxa.com>
Subject: drm/msm/dp: Signal BT.2020 RGB on DP and eDP

This commit exposes the DP Colorspace property and handles VSC packets
in the bridge atomic state. It disables PSR in the controller and sink
while colorimetry VSC packets are active, as they cannot share the
automatic PSR packet generator.

> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -208,12 +208,13 @@ static int msm_edp_bridge_atomic_check(struct drm_bridge *drm_bridge,
>  	if (WARN_ON(!conn_state))
>  		return -ENODEV;
>  
> -	conn_state->self_refresh_aware = dp->psr_supported;
> +	conn_state->self_refresh_aware = dp->psr_supported &&
> +		conn_state->colorspace == DRM_MODE_COLORIMETRY_DEFAULT;

[Severity: High]
Does this logic create a state mismatch for YUV420 modes between software
and hardware?

In msm_dp_display_atomic_check(), state->vsc_enabled is correctly set to
true for YUV420 modes:

msm_dp_display_atomic_check() {
    ...
    state->vsc_enabled = drm_mode_is_420_only(info, mode) ||
                         state->colorspace != DRM_MODE_COLORIMETRY_DEFAULT;
    ...
}

When vsc_enabled is true, hardware PSR is then disabled in
msm_dp_display_post_enable():

msm_dp_display_post_enable() {
    ...
    if (msm_dp_display->psr_supported)
        msm_dp_ctrl_config_psr(dp->ctrl, dp->panel, !dp->panel->vsc_enabled);
    ...
}

However, msm_edp_bridge_atomic_check() only checks the colorspace against
DRM_MODE_COLORIMETRY_DEFAULT to determine if the connector is self-refresh
aware. Because it misses the check for YUV420 modes, the DRM core might
believe PSR is supported and active, while the driver explicitly disables
hardware PSR.

Could this cause the host to stop transmitting video data for self-refresh
while the panel is not actually in self-refresh mode, leading to a blank
screen or PSR entry timeouts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-msm-dp-hdr10-v1-0-dbbf8b2b42bc@radxa.com?part=10

Re: [PATCH 10/13] drm/msm/dp: Signal BT.2020 RGB on DP and eDP

[Dmitry Baryshkov]

On Wed, Sep 30, 2026 at 08:41:54PM +0800, Xilin Wu wrote:
> Expose the DP Colorspace property and carry VSC packets in bridge atomic
> state. Validate VSC support against cached sink capabilities and check it
> again after powering up the sink. Regenerate the packet if link training
> changes the SDR bit depth.

This doesn't sound nice either. Do you know how it gets handled by i915?

> Signal full-range RGB, matching the existing pixel path, and retain CTA
> range for YUV420. Reject BT2020_RGB with a YUV420-only mode. Clear stale
> MSA colorimetry bits and include VSC removal in peripheral flush requests.

This sounds like too many features for a single commit, sorry.

> 
> Force a modeset when colorspace changes. Disable PSR in the controller and
> sink while colorimetry VSC packets are active, since they cannot share the
> automatic PSR packet generator. Restore PSR configuration when returning
> to a stream that does not need colorimetry VSC packets.
> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@radxa.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c    | 33 ++++++++++++++++------
>  drivers/gpu/drm/msm/dp/dp_ctrl.h    |  2 +-
>  drivers/gpu/drm/msm/dp/dp_display.c | 26 +++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_drm.c     | 13 +++++++--
>  drivers/gpu/drm/msm/dp/dp_drm.h     |  3 ++
>  drivers/gpu/drm/msm/dp/dp_panel.c   | 56 +++++--------------------------------
>  drivers/gpu/drm/msm/dp/dp_panel.h   |  4 +++
>  drivers/gpu/drm/msm/dp/dp_utils.c   | 38 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_utils.h   |  7 +++++
>  9 files changed, 118 insertions(+), 64 deletions(-)
> 

-- 
With best wishes
Dmitry

[PATCH 11/13] drm/msm/dp: Initialize the debugfs connector pointer

[Xilin Wu]

The debugfs cleanup removed the connector assignment from the debug
context, but the video-pattern test files still dereference that pointer
to check connector status. Reading those files can therefore dereference
NULL.

Store the supplied connector before creating the debugfs files.

Fixes: ab8420418c2e ("drm/msm/dp: cleanup debugfs handling")

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_debug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c
index cf3838fcd154..00fb1a7ba40e 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.c
+++ b/drivers/gpu/drm/msm/dp/dp_debug.c
@@ -216,6 +216,7 @@ int msm_dp_debug_init(struct device *dev, struct msm_dp_panel *panel,
 
 	debug->link = link;
 	debug->panel = panel;
+	debug->connector = connector;
 
 	debugfs_create_file("dp_debug", 0444, root,
 			debug, &msm_dp_debug_fops);

-- 
2.55.0

Re: [PATCH 11/13] drm/msm/dp: Initialize the debugfs connector pointer

[Dmitry Baryshkov]

On Wed, Sep 30, 2026 at 08:41:55PM +0800, Xilin Wu wrote:
> The debugfs cleanup removed the connector assignment from the debug
> context, but the video-pattern test files still dereference that pointer
> to check connector status. Reading those files can therefore dereference
> NULL.
> 
> Store the supplied connector before creating the debugfs files.
> 
> Fixes: ab8420418c2e ("drm/msm/dp: cleanup debugfs handling")
> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@radxa.com>

Please don't add empty lines between the tags. With the commit message
fixed:


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>



> ---
>  drivers/gpu/drm/msm/dp/dp_debug.c | 1 +
>  1 file changed, 1 insertion(+)
> 

-- 
With best wishes
Dmitry

[PATCH 12/13] drm/msm/dp: Serialize video test state changes

[Xilin Wu]

The debugfs test control writes video_test directly, racing HPD handling
and mode programming. Reading the test flag and its requested depth in
separate calls also allows those values to change between the reads.

Route test state access through the display and use plugged_lock for both
HPD and debugfs updates. Hold the same lock while selecting the test depth
and installing the mode. Pass the display instance to debugfs for access
to its connector and the serialized test controls.

Preserve the existing SDR test behavior. The shared setter provides the
entry point for rejecting incompatible tests when HDR is added.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/dp/dp_debug.c   | 27 ++++++++++++++-------------
 drivers/gpu/drm/msm/dp/dp_debug.h   |  8 +++++---
 drivers/gpu/drm/msm/dp/dp_display.c | 34 ++++++++++++++++++++++------------
 drivers/gpu/drm/msm/dp/dp_display.h |  2 +-
 4 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c
index 00fb1a7ba40e..c26b356f998a 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.c
+++ b/drivers/gpu/drm/msm/dp/dp_debug.c
@@ -21,7 +21,7 @@
 struct msm_dp_debug_private {
 	struct msm_dp_link *link;
 	struct msm_dp_panel *panel;
-	struct drm_connector *connector;
+	struct msm_dp *dp;
 };
 
 static int msm_dp_debug_show(struct seq_file *seq, void *p)
@@ -89,7 +89,7 @@ DEFINE_SHOW_ATTRIBUTE(msm_dp_debug);
 static int msm_dp_test_data_show(struct seq_file *m, void *data)
 {
 	const struct msm_dp_debug_private *debug = m->private;
-	const struct drm_connector *connector = debug->connector;
+	const struct drm_connector *connector = debug->dp->connector;
 	u32 bpc;
 
 	if (connector->status == connector_status_connected) {
@@ -111,7 +111,7 @@ DEFINE_SHOW_ATTRIBUTE(msm_dp_test_data);
 static int msm_dp_test_type_show(struct seq_file *m, void *data)
 {
 	const struct msm_dp_debug_private *debug = m->private;
-	const struct drm_connector *connector = debug->connector;
+	const struct drm_connector *connector = debug->dp->connector;
 
 	if (connector->status == connector_status_connected)
 		seq_printf(m, "%02x", DP_TEST_LINK_VIDEO_PATTERN);
@@ -133,7 +133,7 @@ static ssize_t msm_dp_test_active_write(struct file *file,
 	int val = 0;
 
 	debug = ((struct seq_file *)file->private_data)->private;
-	connector = debug->connector;
+	connector = debug->dp->connector;
 
 	if (len == 0)
 		return 0;
@@ -154,10 +154,11 @@ static ssize_t msm_dp_test_active_write(struct file *file,
 		/* To prevent erroneous activation of the compliance
 		 * testing code, only accept an actual value of 1 here
 		 */
-		if (val == 1)
-			debug->panel->video_test = true;
-		else
-			debug->panel->video_test = false;
+		status = msm_dp_display_set_video_test(debug->dp, val == 1);
+		if (status) {
+			kfree(input_buffer);
+			return status;
+		}
 	}
 	kfree(input_buffer);
 
@@ -168,10 +169,10 @@ static ssize_t msm_dp_test_active_write(struct file *file,
 static int msm_dp_test_active_show(struct seq_file *m, void *data)
 {
 	struct msm_dp_debug_private *debug = m->private;
-	struct drm_connector *connector = debug->connector;
+	struct drm_connector *connector = debug->dp->connector;
 
 	if (connector->status == connector_status_connected) {
-		if (debug->panel->video_test)
+		if (msm_dp_display_check_video_test(debug->dp))
 			seq_puts(m, "1");
 		else
 			seq_puts(m, "0");
@@ -200,12 +201,12 @@ static const struct file_operations test_active_fops = {
 
 int msm_dp_debug_init(struct device *dev, struct msm_dp_panel *panel,
 		  struct msm_dp_link *link,
-		  struct drm_connector *connector,
+		  struct msm_dp *dp,
 		  struct dentry *root, bool is_edp)
 {
 	struct msm_dp_debug_private *debug;
 
-	if (!dev || !panel || !link) {
+	if (!dev || !panel || !link || !dp || !dp->connector) {
 		DRM_ERROR("invalid input\n");
 		return -EINVAL;
 	}
@@ -216,7 +217,7 @@ int msm_dp_debug_init(struct device *dev, struct msm_dp_panel *panel,
 
 	debug->link = link;
 	debug->panel = panel;
-	debug->connector = connector;
+	debug->dp = dp;
 
 	debugfs_create_file("dp_debug", 0444, root,
 			debug, &msm_dp_debug_fops);
diff --git a/drivers/gpu/drm/msm/dp/dp_debug.h b/drivers/gpu/drm/msm/dp/dp_debug.h
index a90083fec856..430891e110fb 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.h
+++ b/drivers/gpu/drm/msm/dp/dp_debug.h
@@ -9,6 +9,8 @@
 #include "dp_panel.h"
 #include "dp_link.h"
 
+struct msm_dp;
+
 #if defined(CONFIG_DEBUG_FS)
 
 /**
@@ -17,7 +19,7 @@
  * @dev: device instance of the caller
  * @panel: instance of panel module
  * @link: instance of link module
- * @connector: double pointer to display connector
+ * @dp: display instance owning the connector and video test state
  * @root: connector's debugfs root
  * @is_edp: set for eDP connectors / panels
  * return: pointer to allocated debug module data
@@ -27,7 +29,7 @@
  */
 int msm_dp_debug_init(struct device *dev, struct msm_dp_panel *panel,
 		  struct msm_dp_link *link,
-		  struct drm_connector *connector,
+		  struct msm_dp *dp,
 		  struct dentry *root,
 		  bool is_edp);
 
@@ -36,7 +38,7 @@ int msm_dp_debug_init(struct device *dev, struct msm_dp_panel *panel,
 static inline
 int msm_dp_debug_init(struct device *dev, struct msm_dp_panel *panel,
 		  struct msm_dp_link *link,
-		  struct drm_connector *connector,
+		  struct msm_dp *dp,
 		  struct dentry *root,
 		  bool is_edp)
 {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 65ac35e41ed8..ec65800f629a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -426,10 +426,20 @@ static void msm_dp_display_host_deinit(struct msm_dp_display_private *dp)
 	dp->core_initialized = false;
 }
 
+static int msm_dp_display_set_video_test_locked(struct msm_dp_display_private *dp,
+						bool enable)
+{
+	lockdep_assert_held(&dp->plugged_lock);
+
+	dp->panel->video_test = enable;
+	return 0;
+}
+
 static void msm_dp_display_handle_video_request(struct msm_dp_display_private *dp)
 {
 	if (dp->link->sink_request & DP_TEST_LINK_VIDEO_PATTERN) {
-		dp->panel->video_test = true;
+		if (msm_dp_display_set_video_test_locked(dp, true))
+			dp->link->test_response = DP_TEST_NAK;
 		msm_dp_link_send_test_response(dp->link);
 	}
 }
@@ -671,8 +681,10 @@ static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
 
 	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
 
-	if (msm_dp_display_check_video_test(msm_dp_display))
-		bpp = msm_dp_display_get_test_bpp(msm_dp_display);
+	guard(mutex)(&dp->plugged_lock);
+
+	if (dp->panel->video_test)
+		bpp = msm_dp_link_bit_depth_to_bpp(dp->link->test_video.test_bit_depth);
 	else
 		bpp = state->bpp;
 
@@ -940,22 +952,20 @@ bool msm_dp_display_check_video_test(struct msm_dp *dp)
 
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
 
+	guard(mutex)(&msm_dp_display->plugged_lock);
+
 	return msm_dp_display->panel->video_test;
 }
 
-int msm_dp_display_get_test_bpp(struct msm_dp *dp)
+int msm_dp_display_set_video_test(struct msm_dp *dp, bool enable)
 {
 	struct msm_dp_display_private *msm_dp_display;
 
-	if (!dp) {
-		DRM_ERROR("invalid params\n");
-		return 0;
-	}
-
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
 
-	return msm_dp_link_bit_depth_to_bpp(
-		msm_dp_display->link->test_video.test_bit_depth);
+	guard(mutex)(&msm_dp_display->plugged_lock);
+
+	return msm_dp_display_set_video_test_locked(msm_dp_display, enable);
 }
 
 void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
@@ -1505,7 +1515,7 @@ void msm_dp_display_debugfs_init(struct msm_dp *msm_dp_display, struct dentry *r
 	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
 	dev = &dp->msm_dp_display.pdev->dev;
 
-	rc = msm_dp_debug_init(dev, dp->panel, dp->link, dp->msm_dp_display.connector, root, is_edp);
+	rc = msm_dp_debug_init(dev, dp->panel, dp->link, msm_dp_display, root, is_edp);
 	if (rc)
 		DRM_ERROR("failed to initialize debug, rc = %d\n", rc);
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 6a77e5952530..ac64c1d3227a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -30,7 +30,7 @@ struct msm_dp {
 
 int msm_dp_display_get_modes(struct msm_dp *msm_dp_display);
 bool msm_dp_display_check_video_test(struct msm_dp *msm_dp_display);
-int msm_dp_display_get_test_bpp(struct msm_dp *msm_dp_display);
+int msm_dp_display_set_video_test(struct msm_dp *dp, bool enable);
 void msm_dp_display_signal_audio_start(struct msm_dp *msm_dp_display);
 void msm_dp_display_signal_audio_complete(struct msm_dp *msm_dp_display);
 void msm_dp_display_set_psr(struct msm_dp *dp, bool enter);

-- 
2.55.0

[PATCH 13/13] drm/msm/dp: Add static HDR metadata support for DP and eDP

[Xilin Wu]

Expose HDR_OUTPUT_METADATA and validate and pack it during atomic check.
Program Generic2 from the checked bridge state on every stream enable.
Reuse the DRM metadata conversion and infoframe packing helpers, omit
the HDMI checksum when constructing the DP SDP, and zero reserved bytes.

Require 10 bpc for HDR transfer functions and reject insufficient
bandwidth after link training instead of silently reducing color depth.
Revalidate sink HDR capabilities after powering up an eDP panel, using
the stream enable failure path if the requested output is unsupported.

Force a modeset for metadata changes and include metadata enable and
disable in peripheral flush requests. Keep HDR streams out of PSR and
clear Generic2 enable state when disabling the stream. Preserve the
software metadata across teardown so that every stream rebuild restores
the checked packets.

Give checked HDR modesets precedence over SDR video-pattern tests. Reject
new test requests with NAK while HDR is configured and return -EBUSY from
the debugfs test control. Clear the test override under plugged_lock when
applying HDR so that a request arriving after atomic check cannot replace
the checked output depth. Keep pending test colorimetry from overriding
HDR signalling. Atomic checks leave the running test state untouched.

Assisted-by: LLM
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/gpu/drm/msm/Kconfig         |  1 +
 drivers/gpu/drm/msm/dp/dp_ctrl.c    |  9 ++++--
 drivers/gpu/drm/msm/dp/dp_display.c | 39 +++++++++++++++++++++---
 drivers/gpu/drm/msm/dp/dp_drm.c     |  4 ++-
 drivers/gpu/drm/msm/dp/dp_drm.h     |  2 ++
 drivers/gpu/drm/msm/dp/dp_panel.c   | 33 ++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_panel.h   |  3 ++
 drivers/gpu/drm/msm/dp/dp_utils.c   | 61 ++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/dp/dp_utils.h   |  5 ++-
 9 files changed, 145 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index ed1c4775d1e9..e988e6509ec8 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -119,6 +119,7 @@ config DRM_MSM_DP
 	depends on DRM_MSM
 	depends on DRM_MSM_KMS
 	select DRM_DISPLAY_HDMI_AUDIO_HELPER
+	select DRM_DISPLAY_HDMI_HELPER
 	select RATIONAL
 	select DRM_DISPLAY_DP_AUX_BUS
 	select DRM_DISPLAY_DP_HELPER
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 3362d65ee4e2..56a18fdb9d18 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -471,7 +471,9 @@ static void msm_dp_ctrl_config_misc1_misc0(struct msm_dp_ctrl_private *ctrl,
 
 	test_bits_depth = msm_dp_link_get_test_bits_depth(ctrl->link,
 							  msm_dp_panel->msm_dp_mode.bpp);
-	colorimetry_cfg = msm_dp_link_get_colorimetry_config(ctrl->link);
+	/* A video test request must not override the HDR quantization range. */
+	colorimetry_cfg = msm_dp_panel->hdr_enabled ? DP_MISC0_COLORIMERY_CFG_LEGACY_RGB :
+		msm_dp_link_get_colorimetry_config(ctrl->link);
 
 	misc_val = msm_dp_read_link(ctrl, REG_DP_MISC1_MISC0);
 
@@ -2635,7 +2637,9 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *
 			clock /= 2;
 		ret = msm_dp_utils_select_bpp(panel->msm_dp_mode.bpp / 3, 10,
 					      clock, ctrl->link->link_params.rate,
-					      ctrl->link->link_params.num_lanes);
+					      ctrl->link->link_params.num_lanes,
+					      panel->hdr_enabled && panel->hdr_sdp.db[2] !=
+					      HDMI_EOTF_TRADITIONAL_GAMMA_SDR);
 		if (ret < 0)
 			return ret;
 		panel->msm_dp_mode.bpp = ret;
@@ -2721,6 +2725,7 @@ void msm_dp_ctrl_off_link(struct msm_dp_ctrl *msm_dp_ctrl,
 	/* Link startup may have failed before enabling the link clocks. */
 	if (ctrl->link_clks_on) {
 		msm_dp_panel_disable_vsc_sdp(panel);
+		msm_dp_panel_disable_hdr_sdp(panel);
 		msm_dp_ctrl_mainlink_disable(ctrl);
 	}
 
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index ec65800f629a..ad2359818ff2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -431,6 +431,9 @@ static int msm_dp_display_set_video_test_locked(struct msm_dp_display_private *d
 {
 	lockdep_assert_held(&dp->plugged_lock);
 
+	if (enable && dp->panel->hdr_enabled)
+		return -EBUSY;
+
 	dp->panel->video_test = enable;
 	return 0;
 }
@@ -683,12 +686,19 @@ static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
 
 	guard(mutex)(&dp->plugged_lock);
 
+	/* A checked HDR modeset supersedes any intervening video test request. */
+	if (state->hdr_enabled)
+		dp->panel->video_test = false;
+
 	if (dp->panel->video_test)
 		bpp = msm_dp_link_bit_depth_to_bpp(dp->link->test_video.test_bit_depth);
 	else
 		bpp = state->bpp;
 
-	msm_dp_panel->sdp_needs_flush = msm_dp_panel->vsc_enabled || state->vsc_enabled;
+	msm_dp_panel->sdp_needs_flush = msm_dp_panel->vsc_enabled || state->vsc_enabled ||
+		msm_dp_panel->hdr_enabled || state->hdr_enabled;
+	msm_dp_panel->hdr_enabled = state->hdr_enabled;
+	msm_dp_panel->hdr_sdp = state->hdr_sdp;
 	msm_dp_panel->colorspace = state->colorspace;
 	msm_dp_panel->vsc_enabled = state->vsc_enabled;
 	msm_dp_panel->vsc_sdp = state->vsc_sdp;
@@ -708,7 +718,7 @@ int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
 	const struct drm_display_info *info = &conn_state->connector->display_info;
 	struct msm_dp_display_private *dp;
 	unsigned int clock = mode->clock;
-	int bpp;
+	int bpp, ret;
 
 	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
 
@@ -717,17 +727,25 @@ int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
 
 	guard(mutex)(&dp->plugged_lock);
 
+	ret = msm_dp_utils_pack_hdr_sdp(conn_state, &state->hdr_sdp);
+	if (ret)
+		return ret;
+
+	state->hdr_enabled = !!conn_state->hdr_output_metadata;
 	bpp = msm_dp_utils_select_bpp(info->bpc, conn_state->max_requested_bpc,
 				      clock, dp->panel->link_caps_valid ?
 				      dp->panel->link_info.rate : 0,
-				      dp->panel->link_info.num_lanes);
+				      dp->panel->link_info.num_lanes,
+				      state->hdr_enabled && state->hdr_sdp.db[2] !=
+				      HDMI_EOTF_TRADITIONAL_GAMMA_SDR);
 	if (bpp < 0)
 		return bpp;
 
 	state->bpp = bpp;
 	state->colorspace = conn_state->colorspace;
 	state->vsc_enabled = drm_mode_is_420_only(info, mode) ||
-			     state->colorspace != DRM_MODE_COLORIMETRY_DEFAULT;
+			     state->colorspace != DRM_MODE_COLORIMETRY_DEFAULT ||
+			     state->hdr_enabled;
 	if (!state->vsc_enabled)
 		return 0;
 
@@ -760,6 +778,18 @@ static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
 
 	dp->stream_pm_active = true;
 
+	if (dp->panel->hdr_enabled) {
+		const struct drm_display_info *info = &msm_dp_display->connector->display_info;
+		const struct hdr_static_metadata *hdr = &info->hdr_sink_metadata.hdmi_type1;
+		u8 eotf = dp->panel->hdr_sdp.db[2];
+
+		if (dp->panel->hw_revision < DP_HW_VERSION_1_0 ||
+		    !(hdr->metadata_type & BIT(HDMI_STATIC_METADATA_TYPE1)) ||
+		    !(hdr->eotf & BIT(eotf)) ||
+		    (eotf != HDMI_EOTF_TRADITIONAL_GAMMA_SDR && info->bpc < 10))
+			return -EINVAL;
+	}
+
 	if (dp->panel->vsc_enabled && !dp->panel->vsc_sdp_supported)
 		return -EINVAL;
 
@@ -850,6 +880,7 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp,
 		return 0;
 
 	msm_dp_panel_disable_vsc_sdp(msm_dp_panel);
+	msm_dp_panel_disable_hdr_sdp(msm_dp_panel);
 
 	msm_dp_ctrl_off_pixel_clk(dp->ctrl);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 48f2b7e79683..1ee57c7365ba 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -209,7 +209,8 @@ static int msm_edp_bridge_atomic_check(struct drm_bridge *drm_bridge,
 		return -ENODEV;
 
 	conn_state->self_refresh_aware = dp->psr_supported &&
-		conn_state->colorspace == DRM_MODE_COLORIMETRY_DEFAULT;
+		conn_state->colorspace == DRM_MODE_COLORIMETRY_DEFAULT &&
+		!conn_state->hdr_output_metadata;
 
 	if (!conn_state->crtc || !crtc_state)
 		return 0;
@@ -459,6 +460,7 @@ struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
 	if (ret)
 		return ERR_PTR(ret);
 	drm_connector_attach_colorspace_property(connector);
+	drm_connector_attach_hdr_output_metadata_property(connector);
 
 	ret = drm_connector_attach_max_bpc_property(connector, 6, 10);
 	if (ret)
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
index c9bd79949f79..42bf0566a736 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.h
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -23,6 +23,8 @@ struct msm_dp_bridge_state {
 	enum drm_colorspace colorspace;
 	bool vsc_enabled;
 	struct dp_sdp vsc_sdp;
+	bool hdr_enabled;
+	struct dp_sdp hdr_sdp;
 };
 
 #define to_dp_bridge_state(x) container_of((x), struct msm_dp_bridge_state, base)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 69327333f7f1..cac63e14bee2 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -528,6 +528,34 @@ void msm_dp_panel_disable_vsc_sdp(struct msm_dp_panel *msm_dp_panel)
 	msm_dp_panel_update_sdp(panel);
 }
 
+static void msm_dp_panel_enable_hdr_sdp(struct msm_dp_panel *msm_dp_panel)
+{
+	struct msm_dp_panel_private *panel =
+		container_of(msm_dp_panel, struct msm_dp_panel_private, msm_dp_panel);
+	u32 cfg, cfg2;
+
+	msm_dp_panel_send_sdp(panel, &msm_dp_panel->hdr_sdp, MMSS_DP_GENERIC2_0);
+
+	cfg2 = msm_dp_read_link(panel, MMSS_DP_SDP_CFG2);
+	msm_dp_write_link(panel, MMSS_DP_SDP_CFG2, cfg2 | GENERIC2_SDPSIZE_VALID);
+	cfg = msm_dp_read_link(panel, MMSS_DP_SDP_CFG);
+	msm_dp_write_link(panel, MMSS_DP_SDP_CFG, cfg | GEN2_SDP_EN);
+	msm_dp_panel_update_sdp(panel);
+}
+
+void msm_dp_panel_disable_hdr_sdp(struct msm_dp_panel *msm_dp_panel)
+{
+	struct msm_dp_panel_private *panel =
+		container_of(msm_dp_panel, struct msm_dp_panel_private, msm_dp_panel);
+	u32 cfg, cfg2;
+
+	cfg = msm_dp_read_link(panel, MMSS_DP_SDP_CFG);
+	msm_dp_write_link(panel, MMSS_DP_SDP_CFG, cfg & ~GEN2_SDP_EN);
+	cfg2 = msm_dp_read_link(panel, MMSS_DP_SDP_CFG2);
+	msm_dp_write_link(panel, MMSS_DP_SDP_CFG2, cfg2 & ~GENERIC2_SDPSIZE_VALID);
+	msm_dp_panel_update_sdp(panel);
+}
+
 int msm_dp_panel_timing_cfg(struct msm_dp_panel *msm_dp_panel, bool wide_bus_en)
 {
 	u32 data, total_ver, total_hor;
@@ -602,6 +630,11 @@ int msm_dp_panel_timing_cfg(struct msm_dp_panel *msm_dp_panel, bool wide_bus_en)
 	else
 		msm_dp_panel_disable_vsc_sdp(msm_dp_panel);
 
+	if (msm_dp_panel->hdr_enabled)
+		msm_dp_panel_enable_hdr_sdp(msm_dp_panel);
+	else
+		msm_dp_panel_disable_hdr_sdp(msm_dp_panel);
+
 	panel->panel_on = true;
 
 	return 0;
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 4b3403b7a300..fcf4315b6ac9 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -43,6 +43,8 @@ struct msm_dp_panel {
 	bool sdp_needs_flush;
 	enum drm_colorspace colorspace;
 	struct dp_sdp vsc_sdp;
+	bool hdr_enabled;
+	struct dp_sdp hdr_sdp;
 	u32 hw_revision;
 
 	u32 max_bw_code;
@@ -65,6 +67,7 @@ void msm_dp_panel_clear_dsc_dto(struct msm_dp_panel *msm_dp_panel);
 
 void msm_dp_panel_enable_vsc_sdp(struct msm_dp_panel *msm_dp_panel, struct dp_sdp *vsc_sdp);
 void msm_dp_panel_disable_vsc_sdp(struct msm_dp_panel *msm_dp_panel);
+void msm_dp_panel_disable_hdr_sdp(struct msm_dp_panel *msm_dp_panel);
 
 /**
  * is_link_rate_valid() - validates the link rate
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c b/drivers/gpu/drm/msm/dp/dp_utils.c
index 657d376d32da..33f5c349e89c 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.c
+++ b/drivers/gpu/drm/msm/dp/dp_utils.c
@@ -4,6 +4,8 @@
  */
 
 #include <linux/types.h>
+#include <drm/display/drm_hdmi_helper.h>
+#include <drm/drm_property.h>
 #include <drm/drm_crtc.h>
 
 #include "dp_utils.h"
@@ -92,16 +94,19 @@ void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
 /* A zero link rate means that a powered-down eDP panel has not been probed. */
 int msm_dp_utils_select_bpp(unsigned int bpc, unsigned int max_bpc,
 			    unsigned int clock, unsigned int rate,
-			    unsigned int lanes)
+			    unsigned int lanes, bool hdr)
 {
 	unsigned int bpp;
 
+	if (hdr && bpc < 10)
+		return -EINVAL;
+
 	bpc = min3(bpc ? bpc : 8, max_bpc, 10U);
 	bpc = round_down(bpc, 2);
-	if (bpc < 6)
+	if (bpc < (hdr ? 10 : 6))
 		return -EINVAL;
 
-	for (bpp = bpc * 3; bpp >= 18; bpp -= 6) {
+	for (bpp = bpc * 3; bpp >= (hdr ? 30 : 18); bpp -= 6) {
 		if (!rate || (u64)clock * bpp <= (u64)rate * lanes * 8)
 			return bpp;
 	}
@@ -140,11 +145,59 @@ int msm_dp_utils_pack_vsc_sdp(struct dp_sdp *sdp, enum drm_colorspace colorspace
 	return ret < 0 ? ret : 0;
 }
 
+int msm_dp_utils_pack_hdr_sdp(const struct drm_connector_state *conn_state,
+			      struct dp_sdp *sdp)
+{
+	const struct hdr_output_metadata *metadata;
+	const struct hdr_static_metadata *sink;
+	struct hdmi_drm_infoframe frame;
+	u8 buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_DRM_INFOFRAME_SIZE];
+	int ret;
+
+	memset(sdp, 0, sizeof(*sdp));
+	if (!conn_state->hdr_output_metadata)
+		return 0;
+
+	if (conn_state->hdr_output_metadata->length != sizeof(*metadata))
+		return -EINVAL;
+
+	metadata = conn_state->hdr_output_metadata->data;
+	sink = &conn_state->connector->display_info.hdr_sink_metadata.hdmi_type1;
+	if (metadata->metadata_type != HDMI_STATIC_METADATA_TYPE1 ||
+	    metadata->hdmi_metadata_type1.metadata_type != HDMI_STATIC_METADATA_TYPE1 ||
+	    metadata->hdmi_metadata_type1.eotf > HDMI_EOTF_BT_2100_HLG)
+		return -EINVAL;
+
+	if (!(sink->metadata_type & BIT(HDMI_STATIC_METADATA_TYPE1)) ||
+	    !(sink->eotf & BIT(metadata->hdmi_metadata_type1.eotf)))
+		return -EINVAL;
+
+	ret = drm_hdmi_infoframe_set_hdr_metadata(&frame, conn_state);
+	if (ret)
+		return ret;
+
+	ret = hdmi_drm_infoframe_pack_only(&frame, buffer, sizeof(buffer));
+	if (ret < 0)
+		return ret;
+
+	sdp->sdp_header.HB1 = HDMI_INFOFRAME_TYPE_DRM;
+	sdp->sdp_header.HB2 = 0x1d;
+	sdp->sdp_header.HB3 = 0x13 << 2;
+	sdp->db[0] = frame.version;
+	sdp->db[1] = frame.length;
+	/* DP carries the version and length, but no HDMI checksum. */
+	memcpy(&sdp->db[2], &buffer[HDMI_INFOFRAME_HEADER_SIZE],
+	       HDMI_DRM_INFOFRAME_SIZE);
+
+	return 0;
+}
+
 bool msm_dp_utils_needs_modeset(struct drm_connector_state *old_state,
 				struct drm_connector_state *new_state)
 {
 	return old_state->max_requested_bpc != new_state->max_requested_bpc ||
-	       old_state->colorspace != new_state->colorspace;
+	       old_state->colorspace != new_state->colorspace ||
+	       !drm_connector_atomic_hdr_metadata_equal(old_state, new_state);
 }
 
 bool msm_dp_utils_psr_needs_modeset(const struct drm_crtc_state *state)
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h
index 146c652e995e..93b46ba186a2 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.h
+++ b/drivers/gpu/drm/msm/dp/dp_utils.h
@@ -39,11 +39,14 @@ void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
 
 int msm_dp_utils_select_bpp(unsigned int bpc, unsigned int max_bpc,
 			    unsigned int clock, unsigned int rate,
-			    unsigned int lanes);
+			    unsigned int lanes, bool hdr);
 
 int msm_dp_utils_pack_vsc_sdp(struct dp_sdp *sdp, enum drm_colorspace colorspace,
 			      unsigned int bpc, bool yuv420);
 
+int msm_dp_utils_pack_hdr_sdp(const struct drm_connector_state *conn_state,
+			      struct dp_sdp *sdp);
+
 bool msm_dp_utils_needs_modeset(struct drm_connector_state *old_state,
 				struct drm_connector_state *new_state);
 

-- 
2.55.0

Re: [PATCH 13/13] drm/msm/dp: Add static HDR metadata support for DP and eDP

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dp: late HDR capability check in atomic_enable violates atomic contract
--

commit dd06afce910f68ea34833e6875a317b2680f0949
Author: Xilin Wu <sophon@radxa.com>

drm/msm/dp: Add static HDR metadata support for DP and eDP

This commit adds static HDR metadata processing for DP and eDP displays.
It introduces Generic2 packet formatting, revalidates sink capabilities,
and forces a modeset on metadata changes.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -708,7 +718,7 @@ int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
>  	const struct drm_display_info *info = &conn_state->connector->display_info;
>  	struct msm_dp_display_private *dp;
>  	unsigned int clock = mode->clock;
> -	int bpp;
> +	int bpp, ret;
>  
>  	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>  
> @@ -717,17 +727,25 @@ int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
>  
>  	guard(mutex)(&dp->plugged_lock);
>  
> +	ret = msm_dp_utils_pack_hdr_sdp(conn_state, &state->hdr_sdp);
> +	if (ret)
> +		return ret;
> +
> +	state->hdr_enabled = !!conn_state->hdr_output_metadata;

[Severity: High]
Should the hardware capability validation for HDR be moved here?

This is the correct function where the DP controller's hw_revision and all
static sink HDR capability checks must be performed. Validating here ensures
that userspace receives an error prior to the commit, rather than failing
late during the enable phase.

[ ... ]
> @@ -760,6 +778,18 @@ static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
>  
>  	dp->stream_pm_active = true;
>  
> +	if (dp->panel->hdr_enabled) {
> +		const struct drm_display_info *info = &msm_dp_display->connector->display_info;
> +		const struct hdr_static_metadata *hdr = &info->hdr_sink_metadata.hdmi_type1;
> +		u8 eotf = dp->panel->hdr_sdp.db[2];
> +
> +		if (dp->panel->hw_revision < DP_HW_VERSION_1_0 ||
> +		    !(hdr->metadata_type & BIT(HDMI_STATIC_METADATA_TYPE1)) ||
> +		    !(hdr->eotf & BIT(eotf)) ||
> +		    (eotf != HDMI_EOTF_TRADITIONAL_GAMMA_SDR && info->bpc < 10))
> +			return -EINVAL;
> +	}
> +

[Severity: High]
Does this late hardware capability validation violate the atomic contract?

Returning an error during the atomic_enable phase (from 
msm_dp_display_prepare_link) causes a silent modesetting failure where the 
hardware state diverges from the software state, leading to a wedged display 
pipeline and DRM core warnings.

If a local user with DRM master privileges submits an atomic commit with HDR 
metadata on a DP controller with an older hw_revision, the commit passes 
validation but fails here. The atomic modesetting contract requires all such 
configuration constraints to be rejected during msm_dp_display_atomic_check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-msm-dp-hdr10-v1-0-dbbf8b2b42bc@radxa.com?part=13

Re: [PATCH 13/13] drm/msm/dp: Add static HDR metadata support for DP and eDP

[Dmitry Baryshkov]

On Wed, Sep 30, 2026 at 08:41:57PM +0800, Xilin Wu wrote:
> Expose HDR_OUTPUT_METADATA and validate and pack it during atomic check.
> Program Generic2 from the checked bridge state on every stream enable.
> Reuse the DRM metadata conversion and infoframe packing helpers, omit
> the HDMI checksum when constructing the DP SDP, and zero reserved bytes.
> 
> Require 10 bpc for HDR transfer functions and reject insufficient
> bandwidth after link training instead of silently reducing color depth.
> Revalidate sink HDR capabilities after powering up an eDP panel, using
> the stream enable failure path if the requested output is unsupported.
> 
> Force a modeset for metadata changes and include metadata enable and
> disable in peripheral flush requests. Keep HDR streams out of PSR and
> clear Generic2 enable state when disabling the stream. Preserve the
> software metadata across teardown so that every stream rebuild restores
> the checked packets.
> 
> Give checked HDR modesets precedence over SDR video-pattern tests. Reject
> new test requests with NAK while HDR is configured and return -EBUSY from
> the debugfs test control. Clear the test override under plugged_lock when
> applying HDR so that a request arriving after atomic check cannot replace
> the checked output depth. Keep pending test colorimetry from overriding
> HDR signalling. Atomic checks leave the running test state untouched.
> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@radxa.com>
> ---
>  drivers/gpu/drm/msm/Kconfig         |  1 +
>  drivers/gpu/drm/msm/dp/dp_ctrl.c    |  9 ++++--
>  drivers/gpu/drm/msm/dp/dp_display.c | 39 +++++++++++++++++++++---
>  drivers/gpu/drm/msm/dp/dp_drm.c     |  4 ++-
>  drivers/gpu/drm/msm/dp/dp_drm.h     |  2 ++
>  drivers/gpu/drm/msm/dp/dp_panel.c   | 33 ++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_panel.h   |  3 ++
>  drivers/gpu/drm/msm/dp/dp_utils.c   | 61 ++++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/msm/dp/dp_utils.h   |  5 ++-
>  9 files changed, 145 insertions(+), 12 deletions(-)
> 
> @@ -717,17 +727,25 @@ int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
>  
>  	guard(mutex)(&dp->plugged_lock);
>  
> +	ret = msm_dp_utils_pack_hdr_sdp(conn_state, &state->hdr_sdp);
> +	if (ret)
> +		return ret;
> +
> +	state->hdr_enabled = !!conn_state->hdr_output_metadata;

This should be done in atomic_check. Also please make sure that we don't
copy data from the state to the long-term structures. I'd rather pass
state pointer around.

>  	bpp = msm_dp_utils_select_bpp(info->bpc, conn_state->max_requested_bpc,
>  				      clock, dp->panel->link_caps_valid ?
>  				      dp->panel->link_info.rate : 0,
> -				      dp->panel->link_info.num_lanes);
> +				      dp->panel->link_info.num_lanes,
> +				      state->hdr_enabled && state->hdr_sdp.db[2] !=
> +				      HDMI_EOTF_TRADITIONAL_GAMMA_SDR);
>  	if (bpp < 0)
>  		return bpp;
>  
>  	state->bpp = bpp;
>  	state->colorspace = conn_state->colorspace;
>  	state->vsc_enabled = drm_mode_is_420_only(info, mode) ||
> -			     state->colorspace != DRM_MODE_COLORIMETRY_DEFAULT;
> +			     state->colorspace != DRM_MODE_COLORIMETRY_DEFAULT ||
> +			     state->hdr_enabled;
>  	if (!state->vsc_enabled)
>  		return 0;
>  

-- 
With best wishes
Dmitry

Re: [PATCH 00/13] drm/msm/dp: Add static HDR support for DP and eDP

[Dmitry Baryshkov]

On Wed, 30 Sep 2026 20:41:44 +0800, Xilin Wu wrote:
> This series adds standard KMS static HDR metadata and BT.2020 RGB
> signalling to MSM DP and eDP, with HDR10 as the intended use case. It uses
> HDR_OUTPUT_METADATA, Colorspace and max bpc. Userspace supplies the HDR
> pixel content; DSC, dynamic HDR and tone mapping are outside this series.
> 
> Patch 1 allows max bpc registration before connector state allocation and
> initializes the corresponding state defaults. Patch 2 prepares generic
> SDP slots. Patches 3-5 establish PHY power ownership, serialize HPD state
> updates and add complete stream enable rollback, including AUX recovery
> for a connected external peer. New error returns are introduced together
> with their cleanup paths.
> 
> [...]

Applied to ttt, thanks!

[04/13] drm/msm/dp: Serialize HPD state updates
        https://gitlab.freedesktop.org/lumag/msm/-/commit/62c6146e5fde

Best regards,
-- 
With best wishes
Dmitry