If the display is not enable()d, then we aren't holding a runtime PM reference here. Thus, it's easy to accidentally cause a hang, if user space is poking around at /dev/drm_dp_aux0 at the "wrong" time.
Let's get a runtime PM reference, and check that we "see" the panel. Don't force any panel power-up, etc., because that can be intrusive, and that's not what other drivers do (see drivers/gpu/drm/bridge/ti-sn65dsi86.c and drivers/gpu/drm/bridge/parade-ps8640.c.)
Fixes: 0d97ad03f422 ("drm/bridge: analogix_dp: Remove duplicated code") Cc: stable@vger.kernel.org Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Signed-off-by: Brian Norris briannorris@chromium.org Reviewed-by: Douglas Anderson dianders@chromium.org ---
Changes in v4: - Add Doug's Reviewed-by
Changes in v3: - Avoid panel power-up; just check for HPD state, and let the rest happen "as-is" (e.g., time out, if the caller hasn't prepared things properly)
Changes in v2: - Fix spelling in Subject - DRM_DEV_ERROR() -> drm_err() - Propagate errors from un-analogix_dp_prepare_panel()
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index b7d2e4449cfa..16be279aed2c 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1632,8 +1632,19 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) { struct analogix_dp_device *dp = to_dp(aux); + int ret; + + pm_runtime_get_sync(dp->dev); + + ret = analogix_dp_detect_hpd(dp); + if (ret) + goto out;
- return analogix_dp_transfer(dp, msg); + ret = analogix_dp_transfer(dp, msg); +out: + pm_runtime_put(dp->dev); + + return ret; }
struct analogix_dp_device *
DP AUX transactions can consist of many short operations. There's no need to power things up/down in short intervals.
I pick an arbitrary 100ms; for the systems I'm testing (Rockchip RK3399), runtime-PM transitions only take a few microseconds.
Signed-off-by: Brian Norris briannorris@chromium.org ---
Changes in v4: - call pm_runtime_mark_last_busy() and pm_runtime_dont_use_autosuspend() - drop excess pm references around drm_get_edid(), now that we grab and hold in the dp-aux helper
Changes in v3: - New in v3
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index 16be279aed2c..b248d352f2bd 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1119,9 +1119,7 @@ static int analogix_dp_get_modes(struct drm_connector *connector) return 0; }
- pm_runtime_get_sync(dp->dev); edid = drm_get_edid(connector, &dp->aux.ddc); - pm_runtime_put(dp->dev); if (edid) { drm_connector_update_edid_property(&dp->connector, edid); @@ -1642,7 +1640,8 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux,
ret = analogix_dp_transfer(dp, msg); out: - pm_runtime_put(dp->dev); + pm_runtime_mark_last_busy(dp->dev); + pm_runtime_put_autosuspend(dp->dev);
return ret; } @@ -1775,6 +1774,8 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev) if (ret) return ret;
+ pm_runtime_use_autosuspend(dp->dev); + pm_runtime_set_autosuspend_delay(dp->dev, 100); pm_runtime_enable(dp->dev);
ret = analogix_dp_create_bridge(drm_dev, dp); @@ -1786,6 +1787,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev) return 0;
err_disable_pm_runtime: + pm_runtime_dont_use_autosuspend(dp->dev); pm_runtime_disable(dp->dev); drm_dp_aux_unregister(&dp->aux);
@@ -1804,6 +1806,7 @@ void analogix_dp_unbind(struct analogix_dp_device *dp) }
drm_dp_aux_unregister(&dp->aux); + pm_runtime_dont_use_autosuspend(dp->dev); pm_runtime_disable(dp->dev); } EXPORT_SYMBOL_GPL(analogix_dp_unbind);
Hi,
On Tue, Mar 1, 2022 at 6:11 PM Brian Norris briannorris@chromium.org wrote:
DP AUX transactions can consist of many short operations. There's no need to power things up/down in short intervals.
I pick an arbitrary 100ms; for the systems I'm testing (Rockchip RK3399), runtime-PM transitions only take a few microseconds.
Signed-off-by: Brian Norris briannorris@chromium.org
Changes in v4:
- call pm_runtime_mark_last_busy() and pm_runtime_dont_use_autosuspend()
- drop excess pm references around drm_get_edid(), now that we grab and hold in the dp-aux helper
Changes in v3:
- New in v3
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
This looks great to me now, thanks.
Reviewed-by: Douglas Anderson dianders@chromium.org
Though I'm not a massive expert on the Analogix DP driver, I'm pretty confident about the DP AUX stuff that Brian is touching. I just checked and I see that this driver isn't changing lots and the last change landed in drm-misc, which means that I can commit this. Thus, unless someone else shouts, I'll plan to wait until next week and commit these two patches to drm-misc.
The first of the two patches is a "Fix" but since it's been broken since 2016 I'll assume that nobody is chomping at the bit for these to get into stable and that it would be easier to land both in "drm-misc-next". Please yell if someone disagrees.
-Doug
Hi,
On Thu, Mar 3, 2022 at 3:02 PM Doug Anderson dianders@chromium.org wrote:
Hi,
On Tue, Mar 1, 2022 at 6:11 PM Brian Norris briannorris@chromium.org wrote:
DP AUX transactions can consist of many short operations. There's no need to power things up/down in short intervals.
I pick an arbitrary 100ms; for the systems I'm testing (Rockchip RK3399), runtime-PM transitions only take a few microseconds.
Signed-off-by: Brian Norris briannorris@chromium.org
Changes in v4:
- call pm_runtime_mark_last_busy() and pm_runtime_dont_use_autosuspend()
- drop excess pm references around drm_get_edid(), now that we grab and hold in the dp-aux helper
Changes in v3:
- New in v3
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
This looks great to me now, thanks.
Reviewed-by: Douglas Anderson dianders@chromium.org
Though I'm not a massive expert on the Analogix DP driver, I'm pretty confident about the DP AUX stuff that Brian is touching. I just checked and I see that this driver isn't changing lots and the last change landed in drm-misc, which means that I can commit this. Thus, unless someone else shouts, I'll plan to wait until next week and commit these two patches to drm-misc.
The first of the two patches is a "Fix" but since it's been broken since 2016 I'll assume that nobody is chomping at the bit for these to get into stable and that it would be easier to land both in "drm-misc-next". Please yell if someone disagrees.
Pushed both to drm-misc-next:
f28dd5075675 drm/bridge: analogix_dp: Enable autosuspend 8fb6c44fe846 drm/bridge: analogix_dp: Grab runtime PM reference for DP-AUX
-Doug
dri-devel@lists.freedesktop.org
-
Brian Norris -
Doug Anderson