diff mbox series

drm/bridge: anx7625: Ensure bridge is suspended in disable()

Message ID 20240118015916.2296741-1-hsinyi@chromium.org
State New
Headers
Series drm/bridge: anx7625: Ensure bridge is suspended in disable() |

Commit Message

Hsin-Yi Wang Jan. 18, 2024, 1:58 a.m. UTC 
  Similar to commit 26db46bc9c67 ("drm/bridge: parade-ps8640: Ensure bridge
is suspended in .post_disable()"). Add a mutex to ensure that aux transfer
won't race with atomic_disable by holding the PM reference and prevent
the bridge from suspend.

Also we need to use pm_runtime_put_sync_suspend() to suspend the bridge
instead of idle with pm_runtime_put_sync().

Fixes: 3203e497eb76 ("drm/bridge: anx7625: Synchronously run runtime suspend.")
Fixes: adca62ec370c ("drm/bridge: anx7625: Support reading edid through aux channel")
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Xuxin Xiong <xuxinxiong@huaqin.corp-partner.google.com>
---
 drivers/gpu/drm/bridge/analogix/anx7625.c | 7 ++++++-
 drivers/gpu/drm/bridge/analogix/anx7625.h | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

Comments

Pin-yen Lin Jan. 18, 2024, 4:05 a.m. UTC | #1 
Hi Hsin-Yi,

On Thu, Jan 18, 2024 at 9:59 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Similar to commit 26db46bc9c67 ("drm/bridge: parade-ps8640: Ensure bridge
> is suspended in .post_disable()"). Add a mutex to ensure that aux transfer
> won't race with atomic_disable by holding the PM reference and prevent
> the bridge from suspend.
>
> Also we need to use pm_runtime_put_sync_suspend() to suspend the bridge
> instead of idle with pm_runtime_put_sync().
>
> Fixes: 3203e497eb76 ("drm/bridge: anx7625: Synchronously run runtime suspend.")
> Fixes: adca62ec370c ("drm/bridge: anx7625: Support reading edid through aux channel")
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Tested-by: Xuxin Xiong <xuxinxiong@huaqin.corp-partner.google.com>
> ---
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 7 ++++++-
>  drivers/gpu/drm/bridge/analogix/anx7625.h | 2 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index ef31033439bc..29d91493b101 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -1762,6 +1762,7 @@ static ssize_t anx7625_aux_transfer(struct drm_dp_aux *aux,
>         u8 request = msg->request & ~DP_AUX_I2C_MOT;
>         int ret = 0;
>
> +       mutex_lock(&ctx->aux_lock);
>         pm_runtime_get_sync(dev);
>         msg->reply = 0;
>         switch (request) {
> @@ -1778,6 +1779,7 @@ static ssize_t anx7625_aux_transfer(struct drm_dp_aux *aux,
>                                         msg->size, msg->buffer);
>         pm_runtime_mark_last_busy(dev);
>         pm_runtime_put_autosuspend(dev);
> +       mutex_unlock(&ctx->aux_lock);
>
>         return ret;
>  }
> @@ -2474,7 +2476,9 @@ static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
>         ctx->connector = NULL;
>         anx7625_dp_stop(ctx);
>
> -       pm_runtime_put_sync(dev);
> +       mutex_lock(&ctx->aux_lock);
> +       pm_runtime_put_sync_suspend(dev);
> +       mutex_unlock(&ctx->aux_lock);
>  }
>
>  static enum drm_connector_status
> @@ -2668,6 +2672,7 @@ static int anx7625_i2c_probe(struct i2c_client *client)
>
>         mutex_init(&platform->lock);
>         mutex_init(&platform->hdcp_wq_lock);
> +       mutex_init(&platform->aux_lock);
>
>         INIT_DELAYED_WORK(&platform->hdcp_work, hdcp_check_work_func);
>         platform->hdcp_workqueue = create_workqueue("hdcp workqueue");
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index 66ebee7f3d83..39ed35d33836 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -475,6 +475,8 @@ struct anx7625_data {
>         struct workqueue_struct *hdcp_workqueue;
>         /* Lock for hdcp work queue */
>         struct mutex hdcp_wq_lock;
> +       /* Lock for aux transfer and disable */
> +       struct mutex aux_lock;
>         char edid_block;
>         struct display_timing dt;
>         u8 display_timing_valid;
> --
> 2.43.0.381.gb435a96ce8-goog
>

Reviewed-by: Pin-yen Lin <treapking@chromium.org>
Doug Anderson Jan. 18, 2024, 5:30 p.m. UTC | #2 
Hi,

On Wed, Jan 17, 2024 at 5:59 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Similar to commit 26db46bc9c67 ("drm/bridge: parade-ps8640: Ensure bridge
> is suspended in .post_disable()"). Add a mutex to ensure that aux transfer
> won't race with atomic_disable by holding the PM reference and prevent
> the bridge from suspend.
>
> Also we need to use pm_runtime_put_sync_suspend() to suspend the bridge
> instead of idle with pm_runtime_put_sync().
>
> Fixes: 3203e497eb76 ("drm/bridge: anx7625: Synchronously run runtime suspend.")
> Fixes: adca62ec370c ("drm/bridge: anx7625: Support reading edid through aux channel")
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Tested-by: Xuxin Xiong <xuxinxiong@huaqin.corp-partner.google.com>
> ---
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 7 ++++++-
>  drivers/gpu/drm/bridge/analogix/anx7625.h | 2 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)

This looks good to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

I guess this started showing up more because of commit 49ddab089611
("drm/panel-edp: use put_sync in unprepare"), right? Since that's a
recent commit then that means there's probably a little more urgency
in getting this landed. That being said, it feels like it would be
most legit to allow this to hang out on the list for a few days before
landing. I'll aim for early next week unless someone else has any
comments.

I guess we should see if we need to do something similar for
ti-sn65dsi86 too since I could imagine it having similar problems.
Doug Anderson Jan. 22, 2024, 5:01 p.m. UTC | #3 
Hi,

On Thu, Jan 18, 2024 at 9:30 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Jan 17, 2024 at 5:59 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > Similar to commit 26db46bc9c67 ("drm/bridge: parade-ps8640: Ensure bridge
> > is suspended in .post_disable()"). Add a mutex to ensure that aux transfer
> > won't race with atomic_disable by holding the PM reference and prevent
> > the bridge from suspend.
> >
> > Also we need to use pm_runtime_put_sync_suspend() to suspend the bridge
> > instead of idle with pm_runtime_put_sync().
> >
> > Fixes: 3203e497eb76 ("drm/bridge: anx7625: Synchronously run runtime suspend.")
> > Fixes: adca62ec370c ("drm/bridge: anx7625: Support reading edid through aux channel")
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > Tested-by: Xuxin Xiong <xuxinxiong@huaqin.corp-partner.google.com>
> > ---
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 7 ++++++-
> >  drivers/gpu/drm/bridge/analogix/anx7625.h | 2 ++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
>
> This looks good to me.
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> I guess this started showing up more because of commit 49ddab089611
> ("drm/panel-edp: use put_sync in unprepare"), right? Since that's a
> recent commit then that means there's probably a little more urgency
> in getting this landed. That being said, it feels like it would be
> most legit to allow this to hang out on the list for a few days before
> landing. I'll aim for early next week unless someone else has any
> comments.

Pushed to drm-misc-fixes.

4d5b7daa3c61 drm/bridge: anx7625: Ensure bridge is suspended in disable()


> I guess we should see if we need to do something similar for
> ti-sn65dsi86 too since I could imagine it having similar problems.

I tried this myself and I couldn't see any problems there. I also
spent some time thinking about it and it seemed fine. There should be
no fundamental reason why we'd have to necessarily power cycle the
bridge chip in this case. Presumably ps8640 needs it because the
embedded firmware on ps8640 is easy to confuse. I'm less familiar with
the anx bridge chip, but I could believe something similar is
happening there.

-Doug
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index ef31033439bc..29d91493b101 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -1762,6 +1762,7 @@  static ssize_t anx7625_aux_transfer(struct drm_dp_aux *aux,
 	u8 request = msg->request & ~DP_AUX_I2C_MOT;
 	int ret = 0;
 
+	mutex_lock(&ctx->aux_lock);
 	pm_runtime_get_sync(dev);
 	msg->reply = 0;
 	switch (request) {
@@ -1778,6 +1779,7 @@  static ssize_t anx7625_aux_transfer(struct drm_dp_aux *aux,
 					msg->size, msg->buffer);
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
+	mutex_unlock(&ctx->aux_lock);
 
 	return ret;
 }
@@ -2474,7 +2476,9 @@  static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
 	ctx->connector = NULL;
 	anx7625_dp_stop(ctx);
 
-	pm_runtime_put_sync(dev);
+	mutex_lock(&ctx->aux_lock);
+	pm_runtime_put_sync_suspend(dev);
+	mutex_unlock(&ctx->aux_lock);
 }
 
 static enum drm_connector_status
@@ -2668,6 +2672,7 @@  static int anx7625_i2c_probe(struct i2c_client *client)
 
 	mutex_init(&platform->lock);
 	mutex_init(&platform->hdcp_wq_lock);
+	mutex_init(&platform->aux_lock);
 
 	INIT_DELAYED_WORK(&platform->hdcp_work, hdcp_check_work_func);
 	platform->hdcp_workqueue = create_workqueue("hdcp workqueue");
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index 66ebee7f3d83..39ed35d33836 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -475,6 +475,8 @@  struct anx7625_data {
 	struct workqueue_struct *hdcp_workqueue;
 	/* Lock for hdcp work queue */
 	struct mutex hdcp_wq_lock;
+	/* Lock for aux transfer and disable */
+	struct mutex aux_lock;
 	char edid_block;
 	struct display_timing dt;
 	u8 display_timing_valid;