Hi Daniel & DRM developers,
could you please comment on the change below?
On 7/3/22 16:41, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Sun, Jul 3, 2022 at 3:54 PM Helge Deller <deller@xxxxxx> wrote:
>> * Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>:
>>>> --- a/drivers/video/fbdev/core/fbmem.c
>>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>>> @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>>> if (ret)
>>>> return ret;
>>>>
>>>> + /* make sure virtual resolution >= physical resolution */
>>>> + if (WARN_ON(var->xres_virtual < var->xres)) {
>>>
>>> WARN_ON_ONCE()?
>>> This does mean we would miss two or more buggy drivers in a single system.
>>>
>>>> + pr_warn("fbcon: Fix up invalid xres %d for %s\n",
>>>
>>> xres_virtual?
>>>
>>>> + var->xres_virtual, info->fix.id);
>>>> + var->xres_virtual = var->xres;
>>>
>>> I think it's better to not fix this up, but return -EINVAL instead.
>>> After all if we get here, we have a buggy driver that needs to be fixed.
>>>
>>>> + }
>>>> + if (WARN_ON(var->yres_virtual < var->yres)) {
>>>> + pr_warn("fbcon: Fix up invalid yres %d for %s\n",
>>>> + var->yres_virtual, info->fix.id);
>>>> + var->yres_virtual = var->yres;
>>>> + }
>>>
>>> Same for yres.
>>
>> Geert, thanks for your valuable feedback!
>>
>> In general I don't like for this case any of the WARN_ON* functions.
>> They don't provide any useful info for us, dumps unneccessarily the
>> stack backtrace and would annoy existing users.
>
> Without the stack trace, most people won't notice...
>
>> We know, that DRM drivers can't change the resolution. If we would leave
>> in any kind of warning, all DRM users will ask back - and we don't have
>> a solution for them. It's also no regression, because it didn't worked
>> before either.
>
> The warning will only be triggered when the requested virtual
> resolution is smaller than the requested physical resolution. As
> long as the requested virtual and physical resolution match what
> was returned by FBIOGET_VSCREENINFO before, the warning won't
> be triggered. So in normal use cases, the user won't be impacted.
> Fuzzers will see the warning, but the kernel will no longer crash
> on invalid values.
Still, fuzzers will crash if they have panic_on_warn enabled, which
was the case for the reproducer I got.
>> But we want to detect fbdev drivers which behave badly - so we should
>> print that info with the driver name.
>>
>> Below is a new patch which addresses this. The search for drm drivers
>> looks somewhat hackish - maybe someone can propose a better approach?
>>
>> Thoughts?
>>
>> Helge
>>
>>
>> From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001
>> From: Helge Deller <deller@xxxxxx>
>> Date: Wed, 29 Jun 2022 15:53:55 +0200
>> Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var()
>>
>> Verify that the fbdev or drm driver correctly adjusted the virtual screen
>> sizes. On failure report (in case of fbdev drivers) the failing driver.
>>
>> Signed-off-by: Helge Deller <deller@xxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx # v5.4+
>>
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 160389365a36..9b75aa4405ee 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>> if (ret)
>> return ret;
>>
>> + /* verify that virtual resolution >= physical resolution */
>> + if (var->xres_virtual < var->xres ||
>> + var->yres_virtual < var->yres) {
This is the part I'd like to get feedback from DRM on:
Shall we leave that in, or drop it as Geert suggested?
>> + /* drm drivers don't support mode changes yet. Do not report. */
>> + if (strstr(info->fix.id, "drm"))
>> + return -ENOTSUPP;
>> +
>> + pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual"
>> + " screen size (%dx%d vs. %dx%d)\n",
>> + info->fix.id,
>> + var->xres_virtual, var->yres_virtual,
>> + var->xres, var->yres);
>> + return -EINVAL;
>> + }
>> +
>> if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
>> return 0;
>
> Hence I think there is no need for ignoring drm.
