| Message ID | 20170704094640.24991-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Tvrtko Ursulin (2017-07-04 10:46:40) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > If we don't release the iomapping we are not able to unpin the > vma which then gets leaked. Oh, we still do unpin the vma on closing the object and we don't hold any extra object reference for the iomap. It is still a good patch for the symmetry, except it doesn't do what you say :-) > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > Some time last year we talked about a pending patch to get rid > of the fbdev VMA leak. I lost track of what happened with that. That was fixed by the intel_unpin_fb_vma() in there. -Chris
On 04/07/2017 11:17, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2017-07-04 10:46:40) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> If we don't release the iomapping we are not able to unpin the >> vma which then gets leaked. > > Oh, we still do unpin the vma on closing the object and we don't hold > any extra object reference for the iomap. It is still a good patch for > the symmetry, except it doesn't do what you say :-) What do you mean that we don't hold any extra reference for the iomap? I see i915_vma_pin_iomap -> __i915_vma_pin -> vma->flags++. I can't spot the place which would override this and still unbind it at some point. >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> Some time last year we talked about a pending patch to get rid >> of the fbdev VMA leak. I lost track of what happened with that. > > That was fixed by the intel_unpin_fb_vma() in there. I suppose I don't see how since I don't see what defeats the elevated pin count from the above. Regards, Tvrtko
Quoting Tvrtko Ursulin (2017-07-04 12:26:31) > > On 04/07/2017 11:17, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2017-07-04 10:46:40) > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >> If we don't release the iomapping we are not able to unpin the > >> vma which then gets leaked. > > > > Oh, we still do unpin the vma on closing the object and we don't hold > > any extra object reference for the iomap. It is still a good patch for > > the symmetry, except it doesn't do what you say :-) > > What do you mean that we don't hold any extra reference for the iomap? I > see i915_vma_pin_iomap -> __i915_vma_pin -> vma->flags++. I can't spot > the place which would override this and still unbind it at some point. free_objects: vma->flags &= ~I915_VMA_PIN_MASK; i915_vma_close(vma); -Chris
On 04/07/2017 12:29, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2017-07-04 12:26:31) >> >> On 04/07/2017 11:17, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2017-07-04 10:46:40) >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> If we don't release the iomapping we are not able to unpin the >>>> vma which then gets leaked. >>> >>> Oh, we still do unpin the vma on closing the object and we don't hold >>> any extra object reference for the iomap. It is still a good patch for >>> the symmetry, except it doesn't do what you say :-) >> >> What do you mean that we don't hold any extra reference for the iomap? I >> see i915_vma_pin_iomap -> __i915_vma_pin -> vma->flags++. I can't spot >> the place which would override this and still unbind it at some point. > > free_objects: > vma->flags &= ~I915_VMA_PIN_MASK; > i915_vma_close(vma); Blast.. :) Do we need the unpin in the API at all then? Regards, Tvrtko
Quoting Tvrtko Ursulin (2017-07-04 13:33:28) > > On 04/07/2017 12:29, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2017-07-04 12:26:31) > >> > >> On 04/07/2017 11:17, Chris Wilson wrote: > >>> Quoting Tvrtko Ursulin (2017-07-04 10:46:40) > >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>> > >>>> If we don't release the iomapping we are not able to unpin the > >>>> vma which then gets leaked. > >>> > >>> Oh, we still do unpin the vma on closing the object and we don't hold > >>> any extra object reference for the iomap. It is still a good patch for > >>> the symmetry, except it doesn't do what you say :-) > >> > >> What do you mean that we don't hold any extra reference for the iomap? I > >> see i915_vma_pin_iomap -> __i915_vma_pin -> vma->flags++. I can't spot > >> the place which would override this and still unbind it at some point. > > > > free_objects: > > vma->flags &= ~I915_VMA_PIN_MASK; > > i915_vma_close(vma); > > Blast.. :) Do we need the unpin in the API at all then? Yes, free_objects is a little late in general :) If we fix up fbdev, we should be close enough to make that a if (WARN_ON(i915_vma_is_pinned(vma))) vma->flags &= ~I915_VMA_PIN_MASK; -Chris
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 03347c6ae599..46831021236f 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -537,6 +537,7 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev) if (ifbdev->fb) { mutex_lock(&ifbdev->helper.dev->struct_mutex); + i915_vma_unpin_iomap(ifbdev->vma); intel_unpin_fb_vma(ifbdev->vma); mutex_unlock(&ifbdev->helper.dev->struct_mutex);