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

[PATCH v1 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.

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.

First patch in this series fixes a clear regression. Second patch is
optional, please read commit message carefully before applying it.

Changes in v1:
- Added "Cc: stable" to the patch which fixes known regression.
- Added more information and detailed reproduction steps in commit
  messages.

Changes in v0:
- RFC patch series introduced.

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

[PATCH v1 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.

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 <mail@saahriktu.org>
Suggested-by: saahriktu <mail@saahriktu.org>
Cc: stable@vger.kernel.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

[PATCH v1 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.

However, while enabling SDL1 applications to work out of the box when
there is no /etc/fb.modes with valid settings, this change affects the
video mode choosing logic in SDL. Depending on current screen
resolution, contents of /etc/fb.modes and resolution requested by
application, this may lead to user-visible difference (not always):
image will be displayed in a right way, but it will be aligned to the
left instead of center. There is no "right behaviour" here as well, as
emulated fbdev, opposing to old fbdev drivers, simply ignores any
requsts of video mode changes with resolutions smaller than current.

Feel free to NAK this patch if you think that it causes breakage of
user-space =).

The easiest way to reproduce this problem is to install sdl-sopwith[2],
remove /etc/fb.modes file if it exists, and then try to run sopwith
from console without X. At least in Fedora 29, sopwith may be simply
installed from standard repositories.

[1] SDL 1.2.15 source code, src/video/fbcon/SDL_fbvideo.c, vesa_timings
[2] http://sdl-sopwith.sourceforge.net/

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: [PATCH v1 2/2] drm/fb-helper: Ignore the value of fb_var_screeninfo.pixclock

[Daniel Vetter]

On Fri, Dec 28, 2018 at 04:13:08AM +0500, Ivan Mironov wrote:
> 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.
> 
> However, while enabling SDL1 applications to work out of the box when
> there is no /etc/fb.modes with valid settings, this change affects the
> video mode choosing logic in SDL. Depending on current screen
> resolution, contents of /etc/fb.modes and resolution requested by
> application, this may lead to user-visible difference (not always):
> image will be displayed in a right way, but it will be aligned to the
> left instead of center. There is no "right behaviour" here as well, as
> emulated fbdev, opposing to old fbdev drivers, simply ignores any
> requsts of video mode changes with resolutions smaller than current.
> 
> Feel free to NAK this patch if you think that it causes breakage of
> user-space =).

It's a regression, we don't nack regression fixes :-)

> The easiest way to reproduce this problem is to install sdl-sopwith[2],
> remove /etc/fb.modes file if it exists, and then try to run sopwith
> from console without X. At least in Fedora 29, sopwith may be simply
> installed from standard repositories.
> 
> [1] SDL 1.2.15 source code, src/video/fbcon/SDL_fbvideo.c, vesa_timings
> [2] http://sdl-sopwith.sourceforge.net/
> 
> Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>

I thought this is also a regression fix, so also needs Fixes: and cc:
stable?
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

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 <mail@saahriktu.org>
> Suggested-by: saahriktu <mail@saahriktu.org>
> Cc: stable@vger.kernel.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);

Please use fb->format->depth here instead of guessing.
-Daniel
> +	}
> +
>  	/*
>  	 * 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: [PATCH v1 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2

[Eugeniy Paltsev]

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 <mail@saahriktu.org>
> Suggested-by: saahriktu <mail@saahriktu.org>
> Cc: stable@vger.kernel.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
> 
-- 
 Eugeniy Paltsev

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

[Ivan Mironov]

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 <mail@saahriktu.org>
> > Suggested-by: saahriktu <mail@saahriktu.org>
> > Cc: stable@vger.kernel.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 . 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?

> > +	}
> > +
> >  	/*
> >  	 * drm fbdev emulation doesn't support changing the pixel format at all,
> >  	 * so reject all pixel format changing requests.oid 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
> > 

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

[Ivan Mironov]

On Fri, 2018-12-28 at 13:06 +0100, Daniel Vetter wrote:
> On Fri, Dec 28, 2018 at 04:13:08AM +0500, Ivan Mironov wrote:
> > 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.
> > 
> > However, while enabling SDL1 applications to work out of the box when
> > there is no /etc/fb.modes with valid settings, this change affects the
> > video mode choosing logic in SDL. Depending on current screen
> > resolution, contents of /etc/fb.modes and resolution requested by
> > application, this may lead to user-visible difference (not always):
> > image will be displayed in a right way, but it will be aligned to the
> > left instead of center. There is no "right behaviour" here as well, as
> > emulated fbdev, opposing to old fbdev drivers, simply ignores any
> > requsts of video mode changes with resolutions smaller than current.
> > 
> > Feel free to NAK this patch if you think that it causes breakage of
> > user-space =).
> 
> It's a regression, we don't nack regression fixes :-)
> 
> > The easiest way to reproduce this problem is to install sdl-sopwith[2],
> > remove /etc/fb.modes file if it exists, and then try to run sopwith
> > from console without X. At least in Fedora 29, sopwith may be simply
> > installed from standard repositories.
> > 
> > [1] SDL 1.2.15 source code, src/video/fbcon/SDL_fbvideo.c, vesa_timings
> > [2] http://sdl-sopwith.sourceforge.net/
> > 
> > Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
> 
> I thought this is also a regression fix, so also needs Fixes: and cc:
> stable?
> -Daniel

This particular patch fixes a bug that existed for 10 years unnoticed.
Are "Fixes:" and "Cc: stable" really required?

"pixclock != 0" check was introduced in this file by 785b93ef8c309
("drm/kms: move driver specific fb common code to helper functions
(v2)"). But actually similar code existed in the device-specific
drivers even earlier.

> 
> > ---
> >  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: [PATCH v1 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2

[Daniel Vetter]

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 <mail@saahriktu.org>
> > > Suggested-by: saahriktu <mail@saahriktu.org>
> > > Cc: stable@vger.kernel.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 . 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

> 
> > > +	}
> > > +
> > >  	/*
> > >  	 * drm fbdev emulation doesn't support changing the pixel format at all,
> > >  	 * so reject all pixel format changing requests.oid 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: [PATCH v1 2/2] drm/fb-helper: Ignore the value of fb_var_screeninfo.pixclock

[Daniel Vetter]

On Sat, Jan 05, 2019 at 09:21:21PM +0500, Ivan Mironov wrote:
> On Fri, 2018-12-28 at 13:06 +0100, Daniel Vetter wrote:
> > On Fri, Dec 28, 2018 at 04:13:08AM +0500, Ivan Mironov wrote:
> > > 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.
> > > 
> > > However, while enabling SDL1 applications to work out of the box when
> > > there is no /etc/fb.modes with valid settings, this change affects the
> > > video mode choosing logic in SDL. Depending on current screen
> > > resolution, contents of /etc/fb.modes and resolution requested by
> > > application, this may lead to user-visible difference (not always):
> > > image will be displayed in a right way, but it will be aligned to the
> > > left instead of center. There is no "right behaviour" here as well, as
> > > emulated fbdev, opposing to old fbdev drivers, simply ignores any
> > > requsts of video mode changes with resolutions smaller than current.
> > > 
> > > Feel free to NAK this patch if you think that it causes breakage of
> > > user-space =).
> > 
> > It's a regression, we don't nack regression fixes :-)
> > 
> > > The easiest way to reproduce this problem is to install sdl-sopwith[2],
> > > remove /etc/fb.modes file if it exists, and then try to run sopwith
> > > from console without X. At least in Fedora 29, sopwith may be simply
> > > installed from standard repositories.
> > > 
> > > [1] SDL 1.2.15 source code, src/video/fbcon/SDL_fbvideo.c, vesa_timings
> > > [2] http://sdl-sopwith.sourceforge.net/
> > > 
> > > Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
> > 
> > I thought this is also a regression fix, so also needs Fixes: and cc:
> > stable?
> > -Daniel
> 
> This particular patch fixes a bug that existed for 10 years unnoticed.
> Are "Fixes:" and "Cc: stable" really required?
> 
> "pixclock != 0" check was introduced in this file by 785b93ef8c309
> ("drm/kms: move driver specific fb common code to helper functions
> (v2)"). But actually similar code existed in the device-specific
> drivers even earlier.

Afaui 785b93ef8c309 broke existing userspace. Broken userspace, but
existing userspace, and hence we need to fix this regression. That's the
rules with linux upstream, existing userspace is never allowed to break,
even if it does really stupid things. Hence Cc: stable.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Greg KH]

On Sat, Jan 05, 2019 at 09:21:21PM +0500, Ivan Mironov wrote:
> On Fri, 2018-12-28 at 13:06 +0100, Daniel Vetter wrote:
> > On Fri, Dec 28, 2018 at 04:13:08AM +0500, Ivan Mironov wrote:
> > > 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.
> > > 
> > > However, while enabling SDL1 applications to work out of the box when
> > > there is no /etc/fb.modes with valid settings, this change affects the
> > > video mode choosing logic in SDL. Depending on current screen
> > > resolution, contents of /etc/fb.modes and resolution requested by
> > > application, this may lead to user-visible difference (not always):
> > > image will be displayed in a right way, but it will be aligned to the
> > > left instead of center. There is no "right behaviour" here as well, as
> > > emulated fbdev, opposing to old fbdev drivers, simply ignores any
> > > requsts of video mode changes with resolutions smaller than current.
> > > 
> > > Feel free to NAK this patch if you think that it causes breakage of
> > > user-space =).
> > 
> > It's a regression, we don't nack regression fixes :-)
> > 
> > > The easiest way to reproduce this problem is to install sdl-sopwith[2],
> > > remove /etc/fb.modes file if it exists, and then try to run sopwith
> > > from console without X. At least in Fedora 29, sopwith may be simply
> > > installed from standard repositories.
> > > 
> > > [1] SDL 1.2.15 source code, src/video/fbcon/SDL_fbvideo.c, vesa_timings
> > > [2] http://sdl-sopwith.sourceforge.net/
> > > 
> > > Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
> > 
> > I thought this is also a regression fix, so also needs Fixes: and cc:
> > stable?
> > -Daniel
> 
> This particular patch fixes a bug that existed for 10 years unnoticed.
> Are "Fixes:" and "Cc: stable" really required?

If you want it backported into a stable kernel release, then yes, they
are needed :)

thanks,

greg k-h

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

[Ivan Mironov]

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.