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@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
