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

Submitted by Chris Wilson on April 12, 2016, 1:32 p.m.

Details

Message ID 1460467951-9716-2-git-send-email-chris@chris-wilson.co.uk
State New
Headers show
Series "Series without cover letter" ( rev: 2 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Chris Wilson April 12, 2016, 1:32 p.m.
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(+)

Patch hide | download patch | download mbox

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;
 }
 

Comments

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
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.
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.