Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6830a829-5b8a-a05a-da6a-5aaaeef23e57@quicinc.com>
Date:   Wed, 10 May 2023 17:39:07 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.2
Subject: Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts
 to hpd_enable/disable
Content-Language: en-US
To:     Stephen Boyd <swboyd@chromium.org>,
        Kuogee Hsieh <quic_khsieh@quicinc.com>, <agross@kernel.org>,
        <airlied@gmail.com>, <andersson@kernel.org>, <daniel@ffwll.ch>,
        <dianders@chromium.org>, <dmitry.baryshkov@linaro.org>,
        <dri-devel@lists.freedesktop.org>, <robdclark@gmail.com>,
        <sean@poorly.run>, <vkoul@kernel.org>
CC:     <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: <1683750665-8764-1-git-send-email-quic_khsieh@quicinc.com>
 <1683750665-8764-2-git-send-email-quic_khsieh@quicinc.com>
 <CAE-0n53ywhgFhJXA9krBo-Ds6ezM0K8n6w0xnVZj+sTJ4qt9cA@mail.gmail.com>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAE-0n53ywhgFhJXA9krBo-Ds6ezM0K8n6w0xnVZj+sTJ4qt9cA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 5/10/2023 4:55 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2023-05-10 13:31:04)
>> The internal_hpd flag was introduced to handle external DP HPD derived from GPIO
>> pinmuxed into DP controller.
> 
> Was it? It looks more like it was done to differentiate between eDP and
> DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the
> bridge and we only set the bridge op if the connector type is DP. The
> assumption looks like if you have DP connector_type, you have the gpio
> pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat
> that gpio as an irq either, because it isn't. Instead the gpio is muxed
> to the mdss inside the SoC and then that generates an mdss interrupt
> that's combined with non-HPD things like "video ready".
> 
> If that all follows, then I don't quite understand why we're setting
> internal_hpd to false at all at runtime. It should be set to true at
> some point, but ideally that point is during probe.
> 

Kuogee had the same thought originally but were not entirely sure of 
this part of the commit message in Bjorn's original commit which 
introduced these changes.

"This difference is not appropriately represented by the "is_edp"
boolean, but is properly represented by the frameworks invocation of the
hpd_enable() and hpd_disable() callbacks. Switch the current condition
to rely on these callbacks instead"

Does this along with below documentation mean we should generate the hpd 
interrupts only after hpd_enable callback happens?

" * Call &drm_bridge_funcs.hpd_enable if implemented and register the 
given @cb
  * and @data as hot plug notification callback. From now on the @cb will be
  * called with @data when an output status change is detected by the 
bridge,
  * until hot plug notification gets disabled with drm_bridge_hpd_disable().
"

Bjorn, can you please clarify this?

>> 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@quicinc.com>
>> ---
>>   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
>> @@ -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;
> 
> Can we set internal_hpd to true during probe when we see that the hpd
> pinmux exists? Or do any of these bits toggle in the irq status register
> when the gpio isn't muxed to "dp_hot" or the controller is for eDP and
> it doesn't have any gpio connection internally? I'm wondering if we can
> get by with simply enabling the "dp_hot" pin interrupts
> (plug/unplug/replug/irq_hpd) unconditionally and not worrying about them
> if eDP is there (because the pin doesn't exist inside the SoC), or if DP
> HPD is being signalled out of band through type-c framework.