On Fri, 2 Jul 2021 11:13:16 -0400
Alyssa Rosenzweig <alyssa@xxxxxxxxxxxxx> wrote:
> ```
> > +/* 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?
Handle is a syncobj handle, point is the point in a syncobj timeline.
I'll document those fields.
> Why is there padding instead of putting point first?
We can move the point field first, but we need to keep the explicit
padding: the struct has to be 64bit aligned because of the __u64 field
(which the compiler takes care of) but if we don't have an explicit
padding, the unused 32bits are undefined, which might cause trouble if
we extend the struct at some point, since we sort of expect that old
userspace keep this unused 32bit slot to 0, while new users set
non-zero values if they have to.
>
> ```
> > #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?
AFAICT, all other drivers make the no-implicit-dep an opt-in, and I
didn't want to do things differently in panfrost. But if that's really
an issue, I can make it an opt-out.
>
> ```
> > + /**
> > + * 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)
Sounds like a good idea. I'll add a version field and map that
to a <job_stride,bo_ref_stride,syncobj_ref_stride> tuple.
>
> ```
> > + /**
> > + * If the submission fails, this encodes the index of the job
> > + * failed.
> > + */
> > + __u32 fail_idx;
> ```
>
> What if multiple jobs fail?
We stop at the first failure. Note that it's not an execution failure,
but a submission failure (AKA, userspace passed wrong params, like
invalid BO or synobj handles).
