[Mesa-dev] [PATCH 2/9] i965: Check last known busy status on bo before asking the kernel

[Chris Wilson]

Quoting Kenneth Graunke (2017-06-15 19:45:19)
> 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.

Yes, import/export both are exposed to others. I have used foreign as
the flag name before, but I always misspell it so I avoid it when
picking names now. bo->external is quite sensible.
> 
> > > 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?

	static int __brw_bo_busy();
	bool brw_bo_read_busy();
	bool brw_bo_write_busy();

is ok. Not thrilled about read_busy or write_busy.

	if (brw_bo_ready_to_read()) 
	if (brw_bo_ready_to_write()

reads ok, but "ready" is quite vague.

brw_bo_busy(, .flags = MAP_WRITE | MAP_READ) ?
brw_bo_map_busy ?

Hmm, brw_bo_map_busy (brw_bo_map_is_busy?) seems quite clear. But keep
brw_bo_busy() for the interim as it used quite extensively atm outside
of mmapping. (A brw_batch_busy() would remove most of those :)

So something like:

static unsigned __brw_bo_busy(struct brw_bo *bo)
{
	struct drm_i915_gem_busy busy = { bo->gem_handle };

	if (bo->idle && !bo->external)
		return 0;

	if (__sys_ioctl(bo->bufmgr->fd, DRM_IOCTL_I915_GEM_BUSY, &busy))
		return -1;

	bo->idle = !busy.busy;
	return busy.busy;
}

bool brw_bo_busy(strut brw_bo *bo)
{
	return __brw_bo_busy(bo);
}

bool brw_bo_map_busy(strut brw_bo *bo, unsigned flags)
{
	unsigned mask;

	if (!(flags & MAP_WRITE))
		mask = 0xffff;
	else
		mask = ~0u;

	return __brw_bo_busy(bo) & mask;
}