Re: [PATCH v2 4/4] drm/panthor: Fix an off-by-one in the heap context retrieval logic

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

On Thu, 2 May 2024 16:36:02 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu, 2 May 2024 15:26:55 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
> > On 02/05/2024 15:15, Boris Brezillon wrote:  
> > > On Thu, 2 May 2024 15:03:51 +0100
> > > Steven Price <steven.price@arm.com> wrote:
> > >     
> > >> On 30/04/2024 12:28, Boris Brezillon wrote:    
> > >>> ID 0 is reserved to encode 'no-tiler-heap', the heap ID range is
> > >>> [1:MAX_HEAPS_PER_POOL], which we occasionally need to turn into an index
> > >>> in the [0:MAX_HEAPS_PER_POOL-1] when we want to access the context object.      
> > >>
> > >> This might be a silly question, but do we need ID 0 to be
> > >> "no-tiler-heap"? Would it be easier to e.g. use a negative number for
> > >> that situation and avoid all the off-by-one problems?
> > >>
> > >> I'm struggling to find the code which needs the 0 value to be special -
> > >> where is it exactly that we encode this "no-tiler-heap" value?    
> > > 
> > > Hm, I thought we were passing the heap handle to the group creation
> > > ioctl, but heap queue/heap association is actually done through a CS
> > > instruction, so I guess you have a point. The only thing that makes a
> > > bit hesitant is that handle=0 is reserved for all other kind of handles
> > > we return, and I think I'd prefer to keep it the same for heap handles.
> > > 
> > > This being said, we could do the `+- 1` in
> > > panthor_ioctl_tiler_heap_{create,destroy}() to keep things simple in
> > > panthor_heap.c.    
> > 
> > The heap handles returned to user space have the upper 16 bits encoding
> > the VM ID - so hopefully no one is doing anything crazy and splitting it
> > up to treat the lower part specially. And (unless I'm mistaken) the VM
> > IDs start from 1 so we'd still not have IDs of 0. So I don't think we
> > need the +- 1 part anywhere for tiler heaps.  
> 
> Ah, I forgot about that too. Guess we're all good with a
> [0,MAX_HEAPS_PER_POOL-1] range then.
> 
> > 
> > I'd certainly consider it a user space bug to treat the handles as
> > anything other than opaque. Really user space shouldn't be treating 0 as
> > special either: the uAPI doesn't say it's not valid. But I'd be open to
> > updating the uAPI to say 0 is invalid if there's some desire for that.  
> 
> Will do that in v3 then.

Taking that back. I don't think it needs to be enforced in the uAPI. As
you said, it's supposed to be opaque, so I'm tempted to update the
drm_panthor_tiler_heap_destroy::handle kerneldoc saying it must be
a valid handle returned by DRM_IOCTL_PANTHOR_TILER_HEAP_CREATE instead.

It's just that making the handle non-zero is kinda nice for debugging
purposes, and as I said, this way it's consistent with other kind of
handles (GEMs, VMs, syncobjs, ...).