[PATCH 1/2] drm/i915: Fix up ERR_PTR handling for pinning the ringbuffer

[PATCH 1/2] drm/i915: Fix up ERR_PTR handling for pinning the ringbuffer

[Chris Wilson]

In commit 0a798eb92e6dcc1cba45d13d7b75a523e5d0fc4c
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Apr 8 12:11:11 2016 +0100

    drm/i915: Refactor duplicate object vmap functions

the vmap function that returned NULL on error was replaced by one that
returned an error pointer instead. Not all callsites were updated...

Reported-by: Dave Gordon <david.s.gordon@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 41b604e69db7..15064a8f706b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2108,8 +2108,9 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 			goto err_unpin;
 
 		ringbuf->virtual_start = i915_gem_object_pin_map(obj);
-		if (ringbuf->virtual_start == NULL) {
-			ret = -ENOMEM;
+		if (IS_ERR(ringbuf->virtual_start)) {
+			ret = PTR_ERR(ringbuf->virtual_start);
+			ringbuf->virtual_start = NULL;
 			goto err_unpin;
 		}
 	} else {
-- 
2.8.0.rc3

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

[PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage

[Chris Wilson]

When reviewing some of Tvrtko's usage for i915_gem_object_pin_map(), he
suggested replacing some use of kmap(i915_gem_object_get_dirty_page())
with a plain i915_gem_object_pin_map(). This raised the question of who
should mark the page as dirty (or the mapping case, the object).
We can write simpler, safer code if we mark the entire object as dirty
upon obtaining the obj->mapping. (The counter-argument is that the
caller should be marking the object as dirty itself, or we should be
passing in a direction parameter.)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b37ffea8b458..c0a4e38d6392 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2448,6 +2448,8 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj)
 		}
 	}
 
+	/* Presume the caller is going to write into the map */
+	obj->dirty = true;
 	return obj->mapping;
 }
 
-- 
2.8.0.rc3

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

Re: [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage

[Chris Wilson]

On Tue, Apr 12, 2016 at 02:32:31PM +0100, Chris Wilson wrote:
> When reviewing some of Tvrtko's usage for i915_gem_object_pin_map(), he
> suggested replacing some use of kmap(i915_gem_object_get_dirty_page())
> with a plain i915_gem_object_pin_map(). This raised the question of who
> should mark the page as dirty (or the mapping case, the object).
> We can write simpler, safer code if we mark the entire object as dirty
> upon obtaining the obj->mapping. (The counter-argument is that the
> caller should be marking the object as dirty itself, or we should be
> passing in a direction parameter.)

What I particularly dislike about the current obj->dirty is that it is
strictly only valid inside a pin_pages/unpin_pages section. That isn't
clear from the API atm.
-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

Re: [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage

[Dave Gordon]

On 12/04/16 16:18, Chris Wilson wrote:
> On Tue, Apr 12, 2016 at 02:32:31PM +0100, Chris Wilson wrote:
>> When reviewing some of Tvrtko's usage for i915_gem_object_pin_map(), he
>> suggested replacing some use of kmap(i915_gem_object_get_dirty_page())
>> with a plain i915_gem_object_pin_map(). This raised the question of who
>> should mark the page as dirty (or the mapping case, the object).
>> We can write simpler, safer code if we mark the entire object as dirty
>> upon obtaining the obj->mapping. (The counter-argument is that the
>> caller should be marking the object as dirty itself, or we should be
>> passing in a direction parameter.)

Hmm, didn't I say that back in December?

> What I particularly dislike about the current obj->dirty is that it is
> strictly only valid inside a pin_pages/unpin_pages section. That isn't
> clear from the API atm.
> -Chris

But no-one ever reads it until after the pincount goes to zero! I don't 
even think reading it is part of the public API at all; so we probably 
should replace 'obj->dirty = true' with i915_gem_object_mark_dirty(obj) 
everywhere.

As for existing callers of pin-and-map:
. populate-context() marks the object dirty
. context pinning already marks the whole object dirty
. context reset already marks the whole object dirty
. the LRC HWSP code doesn't mark it (but it's part of the context)
but
. pin-and-map ringbuffer doesn't mark anything - we could add it
. the dmabuf code doesn't mark anything - should it?

All the above places that set obj->dirty are either touching multiple 
pages themselves or have mapped the whole object for access by the GPU, 
so in all these cases multiple pages are (potentially) dirty.

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

Re: [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage

[Dave Gordon]

On 12/04/16 16:18, Chris Wilson wrote:
> On Tue, Apr 12, 2016 at 02:32:31PM +0100, Chris Wilson wrote:
>> When reviewing some of Tvrtko's usage for i915_gem_object_pin_map(), he
>> suggested replacing some use of kmap(i915_gem_object_get_dirty_page())
>> with a plain i915_gem_object_pin_map(). This raised the question of who
>> should mark the page as dirty (or the mapping case, the object).
>> We can write simpler, safer code if we mark the entire object as dirty
>> upon obtaining the obj->mapping. (The counter-argument is that the
>> caller should be marking the object as dirty itself, or we should be
>> passing in a direction parameter.)
>
> What I particularly dislike about the current obj->dirty is that it is
> strictly only valid inside a pin_pages/unpin_pages section. That isn't
> clear from the API atm.
> -Chris

So, I tried replacing all instances of "obj->dirty = true" with my new 
function i915_gem_object_mark_dirty(), and added an assertion that it's 
called only when (pages_pin_count > 0) - and found a failure.

Stack is:
	i915_gem_object_mark_dirty
	i915_gem_object_set_to_gtt_domain
	i915_gem_set_domain_ioctl

So is i915_gem_object_set_to_gtt_domain() wrong? It's done a get_pages 
but no pin_pages. Also, i915_gem_object_set_to_cpu_domain() doesn't mark 
the object dirty in the corresponding if(write) clause - is that also wrong?

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

[PATCH] drm/i915: check for ERR_PTR from i915_gem_object_pin_map()

[Dave Gordon]

The newly-introduced function i915_gem_object_pin_map() returns an
ERR_PTR (not NULL) if the pin-and-map opertaion fails, so that's what we
must check for. And it's nicer not to assign such a pointer-or-error to
a structure being filled in until after it's been validated, so we
should keep it local and avoid exporting a bogus pointer. Also, for
clarity and symmetry, we should clear 'virtual_start' along with 'vma'
when unmapping a ringbuffer.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>

---
 drivers/gpu/drm/i915/i915_drv.h         |  6 ++++--
 drivers/gpu/drm/i915/intel_ringbuffer.c | 15 +++++++++------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a9c8211..bc8f0a3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3000,9 +3000,11 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
  * pages and then returns a contiguous mapping of the backing storage into
  * the kernel address space.
  *
- * The caller must hold the struct_mutex.
+ * The caller must hold the struct_mutex, and is responsible for calling
+ * i915_gem_object_unpin_map() when the mapping is no longer required.
  *
- * Returns the pointer through which to access the backing storage.
+ * Returns the pointer through which to access the mapped object, or an
+ * ERR_PTR() on error.
  */
 void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 41b604e..35231ff 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2086,6 +2086,7 @@ void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
 		i915_gem_object_unpin_map(ringbuf->obj);
 	else
 		iounmap(ringbuf->virtual_start);
+	ringbuf->virtual_start = NULL;
 	ringbuf->vma = NULL;
 	i915_gem_object_ggtt_unpin(ringbuf->obj);
 }
@@ -2096,6 +2097,7 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct drm_i915_gem_object *obj = ringbuf->obj;
+	void *addr;
 	int ret;
 
 	if (HAS_LLC(dev_priv) && !obj->stolen) {
@@ -2107,9 +2109,9 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 		if (ret)
 			goto err_unpin;
 
-		ringbuf->virtual_start = i915_gem_object_pin_map(obj);
-		if (ringbuf->virtual_start == NULL) {
-			ret = -ENOMEM;
+		addr = i915_gem_object_pin_map(obj);
+		if (IS_ERR(addr)) {
+			ret = PTR_ERR(addr);
 			goto err_unpin;
 		}
 	} else {
@@ -2124,14 +2126,15 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 		/* Access through the GTT requires the device to be awake. */
 		assert_rpm_wakelock_held(dev_priv);
 
-		ringbuf->virtual_start = ioremap_wc(ggtt->mappable_base +
-						    i915_gem_obj_ggtt_offset(obj), ringbuf->size);
-		if (ringbuf->virtual_start == NULL) {
+		addr = ioremap_wc(ggtt->mappable_base +
+				  i915_gem_obj_ggtt_offset(obj), ringbuf->size);
+		if (addr == NULL) {
 			ret = -ENOMEM;
 			goto err_unpin;
 		}
 	}
 
+	ringbuf->virtual_start = addr;
 	ringbuf->vma = i915_gem_obj_to_ggtt(obj);
 	return 0;
 
-- 
1.9.1

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

Re: [PATCH] drm/i915: check for ERR_PTR from i915_gem_object_pin_map()

[Chris Wilson]

On Tue, Apr 12, 2016 at 02:46:16PM +0100, Dave Gordon wrote:
> The newly-introduced function i915_gem_object_pin_map() returns an
> ERR_PTR (not NULL) if the pin-and-map opertaion fails, so that's what we
> must check for. And it's nicer not to assign such a pointer-or-error to
> a structure being filled in until after it's been validated, so we
> should keep it local and avoid exporting a bogus pointer. Also, for
> clarity and symmetry, we should clear 'virtual_start' along with 'vma'
> when unmapping a ringbuffer.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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: failure for series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2)
URL   : https://patchwork.freedesktop.org/series/5591/
State : failure

== Summary ==

Series 5591v2 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5591/revisions/2/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (snb-x220t)

bdw-nuci7        total:203  pass:191  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:203  pass:180  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:202  pass:162  dwarn:1   dfail:0   fail:0   skip:39 
byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:203  pass:179  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:203  pass:184  dwarn:0   dfail:0   fail:0   skip:19 
ivb-t430s        total:203  pass:175  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:203  pass:178  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:203  pass:192  dwarn:0   dfail:0   fail:0   skip:11 
snb-x220t        total:203  pass:164  dwarn:0   dfail:0   fail:2   skip:37 
BOOT FAILED for ilk-hp8440p
BOOT FAILED for snb-dellxps

Results at /archive/results/CI_IGT_test/Patchwork_1872/

d89f227a17b175fce74e11b2d5fa2a41f86fc489 drm-intel-nightly: 2016y-04m-12d-13h-31m-26s UTC integration manifest
33d810a drm/i915: Mark obj->mapping as dirtying the backing storage
0d172b1 drm/i915: check for ERR_PTR from i915_gem_object_pin_map()

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

Re: ✗ Fi.CI.BAT: failure for series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2)

[Dave Gordon]

On 12/04/16 17:03, Patchwork wrote:
> == Series Details ==
>
> Series: series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2)
> URL   : https://patchwork.freedesktop.org/series/5591/
> State : failure
>
> == Summary ==
>
> Series 5591v2 Series without cover letter
> http://patchwork.freedesktop.org/api/1.0/series/5591/revisions/2/mbox/
>
> Test kms_flip:
>          Subgroup basic-flip-vs-wf_vblank:
>                  pass       -> FAIL       (snb-x220t)

https://bugs.freedesktop.org/show_bug.cgi?id=94294

"basic-flip-vs-wf_vblank timestamp vs. seq counter difference"

> bdw-nuci7        total:203  pass:191  dwarn:0   dfail:0   fail:0   skip:12
> bdw-ultra        total:203  pass:180  dwarn:0   dfail:0   fail:0   skip:23
> bsw-nuc-2        total:202  pass:162  dwarn:1   dfail:0   fail:0   skip:39
> byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0   skip:38
> hsw-brixbox      total:203  pass:179  dwarn:0   dfail:0   fail:0   skip:24
> hsw-gt2          total:203  pass:184  dwarn:0   dfail:0   fail:0   skip:19
> ivb-t430s        total:203  pass:175  dwarn:0   dfail:0   fail:0   skip:28
> skl-i7k-2        total:203  pass:178  dwarn:0   dfail:0   fail:0   skip:25
> skl-nuci5        total:203  pass:192  dwarn:0   dfail:0   fail:0   skip:11
> snb-x220t        total:203  pass:164  dwarn:0   dfail:0   fail:2   skip:37
> BOOT FAILED for ilk-hp8440p
> BOOT FAILED for snb-dellxps
>
> Results at /archive/results/CI_IGT_test/Patchwork_1872/
>
> d89f227a17b175fce74e11b2d5fa2a41f86fc489 drm-intel-nightly: 2016y-04m-12d-13h-31m-26s UTC integration manifest
> 33d810a drm/i915: Mark obj->mapping as dirtying the backing storage
> 0d172b1 drm/i915: check for ERR_PTR from i915_gem_object_pin_map()
>

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

Re: ✗ Fi.CI.BAT: failure for series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2)

[Tvrtko Ursulin]

On 12/04/16 17:29, Dave Gordon wrote:
> On 12/04/16 17:03, Patchwork wrote:
>> == Series Details ==
>>
>> Series: series starting with drm/i915: check for ERR_PTR from
>> i915_gem_object_pin_map() (rev2)
>> URL   : https://patchwork.freedesktop.org/series/5591/
>> State : failure
>>
>> == Summary ==
>>
>> Series 5591v2 Series without cover letter
>> http://patchwork.freedesktop.org/api/1.0/series/5591/revisions/2/mbox/
>>
>> Test kms_flip:
>>          Subgroup basic-flip-vs-wf_vblank:
>>                  pass       -> FAIL       (snb-x220t)
>
> https://bugs.freedesktop.org/show_bug.cgi?id=94294
>
> "basic-flip-vs-wf_vblank timestamp vs. seq counter difference"
>
>> bdw-nuci7        total:203  pass:191  dwarn:0   dfail:0   fail:0
>> skip:12
>> bdw-ultra        total:203  pass:180  dwarn:0   dfail:0   fail:0
>> skip:23
>> bsw-nuc-2        total:202  pass:162  dwarn:1   dfail:0   fail:0
>> skip:39
>> byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0
>> skip:38
>> hsw-brixbox      total:203  pass:179  dwarn:0   dfail:0   fail:0
>> skip:24
>> hsw-gt2          total:203  pass:184  dwarn:0   dfail:0   fail:0
>> skip:19
>> ivb-t430s        total:203  pass:175  dwarn:0   dfail:0   fail:0
>> skip:28
>> skl-i7k-2        total:203  pass:178  dwarn:0   dfail:0   fail:0
>> skip:25
>> skl-nuci5        total:203  pass:192  dwarn:0   dfail:0   fail:0
>> skip:11
>> snb-x220t        total:203  pass:164  dwarn:0   dfail:0   fail:2
>> skip:37
>> BOOT FAILED for ilk-hp8440p
>> BOOT FAILED for snb-dellxps
>>
>> Results at /archive/results/CI_IGT_test/Patchwork_1872/
>>
>> d89f227a17b175fce74e11b2d5fa2a41f86fc489 drm-intel-nightly:
>> 2016y-04m-12d-13h-31m-26s UTC integration manifest
>> 33d810a drm/i915: Mark obj->mapping as dirtying the backing storage
>> 0d172b1 drm/i915: check for ERR_PTR from i915_gem_object_pin_map()

Merged the first one, thanks for the patch and review.

Regards,

Tvrtko




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