[PATCH 0/3] drm/dp: extract payload helpers

[PATCH 0/3] drm/dp: extract payload helpers

[Jani Nikula]

Add some mst topology manager independent payload helpers.

Jani Nikula (3):
  drm/dp: extract drm_dp_dpcd_poll_act_handled()
  drm/dp: extract drm_dp_dpcd_write_payload()
  drm/dp: extract drm_dp_dpcd_clear_payload()

 drivers/gpu/drm/display/drm_dp_helper.c       | 124 +++++++++++++++++-
 drivers/gpu/drm/display/drm_dp_mst_topology.c |  88 +------------
 include/drm/display/drm_dp_helper.h           |   5 +
 3 files changed, 133 insertions(+), 84 deletions(-)

-- 
2.39.5

[PATCH 1/3] drm/dp: extract drm_dp_dpcd_poll_act_handled()

[Jani Nikula]

SST with 128b/132b channel coding needs this too. Extract to a separate
helper, independent of MST.

Pass timeout in as a parameter, anticipating that we can reduce the
timeout for SST.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c       | 54 ++++++++++++++++++-
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 36 +------------
 include/drm/display/drm_dp_helper.h           |  2 +
 3 files changed, 57 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index 6ee51003de3c..b7e03bf02cd8 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -22,15 +22,16 @@
 
 #include <linux/backlight.h>
 #include <linux/delay.h>
+#include <linux/dynamic_debug.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
 #include <linux/string_helpers.h>
-#include <linux/dynamic_debug.h>
 
 #include <drm/display/drm_dp_helper.h>
 #include <drm/display/drm_dp_mst_helper.h>
@@ -779,6 +780,57 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
 }
 EXPORT_SYMBOL(drm_dp_dpcd_read_phy_link_status);
 
+static int read_payload_update_status(struct drm_dp_aux *aux)
+{
+	int ret;
+	u8 status;
+
+	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
+	if (ret < 0)
+		return ret;
+
+	return status;
+}
+
+/**
+ * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
+ * @aux: DisplayPort AUX channel
+ * @timeout_ms: Timeout in ms
+ *
+ * Tries waiting for the sink to finish updating its payload table by polling
+ * for the ACT handled bit for up to @timeout_ms milliseconds, defaulting to
+ * 3000 ms if 0.
+ *
+ * Returns:
+ * 0 if the ACT was handled in time, negative error code on failure.
+ */
+int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms)
+{
+	int ret, status;
+
+	/* default to 3 seconds, this is arbitrary */
+	timeout_ms = timeout_ms ?: 3000;
+
+	ret = readx_poll_timeout(read_payload_update_status, aux, status,
+				 status & DP_PAYLOAD_ACT_HANDLED || status < 0,
+				 200, timeout_ms * USEC_PER_MSEC);
+	if (ret < 0 && status >= 0) {
+		drm_err(aux->drm_dev, "Failed to get ACT after %d ms, last status: %02x\n",
+			timeout_ms, status);
+		return -EINVAL;
+	} else if (status < 0) {
+		/*
+		 * Failure here isn't unexpected - the hub may have
+		 * just been unplugged
+		 */
+		drm_dbg_kms(aux->drm_dev, "Failed to read payload table status: %d\n", status);
+		return status;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_dpcd_poll_act_handled);
+
 static bool is_edid_digital_input_dp(const struct drm_edid *drm_edid)
 {
 	/* FIXME: get rid of drm_edid_raw() */
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index ac90118b9e7a..2bdbc1eb282b 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -29,7 +29,6 @@
 #include <linux/random.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
-#include <linux/iopoll.h>
 
 #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
 #include <linux/stacktrace.h>
@@ -4723,18 +4722,6 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
 	return ret;
 }
 
-static int do_get_act_status(struct drm_dp_aux *aux)
-{
-	int ret;
-	u8 status;
-
-	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
-	if (ret < 0)
-		return ret;
-
-	return status;
-}
-
 /**
  * drm_dp_check_act_status() - Polls for ACT handled status.
  * @mgr: manager to use
@@ -4752,28 +4739,9 @@ int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr)
 	 * There doesn't seem to be any recommended retry count or timeout in
 	 * the MST specification. Since some hubs have been observed to take
 	 * over 1 second to update their payload allocations under certain
-	 * conditions, we use a rather large timeout value.
+	 * conditions, we use a rather large timeout value of 3 seconds.
 	 */
-	const int timeout_ms = 3000;
-	int ret, status;
-
-	ret = readx_poll_timeout(do_get_act_status, mgr->aux, status,
-				 status & DP_PAYLOAD_ACT_HANDLED || status < 0,
-				 200, timeout_ms * USEC_PER_MSEC);
-	if (ret < 0 && status >= 0) {
-		drm_err(mgr->dev, "Failed to get ACT after %dms, last status: %02x\n",
-			timeout_ms, status);
-		return -EINVAL;
-	} else if (status < 0) {
-		/*
-		 * Failure here isn't unexpected - the hub may have
-		 * just been unplugged
-		 */
-		drm_dbg_kms(mgr->dev, "Failed to read payload table status: %d\n", status);
-		return status;
-	}
-
-	return 0;
+	return drm_dp_dpcd_poll_act_handled(mgr->aux, 3000);
 }
 EXPORT_SYMBOL(drm_dp_check_act_status);
 
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index 279624833ea9..38eea21d1082 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -567,6 +567,8 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
 				     enum drm_dp_phy dp_phy,
 				     u8 link_status[DP_LINK_STATUS_SIZE]);
 
+int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms);
+
 bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
 				    u8 real_edid_checksum);
 
-- 
2.39.5

[PATCH 2/3] drm/dp: extract drm_dp_dpcd_write_payload()

[Jani Nikula]

SST with 128b/132b channel coding needs this too. Extract to a separate
helper, independent of MST.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c       | 56 +++++++++++++++++++
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 52 +----------------
 include/drm/display/drm_dp_helper.h           |  2 +
 3 files changed, 61 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index b7e03bf02cd8..0c9230f3f994 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -792,6 +792,62 @@ static int read_payload_update_status(struct drm_dp_aux *aux)
 	return status;
 }
 
+/**
+ * drm_dp_dpcd_write_payload() - Write payload
+ * @aux: DisplayPort AUX channel
+ * @vcpid: Virtual Channel Payload ID
+ * @start_time_slot: Starting time slot
+ * @time_slot_count: Time slot count
+ *
+ * Write payload table, poll status.
+ *
+ * Returns:
+ * 0 on success, negative error otherwise
+ */
+int drm_dp_dpcd_write_payload(struct drm_dp_aux *aux,
+			      int vcpid, u8 start_time_slot, u8 time_slot_count)
+{
+	u8 payload_alloc[3], status;
+	int ret;
+	int retries = 0;
+
+	drm_dp_dpcd_writeb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS,
+			   DP_PAYLOAD_TABLE_UPDATED);
+
+	payload_alloc[0] = vcpid;
+	payload_alloc[1] = start_time_slot;
+	payload_alloc[2] = time_slot_count;
+
+	ret = drm_dp_dpcd_write(aux, DP_PAYLOAD_ALLOCATE_SET, payload_alloc, 3);
+	if (ret != 3) {
+		drm_dbg_kms(aux->drm_dev, "failed to write payload allocation %d\n", ret);
+		goto fail;
+	}
+
+retry:
+	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
+	if (ret < 0) {
+		drm_dbg_kms(aux->drm_dev, "failed to read payload table status %d\n", ret);
+		goto fail;
+	}
+
+	if (!(status & DP_PAYLOAD_TABLE_UPDATED)) {
+		retries++;
+		if (retries < 20) {
+			usleep_range(10000, 20000);
+			goto retry;
+		}
+		drm_dbg_kms(aux->drm_dev, "status not set after read payload table status %d\n",
+			    status);
+		ret = -EINVAL;
+		goto fail;
+	}
+	ret = 0;
+fail:
+	return ret;
+}
+EXPORT_SYMBOL(drm_dp_dpcd_write_payload);
+
 /**
  * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
  * @aux: DisplayPort AUX channel
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 2bdbc1eb282b..a426d13a7a36 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -67,9 +67,6 @@ static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
 
 static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port);
 
-static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
-				     int id, u8 start_slot, u8 num_slots);
-
 static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
 				 struct drm_dp_mst_port *port,
 				 int offset, int size, u8 *bytes);
@@ -3263,7 +3260,7 @@ EXPORT_SYMBOL(drm_dp_send_query_stream_enc_status);
 static int drm_dp_create_payload_at_dfp(struct drm_dp_mst_topology_mgr *mgr,
 					struct drm_dp_mst_atomic_payload *payload)
 {
-	return drm_dp_dpcd_write_payload(mgr, payload->vcpi, payload->vc_start_slot,
+	return drm_dp_dpcd_write_payload(mgr->aux, payload->vcpi, payload->vc_start_slot,
 					 payload->time_slots);
 }
 
@@ -3294,7 +3291,7 @@ static void drm_dp_destroy_payload_at_remote_and_dfp(struct drm_dp_mst_topology_
 	}
 
 	if (payload->payload_allocation_status == DRM_DP_MST_PAYLOAD_ALLOCATION_DFP)
-		drm_dp_dpcd_write_payload(mgr, payload->vcpi, payload->vc_start_slot, 0);
+		drm_dp_dpcd_write_payload(mgr->aux, payload->vcpi, payload->vc_start_slot, 0);
 }
 
 /**
@@ -3682,7 +3679,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 			goto out_unlock;
 
 		/* Write reset payload */
-		drm_dp_dpcd_write_payload(mgr, 0, 0, 0x3f);
+		drm_dp_dpcd_write_payload(mgr->aux, 0, 0, 0x3f);
 
 		drm_dp_mst_queue_probe_work(mgr);
 
@@ -4679,49 +4676,6 @@ void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_
 }
 EXPORT_SYMBOL(drm_dp_mst_update_slots);
 
-static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
-				     int id, u8 start_slot, u8 num_slots)
-{
-	u8 payload_alloc[3], status;
-	int ret;
-	int retries = 0;
-
-	drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS,
-			   DP_PAYLOAD_TABLE_UPDATED);
-
-	payload_alloc[0] = id;
-	payload_alloc[1] = start_slot;
-	payload_alloc[2] = num_slots;
-
-	ret = drm_dp_dpcd_write(mgr->aux, DP_PAYLOAD_ALLOCATE_SET, payload_alloc, 3);
-	if (ret != 3) {
-		drm_dbg_kms(mgr->dev, "failed to write payload allocation %d\n", ret);
-		goto fail;
-	}
-
-retry:
-	ret = drm_dp_dpcd_readb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
-	if (ret < 0) {
-		drm_dbg_kms(mgr->dev, "failed to read payload table status %d\n", ret);
-		goto fail;
-	}
-
-	if (!(status & DP_PAYLOAD_TABLE_UPDATED)) {
-		retries++;
-		if (retries < 20) {
-			usleep_range(10000, 20000);
-			goto retry;
-		}
-		drm_dbg_kms(mgr->dev, "status not set after read payload table status %d\n",
-			    status);
-		ret = -EINVAL;
-		goto fail;
-	}
-	ret = 0;
-fail:
-	return ret;
-}
-
 /**
  * drm_dp_check_act_status() - Polls for ACT handled status.
  * @mgr: manager to use
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index 38eea21d1082..69793815aa82 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -567,6 +567,8 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
 				     enum drm_dp_phy dp_phy,
 				     u8 link_status[DP_LINK_STATUS_SIZE]);
 
+int drm_dp_dpcd_write_payload(struct drm_dp_aux *aux,
+			      int vcpid, u8 start_time_slot, u8 time_slot_count);
 int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms);
 
 bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
-- 
2.39.5

[PATCH 3/3] drm/dp: extract drm_dp_dpcd_clear_payload()

[Jani Nikula]

SST with 128b/132b channel coding needs this too. Extract to a separate
helper, independent of MST.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c       | 14 ++++++++++++++
 drivers/gpu/drm/display/drm_dp_mst_topology.c |  2 +-
 include/drm/display/drm_dp_helper.h           |  1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index 0c9230f3f994..9b7f8393440d 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -848,6 +848,20 @@ int drm_dp_dpcd_write_payload(struct drm_dp_aux *aux,
 }
 EXPORT_SYMBOL(drm_dp_dpcd_write_payload);
 
+/**
+ * drm_dp_dpcd_clear_payload() - Clear the entire VC Payload ID table
+ * @aux: DisplayPort AUX channel
+ *
+ * Clear the entire VC Payload ID table.
+ *
+ * Returns: 0 on success, negative error code on errors.
+ */
+int drm_dp_dpcd_clear_payload(struct drm_dp_aux *aux)
+{
+	return drm_dp_dpcd_write_payload(aux, 0, 0, 0x3f);
+}
+EXPORT_SYMBOL(drm_dp_dpcd_clear_payload);
+
 /**
  * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
  * @aux: DisplayPort AUX channel
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index a426d13a7a36..307ce0981e2c 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3679,7 +3679,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 			goto out_unlock;
 
 		/* Write reset payload */
-		drm_dp_dpcd_write_payload(mgr->aux, 0, 0, 0x3f);
+		drm_dp_dpcd_clear_payload(mgr->aux);
 
 		drm_dp_mst_queue_probe_work(mgr);
 
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index 69793815aa82..8f4054a56039 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -569,6 +569,7 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
 
 int drm_dp_dpcd_write_payload(struct drm_dp_aux *aux,
 			      int vcpid, u8 start_time_slot, u8 time_slot_count);
+int drm_dp_dpcd_clear_payload(struct drm_dp_aux *aux);
 int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms);
 
 bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
-- 
2.39.5

Re: [PATCH 1/3] drm/dp: extract drm_dp_dpcd_poll_act_handled()

[Imre Deak]

On Mon, Nov 18, 2024 at 05:14:52PM +0200, Jani Nikula wrote:
> SST with 128b/132b channel coding needs this too. Extract to a separate
> helper, independent of MST.
> 
> Pass timeout in as a parameter, anticipating that we can reduce the
> timeout for SST.

I wish there was a DP Standard section making the above clear,
but I suppose we just deduct that except of the side-band messaging,
every other payload programming and ACT signaling is required for
128b/132b SST.

> 

Cc: Lyude Paul <lyude@redhat.com>

> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c       | 54 ++++++++++++++++++-
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 36 +------------
>  include/drm/display/drm_dp_helper.h           |  2 +
>  3 files changed, 57 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 6ee51003de3c..b7e03bf02cd8 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -22,15 +22,16 @@
>  
>  #include <linux/backlight.h>
>  #include <linux/delay.h>
> +#include <linux/dynamic_debug.h>
>  #include <linux/errno.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> +#include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/sched.h>
>  #include <linux/seq_file.h>
>  #include <linux/string_helpers.h>
> -#include <linux/dynamic_debug.h>
>  
>  #include <drm/display/drm_dp_helper.h>
>  #include <drm/display/drm_dp_mst_helper.h>
> @@ -779,6 +780,57 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
>  }
>  EXPORT_SYMBOL(drm_dp_dpcd_read_phy_link_status);
>  
> +static int read_payload_update_status(struct drm_dp_aux *aux)
> +{
> +	int ret;
> +	u8 status;
> +
> +	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	return status;
> +}
> +
> +/**
> + * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
> + * @aux: DisplayPort AUX channel
> + * @timeout_ms: Timeout in ms
> + *
> + * Tries waiting for the sink to finish updating its payload table by polling
> + * for the ACT handled bit for up to @timeout_ms milliseconds, defaulting to
> + * 3000 ms if 0.
> + *
> + * Returns:
> + * 0 if the ACT was handled in time, negative error code on failure.
> + */
> +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms)

I wonder if it'd make sense to namespace these helpers using ll_mtp or mtp.

> +{
> +	int ret, status;
> +

Extra w/s.

Regardless of the namespace comment:

Reviewed-by: Imre Deak <imre.deak@intel.com>

> +	/* default to 3 seconds, this is arbitrary */
> +	timeout_ms = timeout_ms ?: 3000;
> +
> +	ret = readx_poll_timeout(read_payload_update_status, aux, status,
> +				 status & DP_PAYLOAD_ACT_HANDLED || status < 0,
> +				 200, timeout_ms * USEC_PER_MSEC);
> +	if (ret < 0 && status >= 0) {
> +		drm_err(aux->drm_dev, "Failed to get ACT after %d ms, last status: %02x\n",
> +			timeout_ms, status);
> +		return -EINVAL;
> +	} else if (status < 0) {
> +		/*
> +		 * Failure here isn't unexpected - the hub may have
> +		 * just been unplugged
> +		 */
> +		drm_dbg_kms(aux->drm_dev, "Failed to read payload table status: %d\n", status);
> +		return status;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_poll_act_handled);
> +
>  static bool is_edid_digital_input_dp(const struct drm_edid *drm_edid)
>  {
>  	/* FIXME: get rid of drm_edid_raw() */
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index ac90118b9e7a..2bdbc1eb282b 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -29,7 +29,6 @@
>  #include <linux/random.h>
>  #include <linux/sched.h>
>  #include <linux/seq_file.h>
> -#include <linux/iopoll.h>
>  
>  #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
>  #include <linux/stacktrace.h>
> @@ -4723,18 +4722,6 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
>  	return ret;
>  }
>  
> -static int do_get_act_status(struct drm_dp_aux *aux)
> -{
> -	int ret;
> -	u8 status;
> -
> -	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> -	if (ret < 0)
> -		return ret;
> -
> -	return status;
> -}
> -
>  /**
>   * drm_dp_check_act_status() - Polls for ACT handled status.
>   * @mgr: manager to use
> @@ -4752,28 +4739,9 @@ int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr)
>  	 * There doesn't seem to be any recommended retry count or timeout in
>  	 * the MST specification. Since some hubs have been observed to take
>  	 * over 1 second to update their payload allocations under certain
> -	 * conditions, we use a rather large timeout value.
> +	 * conditions, we use a rather large timeout value of 3 seconds.
>  	 */
> -	const int timeout_ms = 3000;
> -	int ret, status;
> -
> -	ret = readx_poll_timeout(do_get_act_status, mgr->aux, status,
> -				 status & DP_PAYLOAD_ACT_HANDLED || status < 0,
> -				 200, timeout_ms * USEC_PER_MSEC);
> -	if (ret < 0 && status >= 0) {
> -		drm_err(mgr->dev, "Failed to get ACT after %dms, last status: %02x\n",
> -			timeout_ms, status);
> -		return -EINVAL;
> -	} else if (status < 0) {
> -		/*
> -		 * Failure here isn't unexpected - the hub may have
> -		 * just been unplugged
> -		 */
> -		drm_dbg_kms(mgr->dev, "Failed to read payload table status: %d\n", status);
> -		return status;
> -	}
> -
> -	return 0;
> +	return drm_dp_dpcd_poll_act_handled(mgr->aux, 3000);
>  }
>  EXPORT_SYMBOL(drm_dp_check_act_status);
>  
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 279624833ea9..38eea21d1082 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -567,6 +567,8 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
>  				     enum drm_dp_phy dp_phy,
>  				     u8 link_status[DP_LINK_STATUS_SIZE]);
>  
> +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms);
> +
>  bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
>  				    u8 real_edid_checksum);
>  
> -- 
> 2.39.5
> 

Re: [PATCH 2/3] drm/dp: extract drm_dp_dpcd_write_payload()

[Imre Deak]

On Mon, Nov 18, 2024 at 05:14:53PM +0200, Jani Nikula wrote:
> SST with 128b/132b channel coding needs this too. Extract to a separate
> helper, independent of MST.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c       | 56 +++++++++++++++++++
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 52 +----------------
>  include/drm/display/drm_dp_helper.h           |  2 +
>  3 files changed, 61 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index b7e03bf02cd8..0c9230f3f994 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -792,6 +792,62 @@ static int read_payload_update_status(struct drm_dp_aux *aux)
>  	return status;
>  }
>  
> +/**
> + * drm_dp_dpcd_write_payload() - Write payload

Stg like "Write Virtual Channel Payload information to payload table"
instead?

Regardless of the above or the earlier mtp namespacing comment, patch
looks ok:

Reviewed-by: Imre Deak <imre.deak@intel.com>

> + * @aux: DisplayPort AUX channel
> + * @vcpid: Virtual Channel Payload ID
> + * @start_time_slot: Starting time slot
> + * @time_slot_count: Time slot count
> + *
> + * Write payload table, poll status.
> + *
> + * Returns:
> + * 0 on success, negative error otherwise
> + */
> +int drm_dp_dpcd_write_payload(struct drm_dp_aux *aux,
> +			      int vcpid, u8 start_time_slot, u8 time_slot_count)
> +{
> +	u8 payload_alloc[3], status;
> +	int ret;
> +	int retries = 0;
> +
> +	drm_dp_dpcd_writeb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS,
> +			   DP_PAYLOAD_TABLE_UPDATED);
> +
> +	payload_alloc[0] = vcpid;
> +	payload_alloc[1] = start_time_slot;
> +	payload_alloc[2] = time_slot_count;
> +
> +	ret = drm_dp_dpcd_write(aux, DP_PAYLOAD_ALLOCATE_SET, payload_alloc, 3);
> +	if (ret != 3) {
> +		drm_dbg_kms(aux->drm_dev, "failed to write payload allocation %d\n", ret);
> +		goto fail;
> +	}
> +
> +retry:
> +	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> +	if (ret < 0) {
> +		drm_dbg_kms(aux->drm_dev, "failed to read payload table status %d\n", ret);
> +		goto fail;
> +	}
> +
> +	if (!(status & DP_PAYLOAD_TABLE_UPDATED)) {
> +		retries++;
> +		if (retries < 20) {
> +			usleep_range(10000, 20000);
> +			goto retry;
> +		}
> +		drm_dbg_kms(aux->drm_dev, "status not set after read payload table status %d\n",
> +			    status);
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +	ret = 0;
> +fail:
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_write_payload);
> +
>  /**
>   * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
>   * @aux: DisplayPort AUX channel
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 2bdbc1eb282b..a426d13a7a36 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -67,9 +67,6 @@ static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
>  
>  static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port);
>  
> -static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
> -				     int id, u8 start_slot, u8 num_slots);
> -
>  static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
>  				 struct drm_dp_mst_port *port,
>  				 int offset, int size, u8 *bytes);
> @@ -3263,7 +3260,7 @@ EXPORT_SYMBOL(drm_dp_send_query_stream_enc_status);
>  static int drm_dp_create_payload_at_dfp(struct drm_dp_mst_topology_mgr *mgr,
>  					struct drm_dp_mst_atomic_payload *payload)
>  {
> -	return drm_dp_dpcd_write_payload(mgr, payload->vcpi, payload->vc_start_slot,
> +	return drm_dp_dpcd_write_payload(mgr->aux, payload->vcpi, payload->vc_start_slot,
>  					 payload->time_slots);
>  }
>  
> @@ -3294,7 +3291,7 @@ static void drm_dp_destroy_payload_at_remote_and_dfp(struct drm_dp_mst_topology_
>  	}
>  
>  	if (payload->payload_allocation_status == DRM_DP_MST_PAYLOAD_ALLOCATION_DFP)
> -		drm_dp_dpcd_write_payload(mgr, payload->vcpi, payload->vc_start_slot, 0);
> +		drm_dp_dpcd_write_payload(mgr->aux, payload->vcpi, payload->vc_start_slot, 0);
>  }
>  
>  /**
> @@ -3682,7 +3679,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
>  			goto out_unlock;
>  
>  		/* Write reset payload */
> -		drm_dp_dpcd_write_payload(mgr, 0, 0, 0x3f);
> +		drm_dp_dpcd_write_payload(mgr->aux, 0, 0, 0x3f);
>  
>  		drm_dp_mst_queue_probe_work(mgr);
>  
> @@ -4679,49 +4676,6 @@ void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_
>  }
>  EXPORT_SYMBOL(drm_dp_mst_update_slots);
>  
> -static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
> -				     int id, u8 start_slot, u8 num_slots)
> -{
> -	u8 payload_alloc[3], status;
> -	int ret;
> -	int retries = 0;
> -
> -	drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS,
> -			   DP_PAYLOAD_TABLE_UPDATED);
> -
> -	payload_alloc[0] = id;
> -	payload_alloc[1] = start_slot;
> -	payload_alloc[2] = num_slots;
> -
> -	ret = drm_dp_dpcd_write(mgr->aux, DP_PAYLOAD_ALLOCATE_SET, payload_alloc, 3);
> -	if (ret != 3) {
> -		drm_dbg_kms(mgr->dev, "failed to write payload allocation %d\n", ret);
> -		goto fail;
> -	}
> -
> -retry:
> -	ret = drm_dp_dpcd_readb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> -	if (ret < 0) {
> -		drm_dbg_kms(mgr->dev, "failed to read payload table status %d\n", ret);
> -		goto fail;
> -	}
> -
> -	if (!(status & DP_PAYLOAD_TABLE_UPDATED)) {
> -		retries++;
> -		if (retries < 20) {
> -			usleep_range(10000, 20000);
> -			goto retry;
> -		}
> -		drm_dbg_kms(mgr->dev, "status not set after read payload table status %d\n",
> -			    status);
> -		ret = -EINVAL;
> -		goto fail;
> -	}
> -	ret = 0;
> -fail:
> -	return ret;
> -}
> -
>  /**
>   * drm_dp_check_act_status() - Polls for ACT handled status.
>   * @mgr: manager to use
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 38eea21d1082..69793815aa82 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -567,6 +567,8 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
>  				     enum drm_dp_phy dp_phy,
>  				     u8 link_status[DP_LINK_STATUS_SIZE]);
>  
> +int drm_dp_dpcd_write_payload(struct drm_dp_aux *aux,
> +			      int vcpid, u8 start_time_slot, u8 time_slot_count);
>  int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms);
>  
>  bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
> -- 
> 2.39.5
> 

Re: [PATCH 3/3] drm/dp: extract drm_dp_dpcd_clear_payload()

[Imre Deak]

On Mon, Nov 18, 2024 at 05:14:54PM +0200, Jani Nikula wrote:
> SST with 128b/132b channel coding needs this too. Extract to a separate
> helper, independent of MST.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/display/drm_dp_helper.c       | 14 ++++++++++++++
>  drivers/gpu/drm/display/drm_dp_mst_topology.c |  2 +-
>  include/drm/display/drm_dp_helper.h           |  1 +
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 0c9230f3f994..9b7f8393440d 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -848,6 +848,20 @@ int drm_dp_dpcd_write_payload(struct drm_dp_aux *aux,
>  }
>  EXPORT_SYMBOL(drm_dp_dpcd_write_payload);
>  
> +/**
> + * drm_dp_dpcd_clear_payload() - Clear the entire VC Payload ID table
> + * @aux: DisplayPort AUX channel
> + *
> + * Clear the entire VC Payload ID table.
> + *
> + * Returns: 0 on success, negative error code on errors.
> + */
> +int drm_dp_dpcd_clear_payload(struct drm_dp_aux *aux)
> +{
> +	return drm_dp_dpcd_write_payload(aux, 0, 0, 0x3f);
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_clear_payload);
> +
>  /**
>   * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
>   * @aux: DisplayPort AUX channel
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index a426d13a7a36..307ce0981e2c 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -3679,7 +3679,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
>  			goto out_unlock;
>  
>  		/* Write reset payload */
> -		drm_dp_dpcd_write_payload(mgr->aux, 0, 0, 0x3f);
> +		drm_dp_dpcd_clear_payload(mgr->aux);
>  
>  		drm_dp_mst_queue_probe_work(mgr);
>  
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 69793815aa82..8f4054a56039 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -569,6 +569,7 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
>  
>  int drm_dp_dpcd_write_payload(struct drm_dp_aux *aux,
>  			      int vcpid, u8 start_time_slot, u8 time_slot_count);
> +int drm_dp_dpcd_clear_payload(struct drm_dp_aux *aux);
>  int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms);
>  
>  bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
> -- 
> 2.39.5
> 

Re: [PATCH 1/3] drm/dp: extract drm_dp_dpcd_poll_act_handled()

[Imre Deak]

On Wed, Nov 20, 2024 at 04:59:43PM +0200, Imre Deak wrote:
> On Mon, Nov 18, 2024 at 05:14:52PM +0200, Jani Nikula wrote:
> > SST with 128b/132b channel coding needs this too. Extract to a separate
> > helper, independent of MST.
> > 
> > Pass timeout in as a parameter, anticipating that we can reduce the
> > timeout for SST.
> 
> I wish there was a DP Standard section making the above clear,
> but I suppose we just deduct that except of the side-band messaging,
> every other payload programming and ACT signaling is required for
> 128b/132b SST.
> 
> > 
> 
> Cc: Lyude Paul <lyude@redhat.com>
> 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_helper.c       | 54 ++++++++++++++++++-
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 36 +------------
> >  include/drm/display/drm_dp_helper.h           |  2 +
> >  3 files changed, 57 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> > index 6ee51003de3c..b7e03bf02cd8 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -22,15 +22,16 @@
> >  
> >  #include <linux/backlight.h>
> >  #include <linux/delay.h>
> > +#include <linux/dynamic_debug.h>
> >  #include <linux/errno.h>
> >  #include <linux/i2c.h>
> >  #include <linux/init.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/sched.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/string_helpers.h>
> > -#include <linux/dynamic_debug.h>
> >  
> >  #include <drm/display/drm_dp_helper.h>
> >  #include <drm/display/drm_dp_mst_helper.h>
> > @@ -779,6 +780,57 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
> >  }
> >  EXPORT_SYMBOL(drm_dp_dpcd_read_phy_link_status);
> >  
> > +static int read_payload_update_status(struct drm_dp_aux *aux)
> > +{
> > +	int ret;
> > +	u8 status;
> > +
> > +	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return status;
> > +}
> > +
> > +/**
> > + * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
> > + * @aux: DisplayPort AUX channel
> > + * @timeout_ms: Timeout in ms
> > + *
> > + * Tries waiting for the sink to finish updating its payload table by polling
> > + * for the ACT handled bit for up to @timeout_ms milliseconds, defaulting to
> > + * 3000 ms if 0.
> > + *
> > + * Returns:
> > + * 0 if the ACT was handled in time, negative error code on failure.
> > + */
> > +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms)
> 
> I wonder if it'd make sense to namespace these helpers using ll_mtp or mtp.
> 
> > +{
> > +	int ret, status;
> > +
> 
> Extra w/s.

Ah nvm, timeout_ms is a function param so new is needed.

> 
> Regardless of the namespace comment:
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> > +	/* default to 3 seconds, this is arbitrary */
> > +	timeout_ms = timeout_ms ?: 3000;
> > +
> > +	ret = readx_poll_timeout(read_payload_update_status, aux, status,
> > +				 status & DP_PAYLOAD_ACT_HANDLED || status < 0,
> > +				 200, timeout_ms * USEC_PER_MSEC);
> > +	if (ret < 0 && status >= 0) {
> > +		drm_err(aux->drm_dev, "Failed to get ACT after %d ms, last status: %02x\n",
> > +			timeout_ms, status);
> > +		return -EINVAL;
> > +	} else if (status < 0) {
> > +		/*
> > +		 * Failure here isn't unexpected - the hub may have
> > +		 * just been unplugged
> > +		 */
> > +		drm_dbg_kms(aux->drm_dev, "Failed to read payload table status: %d\n", status);
> > +		return status;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_dpcd_poll_act_handled);
> > +
> >  static bool is_edid_digital_input_dp(const struct drm_edid *drm_edid)
> >  {
> >  	/* FIXME: get rid of drm_edid_raw() */
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index ac90118b9e7a..2bdbc1eb282b 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -29,7 +29,6 @@
> >  #include <linux/random.h>
> >  #include <linux/sched.h>
> >  #include <linux/seq_file.h>
> > -#include <linux/iopoll.h>
> >  
> >  #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
> >  #include <linux/stacktrace.h>
> > @@ -4723,18 +4722,6 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
> >  	return ret;
> >  }
> >  
> > -static int do_get_act_status(struct drm_dp_aux *aux)
> > -{
> > -	int ret;
> > -	u8 status;
> > -
> > -	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	return status;
> > -}
> > -
> >  /**
> >   * drm_dp_check_act_status() - Polls for ACT handled status.
> >   * @mgr: manager to use
> > @@ -4752,28 +4739,9 @@ int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr)
> >  	 * There doesn't seem to be any recommended retry count or timeout in
> >  	 * the MST specification. Since some hubs have been observed to take
> >  	 * over 1 second to update their payload allocations under certain
> > -	 * conditions, we use a rather large timeout value.
> > +	 * conditions, we use a rather large timeout value of 3 seconds.
> >  	 */
> > -	const int timeout_ms = 3000;
> > -	int ret, status;
> > -
> > -	ret = readx_poll_timeout(do_get_act_status, mgr->aux, status,
> > -				 status & DP_PAYLOAD_ACT_HANDLED || status < 0,
> > -				 200, timeout_ms * USEC_PER_MSEC);
> > -	if (ret < 0 && status >= 0) {
> > -		drm_err(mgr->dev, "Failed to get ACT after %dms, last status: %02x\n",
> > -			timeout_ms, status);
> > -		return -EINVAL;
> > -	} else if (status < 0) {
> > -		/*
> > -		 * Failure here isn't unexpected - the hub may have
> > -		 * just been unplugged
> > -		 */
> > -		drm_dbg_kms(mgr->dev, "Failed to read payload table status: %d\n", status);
> > -		return status;
> > -	}
> > -
> > -	return 0;
> > +	return drm_dp_dpcd_poll_act_handled(mgr->aux, 3000);
> >  }
> >  EXPORT_SYMBOL(drm_dp_check_act_status);
> >  
> > diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> > index 279624833ea9..38eea21d1082 100644
> > --- a/include/drm/display/drm_dp_helper.h
> > +++ b/include/drm/display/drm_dp_helper.h
> > @@ -567,6 +567,8 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
> >  				     enum drm_dp_phy dp_phy,
> >  				     u8 link_status[DP_LINK_STATUS_SIZE]);
> >  
> > +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms);
> > +
> >  bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
> >  				    u8 real_edid_checksum);
> >  
> > -- 
> > 2.39.5
> > 

Re: [PATCH 1/3] drm/dp: extract drm_dp_dpcd_poll_act_handled()

[Jani Nikula]

On Wed, 20 Nov 2024, Imre Deak <imre.deak@intel.com> wrote:
> On Mon, Nov 18, 2024 at 05:14:52PM +0200, Jani Nikula wrote:
>> SST with 128b/132b channel coding needs this too. Extract to a separate
>> helper, independent of MST.
>> 
>> Pass timeout in as a parameter, anticipating that we can reduce the
>> timeout for SST.
>
> I wish there was a DP Standard section making the above clear,
> but I suppose we just deduct that except of the side-band messaging,
> every other payload programming and ACT signaling is required for
> 128b/132b SST.

Ping. Okay to merge, or do we want to mull over the the timeout?

I'm primarily trying to do a non-functional change. I could omit the
timeout parameter, but for non-hubs three seconds seems excessive, and
reducing it for hubs too is a can of worms I prefer keeping the lid on
top.

> Cc: Lyude Paul <lyude@redhat.com>
>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/display/drm_dp_helper.c       | 54 ++++++++++++++++++-
>>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 36 +------------
>>  include/drm/display/drm_dp_helper.h           |  2 +
>>  3 files changed, 57 insertions(+), 35 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
>> index 6ee51003de3c..b7e03bf02cd8 100644
>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
>> @@ -22,15 +22,16 @@
>>  
>>  #include <linux/backlight.h>
>>  #include <linux/delay.h>
>> +#include <linux/dynamic_debug.h>
>>  #include <linux/errno.h>
>>  #include <linux/i2c.h>
>>  #include <linux/init.h>
>> +#include <linux/iopoll.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/sched.h>
>>  #include <linux/seq_file.h>
>>  #include <linux/string_helpers.h>
>> -#include <linux/dynamic_debug.h>
>>  
>>  #include <drm/display/drm_dp_helper.h>
>>  #include <drm/display/drm_dp_mst_helper.h>
>> @@ -779,6 +780,57 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
>>  }
>>  EXPORT_SYMBOL(drm_dp_dpcd_read_phy_link_status);
>>  
>> +static int read_payload_update_status(struct drm_dp_aux *aux)
>> +{
>> +	int ret;
>> +	u8 status;
>> +
>> +	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return status;
>> +}
>> +
>> +/**
>> + * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
>> + * @aux: DisplayPort AUX channel
>> + * @timeout_ms: Timeout in ms
>> + *
>> + * Tries waiting for the sink to finish updating its payload table by polling
>> + * for the ACT handled bit for up to @timeout_ms milliseconds, defaulting to
>> + * 3000 ms if 0.
>> + *
>> + * Returns:
>> + * 0 if the ACT was handled in time, negative error code on failure.
>> + */
>> +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms)
>
> I wonder if it'd make sense to namespace these helpers using ll_mtp or mtp.

Honestly I think there's already enough of an acronym jumble in the
name. At least "drm_dp_dpcd" is common for most functions around here.


BR,
Jani.

>
>> +{
>> +	int ret, status;
>> +
>
> Extra w/s.
>
> Regardless of the namespace comment:
>
> Reviewed-by: Imre Deak <imre.deak@intel.com>
>
>> +	/* default to 3 seconds, this is arbitrary */
>> +	timeout_ms = timeout_ms ?: 3000;
>> +
>> +	ret = readx_poll_timeout(read_payload_update_status, aux, status,
>> +				 status & DP_PAYLOAD_ACT_HANDLED || status < 0,
>> +				 200, timeout_ms * USEC_PER_MSEC);
>> +	if (ret < 0 && status >= 0) {
>> +		drm_err(aux->drm_dev, "Failed to get ACT after %d ms, last status: %02x\n",
>> +			timeout_ms, status);
>> +		return -EINVAL;
>> +	} else if (status < 0) {
>> +		/*
>> +		 * Failure here isn't unexpected - the hub may have
>> +		 * just been unplugged
>> +		 */
>> +		drm_dbg_kms(aux->drm_dev, "Failed to read payload table status: %d\n", status);
>> +		return status;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_dp_dpcd_poll_act_handled);
>> +
>>  static bool is_edid_digital_input_dp(const struct drm_edid *drm_edid)
>>  {
>>  	/* FIXME: get rid of drm_edid_raw() */
>> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> index ac90118b9e7a..2bdbc1eb282b 100644
>> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> @@ -29,7 +29,6 @@
>>  #include <linux/random.h>
>>  #include <linux/sched.h>
>>  #include <linux/seq_file.h>
>> -#include <linux/iopoll.h>
>>  
>>  #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
>>  #include <linux/stacktrace.h>
>> @@ -4723,18 +4722,6 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
>>  	return ret;
>>  }
>>  
>> -static int do_get_act_status(struct drm_dp_aux *aux)
>> -{
>> -	int ret;
>> -	u8 status;
>> -
>> -	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	return status;
>> -}
>> -
>>  /**
>>   * drm_dp_check_act_status() - Polls for ACT handled status.
>>   * @mgr: manager to use
>> @@ -4752,28 +4739,9 @@ int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr)
>>  	 * There doesn't seem to be any recommended retry count or timeout in
>>  	 * the MST specification. Since some hubs have been observed to take
>>  	 * over 1 second to update their payload allocations under certain
>> -	 * conditions, we use a rather large timeout value.
>> +	 * conditions, we use a rather large timeout value of 3 seconds.
>>  	 */
>> -	const int timeout_ms = 3000;
>> -	int ret, status;
>> -
>> -	ret = readx_poll_timeout(do_get_act_status, mgr->aux, status,
>> -				 status & DP_PAYLOAD_ACT_HANDLED || status < 0,
>> -				 200, timeout_ms * USEC_PER_MSEC);
>> -	if (ret < 0 && status >= 0) {
>> -		drm_err(mgr->dev, "Failed to get ACT after %dms, last status: %02x\n",
>> -			timeout_ms, status);
>> -		return -EINVAL;
>> -	} else if (status < 0) {
>> -		/*
>> -		 * Failure here isn't unexpected - the hub may have
>> -		 * just been unplugged
>> -		 */
>> -		drm_dbg_kms(mgr->dev, "Failed to read payload table status: %d\n", status);
>> -		return status;
>> -	}
>> -
>> -	return 0;
>> +	return drm_dp_dpcd_poll_act_handled(mgr->aux, 3000);
>>  }
>>  EXPORT_SYMBOL(drm_dp_check_act_status);
>>  
>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
>> index 279624833ea9..38eea21d1082 100644
>> --- a/include/drm/display/drm_dp_helper.h
>> +++ b/include/drm/display/drm_dp_helper.h
>> @@ -567,6 +567,8 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
>>  				     enum drm_dp_phy dp_phy,
>>  				     u8 link_status[DP_LINK_STATUS_SIZE]);
>>  
>> +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms);
>> +
>>  bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
>>  				    u8 real_edid_checksum);
>>  
>> -- 
>> 2.39.5
>> 

-- 
Jani Nikula, Intel

Re: [PATCH 1/3] drm/dp: extract drm_dp_dpcd_poll_act_handled()

[Imre Deak]

On Fri, Nov 22, 2024 at 02:22:44PM +0200, Jani Nikula wrote:
> On Wed, 20 Nov 2024, Imre Deak <imre.deak@intel.com> wrote:
> > On Mon, Nov 18, 2024 at 05:14:52PM +0200, Jani Nikula wrote:
> >> SST with 128b/132b channel coding needs this too. Extract to a separate
> >> helper, independent of MST.
> >> 
> >> Pass timeout in as a parameter, anticipating that we can reduce the
> >> timeout for SST.
> >
> > I wish there was a DP Standard section making the above clear,
> > but I suppose we just deduct that except of the side-band messaging,
> > every other payload programming and ACT signaling is required for
> > 128b/132b SST.

The closest I found explaing the above is 5.7.5. Allocation Change
Trigger in DP Standard v2.1, maybe worth mentioning it in the commit
log.

> Ping. Okay to merge, or do we want to mull over the the timeout?

My R-b still applies, if that's enough for merging.

> I'm primarily trying to do a non-functional change. I could omit the
> timeout parameter, but for non-hubs three seconds seems excessive, and
> reducing it for hubs too is a can of worms I prefer keeping the lid on
> top.
> 
> > Cc: Lyude Paul <lyude@redhat.com>
> >
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/display/drm_dp_helper.c       | 54 ++++++++++++++++++-
> >>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 36 +------------
> >>  include/drm/display/drm_dp_helper.h           |  2 +
> >>  3 files changed, 57 insertions(+), 35 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> >> index 6ee51003de3c..b7e03bf02cd8 100644
> >> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> >> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> >> @@ -22,15 +22,16 @@
> >>  
> >>  #include <linux/backlight.h>
> >>  #include <linux/delay.h>
> >> +#include <linux/dynamic_debug.h>
> >>  #include <linux/errno.h>
> >>  #include <linux/i2c.h>
> >>  #include <linux/init.h>
> >> +#include <linux/iopoll.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/module.h>
> >>  #include <linux/sched.h>
> >>  #include <linux/seq_file.h>
> >>  #include <linux/string_helpers.h>
> >> -#include <linux/dynamic_debug.h>
> >>  
> >>  #include <drm/display/drm_dp_helper.h>
> >>  #include <drm/display/drm_dp_mst_helper.h>
> >> @@ -779,6 +780,57 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
> >>  }
> >>  EXPORT_SYMBOL(drm_dp_dpcd_read_phy_link_status);
> >>  
> >> +static int read_payload_update_status(struct drm_dp_aux *aux)
> >> +{
> >> +	int ret;
> >> +	u8 status;
> >> +
> >> +	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	return status;
> >> +}
> >> +
> >> +/**
> >> + * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
> >> + * @aux: DisplayPort AUX channel
> >> + * @timeout_ms: Timeout in ms
> >> + *
> >> + * Tries waiting for the sink to finish updating its payload table by polling
> >> + * for the ACT handled bit for up to @timeout_ms milliseconds, defaulting to
> >> + * 3000 ms if 0.
> >> + *
> >> + * Returns:
> >> + * 0 if the ACT was handled in time, negative error code on failure.
> >> + */
> >> +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms)
> >
> > I wonder if it'd make sense to namespace these helpers using ll_mtp or mtp.
> 
> Honestly I think there's already enough of an acronym jumble in the
> name. At least "drm_dp_dpcd" is common for most functions around here.
> 
> 
> BR,
> Jani.
> 
> >
> >> +{
> >> +	int ret, status;
> >> +
> >
> > Extra w/s.
> >
> > Regardless of the namespace comment:
> >
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> >
> >> +	/* default to 3 seconds, this is arbitrary */
> >> +	timeout_ms = timeout_ms ?: 3000;
> >> +
> >> +	ret = readx_poll_timeout(read_payload_update_status, aux, status,
> >> +				 status & DP_PAYLOAD_ACT_HANDLED || status < 0,
> >> +				 200, timeout_ms * USEC_PER_MSEC);
> >> +	if (ret < 0 && status >= 0) {
> >> +		drm_err(aux->drm_dev, "Failed to get ACT after %d ms, last status: %02x\n",
> >> +			timeout_ms, status);
> >> +		return -EINVAL;
> >> +	} else if (status < 0) {
> >> +		/*
> >> +		 * Failure here isn't unexpected - the hub may have
> >> +		 * just been unplugged
> >> +		 */
> >> +		drm_dbg_kms(aux->drm_dev, "Failed to read payload table status: %d\n", status);
> >> +		return status;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL(drm_dp_dpcd_poll_act_handled);
> >> +
> >>  static bool is_edid_digital_input_dp(const struct drm_edid *drm_edid)
> >>  {
> >>  	/* FIXME: get rid of drm_edid_raw() */
> >> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> >> index ac90118b9e7a..2bdbc1eb282b 100644
> >> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> >> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> >> @@ -29,7 +29,6 @@
> >>  #include <linux/random.h>
> >>  #include <linux/sched.h>
> >>  #include <linux/seq_file.h>
> >> -#include <linux/iopoll.h>
> >>  
> >>  #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
> >>  #include <linux/stacktrace.h>
> >> @@ -4723,18 +4722,6 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
> >>  	return ret;
> >>  }
> >>  
> >> -static int do_get_act_status(struct drm_dp_aux *aux)
> >> -{
> >> -	int ret;
> >> -	u8 status;
> >> -
> >> -	ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> >> -	if (ret < 0)
> >> -		return ret;
> >> -
> >> -	return status;
> >> -}
> >> -
> >>  /**
> >>   * drm_dp_check_act_status() - Polls for ACT handled status.
> >>   * @mgr: manager to use
> >> @@ -4752,28 +4739,9 @@ int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr)
> >>  	 * There doesn't seem to be any recommended retry count or timeout in
> >>  	 * the MST specification. Since some hubs have been observed to take
> >>  	 * over 1 second to update their payload allocations under certain
> >> -	 * conditions, we use a rather large timeout value.
> >> +	 * conditions, we use a rather large timeout value of 3 seconds.
> >>  	 */
> >> -	const int timeout_ms = 3000;
> >> -	int ret, status;
> >> -
> >> -	ret = readx_poll_timeout(do_get_act_status, mgr->aux, status,
> >> -				 status & DP_PAYLOAD_ACT_HANDLED || status < 0,
> >> -				 200, timeout_ms * USEC_PER_MSEC);
> >> -	if (ret < 0 && status >= 0) {
> >> -		drm_err(mgr->dev, "Failed to get ACT after %dms, last status: %02x\n",
> >> -			timeout_ms, status);
> >> -		return -EINVAL;
> >> -	} else if (status < 0) {
> >> -		/*
> >> -		 * Failure here isn't unexpected - the hub may have
> >> -		 * just been unplugged
> >> -		 */
> >> -		drm_dbg_kms(mgr->dev, "Failed to read payload table status: %d\n", status);
> >> -		return status;
> >> -	}
> >> -
> >> -	return 0;
> >> +	return drm_dp_dpcd_poll_act_handled(mgr->aux, 3000);
> >>  }
> >>  EXPORT_SYMBOL(drm_dp_check_act_status);
> >>  
> >> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> >> index 279624833ea9..38eea21d1082 100644
> >> --- a/include/drm/display/drm_dp_helper.h
> >> +++ b/include/drm/display/drm_dp_helper.h
> >> @@ -567,6 +567,8 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
> >>  				     enum drm_dp_phy dp_phy,
> >>  				     u8 link_status[DP_LINK_STATUS_SIZE]);
> >>  
> >> +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms);
> >> +
> >>  bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
> >>  				    u8 real_edid_checksum);
> >>  
> >> -- 
> >> 2.39.5
> >> 
> 
> -- 
> Jani Nikula, Intel

Re: [PATCH 0/3] drm/dp: extract payload helpers

[Jani Nikula]

On Mon, 18 Nov 2024, Jani Nikula <jani.nikula@intel.com> wrote:
> Add some mst topology manager independent payload helpers.

Cc: maintainers, Lyude, are we good to go with this?

BR,
Jani.


>
> Jani Nikula (3):
>   drm/dp: extract drm_dp_dpcd_poll_act_handled()
>   drm/dp: extract drm_dp_dpcd_write_payload()
>   drm/dp: extract drm_dp_dpcd_clear_payload()
>
>  drivers/gpu/drm/display/drm_dp_helper.c       | 124 +++++++++++++++++-
>  drivers/gpu/drm/display/drm_dp_mst_topology.c |  88 +------------
>  include/drm/display/drm_dp_helper.h           |   5 +
>  3 files changed, 133 insertions(+), 84 deletions(-)

-- 
Jani Nikula, Intel