[1/2] drm/bridge: ps8640: Skip redundant bridge enable

  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(-)
  

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