From: Daniel Vetter <daniel@ffwll.ch>
To: Ivan Mironov <mironov.ivan@gmail.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
saahriktu <mail@saahriktu.org>, David Airlie <airlied@linux.ie>,
Sean Paul <sean@poorly.run>
Subject: Re: [RFC PATCH 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
Date: Thu, 27 Dec 2018 13:00:46 +0100 [thread overview]
Message-ID: <20181227120046.GC9058@dvetter-linux.ger.corp.intel.com> (raw)
In-Reply-To: <20181226121124.1632-2-mironov.ivan@gmail.com>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch>
To: Ivan Mironov <mironov.ivan@gmail.com>
Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Maxime Ripard <maxime.ripard@bootlin.com>,
Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
Daniel Vetter <daniel@ffwll.ch>, saahriktu <mail@saahriktu.org>,
Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
Subject: Re: [RFC PATCH 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2
Date: Thu, 27 Dec 2018 13:00:46 +0100 [thread overview]
Message-ID: <20181227120046.GC9058@dvetter-linux.ger.corp.intel.com> (raw)
In-Reply-To: <20181226121124.1632-2-mironov.ivan@gmail.com>
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
next prev parent reply other threads:[~2018-12-27 12:00 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-26 12:11 [RFC PATCH 0/2] Fix SDL 1.2 on emulated fbdev devices (broken in kernels >=4.19) Ivan Mironov
2018-12-26 12:11 ` [RFC PATCH 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2 Ivan Mironov
2018-12-27 12:00 ` Daniel Vetter [this message]
2018-12-27 12:00 ` Daniel Vetter
2018-12-27 23:26 ` Ivan Mironov
2018-12-28 12:02 ` Daniel Vetter
2018-12-26 12:11 ` [RFC PATCH 2/2] drm/fb-helper: Ignore the value of fb_var_screeninfo.pixclock Ivan Mironov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181227120046.GC9058@dvetter-linux.ger.corp.intel.com \
--to=daniel@ffwll.ch \
--cc=Eugeniy.Paltsev@synopsys.com \
--cc=airlied@linux.ie \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mail@saahriktu.org \
--cc=maxime.ripard@bootlin.com \
--cc=mironov.ivan@gmail.com \
--cc=sean@poorly.run \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.