| Message ID | 1373350122-5118-8-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 08, 2013 at 11:08:38PM -0700, Ben Widawsky wrote: > formerly: "drm/i915: Create VMAs (part 3.5) - map and fenceable > tracking" > > The map_and_fenceable tracking is per object. GTT mapping, and fences > only apply to global GTT. As such, object operations which are not > performed on the global GTT should not effect mappable or fenceable > characteristics. > > Functionally, this commit could very well be squashed in to the previous > patch which updated object operations to take a VM argument. This > commit is split out because it's a bit tricky (or at least it was for > me). > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 21015cd..501c590 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2635,7 +2635,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj, > > trace_i915_gem_object_unbind(obj, vm); > > - if (obj->has_global_gtt_mapping) > + if (obj->has_global_gtt_mapping && i915_is_ggtt(vm)) > i915_gem_gtt_unbind_object(obj); Wont this part be done as part of the global gtt clear_range callback? > if (obj->has_aliasing_ppgtt_mapping) { > i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); And I have a hunch that we should shovel the aliasing ppgtt clearing into the ggtt write_ptes/clear_range callbacks, too. Once all this has settled at least. > @@ -2646,7 +2646,8 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj, > > list_del(&obj->mm_list); > /* Avoid an unnecessary call to unbind on rebind. */ > - obj->map_and_fenceable = true; > + if (i915_is_ggtt(vm)) > + obj->map_and_fenceable = true; > > vma = i915_gem_obj_to_vma(obj, vm); > list_del(&vma->vma_link); > @@ -3213,7 +3214,9 @@ search_free: > i915_is_ggtt(vm) && > vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; > > - obj->map_and_fenceable = mappable && fenceable; > + /* Map and fenceable only changes if the VM is the global GGTT */ > + if (i915_is_ggtt(vm)) > + obj->map_and_fenceable = mappable && fenceable; > > trace_i915_gem_object_bind(obj, vm, map_and_fenceable); > i915_gem_verify_gtt(dev); > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jul 09, 2013 at 09:16:54AM +0200, Daniel Vetter wrote: > On Mon, Jul 08, 2013 at 11:08:38PM -0700, Ben Widawsky wrote: > > formerly: "drm/i915: Create VMAs (part 3.5) - map and fenceable > > tracking" > > > > The map_and_fenceable tracking is per object. GTT mapping, and fences > > only apply to global GTT. As such, object operations which are not > > performed on the global GTT should not effect mappable or fenceable > > characteristics. > > > > Functionally, this commit could very well be squashed in to the previous > > patch which updated object operations to take a VM argument. This > > commit is split out because it's a bit tricky (or at least it was for > > me). > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 21015cd..501c590 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -2635,7 +2635,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj, > > > > trace_i915_gem_object_unbind(obj, vm); > > > > - if (obj->has_global_gtt_mapping) > > + if (obj->has_global_gtt_mapping && i915_is_ggtt(vm)) > > i915_gem_gtt_unbind_object(obj); > > Wont this part be done as part of the global gtt clear_range callback? > > > if (obj->has_aliasing_ppgtt_mapping) { > > i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); > > And I have a hunch that we should shovel the aliasing ppgtt clearing into > the ggtt write_ptes/clear_range callbacks, too. Once all this has settled > at least. > Addressing both comments at once: First, this is a rebase mistake AFAICT because this hunk doesn't really belong in this patch anyway. Eventually, I'd want to kill i915_gem_gtt_unbind_object, and i915_ppgtt_unbind_object. In the 66 patch series, I killed the latter, but decided to leave the former to make it clear that is a special case. In the original 66 patch series, I did not move clear_range which is probably why this was left like this. I believe bind was fixed to just be vm->bleh() If you're good with the idea, I'll add a new patch to remove those and use the i915_address_space. I'll do the same in other applicable places. It's easiest if I do that as a patch 12, I think, if you don't mind? I do think this hunk belongs in another patch though until I do the above. I'm not really sure where to put that. > > @@ -2646,7 +2646,8 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj, > > > > list_del(&obj->mm_list); > > /* Avoid an unnecessary call to unbind on rebind. */ > > - obj->map_and_fenceable = true; > > + if (i915_is_ggtt(vm)) > > + obj->map_and_fenceable = true; > > > > vma = i915_gem_obj_to_vma(obj, vm); > > list_del(&vma->vma_link); > > @@ -3213,7 +3214,9 @@ search_free: > > i915_is_ggtt(vm) && > > vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; > > > > - obj->map_and_fenceable = mappable && fenceable; > > + /* Map and fenceable only changes if the VM is the global GGTT */ > > + if (i915_is_ggtt(vm)) > > + obj->map_and_fenceable = mappable && fenceable; > > > > trace_i915_gem_object_bind(obj, vm, map_and_fenceable); > > i915_gem_verify_gtt(dev); > > -- > > 1.8.3.2 > > > > _______________________________________________ > > 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
On Wed, Jul 10, 2013 at 09:39:00AM -0700, Ben Widawsky wrote: > On Tue, Jul 09, 2013 at 09:16:54AM +0200, Daniel Vetter wrote: > > On Mon, Jul 08, 2013 at 11:08:38PM -0700, Ben Widawsky wrote: > > > formerly: "drm/i915: Create VMAs (part 3.5) - map and fenceable > > > tracking" > > > > > > The map_and_fenceable tracking is per object. GTT mapping, and fences > > > only apply to global GTT. As such, object operations which are not > > > performed on the global GTT should not effect mappable or fenceable > > > characteristics. > > > > > > Functionally, this commit could very well be squashed in to the previous > > > patch which updated object operations to take a VM argument. This > > > commit is split out because it's a bit tricky (or at least it was for > > > me). > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > --- > > > drivers/gpu/drm/i915/i915_gem.c | 9 ++++++--- > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > > index 21015cd..501c590 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > @@ -2635,7 +2635,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj, > > > > > > trace_i915_gem_object_unbind(obj, vm); > > > > > > - if (obj->has_global_gtt_mapping) > > > + if (obj->has_global_gtt_mapping && i915_is_ggtt(vm)) > > > i915_gem_gtt_unbind_object(obj); > > > > Wont this part be done as part of the global gtt clear_range callback? > > > > > if (obj->has_aliasing_ppgtt_mapping) { > > > i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); > > > > And I have a hunch that we should shovel the aliasing ppgtt clearing into > > the ggtt write_ptes/clear_range callbacks, too. Once all this has settled > > at least. > > > > Addressing both comments at once: > > First, this is a rebase mistake AFAICT because this hunk doesn't really > belong in this patch anyway. > > Eventually, I'd want to kill i915_gem_gtt_unbind_object, and > i915_ppgtt_unbind_object. In the 66 patch series, I killed the latter, > but decided to leave the former to make it clear that is a special case. > > In the original 66 patch series, I did not move clear_range which is > probably why this was left like this. I believe bind was fixed to just > be vm->bleh() > > If you're good with the idea, I'll add a new patch to remove those and > use the i915_address_space. I'll do the same in other applicable places. > It's easiest if I do that as a patch 12, I think, if you don't mind? Yeah, I'm ok with that, since the above hunk that started my thinking will go away anyway. > I do think this hunk belongs in another patch though until I do the > above. I'm not really sure where to put that. If you want to keep it, please add a comment explaining why and how exactly it will get fixed up. In case of doubt just create a new patch to highlight the special case imo. -Daniel > > > > > @@ -2646,7 +2646,8 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj, > > > > > > list_del(&obj->mm_list); > > > /* Avoid an unnecessary call to unbind on rebind. */ > > > - obj->map_and_fenceable = true; > > > + if (i915_is_ggtt(vm)) > > > + obj->map_and_fenceable = true; > > > > > > vma = i915_gem_obj_to_vma(obj, vm); > > > list_del(&vma->vma_link); > > > @@ -3213,7 +3214,9 @@ search_free: > > > i915_is_ggtt(vm) && > > > vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; > > > > > > - obj->map_and_fenceable = mappable && fenceable; > > > + /* Map and fenceable only changes if the VM is the global GGTT */ > > > + if (i915_is_ggtt(vm)) > > > + obj->map_and_fenceable = mappable && fenceable; > > > > > > trace_i915_gem_object_bind(obj, vm, map_and_fenceable); > > > i915_gem_verify_gtt(dev); > > > -- > > > 1.8.3.2 > > > > > > _______________________________________________ > > > 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 > > -- > Ben Widawsky, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 21015cd..501c590 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2635,7 +2635,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj, trace_i915_gem_object_unbind(obj, vm); - if (obj->has_global_gtt_mapping) + if (obj->has_global_gtt_mapping && i915_is_ggtt(vm)) i915_gem_gtt_unbind_object(obj); if (obj->has_aliasing_ppgtt_mapping) { i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); @@ -2646,7 +2646,8 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj, list_del(&obj->mm_list); /* Avoid an unnecessary call to unbind on rebind. */ - obj->map_and_fenceable = true; + if (i915_is_ggtt(vm)) + obj->map_and_fenceable = true; vma = i915_gem_obj_to_vma(obj, vm); list_del(&vma->vma_link); @@ -3213,7 +3214,9 @@ search_free: i915_is_ggtt(vm) && vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; - obj->map_and_fenceable = mappable && fenceable; + /* Map and fenceable only changes if the VM is the global GGTT */ + if (i915_is_ggtt(vm)) + obj->map_and_fenceable = mappable && fenceable; trace_i915_gem_object_bind(obj, vm, map_and_fenceable); i915_gem_verify_gtt(dev);
formerly: "drm/i915: Create VMAs (part 3.5) - map and fenceable tracking" The map_and_fenceable tracking is per object. GTT mapping, and fences only apply to global GTT. As such, object operations which are not performed on the global GTT should not effect mappable or fenceable characteristics. Functionally, this commit could very well be squashed in to the previous patch which updated object operations to take a VM argument. This commit is split out because it's a bit tricky (or at least it was for me). Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)