Hi,
On Thu, Mar 16, 2023 at 5:34 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Tue, Mar 14, 2023 at 8:28 PM Pin-yen Lin <treapking@xxxxxxxxxxxx> wrote:
> >
> > Hi Doug,
> >
> > On Wed, Mar 15, 2023 at 5:31 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > ---
> > > >
> > > > 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
