Re: [PATCH v2] drm: fb-helper: Reject all pixel format changing requests

[Ville Syrjälä]

On Thu, Oct 04, 2018 at 12:49:38PM +0200, Daniel Vetter wrote:
> On Thu, Oct 04, 2018 at 01:34:22PM +0300, Ville Syrjälä wrote:
> > On Wed, Oct 03, 2018 at 07:45:38PM +0300, Eugeniy Paltsev wrote:
> > > drm fbdev emulation doesn't support changing the pixel format at all,
> > > so reject all pixel format changing requests.
> > > 
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@xxxxxxxxxxxx>
> > > ---
> > > Changes v1->v2:
> > >  * Reject all pixel format changing request, not just the invalid ones.
> > > 
> > >  drivers/gpu/drm/drm_fb_helper.c | 91 ++++++++++++-----------------------------
> > >  1 file changed, 26 insertions(+), 65 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 16ec93b75dbf..48598d7f673f 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -1580,6 +1580,25 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
> > >  }
> > >  EXPORT_SYMBOL(drm_fb_helper_ioctl);
> > >  
> > > +static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1,
> > > +				      const struct fb_var_screeninfo *var_2)
> > > +{
> > > +	return var_1->bits_per_pixel == var_2->bits_per_pixel &&
> > > +	       var_1->grayscale == var_2->grayscale &&
> > > +	       var_1->red.offset == var_2->red.offset &&
> > > +	       var_1->red.length == var_2->red.length &&
> > > +	       var_1->red.msb_right == var_2->red.msb_right &&
> > > +	       var_1->green.offset == var_2->green.offset &&
> > > +	       var_1->green.length == var_2->green.length &&
> > > +	       var_1->green.msb_right == var_2->green.msb_right &&
> > > +	       var_1->blue.offset == var_2->blue.offset &&
> > > +	       var_1->blue.length == var_2->blue.length &&
> > > +	       var_1->blue.msb_right == var_2->blue.msb_right &&
> > > +	       var_1->transp.offset == var_2->transp.offset &&
> > > +	       var_1->transp.length == var_2->transp.length &&
> > > +	       var_1->transp.msb_right == var_2->transp.msb_right;
> > > +}
> > 
> > Could have shortened a bit with memcmp() but this works too.
> 
> I'm always vary of memcmp with structs that might have padding and stuff.

These are uabi so padding would be rather bad.

> 
> > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > 
> > I suppose we really should be doing the same for most of the rest of
> > var_screeninfo as we don't allow changing the timings/etc. either.
> 
> Timing at least doens't have an immediate correctness impact of userspace
> rendering the wrong format :-)
> 
> Thanks for review&patch, applied to drm-misc-fixes.
> -Daniel
> 
> > 
> > > +
> > >  /**
> > >   * drm_fb_helper_check_var - implementation for &fb_ops.fb_check_var
> > >   * @var: screeninfo to check
> > > @@ -1590,7 +1609,6 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > >  {
> > >  	struct drm_fb_helper *fb_helper = info->par;
> > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > > -	int depth;
> > >  
> > >  	if (var->pixclock != 0 || in_dbg_master())
> > >  		return -EINVAL;
> > > @@ -1610,72 +1628,15 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	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;
> > > -	}
> > > -
> > > -	switch (depth) {
> > > -	case 8:
> > > -		var->red.offset = 0;
> > > -		var->green.offset = 0;
> > > -		var->blue.offset = 0;
> > > -		var->red.length = 8;
> > > -		var->green.length = 8;
> > > -		var->blue.length = 8;
> > > -		var->transp.length = 0;
> > > -		var->transp.offset = 0;
> > > -		break;
> > > -	case 15:
> > > -		var->red.offset = 10;
> > > -		var->green.offset = 5;
> > > -		var->blue.offset = 0;
> > > -		var->red.length = 5;
> > > -		var->green.length = 5;
> > > -		var->blue.length = 5;
> > > -		var->transp.length = 1;
> > > -		var->transp.offset = 15;
> > > -		break;
> > > -	case 16:
> > > -		var->red.offset = 11;
> > > -		var->green.offset = 5;
> > > -		var->blue.offset = 0;
> > > -		var->red.length = 5;
> > > -		var->green.length = 6;
> > > -		var->blue.length = 5;
> > > -		var->transp.length = 0;
> > > -		var->transp.offset = 0;
> > > -		break;
> > > -	case 24:
> > > -		var->red.offset = 16;
> > > -		var->green.offset = 8;
> > > -		var->blue.offset = 0;
> > > -		var->red.length = 8;
> > > -		var->green.length = 8;
> > > -		var->blue.length = 8;
> > > -		var->transp.length = 0;
> > > -		var->transp.offset = 0;
> > > -		break;
> > > -	case 32:
> > > -		var->red.offset = 16;
> > > -		var->green.offset = 8;
> > > -		var->blue.offset = 0;
> > > -		var->red.length = 8;
> > > -		var->green.length = 8;
> > > -		var->blue.length = 8;
> > > -		var->transp.length = 8;
> > > -		var->transp.offset = 24;
> > > -		break;
> > > -	default:
> > > +	/*
> > > +	 * drm fbdev emulation doesn't support changing the pixel format at all,
> > > +	 * so reject all pixel format changing requests.
> > > +	 */
> > > +	if (!drm_fb_pixel_format_equal(var, &info->var)) {
> > > +		DRM_DEBUG("fbdev emulation doesn't support changing the pixel format\n");
> > >  		return -EINVAL;
> > >  	}
> > > +
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL(drm_fb_helper_check_var);
> > > -- 
> > > 2.14.4
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel