Date:   Tue, 8 Jan 2019 09:17:49 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Ivan Mironov <mironov.ivan@...il.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        saahriktu <mail@...hriktu.org>,
        Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>,
        stable@...r.kernel.org
Subject: Re: [PATCH v2 1/2] drm/fb-helper: Partially bring back workaround
 for bugs of SDL 1.2

On Tue, Jan 08, 2019 at 12:23:52PM +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@...hriktu.org>
> Suggested-by: saahriktu <mail@...hriktu.org>
> Cc: stable@...r.kernel.org
> Fixes: db05c48197759 ("drm: fb-helper: Reject all pixel format changing requests")
> Signed-off-by: Ivan Mironov <mironov.ivan@...il.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 142 ++++++++++++++++++++------------
>  1 file changed, 89 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d3af098b0922..ed7e91423258 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,36 @@ 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) {
> +		/*
> +		 * There is no way to guess the right value for depth when
> +		 * bits_per_pixel is 16 or 32. Instead of restoring the
> +		 * behaviour previously introduced here by commit 785b93ef8c309,
> +		 * we decided to just use the current depth and do not perform
> +		 * any guessing.
> +		 *
> +		 * Also, if requested bits_per_pixel differs from current,
> +		 * the next call to drm_fb_pixel_format_equal() will fail
> +		 * resulting in EINVAL error from ioctl().
> +		 *
> +		 * However, this still leaves the theoretical possibility of
> +		 * situation when userspace app requests different pixel format
> +		 * and ioctl() call silently succeeds. Garbage will be displayed
> +		 * in this case.
> +		 */

I deleted this comment since I think it's misleading: We only need depth
to decided for the pixel format when userspace hasn't filled it out. We
could also change the function to instead pass fb->format, and use that as
the key to fill out the fbdev format information. That's why I requested
you keep Ville's change here, it was a bugfix.

So ther's no guessing going on here at all, and nothing else fancy: All we
do is fill out the fbdev pixel format information if userspace failed to
supply it. That's imo clear enough without a comment.

Applied both comments for 5.0 drm-misc-fixes, thanks a lot for your
patches and debug work.

Cheers, Daniel

> +		drm_fb_helper_fill_pixel_fmt(var, fb->format->depth);
> +	}
> +
>  	/*
>  	 * drm fbdev emulation doesn't support changing the pixel format at all,
>  	 * so reject all pixel format changing requests.
> @@ -1967,59 +2055,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

Powered by blists - more mailing lists