[PATCH] drm/i915: Unpin vma iomapping when fbdev is destroyed

[PATCH] drm/i915: Unpin vma iomapping when fbdev is destroyed

[Tvrtko Ursulin]

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.

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.

Compile tested only.
---
 drivers/gpu/drm/i915/intel_fbdev.c | 1 +
 1 file changed, 1 insertion(+)

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);
 
-- 
2.9.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Unpin vma iomapping when fbdev is destroyed

[Patchwork]

== Series Details ==

Series: drm/i915: Unpin vma iomapping when fbdev is destroyed
URL   : https://patchwork.freedesktop.org/series/26802/
State : success

== Summary ==

Series 26802v1 drm/i915: Unpin vma iomapping when fbdev is destroyed
https://patchwork.freedesktop.org/api/1.0/series/26802/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-pnv-d510) fdo#101597 +1

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597

fi-bdw-5557u     total:279  pass:264  dwarn:0   dfail:0   fail:3   skip:11  time:435s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:428s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:356s
fi-bsw-n3050     total:279  pass:239  dwarn:0   dfail:0   fail:3   skip:36  time:516s
fi-bxt-j4205     total:279  pass:256  dwarn:0   dfail:0   fail:3   skip:19  time:505s
fi-byt-j1900     total:279  pass:250  dwarn:1   dfail:0   fail:3   skip:24  time:483s
fi-byt-n2820     total:279  pass:246  dwarn:1   dfail:0   fail:3   skip:28  time:482s
fi-glk-2a        total:279  pass:256  dwarn:0   dfail:0   fail:3   skip:19  time:594s
fi-hsw-4770      total:279  pass:259  dwarn:0   dfail:0   fail:3   skip:16  time:430s
fi-hsw-4770r     total:279  pass:259  dwarn:0   dfail:0   fail:3   skip:16  time:412s
fi-ilk-650       total:279  pass:225  dwarn:0   dfail:0   fail:3   skip:50  time:417s
fi-ivb-3520m     total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:495s
fi-ivb-3770      total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:470s
fi-kbl-7500u     total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:459s
fi-kbl-7560u     total:279  pass:264  dwarn:1   dfail:0   fail:3   skip:10  time:564s
fi-kbl-r         total:279  pass:256  dwarn:1   dfail:0   fail:3   skip:18  time:563s
fi-pnv-d510      total:279  pass:222  dwarn:2   dfail:0   fail:0   skip:55  time:561s
fi-skl-6260u     total:279  pass:265  dwarn:0   dfail:0   fail:3   skip:10  time:459s
fi-skl-6700hq    total:279  pass:219  dwarn:1   dfail:0   fail:33  skip:24  time:302s
fi-skl-6700k     total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:463s
fi-skl-6770hq    total:279  pass:265  dwarn:0   dfail:0   fail:3   skip:10  time:477s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:439s
fi-snb-2520m     total:279  pass:247  dwarn:0   dfail:0   fail:3   skip:28  time:536s
fi-snb-2600      total:279  pass:245  dwarn:0   dfail:0   fail:4   skip:29  time:404s

cadaf524db971245c2744e7a76fa6a89143d8cea drm-tip: 2017y-07m-04d-07h-49m-57s UTC integration manifest
9e7f3ca drm/i915: Unpin vma iomapping when fbdev is destroyed

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5104/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Unpin vma iomapping when fbdev is destroyed

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Unpin vma iomapping when fbdev is destroyed

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Unpin vma iomapping when fbdev is destroyed

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Unpin vma iomapping when fbdev is destroyed

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Unpin vma iomapping when fbdev is destroyed

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx