linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support
@ 2024-05-03 19:29 Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 01/10] drm: zynqmp_kms: Fix AUX bus not getting unregistered Sean Anderson
                   ` (11 more replies)
  0 siblings, 12 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, Sean Anderson

This series cleans up the zyqnmp_dp IRQ and locking situation. Once
that's done, it adds debugfs support. The intent is to enable compliance
testing or to help debug signal-integrity issues.

Last time I discussed converting the HPD work(s) to a threaded IRQ. I
did not end up doing that for this series since the steps would be

- Add locking
- Move link retraining to a work function
- Harden the IRQ
- Merge the works into a threaded IRQ (omitted)

Which with the exception of the final step is the same as leaving those
works as-is. Conversion to a threaded IRQ can be done as a follow-up.

Changes in v5:
- Fix AUX bus not getting unregistered
- Rebase onto drm-misc/drm-misc-next

Changes in v4:
- Rebase onto drm/drm-next

Changes in v3:
- Don't delay work
- Convert to a hard IRQ
- Use AUX IRQs instead of polling
- Take dp->lock in zynqmp_dp_hpd_work_func

Changes in v2:
- Rearrange zynqmp_dp for better padding
- Split off the HPD IRQ work into another commit
- Expand the commit message
- Document hpd_irq_work
- Document debugfs files
- Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier
  implicit functionality
- Attempt to fix unreproducable, spurious build warning
- Drop "Optionally ignore DPCD errors" in favor of a debugfs file
  directly affecting zynqmp_dp_aux_transfer.

Sean Anderson (10):
  drm: zynqmp_kms: Fix AUX bus not getting unregistered
  drm: zynqmp_dp: Rearrange zynqmp_dp for better padding
  drm: zynqmp_dp: Don't delay work
  drm: zynqmp_dp: Add locking
  drm: zynqmp_dp: Don't retrain the link in our IRQ
  drm: zynqmp_dp: Convert to a hard IRQ
  drm: zynqmp_dp: Use AUX IRQs instead of polling
  drm: zynqmp_dp: Split off several helper functions
  drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func
  drm: zynqmp_dp: Add debugfs interface for compliance testing

 Documentation/gpu/drivers.rst     |   1 +
 Documentation/gpu/zynqmp.rst      | 149 +++++
 MAINTAINERS                       |   1 +
 drivers/gpu/drm/xlnx/zynqmp_dp.c  | 883 +++++++++++++++++++++++++++---
 drivers/gpu/drm/xlnx/zynqmp_kms.c |  12 +-
 5 files changed, 977 insertions(+), 69 deletions(-)
 create mode 100644 Documentation/gpu/zynqmp.rst

-- 
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 01/10] drm: zynqmp_kms: Fix AUX bus not getting unregistered
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
@ 2024-05-03 19:29 ` Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 02/10] drm: zynqmp_dp: Rearrange zynqmp_dp for better padding Sean Anderson
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, Sean Anderson

drm_encoder_cleanup is responsible for calling drm_bridge_detach for
each bridge attached to the encoder. zynqmp_dp_bridge_detach is in turn
responsible for unregistering the AUX bus. However, we never ended up
calling drm_encoder_cleanup in the remove or error paths, so the AUX bus
would stick around after the rest of the driver had been removed.

I don't really understand why drm_mode_config_cleanup doesn't call
drm_encoder_cleanup for us. It will call destroy (which for
simple_encoder is drm_encoder_cleanup) on encoders in the mode_config's
encoder_list.

Should drm_encoder_cleanup get called before or after
drm_atomic_helper_shutdown?

Fixes: 2dfd045c8435 ("drm: xlnx: zynqmp_dpsub: Register AUX bus at bridge attach time")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v5:
- New

 drivers/gpu/drm/xlnx/zynqmp_kms.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index 43bf416b33d5..f25583ce92e6 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
@@ -433,23 +433,28 @@ static int zynqmp_dpsub_kms_init(struct zynqmp_dpsub *dpsub)
 				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (ret) {
 		dev_err(dpsub->dev, "failed to attach bridge to encoder\n");
-		return ret;
+		goto err_encoder;
 	}
 
 	/* Create the connector for the chain of bridges. */
 	connector = drm_bridge_connector_init(&dpsub->drm->dev, encoder);
 	if (IS_ERR(connector)) {
 		dev_err(dpsub->dev, "failed to created connector\n");
-		return PTR_ERR(connector);
+		ret = PTR_ERR(connector);
+		goto err_encoder;
 	}
 
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret < 0) {
 		dev_err(dpsub->dev, "failed to attach connector to encoder\n");
-		return ret;
+		goto err_encoder;
 	}
 
 	return 0;
+
+err_encoder:
+	drm_encoder_cleanup(encoder);
+	return ret;
 }
 
 static void zynqmp_dpsub_drm_release(struct drm_device *drm, void *res)
@@ -529,5 +534,6 @@ void zynqmp_dpsub_drm_cleanup(struct zynqmp_dpsub *dpsub)
 
 	drm_dev_unregister(drm);
 	drm_atomic_helper_shutdown(drm);
+	drm_encoder_cleanup(&dpsub->drm->encoder);
 	drm_kms_helper_poll_fini(drm);
 }
-- 
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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v5 02/10] drm: zynqmp_dp: Rearrange zynqmp_dp for better padding
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 01/10] drm: zynqmp_kms: Fix AUX bus not getting unregistered Sean Anderson
@ 2024-05-03 19:29 ` Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 03/10] drm: zynqmp_dp: Don't delay work Sean Anderson
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, Sean Anderson

Sort the members of struct zynqmp_dp to reduce padding necessary for
alignment.

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

(no changes since v2)

Changes in v2:
- New

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

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 9df068a413f3..12a8248ed125 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -256,10 +256,10 @@ struct zynqmp_dp_link_config {
  * @fmt: format identifier string
  */
 struct zynqmp_dp_mode {
-	u8 bw_code;
-	u8 lane_cnt;
-	int pclock;
 	const char *fmt;
+	int pclock;
+	u8 bw_code;
+	u8 lane_cnt;
 };
 
 /**
@@ -296,27 +296,27 @@ struct zynqmp_dp_config {
  * @train_set: set of training data
  */
 struct zynqmp_dp {
+	struct drm_dp_aux aux;
+	struct drm_bridge bridge;
+	struct delayed_work hpd_work;
+
+	struct drm_bridge *next_bridge;
 	struct device *dev;
 	struct zynqmp_dpsub *dpsub;
 	void __iomem *iomem;
 	struct reset_control *reset;
-	int irq;
-
-	struct drm_bridge bridge;
-	struct drm_bridge *next_bridge;
-
-	struct zynqmp_dp_config config;
-	struct drm_dp_aux aux;
 	struct phy *phy[ZYNQMP_DP_MAX_LANES];
-	u8 num_lanes;
-	struct delayed_work hpd_work;
+
 	enum drm_connector_status status;
+	int irq;
 	bool enabled;
 
-	u8 dpcd[DP_RECEIVER_CAP_SIZE];
-	struct zynqmp_dp_link_config link_config;
 	struct zynqmp_dp_mode mode;
+	struct zynqmp_dp_link_config link_config;
+	struct zynqmp_dp_config config;
+	u8 dpcd[DP_RECEIVER_CAP_SIZE];
 	u8 train_set[ZYNQMP_DP_MAX_LANES];
+	u8 num_lanes;
 };
 
 static inline struct zynqmp_dp *bridge_to_dp(struct drm_bridge *bridge)
-- 
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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v5 03/10] drm: zynqmp_dp: Don't delay work
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 01/10] drm: zynqmp_kms: Fix AUX bus not getting unregistered Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 02/10] drm: zynqmp_dp: Rearrange zynqmp_dp for better padding Sean Anderson
@ 2024-05-03 19:29 ` Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 04/10] drm: zynqmp_dp: Add locking Sean Anderson
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, Sean Anderson

We always call scheduled_delayed_work with no delay, so just use a
non-delayed work_struct instead.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---

(no changes since v3)

Changes in v3:
- New

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 12a8248ed125..129beac4c073 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -298,7 +298,7 @@ struct zynqmp_dp_config {
 struct zynqmp_dp {
 	struct drm_dp_aux aux;
 	struct drm_bridge bridge;
-	struct delayed_work hpd_work;
+	struct work_struct hpd_work;
 
 	struct drm_bridge *next_bridge;
 	struct device *dev;
@@ -1482,7 +1482,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 	struct zynqmp_dp *dp = bridge_to_dp(bridge);
 
 	dp->enabled = false;
-	cancel_delayed_work(&dp->hpd_work);
+	cancel_work(&dp->hpd_work);
 	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
 	drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
 	zynqmp_dp_write(dp, ZYNQMP_DP_TX_PHY_POWER_DOWN,
@@ -1648,8 +1648,7 @@ void zynqmp_dp_disable_vblank(struct zynqmp_dp *dp)
 
 static void zynqmp_dp_hpd_work_func(struct work_struct *work)
 {
-	struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
-					    hpd_work.work);
+	struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp, hpd_work);
 	enum drm_connector_status status;
 
 	status = zynqmp_dp_bridge_detect(&dp->bridge);
@@ -1685,7 +1684,7 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
 		zynqmp_dpsub_drm_handle_vblank(dp->dpsub);
 
 	if (status & ZYNQMP_DP_INT_HPD_EVENT)
-		schedule_delayed_work(&dp->hpd_work, 0);
+		schedule_work(&dp->hpd_work);
 
 	if (status & ZYNQMP_DP_INT_HPD_IRQ) {
 		int ret;
@@ -1727,7 +1726,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 	dp->dpsub = dpsub;
 	dp->status = connector_status_disconnected;
 
-	INIT_DELAYED_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
+	INIT_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
 
 	/* Acquire all resources (IOMEM, IRQ and PHYs). */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
@@ -1832,7 +1831,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_work);
+	cancel_work_sync(&dp->hpd_work);
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
 	zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, 0xffffffff);
-- 
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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v5 04/10] drm: zynqmp_dp: Add locking
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
                   ` (2 preceding siblings ...)
  2024-05-03 19:29 ` [PATCH v5 03/10] drm: zynqmp_dp: Don't delay work Sean Anderson
@ 2024-05-03 19:29 ` Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 05/10] drm: zynqmp_dp: Don't retrain the link in our IRQ Sean Anderson
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, Sean Anderson

Add some locking to prevent the IRQ/workers/bridge API calls from stepping
on each other's toes. 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). We also access AUX
  while holding this lock, so it would be very tricky to support.
- Link configuration. This is effectively everything in zynqmp_dp which
  isn't read-only after probe time. So from next_bridge onward.

This lock is 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.

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

(no changes since v2)

Changes in v2:
- Split off the HPD IRQ work into another commit
- Expand the commit message

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 129beac4c073..abfccd8bb5a7 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -280,6 +280,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
@@ -294,11 +295,16 @@ 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
+ *
+ * @lock covers the link configuration in this struct and the device's
+ * registers. It does not cover @aux. It is not strictly required for any of
+ * the members which are only modified at probe/remove time (e.g. @dev).
  */
 struct zynqmp_dp {
 	struct drm_dp_aux aux;
 	struct drm_bridge bridge;
 	struct work_struct hpd_work;
+	struct mutex lock;
 
 	struct drm_bridge *next_bridge;
 	struct device *dev;
@@ -1386,8 +1392,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);
@@ -1414,6 +1422,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);
 
 	/*
@@ -1474,6 +1483,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,
@@ -1481,6 +1491,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_work(&dp->hpd_work);
 	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
@@ -1491,6 +1502,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);
 }
@@ -1533,6 +1545,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.
@@ -1560,11 +1574,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;
 }
 
@@ -1725,6 +1741,7 @@ 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_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
 
@@ -1838,4 +1855,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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v5 05/10] drm: zynqmp_dp: Don't retrain the link in our IRQ
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
                   ` (3 preceding siblings ...)
  2024-05-03 19:29 ` [PATCH v5 04/10] drm: zynqmp_dp: Add locking Sean Anderson
@ 2024-05-03 19:29 ` Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 06/10] drm: zynqmp_dp: Convert to a hard IRQ Sean Anderson
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, Sean Anderson

Retraining the link can take a while, and might involve waiting for
DPCD reads/writes to complete. In preparation for unthreading the IRQ
handler, move this into its own work function.

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

(no changes since v2)

Changes in v2:
- Document hpd_irq_work
- Split this off from the locking changes

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

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index abfccd8bb5a7..cec5711c7026 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -289,6 +289,7 @@ struct zynqmp_dp_config {
  * @phy: PHY handles for DP lanes
  * @num_lanes: number of enabled phy lanes
  * @hpd_work: hot plug detection worker
+ * @hpd_irq_work: hot plug detection IRQ worker
  * @status: connection status
  * @enabled: flag to indicate if the device is enabled
  * @dpcd: DP configuration data from currently connected sink device
@@ -304,6 +305,7 @@ struct zynqmp_dp {
 	struct drm_dp_aux aux;
 	struct drm_bridge bridge;
 	struct work_struct hpd_work;
+	struct work_struct hpd_irq_work;
 	struct mutex lock;
 
 	struct drm_bridge *next_bridge;
@@ -1671,6 +1673,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);
+	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;
@@ -1702,23 +1727,9 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
 	if (status & ZYNQMP_DP_INT_HPD_EVENT)
 		schedule_work(&dp->hpd_work);
 
-	if (status & ZYNQMP_DP_INT_HPD_IRQ) {
-		int ret;
-		u8 status[DP_LINK_STATUS_SIZE + 2];
+	if (status & ZYNQMP_DP_INT_HPD_IRQ)
+		schedule_work(&dp->hpd_irq_work);
 
-		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;
 }
 
@@ -1744,6 +1755,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 	mutex_init(&dp->lock);
 
 	INIT_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
+	INIT_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");
@@ -1848,6 +1860,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_work_sync(&dp->hpd_irq_work);
 	cancel_work_sync(&dp->hpd_work);
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
-- 
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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v5 06/10] drm: zynqmp_dp: Convert to a hard IRQ
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
                   ` (4 preceding siblings ...)
  2024-05-03 19:29 ` [PATCH v5 05/10] drm: zynqmp_dp: Don't retrain the link in our IRQ Sean Anderson
@ 2024-05-03 19:29 ` Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 07/10] drm: zynqmp_dp: Use AUX IRQs instead of polling Sean Anderson
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, Sean Anderson

Now that all of the sleeping work is done outside of the IRQ, we can
convert it to a hard IRQ.

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

(no changes since v3)

Changes in v3:
- New

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index cec5711c7026..c9cdfb56f23e 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1831,9 +1831,8 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 	 * Now that the hardware is initialized and won't generate spurious
 	 * interrupts, request the IRQ.
 	 */
-	ret = devm_request_threaded_irq(dp->dev, dp->irq, NULL,
-					zynqmp_dp_irq_handler, IRQF_ONESHOT,
-					dev_name(dp->dev), dp);
+	ret = devm_request_irq(dp->dev, dp->irq, zynqmp_dp_irq_handler,
+			       IRQF_SHARED, dev_name(dp->dev), dp);
 	if (ret < 0)
 		goto err_phy_exit;
 
-- 
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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v5 07/10] drm: zynqmp_dp: Use AUX IRQs instead of polling
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
                   ` (5 preceding siblings ...)
  2024-05-03 19:29 ` [PATCH v5 06/10] drm: zynqmp_dp: Convert to a hard IRQ Sean Anderson
@ 2024-05-03 19:29 ` Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 08/10] drm: zynqmp_dp: Split off several helper functions Sean Anderson
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, Sean Anderson

Instead of polling the status register for the AUX status, just enable
the IRQs and signal a completion.

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

(no changes since v3)

Changes in v3:
- New

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 35 +++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index c9cdfb56f23e..38610a5b932a 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -286,6 +286,7 @@ struct zynqmp_dp_config {
  * @next_bridge: The downstream bridge
  * @config: IP core configuration from DTS
  * @aux: aux channel
+ * @aux_done: Completed when we get an AUX reply or timeout
  * @phy: PHY handles for DP lanes
  * @num_lanes: number of enabled phy lanes
  * @hpd_work: hot plug detection worker
@@ -306,6 +307,7 @@ struct zynqmp_dp {
 	struct drm_bridge bridge;
 	struct work_struct hpd_work;
 	struct work_struct hpd_irq_work;
+	struct completion aux_done;
 	struct mutex lock;
 
 	struct drm_bridge *next_bridge;
@@ -942,12 +944,15 @@ static int zynqmp_dp_aux_cmd_submit(struct zynqmp_dp *dp, u32 cmd, u16 addr,
 				    u8 *buf, u8 bytes, u8 *reply)
 {
 	bool is_read = (cmd & AUX_READ_BIT) ? true : false;
+	unsigned long time_left;
 	u32 reg, i;
 
 	reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
 	if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REQUEST)
 		return -EBUSY;
 
+	reinit_completion(&dp->aux_done);
+
 	zynqmp_dp_write(dp, ZYNQMP_DP_AUX_ADDRESS, addr);
 	if (!is_read)
 		for (i = 0; i < bytes; i++)
@@ -962,17 +967,14 @@ static int zynqmp_dp_aux_cmd_submit(struct zynqmp_dp *dp, u32 cmd, u16 addr,
 	zynqmp_dp_write(dp, ZYNQMP_DP_AUX_COMMAND, reg);
 
 	/* Wait for reply to be delivered upto 2ms */
-	for (i = 0; ; i++) {
-		reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
-		if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY)
-			break;
+	time_left = wait_for_completion_timeout(&dp->aux_done,
+						msecs_to_jiffies(2));
+	if (!time_left)
+		return -ETIMEDOUT;
 
-		if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT ||
-		    i == 2)
-			return -ETIMEDOUT;
-
-		usleep_range(1000, 1100);
-	}
+	reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
+	if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT)
+		return -ETIMEDOUT;
 
 	reg = zynqmp_dp_read(dp, ZYNQMP_DP_AUX_REPLY_CODE);
 	if (reply)
@@ -1056,6 +1058,9 @@ static int zynqmp_dp_aux_init(struct zynqmp_dp *dp)
 			(w << ZYNQMP_DP_AUX_CLK_DIVIDER_AUX_FILTER_SHIFT) |
 			(rate / (1000 * 1000)));
 
+	zynqmp_dp_write(dp, ZYNQMP_DP_INT_EN, ZYNQMP_DP_INT_REPLY_RECEIVED |
+					      ZYNQMP_DP_INT_REPLY_TIMEOUT);
+
 	dp->aux.name = "ZynqMP DP AUX";
 	dp->aux.dev = dp->dev;
 	dp->aux.drm_dev = dp->bridge.dev;
@@ -1073,6 +1078,9 @@ static int zynqmp_dp_aux_init(struct zynqmp_dp *dp)
 static void zynqmp_dp_aux_cleanup(struct zynqmp_dp *dp)
 {
 	drm_dp_aux_unregister(&dp->aux);
+
+	zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_REPLY_RECEIVED |
+					      ZYNQMP_DP_INT_REPLY_TIMEOUT);
 }
 
 /* -----------------------------------------------------------------------------
@@ -1730,6 +1738,12 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
 	if (status & ZYNQMP_DP_INT_HPD_IRQ)
 		schedule_work(&dp->hpd_irq_work);
 
+	if (status & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY)
+		complete(&dp->aux_done);
+
+	if (status & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT)
+		complete(&dp->aux_done);
+
 	return IRQ_HANDLED;
 }
 
@@ -1753,6 +1767,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 	dp->dpsub = dpsub;
 	dp->status = connector_status_disconnected;
 	mutex_init(&dp->lock);
+	init_completion(&dp->aux_done);
 
 	INIT_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
 	INIT_WORK(&dp->hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
-- 
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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v5 08/10] drm: zynqmp_dp: Split off several helper functions
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
                   ` (6 preceding siblings ...)
  2024-05-03 19:29 ` [PATCH v5 07/10] drm: zynqmp_dp: Use AUX IRQs instead of polling Sean Anderson
@ 2024-05-03 19:29 ` Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 09/10] drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func Sean Anderson
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 38610a5b932a..91767ddbe1ce 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -636,6 +636,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.
@@ -643,12 +644,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;
@@ -656,7 +657,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;
@@ -700,7 +701,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;
 
@@ -765,7 +766,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;
 
@@ -788,28 +789,29 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
 }
 
 /**
- * zynqmp_dp_train - Train the link
+ * zynqmp_dp_setup() - Set up major link parameters
  * @dp: DisplayPort IP core structure
+ * @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);
@@ -852,8 +854,25 @@ 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[DP_MAX_DOWNSPREAD] &
+			      DP_MAX_DOWNSPREAD_0_5);
+	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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v5 09/10] drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
                   ` (7 preceding siblings ...)
  2024-05-03 19:29 ` [PATCH v5 08/10] drm: zynqmp_dp: Split off several helper functions Sean Anderson
@ 2024-05-03 19:29 ` Sean Anderson
  2024-05-03 19:29 ` [PATCH v5 10/10] drm: zynqmp_dp: Add debugfs interface for compliance testing Sean Anderson
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, Sean Anderson

Add a non-locking version of zynqmp_dp_bridge_detect and use it in
zynqmp_dp_hpd_work_func so we can take the lock explicitly. This will
make it easier to check for hpd_ignore when we add debugfs support.

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

(no changes since v3)

Changes in v3:
- New

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 91767ddbe1ce..6f3792dcac28 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1567,14 +1567,13 @@ static int zynqmp_dp_bridge_atomic_check(struct drm_bridge *bridge,
 	return 0;
 }
 
-static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *bridge)
+static enum drm_connector_status __zynqmp_dp_bridge_detect(struct zynqmp_dp *dp)
 {
-	struct zynqmp_dp *dp = bridge_to_dp(bridge);
 	struct zynqmp_dp_link_config *link_config = &dp->link_config;
 	u32 state, i;
 	int ret;
 
-	mutex_lock(&dp->lock);
+	lockdep_assert_held(&dp->lock);
 
 	/*
 	 * This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
@@ -1603,16 +1602,26 @@ 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;
 }
 
+static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *bridge)
+{
+	struct zynqmp_dp *dp = bridge_to_dp(bridge);
+	enum drm_connector_status ret;
+
+	mutex_lock(&dp->lock);
+	ret = __zynqmp_dp_bridge_detect(dp);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
 static const struct drm_edid *zynqmp_dp_bridge_edid_read(struct drm_bridge *bridge,
 							 struct drm_connector *connector)
 {
@@ -1696,7 +1705,10 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
 	struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp, hpd_work);
 	enum drm_connector_status status;
 
-	status = zynqmp_dp_bridge_detect(&dp->bridge);
+	mutex_lock(&dp->lock);
+	status = __zynqmp_dp_bridge_detect(dp);
+	mutex_unlock(&dp->lock);
+
 	drm_bridge_hpd_notify(&dp->bridge, status);
 }
 
-- 
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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v5 10/10] drm: zynqmp_dp: Add debugfs interface for compliance testing
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
                   ` (8 preceding siblings ...)
  2024-05-03 19:29 ` [PATCH v5 09/10] drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func Sean Anderson
@ 2024-05-03 19:29 ` Sean Anderson
  2024-05-31 16:27 ` [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
  2024-06-17  7:47 ` Tomi Valkeinen
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-03 19:29 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek, 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.

Additionally, add some debugfs files for ignoring AUX errors and HPD
events, as this can allow testing with equipment that cannot emulate a
DPRX.

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

(no changes since v2)

Changes in v2:
- Document debugfs files
- Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier
  implicit functionality
- Attempt to fix unreproducable, spurious build warning

 Documentation/gpu/drivers.rst    |   1 +
 Documentation/gpu/zynqmp.rst     | 149 +++++++
 MAINTAINERS                      |   1 +
 drivers/gpu/drm/xlnx/zynqmp_dp.c | 682 ++++++++++++++++++++++++++++++-
 4 files changed, 830 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/gpu/zynqmp.rst

diff --git a/Documentation/gpu/drivers.rst b/Documentation/gpu/drivers.rst
index b899cbc5c2b4..187201aedbe5 100644
--- a/Documentation/gpu/drivers.rst
+++ b/Documentation/gpu/drivers.rst
@@ -22,6 +22,7 @@ GPU Driver Documentation
    afbc
    komeda-kms
    panfrost
+   zynqmp
 
 .. only::  subproject and html
 
diff --git a/Documentation/gpu/zynqmp.rst b/Documentation/gpu/zynqmp.rst
new file mode 100644
index 000000000000..f57bfa0ad6ec
--- /dev/null
+++ b/Documentation/gpu/zynqmp.rst
@@ -0,0 +1,149 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+===============================================
+Xilinx ZynqMP Ultrascale+ DisplayPort Subsystem
+===============================================
+
+This subsystem handles DisplayPort video and audio output on the ZynqMP. It
+supports in-memory framebuffers with the DisplayPort DMA controller
+(xilinx-dpdma), as well as "live" video and audio from the programmable logic
+(PL). This subsystem can perform several transformations, including color space
+conversion, alpha blending, and audio mixing, although not all features are
+currently supported.
+
+debugfs
+-------
+
+To support debugging and compliance testing, several test modes can be enabled
+though debugfs. The following files in /sys/kernel/debug/dri/X/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 re-activate/re-deactivate test mode. When test
+        mode is inactive, changes made to other files will have no (immediate)
+        effect, although the settings will be saved for when test mode is
+        activated. When test mode is active, changes made to other files will
+        apply immediately.
+
+custom:
+        Custom test pattern value
+
+downspread:
+        Enable/disable clock downspreading (spread-spectrum clocking) by
+        writing 1/0
+
+enhanced:
+        Enable/disable enhanced framing
+
+ignore_aux_errors:
+        Ignore AUX errors when set to 1. Writes to this file take effect
+        immediately (regardless of whether test mode is active) and affect all
+        AUX transfers.
+
+ignore_hpd:
+        Ignore hotplug events (such as cable removals or monitor link
+        retraining requests) when set to 1. Writes to this file take effect
+        immediately (regardless of whether test mode is active).
+
+laneX_preemphasis:
+        Preemphasis from 0 (lowest) to 2 (highest) for lane X
+
+laneX_swing:
+        Voltage swing from 0 (lowest) to 3 (highest) for lane X
+
+lanes:
+        Number of lanes to use (1, 2, or 4)
+
+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 '%-17s ' ${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
+        ignore_aux_errors 1
+        ignore_hpd        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/ignore_aux_errors
+        echo 1 > /sys/kernel/debug/dri/1/DP-1/test/ignore_hpd
+        echo 1 > /sys/kernel/debug/dri/1/DP-1/test/active
+
+at which point the cable could be disconnected from the monitor.
+
+Internals
+---------
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_disp.h
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_dpsub.h
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_kms.h
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_disp.c
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_dp.c
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_kms.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7d735037a383..774a7973cd37 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7391,6 +7391,7 @@ L:	dri-devel@lists.freedesktop.org
 S:	Maintained
 T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
 F:	Documentation/devicetree/bindings/display/xlnx/
+F:	Documentation/gpu/zynqmp.rst
 F:	drivers/gpu/drm/xlnx/
 
 DRM GPU SCHEDULER
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 6f3792dcac28..35323535bbd1 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>
@@ -51,6 +53,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
@@ -64,6 +67,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
@@ -207,6 +213,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
@@ -274,6 +281,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
@@ -284,23 +354,28 @@ 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
  * @aux_done: Completed when we get an AUX reply or timeout
+ * @ignore_aux_errors: If set, AUX errors are suppressed
  * @phy: PHY handles for DP lanes
  * @num_lanes: number of enabled phy lanes
  * @hpd_work: hot plug detection worker
  * @hpd_irq_work: hot plug detection IRQ worker
+ * @ignore_hpd: If set, HPD events and IRQs are ignored
  * @status: connection status
  * @enabled: flag to indicate if the device is enabled
  * @dpcd: DP configuration data from currently connected sink device
  * @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
  *
  * @lock covers the link configuration in this struct and the device's
- * registers. It does not cover @aux. It is not strictly required for any of
- * the members which are only modified at probe/remove time (e.g. @dev).
+ * registers. It does not cover @aux or @ignore_aux_errors. It is not strictly
+ * required for any of the members which are only modified at probe/remove time
+ * (e.g. @dev).
  */
 struct zynqmp_dp {
 	struct drm_dp_aux aux;
@@ -320,9 +395,13 @@ struct zynqmp_dp {
 	enum drm_connector_status status;
 	int irq;
 	bool enabled;
+	bool ignore_aux_errors;
+	bool ignore_hpd;
 
+	struct zynqmp_dp_train_set_priv debugfs_train_set[ZYNQMP_DP_MAX_LANES];
 	struct zynqmp_dp_mode mode;
 	struct zynqmp_dp_link_config link_config;
+	struct zynqmp_dp_test test;
 	struct zynqmp_dp_config config;
 	u8 dpcd[DP_RECEIVER_CAP_SIZE];
 	u8 train_set[ZYNQMP_DP_MAX_LANES];
@@ -1035,6 +1114,8 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 		if (dp->status == connector_status_disconnected) {
 			dev_dbg(dp->dev, "no connected aux device\n");
+			if (dp->ignore_aux_errors)
+				goto fake_response;
 			return -ENODEV;
 		}
 
@@ -1043,7 +1124,13 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 	dev_dbg(dp->dev, "failed to do aux transfer (%d)\n", ret);
 
-	return ret;
+	if (!dp->ignore_aux_errors)
+		return ret;
+
+fake_response:
+	msg->reply = DP_AUX_NATIVE_REPLY_ACK;
+	memset(msg->buffer, 0, msg->size);
+	return msg->size;
 }
 
 /**
@@ -1659,6 +1746,584 @@ zynqmp_dp_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 		return zynqmp_dp_bridge_default_bus_fmts(num_input_fmts);
 }
 
+/* -----------------------------------------------------------------------------
+ * debugfs
+ */
+
+/**
+ * zynqmp_dp_set_test_pattern() - Configure the link for a test pattern
+ * @dp: DisplayPort IP core structure
+ * @pattern: The test pattern to configure
+ * @custom: The custom pattern to use if @pattern is %TEST_80BIT_CUSTOM
+ *
+ * Return: 0 on success, or negative errno on (DPCD) failure
+ */
+static int 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 ret;
+
+	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);
+		ret = drm_dp_dpcd_write(&dp->aux, DP_LINK_QUAL_LANE0_SET,
+					dpcd_link_lane, ZYNQMP_DP_MAX_LANES);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET, dpcd_train);
+	return ret < 0 ? ret : 0;
+}
+
+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);
+}
+
+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)
+		ret = zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
+						 dp->test.custom) ?: ret;
+	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;
+		}
+
+		ret = zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
+						 dp->test.custom);
+		if (ret)
+			goto out;
+
+		ret = zynqmp_dp_update_vs_emph(dp, dp->test.train_set);
+		if (ret)
+			goto out;
+
+		dp->test.active = true;
+	} else {
+		int err;
+
+		dp->test.active = false;
+		err = zynqmp_dp_set_test_pattern(dp, TEST_VIDEO, NULL);
+		if (err)
+			dev_warn(dp->dev, "could not clear test pattern: %d\n",
+				 err);
+		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)
+		ret = zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
+						 dp->test.custom) ?: ret;
+	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)
+		ret = zynqmp_dp_update_vs_emph(dp, dp->test.train_set);
+	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)
+		ret = zynqmp_dp_update_vs_emph(dp, dp->test.train_set);
+	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;
+	int link_rate;
+	int ret = 0;
+	u8 bw_code;
+
+	if (do_div(val, 10000))
+		return -EINVAL;
+
+	bw_code = drm_dp_link_rate_to_bw_code(val);
+	link_rate = drm_dp_bw_code_to_link_rate(bw_code);
+	if (val != 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 int zynqmp_dp_ignore_aux_errors_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->aux.hw_mutex);
+	*val = dp->ignore_aux_errors;
+	mutex_unlock(&dp->aux.hw_mutex);
+	return 0;
+}
+
+static int zynqmp_dp_ignore_aux_errors_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+
+	if (val != !!val)
+		return -EINVAL;
+
+	mutex_lock(&dp->aux.hw_mutex);
+	dp->ignore_aux_errors = val;
+	mutex_unlock(&dp->aux.hw_mutex);
+	return 0;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_ignore_aux_errors,
+			 zynqmp_dp_ignore_aux_errors_get,
+			 zynqmp_dp_ignore_aux_errors_set, "%llu\n");
+
+static int zynqmp_dp_ignore_hpd_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = dp->ignore_hpd;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_ignore_hpd_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+
+	if (val != !!val)
+		return -EINVAL;
+
+	mutex_lock(&dp->lock);
+	dp->ignore_hpd = val;
+	mutex_lock(&dp->lock);
+	return 0;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_ignore_hpd, zynqmp_dp_ignore_hpd_get,
+			 zynqmp_dp_ignore_hpd_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);
+	CREATE_FILE(ignore_aux_errors);
+	CREATE_FILE(ignore_hpd);
+
+	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;
+
+		snprintf(name, sizeof(name), fmt, i);
+		debugfs_create_file(name, 0600, test,
+				    &dp->debugfs_train_set[i],
+				    &fops_zynqmp_dp_preemphasis);
+
+		snprintf(name, sizeof(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,
@@ -1672,6 +2337,7 @@ static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
 	.detect = zynqmp_dp_bridge_detect,
 	.edid_read = zynqmp_dp_bridge_edid_read,
 	.atomic_get_input_bus_fmts = zynqmp_dp_bridge_get_input_bus_fmts,
+	.debugfs_init = zynqmp_dp_bridge_debugfs_init,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1706,6 +2372,11 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
 	enum drm_connector_status status;
 
 	mutex_lock(&dp->lock);
+	if (dp->ignore_hpd) {
+		mutex_unlock(&dp->lock);
+		return;
+	}
+
 	status = __zynqmp_dp_bridge_detect(dp);
 	mutex_unlock(&dp->lock);
 
@@ -1720,6 +2391,11 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
 	int err;
 
 	mutex_lock(&dp->lock);
+	if (dp->ignore_hpd) {
+		mutex_unlock(&dp->lock);
+		return;
+	}
+
 	err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
 			       DP_LINK_STATUS_SIZE + 2);
 	if (err < 0) {
-- 
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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
                   ` (9 preceding siblings ...)
  2024-05-03 19:29 ` [PATCH v5 10/10] drm: zynqmp_dp: Add debugfs interface for compliance testing Sean Anderson
@ 2024-05-31 16:27 ` Sean Anderson
  2024-06-17  7:47 ` Tomi Valkeinen
  11 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-05-31 16:27 UTC (permalink / raw)
  To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Tomi Valkeinen, Michal Simek

On 5/3/24 15:29, Sean Anderson wrote:
> This series cleans up the zyqnmp_dp IRQ and locking situation. Once
> that's done, it adds debugfs support. The intent is to enable compliance
> testing or to help debug signal-integrity issues.
> 
> Last time I discussed converting the HPD work(s) to a threaded IRQ. I
> did not end up doing that for this series since the steps would be
> 
> - Add locking
> - Move link retraining to a work function
> - Harden the IRQ
> - Merge the works into a threaded IRQ (omitted)
> 
> Which with the exception of the final step is the same as leaving those
> works as-is. Conversion to a threaded IRQ can be done as a follow-up.
> 
> Changes in v5:
> - Fix AUX bus not getting unregistered
> - Rebase onto drm-misc/drm-misc-next
> 
> Changes in v4:
> - Rebase onto drm/drm-next
> 
> Changes in v3:
> - Don't delay work
> - Convert to a hard IRQ
> - Use AUX IRQs instead of polling
> - Take dp->lock in zynqmp_dp_hpd_work_func
> 
> Changes in v2:
> - Rearrange zynqmp_dp for better padding
> - Split off the HPD IRQ work into another commit
> - Expand the commit message
> - Document hpd_irq_work
> - Document debugfs files
> - Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier
>   implicit functionality
> - Attempt to fix unreproducable, spurious build warning
> - Drop "Optionally ignore DPCD errors" in favor of a debugfs file
>   directly affecting zynqmp_dp_aux_transfer.
> 
> Sean Anderson (10):
>   drm: zynqmp_kms: Fix AUX bus not getting unregistered
>   drm: zynqmp_dp: Rearrange zynqmp_dp for better padding
>   drm: zynqmp_dp: Don't delay work
>   drm: zynqmp_dp: Add locking
>   drm: zynqmp_dp: Don't retrain the link in our IRQ
>   drm: zynqmp_dp: Convert to a hard IRQ
>   drm: zynqmp_dp: Use AUX IRQs instead of polling
>   drm: zynqmp_dp: Split off several helper functions
>   drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func
>   drm: zynqmp_dp: Add debugfs interface for compliance testing
> 
>  Documentation/gpu/drivers.rst     |   1 +
>  Documentation/gpu/zynqmp.rst      | 149 +++++
>  MAINTAINERS                       |   1 +
>  drivers/gpu/drm/xlnx/zynqmp_dp.c  | 883 +++++++++++++++++++++++++++---
>  drivers/gpu/drm/xlnx/zynqmp_kms.c |  12 +-
>  5 files changed, 977 insertions(+), 69 deletions(-)
>  create mode 100644 Documentation/gpu/zynqmp.rst
> 

ping

--Sean

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support
  2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
                   ` (10 preceding siblings ...)
  2024-05-31 16:27 ` [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
@ 2024-06-17  7:47 ` Tomi Valkeinen
  2024-06-17 14:48   ` Sean Anderson
  11 siblings, 1 reply; 16+ messages in thread
From: Tomi Valkeinen @ 2024-06-17  7:47 UTC (permalink / raw)
  To: Sean Anderson, Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Michal Simek

Hi Sean,

On 03/05/2024 22:29, Sean Anderson wrote:
> This series cleans up the zyqnmp_dp IRQ and locking situation. Once
> that's done, it adds debugfs support. The intent is to enable compliance
> testing or to help debug signal-integrity issues.
> 
> Last time I discussed converting the HPD work(s) to a threaded IRQ. I
> did not end up doing that for this series since the steps would be
> 
> - Add locking
> - Move link retraining to a work function
> - Harden the IRQ
> - Merge the works into a threaded IRQ (omitted)
> 
> Which with the exception of the final step is the same as leaving those
> works as-is. Conversion to a threaded IRQ can be done as a follow-up.

I tested this, and the "drm: zynqmp_dp: Convert to a hard IRQ" causes a 
hang for me when unloading the drivers. Unfortunately I'm not in the 
condition to debug it at the moment.

I have picked the first three patches into drm-misc-next, though, to 
decrease the number of patches in the series a bit. They looked 
independent and safe enough to apply.

  Tomi

> 
> Changes in v5:
> - Fix AUX bus not getting unregistered
> - Rebase onto drm-misc/drm-misc-next
> 
> Changes in v4:
> - Rebase onto drm/drm-next
> 
> Changes in v3:
> - Don't delay work
> - Convert to a hard IRQ
> - Use AUX IRQs instead of polling
> - Take dp->lock in zynqmp_dp_hpd_work_func
> 
> Changes in v2:
> - Rearrange zynqmp_dp for better padding
> - Split off the HPD IRQ work into another commit
> - Expand the commit message
> - Document hpd_irq_work
> - Document debugfs files
> - Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier
>    implicit functionality
> - Attempt to fix unreproducable, spurious build warning
> - Drop "Optionally ignore DPCD errors" in favor of a debugfs file
>    directly affecting zynqmp_dp_aux_transfer.
> 
> Sean Anderson (10):
>    drm: zynqmp_kms: Fix AUX bus not getting unregistered
>    drm: zynqmp_dp: Rearrange zynqmp_dp for better padding
>    drm: zynqmp_dp: Don't delay work
>    drm: zynqmp_dp: Add locking
>    drm: zynqmp_dp: Don't retrain the link in our IRQ
>    drm: zynqmp_dp: Convert to a hard IRQ
>    drm: zynqmp_dp: Use AUX IRQs instead of polling
>    drm: zynqmp_dp: Split off several helper functions
>    drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func
>    drm: zynqmp_dp: Add debugfs interface for compliance testing
> 
>   Documentation/gpu/drivers.rst     |   1 +
>   Documentation/gpu/zynqmp.rst      | 149 +++++
>   MAINTAINERS                       |   1 +
>   drivers/gpu/drm/xlnx/zynqmp_dp.c  | 883 +++++++++++++++++++++++++++---
>   drivers/gpu/drm/xlnx/zynqmp_kms.c |  12 +-
>   5 files changed, 977 insertions(+), 69 deletions(-)
>   create mode 100644 Documentation/gpu/zynqmp.rst
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support
  2024-06-17  7:47 ` Tomi Valkeinen
@ 2024-06-17 14:48   ` Sean Anderson
  2024-08-08 12:46     ` Tomi Valkeinen
  0 siblings, 1 reply; 16+ messages in thread
From: Sean Anderson @ 2024-06-17 14:48 UTC (permalink / raw)
  To: Tomi Valkeinen, Laurent Pinchart, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, dri-devel
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Michal Simek

On 6/17/24 03:47, Tomi Valkeinen wrote:
> Hi Sean,
> 
> On 03/05/2024 22:29, Sean Anderson wrote:
>> This series cleans up the zyqnmp_dp IRQ and locking situation. Once
>> that's done, it adds debugfs support. The intent is to enable compliance
>> testing or to help debug signal-integrity issues.
>>
>> Last time I discussed converting the HPD work(s) to a threaded IRQ. I
>> did not end up doing that for this series since the steps would be
>>
>> - Add locking
>> - Move link retraining to a work function
>> - Harden the IRQ
>> - Merge the works into a threaded IRQ (omitted)
>>
>> Which with the exception of the final step is the same as leaving those
>> works as-is. Conversion to a threaded IRQ can be done as a follow-up.
> 
> I tested this, and the "drm: zynqmp_dp: Convert to a hard IRQ" causes a hang for me when unloading the drivers. Unfortunately I'm not in the condition to debug it at the moment.
> 
> I have picked the first three patches into drm-misc-next, though, to decrease the number of patches in the series a bit. They looked independent and safe enough to apply.

Are you running into [1]?

--Sean

[1] https://lore.kernel.org/dri-devel/4d8f4c9b-2efb-4774-9a37-2f257f79b2c9@linux.dev/



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support
  2024-06-17 14:48   ` Sean Anderson
@ 2024-08-08 12:46     ` Tomi Valkeinen
  2024-08-09 18:55       ` Sean Anderson
  0 siblings, 1 reply; 16+ messages in thread
From: Tomi Valkeinen @ 2024-08-08 12:46 UTC (permalink / raw)
  To: Sean Anderson
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Michal Simek, Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel

Hi Sean,

On 17/06/2024 17:48, Sean Anderson wrote:
> On 6/17/24 03:47, Tomi Valkeinen wrote:
>> Hi Sean,
>>
>> On 03/05/2024 22:29, Sean Anderson wrote:
>>> This series cleans up the zyqnmp_dp IRQ and locking situation. Once
>>> that's done, it adds debugfs support. The intent is to enable compliance
>>> testing or to help debug signal-integrity issues.
>>>
>>> Last time I discussed converting the HPD work(s) to a threaded IRQ. I
>>> did not end up doing that for this series since the steps would be
>>>
>>> - Add locking
>>> - Move link retraining to a work function
>>> - Harden the IRQ
>>> - Merge the works into a threaded IRQ (omitted)
>>>
>>> Which with the exception of the final step is the same as leaving those
>>> works as-is. Conversion to a threaded IRQ can be done as a follow-up.
>>
>> I tested this, and the "drm: zynqmp_dp: Convert to a hard IRQ" causes a hang for me when unloading the drivers. Unfortunately I'm not in the condition to debug it at the moment.
>>
>> I have picked the first three patches into drm-misc-next, though, to decrease the number of patches in the series a bit. They looked independent and safe enough to apply.
> 
> Are you running into [1]?
> 
> --Sean
> 
> [1] https://lore.kernel.org/dri-devel/4d8f4c9b-2efb-4774-9a37-2f257f79b2c9@linux.dev/
> 

No. Afaics, it breaks because the irq handler is requested with 
IRQF_SHARED, and that means the handler can be called at any time. The 
handler reads DP registers, but the DP IP could already be powered off.

You'll probably see it easily if you enable CONFIG_DEBUG_SHIRQ, and 
unload the module or unbind the device.

  Tomi



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support
  2024-08-08 12:46     ` Tomi Valkeinen
@ 2024-08-09 18:55       ` Sean Anderson
  0 siblings, 0 replies; 16+ messages in thread
From: Sean Anderson @ 2024-08-09 18:55 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: linux-arm-kernel, David Airlie, linux-kernel, Daniel Vetter,
	Michal Simek, Laurent Pinchart, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, dri-devel

On 8/8/24 08:46, Tomi Valkeinen wrote:
> Hi Sean,
> 
> On 17/06/2024 17:48, Sean Anderson wrote:
>> On 6/17/24 03:47, Tomi Valkeinen wrote:
>>> Hi Sean,
>>>
>>> On 03/05/2024 22:29, Sean Anderson wrote:
>>>> This series cleans up the zyqnmp_dp IRQ and locking situation. Once
>>>> that's done, it adds debugfs support. The intent is to enable compliance
>>>> testing or to help debug signal-integrity issues.
>>>>
>>>> Last time I discussed converting the HPD work(s) to a threaded IRQ. I
>>>> did not end up doing that for this series since the steps would be
>>>>
>>>> - Add locking
>>>> - Move link retraining to a work function
>>>> - Harden the IRQ
>>>> - Merge the works into a threaded IRQ (omitted)
>>>>
>>>> Which with the exception of the final step is the same as leaving those
>>>> works as-is. Conversion to a threaded IRQ can be done as a follow-up.
>>>
>>> I tested this, and the "drm: zynqmp_dp: Convert to a hard IRQ" causes a hang for me when unloading the drivers. Unfortunately I'm not in the condition to debug it at the moment.
>>>
>>> I have picked the first three patches into drm-misc-next, though, to decrease the number of patches in the series a bit. They looked independent and safe enough to apply.
>>
>> Are you running into [1]?
>>
>> --Sean
>>
>> [1] https://lore.kernel.org/dri-devel/4d8f4c9b-2efb-4774-9a37-2f257f79b2c9@linux.dev/
>>
> 
> No. Afaics, it breaks because the irq handler is requested with IRQF_SHARED, and that means the handler can be called at any time. The handler reads DP registers, but the DP IP could already be powered off.
> 
> You'll probably see it easily if you enable CONFIG_DEBUG_SHIRQ, and unload the module or unbind the device.

Ah, looks like I need to use devm_free_irq instead of disable_irq.

And after fixing this, [1] turns up again. I guess I'll take a crack at it...

--Sean



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2024-08-09 18:56 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-03 19:29 [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
2024-05-03 19:29 ` [PATCH v5 01/10] drm: zynqmp_kms: Fix AUX bus not getting unregistered Sean Anderson
2024-05-03 19:29 ` [PATCH v5 02/10] drm: zynqmp_dp: Rearrange zynqmp_dp for better padding Sean Anderson
2024-05-03 19:29 ` [PATCH v5 03/10] drm: zynqmp_dp: Don't delay work Sean Anderson
2024-05-03 19:29 ` [PATCH v5 04/10] drm: zynqmp_dp: Add locking Sean Anderson
2024-05-03 19:29 ` [PATCH v5 05/10] drm: zynqmp_dp: Don't retrain the link in our IRQ Sean Anderson
2024-05-03 19:29 ` [PATCH v5 06/10] drm: zynqmp_dp: Convert to a hard IRQ Sean Anderson
2024-05-03 19:29 ` [PATCH v5 07/10] drm: zynqmp_dp: Use AUX IRQs instead of polling Sean Anderson
2024-05-03 19:29 ` [PATCH v5 08/10] drm: zynqmp_dp: Split off several helper functions Sean Anderson
2024-05-03 19:29 ` [PATCH v5 09/10] drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func Sean Anderson
2024-05-03 19:29 ` [PATCH v5 10/10] drm: zynqmp_dp: Add debugfs interface for compliance testing Sean Anderson
2024-05-31 16:27 ` [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support Sean Anderson
2024-06-17  7:47 ` Tomi Valkeinen
2024-06-17 14:48   ` Sean Anderson
2024-08-08 12:46     ` Tomi Valkeinen
2024-08-09 18:55       ` Sean Anderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).