On Tue, 21 Jun 2022 at 12:33, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 21.06.22 um 12:20 schrieb Daniel Vetter:
> > On Tue, 21 Jun 2022 at 12:15, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> >>
> >> Hi
> >>
> >> Am 21.06.22 um 11:23 schrieb Daniel Vetter:
> >>> The drm fbdev emulation does not forward mode changes to the driver,
> >>> and hence all changes where rejected in 865afb11949e ("drm/fb-helper:
> >>> reject any changes to the fbdev").
> >>>
> >>> Unfortunately this resulted in bugs on multiple monitor systems with
> >>> different resolutions. In that case the fbdev emulation code sizes the
> >>> underlying framebuffer for the largest screen (which dictates
> >>> x/yres_virtual), but adjust the fbdev x/yres to match the smallest
> >>> resolution. The above mentioned patch failed to realize that, and
> >>> errornously validated x/yres against the fb dimensions.
> >>>
> >>> This was fixed by just dropping the validation for too small sizes,
> >>> which restored vt switching with 12ffed96d436 ("drm/fb-helper: Allow
> >>> var->x/yres(_virtual) < fb->width/height again").
> >>>
> >>> But this also restored all kinds of validation issues and their
> >>> fallout in the notoriously buggy fbcon code for too small sizes. Since
> >>> no one is volunteering to really make fbcon and vc/vt fully robust
> >>> against these math issues make sure this barn door is closed for good
> >>> again.
> >>>
> >>> Since it's a bit tricky to remember the x/yres we picked across both
> >>> the newer generic fbdev emulation and the older code with more driver
> >>> involvement, we simply check that it doesn't change. This relies on
> >>> drm_fb_helper_fill_var() having done things correctly, and nothing
> >>> having trampled it yet.
> >>>
> >>> Note that this leaves all the other fbdev drivers out in the rain.
> >>> Given that distros have finally started to move away from those
> >>> completely for real I think that's good enough. The code it spaghetti
> >>> enough that I do not feel confident to even review fixes for it.
> >>>
> >>> What might help fbdev is doing something similar to what was done in
> >>> a49145acfb97 ("fbmem: add margin check to fb_check_caps()") and ensure
> >>> x/yres_virtual aren't too small, for some value of "too small". Maybe
> >>> checking that they're at least x/yres makes sense?
> >>>
> >>> Fixes: 12ffed96d436 ("drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again")
> >>> Cc: Michel Dänzer <michel.daenzer@xxxxxxx>
> >>> Cc: Daniel Stone <daniels@xxxxxxxxxxxxx>
> >>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >>> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> >>> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >>> Cc: <stable@xxxxxxxxxxxxxxx> # v4.11+
> >>> Cc: Helge Deller <deller@xxxxxx>
> >>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >>> Cc: openeuler-security@xxxxxxxxxxxxx
> >>> Cc: guodaxing@xxxxxxxxxx
> >>> Cc: Weigang (Jimmy) <weigang12@xxxxxxxxxx>
> >>> Reported-by: Weigang (Jimmy) <weigang12@xxxxxxxxxx>
> >>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> >>> ---
> >>> Note: Weigang asked for this to stay under embargo until it's all
> >>> review and tested.
> >>> -Daniel
> >>> ---
> >>> drivers/gpu/drm/drm_fb_helper.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>> index 695997ae2a7c..5664a177a404 100644
> >>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>> @@ -1355,8 +1355,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >>> * to KMS, hence fail if different settings are requested.
> >>> */
> >>> if (var->bits_per_pixel > fb->format->cpp[0] * 8 ||
> >>> - var->xres > fb->width || var->yres > fb->height ||
> >>> - var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
> >>> + var->xres != info->var.xres || var->yres != info->var.yres ||
> >>
> >> This looks reasonable. We effectively only support a single resolution here.
> >>
> >>> + var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
> >>
> >> AFAIU this change would require that all userspace always uses maximum
> >> values for {xres,yres}_virtual. Seems unrealistic to me.
> >
> > The thing is, they kinda have to, because that's always going to be
> > the actually visible part :-) Otoh I guess we could also allow virtual
> > size to match the fbdev real size, but maybe that kind of sanity check
> > should be done in fbmem.c?
>
> I'm not sure I understand. I'd expect that fbdev userspace allocates
> xres_virtual to be twice the size of xres. So it can do double
> buffering. In many cases fb->height would be larger than that.
>
> >
> > Tbh for these kind of things I'm leaning towards "let's wait until we
> > get a regression report", since there's simply too many random bugs
>
> Not that I disagree, but in this case a regression report seems inevitable.
Hm yeah maybe we need to allow the flexibility.
> > all over in the fbcon/vc/vt code when sou start resizing stuff. So I'm
> > very heavily leaning towards rejecting everything (and e.g. we should
> > have fixed this all up already in 2020 when the bugfix for x/yres
> > related underflows landed in 2020 imo).
>
> Do the fbdev igt tests work with this change if overalloc has been set
> to more than 200?
Hm haven't tried, but I guess I'll just cc the thing to our trybot CI.
The joys of patch testing when you can't really use any of the public
CI systems by default.
-Daniel
>
> Best regards
> Thomas
>
> > -Daniel
> >
> >> Best regards
> >> Thomas
> >>
> >>
> >>> drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb "
> >>> "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
> >>> var->xres, var->yres, var->bits_per_pixel,
> >>
> >> --
> >> Thomas Zimmermann
> >> Graphics Driver Developer
> >> SUSE Software Solutions Germany GmbH
> >> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> (HRB 36809, AG Nürnberg)
> >> Geschäftsführer: Ivo Totev
> >
> >
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
