Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
[...]
>
> +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@xxxxxxxxxx>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
