Message ID | 2b6073d9c2d869c6a4eac6edebd616e0568dec91.1681843245.git.geert+renesas@glider.be |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp3062494vqo; Tue, 18 Apr 2023 11:58:52 -0700 (PDT) X-Google-Smtp-Source: AKy350bvUMGBSjwOFboRacwoXYo5Ib++cNjVKyORF722O+Ic/RBSSx27Rv2WBr9xuU1Ue2MCouKl X-Received: by 2002:a05:6a20:431a:b0:f1:1af5:75fe with SMTP id h26-20020a056a20431a00b000f11af575femr386743pzk.36.1681844332665; Tue, 18 Apr 2023 11:58:52 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681844332; cv=none; d=google.com; s=arc-20160816; b=0Vk4c/yunievbyeB1o2DzrkIAI9zjLPsG1TTI5EUrBBx0MxTd0ia++QIPwraFXdMki B6pPFWF6DXuyO6BwFGABOUhf6mdCqNZ0GY0biK1dOQMQk0rzasbKNwqnYrMyTNoMOPxW PtDKU5uoR8u2Pf95QMw8dYA/owDpblJyqmz51FuPtKMEjwTllcNt/jxx1oa6USkOdaqZ +SFdph7yosVYmuH2CXE2iJGcdtUKYEruJHJuDGuGduaJaHHhQh1inlEo7tw2zugLDXM4 j9YBcZMMC0Zm8XR4TYWmX7yZsmiApZunGiOGclaTdU544hSYj32SUWk7RWF2MJRJZYJG 3tRw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=CGU+nZvybx/lcWqTN0lem2NtkJr2mq1iS8XdCmLhRtM=; b=Co1EybyJQ9CmxTuhohdJbdK358ZYwwGNFEZv786mlzp3zKRcNJNGq97ULYRA7b41Py HNUOFyJr6QeiRoKpxBqgUmGssaPHSFAO8I4Lt6avqXCATRAWTIoPyOnTeY08wKq1cfGQ EyhHyl4FjU7LZMrCvhEoEqpzV8A6wmuZPif3U+CnNjdY0c+65T2TShCBC25VwNCKiY9l BZiUooRxL73Ve9eiW36JCLFILoBEMdabNpcYX0/wxkkSqA/av24YoYU6EWURMNpHXdaB JZkoI0eG9rsrv+9t96HOv95fKSk0+W9CV1+26ta75UISG0bIH7EqmS1AVAUIsVxAfc1L m2eQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b8-20020a63d808000000b0051948f1993fsi15316106pgh.40.2023.04.18.11.58.38; Tue, 18 Apr 2023 11:58:52 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232527AbjDRSrY (ORCPT <rfc822;peter110.wang@gmail.com> + 99 others); Tue, 18 Apr 2023 14:47:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39366 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232660AbjDRSrU (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 18 Apr 2023 14:47:20 -0400 Received: from laurent.telenet-ops.be (laurent.telenet-ops.be [IPv6:2a02:1800:110:4::f00:19]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C2CE72694 for <linux-kernel@vger.kernel.org>; Tue, 18 Apr 2023 11:47:18 -0700 (PDT) Received: from ramsan.of.borg ([84.195.187.55]) by laurent.telenet-ops.be with bizsmtp id mJnE290011C8whw01JnE6D; Tue, 18 Apr 2023 20:47:15 +0200 Received: from rox.of.borg ([192.168.97.57]) by ramsan.of.borg with esmtp (Exim 4.95) (envelope-from <geert@linux-m68k.org>) id 1poqH5-00H8P4-E9; Tue, 18 Apr 2023 20:42:51 +0200 Received: from geert by rox.of.borg with local (Exim 4.95) (envelope-from <geert@linux-m68k.org>) id 1poqHz-00EGQt-IY; Tue, 18 Apr 2023 20:42:51 +0200 From: Geert Uytterhoeven <geert+renesas@glider.be> To: Daniel Vetter <daniel@ffwll.ch>, 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> Cc: dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org, Geert Uytterhoeven <geert+renesas@glider.be> Subject: [PATCH] drm/fb-helper: Fix height, width, and accel_flags in fb_var Date: Tue, 18 Apr 2023 20:42:46 +0200 Message-Id: <2b6073d9c2d869c6a4eac6edebd616e0568dec91.1681843245.git.geert+renesas@glider.be> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.4 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE, T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1763541603228535734?= X-GMAIL-MSGID: =?utf-8?q?1763541603228535734?= |
Series |
drm/fb-helper: Fix height, width, and accel_flags in fb_var
|
|
Commit Message
Geert Uytterhoeven
April 18, 2023, 6:42 p.m. UTC
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>
---
drivers/gpu/drm/drm_fb_helper.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
Comments
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> > --- > drivers/gpu/drm/drm_fb_helper.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 64458982be40c468..ed6ad787915f0b8f 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1537,17 +1537,19 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var, > } > } > > -static void __fill_var(struct fb_var_screeninfo *var, > +static void __fill_var(struct fb_var_screeninfo *var, struct fb_info *info, > struct drm_framebuffer *fb) > { > int i; > > var->xres_virtual = fb->width; > var->yres_virtual = fb->height; > - var->accel_flags = FB_ACCELF_TEXT; > + var->accel_flags = 0; > var->bits_per_pixel = drm_format_info_bpp(fb->format, 0); > > - var->height = var->width = 0; > + var->height = info->var.height; > + var->width = info->var.width; > + > var->left_margin = var->right_margin = 0; > var->upper_margin = var->lower_margin = 0; > var->hsync_len = var->vsync_len = 0; > @@ -1610,7 +1612,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > return -EINVAL; > } > > - __fill_var(var, fb); > + __fill_var(var, info, fb); > > /* > * fb_pan_display() validates this, but fb_set_par() doesn't and just > @@ -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; Either way Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > info->var.activate = FB_ACTIVATE_NOW; > > drm_fb_helper_fill_pixel_fmt(&info->var, format); > -- > 2.34.1 >
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
On Thu, Apr 20, 2023 at 11:15:24AM +0200, Geert Uytterhoeven wrote: > 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()? Yeah ideally that's what we do. And I guess we could limit that to atomic-only drivers since with legacy kms there's really no way to correctly implement any fbdev mode changes (because there's just no check/commit split there and fbdev wants that). I guess the trickier part is reworking the drm fbdev code so that it won't tamper with it's internal structures (or roll them back) when only doing a TEST_ONLY commit. And in theory we could then actually support proper mode changes through fbdev, as long as it fits into the fb we allocated at least. Reallocating fbs would be a lot more intrusive but also not impossible. -Daniel > > 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
Hi, I have just tested this patch on a LoongArch(3a5000+ls7a2000 evb) machine, both fbtest and the fbdev test of IGT finished. fbtest say test001: ~ test013: PASSED, After apply your patch, the warn log `accel_flags changed from 0 to 1` disappeared while running it. So, Tested-by: Sui Jingfeng <suijingfeng@loongson.cn> On 2023/4/19 02:42, 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> > --- > drivers/gpu/drm/drm_fb_helper.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 64458982be40c468..ed6ad787915f0b8f 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1537,17 +1537,19 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var, > } > } > > -static void __fill_var(struct fb_var_screeninfo *var, > +static void __fill_var(struct fb_var_screeninfo *var, struct fb_info *info, > struct drm_framebuffer *fb) > { > int i; > > var->xres_virtual = fb->width; > var->yres_virtual = fb->height; > - var->accel_flags = FB_ACCELF_TEXT; > + var->accel_flags = 0; > var->bits_per_pixel = drm_format_info_bpp(fb->format, 0); > > - var->height = var->width = 0; > + var->height = info->var.height; > + var->width = info->var.width; > + > var->left_margin = var->right_margin = 0; > var->upper_margin = var->lower_margin = 0; > var->hsync_len = var->vsync_len = 0; > @@ -1610,7 +1612,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > return -EINVAL; > } > > - __fill_var(var, fb); > + __fill_var(var, info, fb); > > /* > * fb_pan_display() validates this, but fb_set_par() doesn't and just > @@ -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); > info->var.activate = FB_ACTIVATE_NOW; > > drm_fb_helper_fill_pixel_fmt(&info->var, format);
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 64458982be40c468..ed6ad787915f0b8f 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1537,17 +1537,19 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var, } } -static void __fill_var(struct fb_var_screeninfo *var, +static void __fill_var(struct fb_var_screeninfo *var, struct fb_info *info, struct drm_framebuffer *fb) { int i; var->xres_virtual = fb->width; var->yres_virtual = fb->height; - var->accel_flags = FB_ACCELF_TEXT; + var->accel_flags = 0; var->bits_per_pixel = drm_format_info_bpp(fb->format, 0); - var->height = var->width = 0; + var->height = info->var.height; + var->width = info->var.width; + var->left_margin = var->right_margin = 0; var->upper_margin = var->lower_margin = 0; var->hsync_len = var->vsync_len = 0; @@ -1610,7 +1612,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, return -EINVAL; } - __fill_var(var, fb); + __fill_var(var, info, fb); /* * fb_pan_display() validates this, but fb_set_par() doesn't and just @@ -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); info->var.activate = FB_ACTIVATE_NOW; drm_fb_helper_fill_pixel_fmt(&info->var, format);