[PATCH] staging: vboxvideo: Fix reporting invalid suggested-offset-properties

[Michael Thayer]

Hello Hans,

12.10.2017 20:10, Hans de Goede wrote:
> The x and y hints receives from the host are unsigned 32 bit integers and
> they get set to -1 (0xffffffff) when invalid. Before this commit the
> vboxvideo driver was storing them in an u16 causing the -1 to be truncated
> to 65535 which, once reported to userspace, was breaking gnome 3.26+
> in Wayland mode.
> 
> This commit stores the host values in 32 bit variables, removing the
> truncation and checks for -1, replacing it with 0 as -1 is not a valid
> suggested-offset-property value. Likewise the properties are now
> initialized to 0 instead of -1, since -1 is not a valid value.
> This fixes gnome 3.26+ in Wayland mode not working with the vboxvideo
> driver.

Thank you, looks sensible.  Minor quibble: wouldn't it be nicer to
compare the unsigned values vbox_connector->vbox_crtc->x_hint and y_hint
against ~0 below, rather than -1 (see inline below)?  Other than that,
looks good to me.  I will send Gianfranco a patch to test against
Ubuntu's version of the driver.

Regards
Michael

> Reported-by: Gianfranco Costamagna <locutusofborg at debian.org>
> Cc: stable at vger.kernel.org
> Cc: Michael Thayer <michael.thayer at oracle.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---пишет
>  drivers/staging/vboxvideo/vbox_drv.h  |  8 ++++----
>  drivers/staging/vboxvideo/vbox_irq.c  |  4 ++--
>  drivers/staging/vboxvideo/vbox_mode.c | 26 ++++++++++++++++++--------
>  3 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
> index 4b9302703b36..eeac4f0cb2c6 100644
> --- a/drivers/staging/vboxvideo/vbox_drv.h
> +++ b/drivers/staging/vboxvideo/vbox_drv.h
> @@ -137,8 +137,8 @@ struct vbox_connector {
>  	char name[32];
>  	struct vbox_crtc *vbox_crtc;
>  	struct {
> -		u16 width;
> -		u16 height;
> +		u32 width;
> +		u32 height;
>  		bool disconnected;
>  	} mode_hint;
>  };
> @@ -150,8 +150,8 @@ struct vbox_crtc {
>  	unsigned int crtc_id;
>  	u32 fb_offset;
>  	bool cursor_enabled;
> -	u16 x_hint;
> -	u16 y_hint;
> +	u32 x_hint;
> +	u32 y_hint;
>  };
>  
>  struct vbox_encoder {
> diff --git a/drivers/staging/vboxvideo/vbox_irq.c b/drivers/staging/vboxvideo/vbox_irq.c
> index 3ca8bec62ac4..74abdf02d9fd 100644
> --- a/drivers/staging/vboxvideo/vbox_irq.c
> +++ b/drivers/staging/vboxvideo/vbox_irq.c
> @@ -150,8 +150,8 @@ static void vbox_update_mode_hints(struct vbox_private *vbox)
>  
>  		disconnected = !(hints->enabled);
>  		crtc_id = vbox_conn->vbox_crtc->crtc_id;
> -		vbox_conn->mode_hint.width = hints->cx & 0x8fff;
> -		vbox_conn->mode_hint.height = hints->cy & 0x8fff;
> +		vbox_conn->mode_hint.width = hints->cx;
> +		vbox_conn->mode_hint.height = hints->cy;
>  		vbox_conn->vbox_crtc->x_hint = hints->dx;
>  		vbox_conn->vbox_crtc->y_hint = hints->dy;
>  		vbox_conn->mode_hint.disconnected = disconnected;
> diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
> index 257a77830410..6f08dc966719 100644
> --- a/drivers/staging/vboxvideo/vbox_mode.c
> +++ b/drivers/staging/vboxvideo/vbox_mode.c
> @@ -553,12 +553,22 @@ static int vbox_get_modes(struct drm_connector *connector)
>  		++num_modes;
>  	}
>  	vbox_set_edid(connector, preferred_width, preferred_height);
> -	drm_object_property_set_value(
> -		&connector->base, vbox->dev->mode_config.suggested_x_property,
> -		vbox_connector->vbox_crtc->x_hint);
> -	drm_object_property_set_value(
> -		&connector->base, vbox->dev->mode_config.suggested_y_property,
> -		vbox_connector->vbox_crtc->y_hint);
> +
> +	if (vbox_connector->vbox_crtc->x_hint != -1)

Unsigned with signed comparison above.

> +		drm_object_property_set_value(&connector->base,
> +			vbox->dev->mode_config.suggested_x_property,
> +			vbox_connector->vbox_crtc->x_hint);
> +	else
> +		drm_object_property_set_value(&connector->base,
> +			vbox->dev->mode_config.suggested_x_property, 0);
> +
> +	if (vbox_connector->vbox_crtc->y_hint != -1)

And here.

> +		drm_object_property_set_value(&connector->base,
> +			vbox->dev->mode_config.suggested_y_property,
> +			vbox_connector->vbox_crtc->y_hint);
> +	else
> +		drm_object_property_set_value(&connector->base,
> +			vbox->dev->mode_config.suggested_y_property, 0);
>  
>  	return num_modes;
>  }
> @@ -640,9 +650,9 @@ static int vbox_connector_init(struct drm_device *dev,
>  
>  	drm_mode_create_suggested_offset_properties(dev);
>  	drm_object_attach_property(&connector->base,
> -				   dev->mode_config.suggested_x_property, -1);
> +				   dev->mode_config.suggested_x_property, 0);
>  	drm_object_attach_property(&connector->base,
> -				   dev->mode_config.suggested_y_property, -1);
> +				   dev->mode_config.suggested_y_property, 0);
>  	drm_connector_register(connector);
>  
>  	drm_mode_connector_attach_encoder(connector, encoder);
> 

-- 
Michael Thayer | VirtualBox engineer
ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstraße 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V.
Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister
der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher