| Message ID | 20181227231308.16904-2-mironov.ivan@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix SDL 1.2 on emulated fbdev devices (broken in kernels >=4.19) | expand |
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 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); Please use fb->format->depth here instead of guessing. -Daniel > + } > + > /* > * 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 >
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@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 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 >
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 > >
On Sat, Jan 05, 2019 at 09:11:53PM +0500, Ivan Mironov wrote: > 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? This is supposed to be the case where SDL didn't set any of this stuff. Guess is definitely not what we want to do, we should use the actual depth, which is stored in fb->format->depth. Older code did guess, but we fixed that, and shouldn't reintroduce that guess game. -Daniel > > > > + } > > > + > > > /* > > > * 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 > > > > > >
On Mon, 2019-01-07 at 11:08 +0100, Daniel Vetter wrote: > > > > @@ -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? > > This is supposed to be the case where SDL didn't set any of this stuff. > Guess is definitely not what we want to do, we should use the actual > depth, which is stored in fb->format->depth. Older code did guess, but we > fixed that, and shouldn't reintroduce that guess game. > -Daniel > Done. See the v2 patch series.
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;
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(-)