[PATCH 1/2] drm/probe-helper: Add helper for drm_helper_probe_single_connector_modes()

[PATCH 1/2] drm/probe-helper: Add helper for drm_helper_probe_single_connector_modes()

[Douglas Anderson]

The drm_helper_probe_single_connector_modes() is a bit long. Let's
break a chunk off to update and validate modes. This helps avoid one
goto and also will allow us to more easily call the helper a second
time in a future patch without adding looping or another goto.

This change is intended to be a no-op change--just code movement.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/drm_probe_helper.c | 105 ++++++++++++++++-------------
 1 file changed, 59 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 682359512996..819225629010 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -354,6 +354,61 @@ drm_helper_probe_detect(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_helper_probe_detect);
 
+static bool _drm_helper_update_and_validate(struct drm_connector *connector,
+					    uint32_t maxX, uint32_t maxY,
+					    struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_display_mode *mode;
+	int mode_flags = 0;
+	int ret;
+
+	drm_connector_list_update(connector);
+
+	if (connector->interlace_allowed)
+		mode_flags |= DRM_MODE_FLAG_INTERLACE;
+	if (connector->doublescan_allowed)
+		mode_flags |= DRM_MODE_FLAG_DBLSCAN;
+	if (connector->stereo_allowed)
+		mode_flags |= DRM_MODE_FLAG_3D_MASK;
+
+	list_for_each_entry(mode, &connector->modes, head) {
+		if (mode->status != MODE_OK)
+			continue;
+
+		mode->status = drm_mode_validate_driver(dev, mode);
+		if (mode->status != MODE_OK)
+			continue;
+
+		mode->status = drm_mode_validate_size(mode, maxX, maxY);
+		if (mode->status != MODE_OK)
+			continue;
+
+		mode->status = drm_mode_validate_flag(mode, mode_flags);
+		if (mode->status != MODE_OK)
+			continue;
+
+		ret = drm_mode_validate_pipeline(mode, connector, ctx,
+						 &mode->status);
+		if (ret) {
+			drm_dbg_kms(dev,
+				    "drm_mode_validate_pipeline failed: %d\n",
+				    ret);
+
+			if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK))
+				mode->status = MODE_ERROR;
+			else
+				return true;
+		}
+
+		if (mode->status != MODE_OK)
+			continue;
+		mode->status = drm_mode_validate_ycbcr420(mode, connector);
+	}
+
+	return false;
+}
+
 /**
  * drm_helper_probe_single_connector_modes - get complete set of display modes
  * @connector: connector to probe
@@ -421,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 	const struct drm_connector_helper_funcs *connector_funcs =
 		connector->helper_private;
 	int count = 0, ret;
-	int mode_flags = 0;
 	bool verbose_prune = true;
 	enum drm_connector_status old_status;
 	struct drm_modeset_acquire_ctx ctx;
@@ -519,52 +573,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 			   connector->status == connector_status_unknown))
 		count = drm_add_modes_noedid(connector, 1024, 768);
 	count += drm_helper_probe_add_cmdline_mode(connector);
-	if (count == 0)
-		goto prune;
-
-	drm_connector_list_update(connector);
-
-	if (connector->interlace_allowed)
-		mode_flags |= DRM_MODE_FLAG_INTERLACE;
-	if (connector->doublescan_allowed)
-		mode_flags |= DRM_MODE_FLAG_DBLSCAN;
-	if (connector->stereo_allowed)
-		mode_flags |= DRM_MODE_FLAG_3D_MASK;
-
-	list_for_each_entry(mode, &connector->modes, head) {
-		if (mode->status != MODE_OK)
-			continue;
-
-		mode->status = drm_mode_validate_driver(dev, mode);
-		if (mode->status != MODE_OK)
-			continue;
-
-		mode->status = drm_mode_validate_size(mode, maxX, maxY);
-		if (mode->status != MODE_OK)
-			continue;
-
-		mode->status = drm_mode_validate_flag(mode, mode_flags);
-		if (mode->status != MODE_OK)
-			continue;
-
-		ret = drm_mode_validate_pipeline(mode, connector, &ctx,
-						 &mode->status);
-		if (ret) {
-			drm_dbg_kms(dev,
-				    "drm_mode_validate_pipeline failed: %d\n",
-				    ret);
-
-			if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK)) {
-				mode->status = MODE_ERROR;
-			} else {
-				drm_modeset_backoff(&ctx);
-				goto retry;
-			}
+	if (count != 0) {
+		if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) {
+			drm_modeset_backoff(&ctx);
+			goto retry;
 		}
-
-		if (mode->status != MODE_OK)
-			continue;
-		mode->status = drm_mode_validate_ycbcr420(mode, connector);
 	}
 
 prune:
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Douglas Anderson]

As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
that all detachable sinks shall support 640x480 @60Hz as a fail safe
mode.

A DP compliance test expected us to utilize the above fact when all
modes it presented to the DP source were not achievable. It presented
only modes that would be achievable with more lanes and/or higher
speeds than we had available and expected that when we couldn't do
that then we'd fall back to 640x480 even though it didn't advertise
this size.

In order to pass the compliance test (and also support any users who
might fall into a similar situation with their display), we need to
add 640x480 into the list of modes. However, we don't want to add
640x480 all the time. Despite the fact that the DP spec says all sinks
_shall support_ 640x480, they're not guaranteed to support it
_well_. Continuing to read the spec you can see that the display is
not required to really treat 640x480 equal to all the other modes. It
doesn't need to scale or anything--just display the pixels somehow for
failsafe purposes. It should also be noted that it's not hard to find
a display hooked up via DisplayPort that _doesn't_ support 640x480 at
all. The HP ZR30w screen I'm sitting in front of has a native DP port
and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
display via a DP to HDMI adapter and that screen definitely doesn't
support 640x480.

As a compromise solution, let's only add the 640x480 mode if:
* We're on DP.
* All other modes have been pruned.

This acknowledges that 640x480 might not be the best mode to use but,
since sinks are _supposed_ to support it, we will at least fall back
to it if there's nothing else.

Note that we _don't_ add higher resolution modes like 1024x768 in this
case. We only add those modes for a failed EDID read where we have no
idea what's going on. In the case where we've pruned all modes then
instead we only want 640x480 which is the only defined "Fail Safe"
resolution.

This patch originated in response to Kuogee Hsieh's patch [1].

[1] https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 819225629010..90cd46cbfec1 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -476,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 	const struct drm_connector_helper_funcs *connector_funcs =
 		connector->helper_private;
 	int count = 0, ret;
-	bool verbose_prune = true;
 	enum drm_connector_status old_status;
 	struct drm_modeset_acquire_ctx ctx;
 
@@ -556,8 +555,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
 			connector->base.id, connector->name);
 		drm_connector_update_edid_property(connector, NULL);
-		verbose_prune = false;
-		goto prune;
+		drm_mode_prune_invalid(dev, &connector->modes, false);
+		goto exit;
 	}
 
 	count = (*connector_funcs->get_modes)(connector);
@@ -580,9 +579,26 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		}
 	}
 
-prune:
-	drm_mode_prune_invalid(dev, &connector->modes, verbose_prune);
+	drm_mode_prune_invalid(dev, &connector->modes, true);
 
+	/*
+	 * Displayport spec section 5.2.1.2 ("Video Timing Format") says that
+	 * all detachable sinks shall support 640x480 @60Hz as a fail safe
+	 * mode. If all modes were pruned, perhaps because they need more
+	 * lanes or a higher pixel clock than available, at least try to add
+	 * in 640x480.
+	 */
+	if (list_empty(&connector->modes) &&
+	    connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
+		count = drm_add_modes_noedid(connector, 640, 480);
+		if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) {
+			drm_modeset_backoff(&ctx);
+			goto retry;
+		}
+		drm_mode_prune_invalid(dev, &connector->modes, true);
+	}
+
+exit:
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Abhinav Kumar]

Hi Doug

One minor comment below.

But otherwise, looking at this change this should work for us acc to me.

We will test this out with our equipment and then provide R-b.

Thanks

Abhinav
On 4/26/2022 11:46 AM, Douglas Anderson wrote:
> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
> that all detachable sinks shall support 640x480 @60Hz as a fail safe
> mode.
> 
> A DP compliance test expected us to utilize the above fact when all
> modes it presented to the DP source were not achievable. It presented
> only modes that would be achievable with more lanes and/or higher
> speeds than we had available and expected that when we couldn't do
> that then we'd fall back to 640x480 even though it didn't advertise
> this size.
> 
> In order to pass the compliance test (and also support any users who
> might fall into a similar situation with their display), we need to
> add 640x480 into the list of modes. However, we don't want to add
> 640x480 all the time. Despite the fact that the DP spec says all sinks
> _shall support_ 640x480, they're not guaranteed to support it
> _well_. Continuing to read the spec you can see that the display is
> not required to really treat 640x480 equal to all the other modes. It
> doesn't need to scale or anything--just display the pixels somehow for
> failsafe purposes. It should also be noted that it's not hard to find
> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
> all. The HP ZR30w screen I'm sitting in front of has a native DP port
> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
> display via a DP to HDMI adapter and that screen definitely doesn't
> support 640x480.
> 
> As a compromise solution, let's only add the 640x480 mode if:
> * We're on DP.
> * All other modes have been pruned.
> 
> This acknowledges that 640x480 might not be the best mode to use but,
> since sinks are _supposed_ to support it, we will at least fall back
> to it if there's nothing else.
> 
> Note that we _don't_ add higher resolution modes like 1024x768 in this
> case. We only add those modes for a failed EDID read where we have no
> idea what's going on. In the case where we've pruned all modes then
> instead we only want 640x480 which is the only defined "Fail Safe"
> resolution.
> 
> This patch originated in response to Kuogee Hsieh's patch [1].
> 
> [1] https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 819225629010..90cd46cbfec1 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -476,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>   	const struct drm_connector_helper_funcs *connector_funcs =
>   		connector->helper_private;
>   	int count = 0, ret;
> -	bool verbose_prune = true;
>   	enum drm_connector_status old_status;
>   	struct drm_modeset_acquire_ctx ctx;
>   
> @@ -556,8 +555,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>   		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
>   			connector->base.id, connector->name);
>   		drm_connector_update_edid_property(connector, NULL);
> -		verbose_prune = false;
> -		goto prune;
> +		drm_mode_prune_invalid(dev, &connector->modes, false);
> +		goto exit;
>   	}
>   
>   	count = (*connector_funcs->get_modes)(connector);
> @@ -580,9 +579,26 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>   		}
>   	}
>   
> -prune:
> -	drm_mode_prune_invalid(dev, &connector->modes, verbose_prune);
> +	drm_mode_prune_invalid(dev, &connector->modes, true);
>   
> +	/*
> +	 * Displayport spec section 5.2.1.2 ("Video Timing Format") says that
> +	 * all detachable sinks shall support 640x480 @60Hz as a fail safe
> +	 * mode. If all modes were pruned, perhaps because they need more
> +	 * lanes or a higher pixel clock than available, at least try to add
> +	 * in 640x480.
> +	 */
> +	if (list_empty(&connector->modes) &&
> +	    connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
> +		count = drm_add_modes_noedid(connector, 640, 480);
> +		if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) {
> +			drm_modeset_backoff(&ctx);
> +			goto retry;

Do we need another retry here? This will again repeat everything from
get_modes().
The fact that we are hitting this code is because we have already tried 
that and this is already a second-pass. So I think another retry isnt 
needed?

> +		}
> +		drm_mode_prune_invalid(dev, &connector->modes, true);
> +	}
> +
> +exit:
>   	drm_modeset_drop_locks(&ctx);
>   	drm_modeset_acquire_fini(&ctx);
>   

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Abhinav Kumar]

Missed one more comment.

On 4/26/2022 12:16 PM, Abhinav Kumar wrote:
> Hi Doug
> 
> One minor comment below.
> 
> But otherwise, looking at this change this should work for us acc to me.
> 
> We will test this out with our equipment and then provide R-b.
> 
> Thanks
> 
> Abhinav
> On 4/26/2022 11:46 AM, Douglas Anderson wrote:
>> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
>> that all detachable sinks shall support 640x480 @60Hz as a fail safe
>> mode.
>>
>> A DP compliance test expected us to utilize the above fact when all
>> modes it presented to the DP source were not achievable. It presented
>> only modes that would be achievable with more lanes and/or higher
>> speeds than we had available and expected that when we couldn't do
>> that then we'd fall back to 640x480 even though it didn't advertise
>> this size.
>>
>> In order to pass the compliance test (and also support any users who
>> might fall into a similar situation with their display), we need to
>> add 640x480 into the list of modes. However, we don't want to add
>> 640x480 all the time. Despite the fact that the DP spec says all sinks
>> _shall support_ 640x480, they're not guaranteed to support it
>> _well_. Continuing to read the spec you can see that the display is
>> not required to really treat 640x480 equal to all the other modes. It
>> doesn't need to scale or anything--just display the pixels somehow for
>> failsafe purposes. It should also be noted that it's not hard to find
>> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
>> all. The HP ZR30w screen I'm sitting in front of has a native DP port
>> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
>> display via a DP to HDMI adapter and that screen definitely doesn't
>> support 640x480.
>>
>> As a compromise solution, let's only add the 640x480 mode if:
>> * We're on DP.
>> * All other modes have been pruned.
>>
>> This acknowledges that 640x480 might not be the best mode to use but,
>> since sinks are _supposed_ to support it, we will at least fall back
>> to it if there's nothing else.
>>
>> Note that we _don't_ add higher resolution modes like 1024x768 in this
>> case. We only add those modes for a failed EDID read where we have no
>> idea what's going on. In the case where we've pruned all modes then
>> instead we only want 640x480 which is the only defined "Fail Safe"
>> resolution.
>>
>> This patch originated in response to Kuogee Hsieh's patch [1].
>>
>> [1] 
>> https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com 
>>
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>
>>   drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
>>   1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c 
>> b/drivers/gpu/drm/drm_probe_helper.c
>> index 819225629010..90cd46cbfec1 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -476,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct 
>> drm_connector *connector,
>>       const struct drm_connector_helper_funcs *connector_funcs =
>>           connector->helper_private;
>>       int count = 0, ret;
>> -    bool verbose_prune = true;
>>       enum drm_connector_status old_status;
>>       struct drm_modeset_acquire_ctx ctx;
>> @@ -556,8 +555,8 @@ int drm_helper_probe_single_connector_modes(struct 
>> drm_connector *connector,
>>           DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
>>               connector->base.id, connector->name);
>>           drm_connector_update_edid_property(connector, NULL);
>> -        verbose_prune = false;
>> -        goto prune;
>> +        drm_mode_prune_invalid(dev, &connector->modes, false);
>> +        goto exit;
>>       }
>>       count = (*connector_funcs->get_modes)(connector);
>> @@ -580,9 +579,26 @@ int 
>> drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>           }
>>       }
>> -prune:
>> -    drm_mode_prune_invalid(dev, &connector->modes, verbose_prune);
>> +    drm_mode_prune_invalid(dev, &connector->modes, true);
>> +    /*
>> +     * Displayport spec section 5.2.1.2 ("Video Timing Format") says 
>> that
>> +     * all detachable sinks shall support 640x480 @60Hz as a fail safe
>> +     * mode. If all modes were pruned, perhaps because they need more
>> +     * lanes or a higher pixel clock than available, at least try to add
>> +     * in 640x480.
>> +     */
>> +    if (list_empty(&connector->modes) &&
>> +        connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
>> +        count = drm_add_modes_noedid(connector, 640, 480);
>> +        if (_drm_helper_update_and_validate(connector, maxX, maxY, 
>> &ctx)) {
>> +            drm_modeset_backoff(&ctx);
>> +            goto retry;
> 
> Do we need another retry here? This will again repeat everything from
> get_modes().
> The fact that we are hitting this code is because we have already tried 
> that and this is already a second-pass. So I think another retry isnt 
> needed?

This will help cover the case of 4.2.2.6 but not fix 4.2.2.1.

For 4.2.2.1, we will have 0 modes and so the original DRM fwk code of 
adding all modes <= 1024x768 will kick in.

Now, in that list, we will still need to pick/mark 640x480 as the 
preferred mode.

We still need IGT for that.

So yes, this will cover one of the test but not the other.
> 
>> +        }
>> +        drm_mode_prune_invalid(dev, &connector->modes, true);
>> +    }
>> +
>> +exit:
>>       drm_modeset_drop_locks(&ctx);
>>       drm_modeset_acquire_fini(&ctx);

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Doug Anderson]

Hi,

On Tue, Apr 26, 2022 at 12:16 PM Abhinav Kumar
<quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug
>
> One minor comment below.
>
> But otherwise, looking at this change this should work for us acc to me.
>
> We will test this out with our equipment and then provide R-b.
>
> Thanks
>
> Abhinav
> On 4/26/2022 11:46 AM, Douglas Anderson wrote:
> > As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
> > that all detachable sinks shall support 640x480 @60Hz as a fail safe
> > mode.
> >
> > A DP compliance test expected us to utilize the above fact when all
> > modes it presented to the DP source were not achievable. It presented
> > only modes that would be achievable with more lanes and/or higher
> > speeds than we had available and expected that when we couldn't do
> > that then we'd fall back to 640x480 even though it didn't advertise
> > this size.
> >
> > In order to pass the compliance test (and also support any users who
> > might fall into a similar situation with their display), we need to
> > add 640x480 into the list of modes. However, we don't want to add
> > 640x480 all the time. Despite the fact that the DP spec says all sinks
> > _shall support_ 640x480, they're not guaranteed to support it
> > _well_. Continuing to read the spec you can see that the display is
> > not required to really treat 640x480 equal to all the other modes. It
> > doesn't need to scale or anything--just display the pixels somehow for
> > failsafe purposes. It should also be noted that it's not hard to find
> > a display hooked up via DisplayPort that _doesn't_ support 640x480 at
> > all. The HP ZR30w screen I'm sitting in front of has a native DP port
> > and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
> > display via a DP to HDMI adapter and that screen definitely doesn't
> > support 640x480.
> >
> > As a compromise solution, let's only add the 640x480 mode if:
> > * We're on DP.
> > * All other modes have been pruned.
> >
> > This acknowledges that 640x480 might not be the best mode to use but,
> > since sinks are _supposed_ to support it, we will at least fall back
> > to it if there's nothing else.
> >
> > Note that we _don't_ add higher resolution modes like 1024x768 in this
> > case. We only add those modes for a failed EDID read where we have no
> > idea what's going on. In the case where we've pruned all modes then
> > instead we only want 640x480 which is the only defined "Fail Safe"
> > resolution.
> >
> > This patch originated in response to Kuogee Hsieh's patch [1].
> >
> > [1] https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >   drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
> >   1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 819225629010..90cd46cbfec1 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -476,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >       const struct drm_connector_helper_funcs *connector_funcs =
> >               connector->helper_private;
> >       int count = 0, ret;
> > -     bool verbose_prune = true;
> >       enum drm_connector_status old_status;
> >       struct drm_modeset_acquire_ctx ctx;
> >
> > @@ -556,8 +555,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >               DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
> >                       connector->base.id, connector->name);
> >               drm_connector_update_edid_property(connector, NULL);
> > -             verbose_prune = false;
> > -             goto prune;
> > +             drm_mode_prune_invalid(dev, &connector->modes, false);
> > +             goto exit;
> >       }
> >
> >       count = (*connector_funcs->get_modes)(connector);
> > @@ -580,9 +579,26 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >               }
> >       }
> >
> > -prune:
> > -     drm_mode_prune_invalid(dev, &connector->modes, verbose_prune);
> > +     drm_mode_prune_invalid(dev, &connector->modes, true);
> >
> > +     /*
> > +      * Displayport spec section 5.2.1.2 ("Video Timing Format") says that
> > +      * all detachable sinks shall support 640x480 @60Hz as a fail safe
> > +      * mode. If all modes were pruned, perhaps because they need more
> > +      * lanes or a higher pixel clock than available, at least try to add
> > +      * in 640x480.
> > +      */
> > +     if (list_empty(&connector->modes) &&
> > +         connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
> > +             count = drm_add_modes_noedid(connector, 640, 480);
> > +             if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) {
> > +                     drm_modeset_backoff(&ctx);
> > +                     goto retry;
>
> Do we need another retry here? This will again repeat everything from
> get_modes().
> The fact that we are hitting this code is because we have already tried
> that and this is already a second-pass. So I think another retry isnt
> needed?

The retry is still needed. This gets into the whole wait-wound mutexes
that DRM uses, right? Any time we detect deadlock we release all of
our locks and start from scratch. That's still possible here.

-Doug

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Doug Anderson]

Hi,

On Tue, Apr 26, 2022 at 12:20 PM Abhinav Kumar
<quic_abhinavk@quicinc.com> wrote:
>
> Missed one more comment.
>
> On 4/26/2022 12:16 PM, Abhinav Kumar wrote:
> > Hi Doug
> >
> > One minor comment below.
> >
> > But otherwise, looking at this change this should work for us acc to me.
> >
> > We will test this out with our equipment and then provide R-b.
> >
> > Thanks
> >
> > Abhinav
> > On 4/26/2022 11:46 AM, Douglas Anderson wrote:
> >> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
> >> that all detachable sinks shall support 640x480 @60Hz as a fail safe
> >> mode.
> >>
> >> A DP compliance test expected us to utilize the above fact when all
> >> modes it presented to the DP source were not achievable. It presented
> >> only modes that would be achievable with more lanes and/or higher
> >> speeds than we had available and expected that when we couldn't do
> >> that then we'd fall back to 640x480 even though it didn't advertise
> >> this size.
> >>
> >> In order to pass the compliance test (and also support any users who
> >> might fall into a similar situation with their display), we need to
> >> add 640x480 into the list of modes. However, we don't want to add
> >> 640x480 all the time. Despite the fact that the DP spec says all sinks
> >> _shall support_ 640x480, they're not guaranteed to support it
> >> _well_. Continuing to read the spec you can see that the display is
> >> not required to really treat 640x480 equal to all the other modes. It
> >> doesn't need to scale or anything--just display the pixels somehow for
> >> failsafe purposes. It should also be noted that it's not hard to find
> >> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
> >> all. The HP ZR30w screen I'm sitting in front of has a native DP port
> >> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
> >> display via a DP to HDMI adapter and that screen definitely doesn't
> >> support 640x480.
> >>
> >> As a compromise solution, let's only add the 640x480 mode if:
> >> * We're on DP.
> >> * All other modes have been pruned.
> >>
> >> This acknowledges that 640x480 might not be the best mode to use but,
> >> since sinks are _supposed_ to support it, we will at least fall back
> >> to it if there's nothing else.
> >>
> >> Note that we _don't_ add higher resolution modes like 1024x768 in this
> >> case. We only add those modes for a failed EDID read where we have no
> >> idea what's going on. In the case where we've pruned all modes then
> >> instead we only want 640x480 which is the only defined "Fail Safe"
> >> resolution.
> >>
> >> This patch originated in response to Kuogee Hsieh's patch [1].
> >>
> >> [1]
> >> https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
> >>
> >>
> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >> ---
> >>
> >>   drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
> >>   1 file changed, 21 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> >> b/drivers/gpu/drm/drm_probe_helper.c
> >> index 819225629010..90cd46cbfec1 100644
> >> --- a/drivers/gpu/drm/drm_probe_helper.c
> >> +++ b/drivers/gpu/drm/drm_probe_helper.c
> >> @@ -476,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct
> >> drm_connector *connector,
> >>       const struct drm_connector_helper_funcs *connector_funcs =
> >>           connector->helper_private;
> >>       int count = 0, ret;
> >> -    bool verbose_prune = true;
> >>       enum drm_connector_status old_status;
> >>       struct drm_modeset_acquire_ctx ctx;
> >> @@ -556,8 +555,8 @@ int drm_helper_probe_single_connector_modes(struct
> >> drm_connector *connector,
> >>           DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
> >>               connector->base.id, connector->name);
> >>           drm_connector_update_edid_property(connector, NULL);
> >> -        verbose_prune = false;
> >> -        goto prune;
> >> +        drm_mode_prune_invalid(dev, &connector->modes, false);
> >> +        goto exit;
> >>       }
> >>       count = (*connector_funcs->get_modes)(connector);
> >> @@ -580,9 +579,26 @@ int
> >> drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >>           }
> >>       }
> >> -prune:
> >> -    drm_mode_prune_invalid(dev, &connector->modes, verbose_prune);
> >> +    drm_mode_prune_invalid(dev, &connector->modes, true);
> >> +    /*
> >> +     * Displayport spec section 5.2.1.2 ("Video Timing Format") says
> >> that
> >> +     * all detachable sinks shall support 640x480 @60Hz as a fail safe
> >> +     * mode. If all modes were pruned, perhaps because they need more
> >> +     * lanes or a higher pixel clock than available, at least try to add
> >> +     * in 640x480.
> >> +     */
> >> +    if (list_empty(&connector->modes) &&
> >> +        connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
> >> +        count = drm_add_modes_noedid(connector, 640, 480);
> >> +        if (_drm_helper_update_and_validate(connector, maxX, maxY,
> >> &ctx)) {
> >> +            drm_modeset_backoff(&ctx);
> >> +            goto retry;
> >
> > Do we need another retry here? This will again repeat everything from
> > get_modes().
> > The fact that we are hitting this code is because we have already tried
> > that and this is already a second-pass. So I think another retry isnt
> > needed?
>
> This will help cover the case of 4.2.2.6 but not fix 4.2.2.1.
>
> For 4.2.2.1, we will have 0 modes and so the original DRM fwk code of
> adding all modes <= 1024x768 will kick in.
>
> Now, in that list, we will still need to pick/mark 640x480 as the
> preferred mode.
>
> We still need IGT for that.

Are you sure you don't have those backwards? It seems like 4.2.2.6 is
the test case dealing with corrupt EDID and that's the one that will
still be broken, no? ...and corrupt EDID is still the case where we
have 0 modes.

In any case, let's see what people think about:

https://lore.kernel.org/r/20220426132121.RFC.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid

I've marked that one as RFC just because it seems like a bigger change
to existing behavior, though it still seems correct to me.

NOTE: reading 4.2.2.6 more closely, it actually looks as if we're
actually supposed to be able to try various video modes one at a time
until we find one that works (or land on 640x480). Seems as if we're
supposed to be able to try the higher resolutions one at a time and we
can tell whether the sink "accepted" it by seeing if SINK_STATUS goes
to 1? I have no idea how that works with all the Linux APIs, though.

-Doug

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Abhinav Kumar]

On 4/26/2022 1:26 PM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Apr 26, 2022 at 12:20 PM Abhinav Kumar
> <quic_abhinavk@quicinc.com> wrote:
>>
>> Missed one more comment.
>>
>> On 4/26/2022 12:16 PM, Abhinav Kumar wrote:
>>> Hi Doug
>>>
>>> One minor comment below.
>>>
>>> But otherwise, looking at this change this should work for us acc to me.
>>>
>>> We will test this out with our equipment and then provide R-b.
>>>
>>> Thanks
>>>
>>> Abhinav
>>> On 4/26/2022 11:46 AM, Douglas Anderson wrote:
>>>> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
>>>> that all detachable sinks shall support 640x480 @60Hz as a fail safe
>>>> mode.
>>>>
>>>> A DP compliance test expected us to utilize the above fact when all
>>>> modes it presented to the DP source were not achievable. It presented
>>>> only modes that would be achievable with more lanes and/or higher
>>>> speeds than we had available and expected that when we couldn't do
>>>> that then we'd fall back to 640x480 even though it didn't advertise
>>>> this size.
>>>>
>>>> In order to pass the compliance test (and also support any users who
>>>> might fall into a similar situation with their display), we need to
>>>> add 640x480 into the list of modes. However, we don't want to add
>>>> 640x480 all the time. Despite the fact that the DP spec says all sinks
>>>> _shall support_ 640x480, they're not guaranteed to support it
>>>> _well_. Continuing to read the spec you can see that the display is
>>>> not required to really treat 640x480 equal to all the other modes. It
>>>> doesn't need to scale or anything--just display the pixels somehow for
>>>> failsafe purposes. It should also be noted that it's not hard to find
>>>> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
>>>> all. The HP ZR30w screen I'm sitting in front of has a native DP port
>>>> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
>>>> display via a DP to HDMI adapter and that screen definitely doesn't
>>>> support 640x480.
>>>>
>>>> As a compromise solution, let's only add the 640x480 mode if:
>>>> * We're on DP.
>>>> * All other modes have been pruned.
>>>>
>>>> This acknowledges that 640x480 might not be the best mode to use but,
>>>> since sinks are _supposed_ to support it, we will at least fall back
>>>> to it if there's nothing else.
>>>>
>>>> Note that we _don't_ add higher resolution modes like 1024x768 in this
>>>> case. We only add those modes for a failed EDID read where we have no
>>>> idea what's going on. In the case where we've pruned all modes then
>>>> instead we only want 640x480 which is the only defined "Fail Safe"
>>>> resolution.
>>>>
>>>> This patch originated in response to Kuogee Hsieh's patch [1].
>>>>
>>>> [1]
>>>> https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
>>>>
>>>>
>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>> ---
>>>>
>>>>    drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
>>>>    1 file changed, 21 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
>>>> b/drivers/gpu/drm/drm_probe_helper.c
>>>> index 819225629010..90cd46cbfec1 100644
>>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>>> @@ -476,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct
>>>> drm_connector *connector,
>>>>        const struct drm_connector_helper_funcs *connector_funcs =
>>>>            connector->helper_private;
>>>>        int count = 0, ret;
>>>> -    bool verbose_prune = true;
>>>>        enum drm_connector_status old_status;
>>>>        struct drm_modeset_acquire_ctx ctx;
>>>> @@ -556,8 +555,8 @@ int drm_helper_probe_single_connector_modes(struct
>>>> drm_connector *connector,
>>>>            DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
>>>>                connector->base.id, connector->name);
>>>>            drm_connector_update_edid_property(connector, NULL);
>>>> -        verbose_prune = false;
>>>> -        goto prune;
>>>> +        drm_mode_prune_invalid(dev, &connector->modes, false);
>>>> +        goto exit;
>>>>        }
>>>>        count = (*connector_funcs->get_modes)(connector);
>>>> @@ -580,9 +579,26 @@ int
>>>> drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>>>            }
>>>>        }
>>>> -prune:
>>>> -    drm_mode_prune_invalid(dev, &connector->modes, verbose_prune);
>>>> +    drm_mode_prune_invalid(dev, &connector->modes, true);
>>>> +    /*
>>>> +     * Displayport spec section 5.2.1.2 ("Video Timing Format") says
>>>> that
>>>> +     * all detachable sinks shall support 640x480 @60Hz as a fail safe
>>>> +     * mode. If all modes were pruned, perhaps because they need more
>>>> +     * lanes or a higher pixel clock than available, at least try to add
>>>> +     * in 640x480.
>>>> +     */
>>>> +    if (list_empty(&connector->modes) &&
>>>> +        connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
>>>> +        count = drm_add_modes_noedid(connector, 640, 480);
>>>> +        if (_drm_helper_update_and_validate(connector, maxX, maxY,
>>>> &ctx)) {
>>>> +            drm_modeset_backoff(&ctx);
>>>> +            goto retry;
>>>
>>> Do we need another retry here? This will again repeat everything from
>>> get_modes().
>>> The fact that we are hitting this code is because we have already tried
>>> that and this is already a second-pass. So I think another retry isnt
>>> needed?
>>
>> This will help cover the case of 4.2.2.6 but not fix 4.2.2.1.
>>
>> For 4.2.2.1, we will have 0 modes and so the original DRM fwk code of
>> adding all modes <= 1024x768 will kick in.
>>
>> Now, in that list, we will still need to pick/mark 640x480 as the
>> preferred mode.
>>
>> We still need IGT for that.
> 
> Are you sure you don't have those backwards? It seems like 4.2.2.6 is
> the test case dealing with corrupt EDID and that's the one that will
> still be broken, no? ...and corrupt EDID is still the case where we
> have 0 modes.

Yes indeed, sorry, I did have the numbers backwards.
4.2.2.6 will still be broken.

> 
> In any case, let's see what people think about:
> 
> https://lore.kernel.org/r/20220426132121.RFC.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid

Yes sure. If it gets accepted, it will save us some IGT work.

> 
> I've marked that one as RFC just because it seems like a bigger change
> to existing behavior, though it still seems correct to me.
> 
> NOTE: reading 4.2.2.6 more closely, it actually looks as if we're
> actually supposed to be able to try various video modes one at a time
> until we find one that works (or land on 640x480). Seems as if we're
> supposed to be able to try the higher resolutions one at a time and we
> can tell whether the sink "accepted" it by seeing if SINK_STATUS goes
> to 1? I have no idea how that works with all the Linux APIs, though.
> 

hmmm .... our equipment throws a warning if we dont sent 640x480. So 
perhaps just go with the "or land on 640x480" option.

0006.392.232: [WARNING] Source DUT failed to transmit a video stream 
using fail-safe mode
0006.392.491:   Received 1344 Htotal differs from fail-safe 800
0006.392.621:   Received 1024 Hactive differs from fail-safe 640
0006.392.750:   Received 296 Hstart differs from fail-safe 144
0006.392.868:   Received 136 Hsync width differs from fail-safe 96
0006.392.975:   Received 806 Vtotal differs from fail-safe 525
0006.393.099:   Received 768 Vactive differs from fail-safe 480
0006.393.229:   Received 6 Vsync width differs from fail-safe 2


> -Doug

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Doug Anderson]

Hi,

On Tue, Apr 26, 2022 at 2:11 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 4/26/2022 1:26 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 26, 2022 at 12:20 PM Abhinav Kumar
> > <quic_abhinavk@quicinc.com> wrote:
> >>
> >> Missed one more comment.
> >>
> >> On 4/26/2022 12:16 PM, Abhinav Kumar wrote:
> >>> Hi Doug
> >>>
> >>> One minor comment below.
> >>>
> >>> But otherwise, looking at this change this should work for us acc to me.
> >>>
> >>> We will test this out with our equipment and then provide R-b.
> >>>
> >>> Thanks
> >>>
> >>> Abhinav
> >>> On 4/26/2022 11:46 AM, Douglas Anderson wrote:
> >>>> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
> >>>> that all detachable sinks shall support 640x480 @60Hz as a fail safe
> >>>> mode.
> >>>>
> >>>> A DP compliance test expected us to utilize the above fact when all
> >>>> modes it presented to the DP source were not achievable. It presented
> >>>> only modes that would be achievable with more lanes and/or higher
> >>>> speeds than we had available and expected that when we couldn't do
> >>>> that then we'd fall back to 640x480 even though it didn't advertise
> >>>> this size.
> >>>>
> >>>> In order to pass the compliance test (and also support any users who
> >>>> might fall into a similar situation with their display), we need to
> >>>> add 640x480 into the list of modes. However, we don't want to add
> >>>> 640x480 all the time. Despite the fact that the DP spec says all sinks
> >>>> _shall support_ 640x480, they're not guaranteed to support it
> >>>> _well_. Continuing to read the spec you can see that the display is
> >>>> not required to really treat 640x480 equal to all the other modes. It
> >>>> doesn't need to scale or anything--just display the pixels somehow for
> >>>> failsafe purposes. It should also be noted that it's not hard to find
> >>>> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
> >>>> all. The HP ZR30w screen I'm sitting in front of has a native DP port
> >>>> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
> >>>> display via a DP to HDMI adapter and that screen definitely doesn't
> >>>> support 640x480.
> >>>>
> >>>> As a compromise solution, let's only add the 640x480 mode if:
> >>>> * We're on DP.
> >>>> * All other modes have been pruned.
> >>>>
> >>>> This acknowledges that 640x480 might not be the best mode to use but,
> >>>> since sinks are _supposed_ to support it, we will at least fall back
> >>>> to it if there's nothing else.
> >>>>
> >>>> Note that we _don't_ add higher resolution modes like 1024x768 in this
> >>>> case. We only add those modes for a failed EDID read where we have no
> >>>> idea what's going on. In the case where we've pruned all modes then
> >>>> instead we only want 640x480 which is the only defined "Fail Safe"
> >>>> resolution.
> >>>>
> >>>> This patch originated in response to Kuogee Hsieh's patch [1].
> >>>>
> >>>> [1]
> >>>> https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
> >>>>
> >>>>
> >>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>>> ---
> >>>>
> >>>>    drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
> >>>>    1 file changed, 21 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> >>>> b/drivers/gpu/drm/drm_probe_helper.c
> >>>> index 819225629010..90cd46cbfec1 100644
> >>>> --- a/drivers/gpu/drm/drm_probe_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
> >>>> @@ -476,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct
> >>>> drm_connector *connector,
> >>>>        const struct drm_connector_helper_funcs *connector_funcs =
> >>>>            connector->helper_private;
> >>>>        int count = 0, ret;
> >>>> -    bool verbose_prune = true;
> >>>>        enum drm_connector_status old_status;
> >>>>        struct drm_modeset_acquire_ctx ctx;
> >>>> @@ -556,8 +555,8 @@ int drm_helper_probe_single_connector_modes(struct
> >>>> drm_connector *connector,
> >>>>            DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
> >>>>                connector->base.id, connector->name);
> >>>>            drm_connector_update_edid_property(connector, NULL);
> >>>> -        verbose_prune = false;
> >>>> -        goto prune;
> >>>> +        drm_mode_prune_invalid(dev, &connector->modes, false);
> >>>> +        goto exit;
> >>>>        }
> >>>>        count = (*connector_funcs->get_modes)(connector);
> >>>> @@ -580,9 +579,26 @@ int
> >>>> drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >>>>            }
> >>>>        }
> >>>> -prune:
> >>>> -    drm_mode_prune_invalid(dev, &connector->modes, verbose_prune);
> >>>> +    drm_mode_prune_invalid(dev, &connector->modes, true);
> >>>> +    /*
> >>>> +     * Displayport spec section 5.2.1.2 ("Video Timing Format") says
> >>>> that
> >>>> +     * all detachable sinks shall support 640x480 @60Hz as a fail safe
> >>>> +     * mode. If all modes were pruned, perhaps because they need more
> >>>> +     * lanes or a higher pixel clock than available, at least try to add
> >>>> +     * in 640x480.
> >>>> +     */
> >>>> +    if (list_empty(&connector->modes) &&
> >>>> +        connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
> >>>> +        count = drm_add_modes_noedid(connector, 640, 480);
> >>>> +        if (_drm_helper_update_and_validate(connector, maxX, maxY,
> >>>> &ctx)) {
> >>>> +            drm_modeset_backoff(&ctx);
> >>>> +            goto retry;
> >>>
> >>> Do we need another retry here? This will again repeat everything from
> >>> get_modes().
> >>> The fact that we are hitting this code is because we have already tried
> >>> that and this is already a second-pass. So I think another retry isnt
> >>> needed?
> >>
> >> This will help cover the case of 4.2.2.6 but not fix 4.2.2.1.
> >>
> >> For 4.2.2.1, we will have 0 modes and so the original DRM fwk code of
> >> adding all modes <= 1024x768 will kick in.
> >>
> >> Now, in that list, we will still need to pick/mark 640x480 as the
> >> preferred mode.
> >>
> >> We still need IGT for that.
> >
> > Are you sure you don't have those backwards? It seems like 4.2.2.6 is
> > the test case dealing with corrupt EDID and that's the one that will
> > still be broken, no? ...and corrupt EDID is still the case where we
> > have 0 modes.
>
> Yes indeed, sorry, I did have the numbers backwards.
> 4.2.2.6 will still be broken.
>
> >
> > In any case, let's see what people think about:
> >
> > https://lore.kernel.org/r/20220426132121.RFC.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid
>
> Yes sure. If it gets accepted, it will save us some IGT work.
>
> >
> > I've marked that one as RFC just because it seems like a bigger change
> > to existing behavior, though it still seems correct to me.
> >
> > NOTE: reading 4.2.2.6 more closely, it actually looks as if we're
> > actually supposed to be able to try various video modes one at a time
> > until we find one that works (or land on 640x480). Seems as if we're
> > supposed to be able to try the higher resolutions one at a time and we
> > can tell whether the sink "accepted" it by seeing if SINK_STATUS goes
> > to 1? I have no idea how that works with all the Linux APIs, though.
> >
>
> hmmm .... our equipment throws a warning if we dont sent 640x480. So
> perhaps just go with the "or land on 640x480" option.
>
> 0006.392.232: [WARNING] Source DUT failed to transmit a video stream
> using fail-safe mode
> 0006.392.491:   Received 1344 Htotal differs from fail-safe 800
> 0006.392.621:   Received 1024 Hactive differs from fail-safe 640
> 0006.392.750:   Received 296 Hstart differs from fail-safe 144
> 0006.392.868:   Received 136 Hsync width differs from fail-safe 96
> 0006.392.975:   Received 806 Vtotal differs from fail-safe 525
> 0006.393.099:   Received 768 Vactive differs from fail-safe 480
> 0006.393.229:   Received 6 Vsync width differs from fail-safe 2

Do you actually have code to implement the checking of SINK_STATUS?
I'm not aware of how that would work in Linux, which is why just
defaulting to 640x480 seems like a reasonable thing to do for now. The
test case actually says that you're allowed to try clock rates one at
a time (polling SINK_STATUS in DPCT) as long as you don't spend more
than 5 seconds on each clock rate. According to the test case if you
never saw SINK_STATUS in DPCT go to 1 then you should end at 640x480.

Re: [PATCH 1/2] drm/probe-helper: Add helper for drm_helper_probe_single_connector_modes()

[Abhinav Kumar]

On 4/26/2022 11:46 AM, Douglas Anderson wrote:
> The drm_helper_probe_single_connector_modes() is a bit long. Let's
> break a chunk off to update and validate modes. This helps avoid one
> goto and also will allow us to more easily call the helper a second
> time in a future patch without adding looping or another goto.
> 
> This change is intended to be a no-op change--just code movement.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

I think this cleanup looks reasonable to me.

Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> 
>   drivers/gpu/drm/drm_probe_helper.c | 105 ++++++++++++++++-------------
>   1 file changed, 59 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 682359512996..819225629010 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -354,6 +354,61 @@ drm_helper_probe_detect(struct drm_connector *connector,
>   }
>   EXPORT_SYMBOL(drm_helper_probe_detect);
>   
> +static bool _drm_helper_update_and_validate(struct drm_connector *connector,
> +					    uint32_t maxX, uint32_t maxY,
> +					    struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_mode *mode;
> +	int mode_flags = 0;
> +	int ret;
> +
> +	drm_connector_list_update(connector);
> +
> +	if (connector->interlace_allowed)
> +		mode_flags |= DRM_MODE_FLAG_INTERLACE;
> +	if (connector->doublescan_allowed)
> +		mode_flags |= DRM_MODE_FLAG_DBLSCAN;
> +	if (connector->stereo_allowed)
> +		mode_flags |= DRM_MODE_FLAG_3D_MASK;
> +
> +	list_for_each_entry(mode, &connector->modes, head) {
> +		if (mode->status != MODE_OK)
> +			continue;
> +
> +		mode->status = drm_mode_validate_driver(dev, mode);
> +		if (mode->status != MODE_OK)
> +			continue;
> +
> +		mode->status = drm_mode_validate_size(mode, maxX, maxY);
> +		if (mode->status != MODE_OK)
> +			continue;
> +
> +		mode->status = drm_mode_validate_flag(mode, mode_flags);
> +		if (mode->status != MODE_OK)
> +			continue;
> +
> +		ret = drm_mode_validate_pipeline(mode, connector, ctx,
> +						 &mode->status);
> +		if (ret) {
> +			drm_dbg_kms(dev,
> +				    "drm_mode_validate_pipeline failed: %d\n",
> +				    ret);
> +
> +			if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK))
> +				mode->status = MODE_ERROR;
> +			else
> +				return true;
> +		}
> +
> +		if (mode->status != MODE_OK)
> +			continue;
> +		mode->status = drm_mode_validate_ycbcr420(mode, connector);
> +	}
> +
> +	return false;
> +}
> +
>   /**
>    * drm_helper_probe_single_connector_modes - get complete set of display modes
>    * @connector: connector to probe
> @@ -421,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>   	const struct drm_connector_helper_funcs *connector_funcs =
>   		connector->helper_private;
>   	int count = 0, ret;
> -	int mode_flags = 0;
>   	bool verbose_prune = true;
>   	enum drm_connector_status old_status;
>   	struct drm_modeset_acquire_ctx ctx;
> @@ -519,52 +573,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>   			   connector->status == connector_status_unknown))
>   		count = drm_add_modes_noedid(connector, 1024, 768);
>   	count += drm_helper_probe_add_cmdline_mode(connector);
> -	if (count == 0)
> -		goto prune;
> -
> -	drm_connector_list_update(connector);
> -
> -	if (connector->interlace_allowed)
> -		mode_flags |= DRM_MODE_FLAG_INTERLACE;
> -	if (connector->doublescan_allowed)
> -		mode_flags |= DRM_MODE_FLAG_DBLSCAN;
> -	if (connector->stereo_allowed)
> -		mode_flags |= DRM_MODE_FLAG_3D_MASK;
> -
> -	list_for_each_entry(mode, &connector->modes, head) {
> -		if (mode->status != MODE_OK)
> -			continue;
> -
> -		mode->status = drm_mode_validate_driver(dev, mode);
> -		if (mode->status != MODE_OK)
> -			continue;
> -
> -		mode->status = drm_mode_validate_size(mode, maxX, maxY);
> -		if (mode->status != MODE_OK)
> -			continue;
> -
> -		mode->status = drm_mode_validate_flag(mode, mode_flags);
> -		if (mode->status != MODE_OK)
> -			continue;
> -
> -		ret = drm_mode_validate_pipeline(mode, connector, &ctx,
> -						 &mode->status);
> -		if (ret) {
> -			drm_dbg_kms(dev,
> -				    "drm_mode_validate_pipeline failed: %d\n",
> -				    ret);
> -
> -			if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK)) {
> -				mode->status = MODE_ERROR;
> -			} else {
> -				drm_modeset_backoff(&ctx);
> -				goto retry;
> -			}
> +	if (count != 0) {
> +		if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) {
> +			drm_modeset_backoff(&ctx);
> +			goto retry;
>   		}
> -
> -		if (mode->status != MODE_OK)
> -			continue;
> -		mode->status = drm_mode_validate_ycbcr420(mode, connector);
>   	}
>   
>   prune:

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Kuogee Hsieh]

Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>

On 4/26/2022 2:17 PM, Doug Anderson wrote:
> Hi,
>
> On Tue, Apr 26, 2022 at 2:11 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>> On 4/26/2022 1:26 PM, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Tue, Apr 26, 2022 at 12:20 PM Abhinav Kumar
>>> <quic_abhinavk@quicinc.com> wrote:
>>>> Missed one more comment.
>>>>
>>>> On 4/26/2022 12:16 PM, Abhinav Kumar wrote:
>>>>> Hi Doug
>>>>>
>>>>> One minor comment below.
>>>>>
>>>>> But otherwise, looking at this change this should work for us acc to me.
>>>>>
>>>>> We will test this out with our equipment and then provide R-b.
>>>>>
>>>>> Thanks
>>>>>
>>>>> Abhinav
>>>>> On 4/26/2022 11:46 AM, Douglas Anderson wrote:
>>>>>> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
>>>>>> that all detachable sinks shall support 640x480 @60Hz as a fail safe
>>>>>> mode.
>>>>>>
>>>>>> A DP compliance test expected us to utilize the above fact when all
>>>>>> modes it presented to the DP source were not achievable. It presented
>>>>>> only modes that would be achievable with more lanes and/or higher
>>>>>> speeds than we had available and expected that when we couldn't do
>>>>>> that then we'd fall back to 640x480 even though it didn't advertise
>>>>>> this size.
>>>>>>
>>>>>> In order to pass the compliance test (and also support any users who
>>>>>> might fall into a similar situation with their display), we need to
>>>>>> add 640x480 into the list of modes. However, we don't want to add
>>>>>> 640x480 all the time. Despite the fact that the DP spec says all sinks
>>>>>> _shall support_ 640x480, they're not guaranteed to support it
>>>>>> _well_. Continuing to read the spec you can see that the display is
>>>>>> not required to really treat 640x480 equal to all the other modes. It
>>>>>> doesn't need to scale or anything--just display the pixels somehow for
>>>>>> failsafe purposes. It should also be noted that it's not hard to find
>>>>>> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
>>>>>> all. The HP ZR30w screen I'm sitting in front of has a native DP port
>>>>>> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
>>>>>> display via a DP to HDMI adapter and that screen definitely doesn't
>>>>>> support 640x480.
>>>>>>
>>>>>> As a compromise solution, let's only add the 640x480 mode if:
>>>>>> * We're on DP.
>>>>>> * All other modes have been pruned.
>>>>>>
>>>>>> This acknowledges that 640x480 might not be the best mode to use but,
>>>>>> since sinks are _supposed_ to support it, we will at least fall back
>>>>>> to it if there's nothing else.
>>>>>>
>>>>>> Note that we _don't_ add higher resolution modes like 1024x768 in this
>>>>>> case. We only add those modes for a failed EDID read where we have no
>>>>>> idea what's going on. In the case where we've pruned all modes then
>>>>>> instead we only want 640x480 which is the only defined "Fail Safe"
>>>>>> resolution.
>>>>>>
>>>>>> This patch originated in response to Kuogee Hsieh's patch [1].
>>>>>>
>>>>>> [1]
>>>>>> https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
>>>>>>
>>>>>>
>>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>>> ---
>>>>>>
>>>>>>     drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
>>>>>>     1 file changed, 21 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
>>>>>> b/drivers/gpu/drm/drm_probe_helper.c
>>>>>> index 819225629010..90cd46cbfec1 100644
>>>>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>>>>> @@ -476,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct
>>>>>> drm_connector *connector,
>>>>>>         const struct drm_connector_helper_funcs *connector_funcs =
>>>>>>             connector->helper_private;
>>>>>>         int count = 0, ret;
>>>>>> -    bool verbose_prune = true;
>>>>>>         enum drm_connector_status old_status;
>>>>>>         struct drm_modeset_acquire_ctx ctx;
>>>>>> @@ -556,8 +555,8 @@ int drm_helper_probe_single_connector_modes(struct
>>>>>> drm_connector *connector,
>>>>>>             DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
>>>>>>                 connector->base.id, connector->name);
>>>>>>             drm_connector_update_edid_property(connector, NULL);
>>>>>> -        verbose_prune = false;
>>>>>> -        goto prune;
>>>>>> +        drm_mode_prune_invalid(dev, &connector->modes, false);
>>>>>> +        goto exit;
>>>>>>         }
>>>>>>         count = (*connector_funcs->get_modes)(connector);
>>>>>> @@ -580,9 +579,26 @@ int
>>>>>> drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>>>>>             }
>>>>>>         }
>>>>>> -prune:
>>>>>> -    drm_mode_prune_invalid(dev, &connector->modes, verbose_prune);
>>>>>> +    drm_mode_prune_invalid(dev, &connector->modes, true);
>>>>>> +    /*
>>>>>> +     * Displayport spec section 5.2.1.2 ("Video Timing Format") says
>>>>>> that
>>>>>> +     * all detachable sinks shall support 640x480 @60Hz as a fail safe
>>>>>> +     * mode. If all modes were pruned, perhaps because they need more
>>>>>> +     * lanes or a higher pixel clock than available, at least try to add
>>>>>> +     * in 640x480.
>>>>>> +     */
>>>>>> +    if (list_empty(&connector->modes) &&
>>>>>> +        connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
>>>>>> +        count = drm_add_modes_noedid(connector, 640, 480);
>>>>>> +        if (_drm_helper_update_and_validate(connector, maxX, maxY,
>>>>>> &ctx)) {
>>>>>> +            drm_modeset_backoff(&ctx);
>>>>>> +            goto retry;
>>>>> Do we need another retry here? This will again repeat everything from
>>>>> get_modes().
>>>>> The fact that we are hitting this code is because we have already tried
>>>>> that and this is already a second-pass. So I think another retry isnt
>>>>> needed?
>>>> This will help cover the case of 4.2.2.6 but not fix 4.2.2.1.
>>>>
>>>> For 4.2.2.1, we will have 0 modes and so the original DRM fwk code of
>>>> adding all modes <= 1024x768 will kick in.
>>>>
>>>> Now, in that list, we will still need to pick/mark 640x480 as the
>>>> preferred mode.
>>>>
>>>> We still need IGT for that.
>>> Are you sure you don't have those backwards? It seems like 4.2.2.6 is
>>> the test case dealing with corrupt EDID and that's the one that will
>>> still be broken, no? ...and corrupt EDID is still the case where we
>>> have 0 modes.
>> Yes indeed, sorry, I did have the numbers backwards.
>> 4.2.2.6 will still be broken.
>>
>>> In any case, let's see what people think about:
>>>
>>> https://lore.kernel.org/r/20220426132121.RFC.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid
>> Yes sure. If it gets accepted, it will save us some IGT work.
>>
>>> I've marked that one as RFC just because it seems like a bigger change
>>> to existing behavior, though it still seems correct to me.
>>>
>>> NOTE: reading 4.2.2.6 more closely, it actually looks as if we're
>>> actually supposed to be able to try various video modes one at a time
>>> until we find one that works (or land on 640x480). Seems as if we're
>>> supposed to be able to try the higher resolutions one at a time and we
>>> can tell whether the sink "accepted" it by seeing if SINK_STATUS goes
>>> to 1? I have no idea how that works with all the Linux APIs, though.
>>>
>> hmmm .... our equipment throws a warning if we dont sent 640x480. So
>> perhaps just go with the "or land on 640x480" option.
>>
>> 0006.392.232: [WARNING] Source DUT failed to transmit a video stream
>> using fail-safe mode
>> 0006.392.491:   Received 1344 Htotal differs from fail-safe 800
>> 0006.392.621:   Received 1024 Hactive differs from fail-safe 640
>> 0006.392.750:   Received 296 Hstart differs from fail-safe 144
>> 0006.392.868:   Received 136 Hsync width differs from fail-safe 96
>> 0006.392.975:   Received 806 Vtotal differs from fail-safe 525
>> 0006.393.099:   Received 768 Vactive differs from fail-safe 480
>> 0006.393.229:   Received 6 Vsync width differs from fail-safe 2
> Do you actually have code to implement the checking of SINK_STATUS?
> I'm not aware of how that would work in Linux, which is why just
> defaulting to 640x480 seems like a reasonable thing to do for now. The
> test case actually says that you're allowed to try clock rates one at
> a time (polling SINK_STATUS in DPCT) as long as you don't spend more
> than 5 seconds on each clock rate. According to the test case if you
> never saw SINK_STATUS in DPCT go to 1 then you should end at 640x480.

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Doug Anderson]

Ville,

On Tue, Apr 26, 2022 at 11:47 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
> that all detachable sinks shall support 640x480 @60Hz as a fail safe
> mode.
>
> A DP compliance test expected us to utilize the above fact when all
> modes it presented to the DP source were not achievable. It presented
> only modes that would be achievable with more lanes and/or higher
> speeds than we had available and expected that when we couldn't do
> that then we'd fall back to 640x480 even though it didn't advertise
> this size.
>
> In order to pass the compliance test (and also support any users who
> might fall into a similar situation with their display), we need to
> add 640x480 into the list of modes. However, we don't want to add
> 640x480 all the time. Despite the fact that the DP spec says all sinks
> _shall support_ 640x480, they're not guaranteed to support it
> _well_. Continuing to read the spec you can see that the display is
> not required to really treat 640x480 equal to all the other modes. It
> doesn't need to scale or anything--just display the pixels somehow for
> failsafe purposes. It should also be noted that it's not hard to find
> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
> all. The HP ZR30w screen I'm sitting in front of has a native DP port
> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
> display via a DP to HDMI adapter and that screen definitely doesn't
> support 640x480.
>
> As a compromise solution, let's only add the 640x480 mode if:
> * We're on DP.
> * All other modes have been pruned.
>
> This acknowledges that 640x480 might not be the best mode to use but,
> since sinks are _supposed_ to support it, we will at least fall back
> to it if there's nothing else.
>
> Note that we _don't_ add higher resolution modes like 1024x768 in this
> case. We only add those modes for a failed EDID read where we have no
> idea what's going on. In the case where we've pruned all modes then
> instead we only want 640x480 which is the only defined "Fail Safe"
> resolution.
>
> This patch originated in response to Kuogee Hsieh's patch [1].
>
> [1] https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)

I think this patch is fairly safe / non-controversial, but someone
suggested you might have an opinion on it and another patch I posted
recently [1] so I wanted to double-check. Just to be clear: I'm hoping
to land _both_ this patch and [1]. If you don't have an opinion,
that's OK too.

Abhinav: I think maybe you're happy with this now? Would you be
willing to give a Reviewed-by?

[1] https://lore.kernel.org/r/20220426132121.RFC.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid

-Doug

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Abhinav Kumar]

Hi Doug

On 5/5/2022 8:44 AM, Doug Anderson wrote:
> Ville,
> 
> On Tue, Apr 26, 2022 at 11:47 AM Douglas Anderson <dianders@chromium.org> wrote:
>>
>> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
>> that all detachable sinks shall support 640x480 @60Hz as a fail safe
>> mode.
>>
>> A DP compliance test expected us to utilize the above fact when all
>> modes it presented to the DP source were not achievable. It presented
>> only modes that would be achievable with more lanes and/or higher
>> speeds than we had available and expected that when we couldn't do
>> that then we'd fall back to 640x480 even though it didn't advertise
>> this size.
>>
>> In order to pass the compliance test (and also support any users who
>> might fall into a similar situation with their display), we need to
>> add 640x480 into the list of modes. However, we don't want to add
>> 640x480 all the time. Despite the fact that the DP spec says all sinks
>> _shall support_ 640x480, they're not guaranteed to support it
>> _well_. Continuing to read the spec you can see that the display is
>> not required to really treat 640x480 equal to all the other modes. It
>> doesn't need to scale or anything--just display the pixels somehow for
>> failsafe purposes. It should also be noted that it's not hard to find
>> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
>> all. The HP ZR30w screen I'm sitting in front of has a native DP port
>> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
>> display via a DP to HDMI adapter and that screen definitely doesn't
>> support 640x480.
>>
>> As a compromise solution, let's only add the 640x480 mode if:
>> * We're on DP.
>> * All other modes have been pruned.
>>
>> This acknowledges that 640x480 might not be the best mode to use but,
>> since sinks are _supposed_ to support it, we will at least fall back
>> to it if there's nothing else.
>>
>> Note that we _don't_ add higher resolution modes like 1024x768 in this
>> case. We only add those modes for a failed EDID read where we have no
>> idea what's going on. In the case where we've pruned all modes then
>> instead we only want 640x480 which is the only defined "Fail Safe"
>> resolution.
>>
>> This patch originated in response to Kuogee Hsieh's patch [1].
>>
>> [1] https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>
>>   drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
>>   1 file changed, 21 insertions(+), 5 deletions(-)
> 
> I think this patch is fairly safe / non-controversial, but someone
> suggested you might have an opinion on it and another patch I posted
> recently [1] so I wanted to double-check. Just to be clear: I'm hoping
> to land _both_ this patch and [1]. If you don't have an opinion,
> that's OK too.
> 
> Abhinav: I think maybe you're happy with this now? Would you be
> willing to give a Reviewed-by?

Yes, I have no concerns with this approach from DP spec standpoint and 
in addition, kuogee has tested this out and this does help us to pass 
the tests.

Although, I might be missing some historical context on why this is not 
already done.

But apart from that, LGTM. Hence,

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

> 
> [1] https://lore.kernel.org/r/20220426132121.RFC.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid
> 
> -Doug

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Kuogee Hsieh]

On 5/5/2022 10:20 AM, Abhinav Kumar wrote:
> Hi Doug
>
> On 5/5/2022 8:44 AM, Doug Anderson wrote:
>> Ville,
>>
>> On Tue, Apr 26, 2022 at 11:47 AM Douglas Anderson 
>> <dianders@chromium.org> wrote:
>>>
>>> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
>>> that all detachable sinks shall support 640x480 @60Hz as a fail safe
>>> mode.
>>>
>>> A DP compliance test expected us to utilize the above fact when all
>>> modes it presented to the DP source were not achievable. It presented
>>> only modes that would be achievable with more lanes and/or higher
>>> speeds than we had available and expected that when we couldn't do
>>> that then we'd fall back to 640x480 even though it didn't advertise
>>> this size.
>>>
>>> In order to pass the compliance test (and also support any users who
>>> might fall into a similar situation with their display), we need to
>>> add 640x480 into the list of modes. However, we don't want to add
>>> 640x480 all the time. Despite the fact that the DP spec says all sinks
>>> _shall support_ 640x480, they're not guaranteed to support it
>>> _well_. Continuing to read the spec you can see that the display is
>>> not required to really treat 640x480 equal to all the other modes. It
>>> doesn't need to scale or anything--just display the pixels somehow for
>>> failsafe purposes. It should also be noted that it's not hard to find
>>> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
>>> all. The HP ZR30w screen I'm sitting in front of has a native DP port
>>> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
>>> display via a DP to HDMI adapter and that screen definitely doesn't
>>> support 640x480.
>>>
>>> As a compromise solution, let's only add the 640x480 mode if:
>>> * We're on DP.
>>> * All other modes have been pruned.
>>>
>>> This acknowledges that 640x480 might not be the best mode to use but,
>>> since sinks are _supposed_ to support it, we will at least fall back
>>> to it if there's nothing else.
>>>
>>> Note that we _don't_ add higher resolution modes like 1024x768 in this
>>> case. We only add those modes for a failed EDID read where we have no
>>> idea what's going on. In the case where we've pruned all modes then
>>> instead we only want 640x480 which is the only defined "Fail Safe"
>>> resolution.
>>>
>>> This patch originated in response to Kuogee Hsieh's patch [1].
>>>
>>> [1] 
>>> https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>>   drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
>>>   1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> I think this patch is fairly safe / non-controversial, but someone
>> suggested you might have an opinion on it and another patch I posted
>> recently [1] so I wanted to double-check. Just to be clear: I'm hoping
>> to land _both_ this patch and [1]. If you don't have an opinion,
>> that's OK too.
>>
>> Abhinav: I think maybe you're happy with this now? Would you be
>> willing to give a Reviewed-by?
>
> Yes, I have no concerns with this approach from DP spec standpoint and 
> in addition, kuogee has tested this out and this does help us to pass 
> the tests.
>
> Although, I might be missing some historical context on why this is 
> not already done.
>
> But apart from that, LGTM. Hence,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>
>> [1] 
>> https://lore.kernel.org/r/20220426132121.RFC.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid
>>
>> -Doug

Re: [PATCH 1/2] drm/probe-helper: Add helper for drm_helper_probe_single_connector_modes()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5348 bytes --]

Hi

Am 26.04.22 um 20:46 schrieb Douglas Anderson:
> The drm_helper_probe_single_connector_modes() is a bit long. Let's
> break a chunk off to update and validate modes. This helps avoid one
> goto and also will allow us to more easily call the helper a second
> time in a future patch without adding looping or another goto.
> 
> This change is intended to be a no-op change--just code movement.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>


> ---
> 
>   drivers/gpu/drm/drm_probe_helper.c | 105 ++++++++++++++++-------------
>   1 file changed, 59 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 682359512996..819225629010 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -354,6 +354,61 @@ drm_helper_probe_detect(struct drm_connector *connector,
>   }
>   EXPORT_SYMBOL(drm_helper_probe_detect);
>   
> +static bool _drm_helper_update_and_validate(struct drm_connector *connector,

AFAIK convention is to use two underscores for internal names.

> +					    uint32_t maxX, uint32_t maxY,
> +					    struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_mode *mode;
> +	int mode_flags = 0;
> +	int ret;
> +
> +	drm_connector_list_update(connector);
> +
> +	if (connector->interlace_allowed)
> +		mode_flags |= DRM_MODE_FLAG_INTERLACE;
> +	if (connector->doublescan_allowed)
> +		mode_flags |= DRM_MODE_FLAG_DBLSCAN;
> +	if (connector->stereo_allowed)
> +		mode_flags |= DRM_MODE_FLAG_3D_MASK;
> +
> +	list_for_each_entry(mode, &connector->modes, head) {
> +		if (mode->status != MODE_OK)
> +			continue;
> +
> +		mode->status = drm_mode_validate_driver(dev, mode);
> +		if (mode->status != MODE_OK)
> +			continue;
> +
> +		mode->status = drm_mode_validate_size(mode, maxX, maxY);
> +		if (mode->status != MODE_OK)
> +			continue;
> +
> +		mode->status = drm_mode_validate_flag(mode, mode_flags);
> +		if (mode->status != MODE_OK)
> +			continue;
> +
> +		ret = drm_mode_validate_pipeline(mode, connector, ctx,
> +						 &mode->status);
> +		if (ret) {
> +			drm_dbg_kms(dev,
> +				    "drm_mode_validate_pipeline failed: %d\n",
> +				    ret);
> +
> +			if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK))
> +				mode->status = MODE_ERROR;
> +			else
> +				return true;

Returning true is non-intuitive. It looks as if we report success when 
it actually signals a retry.

I suggest to return 'ret' here and let the caller decide. On success at 
the end of the function, it would return 0 as usual.

Best regards
Thomas

> +		}
> +
> +		if (mode->status != MODE_OK)
> +			continue;
> +		mode->status = drm_mode_validate_ycbcr420(mode, connector);
> +	}
> +
> +	return false;
> +}
> +
>   /**
>    * drm_helper_probe_single_connector_modes - get complete set of display modes
>    * @connector: connector to probe
> @@ -421,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>   	const struct drm_connector_helper_funcs *connector_funcs =
>   		connector->helper_private;
>   	int count = 0, ret;
> -	int mode_flags = 0;
>   	bool verbose_prune = true;
>   	enum drm_connector_status old_status;
>   	struct drm_modeset_acquire_ctx ctx;
> @@ -519,52 +573,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>   			   connector->status == connector_status_unknown))
>   		count = drm_add_modes_noedid(connector, 1024, 768);
>   	count += drm_helper_probe_add_cmdline_mode(connector);
> -	if (count == 0)
> -		goto prune;
> -
> -	drm_connector_list_update(connector);
> -
> -	if (connector->interlace_allowed)
> -		mode_flags |= DRM_MODE_FLAG_INTERLACE;
> -	if (connector->doublescan_allowed)
> -		mode_flags |= DRM_MODE_FLAG_DBLSCAN;
> -	if (connector->stereo_allowed)
> -		mode_flags |= DRM_MODE_FLAG_3D_MASK;
> -
> -	list_for_each_entry(mode, &connector->modes, head) {
> -		if (mode->status != MODE_OK)
> -			continue;
> -
> -		mode->status = drm_mode_validate_driver(dev, mode);
> -		if (mode->status != MODE_OK)
> -			continue;
> -
> -		mode->status = drm_mode_validate_size(mode, maxX, maxY);
> -		if (mode->status != MODE_OK)
> -			continue;
> -
> -		mode->status = drm_mode_validate_flag(mode, mode_flags);
> -		if (mode->status != MODE_OK)
> -			continue;
> -
> -		ret = drm_mode_validate_pipeline(mode, connector, &ctx,
> -						 &mode->status);
> -		if (ret) {
> -			drm_dbg_kms(dev,
> -				    "drm_mode_validate_pipeline failed: %d\n",
> -				    ret);
> -
> -			if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK)) {
> -				mode->status = MODE_ERROR;
> -			} else {
> -				drm_modeset_backoff(&ctx);
> -				goto retry;
> -			}
> +	if (count != 0) {
> +		if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) {
> +			drm_modeset_backoff(&ctx);
> +			goto retry;
>   		}
> -
> -		if (mode->status != MODE_OK)
> -			continue;
> -		mode->status = drm_mode_validate_ycbcr420(mode, connector);
>   	}
>   
>   prune:

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/2] drm/probe-helper: Add helper for drm_helper_probe_single_connector_modes()

[Doug Anderson]

Hi,

On Thu, May 5, 2022 at 11:34 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 26.04.22 um 20:46 schrieb Douglas Anderson:
> > The drm_helper_probe_single_connector_modes() is a bit long. Let's
> > break a chunk off to update and validate modes. This helps avoid one
> > goto and also will allow us to more easily call the helper a second
> > time in a future patch without adding looping or another goto.
> >
> > This change is intended to be a no-op change--just code movement.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
>
> > ---
> >
> >   drivers/gpu/drm/drm_probe_helper.c | 105 ++++++++++++++++-------------
> >   1 file changed, 59 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 682359512996..819225629010 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -354,6 +354,61 @@ drm_helper_probe_detect(struct drm_connector *connector,
> >   }
> >   EXPORT_SYMBOL(drm_helper_probe_detect);
> >
> > +static bool _drm_helper_update_and_validate(struct drm_connector *connector,
>
> AFAIK convention is to use two underscores for internal names.

Sure! I'll spin with this.


> > +                                         uint32_t maxX, uint32_t maxY,
> > +                                         struct drm_modeset_acquire_ctx *ctx)
> > +{
> > +     struct drm_device *dev = connector->dev;
> > +     struct drm_display_mode *mode;
> > +     int mode_flags = 0;
> > +     int ret;
> > +
> > +     drm_connector_list_update(connector);
> > +
> > +     if (connector->interlace_allowed)
> > +             mode_flags |= DRM_MODE_FLAG_INTERLACE;
> > +     if (connector->doublescan_allowed)
> > +             mode_flags |= DRM_MODE_FLAG_DBLSCAN;
> > +     if (connector->stereo_allowed)
> > +             mode_flags |= DRM_MODE_FLAG_3D_MASK;
> > +
> > +     list_for_each_entry(mode, &connector->modes, head) {
> > +             if (mode->status != MODE_OK)
> > +                     continue;
> > +
> > +             mode->status = drm_mode_validate_driver(dev, mode);
> > +             if (mode->status != MODE_OK)
> > +                     continue;
> > +
> > +             mode->status = drm_mode_validate_size(mode, maxX, maxY);
> > +             if (mode->status != MODE_OK)
> > +                     continue;
> > +
> > +             mode->status = drm_mode_validate_flag(mode, mode_flags);
> > +             if (mode->status != MODE_OK)
> > +                     continue;
> > +
> > +             ret = drm_mode_validate_pipeline(mode, connector, ctx,
> > +                                              &mode->status);
> > +             if (ret) {
> > +                     drm_dbg_kms(dev,
> > +                                 "drm_mode_validate_pipeline failed: %d\n",
> > +                                 ret);
> > +
> > +                     if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK))
> > +                             mode->status = MODE_ERROR;
> > +                     else
> > +                             return true;
>
> Returning true is non-intuitive. It looks as if we report success when
> it actually signals a retry.
>
> I suggest to return 'ret' here and let the caller decide. On success at
> the end of the function, it would return 0 as usual.

There's a madness to my method. I originally had it returning "ret"
but then I felt like the callers now needed to handle three cases:

a) ret = -EDEADLK
b) ret = 0
c) ret = some other error

In reality _drm_helper_update_and_validate() never returned anything
other than a) or b), so adding the extra error handling for c) seemed
like a waste. ...but it also felt like if it violated the abstraction
of _drm_helper_update_and_validate() returning an error code if I
didn't handle c).

In any case, I'll change it back to an error code. Maybe a compromise would be:

ret = _drm_helper_update_and_validate(...)
if (ret == -EDEADLK) {
  drm_modeset_backoff(...)
  goto retry;
}
WARN_ON(ret);

...so we at least document that ret can only be one of the two values
and we'll get a warning splat if it ever happens, but we don't need to
add complex error handling for a case that the code can't hit. Ah,
looking above I guess that matches what the function does earlier too.

OK, I'll give a few more days for feedback on this series and I'll
spin with the two changes you've requested.

-Doug

-Doug

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Dmitry Baryshkov]

On Thu, 5 May 2022 at 20:30, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 5/5/2022 10:20 AM, Abhinav Kumar wrote:
> > Hi Doug
> >
> > On 5/5/2022 8:44 AM, Doug Anderson wrote:
> >> Ville,
> >>
> >> On Tue, Apr 26, 2022 at 11:47 AM Douglas Anderson
> >> <dianders@chromium.org> wrote:
> >>>
> >>> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
> >>> that all detachable sinks shall support 640x480 @60Hz as a fail safe
> >>> mode.
> >>>
> >>> A DP compliance test expected us to utilize the above fact when all
> >>> modes it presented to the DP source were not achievable. It presented
> >>> only modes that would be achievable with more lanes and/or higher
> >>> speeds than we had available and expected that when we couldn't do
> >>> that then we'd fall back to 640x480 even though it didn't advertise
> >>> this size.
> >>>
> >>> In order to pass the compliance test (and also support any users who
> >>> might fall into a similar situation with their display), we need to
> >>> add 640x480 into the list of modes. However, we don't want to add
> >>> 640x480 all the time. Despite the fact that the DP spec says all sinks
> >>> _shall support_ 640x480, they're not guaranteed to support it
> >>> _well_. Continuing to read the spec you can see that the display is
> >>> not required to really treat 640x480 equal to all the other modes. It
> >>> doesn't need to scale or anything--just display the pixels somehow for
> >>> failsafe purposes. It should also be noted that it's not hard to find
> >>> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
> >>> all. The HP ZR30w screen I'm sitting in front of has a native DP port
> >>> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
> >>> display via a DP to HDMI adapter and that screen definitely doesn't
> >>> support 640x480.
> >>>
> >>> As a compromise solution, let's only add the 640x480 mode if:
> >>> * We're on DP.
> >>> * All other modes have been pruned.
> >>>
> >>> This acknowledges that 640x480 might not be the best mode to use but,
> >>> since sinks are _supposed_ to support it, we will at least fall back
> >>> to it if there's nothing else.
> >>>
> >>> Note that we _don't_ add higher resolution modes like 1024x768 in this
> >>> case. We only add those modes for a failed EDID read where we have no
> >>> idea what's going on. In the case where we've pruned all modes then
> >>> instead we only want 640x480 which is the only defined "Fail Safe"
> >>> resolution.
> >>>
> >>> This patch originated in response to Kuogee Hsieh's patch [1].
> >>>
> >>> [1]
> >>> https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
> >>>
> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>> ---
> >>>
> >>>   drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
> >>>   1 file changed, 21 insertions(+), 5 deletions(-)
> >>
> >> I think this patch is fairly safe / non-controversial, but someone
> >> suggested you might have an opinion on it and another patch I posted
> >> recently [1] so I wanted to double-check. Just to be clear: I'm hoping
> >> to land _both_ this patch and [1]. If you don't have an opinion,
> >> that's OK too.
> >>
> >> Abhinav: I think maybe you're happy with this now? Would you be
> >> willing to give a Reviewed-by?
> >
> > Yes, I have no concerns with this approach from DP spec standpoint and
> > in addition, kuogee has tested this out and this does help us to pass
> > the tests.
> >
> > Although, I might be missing some historical context on why this is
> > not already done.
> >
> > But apart from that, LGTM. Hence,
> >
> > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>

This line got wrong quotation level, so it will not be noticed by
patchwork (and can be easily missed by other people too). Please
resend.

> >>
> >> [1]
> >> https://lore.kernel.org/r/20220426132121.RFC.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid
> >>
> >> -Doug



-- 
With best wishes
Dmitry

Re: [PATCH 2/2] drm/probe-helper: For DP, add 640x480 if all other modes are bad

[Dmitry Baryshkov]

On Tue, 26 Apr 2022 at 21:47, Douglas Anderson <dianders@chromium.org> wrote:
>
> As per Displayport spec section 5.2.1.2 ("Video Timing Format") says
> that all detachable sinks shall support 640x480 @60Hz as a fail safe
> mode.
>
> A DP compliance test expected us to utilize the above fact when all
> modes it presented to the DP source were not achievable. It presented
> only modes that would be achievable with more lanes and/or higher
> speeds than we had available and expected that when we couldn't do
> that then we'd fall back to 640x480 even though it didn't advertise
> this size.
>
> In order to pass the compliance test (and also support any users who
> might fall into a similar situation with their display), we need to
> add 640x480 into the list of modes. However, we don't want to add
> 640x480 all the time. Despite the fact that the DP spec says all sinks
> _shall support_ 640x480, they're not guaranteed to support it
> _well_. Continuing to read the spec you can see that the display is
> not required to really treat 640x480 equal to all the other modes. It
> doesn't need to scale or anything--just display the pixels somehow for
> failsafe purposes. It should also be noted that it's not hard to find
> a display hooked up via DisplayPort that _doesn't_ support 640x480 at
> all. The HP ZR30w screen I'm sitting in front of has a native DP port
> and doesn't work at 640x480. I also plugged in a tiny 800x480 HDMI
> display via a DP to HDMI adapter and that screen definitely doesn't
> support 640x480.
>
> As a compromise solution, let's only add the 640x480 mode if:
> * We're on DP.
> * All other modes have been pruned.
>
> This acknowledges that 640x480 might not be the best mode to use but,
> since sinks are _supposed_ to support it, we will at least fall back
> to it if there's nothing else.
>
> Note that we _don't_ add higher resolution modes like 1024x768 in this
> case. We only add those modes for a failed EDID read where we have no
> idea what's going on. In the case where we've pruned all modes then
> instead we only want 640x480 which is the only defined "Fail Safe"
> resolution.
>
> This patch originated in response to Kuogee Hsieh's patch [1].
>
> [1] https://lore.kernel.org/r/1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>
>  drivers/gpu/drm/drm_probe_helper.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 819225629010..90cd46cbfec1 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -476,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>         const struct drm_connector_helper_funcs *connector_funcs =
>                 connector->helper_private;
>         int count = 0, ret;
> -       bool verbose_prune = true;
>         enum drm_connector_status old_status;
>         struct drm_modeset_acquire_ctx ctx;
>
> @@ -556,8 +555,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>                 DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
>                         connector->base.id, connector->name);
>                 drm_connector_update_edid_property(connector, NULL);
> -               verbose_prune = false;
> -               goto prune;
> +               drm_mode_prune_invalid(dev, &connector->modes, false);
> +               goto exit;
>         }
>
>         count = (*connector_funcs->get_modes)(connector);
> @@ -580,9 +579,26 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>                 }
>         }
>
> -prune:
> -       drm_mode_prune_invalid(dev, &connector->modes, verbose_prune);
> +       drm_mode_prune_invalid(dev, &connector->modes, true);
>
> +       /*
> +        * Displayport spec section 5.2.1.2 ("Video Timing Format") says that
> +        * all detachable sinks shall support 640x480 @60Hz as a fail safe
> +        * mode. If all modes were pruned, perhaps because they need more
> +        * lanes or a higher pixel clock than available, at least try to add
> +        * in 640x480.
> +        */
> +       if (list_empty(&connector->modes) &&
> +           connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
> +               count = drm_add_modes_noedid(connector, 640, 480);
> +               if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) {
> +                       drm_modeset_backoff(&ctx);
> +                       goto retry;
> +               }
> +               drm_mode_prune_invalid(dev, &connector->modes, true);
> +       }
> +
> +exit:
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>


-- 
With best wishes
Dmitry