| Message ID | 20240109120528.1292601-1-treapking@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/bridge: parade-ps8640: Ensure bridge is suspended in .post_disable() | expand |
Hi, On Tue, Jan 9, 2024 at 4:05 AM Pin-yen Lin <treapking@chromium.org> wrote: > > The ps8640 bridge seems to expect everything to be power cycled at the > disable process, but sometimes ps8640_aux_transfer() holds the runtime > PM reference and prevents the bridge from suspend. > > Prevent that by introducing a mutex lock between ps8640_aux_transfer() > and .post_disable() to make sure the bridge is really powered off. > > Fixes: 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable runtime power management") > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > --- > > Changes in v2: > - Use mutex instead of the completion and autosuspend hack > > drivers/gpu/drm/bridge/parade-ps8640.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) This looks OK to me now. Reviewed-by: Douglas Anderson <dianders@chromium.org> I'll let it stew on the mailing list for ~1 week and then land it in drm-misc-fixes unless there are additional comments. -Doug
Hi, On Tue, Jan 9, 2024 at 8:52 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Jan 9, 2024 at 4:05 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > The ps8640 bridge seems to expect everything to be power cycled at the > > disable process, but sometimes ps8640_aux_transfer() holds the runtime > > PM reference and prevents the bridge from suspend. > > > > Prevent that by introducing a mutex lock between ps8640_aux_transfer() > > and .post_disable() to make sure the bridge is really powered off. > > > > Fixes: 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable runtime power management") > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > --- > > > > Changes in v2: > > - Use mutex instead of the completion and autosuspend hack > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > This looks OK to me now. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > I'll let it stew on the mailing list for ~1 week and then land it in > drm-misc-fixes unless there are additional comments. Pushed to drm-misc-fixes: 26db46bc9c67 drm/bridge: parade-ps8640: Ensure bridge is suspended in .post_disable()
Hi, On Wed, Jan 17, 2024 at 9:34 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Jan 9, 2024 at 8:52 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Tue, Jan 9, 2024 at 4:05 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > > > The ps8640 bridge seems to expect everything to be power cycled at the > > > disable process, but sometimes ps8640_aux_transfer() holds the runtime > > > PM reference and prevents the bridge from suspend. > > > > > > Prevent that by introducing a mutex lock between ps8640_aux_transfer() > > > and .post_disable() to make sure the bridge is really powered off. > > > > > > Fixes: 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable runtime power management") > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > > --- > > > > > > Changes in v2: > > > - Use mutex instead of the completion and autosuspend hack > > > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > This looks OK to me now. > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > > > I'll let it stew on the mailing list for ~1 week and then land it in > > drm-misc-fixes unless there are additional comments. > > Pushed to drm-misc-fixes: > > 26db46bc9c67 drm/bridge: parade-ps8640: Ensure bridge is suspended in > .post_disable() Crud. I landed this and then almost immediately hit a bug with it. :( I've posted up the fix: https://lore.kernel.org/r/20240117103502.1.Ib726a0184913925efc7e99c4d4fc801982e1bc24@changeid -Doug
Hi Doug, On Thu, Jan 18, 2024 at 2:37 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Jan 17, 2024 at 9:34 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Tue, Jan 9, 2024 at 8:52 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Tue, Jan 9, 2024 at 4:05 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > > > > > The ps8640 bridge seems to expect everything to be power cycled at the > > > > disable process, but sometimes ps8640_aux_transfer() holds the runtime > > > > PM reference and prevents the bridge from suspend. > > > > > > > > Prevent that by introducing a mutex lock between ps8640_aux_transfer() > > > > and .post_disable() to make sure the bridge is really powered off. > > > > > > > > Fixes: 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable runtime power management") > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > > > --- > > > > > > > > Changes in v2: > > > > - Use mutex instead of the completion and autosuspend hack > > > > > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 16 ++++++++++++++++ > > > > 1 file changed, 16 insertions(+) > > > > > > This looks OK to me now. > > > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > > > > > I'll let it stew on the mailing list for ~1 week and then land it in > > > drm-misc-fixes unless there are additional comments. > > > > Pushed to drm-misc-fixes: > > > > 26db46bc9c67 drm/bridge: parade-ps8640: Ensure bridge is suspended in > > .post_disable() > > Crud. I landed this and then almost immediately hit a bug with it. :( > I've posted up the fix: > > https://lore.kernel.org/r/20240117103502.1.Ib726a0184913925efc7e99c4d4fc801982e1bc24@changeid > > -Doug Sorry, I missed this because the patch was based on drm-misc-next, so it did not include commit 024b32db43a3 ("drm/bridge: parade-ps8640: Wait for HPD when doing an AUX transfer"). I also forgot to apply that commit when I did my testing. Pin-yen
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 8161b1a1a4b1..46c84ce66041 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -107,6 +107,7 @@ struct ps8640 { struct device_link *link; bool pre_enabled; bool need_post_hpd_delay; + struct mutex aux_lock; }; static const struct regmap_config ps8640_regmap_config[] = { @@ -344,10 +345,12 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev; int ret; + mutex_lock(&ps_bridge->aux_lock); pm_runtime_get_sync(dev); ret = ps8640_aux_transfer_msg(aux, msg); pm_runtime_mark_last_busy(dev); pm_runtime_put_autosuspend(dev); + mutex_unlock(&ps_bridge->aux_lock); return ret; } @@ -469,7 +472,18 @@ static void ps8640_atomic_post_disable(struct drm_bridge *bridge, ps_bridge->pre_enabled = false; ps8640_bridge_vdo_control(ps_bridge, DISABLE); + + /* + * The bridge seems to expect everything to be power cycled at the + * disable process, so grab a lock here to make sure + * ps8640_aux_transfer() is not holding a runtime PM reference and + * preventing the bridge from suspend. + */ + mutex_lock(&ps_bridge->aux_lock); + pm_runtime_put_sync_suspend(&ps_bridge->page[PAGE0_DP_CNTL]->dev); + + mutex_unlock(&ps_bridge->aux_lock); } static int ps8640_bridge_attach(struct drm_bridge *bridge, @@ -618,6 +632,8 @@ static int ps8640_probe(struct i2c_client *client) if (!ps_bridge) return -ENOMEM; + mutex_init(&ps_bridge->aux_lock); + ps_bridge->supplies[0].supply = "vdd12"; ps_bridge->supplies[1].supply = "vdd33"; ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ps_bridge->supplies),
The ps8640 bridge seems to expect everything to be power cycled at the disable process, but sometimes ps8640_aux_transfer() holds the runtime PM reference and prevents the bridge from suspend. Prevent that by introducing a mutex lock between ps8640_aux_transfer() and .post_disable() to make sure the bridge is really powered off. Fixes: 826cff3f7ebb ("drm/bridge: parade-ps8640: Enable runtime power management") Signed-off-by: Pin-yen Lin <treapking@chromium.org> --- Changes in v2: - Use mutex instead of the completion and autosuspend hack drivers/gpu/drm/bridge/parade-ps8640.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)