| Message ID | 20230314110043.2139111-1-treapking@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/bridge: ps8640: Skip redundant bridge enable | expand |
On Tue, Mar 14, 2023 at 12:01 PM Pin-yen Lin <treapking@chromium.org> wrote: > > Skip the drm_bridge_chain_pre_enable call when the bridge is already > pre_enabled. This make pre_enable and post_disable (thus > pm_runtime_get/put) symmetric. > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > --- > > drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > index 4b361d7d5e44..08de501c436e 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > * EDID, for this chip, we need to do a full poweron, otherwise it will > * fail. > */ > - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > + if (poweroff) > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > edid = drm_get_edid(connector, > ps_bridge->page[PAGE0_DP_CNTL]->adapter); > -- > 2.40.0.rc1.284.g88254d51c5-goog > Reviewed-by: Robert Foss <rfoss@kernel.org>
Hi, On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote: > > Skip the drm_bridge_chain_pre_enable call when the bridge is already > pre_enabled. This make pre_enable and post_disable (thus > pm_runtime_get/put) symmetric. > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > --- > > drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > index 4b361d7d5e44..08de501c436e 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > * EDID, for this chip, we need to do a full poweron, otherwise it will > * fail. > */ > - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > + if (poweroff) > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); It always seemed weird to me that this function was asymmetric, so I like this change, thanks! I also remember wondering before how this function was safe, though. The callpath for getting here from the ioctl is documented in the function and when I look at it I wonder if anything is preventing the bridge from being enabled / disabled through normal means at the same time your function is running. That could cause all sorts of badness if it is indeed possible. Does anyone reading this know if that's indeed a problem? I suppose that, if this is unsafe, it's no more unsafe now than it was before your patch, so I guess: Reviewed-by: Douglas Anderson <dianders@chromium.org> If there are no issues, I'll plan to land this patch and the next one to drm-misc-next sometime late-ish next week.
Hi Doug, On Wed, Mar 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > Skip the drm_bridge_chain_pre_enable call when the bridge is already > > pre_enabled. This make pre_enable and post_disable (thus > > pm_runtime_get/put) symmetric. > > > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > --- > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > index 4b361d7d5e44..08de501c436e 100644 > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > > * EDID, for this chip, we need to do a full poweron, otherwise it will > > * fail. > > */ > > - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > + if (poweroff) > > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > It always seemed weird to me that this function was asymmetric, so I > like this change, thanks! > > I also remember wondering before how this function was safe, though. > The callpath for getting here from the ioctl is documented in the > function and when I look at it I wonder if anything is preventing the > bridge from being enabled / disabled through normal means at the same > time your function is running. That could cause all sorts of badness > if it is indeed possible. Does anyone reading this know if that's > indeed a problem? If the "normal mean" is disabling the bridge, then we are probably disabling the whole display pipeline. If so, is the EDID still relevant in this case? drm_get_edid returns NULL whenever error happens, and the helpers seem to handle this case properly. > > I suppose that, if this is unsafe, it's no more unsafe now than it was > before your patch, so I guess: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > If there are no issues, I'll plan to land this patch and the next one > to drm-misc-next sometime late-ish next week. Thanks for the review. I'll submit a v2 to collect the review tags and fix up the nit in patch 2/2. Best regards, Pin-yen
Hi, On Tue, Mar 14, 2023 at 8:28 PM Pin-yen Lin <treapking@chromium.org> wrote: > > Hi Doug, > > On Wed, Mar 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > > > Skip the drm_bridge_chain_pre_enable call when the bridge is already > > > pre_enabled. This make pre_enable and post_disable (thus > > > pm_runtime_get/put) symmetric. > > > > > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > > --- > > > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > > index 4b361d7d5e44..08de501c436e 100644 > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > > > * EDID, for this chip, we need to do a full poweron, otherwise it will > > > * fail. > > > */ > > > - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > > + if (poweroff) > > > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > > > It always seemed weird to me that this function was asymmetric, so I > > like this change, thanks! > > > > I also remember wondering before how this function was safe, though. > > The callpath for getting here from the ioctl is documented in the > > function and when I look at it I wonder if anything is preventing the > > bridge from being enabled / disabled through normal means at the same > > time your function is running. That could cause all sorts of badness > > if it is indeed possible. Does anyone reading this know if that's > > indeed a problem? > > If the "normal mean" is disabling the bridge, then we are probably > disabling the whole display pipeline. If so, is the EDID still > relevant in this case? In general when we do a "modeset" I believe that the display pipeline is disabled and re-enabled. On a Chromebook test image you can see this disable / re-enable happen when you switch between "VT2" and the main login screen. If the display pipeline is disabled / re-enabled then it should still be fine to keep the EDID cached, so that's not what I'm worried about. I'm more worried that someone could be querying the EDID at the same time that someone else was turning the screen off. In that case it would be possible for "poweroff" to be true (because the screen was on when we started reading the EDID) and then partway through the screen could get turned off. -Doug
Hi, On Thu, Mar 16, 2023 at 5:34 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Mar 14, 2023 at 8:28 PM Pin-yen Lin <treapking@chromium.org> wrote: > > > > Hi Doug, > > > > On Wed, Mar 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > > > > > Skip the drm_bridge_chain_pre_enable call when the bridge is already > > > > pre_enabled. This make pre_enable and post_disable (thus > > > > pm_runtime_get/put) symmetric. > > > > > > > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > > > --- > > > > > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > > > index 4b361d7d5e44..08de501c436e 100644 > > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > > > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > > > > * EDID, for this chip, we need to do a full poweron, otherwise it will > > > > * fail. > > > > */ > > > > - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > > > + if (poweroff) > > > > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > > > > > It always seemed weird to me that this function was asymmetric, so I > > > like this change, thanks! > > > > > > I also remember wondering before how this function was safe, though. > > > The callpath for getting here from the ioctl is documented in the > > > function and when I look at it I wonder if anything is preventing the > > > bridge from being enabled / disabled through normal means at the same > > > time your function is running. That could cause all sorts of badness > > > if it is indeed possible. Does anyone reading this know if that's > > > indeed a problem? > > > > If the "normal mean" is disabling the bridge, then we are probably > > disabling the whole display pipeline. If so, is the EDID still > > relevant in this case? > > In general when we do a "modeset" I believe that the display pipeline > is disabled and re-enabled. On a Chromebook test image you can see > this disable / re-enable happen when you switch between "VT2" and the > main login screen. > > If the display pipeline is disabled / re-enabled then it should still > be fine to keep the EDID cached, so that's not what I'm worried about. > I'm more worried that someone could be querying the EDID at the same > time that someone else was turning the screen off. In that case it > would be possible for "poweroff" to be true (because the screen was on You mean "poweroff" to be "false", right? That is, "ps_bridge->pre_enabled" is true. So the .get_edid function assumes that the pipeline is enabled, but another thread is turning off the screen. > when we started reading the EDID) and then partway through the screen > could get turned off. Thanks for the detailed explanation. In that case, we probably get an error and return a NULL EDID. But do we need the EDID when the screen is turned off? And the EDID should be re-read if the screen is turned back on. However, in a reversed setting, if the .get_edid is reading EDID when the pipeline is disabled (poweroff=true), but someone enables the pipeline in between. In that case, .get_edid might disable the bridge and panel after the pipeline is enabled. Anyway, the function is not safe, but it's no more unsafe than before. Patch 2/2 should lower the chance for anything bad to happen by adding a cache by only read EDID once. > > -Doug Thanks and regards, Pin-yen
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 4b361d7d5e44..08de501c436e 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, * EDID, for this chip, we need to do a full poweron, otherwise it will * fail. */ - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); + if (poweroff) + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); edid = drm_get_edid(connector, ps_bridge->page[PAGE0_DP_CNTL]->adapter);
Skip the drm_bridge_chain_pre_enable call when the bridge is already pre_enabled. This make pre_enable and post_disable (thus pm_runtime_get/put) symmetric. Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") Signed-off-by: Pin-yen Lin <treapking@chromium.org> --- drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)