[PATCH RFC 0/3] Add virtio-gpu modifiers support

[PATCH RFC 0/3] Add virtio-gpu modifiers support

[Tina Zhang]

This patchset introduces the modifiers support to virtio-gpu. 

This RFC version tries to add a new virtio-gpu command to let front-end driver
query the modifiers' info from the backend. Besides, the front-end driver
may also need a new drm helper function to add the supported modifiers to a
plane as its properties and allow fbs to use modifiers on that plane.

Tina Zhang (2):
  drm: Add drm_plane_add_modifiers()
  drm/virtio: Add modifier support

Vivek Kasireddy (1):
  drm/virtio: Include modifier as part of set_scanout_blob

 drivers/gpu/drm/drm_plane.c            | 41 +++++++++++++++++++++++
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  3 ++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  3 ++
 drivers/gpu/drm/virtio/virtgpu_plane.c | 16 +++++++++
 drivers/gpu/drm/virtio/virtgpu_vq.c    | 45 +++++++++++++++++++++++++-
 include/drm/drm_plane.h                |  3 ++
 include/uapi/linux/virtio_gpu.h        |  9 ++++++
 7 files changed, 119 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH RFC 1/3] drm: Add drm_plane_add_modifiers()

[Tina Zhang]

Add a function to add modifiers to a plane.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/drm_plane.c | 41 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_plane.h     |  3 +++
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index b570a480090a..793b16d84f86 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -288,6 +288,47 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 }
 EXPORT_SYMBOL(drm_universal_plane_init);
 
+int drm_plane_add_modifiers(struct drm_device *dev,
+				  struct drm_plane *plane,
+				  const uint64_t *format_modifiers)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	const uint64_t *temp_modifiers = format_modifiers;
+	unsigned int format_modifier_count = 0;
+
+	/*
+	 * Only considering adding modifiers when no modifier was
+	 * added to that plane before.
+	 */
+	if (!temp_modifiers || plane->modifier_count)
+		return -EINVAL;
+
+	while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
+		format_modifier_count++;
+
+	if (format_modifier_count)
+		config->allow_fb_modifiers = true;
+
+	plane->modifier_count = format_modifier_count;
+	plane->modifiers = kmalloc_array(format_modifier_count,
+					 sizeof(format_modifiers[0]),
+					 GFP_KERNEL);
+
+	if (format_modifier_count && !plane->modifiers) {
+		DRM_DEBUG_KMS("out of memory when allocating plane\n");
+		return -ENOMEM;
+	}
+
+	memcpy(plane->modifiers, format_modifiers,
+		   format_modifier_count * sizeof(format_modifiers[0]));
+	if (config->allow_fb_modifiers)
+		create_in_format_blob(dev, plane);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_add_modifiers);
+
+
 int drm_plane_register_all(struct drm_device *dev)
 {
 	unsigned int num_planes = 0;
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 50c23eb432b7..0dacdeffc3bc 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -827,6 +827,9 @@ int drm_universal_plane_init(struct drm_device *dev,
 			     const uint64_t *format_modifiers,
 			     enum drm_plane_type type,
 			     const char *name, ...);
+int drm_plane_add_modifiers(struct drm_device *dev,
+			       struct drm_plane *plane,
+			       const uint64_t *format_modifiers);
 int drm_plane_init(struct drm_device *dev,
 		   struct drm_plane *plane,
 		   uint32_t possible_crtcs,
-- 
2.25.1

[PATCH RFC 2/3] drm/virtio: Add modifier support

[Tina Zhang]

Add a command to get modifier info from the backend.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  3 ++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  3 ++
 drivers/gpu/drm/virtio/virtgpu_plane.c | 16 ++++++++++
 drivers/gpu/drm/virtio/virtgpu_vq.c    | 42 ++++++++++++++++++++++++++
 include/uapi/linux/virtio_gpu.h        |  8 +++++
 5 files changed, 72 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index d9dbc4f258f3..e077ea065558 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -250,6 +250,8 @@ struct virtio_gpu_device {
 	spinlock_t resource_export_lock;
 	/* protects map state and host_visible_mm */
 	spinlock_t host_visible_lock;
+
+	u64 modifiers[VIRTIO_GPU_PLANE_MAX_MODIFIERS];
 };
 
 struct virtio_gpu_fpriv {
@@ -329,6 +331,7 @@ int virtio_gpu_detach_status_page(struct virtio_gpu_device *vgdev);
 void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev,
 			    struct virtio_gpu_output *output);
 int virtio_gpu_cmd_get_display_info(struct virtio_gpu_device *vgdev);
+int virtio_gpu_cmd_get_plane_info(struct virtio_gpu_device *vgdev);
 int virtio_gpu_cmd_get_capset_info(struct virtio_gpu_device *vgdev, int idx);
 int virtio_gpu_cmd_get_capset(struct virtio_gpu_device *vgdev,
 			      int idx, int version,
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index b4ec479c32cd..3ecf36d92c5c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -226,6 +226,9 @@ int virtio_gpu_init(struct drm_device *dev)
 	virtio_gpu_notify(vgdev);
 	wait_event_timeout(vgdev->resp_wq, !vgdev->display_info_pending,
 			   5 * HZ);
+
+	virtio_gpu_cmd_get_plane_info(vgdev);
+
 	return 0;
 
 err_scanouts:
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index 42ac08ed1442..1b9b2a7bf0ac 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -73,6 +73,20 @@ static void virtio_gpu_plane_destroy(struct drm_plane *plane)
 	kfree(plane);
 }
 
+static bool virtio_plane_format_mod_supported(struct drm_plane *plane,
+					    u32 format, u64 modifier)
+{
+	struct drm_device *dev = plane->dev;
+	struct virtio_gpu_device *vgdev = dev->dev_private;
+	int i;
+
+	for (i = 0; i < VIRTIO_GPU_PLANE_MAX_MODIFIERS; i++)
+		if (modifier == vgdev->modifiers[i])
+			return true;
+
+	return false;
+}
+
 static const struct drm_plane_funcs virtio_gpu_plane_funcs = {
 	.update_plane		= drm_atomic_helper_update_plane,
 	.disable_plane		= drm_atomic_helper_disable_plane,
@@ -80,6 +94,7 @@ static const struct drm_plane_funcs virtio_gpu_plane_funcs = {
 	.reset			= drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
 	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
+	.format_mod_supported   = virtio_plane_format_mod_supported,
 };
 
 static int virtio_gpu_plane_atomic_check(struct drm_plane *plane,
@@ -348,6 +363,7 @@ struct drm_plane *virtio_gpu_plane_init(struct virtio_gpu_device *vgdev,
 		nformats = ARRAY_SIZE(virtio_gpu_formats);
 		funcs = &virtio_gpu_primary_helper_funcs;
 	}
+
 	ret = drm_universal_plane_init(dev, plane, 1 << index,
 				       &virtio_gpu_plane_funcs,
 				       formats, nformats,
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index cf84d382dd41..7a6d6628e167 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -678,6 +678,26 @@ static void virtio_gpu_cmd_get_display_info_cb(struct virtio_gpu_device *vgdev,
 		drm_kms_helper_hotplug_event(vgdev->ddev);
 }
 
+static void virtio_gpu_cmd_get_plane_info_cb(struct virtio_gpu_device *vgdev,
+					       struct virtio_gpu_vbuffer *vbuf)
+{
+	struct drm_device *dev = vgdev->ddev;
+	struct virtio_gpu_output *output = vgdev->outputs;
+	struct drm_crtc *crtc = &output->crtc;
+	struct virtio_gpu_resp_plane_info *resp =
+		(struct virtio_gpu_resp_plane_info *)vbuf->resp_buf;
+	int i;
+
+	for (i = 0; i < VIRTIO_GPU_PLANE_MAX_MODIFIERS; i++) {
+		vgdev->modifiers[i] = resp->modifiers[i];
+		if (vgdev->modifiers[i] == DRM_FORMAT_MOD_INVALID)
+			break;
+	}
+
+	if (i < VIRTIO_GPU_PLANE_MAX_MODIFIERS)
+		drm_plane_add_modifiers(dev, crtc->primary, vgdev->modifiers);
+}
+
 static void virtio_gpu_cmd_get_capset_info_cb(struct virtio_gpu_device *vgdev,
 					      struct virtio_gpu_vbuffer *vbuf)
 {
@@ -786,6 +806,28 @@ int virtio_gpu_cmd_get_display_info(struct virtio_gpu_device *vgdev)
 	return 0;
 }
 
+int virtio_gpu_cmd_get_plane_info(struct virtio_gpu_device *vgdev)
+{
+	struct virtio_gpu_ctrl_hdr *cmd_p;
+	struct virtio_gpu_vbuffer *vbuf;
+	void *resp_buf;
+
+	resp_buf = kzalloc(sizeof(struct virtio_gpu_resp_plane_info),
+			   GFP_KERNEL);
+	if (!resp_buf)
+		return -ENOMEM;
+
+	cmd_p = virtio_gpu_alloc_cmd_resp
+		(vgdev, &virtio_gpu_cmd_get_plane_info_cb, &vbuf,
+		 sizeof(*cmd_p), sizeof(struct virtio_gpu_resp_plane_info),
+		 resp_buf);
+	memset(cmd_p, 0, sizeof(*cmd_p));
+
+	cmd_p->type = cpu_to_le32(VIRTIO_GPU_CMD_GET_PLANE_INFO);
+	virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+	return 0;
+}
+
 int virtio_gpu_cmd_get_capset_info(struct virtio_gpu_device *vgdev, int idx)
 {
 	struct virtio_gpu_get_capset_info *cmd_p;
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 97523a95781d..f853d7672175 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -78,6 +78,7 @@ enum virtio_gpu_ctrl_type {
 	VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID,
 	VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB,
 	VIRTIO_GPU_CMD_SET_SCANOUT_BLOB,
+	VIRTIO_GPU_CMD_GET_PLANE_INFO,
 
 	/* 3d commands */
 	VIRTIO_GPU_CMD_CTX_CREATE = 0x0200,
@@ -103,6 +104,7 @@ enum virtio_gpu_ctrl_type {
 	VIRTIO_GPU_RESP_OK_EDID,
 	VIRTIO_GPU_RESP_OK_RESOURCE_UUID,
 	VIRTIO_GPU_RESP_OK_MAP_INFO,
+	VIRTIO_GPU_RESP_OK_PLANE_INFO,
 
 	/* error responses */
 	VIRTIO_GPU_RESP_ERR_UNSPEC = 0x1200,
@@ -232,6 +234,12 @@ struct virtio_gpu_resp_display_info {
 	} pmodes[VIRTIO_GPU_MAX_SCANOUTS];
 };
 
+#define VIRTIO_GPU_PLANE_MAX_MODIFIERS 8
+struct virtio_gpu_resp_plane_info {
+	struct virtio_gpu_ctrl_hdr hdr;
+	__le64 modifiers[VIRTIO_GPU_PLANE_MAX_MODIFIERS];
+};
+
 /* data passed in the control vq, 3d related */
 
 struct virtio_gpu_box {
-- 
2.25.1

[PATCH RFC 3/3] drm/virtio: Include modifier as part of set_scanout_blob

[Tina Zhang]

From: Vivek Kasireddy <vivek.kasireddy@intel.com>

With new use-cases coming up that include virtio-gpu:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/9592

the FB associated with a Guest blob may have a modifier. Therefore,
this modifier info needs to be included as part of set_scanout_blob.

v2: (Tina)
* Use drm_plane_add_modifiers() to set allow_fb_modifiers.
* Append the modifier field to the virtio_gpu_set_scanout_blob struct.

Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/virtio/virtgpu_vq.c | 3 ++-
 include/uapi/linux/virtio_gpu.h     | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 7a6d6628e167..351befed105a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -34,7 +34,7 @@
 #include "virtgpu_drv.h"
 #include "virtgpu_trace.h"
 
-#define MAX_INLINE_CMD_SIZE   96
+#define MAX_INLINE_CMD_SIZE   112
 #define MAX_INLINE_RESP_SIZE  24
 #define VBUFFER_SIZE          (sizeof(struct virtio_gpu_vbuffer) \
 			       + MAX_INLINE_CMD_SIZE		 \
@@ -1336,6 +1336,7 @@ void virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
 	cmd_p->format = cpu_to_le32(format);
 	cmd_p->width  = cpu_to_le32(fb->width);
 	cmd_p->height = cpu_to_le32(fb->height);
+	cmd_p->modifier = cpu_to_le64(fb->modifier);
 
 	for (i = 0; i < 4; i++) {
 		cmd_p->strides[i] = cpu_to_le32(fb->pitches[i]);
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index f853d7672175..6d08481ac4ef 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -420,6 +420,7 @@ struct virtio_gpu_set_scanout_blob {
 	__le32 padding;
 	__le32 strides[4];
 	__le32 offsets[4];
+	__le64 modifier;
 };
 
 /* VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB */
-- 
2.25.1

Re: [PATCH RFC 3/3] drm/virtio: Include modifier as part of set_scanout_blob

[Gerd Hoffmann]

> --- a/include/uapi/linux/virtio_gpu.h
> +++ b/include/uapi/linux/virtio_gpu.h
> @@ -420,6 +420,7 @@ struct virtio_gpu_set_scanout_blob {
>  	__le32 padding;
>  	__le32 strides[4];
>  	__le32 offsets[4];
> +	__le64 modifier;
>  };

All protocol changes (uapi/linux/virtio_gpu.h updates) should go to a
separate patch (best first in the series).  A feature flag is needed to
signal whenever modifier support is available or not.  The code needs to
cake care to not send the modifier field in case the host doesn't
support it.  Naming the modifier field "drm_modifier" is probably a good
idea to make clear that we'll use the drm modifier as-is.

A virtio-spec update is needed to document the protocol update.

take care,
  Gerd

Re: [PATCH RFC 1/3] drm: Add drm_plane_add_modifiers()

[Daniel Vetter]

On Mon, May 10, 2021 at 09:49:38PM -0400, Tina Zhang wrote:
> Add a function to add modifiers to a plane.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>

For one, new functions for drivers needs kerneldoc.

But the real issue here is that you're suppoed to supply the modifiers
when creating the plane, not later on. So this function doesn't make
sense.

Please fix virtio code to use the existing functions
(drm_universal_plane_init() to be specific), or explain what that's not
possible.
-Daniel
> ---
>  drivers/gpu/drm/drm_plane.c | 41 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_plane.h     |  3 +++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index b570a480090a..793b16d84f86 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -288,6 +288,47 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  }
>  EXPORT_SYMBOL(drm_universal_plane_init);
>  
> +int drm_plane_add_modifiers(struct drm_device *dev,
> +				  struct drm_plane *plane,
> +				  const uint64_t *format_modifiers)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	const uint64_t *temp_modifiers = format_modifiers;
> +	unsigned int format_modifier_count = 0;
> +
> +	/*
> +	 * Only considering adding modifiers when no modifier was
> +	 * added to that plane before.
> +	 */
> +	if (!temp_modifiers || plane->modifier_count)
> +		return -EINVAL;
> +
> +	while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> +		format_modifier_count++;
> +
> +	if (format_modifier_count)
> +		config->allow_fb_modifiers = true;
> +
> +	plane->modifier_count = format_modifier_count;
> +	plane->modifiers = kmalloc_array(format_modifier_count,
> +					 sizeof(format_modifiers[0]),
> +					 GFP_KERNEL);
> +
> +	if (format_modifier_count && !plane->modifiers) {
> +		DRM_DEBUG_KMS("out of memory when allocating plane\n");
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(plane->modifiers, format_modifiers,
> +		   format_modifier_count * sizeof(format_modifiers[0]));
> +	if (config->allow_fb_modifiers)
> +		create_in_format_blob(dev, plane);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_add_modifiers);
> +
> +
>  int drm_plane_register_all(struct drm_device *dev)
>  {
>  	unsigned int num_planes = 0;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 50c23eb432b7..0dacdeffc3bc 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -827,6 +827,9 @@ int drm_universal_plane_init(struct drm_device *dev,
>  			     const uint64_t *format_modifiers,
>  			     enum drm_plane_type type,
>  			     const char *name, ...);
> +int drm_plane_add_modifiers(struct drm_device *dev,
> +			       struct drm_plane *plane,
> +			       const uint64_t *format_modifiers);
>  int drm_plane_init(struct drm_device *dev,
>  		   struct drm_plane *plane,
>  		   uint32_t possible_crtcs,
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch