[Mesa-dev] [PATCH v2 24/37] panfrost: Cache GPU accesses to BOs

[Alyssa Rosenzweig]

> > > +                /* If ->gpu_access is 0, the BO is idle, and if the WRITE flag
> > > +                 * is cleared, that means we only have readers.
> > > +                 */
> > > +                if (!bo->gpu_access)
> > > +                        return true;
> > > +                else if (!(access_type & PAN_BO_GPU_ACCESS_READ) &&
> > > +                         !(bo->gpu_access & PAN_BO_GPU_ACCESS_WRITE))
> > > +                        return true;  
> > 
> > The second condition is a little confusing, though I think it's correct.
> > Not sure if there's any way to clarify what's meant but just thought I'd
> > comment, since inevitably future readers will squint too.
> 
> I can do:
> 
> 		/* If ->gpu_access is 0, the BO is idle, no need to wait. */
> 		if (!bo->gpu_access)
> 			return true;
> 
> 		/* If the caller only wants to wait for writers and no
> 		 * writes are pending, we don't have to wait.
> 		 */
> 		if (access_type == PAN_BO_GPU_ACCESS_WRITE &&
> 		    !(bo->gpu_access & PAN_BO_GPU_ACCESS_WRITE))
> 			return true;
> 
> instead.

Perfect, thank you :)

> > > +                /* Update the BO access flags so that panfrost_bo_wait() knows
> > > +                 * about all pending accesses.
> > > +                 */
> > > +                bo->gpu_access |= flags & (PAN_BO_GPU_ACCESS_RW);  
> > 
> > This looks like black magic. Maybe just clarify in the comment why this
> > & is reasonable (relying on bit mask magic).
> 
> It's just here to clear all non-RW flags (we only care about the read/write
> information when it comes to BO idleness). I'll add a comment to explain that
> part, and maybe another one to explain why we have a '|=' and not just '='.

Yeah, I figured it out. Mostly, REing means we have so much unexplained
bit magic in the codebase already, we don't need to add more if we can
help it :) Hence the comment would be appreciated.

> > That aside, as I mentioned it would maybe make more sense to squash this
> > into the patch introduce the bo_wait ioctl() in the first place? If
> > that's too complicated with merge conflicts and stuff, don't sweat it,
> > though :)
> 
> I'm fine with that, I'll re-order things to avoid introducing the bo_wait()
> infra before we have the access type info.

+1