Re: [RFC PATCH 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
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. > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [RFC PATCH 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
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.
Re: [RFC PATCH 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
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 > Suggested-by: saahriktu > Fixes: db05c48197759 ("drm: fb-helper: Reject all pixel format changing > requests") > Signed-off-by: Ivan Mironov > --- > 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: > +
[RFC PATCH 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
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 Suggested-by: saahriktu Fixes: db05c48197759 ("drm: fb-helper: Reject all pixel format changing requests") Signed-off-by: Ivan Mironov --- 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; +