Re: [PATCH] drm/i915: report all active objects as busy v2

From: Eric Anholt <eric@anholt.net>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: report all active objects as busy v2
Date: Sun, 02 May 2010 11:13:18 -0700	[thread overview]
Message-ID: <87bpcydvep.fsf@pollan.anholt.net> (raw)
In-Reply-To: <1272025959-2331-1-git-send-email-daniel.vetter@ffwll.ch>

[-- Attachment #1.1: Type: text/plain, Size: 4070 bytes --]

On Fri, 23 Apr 2010 14:32:39 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Reporting objects that are not currently used by the gpu but are
> dirty and not yet flushed creates havoc to libdrm's bo reuse:
> The usually immediatly following gtt_map afterwards will stall
> until the gpu has executed the writeback.
> 
> v2: Fix refcount leak and missing unlock in the error path. Spotted
> by Chris Wilson.

I've thought about writing this patch several times.  Here's my concern.
Say I drew to an FBO (render cache dirty) temporarily then freed it.  It
goes into the bo reuse list in userland, and is on the flushing list in
kernel.  Now I start doing a bunch of texturing -- allocating and
deleting objects, only flushing the texture cache.  When I go to
allocate, I see that that old FBO object is busy, and allocate new
instead of using cache.  When I free, I put my thing at the end of the
list.  Memory usage goes up and up and up.

Also, note that that ioctl says it's trying to implement OpenGL
semantics.  Please don't change its behavior and break those semantics.

It seems like the problem here is that userland doesn't know the
active/flushing distinction, and if it did it could manage its lists
almost in parallel to the kernel's list and reuse the right BOs at the
right times.  So maybe make a new busy ioctl that returns both active
and flushing booleans?  Then libdrm can be smart and OpenGL can be
happy.

(Actually, OpenGL would be happier if the thing it was asking for
results on got off the flushing list sooner please.  It's trying to get
the results of PIPE_CONTROL writes, and right now we're faking tracking
that using GEM_DOMAIN_INSTRUCTION as the write domain).

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   30 +++++++++++++++++++++---------
>  1 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 023f4db..eedc456 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4379,6 +4379,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	struct drm_i915_gem_busy *args = data;
>  	struct drm_gem_object *obj;
>  	struct drm_i915_gem_object *obj_priv;
> +	int ret = 0;
>  
>  	obj = drm_gem_object_lookup(dev, file_priv, args->handle);
>  	if (obj == NULL) {
> @@ -4396,18 +4397,29 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	i915_gem_retire_requests(dev);
>  
>  	obj_priv = to_intel_bo(obj);
> -	/* Don't count being on the flushing list against the object being
> -	 * done.  Otherwise, a buffer left on the flushing list but not getting
> -	 * flushed (because nobody's flushing that domain) won't ever return
> -	 * unbusy and get reused by libdrm's bo cache.  The other expected
> -	 * consumer of this interface, OpenGL's occlusion queries, also specs
> -	 * that the objects get unbusy "eventually" without any interference.
> -	 */
> -	args->busy = obj_priv->active && obj_priv->last_rendering_seqno != 0;
> +
> +	/* Count all active objects as busy, even if they are currently not used
> +	 * by the gpu. Users of this interface expect objects to eventually
> +	 * become non-busy without any further actions, therefore emit any
> +	 * necessary flushes here. */
> +	args->busy = obj_priv->active;
> +
> +	/* Unconditionally flush objects, even when the gpu still uses this
> +	 * object. Userspace calling this function indicates that it wants to
> +	 * use this buffer rather sooner than later, so issuing the required
> +	 * flush earlier is beneficial. */
> +	if (obj->write_domain) {
> +		uint32_t seqno;
> +
> +		i915_gem_flush(dev, 0, obj->write_domain);
> +		seqno = i915_add_request(dev, file_priv, obj->write_domain);
> +		if (seqno == 0)
> +			ret = -ENOMEM;
> +	}
>  
>  	drm_gem_object_unreference(obj);
>  	mutex_unlock(&dev->struct_mutex);
> -	return 0;
> +	return ret;
>  }
>  
>  int
> -- 
> 1.6.6.1
> 

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx