[PATCH 1/2] drm/i915: Pin pages as we flush for CPU access

[PATCH 1/2] drm/i915: Pin pages as we flush for CPU access

[Chris Wilson]

As a precautionary defensive measure against changes to the core (where
we stop changing domains when unbinding), we want to ensure that the pages
are available when we want to start CPU access, otherwise we will not
perform the requisite clflushes to make the contents coherent for the user.

We already do this for the GTT access path, so lift it from the
set-to-gtt-domain to the set-domain-ioctl caller so we also apply it for
set-to-cpu-domain.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a88e6c9e9516..625fb402b67d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1630,11 +1630,27 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto unref;
 
+	/* Flush and acquire obj->pages so that we are coherent through
+	 * direct access in memory with previous cached writes through
+	 * shmemfs and that our cache domain tracking remains valid.
+	 * For example, if the obj->filp was moved to swap without us
+	 * being notified and releasing the pages, we would mistakenly
+	 * continue to assume that the obj remained out of the CPU cached
+	 * domain.
+	 */
+	ret = i915_gem_object_get_pages(obj);
+	if (ret)
+		goto unref;
+
+	i915_gem_object_pin_pages(obj);
+
 	if (read_domains & I915_GEM_DOMAIN_GTT)
 		ret = i915_gem_object_set_to_gtt_domain(obj, write_domain != 0);
 	else
 		ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
 
+	i915_gem_object_unpin_pages(obj);
+
 	if (write_domain != 0)
 		intel_fb_obj_invalidate(obj,
 					write_domain == I915_GEM_DOMAIN_GTT ?
@@ -3754,18 +3770,6 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 	if (ret)
 		return ret;
 
-	/* Flush and acquire obj->pages so that we are coherent through
-	 * direct access in memory with previous cached writes through
-	 * shmemfs and that our cache domain tracking remains valid.
-	 * For example, if the obj->filp was moved to swap without us
-	 * being notified and releasing the pages, we would mistakenly
-	 * continue to assume that the obj remained out of the CPU cached
-	 * domain.
-	 */
-	ret = i915_gem_object_get_pages(obj);
-	if (ret)
-		return ret;
-
 	i915_gem_object_flush_cpu_write_domain(obj);
 
 	/* Serialise direct access to this object with the barriers for
-- 
2.8.1

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

[PATCH 2/2] drm/i915/dmabuf: Pin pages as we flush for CPU access

[Chris Wilson]

As a precautionary defensive measure against changes to the core (where
we stop changing domains when unbinding), we want to ensure that the pages
are available when we want to start CPU access, otherwise we will not
perform the requisite clflushes to make the contents coherent for the user.

In contrast, we do not have to worry about ensuring the pages are
available when flushing after CPU access, as any subsequent use that
requires the pages in the GTT domain will be perform the flush
themselves.

The tricky part is that we can not force the behaviour within
set-to-cpu-domain as that currently needs to be callable from within the
put_pages cleanup path, thus we place the burden on the caller.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 80bbe43a2e92..6d898a201a46 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -182,7 +182,15 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_dire
 	if (ret)
 		return ret;
 
+	ret = i915_gem_object_get_pages(obj);
+	if (ret)
+		goto err_unlock;
+
+	i915_gem_object_pin_pages(obj);
 	ret = i915_gem_object_set_to_cpu_domain(obj, write);
+	i915_gem_object_unpin_pages(obj);
+
+err_unlock:
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
-- 
2.8.1

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

Re: [PATCH 2/2] drm/i915/dmabuf: Pin pages as we flush for CPU access

[Dave Gordon]

On 06/05/16 15:36, Chris Wilson wrote:
> As a precautionary defensive measure against changes to the core (where
> we stop changing domains when unbinding), we want to ensure that the pages
> are available when we want to start CPU access, otherwise we will not
> perform the requisite clflushes to make the contents coherent for the user.
>
> In contrast, we do not have to worry about ensuring the pages are
> available when flushing after CPU access, as any subsequent use that
> requires the pages in the GTT domain will be perform the flush
> themselves.
>
> The tricky part is that we can not force the behaviour within
> set-to-cpu-domain as that currently needs to be callable from within the
> put_pages cleanup path, thus we place the burden on the caller.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 80bbe43a2e92..6d898a201a46 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -182,7 +182,15 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_dire
>   	if (ret)
>   		return ret;
>
> +	ret = i915_gem_object_get_pages(obj);
> +	if (ret)
> +		goto err_unlock;
> +
> +	i915_gem_object_pin_pages(obj);
>   	ret = i915_gem_object_set_to_cpu_domain(obj, write);
> +	i915_gem_object_unpin_pages(obj);
> +
> +err_unlock:
>   	mutex_unlock(&dev->struct_mutex);
>   	return ret;
>   }

So, will these two patches fix the anomalous 
setting-dirty-while-not-pinned behaviour of set-to-gtt/set-to-cpu?

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

Re: [PATCH 2/2] drm/i915/dmabuf: Pin pages as we flush for CPU access

[Chris Wilson]

On Fri, May 06, 2016 at 08:35:17PM +0100, Dave Gordon wrote:
> On 06/05/16 15:36, Chris Wilson wrote:
> >As a precautionary defensive measure against changes to the core (where
> >we stop changing domains when unbinding), we want to ensure that the pages
> >are available when we want to start CPU access, otherwise we will not
> >perform the requisite clflushes to make the contents coherent for the user.
> >
> >In contrast, we do not have to worry about ensuring the pages are
> >available when flushing after CPU access, as any subsequent use that
> >requires the pages in the GTT domain will be perform the flush
> >themselves.
> >
> >The tricky part is that we can not force the behaviour within
> >set-to-cpu-domain as that currently needs to be callable from within the
> >put_pages cleanup path, thus we place the burden on the caller.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> >index 80bbe43a2e92..6d898a201a46 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> >@@ -182,7 +182,15 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_dire
> >  	if (ret)
> >  		return ret;
> >
> >+	ret = i915_gem_object_get_pages(obj);
> >+	if (ret)
> >+		goto err_unlock;
> >+
> >+	i915_gem_object_pin_pages(obj);
> >  	ret = i915_gem_object_set_to_cpu_domain(obj, write);
> >+	i915_gem_object_unpin_pages(obj);
> >+
> >+err_unlock:
> >  	mutex_unlock(&dev->struct_mutex);
> >  	return ret;
> >  }
> 
> So, will these two patches fix the anomalous
> setting-dirty-while-not-pinned behaviour of set-to-gtt/set-to-cpu?

Only by coincidence for set-domain-ioctl with DOMAIN_GTT.

The actual issue I am trying to prevent is missing clflushes when the
object is unbound but the pages are still being tracked... Though now
that appears to be entirely an artifact of removing the struct_mutex
lock rather than my earlier thought that was the lockless patch in
conjuction with the old patch to stop fudging the domains on unbind.
-Chris

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Pin pages as we flush for CPU access

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Pin pages as we flush for CPU access
URL   : https://patchwork.freedesktop.org/series/6838/
State : success

== Summary ==

Series 6838v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/6838/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                fail       -> PASS       (byt-nuc)

bdw-nuci7-2      total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
bdw-ultra        total:219  pass:193  dwarn:0   dfail:0   fail:0   skip:26 
bsw-nuc-2        total:218  pass:174  dwarn:0   dfail:0   fail:2   skip:42 
byt-nuc          total:218  pass:175  dwarn:0   dfail:0   fail:2   skip:41 
hsw-gt2          total:219  pass:197  dwarn:0   dfail:0   fail:1   skip:21 
ilk-hp8440p      total:219  pass:155  dwarn:0   dfail:0   fail:1   skip:63 
ivb-t430s        total:219  pass:188  dwarn:0   dfail:0   fail:0   skip:31 
skl-i7k-2        total:219  pass:191  dwarn:0   dfail:0   fail:0   skip:28 
skl-nuci5        total:219  pass:207  dwarn:0   dfail:0   fail:0   skip:12 
snb-dellxps      total:195  pass:159  dwarn:0   dfail:0   fail:0   skip:35 
snb-x220t failed to collect. IGT log at Patchwork_2151/snb-x220t/igt.log

Results at /archive/results/CI_IGT_test/Patchwork_2151/

4cf0071a8ecbb693f9364c66503fe5b4a7d9aa22 drm-intel-nightly: 2016y-05m-09d-08h-37m-34s UTC integration manifest
c8ae2d1 drm/i915/dmabuf: Pin pages as we flush for CPU access
eccbae7 drm/i915: Pin pages as we flush for CPU access

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