Skip to contentSkip to conversation

drm/msm/gem: Add metadata

PATCH Superseded Rob Clark 2 participants 8 messages Oct 27, 2023 – Oct 29, 2023

Subsystems (5)
DRM drivers DRM misc Arm Qualcomm mailing list DRM driver for Qualcomm display hardware +1 more subsystems

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Rob Clark

author
#0 lore raw

PATCH 1/1 drm/msm/gem: Add metadata

From: Rob Clark <[email protected]>

The EXT_external_objects extension is a bit awkward as it doesn't pass explicit modifiers, leaving the importer to guess with incomplete information. In the case of vk (turnip) exporting and gl (freedreno) importing, the "OPTIMAL_TILING_EXT" layout depends on VkImageCreateInfo flags (among other things), which the importer does not know. Which unfortunately leaves us with the need for a metadata back-channel.

The contents of the metadata are defined by userspace. The EXT_external_objects extension is only required to work between compatible versions of gl and vk drivers, as defined by device and driver UUIDs.

Signed-off-by Rob Clark <[email protected]>
---
 drivers/gpu/drm/msm/msm_drv.c | 59 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/msm/msm_gem.h |  4 +++
 include/uapi/drm/msm_drm.h    |  2 ++
 3 files changed, 63 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/msm_drv.c +57 -2
index b61ccea05327..8fe2677ea37a 100644
@@ -37,9 +37,10 @@
  * - 1.9.0 - Add MSM_SUBMIT_FENCE_SN_IN
  * - 1.10.0 - Add MSM_SUBMIT_BO_NO_IMPLICIT
  * - 1.11.0 - Add wait boost (MSM_WAIT_FENCE_BOOST, MSM_PREP_BOOST)
+ * - 1.12.0 - Add MSM_INFO_SET_METADATA and MSM_INFO_GET_METADATA
  */
 #define MSM_VERSION_MAJOR	1
-#define MSM_VERSION_MINOR	10
+#define MSM_VERSION_MINOR	12
 #define MSM_VERSION_PATCHLEVEL	0
 
 static void msm_deinit_vram(struct drm_device *ddev);

@@ -566,6 +567,8 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 		break;
 	case MSM_INFO_SET_NAME:
 	case MSM_INFO_GET_NAME:
+	case MSM_INFO_SET_METADATA:
+	case MSM_INFO_GET_METADATA:
 		break;
 	default:
 		return -EINVAL;

@@ -618,7 +621,7 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 		break;
 	case MSM_INFO_GET_NAME:
 		if (args->value && (args->len < strlen(msm_obj->name))) {
-			ret = -EINVAL;
+			ret = -ETOOSMALL;
 			break;
 		}
 		args->len = strlen(msm_obj->name);

@@ -627,6 +630,58 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 					 msm_obj->name, args->len))
 				ret = -EFAULT;
 		}
+		break;
+	case MSM_INFO_SET_METADATA:
+		/* Impose a moderate upper bound on metadata size: */
+		if (args->len > 128) {
+			ret = -EOVERFLOW;
+			break;
+		}
+
+		ret = msm_gem_lock_interruptible(obj);
+		if (ret)
+			break;
+
+		msm_obj->metadata =
+			krealloc(msm_obj->metadata, args->len, GFP_KERNEL);
+		msm_obj->metadata_size = args->len;
+
+		if (copy_from_user(msm_obj->metadata, u64_to_user_ptr(args->value),
+				   args->len)) {
+			msm_obj->metadata_size = 0;
+			ret = -EFAULT;
+		}
+
+		msm_gem_unlock(obj);
+
+		break;
+	case MSM_INFO_GET_METADATA:
+		if (!args->value) {
+			/*
+			 * Querying the size is inherently racey, but
+			 * EXT_external_objects expects the app to confirm
+			 * via device and driver UUIDs that the exporter and
+			 * importer versions match.  All we can do from the
+			 * kernel side is check the length under obj lock
+			 * when userspace tries to retrieve the metadata
+			 */
+			args->len = msm_obj->metadata_size;
+			break;
+		}
+
+		ret = msm_gem_lock_interruptible(obj);
+		if (ret)
+			break;
+
+		if (args->len < msm_obj->metadata_size) {
+			ret = -ETOOSMALL;
+		} else if (copy_to_user(u64_to_user_ptr(args->value),
+					msm_obj->metadata, args->len)) {
+			ret = -EFAULT;
+		}
+
+		msm_gem_unlock(obj);
+
 		break;
 	}
drivers/gpu/drm/msm/msm_gem.h +4 -0
index 7f34263048a3..8d414b072c29 100644
@@ -109,6 +109,10 @@ struct msm_gem_object {
 
 	char name[32]; /* Identifier to print for the debugfs files */
 
+	/* userspace metadata backchannel */
+	void *metadata;
+	u32 metadata_size;
+
 	/**
 	 * pin_count: Number of times the pages are pinned
 	 *
include/uapi/drm/msm_drm.h +2 -0
index 6c34272a13fd..6f2a7ad04aa4 100644
@@ -139,6 +139,8 @@ struct drm_msm_gem_new {
 #define MSM_INFO_GET_NAME	0x03   /* get debug name, returned by pointer */
 #define MSM_INFO_SET_IOVA	0x04   /* set the iova, passed by value */
 #define MSM_INFO_GET_FLAGS	0x05   /* get the MSM_BO_x flags */
+#define MSM_INFO_SET_METADATA	0x06   /* set userspace metadata */
+#define MSM_INFO_GET_METADATA	0x07   /* get userspace metadata */
 
 struct drm_msm_gem_info {
 	__u32 handle;         /* in */
Signature · 1 line
2.41.0
#1 lore raw
Quoted 48 lines · Rob Clark

From: Rob Clark <[email protected]>

The EXT_external_objects extension is a bit awkward as it doesn't pass explicit modifiers, leaving the importer to guess with incomplete information. In the case of vk (turnip) exporting and gl (freedreno) importing, the "OPTIMAL_TILING_EXT" layout depends on VkImageCreateInfo flags (among other things), which the importer does not know. Which unfortunately leaves us with the need for a metadata back-channel.

The contents of the metadata are defined by userspace. The EXT_external_objects extension is only required to work between compatible versions of gl and vk drivers, as defined by device and driver UUIDs.

Signed-off-by Rob Clark <[email protected]>
---
 drivers/gpu/drm/msm/msm_drv.c | 59 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/msm/msm_gem.h |  4 +++
 include/uapi/drm/msm_drm.h    |  2 ++
 3 files changed, 63 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/msm_drv.c +4 -1
index b61ccea05327..8fe2677ea37a 100644
@@ -37,9 +37,10 @@
  * - 1.9.0 - Add MSM_SUBMIT_FENCE_SN_IN
  * - 1.10.0 - Add MSM_SUBMIT_BO_NO_IMPLICIT
  * - 1.11.0 - Add wait boost (MSM_WAIT_FENCE_BOOST, MSM_PREP_BOOST)
+ * - 1.12.0 - Add MSM_INFO_SET_METADATA and MSM_INFO_GET_METADATA
  */
 #define MSM_VERSION_MAJOR      1
-#define MSM_VERSION_MINOR      10
+#define MSM_VERSION_MINOR      12
 #define MSM_VERSION_PATCHLEVEL 0
 
 static void msm_deinit_vram(struct drm_device *ddev);

@@ -566,6 +567,8 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
                break;
        case MSM_INFO_SET_NAME:
        case MSM_INFO_GET_NAME:
+       case MSM_INFO_SET_METADATA:
+       case MSM_INFO_GET_METADATA:
                break;
        default:
                return -EINVAL;

drivers/gpu/drm/msm/msm_drv.c static int msm_ioctl_gem_info(struct drm_device *dev, void *data, · line 624

Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +1 -1
@@ -618,7 +621,7 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
                break;
        case MSM_INFO_GET_NAME:
                if (args->value && (args->len < strlen(msm_obj->name))) {
-                       ret = -EINVAL;
+                       ret = -ETOOSMALL;

This is unrelated and it also slightly changes user interface, so it IMO should come as a separate commit/

Quoted 46 lines
drivers/gpu/drm/msm/msm_drv.c +39 -0
                        break;
                }
                args->len = strlen(msm_obj->name);

@@ -627,6 +630,58 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
                                         msm_obj->name, args->len))
                                ret = -EFAULT;
                }
+               break;
+       case MSM_INFO_SET_METADATA:
+               /* Impose a moderate upper bound on metadata size: */
+               if (args->len > 128) {
+                       ret = -EOVERFLOW;
+                       break;
+               }
+
+               ret = msm_gem_lock_interruptible(obj);
+               if (ret)
+                       break;
+
+               msm_obj->metadata =
+                       krealloc(msm_obj->metadata, args->len, GFP_KERNEL);
+               msm_obj->metadata_size = args->len;
+
+               if (copy_from_user(msm_obj->metadata, u64_to_user_ptr(args->value),
+                                  args->len)) {
+                       msm_obj->metadata_size = 0;
+                       ret = -EFAULT;
+               }
+
+               msm_gem_unlock(obj);
+
+               break;
+       case MSM_INFO_GET_METADATA:
+               if (!args->value) {
+                       /*
+                        * Querying the size is inherently racey, but
+                        * EXT_external_objects expects the app to confirm
+                        * via device and driver UUIDs that the exporter and
+                        * importer versions match.  All we can do from the
+                        * kernel side is check the length under obj lock
+                        * when userspace tries to retrieve the metadata
+                        */
+                       args->len = msm_obj->metadata_size;
+                       break;
+               }
+

drivers/gpu/drm/msm/msm_drv.c static int msm_ioctl_gem_info(struct drm_device *dev, void *data, · line 681

Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +10 -0
+               ret = msm_gem_lock_interruptible(obj);
+               if (ret)
+                       break;
+
+               if (args->len < msm_obj->metadata_size) {
+                       ret = -ETOOSMALL;
+               } else if (copy_to_user(u64_to_user_ptr(args->value),
+                                       msm_obj->metadata, args->len)) {
+                       ret = -EFAULT;
+               }

Doesn't checkpwatch warn here about extra curly brackets?

Quoted 37 lines
drivers/gpu/drm/msm/msm_drv.c +3 -0
+
+               msm_gem_unlock(obj);
+
                break;
        }
drivers/gpu/drm/msm/msm_gem.h +4 -0
index 7f34263048a3..8d414b072c29 100644
@@ -109,6 +109,10 @@ struct msm_gem_object {
 
        char name[32]; /* Identifier to print for the debugfs files */
 
+       /* userspace metadata backchannel */
+       void *metadata;
+       u32 metadata_size;
+
        /**
         * pin_count: Number of times the pages are pinned
         *
include/uapi/drm/msm_drm.h +2 -1
index 6c34272a13fd..6f2a7ad04aa4 100644
@@ -139,6 +139,8 @@ struct drm_msm_gem_new {
 #define MSM_INFO_GET_NAME      0x03   /* get debug name, returned by pointer */
 #define MSM_INFO_SET_IOVA      0x04   /* set the iova, passed by value */
 #define MSM_INFO_GET_FLAGS     0x05   /* get the MSM_BO_x flags */
+#define MSM_INFO_SET_METADATA  0x06   /* set userspace metadata */
+#define MSM_INFO_GET_METADATA  0x07   /* get userspace metadata */
 
 struct drm_msm_gem_info {
        __u32 handle;         /* in */
--

2.41.0

Signature · 2 lines
With best wishes
Dmitry
author
#2 lore raw
Quoted 12 lines · Dmitry Baryshkov
Quoted 10 lines · Rob Clark

From: Rob Clark <[email protected]>

The EXT_external_objects extension is a bit awkward as it doesn't pass explicit modifiers, leaving the importer to guess with incomplete information. In the case of vk (turnip) exporting and gl (freedreno) importing, the "OPTIMAL_TILING_EXT" layout depends on VkImageCreateInfo flags (among other things), which the importer does not know. Which unfortunately leaves us with the need for a metadata back-channel.

Quoted 4 lines
Quoted 4 lines

The contents of the metadata are defined by userspace. The EXT_external_objects extension is only required to work between compatible versions of gl and vk drivers, as defined by device and driver UUIDs.

jfyi, userspace side of this at:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25945

Quoted 34 lines
Quoted 34 lines
Signed-off-by Rob Clark <[email protected]>
---
 drivers/gpu/drm/msm/msm_drv.c | 59 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/msm/msm_gem.h |  4 +++
 include/uapi/drm/msm_drm.h    |  2 ++
 3 files changed, 63 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/msm_drv.c +4 -1
index b61ccea05327..8fe2677ea37a 100644
@@ -37,9 +37,10 @@
  * - 1.9.0 - Add MSM_SUBMIT_FENCE_SN_IN
  * - 1.10.0 - Add MSM_SUBMIT_BO_NO_IMPLICIT
  * - 1.11.0 - Add wait boost (MSM_WAIT_FENCE_BOOST, MSM_PREP_BOOST)
+ * - 1.12.0 - Add MSM_INFO_SET_METADATA and MSM_INFO_GET_METADATA
  */
 #define MSM_VERSION_MAJOR      1
-#define MSM_VERSION_MINOR      10
+#define MSM_VERSION_MINOR      12
 #define MSM_VERSION_PATCHLEVEL 0
 
 static void msm_deinit_vram(struct drm_device *ddev);

@@ -566,6 +567,8 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
                break;
        case MSM_INFO_SET_NAME:
        case MSM_INFO_GET_NAME:
+       case MSM_INFO_SET_METADATA:
+       case MSM_INFO_GET_METADATA:
                break;
        default:
                return -EINVAL;

@@ -618,7 +621,7 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
Quoted 8 lines

drivers/gpu/drm/msm/msm_drv.c static int msm_ioctl_gem_info(struct drm_device *dev, void *data, · line 624

Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +1 -1
                break;
        case MSM_INFO_GET_NAME:
                if (args->value && (args->len < strlen(msm_obj->name))) {
-                       ret = -EINVAL;
+                       ret = -ETOOSMALL;

This is unrelated and it also slightly changes user interface, so it IMO should come as a separate commit/

fair, although it was changed for consistency with GET_METADATA

Quoted 50 lines
Quoted 50 lines
drivers/gpu/drm/msm/msm_drv.c +43 -0
                        break;
                }
                args->len = strlen(msm_obj->name);

@@ -627,6 +630,58 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
                                         msm_obj->name, args->len))
                                ret = -EFAULT;
                }
+               break;
+       case MSM_INFO_SET_METADATA:
+               /* Impose a moderate upper bound on metadata size: */
+               if (args->len > 128) {
+                       ret = -EOVERFLOW;
+                       break;
+               }
+
+               ret = msm_gem_lock_interruptible(obj);
+               if (ret)
+                       break;
+
+               msm_obj->metadata =
+                       krealloc(msm_obj->metadata, args->len, GFP_KERNEL);
+               msm_obj->metadata_size = args->len;
+
+               if (copy_from_user(msm_obj->metadata, u64_to_user_ptr(args->value),
+                                  args->len)) {
+                       msm_obj->metadata_size = 0;
+                       ret = -EFAULT;
+               }
+
+               msm_gem_unlock(obj);
+
+               break;
+       case MSM_INFO_GET_METADATA:
+               if (!args->value) {
+                       /*
+                        * Querying the size is inherently racey, but
+                        * EXT_external_objects expects the app to confirm
+                        * via device and driver UUIDs that the exporter and
+                        * importer versions match.  All we can do from the
+                        * kernel side is check the length under obj lock
+                        * when userspace tries to retrieve the metadata
+                        */
+                       args->len = msm_obj->metadata_size;
+                       break;
+               }
+
+               ret = msm_gem_lock_interruptible(obj);
+               if (ret)
+                       break;
+
Quoted 8 lines

drivers/gpu/drm/msm/msm_drv.c static int msm_ioctl_gem_info(struct drm_device *dev, void *data, · line 681

Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +6 -0
+               if (args->len < msm_obj->metadata_size) {
+                       ret = -ETOOSMALL;
+               } else if (copy_to_user(u64_to_user_ptr(args->value),
+                                       msm_obj->metadata, args->len)) {
+                       ret = -EFAULT;
+               }

Doesn't checkpwatch warn here about extra curly brackets?

It might.. it is one of the many checkpatch warnings that I dutifully ignore ;-)

BR,
-R

Quoted 42 lines
Quoted 35 lines
drivers/gpu/drm/msm/msm_drv.c +3 -0
+
+               msm_gem_unlock(obj);
+
                break;
        }
drivers/gpu/drm/msm/msm_gem.h +4 -0
index 7f34263048a3..8d414b072c29 100644
@@ -109,6 +109,10 @@ struct msm_gem_object {
 
        char name[32]; /* Identifier to print for the debugfs files */
 
+       /* userspace metadata backchannel */
+       void *metadata;
+       u32 metadata_size;
+
        /**
         * pin_count: Number of times the pages are pinned
         *
include/uapi/drm/msm_drm.h +2 -1
index 6c34272a13fd..6f2a7ad04aa4 100644
@@ -139,6 +139,8 @@ struct drm_msm_gem_new {
 #define MSM_INFO_GET_NAME      0x03   /* get debug name, returned by pointer */
 #define MSM_INFO_SET_IOVA      0x04   /* set the iova, passed by value */
 #define MSM_INFO_GET_FLAGS     0x05   /* get the MSM_BO_x flags */
+#define MSM_INFO_SET_METADATA  0x06   /* set userspace metadata */
+#define MSM_INFO_GET_METADATA  0x07   /* get userspace metadata */
 
 struct drm_msm_gem_info {
        __u32 handle;         /* in */
--
Quoted 2 lines

2.41.0

--
With best wishes
Dmitry

author
#3 lore raw

v2 cover drm/msm/gem: Add metadata uapi

In reply to Rob Clark

From: Rob Clark <[email protected]>

Add metadata mechanism to provide a back-channel to communicate image layout information between vk and gl, because EXT_external_objects doesn't support explicit modifiers and "OPTIMAL_TILING_EXT" is not enough information for the importer to deduce the layout.

Rob Clark (2):
drm/msm: Small uabi fixes
drm/msm/gem: Add metadata

 drivers/gpu/drm/msm/msm_drv.c | 59 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/msm/msm_gem.c |  1 +
 drivers/gpu/drm/msm/msm_gem.h |  4 +++
 include/uapi/drm/msm_drm.h    |  2 ++
 4 files changed, 64 insertions(+), 2 deletions(-)
Signature · 1 line
2.41.0
author
#4 lore raw

PATCH v2 1/2 drm/msm: Small uabi fixes

In reply to Rob Clark

From: Rob Clark <[email protected]>

Correct the minor version exposed and error return value for MSM_INFO_GET_NAME.

Signed-off-by Rob Clark <[email protected]>
---
 drivers/gpu/drm/msm/msm_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/msm_drv.c +2 -2
index 4bd028fa7500..781db689fb16 100644
@@ -51,7 +51,7 @@
  * - 1.11.0 - Add wait boost (MSM_WAIT_FENCE_BOOST, MSM_PREP_BOOST)
  */
 #define MSM_VERSION_MAJOR	1
-#define MSM_VERSION_MINOR	10
+#define MSM_VERSION_MINOR	11
 #define MSM_VERSION_PATCHLEVEL	0
 
 static void msm_deinit_vram(struct drm_device *ddev);

@@ -896,7 +896,7 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 		break;
 	case MSM_INFO_GET_NAME:
 		if (args->value && (args->len < strlen(msm_obj->name))) {
-			ret = -EINVAL;
+			ret = -ETOOSMALL;
 			break;
 		}
 		args->len = strlen(msm_obj->name);
Signature · 1 line
2.41.0
author
#6 lore raw

PATCH v2 2/2 drm/msm/gem: Add metadata

In reply to Rob Clark

From: Rob Clark <[email protected]>

The EXT_external_objects extension is a bit awkward as it doesn't pass explicit modifiers, leaving the importer to guess with incomplete information. In the case of vk (turnip) exporting and gl (freedreno) importing, the "OPTIMAL_TILING_EXT" layout depends on VkImageCreateInfo flags (among other things), which the importer does not know. Which unfortunately leaves us with the need for a metadata back-channel.

The contents of the metadata are defined by userspace. The EXT_external_objects extension is only required to work between compatible versions of gl and vk drivers, as defined by device and driver UUIDs.

v2: add missing metadata kfree

Signed-off-by Rob Clark <[email protected]>
---
 drivers/gpu/drm/msm/msm_drv.c | 57 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/msm/msm_gem.c |  1 +
 drivers/gpu/drm/msm/msm_gem.h |  4 +++
 include/uapi/drm/msm_drm.h    |  2 ++
 4 files changed, 63 insertions(+), 1 deletion(-)
drivers/gpu/drm/msm/msm_drv.c +56 -1
index 781db689fb16..9ec74ab4cfea 100644
@@ -49,9 +49,10 @@
  * - 1.9.0 - Add MSM_SUBMIT_FENCE_SN_IN
  * - 1.10.0 - Add MSM_SUBMIT_BO_NO_IMPLICIT
  * - 1.11.0 - Add wait boost (MSM_WAIT_FENCE_BOOST, MSM_PREP_BOOST)
+ * - 1.12.0 - Add MSM_INFO_SET_METADATA and MSM_INFO_GET_METADATA
  */
 #define MSM_VERSION_MAJOR	1
-#define MSM_VERSION_MINOR	11
+#define MSM_VERSION_MINOR	12
 #define MSM_VERSION_PATCHLEVEL	0
 
 static void msm_deinit_vram(struct drm_device *ddev);

@@ -844,6 +845,8 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 		break;
 	case MSM_INFO_SET_NAME:
 	case MSM_INFO_GET_NAME:
+	case MSM_INFO_SET_METADATA:
+	case MSM_INFO_GET_METADATA:
 		break;
 	default:
 		return -EINVAL;

@@ -905,6 +908,58 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 					 msm_obj->name, args->len))
 				ret = -EFAULT;
 		}
+		break;
+	case MSM_INFO_SET_METADATA:
+		/* Impose a moderate upper bound on metadata size: */
+		if (args->len > 128) {
+			ret = -EOVERFLOW;
+			break;
+		}
+
+		ret = msm_gem_lock_interruptible(obj);
+		if (ret)
+			break;
+
+		msm_obj->metadata =
+			krealloc(msm_obj->metadata, args->len, GFP_KERNEL);
+		msm_obj->metadata_size = args->len;
+
+		if (copy_from_user(msm_obj->metadata, u64_to_user_ptr(args->value),
+				   args->len)) {
+			msm_obj->metadata_size = 0;
+			ret = -EFAULT;
+		}
+
+		msm_gem_unlock(obj);
+
+		break;
+	case MSM_INFO_GET_METADATA:
+		if (!args->value) {
+			/*
+			 * Querying the size is inherently racey, but
+			 * EXT_external_objects expects the app to confirm
+			 * via device and driver UUIDs that the exporter and
+			 * importer versions match.  All we can do from the
+			 * kernel side is check the length under obj lock
+			 * when userspace tries to retrieve the metadata
+			 */
+			args->len = msm_obj->metadata_size;
+			break;
+		}
+
+		ret = msm_gem_lock_interruptible(obj);
+		if (ret)
+			break;
+
+		if (args->len < msm_obj->metadata_size) {
+			ret = -ETOOSMALL;
+		} else if (copy_to_user(u64_to_user_ptr(args->value),
+					msm_obj->metadata, args->len)) {
+			ret = -EFAULT;
+		}
+
+		msm_gem_unlock(obj);
+
 		break;
 	}
drivers/gpu/drm/msm/msm_gem.c +1 -0
index 1113e6b2ec8e..175ee4ab8a6f 100644
@@ -1058,6 +1058,7 @@ static void msm_gem_free_object(struct drm_gem_object *obj)
 
 	drm_gem_object_release(obj);
 
+	kfree(msm_obj->metadata);
 	kfree(msm_obj);
 }
drivers/gpu/drm/msm/msm_gem.h +4 -0
index 7f34263048a3..8d414b072c29 100644
@@ -109,6 +109,10 @@ struct msm_gem_object {
 
 	char name[32]; /* Identifier to print for the debugfs files */
 
+	/* userspace metadata backchannel */
+	void *metadata;
+	u32 metadata_size;
+
 	/**
 	 * pin_count: Number of times the pages are pinned
 	 *
include/uapi/drm/msm_drm.h +2 -0
index 6c34272a13fd..6f2a7ad04aa4 100644
@@ -139,6 +139,8 @@ struct drm_msm_gem_new {
 #define MSM_INFO_GET_NAME	0x03   /* get debug name, returned by pointer */
 #define MSM_INFO_SET_IOVA	0x04   /* set the iova, passed by value */
 #define MSM_INFO_GET_FLAGS	0x05   /* get the MSM_BO_x flags */
+#define MSM_INFO_SET_METADATA	0x06   /* set userspace metadata */
+#define MSM_INFO_GET_METADATA	0x07   /* get userspace metadata */
 
 struct drm_msm_gem_info {
 	__u32 handle;         /* in */
Signature · 1 line
2.41.0
Reviewed-by on v2 2/2
#7 lore raw
Quoted 10 lines · Rob Clark

From: Rob Clark <[email protected]>

The EXT_external_objects extension is a bit awkward as it doesn't pass explicit modifiers, leaving the importer to guess with incomplete information. In the case of vk (turnip) exporting and gl (freedreno) importing, the "OPTIMAL_TILING_EXT" layout depends on VkImageCreateInfo flags (among other things), which the importer does not know. Which unfortunately leaves us with the need for a metadata back-channel.

Quoted 8 lines

The contents of the metadata are defined by userspace. The EXT_external_objects extension is only required to work between compatible versions of gl and vk drivers, as defined by device and driver UUIDs.

v2: add missing metadata kfree

Signed-off-by Rob Clark <[email protected]>
Reviewed-by Dmitry Baryshkov <[email protected]>
Quoted 6 lines
---
 drivers/gpu/drm/msm/msm_drv.c | 57 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/msm/msm_gem.c |  1 +
 drivers/gpu/drm/msm/msm_gem.h |  4 +++
 include/uapi/drm/msm_drm.h    |  2 ++
 4 files changed, 63 insertions(+), 1 deletion(-)
Signature · 2 lines
With best wishes
Dmitry