Re: [PATCH v1 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2

From: Eugeniy Paltsev
Date: Fri Dec 28 2018 - 12:28:31 EST

On Fri, 2018-12-28 at 04:13 +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.

Yep, reverting commit db05c48197759 ("drm: fb-helper: Reject all pixel
format changing requests") isn't a good idea as it will break Weston with
fbdev-backend where we get exactly described situation - we request one
pixel format but kernel successfully and silently set another one. So we
get picturesque garbage on the screen :)

> 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] https://urldefense.proofpoint.com/v2/url?u=http-3A__www.fceux.com&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=ZlJN1MriPUTkBKCrPSx67GmaplEUGcAEk9yPtCLdUX
> I&m=qIoApAq-LY8cjTow82_lYWwm4L8HiOYnLp_E4AziAxo&s=fM_yxMF6T5-RyXKlbbff_S62k_opHxlolqNPXV0RPa4&e=
> [3] Example ROM: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_bokuweb_rustynes_blob_master_roms_color-5Ftest.nes&d=DwIDAg&c=DPL6_
> X_6JkXFx7AXWqB0tg&r=ZlJN1MriPUTkBKCrPSx67GmaplEUGcAEk9yPtCLdUXI&m=qIoApAq-
> LY8cjTow82_lYWwm4L8HiOYnLp_E4AziAxo&s=4gNM8PW1yNqinYiWW9lGjj3ik0Kmo40XXQYLl0UcEHc&e=
>
> Reported-by: saahriktu <mail@xxxxxxxxxxxxx>
> Suggested-by: saahriktu <mail@xxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: db05c48197759 ("drm: fb-helper: Reject all pixel format changing requests")
> Signed-off-by: Ivan Mironov <mironov.ivan@xxxxxxxxx>
> ---
> 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 785b93ef8c309. 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);
> + }
> +
> /*
> * drm fbdev emulation doesn't support changing the pixel format at all,
> * so reject all pixel format changing requests.
> @@ -1967,59 +2059,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
>
--
Eugeniy Paltsev