Re: [PATCH 2/3] drm/v3d: add generic ioctl extension

On Wed, Aug 18, 2021 at 06:56:41PM +0100, Melissa Wen wrote:

Add support to attach generic extensions on job submission. This patch is a second prep work to enable multiple syncobjs on job submission. With this work, when the job submission interface needs to be extended to accomodate a new feature, we will use a generic extension struct where an id determines the data type to be pointed. The first application is to enable multiples in/out syncobj (next patch), but the base is already done for future features.

Signed-off-by: Melissa Wen mwen@igalia.com

drivers/gpu/drm/v3d/v3d_drv.c | 4 +- drivers/gpu/drm/v3d/v3d_gem.c | 80 ++++++++++++++++++++++++++++++++--- include/uapi/drm/v3d_drm.h | 38 ++++++++++++++++- 3 files changed, 113 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index 9403c3b36aca..6a0516160bb2 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.c +++ b/drivers/gpu/drm/v3d/v3d_drv.c @@ -83,7 +83,6 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void *data, return 0; }

• switch (args->param) { case DRM_V3D_PARAM_SUPPORTS_TFU: args->value = 1;

@@ -147,7 +146,7 @@ v3d_postclose(struct drm_device *dev, struct drm_file *file) DEFINE_DRM_GEM_FOPS(v3d_drm_fops);

/* DRM_AUTH is required on SUBMIT_CL for now, while we don't have GMP

• • protection between clients. Note that render nodes would be be

• • protection between clients. Note that render nodes would be
  • able to submit CLs that could access BOs from clients authenticated
  • with the master node. The TFU doesn't use the GMP, so it would
  • need to stay DRM_AUTH until we do buffer size/offset validation.

@@ -222,7 +221,6 @@ static int v3d_platform_drm_probe(struct platform_device *pdev) u32 mmu_debug; u32 ident1;

• v3d = devm_drm_dev_alloc(dev, &v3d_drm_driver, struct v3d_dev, drm); if (IS_ERR(v3d)) return PTR_ERR(v3d);

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 593ed2206d74..e254919b6c5e 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -521,6 +521,38 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv, } }

+static int +v3d_get_extensions(struct drm_file *file_priv,

•    u32 ext_count, u64 ext_handles)
  

+{

• int i;
• struct drm_v3d_extension __user *handles;
• if (!ext_count)

• return 0;
  

• handles = u64_to_user_ptr(ext_handles);
• for (i = 0; i < ext_count; i++) {

• struct drm_v3d_extension ext;
  

• if (copy_from_user(&ext, handles, sizeof(ext))) {
  

• 	DRM_DEBUG("Failed to copy submit extension\n");
  

• 	return -EFAULT;
  

• }
  

• switch (ext.id) {
  

• case 0:
  

• default:
  

• 	DRM_DEBUG_DRIVER("Unknown extension id: %d\n", ext.id);
  

• 	return -EINVAL;
  

• }
  

• handles = u64_to_user_ptr(ext.next);
  

• }
• return 0;

+}

/**

• v3d_submit_cl_ioctl() - Submits a job (frame) to the V3D.
• @dev: DRM device

@@ -549,15 +581,23 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,

trace_v3d_submit_cl_ioctl(&v3d->drm, args->rcl_start, args->rcl_end);

• if (args->pad != 0)

• return -EINVAL;
  

• if (args->flags != 0 &&

•    args->flags != DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
  

• if (args->flags &&

•    args->flags & ~(DRM_V3D_SUBMIT_CL_FLUSH_CACHE |
  

• 	    DRM_V3D_SUBMIT_EXTENSION)) {
  

  DRM_INFO("invalid flags: %d\n", args->flags); return -EINVAL; }

• if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {

• ret = v3d_get_extensions(file_priv,
  

• 			 args->extension_count,
  

• 			 args->extensions);
  

• if (ret) {
  

• 	DRM_DEBUG("Failed to get extensions.\n");
  

• 	return ret;
  

• }
  

• }

• render = kcalloc(1, sizeof(*render), GFP_KERNEL); if (!render) return -ENOMEM;

@@ -711,6 +751,21 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,

trace_v3d_submit_tfu_ioctl(&v3d->drm, args->iia);

• if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) {

• DRM_DEBUG("invalid flags: %d\n", args->flags);
  

• return -EINVAL;
  

• }
• if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {

• ret = v3d_get_extensions(file_priv,
  

• 			 args->extension_count,
  

• 			 args->extensions);
  

• if (ret) {
  

• 	DRM_DEBUG("Failed to get extensions.\n");
  

• 	return ret;
  

• }
  

• }
• job = kcalloc(1, sizeof(*job), GFP_KERNEL); if (!job) return -ENOMEM;

@@ -806,6 +861,21 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, return -EINVAL; }

• if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) {

• DRM_DEBUG("Invalid flags: %d\n", args->flags);
  

• return -EINVAL;
  

• }
• if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {

• ret = v3d_get_extensions(file_priv,
  

• 			 args->extension_count,
  

• 			 args->extensions);
  

• if (ret) {
  

• 	DRM_DEBUG("Failed to get extensions.\n");
  

• 	return ret;
  

• }
  

• }
• job = kcalloc(1, sizeof(*job), GFP_KERNEL); if (!job) return -ENOMEM;

diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h index 4104f22fb3d3..1f4706010eb5 100644 --- a/include/uapi/drm/v3d_drm.h +++ b/include/uapi/drm/v3d_drm.h @@ -58,6 +58,19 @@ extern "C" { struct drm_v3d_perfmon_get_values)

#define DRM_V3D_SUBMIT_CL_FLUSH_CACHE 0x01 +#define DRM_V3D_SUBMIT_EXTENSION 0x02

+/* struct drm_v3d_extension - ioctl extensions

• • Linked-list of generic extensions where the id identify which struct is
• • pointed by ext_data. Therefore, DRM_V3D_EXT_ID_* is used on id to identify
• • the extension type.
• */

+struct drm_v3d_extension {

• __u64 next;

Why do you both need a next pointer here and extension_count everywhere? That seems one too much.

• __u64 ext_data;

This isn't needed if you link them. Instead each extension can subclass this struct here, and add whatever parameter they need there. Some extensions could be just a flag which only needs to be the extension present. Maybe what you want here is a __u32 for flags? Solves also the aligning.

• __u32 id;

Align to 64bit just to be save.

One thing I wondered is whether we shouldn't lift this to be a drm thing. i915 has something similar with i915_user_extension.

That way we could share some helpers for parsing these, and people would do extensible drm ioctls all the same way? -Daniel

+};

/**

• struct drm_v3d_submit_cl - ioctl argument for submitting commands to the 3D

@@ -135,12 +148,17 @@ struct drm_v3d_submit_cl { /* Number of BO handles passed in (size is that times 4). */ __u32 bo_handle_count;

• /* DRM_V3D_SUBMIT_* properties */ __u32 flags;

  /* ID of the perfmon to attach to this job. 0 means no perfmon. */ __u32 perfmon_id;

• __u32 pad;

• /* Number of extensions*/
• __u32 extension_count;
• /* Pointer to a list of ioctl extensions*/
• __u64 extensions;

};

/** @@ -248,6 +266,15 @@ struct drm_v3d_submit_tfu { __u32 in_sync; /* Sync object to signal when the TFU job is done. */ __u32 out_sync;

• /* Number of extensions*/
• __u32 extension_count;
• /* Pointer to an array of ioctl extensions*/
• __u64 extensions;
• /* DRM_V3D_SUBMIT_* properties */
• __u32 flags;

};

/* Submits a compute shader for dispatch. This job will block on any @@ -276,6 +303,15 @@ struct drm_v3d_submit_csd {

/* ID of the perfmon to attach to this job. 0 means no perfmon. */ __u32 perfmon_id;

• /* DRM_V3D_SUBMIT_* properties */
• __u32 flags;
• /* Number of extensions*/
• __u32 extension_count;
• /* Pointer to a list of ioctl extensions*/
• __u64 extensions;

};

enum {

2.30.2