| Message ID | d5647f22567a71e5ecd3f86f0110aa14f8c6006d.1629307160.git.mwen@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/v3d: add multiple in/out syncobjs support | expand |
On Wed, 2021-08-18 at 18:56 +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; > + __u64 ext_data; > + __u32 id; > +}; > > /** > * 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; A silly nit: maybe put flags before the extension fields above for consistency with the CSD and CL submission commands. > }; > > /* 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 {
On 09/15, Iago Toral wrote: > On Wed, 2021-08-18 at 18:56 +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; > > + __u64 ext_data; > > + __u32 id; > > +}; > > > > /** > > * 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; > > A silly nit: maybe put flags before the extension fields above for > consistency with the CSD and CL submission commands. hmm.. I arranged it that way for alignment reasons (afaiu), but I can (or should) include a _u32 pad right after out_sync to set these in the same sequence. > > > }; > > > > /* 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 { >
On Wed, 2021-09-15 at 17:28 +0100, Melissa Wen wrote: > On 09/15, Iago Toral wrote: > > On Wed, 2021-08-18 at 18:56 +0100, Melissa Wen wrote: (...) > > > > > > /** > > > @@ -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; > > > > A silly nit: maybe put flags before the extension fields above for > > consistency with the CSD and CL submission commands. > > hmm.. I arranged it that way for alignment reasons (afaiu), but I can > (or should) include a _u32 pad right after out_sync to set these in > the > same sequence. Ah, that's fine, my suggestion was just for style, let's keep it as is. Iago
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 >
On 09/16, Daniel Vetter wrote: > 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. Hi Daniel, Thanks for suggesting these improvements. I'll polish it for the next version. > > 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? I think so. I've based on solutions from i915 and amd drivers for ioctl extension. The motivations presented in the commit message for i915_user_extension (vulkan) are similar to ours, and maybe being in drm will make it simpler for the drivers to adopt too. Melissa > -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 > > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
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; + __u64 ext_data; + __u32 id; +}; /** * 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 {
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(-)