Re: [PATCH 4/4 v3] drm/i915: miscellaneous tiny tweaks to GEM object->dirty

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/4 v3] drm/i915: miscellaneous tiny tweaks to GEM object->dirty
Date: Thu, 10 Dec 2015 21:16:08 +0000	[thread overview]
Message-ID: <20151210211608.GF11596@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1449773486-30822-5-git-send-email-david.s.gordon@intel.com>

On Thu, Dec 10, 2015 at 06:51:26PM +0000, Dave Gordon wrote:
> This patch covers a couple more places where a GEM object is (or may be)
> modified by means of CPU writes, and should therefore be marked dirty to
> ensure that the changes are not lost in the event that the object is
> evicted under memory pressure.
> 
> One is in i915_gem_begin_cpu_access(); after this call, the GEM object may
> be written to by the caller (which may not be part of the i915 driver e.g.
> udl). We must therefore assume that the object is dirty hereafter if
> the caller has asked for write access.
> 
> Another is in copy_batch(); the destination object is obviously dirty
> after being written, but failing to mark it doesn't cause a bug at
> present, because the object is page-pinned at this point, and should
> remain either page- pinned or GTT-pinned until it's retired, at which
> point its content can be discarded. However, if the lifecycle of shadow
> batches is ever changed (e.g. by the introduction of a GPU scheduler)
> this might no longer be true, so it's safer to mark it correctly (this
> introduces no overhead if the buffer is never swappable). It also makes
> the content cycle clearer:
> 
> 	---allocate-->
> 	[empty buffer acquired from pool]
> 	---fill-->
> 	[valid buffer full of unsaved data]
> 	---use-->
> 	[buffer full of unsaved but unwanted data]
> 	--retire-->
> 	[purgeable buffer returned to pool]
> 	... repeat ...
> 
> The last change here is just for consistency; since 'dirty' has been
> declared as an (unsigned int) bitfield, let's not treat it as a bool.
> Maybe it should be a byte instead?

No, it's just because obj->dirty is older than C's bool type. Changing
it to be bool obj->dirty:1 would be fine - except that there is one
particular very hot path where moving it to an unsigned obj->flags field
would be even better.

> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index e9c2bfd..5eb7887 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -208,6 +208,9 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size
>  		return ret;
>  
>  	ret = i915_gem_object_set_to_cpu_domain(obj, write);
> +	if (write)
> +		obj->dirty = 1;
> +

So looking at the only existing example (drm/udl which only reads from
te object anyway) this would fall into bug category. Hence separate
patch. But I'll defer to Daniel as to whether the dma-buf is meant to
operate at the object level or at the page level with regards to dirty
tracking (certainly we would struggle at the moment with dma-buf on
massive objects).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx