Received-SPF: pass (google.com: domain of linux-kernel+bounces-94518-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <2295092e-355b-4ebf-f630-14623cf7d9a3@quicinc.com>
Date: Wed, 6 Mar 2024 11:59:12 -0800
Precedence: bulk
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: [RFC PATCH] drm/msm/dp: move link_ready out of HPD event thread
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: <freedreno@lists.freedesktop.org>, Rob Clark <robdclark@gmail.com>,
        "Sean
 Paul" <sean@poorly.run>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        "David Airlie" <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
        <dri-devel@lists.freedesktop.org>, <swboyd@chromium.org>,
        <quic_jesszhan@quicinc.com>, <quic_parellan@quicinc.com>,
        <quic_khsieh@quicinc.com>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20240306195031.490994-1-quic_abhinavk@quicinc.com>
 <CAA8EJpq=5=L5RdVZRkf=e2wyjQufnSzEC+=19FjCCF9S6SSEJA@mail.gmail.com>
Content-Language: en-US
From: Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAA8EJpq=5=L5RdVZRkf=e2wyjQufnSzEC+=19FjCCF9S6SSEJA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 3/6/2024 11:52 AM, Dmitry Baryshkov wrote:
> On Wed, 6 Mar 2024 at 21:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> There are cases where the userspace might still send another
>> frame after the HPD disconnect causing a modeset cycle after
>> a disconnect. This messes the internal state machine of MSM DP driver
>> and can lead to a crash as there can be an imbalance between
>> bridge_disable() and bridge_enable().
>>
>> This was also previously reported on [1] for which [2] was posted
>> and helped resolve the issue by rejecting commits if the DP is not
>> in connected state.
>>
>> The change resolved the bug but there can also be another race condition.
>> If hpd_event_thread does not pick up the EV_USER_NOTIFICATION and process it
>> link_ready will also not be set to false allowing the frame to sneak in.
>>
>> Lets move setting link_ready outside of hpd_event_thread() processing to
>> eliminate a window of race condition.
>>
>> [1] : https://gitlab.freedesktop.org/drm/msm/-/issues/17
>> [2] : https://lore.kernel.org/all/1664408211-25314-1-git-send-email-quic_khsieh@quicinc.com/
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 068d44eeaa07..e00092904ccc 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -345,8 +345,6 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
>>                                                           dp->panel->downstream_ports);
>>          }
>>
>> -       dp->dp_display.link_ready = hpd;
>> -
>>          drm_dbg_dp(dp->drm_dev, "type=%d hpd=%d\n",
>>                          dp->dp_display.connector_type, hpd);
>>          drm_bridge_hpd_notify(bridge, dp->dp_display.link_ready);
>> @@ -399,6 +397,8 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
>>                  goto end;
>>          }
>>
>> +       dp->dp_display.link_ready = true;
> 
> Do we need any kind of locking now?
> 

hmm ... correct me if I have missed any flows but I think all paths 
where we will set link_ready are already protected by event_mutex?

>> +
>>          dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
>>
>>   end:
>> @@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device *dev)
>>   {
>>          struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>
>> +       dp->dp_display.link_ready = false;
>> +
>>          dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
>>
>>          return 0;
>> @@ -487,6 +489,7 @@ static int dp_display_handle_port_status_changed(struct dp_display_private *dp)
>>                  drm_dbg_dp(dp->drm_dev, "sink count is zero, nothing to do\n");
>>                  if (dp->hpd_state != ST_DISCONNECTED) {
>>                          dp->hpd_state = ST_DISCONNECT_PENDING;
>> +                       dp->dp_display.link_ready = false;
>>                          dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
>>                  }
>>          } else {
>> --
>> 2.34.1
>>
> 
>