| Message ID | 20221118013054.182304-3-vivek.kasireddy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/virtio: Add hotplug_mode_update and suggested_x/y properties | expand |
On Thu, Nov 17, 2022 at 05:30:54PM -0800, Vivek Kasireddy wrote: > Setting this property will allow the userspace to look for new modes or > position info when a hotplug event occurs. This works just fine for modes today. I assume this is this need to have userspace also check for position info updates added by patch #1)? take care, Gerd
On Fri, Jan 06, 2023 at 09:56:40AM +0100, Gerd Hoffmann wrote: > On Thu, Nov 17, 2022 at 05:30:54PM -0800, Vivek Kasireddy wrote: > > Setting this property will allow the userspace to look for new modes or > > position info when a hotplug event occurs. > > This works just fine for modes today. > > I assume this is this need to have userspace also check for position > info updates added by patch #1)? What does this thing even do? Quick grep says qxl and vmwgfx also use this, but it's not documented anywhere, and it's also not done with any piece of common code. Which all looks really fishy. I think we need to do a bit of refactoring/documenting here first. Also in principle, userspace needs to look at everything in the connector again when it gets a hotplug event. We do have hotplug events for specific properties nowadays, but those are fairly new. -Daniel
Hi Gerd, > > On Thu, Nov 17, 2022 at 05:30:54PM -0800, Vivek Kasireddy wrote: > > Setting this property will allow the userspace to look for new modes or > > position info when a hotplug event occurs. > > This works just fine for modes today. > > I assume this is this need to have userspace also check for position > info updates added by patch #1)? [Kasireddy, Vivek] Yes, that is exactly the reason why this property is needed. In other words, Mutter does not seem to look at suggested_x/y values (or position info) if hotplug_mode_property is not there. Here is the relevant piece of code in Mutter: static gboolean meta_monitor_normal_get_suggested_position (MetaMonitor *monitor, int *x, int *y) { const MetaOutputInfo *output_info = meta_monitor_get_main_output_info (monitor); if (!output_info->hotplug_mode_update) return FALSE; if (output_info->suggested_x < 0 && output_info->suggested_y < 0) return FALSE; if (x) *x = output_info->suggested_x; if (y) *y = output_info->suggested_y; Thanks, Vivek > > take care, > Gerd
Hi Daniel, > > On Fri, Jan 06, 2023 at 09:56:40AM +0100, Gerd Hoffmann wrote: > > On Thu, Nov 17, 2022 at 05:30:54PM -0800, Vivek Kasireddy wrote: > > > Setting this property will allow the userspace to look for new modes or > > > position info when a hotplug event occurs. > > > > This works just fine for modes today. > > > > I assume this is this need to have userspace also check for position > > info updates added by patch #1)? > > What does this thing even do? Quick grep says qxl and vmwgfx also use > this, but it's not documented anywhere, and it's also not done with any > piece of common code. Which all looks really fishy. [Kasireddy, Vivek] AFAIU, this property appears to be useful only for virtual GPU drivers to share the Host output(s) layout with the Guest compositor. The suggested_x/y properties are specifically used for this purpose but it looks like the hotplug_mode_update property also needs to be set in order to have Guest compositors (Mutter cares but Weston does not) look at suggested_x/y. > > I think we need to do a bit of refactoring/documenting here first. [Kasireddy, Vivek] Just for reference, here is Dave's commit that added this property for qxl: commit 4695b03970df378dcb93fe3e7158381f1e980fa2 Author: Dave Airlie <airlied@redhat.com> Date: Fri Oct 11 11:05:00 2013 +1000 qxl: add a connector property to denote hotplug should rescan modes. So GNOME userspace has an issue with when it rescans for modes on hotplug events, if the monitor has no EDID it assumes that nothing has changed on EDID as with real hw we'd never have new modes without a new EDID, and they kind off rely on the behaviour now, however with virtual GPUs we would like to rescan the modes and get a new preferred mode on hotplug events to handle dynamic guest resizing (where you resize the host window and the guest resizes with it). This is a simple property we can make userspace watch for to trigger new behaviour based on it, and can be used to replaced EDID hacks in virtual drivers. Are you suggesting that this property needs to be part of drm_mode_config just like suggested_x/y properties? > > Also in principle, userspace needs to look at everything in the connector > again when it gets a hotplug event. We do have hotplug events for specific > properties nowadays, but those are fairly new. [Kasireddy, Vivek] From what I understand, Mutter does probe all the connector properties during hotplug but it still needs this property to be set in order to consider suggested_x/y values. And, it appears, some customers and users have relied on this behavior from when these properties were first introduced for virtual GPU drivers. Thanks, Vivek > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, Jan 06, 2023 at 10:35:15AM +0100, Daniel Vetter wrote: > On Fri, Jan 06, 2023 at 09:56:40AM +0100, Gerd Hoffmann wrote: > > On Thu, Nov 17, 2022 at 05:30:54PM -0800, Vivek Kasireddy wrote: > > > Setting this property will allow the userspace to look for new modes or > > > position info when a hotplug event occurs. > > > > This works just fine for modes today. > > > > I assume this is this need to have userspace also check for position > > info updates added by patch #1)? > > What does this thing even do? Quick grep says qxl and vmwgfx also use > this, but it's not documented anywhere, and it's also not done with any > piece of common code. Which all looks really fishy. It's again a virtualization-specific thing. On physical hardware you typically have no idea which of your two monitors stands left and which stands right. On virtual hardware the host knows how the two windows for the two heads are arranged and can pass on that information to the guest. suggested_x/y properties added by patch #1 do pass that information to userspace so the display server can arrange things correctly without manual invention. I have no clue though why this hotplug_mode_update property exists in the first place and why mutter checks it. IMHO mutter could just check for suggested_x/y directly. take care, Gerd
On Tue, Jan 10, 2023 at 10:28:39AM +0100, Gerd Hoffmann wrote: > On Fri, Jan 06, 2023 at 10:35:15AM +0100, Daniel Vetter wrote: > > On Fri, Jan 06, 2023 at 09:56:40AM +0100, Gerd Hoffmann wrote: > > > On Thu, Nov 17, 2022 at 05:30:54PM -0800, Vivek Kasireddy wrote: > > > > Setting this property will allow the userspace to look for new modes or > > > > position info when a hotplug event occurs. > > > > > > This works just fine for modes today. > > > > > > I assume this is this need to have userspace also check for position > > > info updates added by patch #1)? > > > > What does this thing even do? Quick grep says qxl and vmwgfx also use > > this, but it's not documented anywhere, and it's also not done with any > > piece of common code. Which all looks really fishy. > > It's again a virtualization-specific thing. On physical hardware you > typically have no idea which of your two monitors stands left and which > stands right. On virtual hardware the host knows how the two windows > for the two heads are arranged and can pass on that information to the > guest. suggested_x/y properties added by patch #1 do pass that > information to userspace so the display server can arrange things > correctly without manual invention. Yeah suggested_x/y I know about. Would still be good to fix the documentation situation for that. > I have no clue though why this hotplug_mode_update property exists in > the first place and why mutter checks it. IMHO mutter could just check > for suggested_x/y directly. This one is the complete wtf here. I have no idea why this exists. -Daniel
On Tue, Jan 10, 2023 at 01:58:52AM +0000, Kasireddy, Vivek wrote: > Hi Daniel, > > > > > On Fri, Jan 06, 2023 at 09:56:40AM +0100, Gerd Hoffmann wrote: > > > On Thu, Nov 17, 2022 at 05:30:54PM -0800, Vivek Kasireddy wrote: > > > > Setting this property will allow the userspace to look for new modes or > > > > position info when a hotplug event occurs. > > > > > > This works just fine for modes today. > > > > > > I assume this is this need to have userspace also check for position > > > info updates added by patch #1)? > > > > What does this thing even do? Quick grep says qxl and vmwgfx also use > > this, but it's not documented anywhere, and it's also not done with any > > piece of common code. Which all looks really fishy. > [Kasireddy, Vivek] AFAIU, this property appears to be useful only for virtual > GPU drivers to share the Host output(s) layout with the Guest compositor. The > suggested_x/y properties are specifically used for this purpose but it looks like > the hotplug_mode_update property also needs to be set in order to have Guest > compositors (Mutter cares but Weston does not) look at suggested_x/y. > > > > > I think we need to do a bit of refactoring/documenting here first. > [Kasireddy, Vivek] Just for reference, here is Dave's commit that added this > property for qxl: > commit 4695b03970df378dcb93fe3e7158381f1e980fa2 > Author: Dave Airlie <airlied@redhat.com> > Date: Fri Oct 11 11:05:00 2013 +1000 > > qxl: add a connector property to denote hotplug should rescan modes. > > So GNOME userspace has an issue with when it rescans for modes on hotplug > events, if the monitor has no EDID it assumes that nothing has changed on > EDID as with real hw we'd never have new modes without a new EDID, and they > kind off rely on the behaviour now, however with virtual GPUs we would > like to rescan the modes and get a new preferred mode on hotplug events > to handle dynamic guest resizing (where you resize the host window and the > guest resizes with it). Ok this is just terrible. Because _anything_ without an EDID is impacted, and we're certainly not going to sprinkle this property all over gpu drivers just so Gnome takes the right path. Can we fix gnome instead to be slightly less dense about this stuff? > This is a simple property we can make userspace watch for to trigger new > behaviour based on it, and can be used to replaced EDID hacks in virtual > drivers. > > Are you suggesting that this property needs to be part of drm_mode_config > just like suggested_x/y properties? I think this thing shouldn't exist :-) > > Also in principle, userspace needs to look at everything in the connector > > again when it gets a hotplug event. We do have hotplug events for specific > > properties nowadays, but those are fairly new. > [Kasireddy, Vivek] From what I understand, Mutter does probe all the > connector properties during hotplug but it still needs this property to be set in > order to consider suggested_x/y values. And, it appears, some customers and > users have relied on this behavior from when these properties were first > introduced for virtual GPU drivers. Be more like Weston. Whatever it is mutter does, it doesn't make much sense to me. We can't remove this from existing gpu drivers because no regression guarantee, but we definitely shouldn't add it anywhere. Also can you please document that this is a "do not ever use this in any new driver" kind of property when you go about docementing these? Thanks, Daniel
Hi, > > > I think we need to do a bit of refactoring/documenting here first. > > [Kasireddy, Vivek] Just for reference, here is Dave's commit that added this > > property for qxl: > > commit 4695b03970df378dcb93fe3e7158381f1e980fa2 > > Author: Dave Airlie <airlied@redhat.com> > > Date: Fri Oct 11 11:05:00 2013 +1000 > > > > qxl: add a connector property to denote hotplug should rescan modes. > > > > So GNOME userspace has an issue with when it rescans for modes on hotplug > > events, if the monitor has no EDID it assumes that nothing has changed on > > EDID as with real hw we'd never have new modes without a new EDID, and they > > kind off rely on the behaviour now, however with virtual GPUs we would > > like to rescan the modes and get a new preferred mode on hotplug events > > to handle dynamic guest resizing (where you resize the host window and the > > guest resizes with it). > > Ok this is just terrible. Because _anything_ without an EDID is impacted, > and we're certainly not going to sprinkle this property all over gpu > drivers just so Gnome takes the right path. Oh, and (newer) virtio-gpu actually has EDIDs ... take care, Gerd
On Thu, Jan 12, 2023 at 08:17:19AM +0100, Gerd Hoffmann wrote: > Hi, > > > > > I think we need to do a bit of refactoring/documenting here first. > > > [Kasireddy, Vivek] Just for reference, here is Dave's commit that added this > > > property for qxl: > > > commit 4695b03970df378dcb93fe3e7158381f1e980fa2 > > > Author: Dave Airlie <airlied@redhat.com> > > > Date: Fri Oct 11 11:05:00 2013 +1000 > > > > > > qxl: add a connector property to denote hotplug should rescan modes. > > > > > > So GNOME userspace has an issue with when it rescans for modes on hotplug > > > events, if the monitor has no EDID it assumes that nothing has changed on > > > EDID as with real hw we'd never have new modes without a new EDID, and they > > > kind off rely on the behaviour now, however with virtual GPUs we would > > > like to rescan the modes and get a new preferred mode on hotplug events > > > to handle dynamic guest resizing (where you resize the host window and the > > > guest resizes with it). > > > > Ok this is just terrible. Because _anything_ without an EDID is impacted, > > and we're certainly not going to sprinkle this property all over gpu > > drivers just so Gnome takes the right path. > > Oh, and (newer) virtio-gpu actually has EDIDs ... I forgot to mention. If userspace is worried about the expensive probe cycle that calling the GETCONNECTOR ioctl can cause. There's drmModeGetConnectorCurrent. But that's not guaranteed to give you up to date info after a hotplug event, depending upon driver :-) -Daniel
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index 868b0183c6df..09a8089bb62a 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -286,6 +286,8 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index) dev->mode_config.suggested_x_property, 0); drm_object_attach_property(&connector->base, dev->mode_config.suggested_y_property, 0); + drm_object_attach_property(&connector->base, + vgdev->hotplug_mode_update_property, 1); drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_VIRTUAL); drm_encoder_helper_add(encoder, &virtio_gpu_enc_helper_funcs); @@ -336,6 +338,17 @@ static const struct drm_mode_config_funcs virtio_gpu_mode_funcs = { .atomic_commit = drm_atomic_helper_commit, }; +static void +virtio_gpu_create_hotplug_mode_update_property(struct virtio_gpu_device *vgdev) +{ + if (vgdev->hotplug_mode_update_property) + return; + + vgdev->hotplug_mode_update_property = + drm_property_create_range(vgdev->ddev, DRM_MODE_PROP_IMMUTABLE, + "hotplug_mode_update", 0, 1); +} + int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) { int i, ret; @@ -355,6 +368,7 @@ int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) vgdev->ddev->mode_config.fb_modifiers_not_supported = true; drm_mode_create_suggested_offset_properties(vgdev->ddev); + virtio_gpu_create_hotplug_mode_update_property(vgdev); for (i = 0 ; i < vgdev->num_scanouts; ++i) vgdev_output_init(vgdev, i); diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h index b7a64c7dcc2c..35f940302e24 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -257,6 +257,8 @@ struct virtio_gpu_device { uint64_t capset_id_mask; struct list_head cap_cache; + struct drm_property *hotplug_mode_update_property; + /* protects uuid state when exporting */ spinlock_t resource_export_lock; /* protects map state and host_visible_mm */
Setting this property will allow the userspace to look for new modes or position info when a hotplug event occurs. This is really helpful for virtual GPU drivers to handle Host window resizing events which are propogated as hotplug interrupts to the Guest drivers. Mutter already uses this property while configuring the outputs. Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Dongwon Kim <dongwon.kim@intel.com> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> --- drivers/gpu/drm/virtio/virtgpu_display.c | 14 ++++++++++++++ drivers/gpu/drm/virtio/virtgpu_drv.h | 2 ++ 2 files changed, 16 insertions(+)