On Tue, Jan 08, 2019 at 12:23:52PM +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 | 142 ++++++++++++++++++++------------
> 1 file changed, 89 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d3af098b0922..ed7e91423258 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,36 @@ 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) {
> + /*
> + * There is no way to guess the right value for depth when
> + * bits_per_pixel is 16 or 32. Instead of restoring the
> + * behaviour previously introduced here by commit 785b93ef8c309,
> + * we decided to just use the current depth and do not perform
> + * any guessing.
> + *
> + * Also, if requested bits_per_pixel differs from current,
> + * the next call to drm_fb_pixel_format_equal() will fail
> + * resulting in EINVAL error from ioctl().
> + *
> + * However, this still leaves the theoretical possibility of
> + * situation when userspace app requests different pixel format
> + * and ioctl() call silently succeeds. Garbage will be displayed
> + * in this case.
> + */
I deleted this comment since I think it's misleading: We only need depth
to decided for the pixel format when userspace hasn't filled it out. We
could also change the function to instead pass fb->format, and use that as
the key to fill out the fbdev format information. That's why I requested
you keep Ville's change here, it was a bugfix.
So ther's no guessing going on here at all, and nothing else fancy: All we
do is fill out the fbdev pixel format information if userspace failed to
supply it. That's imo clear enough without a comment.
Applied both comments for 5.0 drm-misc-fixes, thanks a lot for your
patches and debug work.
Cheers, Daniel
> + drm_fb_helper_fill_pixel_fmt(var, fb->format->depth);
> + }
> +
> /*
> * drm fbdev emulation doesn't support changing the pixel format at all,
> * so reject all pixel format changing requests.
> @@ -1967,59 +2055,7 @@ void 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
