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
