| Message ID | 20171012181025.10480-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
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); >> >
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);
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(-)