drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set
diff mbox

Message ID 20170316095553.1586-1-michel@daenzer.net
State New
Headers show

Commit Message

Michel Dänzer March 16, 2017, 9:55 a.m. UTC
From: Michel Dänzer <michel.daenzer@amd.com>

Otherwise this can also prevent modesets e.g. for switching VTs.

FB_MISC_USER_EVENT is set when the request originates from userspace,
which is what we're interested in here according to the DRM_DEBUG
output.

Bugzilla: https://bugs.freedesktop.org/99841
Fixes: 865afb11949e ("drm/fb-helper: reject any changes to the fbdev")
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---

I'm not entirely sure why the values can not match for a VT switch. If
anybody thinks this just papers over the real issue, please speak up.

 drivers/gpu/drm/drm_fb_helper.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Ville Syrjälä March 16, 2017, 10:09 a.m. UTC | #1
On Thu, Mar 16, 2017 at 06:55:53PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> Otherwise this can also prevent modesets e.g. for switching VTs.
> 
> FB_MISC_USER_EVENT is set when the request originates from userspace,
> which is what we're interested in here according to the DRM_DEBUG
> output.

So why is the kernel allowed to violate this?

The checks look somewhat bogus to me anyway. The 'virtual size == fb size'
check makes some kind of sense. Although I don't see why the virtual
resolution couldn't be smaller than the fb size. But requiring that the
visible resolutionn matches the fb size as well definitely seems wrong.

Maybe Cc whoever broke this?

> 
> Bugzilla: https://bugs.freedesktop.org/99841
> Fixes: 865afb11949e ("drm/fb-helper: reject any changes to the fbdev")
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
> 
> I'm not entirely sure why the values can not match for a VT switch. If
> anybody thinks this just papers over the real issue, please speak up.
> 
>  drivers/gpu/drm/drm_fb_helper.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index f6d4d9700734..9663f3b8f287 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1259,9 +1259,10 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  	 * Changes struct fb_var_screeninfo are currently not pushed back
>  	 * to KMS, hence fail if different settings are requested.
>  	 */
> -	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
> -	    var->xres != fb->width || var->yres != fb->height ||
> -	    var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
> +	if ((info->flags & FBINFO_MISC_USEREVENT) &&
> +	    (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
> +	     var->xres != fb->width || var->yres != fb->height ||
> +	     var->xres_virtual != fb->width || var->yres_virtual != fb->height)) {
>  		DRM_DEBUG("fb userspace requested width/height/bpp different than current fb "
>  			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
>  			  var->xres, var->yres, var->bits_per_pixel,
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Stone March 16, 2017, 10:09 a.m. UTC | #2
Hi,

On 16 March 2017 at 09:55, Michel Dänzer <michel@daenzer.net> wrote:
> Otherwise this can also prevent modesets e.g. for switching VTs.
>
> FB_MISC_USER_EVENT is set when the request originates from userspace,
> which is what we're interested in here according to the DRM_DEBUG
> output.
>
> Bugzilla: https://bugs.freedesktop.org/99841
> Fixes: 865afb11949e ("drm/fb-helper: reject any changes to the fbdev")
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>
> I'm not entirely sure why the values can not match for a VT switch. If
> anybody thinks this just papers over the real issue, please speak up.

It happens for me in multi-head with different resolutions. A real
compositor will set native resolutions with separate framebuffers, and
then fbcon will try to set one buffer for both outputs. This works on
the output with the larger resolution, but the one with the smaller
resolution will fail due to [xy]res_virtual (IIRC) being different.

I'll test this out later today.

Cheers,
Daniel
Michel Dänzer March 17, 2017, 8:54 a.m. UTC | #3
On 16/03/17 07:09 PM, Daniel Stone wrote:
> On 16 March 2017 at 09:55, Michel Dänzer <michel@daenzer.net> wrote:
>> Otherwise this can also prevent modesets e.g. for switching VTs.
>>
>> FB_MISC_USER_EVENT is set when the request originates from userspace,
>> which is what we're interested in here according to the DRM_DEBUG
>> output.
>>
>> Bugzilla: https://bugs.freedesktop.org/99841
>> Fixes: 865afb11949e ("drm/fb-helper: reject any changes to the fbdev")
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>> ---
>>
>> I'm not entirely sure why the values can not match for a VT switch. If
>> anybody thinks this just papers over the real issue, please speak up.
> 
> It happens for me in multi-head with different resolutions. A real
> compositor will set native resolutions with separate framebuffers, and
> then fbcon will try to set one buffer for both outputs. This works on
> the output with the larger resolution, but the one with the smaller
> resolution will fail due to [xy]res_virtual (IIRC) being different.

That's more or less the line of thinking that lead me to writing this
patch, based on the assumption that the fb->* values correspond to what
was set by whatever we're VT switching from. However, it occurred to me
that it's an invalid assumption; fb here is always fb_helper's
framebuffer for fbdev. I think Ville is right that the tests are bogus
in the first place.

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f6d4d9700734..9663f3b8f287 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1259,9 +1259,10 @@  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	 * Changes struct fb_var_screeninfo are currently not pushed back
 	 * to KMS, hence fail if different settings are requested.
 	 */
-	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
-	    var->xres != fb->width || var->yres != fb->height ||
-	    var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
+	if ((info->flags & FBINFO_MISC_USEREVENT) &&
+	    (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
+	     var->xres != fb->width || var->yres != fb->height ||
+	     var->xres_virtual != fb->width || var->yres_virtual != fb->height)) {
 		DRM_DEBUG("fb userspace requested width/height/bpp different than current fb "
 			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
 			  var->xres, var->yres, var->bits_per_pixel,