On Fri, 2018-12-28 at 13:15 +0100, Daniel Vetter wrote:
> On Fri, Dec 28, 2018 at 04:13:07AM +0500, Ivan Mironov wrote:
> > SDL 1.2 sets all fields related to the pixel format to zero in some
> > cases[1]. Prior to commit db05c48197759 ("drm: fb-helper: Reject all
> > pixel format changing requests"), there was an unintentional workaround
> > for this that existed for more than a decade. First in device-specific DRM
> > drivers, then here in drm_fb_helper.c.
> >
> > Previous code containing this workaround just ignores pixel format fields
> > from userspace code. Not a good thing either, as this way, driver may
> > silently use pixel format different from what client actually requested,
> > and this in turn will lead to displaying garbage on the screen. I think
> > that returning EINVAL to userspace in this particular case is the right
> > option, so I decided to left code from problematic commit untouched
> > instead of just reverting it entirely.
> >
> > Here is the steps required to reproduce this problem exactly:
> > 1) Compile fceux[2] with SDL 1.2.15 and without GTK or OpenGL
> > support. SDL should be compiled with fbdev support (which is
> > on by default).
> > 2) Create /etc/fb.modes with following contents (values seems
> > not used, and just required to trigger problematic code in
> > SDL):
> >
> > mode "test"
> > geometry 1 1 1 1 1
> > timings 1 1 1 1 1 1 1
> > endmode
> >
> > 3) Create ~/.fceux/fceux.cfg with following contents:
> >
> > SDL.Hotkeys.Quit = 27
> > SDL.DoubleBuffering = 1
> >
> > 4) Ensure that screen resolution is at least 1280x960 (e.g.
> > append "video=Virtual-1:1280x960-32" to the kernel cmdline
> > for qemu/QXL).
> >
> > 5) Try to run fceux on VT with some ROM file[3]:
> >
> > # ./fceux color_test.nes
> >
> > [1] SDL 1.2.15 source code, src/video/fbcon/SDL_fbvideo.c,
> > FB_SetVideoMode()
> > [2] http://www.fceux.com
> > [3] Example ROM:
> > https://github.com/bokuweb/rustynes/blob/master/roms/color_test.nes
> >
> > Reported-by: saahriktu <m...@saahriktu.org>
> > Suggested-by: saahriktu <m...@saahriktu.org>
> > Cc: sta...@vger.kernel.org
> > Fixes: db05c48197759 ("drm: fb-helper: Reject all pixel format changing
> > requests")
> > Signed-off-by: Ivan Mironov <mironov.i...@gmail.com>
> > ---
> > drivers/gpu/drm/drm_fb_helper.c | 146 ++++++++++++++++++++------------
> > 1 file changed, 93 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > b/drivers/gpu/drm/drm_fb_helper.c
> > index d3af098b0922..aff576c3c4fb 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1621,6 +1621,64 @@ static bool drm_fb_pixel_format_equal(const struct
> > fb_var_screeninfo *var_1,
> > var_1->transp.msb_right == var_2->transp.msb_right;
> > }
> >
> > +static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
> > + u8 depth)
> > +{
> > + switch (depth) {
> > + case 8:
> > + var->red.offset = 0;
> > + var->green.offset = 0;
> > + var->blue.offset = 0;
> > + var->red.length = 8; /* 8bit DAC */
> > + var->green.length = 8;
> > + var->blue.length = 8;
> > + var->transp.offset = 0;
> > + var->transp.length = 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.offset = 15;
> > + var->transp.length = 1;
> > + 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.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.offset = 0;
> > + var->transp.length = 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.offset = 24;
> > + var->transp.length = 8;
> > + break;
> > + default:
> > + break;
> > + }
> > +}
> > +
> > /**
> > * drm_fb_helper_check_var - implementation for &fb_ops.fb_check_var
> > * @var: screeninfo to check
> > @@ -1654,6 +1712,40 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo
> > *var,
> > return -EINVAL;
> > }
> >
> > + /*
> > + * Workaround for SDL 1.2, which is known to be setting all pixel format
> > + * fields values to zero in some cases. We treat this situation as a
> > + * kind of "use some reasonable autodetected values".
> > + */
> > + if (!var->red.offset && !var->green.offset &&
> > + !var->blue.offset && !var->transp.offset &&
> > + !var->red.length && !var->green.length &&
> > + !var->blue.length && !var->transp.length &&
> > + !var->red.msb_right && !var->green.msb_right &&
> > + !var->blue.msb_right && !var->transp.msb_right) {
> > + u8 depth;
> > +
> > + /*
> > + * There is no way to guess the right value for depth when
> > + * bpp is 16 or 32. So we just restore the behaviour previously
> > + * introduced here by commit . In fact, this was
> > + * implemented even earlier in various device drivers.
> > + */
> > + switch (var->bits_per_pixel) {
> > + case 16:
> > + depth = 15;
> > + break;
> > + case 32:
> > + depth = 24;
> > + break;
> > + default:
> > + depth = var->bits_per_pixel;
> > + break;
> > + }
> > +
> > + drm_fb_helper_fill_pixel_fmt(var, depth);
>
> Please use fb->format->depth here instead of guessing.
> -Daniel
I do not think that this is the right way.
Without guessing, if SDL1 makes a request with bits_per_pixel == 16
(for example) and current fb->format->depth == 24, ioctl() will succeed
while actual pixel format will remain the same. As a result, SDL1 will
display garbage because of invalid pixel format.
Or am I missing something here?
> > + }
> > +
> > /*
> > * drm fbdev emulation doesn't support changing the pixel format at all,
> > * so reject all pixel format changing requests.oid
> > drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> > info->var.yoffset = 0; info->var.activate = FB_ACTIVATE_NOW;
> > - switch (fb->format->depth) {
> > - case 8:
> > - info->var.red.offset = 0;
> > - info->var.green.offset = 0;
> > - info->var.blue.offset = 0;
> > - info->var.red.length = 8; /* 8bit DAC */
> > - info->var.green.length = 8;
> > - info->var.blue.length = 8;
> > - info->var.transp.offset = 0;
> > - info->var.transp.length = 0;
> > - break;
> > - case 15:
> > - info->var.red.offset = 10;
> > - info->var.green.offset = 5;
> > - info->var.blue.offset = 0;
> > - info->var.red.length = 5;
> > - info->var.green.length = 5;
> > - info->var.blue.length = 5;
> > - info->var.transp.offset = 15;
> > - info->var.transp.length = 1;
> > - break;
> > - case 16:
> > - info->var.red.offset = 11;
> > - info->var.green.offset = 5;
> > - info->var.blue.offset = 0;
> > - info->var.red.length = 5;
> > - info->var.green.length = 6;
> > - info->var.blue.length = 5;
> > - info->var.transp.offset = 0;
> > - break;
> > - case 24:
> > - info->var.red.offset = 16;
> > - info->var.green.offset = 8;
> > - info->var.blue.offset = 0;
> > - info->var.red.length = 8;
> > - info->var.green.length = 8;
> > - info->var.blue.length = 8;
> > - info->var.transp.offset = 0;
> > - info->var.transp.length = 0;
> > - break;
> > - case 32:
> > - info->var.red.offset = 16;
> > - info->var.green.offset = 8;
> > - info->var.blue.offset = 0;
> > - info->var.red.length = 8;
> > - info->var.green.length = 8;
> > - info->var.blue.length = 8;
> > - info->var.transp.offset = 24;
> > - info->var.transp.length = 8;
> > - break;
> > - default:
> > - break;
> > - }
> > + drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth);
> >
> > info->var.xres = fb_width;
> > info->var.yres = fb_height;
> > --
> > 2.20.1
> >
