[RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Douglas Anderson]

If we're unable to read the EDID for a display because it's corrupt /
bogus / invalid then we'll add a set of standard modes for the
display. When userspace looks at these modes it doesn't really have a
good concept for which mode to pick and it'll likely pick the highest
resolution one by default. That's probably not ideal because the modes
were purely guesses on the part of the Linux kernel.

Let's instead set 640x480 as the "preferred" mode when we have no EDID.

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

 drivers/gpu/drm/drm_edid.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7a8482b75071..64ccfff4167e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5839,6 +5839,15 @@ int drm_add_modes_noedid(struct drm_connector *connector,
 			continue;
 		mode = drm_mode_duplicate(dev, ptr);
 		if (mode) {
+			/*
+			 * The drm_dmt_modes array is sorted so that lower
+			 * resolutions come first. We'll set the lowest
+			 * resolution mode as preferred. We have no EDID so
+			 * we should prefer the lowest resolution mode as
+			 * the safest one.
+			 */
+			if (num_modes == 0)
+				mode->type |= DRM_MODE_TYPE_PREFERRED;
 			drm_mode_probed_add(connector, mode);
 			num_modes++;
 		}
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Abhinav Kumar]

On 4/26/2022 1:21 PM, Douglas Anderson wrote:
> If we're unable to read the EDID for a display because it's corrupt /
> bogus / invalid then we'll add a set of standard modes for the
> display. When userspace looks at these modes it doesn't really have a
> good concept for which mode to pick and it'll likely pick the highest
> resolution one by default. That's probably not ideal because the modes
> were purely guesses on the part of the Linux kernel.
> 
> Let's instead set 640x480 as the "preferred" mode when we have no EDID.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

drm_dmt_modes array is sorted but you are also relying on this check to 
eliminate the non-60fps modes

5611 		if (drm_mode_vrefresh(ptr) > 61)
5612 			continue;

I am not sure why we filter out the modes > 61 vrefresh.

If that check will remain this is okay.

If its not, its not reliable that the first mode will be 640x480@60

> ---
> 
>   drivers/gpu/drm/drm_edid.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7a8482b75071..64ccfff4167e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5839,6 +5839,15 @@ int drm_add_modes_noedid(struct drm_connector *connector,
>   			continue;
>   		mode = drm_mode_duplicate(dev, ptr);
>   		if (mode) {
> +			/*
> +			 * The drm_dmt_modes array is sorted so that lower
> +			 * resolutions come first. We'll set the lowest
> +			 * resolution mode as preferred. We have no EDID so
> +			 * we should prefer the lowest resolution mode as
> +			 * the safest one.
> +			 */
> +			if (num_modes == 0)
> +				mode->type |= DRM_MODE_TYPE_PREFERRED;
>   			drm_mode_probed_add(connector, mode);
>   			num_modes++;
>   		}

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Doug Anderson]

Hi,

On Tue, Apr 26, 2022 at 1:46 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> On 4/26/2022 1:21 PM, Douglas Anderson wrote:
> > If we're unable to read the EDID for a display because it's corrupt /
> > bogus / invalid then we'll add a set of standard modes for the
> > display. When userspace looks at these modes it doesn't really have a
> > good concept for which mode to pick and it'll likely pick the highest
> > resolution one by default. That's probably not ideal because the modes
> > were purely guesses on the part of the Linux kernel.
> >
> > Let's instead set 640x480 as the "preferred" mode when we have no EDID.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> drm_dmt_modes array is sorted but you are also relying on this check to
> eliminate the non-60fps modes
>
> 5611            if (drm_mode_vrefresh(ptr) > 61)
> 5612                    continue;
>
> I am not sure why we filter out the modes > 61 vrefresh.
>
> If that check will remain this is okay.
>
> If its not, its not reliable that the first mode will be 640x480@60

I suspect that the check will remain. I guess I could try to do
something fancier if people want, but I'd be interested in _what_
fancier thing I should do if so. Do we want the rule to remain that we
always prefer 640x480, or do we want to prefer the lowest resolution?
...do we want to prefer 60 Hz or the lowest refresh rate? Do we do
this only for DP (which explicitly calls out 640x480 @60Hz as the best
failsafe) or for everything?

For now, the way it's coded up seems reasonable (to me). It's the
lowest resolution _and_ it's 640x480 just because of the current
values of the table. I suspect that extra lower resolution failsafe
modes won't be added, but we can always change the rules here if/when
they are.

-Doug

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Abhinav Kumar]

On 4/26/2022 1:52 PM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Apr 26, 2022 at 1:46 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> On 4/26/2022 1:21 PM, Douglas Anderson wrote:
>>> If we're unable to read the EDID for a display because it's corrupt /
>>> bogus / invalid then we'll add a set of standard modes for the
>>> display. When userspace looks at these modes it doesn't really have a
>>> good concept for which mode to pick and it'll likely pick the highest
>>> resolution one by default. That's probably not ideal because the modes
>>> were purely guesses on the part of the Linux kernel.
>>>
>>> Let's instead set 640x480 as the "preferred" mode when we have no EDID.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>
>> drm_dmt_modes array is sorted but you are also relying on this check to
>> eliminate the non-60fps modes
>>
>> 5611            if (drm_mode_vrefresh(ptr) > 61)
>> 5612                    continue;
>>
>> I am not sure why we filter out the modes > 61 vrefresh.
>>
>> If that check will remain this is okay.
>>
>> If its not, its not reliable that the first mode will be 640x480@60
> 
> I suspect that the check will remain. I guess I could try to do
> something fancier if people want, but I'd be interested in _what_
> fancier thing I should do if so. Do we want the rule to remain that we
> always prefer 640x480, or do we want to prefer the lowest resolution?
> ...do we want to prefer 60 Hz or the lowest refresh rate? Do we do
> this only for DP (which explicitly calls out 640x480 @60Hz as the best
> failsafe) or for everything?
> 
> For now, the way it's coded up seems reasonable (to me). It's the
> lowest resolution _and_ it's 640x480 just because of the current
> values of the table. I suspect that extra lower resolution failsafe
> modes won't be added, but we can always change the rules here if/when
> they are.
> 
> -Doug

Alright, agreed. The way the API is today, I dont see anything getting 
broken with this.

So typically, as per spec, when a preferred mode is not set by the sink, 
the first entry becomes the preferred mode.

This also aligns with that expectation.

So FWIW,

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

We will test this one also out with our equipment, then give tested-by tags.

Thanks

Abhinav

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Kuogee Hsieh]

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


On 4/26/2022 2:21 PM, Abhinav Kumar wrote:
>
>
> On 4/26/2022 1:52 PM, Doug Anderson wrote:
>> Hi,
>>
>> On Tue, Apr 26, 2022 at 1:46 PM Abhinav Kumar 
>> <quic_abhinavk@quicinc.com> wrote:
>>>
>>> On 4/26/2022 1:21 PM, Douglas Anderson wrote:
>>>> If we're unable to read the EDID for a display because it's corrupt /
>>>> bogus / invalid then we'll add a set of standard modes for the
>>>> display. When userspace looks at these modes it doesn't really have a
>>>> good concept for which mode to pick and it'll likely pick the highest
>>>> resolution one by default. That's probably not ideal because the modes
>>>> were purely guesses on the part of the Linux kernel.
>>>>
>>>> Let's instead set 640x480 as the "preferred" mode when we have no 
>>>> EDID.
>>>>
>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>
>>> drm_dmt_modes array is sorted but you are also relying on this check to
>>> eliminate the non-60fps modes
>>>
>>> 5611            if (drm_mode_vrefresh(ptr) > 61)
>>> 5612                    continue;
>>>
>>> I am not sure why we filter out the modes > 61 vrefresh.
>>>
>>> If that check will remain this is okay.
>>>
>>> If its not, its not reliable that the first mode will be 640x480@60
>>
>> I suspect that the check will remain. I guess I could try to do
>> something fancier if people want, but I'd be interested in _what_
>> fancier thing I should do if so. Do we want the rule to remain that we
>> always prefer 640x480, or do we want to prefer the lowest resolution?
>> ...do we want to prefer 60 Hz or the lowest refresh rate? Do we do
>> this only for DP (which explicitly calls out 640x480 @60Hz as the best
>> failsafe) or for everything?
>>
>> For now, the way it's coded up seems reasonable (to me). It's the
>> lowest resolution _and_ it's 640x480 just because of the current
>> values of the table. I suspect that extra lower resolution failsafe
>> modes won't be added, but we can always change the rules here if/when
>> they are.
>>
>> -Doug
>
> Alright, agreed. The way the API is today, I dont see anything getting 
> broken with this.
>
> So typically, as per spec, when a preferred mode is not set by the 
> sink, the first entry becomes the preferred mode.
>
> This also aligns with that expectation.
>
> So FWIW,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> We will test this one also out with our equipment, then give tested-by 
> tags.
>
> Thanks
>
> Abhinav
>

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Doug Anderson]

Ville,

On Tue, Apr 26, 2022 at 1:21 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> If we're unable to read the EDID for a display because it's corrupt /
> bogus / invalid then we'll add a set of standard modes for the
> display. When userspace looks at these modes it doesn't really have a
> good concept for which mode to pick and it'll likely pick the highest
> resolution one by default. That's probably not ideal because the modes
> were purely guesses on the part of the Linux kernel.
>
> Let's instead set 640x480 as the "preferred" mode when we have no EDID.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/gpu/drm/drm_edid.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Someone suggested that you might have an opinion on this patch and
another one I posted recently [1]. Do you have any thoughts on it?
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.

[1] https://lore.kernel.org/r/20220426114627.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid

-Doug

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Jani Nikula]

On Thu, 05 May 2022, Doug Anderson <dianders@chromium.org> wrote:
> Ville,
>
> On Tue, Apr 26, 2022 at 1:21 PM Douglas Anderson <dianders@chromium.org> wrote:
>>
>> If we're unable to read the EDID for a display because it's corrupt /
>> bogus / invalid then we'll add a set of standard modes for the
>> display. When userspace looks at these modes it doesn't really have a
>> good concept for which mode to pick and it'll likely pick the highest
>> resolution one by default. That's probably not ideal because the modes
>> were purely guesses on the part of the Linux kernel.
>>
>> Let's instead set 640x480 as the "preferred" mode when we have no EDID.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>
>>  drivers/gpu/drm/drm_edid.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>
> Someone suggested that you might have an opinion on this patch and
> another one I posted recently [1]. Do you have any thoughts on it?
> 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.
>
> [1] https://lore.kernel.org/r/20220426114627.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid

There are a number of drivers with combos:

	drm_add_modes_noedid()
	drm_set_preferred_mode()

which I think would be affected by the change. Perhaps you should just
call drm_set_preferred_mode() in your referenced patch?

Alternatively, perhaps drm_set_preferred_mode() should erase the
previous preferred mode(s) if it finds a matching new preferred mode.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Abhinav Kumar]

Hi Jani

On 5/6/2022 4:16 AM, Jani Nikula wrote:
> On Thu, 05 May 2022, Doug Anderson <dianders@chromium.org> wrote:
>> Ville,
>>
>> On Tue, Apr 26, 2022 at 1:21 PM Douglas Anderson <dianders@chromium.org> wrote:
>>>
>>> If we're unable to read the EDID for a display because it's corrupt /
>>> bogus / invalid then we'll add a set of standard modes for the
>>> display. When userspace looks at these modes it doesn't really have a
>>> good concept for which mode to pick and it'll likely pick the highest
>>> resolution one by default. That's probably not ideal because the modes
>>> were purely guesses on the part of the Linux kernel.
>>>
>>> Let's instead set 640x480 as the "preferred" mode when we have no EDID.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>>   drivers/gpu/drm/drm_edid.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>
>> Someone suggested that you might have an opinion on this patch and
>> another one I posted recently [1]. Do you have any thoughts on it?
>> 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.
>>
>> [1] https://lore.kernel.org/r/20220426114627.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid
> 
> There are a number of drivers with combos:
> 
> 	drm_add_modes_noedid()
> 	drm_set_preferred_mode()
> 
> which I think would be affected by the change. Perhaps you should just
> call drm_set_preferred_mode() in your referenced patch?
> So it seems like many drivers handle the !edid case within their 
respective get_modes() call which probably is because they know the max 
capability of their connector and because they know which mode should be 
set as preferred. But at the same time, perhaps the code below which 
handles the count == 0 case should be changed like below to make sure we 
are within the max_width/height of the connector (to handle the first 
condition)?

diff --git a/drivers/gpu/drm/drm_probe_helper.c 
b/drivers/gpu/drm/drm_probe_helper.c
index 682359512996..6eb89d90777b 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -517,7 +517,8 @@ int drm_helper_probe_single_connector_modes(struct 
drm_connector *connector,

         if (count == 0 && (connector->status == 
connector_status_connected ||
                            connector->status == connector_status_unknown))
-               count = drm_add_modes_noedid(connector, 1024, 768);
+               count = drm_add_modes_noedid(connector, 
connector->dev->mode_config.max_width,
+                               connector->dev->mode_config.max_height);
         count += drm_helper_probe_add_cmdline_mode(connector);
         if (count == 0)
                 goto prune;


> Alternatively, perhaps drm_set_preferred_mode() should erase the
> previous preferred mode(s) if it finds a matching new preferred mode.
> 

But still yes, even if we change it like above perhaps for other non-DP 
cases its still better to allow individual drivers to pick their 
preferred modes.

If we call drm_set_preferred_mode() in the referenced patch, it will not 
address the no EDID cases because the patch comes into picture when 
there was a EDID with some modes but not with 640x480.

So i think the second proposal is a good one. It will cover existing 
users of drm_set_preferred_mode() as typically its called after 
drm_add_modes_noedid() which means the existing users want to "override" 
their preferred mode.

> 
> BR,
> Jani.
> 

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Doug Anderson]

Hi,

On Fri, May 6, 2022 at 9:33 AM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Jani
>
> On 5/6/2022 4:16 AM, Jani Nikula wrote:
> > On Thu, 05 May 2022, Doug Anderson <dianders@chromium.org> wrote:
> >> Ville,
> >>
> >> On Tue, Apr 26, 2022 at 1:21 PM Douglas Anderson <dianders@chromium.org> wrote:
> >>>
> >>> If we're unable to read the EDID for a display because it's corrupt /
> >>> bogus / invalid then we'll add a set of standard modes for the
> >>> display. When userspace looks at these modes it doesn't really have a
> >>> good concept for which mode to pick and it'll likely pick the highest
> >>> resolution one by default. That's probably not ideal because the modes
> >>> were purely guesses on the part of the Linux kernel.
> >>>
> >>> Let's instead set 640x480 as the "preferred" mode when we have no EDID.
> >>>
> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>> ---
> >>>
> >>>   drivers/gpu/drm/drm_edid.c | 9 +++++++++
> >>>   1 file changed, 9 insertions(+)
> >>
> >> Someone suggested that you might have an opinion on this patch and
> >> another one I posted recently [1]. Do you have any thoughts on it?
> >> 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.
> >>
> >> [1] https://lore.kernel.org/r/20220426114627.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid
> >
> > There are a number of drivers with combos:
> >
> >       drm_add_modes_noedid()
> >       drm_set_preferred_mode()
> >
> > which I think would be affected by the change. Perhaps you should just
> > call drm_set_preferred_mode() in your referenced patch?

I'm going to do that and I think it works out pretty well. Patch is at:

https://lore.kernel.org/r/20220510135101.v2.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid


> So it seems like many drivers handle the !edid case within their
> respective get_modes() call which probably is because they know the max
> capability of their connector and because they know which mode should be
> set as preferred. But at the same time, perhaps the code below which
> handles the count == 0 case should be changed like below to make sure we
> are within the max_width/height of the connector (to handle the first
> condition)?
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index 682359512996..6eb89d90777b 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -517,7 +517,8 @@ int drm_helper_probe_single_connector_modes(struct
> drm_connector *connector,
>
>          if (count == 0 && (connector->status ==
> connector_status_connected ||
>                             connector->status == connector_status_unknown))
> -               count = drm_add_modes_noedid(connector, 1024, 768);
> +               count = drm_add_modes_noedid(connector,
> connector->dev->mode_config.max_width,
> +                               connector->dev->mode_config.max_height);
>          count += drm_helper_probe_add_cmdline_mode(connector);
>          if (count == 0)
>                  goto prune;
>
>
> > Alternatively, perhaps drm_set_preferred_mode() should erase the
> > previous preferred mode(s) if it finds a matching new preferred mode.
> >
>
> But still yes, even if we change it like above perhaps for other non-DP
> cases its still better to allow individual drivers to pick their
> preferred modes.
>
> If we call drm_set_preferred_mode() in the referenced patch, it will not
> address the no EDID cases because the patch comes into picture when
> there was a EDID with some modes but not with 640x480.

I'm not sure I understand the above paragraph. I think the "there's an
EDID but no 640x480" is handled by my other patch [1]. Here we're only
worried about the "no EDID" case, right?


> So i think the second proposal is a good one. It will cover existing
> users of drm_set_preferred_mode() as typically its called after
> drm_add_modes_noedid() which means the existing users want to "override"
> their preferred mode.

I looked at this, and I'm pretty sure that we can't clear the
preferred modes. It looks like it's possible for there to be more than
one preferred mode and I'm worried about borking that up.

[1] https://lore.kernel.org/r/20220510131309.v2.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid

-Doug

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Abhinav Kumar]

Hi Doug

On 5/10/2022 1:53 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, May 6, 2022 at 9:33 AM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Jani
>>
>> On 5/6/2022 4:16 AM, Jani Nikula wrote:
>>> On Thu, 05 May 2022, Doug Anderson <dianders@chromium.org> wrote:
>>>> Ville,
>>>>
>>>> On Tue, Apr 26, 2022 at 1:21 PM Douglas Anderson <dianders@chromium.org> wrote:
>>>>>
>>>>> If we're unable to read the EDID for a display because it's corrupt /
>>>>> bogus / invalid then we'll add a set of standard modes for the
>>>>> display. When userspace looks at these modes it doesn't really have a
>>>>> good concept for which mode to pick and it'll likely pick the highest
>>>>> resolution one by default. That's probably not ideal because the modes
>>>>> were purely guesses on the part of the Linux kernel.
>>>>>
>>>>> Let's instead set 640x480 as the "preferred" mode when we have no EDID.
>>>>>
>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>> ---
>>>>>
>>>>>    drivers/gpu/drm/drm_edid.c | 9 +++++++++
>>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> Someone suggested that you might have an opinion on this patch and
>>>> another one I posted recently [1]. Do you have any thoughts on it?
>>>> 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.
>>>>
>>>> [1] https://lore.kernel.org/r/20220426114627.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid
>>>
>>> There are a number of drivers with combos:
>>>
>>>        drm_add_modes_noedid()
>>>        drm_set_preferred_mode()
>>>
>>> which I think would be affected by the change. Perhaps you should just
>>> call drm_set_preferred_mode() in your referenced patch?
> 
> I'm going to do that and I think it works out pretty well. Patch is at:
> 
> https://lore.kernel.org/r/20220510135101.v2.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid
> 
> 
>> So it seems like many drivers handle the !edid case within their
>> respective get_modes() call which probably is because they know the max
>> capability of their connector and because they know which mode should be
>> set as preferred. But at the same time, perhaps the code below which
>> handles the count == 0 case should be changed like below to make sure we
>> are within the max_width/height of the connector (to handle the first
>> condition)?
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
>> b/drivers/gpu/drm/drm_probe_helper.c
>> index 682359512996..6eb89d90777b 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -517,7 +517,8 @@ int drm_helper_probe_single_connector_modes(struct
>> drm_connector *connector,
>>
>>           if (count == 0 && (connector->status ==
>> connector_status_connected ||
>>                              connector->status == connector_status_unknown))
>> -               count = drm_add_modes_noedid(connector, 1024, 768);
>> +               count = drm_add_modes_noedid(connector,
>> connector->dev->mode_config.max_width,
>> +                               connector->dev->mode_config.max_height);
>>           count += drm_helper_probe_add_cmdline_mode(connector);
>>           if (count == 0)
>>                   goto prune;
>>
>>
>>> Alternatively, perhaps drm_set_preferred_mode() should erase the
>>> previous preferred mode(s) if it finds a matching new preferred mode.
>>>
>>
>> But still yes, even if we change it like above perhaps for other non-DP
>> cases its still better to allow individual drivers to pick their
>> preferred modes.
>>
>> If we call drm_set_preferred_mode() in the referenced patch, it will not
>> address the no EDID cases because the patch comes into picture when
>> there was a EDID with some modes but not with 640x480.
> 
> I'm not sure I understand the above paragraph. I think the "there's an
> EDID but no 640x480" is handled by my other patch [1]. Here we're only
> worried about the "no EDID" case, right?
> 
Yes, there are two fixes which have been done (OR have to be done).

1) Case when EDID read failed and count of modes was 0.

Here the DRM framework was already adding 640x480@60fps. The fix we had 
to make was making 640x480@60fps as the preferred mode. Which is what 
your current patch aims at addressing.

https://lore.kernel.org/all/20220510135101.v2.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid/

So I thought the suggestion which Jani was giving was to call 
drm_set_preferred_mode() on the referenced patch which was:

https://lore.kernel.org/all/20220510131309.v2.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid/

So that would not have fixed this case.

Perhaps, I misunderstood the patch which was being referenced?

2) Case where there were other modes, which got filtered out and in the 
end no modes were left and we had to end up adding 640x480.

No need to set the preferred mode in this case as this would have been 
the only mode in the list ( so becomes preferred by default ).

Thats this change

https://lore.kernel.org/all/20220426114627.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid/

I agree with combination of these 2 it should work.



> 
>> So i think the second proposal is a good one. It will cover existing
>> users of drm_set_preferred_mode() as typically its called after
>> drm_add_modes_noedid() which means the existing users want to "override"
>> their preferred mode.
> 
> I looked at this, and I'm pretty sure that we can't clear the
> preferred modes. It looks like it's possible for there to be more than
> one preferred mode and I'm worried about borking that up.
> 
> [1] https://lore.kernel.org/r/20220510131309.v2.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid
> 
> -Doug

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Doug Anderson]

Hi,

On Tue, May 10, 2022 at 2:33 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug
>
> On 5/10/2022 1:53 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, May 6, 2022 at 9:33 AM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> Hi Jani
> >>
> >> On 5/6/2022 4:16 AM, Jani Nikula wrote:
> >>> On Thu, 05 May 2022, Doug Anderson <dianders@chromium.org> wrote:
> >>>> Ville,
> >>>>
> >>>> On Tue, Apr 26, 2022 at 1:21 PM Douglas Anderson <dianders@chromium.org> wrote:
> >>>>>
> >>>>> If we're unable to read the EDID for a display because it's corrupt /
> >>>>> bogus / invalid then we'll add a set of standard modes for the
> >>>>> display. When userspace looks at these modes it doesn't really have a
> >>>>> good concept for which mode to pick and it'll likely pick the highest
> >>>>> resolution one by default. That's probably not ideal because the modes
> >>>>> were purely guesses on the part of the Linux kernel.
> >>>>>
> >>>>> Let's instead set 640x480 as the "preferred" mode when we have no EDID.
> >>>>>
> >>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>>>> ---
> >>>>>
> >>>>>    drivers/gpu/drm/drm_edid.c | 9 +++++++++
> >>>>>    1 file changed, 9 insertions(+)
> >>>>
> >>>> Someone suggested that you might have an opinion on this patch and
> >>>> another one I posted recently [1]. Do you have any thoughts on it?
> >>>> 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.
> >>>>
> >>>> [1] https://lore.kernel.org/r/20220426114627.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid
> >>>
> >>> There are a number of drivers with combos:
> >>>
> >>>        drm_add_modes_noedid()
> >>>        drm_set_preferred_mode()
> >>>
> >>> which I think would be affected by the change. Perhaps you should just
> >>> call drm_set_preferred_mode() in your referenced patch?
> >
> > I'm going to do that and I think it works out pretty well. Patch is at:
> >
> > https://lore.kernel.org/r/20220510135101.v2.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid
> >
> >
> >> So it seems like many drivers handle the !edid case within their
> >> respective get_modes() call which probably is because they know the max
> >> capability of their connector and because they know which mode should be
> >> set as preferred. But at the same time, perhaps the code below which
> >> handles the count == 0 case should be changed like below to make sure we
> >> are within the max_width/height of the connector (to handle the first
> >> condition)?
> >>
> >> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> >> b/drivers/gpu/drm/drm_probe_helper.c
> >> index 682359512996..6eb89d90777b 100644
> >> --- a/drivers/gpu/drm/drm_probe_helper.c
> >> +++ b/drivers/gpu/drm/drm_probe_helper.c
> >> @@ -517,7 +517,8 @@ int drm_helper_probe_single_connector_modes(struct
> >> drm_connector *connector,
> >>
> >>           if (count == 0 && (connector->status ==
> >> connector_status_connected ||
> >>                              connector->status == connector_status_unknown))
> >> -               count = drm_add_modes_noedid(connector, 1024, 768);
> >> +               count = drm_add_modes_noedid(connector,
> >> connector->dev->mode_config.max_width,
> >> +                               connector->dev->mode_config.max_height);
> >>           count += drm_helper_probe_add_cmdline_mode(connector);
> >>           if (count == 0)
> >>                   goto prune;
> >>
> >>
> >>> Alternatively, perhaps drm_set_preferred_mode() should erase the
> >>> previous preferred mode(s) if it finds a matching new preferred mode.
> >>>
> >>
> >> But still yes, even if we change it like above perhaps for other non-DP
> >> cases its still better to allow individual drivers to pick their
> >> preferred modes.
> >>
> >> If we call drm_set_preferred_mode() in the referenced patch, it will not
> >> address the no EDID cases because the patch comes into picture when
> >> there was a EDID with some modes but not with 640x480.
> >
> > I'm not sure I understand the above paragraph. I think the "there's an
> > EDID but no 640x480" is handled by my other patch [1]. Here we're only
> > worried about the "no EDID" case, right?
> >
> Yes, there are two fixes which have been done (OR have to be done).
>
> 1) Case when EDID read failed and count of modes was 0.
>
> Here the DRM framework was already adding 640x480@60fps. The fix we had
> to make was making 640x480@60fps as the preferred mode. Which is what
> your current patch aims at addressing.
>
> https://lore.kernel.org/all/20220510135101.v2.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid/
>
> So I thought the suggestion which Jani was giving was to call
> drm_set_preferred_mode() on the referenced patch which was:
>
> https://lore.kernel.org/all/20220510131309.v2.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid/
>
> So that would not have fixed this case.
>
> Perhaps, I misunderstood the patch which was being referenced?

Ah! I couldn't quite understand what the "referenced patch" meant. I
assumed that Jani was meaning that we add a call to
drm_set_preferred_mode() to whatever was calling
drm_add_modes_noedid().


> 2) Case where there were other modes, which got filtered out and in the
> end no modes were left and we had to end up adding 640x480.
>
> No need to set the preferred mode in this case as this would have been
> the only mode in the list ( so becomes preferred by default ).
>
> Thats this change
>
> https://lore.kernel.org/all/20220426114627.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid/
>
> I agree with combination of these 2 it should work.

OK, cool. So just to be clear: you're good with both "v2" patches that
I sent out today and they should fix both use cases, right? ;-)

-Doug

Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred

[Abhinav Kumar]

Hi Doug

On 5/10/2022 2:41 PM, Doug Anderson wrote:
> Hi,
> 
> On Tue, May 10, 2022 at 2:33 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Doug
>>
>> On 5/10/2022 1:53 PM, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Fri, May 6, 2022 at 9:33 AM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>> Hi Jani
>>>>
>>>> On 5/6/2022 4:16 AM, Jani Nikula wrote:
>>>>> On Thu, 05 May 2022, Doug Anderson <dianders@chromium.org> wrote:
>>>>>> Ville,
>>>>>>
>>>>>> On Tue, Apr 26, 2022 at 1:21 PM Douglas Anderson <dianders@chromium.org> wrote:
>>>>>>>
>>>>>>> If we're unable to read the EDID for a display because it's corrupt /
>>>>>>> bogus / invalid then we'll add a set of standard modes for the
>>>>>>> display. When userspace looks at these modes it doesn't really have a
>>>>>>> good concept for which mode to pick and it'll likely pick the highest
>>>>>>> resolution one by default. That's probably not ideal because the modes
>>>>>>> were purely guesses on the part of the Linux kernel.
>>>>>>>
>>>>>>> Let's instead set 640x480 as the "preferred" mode when we have no EDID.
>>>>>>>
>>>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>>>> ---
>>>>>>>
>>>>>>>     drivers/gpu/drm/drm_edid.c | 9 +++++++++
>>>>>>>     1 file changed, 9 insertions(+)
>>>>>>
>>>>>> Someone suggested that you might have an opinion on this patch and
>>>>>> another one I posted recently [1]. Do you have any thoughts on it?
>>>>>> 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.
>>>>>>
>>>>>> [1] https://lore.kernel.org/r/20220426114627.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid
>>>>>
>>>>> There are a number of drivers with combos:
>>>>>
>>>>>         drm_add_modes_noedid()
>>>>>         drm_set_preferred_mode()
>>>>>
>>>>> which I think would be affected by the change. Perhaps you should just
>>>>> call drm_set_preferred_mode() in your referenced patch?
>>>
>>> I'm going to do that and I think it works out pretty well. Patch is at:
>>>
>>> https://lore.kernel.org/r/20220510135101.v2.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid
>>>
>>>
>>>> So it seems like many drivers handle the !edid case within their
>>>> respective get_modes() call which probably is because they know the max
>>>> capability of their connector and because they know which mode should be
>>>> set as preferred. But at the same time, perhaps the code below which
>>>> handles the count == 0 case should be changed like below to make sure we
>>>> are within the max_width/height of the connector (to handle the first
>>>> condition)?
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
>>>> b/drivers/gpu/drm/drm_probe_helper.c
>>>> index 682359512996..6eb89d90777b 100644
>>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>>> @@ -517,7 +517,8 @@ int drm_helper_probe_single_connector_modes(struct
>>>> drm_connector *connector,
>>>>
>>>>            if (count == 0 && (connector->status ==
>>>> connector_status_connected ||
>>>>                               connector->status == connector_status_unknown))
>>>> -               count = drm_add_modes_noedid(connector, 1024, 768);
>>>> +               count = drm_add_modes_noedid(connector,
>>>> connector->dev->mode_config.max_width,
>>>> +                               connector->dev->mode_config.max_height);
>>>>            count += drm_helper_probe_add_cmdline_mode(connector);
>>>>            if (count == 0)
>>>>                    goto prune;
>>>>
>>>>
>>>>> Alternatively, perhaps drm_set_preferred_mode() should erase the
>>>>> previous preferred mode(s) if it finds a matching new preferred mode.
>>>>>
>>>>
>>>> But still yes, even if we change it like above perhaps for other non-DP
>>>> cases its still better to allow individual drivers to pick their
>>>> preferred modes.
>>>>
>>>> If we call drm_set_preferred_mode() in the referenced patch, it will not
>>>> address the no EDID cases because the patch comes into picture when
>>>> there was a EDID with some modes but not with 640x480.
>>>
>>> I'm not sure I understand the above paragraph. I think the "there's an
>>> EDID but no 640x480" is handled by my other patch [1]. Here we're only
>>> worried about the "no EDID" case, right?
>>>
>> Yes, there are two fixes which have been done (OR have to be done).
>>
>> 1) Case when EDID read failed and count of modes was 0.
>>
>> Here the DRM framework was already adding 640x480@60fps. The fix we had
>> to make was making 640x480@60fps as the preferred mode. Which is what
>> your current patch aims at addressing.
>>
>> https://lore.kernel.org/all/20220510135101.v2.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid/
>>
>> So I thought the suggestion which Jani was giving was to call
>> drm_set_preferred_mode() on the referenced patch which was:
>>
>> https://lore.kernel.org/all/20220510131309.v2.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid/
>>
>> So that would not have fixed this case.
>>
>> Perhaps, I misunderstood the patch which was being referenced?
> 
> Ah! I couldn't quite understand what the "referenced patch" meant. I
> assumed that Jani was meaning that we add a call to
> drm_set_preferred_mode() to whatever was calling
> drm_add_modes_noedid().
> 
> 
>> 2) Case where there were other modes, which got filtered out and in the
>> end no modes were left and we had to end up adding 640x480.
>>
>> No need to set the preferred mode in this case as this would have been
>> the only mode in the list ( so becomes preferred by default ).
>>
>> Thats this change
>>
>> https://lore.kernel.org/all/20220426114627.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid/
>>
>> I agree with combination of these 2 it should work.
> 
> OK, cool. So just to be clear: you're good with both "v2" patches that
> I sent out today and they should fix both use cases, right? ;-)

Yes, I did go through the V2s of both the changes and it should address 
both the use cases.

FWIW, I have acked both of them.

Thanks

Abhinav
> 
> -Doug