[RFC PATCH 0/2] Fix SDL 1.2 on emulated fbdev devices (broken in kernels >=4.19)

[RFC PATCH 0/2] Fix SDL 1.2 on emulated fbdev devices (broken in kernels >=4.19)

[Ivan Mironov]

Hi,

Originally this issue was brought up on linux.org.ru forum by user
saahriktu, he is on Cc. He discovered that commit db05c48197759
("drm: fb-helper: Reject all pixel format changing requests") breaks
support of SDL1 programs, like various old games and emulators of old
game consoles. First patch contains fix for that commit.

I tried to reproduce the same issue in a VM under qemu, and found yet
another part of kernel code which prevents SDL1 apps from running
normally. Second patch in this series fixes this problem.

However, I still have few unresolved questions regarding this patches:
* Why simple revert of commit db05c48197759 fixes SDL1 support for
  saahriktu? I do not understand how this could work when SDL1 has
  hardcoded non-zero pixclock values, for which current code always
  returns EINVAL.
* How this worked before? How this is working for non-DRM fbdev drivers?
  fbdev support code in SDL1 is also very old by the way.

Also, it seems that at least in some cases both problems could be
circumvented by adding appropriate modes into /etc/fb.modes. But without
examining the kernel code it is not clear which values are correct. I am
not sure that such circumvention covers all possible cases, and it is
definitely far from any user-frienliness.

Ah, almost forgot this: the easiest way to reproduce both problems is to
install sdl-sopwith[1] and try to run it from console without X. At
least in Fedora 29, sopwith may be simply installed from standard
repositories.

[1] http://sdl-sopwith.sourceforge.net/

Ivan Mironov (2):
  drm/fb-helper: Bring back workaround for bugs of SDL 1.2
  drm/fb-helper: Ignore the value of fb_var_screeninfo.pixclock

 drivers/gpu/drm/drm_fb_helper.c | 153 +++++++++++++++++++++-----------
 1 file changed, 99 insertions(+), 54 deletions(-)

-- 
2.20.1

[RFC PATCH 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2

[Ivan Mironov]

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;
+		}
+
+		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

[RFC PATCH 2/2] drm/fb-helper: Ignore the value of fb_var_screeninfo.pixclock

[Ivan Mironov]

Strict requirement of pixclock to be zero breaks support of SDL 1.2
which contains hardcoded table of supported video modes with non-zero
pixclock values[1].

To better understand which pixclock values are considered valid and how
driver should handle these values, I briefly examined few existing fbdev
drivers and documentation in Documentation/fb/. And it looks like there
are no strict rules on that and actual behaviour varies:

	* some drivers treat (pixclock == 0) as "use defaults" (uvesafb.c);
	* some treat (pixclock == 0) as invalid value which leads to
	  -EINVAL (clps711x-fb.c);
	* some pass converted pixclock value to hardware (uvesafb.c);
	* some are trying to find nearest value from predefined table
          (vga16fb.c, video_gx.c).

Given this, I believe that it should be safe to just ignore this value if
changing is not supported. It seems that any portable fbdev application
which was not written only for one specific device working under one
specific kernel version should not rely on any particular behaviour of
pixclock anyway.

[1] SDL 1.2.15 source code, src/video/fbcon/SDL_fbvideo.c, vesa_timings

Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index aff576c3c4fb..b95a0c23c7c8 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1690,9 +1690,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_framebuffer *fb = fb_helper->fb;
 
-	if (var->pixclock != 0 || in_dbg_master())
+	if (in_dbg_master())
 		return -EINVAL;
 
+	if (var->pixclock != 0) {
+		DRM_DEBUG("fbdev emulation doesn't support changing the pixel clock, value of pixclock is ignored\n");
+		var->pixclock = 0;
+	}
+
 	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
 	    (drm_format_info_block_height(fb->format, 0) > 1))
 		return -EINVAL;
-- 
2.20.1

Re: [RFC PATCH 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2

[Daniel Vetter]

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
> 

-- 
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

[Ivan Mironov]

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

[Daniel Vetter]

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