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 _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:
> +
