Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
In-Reply-To: <58701008-bb93-e5c6-9ca0-5bc43f9a46f0@quicinc.com>
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> <58701008-bb93-e5c6-9ca0-5bc43f9a46f0@quicinc.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Mon, 2 Oct 2023 17:58:06 -0500
Message-ID: <CAE-0n50N6hXM7qQZzccKy2X-kcru9n7Nvgn_V4tOHTnLn64qjw@mail.gmail.com>
Subject: Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()
To:     Abhinav Kumar <quic_abhinavk@quicinc.com>,
        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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Quoting Abhinav Kumar (2023-09-28 17:46:11)
> On 9/27/2023 3:01 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2023-09-25 09:07:18)
> >>
> >> However for external DP case, link training can not be guarantee alway=
s
> >> success without link rate or lane being reduced as Abhinav mentioned.
> >>
> >> In addition,=C2=A0 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 CT=
S
> > parameters because I'm pretty confused how it can work. What do we do i=
f
> > DP wakes the system from suspend and asserts HPD? We need resume time t=
o
> > 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.

Great!

>
> >>
> >> I am not sure do link training at atomic_enable() can meet this timing
> >> requirement.

Why? It's putting some time bound on link training in general to only
take 10ms, right?

> >>
> >
> > At least in the DP spec itself it doesn't require the link to be traine=
d
> > within 10ms of HPD being asserted. Instead it simply recommends that th=
e
> > 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.

What needs to be done to recover? Userspace will try to set a mode on
the connector again if the link status is bad and there were some modes
available. If there are zero modes and the link is bad, then it ignores
the connector. I'm not sure what else could be done to recover besides
try again and stop trying if no modes exist.

Acting like the connector isn't connected makes the situation worse for
ChromeOS because userspace thinks there's nothing there so it can't try
to retrain the link again. Instead, userspace has to rely on the kernel
driver to train the link again. The kernel should just tell userspace
the link is bad so userspace can implement the policy to either ignore
the connector entirely or to consider it a display that is having link
training problems.

So again, I see no reason why the kernel driver thinks it can implement
a policy to train the link before indicating the drm connector is
connected. It should stop doing that. Instead it should tell userspace
that the connector is connected and then train the link when there's a
modeset. If the modeset fails then userspace can take action to either
figure out that the link is bad, or notify the user that the cable is
bad, or to try replugging or power cycle the monitor, etc. None of that
can be done if the kernel lies about the state of the connector because
the link training failed.