From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7A3AAC54E5D for ; Mon, 18 Mar 2024 17:59:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=/cwEj+YmH9RMhf36Ktc73ktNsshiuxt1791GpKixqwU=; b=KX3zoUbKR+oTDX 0GPtntrp3PU7xU8T1PR8Zz3JrawgRjnYvpnKuCZd4mIbODtBp9vyHzD82KIECRdTRi3etW6bWHlNg qSoHf8lMTMhmWmkBWWIL/DnRWXP5o4P5dZYXqw7hoDhymnlt1CwYqU9QlHNI0m5K0DFMBYIshR+MA fhjAbxNV/VdyrhfWlT33wmJG7jyUiHxEykT6+y1Jjt4A6rF7tMi6627tnKBKj4nisw+uX0WvtsMQC oINxu9oEp0vJIVLEr35sHJnmSp1ToSRDozim6QB92Ap0oIhRX9m4MRV79LgDwxBxUy5FGhJ02SS4E Q8IHRr4JjaP1SlsWzryA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rmHGk-00000009ZKX-1A8y; Mon, 18 Mar 2024 17:59:30 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rmHGg-00000009ZJ8-0uT3 for linux-arm-kernel@lists.infradead.org; Mon, 18 Mar 2024 17:59:29 +0000 Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 2FE4B2A5; Mon, 18 Mar 2024 18:58:58 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1710784738; bh=29QokDbK+YqlwiKi4ja1azJmClsj4cM4JBPFC/xeDRA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=XgFTJmjw1Srm9ULwtzrbvSu94HK0TKPDNvWp3i2+wriDBqHZp7Pdy+Qh3XioYriD+ DLhxL7CdQr5bG/Qf7/rP5cB/FaVK8LWLq2OpvUw0OYvlvTbrzZRfNpfwhHIiheQHLd PmkAIvC+365fABggQtEmx5G07ipMEP7ZrWhYBg/g= Date: Mon, 18 Mar 2024 19:59:21 +0200 From: Laurent Pinchart To: Sean Anderson Cc: Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , dri-devel@lists.freedesktop.org, David Airlie , linux-kernel@vger.kernel.org, Michal Simek , linux-arm-kernel@lists.infradead.org, Daniel Vetter Subject: Re: [PATCH 3/6] drm: zynqmp_dp: Add locking Message-ID: <20240318175921.GO13682@pendragon.ideasonboard.com> References: <20240315230916.1759060-1-sean.anderson@linux.dev> <20240315230916.1759060-4-sean.anderson@linux.dev> <20240318171651.GJ13682@pendragon.ideasonboard.com> <7e8e5e8e-ad50-4a6a-ac47-7fb1536a9df8@linux.dev> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <7e8e5e8e-ad50-4a6a-ac47-7fb1536a9df8@linux.dev> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240318_105927_346892_9AE4099B X-CRM114-Status: GOOD ( 42.30 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > >> --- > >> > >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id F2EB4C54E58 for ; Mon, 18 Mar 2024 17:59:26 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2BAB410E705; Mon, 18 Mar 2024 17:59:26 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="XgFTJmjw"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by gabe.freedesktop.org (Postfix) with ESMTPS id 965B410F4D9 for ; Mon, 18 Mar 2024 17:59:25 +0000 (UTC) Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 2FE4B2A5; Mon, 18 Mar 2024 18:58:58 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1710784738; bh=29QokDbK+YqlwiKi4ja1azJmClsj4cM4JBPFC/xeDRA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=XgFTJmjw1Srm9ULwtzrbvSu94HK0TKPDNvWp3i2+wriDBqHZp7Pdy+Qh3XioYriD+ DLhxL7CdQr5bG/Qf7/rP5cB/FaVK8LWLq2OpvUw0OYvlvTbrzZRfNpfwhHIiheQHLd PmkAIvC+365fABggQtEmx5G07ipMEP7ZrWhYBg/g= Date: Mon, 18 Mar 2024 19:59:21 +0200 From: Laurent Pinchart To: Sean Anderson Cc: Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , dri-devel@lists.freedesktop.org, David Airlie , linux-kernel@vger.kernel.org, Michal Simek , linux-arm-kernel@lists.infradead.org, Daniel Vetter Subject: Re: [PATCH 3/6] drm: zynqmp_dp: Add locking Message-ID: <20240318175921.GO13682@pendragon.ideasonboard.com> References: <20240315230916.1759060-1-sean.anderson@linux.dev> <20240315230916.1759060-4-sean.anderson@linux.dev> <20240318171651.GJ13682@pendragon.ideasonboard.com> <7e8e5e8e-ad50-4a6a-ac47-7fb1536a9df8@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <7e8e5e8e-ad50-4a6a-ac47-7fb1536a9df8@linux.dev> X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 > >> --- > >> > >> 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