Received-SPF: pass (google.com: domain of linux-kernel+bounces-102500-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <d6ac0328-3d3d-75bc-09b9-ed0190a6a8c5@quicinc.com>
Date: Wed, 13 Mar 2024 13:41:16 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if
 nothing connected
Content-Language: en-US
To: Douglas Anderson <dianders@chromium.org>, Rob Clark <robdclark@gmail.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: Bjorn Andersson <andersson@kernel.org>, Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@gmail.com>, Guenter Roeck <groeck@chromium.org>,
        "Kuogee Hsieh" <quic_khsieh@quicinc.com>,
        Marijn Suijten
	<marijn.suijten@somainline.org>,
        Sean Paul <sean@poorly.run>, Tanmay Shah
	<tanmay@codeaurora.org>,
        Vinod Polimera <quic_vpolimer@quicinc.com>,
        <dri-devel@lists.freedesktop.org>, <freedreno@lists.freedesktop.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240313001345.2623074-1-dianders@chromium.org>
 <20240312171305.1.I16aff881c9fe82b5e0fc06ca312da017aa7b5b3e@changeid>
From: Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <20240312171305.1.I16aff881c9fe82b5e0fc06ca312da017aa7b5b3e@changeid>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 3/12/2024 5:13 PM, Douglas Anderson wrote:
> As documented in the description of the transfer() function of
> "struct drm_dp_aux", the transfer() function can be called at any time
> regardless of the state of the DP port. Specifically if the kernel has
> the DP AUX character device enabled and userspace accesses
> "/dev/drm_dp_auxN" directly then the AUX transfer function will be
> called regardless of whether a DP device is connected.
> 

I do see

"
* Also note that this callback can be called no matter the
* state @dev is in and also no matter what state the panel is
* in. It's expected:
"

I understand about the host state that we need to allow the transfers by 
powering on if the host was off.

But I wonder why we should allow the transfer if the sink is not 
connected because it will anyway timeout.

Does it make sense to have get_hpd_status() from the aux dev and not 
issue the transfers if the sink was not connected?

This is more of questioning the intent of drm_dp_helpers to allow 
transfers without checking the sink status.

> For eDP panels we have a special rule where we wait (with a 5 second
> timeout) for HPD to go high. This rule was important before all panels
> drivers were converted to call wait_hpd_asserted() and actually can be
> removed in a future commit.
> 
> For external DP devices we never checked for HPD. That means that
> trying to access the DP AUX character device (AKA `hexdump -C
> /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my
> system:
>    $ time hexdump -C /dev/drm_dp_aux0
>    hexdump: /dev/drm_dp_aux0: Connection timed out
> 
>    real    0m8.200s
> 

IIUC, we want to timeout faster by not bailing out if not connected right?


> Let's add a check for HPD to avoid the slow timeout. This matches
> what, for instance, the intel_dp_aux_xfer() function does when it
> calls intel_tc_port_connected_locked(). That call has a document by it
> explaining that it's important to avoid the long timeouts.
> 
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/gpu/drm/msm/dp/dp_aux.c     |  8 +++++++-
>   drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++++++++++
>   drivers/gpu/drm/msm/dp/dp_catalog.h |  1 +
>   3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 03f4951c49f4..de0b0eabced9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>   	 * turned on the panel and then tried to do an AUX transfer. The panel
>   	 * driver has no way of knowing when the panel is ready, so it's up
>   	 * to us to wait. For DP we never get into this situation so let's
> -	 * avoid ever doing the extra long wait for DP.
> +	 * avoid ever doing the extra long wait for DP and just query HPD
> +	 * directly.
>   	 */
>   	if (aux->is_edp) {
>   		ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> @@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>   			DRM_DEBUG_DP("Panel not ready for aux transactions\n");
>   			goto exit;
>   		}
> +	} else {
> +		if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) {
> +			ret = -ENXIO;
> +			goto exit;
> +		}
>   	}
>   
>   	dp_aux_update_offset_and_segment(aux, msg);
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 5142aeb705a4..93e2d413a1e7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -266,6 +266,16 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
>   				2000, 500000);
>   }
>   
> +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog)
> +{
> +	struct dp_catalog_private *catalog = container_of(dp_catalog,
> +				struct dp_catalog_private, dp_catalog);
> +
> +	/* poll for hpd connected status every 2ms and timeout after 500ms */
> +	return readl(catalog->io->dp_controller.aux.base + REG_DP_DP_HPD_INT_STATUS) &
> +	       DP_DP_HPD_STATE_STATUS_CONNECTED;
> +}

This method of checking HPD status works for devices which use internal 
HPD block to control the HPD (like sc7180/sc7280) but not for devices 
where HPD is controlled outside the MSM DP controller like sc8280xp, 
sc835-/sm8450 etc etc which use pmic_glink and DP driver only receives 
the hpd status using the dp_bridge_hpd_notify() callback.

If we want to make this generic, we have to do something like:

dp_hpd_unplug_handle() notifies the dp_aux.c module that status is 
disconncted and we should bail out

dp_hpd_plug_handle() notifies dp_aux.c module that status is connected 
again and we allow the aux transfers.

> +
>   static void dump_regs(void __iomem *base, int len)
>   {
>   	int i;
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 38786e855b51..1694040c530f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -86,6 +86,7 @@ void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
>   void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
>   void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
>   int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
> +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog);
>   u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
>   
>   /* DP Controller APIs */