On Thursday, June 15, 2017 1:41:39 AM PDT Chris Wilson wrote: > Quoting Kenneth Graunke (2017-06-14 22:49:01) > > On Friday, June 9, 2017 6:01:33 AM PDT Chris Wilson wrote: > > > If we know the bo is idle (that is we have no submitted a command buffer > > > referencing this bo since the last query) we can skip asking the kernel. > > > Note this may report a false negative if the target is being shared > > > between processes (exported via dmabuf or flink). To allow the caller > > > control over using the last known flag, the query is split into two. > > > > I'm not crazy about exposing __brw_bo_busy and brw_bo_busy, with slightly > > different semantics. Why not just make brw_bo_busy do: > > > > if (bo->idle && bo->reusable) > > return false; > > > > /* otherwise query the kernel */ > > > > These days, it appears that bo->reusable is false for any buffers that > > have been imported/exported via dmabuf or flink, and true otherwise. > > (We might want to rename it to bo->foreign or such.) > > I set bo->reusable to false on snooped buffers as we don't yet handle > mixed modes in the bo cache. To offset that I was thinking of having a > specific cache for qbo. We do have flags we pass to bo_alloc, so > supporting a cache of snoopable, or even uncached on llc, within bufmgr > isn't hard. > > An alternative is to have bo->exported, which is useful if we ever want > to disregard our state tracking.
Imported is a problem too, right? Whoever gave us the buffer could still
do things with it, and we wouldn't be able to track that.
That's why I was thinking bo->foreign, bo->external, bo->shared, or the
like. Adding a new flag seems reasonable - bo->reusable isn't quite what
we want.
> > With that change, brw_bo_busy should bypass the ioctl for most BOs, but
> > would still work for foreign BOs, without the caller having to worry
> > about it.
>
> Just not qbo right now :)
Good point. Adding a new flag would be better.
> > > /**
> > > - * Returns 1 if mapping the buffer for write could cause the process
> > > - * to block, due to the object being active in the GPU.
> > > + * Returns 0 if mapping the buffer is not in active use by the gpu.
> > > + * If non-zero, any mapping for for write could cause the process
> > > + * to block, due to the object being active in the GPU. If the lower
> > > + * 16 bits are zero, then we can map for read without stalling.
> > > + *
> > > + * The last-known busy status of the brw_bo is checked first. This may be
> > > + * stale if the brw_bo has been exported to a foriegn process. If used
> > > on an
> > > + * exported bo, call __brw_bo_busy() directly to bypass the local check.
> > > */
> > > -int brw_bo_busy(struct brw_bo *bo);
> > > +static inline int brw_bo_busy(struct brw_bo *bo)
> > > +{
> > > + if (bo->idle) /* Note this may be stale if the bo is exported */
> > > + return 0;
> > > +
> > > + return __brw_bo_busy(bo);
> > > +}
> >
> > I'd rather keep this as a boolean result, rather than an integer with
> > certain bits having particular meanings. Bonus points for changing the
> > return type to "bool".
>
> The different bits are significant, especially in the case where you do
> want to distinguish ready to read vs ready to write. Such as reading
> back a qbo that might also be still in use for GPU queries.
> -Chris
Right, so different bits are how the kernel exposes that...but IMO we
should just have two boolean functions, one for "ready to read" and one
for "ready to write"...
I sort of thought that's what you were doing. Or do we need three?
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
