Re: [PATCH v1 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
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.
Re: [PATCH v1 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
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
> > > Suggested-by: saahriktu
> > > Cc: sta...@vger.kernel.org
> > > 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;
> > >
Re: [PATCH v1 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
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
> > Suggested-by: saahriktu
> > Cc: sta...@vger.kernel.org
> > 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 @@ in
Re: [PATCH v1 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
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
> Suggested-by: saahriktu
> Cc: sta...@vger.kernel.org
> 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;
> +
Re: [PATCH v1 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
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
> Suggested-by: saahriktu
> Cc: sta...@vger.kernel.org
> 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 b
