| Message ID | 1375315222-4785-21-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 31, 2013 at 05:00:13PM -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 a 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 d4d6444..ec23a5c 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2626,7 +2626,7 @@ int i915_vma_unbind(struct i915_vma *vma) > > trace_i915_vma_unbind(vma); > > - if (obj->has_global_gtt_mapping) > + if (obj->has_global_gtt_mapping && i915_is_ggtt(vma->vm)) > i915_gem_gtt_unbind_object(obj); > if (obj->has_aliasing_ppgtt_mapping) { > i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); Hm, shouldn't we do the is_ggtt check for both? After all only the global ggtt can be aliased ever ... This would also be more symmetric with some of the other global gtt checks I've spotted. You're take or will that run afoul of your Great Plan? -Daniel > @@ -2637,7 +2637,8 @@ int i915_vma_unbind(struct i915_vma *vma) > > list_del(&obj->mm_list); > /* Avoid an unnecessary call to unbind on rebind. */ > - obj->map_and_fenceable = true; > + if (i915_is_ggtt(vma->vm)) > + obj->map_and_fenceable = true; > > i915_gem_vma_destroy(vma); > > @@ -3196,7 +3197,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_vma_bind(vma, map_and_fenceable); > i915_gem_verify_gtt(dev); > -- > 1.8.3.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Aug 06, 2013 at 09:11:54PM +0200, Daniel Vetter wrote: > On Wed, Jul 31, 2013 at 05:00:13PM -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 a 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 d4d6444..ec23a5c 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -2626,7 +2626,7 @@ int i915_vma_unbind(struct i915_vma *vma) > > > > trace_i915_vma_unbind(vma); > > > > - if (obj->has_global_gtt_mapping) > > + if (obj->has_global_gtt_mapping && i915_is_ggtt(vma->vm)) > > i915_gem_gtt_unbind_object(obj); > > if (obj->has_aliasing_ppgtt_mapping) { > > i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); > > Hm, shouldn't we do the is_ggtt check for both? After all only the global > ggtt can be aliased ever ... This would also be more symmetric with some > of the other global gtt checks I've spotted. You're take or will that run > afoul of your Great Plan? > -Daniel > You're right. The check makes sense for both cases. In both the original series, and n a few patches, this code turns into: vma->vm->unbind_vma(vma); This ugliness is a result of bad rebasing on my part.
On Wed, Aug 07, 2013 at 11:37:04AM -0700, Ben Widawsky wrote: > On Tue, Aug 06, 2013 at 09:11:54PM +0200, Daniel Vetter wrote: > > On Wed, Jul 31, 2013 at 05:00:13PM -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 a 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 d4d6444..ec23a5c 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > @@ -2626,7 +2626,7 @@ int i915_vma_unbind(struct i915_vma *vma) > > > > > > trace_i915_vma_unbind(vma); > > > > > > - if (obj->has_global_gtt_mapping) > > > + if (obj->has_global_gtt_mapping && i915_is_ggtt(vma->vm)) > > > i915_gem_gtt_unbind_object(obj); > > > if (obj->has_aliasing_ppgtt_mapping) { > > > i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); > > > > Hm, shouldn't we do the is_ggtt check for both? After all only the global > > ggtt can be aliased ever ... This would also be more symmetric with some > > of the other global gtt checks I've spotted. You're take or will that run > > afoul of your Great Plan? > > -Daniel > > > > You're right. The check makes sense for both cases. In both the original > series, and n a few patches, this code turns into: > vma->vm->unbind_vma(vma); Ok, I've killed it and merged the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index d4d6444..ec23a5c 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2626,7 +2626,7 @@ int i915_vma_unbind(struct i915_vma *vma) trace_i915_vma_unbind(vma); - if (obj->has_global_gtt_mapping) + if (obj->has_global_gtt_mapping && i915_is_ggtt(vma->vm)) i915_gem_gtt_unbind_object(obj); if (obj->has_aliasing_ppgtt_mapping) { i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); @@ -2637,7 +2637,8 @@ int i915_vma_unbind(struct i915_vma *vma) list_del(&obj->mm_list); /* Avoid an unnecessary call to unbind on rebind. */ - obj->map_and_fenceable = true; + if (i915_is_ggtt(vma->vm)) + obj->map_and_fenceable = true; i915_gem_vma_destroy(vma); @@ -3196,7 +3197,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_vma_bind(vma, 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 a 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(-)