From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [PATCH] drm/i915: Always reset vma->ggtt_view.pages cache on unbinding Date: Thu, 11 Jun 2015 10:59:24 +0100 Message-ID: <55795BFC.8010508@linux.intel.com> References: <1434006368-26742-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id DBEE46E9E2 for ; Thu, 11 Jun 2015 02:59:26 -0700 (PDT) In-Reply-To: <1434006368-26742-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , intel-gfx@lists.freedesktop.org Cc: Daniel Vetter , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org Ck9uIDA2LzExLzIwMTUgMDg6MDYgQU0sIENocmlzIFdpbHNvbiB3cm90ZToKPiBXaXRoIHRoZSBp bnRyb2R1Y3Rpb24gb2YgbXVsdGlwbGUgdmlld3Mgb2YgYW4gb2JqIGluIHRoZSBzYW1lIHZtLCBl YWNoCj4gdm1hIHdhcyB0YXVnaHQgdG8gY2FjaGUgaXRzIGNvcHkgb2YgdGhlIHBhZ2VzIChzbyB0 aGF0IGRpZmZlcmVudCB2aWV3cwo+IGNvdWxkIGhhdmUgZGlmZmVyZW50IHBhZ2UgYXJyYW5nZW1l bnRzKS4gSG93ZXZlciwgdGhpcyBtaXNzZWQgZGVjb3VwbGluZwo+IHRob3NlIHZtYS0+Z2d0dF92 aWV3LnBhZ2VzIHdoZW4gdGhlIHZtYSByZWxlYXNlZCBpdHMgcmVmZXJlbmNlIG9uIHRoZQo+IG9i ai0+cGFnZXMuIEFzIHdlIGRvbid0IGFsd2F5cyBmcmVlIHRoZSB2bWEsIHRoaXMgbGVhZHMgdG8g YSBwb3NzaWJsZQo+IHNjZW5hcmlvIChlLmcuIGV4ZWNidWZmZXIgaW50ZXJydXB0ZWQgYnkgdGhl IHNocmlua2VyKSB3aGVyZSB0aGUgdm1hCj4gcG9pbnRzIHRvIGEgc3RhbGUgb2JqLT5wYWdlcywg YW5kIGV4cGxvZGVzLgo+Cj4gRml4ZXMgcmVncmVzc2lvbiBmcm9tIGNvbW1pdCBmZTE0ZDVmNGU1 NDY4YzViODBhMjRmMWE2NGFiY2JlMTE2MTQzNjcwCj4gQXV0aG9yOiBUdnJ0a28gVXJzdWxpbiA8 dHZydGtvLnVyc3VsaW5AaW50ZWwuY29tPgo+IERhdGU6ICAgV2VkIERlYyAxMCAxNzoyNzo1OCAy MDE0ICswMDAwCj4KPiAgICAgIGRybS9pOTE1OiBJbmZyYXN0cnVjdHVyZSBmb3Igc3VwcG9ydGlu ZyBkaWZmZXJlbnQgR0dUVCB2aWV3cyBwZXIgb2JqZWN0Cj4KPiBCdWd6aWxsYTogaHR0cHM6Ly9i dWd6aWxsYS5yZWRoYXQuY29tL3Nob3dfYnVnLmNnaT9pZD0xMjI3ODkyCj4gU2lnbmVkLW9mZi1i eTogQ2hyaXMgV2lsc29uIDxjaHJpc0BjaHJpcy13aWxzb24uY28udWs+Cj4gQ2M6IFR2cnRrbyBV cnN1bGluIDx0dnJ0a28udXJzdWxpbkBpbnRlbC5jb20+Cj4gQ2M6IERhbmllbCBWZXR0ZXIgPGRh bmllbC52ZXR0ZXJAZmZ3bGwuY2g+Cj4gQ2M6IE1pY2hlbCBUaGllcnJ5IDxtaWNoZWwudGhpZXJy eUBpbnRlbC5jb20+Cj4gQ2M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcKPiAtLS0KPiAgIGRyaXZl cnMvZ3B1L2RybS9pOTE1L2k5MTVfZ2VtLmMgfCAyICstCj4gICAxIGZpbGUgY2hhbmdlZCwgMSBp bnNlcnRpb24oKyksIDEgZGVsZXRpb24oLSkKPgo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9k cm0vaTkxNS9pOTE1X2dlbS5jIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9nZW0uYwo+IGlu ZGV4IDlhZTk4YjAwZmY1Ni4uMzc3YTZkYTMxYTFjIDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvZ3B1 L2RybS9pOTE1L2k5MTVfZ2VtLmMKPiArKysgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2dl bS5jCj4gQEAgLTMyMTQsOCArMzIxNCw4IEBAIGludCBpOTE1X3ZtYV91bmJpbmQoc3RydWN0IGk5 MTVfdm1hICp2bWEpCj4gICAJCX0gZWxzZSBpZiAodm1hLT5nZ3R0X3ZpZXcucGFnZXMpIHsKPiAg IAkJCXNnX2ZyZWVfdGFibGUodm1hLT5nZ3R0X3ZpZXcucGFnZXMpOwo+ICAgCQkJa2ZyZWUodm1h LT5nZ3R0X3ZpZXcucGFnZXMpOwo+IC0JCQl2bWEtPmdndHRfdmlldy5wYWdlcyA9IE5VTEw7Cj4g ICAJCX0KPiArCQl2bWEtPmdndHRfdmlldy5wYWdlcyA9IE5VTEw7Cj4gICAJfQo+Cj4gICAJZHJt X21tX3JlbW92ZV9ub2RlKCZ2bWEtPm5vZGUpOwoKTmFzdHksIHRoYW5rcyBmb3IgZml4aW5nIHRo aXMuCgpSZXZpZXdlZC1ieTogVHZydGtvIFVyc3VsaW4gPHR2cnRrby51cnN1bGluQGludGVsLmNv bT4KCklmIHNvbWVvbmUgZWxzZSB3aWxsIGJlIGNvbmZ1c2VkIGhvdyB0aGlzIGNhbiBoYXBwZW4s IGtleSBpcyB0aGUgCnJlc2VydmF0aW9uIGV4ZWNidWZmZXIgcGF0aC4gVGhhdCBwdXRzIHRoZSBW TUEgb24gdGhlIGV4ZWNfbGlzdCB3aGljaCAKcHJldmVudHMgaTkxNV92bWFfdW5iaW5kIGFuZCBp OTE1X2dlbV92bWFfZGVzdHJveSBmcm9tIGZ1bGx5IGRlc3Ryb3lpbmcgCnRoZSBWTUEuIFNvIHRo ZSBWTUEgaXMgbGVmdCBleGlzdGluZyBhcyBhbiBlbXB0eSBvYmplY3QgaW4gdGhlIGxpc3QgLSAK dW5ib3VuZCBhbmQgZGlzYXNzb2NpYXRlZCB3aXRoIHRoZSBiYWNraW5nIHN0b3JlLiBLaW5kIG9m IGEgY2FjaGVkIAptZW1vcnkgb2JqZWN0LiBBbmQgdGhlbiByZS11c2luZyBpdCBuZWVkcyB0byBj bGVhciB0aGUgY2FjaGVkIHBhZ2VzIApwb2ludGVyIHdoaWNoIGlzIGZpeGVkIGFib3ZlLgoKUmVn YXJkcywKClR2cnRrbwpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fXwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5v cmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2ludGVsLWdm eAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com ([192.55.52.115]:4399 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754287AbbFKJ71 (ORCPT ); Thu, 11 Jun 2015 05:59:27 -0400 Message-ID: <55795BFC.8010508@linux.intel.com> Date: Thu, 11 Jun 2015 10:59:24 +0100 From: Tvrtko Ursulin MIME-Version: 1.0 To: Chris Wilson , intel-gfx@lists.freedesktop.org CC: Daniel Vetter , stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: Always reset vma->ggtt_view.pages cache on unbinding References: <1434006368-26742-1-git-send-email-chris@chris-wilson.co.uk> In-Reply-To: <1434006368-26742-1-git-send-email-chris@chris-wilson.co.uk> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: On 06/11/2015 08:06 AM, Chris Wilson wrote: > With the introduction of multiple views of an obj in the same vm, each > vma was taught to cache its copy of the pages (so that different views > could have different page arrangements). However, this missed decoupling > those vma->ggtt_view.pages when the vma released its reference on the > obj->pages. As we don't always free the vma, this leads to a possible > scenario (e.g. execbuffer interrupted by the shrinker) where the vma > points to a stale obj->pages, and explodes. > > Fixes regression from commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 > Author: Tvrtko Ursulin > Date: Wed Dec 10 17:27:58 2014 +0000 > > drm/i915: Infrastructure for supporting different GGTT views per object > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1227892 > Signed-off-by: Chris Wilson > Cc: Tvrtko Ursulin > Cc: Daniel Vetter > Cc: Michel Thierry > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/i915_gem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 9ae98b00ff56..377a6da31a1c 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3214,8 +3214,8 @@ int i915_vma_unbind(struct i915_vma *vma) > } else if (vma->ggtt_view.pages) { > sg_free_table(vma->ggtt_view.pages); > kfree(vma->ggtt_view.pages); > - vma->ggtt_view.pages = NULL; > } > + vma->ggtt_view.pages = NULL; > } > > drm_mm_remove_node(&vma->node); Nasty, thanks for fixing this. Reviewed-by: Tvrtko Ursulin If someone else will be confused how this can happen, key is the reservation execbuffer path. That puts the VMA on the exec_list which prevents i915_vma_unbind and i915_gem_vma_destroy from fully destroying the VMA. So the VMA is left existing as an empty object in the list - unbound and disassociated with the backing store. Kind of a cached memory object. And then re-using it needs to clear the cached pages pointer which is fixed above. Regards, Tvrtko