| Message ID | 1417618766-14845-2-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Multiple GGTT VMAs per object will be introduced in the near future which will > make it impossible to guarantee normal GGTT view is at the head of the list. > > Purpose of this patch is to break this assumption straight away so any > potential hidden assumptions in the code base can be bisected to this > simple patch. No, please don't. The rationale for putting ggtt first is because it puts the one vma that every object is likely to use at the front, and makes it also the first in the list for debugging. -Chris
On 12/04/2014 09:48 AM, Chris Wilson wrote: > On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Multiple GGTT VMAs per object will be introduced in the near future which will >> make it impossible to guarantee normal GGTT view is at the head of the list. >> >> Purpose of this patch is to break this assumption straight away so any >> potential hidden assumptions in the code base can be bisected to this >> simple patch. > > No, please don't. The rationale for putting ggtt first is because it > puts the one vma that every object is likely to use at the front, and > makes it also the first in the list for debugging. This came from talking with Daniel, but I don't understand why every object is likely to have a GGTT mapping? With multiple GGTT views it may happen that only single GGTT VMA exists in the list but it is not the one current code would expect. So the idea was to break that to flesh out any potential hidden assumptions in the code. (I did not find any by just looking though.) What kind of debugging you have in mind, error capture? Or actually inspecting memory of a live kernel? Regards, Tvrtko
On Thu, Dec 04, 2014 at 10:02:19AM +0000, Tvrtko Ursulin wrote: > > On 12/04/2014 09:48 AM, Chris Wilson wrote: > >On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>Multiple GGTT VMAs per object will be introduced in the near future which will > >>make it impossible to guarantee normal GGTT view is at the head of the list. > >> > >>Purpose of this patch is to break this assumption straight away so any > >>potential hidden assumptions in the code base can be bisected to this > >>simple patch. > > > >No, please don't. The rationale for putting ggtt first is because it > >puts the one vma that every object is likely to use at the front, and > >makes it also the first in the list for debugging. > > This came from talking with Daniel, but I don't understand why every > object is likely to have a GGTT mapping? Because the userspace usage pattern is such that it is typical that every object is accessed through the GTT at some point in its life. It's actually becoming less so as we use alternative mmappings, but it will remain so for quite some time yet. > With multiple GGTT views it may happen that only single GGTT VMA > exists in the list but it is not the one current code would expect. > So the idea was to break that to flesh out any potential hidden > assumptions in the code. (I did not find any by just looking > though.) > > What kind of debugging you have in mind, error capture? Or actually > inspecting memory of a live kernel? Error capture inspection of memory of a live kernel. -Chris
On 12/04/2014 10:17 AM, Chris Wilson wrote: > On Thu, Dec 04, 2014 at 10:02:19AM +0000, Tvrtko Ursulin wrote: >> >> On 12/04/2014 09:48 AM, Chris Wilson wrote: >>> On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> Multiple GGTT VMAs per object will be introduced in the near future which will >>>> make it impossible to guarantee normal GGTT view is at the head of the list. >>>> >>>> Purpose of this patch is to break this assumption straight away so any >>>> potential hidden assumptions in the code base can be bisected to this >>>> simple patch. >>> >>> No, please don't. The rationale for putting ggtt first is because it >>> puts the one vma that every object is likely to use at the front, and >>> makes it also the first in the list for debugging. >> >> This came from talking with Daniel, but I don't understand why every >> object is likely to have a GGTT mapping? > > Because the userspace usage pattern is such that it is typical that > every object is accessed through the GTT at some point in its life. > > It's actually becoming less so as we use alternative mmappings, but it > will remain so for quite some time yet. > >> With multiple GGTT views it may happen that only single GGTT VMA >> exists in the list but it is not the one current code would expect. >> So the idea was to break that to flesh out any potential hidden >> assumptions in the code. (I did not find any by just looking >> though.) >> >> What kind of debugging you have in mind, error capture? Or actually >> inspecting memory of a live kernel? > > Error capture inspection of memory of a live kernel. So personally you don't think it should be of any concern if a GGTT VMA is at the head of the list, but it is not the same GGTT VMA which you would find there in majority of cases? Regards, Tvrtko
On Thu, Dec 04, 2014 at 10:30:30AM +0000, Tvrtko Ursulin wrote: > > On 12/04/2014 10:17 AM, Chris Wilson wrote: > >On Thu, Dec 04, 2014 at 10:02:19AM +0000, Tvrtko Ursulin wrote: > >> > >>On 12/04/2014 09:48 AM, Chris Wilson wrote: > >>>On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote: > >>>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>> > >>>>Multiple GGTT VMAs per object will be introduced in the near future which will > >>>>make it impossible to guarantee normal GGTT view is at the head of the list. > >>>> > >>>>Purpose of this patch is to break this assumption straight away so any > >>>>potential hidden assumptions in the code base can be bisected to this > >>>>simple patch. > >>> > >>>No, please don't. The rationale for putting ggtt first is because it > >>>puts the one vma that every object is likely to use at the front, and > >>>makes it also the first in the list for debugging. > >> > >>This came from talking with Daniel, but I don't understand why every > >>object is likely to have a GGTT mapping? > > > >Because the userspace usage pattern is such that it is typical that > >every object is accessed through the GTT at some point in its life. > > > >It's actually becoming less so as we use alternative mmappings, but it > >will remain so for quite some time yet. > > > >>With multiple GGTT views it may happen that only single GGTT VMA > >>exists in the list but it is not the one current code would expect. > >>So the idea was to break that to flesh out any potential hidden > >>assumptions in the code. (I did not find any by just looking > >>though.) > >> > >>What kind of debugging you have in mind, error capture? Or actually > >>inspecting memory of a live kernel? > > > >Error capture inspection of memory of a live kernel. > > So personally you don't think it should be of any concern if a GGTT > VMA is at the head of the list, but it is not the same GGTT VMA > which you would find there in majority of cases? Actually, regarding error capture it is sorting of the vm list that is important - so a different list. I am still confused about why having different GGTT views makes it difficult putting the normal GGTT vm first. I would very much like the standard GGTT view remain first, and I would actually like the MRU ppgtt last (so that the two most common uses are easily found). -Chris
On 12/04/2014 10:39 AM, Chris Wilson wrote: > On Thu, Dec 04, 2014 at 10:30:30AM +0000, Tvrtko Ursulin wrote: >> So personally you don't think it should be of any concern if a GGTT >> VMA is at the head of the list, but it is not the same GGTT VMA >> which you would find there in majority of cases? > > Actually, regarding error capture it is sorting of the vm list that is > important - so a different list. > > I am still confused about why having different GGTT views makes it > difficult putting the normal GGTT vm first. I would very much like the > standard GGTT view remain first, and I would actually like the MRU > ppgtt last (so that the two most common uses are easily found). It is not difficult, but it is not guaranteed that, when there is a single GGTT vma on the list, it is a normal one. + Daniel since this is at least how I interpreted our discussion about this. Maybe I got it wrong, who knows. Regards, Tvrtko
On Thu, Dec 04, 2014 at 09:48:11AM +0000, Chris Wilson wrote: > On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Multiple GGTT VMAs per object will be introduced in the near future which will > > make it impossible to guarantee normal GGTT view is at the head of the list. > > > > Purpose of this patch is to break this assumption straight away so any > > potential hidden assumptions in the code base can be bisected to this > > simple patch. > > No, please don't. The rationale for putting ggtt first is because it > puts the one vma that every object is likely to use at the front, and > makes it also the first in the list for debugging. With hindsight I think this was just premature optimization (and way too many obj->vma lookups to boot). Imo we should just fix the remaining spurious obj->vma lookups we have and if there's still a problem then we need to have actual benchmark data or profiles to support this trick. Hence I pulled this patch in, thanks. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 9d362d3..c1c1141 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5303,11 +5303,13 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr) struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj) { + struct i915_address_space *ggtt = i915_obj_to_ggtt(obj); struct i915_vma *vma; - vma = list_first_entry(&obj->vma_list, typeof(*vma), vma_link); - if (vma->vm != i915_obj_to_ggtt(obj)) - return NULL; + list_for_each_entry(vma, &obj->vma_list, vma_link) { + if (vma->vm == ggtt) + return vma; + } - return vma; + return NULL; } diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 171f6ea..ac03a38 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2202,13 +2202,9 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj, BUG(); } - /* Keep GGTT vmas first to make debug easier */ - if (i915_is_ggtt(vm)) - list_add(&vma->vma_link, &obj->vma_list); - else { - list_add_tail(&vma->vma_link, &obj->vma_list); + list_add_tail(&vma->vma_link, &obj->vma_list); + if (!i915_is_ggtt(vm)) i915_ppgtt_get(i915_vm_to_ppgtt(vm)); - } return vma; }