On Fri, Jul 12, 2013 at 08:46:48AM -0700, Ben Widawsky wrote:
> On Fri, Jul 12, 2013 at 08:26:07AM +0200, Daniel Vetter wrote:
> > On Thu, Jul 11, 2013 at 07:23:08PM -0700, Ben Widawsky wrote:
> > > On Tue, Jul 09, 2013 at 09:15:01AM +0200, Daniel Vetter wrote:
> > > > On Mon, Jul 08, 2013 at 11:08:37PM -0700, Ben Widawsky wrote:
[snip]
> > > > > @@ -3333,12 +3376,15 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > > > > }
> > > > >
> > > > > if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
> > > > > - ret = i915_gem_object_unbind(obj);
> > > > > + ret = i915_gem_object_unbind(obj, vm);
> > > > > if (ret)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > - if (i915_gem_obj_ggtt_bound(obj)) {
> > > > > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> > > > > + if (!i915_gem_obj_bound(obj, vm))
> > > > > + continue;
> > > >
> > > > Hm, shouldn't we have a per-object list of vmas? Or will that follow later
> > > > on?
> > > >
> > > > Self-correction: It exists already ... why can't we use this here?
> > >
> > > Yes. That should work, I'll fix it and test it. It looks slightly worse
> > > IMO in terms of code clarity, but I don't mind the change.
> >
> > Actually I think it'd gain in clarity, doing pte updatest (which
> > set_cache_level does) on the vma instead of the (obj, vm) pair feels more
> > natural. And we'd be able to drop lots of (obj, vm) -> vma lookups here.
>
> That sounds good to me. Would you mind a patch on top?
If you want I guess we can refactor this after everything has settled. Has
the upside that assessing whether using vma or (obj, vm) is much easier.
So fine with me.
>
> >
> > >
> > > >
> > > > > +
> > > > > ret = i915_gem_object_finish_gpu(obj);
> > > > > if (ret)
> > > > > return ret;
> > > > > @@ -3361,7 +3407,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > > > > i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt,
> > > > > obj, cache_level);
> > > > >
> > > > > - i915_gem_obj_ggtt_set_color(obj, cache_level);
> > > > > + i915_gem_obj_set_color(obj, vm, cache_level);
> > > > > }
> > > > >
> > > > > if (cache_level == I915_CACHE_NONE) {
> > > > > @@ -3421,6 +3467,7 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
> > > > > struct drm_file *file)
> > > > > {
> > > > > struct drm_i915_gem_caching *args = data;
> > > > > + struct drm_i915_private *dev_priv;
> > > > > struct drm_i915_gem_object *obj;
> > > > > enum i915_cache_level level;
> > > > > int ret;
> > > > > @@ -3445,8 +3492,10 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
> > > > > ret = -ENOENT;
> > > > > goto unlock;
> > > > > }
> > > > > + dev_priv = obj->base.dev->dev_private;
> > > > >
> > > > > - ret = i915_gem_object_set_cache_level(obj, level);
> > > > > + /* FIXME: Add interface for specific VM? */
> > > > > + ret = i915_gem_object_set_cache_level(obj, &dev_priv->gtt.base, level);
> > > > >
> > > > > drm_gem_object_unreference(&obj->base);
> > > > > unlock:
> > > > > @@ -3464,6 +3513,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > > > u32 alignment,
> > > > > struct intel_ring_buffer *pipelined)
> > > > > {
> > > > > + struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > > > > u32 old_read_domains, old_write_domain;
> > > > > int ret;
> > > > >
> > > > > @@ -3482,7 +3532,8 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > > > * of uncaching, which would allow us to flush all the LLC-cached data
> > > > > * with that bit in the PTE to main memory with just one PIPE_CONTROL.
> > > > > */
> > > > > - ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
> > > > > + ret = i915_gem_object_set_cache_level(obj, &dev_priv->gtt.base,
> > > > > + I915_CACHE_NONE);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > @@ -3490,7 +3541,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > > > * (e.g. libkms for the bootup splash), we have to ensure that we
> > > > > * always use map_and_fenceable for all scanout buffers.
> > > > > */
> > > > > - ret = i915_gem_object_pin(obj, alignment, true, false);
> > > > > + ret = i915_gem_ggtt_pin(obj, alignment, true, false);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > @@ -3633,6 +3684,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
> > > > >
> > > > > int
> > > > > i915_gem_object_pin(struct drm_i915_gem_object *obj,
> > > > > + struct i915_address_space *vm,
> > > > > uint32_t alignment,
> > > > > bool map_and_fenceable,
> > > > > bool nonblocking)
> > > > > @@ -3642,26 +3694,29 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> > > > > if (WARN_ON(obj->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
> > > > > return -EBUSY;
> > > > >
> > > > > - if (i915_gem_obj_ggtt_bound(obj)) {
> > > > > - if ((alignment && i915_gem_obj_ggtt_offset(obj) & (alignment - 1)) ||
> > > > > + BUG_ON(map_and_fenceable && !i915_is_ggtt(vm));
> > > >
> > > > WARN_ON, since presumably we can keep on going if we get this wrong
> > > > (albeit with slightly corrupted state, so render corruptions might
> > > > follow).
> > >
> > > Can we make a deal, can we leave this as BUG_ON with a FIXME to convert
> > > it at the end of merging?
> >
> > Adding a FIXME right above it will cause equal amounts of conflicts, so I
> > don't see the point that much ...
>
> I'm just really fearful that in doing the reworks I will end up with
> this condition, and I am afraid I will miss them if it's a WARN_ON.
> Definitely it's more likely to miss than a BUG.
>
> Also, and we've disagreed on this a few times by now, this is an
> internal interface which I think should carry such a fatal error for
> this level of mistake.
Ime every time I argue this with myself and state your case it ends up
biting me horribly because I'm regularly too incompetent and hit my very
on BUG_ONs ;-) Hence why I insist so much on using WARN_ON wherever
possible. Of course if people don't check they're logs that's a different
matter (*cough* Jesse *cough*) ...
> In any case I've made the change locally. Will yell at you later if I
> was right.
Getting yelled at is part of my job, so bring it on ;-)
[snip]
> > > > > @@ -4645,11 +4719,93 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
> > > > > list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
> > > > > if (obj->pages_pin_count == 0)
> > > > > cnt += obj->base.size >> PAGE_SHIFT;
> > > > > - list_for_each_entry(obj, &vm->inactive_list, mm_list)
> > > > > - if (obj->pin_count == 0 && obj->pages_pin_count == 0)
> > > > > - cnt += obj->base.size >> PAGE_SHIFT;
> > > > > +
> > > > > + list_for_each_entry(vm, &dev_priv->vm_list, global_link)
> > > > > + list_for_each_entry(obj, &vm->inactive_list, global_list)
> > > > > + if (obj->pin_count == 0 && obj->pages_pin_count == 0)
> > > > > + cnt += obj->base.size >> PAGE_SHIFT;
> > > >
> > > > Isn't this now double-counting objects? In the shrinker we only care about
> > > > how much physical RAM an object occupies, not how much virtual space it
> > > > occupies. So just walking the bound list of objects here should be good
> > > > enough ...
> > > >
> > >
> > > Maybe I've misunderstood you. My code is wrong, but I think you're idea
> > > requires a prep patch because it changes functionality, right?
> > >
> > > So let me know if I've understood you.
> >
> > Don't we have both the bound and unbound list? So we could just switch
> > over to counting the bound objects here ... Otherwise yes, we need a prep
> > patch to create the bound list first.
>
> Of course there is a bound list.
>
> The old code automatically added the size of unbound objects with
> unpinned pages, and unpinned inactive objects with unpinned pages.
>
> The latter check for inactive, needs to be checked for all VMAs. That
> was my point.
Oh right. The thing is that technically there's no reason to not also scan
the active objects, i.e. just the unbound list. So yeah, sounds like we
need a prep patch to switch to the unbound list here first. My apologies
for being dense and not fully grasphing this right away.
>
> >
> > >
> > > > >
> > > > > if (unlock)
> > > > > mutex_unlock(&dev->struct_mutex);
> > > > > return cnt;
> > > > > }
> > > > > +
> > > > > +/* All the new VM stuff */
> > > > > +unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > > > > + struct i915_address_space *vm)
> > > > > +{
> > > > > + struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> > > > > + struct i915_vma *vma;
> > > > > +
> > > > > + if (vm == &dev_priv->mm.aliasing_ppgtt->base)
> > > > > + vm = &dev_priv->gtt.base;
> > > > > +
> > > > > + BUG_ON(list_empty(&o->vma_list));
> > > > > + list_for_each_entry(vma, &o->vma_list, vma_link) {
> > > >
> > > > Imo the vma list walking here and in the other helpers below indicates
> > > > that we should deal more often in vmas instead of (object, vm) pairs. Or
> > > > is this again something that'll get fixed later on?
> > > >
> > > > I just want to avoid diff churn, and it also makes reviewing easier if the
> > > > foreshadowing is correct ;-) So generally I'd vote for more liberal
> > > > sprinkling of obj_to_vma in callers.
> > >
> > > It's not something I fixed in the whole series. I think it makes sense
> > > conceptually, to keep some things as <obj,vm> and others as direct vma.
> > >
> > > If you want me to change something, you need to be more specific since
> > > no action specifically comes to mind at this point in the series.
> >
> > It's just that the (obj, vm) -> vma lookup is a list-walk, so imo we
> > should try to avoid it whenever possible. Since the vma has both and obj
> > and a vm pointer the vma is imo strictly better than the (obj, vm) pair.
> > And the look-up should be pushed down the callchain as much as possible.
> >
> > So I think generally we want to pass the vma around to functions
> > everywhere, and the (obj, vm) pair would be the exception (which needs
> > special justification).
> >
>
> Without actually coding it, I am not sure. I think there are probably a
> decent number of reasonable exceptions where we want the object (ie.
> it's not really that much of a special case). In any case, I think we'll
> find you have to do this list walk at some point in the call chain
> anyway, but I can try to start changing around the code as a patch on
> top of this. I really want to leave as much as this patch in place as
> is, since it's decently tested (pre-rebase at least).
Ok, I can life with this if we clean things up afterwards. But imo
vma->obj isn't worse for readability than just obj, and passing pairs of
(obj,vm) around all the time just feels wrong conceptually. In C we have
structs for this, and since we already have a suitable one created we
might as well use it.
Aside: I know that we have the (ring, seqno) pair splattered all over the
code. It's been on my todo to fix that ever since I've proposed to add a
i915_gpu_sync_cookie with the original multi-ring enabling. As you can see
I've been ignored, but I hope we can finally fix this with the dma_buf
fence rework.
And we did just recently discover a bug where such a (ring, seqno) pair
got mixed up, so imo not using vma is fraught with unecessary peril.
[snip]
> > > > > @@ -158,13 +158,18 @@ int
> > > > > i915_gem_evict_everything(struct drm_device *dev)
> > > >
> > > > I suspect evict_everything eventually wants a address_space *vm argument
> > > > for those cases where we only want to evict everything in a given vm. Atm
> > > > we have two use-cases of this:
> > > > - Called from the shrinker as a last-ditch effort. For that it should move
> > > > _every_ object onto the unbound list.
> > > > - Called from execbuf for badly-fragmented address spaces to clean up the
> > > > mess. For that case we only care about one address space.
> > >
> > > The current thing is more or less a result of Chris' suggestions. A
> > > non-posted iteration did plumb the vm, and after reworking to the
> > > suggestion made by Chris, the vm didn't make much sense anymore.
> > >
> > > For point #1, it requires VM prioritization I think. I don't really see
> > > any other way to fairly manage it.
> >
> > The shrinker will rip out objects in lru order by walking first unbound
> > and then bound objects. That's imo as fair as it gets, we don't need
> > priorities between vms.
>
> If you pass in a vm, the semantics would be, evict everything for the
> vm, right?
Yes.
>
> >
> > > For point #2, that I agree it might be useful, but we can easily create
> > > a new function, and not call it "shrinker" to do it.
> >
> > Well my point was that this function is called
> > i915_gem_evict_everything(dev, vm) and for the first use case we simply
> > pass in vm = NULL. But essentially thrashing the vm should be rare enough
> > that for now we don't need to care.
> >
>
> IIRC, this is exactly how my original patch worked pre-Chris.
Oops. Do you or Chris still now the argument for changing things? Maybe I
just don't see another facet of the issue at hand ...
[snip]
> > > > > @@ -475,6 +483,7 @@ i915_gem_execbuffer_unreserve_object(struct drm_i915_gem_object *obj)
> > > > > static int
> > > > > i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring,
> > > > > struct list_head *objects,
> > > > > + struct i915_address_space *vm,
> > > > > bool *need_relocs)
> > > > > {
> > > > > struct drm_i915_gem_object *obj;
> > > > > @@ -529,32 +538,37 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring,
> > > > > list_for_each_entry(obj, objects, exec_list) {
> > > > > struct drm_i915_gem_exec_object2 *entry = obj->exec_entry;
> > > > > bool need_fence, need_mappable;
> > > > > + u32 obj_offset;
> > > > >
> > > > > - if (!i915_gem_obj_ggtt_bound(obj))
> > > > > + if (!i915_gem_obj_bound(obj, vm))
> > > > > continue;
> > > >
> > > > I wonder a bit how we could avoid the multipler (obj, vm) -> vma lookups
> > > > here ... Maybe we should cache them in some pointer somewhere (either in
> > > > the eb object or by adding a new pointer to the object struct, e.g.
> > > > obj->eb_vma, similar to obj->eb_list).
> > > >
> > >
> > > I agree, and even did this at one unposted patch too. However, I think
> > > it's a premature optimization which risks code correctness. So I think
> > > somewhere a FIXME needs to happen to address that issue. (Or if Chris
> > > complains bitterly about some perf hit).
> >
> > If you bring up code correctness I'd vote strongly in favour of using vmas
> > everywhere - vma has the (obj, vm) pair locked down, doing the lookup all
> > the thing risks us mixing them up eventually and creating a hella lot of
> > confusion ;-)
>
> I think this is addressed with the previous comments.
See my example for (ring, seqno). I really strongly believe passing pairs
is the wrong thing and passing structs is the right thing. Especially if
we have one at hand. But I'm ok if you want to clean this up afterwards.
Ofc if the cleanup afterwards doesn't happend I'll be a bit pissed ;-)
[snip]
> > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > > > index 245eb1d..bfe61fa 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > > > @@ -391,7 +391,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> > > > > if (gtt_offset == I915_GTT_OFFSET_NONE)
> > > > > return obj;
> > > > >
> > > > > - vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> > > > > + vma = i915_gem_vma_create(obj, vm);
> > > > > if (!vma) {
> > > > > drm_gem_object_unreference(&obj->base);
> > > > > return NULL;
> > > > > @@ -404,8 +404,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> > > > > */
> > > > > vma->node.start = gtt_offset;
> > > > > vma->node.size = size;
> > > > > - if (drm_mm_initialized(&dev_priv->gtt.base.mm)) {
> > > > > - ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
> > > > > + if (drm_mm_initialized(&vm->mm)) {
> > > > > + ret = drm_mm_reserve_node(&vm->mm, &vma->node);
> > > >
> > > > These two hunks here for stolen look fishy - we only ever use the stolen
> > > > preallocated stuff for objects with mappings in the global gtt. So keeping
> > > > that explicit is imo the better approach. And tbh I'm confused where the
> > > > local variable vm is from ...
> > >
> > > If we don't create a vma for it, we potentially have to special case a
> > > bunch of places, I think. I'm not actually sure of this, but the
> > > overhead to do it is quite small.
> > >
> > > Anyway, I'll look this over again nd see what I think.
> >
> > I'm not against the vma, I've just wonedered why you do the
> > /dev_priv->gtt.base/vm/ replacement here since
> > - it's never gonna be used with another vm than ggtt
> > - this patch doesn't add the vm variable, so I'm even more confused where
> > this started ;-)
>
> It started from the rebase. In the original series, I did that
> "deferred_offset" thing, and having a vm variable made that code pass
> the checkpatch.pl. There wasn't a particular reason for naming it vm
> other than I had done it all over the place.
>
> I've fixed this locally, leaving the vma, and renamed the local variable
> ggtt. It still has 3 uses in this function, so it's a bit less typing.
Yeah, make sense to keep it then.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
