diff mbox series

[v3,1/2] drm/display/dsc: Refactor DRM MST DSC Determination Policy

Message ID 20241106150444.424579-2-Jerry.Zuo@amd.com (mailing list archive)
State New
Headers show
Series Refactor MST DSC Determination Policy | expand

Commit Message

Fangzhi Zuo Nov. 6, 2024, 3:03 p.m. UTC
[why]
How we determine the dsc_aux used for dsc decompression in
drm_dp_mst_dsc_aux_for_port() today has defects:

1. The method how we determine a connected peer device is virtual or not
   in drm_dp_mst_is_virtual_dpcd() is not always correct. There are DP1.4 products
   in the market which don't fully comply with DP spec to enumerate virtual peer device.
   That leads to existing logic defects. For example:

   -  Some 1.4 mst hubs with hdmi output port don't enumerate virtual dpcd/peer device.
      When probing the hub, its topology is constructed with a branch device only, with
      peer device type set as DP-to-Legacy converter for its HDMI output port.
      Under this condition, drm_dp_mst_is_virtual_dpcd() will still determine it's connected
      with a virtual peer device with virtual dpcd. And results in the section for
      analyzing DP-to-DP peer device case of drm_dp_mst_dsc_aux_for_port(). That's logically
      incorrect.

2. Existing routine is designed based on analyzing different connected peer device types, such
   as dp-dp, dp-hdmi peer device, and virtual sink. Such categorization is redundant and unnecessary.
   The key info of determining where to do dsc decompression relies on the dsc capability from dpcd
   only. No matter the mst branch device enumerates virtual dpcd or not, if it's supporting dsc, it
   must declare it's dsc capability at somewhere within its responded topology.

Therefore, we would like to refactor the logic how we determine the dsc aux.

[how]
1. dsc_aux should be determined by the topology connection status and dpcd capability info only.
   In this way, dsc aux could be determined in a more generic way,
   instead of enumerating and analyzing on different connected peer device types.

2. Synaptics quirk DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD analyzing is no longer needed
   as long as we determine dsc aux generically by dpcd info.

Signed-off-by: Fangzhi Zuo <Jerry.Zuo@amd.com>
Signed-off-by: Wayne Lin <wayne.lin@amd.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 238 ++++++++----------
 include/drm/display/drm_dp_mst_helper.h       |   3 +
 2 files changed, 104 insertions(+), 137 deletions(-)

Comments

Dan Carpenter Nov. 7, 2024, 10:38 a.m. UTC | #1
Hi Fangzhi,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Fangzhi-Zuo/drm-display-dsc-Refactor-DRM-MST-DSC-Determination-Policy/20241106-230854
base:   git://anongit.freedesktop.org/drm/drm drm-next
patch link:    https://lore.kernel.org/r/20241106150444.424579-2-Jerry.Zuo%40amd.com
patch subject: [PATCH v3 1/2] drm/display/dsc: Refactor DRM MST DSC Determination Policy
config: i386-randconfig-141-20241107 (https://download.01.org/0day-ci/archive/20241107/202411071102.0CMTIEIo-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202411071102.0CMTIEIo-lkp@intel.com/

smatch warnings:
drivers/gpu/drm/display/drm_dp_mst_topology.c:6043 drm_dp_mst_dsc_aux_for_port() warn: variable dereferenced before check 'port' (see line 6033)

vim +/port +6043 drivers/gpu/drm/display/drm_dp_mst_topology.c

c2bc1b6eabe65d drivers/gpu/drm/drm_dp_mst_topology.c         David Francis 2019-08-26  6031  struct drm_dp_aux *drm_dp_mst_dsc_aux_for_port(struct drm_dp_mst_port *port)
c2bc1b6eabe65d drivers/gpu/drm/drm_dp_mst_topology.c         David Francis 2019-08-26  6032  {
fd05275592ae07 drivers/gpu/drm/display/drm_dp_mst_topology.c Fangzhi Zuo   2024-11-06 @6033  	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
                                                                                                                                      ^^^^^^^^^
Dereferenced

fd05275592ae07 drivers/gpu/drm/display/drm_dp_mst_topology.c Fangzhi Zuo   2024-11-06  6034  	struct drm_dp_mst_port *immediate_upstream_port = NULL;
fd05275592ae07 drivers/gpu/drm/display/drm_dp_mst_topology.c Fangzhi Zuo   2024-11-06  6035  	struct drm_dp_mst_port *fec_port = NULL;
fd05275592ae07 drivers/gpu/drm/display/drm_dp_mst_topology.c Fangzhi Zuo   2024-11-06  6036  	struct drm_dp_mst_port *dsc_port = NULL;
fd05275592ae07 drivers/gpu/drm/display/drm_dp_mst_topology.c Fangzhi Zuo   2024-11-06  6037  	struct drm_dp_aux *upstream_aux;
fd05275592ae07 drivers/gpu/drm/display/drm_dp_mst_topology.c Fangzhi Zuo   2024-11-06  6038  	bool end_has_dpcd = (port->dpcd_rev > 0);
fd05275592ae07 drivers/gpu/drm/display/drm_dp_mst_topology.c Fangzhi Zuo   2024-11-06  6039  	u8 endpoint_dsc = 0;
70a6587dca37a3 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak     2024-09-09  6040  	u8 upstream_dsc;
fd05275592ae07 drivers/gpu/drm/display/drm_dp_mst_topology.c Fangzhi Zuo   2024-11-06  6041  	u8 fec_cap;
c2bc1b6eabe65d drivers/gpu/drm/drm_dp_mst_topology.c         David Francis 2019-08-26  6042  
c2bc1b6eabe65d drivers/gpu/drm/drm_dp_mst_topology.c         David Francis 2019-08-26 @6043  	if (!port)
                                                                                                    ^^^^^
Checked too late.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index ac90118b9e7a..a4551c17a07f 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2258,6 +2258,8 @@  void drm_dp_mst_connector_early_unregister(struct drm_connector *connector,
 	drm_dbg_kms(port->mgr->dev, "unregistering %s remote bus for %s\n",
 		    port->aux.name, connector->kdev->kobj.name);
 	drm_dp_aux_unregister_devnode(&port->aux);
+	port->dsc_aux = NULL;
+	port->passthrough_aux = NULL;
 }
 EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
 
@@ -5994,57 +5996,6 @@  static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port)
 	i2c_del_adapter(&port->aux.ddc);
 }
 
-/**
- * drm_dp_mst_is_virtual_dpcd() - Is the given port a virtual DP Peer Device
- * @port: The port to check
- *
- * A single physical MST hub object can be represented in the topology
- * by multiple branches, with virtual ports between those branches.
- *
- * As of DP1.4, An MST hub with internal (virtual) ports must expose
- * certain DPCD registers over those ports. See sections 2.6.1.1.1
- * and 2.6.1.1.2 of Display Port specification v1.4 for details.
- *
- * May acquire mgr->lock
- *
- * Returns:
- * true if the port is a virtual DP peer device, false otherwise
- */
-static bool drm_dp_mst_is_virtual_dpcd(struct drm_dp_mst_port *port)
-{
-	struct drm_dp_mst_port *downstream_port;
-
-	if (!port || port->dpcd_rev < DP_DPCD_REV_14)
-		return false;
-
-	/* Virtual DP Sink (Internal Display Panel) */
-	if (drm_dp_mst_port_is_logical(port))
-		return true;
-
-	/* DP-to-HDMI Protocol Converter */
-	if (port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV &&
-	    !port->mcs &&
-	    port->ldps)
-		return true;
-
-	/* DP-to-DP */
-	mutex_lock(&port->mgr->lock);
-	if (port->pdt == DP_PEER_DEVICE_MST_BRANCHING &&
-	    port->mstb &&
-	    port->mstb->num_ports == 2) {
-		list_for_each_entry(downstream_port, &port->mstb->ports, next) {
-			if (downstream_port->pdt == DP_PEER_DEVICE_SST_SINK &&
-			    !downstream_port->input) {
-				mutex_unlock(&port->mgr->lock);
-				return true;
-			}
-		}
-	}
-	mutex_unlock(&port->mgr->lock);
-
-	return false;
-}
-
 /**
  * drm_dp_mst_aux_for_parent() - Get the AUX device for an MST port's parent
  * @port: MST port whose parent's AUX device is returned
@@ -6079,115 +6030,128 @@  EXPORT_SYMBOL(drm_dp_mst_aux_for_parent);
  */
 struct drm_dp_aux *drm_dp_mst_dsc_aux_for_port(struct drm_dp_mst_port *port)
 {
-	struct drm_dp_mst_port *immediate_upstream_port;
-	struct drm_dp_aux *immediate_upstream_aux;
-	struct drm_dp_mst_port *fec_port;
-	struct drm_dp_desc desc = {};
+	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
+	struct drm_dp_mst_port *immediate_upstream_port = NULL;
+	struct drm_dp_mst_port *fec_port = NULL;
+	struct drm_dp_mst_port *dsc_port = NULL;
+	struct drm_dp_aux *upstream_aux;
+	bool end_has_dpcd = (port->dpcd_rev > 0);
+	u8 endpoint_dsc = 0;
 	u8 upstream_dsc;
-	u8 endpoint_fec;
-	u8 endpoint_dsc;
+	u8 fec_cap;
 
 	if (!port)
 		return NULL;
 
+	port->dsc_aux = NULL;
+	port->passthrough_aux = NULL;
+
+	/* Policy start */
+	if (!drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
+		drm_err(mgr->dev,
+			"MST_DSC Can't determine dsc aux for port %p which is not connected to end device\n",
+			port);
+		return NULL;
+	}
+
 	if (port->parent->port_parent)
 		immediate_upstream_port = port->parent->port_parent;
-	else
-		immediate_upstream_port = NULL;
-
-	fec_port = immediate_upstream_port;
-	while (fec_port) {
-		/*
-		 * Each physical link (i.e. not a virtual port) between the
-		 * output and the primary device must support FEC
-		 */
-		if (!drm_dp_mst_is_virtual_dpcd(fec_port) &&
-		    !fec_port->fec_capable)
-			return NULL;
 
-		fec_port = fec_port->parent->port_parent;
+	if (end_has_dpcd) {
+		drm_info(mgr->dev, "MST_DSC check port %p for dsc decompression capability\n", port);
+		if (drm_dp_dpcd_read(&port->aux, DP_DSC_SUPPORT, &endpoint_dsc, 1) != 1) {
+			drm_err(mgr->dev, "MST_DSC Can't retrieve dsc caps from endpoint port\n");
+			goto out_dsc_fail;
+		}
 	}
 
-	/* DP-to-DP peer device */
-	if (drm_dp_mst_is_virtual_dpcd(immediate_upstream_port)) {
-		if (drm_dp_dpcd_read(&port->aux,
-				     DP_DSC_SUPPORT, &endpoint_dsc, 1) != 1)
-			return NULL;
-		if (drm_dp_dpcd_read(&port->aux,
-				     DP_FEC_CAPABILITY, &endpoint_fec, 1) != 1)
-			return NULL;
-		if (drm_dp_dpcd_read(&immediate_upstream_port->aux,
-				     DP_DSC_SUPPORT, &upstream_dsc, 1) != 1)
-			return NULL;
-
-		/* Enpoint decompression with DP-to-DP peer device */
-		if ((endpoint_dsc & DP_DSC_DECOMPRESSION_IS_SUPPORTED) &&
-		    (endpoint_fec & DP_FEC_CAPABLE) &&
-		    (upstream_dsc & DP_DSC_PASSTHROUGH_IS_SUPPORTED)) {
-			port->passthrough_aux = &immediate_upstream_port->aux;
-			return &port->aux;
-		}
+	if (immediate_upstream_port) {
+		upstream_aux = &immediate_upstream_port->aux;
+		drm_info(mgr->dev, "MST_DSC check immediate_upstream_port %p for dsc passthrough capability\n",
+				    immediate_upstream_port);
+	} else {
+		upstream_aux = mgr->aux;
+		drm_info(mgr->dev, "MST_DSC check root aux for dsc passthrough capability\n");
+	}
 
-		/* Virtual DPCD decompression with DP-to-DP peer device */
-		return &immediate_upstream_port->aux;
+	if (drm_dp_dpcd_read(upstream_aux, DP_DSC_SUPPORT, &upstream_dsc, 1) != 1) {
+		drm_err(mgr->dev, "MST_DSC Can't retrieve dsc caps from upstream port\n");
+		goto out_dsc_fail;
 	}
 
-	/* Virtual DPCD decompression with DP-to-HDMI or Virtual DP Sink */
-	if (drm_dp_mst_is_virtual_dpcd(port))
-		return &port->aux;
+	/* Consider passthrough as the first option for dsc_aux/passthrough_aux */
+	if (endpoint_dsc & DP_DSC_DECOMPRESSION_IS_SUPPORTED &&
+			upstream_dsc & DP_DSC_PASSTHROUGH_IS_SUPPORTED) {
+		dsc_port = port;
+		port->dsc_aux = &port->aux;
+		port->passthrough_aux = upstream_aux;
+		drm_info(mgr->dev, "MST_DSC dsc passthrough to endpoint\n");
+	}
 
-	/*
-	 * Synaptics quirk
-	 * Applies to ports for which:
-	 * - Physical aux has Synaptics OUI
-	 * - DPv1.4 or higher
-	 * - Port is on primary branch device
-	 * - Not a VGA adapter (DP_DWN_STRM_PORT_TYPE_ANALOG)
-	 */
-	if (immediate_upstream_port)
-		immediate_upstream_aux = &immediate_upstream_port->aux;
-	else
-		immediate_upstream_aux = port->mgr->aux;
+	if (!dsc_port) {
+		if (!immediate_upstream_port) {
+			/* Topology with 1 mstb only */
+			if (upstream_dsc & DP_DSC_DECOMPRESSION_IS_SUPPORTED)
+				port->dsc_aux = mgr->aux;
 
-	if (drm_dp_read_desc(immediate_upstream_aux, &desc, true))
-		return NULL;
+			if (!port->dsc_aux) {
+				drm_err(mgr->dev, "MST_DSC dsc decompression not support at root branch\n");
+				goto out_dsc_fail;
+			}
 
-	if (drm_dp_has_quirk(&desc, DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD)) {
-		u8 dpcd_ext[DP_RECEIVER_CAP_SIZE];
+			drm_info(mgr->dev, "MST_DSC topology with 1 mstb only, dsc decompression at root branch\n");
+		} else {
+			/* Topology with multiple mstbs */
+			dsc_port = immediate_upstream_port;
+			endpoint_dsc = upstream_dsc;
+
+			if (endpoint_dsc & DP_DSC_DECOMPRESSION_IS_SUPPORTED)
+				port->dsc_aux = &dsc_port->aux;
+			else {
+				drm_err(mgr->dev,
+					"MST_DSC dsc decompression not support at immediate_upstream_port %p\n",
+					dsc_port);
+				goto out_dsc_fail;
+			}
 
-		if (drm_dp_dpcd_read(immediate_upstream_aux,
-				     DP_DSC_SUPPORT, &upstream_dsc, 1) != 1)
-			return NULL;
+			drm_info(mgr->dev, "MST_DSC topology with multiple mstbs, dsc decompression at immediate_upstream_port %p\n",
+					    dsc_port);
+		}
+	}
 
-		if (!(upstream_dsc & DP_DSC_DECOMPRESSION_IS_SUPPORTED))
-			return NULL;
+	/* Check the virtual channel from source till dsc port link support FEC */
+	fec_port = dsc_port;
+	while (fec_port) {
+		/*
+		 * Each link between the output and the source
+		 * must support FEC. Note that virtual dpcd fec is identical
+		 * to the fec capability of it's MST BU's DPRx
+		 */
+		if (!fec_port->fec_capable) {
+			/* read fec cap one more time in case fec not capable return from enum path result */
+			if ((drm_dp_dpcd_read(&fec_port->aux, DP_FEC_CAPABILITY, &fec_cap, 1) != 1) ||
+					!(fec_cap & DP_FEC_CAPABLE)) {
+				drm_err(mgr->dev, "MST_DSC Failed to retrieve fec caps at port %p\n", fec_port);
+				goto out_dsc_fail;
+			}
+			fec_port->fec_capable = true;
+		}
 
-		if (drm_dp_read_dpcd_caps(immediate_upstream_aux, dpcd_ext) < 0)
-			return NULL;
+		fec_port = fec_port->parent->port_parent;
+	}
 
-		if (dpcd_ext[DP_DPCD_REV] >= DP_DPCD_REV_14 &&
-		    ((dpcd_ext[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT) &&
-		    ((dpcd_ext[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_TYPE_MASK)
-		     != DP_DWN_STRM_PORT_TYPE_ANALOG)))
-			return immediate_upstream_aux;
+	/* Ensure fec between source and the connected DPRx */
+	if ((drm_dp_dpcd_read(mgr->aux, DP_FEC_CAPABILITY, &fec_cap, 1) != 1) ||
+			!(fec_cap & DP_FEC_CAPABLE)) {
+		drm_err(mgr->dev, "MST_DSC fec not supported between source and the connected DPRx\n");
+		goto out_dsc_fail;
 	}
 
-	/*
-	 * The check below verifies if the MST sink
-	 * connected to the GPU is capable of DSC -
-	 * therefore the endpoint needs to be
-	 * both DSC and FEC capable.
-	 */
-	if (drm_dp_dpcd_read(&port->aux,
-	   DP_DSC_SUPPORT, &endpoint_dsc, 1) != 1)
-		return NULL;
-	if (drm_dp_dpcd_read(&port->aux,
-	   DP_FEC_CAPABILITY, &endpoint_fec, 1) != 1)
-		return NULL;
-	if ((endpoint_dsc & DP_DSC_DECOMPRESSION_IS_SUPPORTED) &&
-	   (endpoint_fec & DP_FEC_CAPABLE))
-		return &port->aux;
+	return port->dsc_aux;
 
+out_dsc_fail:
+	port->dsc_aux = NULL;
+	port->passthrough_aux = NULL;
 	return NULL;
 }
 EXPORT_SYMBOL(drm_dp_mst_dsc_aux_for_port);
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index f6a1cbb0f600..672e8f6b5655 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -80,6 +80,8 @@  struct drm_dp_mst_branch;
  * @next: link to next port on this branch device
  * @aux: i2c aux transport to talk to device connected to this port, protected
  * by &drm_dp_mst_topology_mgr.base.lock.
+ * @dsc_aux: aux to which DSC decompression request should be sent,
+ * only set if DSC decompression is possible.
  * @passthrough_aux: parent aux to which DSC pass-through requests should be
  * sent, only set if DSC pass-through is possible.
  * @parent: branch device parent of this port
@@ -135,6 +137,7 @@  struct drm_dp_mst_port {
 	 */
 	struct drm_dp_mst_branch *mstb;
 	struct drm_dp_aux aux; /* i2c bus for this port? */
+	struct drm_dp_aux *dsc_aux;
 	struct drm_dp_aux *passthrough_aux;
 	struct drm_dp_mst_branch *parent;