[PATCH 0/6] drm: zynqmp_dp: Misc. patches and debugfs support

[PATCH 0/6] drm: zynqmp_dp: Misc. patches and debugfs support

[Sean Anderson]

This series adds debugfs support for the zynqmp_dp driver. The intent is
to enable compliance testing or to help debug signal-integrity issues.

The first three patches are general improvements (and could be applied
independently), while the last three add debugfs support.


Sean Anderson (6):
  drm: zynqmp_dp: Downgrade log level for aux retries message
  drm: zynqmp_dp: Adjust training values per-lane
  drm: zynqmp_dp: Add locking
  drm: zynqmp_dp: Split off several helper functions
  drm: zynqmp_dp: Optionally ignore DPCD errors
  drm: zynqmp_dp: Add debugfs interface for compliance testing

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 749 ++++++++++++++++++++++++++++---
 1 file changed, 691 insertions(+), 58 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] drm: zynqmp_dp: Downgrade log level for aux retries message

[Sean Anderson]

Enable this message for verbose debugging only as it is otherwise
printed after every AUX message, quickly filling the log buffer.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index a0606fab0e22..98a32e6a0459 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1006,7 +1006,7 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 					       msg->buffer, msg->size,
 					       &msg->reply);
 		if (!ret) {
-			dev_dbg(dp->dev, "aux %d retries\n", i);
+			dev_vdbg(dp->dev, "aux %d retries\n", i);
 			return msg->size;
 		}
 
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] drm: zynqmp_dp: Downgrade log level for aux retries message

[Laurent Pinchart]

Hi Sean,

Thank you for the patch.

On Fri, Mar 15, 2024 at 07:09:11PM -0400, Sean Anderson wrote:
> Enable this message for verbose debugging only as it is otherwise
> printed after every AUX message, quickly filling the log buffer.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0606fab0e22..98a32e6a0459 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1006,7 +1006,7 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  					       msg->buffer, msg->size,
>  					       &msg->reply);
>  		if (!ret) {
> -			dev_dbg(dp->dev, "aux %d retries\n", i);
> +			dev_vdbg(dp->dev, "aux %d retries\n", i);

I didn't know about dev_vdbg(). I suppose this makes sense,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  			return msg->size;
>  		}
>  

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/6] drm: zynqmp_dp: Adjust training values per-lane

[Sean Anderson]

The feedback we get from the DPRX is per-lane. Make changes using this
information, instead of picking the maximum values from all lanes. This
results in more-consistent training on marginal links.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 98a32e6a0459..8635b5673386 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -605,28 +605,21 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
 				   u8 link_status[DP_LINK_STATUS_SIZE])
 {
 	u8 *train_set = dp->train_set;
-	u8 voltage = 0, preemphasis = 0;
 	u8 i;
 
 	for (i = 0; i < dp->mode.lane_cnt; i++) {
-		u8 v = drm_dp_get_adjust_request_voltage(link_status, i);
-		u8 p = drm_dp_get_adjust_request_pre_emphasis(link_status, i);
+		u8 voltage = drm_dp_get_adjust_request_voltage(link_status, i);
+		u8 preemphasis =
+			drm_dp_get_adjust_request_pre_emphasis(link_status, i);
 
-		if (v > voltage)
-			voltage = v;
+		if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
+			voltage |= DP_TRAIN_MAX_SWING_REACHED;
 
-		if (p > preemphasis)
-			preemphasis = p;
-	}
+		if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
+			preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
 
-	if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
-		voltage |= DP_TRAIN_MAX_SWING_REACHED;
-
-	if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
-		preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
-
-	for (i = 0; i < dp->mode.lane_cnt; i++)
 		train_set[i] = voltage | preemphasis;
+	}
 }
 
 /**
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] drm: zynqmp_dp: Adjust training values per-lane

[Laurent Pinchart]

Hi Sean,

Thank you for the patch.

On Fri, Mar 15, 2024 at 07:09:12PM -0400, Sean Anderson wrote:
> The feedback we get from the DPRX is per-lane. Make changes using this
> information, instead of picking the maximum values from all lanes. This
> results in more-consistent training on marginal links.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 98a32e6a0459..8635b5673386 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -605,28 +605,21 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
>  				   u8 link_status[DP_LINK_STATUS_SIZE])
>  {
>  	u8 *train_set = dp->train_set;
> -	u8 voltage = 0, preemphasis = 0;
>  	u8 i;
>  
>  	for (i = 0; i < dp->mode.lane_cnt; i++) {
> -		u8 v = drm_dp_get_adjust_request_voltage(link_status, i);
> -		u8 p = drm_dp_get_adjust_request_pre_emphasis(link_status, i);
> +		u8 voltage = drm_dp_get_adjust_request_voltage(link_status, i);
> +		u8 preemphasis =
> +			drm_dp_get_adjust_request_pre_emphasis(link_status, i);
>  
> -		if (v > voltage)
> -			voltage = v;
> +		if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
> +			voltage |= DP_TRAIN_MAX_SWING_REACHED;
>  
> -		if (p > preemphasis)
> -			preemphasis = p;
> -	}
> +		if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
> +			preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
>  
> -	if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
> -		voltage |= DP_TRAIN_MAX_SWING_REACHED;
> -
> -	if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
> -		preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
> -
> -	for (i = 0; i < dp->mode.lane_cnt; i++)
>  		train_set[i] = voltage | preemphasis;
> +	}

I don't have enough DP knowledge to review this :-(

>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] drm: zynqmp_dp: Add locking

[Sean Anderson]

Add some locking, since none is provided by the drm subsystem. This will
prevent the IRQ/workers/bridge API calls from stepping on each other's
toes.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 59 +++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 8635b5673386..d2dee58e7bf2 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -279,6 +279,7 @@ struct zynqmp_dp_config {
  * @dpsub: Display subsystem
  * @iomem: device I/O memory for register access
  * @reset: reset controller
+ * @lock: Mutex protecting this struct and register access (but not AUX)
  * @irq: irq
  * @bridge: DRM bridge for the DP encoder
  * @next_bridge: The downstream bridge
@@ -299,6 +300,7 @@ struct zynqmp_dp {
 	struct zynqmp_dpsub *dpsub;
 	void __iomem *iomem;
 	struct reset_control *reset;
+	struct mutex lock;
 	int irq;
 
 	struct drm_bridge bridge;
@@ -308,7 +310,7 @@ struct zynqmp_dp {
 	struct drm_dp_aux aux;
 	struct phy *phy[ZYNQMP_DP_MAX_LANES];
 	u8 num_lanes;
-	struct delayed_work hpd_work;
+	struct delayed_work hpd_work, hpd_irq_work;
 	enum drm_connector_status status;
 	bool enabled;
 
@@ -1371,8 +1373,10 @@ zynqmp_dp_bridge_mode_valid(struct drm_bridge *bridge,
 	}
 
 	/* Check with link rate and lane count */
+	mutex_lock(&dp->lock);
 	rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
 				  dp->link_config.max_lanes, dp->config.bpp);
+	mutex_unlock(&dp->lock);
 	if (mode->clock > rate) {
 		dev_dbg(dp->dev, "filtered mode %s for high pixel rate\n",
 			mode->name);
@@ -1399,6 +1403,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 
 	pm_runtime_get_sync(dp->dev);
 
+	mutex_lock(&dp->lock);
 	zynqmp_dp_disp_enable(dp, old_bridge_state);
 
 	/*
@@ -1459,6 +1464,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 	zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
 			ZYNQMP_DP_SOFTWARE_RESET_ALL);
 	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 1);
+	mutex_unlock(&dp->lock);
 }
 
 static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
@@ -1466,6 +1472,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 {
 	struct zynqmp_dp *dp = bridge_to_dp(bridge);
 
+	mutex_lock(&dp->lock);
 	dp->enabled = false;
 	cancel_delayed_work(&dp->hpd_work);
 	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
@@ -1476,6 +1483,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 		zynqmp_dp_write(dp, ZYNQMP_DP_TX_AUDIO_CONTROL, 0);
 
 	zynqmp_dp_disp_disable(dp, old_bridge_state);
+	mutex_unlock(&dp->lock);
 
 	pm_runtime_put_sync(dp->dev);
 }
@@ -1518,6 +1526,8 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
 	u32 state, i;
 	int ret;
 
+	mutex_lock(&dp->lock);
+
 	/*
 	 * This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
 	 * get the HPD signal with some monitors.
@@ -1545,11 +1555,13 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
 					       dp->num_lanes);
 
 		dp->status = connector_status_connected;
+		mutex_unlock(&dp->lock);
 		return connector_status_connected;
 	}
 
 disconnected:
 	dp->status = connector_status_disconnected;
+	mutex_unlock(&dp->lock);
 	return connector_status_disconnected;
 }
 
@@ -1611,6 +1623,29 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
 	drm_bridge_hpd_notify(&dp->bridge, status);
 }
 
+static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
+{
+	struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
+					    hpd_irq_work.work);
+	u8 status[DP_LINK_STATUS_SIZE + 2];
+	int err;
+
+	mutex_lock(&dp->lock);
+	err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
+			       DP_LINK_STATUS_SIZE + 2);
+	if (err < 0) {
+		dev_dbg_ratelimited(dp->dev,
+				    "could not read sink status: %d\n", err);
+	} else {
+		if (status[4] & DP_LINK_STATUS_UPDATED ||
+		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
+		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
+			zynqmp_dp_train_loop(dp);
+		}
+	}
+	mutex_unlock(&dp->lock);
+}
+
 static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
 {
 	struct zynqmp_dp *dp = (struct zynqmp_dp *)data;
@@ -1635,23 +1670,9 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
 	if (status & ZYNQMP_DP_INT_HPD_EVENT)
 		schedule_delayed_work(&dp->hpd_work, 0);
 
-	if (status & ZYNQMP_DP_INT_HPD_IRQ) {
-		int ret;
-		u8 status[DP_LINK_STATUS_SIZE + 2];
+	if (status & ZYNQMP_DP_INT_HPD_IRQ)
+		schedule_delayed_work(&dp->hpd_irq_work, 0);
 
-		ret = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
-				       DP_LINK_STATUS_SIZE + 2);
-		if (ret < 0)
-			goto handled;
-
-		if (status[4] & DP_LINK_STATUS_UPDATED ||
-		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
-		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
-			zynqmp_dp_train_loop(dp);
-		}
-	}
-
-handled:
 	return IRQ_HANDLED;
 }
 
@@ -1674,8 +1695,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 	dp->dev = &pdev->dev;
 	dp->dpsub = dpsub;
 	dp->status = connector_status_disconnected;
+	mutex_init(&dp->lock);
 
 	INIT_DELAYED_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
+	INIT_DELAYED_WORK(&dp->hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
 
 	/* Acquire all resources (IOMEM, IRQ and PHYs). */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
@@ -1775,6 +1798,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
 	zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
 	disable_irq(dp->irq);
 
+	cancel_delayed_work_sync(&dp->hpd_irq_work);
 	cancel_delayed_work_sync(&dp->hpd_work);
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
@@ -1782,4 +1806,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
 
 	zynqmp_dp_phy_exit(dp);
 	zynqmp_dp_reset(dp, true);
+	mutex_destroy(&dp->lock);
 }
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] drm: zynqmp_dp: Add locking

[kernel test robot]

Hi Sean,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.8]
[also build test WARNING on linus/master next-20240315]
[cannot apply to drm-misc/drm-misc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
base:   v6.8
patch link:    https://lore.kernel.org/r/20240315230916.1759060-4-sean.anderson%40linux.dev
patch subject: [PATCH 3/6] drm: zynqmp_dp: Add locking
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240316/202403161747.TRmfawhM-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161747.TRmfawhM-lkp@intel.com/reproduce)

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>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403161747.TRmfawhM-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/xlnx/zynqmp_dp.c:321: warning: Function parameter or struct member 'hpd_irq_work' not described in 'zynqmp_dp'


vim +321 drivers/gpu/drm/xlnx/zynqmp_dp.c

d76271d22694e8 Hyun Kwon        2018-07-07  275  
d76271d22694e8 Hyun Kwon        2018-07-07  276  /**
d76271d22694e8 Hyun Kwon        2018-07-07  277   * struct zynqmp_dp - Xilinx DisplayPort core
d76271d22694e8 Hyun Kwon        2018-07-07  278   * @dev: device structure
d76271d22694e8 Hyun Kwon        2018-07-07  279   * @dpsub: Display subsystem
d76271d22694e8 Hyun Kwon        2018-07-07  280   * @iomem: device I/O memory for register access
d76271d22694e8 Hyun Kwon        2018-07-07  281   * @reset: reset controller
8ce380e6568015 Sean Anderson    2024-03-15  282   * @lock: Mutex protecting this struct and register access (but not AUX)
d76271d22694e8 Hyun Kwon        2018-07-07  283   * @irq: irq
47e801bd0749f0 Laurent Pinchart 2021-08-04  284   * @bridge: DRM bridge for the DP encoder
bd68b9b3cb2e0d Laurent Pinchart 2021-08-04  285   * @next_bridge: The downstream bridge
d76271d22694e8 Hyun Kwon        2018-07-07  286   * @config: IP core configuration from DTS
d76271d22694e8 Hyun Kwon        2018-07-07  287   * @aux: aux channel
d76271d22694e8 Hyun Kwon        2018-07-07  288   * @phy: PHY handles for DP lanes
d76271d22694e8 Hyun Kwon        2018-07-07  289   * @num_lanes: number of enabled phy lanes
d76271d22694e8 Hyun Kwon        2018-07-07  290   * @hpd_work: hot plug detection worker
d76271d22694e8 Hyun Kwon        2018-07-07  291   * @status: connection status
d76271d22694e8 Hyun Kwon        2018-07-07  292   * @enabled: flag to indicate if the device is enabled
d76271d22694e8 Hyun Kwon        2018-07-07  293   * @dpcd: DP configuration data from currently connected sink device
d76271d22694e8 Hyun Kwon        2018-07-07  294   * @link_config: common link configuration between IP core and sink device
d76271d22694e8 Hyun Kwon        2018-07-07  295   * @mode: current mode between IP core and sink device
d76271d22694e8 Hyun Kwon        2018-07-07  296   * @train_set: set of training data
d76271d22694e8 Hyun Kwon        2018-07-07  297   */
d76271d22694e8 Hyun Kwon        2018-07-07  298  struct zynqmp_dp {
d76271d22694e8 Hyun Kwon        2018-07-07  299  	struct device *dev;
d76271d22694e8 Hyun Kwon        2018-07-07  300  	struct zynqmp_dpsub *dpsub;
d76271d22694e8 Hyun Kwon        2018-07-07  301  	void __iomem *iomem;
d76271d22694e8 Hyun Kwon        2018-07-07  302  	struct reset_control *reset;
8ce380e6568015 Sean Anderson    2024-03-15  303  	struct mutex lock;
d76271d22694e8 Hyun Kwon        2018-07-07  304  	int irq;
d76271d22694e8 Hyun Kwon        2018-07-07  305  
47e801bd0749f0 Laurent Pinchart 2021-08-04  306  	struct drm_bridge bridge;
bd68b9b3cb2e0d Laurent Pinchart 2021-08-04  307  	struct drm_bridge *next_bridge;
47e801bd0749f0 Laurent Pinchart 2021-08-04  308  
d76271d22694e8 Hyun Kwon        2018-07-07  309  	struct zynqmp_dp_config config;
d76271d22694e8 Hyun Kwon        2018-07-07  310  	struct drm_dp_aux aux;
d76271d22694e8 Hyun Kwon        2018-07-07  311  	struct phy *phy[ZYNQMP_DP_MAX_LANES];
d76271d22694e8 Hyun Kwon        2018-07-07  312  	u8 num_lanes;
8ce380e6568015 Sean Anderson    2024-03-15  313  	struct delayed_work hpd_work, hpd_irq_work;
d76271d22694e8 Hyun Kwon        2018-07-07  314  	enum drm_connector_status status;
d76271d22694e8 Hyun Kwon        2018-07-07  315  	bool enabled;
d76271d22694e8 Hyun Kwon        2018-07-07  316  
d76271d22694e8 Hyun Kwon        2018-07-07  317  	u8 dpcd[DP_RECEIVER_CAP_SIZE];
d76271d22694e8 Hyun Kwon        2018-07-07  318  	struct zynqmp_dp_link_config link_config;
d76271d22694e8 Hyun Kwon        2018-07-07  319  	struct zynqmp_dp_mode mode;
d76271d22694e8 Hyun Kwon        2018-07-07  320  	u8 train_set[ZYNQMP_DP_MAX_LANES];
d76271d22694e8 Hyun Kwon        2018-07-07 @321  };
d76271d22694e8 Hyun Kwon        2018-07-07  322  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] drm: zynqmp_dp: Add locking

[Sean Anderson]

On 3/16/24 05:52, kernel test robot wrote:
> Hi Sean,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on v6.8]
> [also build test WARNING on linus/master next-20240315]
> [cannot apply to drm-misc/drm-misc-next]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
> base:   v6.8
> patch link:    https://lore.kernel.org/r/20240315230916.1759060-4-sean.anderson%40linux.dev
> patch subject: [PATCH 3/6] drm: zynqmp_dp: Add locking
> config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240316/202403161747.TRmfawhM-lkp@intel.com/config)
> compiler: alpha-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161747.TRmfawhM-lkp@intel.com/reproduce)
> 
> 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>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202403161747.TRmfawhM-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>>> drivers/gpu/drm/xlnx/zynqmp_dp.c:321: warning: Function parameter or struct member 'hpd_irq_work' not described in 'zynqmp_dp'

Will document.

--Sean

> 
> vim +321 drivers/gpu/drm/xlnx/zynqmp_dp.c
> 
> d76271d22694e8 Hyun Kwon        2018-07-07  275  
> d76271d22694e8 Hyun Kwon        2018-07-07  276  /**
> d76271d22694e8 Hyun Kwon        2018-07-07  277   * struct zynqmp_dp - Xilinx DisplayPort core
> d76271d22694e8 Hyun Kwon        2018-07-07  278   * @dev: device structure
> d76271d22694e8 Hyun Kwon        2018-07-07  279   * @dpsub: Display subsystem
> d76271d22694e8 Hyun Kwon        2018-07-07  280   * @iomem: device I/O memory for register access
> d76271d22694e8 Hyun Kwon        2018-07-07  281   * @reset: reset controller
> 8ce380e6568015 Sean Anderson    2024-03-15  282   * @lock: Mutex protecting this struct and register access (but not AUX)
> d76271d22694e8 Hyun Kwon        2018-07-07  283   * @irq: irq
> 47e801bd0749f0 Laurent Pinchart 2021-08-04  284   * @bridge: DRM bridge for the DP encoder
> bd68b9b3cb2e0d Laurent Pinchart 2021-08-04  285   * @next_bridge: The downstream bridge
> d76271d22694e8 Hyun Kwon        2018-07-07  286   * @config: IP core configuration from DTS
> d76271d22694e8 Hyun Kwon        2018-07-07  287   * @aux: aux channel
> d76271d22694e8 Hyun Kwon        2018-07-07  288   * @phy: PHY handles for DP lanes
> d76271d22694e8 Hyun Kwon        2018-07-07  289   * @num_lanes: number of enabled phy lanes
> d76271d22694e8 Hyun Kwon        2018-07-07  290   * @hpd_work: hot plug detection worker
> d76271d22694e8 Hyun Kwon        2018-07-07  291   * @status: connection status
> d76271d22694e8 Hyun Kwon        2018-07-07  292   * @enabled: flag to indicate if the device is enabled
> d76271d22694e8 Hyun Kwon        2018-07-07  293   * @dpcd: DP configuration data from currently connected sink device
> d76271d22694e8 Hyun Kwon        2018-07-07  294   * @link_config: common link configuration between IP core and sink device
> d76271d22694e8 Hyun Kwon        2018-07-07  295   * @mode: current mode between IP core and sink device
> d76271d22694e8 Hyun Kwon        2018-07-07  296   * @train_set: set of training data
> d76271d22694e8 Hyun Kwon        2018-07-07  297   */
> d76271d22694e8 Hyun Kwon        2018-07-07  298  struct zynqmp_dp {
> d76271d22694e8 Hyun Kwon        2018-07-07  299  	struct device *dev;
> d76271d22694e8 Hyun Kwon        2018-07-07  300  	struct zynqmp_dpsub *dpsub;
> d76271d22694e8 Hyun Kwon        2018-07-07  301  	void __iomem *iomem;
> d76271d22694e8 Hyun Kwon        2018-07-07  302  	struct reset_control *reset;
> 8ce380e6568015 Sean Anderson    2024-03-15  303  	struct mutex lock;
> d76271d22694e8 Hyun Kwon        2018-07-07  304  	int irq;
> d76271d22694e8 Hyun Kwon        2018-07-07  305  
> 47e801bd0749f0 Laurent Pinchart 2021-08-04  306  	struct drm_bridge bridge;
> bd68b9b3cb2e0d Laurent Pinchart 2021-08-04  307  	struct drm_bridge *next_bridge;
> 47e801bd0749f0 Laurent Pinchart 2021-08-04  308  
> d76271d22694e8 Hyun Kwon        2018-07-07  309  	struct zynqmp_dp_config config;
> d76271d22694e8 Hyun Kwon        2018-07-07  310  	struct drm_dp_aux aux;
> d76271d22694e8 Hyun Kwon        2018-07-07  311  	struct phy *phy[ZYNQMP_DP_MAX_LANES];
> d76271d22694e8 Hyun Kwon        2018-07-07  312  	u8 num_lanes;
> 8ce380e6568015 Sean Anderson    2024-03-15  313  	struct delayed_work hpd_work, hpd_irq_work;
> d76271d22694e8 Hyun Kwon        2018-07-07  314  	enum drm_connector_status status;
> d76271d22694e8 Hyun Kwon        2018-07-07  315  	bool enabled;
> d76271d22694e8 Hyun Kwon        2018-07-07  316  
> d76271d22694e8 Hyun Kwon        2018-07-07  317  	u8 dpcd[DP_RECEIVER_CAP_SIZE];
> d76271d22694e8 Hyun Kwon        2018-07-07  318  	struct zynqmp_dp_link_config link_config;
> d76271d22694e8 Hyun Kwon        2018-07-07  319  	struct zynqmp_dp_mode mode;
> d76271d22694e8 Hyun Kwon        2018-07-07  320  	u8 train_set[ZYNQMP_DP_MAX_LANES];
> d76271d22694e8 Hyun Kwon        2018-07-07 @321  };
> d76271d22694e8 Hyun Kwon        2018-07-07  322  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] drm: zynqmp_dp: Add locking

[Laurent Pinchart]

Hi Sean,

Thank you for the patch.

On Fri, Mar 15, 2024 at 07:09:13PM -0400, Sean Anderson wrote:
> Add some locking, since none is provided by the drm subsystem. This will

That's not quite right, the DRM core doesn't call bridge operations
concurrently. We may need locking to protect against race conditions
between bridge operations and interrupts though.

> prevent the IRQ/workers/bridge API calls from stepping on each other's
> toes.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 59 +++++++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 8635b5673386..d2dee58e7bf2 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -279,6 +279,7 @@ struct zynqmp_dp_config {
>   * @dpsub: Display subsystem
>   * @iomem: device I/O memory for register access
>   * @reset: reset controller
> + * @lock: Mutex protecting this struct and register access (but not AUX)

This patch does two things at once, it defers link training from the IRQ
handler to a work queue, and covers everything with a big lock. The
scope is too large. Please restrict the lock scope and document the
individual fields that need to be protected, and explain the locking
design in the commit message (or comments in the code).

>   * @irq: irq
>   * @bridge: DRM bridge for the DP encoder
>   * @next_bridge: The downstream bridge
> @@ -299,6 +300,7 @@ struct zynqmp_dp {
>  	struct zynqmp_dpsub *dpsub;
>  	void __iomem *iomem;
>  	struct reset_control *reset;
> +	struct mutex lock;
>  	int irq;
>  
>  	struct drm_bridge bridge;
> @@ -308,7 +310,7 @@ struct zynqmp_dp {
>  	struct drm_dp_aux aux;
>  	struct phy *phy[ZYNQMP_DP_MAX_LANES];
>  	u8 num_lanes;
> -	struct delayed_work hpd_work;
> +	struct delayed_work hpd_work, hpd_irq_work;

One variable per line please.

>  	enum drm_connector_status status;
>  	bool enabled;
>  
> @@ -1371,8 +1373,10 @@ zynqmp_dp_bridge_mode_valid(struct drm_bridge *bridge,
>  	}
>  
>  	/* Check with link rate and lane count */
> +	mutex_lock(&dp->lock);
>  	rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
>  				  dp->link_config.max_lanes, dp->config.bpp);
> +	mutex_unlock(&dp->lock);
>  	if (mode->clock > rate) {
>  		dev_dbg(dp->dev, "filtered mode %s for high pixel rate\n",
>  			mode->name);
> @@ -1399,6 +1403,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  
>  	pm_runtime_get_sync(dp->dev);
>  
> +	mutex_lock(&dp->lock);
>  	zynqmp_dp_disp_enable(dp, old_bridge_state);
>  
>  	/*
> @@ -1459,6 +1464,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
>  			ZYNQMP_DP_SOFTWARE_RESET_ALL);
>  	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 1);
> +	mutex_unlock(&dp->lock);
>  }
>  
>  static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
> @@ -1466,6 +1472,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  {
>  	struct zynqmp_dp *dp = bridge_to_dp(bridge);
>  
> +	mutex_lock(&dp->lock);
>  	dp->enabled = false;
>  	cancel_delayed_work(&dp->hpd_work);
>  	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
> @@ -1476,6 +1483,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  		zynqmp_dp_write(dp, ZYNQMP_DP_TX_AUDIO_CONTROL, 0);
>  
>  	zynqmp_dp_disp_disable(dp, old_bridge_state);
> +	mutex_unlock(&dp->lock);
>  
>  	pm_runtime_put_sync(dp->dev);
>  }
> @@ -1518,6 +1526,8 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
>  	u32 state, i;
>  	int ret;
>  
> +	mutex_lock(&dp->lock);
> +
>  	/*
>  	 * This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
>  	 * get the HPD signal with some monitors.
> @@ -1545,11 +1555,13 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
>  					       dp->num_lanes);
>  
>  		dp->status = connector_status_connected;
> +		mutex_unlock(&dp->lock);
>  		return connector_status_connected;
>  	}
>  
>  disconnected:
>  	dp->status = connector_status_disconnected;
> +	mutex_unlock(&dp->lock);
>  	return connector_status_disconnected;
>  }
>  
> @@ -1611,6 +1623,29 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
>  	drm_bridge_hpd_notify(&dp->bridge, status);
>  }
>  
> +static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
> +{
> +	struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
> +					    hpd_irq_work.work);
> +	u8 status[DP_LINK_STATUS_SIZE + 2];
> +	int err;
> +
> +	mutex_lock(&dp->lock);
> +	err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
> +			       DP_LINK_STATUS_SIZE + 2);
> +	if (err < 0) {
> +		dev_dbg_ratelimited(dp->dev,
> +				    "could not read sink status: %d\n", err);
> +	} else {
> +		if (status[4] & DP_LINK_STATUS_UPDATED ||
> +		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
> +		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
> +			zynqmp_dp_train_loop(dp);
> +		}
> +	}
> +	mutex_unlock(&dp->lock);
> +}
> +
>  static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
>  {
>  	struct zynqmp_dp *dp = (struct zynqmp_dp *)data;
> @@ -1635,23 +1670,9 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
>  	if (status & ZYNQMP_DP_INT_HPD_EVENT)
>  		schedule_delayed_work(&dp->hpd_work, 0);
>  
> -	if (status & ZYNQMP_DP_INT_HPD_IRQ) {
> -		int ret;
> -		u8 status[DP_LINK_STATUS_SIZE + 2];
> +	if (status & ZYNQMP_DP_INT_HPD_IRQ)
> +		schedule_delayed_work(&dp->hpd_irq_work, 0);
>  
> -		ret = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
> -				       DP_LINK_STATUS_SIZE + 2);
> -		if (ret < 0)
> -			goto handled;
> -
> -		if (status[4] & DP_LINK_STATUS_UPDATED ||
> -		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
> -		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
> -			zynqmp_dp_train_loop(dp);
> -		}
> -	}
> -
> -handled:
>  	return IRQ_HANDLED;
>  }
>  
> @@ -1674,8 +1695,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>  	dp->dev = &pdev->dev;
>  	dp->dpsub = dpsub;
>  	dp->status = connector_status_disconnected;
> +	mutex_init(&dp->lock);
>  
>  	INIT_DELAYED_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
> +	INIT_DELAYED_WORK(&dp->hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
>  
>  	/* Acquire all resources (IOMEM, IRQ and PHYs). */
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
> @@ -1775,6 +1798,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>  	zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
>  	disable_irq(dp->irq);
>  
> +	cancel_delayed_work_sync(&dp->hpd_irq_work);
>  	cancel_delayed_work_sync(&dp->hpd_work);
>  
>  	zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
> @@ -1782,4 +1806,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>  
>  	zynqmp_dp_phy_exit(dp);
>  	zynqmp_dp_reset(dp, true);
> +	mutex_destroy(&dp->lock);
>  }

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] drm: zynqmp_dp: Add locking

[Sean Anderson]

On 3/18/24 13:16, Laurent Pinchart wrote:
> Hi Sean,
> 
> Thank you for the patch.
> 
> On Fri, Mar 15, 2024 at 07:09:13PM -0400, Sean Anderson wrote:
>> Add some locking, since none is provided by the drm subsystem. This will
> 
> That's not quite right, the DRM core doesn't call bridge operations
> concurrently.

I figured something like this was going on.

> We may need locking to protect against race conditions
> between bridge operations and interrupts though.

And of course this will only get worse once we let userspace get involved.

>> prevent the IRQ/workers/bridge API calls from stepping on each other's
>> toes.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 59 +++++++++++++++++++++++---------
>>  1 file changed, 42 insertions(+), 17 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> index 8635b5673386..d2dee58e7bf2 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> @@ -279,6 +279,7 @@ struct zynqmp_dp_config {
>>   * @dpsub: Display subsystem
>>   * @iomem: device I/O memory for register access
>>   * @reset: reset controller
>> + * @lock: Mutex protecting this struct and register access (but not AUX)
> 
> This patch does two things at once, it defers link training from the IRQ
> handler to a work queue, and covers everything with a big lock. The
> scope is too large.

OK, I can split this.

> Please restrict the lock scope and document the
> individual fields that need to be protected, and explain the locking
> design in the commit message (or comments in the code).

As said, this lock protects

- Non-atomic registers configuring the link. That is, everything but the IRQ
  registers (since these are accessed in an atomic fashion), and the DP AUX
  registers (since these don't affect the link).
- Link configuration. This is effectively everything in zynqmp_dp which isn't
  read-only after probe time. So from next_bridge onward.

It's designed to protect configuration changes so we don't have to do anything
tricky. Configuration should never be in the hot path, so I'm not worried about
performance.

>>   * @irq: irq
>>   * @bridge: DRM bridge for the DP encoder
>>   * @next_bridge: The downstream bridge
>> @@ -299,6 +300,7 @@ struct zynqmp_dp {
>>  	struct zynqmp_dpsub *dpsub;
>>  	void __iomem *iomem;
>>  	struct reset_control *reset;
>> +	struct mutex lock;
>>  	int irq;
>>  
>>  	struct drm_bridge bridge;
>> @@ -308,7 +310,7 @@ struct zynqmp_dp {
>>  	struct drm_dp_aux aux;
>>  	struct phy *phy[ZYNQMP_DP_MAX_LANES];
>>  	u8 num_lanes;
>> -	struct delayed_work hpd_work;
>> +	struct delayed_work hpd_work, hpd_irq_work;
> 
> One variable per line please.

OK

>>  	enum drm_connector_status status;
>>  	bool enabled;
>>  
>> @@ -1371,8 +1373,10 @@ zynqmp_dp_bridge_mode_valid(struct drm_bridge *bridge,
>>  	}
>>  
>>  	/* Check with link rate and lane count */
>> +	mutex_lock(&dp->lock);
>>  	rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
>>  				  dp->link_config.max_lanes, dp->config.bpp);
>> +	mutex_unlock(&dp->lock);
>>  	if (mode->clock > rate) {
>>  		dev_dbg(dp->dev, "filtered mode %s for high pixel rate\n",
>>  			mode->name);
>> @@ -1399,6 +1403,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>>  
>>  	pm_runtime_get_sync(dp->dev);
>>  
>> +	mutex_lock(&dp->lock);
>>  	zynqmp_dp_disp_enable(dp, old_bridge_state);
>>  
>>  	/*
>> @@ -1459,6 +1464,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>>  	zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
>>  			ZYNQMP_DP_SOFTWARE_RESET_ALL);
>>  	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 1);
>> +	mutex_unlock(&dp->lock);
>>  }
>>  
>>  static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>> @@ -1466,6 +1472,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>>  {
>>  	struct zynqmp_dp *dp = bridge_to_dp(bridge);
>>  
>> +	mutex_lock(&dp->lock);
>>  	dp->enabled = false;
>>  	cancel_delayed_work(&dp->hpd_work);
>>  	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
>> @@ -1476,6 +1483,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>>  		zynqmp_dp_write(dp, ZYNQMP_DP_TX_AUDIO_CONTROL, 0);
>>  
>>  	zynqmp_dp_disp_disable(dp, old_bridge_state);
>> +	mutex_unlock(&dp->lock);
>>  
>>  	pm_runtime_put_sync(dp->dev);
>>  }
>> @@ -1518,6 +1526,8 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
>>  	u32 state, i;
>>  	int ret;
>>  
>> +	mutex_lock(&dp->lock);
>> +
>>  	/*
>>  	 * This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
>>  	 * get the HPD signal with some monitors.
>> @@ -1545,11 +1555,13 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
>>  					       dp->num_lanes);
>>  
>>  		dp->status = connector_status_connected;
>> +		mutex_unlock(&dp->lock);
>>  		return connector_status_connected;
>>  	}
>>  
>>  disconnected:
>>  	dp->status = connector_status_disconnected;
>> +	mutex_unlock(&dp->lock);
>>  	return connector_status_disconnected;
>>  }
>>  
>> @@ -1611,6 +1623,29 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
>>  	drm_bridge_hpd_notify(&dp->bridge, status);
>>  }
>>  
>> +static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
>> +{
>> +	struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
>> +					    hpd_irq_work.work);
>> +	u8 status[DP_LINK_STATUS_SIZE + 2];
>> +	int err;
>> +
>> +	mutex_lock(&dp->lock);
>> +	err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
>> +			       DP_LINK_STATUS_SIZE + 2);
>> +	if (err < 0) {
>> +		dev_dbg_ratelimited(dp->dev,
>> +				    "could not read sink status: %d\n", err);
>> +	} else {
>> +		if (status[4] & DP_LINK_STATUS_UPDATED ||
>> +		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
>> +		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
>> +			zynqmp_dp_train_loop(dp);
>> +		}
>> +	}
>> +	mutex_unlock(&dp->lock);
>> +}
>> +
>>  static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
>>  {
>>  	struct zynqmp_dp *dp = (struct zynqmp_dp *)data;
>> @@ -1635,23 +1670,9 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
>>  	if (status & ZYNQMP_DP_INT_HPD_EVENT)
>>  		schedule_delayed_work(&dp->hpd_work, 0);
>>  
>> -	if (status & ZYNQMP_DP_INT_HPD_IRQ) {
>> -		int ret;
>> -		u8 status[DP_LINK_STATUS_SIZE + 2];
>> +	if (status & ZYNQMP_DP_INT_HPD_IRQ)
>> +		schedule_delayed_work(&dp->hpd_irq_work, 0);
>>  
>> -		ret = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
>> -				       DP_LINK_STATUS_SIZE + 2);
>> -		if (ret < 0)
>> -			goto handled;
>> -
>> -		if (status[4] & DP_LINK_STATUS_UPDATED ||
>> -		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
>> -		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
>> -			zynqmp_dp_train_loop(dp);
>> -		}
>> -	}
>> -
>> -handled:
>>  	return IRQ_HANDLED;
>>  }
>>  
>> @@ -1674,8 +1695,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>>  	dp->dev = &pdev->dev;
>>  	dp->dpsub = dpsub;
>>  	dp->status = connector_status_disconnected;
>> +	mutex_init(&dp->lock);
>>  
>>  	INIT_DELAYED_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
>> +	INIT_DELAYED_WORK(&dp->hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
>>  
>>  	/* Acquire all resources (IOMEM, IRQ and PHYs). */
>>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
>> @@ -1775,6 +1798,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>>  	zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
>>  	disable_irq(dp->irq);
>>  
>> +	cancel_delayed_work_sync(&dp->hpd_irq_work);
>>  	cancel_delayed_work_sync(&dp->hpd_work);
>>  
>>  	zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
>> @@ -1782,4 +1806,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>>  
>>  	zynqmp_dp_phy_exit(dp);
>>  	zynqmp_dp_reset(dp, true);
>> +	mutex_destroy(&dp->lock);
>>  }
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] drm: zynqmp_dp: Add locking

[Laurent Pinchart]

Hi Sean,

On Mon, Mar 18, 2024 at 01:29:12PM -0400, Sean Anderson wrote:
> On 3/18/24 13:16, Laurent Pinchart wrote:
> > On Fri, Mar 15, 2024 at 07:09:13PM -0400, Sean Anderson wrote:
> >> Add some locking, since none is provided by the drm subsystem. This will
> > 
> > That's not quite right, the DRM core doesn't call bridge operations
> > concurrently.
> 
> I figured something like this was going on.
> 
> > We may need locking to protect against race conditions
> > between bridge operations and interrupts though.
> 
> And of course this will only get worse once we let userspace get involved.
> 
> >> prevent the IRQ/workers/bridge API calls from stepping on each other's
> >> toes.
> >> 
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> ---
> >> 
> >>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 59 +++++++++++++++++++++++---------
> >>  1 file changed, 42 insertions(+), 17 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> index 8635b5673386..d2dee58e7bf2 100644
> >> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> @@ -279,6 +279,7 @@ struct zynqmp_dp_config {
> >>   * @dpsub: Display subsystem
> >>   * @iomem: device I/O memory for register access
> >>   * @reset: reset controller
> >> + * @lock: Mutex protecting this struct and register access (but not AUX)
> > 
> > This patch does two things at once, it defers link training from the IRQ
> > handler to a work queue, and covers everything with a big lock. The
> > scope is too large.
> 
> OK, I can split this.
> 
> > Please restrict the lock scope and document the
> > individual fields that need to be protected, and explain the locking
> > design in the commit message (or comments in the code).
> 
> As said, this lock protects
> 
> - Non-atomic registers configuring the link. That is, everything but the IRQ
>   registers (since these are accessed in an atomic fashion), and the DP AUX
>   registers (since these don't affect the link).
> - Link configuration. This is effectively everything in zynqmp_dp which isn't
>   read-only after probe time. So from next_bridge onward.
> 
> It's designed to protect configuration changes so we don't have to do anything
> tricky. Configuration should never be in the hot path, so I'm not worried about
> performance.

If userspace can control all this directly through debugfs, can you
guarantee that locks will be enough ? The driver doesn't expect direct
userspace access. I have a feeling this is really quite hacky.

> >>   * @irq: irq
> >>   * @bridge: DRM bridge for the DP encoder
> >>   * @next_bridge: The downstream bridge
> >> @@ -299,6 +300,7 @@ struct zynqmp_dp {
> >>  	struct zynqmp_dpsub *dpsub;
> >>  	void __iomem *iomem;
> >>  	struct reset_control *reset;
> >> +	struct mutex lock;
> >>  	int irq;
> >>  
> >>  	struct drm_bridge bridge;
> >> @@ -308,7 +310,7 @@ struct zynqmp_dp {
> >>  	struct drm_dp_aux aux;
> >>  	struct phy *phy[ZYNQMP_DP_MAX_LANES];
> >>  	u8 num_lanes;
> >> -	struct delayed_work hpd_work;
> >> +	struct delayed_work hpd_work, hpd_irq_work;
> > 
> > One variable per line please.
> 
> OK
> 
> >>  	enum drm_connector_status status;
> >>  	bool enabled;
> >>  
> >> @@ -1371,8 +1373,10 @@ zynqmp_dp_bridge_mode_valid(struct drm_bridge *bridge,
> >>  	}
> >>  
> >>  	/* Check with link rate and lane count */
> >> +	mutex_lock(&dp->lock);
> >>  	rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
> >>  				  dp->link_config.max_lanes, dp->config.bpp);
> >> +	mutex_unlock(&dp->lock);
> >>  	if (mode->clock > rate) {
> >>  		dev_dbg(dp->dev, "filtered mode %s for high pixel rate\n",
> >>  			mode->name);
> >> @@ -1399,6 +1403,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
> >>  
> >>  	pm_runtime_get_sync(dp->dev);
> >>  
> >> +	mutex_lock(&dp->lock);
> >>  	zynqmp_dp_disp_enable(dp, old_bridge_state);
> >>  
> >>  	/*
> >> @@ -1459,6 +1464,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
> >>  	zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
> >>  			ZYNQMP_DP_SOFTWARE_RESET_ALL);
> >>  	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 1);
> >> +	mutex_unlock(&dp->lock);
> >>  }
> >>  
> >>  static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
> >> @@ -1466,6 +1472,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
> >>  {
> >>  	struct zynqmp_dp *dp = bridge_to_dp(bridge);
> >>  
> >> +	mutex_lock(&dp->lock);
> >>  	dp->enabled = false;
> >>  	cancel_delayed_work(&dp->hpd_work);
> >>  	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
> >> @@ -1476,6 +1483,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
> >>  		zynqmp_dp_write(dp, ZYNQMP_DP_TX_AUDIO_CONTROL, 0);
> >>  
> >>  	zynqmp_dp_disp_disable(dp, old_bridge_state);
> >> +	mutex_unlock(&dp->lock);
> >>  
> >>  	pm_runtime_put_sync(dp->dev);
> >>  }
> >> @@ -1518,6 +1526,8 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
> >>  	u32 state, i;
> >>  	int ret;
> >>  
> >> +	mutex_lock(&dp->lock);
> >> +
> >>  	/*
> >>  	 * This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
> >>  	 * get the HPD signal with some monitors.
> >> @@ -1545,11 +1555,13 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
> >>  					       dp->num_lanes);
> >>  
> >>  		dp->status = connector_status_connected;
> >> +		mutex_unlock(&dp->lock);
> >>  		return connector_status_connected;
> >>  	}
> >>  
> >>  disconnected:
> >>  	dp->status = connector_status_disconnected;
> >> +	mutex_unlock(&dp->lock);
> >>  	return connector_status_disconnected;
> >>  }
> >>  
> >> @@ -1611,6 +1623,29 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
> >>  	drm_bridge_hpd_notify(&dp->bridge, status);
> >>  }
> >>  
> >> +static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
> >> +{
> >> +	struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
> >> +					    hpd_irq_work.work);
> >> +	u8 status[DP_LINK_STATUS_SIZE + 2];
> >> +	int err;
> >> +
> >> +	mutex_lock(&dp->lock);
> >> +	err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
> >> +			       DP_LINK_STATUS_SIZE + 2);
> >> +	if (err < 0) {
> >> +		dev_dbg_ratelimited(dp->dev,
> >> +				    "could not read sink status: %d\n", err);
> >> +	} else {
> >> +		if (status[4] & DP_LINK_STATUS_UPDATED ||
> >> +		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
> >> +		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
> >> +			zynqmp_dp_train_loop(dp);
> >> +		}
> >> +	}
> >> +	mutex_unlock(&dp->lock);
> >> +}
> >> +
> >>  static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
> >>  {
> >>  	struct zynqmp_dp *dp = (struct zynqmp_dp *)data;
> >> @@ -1635,23 +1670,9 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
> >>  	if (status & ZYNQMP_DP_INT_HPD_EVENT)
> >>  		schedule_delayed_work(&dp->hpd_work, 0);
> >>  
> >> -	if (status & ZYNQMP_DP_INT_HPD_IRQ) {
> >> -		int ret;
> >> -		u8 status[DP_LINK_STATUS_SIZE + 2];
> >> +	if (status & ZYNQMP_DP_INT_HPD_IRQ)
> >> +		schedule_delayed_work(&dp->hpd_irq_work, 0);
> >>  
> >> -		ret = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
> >> -				       DP_LINK_STATUS_SIZE + 2);
> >> -		if (ret < 0)
> >> -			goto handled;
> >> -
> >> -		if (status[4] & DP_LINK_STATUS_UPDATED ||
> >> -		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
> >> -		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
> >> -			zynqmp_dp_train_loop(dp);
> >> -		}
> >> -	}
> >> -
> >> -handled:
> >>  	return IRQ_HANDLED;
> >>  }
> >>  
> >> @@ -1674,8 +1695,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> >>  	dp->dev = &pdev->dev;
> >>  	dp->dpsub = dpsub;
> >>  	dp->status = connector_status_disconnected;
> >> +	mutex_init(&dp->lock);
> >>  
> >>  	INIT_DELAYED_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
> >> +	INIT_DELAYED_WORK(&dp->hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
> >>  
> >>  	/* Acquire all resources (IOMEM, IRQ and PHYs). */
> >>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
> >> @@ -1775,6 +1798,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
> >>  	zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
> >>  	disable_irq(dp->irq);
> >>  
> >> +	cancel_delayed_work_sync(&dp->hpd_irq_work);
> >>  	cancel_delayed_work_sync(&dp->hpd_work);
> >>  
> >>  	zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
> >> @@ -1782,4 +1806,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
> >>  
> >>  	zynqmp_dp_phy_exit(dp);
> >>  	zynqmp_dp_reset(dp, true);
> >> +	mutex_destroy(&dp->lock);
> >>  }

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] drm: zynqmp_dp: Add locking

[Sean Anderson]

On 3/18/24 13:59, Laurent Pinchart wrote:
> Hi Sean,
> 
> On Mon, Mar 18, 2024 at 01:29:12PM -0400, Sean Anderson wrote:
>> On 3/18/24 13:16, Laurent Pinchart wrote:
>> > On Fri, Mar 15, 2024 at 07:09:13PM -0400, Sean Anderson wrote:
>> >> Add some locking, since none is provided by the drm subsystem. This will
>> > 
>> > That's not quite right, the DRM core doesn't call bridge operations
>> > concurrently.
>> 
>> I figured something like this was going on.
>> 
>> > We may need locking to protect against race conditions
>> > between bridge operations and interrupts though.
>> 
>> And of course this will only get worse once we let userspace get involved.
>> 
>> >> prevent the IRQ/workers/bridge API calls from stepping on each other's
>> >> toes.
>> >> 
>> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> >> ---
>> >> 
>> >>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 59 +++++++++++++++++++++++---------
>> >>  1 file changed, 42 insertions(+), 17 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> >> index 8635b5673386..d2dee58e7bf2 100644
>> >> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> >> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> >> @@ -279,6 +279,7 @@ struct zynqmp_dp_config {
>> >>   * @dpsub: Display subsystem
>> >>   * @iomem: device I/O memory for register access
>> >>   * @reset: reset controller
>> >> + * @lock: Mutex protecting this struct and register access (but not AUX)
>> > 
>> > This patch does two things at once, it defers link training from the IRQ
>> > handler to a work queue, and covers everything with a big lock. The
>> > scope is too large.
>> 
>> OK, I can split this.
>> 
>> > Please restrict the lock scope and document the
>> > individual fields that need to be protected, and explain the locking
>> > design in the commit message (or comments in the code).
>> 
>> As said, this lock protects
>> 
>> - Non-atomic registers configuring the link. That is, everything but the IRQ
>>   registers (since these are accessed in an atomic fashion), and the DP AUX
>>   registers (since these don't affect the link).
>> - Link configuration. This is effectively everything in zynqmp_dp which isn't
>>   read-only after probe time. So from next_bridge onward.
>> 
>> It's designed to protect configuration changes so we don't have to do anything
>> tricky. Configuration should never be in the hot path, so I'm not worried about
>> performance.
> 
> If userspace can control all this directly through debugfs, can you
> guarantee that locks will be enough ? The driver doesn't expect direct
> userspace access. I have a feeling this is really quite hacky.

Yes, this is fine. The most userspace can do is force a lot of retraining. But we
have timeouts on everything so I'm not really concerned.

--Sean

>> >>   * @irq: irq
>> >>   * @bridge: DRM bridge for the DP encoder
>> >>   * @next_bridge: The downstream bridge
>> >> @@ -299,6 +300,7 @@ struct zynqmp_dp {
>> >>  	struct zynqmp_dpsub *dpsub;
>> >>  	void __iomem *iomem;
>> >>  	struct reset_control *reset;
>> >> +	struct mutex lock;
>> >>  	int irq;
>> >>  
>> >>  	struct drm_bridge bridge;
>> >> @@ -308,7 +310,7 @@ struct zynqmp_dp {
>> >>  	struct drm_dp_aux aux;
>> >>  	struct phy *phy[ZYNQMP_DP_MAX_LANES];
>> >>  	u8 num_lanes;
>> >> -	struct delayed_work hpd_work;
>> >> +	struct delayed_work hpd_work, hpd_irq_work;
>> > 
>> > One variable per line please.
>> 
>> OK
>> 
>> >>  	enum drm_connector_status status;
>> >>  	bool enabled;
>> >>  
>> >> @@ -1371,8 +1373,10 @@ zynqmp_dp_bridge_mode_valid(struct drm_bridge *bridge,
>> >>  	}
>> >>  
>> >>  	/* Check with link rate and lane count */
>> >> +	mutex_lock(&dp->lock);
>> >>  	rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
>> >>  				  dp->link_config.max_lanes, dp->config.bpp);
>> >> +	mutex_unlock(&dp->lock);
>> >>  	if (mode->clock > rate) {
>> >>  		dev_dbg(dp->dev, "filtered mode %s for high pixel rate\n",
>> >>  			mode->name);
>> >> @@ -1399,6 +1403,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>> >>  
>> >>  	pm_runtime_get_sync(dp->dev);
>> >>  
>> >> +	mutex_lock(&dp->lock);
>> >>  	zynqmp_dp_disp_enable(dp, old_bridge_state);
>> >>  
>> >>  	/*
>> >> @@ -1459,6 +1464,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>> >>  	zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
>> >>  			ZYNQMP_DP_SOFTWARE_RESET_ALL);
>> >>  	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 1);
>> >> +	mutex_unlock(&dp->lock);
>> >>  }
>> >>  
>> >>  static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>> >> @@ -1466,6 +1472,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>> >>  {
>> >>  	struct zynqmp_dp *dp = bridge_to_dp(bridge);
>> >>  
>> >> +	mutex_lock(&dp->lock);
>> >>  	dp->enabled = false;
>> >>  	cancel_delayed_work(&dp->hpd_work);
>> >>  	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
>> >> @@ -1476,6 +1483,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>> >>  		zynqmp_dp_write(dp, ZYNQMP_DP_TX_AUDIO_CONTROL, 0);
>> >>  
>> >>  	zynqmp_dp_disp_disable(dp, old_bridge_state);
>> >> +	mutex_unlock(&dp->lock);
>> >>  
>> >>  	pm_runtime_put_sync(dp->dev);
>> >>  }
>> >> @@ -1518,6 +1526,8 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
>> >>  	u32 state, i;
>> >>  	int ret;
>> >>  
>> >> +	mutex_lock(&dp->lock);
>> >> +
>> >>  	/*
>> >>  	 * This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
>> >>  	 * get the HPD signal with some monitors.
>> >> @@ -1545,11 +1555,13 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
>> >>  					       dp->num_lanes);
>> >>  
>> >>  		dp->status = connector_status_connected;
>> >> +		mutex_unlock(&dp->lock);
>> >>  		return connector_status_connected;
>> >>  	}
>> >>  
>> >>  disconnected:
>> >>  	dp->status = connector_status_disconnected;
>> >> +	mutex_unlock(&dp->lock);
>> >>  	return connector_status_disconnected;
>> >>  }
>> >>  
>> >> @@ -1611,6 +1623,29 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
>> >>  	drm_bridge_hpd_notify(&dp->bridge, status);
>> >>  }
>> >>  
>> >> +static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
>> >> +{
>> >> +	struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
>> >> +					    hpd_irq_work.work);
>> >> +	u8 status[DP_LINK_STATUS_SIZE + 2];
>> >> +	int err;
>> >> +
>> >> +	mutex_lock(&dp->lock);
>> >> +	err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
>> >> +			       DP_LINK_STATUS_SIZE + 2);
>> >> +	if (err < 0) {
>> >> +		dev_dbg_ratelimited(dp->dev,
>> >> +				    "could not read sink status: %d\n", err);
>> >> +	} else {
>> >> +		if (status[4] & DP_LINK_STATUS_UPDATED ||
>> >> +		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
>> >> +		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
>> >> +			zynqmp_dp_train_loop(dp);
>> >> +		}
>> >> +	}
>> >> +	mutex_unlock(&dp->lock);
>> >> +}
>> >> +
>> >>  static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
>> >>  {
>> >>  	struct zynqmp_dp *dp = (struct zynqmp_dp *)data;
>> >> @@ -1635,23 +1670,9 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
>> >>  	if (status & ZYNQMP_DP_INT_HPD_EVENT)
>> >>  		schedule_delayed_work(&dp->hpd_work, 0);
>> >>  
>> >> -	if (status & ZYNQMP_DP_INT_HPD_IRQ) {
>> >> -		int ret;
>> >> -		u8 status[DP_LINK_STATUS_SIZE + 2];
>> >> +	if (status & ZYNQMP_DP_INT_HPD_IRQ)
>> >> +		schedule_delayed_work(&dp->hpd_irq_work, 0);
>> >>  
>> >> -		ret = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
>> >> -				       DP_LINK_STATUS_SIZE + 2);
>> >> -		if (ret < 0)
>> >> -			goto handled;
>> >> -
>> >> -		if (status[4] & DP_LINK_STATUS_UPDATED ||
>> >> -		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
>> >> -		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
>> >> -			zynqmp_dp_train_loop(dp);
>> >> -		}
>> >> -	}
>> >> -
>> >> -handled:
>> >>  	return IRQ_HANDLED;
>> >>  }
>> >>  
>> >> @@ -1674,8 +1695,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>> >>  	dp->dev = &pdev->dev;
>> >>  	dp->dpsub = dpsub;
>> >>  	dp->status = connector_status_disconnected;
>> >> +	mutex_init(&dp->lock);
>> >>  
>> >>  	INIT_DELAYED_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
>> >> +	INIT_DELAYED_WORK(&dp->hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
>> >>  
>> >>  	/* Acquire all resources (IOMEM, IRQ and PHYs). */
>> >>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
>> >> @@ -1775,6 +1798,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>> >>  	zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
>> >>  	disable_irq(dp->irq);
>> >>  
>> >> +	cancel_delayed_work_sync(&dp->hpd_irq_work);
>> >>  	cancel_delayed_work_sync(&dp->hpd_work);
>> >>  
>> >>  	zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
>> >> @@ -1782,4 +1806,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>> >>  
>> >>  	zynqmp_dp_phy_exit(dp);
>> >>  	zynqmp_dp_reset(dp, true);
>> >> +	mutex_destroy(&dp->lock);
>> >>  }
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] drm: zynqmp_dp: Split off several helper functions

[Sean Anderson]

In preparation for supporting compliance testing, split off several
helper functions. No functional change intended.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 49 +++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index d2dee58e7bf2..24043847dab4 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -627,6 +627,7 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
 /**
  * zynqmp_dp_update_vs_emph - Update the training values
  * @dp: DisplayPort IP core structure
+ * @train_set: A set of training values
  *
  * Update the training values based on the request from sink. The mapped values
  * are predefined, and values(vs, pe, pc) are from the device manual.
@@ -634,12 +635,12 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
  * Return: 0 if vs and emph are updated successfully, or the error code returned
  * by drm_dp_dpcd_write().
  */
-static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp)
+static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp, u8 *train_set)
 {
 	unsigned int i;
 	int ret;
 
-	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, dp->train_set,
+	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, train_set,
 				dp->mode.lane_cnt);
 	if (ret < 0)
 		return ret;
@@ -647,7 +648,7 @@ static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp)
 	for (i = 0; i < dp->mode.lane_cnt; i++) {
 		u32 reg = ZYNQMP_DP_SUB_TX_PHY_PRECURSOR_LANE_0 + i * 4;
 		union phy_configure_opts opts = { 0 };
-		u8 train = dp->train_set[i];
+		u8 train = train_set[i];
 
 		opts.dp.voltage[0] = (train & DP_TRAIN_VOLTAGE_SWING_MASK)
 				   >> DP_TRAIN_VOLTAGE_SWING_SHIFT;
@@ -691,7 +692,7 @@ static int zynqmp_dp_link_train_cr(struct zynqmp_dp *dp)
 	 * So, This loop should exit before 512 iterations
 	 */
 	for (max_tries = 0; max_tries < 512; max_tries++) {
-		ret = zynqmp_dp_update_vs_emph(dp);
+		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
 		if (ret)
 			return ret;
 
@@ -756,7 +757,7 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
 		return ret;
 
 	for (tries = 0; tries < DP_MAX_TRAINING_TRIES; tries++) {
-		ret = zynqmp_dp_update_vs_emph(dp);
+		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
 		if (ret)
 			return ret;
 
@@ -779,28 +780,28 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
 }
 
 /**
- * zynqmp_dp_train - Train the link
- * @dp: DisplayPort IP core structure
+ * zynqmp_dp_setup() - Set up major link parameters
+ * @bw_code: The link bandwidth as a multiple of 270 MHz
+ * @lane_cnt: The number of lanes to use
+ * @enhanced: Use enhanced framing
+ * @downspread: Enable spread-spectrum clocking
  *
- * Return: 0 if all trains are done successfully, or corresponding error code.
+ * Return: 0 on success, or -errno on failure
  */
-static int zynqmp_dp_train(struct zynqmp_dp *dp)
+static int zynqmp_dp_setup(struct zynqmp_dp *dp, u8 bw_code, u8 lane_cnt,
+			   bool enhanced, bool downspread)
 {
 	u32 reg;
-	u8 bw_code = dp->mode.bw_code;
-	u8 lane_cnt = dp->mode.lane_cnt;
 	u8 aux_lane_cnt = lane_cnt;
-	bool enhanced;
 	int ret;
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_LANE_COUNT_SET, lane_cnt);
-	enhanced = drm_dp_enhanced_frame_cap(dp->dpcd);
 	if (enhanced) {
 		zynqmp_dp_write(dp, ZYNQMP_DP_ENHANCED_FRAME_EN, 1);
 		aux_lane_cnt |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
 	}
 
-	if (dp->dpcd[3] & 0x1) {
+	if (downspread) {
 		zynqmp_dp_write(dp, ZYNQMP_DP_DOWNSPREAD_CTL, 1);
 		drm_dp_dpcd_writeb(&dp->aux, DP_DOWNSPREAD_CTRL,
 				   DP_SPREAD_AMP_0_5);
@@ -843,8 +844,24 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
 	}
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_PHY_CLOCK_SELECT, reg);
-	ret = zynqmp_dp_phy_ready(dp);
-	if (ret < 0)
+	return zynqmp_dp_phy_ready(dp);
+}
+
+
+/**
+ * zynqmp_dp_train - Train the link
+ * @dp: DisplayPort IP core structure
+ *
+ * Return: 0 if all trains are done successfully, or corresponding error code.
+ */
+static int zynqmp_dp_train(struct zynqmp_dp *dp)
+{
+	int ret;
+
+	ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
+			      drm_dp_enhanced_frame_cap(dp->dpcd),
+			      dp->dpcd[3] & 0x1);
+	if (ret)
 		return ret;
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_SCRAMBLING_DISABLE, 1);
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] drm: zynqmp_dp: Split off several helper functions

[Laurent Pinchart]

Hi Sean,

Thank you for the patch.

On Fri, Mar 15, 2024 at 07:09:14PM -0400, Sean Anderson wrote:
> In preparation for supporting compliance testing, split off several
> helper functions. No functional change intended.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 49 +++++++++++++++++++++-----------
>  1 file changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index d2dee58e7bf2..24043847dab4 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -627,6 +627,7 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
>  /**
>   * zynqmp_dp_update_vs_emph - Update the training values
>   * @dp: DisplayPort IP core structure
> + * @train_set: A set of training values
>   *
>   * Update the training values based on the request from sink. The mapped values
>   * are predefined, and values(vs, pe, pc) are from the device manual.
> @@ -634,12 +635,12 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
>   * Return: 0 if vs and emph are updated successfully, or the error code returned
>   * by drm_dp_dpcd_write().
>   */
> -static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp)
> +static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp, u8 *train_set)
>  {
>  	unsigned int i;
>  	int ret;
>  
> -	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, dp->train_set,
> +	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, train_set,
>  				dp->mode.lane_cnt);
>  	if (ret < 0)
>  		return ret;
> @@ -647,7 +648,7 @@ static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp)
>  	for (i = 0; i < dp->mode.lane_cnt; i++) {
>  		u32 reg = ZYNQMP_DP_SUB_TX_PHY_PRECURSOR_LANE_0 + i * 4;
>  		union phy_configure_opts opts = { 0 };
> -		u8 train = dp->train_set[i];
> +		u8 train = train_set[i];
>  
>  		opts.dp.voltage[0] = (train & DP_TRAIN_VOLTAGE_SWING_MASK)
>  				   >> DP_TRAIN_VOLTAGE_SWING_SHIFT;
> @@ -691,7 +692,7 @@ static int zynqmp_dp_link_train_cr(struct zynqmp_dp *dp)
>  	 * So, This loop should exit before 512 iterations
>  	 */
>  	for (max_tries = 0; max_tries < 512; max_tries++) {
> -		ret = zynqmp_dp_update_vs_emph(dp);
> +		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
>  		if (ret)
>  			return ret;
>  
> @@ -756,7 +757,7 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
>  		return ret;
>  
>  	for (tries = 0; tries < DP_MAX_TRAINING_TRIES; tries++) {
> -		ret = zynqmp_dp_update_vs_emph(dp);
> +		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
>  		if (ret)
>  			return ret;
>  
> @@ -779,28 +780,28 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
>  }
>  
>  /**
> - * zynqmp_dp_train - Train the link
> - * @dp: DisplayPort IP core structure
> + * zynqmp_dp_setup() - Set up major link parameters
> + * @bw_code: The link bandwidth as a multiple of 270 MHz
> + * @lane_cnt: The number of lanes to use
> + * @enhanced: Use enhanced framing
> + * @downspread: Enable spread-spectrum clocking
>   *
> - * Return: 0 if all trains are done successfully, or corresponding error code.
> + * Return: 0 on success, or -errno on failure
>   */
> -static int zynqmp_dp_train(struct zynqmp_dp *dp)
> +static int zynqmp_dp_setup(struct zynqmp_dp *dp, u8 bw_code, u8 lane_cnt,
> +			   bool enhanced, bool downspread)
>  {
>  	u32 reg;
> -	u8 bw_code = dp->mode.bw_code;
> -	u8 lane_cnt = dp->mode.lane_cnt;
>  	u8 aux_lane_cnt = lane_cnt;
> -	bool enhanced;
>  	int ret;
>  
>  	zynqmp_dp_write(dp, ZYNQMP_DP_LANE_COUNT_SET, lane_cnt);
> -	enhanced = drm_dp_enhanced_frame_cap(dp->dpcd);
>  	if (enhanced) {
>  		zynqmp_dp_write(dp, ZYNQMP_DP_ENHANCED_FRAME_EN, 1);
>  		aux_lane_cnt |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>  	}
>  
> -	if (dp->dpcd[3] & 0x1) {
> +	if (downspread) {
>  		zynqmp_dp_write(dp, ZYNQMP_DP_DOWNSPREAD_CTL, 1);
>  		drm_dp_dpcd_writeb(&dp->aux, DP_DOWNSPREAD_CTRL,
>  				   DP_SPREAD_AMP_0_5);
> @@ -843,8 +844,24 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
>  	}
>  
>  	zynqmp_dp_write(dp, ZYNQMP_DP_PHY_CLOCK_SELECT, reg);
> -	ret = zynqmp_dp_phy_ready(dp);
> -	if (ret < 0)
> +	return zynqmp_dp_phy_ready(dp);
> +}
> +
> +
> +/**
> + * zynqmp_dp_train - Train the link
> + * @dp: DisplayPort IP core structure
> + *
> + * Return: 0 if all trains are done successfully, or corresponding error code.
> + */
> +static int zynqmp_dp_train(struct zynqmp_dp *dp)
> +{
> +	int ret;
> +
> +	ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
> +			      drm_dp_enhanced_frame_cap(dp->dpcd),
> +			      dp->dpcd[3] & 0x1);

This patch looks OK. Assuming you make correct use of the new functions
in the next patches,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

On a side note, I think the above could be written

	ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
			      drm_dp_enhanced_frame_cap(dp->dpcd),
			      dp->dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5);

> +	if (ret)
>  		return ret;
>  
>  	zynqmp_dp_write(dp, ZYNQMP_DP_SCRAMBLING_DISABLE, 1);

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] drm: zynqmp_dp: Optionally ignore DPCD errors

[Sean Anderson]

When testing, it's convenient to be able to ignore DPCD errors if there
is test equipment which can't emulate a DPRX connected to the output.
Add some (currently-unused) options to ignore these errors and just
reconfigure our internal registers as we usually would.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 37 ++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 24043847dab4..040f7b88ee51 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -628,6 +628,7 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
  * zynqmp_dp_update_vs_emph - Update the training values
  * @dp: DisplayPort IP core structure
  * @train_set: A set of training values
+ * @ignore_dpcd: Ignore DPCD errors
  *
  * Update the training values based on the request from sink. The mapped values
  * are predefined, and values(vs, pe, pc) are from the device manual.
@@ -635,15 +636,19 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
  * Return: 0 if vs and emph are updated successfully, or the error code returned
  * by drm_dp_dpcd_write().
  */
-static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp, u8 *train_set)
+static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp, u8 *train_set,
+				    bool ignore_dpcd)
 {
 	unsigned int i;
 	int ret;
 
 	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, train_set,
 				dp->mode.lane_cnt);
-	if (ret < 0)
-		return ret;
+	if (ret < 0) {
+		if (!ignore_dpcd)
+			return ret;
+		dev_warn(dp->dev, "failed to update vs/emph\n");
+	}
 
 	for (i = 0; i < dp->mode.lane_cnt; i++) {
 		u32 reg = ZYNQMP_DP_SUB_TX_PHY_PRECURSOR_LANE_0 + i * 4;
@@ -692,7 +697,7 @@ static int zynqmp_dp_link_train_cr(struct zynqmp_dp *dp)
 	 * So, This loop should exit before 512 iterations
 	 */
 	for (max_tries = 0; max_tries < 512; max_tries++) {
-		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
+		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set, false);
 		if (ret)
 			return ret;
 
@@ -757,7 +762,7 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
 		return ret;
 
 	for (tries = 0; tries < DP_MAX_TRAINING_TRIES; tries++) {
-		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
+		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set, false);
 		if (ret)
 			return ret;
 
@@ -785,11 +790,12 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
  * @lane_cnt: The number of lanes to use
  * @enhanced: Use enhanced framing
  * @downspread: Enable spread-spectrum clocking
+ * @ignore_dpcd: Ignore DPCD errors; useful for testing
  *
  * Return: 0 on success, or -errno on failure
  */
 static int zynqmp_dp_setup(struct zynqmp_dp *dp, u8 bw_code, u8 lane_cnt,
-			   bool enhanced, bool downspread)
+			   bool enhanced, bool downspread, bool ignore_dpcd)
 {
 	u32 reg;
 	u8 aux_lane_cnt = lane_cnt;
@@ -812,21 +818,24 @@ static int zynqmp_dp_setup(struct zynqmp_dp *dp, u8 bw_code, u8 lane_cnt,
 
 	ret = drm_dp_dpcd_writeb(&dp->aux, DP_LANE_COUNT_SET, aux_lane_cnt);
 	if (ret < 0) {
-		dev_err(dp->dev, "failed to set lane count\n");
-		return ret;
+		dev_warn(dp->dev, "failed to set lane count\n");
+		if (!ignore_dpcd)
+			return ret;
 	}
 
 	ret = drm_dp_dpcd_writeb(&dp->aux, DP_MAIN_LINK_CHANNEL_CODING_SET,
 				 DP_SET_ANSI_8B10B);
 	if (ret < 0) {
-		dev_err(dp->dev, "failed to set ANSI 8B/10B encoding\n");
-		return ret;
+		dev_warn(dp->dev, "failed to set ANSI 8B/10B encoding\n");
+		if (!ignore_dpcd)
+			return ret;
 	}
 
 	ret = drm_dp_dpcd_writeb(&dp->aux, DP_LINK_BW_SET, bw_code);
 	if (ret < 0) {
-		dev_err(dp->dev, "failed to set DP bandwidth\n");
-		return ret;
+		dev_warn(dp->dev, "failed to set DP bandwidth\n");
+		if (!ignore_dpcd)
+			return ret;
 	}
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_LINK_BW_SET, bw_code);
@@ -860,7 +869,7 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
 
 	ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
 			      drm_dp_enhanced_frame_cap(dp->dpcd),
-			      dp->dpcd[3] & 0x1);
+			      dp->dpcd[3] & 0x1, false);
 	if (ret)
 		return ret;
 
@@ -877,7 +886,7 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
 	ret = drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET,
 				 DP_TRAINING_PATTERN_DISABLE);
 	if (ret < 0) {
-		dev_err(dp->dev, "failed to disable training pattern\n");
+		dev_warn(dp->dev, "failed to disable training pattern\n");
 		return ret;
 	}
 	zynqmp_dp_write(dp, ZYNQMP_DP_TRAINING_PATTERN_SET,
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/6] drm: zynqmp_dp: Optionally ignore DPCD errors

[Laurent Pinchart]

Hi Sean,

Thank you for the patch.

On Fri, Mar 15, 2024 at 07:09:15PM -0400, Sean Anderson wrote:
> When testing, it's convenient to be able to ignore DPCD errors if there
> is test equipment which can't emulate a DPRX connected to the output.
> Add some (currently-unused) options to ignore these errors and just
> reconfigure our internal registers as we usually would.

This seems to be a problem that is not limited to the ZynqMP DP.
Wouldn't it be better to solve it in the DRM DP DPCD helpers instead ?
You could expose a parameter on the AUX bus in debugfs to ignore errors,
and cause the drm_dp_dpcd_write*() functions to return 0.

> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 37 ++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 24043847dab4..040f7b88ee51 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -628,6 +628,7 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
>   * zynqmp_dp_update_vs_emph - Update the training values
>   * @dp: DisplayPort IP core structure
>   * @train_set: A set of training values
> + * @ignore_dpcd: Ignore DPCD errors
>   *
>   * Update the training values based on the request from sink. The mapped values
>   * are predefined, and values(vs, pe, pc) are from the device manual.
> @@ -635,15 +636,19 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
>   * Return: 0 if vs and emph are updated successfully, or the error code returned
>   * by drm_dp_dpcd_write().
>   */
> -static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp, u8 *train_set)
> +static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp, u8 *train_set,
> +				    bool ignore_dpcd)
>  {
>  	unsigned int i;
>  	int ret;
>  
>  	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, train_set,
>  				dp->mode.lane_cnt);
> -	if (ret < 0)
> -		return ret;
> +	if (ret < 0) {
> +		if (!ignore_dpcd)
> +			return ret;
> +		dev_warn(dp->dev, "failed to update vs/emph\n");
> +	}
>  
>  	for (i = 0; i < dp->mode.lane_cnt; i++) {
>  		u32 reg = ZYNQMP_DP_SUB_TX_PHY_PRECURSOR_LANE_0 + i * 4;
> @@ -692,7 +697,7 @@ static int zynqmp_dp_link_train_cr(struct zynqmp_dp *dp)
>  	 * So, This loop should exit before 512 iterations
>  	 */
>  	for (max_tries = 0; max_tries < 512; max_tries++) {
> -		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
> +		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set, false);
>  		if (ret)
>  			return ret;
>  
> @@ -757,7 +762,7 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
>  		return ret;
>  
>  	for (tries = 0; tries < DP_MAX_TRAINING_TRIES; tries++) {
> -		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
> +		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set, false);
>  		if (ret)
>  			return ret;
>  
> @@ -785,11 +790,12 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
>   * @lane_cnt: The number of lanes to use
>   * @enhanced: Use enhanced framing
>   * @downspread: Enable spread-spectrum clocking
> + * @ignore_dpcd: Ignore DPCD errors; useful for testing
>   *
>   * Return: 0 on success, or -errno on failure
>   */
>  static int zynqmp_dp_setup(struct zynqmp_dp *dp, u8 bw_code, u8 lane_cnt,
> -			   bool enhanced, bool downspread)
> +			   bool enhanced, bool downspread, bool ignore_dpcd)
>  {
>  	u32 reg;
>  	u8 aux_lane_cnt = lane_cnt;
> @@ -812,21 +818,24 @@ static int zynqmp_dp_setup(struct zynqmp_dp *dp, u8 bw_code, u8 lane_cnt,
>  
>  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_LANE_COUNT_SET, aux_lane_cnt);
>  	if (ret < 0) {
> -		dev_err(dp->dev, "failed to set lane count\n");
> -		return ret;
> +		dev_warn(dp->dev, "failed to set lane count\n");
> +		if (!ignore_dpcd)
> +			return ret;
>  	}
>  
>  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_MAIN_LINK_CHANNEL_CODING_SET,
>  				 DP_SET_ANSI_8B10B);
>  	if (ret < 0) {
> -		dev_err(dp->dev, "failed to set ANSI 8B/10B encoding\n");
> -		return ret;
> +		dev_warn(dp->dev, "failed to set ANSI 8B/10B encoding\n");
> +		if (!ignore_dpcd)
> +			return ret;
>  	}
>  
>  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_LINK_BW_SET, bw_code);
>  	if (ret < 0) {
> -		dev_err(dp->dev, "failed to set DP bandwidth\n");
> -		return ret;
> +		dev_warn(dp->dev, "failed to set DP bandwidth\n");
> +		if (!ignore_dpcd)
> +			return ret;
>  	}
>  
>  	zynqmp_dp_write(dp, ZYNQMP_DP_LINK_BW_SET, bw_code);
> @@ -860,7 +869,7 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
>  
>  	ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
>  			      drm_dp_enhanced_frame_cap(dp->dpcd),
> -			      dp->dpcd[3] & 0x1);
> +			      dp->dpcd[3] & 0x1, false);
>  	if (ret)
>  		return ret;
>  
> @@ -877,7 +886,7 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
>  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET,
>  				 DP_TRAINING_PATTERN_DISABLE);
>  	if (ret < 0) {
> -		dev_err(dp->dev, "failed to disable training pattern\n");
> +		dev_warn(dp->dev, "failed to disable training pattern\n");
>  		return ret;
>  	}
>  	zynqmp_dp_write(dp, ZYNQMP_DP_TRAINING_PATTERN_SET,

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/6] drm: zynqmp_dp: Optionally ignore DPCD errors

[Sean Anderson]

On 3/18/24 13:47, Laurent Pinchart wrote:
> Hi Sean,
> 
> Thank you for the patch.
> 
> On Fri, Mar 15, 2024 at 07:09:15PM -0400, Sean Anderson wrote:
>> When testing, it's convenient to be able to ignore DPCD errors if there
>> is test equipment which can't emulate a DPRX connected to the output.
>> Add some (currently-unused) options to ignore these errors and just
>> reconfigure our internal registers as we usually would.
> 
> This seems to be a problem that is not limited to the ZynqMP DP.
> Wouldn't it be better to solve it in the DRM DP DPCD helpers instead ?
> You could expose a parameter on the AUX bus in debugfs to ignore errors,
> and cause the drm_dp_dpcd_write*() functions to return 0.

I think this is probably the easiest thing. I'll add this for v2.

I think something similar might be nice for HPD events (instead of always
ignoring them in test mode). This would make it easier to add DPRX-driven
testing in the future.

--Sean

>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 37 ++++++++++++++++++++------------
>>  1 file changed, 23 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> index 24043847dab4..040f7b88ee51 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> @@ -628,6 +628,7 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
>>   * zynqmp_dp_update_vs_emph - Update the training values
>>   * @dp: DisplayPort IP core structure
>>   * @train_set: A set of training values
>> + * @ignore_dpcd: Ignore DPCD errors
>>   *
>>   * Update the training values based on the request from sink. The mapped values
>>   * are predefined, and values(vs, pe, pc) are from the device manual.
>> @@ -635,15 +636,19 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
>>   * Return: 0 if vs and emph are updated successfully, or the error code returned
>>   * by drm_dp_dpcd_write().
>>   */
>> -static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp, u8 *train_set)
>> +static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp, u8 *train_set,
>> +				    bool ignore_dpcd)
>>  {
>>  	unsigned int i;
>>  	int ret;
>>  
>>  	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, train_set,
>>  				dp->mode.lane_cnt);
>> -	if (ret < 0)
>> -		return ret;
>> +	if (ret < 0) {
>> +		if (!ignore_dpcd)
>> +			return ret;
>> +		dev_warn(dp->dev, "failed to update vs/emph\n");
>> +	}
>>  
>>  	for (i = 0; i < dp->mode.lane_cnt; i++) {
>>  		u32 reg = ZYNQMP_DP_SUB_TX_PHY_PRECURSOR_LANE_0 + i * 4;
>> @@ -692,7 +697,7 @@ static int zynqmp_dp_link_train_cr(struct zynqmp_dp *dp)
>>  	 * So, This loop should exit before 512 iterations
>>  	 */
>>  	for (max_tries = 0; max_tries < 512; max_tries++) {
>> -		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
>> +		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set, false);
>>  		if (ret)
>>  			return ret;
>>  
>> @@ -757,7 +762,7 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
>>  		return ret;
>>  
>>  	for (tries = 0; tries < DP_MAX_TRAINING_TRIES; tries++) {
>> -		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
>> +		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set, false);
>>  		if (ret)
>>  			return ret;
>>  
>> @@ -785,11 +790,12 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
>>   * @lane_cnt: The number of lanes to use
>>   * @enhanced: Use enhanced framing
>>   * @downspread: Enable spread-spectrum clocking
>> + * @ignore_dpcd: Ignore DPCD errors; useful for testing
>>   *
>>   * Return: 0 on success, or -errno on failure
>>   */
>>  static int zynqmp_dp_setup(struct zynqmp_dp *dp, u8 bw_code, u8 lane_cnt,
>> -			   bool enhanced, bool downspread)
>> +			   bool enhanced, bool downspread, bool ignore_dpcd)
>>  {
>>  	u32 reg;
>>  	u8 aux_lane_cnt = lane_cnt;
>> @@ -812,21 +818,24 @@ static int zynqmp_dp_setup(struct zynqmp_dp *dp, u8 bw_code, u8 lane_cnt,
>>  
>>  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_LANE_COUNT_SET, aux_lane_cnt);
>>  	if (ret < 0) {
>> -		dev_err(dp->dev, "failed to set lane count\n");
>> -		return ret;
>> +		dev_warn(dp->dev, "failed to set lane count\n");
>> +		if (!ignore_dpcd)
>> +			return ret;
>>  	}
>>  
>>  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_MAIN_LINK_CHANNEL_CODING_SET,
>>  				 DP_SET_ANSI_8B10B);
>>  	if (ret < 0) {
>> -		dev_err(dp->dev, "failed to set ANSI 8B/10B encoding\n");
>> -		return ret;
>> +		dev_warn(dp->dev, "failed to set ANSI 8B/10B encoding\n");
>> +		if (!ignore_dpcd)
>> +			return ret;
>>  	}
>>  
>>  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_LINK_BW_SET, bw_code);
>>  	if (ret < 0) {
>> -		dev_err(dp->dev, "failed to set DP bandwidth\n");
>> -		return ret;
>> +		dev_warn(dp->dev, "failed to set DP bandwidth\n");
>> +		if (!ignore_dpcd)
>> +			return ret;
>>  	}
>>  
>>  	zynqmp_dp_write(dp, ZYNQMP_DP_LINK_BW_SET, bw_code);
>> @@ -860,7 +869,7 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
>>  
>>  	ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
>>  			      drm_dp_enhanced_frame_cap(dp->dpcd),
>> -			      dp->dpcd[3] & 0x1);
>> +			      dp->dpcd[3] & 0x1, false);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -877,7 +886,7 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
>>  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET,
>>  				 DP_TRAINING_PATTERN_DISABLE);
>>  	if (ret < 0) {
>> -		dev_err(dp->dev, "failed to disable training pattern\n");
>> +		dev_warn(dp->dev, "failed to disable training pattern\n");
>>  		return ret;
>>  	}
>>  	zynqmp_dp_write(dp, ZYNQMP_DP_TRAINING_PATTERN_SET,
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Sean Anderson]

Add a debugfs interface for exercising the various test modes supported
by the DisplayPort controller. This allows performing compliance
testing, or performing signal integrity measurements on a failing link.
At the moment, we do not support sink-driven link quality testing,
although such support would be fairly easy to add.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 591 ++++++++++++++++++++++++++++++-
 1 file changed, 590 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 040f7b88ee51..57032186e1ca 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -18,7 +18,9 @@
 #include <drm/drm_modes.h>
 #include <drm/drm_of.h>
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/io.h>
@@ -50,6 +52,7 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
 #define ZYNQMP_DP_LANE_COUNT_SET			0x4
 #define ZYNQMP_DP_ENHANCED_FRAME_EN			0x8
 #define ZYNQMP_DP_TRAINING_PATTERN_SET			0xc
+#define ZYNQMP_DP_LINK_QUAL_PATTERN_SET			0x10
 #define ZYNQMP_DP_SCRAMBLING_DISABLE			0x14
 #define ZYNQMP_DP_DOWNSPREAD_CTL			0x18
 #define ZYNQMP_DP_SOFTWARE_RESET			0x1c
@@ -63,6 +66,9 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
 							 ZYNQMP_DP_SOFTWARE_RESET_STREAM3 | \
 							 ZYNQMP_DP_SOFTWARE_RESET_STREAM4 | \
 							 ZYNQMP_DP_SOFTWARE_RESET_AUX)
+#define ZYNQMP_DP_COMP_PATTERN_80BIT_1			0x20
+#define ZYNQMP_DP_COMP_PATTERN_80BIT_2			0x24
+#define ZYNQMP_DP_COMP_PATTERN_80BIT_3			0x28
 
 /* Core enable registers */
 #define ZYNQMP_DP_TRANSMITTER_ENABLE			0x80
@@ -206,6 +212,7 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
 #define ZYNQMP_DP_TX_PHY_POWER_DOWN_LANE_2		BIT(2)
 #define ZYNQMP_DP_TX_PHY_POWER_DOWN_LANE_3		BIT(3)
 #define ZYNQMP_DP_TX_PHY_POWER_DOWN_ALL			0xf
+#define ZYNQMP_DP_TRANSMIT_PRBS7			0x230
 #define ZYNQMP_DP_PHY_PRECURSOR_LANE_0			0x23c
 #define ZYNQMP_DP_PHY_PRECURSOR_LANE_1			0x240
 #define ZYNQMP_DP_PHY_PRECURSOR_LANE_2			0x244
@@ -273,6 +280,69 @@ struct zynqmp_dp_config {
 	u8 bpp;
 };
 
+/**
+ * enum test_pattern - Test patterns for test testing
+ * TEST_VIDEO: Use regular video input
+ * TEST_SYMBOL_ERROR: Symbol error measurement pattern
+ * TEST_PRBS7: Output of the PRBS7 (x^7 + x^6 + 1) polynomial
+ * TEST_80BIT_CUSTOM: A custom 80-bit pattern
+ * TEST_CP2520: HBR2 compliance eye pattern
+ * TEST_TPS1: Link training symbol pattern TPS1 (/D10.2/)
+ * TEST_TPS2: Link training symbol pattern TPS2
+ * TEST_TPS3: Link training symbol pattern TPS3 (for HBR2)
+ */
+enum test_pattern {
+	TEST_VIDEO,
+	TEST_TPS1,
+	TEST_TPS2,
+	TEST_TPS3,
+	TEST_SYMBOL_ERROR,
+	TEST_PRBS7,
+	TEST_80BIT_CUSTOM,
+	TEST_CP2520,
+};
+
+static const char *const test_pattern_str[] = {
+	[TEST_VIDEO] = "video",
+	[TEST_TPS1] = "tps1",
+	[TEST_TPS2] = "tps2",
+	[TEST_TPS3] = "tps3",
+	[TEST_SYMBOL_ERROR] = "symbol-error",
+	[TEST_PRBS7] = "prbs7",
+	[TEST_80BIT_CUSTOM] = "80bit-custom",
+	[TEST_CP2520] = "cp2520",
+};
+
+/**
+ * struct zynqmp_dp_test - Configuration for test mode
+ * @pattern: The test pattern
+ * @enhanced: Use enhanced framing
+ * @downspread: Use SSC
+ * @active: Whether test mode is active
+ * @custom: Custom pattern for %TEST_80BIT_CUSTOM
+ * @train_set: Voltage/preemphasis settings
+ * @bw_code: Bandwidth code for the link
+ * @link_cnt: Number of lanes
+ */
+struct zynqmp_dp_test {
+	enum test_pattern pattern;
+	bool enhanced, downspread, active;
+	u8 custom[10];
+	u8 train_set[ZYNQMP_DP_MAX_LANES];
+	u8 bw_code;
+	u8 link_cnt;
+};
+
+/**
+ * struct zynqmp_dp_train_set_priv - Private data for train_set debugfs files
+ * @dp: DisplayPort IP core structure
+ * @lane: The lane for this file
+ */
+struct zynqmp_dp_train_set_priv {
+	struct zynqmp_dp *dp;
+	int lane;
+};
+
 /**
  * struct zynqmp_dp - Xilinx DisplayPort core
  * @dev: device structure
@@ -283,6 +353,7 @@ struct zynqmp_dp_config {
  * @irq: irq
  * @bridge: DRM bridge for the DP encoder
  * @next_bridge: The downstream bridge
+ * @test: Configuration for test mode
  * @config: IP core configuration from DTS
  * @aux: aux channel
  * @phy: PHY handles for DP lanes
@@ -294,6 +365,7 @@ struct zynqmp_dp_config {
  * @link_config: common link configuration between IP core and sink device
  * @mode: current mode between IP core and sink device
  * @train_set: set of training data
+ * @debugfs_train_set: Debugfs private data for @train_set
  */
 struct zynqmp_dp {
 	struct device *dev;
@@ -306,6 +378,7 @@ struct zynqmp_dp {
 	struct drm_bridge bridge;
 	struct drm_bridge *next_bridge;
 
+	struct zynqmp_dp_test test;
 	struct zynqmp_dp_config config;
 	struct drm_dp_aux aux;
 	struct phy *phy[ZYNQMP_DP_MAX_LANES];
@@ -318,6 +391,7 @@ struct zynqmp_dp {
 	struct zynqmp_dp_link_config link_config;
 	struct zynqmp_dp_mode mode;
 	u8 train_set[ZYNQMP_DP_MAX_LANES];
+	struct zynqmp_dp_train_set_priv debugfs_train_set[ZYNQMP_DP_MAX_LANES];
 };
 
 static inline struct zynqmp_dp *bridge_to_dp(struct drm_bridge *bridge)
@@ -1599,6 +1673,510 @@ static struct edid *zynqmp_dp_bridge_get_edid(struct drm_bridge *bridge,
 	return drm_get_edid(connector, &dp->aux.ddc);
 }
 
+/* -----------------------------------------------------------------------------
+ * debugfs
+ */
+
+/**
+ * zynqmp_dp_set_test_pattern() - Configure the link for a test pattern
+ * @dp: DisplayPort IP core structure
+ */
+static void zynqmp_dp_set_test_pattern(struct zynqmp_dp *dp,
+				       enum test_pattern pattern,
+				       u8 *const custom)
+{
+	bool scramble = false;
+	u32 train_pattern = 0;
+	u32 link_pattern = 0;
+	u8 dpcd_train = 0;
+	u8 dpcd_link = 0;
+	int err;
+
+	switch (pattern) {
+	case TEST_TPS1:
+		train_pattern = 1;
+		break;
+	case TEST_TPS2:
+		train_pattern = 2;
+		break;
+	case TEST_TPS3:
+		train_pattern = 3;
+		break;
+	case TEST_SYMBOL_ERROR:
+		scramble = true;
+		link_pattern = DP_PHY_TEST_PATTERN_ERROR_COUNT;
+		break;
+	case TEST_PRBS7:
+		/* We use a dedicated register to enable PRBS7 */
+		dpcd_link = DP_LINK_QUAL_PATTERN_ERROR_RATE;
+		break;
+	case TEST_80BIT_CUSTOM: {
+		const u8 *p = custom;
+
+		link_pattern = DP_LINK_QUAL_PATTERN_80BIT_CUSTOM;
+
+		zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_1,
+				(p[3] << 24) | (p[2] << 16) | (p[1] << 8) | p[0]);
+		zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_2,
+				(p[7] << 24) | (p[6] << 16) | (p[5] << 8) | p[4]);
+		zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_3,
+				(p[9] << 8) | p[8]);
+		break;
+	}
+	case TEST_CP2520:
+		link_pattern = DP_LINK_QUAL_PATTERN_CP2520_PAT_1;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		fallthrough;
+	case TEST_VIDEO:
+		scramble = true;
+	}
+
+	zynqmp_dp_write(dp, ZYNQMP_DP_SCRAMBLING_DISABLE, !scramble);
+	zynqmp_dp_write(dp, ZYNQMP_DP_TRAINING_PATTERN_SET, train_pattern);
+	zynqmp_dp_write(dp, ZYNQMP_DP_LINK_QUAL_PATTERN_SET, link_pattern);
+	zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMIT_PRBS7, pattern == TEST_PRBS7);
+
+	dpcd_link = dpcd_link ?: link_pattern;
+	dpcd_train = train_pattern;
+	if (!scramble)
+		dpcd_train |= DP_LINK_SCRAMBLING_DISABLE;
+
+	if (dp->dpcd[DP_DPCD_REV] < 0x12) {
+		if (pattern == TEST_CP2520)
+			dev_warn(dp->dev,
+				"can't set sink link quality pattern to CP2520 for DPCD < r1.2; error counters will be invalid\n");
+		else
+			dpcd_train |= FIELD_PREP(DP_LINK_QUAL_PATTERN_11_MASK,
+						 dpcd_link);
+	} else {
+		u8 dpcd_link_lane[ZYNQMP_DP_MAX_LANES];
+
+		memset(dpcd_link_lane, dpcd_link, ZYNQMP_DP_MAX_LANES);
+		err = drm_dp_dpcd_write(&dp->aux, DP_LINK_QUAL_LANE0_SET,
+					dpcd_link_lane, ZYNQMP_DP_MAX_LANES);
+		if (err < 0)
+			dev_err(dp->dev, "failed to set quality pattern\n");
+	}
+
+	err = drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET, dpcd_train);
+	if (err < 0)
+		dev_err(dp->dev, "failed to set training pattern\n");
+}
+
+static int zynqmp_dp_test_setup(struct zynqmp_dp *dp)
+{
+	return zynqmp_dp_setup(dp, dp->test.bw_code, dp->test.link_cnt,
+			       dp->test.enhanced, dp->test.downspread, true);
+}
+
+static ssize_t zynqmp_dp_pattern_read(struct file *file, char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	struct zynqmp_dp *dp = file->private_data;
+	char buf[16];
+	ssize_t ret;
+
+	ret = debugfs_file_get(dentry);
+	if (unlikely(ret))
+		return ret;
+
+	mutex_lock(&dp->lock);
+	ret = snprintf(buf, sizeof(buf), "%s\n",
+		       test_pattern_str[dp->test.pattern]);
+	mutex_unlock(&dp->lock);
+
+	debugfs_file_put(dentry);
+	return simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+}
+
+static ssize_t zynqmp_dp_pattern_write(struct file *file,
+				       const char __user *user_buf,
+				       size_t count, loff_t *ppos)
+{
+
+	struct dentry *dentry = file->f_path.dentry;
+	struct zynqmp_dp *dp = file->private_data;
+	char buf[16];
+	ssize_t ret;
+	int pattern;
+
+	ret = debugfs_file_get(dentry);
+	if (unlikely(ret))
+		return ret;
+
+	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf,
+				     count);
+	if (ret < 0)
+		goto out;
+	buf[ret] = '\0';
+
+	pattern = sysfs_match_string(test_pattern_str, buf);
+	if (pattern < 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	mutex_lock(&dp->lock);
+	dp->test.pattern = pattern;
+	if (dp->test.active)
+		zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
+					   dp->test.custom);
+	mutex_unlock(&dp->lock);
+
+out:
+	debugfs_file_put(dentry);
+	return ret;
+}
+
+static const struct file_operations fops_zynqmp_dp_pattern = {
+	.read = zynqmp_dp_pattern_read,
+	.write = zynqmp_dp_pattern_write,
+	.open = simple_open,
+	.llseek = noop_llseek,
+};
+
+static int zynqmp_dp_enhanced_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = dp->test.enhanced;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_enhanced_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+	int ret = 0;
+
+	mutex_lock(&dp->lock);
+	dp->test.enhanced = val;
+	if (dp->test.active)
+		ret = zynqmp_dp_test_setup(dp);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_enhanced, zynqmp_dp_enhanced_get,
+			 zynqmp_dp_enhanced_set, "%llu\n");
+
+static int zynqmp_dp_downspread_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = dp->test.downspread;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_downspread_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+	int ret = 0;
+
+	mutex_lock(&dp->lock);
+	dp->test.downspread = val;
+	if (dp->test.active)
+		ret = zynqmp_dp_test_setup(dp);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_downspread, zynqmp_dp_downspread_get,
+			 zynqmp_dp_downspread_set, "%llu\n");
+
+static int zynqmp_dp_active_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = dp->test.active;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_active_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+	int ret = 0;
+
+	mutex_lock(&dp->lock);
+	if (val) {
+		if (val < 2) {
+			ret = zynqmp_dp_test_setup(dp);
+			if (ret)
+				goto out;
+		}
+
+		zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
+					   dp->test.custom);
+		zynqmp_dp_update_vs_emph(dp, dp->test.train_set, true);
+		dp->test.active = true;
+	} else {
+		dp->test.active = false;
+		zynqmp_dp_set_test_pattern(dp, TEST_VIDEO, NULL);
+		zynqmp_dp_train_loop(dp);
+	}
+out:
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_active, zynqmp_dp_active_get,
+			 zynqmp_dp_active_set, "%llu\n");
+
+static ssize_t zynqmp_dp_custom_read(struct file *file, char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	struct zynqmp_dp *dp = file->private_data;
+	ssize_t ret;
+
+	ret = debugfs_file_get(dentry);
+	if (unlikely(ret))
+		return ret;
+
+	mutex_lock(&dp->lock);
+	ret = simple_read_from_buffer(user_buf, count, ppos, &dp->test.custom,
+				      sizeof(dp->test.custom));
+	mutex_unlock(&dp->lock);
+
+	debugfs_file_put(dentry);
+	return ret;
+}
+
+static ssize_t zynqmp_dp_custom_write(struct file *file,
+				      const char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+
+	struct dentry *dentry = file->f_path.dentry;
+	struct zynqmp_dp *dp = file->private_data;
+	ssize_t ret;
+	char buf[sizeof(dp->test.custom)];
+
+	ret = debugfs_file_get(dentry);
+	if (unlikely(ret))
+		return ret;
+
+	ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
+	if (ret < 0)
+		goto out;
+
+	mutex_lock(&dp->lock);
+	memcpy(dp->test.custom, buf, ret);
+	if (dp->test.active)
+		zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
+					   dp->test.custom);
+	mutex_unlock(&dp->lock);
+
+out:
+	debugfs_file_put(dentry);
+	return ret;
+}
+
+static const struct file_operations fops_zynqmp_dp_custom = {
+	.read = zynqmp_dp_custom_read,
+	.write = zynqmp_dp_custom_write,
+	.open = simple_open,
+	.llseek = noop_llseek,
+};
+
+static int zynqmp_dp_swing_get(void *data, u64 *val)
+{
+	struct zynqmp_dp_train_set_priv *priv = data;
+	struct zynqmp_dp *dp = priv->dp;
+
+	mutex_lock(&dp->lock);
+	*val = dp->test.train_set[priv->lane] & DP_TRAIN_VOLTAGE_SWING_MASK;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_swing_set(void *data, u64 val)
+{
+	struct zynqmp_dp_train_set_priv *priv = data;
+	struct zynqmp_dp *dp = priv->dp;
+	u8 *train_set = &dp->test.train_set[priv->lane];
+	int ret = 0;
+
+	if (val > 3)
+		return -EINVAL;
+
+	mutex_lock(&dp->lock);
+	*train_set &= ~(DP_TRAIN_MAX_SWING_REACHED |
+			DP_TRAIN_VOLTAGE_SWING_MASK);
+	*train_set |= val;
+	if (val == 3)
+		*train_set |= DP_TRAIN_MAX_SWING_REACHED;
+
+	if (dp->test.active)
+		zynqmp_dp_update_vs_emph(dp, dp->test.train_set, true);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_swing, zynqmp_dp_swing_get,
+			 zynqmp_dp_swing_set, "%llu\n");
+
+static int zynqmp_dp_preemphasis_get(void *data, u64 *val)
+{
+	struct zynqmp_dp_train_set_priv *priv = data;
+	struct zynqmp_dp *dp = priv->dp;
+
+	mutex_lock(&dp->lock);
+	*val = FIELD_GET(DP_TRAIN_PRE_EMPHASIS_MASK,
+			 dp->test.train_set[priv->lane]);
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_preemphasis_set(void *data, u64 val)
+{
+	struct zynqmp_dp_train_set_priv *priv = data;
+	struct zynqmp_dp *dp = priv->dp;
+	u8 *train_set = &dp->test.train_set[priv->lane];
+	int ret = 0;
+
+	if (val > 2)
+		return -EINVAL;
+
+	mutex_lock(&dp->lock);
+	*train_set &= ~(DP_TRAIN_MAX_PRE_EMPHASIS_REACHED |
+			DP_TRAIN_PRE_EMPHASIS_MASK);
+	*train_set |= val;
+	if (val == 2)
+		*train_set |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
+
+	if (dp->test.active)
+		zynqmp_dp_update_vs_emph(dp, dp->test.train_set, true);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_preemphasis, zynqmp_dp_preemphasis_get,
+			 zynqmp_dp_preemphasis_set, "%llu\n");
+
+static int zynqmp_dp_lanes_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = dp->test.link_cnt;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_lanes_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+	int ret = 0;
+
+	if (val > ZYNQMP_DP_MAX_LANES)
+		return -EINVAL;
+
+	mutex_lock(&dp->lock);
+	if (val > dp->num_lanes) {
+		ret = -EINVAL;
+	} else {
+		dp->test.link_cnt = val;
+		if (dp->test.active)
+			ret = zynqmp_dp_test_setup(dp);
+	}
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_lanes, zynqmp_dp_lanes_get,
+			 zynqmp_dp_lanes_set, "%llu\n");
+
+static int zynqmp_dp_rate_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = drm_dp_bw_code_to_link_rate(dp->test.bw_code) * 10000;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_rate_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+	u8 bw_code = drm_dp_link_rate_to_bw_code(val / 10000);
+	int link_rate = drm_dp_bw_code_to_link_rate(bw_code);
+	int ret = 0;
+
+	if (val / 10000 != link_rate)
+		return -EINVAL;
+
+	if (bw_code != DP_LINK_BW_1_62 && bw_code != DP_LINK_BW_2_7 &&
+	    bw_code != DP_LINK_BW_5_4)
+		return -EINVAL;
+
+	mutex_lock(&dp->lock);
+	dp->test.bw_code = bw_code;
+	if (dp->test.active)
+		ret = zynqmp_dp_test_setup(dp);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
+			 zynqmp_dp_rate_set, "%llu\n");
+
+static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
+					  struct dentry *root)
+{
+	struct zynqmp_dp *dp = bridge_to_dp(bridge);
+	struct dentry *test;
+	int i;
+
+	dp->test.bw_code = DP_LINK_BW_5_4;
+	dp->test.link_cnt = dp->num_lanes;
+
+	test = debugfs_create_dir("test", root);
+#define CREATE_FILE(name) \
+	debugfs_create_file(#name, 0600, test, dp, &fops_zynqmp_dp_##name)
+	CREATE_FILE(pattern);
+	CREATE_FILE(enhanced);
+	CREATE_FILE(downspread);
+	CREATE_FILE(active);
+	CREATE_FILE(custom);
+	CREATE_FILE(rate);
+	CREATE_FILE(lanes);
+
+	for (i = 0; i < dp->num_lanes; i++) {
+		static const char fmt[] = "lane%d_preemphasis";
+		char name[sizeof(fmt)];
+
+		dp->debugfs_train_set[i].dp = dp;
+		dp->debugfs_train_set[i].lane = i;
+
+		sprintf(name, fmt, i);
+		debugfs_create_file(name, 0600, test,
+				    &dp->debugfs_train_set[i],
+				    &fops_zynqmp_dp_preemphasis);
+
+		sprintf(name, "lane%d_swing", i);
+		debugfs_create_file(name, 0600, test,
+				    &dp->debugfs_train_set[i],
+				    &fops_zynqmp_dp_swing);
+	}
+}
+
 static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
 	.attach = zynqmp_dp_bridge_attach,
 	.detach = zynqmp_dp_bridge_detach,
@@ -1611,6 +2189,7 @@ static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
 	.atomic_check = zynqmp_dp_bridge_atomic_check,
 	.detect = zynqmp_dp_bridge_detect,
 	.get_edid = zynqmp_dp_bridge_get_edid,
+	.debugfs_init = zynqmp_dp_bridge_debugfs_init,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1645,6 +2224,9 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
 					    hpd_work.work);
 	enum drm_connector_status status;
 
+	if (dp->test.active)
+		return;
+
 	status = zynqmp_dp_bridge_detect(&dp->bridge);
 	drm_bridge_hpd_notify(&dp->bridge, status);
 }
@@ -1666,7 +2248,14 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
 		if (status[4] & DP_LINK_STATUS_UPDATED ||
 		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
 		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
-			zynqmp_dp_train_loop(dp);
+			if (dp->test.active) {
+				dev_dbg(dp->dev, "Ignoring HPD IRQ in test mode\n");
+			} else {
+				dev_dbg(dp->dev,
+					"Retraining due to HPD IRQ (status is [%*ph])\n",
+					(int)sizeof(status), status);
+				zynqmp_dp_train_loop(dp);
+			}
 		}
 	}
 	mutex_unlock(&dp->lock);
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[kernel test robot]

Hi Sean,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.8]
[cannot apply to drm-misc/drm-misc-next linus/master next-20240315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
base:   v6.8
patch link:    https://lore.kernel.org/r/20240315230916.1759060-7-sean.anderson%40linux.dev
patch subject: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
config: microblaze-allmodconfig (https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/reproduce)

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>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403161704.ACHJdSJG-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/xlnx/zynqmp_dp.c: In function 'zynqmp_dp_bridge_debugfs_init':
>> drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:31: warning: 'sprintf' may write a terminating nul past the end of the destination [-Wformat-overflow=]
    2168 |                 sprintf(name, fmt, i);
         |                               ^~~
   drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:17: note: 'sprintf' output between 18 and 20 bytes into a destination of size 19
    2168 |                 sprintf(name, fmt, i);
         |                 ^~~~~~~~~~~~~~~~~~~~~


vim +/sprintf +2168 drivers/gpu/drm/xlnx/zynqmp_dp.c

  2136	
  2137	DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
  2138				 zynqmp_dp_rate_set, "%llu\n");
  2139	
  2140	static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
  2141						  struct dentry *root)
  2142	{
  2143		struct zynqmp_dp *dp = bridge_to_dp(bridge);
  2144		struct dentry *test;
  2145		int i;
  2146	
  2147		dp->test.bw_code = DP_LINK_BW_5_4;
  2148		dp->test.link_cnt = dp->num_lanes;
  2149	
  2150		test = debugfs_create_dir("test", root);
  2151	#define CREATE_FILE(name) \
  2152		debugfs_create_file(#name, 0600, test, dp, &fops_zynqmp_dp_##name)
  2153		CREATE_FILE(pattern);
  2154		CREATE_FILE(enhanced);
  2155		CREATE_FILE(downspread);
  2156		CREATE_FILE(active);
  2157		CREATE_FILE(custom);
  2158		CREATE_FILE(rate);
  2159		CREATE_FILE(lanes);
  2160	
  2161		for (i = 0; i < dp->num_lanes; i++) {
  2162			static const char fmt[] = "lane%d_preemphasis";
  2163			char name[sizeof(fmt)];
  2164	
  2165			dp->debugfs_train_set[i].dp = dp;
  2166			dp->debugfs_train_set[i].lane = i;
  2167	
> 2168			sprintf(name, fmt, i);
  2169			debugfs_create_file(name, 0600, test,
  2170					    &dp->debugfs_train_set[i],
  2171					    &fops_zynqmp_dp_preemphasis);
  2172	
  2173			sprintf(name, "lane%d_swing", i);
  2174			debugfs_create_file(name, 0600, test,
  2175					    &dp->debugfs_train_set[i],
  2176					    &fops_zynqmp_dp_swing);
  2177		}
  2178	}
  2179	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Sean Anderson]

On 3/16/24 06:14, kernel test robot wrote:
> Hi Sean,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on v6.8]
> [cannot apply to drm-misc/drm-misc-next linus/master next-20240315]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
> base:   v6.8
> patch link:    https://lore.kernel.org/r/20240315230916.1759060-7-sean.anderson%40linux.dev
> patch subject: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
> config: microblaze-allmodconfig (https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/config)
> compiler: microblaze-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/reproduce)
> 
> 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>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202403161704.ACHJdSJG-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>    drivers/gpu/drm/xlnx/zynqmp_dp.c: In function 'zynqmp_dp_bridge_debugfs_init':
>>> drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:31: warning: 'sprintf' may write a terminating nul past the end of the destination [-Wformat-overflow=]
>     2168 |                 sprintf(name, fmt, i);
>          |                               ^~~
>    drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:17: note: 'sprintf' output between 18 and 20 bytes into a destination of size 19
>     2168 |                 sprintf(name, fmt, i);
>          |                 ^~~~~~~~~~~~~~~~~~~~~

Not a bug, as i will be at most 4, which uses 1 digit.

--Sean

> vim +/sprintf +2168 drivers/gpu/drm/xlnx/zynqmp_dp.c
> 
>   2136	
>   2137	DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
>   2138				 zynqmp_dp_rate_set, "%llu\n");
>   2139	
>   2140	static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
>   2141						  struct dentry *root)
>   2142	{
>   2143		struct zynqmp_dp *dp = bridge_to_dp(bridge);
>   2144		struct dentry *test;
>   2145		int i;
>   2146	
>   2147		dp->test.bw_code = DP_LINK_BW_5_4;
>   2148		dp->test.link_cnt = dp->num_lanes;
>   2149	
>   2150		test = debugfs_create_dir("test", root);
>   2151	#define CREATE_FILE(name) \
>   2152		debugfs_create_file(#name, 0600, test, dp, &fops_zynqmp_dp_##name)
>   2153		CREATE_FILE(pattern);
>   2154		CREATE_FILE(enhanced);
>   2155		CREATE_FILE(downspread);
>   2156		CREATE_FILE(active);
>   2157		CREATE_FILE(custom);
>   2158		CREATE_FILE(rate);
>   2159		CREATE_FILE(lanes);
>   2160	
>   2161		for (i = 0; i < dp->num_lanes; i++) {
>   2162			static const char fmt[] = "lane%d_preemphasis";
>   2163			char name[sizeof(fmt)];
>   2164	
>   2165			dp->debugfs_train_set[i].dp = dp;
>   2166			dp->debugfs_train_set[i].lane = i;
>   2167	
>> 2168			sprintf(name, fmt, i);
>   2169			debugfs_create_file(name, 0600, test,
>   2170					    &dp->debugfs_train_set[i],
>   2171					    &fops_zynqmp_dp_preemphasis);
>   2172	
>   2173			sprintf(name, "lane%d_swing", i);
>   2174			debugfs_create_file(name, 0600, test,
>   2175					    &dp->debugfs_train_set[i],
>   2176					    &fops_zynqmp_dp_swing);
>   2177		}
>   2178	}
>   2179	
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Laurent Pinchart]

On Mon, Mar 18, 2024 at 11:06:40AM -0400, Sean Anderson wrote:
> On 3/16/24 06:14, kernel test robot wrote:
> > Hi Sean,
> > 
> > kernel test robot noticed the following build warnings:
> > 
> > [auto build test WARNING on v6.8]
> > [cannot apply to drm-misc/drm-misc-next linus/master next-20240315]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
> > base:   v6.8
> > patch link:    https://lore.kernel.org/r/20240315230916.1759060-7-sean.anderson%40linux.dev
> > patch subject: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
> > config: microblaze-allmodconfig (https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/config)
> > compiler: microblaze-linux-gcc (GCC) 13.2.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/reproduce)
> > 
> > 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>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202403161704.ACHJdSJG-lkp@intel.com/
> > 
> > All warnings (new ones prefixed by >>):
> > 
> >    drivers/gpu/drm/xlnx/zynqmp_dp.c: In function 'zynqmp_dp_bridge_debugfs_init':
> >>> drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:31: warning: 'sprintf' may write a terminating nul past the end of the destination [-Wformat-overflow=]
> >     2168 |                 sprintf(name, fmt, i);
> >          |                               ^~~
> >    drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:17: note: 'sprintf' output between 18 and 20 bytes into a destination of size 19
> >     2168 |                 sprintf(name, fmt, i);
> >          |                 ^~~~~~~~~~~~~~~~~~~~~
> 
> Not a bug, as i will be at most 4, which uses 1 digit.

The compiler can't know that. Please fix this, there's a zero warning
policy.

> > vim +/sprintf +2168 drivers/gpu/drm/xlnx/zynqmp_dp.c
> > 
> >   2136	
> >   2137	DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
> >   2138				 zynqmp_dp_rate_set, "%llu\n");
> >   2139	
> >   2140	static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
> >   2141						  struct dentry *root)
> >   2142	{
> >   2143		struct zynqmp_dp *dp = bridge_to_dp(bridge);
> >   2144		struct dentry *test;
> >   2145		int i;
> >   2146	
> >   2147		dp->test.bw_code = DP_LINK_BW_5_4;
> >   2148		dp->test.link_cnt = dp->num_lanes;
> >   2149	
> >   2150		test = debugfs_create_dir("test", root);
> >   2151	#define CREATE_FILE(name) \
> >   2152		debugfs_create_file(#name, 0600, test, dp, &fops_zynqmp_dp_##name)
> >   2153		CREATE_FILE(pattern);
> >   2154		CREATE_FILE(enhanced);
> >   2155		CREATE_FILE(downspread);
> >   2156		CREATE_FILE(active);
> >   2157		CREATE_FILE(custom);
> >   2158		CREATE_FILE(rate);
> >   2159		CREATE_FILE(lanes);
> >   2160	
> >   2161		for (i = 0; i < dp->num_lanes; i++) {
> >   2162			static const char fmt[] = "lane%d_preemphasis";
> >   2163			char name[sizeof(fmt)];
> >   2164	
> >   2165			dp->debugfs_train_set[i].dp = dp;
> >   2166			dp->debugfs_train_set[i].lane = i;
> >   2167	
> >> 2168			sprintf(name, fmt, i);
> >   2169			debugfs_create_file(name, 0600, test,
> >   2170					    &dp->debugfs_train_set[i],
> >   2171					    &fops_zynqmp_dp_preemphasis);
> >   2172	
> >   2173			sprintf(name, "lane%d_swing", i);
> >   2174			debugfs_create_file(name, 0600, test,
> >   2175					    &dp->debugfs_train_set[i],
> >   2176					    &fops_zynqmp_dp_swing);
> >   2177		}
> >   2178	}
> >   2179	

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Sean Anderson]

On 3/18/24 13:50, Laurent Pinchart wrote:
> On Mon, Mar 18, 2024 at 11:06:40AM -0400, Sean Anderson wrote:
>> On 3/16/24 06:14, kernel test robot wrote:
>> > Hi Sean,
>> > 
>> > kernel test robot noticed the following build warnings:
>> > 
>> > [auto build test WARNING on v6.8]
>> > [cannot apply to drm-misc/drm-misc-next linus/master next-20240315]
>> > [If your patch is applied to the wrong git tree, kindly drop us a note.
>> > And when submitting patch, we suggest to use '--base' as documented in
>> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
>> > 
>> > url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
>> > base:   v6.8
>> > patch link:    https://lore.kernel.org/r/20240315230916.1759060-7-sean.anderson%40linux.dev
>> > patch subject: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
>> > config: microblaze-allmodconfig (https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/config)
>> > compiler: microblaze-linux-gcc (GCC) 13.2.0
>> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/reproduce)
>> > 
>> > 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>
>> > | Closes: https://lore.kernel.org/oe-kbuild-all/202403161704.ACHJdSJG-lkp@intel.com/
>> > 
>> > All warnings (new ones prefixed by >>):
>> > 
>> >    drivers/gpu/drm/xlnx/zynqmp_dp.c: In function 'zynqmp_dp_bridge_debugfs_init':
>> >>> drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:31: warning: 'sprintf' may write a terminating nul past the end of the destination [-Wformat-overflow=]
>> >     2168 |                 sprintf(name, fmt, i);
>> >          |                               ^~~
>> >    drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:17: note: 'sprintf' output between 18 and 20 bytes into a destination of size 19
>> >     2168 |                 sprintf(name, fmt, i);
>> >          |                 ^~~~~~~~~~~~~~~~~~~~~
>> 
>> Not a bug, as i will be at most 4, which uses 1 digit.
> 
> The compiler can't know that. Please fix this, there's a zero warning
> policy.

I cannot reproduce this with GCC 13.2.0. So given that this is not a bug and I can't reproduce
it, I don't see how I can verify any fix.

--Sean

>> > vim +/sprintf +2168 drivers/gpu/drm/xlnx/zynqmp_dp.c
>> > 
>> >   2136	
>> >   2137	DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
>> >   2138				 zynqmp_dp_rate_set, "%llu\n");
>> >   2139	
>> >   2140	static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
>> >   2141						  struct dentry *root)
>> >   2142	{
>> >   2143		struct zynqmp_dp *dp = bridge_to_dp(bridge);
>> >   2144		struct dentry *test;
>> >   2145		int i;
>> >   2146	
>> >   2147		dp->test.bw_code = DP_LINK_BW_5_4;
>> >   2148		dp->test.link_cnt = dp->num_lanes;
>> >   2149	
>> >   2150		test = debugfs_create_dir("test", root);
>> >   2151	#define CREATE_FILE(name) \
>> >   2152		debugfs_create_file(#name, 0600, test, dp, &fops_zynqmp_dp_##name)
>> >   2153		CREATE_FILE(pattern);
>> >   2154		CREATE_FILE(enhanced);
>> >   2155		CREATE_FILE(downspread);
>> >   2156		CREATE_FILE(active);
>> >   2157		CREATE_FILE(custom);
>> >   2158		CREATE_FILE(rate);
>> >   2159		CREATE_FILE(lanes);
>> >   2160	
>> >   2161		for (i = 0; i < dp->num_lanes; i++) {
>> >   2162			static const char fmt[] = "lane%d_preemphasis";
>> >   2163			char name[sizeof(fmt)];
>> >   2164	
>> >   2165			dp->debugfs_train_set[i].dp = dp;
>> >   2166			dp->debugfs_train_set[i].lane = i;
>> >   2167	
>> >> 2168			sprintf(name, fmt, i);
>> >   2169			debugfs_create_file(name, 0600, test,
>> >   2170					    &dp->debugfs_train_set[i],
>> >   2171					    &fops_zynqmp_dp_preemphasis);
>> >   2172	
>> >   2173			sprintf(name, "lane%d_swing", i);
>> >   2174			debugfs_create_file(name, 0600, test,
>> >   2175					    &dp->debugfs_train_set[i],
>> >   2176					    &fops_zynqmp_dp_swing);
>> >   2177		}
>> >   2178	}
>> >   2179	
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Yujie Liu]

Hi Sean,

On Mon, Mar 18, 2024 at 03:05:57PM -0400, Sean Anderson wrote:
> On 3/18/24 13:50, Laurent Pinchart wrote:
> > On Mon, Mar 18, 2024 at 11:06:40AM -0400, Sean Anderson wrote:
> >> On 3/16/24 06:14, kernel test robot wrote:
> >> > Hi Sean,
> >> > 
> >> > kernel test robot noticed the following build warnings:
> >> > 
> >> > [auto build test WARNING on v6.8]
> >> > [cannot apply to drm-misc/drm-misc-next linus/master next-20240315]
> >> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> >> > And when submitting patch, we suggest to use '--base' as documented in
> >> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >> > 
> >> > url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
> >> > base:   v6.8
> >> > patch link:    https://lore.kernel.org/r/20240315230916.1759060-7-sean.anderson%40linux.dev
> >> > patch subject: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
> >> > config: microblaze-allmodconfig (https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/config)
> >> > compiler: microblaze-linux-gcc (GCC) 13.2.0
> >> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/reproduce)

[...]

> >> > 
> >> > 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>
> >> > | Closes: https://lore.kernel.org/oe-kbuild-all/202403161704.ACHJdSJG-lkp@intel.com/
> >> > 
> >> > All warnings (new ones prefixed by >>):
> >> > 
> >> >    drivers/gpu/drm/xlnx/zynqmp_dp.c: In function 'zynqmp_dp_bridge_debugfs_init':
> >> >>> drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:31: warning: 'sprintf' may write a terminating nul past the end of the destination [-Wformat-overflow=]
> >> >     2168 |                 sprintf(name, fmt, i);
> >> >          |                               ^~~
> >> >    drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:17: note: 'sprintf' output between 18 and 20 bytes into a destination of size 19
> >> >     2168 |                 sprintf(name, fmt, i);
> >> >          |                 ^~~~~~~~~~~~~~~~~~~~~
> >> 
> >> Not a bug, as i will be at most 4, which uses 1 digit.
> > 
> > The compiler can't know that. Please fix this, there's a zero warning
> > policy.
> 
> I cannot reproduce this with GCC 13.2.0. So given that this is not a bug and I can't reproduce
> it, I don't see how I can verify any fix.

There is a "reproduce" link in the bot's report. We followed the steps
in that link and found that the warning could be reproduced. Please
notice that this is a "W=1" warning. BTW, we also tested the v2 patch
and the warning has been fixed there. Just for your information.

$ cd linux
$ git checkout v6.8
HEAD is now at e8f897f4afef0 Linux 6.8
$ b4 am https://lore.kernel.org/r/20240315230916.1759060-7-sean.anderson@linux.dev
$ git am ./20240315_sean_anderson_drm_zynqmp_dp_misc_patches_and_debugfs_support.mbx
Applying: drm: zynqmp_dp: Downgrade log level for aux retries message
Applying: drm: zynqmp_dp: Adjust training values per-lane
Applying: drm: zynqmp_dp: Add locking
Applying: drm: zynqmp_dp: Split off several helper functions
Applying: drm: zynqmp_dp: Optionally ignore DPCD errors
Applying: drm: zynqmp_dp: Add debugfs interface for compliance testing

$ wget https://download.01.org/0day-ci/archive/20240316/202403161704.ACHJdSJG-lkp@intel.com/config
$ mkdir build_dir
$ cp config build_dir/.config
$ git clone https://github.com/intel/lkp-tests.git ~/lkp-tests
$ COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-13.2.0 ~/lkp-tests/kbuild/make.cross W=1 O=build_dir ARCH=microblaze olddefconfig
$ COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-13.2.0 ~/lkp-tests/kbuild/make.cross W=1 O=build_dir ARCH=microblaze drivers/gpu/drm/xlnx/zynqmp_dp.o
...
  CC [M]  drivers/gpu/drm/xlnx/zynqmp_dp.o
../drivers/gpu/drm/xlnx/zynqmp_dp.c: In function 'zynqmp_dp_bridge_debugfs_init':
../drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:31: warning: 'sprintf' may write a terminating nul past the end of the destination [-Wformat-overflow=]
 2168 |                 sprintf(name, fmt, i);
      |                               ^~~
../drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:17: note: 'sprintf' output between 18 and 20 bytes into a destination of size 19
 2168 |                 sprintf(name, fmt, i);
      |                 ^~~~~~~~~~~~~~~~~~~~~

Best Regards,
Yujie

> 
> --Sean
> 
> >> > vim +/sprintf +2168 drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> > 
> >> >   2136	
> >> >   2137	DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
> >> >   2138				 zynqmp_dp_rate_set, "%llu\n");
> >> >   2139	
> >> >   2140	static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
> >> >   2141						  struct dentry *root)
> >> >   2142	{
> >> >   2143		struct zynqmp_dp *dp = bridge_to_dp(bridge);
> >> >   2144		struct dentry *test;
> >> >   2145		int i;
> >> >   2146	
> >> >   2147		dp->test.bw_code = DP_LINK_BW_5_4;
> >> >   2148		dp->test.link_cnt = dp->num_lanes;
> >> >   2149	
> >> >   2150		test = debugfs_create_dir("test", root);
> >> >   2151	#define CREATE_FILE(name) \
> >> >   2152		debugfs_create_file(#name, 0600, test, dp, &fops_zynqmp_dp_##name)
> >> >   2153		CREATE_FILE(pattern);
> >> >   2154		CREATE_FILE(enhanced);
> >> >   2155		CREATE_FILE(downspread);
> >> >   2156		CREATE_FILE(active);
> >> >   2157		CREATE_FILE(custom);
> >> >   2158		CREATE_FILE(rate);
> >> >   2159		CREATE_FILE(lanes);
> >> >   2160	
> >> >   2161		for (i = 0; i < dp->num_lanes; i++) {
> >> >   2162			static const char fmt[] = "lane%d_preemphasis";
> >> >   2163			char name[sizeof(fmt)];
> >> >   2164	
> >> >   2165			dp->debugfs_train_set[i].dp = dp;
> >> >   2166			dp->debugfs_train_set[i].lane = i;
> >> >   2167	
> >> >> 2168			sprintf(name, fmt, i);
> >> >   2169			debugfs_create_file(name, 0600, test,
> >> >   2170					    &dp->debugfs_train_set[i],
> >> >   2171					    &fops_zynqmp_dp_preemphasis);
> >> >   2172	
> >> >   2173			sprintf(name, "lane%d_swing", i);
> >> >   2174			debugfs_create_file(name, 0600, test,
> >> >   2175					    &dp->debugfs_train_set[i],
> >> >   2176					    &fops_zynqmp_dp_swing);
> >> >   2177		}
> >> >   2178	}
> >> >   2179	
> > 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[kernel test robot]

Hi Sean,

kernel test robot noticed the following build errors:

[auto build test ERROR on v6.8]
[cannot apply to drm-misc/drm-misc-next linus/master next-20240315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
base:   v6.8
patch link:    https://lore.kernel.org/r/20240315230916.1759060-7-sean.anderson%40linux.dev
patch subject: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20240316/202403161837.76okceZN-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161837.76okceZN-lkp@intel.com/reproduce)

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>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403161837.76okceZN-lkp@intel.com/

All errors (new ones prefixed by >>):

   m68k-linux-ld: drivers/gpu/drm/xlnx/zynqmp_dp.o: in function `zynqmp_dp_rate_set':
>> zynqmp_dp.c:(.text+0x1a7a): undefined reference to `__udivdi3'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Sean Anderson]

On 3/16/24 06:55, kernel test robot wrote:
> Hi Sean,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on v6.8]
> [cannot apply to drm-misc/drm-misc-next linus/master next-20240315]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
> base:   v6.8
> patch link:    https://lore.kernel.org/r/20240315230916.1759060-7-sean.anderson%40linux.dev
> patch subject: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20240316/202403161837.76okceZN-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403161837.76okceZN-lkp@intel.com/reproduce)
> 
> 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>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202403161837.76okceZN-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    m68k-linux-ld: drivers/gpu/drm/xlnx/zynqmp_dp.o: in function `zynqmp_dp_rate_set':
>>> zynqmp_dp.c:(.text+0x1a7a): undefined reference to `__udivdi3'
> 

Will fix.

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Dmitry Baryshkov]

On Sat, 16 Mar 2024 at 01:09, Sean Anderson <sean.anderson@linux.dev> wrote:
>
> Add a debugfs interface for exercising the various test modes supported
> by the DisplayPort controller. This allows performing compliance
> testing, or performing signal integrity measurements on a failing link.
> At the moment, we do not support sink-driven link quality testing,
> although such support would be fairly easy to add.

Could you please point out how this is used for compliance testing? We
have been using the msm_dp_compliance tool [1].

I think it would be nice to rework our drivers towards a common
debugfs interface used for DP connectors, maybe defining generic
internal interface/helpers like Maxime is implementing for HDMI
connectors.

[1] https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tools/msm_dp_compliance.c?ref_type=heads

>
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
>
>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 591 ++++++++++++++++++++++++++++++-
>  1 file changed, 590 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 040f7b88ee51..57032186e1ca 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -18,7 +18,9 @@
>  #include <drm/drm_modes.h>
>  #include <drm/drm_of.h>
>
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/io.h>
> @@ -50,6 +52,7 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
>  #define ZYNQMP_DP_LANE_COUNT_SET                       0x4
>  #define ZYNQMP_DP_ENHANCED_FRAME_EN                    0x8
>  #define ZYNQMP_DP_TRAINING_PATTERN_SET                 0xc
> +#define ZYNQMP_DP_LINK_QUAL_PATTERN_SET                        0x10
>  #define ZYNQMP_DP_SCRAMBLING_DISABLE                   0x14
>  #define ZYNQMP_DP_DOWNSPREAD_CTL                       0x18
>  #define ZYNQMP_DP_SOFTWARE_RESET                       0x1c
> @@ -63,6 +66,9 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
>                                                          ZYNQMP_DP_SOFTWARE_RESET_STREAM3 | \
>                                                          ZYNQMP_DP_SOFTWARE_RESET_STREAM4 | \
>                                                          ZYNQMP_DP_SOFTWARE_RESET_AUX)
> +#define ZYNQMP_DP_COMP_PATTERN_80BIT_1                 0x20
> +#define ZYNQMP_DP_COMP_PATTERN_80BIT_2                 0x24
> +#define ZYNQMP_DP_COMP_PATTERN_80BIT_3                 0x28
>
>  /* Core enable registers */
>  #define ZYNQMP_DP_TRANSMITTER_ENABLE                   0x80
> @@ -206,6 +212,7 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
>  #define ZYNQMP_DP_TX_PHY_POWER_DOWN_LANE_2             BIT(2)
>  #define ZYNQMP_DP_TX_PHY_POWER_DOWN_LANE_3             BIT(3)
>  #define ZYNQMP_DP_TX_PHY_POWER_DOWN_ALL                        0xf
> +#define ZYNQMP_DP_TRANSMIT_PRBS7                       0x230
>  #define ZYNQMP_DP_PHY_PRECURSOR_LANE_0                 0x23c
>  #define ZYNQMP_DP_PHY_PRECURSOR_LANE_1                 0x240
>  #define ZYNQMP_DP_PHY_PRECURSOR_LANE_2                 0x244
> @@ -273,6 +280,69 @@ struct zynqmp_dp_config {
>         u8 bpp;
>  };
>
> +/**
> + * enum test_pattern - Test patterns for test testing
> + * TEST_VIDEO: Use regular video input
> + * TEST_SYMBOL_ERROR: Symbol error measurement pattern
> + * TEST_PRBS7: Output of the PRBS7 (x^7 + x^6 + 1) polynomial
> + * TEST_80BIT_CUSTOM: A custom 80-bit pattern
> + * TEST_CP2520: HBR2 compliance eye pattern
> + * TEST_TPS1: Link training symbol pattern TPS1 (/D10.2/)
> + * TEST_TPS2: Link training symbol pattern TPS2
> + * TEST_TPS3: Link training symbol pattern TPS3 (for HBR2)
> + */
> +enum test_pattern {
> +       TEST_VIDEO,
> +       TEST_TPS1,
> +       TEST_TPS2,
> +       TEST_TPS3,
> +       TEST_SYMBOL_ERROR,
> +       TEST_PRBS7,
> +       TEST_80BIT_CUSTOM,
> +       TEST_CP2520,
> +};
> +
> +static const char *const test_pattern_str[] = {
> +       [TEST_VIDEO] = "video",
> +       [TEST_TPS1] = "tps1",
> +       [TEST_TPS2] = "tps2",
> +       [TEST_TPS3] = "tps3",
> +       [TEST_SYMBOL_ERROR] = "symbol-error",
> +       [TEST_PRBS7] = "prbs7",
> +       [TEST_80BIT_CUSTOM] = "80bit-custom",
> +       [TEST_CP2520] = "cp2520",
> +};
> +
> +/**
> + * struct zynqmp_dp_test - Configuration for test mode
> + * @pattern: The test pattern
> + * @enhanced: Use enhanced framing
> + * @downspread: Use SSC
> + * @active: Whether test mode is active
> + * @custom: Custom pattern for %TEST_80BIT_CUSTOM
> + * @train_set: Voltage/preemphasis settings
> + * @bw_code: Bandwidth code for the link
> + * @link_cnt: Number of lanes
> + */
> +struct zynqmp_dp_test {
> +       enum test_pattern pattern;
> +       bool enhanced, downspread, active;
> +       u8 custom[10];
> +       u8 train_set[ZYNQMP_DP_MAX_LANES];
> +       u8 bw_code;
> +       u8 link_cnt;
> +};
> +
> +/**
> + * struct zynqmp_dp_train_set_priv - Private data for train_set debugfs files
> + * @dp: DisplayPort IP core structure
> + * @lane: The lane for this file
> + */
> +struct zynqmp_dp_train_set_priv {
> +       struct zynqmp_dp *dp;
> +       int lane;
> +};
> +
>  /**
>   * struct zynqmp_dp - Xilinx DisplayPort core
>   * @dev: device structure
> @@ -283,6 +353,7 @@ struct zynqmp_dp_config {
>   * @irq: irq
>   * @bridge: DRM bridge for the DP encoder
>   * @next_bridge: The downstream bridge
> + * @test: Configuration for test mode
>   * @config: IP core configuration from DTS
>   * @aux: aux channel
>   * @phy: PHY handles for DP lanes
> @@ -294,6 +365,7 @@ struct zynqmp_dp_config {
>   * @link_config: common link configuration between IP core and sink device
>   * @mode: current mode between IP core and sink device
>   * @train_set: set of training data
> + * @debugfs_train_set: Debugfs private data for @train_set
>   */
>  struct zynqmp_dp {
>         struct device *dev;
> @@ -306,6 +378,7 @@ struct zynqmp_dp {
>         struct drm_bridge bridge;
>         struct drm_bridge *next_bridge;
>
> +       struct zynqmp_dp_test test;
>         struct zynqmp_dp_config config;
>         struct drm_dp_aux aux;
>         struct phy *phy[ZYNQMP_DP_MAX_LANES];
> @@ -318,6 +391,7 @@ struct zynqmp_dp {
>         struct zynqmp_dp_link_config link_config;
>         struct zynqmp_dp_mode mode;
>         u8 train_set[ZYNQMP_DP_MAX_LANES];
> +       struct zynqmp_dp_train_set_priv debugfs_train_set[ZYNQMP_DP_MAX_LANES];
>  };
>
>  static inline struct zynqmp_dp *bridge_to_dp(struct drm_bridge *bridge)
> @@ -1599,6 +1673,510 @@ static struct edid *zynqmp_dp_bridge_get_edid(struct drm_bridge *bridge,
>         return drm_get_edid(connector, &dp->aux.ddc);
>  }
>
> +/* -----------------------------------------------------------------------------
> + * debugfs
> + */
> +
> +/**
> + * zynqmp_dp_set_test_pattern() - Configure the link for a test pattern
> + * @dp: DisplayPort IP core structure
> + */
> +static void zynqmp_dp_set_test_pattern(struct zynqmp_dp *dp,
> +                                      enum test_pattern pattern,
> +                                      u8 *const custom)
> +{
> +       bool scramble = false;
> +       u32 train_pattern = 0;
> +       u32 link_pattern = 0;
> +       u8 dpcd_train = 0;
> +       u8 dpcd_link = 0;
> +       int err;
> +
> +       switch (pattern) {
> +       case TEST_TPS1:
> +               train_pattern = 1;
> +               break;
> +       case TEST_TPS2:
> +               train_pattern = 2;
> +               break;
> +       case TEST_TPS3:
> +               train_pattern = 3;
> +               break;
> +       case TEST_SYMBOL_ERROR:
> +               scramble = true;
> +               link_pattern = DP_PHY_TEST_PATTERN_ERROR_COUNT;
> +               break;
> +       case TEST_PRBS7:
> +               /* We use a dedicated register to enable PRBS7 */
> +               dpcd_link = DP_LINK_QUAL_PATTERN_ERROR_RATE;
> +               break;
> +       case TEST_80BIT_CUSTOM: {
> +               const u8 *p = custom;
> +
> +               link_pattern = DP_LINK_QUAL_PATTERN_80BIT_CUSTOM;
> +
> +               zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_1,
> +                               (p[3] << 24) | (p[2] << 16) | (p[1] << 8) | p[0]);
> +               zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_2,
> +                               (p[7] << 24) | (p[6] << 16) | (p[5] << 8) | p[4]);
> +               zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_3,
> +                               (p[9] << 8) | p[8]);
> +               break;
> +       }
> +       case TEST_CP2520:
> +               link_pattern = DP_LINK_QUAL_PATTERN_CP2520_PAT_1;
> +               break;
> +       default:
> +               WARN_ON_ONCE(1);
> +               fallthrough;
> +       case TEST_VIDEO:
> +               scramble = true;
> +       }
> +
> +       zynqmp_dp_write(dp, ZYNQMP_DP_SCRAMBLING_DISABLE, !scramble);
> +       zynqmp_dp_write(dp, ZYNQMP_DP_TRAINING_PATTERN_SET, train_pattern);
> +       zynqmp_dp_write(dp, ZYNQMP_DP_LINK_QUAL_PATTERN_SET, link_pattern);
> +       zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMIT_PRBS7, pattern == TEST_PRBS7);
> +
> +       dpcd_link = dpcd_link ?: link_pattern;
> +       dpcd_train = train_pattern;
> +       if (!scramble)
> +               dpcd_train |= DP_LINK_SCRAMBLING_DISABLE;
> +
> +       if (dp->dpcd[DP_DPCD_REV] < 0x12) {
> +               if (pattern == TEST_CP2520)
> +                       dev_warn(dp->dev,
> +                               "can't set sink link quality pattern to CP2520 for DPCD < r1.2; error counters will be invalid\n");
> +               else
> +                       dpcd_train |= FIELD_PREP(DP_LINK_QUAL_PATTERN_11_MASK,
> +                                                dpcd_link);
> +       } else {
> +               u8 dpcd_link_lane[ZYNQMP_DP_MAX_LANES];
> +
> +               memset(dpcd_link_lane, dpcd_link, ZYNQMP_DP_MAX_LANES);
> +               err = drm_dp_dpcd_write(&dp->aux, DP_LINK_QUAL_LANE0_SET,
> +                                       dpcd_link_lane, ZYNQMP_DP_MAX_LANES);
> +               if (err < 0)
> +                       dev_err(dp->dev, "failed to set quality pattern\n");
> +       }
> +
> +       err = drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET, dpcd_train);
> +       if (err < 0)
> +               dev_err(dp->dev, "failed to set training pattern\n");
> +}
> +
> +static int zynqmp_dp_test_setup(struct zynqmp_dp *dp)
> +{
> +       return zynqmp_dp_setup(dp, dp->test.bw_code, dp->test.link_cnt,
> +                              dp->test.enhanced, dp->test.downspread, true);
> +}
> +
> +static ssize_t zynqmp_dp_pattern_read(struct file *file, char __user *user_buf,
> +                                     size_t count, loff_t *ppos)
> +{
> +       struct dentry *dentry = file->f_path.dentry;
> +       struct zynqmp_dp *dp = file->private_data;
> +       char buf[16];
> +       ssize_t ret;
> +
> +       ret = debugfs_file_get(dentry);
> +       if (unlikely(ret))
> +               return ret;
> +
> +       mutex_lock(&dp->lock);
> +       ret = snprintf(buf, sizeof(buf), "%s\n",
> +                      test_pattern_str[dp->test.pattern]);
> +       mutex_unlock(&dp->lock);
> +
> +       debugfs_file_put(dentry);
> +       return simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> +}
> +
> +static ssize_t zynqmp_dp_pattern_write(struct file *file,
> +                                      const char __user *user_buf,
> +                                      size_t count, loff_t *ppos)
> +{
> +
> +       struct dentry *dentry = file->f_path.dentry;
> +       struct zynqmp_dp *dp = file->private_data;
> +       char buf[16];
> +       ssize_t ret;
> +       int pattern;
> +
> +       ret = debugfs_file_get(dentry);
> +       if (unlikely(ret))
> +               return ret;
> +
> +       ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf,
> +                                    count);
> +       if (ret < 0)
> +               goto out;
> +       buf[ret] = '\0';
> +
> +       pattern = sysfs_match_string(test_pattern_str, buf);
> +       if (pattern < 0) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       mutex_lock(&dp->lock);
> +       dp->test.pattern = pattern;
> +       if (dp->test.active)
> +               zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
> +                                          dp->test.custom);
> +       mutex_unlock(&dp->lock);
> +
> +out:
> +       debugfs_file_put(dentry);
> +       return ret;
> +}
> +
> +static const struct file_operations fops_zynqmp_dp_pattern = {
> +       .read = zynqmp_dp_pattern_read,
> +       .write = zynqmp_dp_pattern_write,
> +       .open = simple_open,
> +       .llseek = noop_llseek,
> +};
> +
> +static int zynqmp_dp_enhanced_get(void *data, u64 *val)
> +{
> +       struct zynqmp_dp *dp = data;
> +
> +       mutex_lock(&dp->lock);
> +       *val = dp->test.enhanced;
> +       mutex_unlock(&dp->lock);
> +       return 0;
> +}
> +
> +static int zynqmp_dp_enhanced_set(void *data, u64 val)
> +{
> +       struct zynqmp_dp *dp = data;
> +       int ret = 0;
> +
> +       mutex_lock(&dp->lock);
> +       dp->test.enhanced = val;
> +       if (dp->test.active)
> +               ret = zynqmp_dp_test_setup(dp);
> +       mutex_unlock(&dp->lock);
> +
> +       return ret;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_enhanced, zynqmp_dp_enhanced_get,
> +                        zynqmp_dp_enhanced_set, "%llu\n");
> +
> +static int zynqmp_dp_downspread_get(void *data, u64 *val)
> +{
> +       struct zynqmp_dp *dp = data;
> +
> +       mutex_lock(&dp->lock);
> +       *val = dp->test.downspread;
> +       mutex_unlock(&dp->lock);
> +       return 0;
> +}
> +
> +static int zynqmp_dp_downspread_set(void *data, u64 val)
> +{
> +       struct zynqmp_dp *dp = data;
> +       int ret = 0;
> +
> +       mutex_lock(&dp->lock);
> +       dp->test.downspread = val;
> +       if (dp->test.active)
> +               ret = zynqmp_dp_test_setup(dp);
> +       mutex_unlock(&dp->lock);
> +
> +       return ret;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_downspread, zynqmp_dp_downspread_get,
> +                        zynqmp_dp_downspread_set, "%llu\n");
> +
> +static int zynqmp_dp_active_get(void *data, u64 *val)
> +{
> +       struct zynqmp_dp *dp = data;
> +
> +       mutex_lock(&dp->lock);
> +       *val = dp->test.active;
> +       mutex_unlock(&dp->lock);
> +       return 0;
> +}
> +
> +static int zynqmp_dp_active_set(void *data, u64 val)
> +{
> +       struct zynqmp_dp *dp = data;
> +       int ret = 0;
> +
> +       mutex_lock(&dp->lock);
> +       if (val) {
> +               if (val < 2) {
> +                       ret = zynqmp_dp_test_setup(dp);
> +                       if (ret)
> +                               goto out;
> +               }
> +
> +               zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
> +                                          dp->test.custom);
> +               zynqmp_dp_update_vs_emph(dp, dp->test.train_set, true);
> +               dp->test.active = true;
> +       } else {
> +               dp->test.active = false;
> +               zynqmp_dp_set_test_pattern(dp, TEST_VIDEO, NULL);
> +               zynqmp_dp_train_loop(dp);
> +       }
> +out:
> +       mutex_unlock(&dp->lock);
> +
> +       return ret;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_active, zynqmp_dp_active_get,
> +                        zynqmp_dp_active_set, "%llu\n");
> +
> +static ssize_t zynqmp_dp_custom_read(struct file *file, char __user *user_buf,
> +                                     size_t count, loff_t *ppos)
> +{
> +       struct dentry *dentry = file->f_path.dentry;
> +       struct zynqmp_dp *dp = file->private_data;
> +       ssize_t ret;
> +
> +       ret = debugfs_file_get(dentry);
> +       if (unlikely(ret))
> +               return ret;
> +
> +       mutex_lock(&dp->lock);
> +       ret = simple_read_from_buffer(user_buf, count, ppos, &dp->test.custom,
> +                                     sizeof(dp->test.custom));
> +       mutex_unlock(&dp->lock);
> +
> +       debugfs_file_put(dentry);
> +       return ret;
> +}
> +
> +static ssize_t zynqmp_dp_custom_write(struct file *file,
> +                                     const char __user *user_buf,
> +                                     size_t count, loff_t *ppos)
> +{
> +
> +       struct dentry *dentry = file->f_path.dentry;
> +       struct zynqmp_dp *dp = file->private_data;
> +       ssize_t ret;
> +       char buf[sizeof(dp->test.custom)];
> +
> +       ret = debugfs_file_get(dentry);
> +       if (unlikely(ret))
> +               return ret;
> +
> +       ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
> +       if (ret < 0)
> +               goto out;
> +
> +       mutex_lock(&dp->lock);
> +       memcpy(dp->test.custom, buf, ret);
> +       if (dp->test.active)
> +               zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
> +                                          dp->test.custom);
> +       mutex_unlock(&dp->lock);
> +
> +out:
> +       debugfs_file_put(dentry);
> +       return ret;
> +}
> +
> +static const struct file_operations fops_zynqmp_dp_custom = {
> +       .read = zynqmp_dp_custom_read,
> +       .write = zynqmp_dp_custom_write,
> +       .open = simple_open,
> +       .llseek = noop_llseek,
> +};
> +
> +static int zynqmp_dp_swing_get(void *data, u64 *val)
> +{
> +       struct zynqmp_dp_train_set_priv *priv = data;
> +       struct zynqmp_dp *dp = priv->dp;
> +
> +       mutex_lock(&dp->lock);
> +       *val = dp->test.train_set[priv->lane] & DP_TRAIN_VOLTAGE_SWING_MASK;
> +       mutex_unlock(&dp->lock);
> +       return 0;
> +}
> +
> +static int zynqmp_dp_swing_set(void *data, u64 val)
> +{
> +       struct zynqmp_dp_train_set_priv *priv = data;
> +       struct zynqmp_dp *dp = priv->dp;
> +       u8 *train_set = &dp->test.train_set[priv->lane];
> +       int ret = 0;
> +
> +       if (val > 3)
> +               return -EINVAL;
> +
> +       mutex_lock(&dp->lock);
> +       *train_set &= ~(DP_TRAIN_MAX_SWING_REACHED |
> +                       DP_TRAIN_VOLTAGE_SWING_MASK);
> +       *train_set |= val;
> +       if (val == 3)
> +               *train_set |= DP_TRAIN_MAX_SWING_REACHED;
> +
> +       if (dp->test.active)
> +               zynqmp_dp_update_vs_emph(dp, dp->test.train_set, true);
> +       mutex_unlock(&dp->lock);
> +
> +       return ret;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_swing, zynqmp_dp_swing_get,
> +                        zynqmp_dp_swing_set, "%llu\n");
> +
> +static int zynqmp_dp_preemphasis_get(void *data, u64 *val)
> +{
> +       struct zynqmp_dp_train_set_priv *priv = data;
> +       struct zynqmp_dp *dp = priv->dp;
> +
> +       mutex_lock(&dp->lock);
> +       *val = FIELD_GET(DP_TRAIN_PRE_EMPHASIS_MASK,
> +                        dp->test.train_set[priv->lane]);
> +       mutex_unlock(&dp->lock);
> +       return 0;
> +}
> +
> +static int zynqmp_dp_preemphasis_set(void *data, u64 val)
> +{
> +       struct zynqmp_dp_train_set_priv *priv = data;
> +       struct zynqmp_dp *dp = priv->dp;
> +       u8 *train_set = &dp->test.train_set[priv->lane];
> +       int ret = 0;
> +
> +       if (val > 2)
> +               return -EINVAL;
> +
> +       mutex_lock(&dp->lock);
> +       *train_set &= ~(DP_TRAIN_MAX_PRE_EMPHASIS_REACHED |
> +                       DP_TRAIN_PRE_EMPHASIS_MASK);
> +       *train_set |= val;
> +       if (val == 2)
> +               *train_set |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
> +
> +       if (dp->test.active)
> +               zynqmp_dp_update_vs_emph(dp, dp->test.train_set, true);
> +       mutex_unlock(&dp->lock);
> +
> +       return ret;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_preemphasis, zynqmp_dp_preemphasis_get,
> +                        zynqmp_dp_preemphasis_set, "%llu\n");
> +
> +static int zynqmp_dp_lanes_get(void *data, u64 *val)
> +{
> +       struct zynqmp_dp *dp = data;
> +
> +       mutex_lock(&dp->lock);
> +       *val = dp->test.link_cnt;
> +       mutex_unlock(&dp->lock);
> +       return 0;
> +}
> +
> +static int zynqmp_dp_lanes_set(void *data, u64 val)
> +{
> +       struct zynqmp_dp *dp = data;
> +       int ret = 0;
> +
> +       if (val > ZYNQMP_DP_MAX_LANES)
> +               return -EINVAL;
> +
> +       mutex_lock(&dp->lock);
> +       if (val > dp->num_lanes) {
> +               ret = -EINVAL;
> +       } else {
> +               dp->test.link_cnt = val;
> +               if (dp->test.active)
> +                       ret = zynqmp_dp_test_setup(dp);
> +       }
> +       mutex_unlock(&dp->lock);
> +
> +       return ret;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_lanes, zynqmp_dp_lanes_get,
> +                        zynqmp_dp_lanes_set, "%llu\n");
> +
> +static int zynqmp_dp_rate_get(void *data, u64 *val)
> +{
> +       struct zynqmp_dp *dp = data;
> +
> +       mutex_lock(&dp->lock);
> +       *val = drm_dp_bw_code_to_link_rate(dp->test.bw_code) * 10000;
> +       mutex_unlock(&dp->lock);
> +       return 0;
> +}
> +
> +static int zynqmp_dp_rate_set(void *data, u64 val)
> +{
> +       struct zynqmp_dp *dp = data;
> +       u8 bw_code = drm_dp_link_rate_to_bw_code(val / 10000);
> +       int link_rate = drm_dp_bw_code_to_link_rate(bw_code);
> +       int ret = 0;
> +
> +       if (val / 10000 != link_rate)
> +               return -EINVAL;
> +
> +       if (bw_code != DP_LINK_BW_1_62 && bw_code != DP_LINK_BW_2_7 &&
> +           bw_code != DP_LINK_BW_5_4)
> +               return -EINVAL;
> +
> +       mutex_lock(&dp->lock);
> +       dp->test.bw_code = bw_code;
> +       if (dp->test.active)
> +               ret = zynqmp_dp_test_setup(dp);
> +       mutex_unlock(&dp->lock);
> +
> +       return ret;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
> +                        zynqmp_dp_rate_set, "%llu\n");
> +
> +static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
> +                                         struct dentry *root)
> +{
> +       struct zynqmp_dp *dp = bridge_to_dp(bridge);
> +       struct dentry *test;
> +       int i;
> +
> +       dp->test.bw_code = DP_LINK_BW_5_4;
> +       dp->test.link_cnt = dp->num_lanes;
> +
> +       test = debugfs_create_dir("test", root);
> +#define CREATE_FILE(name) \
> +       debugfs_create_file(#name, 0600, test, dp, &fops_zynqmp_dp_##name)
> +       CREATE_FILE(pattern);
> +       CREATE_FILE(enhanced);
> +       CREATE_FILE(downspread);
> +       CREATE_FILE(active);
> +       CREATE_FILE(custom);
> +       CREATE_FILE(rate);
> +       CREATE_FILE(lanes);
> +
> +       for (i = 0; i < dp->num_lanes; i++) {
> +               static const char fmt[] = "lane%d_preemphasis";
> +               char name[sizeof(fmt)];
> +
> +               dp->debugfs_train_set[i].dp = dp;
> +               dp->debugfs_train_set[i].lane = i;
> +
> +               sprintf(name, fmt, i);
> +               debugfs_create_file(name, 0600, test,
> +                                   &dp->debugfs_train_set[i],
> +                                   &fops_zynqmp_dp_preemphasis);
> +
> +               sprintf(name, "lane%d_swing", i);
> +               debugfs_create_file(name, 0600, test,
> +                                   &dp->debugfs_train_set[i],
> +                                   &fops_zynqmp_dp_swing);
> +       }
> +}
> +
>  static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
>         .attach = zynqmp_dp_bridge_attach,
>         .detach = zynqmp_dp_bridge_detach,
> @@ -1611,6 +2189,7 @@ static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
>         .atomic_check = zynqmp_dp_bridge_atomic_check,
>         .detect = zynqmp_dp_bridge_detect,
>         .get_edid = zynqmp_dp_bridge_get_edid,
> +       .debugfs_init = zynqmp_dp_bridge_debugfs_init,
>  };
>
>  /* -----------------------------------------------------------------------------
> @@ -1645,6 +2224,9 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
>                                             hpd_work.work);
>         enum drm_connector_status status;
>
> +       if (dp->test.active)
> +               return;
> +
>         status = zynqmp_dp_bridge_detect(&dp->bridge);
>         drm_bridge_hpd_notify(&dp->bridge, status);
>  }
> @@ -1666,7 +2248,14 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
>                 if (status[4] & DP_LINK_STATUS_UPDATED ||
>                     !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
>                     !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
> -                       zynqmp_dp_train_loop(dp);
> +                       if (dp->test.active) {
> +                               dev_dbg(dp->dev, "Ignoring HPD IRQ in test mode\n");
> +                       } else {
> +                               dev_dbg(dp->dev,
> +                                       "Retraining due to HPD IRQ (status is [%*ph])\n",
> +                                       (int)sizeof(status), status);
> +                               zynqmp_dp_train_loop(dp);
> +                       }
>                 }
>         }
>         mutex_unlock(&dp->lock);
> --
> 2.35.1.1320.gc452695387.dirty
>


-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Sean Anderson]

On 3/16/24 13:56, Dmitry Baryshkov wrote:
> On Sat, 16 Mar 2024 at 01:09, Sean Anderson <sean.anderson@linux.dev> wrote:
>>
>> Add a debugfs interface for exercising the various test modes supported
>> by the DisplayPort controller. This allows performing compliance
>> testing, or performing signal integrity measurements on a failing link.
>> At the moment, we do not support sink-driven link quality testing,
>> although such support would be fairly easy to add.
> 
> Could you please point out how this is used for compliance testing? We
> have been using the msm_dp_compliance tool [1].

Here's some quick documentation I wrote up. This probably could be put
under Documentation for v2.

The following files in /sys/kernel/debug/dri/1/DP-1/test/ control the
DisplayPort test modes:

active:
  Writing a 1 to this file will activate test mode, and writing a 0 will
  deactivate test mode. Writing a 1 or 0 when the test mode is already
  active/inactive will reactivate/re-deactivate test mode. When test mode
  is inactive, changes made to other files will have no effect. When
  test mode is active, changes made to other files will apply instantly.
  Additionally, hotplug events (as removing the cable or if the monitor
  requests link retraining) are ignored.
custom:
  Custom test pattern value
downspread:
  Enable/disable clock downspreading (spread-spectrum clocking) by
  writing 1/0
enhanced:
  Enable/disable enhanced framing
lane0_preemphasis:
  Preemphasis from 0 (lowest) to 2 (most) for lane 0
lane0_swing:
  Voltage swing from 0 (lowest) to 3 (most) for lane 0
lane1_preemphasis:
  Preemphasis from 0 (lowest) to 2 (most) for lane 1
lane1_swing:
  Voltage swing from 0 (lowest) to 3 (most) for lane 1
lanes:
  Number of lanes to use (1 or 2)
pattern:
  Test pattern. May be one of:
    - video: Use regular video input
    - symbol-error: Symbol error measurement pattern
    - prbs7: Output of the PRBS7 (x^7 + x^6 + 1) polynomial
    - 80bit-custom: A custom 80-bit pattern
    - cp2520: HBR2 compliance eye pattern
    - tps1: Link training symbol pattern TPS1 (/D10.2/)
    - tps2: Link training symbol pattern TPS2
    - tps3: Link training symbol pattern TPS3 (for HBR2)
rate:
  Rate in hertz. One of
    - 5400000000: HBR2
    - 2700000000: HBR
    - 1620000000: RBR

You can dump the displayport test settings with the following command:

for prop in /sys/kernel/debug/dri/1/DP-1/test/*; do
	printf '%-20s ' ${prop##*/}
	if [ ${prop##*/} = custom ]; then
		hexdump -C $prop | head -1
	else
		cat $prop
	fi
done

The output could look something like

active               1
custom               00000000  00 00 00 00 00 00 00 00  00 00                    |..........|
downspread           0
enhanced             1
lane0_preemphasis    0
lane0_swing          3
lane1_preemphasis    0
lane1_swing          3
lanes                2
pattern              prbs7
rate                 1620000000

The recommended test procedure is to connect the board to a monitor,
configure test mode, activate test mode, and then disconnect the cable
and connect it to your test equipment of choice. For example, one
sequence of commands could be:

echo 1 > /sys/kernel/debug/dri/1/DP-1/test/enhanced
echo tps1 > /sys/kernel/debug/dri/1/DP-1/test/pattern
echo 1620000000 > /sys/kernel/debug/dri/1/DP-1/test/rate
echo 1 > /sys/kernel/debug/dri/1/DP-1/test/active

at which point the cable could be disconnected from the monitor. When
the cable is disconnected there will be several errors while changing
the settings. This is expected.

> I think it would be nice to rework our drivers towards a common
> debugfs interface used for DP connectors, maybe defining generic
> internal interface/helpers like Maxime is implementing for HDMI
> connectors.
> 
> [1] https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tools/msm_dp_compliance.c?ref_type=heads

I was definitely inspired by the msm, intel, and amd approaches.
However, those debugfs implementations seem to be oriented towards
DisplayPort text fixtures which emulate DPRXs. In particular, both the
intel and msm debugfs interfaces provide no method for configuring test
parameters in userspace. As test fixtures supporting DPCD can run into
the thousands of dollars, I think it is more economical to support
userspace-driven testing. I was particularly inspired by the AMD
approach:

	/* Usage: set DP physical test pattern using debugfs with normal DP
	 * panel. Then plug out DP panel and connect a scope to measure
	 * For normal video mode and test pattern generated from CRCT,
	 * they are visibile to user. So do not disable HPD.
	 * Video Mode is also set to clear the test pattern, so enable HPD
	 * because it might have been disabled after a test pattern was set.
	 * AUX depends on HPD * sequence dependent, do not move!
	 */

But I chose to always disable HPD events and ignore DPCD
errors in test mode. I think this is pretty convenient, since you can
run the same commands regardless of whether you have a monitor attached.
Although the initial setup does need a monitor (which is likely since
not everything gets set up by activating test mode; definitely fixable
but I didn't need it).

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Laurent Pinchart]

On Sat, Mar 16, 2024 at 07:56:52PM +0200, Dmitry Baryshkov wrote:
> On Sat, 16 Mar 2024 at 01:09, Sean Anderson <sean.anderson@linux.dev> wrote:
> >
> > Add a debugfs interface for exercising the various test modes supported
> > by the DisplayPort controller. This allows performing compliance
> > testing, or performing signal integrity measurements on a failing link.
> > At the moment, we do not support sink-driven link quality testing,
> > although such support would be fairly easy to add.
> 
> Could you please point out how this is used for compliance testing? We
> have been using the msm_dp_compliance tool [1].
> 
> I think it would be nice to rework our drivers towards a common
> debugfs interface used for DP connectors, maybe defining generic
> internal interface/helpers like Maxime is implementing for HDMI
> connectors.

This would be really nice :-)

> [1] https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tools/msm_dp_compliance.c?ref_type=heads
> 
> >
> > Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> > ---
> >
> >  drivers/gpu/drm/xlnx/zynqmp_dp.c | 591 ++++++++++++++++++++++++++++++-
> >  1 file changed, 590 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > index 040f7b88ee51..57032186e1ca 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > @@ -18,7 +18,9 @@
> >  #include <drm/drm_modes.h>
> >  #include <drm/drm_of.h>
> >
> > +#include <linux/bitfield.h>
> >  #include <linux/clk.h>
> > +#include <linux/debugfs.h>
> >  #include <linux/delay.h>
> >  #include <linux/device.h>
> >  #include <linux/io.h>
> > @@ -50,6 +52,7 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
> >  #define ZYNQMP_DP_LANE_COUNT_SET                       0x4
> >  #define ZYNQMP_DP_ENHANCED_FRAME_EN                    0x8
> >  #define ZYNQMP_DP_TRAINING_PATTERN_SET                 0xc
> > +#define ZYNQMP_DP_LINK_QUAL_PATTERN_SET                        0x10
> >  #define ZYNQMP_DP_SCRAMBLING_DISABLE                   0x14
> >  #define ZYNQMP_DP_DOWNSPREAD_CTL                       0x18
> >  #define ZYNQMP_DP_SOFTWARE_RESET                       0x1c
> > @@ -63,6 +66,9 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
> >                                                          ZYNQMP_DP_SOFTWARE_RESET_STREAM3 | \
> >                                                          ZYNQMP_DP_SOFTWARE_RESET_STREAM4 | \
> >                                                          ZYNQMP_DP_SOFTWARE_RESET_AUX)
> > +#define ZYNQMP_DP_COMP_PATTERN_80BIT_1                 0x20
> > +#define ZYNQMP_DP_COMP_PATTERN_80BIT_2                 0x24
> > +#define ZYNQMP_DP_COMP_PATTERN_80BIT_3                 0x28
> >
> >  /* Core enable registers */
> >  #define ZYNQMP_DP_TRANSMITTER_ENABLE                   0x80
> > @@ -206,6 +212,7 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
> >  #define ZYNQMP_DP_TX_PHY_POWER_DOWN_LANE_2             BIT(2)
> >  #define ZYNQMP_DP_TX_PHY_POWER_DOWN_LANE_3             BIT(3)
> >  #define ZYNQMP_DP_TX_PHY_POWER_DOWN_ALL                        0xf
> > +#define ZYNQMP_DP_TRANSMIT_PRBS7                       0x230
> >  #define ZYNQMP_DP_PHY_PRECURSOR_LANE_0                 0x23c
> >  #define ZYNQMP_DP_PHY_PRECURSOR_LANE_1                 0x240
> >  #define ZYNQMP_DP_PHY_PRECURSOR_LANE_2                 0x244
> > @@ -273,6 +280,69 @@ struct zynqmp_dp_config {
> >         u8 bpp;
> >  };
> >
> > +/**
> > + * enum test_pattern - Test patterns for test testing
> > + * TEST_VIDEO: Use regular video input
> > + * TEST_SYMBOL_ERROR: Symbol error measurement pattern
> > + * TEST_PRBS7: Output of the PRBS7 (x^7 + x^6 + 1) polynomial
> > + * TEST_80BIT_CUSTOM: A custom 80-bit pattern
> > + * TEST_CP2520: HBR2 compliance eye pattern
> > + * TEST_TPS1: Link training symbol pattern TPS1 (/D10.2/)
> > + * TEST_TPS2: Link training symbol pattern TPS2
> > + * TEST_TPS3: Link training symbol pattern TPS3 (for HBR2)
> > + */
> > +enum test_pattern {
> > +       TEST_VIDEO,
> > +       TEST_TPS1,
> > +       TEST_TPS2,
> > +       TEST_TPS3,
> > +       TEST_SYMBOL_ERROR,
> > +       TEST_PRBS7,
> > +       TEST_80BIT_CUSTOM,
> > +       TEST_CP2520,
> > +};
> > +
> > +static const char *const test_pattern_str[] = {
> > +       [TEST_VIDEO] = "video",
> > +       [TEST_TPS1] = "tps1",
> > +       [TEST_TPS2] = "tps2",
> > +       [TEST_TPS3] = "tps3",
> > +       [TEST_SYMBOL_ERROR] = "symbol-error",
> > +       [TEST_PRBS7] = "prbs7",
> > +       [TEST_80BIT_CUSTOM] = "80bit-custom",
> > +       [TEST_CP2520] = "cp2520",
> > +};
> > +
> > +/**
> > + * struct zynqmp_dp_test - Configuration for test mode
> > + * @pattern: The test pattern
> > + * @enhanced: Use enhanced framing
> > + * @downspread: Use SSC
> > + * @active: Whether test mode is active
> > + * @custom: Custom pattern for %TEST_80BIT_CUSTOM
> > + * @train_set: Voltage/preemphasis settings
> > + * @bw_code: Bandwidth code for the link
> > + * @link_cnt: Number of lanes
> > + */
> > +struct zynqmp_dp_test {
> > +       enum test_pattern pattern;
> > +       bool enhanced, downspread, active;
> > +       u8 custom[10];
> > +       u8 train_set[ZYNQMP_DP_MAX_LANES];
> > +       u8 bw_code;
> > +       u8 link_cnt;
> > +};
> > +
> > +/**
> > + * struct zynqmp_dp_train_set_priv - Private data for train_set debugfs files
> > + * @dp: DisplayPort IP core structure
> > + * @lane: The lane for this file
> > + */
> > +struct zynqmp_dp_train_set_priv {
> > +       struct zynqmp_dp *dp;
> > +       int lane;
> > +};
> > +
> >  /**
> >   * struct zynqmp_dp - Xilinx DisplayPort core
> >   * @dev: device structure
> > @@ -283,6 +353,7 @@ struct zynqmp_dp_config {
> >   * @irq: irq
> >   * @bridge: DRM bridge for the DP encoder
> >   * @next_bridge: The downstream bridge
> > + * @test: Configuration for test mode
> >   * @config: IP core configuration from DTS
> >   * @aux: aux channel
> >   * @phy: PHY handles for DP lanes
> > @@ -294,6 +365,7 @@ struct zynqmp_dp_config {
> >   * @link_config: common link configuration between IP core and sink device
> >   * @mode: current mode between IP core and sink device
> >   * @train_set: set of training data
> > + * @debugfs_train_set: Debugfs private data for @train_set
> >   */
> >  struct zynqmp_dp {
> >         struct device *dev;
> > @@ -306,6 +378,7 @@ struct zynqmp_dp {
> >         struct drm_bridge bridge;
> >         struct drm_bridge *next_bridge;
> >
> > +       struct zynqmp_dp_test test;
> >         struct zynqmp_dp_config config;
> >         struct drm_dp_aux aux;
> >         struct phy *phy[ZYNQMP_DP_MAX_LANES];
> > @@ -318,6 +391,7 @@ struct zynqmp_dp {
> >         struct zynqmp_dp_link_config link_config;
> >         struct zynqmp_dp_mode mode;
> >         u8 train_set[ZYNQMP_DP_MAX_LANES];
> > +       struct zynqmp_dp_train_set_priv debugfs_train_set[ZYNQMP_DP_MAX_LANES];
> >  };
> >
> >  static inline struct zynqmp_dp *bridge_to_dp(struct drm_bridge *bridge)
> > @@ -1599,6 +1673,510 @@ static struct edid *zynqmp_dp_bridge_get_edid(struct drm_bridge *bridge,
> >         return drm_get_edid(connector, &dp->aux.ddc);
> >  }
> >
> > +/* -----------------------------------------------------------------------------
> > + * debugfs
> > + */
> > +
> > +/**
> > + * zynqmp_dp_set_test_pattern() - Configure the link for a test pattern
> > + * @dp: DisplayPort IP core structure
> > + */
> > +static void zynqmp_dp_set_test_pattern(struct zynqmp_dp *dp,
> > +                                      enum test_pattern pattern,
> > +                                      u8 *const custom)
> > +{
> > +       bool scramble = false;
> > +       u32 train_pattern = 0;
> > +       u32 link_pattern = 0;
> > +       u8 dpcd_train = 0;
> > +       u8 dpcd_link = 0;
> > +       int err;
> > +
> > +       switch (pattern) {
> > +       case TEST_TPS1:
> > +               train_pattern = 1;
> > +               break;
> > +       case TEST_TPS2:
> > +               train_pattern = 2;
> > +               break;
> > +       case TEST_TPS3:
> > +               train_pattern = 3;
> > +               break;
> > +       case TEST_SYMBOL_ERROR:
> > +               scramble = true;
> > +               link_pattern = DP_PHY_TEST_PATTERN_ERROR_COUNT;
> > +               break;
> > +       case TEST_PRBS7:
> > +               /* We use a dedicated register to enable PRBS7 */
> > +               dpcd_link = DP_LINK_QUAL_PATTERN_ERROR_RATE;
> > +               break;
> > +       case TEST_80BIT_CUSTOM: {
> > +               const u8 *p = custom;
> > +
> > +               link_pattern = DP_LINK_QUAL_PATTERN_80BIT_CUSTOM;
> > +
> > +               zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_1,
> > +                               (p[3] << 24) | (p[2] << 16) | (p[1] << 8) | p[0]);
> > +               zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_2,
> > +                               (p[7] << 24) | (p[6] << 16) | (p[5] << 8) | p[4]);
> > +               zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_3,
> > +                               (p[9] << 8) | p[8]);
> > +               break;
> > +       }
> > +       case TEST_CP2520:
> > +               link_pattern = DP_LINK_QUAL_PATTERN_CP2520_PAT_1;
> > +               break;
> > +       default:
> > +               WARN_ON_ONCE(1);
> > +               fallthrough;
> > +       case TEST_VIDEO:
> > +               scramble = true;
> > +       }
> > +
> > +       zynqmp_dp_write(dp, ZYNQMP_DP_SCRAMBLING_DISABLE, !scramble);
> > +       zynqmp_dp_write(dp, ZYNQMP_DP_TRAINING_PATTERN_SET, train_pattern);
> > +       zynqmp_dp_write(dp, ZYNQMP_DP_LINK_QUAL_PATTERN_SET, link_pattern);
> > +       zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMIT_PRBS7, pattern == TEST_PRBS7);
> > +
> > +       dpcd_link = dpcd_link ?: link_pattern;
> > +       dpcd_train = train_pattern;
> > +       if (!scramble)
> > +               dpcd_train |= DP_LINK_SCRAMBLING_DISABLE;
> > +
> > +       if (dp->dpcd[DP_DPCD_REV] < 0x12) {
> > +               if (pattern == TEST_CP2520)
> > +                       dev_warn(dp->dev,
> > +                               "can't set sink link quality pattern to CP2520 for DPCD < r1.2; error counters will be invalid\n");
> > +               else
> > +                       dpcd_train |= FIELD_PREP(DP_LINK_QUAL_PATTERN_11_MASK,
> > +                                                dpcd_link);
> > +       } else {
> > +               u8 dpcd_link_lane[ZYNQMP_DP_MAX_LANES];
> > +
> > +               memset(dpcd_link_lane, dpcd_link, ZYNQMP_DP_MAX_LANES);
> > +               err = drm_dp_dpcd_write(&dp->aux, DP_LINK_QUAL_LANE0_SET,
> > +                                       dpcd_link_lane, ZYNQMP_DP_MAX_LANES);
> > +               if (err < 0)
> > +                       dev_err(dp->dev, "failed to set quality pattern\n");
> > +       }
> > +
> > +       err = drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET, dpcd_train);
> > +       if (err < 0)
> > +               dev_err(dp->dev, "failed to set training pattern\n");
> > +}
> > +
> > +static int zynqmp_dp_test_setup(struct zynqmp_dp *dp)
> > +{
> > +       return zynqmp_dp_setup(dp, dp->test.bw_code, dp->test.link_cnt,
> > +                              dp->test.enhanced, dp->test.downspread, true);
> > +}
> > +
> > +static ssize_t zynqmp_dp_pattern_read(struct file *file, char __user *user_buf,
> > +                                     size_t count, loff_t *ppos)
> > +{
> > +       struct dentry *dentry = file->f_path.dentry;
> > +       struct zynqmp_dp *dp = file->private_data;
> > +       char buf[16];
> > +       ssize_t ret;
> > +
> > +       ret = debugfs_file_get(dentry);
> > +       if (unlikely(ret))
> > +               return ret;
> > +
> > +       mutex_lock(&dp->lock);
> > +       ret = snprintf(buf, sizeof(buf), "%s\n",
> > +                      test_pattern_str[dp->test.pattern]);
> > +       mutex_unlock(&dp->lock);
> > +
> > +       debugfs_file_put(dentry);
> > +       return simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> > +}
> > +
> > +static ssize_t zynqmp_dp_pattern_write(struct file *file,
> > +                                      const char __user *user_buf,
> > +                                      size_t count, loff_t *ppos)
> > +{
> > +
> > +       struct dentry *dentry = file->f_path.dentry;
> > +       struct zynqmp_dp *dp = file->private_data;
> > +       char buf[16];
> > +       ssize_t ret;
> > +       int pattern;
> > +
> > +       ret = debugfs_file_get(dentry);
> > +       if (unlikely(ret))
> > +               return ret;
> > +
> > +       ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf,
> > +                                    count);
> > +       if (ret < 0)
> > +               goto out;
> > +       buf[ret] = '\0';
> > +
> > +       pattern = sysfs_match_string(test_pattern_str, buf);
> > +       if (pattern < 0) {
> > +               ret = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       mutex_lock(&dp->lock);
> > +       dp->test.pattern = pattern;
> > +       if (dp->test.active)
> > +               zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
> > +                                          dp->test.custom);
> > +       mutex_unlock(&dp->lock);
> > +
> > +out:
> > +       debugfs_file_put(dentry);
> > +       return ret;
> > +}
> > +
> > +static const struct file_operations fops_zynqmp_dp_pattern = {
> > +       .read = zynqmp_dp_pattern_read,
> > +       .write = zynqmp_dp_pattern_write,
> > +       .open = simple_open,
> > +       .llseek = noop_llseek,
> > +};
> > +
> > +static int zynqmp_dp_enhanced_get(void *data, u64 *val)
> > +{
> > +       struct zynqmp_dp *dp = data;
> > +
> > +       mutex_lock(&dp->lock);
> > +       *val = dp->test.enhanced;
> > +       mutex_unlock(&dp->lock);
> > +       return 0;
> > +}
> > +
> > +static int zynqmp_dp_enhanced_set(void *data, u64 val)
> > +{
> > +       struct zynqmp_dp *dp = data;
> > +       int ret = 0;
> > +
> > +       mutex_lock(&dp->lock);
> > +       dp->test.enhanced = val;
> > +       if (dp->test.active)
> > +               ret = zynqmp_dp_test_setup(dp);
> > +       mutex_unlock(&dp->lock);
> > +
> > +       return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_enhanced, zynqmp_dp_enhanced_get,
> > +                        zynqmp_dp_enhanced_set, "%llu\n");
> > +
> > +static int zynqmp_dp_downspread_get(void *data, u64 *val)
> > +{
> > +       struct zynqmp_dp *dp = data;
> > +
> > +       mutex_lock(&dp->lock);
> > +       *val = dp->test.downspread;
> > +       mutex_unlock(&dp->lock);
> > +       return 0;
> > +}
> > +
> > +static int zynqmp_dp_downspread_set(void *data, u64 val)
> > +{
> > +       struct zynqmp_dp *dp = data;
> > +       int ret = 0;
> > +
> > +       mutex_lock(&dp->lock);
> > +       dp->test.downspread = val;
> > +       if (dp->test.active)
> > +               ret = zynqmp_dp_test_setup(dp);
> > +       mutex_unlock(&dp->lock);
> > +
> > +       return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_downspread, zynqmp_dp_downspread_get,
> > +                        zynqmp_dp_downspread_set, "%llu\n");
> > +
> > +static int zynqmp_dp_active_get(void *data, u64 *val)
> > +{
> > +       struct zynqmp_dp *dp = data;
> > +
> > +       mutex_lock(&dp->lock);
> > +       *val = dp->test.active;
> > +       mutex_unlock(&dp->lock);
> > +       return 0;
> > +}
> > +
> > +static int zynqmp_dp_active_set(void *data, u64 val)
> > +{
> > +       struct zynqmp_dp *dp = data;
> > +       int ret = 0;
> > +
> > +       mutex_lock(&dp->lock);
> > +       if (val) {
> > +               if (val < 2) {
> > +                       ret = zynqmp_dp_test_setup(dp);
> > +                       if (ret)
> > +                               goto out;
> > +               }
> > +
> > +               zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
> > +                                          dp->test.custom);
> > +               zynqmp_dp_update_vs_emph(dp, dp->test.train_set, true);
> > +               dp->test.active = true;
> > +       } else {
> > +               dp->test.active = false;
> > +               zynqmp_dp_set_test_pattern(dp, TEST_VIDEO, NULL);
> > +               zynqmp_dp_train_loop(dp);
> > +       }
> > +out:
> > +       mutex_unlock(&dp->lock);
> > +
> > +       return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_active, zynqmp_dp_active_get,
> > +                        zynqmp_dp_active_set, "%llu\n");
> > +
> > +static ssize_t zynqmp_dp_custom_read(struct file *file, char __user *user_buf,
> > +                                     size_t count, loff_t *ppos)
> > +{
> > +       struct dentry *dentry = file->f_path.dentry;
> > +       struct zynqmp_dp *dp = file->private_data;
> > +       ssize_t ret;
> > +
> > +       ret = debugfs_file_get(dentry);
> > +       if (unlikely(ret))
> > +               return ret;
> > +
> > +       mutex_lock(&dp->lock);
> > +       ret = simple_read_from_buffer(user_buf, count, ppos, &dp->test.custom,
> > +                                     sizeof(dp->test.custom));
> > +       mutex_unlock(&dp->lock);
> > +
> > +       debugfs_file_put(dentry);
> > +       return ret;
> > +}
> > +
> > +static ssize_t zynqmp_dp_custom_write(struct file *file,
> > +                                     const char __user *user_buf,
> > +                                     size_t count, loff_t *ppos)
> > +{
> > +
> > +       struct dentry *dentry = file->f_path.dentry;
> > +       struct zynqmp_dp *dp = file->private_data;
> > +       ssize_t ret;
> > +       char buf[sizeof(dp->test.custom)];
> > +
> > +       ret = debugfs_file_get(dentry);
> > +       if (unlikely(ret))
> > +               return ret;
> > +
> > +       ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
> > +       if (ret < 0)
> > +               goto out;
> > +
> > +       mutex_lock(&dp->lock);
> > +       memcpy(dp->test.custom, buf, ret);
> > +       if (dp->test.active)
> > +               zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
> > +                                          dp->test.custom);
> > +       mutex_unlock(&dp->lock);
> > +
> > +out:
> > +       debugfs_file_put(dentry);
> > +       return ret;
> > +}
> > +
> > +static const struct file_operations fops_zynqmp_dp_custom = {
> > +       .read = zynqmp_dp_custom_read,
> > +       .write = zynqmp_dp_custom_write,
> > +       .open = simple_open,
> > +       .llseek = noop_llseek,
> > +};
> > +
> > +static int zynqmp_dp_swing_get(void *data, u64 *val)
> > +{
> > +       struct zynqmp_dp_train_set_priv *priv = data;
> > +       struct zynqmp_dp *dp = priv->dp;
> > +
> > +       mutex_lock(&dp->lock);
> > +       *val = dp->test.train_set[priv->lane] & DP_TRAIN_VOLTAGE_SWING_MASK;
> > +       mutex_unlock(&dp->lock);
> > +       return 0;
> > +}
> > +
> > +static int zynqmp_dp_swing_set(void *data, u64 val)
> > +{
> > +       struct zynqmp_dp_train_set_priv *priv = data;
> > +       struct zynqmp_dp *dp = priv->dp;
> > +       u8 *train_set = &dp->test.train_set[priv->lane];
> > +       int ret = 0;
> > +
> > +       if (val > 3)
> > +               return -EINVAL;
> > +
> > +       mutex_lock(&dp->lock);
> > +       *train_set &= ~(DP_TRAIN_MAX_SWING_REACHED |
> > +                       DP_TRAIN_VOLTAGE_SWING_MASK);
> > +       *train_set |= val;
> > +       if (val == 3)
> > +               *train_set |= DP_TRAIN_MAX_SWING_REACHED;
> > +
> > +       if (dp->test.active)
> > +               zynqmp_dp_update_vs_emph(dp, dp->test.train_set, true);
> > +       mutex_unlock(&dp->lock);
> > +
> > +       return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_swing, zynqmp_dp_swing_get,
> > +                        zynqmp_dp_swing_set, "%llu\n");
> > +
> > +static int zynqmp_dp_preemphasis_get(void *data, u64 *val)
> > +{
> > +       struct zynqmp_dp_train_set_priv *priv = data;
> > +       struct zynqmp_dp *dp = priv->dp;
> > +
> > +       mutex_lock(&dp->lock);
> > +       *val = FIELD_GET(DP_TRAIN_PRE_EMPHASIS_MASK,
> > +                        dp->test.train_set[priv->lane]);
> > +       mutex_unlock(&dp->lock);
> > +       return 0;
> > +}
> > +
> > +static int zynqmp_dp_preemphasis_set(void *data, u64 val)
> > +{
> > +       struct zynqmp_dp_train_set_priv *priv = data;
> > +       struct zynqmp_dp *dp = priv->dp;
> > +       u8 *train_set = &dp->test.train_set[priv->lane];
> > +       int ret = 0;
> > +
> > +       if (val > 2)
> > +               return -EINVAL;
> > +
> > +       mutex_lock(&dp->lock);
> > +       *train_set &= ~(DP_TRAIN_MAX_PRE_EMPHASIS_REACHED |
> > +                       DP_TRAIN_PRE_EMPHASIS_MASK);
> > +       *train_set |= val;
> > +       if (val == 2)
> > +               *train_set |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
> > +
> > +       if (dp->test.active)
> > +               zynqmp_dp_update_vs_emph(dp, dp->test.train_set, true);
> > +       mutex_unlock(&dp->lock);
> > +
> > +       return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_preemphasis, zynqmp_dp_preemphasis_get,
> > +                        zynqmp_dp_preemphasis_set, "%llu\n");
> > +
> > +static int zynqmp_dp_lanes_get(void *data, u64 *val)
> > +{
> > +       struct zynqmp_dp *dp = data;
> > +
> > +       mutex_lock(&dp->lock);
> > +       *val = dp->test.link_cnt;
> > +       mutex_unlock(&dp->lock);
> > +       return 0;
> > +}
> > +
> > +static int zynqmp_dp_lanes_set(void *data, u64 val)
> > +{
> > +       struct zynqmp_dp *dp = data;
> > +       int ret = 0;
> > +
> > +       if (val > ZYNQMP_DP_MAX_LANES)
> > +               return -EINVAL;
> > +
> > +       mutex_lock(&dp->lock);
> > +       if (val > dp->num_lanes) {
> > +               ret = -EINVAL;
> > +       } else {
> > +               dp->test.link_cnt = val;
> > +               if (dp->test.active)
> > +                       ret = zynqmp_dp_test_setup(dp);
> > +       }
> > +       mutex_unlock(&dp->lock);
> > +
> > +       return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_lanes, zynqmp_dp_lanes_get,
> > +                        zynqmp_dp_lanes_set, "%llu\n");
> > +
> > +static int zynqmp_dp_rate_get(void *data, u64 *val)
> > +{
> > +       struct zynqmp_dp *dp = data;
> > +
> > +       mutex_lock(&dp->lock);
> > +       *val = drm_dp_bw_code_to_link_rate(dp->test.bw_code) * 10000;
> > +       mutex_unlock(&dp->lock);
> > +       return 0;
> > +}
> > +
> > +static int zynqmp_dp_rate_set(void *data, u64 val)
> > +{
> > +       struct zynqmp_dp *dp = data;
> > +       u8 bw_code = drm_dp_link_rate_to_bw_code(val / 10000);
> > +       int link_rate = drm_dp_bw_code_to_link_rate(bw_code);
> > +       int ret = 0;
> > +
> > +       if (val / 10000 != link_rate)
> > +               return -EINVAL;
> > +
> > +       if (bw_code != DP_LINK_BW_1_62 && bw_code != DP_LINK_BW_2_7 &&
> > +           bw_code != DP_LINK_BW_5_4)
> > +               return -EINVAL;
> > +
> > +       mutex_lock(&dp->lock);
> > +       dp->test.bw_code = bw_code;
> > +       if (dp->test.active)
> > +               ret = zynqmp_dp_test_setup(dp);
> > +       mutex_unlock(&dp->lock);
> > +
> > +       return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
> > +                        zynqmp_dp_rate_set, "%llu\n");
> > +
> > +static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
> > +                                         struct dentry *root)
> > +{
> > +       struct zynqmp_dp *dp = bridge_to_dp(bridge);
> > +       struct dentry *test;
> > +       int i;
> > +
> > +       dp->test.bw_code = DP_LINK_BW_5_4;
> > +       dp->test.link_cnt = dp->num_lanes;
> > +
> > +       test = debugfs_create_dir("test", root);
> > +#define CREATE_FILE(name) \
> > +       debugfs_create_file(#name, 0600, test, dp, &fops_zynqmp_dp_##name)
> > +       CREATE_FILE(pattern);
> > +       CREATE_FILE(enhanced);
> > +       CREATE_FILE(downspread);
> > +       CREATE_FILE(active);
> > +       CREATE_FILE(custom);
> > +       CREATE_FILE(rate);
> > +       CREATE_FILE(lanes);
> > +
> > +       for (i = 0; i < dp->num_lanes; i++) {
> > +               static const char fmt[] = "lane%d_preemphasis";
> > +               char name[sizeof(fmt)];
> > +
> > +               dp->debugfs_train_set[i].dp = dp;
> > +               dp->debugfs_train_set[i].lane = i;
> > +
> > +               sprintf(name, fmt, i);
> > +               debugfs_create_file(name, 0600, test,
> > +                                   &dp->debugfs_train_set[i],
> > +                                   &fops_zynqmp_dp_preemphasis);
> > +
> > +               sprintf(name, "lane%d_swing", i);
> > +               debugfs_create_file(name, 0600, test,
> > +                                   &dp->debugfs_train_set[i],
> > +                                   &fops_zynqmp_dp_swing);
> > +       }
> > +}
> > +
> >  static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
> >         .attach = zynqmp_dp_bridge_attach,
> >         .detach = zynqmp_dp_bridge_detach,
> > @@ -1611,6 +2189,7 @@ static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
> >         .atomic_check = zynqmp_dp_bridge_atomic_check,
> >         .detect = zynqmp_dp_bridge_detect,
> >         .get_edid = zynqmp_dp_bridge_get_edid,
> > +       .debugfs_init = zynqmp_dp_bridge_debugfs_init,
> >  };
> >
> >  /* -----------------------------------------------------------------------------
> > @@ -1645,6 +2224,9 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
> >                                             hpd_work.work);
> >         enum drm_connector_status status;
> >
> > +       if (dp->test.active)
> > +               return;
> > +
> >         status = zynqmp_dp_bridge_detect(&dp->bridge);
> >         drm_bridge_hpd_notify(&dp->bridge, status);
> >  }
> > @@ -1666,7 +2248,14 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
> >                 if (status[4] & DP_LINK_STATUS_UPDATED ||
> >                     !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
> >                     !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
> > -                       zynqmp_dp_train_loop(dp);
> > +                       if (dp->test.active) {
> > +                               dev_dbg(dp->dev, "Ignoring HPD IRQ in test mode\n");
> > +                       } else {
> > +                               dev_dbg(dp->dev,
> > +                                       "Retraining due to HPD IRQ (status is [%*ph])\n",
> > +                                       (int)sizeof(status), status);
> > +                               zynqmp_dp_train_loop(dp);
> > +                       }
> >                 }
> >         }
> >         mutex_unlock(&dp->lock);

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel