Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <1647452154-16361-7-git-send-email-quic_sbillaka@quicinc.com>
References: <1647452154-16361-1-git-send-email-quic_sbillaka@quicinc.com> <1647452154-16361-7-git-send-email-quic_sbillaka@quicinc.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Thu, 17 Mar 2022 21:19:11 -0400
Message-ID: <CAE-0n520pQKM7mFSE_00ER+F9RKUPrN+y4U8fmsxi7FoFMyOrA@mail.gmail.com>
Subject: Re: [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink interaction
To:     Sankeerth Billakanti <quic_sbillaka@quicinc.com>,
        devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
        freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     robdclark@gmail.com, seanpaul@chromium.org,
        quic_kalyant@quicinc.com, quic_abhinavk@quicinc.com,
        dianders@chromium.org, quic_khsieh@quicinc.com, agross@kernel.org,
        bjorn.andersson@linaro.org, robh+dt@kernel.org, krzk+dt@kernel.org,
        sean@poorly.run, airlied@linux.ie, daniel@ffwll.ch,
        thierry.reding@gmail.com, sam@ravnborg.org,
        dmitry.baryshkov@linaro.org, quic_vproddut@quicinc.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Sankeerth Billakanti (2022-03-16 10:35:51)
>         The source device should ensure the sink is ready before
> proceeding to read the sink capability or performing any aux transactions.
> The sink will indicate its readiness by asserting the HPD line.
>
>         The eDP sink requires power from the source and its HPD line will
> be asserted only after the panel is powered on. The panel power will be
> enabled from the panel-edp driver.
>
>         The controller driver needs to wait for the hpd line to be asserted
> by the sink.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c     |  6 ++++++
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 23 +++++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_catalog.h |  1 +
>  drivers/gpu/drm/msm/dp/dp_reg.h     |  7 ++++++-
>  4 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 6d36f63..2ddc303 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -337,6 +337,12 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>                 goto exit;
>         }
>
> +       ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);

Why are we making aux transactions when hpd isn't asserted? Can we only
register the aux device once we know that state is "connected"? I'm
concerned that we're going to be possibly polling the connected bit up
to some amount of time (0x0003FFFF usecs?) for each aux transfer when
that doesn't make any sense to keep checking. We should be able to check
it once, register aux, and then when disconnect happens we can
unregister aux.

> +       if (ret) {
> +               DRM_DEBUG_DP("DP sink not ready for aux transactions\n");
> +               goto exit;
> +       }
> +
>         dp_aux_update_offset_and_segment(aux, msg);
>         dp_aux_transfer_helper(aux, msg, true);
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index fac815f..2c3b0f7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog)
>         phy_calibrate(phy);
>  }
>
> +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
> +{
> +       u32 state, hpd_en, timeout;
> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
> +                               struct dp_catalog_private, dp_catalog);
> +
> +       hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL) &
> +                                       DP_DP_HPD_CTRL_HPD_EN;

Use two lines

	hpd_en = dp_read_aux();
	hpd_en &= DP_DP_HPD_CTRL_HPD_EN;

> +
> +       /* no-hpd case */
> +       if (!hpd_en)
> +               return 0;
> +
> +       /* Poll for HPD connected status */
> +       timeout = dp_read_aux(catalog, REG_DP_DP_HPD_EVENT_TIME_0) &
> +                                       DP_HPD_CONNECT_TIME_MASK;
> +
> +       return readl_poll_timeout(catalog->io->dp_controller.aux.base +
> +                               REG_DP_DP_HPD_INT_STATUS,
> +                               state, state & DP_DP_HPD_STATE_STATUS_CONNECTED,
> +                               2000, timeout);
> +}
> +
>  static void dump_regs(void __iomem *base, int len)
>  {
>         int i;