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.
> 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
