From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark yao Subject: Re: [PATCH 3/3] drm/rockchip: explain why we can't wait_for_vblanks Date: Mon, 18 Jan 2016 09:40:32 +0800 Message-ID: <569C4290.1030902@rock-chips.com> References: <20160114142047.GD19130@phenom.ffwll.local> <107bbc36a316ed0ddc7b5a8bcd9b6db6cbc71d4f.1452782114.git.john@metanate.com> <20160114145705.GA24005@ulmo> <20160114162619.233ab39c.john@metanate.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160114162619.233ab39c.john@metanate.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: John Keeping , Thierry Reding Cc: "linux-arm-kernel@lists.infradead.org" , "open list:ARM/Rockchip SoC..." , Linux Kernel Mailing List , dri-devel List-Id: linux-rockchip.vger.kernel.org T24gMjAxNuW5tDAx5pyIMTXml6UgMDA6MjYsIEpvaG4gS2VlcGluZyB3cm90ZToKPiBPbiBUaHUs IDE0IEphbiAyMDE2IDE1OjU3OjA1ICswMTAwLCBUaGllcnJ5IFJlZGluZyB3cm90ZToKPgo+PiBP biBUaHUsIEphbiAxNCwgMjAxNiBhdCAwMjozOTo0MlBNICswMDAwLCBKb2huIEtlZXBpbmcgd3Jv dGU6Cj4+PiBTaWduZWQtb2ZmLWJ5OiBKb2huIEtlZXBpbmcgPGpvaG5AbWV0YW5hdGUuY29tPgo+ Pj4gLS0tCj4+PiAgIGRyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fZmIuYyB8 IDYgKysrKysrCj4+PiAgIDEgZmlsZSBjaGFuZ2VkLCA2IGluc2VydGlvbnMoKykKPj4+Cj4+PiBk aWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV9mYi5jIGIv ZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV9mYi5jCj4+PiBpbmRleCA2Nzlk MjNhLi5iMjY3Y2U0IDEwMDY0NAo+Pj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3Jv Y2tjaGlwX2RybV9mYi5jCj4+PiArKysgYi9kcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAvcm9ja2No aXBfZHJtX2ZiLmMKPj4+IEBAIC0xNzcsNiArMTc3LDEyIEBAIHN0YXRpYyB2b2lkIHJvY2tjaGlw X2NydGNfd2FpdF9mb3JfdXBkYXRlKHN0cnVjdCBkcm1fY3J0YyAqY3J0YykKPj4+ICAgCQljcnRj X2Z1bmNzLT53YWl0X2Zvcl91cGRhdGUoY3J0Yyk7Cj4+PiAgIH0KPj4+ICAgCj4+PiArLyoKPj4+ ICsgKiBXZSBjYW4ndCB1c2UgZHJtX2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpIGJl Y2F1c2UgcmszMjg4IGFuZCByazMwNjYKPj4+ICsgKiBoYXZlIGhhcmR3YXJlIGNvdW50ZXJzIGZv ciBuZWl0aGVyIHZibGFua3Mgbm9yIHNjYW5saW5lcy4gIFRoaXMgZnVuY3Rpb24gaXMKPj4+ICsg KiBlcXVpdmFsZW50IGJ1dCB1c2VzIHJvY2tjaGlwX2NydGNfd2FpdF9mb3JfdXBkYXRlKCkgaW5z dGVhZCBvZiB3YWl0aW5nIGZvcgo+Pj4gKyAqIHZibGFua19jb3VudCB0byBjaGFuZ2UuCj4+PiAr ICovCj4+IFRoaXMgaXMga2luZCBvZiBtaXNsZWFkaW5nLiBGcm9tIHJlYWRpbmcgZWFybGllciBw YXJ0cyBvZiB0aGUgdGhyZWFkIHRoZQo+PiByZWFzb24gd2h5IGRybV9hdG9taWNfaGVscGVyX3dh aXRfZm9yX3ZibGFua3MoKSB3b24ndCB3b3JrIGlzIGJlY2F1c2UgaXQKPj4gaGFzIGEgcG90ZW50 aWFsIHJhY2UgY29uZGl0aW9uIHRoYXQgY2FuJ3QgYmUgZGV0ZWN0ZWQgdW5sZXNzIHlvdSBhbHNv Cj4+IGhhdmUgYSB2YmxhbmsgY291bnRlci4gSG93ZXZlciwgdGhlIGFib3ZlIGNvbW1lbnQgbWFr ZXMgaXQgd29yayBsaWtlCj4+IGRybV9hdG9taWNfaGVscGVyX3dhaXRfZm9yX3ZibGFua3MoKSBk b2Vzbid0IHdvcmsgaW4gdGhlIGFic2VuY2Ugb2YgYQo+PiB2YmxhbmsgY291bnRlciwgd2hpY2gg aXNuJ3QgcXVpdGUgdHJ1ZS4KPiBIb3cgYWJvdXQgc29tZXRoaW5nIGxpa2UgdGhpcyAodXNpbmcg dGhlIHNlcXVlbmNlIGZyb20gTWFyaydzIG1lc3NhZ2UpOgo+Cj4gLyoKPiAgICogV2UgY2FuJ3Qg dXNlIGRybV9hdG9taWNfaGVscGVyX3dhaXRfZm9yX3ZibGFua3MoKSBiZWNhdXNlIHJrMzI4OCBh bmQgcmszMDY2Cj4gICAqIGhhdmUgaGFyZHdhcmUgY291bnRlcnMgZm9yIG5laXRoZXIgdmJsYW5r cyBub3Igc2NhbmxpbmVzLCB3aGljaCByZXN1bHRzIGluCj4gICAqIGEgcmFjZSB3aGVyZToKPiAg ICoJCQkJfCA8LS0gSFcgdnN5bmMgaXJxIGFuZCByZWcgdGFrZSBlZmZlY3QKPiAgICoJICAgICAg IHBsYW5lX2NvbW1pdCAtLT4gfAo+ICAgKglnZXRfdmJsYW5rIGFuZCB3YWl0IC0tPiB8Cj4gICAq CQkJCXwgPC0tIGhhbmRsZV92YmxhbmssIHZibGFuay0+Y291bnQgKyAxCj4gICAqCQkgY2xlYW51 cF9mYiAtLT4gfAo+ICAgKgkJaW9tbXUgY3Jhc2ggLS0+IHwKPiAgICoJCQkJfCA8LS0gSFcgdnN5 bmMgaXJxIGFuZCByZWcgdGFrZSBlZmZlY3QKPiAgICoKPiAgICogVGhpcyBmdW5jdGlvbiBpcyBl cXVpdmFsZW50IGJ1dCB1c2VzIHJvY2tjaGlwX2NydGNfd2FpdF9mb3JfdXBkYXRlKCkgaW5zdGVh ZAo+ICAgKiBvZiB3YWl0aW5nIGZvciB2YmxhbmtfY291bnQgdG8gY2hhbmdlLgo+ICAgKi8KPgo+ Cj4KCkxvb2tzIGdvb2QgZm9yIG1lLCBidXQgbWF5YmUgVGhpZXJyeSBoYXMgc29tZSBtb3JlIGFk dmljZXMuIDotKQoKLS0gCu+8rWFyayBZYW8KCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0 cy5mcmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xp c3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.yao@rock-chips.com (Mark yao) Date: Mon, 18 Jan 2016 09:40:32 +0800 Subject: [PATCH 3/3] drm/rockchip: explain why we can't wait_for_vblanks In-Reply-To: <20160114162619.233ab39c.john@metanate.com> References: <20160114142047.GD19130@phenom.ffwll.local> <107bbc36a316ed0ddc7b5a8bcd9b6db6cbc71d4f.1452782114.git.john@metanate.com> <20160114145705.GA24005@ulmo> <20160114162619.233ab39c.john@metanate.com> Message-ID: <569C4290.1030902@rock-chips.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2016?01?15? 00:26, John Keeping wrote: > On Thu, 14 Jan 2016 15:57:05 +0100, Thierry Reding wrote: > >> On Thu, Jan 14, 2016 at 02:39:42PM +0000, John Keeping wrote: >>> Signed-off-by: John Keeping >>> --- >>> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c >>> index 679d23a..b267ce4 100644 >>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c >>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c >>> @@ -177,6 +177,12 @@ static void rockchip_crtc_wait_for_update(struct drm_crtc *crtc) >>> crtc_funcs->wait_for_update(crtc); >>> } >>> >>> +/* >>> + * We can't use drm_atomic_helper_wait_for_vblanks() because rk3288 and rk3066 >>> + * have hardware counters for neither vblanks nor scanlines. This function is >>> + * equivalent but uses rockchip_crtc_wait_for_update() instead of waiting for >>> + * vblank_count to change. >>> + */ >> This is kind of misleading. From reading earlier parts of the thread the >> reason why drm_atomic_helper_wait_for_vblanks() won't work is because it >> has a potential race condition that can't be detected unless you also >> have a vblank counter. However, the above comment makes it work like >> drm_atomic_helper_wait_for_vblanks() doesn't work in the absence of a >> vblank counter, which isn't quite true. > How about something like this (using the sequence from Mark's message): > > /* > * We can't use drm_atomic_helper_wait_for_vblanks() because rk3288 and rk3066 > * have hardware counters for neither vblanks nor scanlines, which results in > * a race where: > * | <-- HW vsync irq and reg take effect > * plane_commit --> | > * get_vblank and wait --> | > * | <-- handle_vblank, vblank->count + 1 > * cleanup_fb --> | > * iommu crash --> | > * | <-- HW vsync irq and reg take effect > * > * This function is equivalent but uses rockchip_crtc_wait_for_update() instead > * of waiting for vblank_count to change. > */ > > > Looks good for me, but maybe Thierry has some more advices. :-) -- ?ark Yao From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753406AbcARBkk (ORCPT ); Sun, 17 Jan 2016 20:40:40 -0500 Received: from regular1.263xmail.com ([211.150.99.138]:60573 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753159AbcARBkh (ORCPT ); Sun, 17 Jan 2016 20:40:37 -0500 X-263anti-spam: KSV:0;BIG:0;ABS:1;DNS:0;ATT:0;SPF:S; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ADDR-CHECKED: 0 X-RL-SENDER: mark.yao@rock-chips.com X-FST-TO: linux-arm-kernel@lists.infradead.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: mark.yao@rock-chips.com X-UNIQUE-TAG: <7882ddb88f23732f7f891e67b0a6b6ba> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <569C4290.1030902@rock-chips.com> Date: Mon, 18 Jan 2016 09:40:32 +0800 From: Mark yao User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: John Keeping , Thierry Reding CC: Linux Kernel Mailing List , dri-devel , "open list:ARM/Rockchip SoC..." , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH 3/3] drm/rockchip: explain why we can't wait_for_vblanks References: <20160114142047.GD19130@phenom.ffwll.local> <107bbc36a316ed0ddc7b5a8bcd9b6db6cbc71d4f.1452782114.git.john@metanate.com> <20160114145705.GA24005@ulmo> <20160114162619.233ab39c.john@metanate.com> In-Reply-To: <20160114162619.233ab39c.john@metanate.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016年01月15日 00:26, John Keeping wrote: > On Thu, 14 Jan 2016 15:57:05 +0100, Thierry Reding wrote: > >> On Thu, Jan 14, 2016 at 02:39:42PM +0000, John Keeping wrote: >>> Signed-off-by: John Keeping >>> --- >>> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c >>> index 679d23a..b267ce4 100644 >>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c >>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c >>> @@ -177,6 +177,12 @@ static void rockchip_crtc_wait_for_update(struct drm_crtc *crtc) >>> crtc_funcs->wait_for_update(crtc); >>> } >>> >>> +/* >>> + * We can't use drm_atomic_helper_wait_for_vblanks() because rk3288 and rk3066 >>> + * have hardware counters for neither vblanks nor scanlines. This function is >>> + * equivalent but uses rockchip_crtc_wait_for_update() instead of waiting for >>> + * vblank_count to change. >>> + */ >> This is kind of misleading. From reading earlier parts of the thread the >> reason why drm_atomic_helper_wait_for_vblanks() won't work is because it >> has a potential race condition that can't be detected unless you also >> have a vblank counter. However, the above comment makes it work like >> drm_atomic_helper_wait_for_vblanks() doesn't work in the absence of a >> vblank counter, which isn't quite true. > How about something like this (using the sequence from Mark's message): > > /* > * We can't use drm_atomic_helper_wait_for_vblanks() because rk3288 and rk3066 > * have hardware counters for neither vblanks nor scanlines, which results in > * a race where: > * | <-- HW vsync irq and reg take effect > * plane_commit --> | > * get_vblank and wait --> | > * | <-- handle_vblank, vblank->count + 1 > * cleanup_fb --> | > * iommu crash --> | > * | <-- HW vsync irq and reg take effect > * > * This function is equivalent but uses rockchip_crtc_wait_for_update() instead > * of waiting for vblank_count to change. > */ > > > Looks good for me, but maybe Thierry has some more advices. :-) -- Ｍark Yao