[PATCH v3 5/7] drm/panfrost: Add a new ioctl to submit batches
Alyssa Rosenzweig
alyssa at collabora.com
Fri Jul 2 15:13:16 UTC 2021
```
> +/* Syncobj reference passed at job submission time to encode explicit
> + * input/output fences.
> + */
> +struct drm_panfrost_syncobj_ref {
> + __u32 handle;
> + __u32 pad;
> + __u64 point;
> +};
```
What is handle? What is point? Why is there padding instead of putting
point first?
```
> #define PANFROST_BO_REF_EXCLUSIVE 0x1
> +#define PANFROST_BO_REF_NO_IMPLICIT_DEP 0x2
```
This seems logically backwards. NO_IMPLICIT_DEP makes sense if we're
trying to keep backwards compatibility, but here you're crafting a new
interface totally from scratch. If anything, isn't BO_REF_IMPLICIT_DEP
the flag you'd want?
```
> + /**
> + * Stride of the jobs array (needed to ease extension of the
> + * BATCH_SUBMIT ioctl). Should be set to
> + * sizeof(struct drm_panfrost_job).
> + */
> + __u32 job_stride;
...
> + /**
> + * Stride of the BO and syncobj reference arrays (needed to ease
> + * extension of the BATCH_SUBMIT ioctl). Should be set to
> + * sizeof(struct drm_panfrost_bo_ref).
> + */
> + __u32 bo_ref_stride;
> + __u32 syncobj_ref_stride;
```
Hmm. I'm not /opposed/ and I know kbase uses strides but it seems like
somewhat unwarranted complexity, and there is a combinatoric explosion
here (if jobs, bo refs, and syncobj refs use 3 different versions, as
this encoding permits... as opposed to just specifying a UABI version or
something like that)
```
> + /**
> + * If the submission fails, this encodes the index of the job
> + * failed.
> + */
> + __u32 fail_idx;
```
What if multiple jobs fail?
```
> + /**
> + * ID of the queue to submit those jobs to. 0 is the default
> + * submit queue and should always exists. If you need a dedicated
> + * queue, create it with DRM_IOCTL_PANFROST_CREATE_SUBMITQUEUE.
> + */
> + __u32 queue;
```
s/exists/exist/
More information about the dri-devel
mailing list