| Message ID | 20181226121124.1632-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 Wed, Dec 26, 2018 at 05:11:23PM +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. > > [1] SDL 1.2.15 source code, src/video/fbcon/SDL_fbvideo.c, > FB_SetVideoMode() > > Reported-by: saahriktu <mail@saahriktu.org> > Suggested-by: saahriktu <mail@saahriktu.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; > + } The guesswork here looks fishy. We should still have the drm-side format, and should use that. Otherwise the patches look good I think, but they need a Fixes: tag and cc: stable so backporters know what to do with these. -Daniel > + > + 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 Thu, 2018-12-27 at 13:00 +0100, Daniel Vetter wrote: > > + /* > > + * 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; > > + } > > The guesswork here looks fishy. We should still have the drm-side format, > and should use that. This existed for a very long time until problematic commit was applied. And there is a clear evidence that it was actually used by applications. > Otherwise the patches look good I think, but they > need a Fixes: tag and cc: stable so backporters know what to do with > these. > I added "cc: stable" into the regression fix. Also added more info into the commit messages. See the PATCH v1 in the mailing list. Thanks.
On Fri, Dec 28, 2018 at 04:26:56AM +0500, Ivan Mironov wrote: > On Thu, 2018-12-27 at 13:00 +0100, Daniel Vetter wrote: > > > + /* > > > + * 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; > > > + } > > > > The guesswork here looks fishy. We should still have the drm-side format, > > and should use that. > > This existed for a very long time until problematic commit was applied. > And there is a clear evidence that it was actually used by > applications. I'm not against guessing this stuff, but we know have much better format handling code than when this code was originally written. I just want to use that (like it's used everywhere else in this file now). fb->format gives you the right depth, no guessing needed at all. And if you guess wrong here, you'll fail in these format checks later on. -Daniel > > > Otherwise the patches look good I think, but they > > need a Fixes: tag and cc: stable so backporters know what to do with > > these. > > > > I added "cc: stable" into the regression fix. Also added more info into > the commit messages. See the PATCH v1 in the mailing list. > > Thanks. > > >
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. [1] SDL 1.2.15 source code, src/video/fbcon/SDL_fbvideo.c, FB_SetVideoMode() Reported-by: saahriktu <mail@saahriktu.org> Suggested-by: saahriktu <mail@saahriktu.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(-)