* [PATCH] staging: vboxvideo: Fix reporting invalid suggested-offset-properties
@ 2017-10-12 18:10 Hans de Goede
2017-10-13 6:26 ` Michael Thayer
0 siblings, 1 reply; 3+ messages in thread
From: Hans de Goede @ 2017-10-12 18:10 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Hans de Goede, Daniel Vetter, Michael Thayer, dri-devel, devel, stable
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.
Reported-by: Gianfranco Costamagna <locutusofborg@debian.org>
Cc: stable@vger.kernel.org
Cc: Michael Thayer <michael.thayer@oracle.com>
Signed-off-by: Hans de Goede <hdegoede@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)
+ 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)
+ 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);
--
2.14.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] staging: vboxvideo: Fix reporting invalid suggested-offset-properties
2017-10-12 18:10 [PATCH] staging: vboxvideo: Fix reporting invalid suggested-offset-properties Hans de Goede
@ 2017-10-13 6:26 ` Michael Thayer
2017-10-13 11:02 ` Hans de Goede
0 siblings, 1 reply; 3+ messages in thread
From: Michael Thayer @ 2017-10-13 6:26 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman; +Cc: Daniel Vetter, dri-devel, devel, stable
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@debian.org>
> Cc: stable@vger.kernel.org
> Cc: Michael Thayer <michael.thayer@oracle.com>
> Signed-off-by: Hans de Goede <hdegoede@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] staging: vboxvideo: Fix reporting invalid suggested-offset-properties
2017-10-13 6:26 ` Michael Thayer
@ 2017-10-13 11:02 ` Hans de Goede
0 siblings, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2017-10-13 11:02 UTC (permalink / raw)
To: Michael Thayer, Greg Kroah-Hartman
Cc: Daniel Vetter, dri-devel, devel, stable
Hi,
On 13-10-17 08:26, Michael Thayer wrote:
> 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)?
It seems to me that the intent clearly is for -1 to be used as a
"not set / invalid" value even though it is an unsigned integer.
unsigned vs signed comparisons on integers of the same size are
really only a problem when using < or > not when using == or !=,
so this really is fine as is IMHO.
Regards,
Hans
>> Reported-by: Gianfranco Costamagna <locutusofborg@debian.org>
>> Cc: stable@vger.kernel.org
>> Cc: Michael Thayer <michael.thayer@oracle.com>
>> Signed-off-by: Hans de Goede <hdegoede@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);
>>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-13 11:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-12 18:10 [PATCH] staging: vboxvideo: Fix reporting invalid suggested-offset-properties Hans de Goede
2017-10-13 6:26 ` Michael Thayer
2017-10-13 11:02 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.