From: Geert Uytterhoeven <geert@linux-m68k.org> To: Daniel Vetter <daniel@ffwll.ch> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, Javier Martinez Canillas <javierm@redhat.com>, dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/fb-helper: Fix height, width, and accel_flags in fb_var Date: Thu, 20 Apr 2023 11:15:24 +0200 [thread overview] Message-ID: <CAMuHMdWbsFHP7Amoz16o5ge5a=wv5u2x0B+yP7e-0bRJufqrQQ@mail.gmail.com> (raw) In-Reply-To: <ZEAY5Sf/V10ipDZk@phenom.ffwll.local> Hi Daniel, On Wed, Apr 19, 2023 at 6:38 PM Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Apr 18, 2023 at 08:42:46PM +0200, Geert Uytterhoeven wrote: > > Fbtest contains some very simple validation of the fbdev userspace API > > contract. When used with shmob-drm, it reports the following warnings > > and errors: > > > > height changed from 68 to 0 > > height was rounded down > > width changed from 111 to 0 > > width was rounded down > > accel_flags changed from 0 to 1 > > > > The first part happens because __fill_var() resets the physical > > dimensions of the first connector, as filled in by drm_setup_crtcs_fb(). > > Fix this by retaining the original values. > > > > The last part happens because __fill_var() forces the FB_ACCELF_TEXT > > flag on, while fbtest disables all acceleration on purpose, so it can > > draw safely to the frame buffer. Fix this by setting accel_flags to > > zero, as DRM does not implement any text console acceleration. > > Note that this issue can also be seen in the output of fbset, which > > reports "accel true". > > > > Fixes: ee4cce0a8f03a333 ("drm/fb-helper: fix input validation gaps in check_var") > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -2066,7 +2068,7 @@ static void drm_fb_helper_fill_var(struct fb_info *info, > > info->pseudo_palette = fb_helper->pseudo_palette; > > info->var.xoffset = 0; > > info->var.yoffset = 0; > > - __fill_var(&info->var, fb); > > + __fill_var(&info->var, info, fb); > > Bit a bikeshed since it zeroed-allocated anyway, but I'd pass NULL here > for info and catch that in __fill_var and then keep the explicit = 0; Yeah, it's a bit unfortunate this is done in two places, and info->var.{height,width} are initialized by drm_setup_crtcs_fb() only later. Most of the var contents cannot change as mode changes are not supported, so drm_fb_helper_check_var() should just do if (var->foo > info->var.foo) return -EINVAL; var->foo = info->var.foo; For the parts that can change, based on earlier discussions I saw pass by, I believe there should be a call into atomic try-modesetting at the end of drm_fb_helper_check_var()? > Either way Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
WARNING: multiple messages have this Message-ID (diff)
From: Geert Uytterhoeven <geert@linux-m68k.org> To: Daniel Vetter <daniel@ffwll.ch> Cc: linux-fbdev@vger.kernel.org, Javier Martinez Canillas <javierm@redhat.com>, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Thomas Zimmermann <tzimmermann@suse.de> Subject: Re: [PATCH] drm/fb-helper: Fix height, width, and accel_flags in fb_var Date: Thu, 20 Apr 2023 11:15:24 +0200 [thread overview] Message-ID: <CAMuHMdWbsFHP7Amoz16o5ge5a=wv5u2x0B+yP7e-0bRJufqrQQ@mail.gmail.com> (raw) In-Reply-To: <ZEAY5Sf/V10ipDZk@phenom.ffwll.local> Hi Daniel, On Wed, Apr 19, 2023 at 6:38 PM Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Apr 18, 2023 at 08:42:46PM +0200, Geert Uytterhoeven wrote: > > Fbtest contains some very simple validation of the fbdev userspace API > > contract. When used with shmob-drm, it reports the following warnings > > and errors: > > > > height changed from 68 to 0 > > height was rounded down > > width changed from 111 to 0 > > width was rounded down > > accel_flags changed from 0 to 1 > > > > The first part happens because __fill_var() resets the physical > > dimensions of the first connector, as filled in by drm_setup_crtcs_fb(). > > Fix this by retaining the original values. > > > > The last part happens because __fill_var() forces the FB_ACCELF_TEXT > > flag on, while fbtest disables all acceleration on purpose, so it can > > draw safely to the frame buffer. Fix this by setting accel_flags to > > zero, as DRM does not implement any text console acceleration. > > Note that this issue can also be seen in the output of fbset, which > > reports "accel true". > > > > Fixes: ee4cce0a8f03a333 ("drm/fb-helper: fix input validation gaps in check_var") > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -2066,7 +2068,7 @@ static void drm_fb_helper_fill_var(struct fb_info *info, > > info->pseudo_palette = fb_helper->pseudo_palette; > > info->var.xoffset = 0; > > info->var.yoffset = 0; > > - __fill_var(&info->var, fb); > > + __fill_var(&info->var, info, fb); > > Bit a bikeshed since it zeroed-allocated anyway, but I'd pass NULL here > for info and catch that in __fill_var and then keep the explicit = 0; Yeah, it's a bit unfortunate this is done in two places, and info->var.{height,width} are initialized by drm_setup_crtcs_fb() only later. Most of the var contents cannot change as mode changes are not supported, so drm_fb_helper_check_var() should just do if (var->foo > info->var.foo) return -EINVAL; var->foo = info->var.foo; For the parts that can change, based on earlier discussions I saw pass by, I believe there should be a call into atomic try-modesetting at the end of drm_fb_helper_check_var()? > Either way Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
next prev parent reply other threads:[~2023-04-20 9:15 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-04-18 18:42 [PATCH] drm/fb-helper: Fix height, width, and accel_flags in fb_var Geert Uytterhoeven 2023-04-18 18:42 ` Geert Uytterhoeven 2023-04-19 16:37 ` Daniel Vetter 2023-04-19 16:37 ` Daniel Vetter 2023-04-20 9:15 ` Geert Uytterhoeven [this message] 2023-04-20 9:15 ` Geert Uytterhoeven 2023-04-27 9:43 ` Daniel Vetter 2023-04-27 9:43 ` Daniel Vetter 2023-04-29 3:45 ` Sui Jingfeng
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAMuHMdWbsFHP7Amoz16o5ge5a=wv5u2x0B+yP7e-0bRJufqrQQ@mail.gmail.com' \ --to=geert@linux-m68k.org \ --cc=airlied@gmail.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=javierm@redhat.com \ --cc=linux-fbdev@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-renesas-soc@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mripard@kernel.org \ --cc=tzimmermann@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.