Received-SPF: pass (google.com: domain of linux-kernel+bounces-108016-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <591d1c33-df3d-7ab6-9cb7-1422784a8e51@quicinc.com>
Date: Tue, 19 Mar 2024 11:27:43 -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: Doug Anderson <dianders@chromium.org>,
        Dmitry Baryshkov
	<dmitry.baryshkov@linaro.org>
CC: Rob Clark <robdclark@gmail.com>, 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>
 <e2319b19-d999-24e7-48fa-bbc19bbfbeea@quicinc.com>
 <CAA8EJppau--vt3RLkH96K0SF2x-QGWz+5U8tErvLFDvz-GQN4Q@mail.gmail.com>
 <d04711c1-6df0-f988-9227-2161f4109dd1@quicinc.com>
 <CAA8EJppcuftv4hfKkZeaXCDQJ4Z5+8-P99wHc9X8WgYJbk6CjA@mail.gmail.com>
 <CAD=FV=WMbqmU1q+6hJZV9a16PdPUtdDJuMmsQ1wb27jDdeQFmg@mail.gmail.com>
From: Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAD=FV=WMbqmU1q+6hJZV9a16PdPUtdDJuMmsQ1wb27jDdeQFmg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


On 3/19/2024 11:15 AM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Mar 19, 2024 at 10:27 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On Tue, 19 Mar 2024 at 19:13, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 3/18/2024 5:55 PM, Dmitry Baryshkov wrote:
>>>> On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>> +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()?
>>>>
>>>> It is optional for the DP AUX implementations, not for the panel to be called.
>>>>
>>>
>>> Yes, I understood that part, but is there anything from the panel side
>>> which mandates calling wait_hpd_asserted()?
>>>
>>> Is this documented somewhere for all edp panels to do:
>>>
>>> if (aux->wait_hpd_asserted)
>>>          aux->wait_hpd_asserted(aux, wait_us);
>>
>> That's obviously not true, e.g. if panel-edp.c handled HPD signal via
>> the GPIO pin.
>>
>> But the documentation explicitly says that the host will be powered up
>> automatically, but the caller must ensure that the panel is powered
>> on. `It's up to the caller of this code to make sure that the panel is
>> powered on if getting an error back is not OK.'
> 
> It wouldn't hurt to send out a documentation patch that makes this
> more explicit. OK, I sent:
> 
> https://lore.kernel.org/r/20240319111432.1.I521dad0693cc24fe4dd14cba0c7048d94f5b6b41@changeid
> 
> -Doug

Thanks, with that in place, this is

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>