```
> +/* 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/

Reply via email to