[PATCH v2 0/3] drm/dp: shared HDR metadata SDP helper, and max bpc for dw-dp

[PATCH v2 0/3] drm/dp: shared HDR metadata SDP helper, and max bpc for dw-dp

[Igor Paunovic]

This is v2 of "drm/bridge: synopsys: dw-dp: Add HDR support" [1], resent
as three patches instead of five.

The two patches that actually enabled HDR output on dw-dp are not here.
They were written on top of Sebastian's "Synopsys DisplayPort Controller
improvements for RK3588" series, and that series has not landed. I
checked rather than assumed: against drm-misc-next they no longer apply,
and every failing hunk sits on context that only exists in that series -
vsc_sdp_nr, audio_sdp_nr, dw_dp_clear_sdp(), the runtime PM conversion
and dw_dp_rockchip_hpd_sw_cfg(). None of those symbols are in mainline.
Holding the rest hostage to that seemed wrong when the first three
patches stand on their own, so they go alone and I will resend the other
two once the dw-dp work is in.

What is left is useful without them:

 1/3 moves i915's private HDR metadata SDP packing into a shared DP
     helper and converts i915 to use it. No functional change; the
     second user follows later. Jani's Ack from v1 is carried over.

 2/3 fixes a real bug: the bridge connector loses its max bpc value on
     connector reset, so a driver that attaches the property gets it
     reset behind userspace's back.

 3/3 attaches the "max bpc" property on dw-dp. To be precise about what
     that does: the property is a limit, not an enabler.
     drm_atomic_connector_check() derives max_bpc from the sink's EDID
     and only clamps it against max_requested_bpc when the property
     exists, so the min() can never raise it. dw-dp's format filter
     therefore already lets its 10 bpc entries through on a sink that
     reports 10 bpc, with no property attached. What the property adds
     is userspace control - the ability to ask for less - and a defined
     upper bound of 10, the deepest entry in dw_dp_output_formats[]. It
     is also the case that motivated 2/3, and a prerequisite for the
     HDR patches that follow later.

Changes in v2:
 - Dropped 4/5 and 5/5; they depend on the dw-dp improvements series
   and no longer apply to drm-misc-next.
 - Retitled the series, since it no longer adds HDR support by itself.
 - Rebased onto drm-misc-next bf63a72ea005. No changes to the code.

One thing worth flagging for 2/3 and 3/3: Cristian's HDMI 2.0 series
removes connector->max_bpc in its patch 71. That series is not in
drm-misc-next either, so these two are correct as they stand today, but
if it lands first I will respin them against whatever replaces that
field. Say the word if you would rather I wait for it.

A note on how this was made, since the patches carry Assisted-by tags.
I work in a pair: I run the board, take the measurements and decide what
goes in; an LLM assistant (Claude, by Anthropic) helps me read the
source, draft the changelogs and re-check my claims against the tree
before I send anything.

What I have not done here is boot-test this exact three-patch series on
hardware. The code is unchanged from v1, whose runtime testing was done
on an Orange Pi 5 Plus running a rockchip-devel kernel that carried the
v9 generation of the dw-dp series, with the patches on top; those runtime
tests were never repeated on plain drm-misc-next. That caveat bites
hardest on 3/3: mainline's dw_dp-rockchip.c is not the file I tested
against. For v2 I have only checked that the patches apply and build-
tested nothing. Say so and I will do a full build and boot test before
you take it.

[1] https://lore.kernel.org/all/20260808095749.9428-1-royalnet026@gmail.com/

Igor Paunovic (3):
  drm/dp: Add drm_dp_hdr_metadata_infoframe_sdp_pack()
  drm/display: bridge-connector: Preserve max bpc across connector reset
  drm/rockchip: dw_dp: Attach "max bpc" connector property

 drivers/gpu/drm/display/drm_bridge_connector.c |  6 +-
 drivers/gpu/drm/display/drm_dp_helper.c        | 88 ++++++++++++++++++++++++++
 drivers/gpu/drm/drm_connector.c                |  5 ++
 drivers/gpu/drm/i915/display/intel_dp.c        | 84 +-----------------------
 drivers/gpu/drm/rockchip/dw_dp-rockchip.c      | 26 ++++++++
 include/drm/display/drm_dp_helper.h            |  4 ++
 6 files changed, 131 insertions(+), 82 deletions(-)

base-commit: bf63a72ea005d5e1cc12a9ad5e228c4b44246b0f

Re: [PATCH v2 0/3] drm/dp: shared HDR metadata SDP helper, and max bpc for dw-dp

[Igor Paunovic]

Following up on my own cover letter, where I said the series had not
been built.

It has now. On top of drm-misc-next bf63a72ea005, x86_64 allmodconfig,
building the five objects the series touches with W=1:

  drivers/gpu/drm/display/drm_dp_helper.o
  drivers/gpu/drm/display/drm_bridge_connector.o
  drivers/gpu/drm/drm_connector.o
  drivers/gpu/drm/i915/display/intel_dp.o
  drivers/gpu/drm/rockchip/dw_dp-rockchip.o

All five build with no errors and no new warnings. intel_dp.o is the one
I most wanted to see, since 1/3 moves code out of it.

To be exact about what this is and is not: it is a compile test only. I
have still not booted this exact series, for the reason given in the
cover - the runtime testing behind v1 was done on a rockchip-devel
kernel carrying the v9 generation of the dw-dp series, and mainline's
dw_dp-rockchip.c is not the file that was exercised. If you want a boot
test on the Orange Pi 5 Plus before taking this, say so and I will do it.

Igor

Re: [PATCH v2 0/3] drm/dp: shared HDR metadata SDP helper, and max bpc for dw-dp

[Igor Paunovic]

The three patches of this series never reached the lists. The mistake
is mine: when sending, only the cover letter carried the recipients,
so the patches went to nobody but my own inbox. I noticed while
checking the archive after the fact.

Resent below as replies to this cover, byte-identical to what should
have arrived with it. Sorry for the noise.

Igor

[PATCH v2 1/3] drm/dp: Add drm_dp_hdr_metadata_infoframe_sdp_pack()

[Igor Paunovic]

i915 packs HDR static metadata into an HDR Metadata InfoFrame SDP
(DP 1.4a spec, Table 2-100 and Table 2-101) with a driver-private
helper, intel_dp_hdr_metadata_infoframe_sdp_pack(). Nothing in it is
i915 specific: it converts a generic struct hdmi_drm_infoframe into a
generic struct dp_sdp.

Move it to drm_dp_helper.c as drm_dp_hdr_metadata_infoframe_sdp_pack(),
next to drm_dp_vsc_sdp_pack(), and convert i915 to the new helper. The
Synopsys DesignWare DisplayPort bridge driver (dw-dp) needs to send the
same SDP to support HDR and should not have to duplicate the packing
code.

The helper takes a struct drm_device pointer in place of the i915
display pointer for its two debug messages, and an i915-specific
comment about GEN11+ GMP register sizes is dropped. No functional
change.

Assisted-by: LLM checkpatch
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c | 88 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c | 84 +----------------------
 include/drm/display/drm_dp_helper.h     |  4 ++
 3 files changed, 95 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index e623ccb4c1d8a..7b80937255fe0 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -25,6 +25,7 @@
 #include <linux/dynamic_debug.h>
 #include <linux/errno.h>
 #include <linux/export.h>
+#include <linux/hdmi.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/iopoll.h>
@@ -3672,6 +3673,93 @@ ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
 }
 EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
 
+/**
+ * drm_dp_hdr_metadata_infoframe_sdp_pack() - pack HDR Metadata InfoFrame SDP
+ * @dev: DRM device
+ * @drm_infoframe: HDMI DRM infoframe carrying the HDR static metadata
+ * @sdp: valid handle to the generic dp_sdp which will be packed
+ * @size: valid size of the passed sdp handle
+ *
+ * Pack a CTA-861 Dynamic Range and Mastering infoframe into an HDR
+ * Metadata InfoFrame SDP, as defined in DP 1.4a spec, Table 2-100 and
+ * Table 2-101.
+ *
+ * Returns: length of sdp on success and error code on failure
+ */
+ssize_t drm_dp_hdr_metadata_infoframe_sdp_pack(struct drm_device *dev,
+					       const struct hdmi_drm_infoframe *drm_infoframe,
+					       struct dp_sdp *sdp,
+					       size_t size)
+{
+	size_t length = sizeof(struct dp_sdp);
+	const int infoframe_size = HDMI_INFOFRAME_HEADER_SIZE + HDMI_DRM_INFOFRAME_SIZE;
+	unsigned char buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_DRM_INFOFRAME_SIZE];
+	ssize_t len;
+
+	if (size < length)
+		return -ENOSPC;
+
+	memset(sdp, 0, size);
+
+	len = hdmi_drm_infoframe_pack_only(drm_infoframe, buf, sizeof(buf));
+	if (len < 0) {
+		drm_dbg_kms(dev,
+			    "buffer size is smaller than hdr metadata infoframe\n");
+		return -ENOSPC;
+	}
+
+	if (len != infoframe_size) {
+		drm_dbg_kms(dev, "wrong static hdr metadata size\n");
+		return -ENOSPC;
+	}
+
+	/*
+	 * Set up the infoframe sdp packet for HDR static metadata.
+	 * Prepare VSC Header for SU as per DP 1.4a spec,
+	 * Table 2-100 and Table 2-101
+	 */
+
+	/* Secondary-Data Packet ID, 00h for non-Audio INFOFRAME */
+	sdp->sdp_header.HB0 = 0;
+	/*
+	 * Packet Type 80h + Non-audio INFOFRAME Type value
+	 * HDMI_INFOFRAME_TYPE_DRM: 0x87
+	 * - 80h + Non-audio INFOFRAME Type value
+	 * - InfoFrame Type: 0x07
+	 *    [CTA-861-G Table-42 Dynamic Range and Mastering InfoFrame]
+	 */
+	sdp->sdp_header.HB1 = drm_infoframe->type;
+	/*
+	 * Least Significant Eight Bits of (Data Byte Count – 1)
+	 * infoframe_size - 1
+	 */
+	sdp->sdp_header.HB2 = 0x1D;
+	/* INFOFRAME SDP Version Number */
+	sdp->sdp_header.HB3 = (0x13 << 2);
+	/* CTA Header Byte 2 (INFOFRAME Version Number) */
+	sdp->db[0] = drm_infoframe->version;
+	/* CTA Header Byte 3 (Length of INFOFRAME): HDMI_DRM_INFOFRAME_SIZE */
+	sdp->db[1] = drm_infoframe->length;
+	/*
+	 * Copy HDMI_DRM_INFOFRAME_SIZE size from a buffer after
+	 * HDMI_INFOFRAME_HEADER_SIZE
+	 */
+	BUILD_BUG_ON(sizeof(sdp->db) < HDMI_DRM_INFOFRAME_SIZE + 2);
+	memcpy(&sdp->db[2], &buf[HDMI_INFOFRAME_HEADER_SIZE],
+	       HDMI_DRM_INFOFRAME_SIZE);
+
+	/*
+	 * Size of DP infoframe sdp packet for HDR static metadata consists of
+	 * - DP SDP Header(struct dp_sdp_header): 4 bytes
+	 * - Two Data Blocks: 2 bytes
+	 *    CTA Header Byte2 (INFOFRAME Version Number)
+	 *    CTA Header Byte3 (Length of INFOFRAME)
+	 * - HDMI_DRM_INFOFRAME_SIZE: 26 bytes
+	 */
+	return sizeof(struct dp_sdp_header) + 2 + HDMI_DRM_INFOFRAME_SIZE;
+}
+EXPORT_SYMBOL(drm_dp_hdr_metadata_infoframe_sdp_pack);
+
 /**
  * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
  * @dpcd: DisplayPort configuration data
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 113d767e62e9c..d0d3f517df85a 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5189,84 +5189,6 @@ static ssize_t intel_dp_as_sdp_pack(const struct drm_dp_as_sdp *as_sdp,
 	return length;
 }
 
-static ssize_t
-intel_dp_hdr_metadata_infoframe_sdp_pack(struct intel_display *display,
-					 const struct hdmi_drm_infoframe *drm_infoframe,
-					 struct dp_sdp *sdp,
-					 size_t size)
-{
-	size_t length = sizeof(struct dp_sdp);
-	const int infoframe_size = HDMI_INFOFRAME_HEADER_SIZE + HDMI_DRM_INFOFRAME_SIZE;
-	unsigned char buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_DRM_INFOFRAME_SIZE];
-	ssize_t len;
-
-	if (size < length)
-		return -ENOSPC;
-
-	memset(sdp, 0, size);
-
-	len = hdmi_drm_infoframe_pack_only(drm_infoframe, buf, sizeof(buf));
-	if (len < 0) {
-		drm_dbg_kms(display->drm,
-			    "buffer size is smaller than hdr metadata infoframe\n");
-		return -ENOSPC;
-	}
-
-	if (len != infoframe_size) {
-		drm_dbg_kms(display->drm, "wrong static hdr metadata size\n");
-		return -ENOSPC;
-	}
-
-	/*
-	 * Set up the infoframe sdp packet for HDR static metadata.
-	 * Prepare VSC Header for SU as per DP 1.4a spec,
-	 * Table 2-100 and Table 2-101
-	 */
-
-	/* Secondary-Data Packet ID, 00h for non-Audio INFOFRAME */
-	sdp->sdp_header.HB0 = 0;
-	/*
-	 * Packet Type 80h + Non-audio INFOFRAME Type value
-	 * HDMI_INFOFRAME_TYPE_DRM: 0x87
-	 * - 80h + Non-audio INFOFRAME Type value
-	 * - InfoFrame Type: 0x07
-	 *    [CTA-861-G Table-42 Dynamic Range and Mastering InfoFrame]
-	 */
-	sdp->sdp_header.HB1 = drm_infoframe->type;
-	/*
-	 * Least Significant Eight Bits of (Data Byte Count – 1)
-	 * infoframe_size - 1
-	 */
-	sdp->sdp_header.HB2 = 0x1D;
-	/* INFOFRAME SDP Version Number */
-	sdp->sdp_header.HB3 = (0x13 << 2);
-	/* CTA Header Byte 2 (INFOFRAME Version Number) */
-	sdp->db[0] = drm_infoframe->version;
-	/* CTA Header Byte 3 (Length of INFOFRAME): HDMI_DRM_INFOFRAME_SIZE */
-	sdp->db[1] = drm_infoframe->length;
-	/*
-	 * Copy HDMI_DRM_INFOFRAME_SIZE size from a buffer after
-	 * HDMI_INFOFRAME_HEADER_SIZE
-	 */
-	BUILD_BUG_ON(sizeof(sdp->db) < HDMI_DRM_INFOFRAME_SIZE + 2);
-	memcpy(&sdp->db[2], &buf[HDMI_INFOFRAME_HEADER_SIZE],
-	       HDMI_DRM_INFOFRAME_SIZE);
-
-	/*
-	 * Size of DP infoframe sdp packet for HDR static metadata consists of
-	 * - DP SDP Header(struct dp_sdp_header): 4 bytes
-	 * - Two Data Blocks: 2 bytes
-	 *    CTA Header Byte2 (INFOFRAME Version Number)
-	 *    CTA Header Byte3 (Length of INFOFRAME)
-	 * - HDMI_DRM_INFOFRAME_SIZE: 26 bytes
-	 *
-	 * Prior to GEN11's GMP register size is identical to DP HDR static metadata
-	 * infoframe size. But GEN11+ has larger than that size, write_infoframe
-	 * will pad rest of the size.
-	 */
-	return sizeof(struct dp_sdp_header) + 2 + HDMI_DRM_INFOFRAME_SIZE;
-}
-
 static void intel_write_dp_sdp(struct intel_encoder *encoder,
 			       const struct intel_crtc_state *crtc_state,
 			       unsigned int type)
@@ -5285,9 +5207,9 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder,
 		len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp);
 		break;
 	case HDMI_PACKET_TYPE_GAMUT_METADATA:
-		len = intel_dp_hdr_metadata_infoframe_sdp_pack(display,
-							       &crtc_state->infoframes.drm.drm,
-							       &sdp, sizeof(sdp));
+		len = drm_dp_hdr_metadata_infoframe_sdp_pack(display->drm,
+							     &crtc_state->infoframes.drm.drm,
+							     &sdp, sizeof(sdp));
 		break;
 	case DP_SDP_ADAPTIVE_SYNC:
 		len = intel_dp_as_sdp_pack(&crtc_state->infoframes.as_sdp, &sdp,
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index 5b05a7fbe174a..3e3f119775b38 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -32,6 +32,7 @@
 struct drm_device;
 struct drm_dp_aux;
 struct drm_panel;
+struct hdmi_drm_infoframe;
 
 bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
 			  int lane_count);
@@ -1035,6 +1036,9 @@ int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
 int drm_dp_max_dprx_data_rate(int max_link_rate, int max_lanes);
 
 ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, struct dp_sdp *sdp);
+ssize_t drm_dp_hdr_metadata_infoframe_sdp_pack(struct drm_device *dev,
+					       const struct hdmi_drm_infoframe *drm_infoframe,
+					       struct dp_sdp *sdp, size_t size);
 int drm_dp_link_symbol_cycles(int lane_count, int pixels, int dsc_slice_count,
 			      int bpp_x16, int symbol_size, bool is_mst);
 
-- 
2.43.0

[PATCH v2 2/3] drm/display: bridge-connector: Preserve max bpc across connector reset

[Igor Paunovic]

drm_connector_attach_max_bpc_property() initializes max_requested_bpc
and max_bpc in the connector state, but nothing restores them when the
connector state is later thrown away and re-created:
drm_bridge_connector_create_state() returns a zeroed state for
non-HDMI bridge connectors, and drm_mode_config_reset() installs such
a fresh state on every connector that implements
&drm_connector_funcs.atomic_create_state instead of a .reset hook.

This matters because drivers can attach the property before
drm_mode_config_reset() runs. Rockchip, for example, binds its
component drivers (which create connectors and attach properties)
before calling drm_mode_config_reset(). After that reset,
max_requested_bpc is 0, so drm_atomic_connector_check() computes
max_bpc = min(info->bpc ?: 8, 0) = 0. Any driver that filters output
formats on conn_state->max_bpc then rejects every format, and clients
that never set the "max bpc" property - fbcon in particular - end up
with a black screen. meson already attaches the property on a
drm_bridge_connector before drm_mode_config_reset() and so already
boots with max_requested_bpc = 0 today. dw-hdmi's format negotiation
does consume the value there, but meson attaches the property with a
maximum of 8 and the 8-bit fallback formats are not gated on it, so
negotiation yields the same result for 0 and 8 - the change is a
no-op for meson.

HDMI bridge connectors are immune: drm_bridge_connector_create_state()
calls __drm_atomic_helper_connector_hdmi_state_init(), which
initializes both fields from connector->max_bpc. amdgpu likewise
re-initializes max_requested_bpc in its own .reset implementation.
Non-HDMI bridge connectors have no equivalent.

Mirror the HDMI helper on the non-HDMI path: record the upper attach
limit in connector->max_bpc (drmm_connector_hdmi_init() already stores
the same value there) and restore max_requested_bpc and max_bpc from
it in drm_bridge_connector_create_state() whenever the "max bpc"
property is attached. Connectors without the property behave exactly
as before.

Assisted-by: LLM checkpatch
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 6 +++++-
 drivers/gpu/drm/drm_connector.c                | 5 +++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 00feb73e63d7f..6197fb80f7cc2 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -279,9 +279,13 @@ drm_bridge_connector_create_state(struct drm_connector *connector)
 	if (IS_ERR(conn_state))
 		return conn_state;
 
-	if (bridge_connector->bridge_hdmi)
+	if (bridge_connector->bridge_hdmi) {
 		__drm_atomic_helper_connector_hdmi_state_init(connector,
 							      conn_state);
+	} else if (connector->max_bpc_property) {
+		conn_state->max_requested_bpc = connector->max_bpc;
+		conn_state->max_bpc = connector->max_bpc;
+	}
 
 	return conn_state;
 }
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 8b4baed060f3a..0400a6a92e2b7 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2866,6 +2866,10 @@ EXPORT_SYMBOL(drm_connector_set_link_status_property);
  * @max: The maximum bit depth supported by the connector.
  *
  * This is used to add support for limiting the bit depth on a connector.
+ * @max is also recorded in &drm_connector.max_bpc, so that
+ * &drm_connector_funcs.atomic_create_state and &drm_connector_funcs.reset
+ * implementations can restore the property default when re-creating the
+ * connector state.
  *
  * Returns:
  * Zero on success, negative errno on failure.
@@ -2888,6 +2892,7 @@ 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;
+	connector->max_bpc = max;
 
 	return 0;
 }
-- 
2.43.0

[PATCH v2 3/3] drm/rockchip: dw_dp: Attach "max bpc" connector property

[Igor Paunovic]

The dw-dp bridge already filters candidate output formats on
conn_state->max_bpc in dw_dp_bridge_atomic_get_output_bus_fmts() and
validates the result against the link bandwidth, but the connector
never exposes the "max bpc" property, so userspace cannot limit the
bit depth. Attach the property with a range of 6 to 10, matching
dw_dp_output_formats[]: the smallest entry is 6 bpc RGB and the
deepest entries are 10 bpc.

drm_connector_attach_max_bpc_property() initializes
connector->state->max_requested_bpc and therefore requires the
connector to have a state. Rockchip binds its components before
drm_mode_config_reset() runs, so the freshly created bridge connector
does not have one yet - create the connector state before attaching
the property, exactly like drmm_connector_hdmi_init() does for HDMI
bridge connectors. The later drm_mode_config_reset() then re-creates
the connector state, and the preceding patch makes
drm_bridge_connector_create_state() restore max_requested_bpc from
connector->max_bpc, so clients that never set the property (fbcon in
particular) keep the full range instead of ending up with 0.

With the property attached, compositors such as KWin write "max bpc"
on every atomic modeset; any in-range value is handled by the existing
format filter and bandwidth checks.

Assisted-by: LLM checkpatch
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
 drivers/gpu/drm/rockchip/dw_dp-rockchip.c | 26 +++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
index b23efb153c9e6..8ff27353b1417 100644
--- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
@@ -112,6 +112,32 @@ static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *
 				     "Failed to init bridge connector\n");
 	}
 
+	/*
+	 * The components are bound before drm_mode_config_reset() runs, so
+	 * the connector has no state yet, but
+	 * drm_connector_attach_max_bpc_property() requires one. Create the
+	 * connector state first, like drmm_connector_hdmi_init() does.
+	 */
+	if (connector->funcs->atomic_create_state) {
+		struct drm_connector_state *state;
+
+		state = connector->funcs->atomic_create_state(connector);
+		if (IS_ERR(state)) {
+			dw_dp_unbind(dp->base);
+			return PTR_ERR(state);
+		}
+
+		connector->state = state;
+	} else if (connector->funcs->reset) {
+		connector->funcs->reset(connector);
+	}
+
+	ret = drm_connector_attach_max_bpc_property(connector, 6, 10);
+	if (ret) {
+		dw_dp_unbind(dp->base);
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v2 3/3] drm/rockchip: dw_dp: Attach "max bpc" connector property

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Unconditional connector state allocation causes a memory leak if an HDMI bridge is present in the chain.

Pre-existing issues:
- [High] `devm_` allocations tied to the component device in `dw_dp_bind` cause resource leaks and permanent driver failure if `bind` fails.
--

commit 47f0f730df97f46871d26a1858e6f2594ea7c383
Author: Igor Paunovic <royalnet026@gmail.com>

drm/rockchip: dw_dp: Attach "max bpc" connector property

This commit attaches the "max bpc" property to the dw-dp bridge connector
with a range of 6 to 10. It also explicitly creates the connector state
before drm_mode_config_reset() runs, allowing the property to be attached
successfully during driver initialization.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index b23efb153c9e6..8ff27353b1417 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
[ ... ]
> @@ -112,6 +112,32 @@ static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *
>  				     "Failed to init bridge connector\n");
>  	}
>  
> +	/*
> +	 * The components are bound before drm_mode_config_reset() runs, so
> +	 * the connector has no state yet, but
> +	 * drm_connector_attach_max_bpc_property() requires one. Create the
> +	 * connector state first, like drmm_connector_hdmi_init() does.
> +	 */
> +	if (connector->funcs->atomic_create_state) {
> +		struct drm_connector_state *state;
> +
> +		state = connector->funcs->atomic_create_state(connector);
> +		if (IS_ERR(state)) {
> +			dw_dp_unbind(dp->base);
> +			return PTR_ERR(state);
> +		}
> +
> +		connector->state = state;

[Severity: Medium]
Does this unconditionally overwrite connector->state?

If the display pipeline contains an HDMI bridge (e.g., a DP-to-HDMI chip),
drm_bridge_connector_init() internally calls drmm_connector_hdmi_init(),
which allocates and assigns connector->state. Overwriting it here seems to
leak the previously allocated drm_connector_state.

> +	} else if (connector->funcs->reset) {
> +		connector->funcs->reset(connector);
> +	}
> +
> +	ret = drm_connector_attach_max_bpc_property(connector, 6, 10);
> +	if (ret) {
> +		dw_dp_unbind(dp->base);
> +		return ret;

[Severity: High]
This isn't a regression introduced by this patch, but if this newly added
error path is taken, does it permanently break the driver on deferred probe?

The component bind callback invokes dw_dp_bind() earlier in this function,
which performs devm_ allocations tied to the component device (dev) rather
than the master device. For example:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind() {
    ...
    ret = devm_request_threaded_irq(dev, dp->irq, NULL, dw_dp_irq,
                                    IRQF_ONESHOT, dev_name(dev), dp);
    ...
}

Because the component device remains probed even if the bind callback fails,
these devm_ resources are not freed when this error path returns. On a
subsequent bind retry, dw_dp_bind() executes again. Since
devm_request_threaded_irq() does not use IRQF_SHARED, it will return
-EBUSY, permanently failing.

> +	}
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909163206.36409-1-royalnet026@gmail.com?part=3

Re: [PATCH v2 3/3] drm/rockchip: dw_dp: Attach "max bpc" connector property

[Igor Paunovic]

The bot is right, and it is worse than it reported. I went and read the
code rather than taking either its word or my own.

drm_bridge_connector_init() has two paths. With an HDMI bridge in the
chain it calls drmm_connector_hdmi_init() (drm_bridge_connector.c:1016);
otherwise drmm_connector_init() (:1027). And drmm_connector_hdmi_init()
already contains, at drm_connector.c:621-631, the exact block this patch
adds - same shape, same comment, because that is where I took it from -
and then calls drm_connector_attach_max_bpc_property() itself at :633.

So on a chain with an HDMI bridge my patch does two wrong things, not
one:

 - it overwrites a connector->state that has just been allocated, which
   is the leak the bot found;

 - it calls drm_connector_attach_max_bpc_property() a second time. That
   one is not a leak: the helper reuses connector->max_bpc_property when
   it already exists, so my 6..10 range is silently discarded and the
   8..max_bpc property created by the HDMI path stays. It does re-attach
   the same property to the object and overwrite state->max_bpc.

On the boards I care about there is no HDMI bridge behind dw-dp, so
neither fires today - but "does not fire on my board" is not a reason to
leave it.

For v3 both blocks get guarded on what is already there:

	if (!connector->state) {
		... create the state ...
	}

	if (!connector->max_bpc_property) {
		ret = drm_connector_attach_max_bpc_property(connector, 6, 10);
		...
	}

That leaves a question I would rather ask than paper over. The state
creation exists only in drmm_connector_hdmi_init(), while
drm_bridge_connector_init() leaves the plain path without a state - so
any bridge driver that wants a connector property at bind time has to
copy that block, as I did. Would it be better to move it into
drm_bridge_connector_init() so both paths get a state, and have drivers
just attach their properties? I am happy to write that instead, but it
touches shared code and I would rather be told than guess.

On the pre-existing [High]: the bot is right that the error path I add
inherits it. dw_dp_bind() takes devm resources on the component device,
including a non-shared devm_request_threaded_irq(), so a failed bind
leaves them held and the retry gets -EBUSY. My dw_dp_unbind() call does
not release devres. With the guards above the new path becomes hard to
reach, but it does not stop being wrong, and I did not introduce it.
I can send a separate patch moving those to the master device if the
maintainers want it - it is a different change from this one.

Igor

Re: [PATCH v2 3/3] drm/rockchip: dw_dp: Attach "max bpc" connector property

[Chaoyi Chen]

Hello Igor,

On 9/10/2026 1:35 AM, Igor Paunovic wrote:
> The dw-dp bridge already filters candidate output formats on
> conn_state->max_bpc in dw_dp_bridge_atomic_get_output_bus_fmts() and
> validates the result against the link bandwidth, but the connector
> never exposes the "max bpc" property, so userspace cannot limit the
> bit depth. Attach the property with a range of 6 to 10, matching
> dw_dp_output_formats[]: the smallest entry is 6 bpc RGB and the
> deepest entries are 10 bpc.
> 
> drm_connector_attach_max_bpc_property() initializes
> connector->state->max_requested_bpc and therefore requires the
> connector to have a state. Rockchip binds its components before
> drm_mode_config_reset() runs, so the freshly created bridge connector
> does not have one yet - create the connector state before attaching
> the property, exactly like drmm_connector_hdmi_init() does for HDMI
> bridge connectors. The later drm_mode_config_reset() then re-creates
> the connector state, and the preceding patch makes
> drm_bridge_connector_create_state() restore max_requested_bpc from
> connector->max_bpc, so clients that never set the property (fbcon in
> particular) keep the full range instead of ending up with 0.
> 
> With the property attached, compositors such as KWin write "max bpc"
> on every atomic modeset; any in-range value is handled by the existing
> format filter and bandwidth checks.
> 
> Assisted-by: LLM checkpatch
> Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
> ---
>  drivers/gpu/drm/rockchip/dw_dp-rockchip.c | 26 +++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index b23efb153c9e6..8ff27353b1417 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> @@ -112,6 +112,32 @@ static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *
>  				     "Failed to init bridge connector\n");
>  	}
>  
> +	/*
> +	 * The components are bound before drm_mode_config_reset() runs, so
> +	 * the connector has no state yet, but
> +	 * drm_connector_attach_max_bpc_property() requires one. Create the
> +	 * connector state first, like drmm_connector_hdmi_init() does.
> +	 */
> +	if (connector->funcs->atomic_create_state) {
> +		struct drm_connector_state *state;
> +
> +		state = connector->funcs->atomic_create_state(connector);
> +		if (IS_ERR(state)) {
> +			dw_dp_unbind(dp->base);
> +			return PTR_ERR(state);
> +		}
> +
> +		connector->state = state;
> +	} else if (connector->funcs->reset) {
> +		connector->funcs->reset(connector);
> +	}
> +
> +	ret = drm_connector_attach_max_bpc_property(connector, 6, 10);
> +	if (ret) {
> +		dw_dp_unbind(dp->base);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
> 

I don't think this is the correct way to go.

I believe max_bpc should be handled in drm_bridge_connector_init(). 
Take a look at its implementation. HDMI already does max_bpc there, 
so DP should probably do something similar.

-- 
Best, 
Chaoyi

Re: [PATCH v2 3/3] drm/rockchip: dw_dp: Attach "max bpc" connector property

[Igor Paunovic]

Hi Chaoyi,

On 9/10/26 09:16, Chaoyi Chen wrote:
> I don't think this is the correct way to go.
>
> I believe max_bpc should be handled in drm_bridge_connector_init().
> Take a look at its implementation. HDMI already does max_bpc there,
> so DP should probably do something similar.

Agreed - that is the question I left open in my reply to the bot. v3
moves it into drm_bridge_connector_init() and drops this glue patch.

Three points I would rather ask than guess, since it is shared code:

1. How the bridge advertises it. drm_bridge.max_bpc is documented as
   "only relevant if DRM_BRIDGE_OP_HDMI is set" and the helper only
   reads it inside the OP_HDMI block. I intend to honour it on the
   non-HDMI path too, relax that doc sentence, and have dw-dp set
   bridge->max_bpc = 10. If you prefer to keep the field HDMI-only,
   a separate field is easy - I just did not want to add one unasked.

2. The range. HDMI attaches 8..max_bpc. dw-dp's format table goes down
   to 6 bpc RGB and i915 attaches 6..N on its DP connectors, so I intend
   6..max_bpc when connector_type is DisplayPort, 8..max_bpc otherwise -
   the same gate Dmitry used for the DP subconnector property in his
   msm series [1]. Nothing is attached unless a bridge declared
   max_bpc, so existing non-HDMI bridge connectors are unchanged.

3. Where the default lives. 2/3 restores max_requested_bpc from
   connector->max_bpc after drm_mode_config_reset(), but Cristian's
   v11 71/74 removes that field [2] (HDMI keeps it in hdmi.funcs). I
   intend to keep the value in struct drm_bridge_connector and restore
   it from there in drm_bridge_connector_create_state(), which folds
   2/3 into the same patch. 1/3 is unaffected.

The state still has to exist before the property is attached, so the
non-HDMI path gets the same create-state block drmm_connector_hdmi_init()
has. If you would rather see that block factored into a helper for both
callers, say so and I will do that instead.

[1] https://lore.kernel.org/all/20260722-drm-msm-display-interface-v1-24-368c10fe62fd@oss.qualcomm.com/
[2] https://lore.kernel.org/all/20260901-dw-hdmi-qp-scramb-v11-71-bc12954a0688@collabora.com/

Thanks,
Igor

Re: [PATCH v2 3/3] drm/rockchip: dw_dp: Attach "max bpc" connector property

[Chaoyi Chen]

On 9/10/2026 3:51 PM, Igor Paunovic wrote:
> Hi Chaoyi,
> 
> On 9/10/26 09:16, Chaoyi Chen wrote:
>> I don't think this is the correct way to go.
>>
>> I believe max_bpc should be handled in drm_bridge_connector_init().
>> Take a look at its implementation. HDMI already does max_bpc there,
>> so DP should probably do something similar.
> 
> Agreed - that is the question I left open in my reply to the bot. v3
> moves it into drm_bridge_connector_init() and drops this glue patch.
> 
> Three points I would rather ask than guess, since it is shared code:
> 
> 1. How the bridge advertises it. drm_bridge.max_bpc is documented as
>    "only relevant if DRM_BRIDGE_OP_HDMI is set" and the helper only
>    reads it inside the OP_HDMI block. I intend to honour it on the
>    non-HDMI path too, relax that doc sentence, and have dw-dp set
>    bridge->max_bpc = 10. If you prefer to keep the field HDMI-only,
>    a separate field is easy - I just did not want to add one unasked.
>

Maybe we should consider adding a new "DRM_BRIDGE_OP_DP" :)

> 2. The range. HDMI attaches 8..max_bpc. dw-dp's format table goes down
>    to 6 bpc RGB and i915 attaches 6..N on its DP connectors, so I intend
>    6..max_bpc when connector_type is DisplayPort, 8..max_bpc otherwise -
>    the same gate Dmitry used for the DP subconnector property in his
>    msm series [1]. Nothing is attached unless a bridge declared
>    max_bpc, so existing non-HDMI bridge connectors are unchanged.
> 

And once you use DRM_BRIDGE_OP_DP for the check, it shouldn't affect
other users. The max_bpc looks reasonable.

> 3. Where the default lives. 2/3 restores max_requested_bpc from
>    connector->max_bpc after drm_mode_config_reset(), but Cristian's
>    v11 71/74 removes that field [2] (HDMI keeps it in hdmi.funcs). I
>    intend to keep the value in struct drm_bridge_connector and restore
>    it from there in drm_bridge_connector_create_state(), which folds
>    2/3 into the same patch. 1/3 is unaffected.
> 
> The state still has to exist before the property is attached, so the
> non-HDMI path gets the same create-state block drmm_connector_hdmi_init()
> has. If you would rather see that block factored into a helper for both
> callers, say so and I will do that instead.
> 

Perhaps @Cristian and @Maxime have better ideas?

> [1] https://lore.kernel.org/all/20260722-drm-msm-display-interface-v1-24-368c10fe62fd@oss.qualcomm.com/
> [2] https://lore.kernel.org/all/20260901-dw-hdmi-qp-scramb-v11-71-bc12954a0688@collabora.com/
> 
> Thanks,
> Igor

-- 
Best, 
Chaoyi

Re: [PATCH v2 3/3] drm/rockchip: dw_dp: Attach "max bpc" connector property

[Igor Paunovic]

Hi Chaoyi,

On 9/10/26 10:16, Chaoyi Chen wrote:
> Maybe we should consider adding a new "DRM_BRIDGE_OP_DP" :)

Works for me - it keeps the HDMI-only wording of max_bpc and makes the
check explicit. So that Maxime and Cristian have something concrete to
object to, this is all I would give the flag:

 - DRM_BRIDGE_OP_DP: the bridge drives a DisplayPort connector and
   fills in max_bpc. Nothing else is read from it for now.
 - drm_bridge_connector_init() treats it like DRM_BRIDGE_OP_HDMI:
   at most one such bridge in the chain (-EBUSY), max_bpc must be set
   (-EINVAL), and the connector gets "max bpc" with range 6..max_bpc.
 - dw-dp sets the flag and max_bpc = 10.

The connector itself stays a plain drmm_connector_init() one - no DP
counterpart of drmm_connector_hdmi_init() - so bridges without the flag
see no change at all.

> Perhaps @Cristian and @Maxime have better ideas?

That is point 3, and I will wait for it before writing v3: whether the
create-state block gets duplicated into the non-HDMI path or factored
out of drmm_connector_hdmi_init(), and whether keeping the restore value
in struct drm_bridge_connector is acceptable once 71/74 lands.

Thanks,
Igor

✗ LGCI.VerificationFailed: failure for drm/dp: shared HDR metadata SDP helper, and max bpc for dw-dp

[Patchwork]

== Series Details ==

Series: drm/dp: shared HDR metadata SDP helper, and max bpc for dw-dp
URL   : https://patchwork.freedesktop.org/series/173796/
State : failure

== Summary ==

Series author address 'royalnet026@gmail.com' is not on the allowlist, which prevents CI from being automatically triggered.
If you want CI to run for this series, ask Patchwork project owners to click 'retest' on the series in Patchwork.
Exception occurred during validation, bailing out!
Build URL: http://gfx-ci.igk.intel.com:8080/job/CI_PW_kernel/187129/ (on built-in)