Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <58701008-bb93-e5c6-9ca0-5bc43f9a46f0@quicinc.com>
Date:   Thu, 28 Sep 2023 17:46:11 -0700
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 v3 6/7] drm/msm/dp: add
 pm_runtime_force_suspend()/resume()
Content-Language: en-US
To:     Stephen Boyd <swboyd@chromium.org>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Kuogee Hsieh <quic_khsieh@quicinc.com>
CC:     <dri-devel@lists.freedesktop.org>, <robdclark@gmail.com>,
        <sean@poorly.run>, <dianders@chromium.org>, <vkoul@kernel.org>,
        <daniel@ffwll.ch>, <airlied@gmail.com>, <agross@kernel.org>,
        <andersson@kernel.org>, <quic_jesszhan@quicinc.com>,
        <quic_sbillaka@quicinc.com>, <marijn.suijten@somainline.org>,
        <freedreno@lists.freedesktop.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <1694813901-26952-1-git-send-email-quic_khsieh@quicinc.com>
 <1694813901-26952-7-git-send-email-quic_khsieh@quicinc.com>
 <CAA8EJpqPXoFX4LXyXYgfh07Vpxg-KgD8VBR6x5bXf4GOJmbOtw@mail.gmail.com>
 <2f98d5f1-57c1-d9fe-cb1c-b975db057287@quicinc.com>
 <CAA8EJpr2wRq6Txi7YAQpJKa_9UGqH_nmHzvVOaAPkwOrtDg4Tw@mail.gmail.com>
 <CAE-0n53dqHONzMTd_ZC-fKWTzDVq6Wqwo4OFZMUcghZ5SD5RhA@mail.gmail.com>
 <65566a68-3510-2e5f-7d57-e4dba08c008c@quicinc.com>
 <1d9bf80d-0267-937b-4dd9-c57db7a89cb4@quicinc.com>
 <CAE-0n51Hrs66oG4NF5rDETkVO-ocG_6_=Aqc5cE-qPDViSgKyA@mail.gmail.com>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAE-0n51Hrs66oG4NF5rDETkVO-ocG_6_=Aqc5cE-qPDViSgKyA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 9/27/2023 3:01 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2023-09-25 09:07:18)
>>
>> On 9/22/2023 6:35 PM, Abhinav Kumar wrote:
>>>
>>> Doing link training when we get hpd instead of atomic_enable() is a
>>> design choice we have been following for a while because for the case
>>> when link training fails in atomic_enable() and setting the link
>>> status property as you mentioned, the compositor needs to be able to
>>> handle that and also needs to try with a different resolution or take
>>> some other corrective action. We have seen many compositors not able
>>> to handle this complexity. So the design sends the hotplug to usermode
>>> only after link training succeeds.
>>>
>>> I do not think we should change this design unless prototyped with an
>>> existing compositor such as chrome or android at this point.
>>>
>>> Thanks
>>>
>>> Abhinav
>>
>>
>> We did perform link training at atomic_enable() at eDP case since we can
>> assume link training will always success without link rate or link lane
>> being reduced.
>>
>> However for external DP case, link training can not be guarantee always
>> success without link rate or lane being reduced as Abhinav mentioned.
>>
>> In addition,  CTS (compliance test) it required to complete link
>> training within 10ms after hpd asserted.
> 
> Is it possible to change that timeout? I have to look around for the CTS
> parameters because I'm pretty confused how it can work. What do we do if
> DP wakes the system from suspend and asserts HPD? We need resume time to
> be < 10ms?  That's not realistic.
> 

No, the CTS doesnt say we need to finish link training within 10ms after 
HPD is asserted. It says it must be completed in 10ms after 
TRAINING_PATTERN_SET dpcd write.

"Wait until the Source DUT writes 00h to the TRAINING_PATTERN_SET byte 
of Reference Sink DPCD Link Configuration Field to indicate the end of 
the link training. Stop the link training timer. Verify that link 
training completed in 10ms or less"

That needs to be done independent of HPD so we can ignore the CTS point.

>>
>> I am not sure do link training at atomic_enable() can meet this timing
>> requirement.
>>
> 
> At least in the DP spec itself it doesn't require the link to be trained
> within 10ms of HPD being asserted. Instead it simply recommends that the
> OS start configuring the display promptly after HPD is asserted, e.g.
> within 100ms. There's some strict timing on IRQ_HPD, so the driver must
> read DPCD registers within 100ms of IRQ_HPD rising edge; maybe that is
> what CTS is checking for?
> 
> TL;DR: I don't see why CTS should stop us from link training in
> atomic_enable(). It would be beneficial to do so to make eDP and DP the
> same. It would also help to report a drm connector being connected
> _before_ link training so that userspace knows the link itself is the
> bad part of the equation (and not that the DP connector looks
> disconnected to userspace when in fact it really is connected and the
> monitor is asserting HPD, just the link training failed).

Its the corrective action of the userspace when it finds link is bad is 
the concern as I highlighted in the other response. Just reading and 
resetting link_status is not enough to recover.