From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sam Ravnborg Date: Thu, 13 Jun 2019 05:04:03 +0000 Subject: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno] Message-Id: <20190613050403.GA21502@ravnborg.org> List-Id: References: <20190613021054.cdewdb3azy6zuoyw@smtp.gmail.com> In-Reply-To: <20190613021054.cdewdb3azy6zuoyw@smtp.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: Rodrigo Siqueira Cc: Maxime Ripard , kernel-janitors@vger.kernel.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, David Airlie Hi Rodrigo. On Wed, Jun 12, 2019 at 11:10:54PM -0300, Rodrigo Siqueira wrote: > For historical reason, the function drm_wait_vblank_ioctl always return > -EINVAL if something gets wrong. This scenario limits the flexibility > for the userspace make detailed verification of the problem and take > some action. In particular, the validation of “if (!dev->irq_enabled)” > in the drm_wait_vblank_ioctl is responsible for checking if the driver > support vblank or not. If the driver does not support VBlank, the > function drm_wait_vblank_ioctl returns EINVAL which does not represent > the real issue; this patch changes this behavior by return EOPNOTSUPP. > Additionally, some operations are unsupported by this function, and > returns EINVAL; this patch also changes the return value to EOPNOTSUPP > in this case. Lastly, the function drm_wait_vblank_ioctl is invoked by > libdrm, which is used by many compositors; because of this, it is > important to check if this change breaks any compositor. In this sense, > the following projects were examined: > > * Drm-hwcomposer > * Kwin > * Sway > * Wlroots > * Wayland-core > * Weston > * Xorg (67 different drivers) > > For each repository the verification happened in three steps: > > * Update the main branch > * Look for any occurrence "drmWaitVBlank" with the command: > git grep -n "drmWaitVBlank" > * Look in the git history of the project with the command: > git log -SdrmWaitVBlank > > Finally, none of the above projects validate the use of EINVAL which > make safe, at least for these projects, to change the return values. > > Change since V2: > Daniel Vetter and Chris Wilson > - Replace ENOTTY by EOPNOTSUPP > - Return EINVAL if the parameters are wrong > > Signed-off-by: Rodrigo Siqueira > --- > Update: > Now IGT has a way to validate if a driver has vblank support or not. > See: https://gitlab.freedesktop.org/drm/igt-gpu-tools/commit/2d244aed69165753f3adbbd6468db073dc1acf9A > > drivers/gpu/drm/drm_vblank.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 0d704bddb1a6..d76a783a7d4b 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -1578,10 +1578,10 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, > unsigned int flags, pipe, high_pipe; > > if (!dev->irq_enabled) > - return -EINVAL; > + return -EOPNOTSUPP; > > if (vblwait->request.type & _DRM_VBLANK_SIGNAL) > - return -EINVAL; > + return -EOPNOTSUPP; > > if (vblwait->request.type & > ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK | When touching this function, could I ask you to take a look at eliminating the use of DRM_WAIT_ON()? It comes from the deprecated drm_os_linux.h header, and it is only of the few remaining users of DRM_WAIT_ON(). Below you can find my untested first try - where I did an attempt not to change behaviour. Sam commit 17b119b02467356198b57bca9949b146082bcaa1 Author: Sam Ravnborg Date: Thu May 30 09:38:47 2019 +0200 drm/vblank: drop use of DRM_WAIT_ON() DRM_WAIT_ON() is from the deprecated drm_os_linux header and the modern replacement is the wait_event_*. The return values differ, so a conversion is needed to keep the original interface towards userspace. Introduced a switch/case to make code obvious and to allow different debug prints depending on the result. The timeout value of 3 * HZ was translated to 30 msec Signed-off-by: Sam Ravnborg Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Sean Paul Cc: David Airlie Cc: Daniel Vetter diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 0d704bddb1a6..51fc6b106333 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -31,7 +31,6 @@ #include #include #include -#include #include #include "drm_internal.h" @@ -1668,18 +1667,27 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, if (req_seq != seq) { DRM_DEBUG("waiting on vblank count %llu, crtc %u\n", req_seq, pipe); - DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, - vblank_passed(drm_vblank_count(dev, pipe), - req_seq) || - !READ_ONCE(vblank->enabled)); + ret = wait_event_interruptible_timeout(vblank->queue, + vblank_passed(drm_vblank_count(dev, pipe), req_seq) || + !READ_ONCE(vblank->enabled), + msecs_to_jiffies(30)); } - if (ret != -EINTR) { + switch (ret) { + case 1: + ret = 0; drm_wait_vblank_reply(dev, pipe, &vblwait->reply); - DRM_DEBUG("crtc %d returning %u to client\n", pipe, vblwait->reply.sequence); - } else { + break; + case 0: + ret = -EBUSY; + drm_wait_vblank_reply(dev, pipe, &vblwait->reply); + DRM_DEBUG("timeout waiting for vblank. crtc %d returning %u to client\n", + pipe, vblwait->reply.sequence); + break; + default: + ret = -EINTR; DRM_DEBUG("crtc %d vblank wait interrupted by signal\n", pipe); } From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sam Ravnborg Subject: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno] Date: Thu, 13 Jun 2019 07:04:03 +0200 Message-ID: <20190613050403.GA21502@ravnborg.org> References: <20190613021054.cdewdb3azy6zuoyw@smtp.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20190613021054.cdewdb3azy6zuoyw@smtp.gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Rodrigo Siqueira Cc: Maxime Ripard , kernel-janitors@vger.kernel.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, David Airlie List-Id: dri-devel@lists.freedesktop.org SGkgUm9kcmlnby4KCk9uIFdlZCwgSnVuIDEyLCAyMDE5IGF0IDExOjEwOjU0UE0gLTAzMDAsIFJv ZHJpZ28gU2lxdWVpcmEgd3JvdGU6Cj4gRm9yIGhpc3RvcmljYWwgcmVhc29uLCB0aGUgZnVuY3Rp b24gZHJtX3dhaXRfdmJsYW5rX2lvY3RsIGFsd2F5cyByZXR1cm4KPiAtRUlOVkFMIGlmIHNvbWV0 aGluZyBnZXRzIHdyb25nLiBUaGlzIHNjZW5hcmlvIGxpbWl0cyB0aGUgZmxleGliaWxpdHkKPiBm b3IgdGhlIHVzZXJzcGFjZSBtYWtlIGRldGFpbGVkIHZlcmlmaWNhdGlvbiBvZiB0aGUgcHJvYmxl bSBhbmQgdGFrZQo+IHNvbWUgYWN0aW9uLiBJbiBwYXJ0aWN1bGFyLCB0aGUgdmFsaWRhdGlvbiBv ZiDigJxpZiAoIWRldi0+aXJxX2VuYWJsZWQp4oCdCj4gaW4gdGhlIGRybV93YWl0X3ZibGFua19p b2N0bCBpcyByZXNwb25zaWJsZSBmb3IgY2hlY2tpbmcgaWYgdGhlIGRyaXZlcgo+IHN1cHBvcnQg dmJsYW5rIG9yIG5vdC4gSWYgdGhlIGRyaXZlciBkb2VzIG5vdCBzdXBwb3J0IFZCbGFuaywgdGhl Cj4gZnVuY3Rpb24gZHJtX3dhaXRfdmJsYW5rX2lvY3RsIHJldHVybnMgRUlOVkFMIHdoaWNoIGRv ZXMgbm90IHJlcHJlc2VudAo+IHRoZSByZWFsIGlzc3VlOyB0aGlzIHBhdGNoIGNoYW5nZXMgdGhp cyBiZWhhdmlvciBieSByZXR1cm4gRU9QTk9UU1VQUC4KPiBBZGRpdGlvbmFsbHksIHNvbWUgb3Bl cmF0aW9ucyBhcmUgdW5zdXBwb3J0ZWQgYnkgdGhpcyBmdW5jdGlvbiwgYW5kCj4gcmV0dXJucyBF SU5WQUw7IHRoaXMgcGF0Y2ggYWxzbyBjaGFuZ2VzIHRoZSByZXR1cm4gdmFsdWUgdG8gRU9QTk9U U1VQUAo+IGluIHRoaXMgY2FzZS4gTGFzdGx5LCB0aGUgZnVuY3Rpb24gZHJtX3dhaXRfdmJsYW5r X2lvY3RsIGlzIGludm9rZWQgYnkKPiBsaWJkcm0sIHdoaWNoIGlzIHVzZWQgYnkgbWFueSBjb21w b3NpdG9yczsgYmVjYXVzZSBvZiB0aGlzLCBpdCBpcwo+IGltcG9ydGFudCB0byBjaGVjayBpZiB0 aGlzIGNoYW5nZSBicmVha3MgYW55IGNvbXBvc2l0b3IuIEluIHRoaXMgc2Vuc2UsCj4gdGhlIGZv bGxvd2luZyBwcm9qZWN0cyB3ZXJlIGV4YW1pbmVkOgo+IAo+ICogRHJtLWh3Y29tcG9zZXIKPiAq IEt3aW4KPiAqIFN3YXkKPiAqIFdscm9vdHMKPiAqIFdheWxhbmQtY29yZQo+ICogV2VzdG9uCj4g KiBYb3JnICg2NyBkaWZmZXJlbnQgZHJpdmVycykKPiAKPiBGb3IgZWFjaCByZXBvc2l0b3J5IHRo ZSB2ZXJpZmljYXRpb24gaGFwcGVuZWQgaW4gdGhyZWUgc3RlcHM6Cj4gCj4gKiBVcGRhdGUgdGhl IG1haW4gYnJhbmNoCj4gKiBMb29rIGZvciBhbnkgb2NjdXJyZW5jZSAiZHJtV2FpdFZCbGFuayIg d2l0aCB0aGUgY29tbWFuZDoKPiAgIGdpdCBncmVwIC1uICJkcm1XYWl0VkJsYW5rIgo+ICogTG9v ayBpbiB0aGUgZ2l0IGhpc3Rvcnkgb2YgdGhlIHByb2plY3Qgd2l0aCB0aGUgY29tbWFuZDoKPiAg IGdpdCBsb2cgLVNkcm1XYWl0VkJsYW5rCj4gCj4gRmluYWxseSwgbm9uZSBvZiB0aGUgYWJvdmUg cHJvamVjdHMgdmFsaWRhdGUgdGhlIHVzZSBvZiBFSU5WQUwgd2hpY2gKPiBtYWtlIHNhZmUsIGF0 IGxlYXN0IGZvciB0aGVzZSBwcm9qZWN0cywgdG8gY2hhbmdlIHRoZSByZXR1cm4gdmFsdWVzLgo+ IAo+IENoYW5nZSBzaW5jZSBWMjoKPiAgRGFuaWVsIFZldHRlciBhbmQgQ2hyaXMgV2lsc29uCj4g IC0gUmVwbGFjZSBFTk9UVFkgYnkgRU9QTk9UU1VQUAo+ICAtIFJldHVybiBFSU5WQUwgaWYgdGhl IHBhcmFtZXRlcnMgYXJlIHdyb25nCj4gCj4gU2lnbmVkLW9mZi1ieTogUm9kcmlnbyBTaXF1ZWly YSA8cm9kcmlnb3NpcXVlaXJhbWVsb0BnbWFpbC5jb20+Cj4gLS0tCj4gVXBkYXRlOgo+ICAgTm93 IElHVCBoYXMgYSB3YXkgdG8gdmFsaWRhdGUgaWYgYSBkcml2ZXIgaGFzIHZibGFuayBzdXBwb3J0 IG9yIG5vdC4KPiAgIFNlZTogaHR0cHM6Ly9naXRsYWIuZnJlZWRlc2t0b3Aub3JnL2RybS9pZ3Qt Z3B1LXRvb2xzL2NvbW1pdC8yZDI0NGFlZDY5MTY1NzUzZjNhZGJiZDY0NjhkYjA3M2RjMWFjZjlB Cj4gCj4gIGRyaXZlcnMvZ3B1L2RybS9kcm1fdmJsYW5rLmMgfCA0ICsrLS0KPiAgMSBmaWxlIGNo YW5nZWQsIDIgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkKPiAKPiBkaWZmIC0tZ2l0IGEv ZHJpdmVycy9ncHUvZHJtL2RybV92YmxhbmsuYyBiL2RyaXZlcnMvZ3B1L2RybS9kcm1fdmJsYW5r LmMKPiBpbmRleCAwZDcwNGJkZGIxYTYuLmQ3NmE3ODNhN2Q0YiAxMDA2NDQKPiAtLS0gYS9kcml2 ZXJzL2dwdS9kcm0vZHJtX3ZibGFuay5jCj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2RybV92Ymxh bmsuYwo+IEBAIC0xNTc4LDEwICsxNTc4LDEwIEBAIGludCBkcm1fd2FpdF92YmxhbmtfaW9jdGwo c3RydWN0IGRybV9kZXZpY2UgKmRldiwgdm9pZCAqZGF0YSwKPiAgCXVuc2lnbmVkIGludCBmbGFn cywgcGlwZSwgaGlnaF9waXBlOwo+ICAKPiAgCWlmICghZGV2LT5pcnFfZW5hYmxlZCkKPiAtCQly ZXR1cm4gLUVJTlZBTDsKPiArCQlyZXR1cm4gLUVPUE5PVFNVUFA7Cj4gIAo+ICAJaWYgKHZibHdh aXQtPnJlcXVlc3QudHlwZSAmIF9EUk1fVkJMQU5LX1NJR05BTCkKPiAtCQlyZXR1cm4gLUVJTlZB TDsKPiArCQlyZXR1cm4gLUVPUE5PVFNVUFA7Cj4gIAo+ICAJaWYgKHZibHdhaXQtPnJlcXVlc3Qu dHlwZSAmCj4gIAkgICAgfihfRFJNX1ZCTEFOS19UWVBFU19NQVNLIHwgX0RSTV9WQkxBTktfRkxB R1NfTUFTSyB8CgpXaGVuIHRvdWNoaW5nIHRoaXMgZnVuY3Rpb24sIGNvdWxkIEkgYXNrIHlvdSB0 byB0YWtlIGEgbG9vayBhdAplbGltaW5hdGluZyB0aGUgdXNlIG9mIERSTV9XQUlUX09OKCk/Ckl0 IGNvbWVzIGZyb20gdGhlIGRlcHJlY2F0ZWQgZHJtX29zX2xpbnV4LmggaGVhZGVyLCBhbmQgaXQg aXMgb25seSBvZgp0aGUgZmV3IHJlbWFpbmluZyB1c2VycyBvZiBEUk1fV0FJVF9PTigpLgoKQmVs b3cgeW91IGNhbiBmaW5kIG15IHVudGVzdGVkIGZpcnN0IHRyeSAtIHdoZXJlIEkgZGlkIGFuIGF0 dGVtcHQgbm90IHRvCmNoYW5nZSBiZWhhdmlvdXIuCgoJU2FtCgpjb21taXQgMTdiMTE5YjAyNDY3 MzU2MTk4YjU3YmNhOTk0OWIxNDYwODJiY2FhMQpBdXRob3I6IFNhbSBSYXZuYm9yZyA8c2FtQHJh dm5ib3JnLm9yZz4KRGF0ZTogICBUaHUgTWF5IDMwIDA5OjM4OjQ3IDIwMTkgKzAyMDAKCiAgICBk cm0vdmJsYW5rOiBkcm9wIHVzZSBvZiBEUk1fV0FJVF9PTigpCiAgICAKICAgIERSTV9XQUlUX09O KCkgaXMgZnJvbSB0aGUgZGVwcmVjYXRlZCBkcm1fb3NfbGludXggaGVhZGVyIGFuZAogICAgdGhl IG1vZGVybiByZXBsYWNlbWVudCBpcyB0aGUgd2FpdF9ldmVudF8qLgogICAgCiAgICBUaGUgcmV0 dXJuIHZhbHVlcyBkaWZmZXIsIHNvIGEgY29udmVyc2lvbiBpcyBuZWVkZWQgdG8KICAgIGtlZXAg dGhlIG9yaWdpbmFsIGludGVyZmFjZSB0b3dhcmRzIHVzZXJzcGFjZS4KICAgIEludHJvZHVjZWQg YSBzd2l0Y2gvY2FzZSB0byBtYWtlIGNvZGUgb2J2aW91cyBhbmQgdG8gYWxsb3cKICAgIGRpZmZl cmVudCBkZWJ1ZyBwcmludHMgZGVwZW5kaW5nIG9uIHRoZSByZXN1bHQuCiAgICAKICAgIFRoZSB0 aW1lb3V0IHZhbHVlIG9mIDMgKiBIWiB3YXMgdHJhbnNsYXRlZCB0byAzMCBtc2VjCiAgICAKICAg IFNpZ25lZC1vZmYtYnk6IFNhbSBSYXZuYm9yZyA8c2FtQHJhdm5ib3JnLm9yZz4KICAgIENjOiBN YWFydGVuIExhbmtob3JzdCA8bWFhcnRlbi5sYW5raG9yc3RAbGludXguaW50ZWwuY29tPgogICAg Q2M6IE1heGltZSBSaXBhcmQgPG1heGltZS5yaXBhcmRAYm9vdGxpbi5jb20+CiAgICBDYzogU2Vh biBQYXVsIDxzZWFuQHBvb3JseS5ydW4+CiAgICBDYzogRGF2aWQgQWlybGllIDxhaXJsaWVkQGxp bnV4LmllPgogICAgQ2M6IERhbmllbCBWZXR0ZXIgPGRhbmllbEBmZndsbC5jaD4KCmRpZmYgLS1n aXQgYS9kcml2ZXJzL2dwdS9kcm0vZHJtX3ZibGFuay5jIGIvZHJpdmVycy9ncHUvZHJtL2RybV92 YmxhbmsuYwppbmRleCAwZDcwNGJkZGIxYTYuLjUxZmM2YjEwNjMzMyAxMDA2NDQKLS0tIGEvZHJp dmVycy9ncHUvZHJtL2RybV92YmxhbmsuYworKysgYi9kcml2ZXJzL2dwdS9kcm0vZHJtX3ZibGFu ay5jCkBAIC0zMSw3ICszMSw2IEBACiAjaW5jbHVkZSA8ZHJtL2RybV9kcnYuaD4KICNpbmNsdWRl IDxkcm0vZHJtX2ZyYW1lYnVmZmVyLmg+CiAjaW5jbHVkZSA8ZHJtL2RybV9wcmludC5oPgotI2lu Y2x1ZGUgPGRybS9kcm1fb3NfbGludXguaD4KICNpbmNsdWRlIDxkcm0vZHJtX3ZibGFuay5oPgog CiAjaW5jbHVkZSAiZHJtX2ludGVybmFsLmgiCkBAIC0xNjY4LDE4ICsxNjY3LDI3IEBAIGludCBk cm1fd2FpdF92YmxhbmtfaW9jdGwoc3RydWN0IGRybV9kZXZpY2UgKmRldiwgdm9pZCAqZGF0YSwK IAlpZiAocmVxX3NlcSAhPSBzZXEpIHsKIAkJRFJNX0RFQlVHKCJ3YWl0aW5nIG9uIHZibGFuayBj b3VudCAlbGx1LCBjcnRjICV1XG4iLAogCQkJICByZXFfc2VxLCBwaXBlKTsKLQkJRFJNX1dBSVRf T04ocmV0LCB2YmxhbmstPnF1ZXVlLCAzICogSFosCi0JCQkgICAgdmJsYW5rX3Bhc3NlZChkcm1f dmJsYW5rX2NvdW50KGRldiwgcGlwZSksCi0JCQkJCSAgcmVxX3NlcSkgfHwKLQkJCSAgICAhUkVB RF9PTkNFKHZibGFuay0+ZW5hYmxlZCkpOworCQlyZXQgPSB3YWl0X2V2ZW50X2ludGVycnVwdGli bGVfdGltZW91dCh2YmxhbmstPnF1ZXVlLAorCQkJdmJsYW5rX3Bhc3NlZChkcm1fdmJsYW5rX2Nv dW50KGRldiwgcGlwZSksIHJlcV9zZXEpIHx8CisJCQkJICAgICAgIVJFQURfT05DRSh2Ymxhbmst PmVuYWJsZWQpLAorCQkJbXNlY3NfdG9famlmZmllcygzMCkpOwogCX0KIAotCWlmIChyZXQgIT0g LUVJTlRSKSB7CisJc3dpdGNoIChyZXQpIHsKKwljYXNlIDE6CisJCXJldCA9IDA7CiAJCWRybV93 YWl0X3ZibGFua19yZXBseShkZXYsIHBpcGUsICZ2Ymx3YWl0LT5yZXBseSk7Ci0KIAkJRFJNX0RF QlVHKCJjcnRjICVkIHJldHVybmluZyAldSB0byBjbGllbnRcbiIsCiAJCQkgIHBpcGUsIHZibHdh aXQtPnJlcGx5LnNlcXVlbmNlKTsKLQl9IGVsc2UgeworCQlicmVhazsKKwljYXNlIDA6CisJCXJl dCA9IC1FQlVTWTsKKwkJZHJtX3dhaXRfdmJsYW5rX3JlcGx5KGRldiwgcGlwZSwgJnZibHdhaXQt PnJlcGx5KTsKKwkJRFJNX0RFQlVHKCJ0aW1lb3V0IHdhaXRpbmcgZm9yIHZibGFuay4gY3J0YyAl ZCByZXR1cm5pbmcgJXUgdG8gY2xpZW50XG4iLAorCQkJICBwaXBlLCB2Ymx3YWl0LT5yZXBseS5z ZXF1ZW5jZSk7CisJCWJyZWFrOworCWRlZmF1bHQ6CisJCXJldCA9IC1FSU5UUjsKIAkJRFJNX0RF QlVHKCJjcnRjICVkIHZibGFuayB3YWl0IGludGVycnVwdGVkIGJ5IHNpZ25hbFxuIiwgcGlwZSk7 CiAJfQogCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCklu dGVsLWdmeCBtYWlsaW5nIGxpc3QKSW50ZWwtZ2Z4QGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRw czovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2ludGVsLWdmeA== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AD685C31E4A for ; Thu, 13 Jun 2019 16:47:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8EC5A20673 for ; Thu, 13 Jun 2019 16:47:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2392882AbfFMQrJ (ORCPT ); Thu, 13 Jun 2019 12:47:09 -0400 Received: from asavdk3.altibox.net ([109.247.116.14]:53141 "EHLO asavdk3.altibox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730266AbfFMFEL (ORCPT ); Thu, 13 Jun 2019 01:04:11 -0400 Received: from ravnborg.org (unknown [158.248.194.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by asavdk3.altibox.net (Postfix) with ESMTPS id 81A992001E; Thu, 13 Jun 2019 07:04:05 +0200 (CEST) Date: Thu, 13 Jun 2019 07:04:03 +0200 From: Sam Ravnborg To: Rodrigo Siqueira Cc: Maarten Lankhorst , Maxime Ripard , Sean Paul , David Airlie , Daniel Vetter , intel-gfx@lists.freedesktop.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno] Message-ID: <20190613050403.GA21502@ravnborg.org> References: <20190613021054.cdewdb3azy6zuoyw@smtp.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190613021054.cdewdb3azy6zuoyw@smtp.gmail.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.3 cv=dqr19Wo4 c=1 sm=1 tr=0 a=UWs3HLbX/2nnQ3s7vZ42gw==:117 a=UWs3HLbX/2nnQ3s7vZ42gw==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=IkcTkHD0fZMA:10 a=pGLkceISAAAA:8 a=e5mUnYsNAAAA:8 a=7gkXJVJtAAAA:8 a=QyXUC8HyAAAA:8 a=P-IC7800AAAA:8 a=Jn-_o28iN5fng-a6wa4A:9 a=QEXdDO2ut3YA:10 a=Vxmtnl_E_bksehYqCbjh:22 a=E9Po1WZjFZOl8hwRPBS3:22 a=d3PnA9EDa4IxuAV0gXij:22 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Rodrigo. On Wed, Jun 12, 2019 at 11:10:54PM -0300, Rodrigo Siqueira wrote: > For historical reason, the function drm_wait_vblank_ioctl always return > -EINVAL if something gets wrong. This scenario limits the flexibility > for the userspace make detailed verification of the problem and take > some action. In particular, the validation of “if (!dev->irq_enabled)” > in the drm_wait_vblank_ioctl is responsible for checking if the driver > support vblank or not. If the driver does not support VBlank, the > function drm_wait_vblank_ioctl returns EINVAL which does not represent > the real issue; this patch changes this behavior by return EOPNOTSUPP. > Additionally, some operations are unsupported by this function, and > returns EINVAL; this patch also changes the return value to EOPNOTSUPP > in this case. Lastly, the function drm_wait_vblank_ioctl is invoked by > libdrm, which is used by many compositors; because of this, it is > important to check if this change breaks any compositor. In this sense, > the following projects were examined: > > * Drm-hwcomposer > * Kwin > * Sway > * Wlroots > * Wayland-core > * Weston > * Xorg (67 different drivers) > > For each repository the verification happened in three steps: > > * Update the main branch > * Look for any occurrence "drmWaitVBlank" with the command: > git grep -n "drmWaitVBlank" > * Look in the git history of the project with the command: > git log -SdrmWaitVBlank > > Finally, none of the above projects validate the use of EINVAL which > make safe, at least for these projects, to change the return values. > > Change since V2: > Daniel Vetter and Chris Wilson > - Replace ENOTTY by EOPNOTSUPP > - Return EINVAL if the parameters are wrong > > Signed-off-by: Rodrigo Siqueira > --- > Update: > Now IGT has a way to validate if a driver has vblank support or not. > See: https://gitlab.freedesktop.org/drm/igt-gpu-tools/commit/2d244aed69165753f3adbbd6468db073dc1acf9A > > drivers/gpu/drm/drm_vblank.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 0d704bddb1a6..d76a783a7d4b 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -1578,10 +1578,10 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, > unsigned int flags, pipe, high_pipe; > > if (!dev->irq_enabled) > - return -EINVAL; > + return -EOPNOTSUPP; > > if (vblwait->request.type & _DRM_VBLANK_SIGNAL) > - return -EINVAL; > + return -EOPNOTSUPP; > > if (vblwait->request.type & > ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK | When touching this function, could I ask you to take a look at eliminating the use of DRM_WAIT_ON()? It comes from the deprecated drm_os_linux.h header, and it is only of the few remaining users of DRM_WAIT_ON(). Below you can find my untested first try - where I did an attempt not to change behaviour. Sam commit 17b119b02467356198b57bca9949b146082bcaa1 Author: Sam Ravnborg Date: Thu May 30 09:38:47 2019 +0200 drm/vblank: drop use of DRM_WAIT_ON() DRM_WAIT_ON() is from the deprecated drm_os_linux header and the modern replacement is the wait_event_*. The return values differ, so a conversion is needed to keep the original interface towards userspace. Introduced a switch/case to make code obvious and to allow different debug prints depending on the result. The timeout value of 3 * HZ was translated to 30 msec Signed-off-by: Sam Ravnborg Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Sean Paul Cc: David Airlie Cc: Daniel Vetter diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 0d704bddb1a6..51fc6b106333 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -31,7 +31,6 @@ #include #include #include -#include #include #include "drm_internal.h" @@ -1668,18 +1667,27 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, if (req_seq != seq) { DRM_DEBUG("waiting on vblank count %llu, crtc %u\n", req_seq, pipe); - DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, - vblank_passed(drm_vblank_count(dev, pipe), - req_seq) || - !READ_ONCE(vblank->enabled)); + ret = wait_event_interruptible_timeout(vblank->queue, + vblank_passed(drm_vblank_count(dev, pipe), req_seq) || + !READ_ONCE(vblank->enabled), + msecs_to_jiffies(30)); } - if (ret != -EINTR) { + switch (ret) { + case 1: + ret = 0; drm_wait_vblank_reply(dev, pipe, &vblwait->reply); - DRM_DEBUG("crtc %d returning %u to client\n", pipe, vblwait->reply.sequence); - } else { + break; + case 0: + ret = -EBUSY; + drm_wait_vblank_reply(dev, pipe, &vblwait->reply); + DRM_DEBUG("timeout waiting for vblank. crtc %d returning %u to client\n", + pipe, vblwait->reply.sequence); + break; + default: + ret = -EINTR; DRM_DEBUG("crtc %d vblank wait interrupted by signal\n", pipe); }