[RFC] drm/fb-helper: reject any changes to the fbdev

[Ville Syrjälä]

On Wed, Oct 12, 2016 at 08:55:45AM -0700, Stefan Agner wrote:
> On 2016-10-12 03:42, Ville Syrjälä wrote:
> > On Tue, Oct 11, 2016 at 04:15:04PM -0700, Stefan Agner wrote:
> >> The current fbdev emulation does not allow to push back changes in
> >> width, height or depth to KMS, hence reject any changes with an
> >> error. This makes sure that fbdev ioctl's fail properly and user
> >> space does not assume that changes succeeded.
> >>
> >> Signed-off-by: Stefan Agner <stefan at agner.ch>
> >> ---
> >> This rejects reconfiguration of framebuffer like
> >> fbset -rgba 5,6,5 -depth 16 (when in 24 bit mode by default)
> >> fbset -xres 123
> >>
> >> I think all users of drm_fb_helper_check_var use also the generic
> >> drm_fb_helper_set_par (or do otherwise not support changing size/
> >> depth). Hence, afaict, the change should be the right thing to do
> >> for all driver...
> >>
> >>  drivers/gpu/drm/drm_fb_helper.c | 13 ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >> index 03414bd..596c056 100644
> >> --- a/drivers/gpu/drm/drm_fb_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >> @@ -1211,11 +1211,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >>  	if (var->pixclock != 0 || in_dbg_master())
> >>  		return -EINVAL;
> >>
> >> -	/* Need to resize the fb object !!! */
> >> -	if (var->bits_per_pixel > fb->bits_per_pixel ||
> >> -	    var->xres > fb->width || var->yres > fb->height ||
> >> -	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
> >> -		DRM_DEBUG("fb userspace requested width/height/bpp is greater than current fb "
> >> +	/*
> >> +	 * Changes struct fb_var_screeninfo are currently not pushed back
> >> +	 * to KMS, hence fail if different settings are requested.
> >> +	 */
> >> +	if (var->bits_per_pixel != fb->bits_per_pixel ||
> >> +	    var->xres != fb->width || var->yres != fb->height ||
> >> +	    var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
> > 
> > This still looks somewhat incomplete. Sounds like we should just
> > reject changes to everything except xoffset/yoffset.
> > 
> 
> What other parameters would you check? struct fb_var_screeninfo also has
> timings, but that is not something which is handy right here...

We should check the timings I think since we don't allow chaning them.
Though I suppose we don't even populate them. But then the user
shouldn't be allowed try to set them either I guess.

> 
> One parameter we could check too is the depth calculated from the bit
> information, e.g.
> 
> 
> 	switch (var->bits_per_pixel) {
> 	case 16:
> 		depth = (var->green.length == 6) ? 16 : 15;
> 		break;
> 	case 32:
> 		depth = (var->transp.length > 0) ? 32 : 24;
> 		break;
> 	default:
> 		depth = var->bits_per_pixel;
> 		break;
> 	}
> +
> +	if (depth != fb->depth) {
> +		DRM_DEBUG("fb userspace requested depth different than current fb "
> + 			  "request %d vs. %dx\n", depth, fb->depth);
> +		return -EINVAL;
> +	}
> 
> Or, maybe better, we could use drm_mode_legacy_fb_format to convert
> bpp/depth to fourcc and check that against the pixel_format field...

Also the red/green/... definitions for the actual bits of the color
channels. .grayscale is there well, and some colorspace thing which
I don't even know what it does. And rotation...

So I think in the end it pretty much boils down to everything except
xoffset/yoffset ;)

> 
> --
> Stefan
> 
> 
> >> +		DRM_DEBUG("fb userspace requested width/height/bpp different than 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,
> >> --
> >> 2.10.0
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC