| Message ID | 1433407746.6183.0.camel@jlahtine-mobl1 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 04, 2015 at 11:49:06AM +0300, Joonas Lahtinen wrote: > Get rid of the over-generic i915_gem_obj_is_pinned and replace it > with i915_gem_obj_ggtt_is_pinned or more specific VMA handling. > > Requested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Joonas Lahtinen I take it you didn't read my patch to accomplish the same. > --- > drivers/gpu/drm/i915/i915_debugfs.c | 8 +++++-- > drivers/gpu/drm/i915/i915_drv.h | 9 +++++++- > drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++------------ > drivers/gpu/drm/i915/i915_gem_context.c | 2 +- > drivers/gpu/drm/i915/i915_gpu_error.c | 4 +--- > drivers/gpu/drm/i915/intel_lrc.c | 8 +++---- > 6 files changed, 44 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 3e17210..5442a18 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -519,6 +519,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data) > uintptr_t list = (uintptr_t) node->info_ent->data; > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_i915_gem_object *obj; > + struct i915_vma *vma; > size_t total_obj_size, total_gtt_size; > int count, ret; > > @@ -528,14 +529,17 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data) > > total_obj_size = total_gtt_size = count = 0; > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { For instance here, we only care about the ggtt vma so lookup it first and then use it directly. > - if (list == PINNED_LIST && !i915_gem_obj_is_pinned(obj)) > + if (list == PINNED_LIST && !i915_gem_obj_ggtt_is_pinned(obj)) > continue; > > seq_puts(m, " "); > describe_obj(m, obj); > seq_putc(m, '\n'); > total_obj_size += obj->base.size; > - total_gtt_size += i915_gem_obj_ggtt_size(obj); > + list_for_each_entry(vma, &obj->vma_list, vma_link) > + if (i915_is_ggtt(vma->vm) && > + (list != PINNED_LIST || vma->pin_count > 0)) > + total_gtt_size += vma->node.size; > count++; > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 60aa962..be7bcc6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2960,7 +2960,14 @@ i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj) > { > return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal); > } > -bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj); > + > +bool > +i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj, > + const struct i915_ggtt_view *view); > +static inline bool > +i915_gem_obj_ggtt_is_pinned(struct drm_i915_gem_object *obj) { > + return i915_gem_obj_ggtt_is_pinned_view(obj, &i915_ggtt_view_normal); > +} > > /* Some GGTT VM helpers */ > #define i915_obj_to_ggtt(obj) \ > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index be35f04..75218c2 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -150,13 +150,15 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data, > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_i915_gem_get_aperture *args = data; > struct drm_i915_gem_object *obj; > + struct i915_vma *vma; > size_t pinned; > > pinned = 0; > mutex_lock(&dev->struct_mutex); > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) > - if (i915_gem_obj_is_pinned(obj)) > - pinned += i915_gem_obj_ggtt_size(obj); > + list_for_each_entry(vma, &obj->vma_list, vma_link) > + if (vma->pin_count > 0) > + pinned += vma->node.size; Same again. > mutex_unlock(&dev->struct_mutex); > > args->aper_size = dev_priv->gtt.base.total; > @@ -3967,12 +3969,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, > if (obj->cache_level == cache_level) > return 0; > > - if (i915_gem_obj_is_pinned(obj)) { > - DRM_DEBUG("can not change the cache level of pinned objects\n"); > - return -EBUSY; > - } > - > list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) { > + if (vma->pin_count > 0) { > + DRM_DEBUG("can not change the cache level of pinned objects\n"); > + return -EBUSY; > + } > + > if (!i915_gem_valid_gtt_space(vma, cache_level)) { > ret = i915_vma_unbind(vma); > if (ret) > @@ -4506,6 +4508,7 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_i915_gem_madvise *args = data; > struct drm_i915_gem_object *obj; > + struct i915_vma *vma; > int ret; > > switch (args->madv) { > @@ -4526,9 +4529,11 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > goto unlock; > } > > - if (i915_gem_obj_is_pinned(obj)) { > - ret = -EINVAL; > - goto out; > + list_for_each_entry(vma, &obj->vma_list, vma_link) { > + if (vma->pin_count > 0) { > + ret = -EINVAL; > + goto out; > + } This does not need the pinned check. > } > > if (obj->pages && > @@ -5382,13 +5387,18 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, > return 0; > } > > -bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) > +bool > +i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj, > + const struct i915_ggtt_view *view) > { > struct i915_vma *vma; > - list_for_each_entry(vma, &obj->vma_list, vma_link) > - if (vma->pin_count > 0) > + list_for_each_entry(vma, &obj->vma_list, vma_link) { > + if (!i915_is_ggtt(vma->vm)) > + continue; > + if (i915_ggtt_view_equal(&vma->ggtt_view, view) && > + vma->pin_count > 0) > return true; This function is not required when you succeed in removing the is-pinned queries. > - > + } > return false; > } > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 133afcf..ed4a16b 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -634,7 +634,7 @@ static int do_switch(struct intel_engine_cs *ring, > > if (from != NULL && ring == &dev_priv->ring[RCS]) { > BUG_ON(from->legacy_hw_ctx.rcs_state == NULL); > - BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state)); > + BUG_ON(!i915_gem_obj_ggtt_is_pinned(from->legacy_hw_ctx.rcs_state)); The API is at fault here. > } > > if (should_skip_switch(ring, from, to)) > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index 6f42569..cc52f9c 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -697,9 +697,7 @@ static void capture_bo(struct drm_i915_error_buffer *err, > err->read_domains = obj->base.read_domains; > err->write_domain = obj->base.write_domain; > err->fence_reg = obj->fence_reg; > - err->pinned = 0; > - if (i915_gem_obj_is_pinned(obj)) > - err->pinned = 1; > + err->pinned = vma->pin_count > 0; Fix gpu error capturing for. Patches have been on the list for years. > err->tiling = obj->tiling_mode; > err->dirty = obj->dirty; > err->purgeable = obj->madv != I915_MADV_WILLNEED; > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 9f5485d..dc595a0 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -369,8 +369,8 @@ static void execlists_submit_contexts(struct intel_engine_cs *ring, > struct intel_ringbuffer *ringbuf1 = NULL; > > BUG_ON(!ctx_obj0); > - WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0)); > - WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj)); > + WARN_ON(!i915_gem_obj_ggtt_is_pinned(ctx_obj0)); > + WARN_ON(!i915_gem_obj_ggtt_is_pinned(ringbuf0->obj)); Fix execlists, this is simply lazy code. -Chris
On to, 2015-06-04 at 10:01 +0100, Chris Wilson wrote: > On Thu, Jun 04, 2015 at 11:49:06AM +0300, Joonas Lahtinen wrote: > > Get rid of the over-generic i915_gem_obj_is_pinned and replace it > > with i915_gem_obj_ggtt_is_pinned or more specific VMA handling. > > > > Requested-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Joonas Lahtinen > > I take it you didn't read my patch to accomplish the same. > Nope, do not recall seeing one. > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 8 +++++-- > > drivers/gpu/drm/i915/i915_drv.h | 9 +++++++- > > drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++------------ > > drivers/gpu/drm/i915/i915_gem_context.c | 2 +- > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 +--- > > drivers/gpu/drm/i915/intel_lrc.c | 8 +++---- > > 6 files changed, 44 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > index 3e17210..5442a18 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -519,6 +519,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data) > > uintptr_t list = (uintptr_t) node->info_ent->data; > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_i915_gem_object *obj; > > + struct i915_vma *vma; > > size_t total_obj_size, total_gtt_size; > > int count, ret; > > > > @@ -528,14 +529,17 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data) > > > > total_obj_size = total_gtt_size = count = 0; > > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { > > For instance here, we only care about the ggtt vma so lookup it first and > then use it directly. > Ack. Although, depending on what this information is used for, they might or might not be interested in how much of the GTT the partial/rotated or other views consume. > > - if (list == PINNED_LIST && !i915_gem_obj_is_pinned(obj)) > > + if (list == PINNED_LIST && !i915_gem_obj_ggtt_is_pinned(obj)) > > continue; > > > > seq_puts(m, " "); > > describe_obj(m, obj); > > seq_putc(m, '\n'); > > total_obj_size += obj->base.size; > > - total_gtt_size += i915_gem_obj_ggtt_size(obj); > > + list_for_each_entry(vma, &obj->vma_list, vma_link) > > + if (i915_is_ggtt(vma->vm) && > > + (list != PINNED_LIST || vma->pin_count > 0)) > > + total_gtt_size += vma->node.size; > > count++; > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 60aa962..be7bcc6 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2960,7 +2960,14 @@ i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj) > > { > > return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal); > > } > > -bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj); > > + > > +bool > > +i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj, > > + const struct i915_ggtt_view *view); > > +static inline bool > > +i915_gem_obj_ggtt_is_pinned(struct drm_i915_gem_object *obj) { > > + return i915_gem_obj_ggtt_is_pinned_view(obj, &i915_ggtt_view_normal); > > +} > > > > /* Some GGTT VM helpers */ > > #define i915_obj_to_ggtt(obj) \ > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index be35f04..75218c2 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -150,13 +150,15 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data, > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_i915_gem_get_aperture *args = data; > > struct drm_i915_gem_object *obj; > > + struct i915_vma *vma; > > size_t pinned; > > > > pinned = 0; > > mutex_lock(&dev->struct_mutex); > > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) > > - if (i915_gem_obj_is_pinned(obj)) > > - pinned += i915_gem_obj_ggtt_size(obj); > > + list_for_each_entry(vma, &obj->vma_list, vma_link) > > + if (vma->pin_count > 0) > > + pinned += vma->node.size; > > Same again. I think adding is_ggtt() test for the VMA prior checking for pinning would be the right thing to do? Otherwise the pinned partial/rotated or other views will be unaccounted for. > > > mutex_unlock(&dev->struct_mutex); > > > > args->aper_size = dev_priv->gtt.base.total; > > @@ -3967,12 +3969,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, > > if (obj->cache_level == cache_level) > > return 0; > > > > - if (i915_gem_obj_is_pinned(obj)) { > > - DRM_DEBUG("can not change the cache level of pinned objects\n"); > > - return -EBUSY; > > - } > > - > > list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) { > > + if (vma->pin_count > 0) { > > + DRM_DEBUG("can not change the cache level of pinned objects\n"); > > + return -EBUSY; > > + } > > + > > if (!i915_gem_valid_gtt_space(vma, cache_level)) { > > ret = i915_vma_unbind(vma); > > if (ret) > > @@ -4506,6 +4508,7 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_i915_gem_madvise *args = data; > > struct drm_i915_gem_object *obj; > > + struct i915_vma *vma; > > int ret; > > > > switch (args->madv) { > > @@ -4526,9 +4529,11 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > > goto unlock; > > } > > > > - if (i915_gem_obj_is_pinned(obj)) { > > - ret = -EINVAL; > > - goto out; > > + list_for_each_entry(vma, &obj->vma_list, vma_link) { > > + if (vma->pin_count > 0) { > > + ret = -EINVAL; > > + goto out; > > + } > > This does not need the pinned check. > Right, I wonder why it was there to begin with? > > } > > > > if (obj->pages && > > @@ -5382,13 +5387,18 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, > > return 0; > > } > > > > -bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) > > +bool > > +i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj, > > + const struct i915_ggtt_view *view) > > { > > struct i915_vma *vma; > > - list_for_each_entry(vma, &obj->vma_list, vma_link) > > - if (vma->pin_count > 0) > > + list_for_each_entry(vma, &obj->vma_list, vma_link) { > > + if (!i915_is_ggtt(vma->vm)) > > + continue; > > + if (i915_ggtt_view_equal(&vma->ggtt_view, view) && > > + vma->pin_count > 0) > > return true; > > This function is not required when you succeed in removing the is-pinned > queries. > True, but I left it exist due to the contex/error/execlist patches. It's a step in the right direction, and Ville agreed on that. > > - > > + } > > return false; > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > index 133afcf..ed4a16b 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -634,7 +634,7 @@ static int do_switch(struct intel_engine_cs *ring, > > > > if (from != NULL && ring == &dev_priv->ring[RCS]) { > > BUG_ON(from->legacy_hw_ctx.rcs_state == NULL); > > - BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state)); > > + BUG_ON(!i915_gem_obj_ggtt_is_pinned(from->legacy_hw_ctx.rcs_state)); > > The API is at fault here. Not sure what you mean by that. > > > } > > > > if (should_skip_switch(ring, from, to)) > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > > index 6f42569..cc52f9c 100644 > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > @@ -697,9 +697,7 @@ static void capture_bo(struct drm_i915_error_buffer *err, > > err->read_domains = obj->base.read_domains; > > err->write_domain = obj->base.write_domain; > > err->fence_reg = obj->fence_reg; > > - err->pinned = 0; > > - if (i915_gem_obj_is_pinned(obj)) > > - err->pinned = 1; > > + err->pinned = vma->pin_count > 0; > > Fix gpu error capturing for. Patches have been on the list for years. Which patches exactly? I can have a look. I think we should refine the review process a bit if such bits are still floating in the cyberspace. > > > err->tiling = obj->tiling_mode; > > err->dirty = obj->dirty; > > err->purgeable = obj->madv != I915_MADV_WILLNEED; > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index 9f5485d..dc595a0 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -369,8 +369,8 @@ static void execlists_submit_contexts(struct intel_engine_cs *ring, > > struct intel_ringbuffer *ringbuf1 = NULL; > > > > BUG_ON(!ctx_obj0); > > - WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0)); > > - WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj)); > > + WARN_ON(!i915_gem_obj_ggtt_is_pinned(ctx_obj0)); > > + WARN_ON(!i915_gem_obj_ggtt_is_pinned(ringbuf0->obj)); > > Fix execlists, this is simply lazy code. Doesn't sound like a very small task? Can you elborate. This is also one part discussed with Ville. Regards, Joonas > -Chris >
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6534
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 270/270 270/270
ILK 303/303 303/303
SNB 312/312 312/312
IVB 343/343 343/343
BYT 287/287 287/287
BDW 318/318 318/318
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
Note: You need to pay more attention to line start with '*'
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 3e17210..5442a18 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -519,6 +519,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data) uintptr_t list = (uintptr_t) node->info_ent->data; struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_gem_object *obj; + struct i915_vma *vma; size_t total_obj_size, total_gtt_size; int count, ret; @@ -528,14 +529,17 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data) total_obj_size = total_gtt_size = count = 0; list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { - if (list == PINNED_LIST && !i915_gem_obj_is_pinned(obj)) + if (list == PINNED_LIST && !i915_gem_obj_ggtt_is_pinned(obj)) continue; seq_puts(m, " "); describe_obj(m, obj); seq_putc(m, '\n'); total_obj_size += obj->base.size; - total_gtt_size += i915_gem_obj_ggtt_size(obj); + list_for_each_entry(vma, &obj->vma_list, vma_link) + if (i915_is_ggtt(vma->vm) && + (list != PINNED_LIST || vma->pin_count > 0)) + total_gtt_size += vma->node.size; count++; } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 60aa962..be7bcc6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2960,7 +2960,14 @@ i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj) { return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal); } -bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj); + +bool +i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj, + const struct i915_ggtt_view *view); +static inline bool +i915_gem_obj_ggtt_is_pinned(struct drm_i915_gem_object *obj) { + return i915_gem_obj_ggtt_is_pinned_view(obj, &i915_ggtt_view_normal); +} /* Some GGTT VM helpers */ #define i915_obj_to_ggtt(obj) \ diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index be35f04..75218c2 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -150,13 +150,15 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data, struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_gem_get_aperture *args = data; struct drm_i915_gem_object *obj; + struct i915_vma *vma; size_t pinned; pinned = 0; mutex_lock(&dev->struct_mutex); list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) - if (i915_gem_obj_is_pinned(obj)) - pinned += i915_gem_obj_ggtt_size(obj); + list_for_each_entry(vma, &obj->vma_list, vma_link) + if (vma->pin_count > 0) + pinned += vma->node.size; mutex_unlock(&dev->struct_mutex); args->aper_size = dev_priv->gtt.base.total; @@ -3967,12 +3969,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, if (obj->cache_level == cache_level) return 0; - if (i915_gem_obj_is_pinned(obj)) { - DRM_DEBUG("can not change the cache level of pinned objects\n"); - return -EBUSY; - } - list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) { + if (vma->pin_count > 0) { + DRM_DEBUG("can not change the cache level of pinned objects\n"); + return -EBUSY; + } + if (!i915_gem_valid_gtt_space(vma, cache_level)) { ret = i915_vma_unbind(vma); if (ret) @@ -4506,6 +4508,7 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_gem_madvise *args = data; struct drm_i915_gem_object *obj; + struct i915_vma *vma; int ret; switch (args->madv) { @@ -4526,9 +4529,11 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, goto unlock; } - if (i915_gem_obj_is_pinned(obj)) { - ret = -EINVAL; - goto out; + list_for_each_entry(vma, &obj->vma_list, vma_link) { + if (vma->pin_count > 0) { + ret = -EINVAL; + goto out; + } } if (obj->pages && @@ -5382,13 +5387,18 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, return 0; } -bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) +bool +i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj, + const struct i915_ggtt_view *view) { struct i915_vma *vma; - list_for_each_entry(vma, &obj->vma_list, vma_link) - if (vma->pin_count > 0) + list_for_each_entry(vma, &obj->vma_list, vma_link) { + if (!i915_is_ggtt(vma->vm)) + continue; + if (i915_ggtt_view_equal(&vma->ggtt_view, view) && + vma->pin_count > 0) return true; - + } return false; } diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 133afcf..ed4a16b 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -634,7 +634,7 @@ static int do_switch(struct intel_engine_cs *ring, if (from != NULL && ring == &dev_priv->ring[RCS]) { BUG_ON(from->legacy_hw_ctx.rcs_state == NULL); - BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state)); + BUG_ON(!i915_gem_obj_ggtt_is_pinned(from->legacy_hw_ctx.rcs_state)); } if (should_skip_switch(ring, from, to)) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 6f42569..cc52f9c 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -697,9 +697,7 @@ static void capture_bo(struct drm_i915_error_buffer *err, err->read_domains = obj->base.read_domains; err->write_domain = obj->base.write_domain; err->fence_reg = obj->fence_reg; - err->pinned = 0; - if (i915_gem_obj_is_pinned(obj)) - err->pinned = 1; + err->pinned = vma->pin_count > 0; err->tiling = obj->tiling_mode; err->dirty = obj->dirty; err->purgeable = obj->madv != I915_MADV_WILLNEED; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 9f5485d..dc595a0 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -369,8 +369,8 @@ static void execlists_submit_contexts(struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf1 = NULL; BUG_ON(!ctx_obj0); - WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0)); - WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj)); + WARN_ON(!i915_gem_obj_ggtt_is_pinned(ctx_obj0)); + WARN_ON(!i915_gem_obj_ggtt_is_pinned(ringbuf0->obj)); execlists_update_context(ctx_obj0, ringbuf0->obj, to0->ppgtt, tail0); @@ -378,8 +378,8 @@ static void execlists_submit_contexts(struct intel_engine_cs *ring, ringbuf1 = to1->engine[ring->id].ringbuf; ctx_obj1 = to1->engine[ring->id].state; BUG_ON(!ctx_obj1); - WARN_ON(!i915_gem_obj_is_pinned(ctx_obj1)); - WARN_ON(!i915_gem_obj_is_pinned(ringbuf1->obj)); + WARN_ON(!i915_gem_obj_ggtt_is_pinned(ctx_obj1)); + WARN_ON(!i915_gem_obj_ggtt_is_pinned(ringbuf1->obj)); execlists_update_context(ctx_obj1, ringbuf1->obj, to1->ppgtt, tail1); }
Get rid of the over-generic i915_gem_obj_is_pinned and replace it with i915_gem_obj_ggtt_is_pinned or more specific VMA handling. Requested-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Joonas Lahtinen --- drivers/gpu/drm/i915/i915_debugfs.c | 8 +++++-- drivers/gpu/drm/i915/i915_drv.h | 9 +++++++- drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++------------ drivers/gpu/drm/i915/i915_gem_context.c | 2 +- drivers/gpu/drm/i915/i915_gpu_error.c | 4 +--- drivers/gpu/drm/i915/intel_lrc.c | 8 +++---- 6 files changed, 44 insertions(+), 25 deletions(-)