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

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 mail@saahriktu.org Suggested-by: saahriktu mail@saahriktu.org Cc: stable@vger.kernel.org Fixes: db05c48197759 ("drm: fb-helper: Reject all pixel format changing requests") Signed-off-by: Ivan Mironov mironov.ivan@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