MIME-Version: 1.0
References: <20230314110043.2139111-1-treapking@chromium.org>
 <CAD=FV=W=jVsvD620wWKfniRYQNJnb7goDUKb_HhL_qVxLYGZOA@mail.gmail.com> <CAEXTbpe6EyukjKfgaVtHdMK2Ppw715kUUnOqvFa+tEX913p9aQ@mail.gmail.com>
In-Reply-To: <CAEXTbpe6EyukjKfgaVtHdMK2Ppw715kUUnOqvFa+tEX913p9aQ@mail.gmail.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Wed, 15 Mar 2023 14:33:51 -0700
Message-ID: <CAD=FV=VLh37hFpLwyuPoGNDCpvVL7FGLySVp7d1W788YkjNYog@mail.gmail.com>
Subject: Re: [PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable
To:     Pin-yen Lin <treapking@chromium.org>
Cc:     Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Robert Foss <rfoss@kernel.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Tue, Mar 14, 2023 at 8:28=E2=80=AFPM Pin-yen Lin <treapking@chromium.org=
> wrote:
>
> Hi Doug,
>
> On Wed, Mar 15, 2023 at 5:31=E2=80=AFAM Doug Anderson <dianders@chromium.=
org> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 14, 2023 at 4:00=E2=80=AFAM 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, otherwi=
se 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