| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5ef83699-01de-d062-6239-9bb834c70458@linaro.org>
Date: Thu, 11 May 2023 18:57:59 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Kuogee Hsieh <quic_khsieh@...cinc.com>,
dri-devel@...ts.freedesktop.org, robdclark@...il.com,
sean@...rly.run, swboyd@...omium.org, dianders@...omium.org,
vkoul@...nel.org, daniel@...ll.ch, airlied@...il.com,
agross@...nel.org, quic_abhinavk@...cinc.com,
quic_jesszhan@...cinc.com, quic_sbillaka@...cinc.com,
marijn.suijten@...ainline.org, freedreno@...ts.freedesktop.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts
to hpd_enable/disable
On 11/05/2023 18:53, Bjorn Andersson wrote:
> On Thu, May 11, 2023 at 07:24:46AM +0300, Dmitry Baryshkov wrote:
>> On Wed, 10 May 2023 at 23:31, Kuogee Hsieh <quic_khsieh@...cinc.com> wrote:
>>>
>>> The internal_hpd flag was introduced to handle external DP HPD derived from GPIO
>>> pinmuxed into DP controller. HPD plug/unplug interrupts cannot be enabled until
>>> internal_hpd flag is set to true.
>>> At both bootup and resume time, the DP driver will enable external DP
>>> plugin interrupts and handle plugin interrupt accordingly. Unfortunately
>>> dp_bridge_hpd_enable() bridge ops function was called to set internal_hpd
>>> flag to true later than where DP driver expected during bootup time.
>>>
>>> This causes external DP plugin event to not get detected and display stays blank.
>>> Move enabling HDP plugin/unplugged interrupts to dp_bridge_hpd_enable()/disable() to
>>> set internal_hpd to true along with enabling HPD plugin/unplugged interrupts
>>> simultaneously to avoid timing issue during bootup and resume.
>>>
>>> Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
>>
>> Thanks for debugging this!
>>
>> However after looking at the driver I think there is more than this.
>>
>> We have several other places gated on internal_hpd flag, where we do
>> not have a strict ordering of events.
>> I see that dp_hpd_plug_handle() and dp_hpd_unplug_handle() also toggle
>> DP_DP_IRQ_HPD_INT_MASK and DP_DP_HPD_REPLUG_INT_MASK depending on
>> internal_hpd. Can we toggle all 4 interrupts from the
>> hpd_enable/hpd_disable functions? If we can do it, then I think we can
>> drop the internal_hpd flag completely.
>>
>
> Yes, that's what I believe the DRM framework intend us to do.
>
> The problem, and reason why I didn't do tat in my series, was that in
> order to update the INT_MASKs you need to clock the IP-block and that's
> done elsewhere.
>
> So, for the internal_hpd case, it seems appropriate to pm_runtime_get()
> in hpd_enable() and unmask the HPD interrupts, and mask the interrupts
> and pm_runtime_put() in hpd_disable().
>
>
> But for edp and external HPD-signal we also need to make sure power is
> on while something is connected...
I think this is already handled by the existing code, see calls to the
dp_display_host_init().
>
>> I went on and checked other places where it is used:
>> - dp_hpd_unplug_handle(), guarding DP_DP_HPD_PLUG_INT_MASK toggling. I
>> think we can drop these two calls completely. The function is under
>> the event_mutex protection, so other events can not interfere.
>> - dp_bridge_hpd_notify(). What is the point of this check? If some
>> other party informs us of the HPD event, we'd better handle it instead
>> of dropping it. Correct? In other words, I'd prefer seeing the
>> hpd_event_thread removal. Instead of that I think that on
>> HPD/plug/unplug/etc. IRQ the driver should call into the drm stack,
>> then the hpd_notify call should process those events.
>>
>
> I agree, that seems to be what's expected of us from the DRM framework.
>
> Regards,
> Bjorn
>
>>
>>> ---
>>> drivers/gpu/drm/msm/dp/dp_display.c | 27 ++++++++++++++-------------
>>> 1 file changed, 14 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index 3e13acdf..71aa944 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -1088,13 +1088,6 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
>>> dp_display_host_init(dp);
>>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>>
>>> - /* Enable plug and unplug interrupts only if requested */
>>> - if (dp->dp_display.internal_hpd)
>>> - dp_catalog_hpd_config_intr(dp->catalog,
>>> - DP_DP_HPD_PLUG_INT_MASK |
>>> - DP_DP_HPD_UNPLUG_INT_MASK,
>>> - true);
>>> -
>>> /* Enable interrupt first time
>>> * we are leaving dp clocks on during disconnect
>>> * and never disable interrupt
>>> @@ -1396,12 +1389,6 @@ static int dp_pm_resume(struct device *dev)
>>>
>>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>>
>>> - if (dp->dp_display.internal_hpd)
>>> - dp_catalog_hpd_config_intr(dp->catalog,
>>> - DP_DP_HPD_PLUG_INT_MASK |
>>> - DP_DP_HPD_UNPLUG_INT_MASK,
>>> - true);
>>> -
>>> if (dp_catalog_link_is_connected(dp->catalog)) {
>>> /*
>>> * set sink to normal operation mode -- D0
>>> @@ -1801,15 +1788,29 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
>>> {
>>> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
>>> struct msm_dp *dp_display = dp_bridge->dp_display;
>>> + struct dp_display_private *dp;
>>> +
>>> + dp = container_of(dp_display, struct dp_display_private, dp_display);
>>>
>>> dp_display->internal_hpd = true;
>>> + dp_catalog_hpd_config_intr(dp->catalog,
>>> + DP_DP_HPD_PLUG_INT_MASK |
>>> + DP_DP_HPD_UNPLUG_INT_MASK,
>>> + true);
>>> }
>>>
>>> void dp_bridge_hpd_disable(struct drm_bridge *bridge)
>>> {
>>> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
>>> struct msm_dp *dp_display = dp_bridge->dp_display;
>>> + struct dp_display_private *dp;
>>> +
>>> + dp = container_of(dp_display, struct dp_display_private, dp_display);
>>>
>>> + dp_catalog_hpd_config_intr(dp->catalog,
>>> + DP_DP_HPD_PLUG_INT_MASK |
>>> + DP_DP_HPD_UNPLUG_INT_MASK,
>>> + false);
>>> dp_display->internal_hpd = false;
>>> }
>>
>> --
>> With best wishes
>> Dmitry
--
With best wishes
Dmitry
Powered by blists - more mailing lists