On Mon, 16 Sep 2019 10:05:52 -0400
Alyssa Rosenzweig <aly...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
