[PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

Sean Anderson
2024-03-19T22:51:48Z

Retraining the link can take a while, and might involve waiting for
DPCD reads/writes to complete. This is inappropriate for an IRQ handler.
Just schedule this work for later completion. This is racy, but will be
fixed in the next commit.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---
Actually, on second look this IRQ is threaded. So why do we have a
workqueue for HPD events? Maybe we should make it unthreaded?

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

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

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index f1834c8e3c02..f3fcdbf662fa 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -287,6 +287,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
@@ -298,6 +299,7 @@ struct zynqmp_dp {
struct drm_dp_aux aux;
struct drm_bridge bridge;
struct delayed_work hpd_work;
+ struct delayed_work hpd_irq_work;

struct drm_bridge *next_bridge;
struct device *dev;
@@ -1611,6 +1613,27 @@ 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;
+
+ 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);
+ }
+ }
+}
+
static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
{
struct zynqmp_dp *dp = (struct zynqmp_dp *)data;
@@ -1635,23 +1658,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;
}

@@ -1676,6 +1685,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
dp->status = connector_status_disconnected;

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 +1785,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);
--
2.35.1.1320.gc452695387.dirty