Re: [PATCH v3 5/7] drm/panfrost: Add a new ioctl to submit batches

[Boris Brezillon <boris.brezillon@collabora.com>]

Hi Steven,

On Mon, 5 Jul 2021 09:22:39 +0100
Steven Price <steven.price@arm.com> wrote:

> On 02/07/2021 19:11, Boris Brezillon wrote:
> > On Fri, 2 Jul 2021 12:49:55 -0400
> > Alyssa Rosenzweig <alyssa@collabora.com> wrote:
> >   
> >>>> ```    
> >>>>>  #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.    
> >>
> >> I don't have strong feelings either way. I was just under the
> >> impressions other drivers did this for b/w compat reasons which don't
> >> apply here.  
> > 
> > Okay, I think I'll keep it like that unless there's a strong reason to
> > make no-implicit dep the default. It's safer to oversync than the skip
> > the synchronization, so it does feel like something the user should
> > explicitly enable.  
> 
> I don't have strong feelings - ultimately the number of projects caring
> about the uABI is so limited the "default" is pretty irrelevant (it's
> not as if we are needing to guide random developers who are new to the
> interface). But a conservative default seems sensible.
> 
> >>  
> >>>> 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.    
> >>
> >> Cc Steven, does this make sense?  
> > 
> > I have this approach working, and I must admit I prefer it to the
> > per-object stride field passed to the submit struct.
> >   
> 
> There are benefits both ways:
> 
>  * Version number: easier to think about, and less worries about
> combinatorial explosion of possible options to test.
> 
>  * Explicit structure sizes/strides: much harder to accidentally forgot
> to change, clients 'naturally' move to newer versions just with recompiling.

The version I just sent has a PANFROST_SUBMIT_BATCH_VERSION macro
defined in the the uAPI header, so getting right without changing the
code should be fine (same has with the sizeof(struct xx)) trick with
the per-desc stride approach).

> 
> For now I'd be tempted to go for the version number, but I suspect we
> should also ensure there's a generic 'flags' field in there. That would
> allow us to introduce new features/behaviours in a way which can be
> backported more easily if necessary.

Adding features at the submit level without changing the version number
is already possible (we can extend drm_panfrost_batch_submit without
breaking the uABI), but I'm not sure that's a good idea...

If you mean adding a flags field at the job level, I can add it, but I
wonder what you have in mind when you say some features might be
interesting to backport. I really thought we'd force people to update
their kernel when they want those new features.

Regards,

Boris