[PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[Dmitry Baryshkov]

This reverts commit e467e0bde881 ("drm/msm/dp: use
drm_bridge_hpd_notify() to report HPD status changes").

The commit changed the way how the MSM DP driver communicates
HPD-related events to the userspace. The mentioned commit made some of
the HPD events being reported earlier. This way userspace starts poking
around. It interacts in a bad way with the dp_bridge_detect and the
driver's state machine, ending up either with the very long delays
during hotplug detection or even inability of the DP driver to report
the display as connected.

A proper fix will involve redesigning of the HPD handling in the MSM DP
driver. It is underway, but it will be intrusive and can not be thought
about as a simple fix for the issue. Thus, revert the offending commit.

Fixes: e467e0bde881 ("drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes")
Link: https://gitlab.freedesktop.org/drm/msm/-/issues/50
Reported-by: Johan Hovold <johan@kernel.org>
Link: https://lore.kernel.org/r/Zd3YPGmrprxv-N-O@hovoldconsulting.com/
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index d37d599aec27..4c72124ffb5d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -329,10 +329,26 @@ static const struct component_ops dp_display_comp_ops = {
 	.unbind = dp_display_unbind,
 };
 
+static void dp_display_send_hpd_event(struct msm_dp *dp_display)
+{
+	struct dp_display_private *dp;
+	struct drm_connector *connector;
+
+	dp = container_of(dp_display, struct dp_display_private, dp_display);
+
+	connector = dp->dp_display.connector;
+	drm_helper_hpd_irq_event(connector->dev);
+}
+
 static int dp_display_send_hpd_notification(struct dp_display_private *dp,
 					    bool hpd)
 {
-	struct drm_bridge *bridge = dp->dp_display.bridge;
+	if ((hpd && dp->dp_display.link_ready) ||
+			(!hpd && !dp->dp_display.link_ready)) {
+		drm_dbg_dp(dp->drm_dev, "HPD already %s\n",
+				(hpd ? "on" : "off"));
+		return 0;
+	}
 
 	/* reset video pattern flag on disconnect */
 	if (!hpd) {
@@ -348,7 +364,7 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
 
 	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);
+	dp_display_send_hpd_event(&dp->dp_display);
 
 	return 0;
 }
-- 
2.39.2

Re: [PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[Abhinav Kumar]

On 2/27/2024 2:08 PM, Dmitry Baryshkov wrote:
> This reverts commit e467e0bde881 ("drm/msm/dp: use
> drm_bridge_hpd_notify() to report HPD status changes").
> 
> The commit changed the way how the MSM DP driver communicates
> HPD-related events to the userspace. The mentioned commit made some of
> the HPD events being reported earlier. This way userspace starts poking
> around. It interacts in a bad way with the dp_bridge_detect and the
> driver's state machine, ending up either with the very long delays
> during hotplug detection or even inability of the DP driver to report
> the display as connected.
> 
> A proper fix will involve redesigning of the HPD handling in the MSM DP
> driver. It is underway, but it will be intrusive and can not be thought
> about as a simple fix for the issue. Thus, revert the offending commit.
> 

Yes, for fixing this on 6.9 I am fine with this.

I hope there were not other changes which were built on top of this. So 
it will be better if we retest internal HPD case as well with this.

We will do that in a day or two and give Tested-by.

> Fixes: e467e0bde881 ("drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes")
> Link: https://gitlab.freedesktop.org/drm/msm/-/issues/50
> Reported-by: Johan Hovold <johan@kernel.org>
> Link: https://lore.kernel.org/r/Zd3YPGmrprxv-N-O@hovoldconsulting.com/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
> 

For the change itself,


Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[Paloma Arellano]

On 2/27/2024 2:11 PM, Abhinav Kumar wrote:
>
>
> On 2/27/2024 2:08 PM, Dmitry Baryshkov wrote:
>> This reverts commit e467e0bde881 ("drm/msm/dp: use
>> drm_bridge_hpd_notify() to report HPD status changes").
>>
>> The commit changed the way how the MSM DP driver communicates
>> HPD-related events to the userspace. The mentioned commit made some of
>> the HPD events being reported earlier. This way userspace starts poking
>> around. It interacts in a bad way with the dp_bridge_detect and the
>> driver's state machine, ending up either with the very long delays
>> during hotplug detection or even inability of the DP driver to report
>> the display as connected.
>>
>> A proper fix will involve redesigning of the HPD handling in the MSM DP
>> driver. It is underway, but it will be intrusive and can not be thought
>> about as a simple fix for the issue. Thus, revert the offending commit.
>>
>
> Yes, for fixing this on 6.9 I am fine with this.
>
> I hope there were not other changes which were built on top of this. 
> So it will be better if we retest internal HPD case as well with this.
>
> We will do that in a day or two and give Tested-by.
>
>> Fixes: e467e0bde881 ("drm/msm/dp: use drm_bridge_hpd_notify() to 
>> report HPD status changes")
>> Link: https://gitlab.freedesktop.org/drm/msm/-/issues/50
>> Reported-by: Johan Hovold <johan@kernel.org>
>> Link: https://lore.kernel.org/r/Zd3YPGmrprxv-N-O@hovoldconsulting.com/
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 20 ++++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>
> For the change itself,
>
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>


Tested-by: Paloma Arellano <quic_parellan@quicinc.com>

Re: [PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[Johan Hovold]

On Tue, Feb 27, 2024 at 02:11:56PM -0800, Abhinav Kumar wrote:
> On 2/27/2024 2:08 PM, Dmitry Baryshkov wrote:
> > This reverts commit e467e0bde881 ("drm/msm/dp: use
> > drm_bridge_hpd_notify() to report HPD status changes").
> > 
> > The commit changed the way how the MSM DP driver communicates
> > HPD-related events to the userspace. The mentioned commit made some of
> > the HPD events being reported earlier. This way userspace starts poking
> > around. It interacts in a bad way with the dp_bridge_detect and the
> > driver's state machine, ending up either with the very long delays
> > during hotplug detection or even inability of the DP driver to report
> > the display as connected.
> > 
> > A proper fix will involve redesigning of the HPD handling in the MSM DP
> > driver. It is underway, but it will be intrusive and can not be thought
> > about as a simple fix for the issue. Thus, revert the offending commit.
> 
> Yes, for fixing this on 6.9 I am fine with this.

Since this is a regression in 6.8-rc1, I hope you meant to say 6.8 here?

> I hope there were not other changes which were built on top of this. So 
> it will be better if we retest internal HPD case as well with this.
> 
> We will do that in a day or two and give Tested-by.

Johan

Re: [PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[Dmitry Baryshkov]

On Wed, 28 Feb 2024 at 11:50, Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Feb 27, 2024 at 02:11:56PM -0800, Abhinav Kumar wrote:
> > On 2/27/2024 2:08 PM, Dmitry Baryshkov wrote:
> > > This reverts commit e467e0bde881 ("drm/msm/dp: use
> > > drm_bridge_hpd_notify() to report HPD status changes").
> > >
> > > The commit changed the way how the MSM DP driver communicates
> > > HPD-related events to the userspace. The mentioned commit made some of
> > > the HPD events being reported earlier. This way userspace starts poking
> > > around. It interacts in a bad way with the dp_bridge_detect and the
> > > driver's state machine, ending up either with the very long delays
> > > during hotplug detection or even inability of the DP driver to report
> > > the display as connected.
> > >
> > > A proper fix will involve redesigning of the HPD handling in the MSM DP
> > > driver. It is underway, but it will be intrusive and can not be thought
> > > about as a simple fix for the issue. Thus, revert the offending commit.
> >
> > Yes, for fixing this on 6.9 I am fine with this.
>
> Since this is a regression in 6.8-rc1, I hope you meant to say 6.8 here?

In the worst case it will land to 6.8.x via the stable tree process.

>
> > I hope there were not other changes which were built on top of this. So
> > it will be better if we retest internal HPD case as well with this.
> >
> > We will do that in a day or two and give Tested-by.
>
> Johan



-- 
With best wishes
Dmitry

Re: [PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[Johan Hovold]

On Wed, Feb 28, 2024 at 01:08:04PM +0200, Dmitry Baryshkov wrote:
> On Wed, 28 Feb 2024 at 11:50, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Tue, Feb 27, 2024 at 02:11:56PM -0800, Abhinav Kumar wrote:
> > > On 2/27/2024 2:08 PM, Dmitry Baryshkov wrote:
> > > > This reverts commit e467e0bde881 ("drm/msm/dp: use
> > > > drm_bridge_hpd_notify() to report HPD status changes").
> > > >
> > > > The commit changed the way how the MSM DP driver communicates
> > > > HPD-related events to the userspace. The mentioned commit made some of
> > > > the HPD events being reported earlier. This way userspace starts poking
> > > > around. It interacts in a bad way with the dp_bridge_detect and the
> > > > driver's state machine, ending up either with the very long delays
> > > > during hotplug detection or even inability of the DP driver to report
> > > > the display as connected.
> > > >
> > > > A proper fix will involve redesigning of the HPD handling in the MSM DP
> > > > driver. It is underway, but it will be intrusive and can not be thought
> > > > about as a simple fix for the issue. Thus, revert the offending commit.
> > >
> > > Yes, for fixing this on 6.9 I am fine with this.
> >
> > Since this is a regression in 6.8-rc1, I hope you meant to say 6.8 here?
> 
> In the worst case it will land to 6.8.x via the stable tree process.

This is a fix for a user-visible regression that was reported formally
two weeks ago and informally (e.g. to you) soon after rc1 came out, and
which now also has an identified cause and an analysis of the problem.
And we're at rc6 so there should be no reason to delay fixing this (e.g.
even if you want to run some more tests for a couple of days).

Johan

Re: [PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[Abhinav Kumar]

On 2/28/2024 5:28 AM, Johan Hovold wrote:
> On Wed, Feb 28, 2024 at 01:08:04PM +0200, Dmitry Baryshkov wrote:
>> On Wed, 28 Feb 2024 at 11:50, Johan Hovold <johan@kernel.org> wrote:
>>>
>>> On Tue, Feb 27, 2024 at 02:11:56PM -0800, Abhinav Kumar wrote:
>>>> On 2/27/2024 2:08 PM, Dmitry Baryshkov wrote:
>>>>> This reverts commit e467e0bde881 ("drm/msm/dp: use
>>>>> drm_bridge_hpd_notify() to report HPD status changes").
>>>>>
>>>>> The commit changed the way how the MSM DP driver communicates
>>>>> HPD-related events to the userspace. The mentioned commit made some of
>>>>> the HPD events being reported earlier. This way userspace starts poking
>>>>> around. It interacts in a bad way with the dp_bridge_detect and the
>>>>> driver's state machine, ending up either with the very long delays
>>>>> during hotplug detection or even inability of the DP driver to report
>>>>> the display as connected.
>>>>>
>>>>> A proper fix will involve redesigning of the HPD handling in the MSM DP
>>>>> driver. It is underway, but it will be intrusive and can not be thought
>>>>> about as a simple fix for the issue. Thus, revert the offending commit.
>>>>
>>>> Yes, for fixing this on 6.9 I am fine with this.
>>>
>>> Since this is a regression in 6.8-rc1, I hope you meant to say 6.8 here?
>>
>> In the worst case it will land to 6.8.x via the stable tree process.
> 
> This is a fix for a user-visible regression that was reported formally
> two weeks ago and informally (e.g. to you) soon after rc1 came out, and
> which now also has an identified cause and an analysis of the problem.
> And we're at rc6 so there should be no reason to delay fixing this (e.g.
> even if you want to run some more tests for a couple of days).
> 

Yup, we landed it in msm-fixes now, so this will go as a late -fixes PR 
for 6.8.


> Johan

Re: [PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[Johan Hovold]

On Wed, Feb 28, 2024 at 10:10:10AM -0800, Abhinav Kumar wrote:
> On 2/28/2024 5:28 AM, Johan Hovold wrote:

> > This is a fix for a user-visible regression that was reported formally
> > two weeks ago and informally (e.g. to you) soon after rc1 came out, and
> > which now also has an identified cause and an analysis of the problem.
> > And we're at rc6 so there should be no reason to delay fixing this (e.g.
> > even if you want to run some more tests for a couple of days).
> 
> Yup, we landed it in msm-fixes now, so this will go as a late -fixes PR 
> for 6.8.

Perfect, thanks!

Johan

Re: [PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[Johan Hovold]

On Wed, Feb 28, 2024 at 12:08:08AM +0200, Dmitry Baryshkov wrote:
> This reverts commit e467e0bde881 ("drm/msm/dp: use
> drm_bridge_hpd_notify() to report HPD status changes").
> 
> The commit changed the way how the MSM DP driver communicates
> HPD-related events to the userspace. The mentioned commit made some of
> the HPD events being reported earlier. This way userspace starts poking
> around. It interacts in a bad way with the dp_bridge_detect and the
> driver's state machine, ending up either with the very long delays
> during hotplug detection or even inability of the DP driver to report
> the display as connected.
> 
> A proper fix will involve redesigning of the HPD handling in the MSM DP
> driver. It is underway, but it will be intrusive and can not be thought
> about as a simple fix for the issue. Thus, revert the offending commit.
> 
> Fixes: e467e0bde881 ("drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes")
> Link: https://gitlab.freedesktop.org/drm/msm/-/issues/50
> Reported-by: Johan Hovold <johan@kernel.org>
> Link: https://lore.kernel.org/r/Zd3YPGmrprxv-N-O@hovoldconsulting.com/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Tested-by: Johan Hovold <johan+linaro@kernel.org>

Re: [PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

[neil.armstrong]

On 27/02/2024 23:08, Dmitry Baryshkov wrote:
> This reverts commit e467e0bde881 ("drm/msm/dp: use
> drm_bridge_hpd_notify() to report HPD status changes").
> 
> The commit changed the way how the MSM DP driver communicates
> HPD-related events to the userspace. The mentioned commit made some of
> the HPD events being reported earlier. This way userspace starts poking
> around. It interacts in a bad way with the dp_bridge_detect and the
> driver's state machine, ending up either with the very long delays
> during hotplug detection or even inability of the DP driver to report
> the display as connected.
> 
> A proper fix will involve redesigning of the HPD handling in the MSM DP
> driver. It is underway, but it will be intrusive and can not be thought
> about as a simple fix for the issue. Thus, revert the offending commit.
> 
> Fixes: e467e0bde881 ("drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes")
> Link: https://gitlab.freedesktop.org/drm/msm/-/issues/50
> Reported-by: Johan Hovold <johan@kernel.org>
> Link: https://lore.kernel.org/r/Zd3YPGmrprxv-N-O@hovoldconsulting.com/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index d37d599aec27..4c72124ffb5d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -329,10 +329,26 @@ static const struct component_ops dp_display_comp_ops = {
>   	.unbind = dp_display_unbind,
>   };
>   
> +static void dp_display_send_hpd_event(struct msm_dp *dp_display)
> +{
> +	struct dp_display_private *dp;
> +	struct drm_connector *connector;
> +
> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> +	connector = dp->dp_display.connector;
> +	drm_helper_hpd_irq_event(connector->dev);
> +}
> +
>   static int dp_display_send_hpd_notification(struct dp_display_private *dp,
>   					    bool hpd)
>   {
> -	struct drm_bridge *bridge = dp->dp_display.bridge;
> +	if ((hpd && dp->dp_display.link_ready) ||
> +			(!hpd && !dp->dp_display.link_ready)) {
> +		drm_dbg_dp(dp->drm_dev, "HPD already %s\n",
> +				(hpd ? "on" : "off"));
> +		return 0;
> +	}
>   
>   	/* reset video pattern flag on disconnect */
>   	if (!hpd) {
> @@ -348,7 +364,7 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
>   
>   	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);
> +	dp_display_send_hpd_event(&dp->dp_display);
>   
>   	return 0;
>   }

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-HDK