From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb. Date: Tue, 22 Mar 2016 10:32:32 +0100 Message-ID: <56F11130.5020506@linux.intel.com> References: <1458569477-13364-1-git-send-email-maarten.lankhorst@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: stable , intel-gfx , Thomas Hellstrom , dri-devel List-Id: dri-devel@lists.freedesktop.org T3AgMjEtMDMtMTYgb20gMTg6Mzcgc2NocmVlZiBEYW5pZWwgVmV0dGVyOgo+IE9uIE1vbiwgTWFy IDIxLCAyMDE2IGF0IDAzOjExOjE3UE0gKzAxMDAsIE1hYXJ0ZW4gTGFua2hvcnN0IHdyb3RlOgo+ PiBJdCB0dXJucyBvdXQgdGhhdCBwcmVzZXJ2aW5nIGZyYW1lYnVmZmVycyBhZnRlciB0aGUgcm1m YiBjYWxsIGJyZWFrcwo+PiB2bXdnZnggdXNlcnNwYWNlLiBUaGlzIHdhcyBvcmlnaW5hbGx5IGlu dHJvZHVjZWQgYmVjYXVzZSBpdCB3YXMgdGhvdWdodAo+PiBub2JvZHkgcmVsaWVkIG9uIHRoZSBi ZWhhdmlvciwgYnV0IHVuZm9ydHVuYXRlbHkgaXQgc2VlbXMgdGhlcmUgYXJlCj4+IGV4Y2VwdGlv bnMuCj4+Cj4+IGRybV9mcmFtZWJ1ZmZlcl9yZW1vdmUgbWF5IGZhaWwgd2l0aCAtRUlOVFIgbm93 LCBzbyBhIHN0cmFpZ2h0IHJldmVydAo+PiBpcyBpbXBvc3NpYmxlLiBUaGVyZSBpcyBubyB3YXkg dG8gcmVtb3ZlIHRoZSBmcmFtZWJ1ZmZlciBmcm9tIHRoZSBsaXN0cwo+PiBhbmQgYWN0aXZlIHBs YW5lcyB3aXRob3V0IGludHJvZHVjaW5nIGEgcmFjZSBiZWNhdXNlIG9mIHRoZSBkaWZmZXJlbnQK Pj4gbG9ja2luZyByZXF1aXJlbWVudHMuIEluc3RlYWQgY2FsbCBkcm1fZnJhbWVidWZmZXJfcmVt b3ZlIGZyb20gYQo+PiB3b3JrcXVldWUsIHdoaWNoIGlzIHVuYWZmZWN0ZWQgYnkgc2lnbmFscy4K Pj4KPj4gQ2M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcgI3Y0LjQrCj4+IEZpeGVzOiAxMzgwMzEz MjgxOGMgKCJkcm0vY29yZTogUHJlc2VydmUgdGhlIGZyYW1lYnVmZmVyIGFmdGVyIHJlbW92aW5n IGl0LiIpCj4+IFRlc3RjYXNlOiBrbXNfZmxpcC5mbGlwLXZzLXJtZmItaW50ZXJydXB0aWJsZQo+ PiBSZWZlcmVuY2VzOiBodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9hcmNoaXZlcy9kcmkt ZGV2ZWwvMjAxNi1NYXJjaC8xMDI4NzYuaHRtbAo+PiBDYzogVGhvbWFzIEhlbGxzdHJvbSA8dGhl bGxzdHJvbUB2bXdhcmUuY29tPgo+PiBDYzogRGF2aWQgSGVycm1hbm4gPGRoLmhlcnJtYW5uQGdt YWlsLmNvbT4KPj4gLS0tCj4+ICBkcml2ZXJzL2dwdS9kcm0vZHJtX2NydGMuYyB8IDIwICsrKysr KysrKysrKysrKysrKystCj4+ICAxIGZpbGUgY2hhbmdlZCwgMTkgaW5zZXJ0aW9ucygrKSwgMSBk ZWxldGlvbigtKQo+Pgo+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2RybV9jcnRjLmMg Yi9kcml2ZXJzL2dwdS9kcm0vZHJtX2NydGMuYwo+PiBpbmRleCBlMDhmOTYyMjg4ZDkuLmI3ZDBi OTU5ZjA4OCAxMDA2NDQKPj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2RybV9jcnRjLmMKPj4gKysr IGIvZHJpdmVycy9ncHUvZHJtL2RybV9jcnRjLmMKPj4gQEAgLTM0MzQsNiArMzQzNCwxOCBAQCBp bnQgZHJtX21vZGVfYWRkZmIyKHN0cnVjdCBkcm1fZGV2aWNlICpkZXYsCj4+ICAgcmV0dXJuIDA7 Cj4+ICB9Cj4+Cj4+ICtzdHJ1Y3QgZHJtX21vZGVfcm1mYl93b3JrIHsKPj4gKyBzdHJ1Y3Qgd29y a19zdHJ1Y3Qgd29yazsKPj4gKyBzdHJ1Y3QgZHJtX2ZyYW1lYnVmZmVyICpmYjsKPj4gK307Cj4+ ICsKPj4gK3N0YXRpYyB2b2lkIGRybV9tb2RlX3JtZmJfd29ya19mbihzdHJ1Y3Qgd29ya19zdHJ1 Y3QgKncpCj4+ICt7Cj4+ICsgc3RydWN0IGRybV9tb2RlX3JtZmJfd29yayAqYXJnID0gY29udGFp bmVyX29mKHcsIHR5cGVvZigqYXJnKSwgd29yayk7Cj4+ICsKPj4gKyBkcm1fZnJhbWVidWZmZXJf cmVtb3ZlKGFyZy0+ZmIpOwo+IGRybV9mcmFtZWJ1ZmZlcl9yZW1vdmUgc3RpbGwgaGFzIHRoZSBw cm9ibGVtIG9mIG5vdCB3b3JraW5nIGNvcnJlY3RseSB3aXRoCj4gYXRvbWljIHNpbmNlIGF0b21p YyBjb21taXQgd2lsbCBjb21wbGFpbiBpZiB3ZSB0cnkgdG8gZG8gbW9yZSB0aGFuIDEKPiBjb21t aXQgcGVyIHd3X2FjcXVpcmVfY3R4LiBJIHRoaW5rIHdlIHN0aWxsIG5lZWQgYW4gYXRvbWljIHZl cnNpb24gb2YKPiB0aGlzLiBBbHNvIHByb2JhYmx5IGEgbW9yZSBuYXN0eSBpZ3QgdGVzdGNhc2Ug d2hpY2ggdXNlcyB0aGUgc2FtZSBmYiBvbgo+IG1vcmUgdGhhbiBvbmUgcGxhbmUgdG8gYmUgYWJs ZSB0byBoaXQgdGhpcyBjYXNlLgpUaGF0J3MgdHJ1ZSwgYnV0IGEgc2VwYXJhdGUgYnVnLiA6KQo+ PiArfQo+PiArCj4+ICAvKioKPj4gICAqIGRybV9tb2RlX3JtZmIgLSByZW1vdmUgYW4gRkIgZnJv bSB0aGUgY29uZmlndXJhdGlvbgo+PiAgICogQGRldjogZHJtIGRldmljZSBmb3IgdGhlIGlvY3Rs Cj4+IEBAIC0zNDU0LDYgKzM0NjYsNyBAQCBpbnQgZHJtX21vZGVfcm1mYihzdHJ1Y3QgZHJtX2Rl dmljZSAqZGV2LAo+PiAgIHN0cnVjdCBkcm1fZnJhbWVidWZmZXIgKmZibCA9IE5VTEw7Cj4+ICAg dWludDMyX3QgKmlkID0gZGF0YTsKPj4gICBpbnQgZm91bmQgPSAwOwo+PiArIHN0cnVjdCBkcm1f bW9kZV9ybWZiX3dvcmsgYXJnOwo+Pgo+PiAgIGlmICghZHJtX2NvcmVfY2hlY2tfZmVhdHVyZShk ZXYsIERSSVZFUl9NT0RFU0VUKSkKPj4gICByZXR1cm4gLUVJTlZBTDsKPj4gQEAgLTM0NzQsNyAr MzQ4NywxMiBAQCBpbnQgZHJtX21vZGVfcm1mYihzdHJ1Y3QgZHJtX2RldmljZSAqZGV2LAo+PiAg IG11dGV4X3VubG9jaygmZGV2LT5tb2RlX2NvbmZpZy5mYl9sb2NrKTsKPj4gICBtdXRleF91bmxv Y2soJmZpbGVfcHJpdi0+ZmJzX2xvY2spOwo+Pgo+PiAtIGRybV9mcmFtZWJ1ZmZlcl91bnJlZmVy ZW5jZShmYik7Cj4gTmVlZHMgYSBjb21tZW50IGhlcmUgdG8gZXhwbGFpbiB0aGF0IHdlIGV2YWRl IEVJTlRSL3NpZ25hbHMsIGFuZCB0aGF0IGl0J3MKPiBub3QgYSB0cmljayB0byBoaWRlIGEgbG9j a2luZyBpbnZlcnNpb24gZnJvbSBsb2NrZGVwLgo+Cj4gT3RoZXJ3aXNlIEkgdGhpbmsgdGhpcyBw YXRjaCBoZXJlIGlzIHRoZSBiZXN0IGZpeCBvZiBhbGwgdGhlIGFwcHJvYWNoZXMKPiBkaXNjdXNz ZWQgb24gaXJjLCB1bmRlciB0aGUgY29uc3RyYWludCB0aGF0IHdlIG5lZWQgc29tZSBvYnZpb3Vz bHkKPiBzYXZlJnNtYWxsIGZvciBjYzogc3RhYmxlLgo+CkluZGVlZCwgd2lsbCBhZGQgYSBjb21t ZW50LgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpJbnRl bC1nZnggbWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6 Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com ([192.55.52.120]:54079 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753349AbcCVJcg (ORCPT ); Tue, 22 Mar 2016 05:32:36 -0400 Subject: Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb. To: Daniel Vetter References: <1458569477-13364-1-git-send-email-maarten.lankhorst@linux.intel.com> Cc: dri-devel , intel-gfx , Thomas Hellstrom , stable From: Maarten Lankhorst Message-ID: <56F11130.5020506@linux.intel.com> Date: Tue, 22 Mar 2016 10:32:32 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: Op 21-03-16 om 18:37 schreef Daniel Vetter: > On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote: >> It turns out that preserving framebuffers after the rmfb call breaks >> vmwgfx userspace. This was originally introduced because it was thought >> nobody relied on the behavior, but unfortunately it seems there are >> exceptions. >> >> drm_framebuffer_remove may fail with -EINTR now, so a straight revert >> is impossible. There is no way to remove the framebuffer from the lists >> and active planes without introducing a race because of the different >> locking requirements. Instead call drm_framebuffer_remove from a >> workqueue, which is unaffected by signals. >> >> Cc: stable@vger.kernel.org #v4.4+ >> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.") >> Testcase: kms_flip.flip-vs-rmfb-interruptible >> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html >> Cc: Thomas Hellstrom >> Cc: David Herrmann >> --- >> drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index e08f962288d9..b7d0b959f088 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev, >> return 0; >> } >> >> +struct drm_mode_rmfb_work { >> + struct work_struct work; >> + struct drm_framebuffer *fb; >> +}; >> + >> +static void drm_mode_rmfb_work_fn(struct work_struct *w) >> +{ >> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work); >> + >> + drm_framebuffer_remove(arg->fb); > drm_framebuffer_remove still has the problem of not working correctly with > atomic since atomic commit will complain if we try to do more than 1 > commit per ww_acquire_ctx. I think we still need an atomic version of > this. Also probably a more nasty igt testcase which uses the same fb on > more than one plane to be able to hit this case. That's true, but a separate bug. :) >> +} >> + >> /** >> * drm_mode_rmfb - remove an FB from the configuration >> * @dev: drm device for the ioctl >> @@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev, >> struct drm_framebuffer *fbl = NULL; >> uint32_t *id = data; >> int found = 0; >> + struct drm_mode_rmfb_work arg; >> >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) >> return -EINVAL; >> @@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev, >> mutex_unlock(&dev->mode_config.fb_lock); >> mutex_unlock(&file_priv->fbs_lock); >> >> - drm_framebuffer_unreference(fb); > Needs a comment here to explain that we evade EINTR/signals, and that it's > not a trick to hide a locking inversion from lockdep. > > Otherwise I think this patch here is the best fix of all the approaches > discussed on irc, under the constraint that we need some obviously > save&small for cc: stable. > Indeed, will add a comment.