[PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Michel Dänzer]

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

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

Re: [PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Daniel Stone]

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

Re: [PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Michel Dänzer]

On 16/03/17 07:09 PM, Ville Syrjälä wrote:
> 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.

Agreed on all counts. So, I think what's needed is almost a revert of
865afb11949e, except for the bits_per_pixel comparison, since we really
can't change that. See diff below.

Does that make sense? Stefan, would this break any test case you wrote
your patch for?


diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f6d4d9700734..f4f70ce24fcc 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1260,8 +1260,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
         * 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) {
+           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,



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


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Ville Syrjälä]

On Fri, Mar 17, 2017 at 06:01:41PM +0900, Michel Dänzer wrote:
> On 16/03/17 07:09 PM, Ville Syrjälä wrote:
> > 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.
> 
> Agreed on all counts. So, I think what's needed is almost a revert of
> 865afb11949e, except for the bits_per_pixel comparison, since we really
> can't change that. See diff below.

So one semi-crazy idea I had was:
12:18 < vsyrjala> daniels: hmm. given that the fb_helper doesn't
 implement a modeset in set_par, i guess what we really
 should do is look at the currently active crtcs and see if the mode on
 one of them more or less matches what the user is asking for

I tried to trip this current check by booting with a big screen and
later switching to a small screen. For some reason that worked out
just fine, so I'm not even sure what kind of values we stuff into
xres & co.

> 
> Does that make sense? Stefan, would this break any test case you wrote
> your patch for?
> 
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index f6d4d9700734..f4f70ce24fcc 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1260,8 +1260,8 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>          * 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) {
> +           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,
> 
> 
> 
> >> 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
> > 
> 
> 
> -- 
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Michel Dänzer]

On 17/03/17 07:01 PM, Ville Syrjälä wrote:
> On Fri, Mar 17, 2017 at 06:01:41PM +0900, Michel Dänzer wrote:
>> On 16/03/17 07:09 PM, Ville Syrjälä wrote:
>>> 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.
>>
>> Agreed on all counts. So, I think what's needed is almost a revert of
>> 865afb11949e, except for the bits_per_pixel comparison, since we really
>> can't change that. See diff below.
> 
> So one semi-crazy idea I had was:
> 12:18 < vsyrjala> daniels: hmm. given that the fb_helper doesn't
>  implement a modeset in set_par, i guess what we really
>  should do is look at the currently active crtcs and see if the mode on
>  one of them more or less matches what the user is asking for

I don't get the idea. What's the point of comparing the var->* values to
whatever is currently active in the hardware? The purpose of the code is
to check that the requested parameters fit into fb_helper's framebuffer.


> I tried to trip this current check by booting with a big screen and
> later switching to a small screen. For some reason that worked out
> just fine,

I'd need more details about what exactly you tried.

> so I'm not even sure what kind of values we stuff into xres & co.

It should be the values shown / attempted to set by fbset.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Ville Syrjälä]

On Fri, Mar 17, 2017 at 07:19:27PM +0900, Michel Dänzer wrote:
> On 17/03/17 07:01 PM, Ville Syrjälä wrote:
> > On Fri, Mar 17, 2017 at 06:01:41PM +0900, Michel Dänzer wrote:
> >> On 16/03/17 07:09 PM, Ville Syrjälä wrote:
> >>> 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.
> >>
> >> Agreed on all counts. So, I think what's needed is almost a revert of
> >> 865afb11949e, except for the bits_per_pixel comparison, since we really
> >> can't change that. See diff below.
> > 
> > So one semi-crazy idea I had was:
> > 12:18 < vsyrjala> daniels: hmm. given that the fb_helper doesn't
> >  implement a modeset in set_par, i guess what we really
> >  should do is look at the currently active crtcs and see if the mode on
> >  one of them more or less matches what the user is asking for
> 
> I don't get the idea. What's the point of comparing the var->* values to
> whatever is currently active in the hardware?

Not currently active in the hardware, but currently active as far as
fb_helper is concerned. I guess I should say "matches fb_helper's
current crtc configuration" or something.

> The purpose of the code is
> to check that the requested parameters fit into fb_helper's framebuffer.
> 
> 
> > I tried to trip this current check by booting with a big screen and
> > later switching to a small screen. For some reason that worked out
> > just fine,
> 
> I'd need more details about what exactly you tried.
> 
> > so I'm not even sure what kind of values we stuff into xres & co.
> 
> It should be the values shown / attempted to set by fbset.

I meant what does the kernel put there for fbcon etc. I was expecting
that it xres/yres would be the visible resolution of the smallest
screen, and the virtual resolution would be either the same or the
size of the fb perhaps. But that can't be the case or else my experiment
would have produced a failure. So I would have to dump that stuff out to
see what really ends up there.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Michel Dänzer]

On 17/03/17 08:13 PM, Ville Syrjälä wrote:
> On Fri, Mar 17, 2017 at 07:19:27PM +0900, Michel Dänzer wrote:
>> On 17/03/17 07:01 PM, Ville Syrjälä wrote:
>>> On Fri, Mar 17, 2017 at 06:01:41PM +0900, Michel Dänzer wrote:
>>>> On 16/03/17 07:09 PM, Ville Syrjälä wrote:
>>>>> 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.
>>>>
>>>> Agreed on all counts. So, I think what's needed is almost a revert of
>>>> 865afb11949e, except for the bits_per_pixel comparison, since we really
>>>> can't change that. See diff below.
>>>
>>> So one semi-crazy idea I had was:
>>> 12:18 < vsyrjala> daniels: hmm. given that the fb_helper doesn't
>>>  implement a modeset in set_par, i guess what we really
>>>  should do is look at the currently active crtcs and see if the mode on
>>>  one of them more or less matches what the user is asking for
>>
>> I don't get the idea. What's the point of comparing the var->* values to
>> whatever is currently active in the hardware?
> 
> Not currently active in the hardware, but currently active as far as
> fb_helper is concerned. I guess I should say "matches fb_helper's
> current crtc configuration" or something.

I see, thanks for clarifying.

Something like that could work, but it might still be artificially
limiting. Thinking through all the ramifications is too involved for me
right now.

Anyway, I don't think we can do anything fancier than the quasi-revert
for 4.10 or even 4.11.


>>> I tried to trip this current check by booting with a big screen and
>>> later switching to a small screen. For some reason that worked out
>>> just fine,
>>
>> I'd need more details about what exactly you tried.
>>
>>> so I'm not even sure what kind of values we stuff into xres & co.
>>
>> It should be the values shown / attempted to set by fbset.
> 
> I meant what does the kernel put there for fbcon etc. I was expecting
> that it xres/yres would be the visible resolution of the smallest
> screen, and the virtual resolution would be either the same or the
> size of the fb perhaps. But that can't be the case or else my experiment
> would have produced a failure. So I would have to dump that stuff out to
> see what really ends up there.

I'd also assume something like what you describe when fbdev emulation is
initialized, but I'm not sure there's any code to update any of that
stuff when hotplugging displays.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

[PATCH] drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

Otherwise this can also prevent modesets e.g. for switching VTs, when
multiple monitors with different native resolutions are connected.

The depths must match though, so keep the != test for that.

Also update the DRM_DEBUG output to be slightly more accurate, this
doesn't only affect requests from userspace.

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>
---
 drivers/gpu/drm/drm_fb_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f6d4d9700734..324a688b3f30 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1260,9 +1260,9 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	 * 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) {
-		DRM_DEBUG("fb userspace requested width/height/bpp different than current fb "
+	    var->xres > fb->width || var->yres > fb->height ||
+	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
+		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
 			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
 			  var->xres, var->yres, var->bits_per_pixel,
 			  var->xres_virtual, var->yres_virtual,
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again

[Daniel Stone]

Hi Michel,

On 23 March 2017 at 08:53, Michel Dänzer <michel@daenzer.net> wrote:
> Otherwise this can also prevent modesets e.g. for switching VTs, when
> multiple monitors with different native resolutions are connected.
>
> The depths must match though, so keep the != test for that.
>
> Also update the DRM_DEBUG output to be slightly more accurate, this
> doesn't only affect requests from userspace.

This test looks much more sensible, and also fixes VT switching for me.

Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel

Re: [PATCH] drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again

[Daniel Vetter]

On Thu, Mar 23, 2017 at 12:01:30PM +0000, Daniel Stone wrote:
> Hi Michel,
> 
> On 23 March 2017 at 08:53, Michel Dänzer <michel@daenzer.net> wrote:
> > Otherwise this can also prevent modesets e.g. for switching VTs, when
> > multiple monitors with different native resolutions are connected.
> >
> > The depths must match though, so keep the != test for that.
> >
> > Also update the DRM_DEBUG output to be slightly more accurate, this
> > doesn't only affect requests from userspace.
> 
> This test looks much more sensible, and also fixes VT switching for me.
> 
> Reviewed-by: Daniel Stone <daniels@collabora.com>

Applied to drm-misc-fixes, will send a pull request for it asap.

Thanks, Daniel
-- 
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

Re: [PATCH] drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again

[Stefan Agner]

On 2017-03-23 01:53, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> Otherwise this can also prevent modesets e.g. for switching VTs, when
> multiple monitors with different native resolutions are connected.

When changing resolution a regular fbdev driver also changes display
timing accordingly (fb_var_screeninfo contains timings as well). The
fbdev emulation layer does not change timing, so that was the idea why
my patch also rejected any kind of change to the resolution... In the
end it is really a definition of the semantics of that userspace API
(FBIOPUT_VSCREENINFO) and how true DRM emulates them...

Using less of the available framebuffer seems safe, especially since
line width is handled by pitch/line_length.

As far as I remember my case was a fbdev application which fiddled with
bpp/depth, which lead to a totally messed up outcome... So my use case
will still be fine with your change.

I am ok with relaxing that again, so from my side:
Acked-by: Stefan Agner <stefan@agner.ch>

--
Stefan


> 
> The depths must match though, so keep the != test for that.
> 
> Also update the DRM_DEBUG output to be slightly more accurate, this
> doesn't only affect requests from userspace.
> 
> 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>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index f6d4d9700734..324a688b3f30 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1260,9 +1260,9 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  	 * 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) {
> -		DRM_DEBUG("fb userspace requested width/height/bpp different than
> current fb "
> +	    var->xres > fb->width || var->yres > fb->height ||
> +	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
> +		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
>  			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
>  			  var->xres, var->yres, var->bits_per_pixel,
>  			  var->xres_virtual, var->yres_virtual,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel