Received-SPF: pass (google.com: domain of linux-kernel+bounces-106850-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <e2319b19-d999-24e7-48fa-bbc19bbfbeea@quicinc.com>
Date: Mon, 18 Mar 2024 17:19:19 -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 v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD
 in aux transfer
Content-Language: en-US
To: Douglas Anderson <dianders@chromium.org>, Rob Clark <robdclark@gmail.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@gmail.com>,
        "Kuogee
 Hsieh" <quic_khsieh@quicinc.com>,
        Marijn Suijten
	<marijn.suijten@somainline.org>,
        Sean Paul <sean@poorly.run>, <dri-devel@lists.freedesktop.org>,
        <freedreno@lists.freedesktop.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        "Bjorn
 Andersson" <quic_bjorande@quicinc.com>,
        Johan Hovold <johan@kernel.org>
References: <20240315213717.1411017-1-dianders@chromium.org>
 <20240315143621.v2.3.I535606f6d4f7e3e5588bb75c55996f61980183cd@changeid>
From: Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <20240315143621.v2.3.I535606f6d4f7e3e5588bb75c55996f61980183cd@changeid>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

+bjorn, johan as fyi for sc8280xp

On 3/15/2024 2:36 PM, Douglas Anderson wrote:
> Before the introduction of the wait_hpd_asserted() callback in commit
> 841d742f094e ("drm/dp: Add wait_hpd_asserted() callback to struct
> drm_dp_aux") the API between panel drivers and DP AUX bus drivers was
> that it was up to the AUX bus driver to wait for HPD in the transfer()
> function.
> 
> Now wait_hpd_asserted() has been added. The two panel drivers that are
> DP AUX endpoints use it. See commit 2327b13d6c47 ("drm/panel-edp: Take
> advantage of wait_hpd_asserted() in struct drm_dp_aux") and commit
> 3b5765df375c ("drm/panel: atna33xc20: Take advantage of
> wait_hpd_asserted() in struct drm_dp_aux"). We've implemented
> wait_hpd_asserted() in the MSM DP driver as of commit e2969ee30252
> ("drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()"). There is
> no longer any reason for long wait in the AUX transfer() function.
> Remove it.
> 
> NOTE: the wait_hpd_asserted() is listed as "optional". That means it's
> optional for the DP AUX bus to implement. In the case of the MSM DP
> driver we implement it so we can assume it will be called.
> 

How do we enforce that for any new edp panels to be used with MSM, the 
panels should atleast invoke wait_hpd_asserted()?

I agree that since MSM implements it, even though its listed as 
optional, we can drop this additional wait. So nothing wrong with this 
patch for current users including sc8280xp, sc7280 and sc7180.

But, does there need to be some documentation that the edp panels not 
using the panel-edp framework should still invoke wait_hpd_asserted()?

Since its marked as optional, what happens if the edp panel driver, 
skips calling wait_hpd_asserted()?

Now, since the wait from MSM is removed, it has a potential to fail.

> ALSO NOTE: the wait wasn't actually _hurting_ anything and wasn't even
> causing long timeouts, but it's still nice to get rid of unneeded
> code. Specificaly it's not truly needed because to handle other DP
> drivers that can't power on as quickly (specifically parade-ps8640) we
> already avoid DP AUX transfers for eDP panels that aren't powered
> on. See commit 8df1ddb5bf11 ("drm/dp: Don't attempt AUX transfers when
> eDP panels are not powered").
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> (no changes since v1)
> 
>   drivers/gpu/drm/msm/dp/dp_aux.c | 17 -----------------
>   1 file changed, 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 75c51f3ee106..ecefd1922d6d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -313,23 +313,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>   		goto exit;
>   	}
>   
> -	/*
> -	 * For eDP it's important to give a reasonably long wait here for HPD
> -	 * to be asserted. This is because the panel driver may have _just_
> -	 * 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.
> -	 */
> -	if (aux->is_edp) {
> -		ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog,
> -								500000);
> -		if (ret) {
> -			DRM_DEBUG_DP("Panel not ready for aux transactions\n");
> -			goto exit;
> -		}
> -	}
> -
>   	dp_aux_update_offset_and_segment(aux, msg);
>   	dp_aux_transfer_helper(aux, msg, true);
>