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 | 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. + */ + 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;
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:
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).
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
Create ~/.fceux/fceux.cfg with following contents:
SDL.Hotkeys.Quit = 27 SDL.DoubleBuffering = 1
Ensure that screen resolution is at least 1280x960 (e.g. append "video=Virtual-1:1280x960-32" to the kernel cmdline for qemu/QXL).
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 | 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
linux-stable-mirror@lists.linaro.org