linux-kernel - Re: [PATCH drm-dp 02/10] drm/hisilicon/hibmc: fix dp probabilistical detect errors after HPD irq

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a5658165-9e8a-49e6-b2ed-75b25c074b87@oss.qualcomm.com>
Date: Mon, 9 Jun 2025 17:49:04 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Yongbang Shi <shiyongbang@...wei.com>
Cc: xinliang.liu@...aro.org, tiantao6@...ilicon.com,
        maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
        tzimmermann@...e.de, airlied@...il.com, daniel@...ll.ch,
        kong.kongxinwei@...ilicon.com, liangjian010@...wei.com,
        chenjianmin@...wei.com, lidongming5@...wei.com, libaihan@...wei.com,
        shenjian15@...wei.com, shaojijie@...wei.com,
        jani.nikula@...ux.intel.com, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH drm-dp 02/10] drm/hisilicon/hibmc: fix dp probabilistical
 detect errors after HPD irq

On 09/06/2025 17:47, Yongbang Shi wrote:
> 
>> On Fri, May 30, 2025 at 05:54:24PM +0800, Yongbang Shi wrote:
>>> From: Baihan Li <libaihan@...wei.com>
>>>
>>> The debouncing when HPD pulled out still remains sometimes, 200ms 
>>> still can
>>> not ensure helper_detect() is correct. So add a flag to hold the sink
>>> status, and changed detect_ctx() functions by using flag to check 
>>> status.
>>>
>>> Fixes: 3c7623fb5bb6 ("drm/hisilicon/hibmc: Enable this hot plug 
>>> detect of irq feature")
>>> Signed-off-by: Baihan Li <libaihan@...wei.com>
>>> ---
>>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  1 +
>>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 38 +++++++++++++------
>>>   2 files changed, 28 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/ 
>>> gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>>> index 665f5b166dfb..68867475508c 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>>> @@ -50,6 +50,7 @@ struct hibmc_dp {
>>>       struct drm_dp_aux aux;
>>>       struct hibmc_dp_cbar_cfg cfg;
>>>       u32 irq_status;
>>> +    int hpd_status;
>>>   };
>>>   int hibmc_dp_hw_init(struct hibmc_dp *dp);
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/ 
>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>>> index d06832e62e96..191fb434baa7 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>>> @@ -13,7 +13,8 @@
>>>   #include "hibmc_drm_drv.h"
>>>   #include "dp/dp_hw.h"
>>> -#define DP_MASKED_SINK_HPD_PLUG_INT    BIT(2)
>>> +#define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT    BIT(2)
>>> +#define HIBMC_DP_MASKED_SINK_HPD_UNPLUG_INT    BIT(3)
>>>   static int hibmc_dp_connector_get_modes(struct drm_connector 
>>> *connector)
>>>   {
>>> @@ -34,9 +35,12 @@ static int hibmc_dp_connector_get_modes(struct 
>>> drm_connector *connector)
>>>   static int hibmc_dp_detect(struct drm_connector *connector,
>>>                  struct drm_modeset_acquire_ctx *ctx, bool force)
>>>   {
>>> -    mdelay(200);
>>> +    struct hibmc_dp *dp = to_hibmc_dp(connector);
>>> -    return drm_connector_helper_detect_from_ddc(connector, ctx, force);
>>> +    if (dp->hpd_status)
>>> +        return connector_status_connected;
>>> +    else
>>> +        return connector_status_disconnected;
>>>   }
>>>   static const struct drm_connector_helper_funcs 
>>> hibmc_dp_conn_helper_funcs = {
>>> @@ -115,22 +119,34 @@ irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
>>>   {
>>>       struct drm_device *dev = (struct drm_device *)arg;
>>>       struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>>> +    struct hibmc_dp *dp = &priv->dp;
>>>       int idx;
>>>       if (!drm_dev_enter(dev, &idx))
>>>           return -ENODEV;
>>> -    if (priv->dp.irq_status & DP_MASKED_SINK_HPD_PLUG_INT) {
>>> -        drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
>>> -        hibmc_dp_hpd_cfg(&priv->dp);
>>> +    if (dp->hpd_status) { /* only check unplug int when the last 
>>> status is HPD in */
>> I think this way you'll ignore HPD short pulses. Could you possibly
>> clarify whether it is the case or not?
> 
> We actually doesn't enable short HPD here, this feature just used in our 
> electrical tests.
> 
> 
>>> +        if ((dp->irq_status & HIBMC_DP_MASKED_SINK_HPD_UNPLUG_INT)) {
>>> +            drm_dbg_dp(dev, "HPD OUT isr occur.");
>>> +            hibmc_dp_reset_link(dp);
>>> +            dp->hpd_status = 0;
>>> +            if (dev->registered)
>>> +                drm_connector_helper_hpd_irq_event(&dp->connector);
>>> +        } else {
>>> +            drm_warn(dev, "HPD OUT occurs, irq status err: %u", dp- 
>>> >irq_status);
>> These should be ratelimited.
> 
> Sorry, I didn't get it. Do you mean I need print the link rate here?
> 

No, I was thinking about drm_err_ratelimited() in case something gets 
stuck in the hw.

-- 
With best wishes
Dmitry