[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 at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel