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

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

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