* [PATCH 1/2] drm/i915: Fix up ERR_PTR handling for pinning the ringbuffer
@ 2016-04-12 13:32 Chris Wilson
2016-04-12 13:32 ` [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Chris Wilson @ 2016-04-12 13:32 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage
2016-04-12 13:32 [PATCH 1/2] drm/i915: Fix up ERR_PTR handling for pinning the ringbuffer Chris Wilson
@ 2016-04-12 13:32 ` Chris Wilson
2016-04-12 15:18 ` Chris Wilson
2016-04-12 13:46 ` [PATCH] drm/i915: check for ERR_PTR from i915_gem_object_pin_map() Dave Gordon
2016-04-12 16:03 ` ✗ Fi.CI.BAT: failure for series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2) Patchwork
2 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2016-04-12 13:32 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] drm/i915: check for ERR_PTR from i915_gem_object_pin_map()
2016-04-12 13:32 [PATCH 1/2] drm/i915: Fix up ERR_PTR handling for pinning the ringbuffer Chris Wilson
2016-04-12 13:32 ` [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage Chris Wilson
@ 2016-04-12 13:46 ` Dave Gordon
2016-04-12 13:53 ` Chris Wilson
2016-04-12 16:03 ` ✗ Fi.CI.BAT: failure for series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2) Patchwork
2 siblings, 1 reply; 10+ messages in thread
From: Dave Gordon @ 2016-04-12 13:46 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: check for ERR_PTR from i915_gem_object_pin_map()
2016-04-12 13:46 ` [PATCH] drm/i915: check for ERR_PTR from i915_gem_object_pin_map() Dave Gordon
@ 2016-04-12 13:53 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2016-04-12 13:53 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage
2016-04-12 13:32 ` [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage Chris Wilson
@ 2016-04-12 15:18 ` Chris Wilson
2016-04-20 19:38 ` Dave Gordon
2016-04-21 10:02 ` Dave Gordon
0 siblings, 2 replies; 10+ messages in thread
From: Chris Wilson @ 2016-04-12 15:18 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2)
2016-04-12 13:32 [PATCH 1/2] drm/i915: Fix up ERR_PTR handling for pinning the ringbuffer Chris Wilson
2016-04-12 13:32 ` [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage Chris Wilson
2016-04-12 13:46 ` [PATCH] drm/i915: check for ERR_PTR from i915_gem_object_pin_map() Dave Gordon
@ 2016-04-12 16:03 ` Patchwork
2016-04-12 16:29 ` Dave Gordon
2 siblings, 1 reply; 10+ messages in thread
From: Patchwork @ 2016-04-12 16:03 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ✗ Fi.CI.BAT: failure for series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2)
2016-04-12 16:03 ` ✗ Fi.CI.BAT: failure for series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2) Patchwork
@ 2016-04-12 16:29 ` Dave Gordon
2016-04-20 16:01 ` Tvrtko Ursulin
0 siblings, 1 reply; 10+ messages in thread
From: Dave Gordon @ 2016-04-12 16:29 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ✗ Fi.CI.BAT: failure for series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2)
2016-04-12 16:29 ` Dave Gordon
@ 2016-04-20 16:01 ` Tvrtko Ursulin
0 siblings, 0 replies; 10+ messages in thread
From: Tvrtko Ursulin @ 2016-04-20 16:01 UTC (permalink / raw)
To: Dave Gordon, intel-gfx
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage
2016-04-12 15:18 ` Chris Wilson
@ 2016-04-20 19:38 ` Dave Gordon
2016-04-21 10:02 ` Dave Gordon
1 sibling, 0 replies; 10+ messages in thread
From: Dave Gordon @ 2016-04-20 19:38 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Tvrtko Ursulin
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage
2016-04-12 15:18 ` Chris Wilson
2016-04-20 19:38 ` Dave Gordon
@ 2016-04-21 10:02 ` Dave Gordon
1 sibling, 0 replies; 10+ messages in thread
From: Dave Gordon @ 2016-04-21 10:02 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Tvrtko Ursulin
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-04-21 10:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-12 13:32 [PATCH 1/2] drm/i915: Fix up ERR_PTR handling for pinning the ringbuffer Chris Wilson
2016-04-12 13:32 ` [PATCH 2/2] drm/i915: Mark obj->mapping as dirtying the backing storage Chris Wilson
2016-04-12 15:18 ` Chris Wilson
2016-04-20 19:38 ` Dave Gordon
2016-04-21 10:02 ` Dave Gordon
2016-04-12 13:46 ` [PATCH] drm/i915: check for ERR_PTR from i915_gem_object_pin_map() Dave Gordon
2016-04-12 13:53 ` Chris Wilson
2016-04-12 16:03 ` ✗ Fi.CI.BAT: failure for series starting with drm/i915: check for ERR_PTR from i915_gem_object_pin_map() (rev2) Patchwork
2016-04-12 16:29 ` Dave Gordon
2016-04-20 16:01 ` Tvrtko Ursulin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox