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@xxxxxxxxxxxxxxxxxx>
> > 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
