Messages in this thread | | | Date | Tue, 10 May 2022 17:03:46 -0700 | Subject | Re: [RFC PATCH] drm/edid: drm_add_modes_noedid() should set lowest resolution as preferred | From | 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
| |