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@xxxxxxxxxxx> 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@xxxxxxxxxxx> wrote:
> >>
> >> Hi Jani
> >>
> >> On 5/6/2022 4:16 AM, Jani Nikula wrote:
> >>> On Thu, 05 May 2022, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> >>>> Ville,
> >>>>
> >>>> On Tue, Apr 26, 2022 at 1:21 PM Douglas Anderson <dianders@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >>>>> ---
> >>>>>
> >>>>>    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