| Message ID | 20240118015916.2296741-1-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: anx7625: Ensure bridge is suspended in disable() | expand |
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>
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.
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 --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;
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(-)