[RFC] drm/fb-helper: reject any changes to the fbdev

[RFC] drm/fb-helper: reject any changes to the fbdev

[Stefan Agner]

The current fbdev emulation does not allow to push back changes in
width, height or depth to KMS, hence reject any changes with an
error. This makes sure that fbdev ioctl's fail properly and user
space does not assume that changes succeeded.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
This rejects reconfiguration of framebuffer like
fbset -rgba 5,6,5 -depth 16 (when in 24 bit mode by default)
fbset -xres 123

I think all users of drm_fb_helper_check_var use also the generic
drm_fb_helper_set_par (or do otherwise not support changing size/
depth). Hence, afaict, the change should be the right thing to do
for all driver...

 drivers/gpu/drm/drm_fb_helper.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 03414bd..596c056 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1211,11 +1211,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	if (var->pixclock != 0 || in_dbg_master())
 		return -EINVAL;
 
-	/* Need to resize the fb object !!! */
-	if (var->bits_per_pixel > fb->bits_per_pixel ||
-	    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 is greater than current fb "
+	/*
+	 * 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->bits_per_pixel ||
+	    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,
 			  var->xres_virtual, var->yres_virtual,
-- 
2.10.0

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

Re: [RFC] drm/fb-helper: reject any changes to the fbdev

[Daniel Vetter]

On Tue, Oct 11, 2016 at 04:15:04PM -0700, Stefan Agner wrote:
> The current fbdev emulation does not allow to push back changes in
> width, height or depth to KMS, hence reject any changes with an
> error. This makes sure that fbdev ioctl's fail properly and user
> space does not assume that changes succeeded.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>

Make sense to me, but would be great to have an r-b from someone who has
some clue about fbdev ... Tomi, Laurent?
-Daniel

> ---
> This rejects reconfiguration of framebuffer like
> fbset -rgba 5,6,5 -depth 16 (when in 24 bit mode by default)
> fbset -xres 123
> 
> I think all users of drm_fb_helper_check_var use also the generic
> drm_fb_helper_set_par (or do otherwise not support changing size/
> depth). Hence, afaict, the change should be the right thing to do
> for all driver...
> 
>  drivers/gpu/drm/drm_fb_helper.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 03414bd..596c056 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1211,11 +1211,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  	if (var->pixclock != 0 || in_dbg_master())
>  		return -EINVAL;
>  
> -	/* Need to resize the fb object !!! */
> -	if (var->bits_per_pixel > fb->bits_per_pixel ||
> -	    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 is greater than current fb "
> +	/*
> +	 * 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->bits_per_pixel ||
> +	    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,
>  			  var->xres_virtual, var->yres_virtual,
> -- 
> 2.10.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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: [RFC] drm/fb-helper: reject any changes to the fbdev

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 2413 bytes --]

On 12/10/16 02:15, Stefan Agner wrote:
> The current fbdev emulation does not allow to push back changes in
> width, height or depth to KMS, hence reject any changes with an
> error. This makes sure that fbdev ioctl's fail properly and user
> space does not assume that changes succeeded.

The description is a bit confusing. Don't you mean that currently the
fbdev emulation does allow changes, but doesn't actually push those
changes to KMS? And this patch makes the driver reject those changes.

The change itself sounds good to me.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> This rejects reconfiguration of framebuffer like
> fbset -rgba 5,6,5 -depth 16 (when in 24 bit mode by default)
> fbset -xres 123
> 
> I think all users of drm_fb_helper_check_var use also the generic
> drm_fb_helper_set_par (or do otherwise not support changing size/
> depth). Hence, afaict, the change should be the right thing to do
> for all driver...
> 
>  drivers/gpu/drm/drm_fb_helper.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 03414bd..596c056 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1211,11 +1211,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  	if (var->pixclock != 0 || in_dbg_master())
>  		return -EINVAL;
>  
> -	/* Need to resize the fb object !!! */
> -	if (var->bits_per_pixel > fb->bits_per_pixel ||
> -	    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 is greater than current fb "
> +	/*
> +	 * 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->bits_per_pixel ||
> +	    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,
>  			  var->xres_virtual, var->yres_virtual,
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [RFC] drm/fb-helper: reject any changes to the fbdev

[Daniel Vetter]

On Wed, Oct 12, 2016 at 12:37:07PM +0300, Tomi Valkeinen wrote:
> On 12/10/16 02:15, Stefan Agner wrote:
> > The current fbdev emulation does not allow to push back changes in
> > width, height or depth to KMS, hence reject any changes with an
> > error. This makes sure that fbdev ioctl's fail properly and user
> > space does not assume that changes succeeded.
> 
> The description is a bit confusing. Don't you mean that currently the
> fbdev emulation does allow changes, but doesn't actually push those
> changes to KMS? And this patch makes the driver reject those changes.
> 
> The change itself sounds good to me.
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Applied to drm-misc, thanks.
-Daniel

> 
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > ---
> > This rejects reconfiguration of framebuffer like
> > fbset -rgba 5,6,5 -depth 16 (when in 24 bit mode by default)
> > fbset -xres 123
> > 
> > I think all users of drm_fb_helper_check_var use also the generic
> > drm_fb_helper_set_par (or do otherwise not support changing size/
> > depth). Hence, afaict, the change should be the right thing to do
> > for all driver...
> > 
> >  drivers/gpu/drm/drm_fb_helper.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 03414bd..596c056 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1211,11 +1211,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >  	if (var->pixclock != 0 || in_dbg_master())
> >  		return -EINVAL;
> >  
> > -	/* Need to resize the fb object !!! */
> > -	if (var->bits_per_pixel > fb->bits_per_pixel ||
> > -	    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 is greater than current fb "
> > +	/*
> > +	 * 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->bits_per_pixel ||
> > +	    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,
> >  			  var->xres_virtual, var->yres_virtual,
> > 
> 




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


-- 
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: [RFC] drm/fb-helper: reject any changes to the fbdev

[Ville Syrjälä]

On Tue, Oct 11, 2016 at 04:15:04PM -0700, Stefan Agner wrote:
> The current fbdev emulation does not allow to push back changes in
> width, height or depth to KMS, hence reject any changes with an
> error. This makes sure that fbdev ioctl's fail properly and user
> space does not assume that changes succeeded.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> This rejects reconfiguration of framebuffer like
> fbset -rgba 5,6,5 -depth 16 (when in 24 bit mode by default)
> fbset -xres 123
> 
> I think all users of drm_fb_helper_check_var use also the generic
> drm_fb_helper_set_par (or do otherwise not support changing size/
> depth). Hence, afaict, the change should be the right thing to do
> for all driver...
> 
>  drivers/gpu/drm/drm_fb_helper.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 03414bd..596c056 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1211,11 +1211,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  	if (var->pixclock != 0 || in_dbg_master())
>  		return -EINVAL;
>  
> -	/* Need to resize the fb object !!! */
> -	if (var->bits_per_pixel > fb->bits_per_pixel ||
> -	    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 is greater than current fb "
> +	/*
> +	 * 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->bits_per_pixel ||
> +	    var->xres != fb->width || var->yres != fb->height ||
> +	    var->xres_virtual != fb->width || var->yres_virtual != fb->height) {

This still looks somewhat incomplete. Sounds like we should just
reject changes to everything except xoffset/yoffset.

> +		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,
>  			  var->xres_virtual, var->yres_virtual,
> -- 
> 2.10.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: [RFC] drm/fb-helper: reject any changes to the fbdev

[Stefan Agner]

On 2016-10-12 03:42, Ville Syrjälä wrote:
> On Tue, Oct 11, 2016 at 04:15:04PM -0700, Stefan Agner wrote:
>> The current fbdev emulation does not allow to push back changes in
>> width, height or depth to KMS, hence reject any changes with an
>> error. This makes sure that fbdev ioctl's fail properly and user
>> space does not assume that changes succeeded.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>> This rejects reconfiguration of framebuffer like
>> fbset -rgba 5,6,5 -depth 16 (when in 24 bit mode by default)
>> fbset -xres 123
>>
>> I think all users of drm_fb_helper_check_var use also the generic
>> drm_fb_helper_set_par (or do otherwise not support changing size/
>> depth). Hence, afaict, the change should be the right thing to do
>> for all driver...
>>
>>  drivers/gpu/drm/drm_fb_helper.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 03414bd..596c056 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -1211,11 +1211,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>>  	if (var->pixclock != 0 || in_dbg_master())
>>  		return -EINVAL;
>>
>> -	/* Need to resize the fb object !!! */
>> -	if (var->bits_per_pixel > fb->bits_per_pixel ||
>> -	    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 is greater than current fb "
>> +	/*
>> +	 * 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->bits_per_pixel ||
>> +	    var->xres != fb->width || var->yres != fb->height ||
>> +	    var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
> 
> This still looks somewhat incomplete. Sounds like we should just
> reject changes to everything except xoffset/yoffset.
> 

What other parameters would you check? struct fb_var_screeninfo also has
timings, but that is not something which is handy right here...

One parameter we could check too is the depth calculated from the bit
information, e.g.


	switch (var->bits_per_pixel) {
	case 16:
		depth = (var->green.length == 6) ? 16 : 15;
		break;
	case 32:
		depth = (var->transp.length > 0) ? 32 : 24;
		break;
	default:
		depth = var->bits_per_pixel;
		break;
	}
+
+	if (depth != fb->depth) {
+		DRM_DEBUG("fb userspace requested depth different than current fb "
+ 			  "request %d vs. %dx\n", depth, fb->depth);
+		return -EINVAL;
+	}

Or, maybe better, we could use drm_mode_legacy_fb_format to convert
bpp/depth to fourcc and check that against the pixel_format field...

--
Stefan


>> +		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,
>>  			  var->xres_virtual, var->yres_virtual,
>> --
>> 2.10.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm/fb-helper: reject any changes to the fbdev

[Ville Syrjälä]

On Wed, Oct 12, 2016 at 08:55:45AM -0700, Stefan Agner wrote:
> On 2016-10-12 03:42, Ville Syrjälä wrote:
> > On Tue, Oct 11, 2016 at 04:15:04PM -0700, Stefan Agner wrote:
> >> The current fbdev emulation does not allow to push back changes in
> >> width, height or depth to KMS, hence reject any changes with an
> >> error. This makes sure that fbdev ioctl's fail properly and user
> >> space does not assume that changes succeeded.
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >> This rejects reconfiguration of framebuffer like
> >> fbset -rgba 5,6,5 -depth 16 (when in 24 bit mode by default)
> >> fbset -xres 123
> >>
> >> I think all users of drm_fb_helper_check_var use also the generic
> >> drm_fb_helper_set_par (or do otherwise not support changing size/
> >> depth). Hence, afaict, the change should be the right thing to do
> >> for all driver...
> >>
> >>  drivers/gpu/drm/drm_fb_helper.c | 13 ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >> index 03414bd..596c056 100644
> >> --- a/drivers/gpu/drm/drm_fb_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >> @@ -1211,11 +1211,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >>  	if (var->pixclock != 0 || in_dbg_master())
> >>  		return -EINVAL;
> >>
> >> -	/* Need to resize the fb object !!! */
> >> -	if (var->bits_per_pixel > fb->bits_per_pixel ||
> >> -	    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 is greater than current fb "
> >> +	/*
> >> +	 * 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->bits_per_pixel ||
> >> +	    var->xres != fb->width || var->yres != fb->height ||
> >> +	    var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
> > 
> > This still looks somewhat incomplete. Sounds like we should just
> > reject changes to everything except xoffset/yoffset.
> > 
> 
> What other parameters would you check? struct fb_var_screeninfo also has
> timings, but that is not something which is handy right here...

We should check the timings I think since we don't allow chaning them.
Though I suppose we don't even populate them. But then the user
shouldn't be allowed try to set them either I guess.

> 
> One parameter we could check too is the depth calculated from the bit
> information, e.g.
> 
> 
> 	switch (var->bits_per_pixel) {
> 	case 16:
> 		depth = (var->green.length == 6) ? 16 : 15;
> 		break;
> 	case 32:
> 		depth = (var->transp.length > 0) ? 32 : 24;
> 		break;
> 	default:
> 		depth = var->bits_per_pixel;
> 		break;
> 	}
> +
> +	if (depth != fb->depth) {
> +		DRM_DEBUG("fb userspace requested depth different than current fb "
> + 			  "request %d vs. %dx\n", depth, fb->depth);
> +		return -EINVAL;
> +	}
> 
> Or, maybe better, we could use drm_mode_legacy_fb_format to convert
> bpp/depth to fourcc and check that against the pixel_format field...

Also the red/green/... definitions for the actual bits of the color
channels. .grayscale is there well, and some colorspace thing which
I don't even know what it does. And rotation...

So I think in the end it pretty much boils down to everything except
xoffset/yoffset ;)

> 
> --
> Stefan
> 
> 
> >> +		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,
> >>  			  var->xres_virtual, var->yres_virtual,
> >> --
> >> 2.10.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: [RFC] drm/fb-helper: reject any changes to the fbdev

[Stefan Agner]

On 2016-10-12 09:12, Ville Syrjälä wrote:
> On Wed, Oct 12, 2016 at 08:55:45AM -0700, Stefan Agner wrote:
>> On 2016-10-12 03:42, Ville Syrjälä wrote:
>> > On Tue, Oct 11, 2016 at 04:15:04PM -0700, Stefan Agner wrote:
>> >> The current fbdev emulation does not allow to push back changes in
>> >> width, height or depth to KMS, hence reject any changes with an
>> >> error. This makes sure that fbdev ioctl's fail properly and user
>> >> space does not assume that changes succeeded.
>> >>
>> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >> ---
>> >> This rejects reconfiguration of framebuffer like
>> >> fbset -rgba 5,6,5 -depth 16 (when in 24 bit mode by default)
>> >> fbset -xres 123
>> >>
>> >> I think all users of drm_fb_helper_check_var use also the generic
>> >> drm_fb_helper_set_par (or do otherwise not support changing size/
>> >> depth). Hence, afaict, the change should be the right thing to do
>> >> for all driver...
>> >>
>> >>  drivers/gpu/drm/drm_fb_helper.c | 13 ++++++++-----
>> >>  1 file changed, 8 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> >> index 03414bd..596c056 100644
>> >> --- a/drivers/gpu/drm/drm_fb_helper.c
>> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> >> @@ -1211,11 +1211,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>> >>  	if (var->pixclock != 0 || in_dbg_master())
>> >>  		return -EINVAL;
>> >>
>> >> -	/* Need to resize the fb object !!! */
>> >> -	if (var->bits_per_pixel > fb->bits_per_pixel ||
>> >> -	    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 is greater than current fb "
>> >> +	/*
>> >> +	 * 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->bits_per_pixel ||
>> >> +	    var->xres != fb->width || var->yres != fb->height ||
>> >> +	    var->xres_virtual != fb->width || var->yres_virtual != fb->height) {
>> >
>> > This still looks somewhat incomplete. Sounds like we should just
>> > reject changes to everything except xoffset/yoffset.
>> >
>>
>> What other parameters would you check? struct fb_var_screeninfo also has
>> timings, but that is not something which is handy right here...
> 
> We should check the timings I think since we don't allow chaning them.
> Though I suppose we don't even populate them. But then the user
> shouldn't be allowed try to set them either I guess.
> 

Ultimately we should do that agreed.

However, the framebuffer settings are a bigger problem currently: It is
not just that we silently not apply settings, but because we don't error
out on check_var the fbcon subsystem assumes the changes have been
applied to the display controller and redraws the console with the new
format... Timings do not affect the drawing of fbcon, hence this is less
of a problem. I probably should have mentioned that in the commit log
too.

The callback is also used for initial configuration (which I guess is
some kind of struct fb_var_screeninfo populated by the KMS subsystem?).
Checking more parameters increases the risk that we wrongfully reject
initial configurations which would left people with a broken fbdev
emulation.

>>
>> One parameter we could check too is the depth calculated from the bit
>> information, e.g.
>>
>>
>> 	switch (var->bits_per_pixel) {
>> 	case 16:
>> 		depth = (var->green.length == 6) ? 16 : 15;
>> 		break;
>> 	case 32:
>> 		depth = (var->transp.length > 0) ? 32 : 24;
>> 		break;
>> 	default:
>> 		depth = var->bits_per_pixel;
>> 		break;
>> 	}
>> +
>> +	if (depth != fb->depth) {
>> +		DRM_DEBUG("fb userspace requested depth different than current fb "
>> + 			  "request %d vs. %dx\n", depth, fb->depth);
>> +		return -EINVAL;
>> +	}
>>
>> Or, maybe better, we could use drm_mode_legacy_fb_format to convert
>> bpp/depth to fourcc and check that against the pixel_format field...
> 
> Also the red/green/... definitions for the actual bits of the color
> channels. .grayscale is there well, and some colorspace thing which
> I don't even know what it does. And rotation...

We can only easily check what is in the intersection of struct
drm_framebuffer and struct fb_var_screeninfo.

Not sure how the color bit definitions actually map to DRM/KMS stuff, I
guess just through the pixel_format field?

Rotation is something which is per plane, which again is not easily
accessible.

I don't disagree, we should check all this. But we haven't checked any
of this so far, and I would rather start with a small subset (e.g. only
check the stuff easily convertible and available in struct
drm_framebuffer).

--
Stefan

> 
> So I think in the end it pretty much boils down to everything except
> xoffset/yoffset ;)
> 
>>
>> --
>> Stefan
>>
>>
>> >> +		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,
>> >>  			  var->xres_virtual, var->yres_virtual,
>> >> --
>> >> 2.10.0
>> >>
>> >> _______________________________________________
>> >> dri-devel mailing list
>> >> dri-devel@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel