Message-ID: <CAD=FV=XWYFDMYvZJWDnV2Yc+5X=NThYnE78aOeMrhrEvBiDLkQ@mail.gmail.com>
Date:   Thu, 19 May 2022 17:23:26 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     dri-devel <dri-devel@...ts.freedesktop.org>
Cc:     Robert Foss <robert.foss@...aro.org>,
        Lyude Paul <lyude@...hat.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Maxime Ripard <maxime@...no.tech>,
        freedreno <freedreno@...ts.freedesktop.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Jani Nikula <jani.nikula@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drm: Document the power requirements for DP AUX transfers

Hi,

On Mon, May 9, 2022 at 4:18 PM Douglas Anderson <dianders@...omium.org> wrote:
>
> When doing DP AUX transfers there are two actors that need to be
> powered in order for the DP AUX transfer to work: the DP source and
> the DP sink. Commit bacbab58f09d ("drm: Mention the power state
> requirement on side-channel operations") added some documentation
> saying that the DP source is required to power itself up (if needed)
> to do AUX transfers. However, that commit doesn't talk anything about
> the DP sink.
>
> For full fledged DP the sink isn't really a problem. It's expected
> that if an external DP monitor isn't plugged in that attempting to do
> AUX transfers won't work. It's also expected that if a DP monitor is
> plugged in (and thus asserting HPD) then AUX transfers will work.
>
> When we're looking at eDP, however, things are less obvious. Let's add
> some documentation about expectations. Here's what we'll say:
>
> 1. We don't expect the DP AUX transfer function to power on an eDP
> panel. If an eDP panel is physically connected but powered off then it
> makes sense for the transfer to fail.
>
> 2. We'll document that the official way to power on a panel is via the
> bridge chain, specifically by making sure that the panel's prepare
> function has been called (which is called by
> panel_bridge_pre_enable()). It's already specified in the kernel doc
> of drm_panel_prepare() that this is the way to power the panel on and
> also that after this call "it is possible to communicate with any
> integrated circuitry via a command bus."
>
> 3. We'll also document that for code running in the panel driver
> itself that it is legal for the panel driver to power itself up
> however it wants (it doesn't need to officially call
> drm_panel_pre_enable()) and then it can do AUX bus transfers. This is
> currently the way that edp-panel works when it's running atop the DP
> AUX bus.
>
> NOTE: there was much discussion of all of this in response to v1 [1]
> of this patch. A summary of that is:
> * With the Intel i195 driver, apparently eDP panels do get powered
>   up. We won't forbid this but it is expected that code that wants to
>   run on a variety of platforms should ensure that the drm_panel's
>   prepare() function has been called.
> * There is at least a reasonable amount of agreement that the
>   transfer() functions itself shouldn't be responsible for powering
>   the panel. It's proposed that if we need the DP AUX dev nodes to be
>   robust for eDP that the code handling the DP AUX dev nodes could
>   handle powering the panel by ensuring that the panel's prepare()
>   call was made. Potentially drm_dp_aux_dev_get_by_minor() could be a
>   good place to do this. This is left as a future exercise. Until
>   that's fixed the DP AUX dev nodes for eDP are probably best just
>   used for debugging.
> * If a panel could be in PSR and DP AUX via the dev node needs to be
>   reliable then we need to be able to pull the panel out of PSR. On
>   i915 this is also apparently handled as part of the transfer()
>   function.
>
> [1] https://lore.kernel.org/r/20220503162033.1.Ia8651894026707e4fa61267da944ff739610d180@changeid
>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> Reviewed-by: Lyude Paul <lyude@...hat.com>
> ---
> Hopefully I've resolved everything here to everyone's
> satisfaction. There's no crazy hurry here. I'll give this a week or so
> and then land it on drm-misc if there is no additional discussion.
>
> Changes in v2:
> - Updated wording as per discussion on v1.
> - Updated commit message as per discussion on v1.
> - Add pointer to v1 discussion for future reference.
>
>  include/drm/display/drm_dp_helper.h | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)

Pushed to drm-misc-next:

69ef4a192bba drm: Document the power requirements for DP AUX transfers

Powered by blists - more mailing lists