Date:   Wed, 27 Oct 2021 15:07:47 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Philip Chen <philipchen@...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,

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?

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?


> +       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.

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