| Message ID | 20230404194038.472803-3-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/fb-helper: set x/yres_virtual in drm_fb_helper_check_var | expand |
Daniel Vetter <daniel.vetter@ffwll.ch> writes: > Apparently drivers need to check all this stuff themselves, which for > most things makes sense I guess. And for everything else we luck out, > because modern distros stopped supporting any other fbdev drivers than > drm ones and I really don't want to argue anymore about who needs to > check stuff. Therefore fixing all this just for drm fbdev emulation is > good enough. > Agreed. > Note that var->active is not set or validated. This is just control > flow for fbmem.c and needs to be validated in there as needed. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > --- [...] > > +static void __fill_var(struct fb_var_screeninfo *var, > + struct drm_framebuffer *fb) > +{ > + int i; > + > + var->xres_virtual = fb->width; > + var->yres_virtual = fb->height; > + var->accel_flags = FB_ACCELF_TEXT; > + var->bits_per_pixel = drm_format_info_bpp(fb->format, 0); > + > + var->height = var->width = 0; > + var->left_margin = var->right_margin = 0; > + var->upper_margin = var->lower_margin = 0; > + var->hsync_len = var->vsync_len = 0; > + var->sync = var->vmode = 0; > + var->rotate = 0; > + var->colorspace = 0; > + for (i = 0; i < 4; i++) > + var->reserved[i] = 0; > +} > + > /** > * drm_fb_helper_check_var - implementation for &fb_ops.fb_check_var > * @var: screeninfo to check > @@ -1595,8 +1616,22 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > return -EINVAL; > } > > - var->xres_virtual = fb->width; > - var->yres_virtual = fb->height; > + __fill_var(var, fb); > + [...] There is the following here (in latest drm-misc/drm-misc-next at least): /* * Changes struct fb_var_screeninfo are currently not pushed back * to KMS, hence fail if different settings are requested. */ bpp = drm_format_info_bpp(format, 0); if (var->bits_per_pixel > bpp || var->xres > fb->width || var->yres > fb->height || var->xres_virtual > fb->width || var->yres_virtual > fb->height) { 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, var->xres_virtual, var->yres_virtual, fb->width, fb->height, bpp); return -EINVAL; } but only the 'var->xres > fb->width || var->yres > fb->height' from the conditions checked could be false after your __fill_var() call above. You should drop the 'var->bits_per_pixel > bpp', 'var->xres_virtual > fb->width' and 'var->yres_virtual > fb->height' checks I believe since those will always be true. > + /* > + * fb_pan_display() validates this, but fb_set_par() doesn't and just > + * falls over. Note that __fill_var above adjusts y/res_virtual. > + */ > + if (var->yoffset > var->yres_virtual - var->yres || > + var->xoffset > var->xres_virtual - var->xres) > + return -EINVAL; > + > + /* We neither support grayscale nor FOURCC (also stored in here). */ > + if (var->grayscale > 0) > + return -EINVAL; > + > + if (var->nonstd) > + return -EINVAL; > > /* > * Workaround for SDL 1.2, which is known to be setting all pixel format > @@ -1612,11 +1647,6 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > drm_fb_helper_fill_pixel_fmt(var, format); > } > Other than what I mentioned, the patch makes sense to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
On Wed, Apr 05, 2023 at 12:52:12PM +0200, Javier Martinez Canillas wrote: > Daniel Vetter <daniel.vetter@ffwll.ch> writes: > > > Apparently drivers need to check all this stuff themselves, which for > > most things makes sense I guess. And for everything else we luck out, > > because modern distros stopped supporting any other fbdev drivers than > > drm ones and I really don't want to argue anymore about who needs to > > check stuff. Therefore fixing all this just for drm fbdev emulation is > > good enough. > > > > Agreed. > > > Note that var->active is not set or validated. This is just control > > flow for fbmem.c and needs to be validated in there as needed. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > --- > > [...] > > > > > +static void __fill_var(struct fb_var_screeninfo *var, > > + struct drm_framebuffer *fb) > > +{ > > + int i; > > + > > + var->xres_virtual = fb->width; > > + var->yres_virtual = fb->height; > > + var->accel_flags = FB_ACCELF_TEXT; > > + var->bits_per_pixel = drm_format_info_bpp(fb->format, 0); > > + > > + var->height = var->width = 0; > > + var->left_margin = var->right_margin = 0; > > + var->upper_margin = var->lower_margin = 0; > > + var->hsync_len = var->vsync_len = 0; > > + var->sync = var->vmode = 0; > > + var->rotate = 0; > > + var->colorspace = 0; > > + for (i = 0; i < 4; i++) > > + var->reserved[i] = 0; > > +} > > + > > /** > > * drm_fb_helper_check_var - implementation for &fb_ops.fb_check_var > > * @var: screeninfo to check > > @@ -1595,8 +1616,22 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > > return -EINVAL; > > } > > > > - var->xres_virtual = fb->width; > > - var->yres_virtual = fb->height; > > + __fill_var(var, fb); > > + > > [...] > > There is the following here (in latest drm-misc/drm-misc-next at least): > > /* > * Changes struct fb_var_screeninfo are currently not pushed back > * to KMS, hence fail if different settings are requested. > */ > bpp = drm_format_info_bpp(format, 0); > if (var->bits_per_pixel > bpp || > var->xres > fb->width || var->yres > fb->height || > var->xres_virtual > fb->width || var->yres_virtual > fb->height) { > 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, > var->xres_virtual, var->yres_virtual, > fb->width, fb->height, bpp); > return -EINVAL; > } > > but only the 'var->xres > fb->width || var->yres > fb->height' from the > conditions checked could be false after your __fill_var() call above. > > You should drop the 'var->bits_per_pixel > bpp', 'var->xres_virtual > > fb->width' and 'var->yres_virtual > fb->height' checks I believe since > those will always be true. The __fill_var is after this. I'm honestly not sure what the exact semantics are supposed to be, but essentially if userspace asks for too big virtual size, we reject it. And for anything else we then tell it (with __fill_var) how big the actually available space is. What I'm wondering now is whether too small x/yres won't lead to problems of some sorts ... For multi-screen we set the virtual size to be big enough for all crtc, and then just set x/yres to be the smallest output. That way fbcon knows to only draw as much as is visible on all screens. But if you then pan that too much, the bigger screens might not have a big enough buffer anymore and things fail (but shouldn't). Not sure how to fix that tbh. -Daniel > > > + /* > > + * fb_pan_display() validates this, but fb_set_par() doesn't and just > > + * falls over. Note that __fill_var above adjusts y/res_virtual. > > + */ > > + if (var->yoffset > var->yres_virtual - var->yres || > > + var->xoffset > var->xres_virtual - var->xres) > > + return -EINVAL; > > + > > + /* We neither support grayscale nor FOURCC (also stored in here). */ > > + if (var->grayscale > 0) > > + return -EINVAL; > > + > > + if (var->nonstd) > > + return -EINVAL; > > > > /* > > * Workaround for SDL 1.2, which is known to be setting all pixel format > > @@ -1612,11 +1647,6 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > > drm_fb_helper_fill_pixel_fmt(var, format); > > } > > > > Other than what I mentioned, the patch makes sense to me. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > -- > Best regards, > > Javier Martinez Canillas > Core Platforms > Red Hat >
Daniel Vetter <daniel@ffwll.ch> writes: [...] >> >> but only the 'var->xres > fb->width || var->yres > fb->height' from the >> conditions checked could be false after your __fill_var() call above. >> >> You should drop the 'var->bits_per_pixel > bpp', 'var->xres_virtual > >> fb->width' and 'var->yres_virtual > fb->height' checks I believe since >> those will always be true. > > The __fill_var is after this. I'm honestly not sure what the exact Ah, your patch adds it after that indeed. Please ignore my comment then. > semantics are supposed to be, but essentially if userspace asks for too > big virtual size, we reject it. And for anything else we then tell it > (with __fill_var) how big the actually available space is. > > What I'm wondering now is whether too small x/yres won't lead to problems > of some sorts ... For multi-screen we set the virtual size to be big > enough for all crtc, and then just set x/yres to be the smallest output. > That way fbcon knows to only draw as much as is visible on all screens. > But if you then pan that too much, the bigger screens might not have a big > enough buffer anymore and things fail (but shouldn't). > > Not sure how to fix that tbh. Would this be a problem in practice?
On Wed, Apr 05, 2023 at 06:27:17PM +0200, Javier Martinez Canillas wrote: > Daniel Vetter <daniel@ffwll.ch> writes: > > [...] > > >> > >> but only the 'var->xres > fb->width || var->yres > fb->height' from the > >> conditions checked could be false after your __fill_var() call above. > >> > >> You should drop the 'var->bits_per_pixel > bpp', 'var->xres_virtual > > >> fb->width' and 'var->yres_virtual > fb->height' checks I believe since > >> those will always be true. > > > > The __fill_var is after this. I'm honestly not sure what the exact > > Ah, your patch adds it after that indeed. Please ignore my comment then. So rb: you? > > semantics are supposed to be, but essentially if userspace asks for too > > big virtual size, we reject it. And for anything else we then tell it > > (with __fill_var) how big the actually available space is. > > > > What I'm wondering now is whether too small x/yres won't lead to problems > > of some sorts ... For multi-screen we set the virtual size to be big > > enough for all crtc, and then just set x/yres to be the smallest output. > > That way fbcon knows to only draw as much as is visible on all screens. > > But if you then pan that too much, the bigger screens might not have a big > > enough buffer anymore and things fail (but shouldn't). > > > > Not sure how to fix that tbh. > > Would this be a problem in practice? I'm frankly not sure. You'd get a black screen for fbcon/fbdev across all outputs, but only if you have userspace doing this intentionally. In a way it's just another artifact of the drm fbdev emulation not using ATOMIC_TEST_ONLY in the various places where it should, and so doesn't really know whether a configuration change will work out. We already have this in obscure mulit-monitor cases where adding another screen kills fbcon, because the display hw is running out of fifo or clocks or whatever, and because the drm fbdev code doesn't check but just blindly commits the entire thing as an atomic commit, the overall commit fails. This worked "better" with legacy kms because there we commit per-crtc, so if any specific crtc runs into a limit check, only that one fails to light up. Imo given that no one cared enough yet to write up atomic TEST_ONLY support for fbdev emulation I think we can continue to just ignore this problem. What should not happen is that fbcon code blows up drawing out of bounds or something like that, resulting in a kernel crash. So from that pov I think it's "safe" :-) -Daniel
Daniel Vetter <daniel@ffwll.ch> writes: > On Wed, Apr 05, 2023 at 06:27:17PM +0200, Javier Martinez Canillas wrote: >> Daniel Vetter <daniel@ffwll.ch> writes: [...] >> > >> > The __fill_var is after this. I'm honestly not sure what the exact >> >> Ah, your patch adds it after that indeed. Please ignore my comment then. > > So rb: you? > Yes, I already provided it in my previous email and has been picked by patchwork. I could do again but probably will confuse dim when applying. The only patch from your series that is missing an {r,a}b is #1 right now: https://patchwork.kernel.org/project/dri-devel/list/?series=736966&archived=both [...] >> > What I'm wondering now is whether too small x/yres won't lead to problems >> > of some sorts ... For multi-screen we set the virtual size to be big >> > enough for all crtc, and then just set x/yres to be the smallest output. >> > That way fbcon knows to only draw as much as is visible on all screens. >> > But if you then pan that too much, the bigger screens might not have a big >> > enough buffer anymore and things fail (but shouldn't). >> > >> > Not sure how to fix that tbh. >> >> Would this be a problem in practice? > > I'm frankly not sure. You'd get a black screen for fbcon/fbdev across all > outputs, but only if you have userspace doing this intentionally. > > In a way it's just another artifact of the drm fbdev emulation not using > ATOMIC_TEST_ONLY in the various places where it should, and so doesn't > really know whether a configuration change will work out. > > We already have this in obscure mulit-monitor cases where adding another > screen kills fbcon, because the display hw is running out of fifo or > clocks or whatever, and because the drm fbdev code doesn't check but just > blindly commits the entire thing as an atomic commit, the overall commit > fails. > > This worked "better" with legacy kms because there we commit per-crtc, so > if any specific crtc runs into a limit check, only that one fails to light > up. > > Imo given that no one cared enough yet to write up atomic TEST_ONLY > support for fbdev emulation I think we can continue to just ignore this > problem. > Agreed. If that ends being a problem for people in practice then I guess someone can type atomic TEST_ONLY support for the fbdev emulation layer. > What should not happen is that fbcon code blows up drawing out of bounds > or something like that, resulting in a kernel crash. So from that pov I > think it's "safe" :-) Great. Thanks a lot for your explanations. > -Daniel
On Wed, Apr 05, 2023 at 07:42:08PM +0200, Javier Martinez Canillas wrote: > Daniel Vetter <daniel@ffwll.ch> writes: > > > On Wed, Apr 05, 2023 at 06:27:17PM +0200, Javier Martinez Canillas wrote: > >> Daniel Vetter <daniel@ffwll.ch> writes: > > [...] > > >> > > >> > The __fill_var is after this. I'm honestly not sure what the exact > >> > >> Ah, your patch adds it after that indeed. Please ignore my comment then. > > > > So rb: you? > > > > Yes, I already provided it in my previous email and has been picked by > patchwork. I could do again but probably will confuse dim when applying. Yeah just wanted to confirm I cleared up all your questions. Merged the entire series to drm-misc-next, thanks for the review. > The only patch from your series that is missing an {r,a}b is #1 right now: > > https://patchwork.kernel.org/project/dri-devel/list/?series=736966&archived=both That's a different one :-) I'll respin with your comments and then let you&Thomas duke it out about patch 1. -Daniel > > [...] > > >> > What I'm wondering now is whether too small x/yres won't lead to problems > >> > of some sorts ... For multi-screen we set the virtual size to be big > >> > enough for all crtc, and then just set x/yres to be the smallest output. > >> > That way fbcon knows to only draw as much as is visible on all screens. > >> > But if you then pan that too much, the bigger screens might not have a big > >> > enough buffer anymore and things fail (but shouldn't). > >> > > >> > Not sure how to fix that tbh. > >> > >> Would this be a problem in practice? > > > > I'm frankly not sure. You'd get a black screen for fbcon/fbdev across all > > outputs, but only if you have userspace doing this intentionally. > > > > In a way it's just another artifact of the drm fbdev emulation not using > > ATOMIC_TEST_ONLY in the various places where it should, and so doesn't > > really know whether a configuration change will work out. > > > > We already have this in obscure mulit-monitor cases where adding another > > screen kills fbcon, because the display hw is running out of fifo or > > clocks or whatever, and because the drm fbdev code doesn't check but just > > blindly commits the entire thing as an atomic commit, the overall commit > > fails. > > > > This worked "better" with legacy kms because there we commit per-crtc, so > > if any specific crtc runs into a limit check, only that one fails to light > > up. > > > > Imo given that no one cared enough yet to write up atomic TEST_ONLY > > support for fbdev emulation I think we can continue to just ignore this > > problem. > > > > Agreed. If that ends being a problem for people in practice then I guess > someone can type atomic TEST_ONLY support for the fbdev emulation layer. > > > What should not happen is that fbcon code blows up drawing out of bounds > > or something like that, resulting in a kernel crash. So from that pov I > > think it's "safe" :-) > > Great. Thanks a lot for your explanations. > > > -Daniel > > -- > Best regards, > > Javier Martinez Canillas > Core Platforms > Red Hat >
Daniel Vetter <daniel@ffwll.ch> writes: > On Wed, Apr 05, 2023 at 07:42:08PM +0200, Javier Martinez Canillas wrote: [...] >> >> Ah, your patch adds it after that indeed. Please ignore my comment then. >> > >> > So rb: you? >> > >> >> Yes, I already provided it in my previous email and has been picked by >> patchwork. I could do again but probably will confuse dim when applying. > > Yeah just wanted to confirm I cleared up all your questions. Merged the > entire series to drm-misc-next, thanks for the review. > You are welcome. >> The only patch from your series that is missing an {r,a}b is #1 right now: >> >> https://patchwork.kernel.org/project/dri-devel/list/?series=736966&archived=both > > That's a different one :-) > Oh, sorry about that. Somehow I switched threads in my head in the middle of the response. > I'll respin with your comments and then let you&Thomas duke it out about > patch 1. > -Daniel > Perfect, thanks! It would be good to finally have that issue fixed.
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index b9696d13a741..ef4eb8b12766 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1543,6 +1543,27 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var, } } +static void __fill_var(struct fb_var_screeninfo *var, + struct drm_framebuffer *fb) +{ + int i; + + var->xres_virtual = fb->width; + var->yres_virtual = fb->height; + var->accel_flags = FB_ACCELF_TEXT; + var->bits_per_pixel = drm_format_info_bpp(fb->format, 0); + + var->height = var->width = 0; + var->left_margin = var->right_margin = 0; + var->upper_margin = var->lower_margin = 0; + var->hsync_len = var->vsync_len = 0; + var->sync = var->vmode = 0; + var->rotate = 0; + var->colorspace = 0; + for (i = 0; i < 4; i++) + var->reserved[i] = 0; +} + /** * drm_fb_helper_check_var - implementation for &fb_ops.fb_check_var * @var: screeninfo to check @@ -1595,8 +1616,22 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, return -EINVAL; } - var->xres_virtual = fb->width; - var->yres_virtual = fb->height; + __fill_var(var, fb); + + /* + * fb_pan_display() validates this, but fb_set_par() doesn't and just + * falls over. Note that __fill_var above adjusts y/res_virtual. + */ + if (var->yoffset > var->yres_virtual - var->yres || + var->xoffset > var->xres_virtual - var->xres) + return -EINVAL; + + /* We neither support grayscale nor FOURCC (also stored in here). */ + if (var->grayscale > 0) + return -EINVAL; + + if (var->nonstd) + return -EINVAL; /* * Workaround for SDL 1.2, which is known to be setting all pixel format @@ -1612,11 +1647,6 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, drm_fb_helper_fill_pixel_fmt(var, format); } - /* - * Likewise, bits_per_pixel should be rounded up to a supported value. - */ - var->bits_per_pixel = bpp; - /* * drm fbdev emulation doesn't support changing the pixel format at all, * so reject all pixel format changing requests. @@ -2040,12 +2070,9 @@ static void drm_fb_helper_fill_var(struct fb_info *info, } info->pseudo_palette = fb_helper->pseudo_palette; - info->var.xres_virtual = fb->width; - info->var.yres_virtual = fb->height; - info->var.bits_per_pixel = drm_format_info_bpp(format, 0); - info->var.accel_flags = FB_ACCELF_TEXT; info->var.xoffset = 0; info->var.yoffset = 0; + __fill_var(&info->var, fb); info->var.activate = FB_ACTIVATE_NOW; drm_fb_helper_fill_pixel_fmt(&info->var, format);
Apparently drivers need to check all this stuff themselves, which for most things makes sense I guess. And for everything else we luck out, because modern distros stopped supporting any other fbdev drivers than drm ones and I really don't want to argue anymore about who needs to check stuff. Therefore fixing all this just for drm fbdev emulation is good enough. Note that var->active is not set or validated. This is just control flow for fbmem.c and needs to be validated in there as needed. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_fb_helper.c | 49 +++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 11 deletions(-)