Message-ID: <CA+cxXh=pxw7iPnmfs998fd=OZ1QHDRtc913GFOXpLzKF8ZkOeA@mail.gmail.com>
Date:   Wed, 27 Oct 2021 15:37:59 -0700
From:   Philip Chen <philipchen@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Jonas Karlman <jonas@...boo.se>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Robert Foss <robert.foss@...aro.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v4 1/2] drm/bridge: parade-ps8640: Enable runtime power management

Hi Doug,

On Wed, Oct 27, 2021 at 3:08 PM Doug Anderson <dianders@...omium.org> wrote:
>
> Hi,
>
> On Tue, Oct 26, 2021 at 2:56 PM Philip Chen <philipchen@...omium.org> wrote:
> >
> > Fit ps8640 driver into runtime power management framework:
> >
> > First, break _poweron() to 3 parts: (1) turn on power and wait for
> > ps8640's internal MCU to finish init (2) check panel HPD (which is
> > proxied by GPIO9) (3) the other configs. As runtime_resume() can be
> > called before panel is powered, we only add (1) to _resume() and leave
> > (2)(3) to _pre_enable(). We also add (2) to _aux_transfer() as we want
> > to ensure panel HPD is asserted before we start AUX CH transactions.
> >
> > Second, the original driver has a mysterious delay of 50 ms between (2)
> > and (3). Since Parade's support can't explain what the delay is for,
> > and we don't see removing the delay break any boards at hand, remove
> > the delay to fit into this driver change.
> >
> > In addition, rename "powered" to "pre_enabled" and don't check for it
> > in the pm_runtime calls. The pm_runtime calls are already refcounted
> > so there's no reason to check there. The other user of "powered",
> > _get_edid(), only cares if pre_enable() has already been called.
> >
> > Lastly, change some existing DRM_...() logging to dev_...() along the
> > way, since DRM_...() seem to be deprecated in [1].
> >
> > [1] https://patchwork.freedesktop.org/patch/454760/
> >
> > Signed-off-by: Philip Chen <philipchen@...omium.org>
> > Reviewed-by: Douglas Anderson <dianders@...omium.org>
> > Reviewed-by: Stephen Boyd <swboyd@...omium.org>
> > ---
> > In v3, I also added pm_suspend_ignore_children() in the ps8640_probe()
> > but forgot to mention that in the v3 change log.
> >
> > Changes in v4:
> > - Make ps8640_ensure_hpd() return int (This change was mis-added to
> >   another patch [2] in this patch series:
> >   [2] https://patchwork.kernel.org/project/dri-devel/patch/20211026121058.v3.2.I09899dea340f11feab97d719cb4b62bef3179e4b@changeid/)
> >
> > Changes in v3:
> > - Fix typo/wording in the commit message.
> > - Add ps8640_aux_transfer_msg() for AUX operation. In
> >   ps8640_aux_transfer(), wrap around ps8640_aux_transfer_msg()
> >   with PM operations and HPD check.
> > - Document why autosuspend_delay is set to 500ms.
> >
> >  drivers/gpu/drm/bridge/parade-ps8640.c | 186 +++++++++++++++----------
> >  1 file changed, 115 insertions(+), 71 deletions(-)
>
> Unfortunately, your patch no longer applies to drm-misc/drm-misc-next.
> Commit 7abbc26fd667 ("drm/bridge: ps8640: Register and attach our DSI
> device at probe") landed and that causes a merge conflict. Can you
> rebase and resend?
Yes, I will rebase and resend.

>
> This will also cause a conflict when Sam's change lands [1] so I guess
> we can see whose lands first. Let me review that now and maybe you add
> a Tested-by? If it lands that'll make it easier and you can just
> rebase on both of them?
As per your latest reply, I'll just rebase atop drm-misc-next for now.

>
>
> > +       pm_runtime_enable(dev);
> > +       /*
> > +        * In practice, ps8640_aux_transfer_msg() takes ~300ms to complete in
> > +        * the worst case. Set autosuspend_delay to 500ms.
> > +        */
> > +       pm_runtime_set_autosuspend_delay(dev, 500);
>
> To be a little nitpicky, this makes it sound as if the 500 ms needs to
> be greater than the 300 ms for correctness. That's not _really_ the
> case. During the whole ps8640_aux_transfer_msg() we're holding a PM
> Runtime reference (so it won't autosuspend no matter what) and at the
> end of it we mark that we were busy so the timer won't start ticking
> until then.
Yeah....sorry, looking again, I agree the comment I added in v3 is
pretty misleading.
I think autosuspend_delay just needs to be consistently longer than
the interval between each aux_transfer call during EDID read.
I'll measure the interval and add the number to the comment.

>
> Really the 500 ms is because we're quite slow to power up (~300 ms)
> and so we want to avoid powering down and powering up constantly. We
> definitely need to avoid a power down / power up when reading the EDID
> and an EDID read involves _several_ calls in a row to the AUX transfer
> function. We also expect that after userspace reads the EDID it will
> call us again "soon" to turn the power on and it's nice to not have to
> wait the 300 ms again. The 500 ms here is really just a number that's
> not too short but not too long. I suppose it's roughly related to the
> 300 ms because that's the delay we're trying to avoid, but otherwise
> they are unrelated. NOTE: the code will still be _correct_ if we miss
> the 500 ms window, it'll just be a lot slower.
>
> [1] https://lore.kernel.org/r/20211020181901.2114645-2-sam@ravnborg.org
>
> -Doug

Powered by blists - more mailing lists