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

[Boris Brezillon]

On Mon, 16 Sep 2019 10:05:52 -0400
Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:

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

> 
> > +                /* 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 '='.

> 
> ---
> 
> 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.