| Message ID | 1375315222-4785-20-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 31, 2013 at 05:00:12PM -0700, Ben Widawsky wrote: > In some places, we want to know if an object is bound in any address > space, and not just the global GTT. This often applies when there is a > single global resource (object, pages, etc.) > > function | reason > -------------------------------------------------- > i915_gem_object_is_inactive | global object > i915_gem_object_put_pages | object's pages > 915_gem_object_unpin | global object > i915_gem_execbuffer_unreserve_object | temporary until we plumb vma > pread/pread | object's domain Most of these we want to operate on ggtt, so this change looks confused at best. -Chris
On Sat, Aug 03, 2013 at 12:03:35PM +0100, Chris Wilson wrote: > On Wed, Jul 31, 2013 at 05:00:12PM -0700, Ben Widawsky wrote: > > In some places, we want to know if an object is bound in any address > > space, and not just the global GTT. This often applies when there is a > > single global resource (object, pages, etc.) > > > > function | reason > > -------------------------------------------------- > > i915_gem_object_is_inactive | global object > > i915_gem_object_put_pages | object's pages > > 915_gem_object_unpin | global object > > i915_gem_execbuffer_unreserve_object | temporary until we plumb vma > > pread/pread | object's domain > > Most of these we want to operate on ggtt, so this change looks confused > at best. > -Chris > And at worst? I guess I should have explained it a bit differently - yes the operations may very well be on the global GTT, but for the given reason (in the table), we need to make sure we unbind (or assert the object isn't bound) in another address space first. Possibly I've misunderstood what you'd like me to do however.
On Wed, Jul 31, 2013 at 05:00:12PM -0700, Ben Widawsky wrote: > In some places, we want to know if an object is bound in any address > space, and not just the global GTT. This often applies when there is a > single global resource (object, pages, etc.) > > function | reason > -------------------------------------------------- > i915_gem_object_is_inactive | global object > i915_gem_object_put_pages | object's pages > 915_gem_object_unpin | global object > i915_gem_execbuffer_unreserve_object | temporary until we plumb vma > pread/pread | object's domain pread/pwrite isn't about the object's domain at all, but purely about synchronizing for outstanding rendering. Replacing the call to set_to_gtt_domain with a wait_rendering would imo improve code readability. Furthermore we could pimp pread to only block for outstanding writes and not for reads. Since you're not the first one to trip over this: Can I volunteer you for a follow-up patch to fix this? Otherwise patch looks good. -Daniel > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem.c | 12 ++++++------ > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 1013105..d4d6444 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -122,7 +122,7 @@ int i915_mutex_lock_interruptible(struct drm_device *dev) > static inline bool > i915_gem_object_is_inactive(struct drm_i915_gem_object *obj) > { > - return i915_gem_obj_ggtt_bound(obj) && !obj->active; > + return i915_gem_obj_bound_any(obj) && !obj->active; > } > > int > @@ -408,7 +408,7 @@ i915_gem_shmem_pread(struct drm_device *dev, > * anyway again before the next pread happens. */ > if (obj->cache_level == I915_CACHE_NONE) > needs_clflush = 1; > - if (i915_gem_obj_ggtt_bound(obj)) { > + if (i915_gem_obj_bound_any(obj)) { > ret = i915_gem_object_set_to_gtt_domain(obj, false); > if (ret) > return ret; > @@ -725,7 +725,7 @@ i915_gem_shmem_pwrite(struct drm_device *dev, > * right away and we therefore have to clflush anyway. */ > if (obj->cache_level == I915_CACHE_NONE) > needs_clflush_after = 1; > - if (i915_gem_obj_ggtt_bound(obj)) { > + if (i915_gem_obj_bound_any(obj)) { > ret = i915_gem_object_set_to_gtt_domain(obj, true); > if (ret) > return ret; > @@ -1659,7 +1659,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj) > if (obj->pages_pin_count) > return -EBUSY; > > - BUG_ON(i915_gem_obj_ggtt_bound(obj)); > + BUG_ON(i915_gem_obj_bound_any(obj)); > > /* ->put_pages might need to allocate memory for the bit17 swizzle > * array, hence protect them from being reaped by removing them from gtt > @@ -3301,7 +3301,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write) > int ret; > > /* Not valid to be called on unbound objects. */ > - if (!i915_gem_obj_ggtt_bound(obj)) > + if (!i915_gem_obj_bound_any(obj)) > return -EINVAL; > > if (obj->base.write_domain == I915_GEM_DOMAIN_GTT) > @@ -3725,7 +3725,7 @@ void > i915_gem_object_unpin(struct drm_i915_gem_object *obj) > { > BUG_ON(obj->pin_count == 0); > - BUG_ON(!i915_gem_obj_ggtt_bound(obj)); > + BUG_ON(!i915_gem_obj_bound_any(obj)); > > if (--obj->pin_count == 0) > obj->pin_mappable = false; > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 5e68f1e..64dc6b5 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -466,7 +466,7 @@ i915_gem_execbuffer_unreserve_object(struct drm_i915_gem_object *obj) > { > struct drm_i915_gem_exec_object2 *entry; > > - if (!i915_gem_obj_ggtt_bound(obj)) > + if (!i915_gem_obj_bound_any(obj)) > return; > > entry = obj->exec_entry; > -- > 1.8.3.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Aug 06, 2013 at 08:43:42PM +0200, Daniel Vetter wrote: > On Wed, Jul 31, 2013 at 05:00:12PM -0700, Ben Widawsky wrote: > > In some places, we want to know if an object is bound in any address > > space, and not just the global GTT. This often applies when there is a > > single global resource (object, pages, etc.) > > > > function | reason > > -------------------------------------------------- > > i915_gem_object_is_inactive | global object > > i915_gem_object_put_pages | object's pages > > 915_gem_object_unpin | global object > > i915_gem_execbuffer_unreserve_object | temporary until we plumb vma > > pread/pread | object's domain > > pread/pwrite isn't about the object's domain at all, but purely about > synchronizing for outstanding rendering. Replacing the call to > set_to_gtt_domain with a wait_rendering would imo improve code > readability. Furthermore we could pimp pread to only block for outstanding > writes and not for reads. I might have been the first to trip over it, but this isn't my first instance ;-). > > Since you're not the first one to trip over this: Can I volunteer you for > a follow-up patch to fix this? Working on it now. > > Otherwise patch looks good. > -Daniel > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 12 ++++++------ > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- > > 2 files changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 1013105..d4d6444 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -122,7 +122,7 @@ int i915_mutex_lock_interruptible(struct drm_device *dev) > > static inline bool > > i915_gem_object_is_inactive(struct drm_i915_gem_object *obj) > > { > > - return i915_gem_obj_ggtt_bound(obj) && !obj->active; > > + return i915_gem_obj_bound_any(obj) && !obj->active; > > } > > > > int > > @@ -408,7 +408,7 @@ i915_gem_shmem_pread(struct drm_device *dev, > > * anyway again before the next pread happens. */ > > if (obj->cache_level == I915_CACHE_NONE) > > needs_clflush = 1; > > - if (i915_gem_obj_ggtt_bound(obj)) { > > + if (i915_gem_obj_bound_any(obj)) { > > ret = i915_gem_object_set_to_gtt_domain(obj, false); > > if (ret) > > return ret; > > @@ -725,7 +725,7 @@ i915_gem_shmem_pwrite(struct drm_device *dev, > > * right away and we therefore have to clflush anyway. */ > > if (obj->cache_level == I915_CACHE_NONE) > > needs_clflush_after = 1; > > - if (i915_gem_obj_ggtt_bound(obj)) { > > + if (i915_gem_obj_bound_any(obj)) { > > ret = i915_gem_object_set_to_gtt_domain(obj, true); > > if (ret) > > return ret; > > @@ -1659,7 +1659,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj) > > if (obj->pages_pin_count) > > return -EBUSY; > > > > - BUG_ON(i915_gem_obj_ggtt_bound(obj)); > > + BUG_ON(i915_gem_obj_bound_any(obj)); > > > > /* ->put_pages might need to allocate memory for the bit17 swizzle > > * array, hence protect them from being reaped by removing them from gtt > > @@ -3301,7 +3301,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write) > > int ret; > > > > /* Not valid to be called on unbound objects. */ > > - if (!i915_gem_obj_ggtt_bound(obj)) > > + if (!i915_gem_obj_bound_any(obj)) > > return -EINVAL; > > > > if (obj->base.write_domain == I915_GEM_DOMAIN_GTT) > > @@ -3725,7 +3725,7 @@ void > > i915_gem_object_unpin(struct drm_i915_gem_object *obj) > > { > > BUG_ON(obj->pin_count == 0); > > - BUG_ON(!i915_gem_obj_ggtt_bound(obj)); > > + BUG_ON(!i915_gem_obj_bound_any(obj)); > > > > if (--obj->pin_count == 0) > > obj->pin_mappable = false; > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index 5e68f1e..64dc6b5 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -466,7 +466,7 @@ i915_gem_execbuffer_unreserve_object(struct drm_i915_gem_object *obj) > > { > > struct drm_i915_gem_exec_object2 *entry; > > > > - if (!i915_gem_obj_ggtt_bound(obj)) > > + if (!i915_gem_obj_bound_any(obj)) > > return; > > > > entry = obj->exec_entry; > > -- > > 1.8.3.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 1013105..d4d6444 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -122,7 +122,7 @@ int i915_mutex_lock_interruptible(struct drm_device *dev) static inline bool i915_gem_object_is_inactive(struct drm_i915_gem_object *obj) { - return i915_gem_obj_ggtt_bound(obj) && !obj->active; + return i915_gem_obj_bound_any(obj) && !obj->active; } int @@ -408,7 +408,7 @@ i915_gem_shmem_pread(struct drm_device *dev, * anyway again before the next pread happens. */ if (obj->cache_level == I915_CACHE_NONE) needs_clflush = 1; - if (i915_gem_obj_ggtt_bound(obj)) { + if (i915_gem_obj_bound_any(obj)) { ret = i915_gem_object_set_to_gtt_domain(obj, false); if (ret) return ret; @@ -725,7 +725,7 @@ i915_gem_shmem_pwrite(struct drm_device *dev, * right away and we therefore have to clflush anyway. */ if (obj->cache_level == I915_CACHE_NONE) needs_clflush_after = 1; - if (i915_gem_obj_ggtt_bound(obj)) { + if (i915_gem_obj_bound_any(obj)) { ret = i915_gem_object_set_to_gtt_domain(obj, true); if (ret) return ret; @@ -1659,7 +1659,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj) if (obj->pages_pin_count) return -EBUSY; - BUG_ON(i915_gem_obj_ggtt_bound(obj)); + BUG_ON(i915_gem_obj_bound_any(obj)); /* ->put_pages might need to allocate memory for the bit17 swizzle * array, hence protect them from being reaped by removing them from gtt @@ -3301,7 +3301,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write) int ret; /* Not valid to be called on unbound objects. */ - if (!i915_gem_obj_ggtt_bound(obj)) + if (!i915_gem_obj_bound_any(obj)) return -EINVAL; if (obj->base.write_domain == I915_GEM_DOMAIN_GTT) @@ -3725,7 +3725,7 @@ void i915_gem_object_unpin(struct drm_i915_gem_object *obj) { BUG_ON(obj->pin_count == 0); - BUG_ON(!i915_gem_obj_ggtt_bound(obj)); + BUG_ON(!i915_gem_obj_bound_any(obj)); if (--obj->pin_count == 0) obj->pin_mappable = false; diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 5e68f1e..64dc6b5 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -466,7 +466,7 @@ i915_gem_execbuffer_unreserve_object(struct drm_i915_gem_object *obj) { struct drm_i915_gem_exec_object2 *entry; - if (!i915_gem_obj_ggtt_bound(obj)) + if (!i915_gem_obj_bound_any(obj)) return; entry = obj->exec_entry;
In some places, we want to know if an object is bound in any address space, and not just the global GTT. This often applies when there is a single global resource (object, pages, etc.) function | reason -------------------------------------------------- i915_gem_object_is_inactive | global object i915_gem_object_put_pages | object's pages 915_gem_object_unpin | global object i915_gem_execbuffer_unreserve_object | temporary until we plumb vma pread/pread | object's domain Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem.c | 12 ++++++------ drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-)