All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/2] drm/virtio: Add hotplug_mode_update and suggested_x/y properties
@ 2022-11-18  1:30 Vivek Kasireddy
  2022-11-18  1:30 ` [PATCH v1 1/2] drm/virtio: Attach and set suggested_x/y properties for the connector Vivek Kasireddy
  2022-11-18  1:30 ` [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes Vivek Kasireddy
  0 siblings, 2 replies; 13+ messages in thread
From: Vivek Kasireddy @ 2022-11-18  1:30 UTC (permalink / raw)
  To: dri-devel; +Cc: Dongwon Kim, Vivek Kasireddy, Gerd Hoffmann

These properties provide a way to influence the userspace with regard
to positioning of the outputs and also enable it to look for new
preferred modes when hotplug interrupts occur. 

These properties are currently used by other virtual GPU drivers
such as vmwgfx and qxl.

Testcase: After positioning the VMM's (e.g, Qemu) windows on various
monitors and launching the Guest VM, run the get-state tool (tools/
get-state.py) associated with Mutter to ensure that the Hosts' and
Guests' outputs are aligned.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>

Vivek Kasireddy (2):
  drm/virtio: Attach and set suggested_x/y properties for the connector
  drm/virtio: Add the hotplug_mode_update property for rescanning of
    modes

 drivers/gpu/drm/virtio/virtgpu_display.c | 19 +++++++++++++++++++
 drivers/gpu/drm/virtio/virtgpu_drv.h     |  2 ++
 drivers/gpu/drm/virtio/virtgpu_vq.c      | 12 ++++++++++++
 3 files changed, 33 insertions(+)

-- 
2.37.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v1 1/2] drm/virtio: Attach and set suggested_x/y properties for the connector
  2022-11-18  1:30 [PATCH v1 0/2] drm/virtio: Add hotplug_mode_update and suggested_x/y properties Vivek Kasireddy
@ 2022-11-18  1:30 ` Vivek Kasireddy
  2023-01-10  9:33   ` Gerd Hoffmann
  2022-11-18  1:30 ` [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes Vivek Kasireddy
  1 sibling, 1 reply; 13+ messages in thread
From: Vivek Kasireddy @ 2022-11-18  1:30 UTC (permalink / raw)
  To: dri-devel; +Cc: Dongwon Kim, Vivek Kasireddy, Gerd Hoffmann

These properties provide a way to suggest to the userspace the preferred
positions for the outputs. Mutter already uses these properties to
determine the best positions for 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 |  5 +++++
 drivers/gpu/drm/virtio/virtgpu_vq.c      | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
index 9ea7611a9e0f..868b0183c6df 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -282,6 +282,10 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index)
 	drm_connector_helper_add(connector, &virtio_gpu_conn_helper_funcs);
 	if (vgdev->has_edid)
 		drm_connector_attach_edid_property(connector);
+	drm_object_attach_property(&connector->base,
+				   dev->mode_config.suggested_x_property, 0);
+	drm_object_attach_property(&connector->base,
+				   dev->mode_config.suggested_y_property, 0);
 
 	drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_VIRTUAL);
 	drm_encoder_helper_add(encoder, &virtio_gpu_enc_helper_funcs);
@@ -350,6 +354,7 @@ int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
 	vgdev->ddev->mode_config.max_height = YRES_MAX;
 
 	vgdev->ddev->mode_config.fb_modifiers_not_supported = true;
+	drm_mode_create_suggested_offset_properties(vgdev->ddev);
 
 	for (i = 0 ; i < vgdev->num_scanouts; ++i)
 		vgdev_output_init(vgdev, i);
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 9ff8660b50ad..8e2e512e7c4b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -638,6 +638,17 @@ virtio_gpu_cmd_resource_attach_backing(struct virtio_gpu_device *vgdev,
 	virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, fence);
 }
 
+static void virtio_gpu_update_output_position(struct virtio_gpu_output *output)
+{
+	struct drm_connector *connector = &output->conn;
+	struct drm_device *dev = connector->dev;
+
+	drm_object_property_set_value(&connector->base,
+		dev->mode_config.suggested_x_property, output->info.r.x);
+	drm_object_property_set_value(&connector->base,
+		dev->mode_config.suggested_y_property, output->info.r.y);
+}
+
 static void virtio_gpu_cmd_get_display_info_cb(struct virtio_gpu_device *vgdev,
 					       struct virtio_gpu_vbuffer *vbuf)
 {
@@ -648,6 +659,7 @@ static void virtio_gpu_cmd_get_display_info_cb(struct virtio_gpu_device *vgdev,
 	spin_lock(&vgdev->display_info_lock);
 	for (i = 0; i < vgdev->num_scanouts; i++) {
 		vgdev->outputs[i].info = resp->pmodes[i];
+		virtio_gpu_update_output_position(&vgdev->outputs[i]);
 		if (resp->pmodes[i].enabled) {
 			DRM_DEBUG("output %d: %dx%d+%d+%d", i,
 				  le32_to_cpu(resp->pmodes[i].r.width),
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
  2022-11-18  1:30 [PATCH v1 0/2] drm/virtio: Add hotplug_mode_update and suggested_x/y properties Vivek Kasireddy
  2022-11-18  1:30 ` [PATCH v1 1/2] drm/virtio: Attach and set suggested_x/y properties for the connector Vivek Kasireddy
@ 2022-11-18  1:30 ` Vivek Kasireddy
  2023-01-06  8:56   ` Gerd Hoffmann
  1 sibling, 1 reply; 13+ messages in thread
From: Vivek Kasireddy @ 2022-11-18  1:30 UTC (permalink / raw)
  To: dri-devel; +Cc: Dongwon Kim, Vivek Kasireddy, Gerd Hoffmann

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(+)

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 */
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
  2022-11-18  1:30 ` [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes Vivek Kasireddy
@ 2023-01-06  8:56   ` Gerd Hoffmann
  2023-01-06  9:35     ` Daniel Vetter
  2023-01-10  1:42     ` Kasireddy, Vivek
  0 siblings, 2 replies; 13+ messages in thread
From: Gerd Hoffmann @ 2023-01-06  8:56 UTC (permalink / raw)
  To: Vivek Kasireddy; +Cc: Dongwon Kim, dri-devel

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
  2023-01-06  8:56   ` Gerd Hoffmann
@ 2023-01-06  9:35     ` Daniel Vetter
  2023-01-10  1:58       ` Kasireddy, Vivek
  2023-01-10  9:28       ` Gerd Hoffmann
  2023-01-10  1:42     ` Kasireddy, Vivek
  1 sibling, 2 replies; 13+ messages in thread
From: Daniel Vetter @ 2023-01-06  9:35 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Vivek Kasireddy, dri-devel, Dongwon Kim

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
  2023-01-06  8:56   ` Gerd Hoffmann
  2023-01-06  9:35     ` Daniel Vetter
@ 2023-01-10  1:42     ` Kasireddy, Vivek
  1 sibling, 0 replies; 13+ messages in thread
From: Kasireddy, Vivek @ 2023-01-10  1:42 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Kim, Dongwon, dri-devel

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
  2023-01-06  9:35     ` Daniel Vetter
@ 2023-01-10  1:58       ` Kasireddy, Vivek
  2023-01-11 22:12         ` Daniel Vetter
  2023-01-10  9:28       ` Gerd Hoffmann
  1 sibling, 1 reply; 13+ messages in thread
From: Kasireddy, Vivek @ 2023-01-10  1:58 UTC (permalink / raw)
  To: Daniel Vetter, Gerd Hoffmann; +Cc: Kim, Dongwon, dri-devel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
  2023-01-06  9:35     ` Daniel Vetter
  2023-01-10  1:58       ` Kasireddy, Vivek
@ 2023-01-10  9:28       ` Gerd Hoffmann
  2023-01-11 22:08         ` Daniel Vetter
  1 sibling, 1 reply; 13+ messages in thread
From: Gerd Hoffmann @ 2023-01-10  9:28 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Vivek Kasireddy, dri-devel, Dongwon Kim

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/2] drm/virtio: Attach and set suggested_x/y properties for the connector
  2022-11-18  1:30 ` [PATCH v1 1/2] drm/virtio: Attach and set suggested_x/y properties for the connector Vivek Kasireddy
@ 2023-01-10  9:33   ` Gerd Hoffmann
  0 siblings, 0 replies; 13+ messages in thread
From: Gerd Hoffmann @ 2023-01-10  9:33 UTC (permalink / raw)
  To: Vivek Kasireddy; +Cc: Dongwon Kim, dri-devel

  Hi,

> +static void virtio_gpu_update_output_position(struct virtio_gpu_output *output)
> +{
> +	struct drm_connector *connector = &output->conn;
> +	struct drm_device *dev = connector->dev;
> +
> +	drm_object_property_set_value(&connector->base,
> +		dev->mode_config.suggested_x_property, output->info.r.x);
> +	drm_object_property_set_value(&connector->base,
> +		dev->mode_config.suggested_y_property, output->info.r.y);
> +}

This fails sparse checking

sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/virtio/virtgpu_vq.c:654:70: sparse: sparse: incorrect type in argument 3 (different base types) @@     expected unsigned long long [usertype] val @@     got restricted __le32 [usertype] x @@
   drivers/gpu/drm/virtio/virtgpu_vq.c:654:70: sparse:     expected unsigned long long [usertype] val
   drivers/gpu/drm/virtio/virtgpu_vq.c:654:70: sparse:     got restricted __le32 [usertype] x
>> drivers/gpu/drm/virtio/virtgpu_vq.c:656:70: sparse: sparse: incorrect type in argument 3 (different base types) @@     expected unsigned long long [usertype] val @@     got restricted __le32 [usertype] y @@
   drivers/gpu/drm/virtio/virtgpu_vq.c:656:70: sparse:     expected unsigned long long [usertype] val
   drivers/gpu/drm/virtio/virtgpu_vq.c:656:70: sparse:     got restricted __le32 [usertype] y

take care,
  Gerd


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
  2023-01-10  9:28       ` Gerd Hoffmann
@ 2023-01-11 22:08         ` Daniel Vetter
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2023-01-11 22:08 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: dri-devel, Vivek Kasireddy, Dongwon Kim

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
  2023-01-10  1:58       ` Kasireddy, Vivek
@ 2023-01-11 22:12         ` Daniel Vetter
  2023-01-12  7:17           ` Gerd Hoffmann
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Vetter @ 2023-01-11 22:12 UTC (permalink / raw)
  To: Kasireddy, Vivek; +Cc: dri-devel, Gerd Hoffmann, Kim, Dongwon

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
  2023-01-11 22:12         ` Daniel Vetter
@ 2023-01-12  7:17           ` Gerd Hoffmann
  2023-01-12  9:17             ` Daniel Vetter
  0 siblings, 1 reply; 13+ messages in thread
From: Gerd Hoffmann @ 2023-01-12  7:17 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Kasireddy, Vivek, dri-devel, Kim, Dongwon

  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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
  2023-01-12  7:17           ` Gerd Hoffmann
@ 2023-01-12  9:17             ` Daniel Vetter
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2023-01-12  9:17 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: dri-devel, Kasireddy, Vivek, Kim, Dongwon

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-01-12  9:17 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-18  1:30 [PATCH v1 0/2] drm/virtio: Add hotplug_mode_update and suggested_x/y properties Vivek Kasireddy
2022-11-18  1:30 ` [PATCH v1 1/2] drm/virtio: Attach and set suggested_x/y properties for the connector Vivek Kasireddy
2023-01-10  9:33   ` Gerd Hoffmann
2022-11-18  1:30 ` [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes Vivek Kasireddy
2023-01-06  8:56   ` Gerd Hoffmann
2023-01-06  9:35     ` Daniel Vetter
2023-01-10  1:58       ` Kasireddy, Vivek
2023-01-11 22:12         ` Daniel Vetter
2023-01-12  7:17           ` Gerd Hoffmann
2023-01-12  9:17             ` Daniel Vetter
2023-01-10  9:28       ` Gerd Hoffmann
2023-01-11 22:08         ` Daniel Vetter
2023-01-10  1:42     ` Kasireddy, Vivek

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.