From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark yao Subject: Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API Date: Fri, 11 Dec 2015 14:26:31 +0800 Message-ID: <566A6C97.4020207@rock-chips.com> References: <1448940391-23333-1-git-send-email-mark.yao@rock-chips.com> <1448940391-23333-4-git-send-email-mark.yao@rock-chips.com> <565D68F0.3010905@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Stone Cc: Linux Kernel Mailing List , dri-devel , Tomasz Figa , linux-rockchip , "linux-arm-kernel@lists.infradead.org" List-Id: linux-rockchip.vger.kernel.org T24gMjAxNeW5tDEy5pyIMDLml6UgMjI6MTgsIERhbmllbCBTdG9uZSB3cm90ZToKPiBIaSBNYXJr LAo+IFRoYW5rcyBmb3IgZ2V0dGluZyBiYWNrIHRvIHRoaXMuCj4KPiBPbiAxIERlY2VtYmVyIDIw MTUgYXQgMDk6MzEsIE1hcmsgeWFvIDxtYXJrLnlhb0Byb2NrLWNoaXBzLmNvbT4gd3JvdGU6Cj4+ IE9uIDIwMTXlubQxMuaciDAx5pelIDE2OjE4LCBEYW5pZWwgU3RvbmUgd3JvdGU6Cj4+PiBPbiAx IERlY2VtYmVyIDIwMTUgYXQgMDM6MjYsIE1hcmsgWWFvPG1hcmsueWFvQHJvY2stY2hpcHMuY29t PiAgd3JvdGU6Cj4+Pj4+ICsgICAgICAgZm9yX2VhY2hfY3J0Y19pbl9zdGF0ZShzdGF0ZSwgY3J0 YywgY3J0Y19zdGF0ZSwgaSkgewo+Pj4+PiArICAgICAgICAgICAgICAgaWYgKCFjcnRjLT5zdGF0 ZS0+YWN0aXZlKQo+Pj4+PiArICAgICAgICAgICAgICAgICAgICAgICBjb250aW51ZTsKPj4+Pj4g Kwo+Pj4+PiArICAgICAgICAgICAgICAgcm9ja2NoaXBfY3J0Y193YWl0X2Zvcl91cGRhdGUoY3J0 Yyk7Cj4+Pj4+ICsgICAgICAgfQo+Pj4gSSdkIGJlIG11Y2ggbW9yZSBjb21mb3J0YWJsZSBpZiB0 aGlzIHBhc3NlZCBpbiBhbiBleHBsaWNpdCBwb2ludGVyIHRvCj4+PiBzdGF0ZSwgb3IgYW4gYWRk cmVzcyB0byB3YWl0IGZvciwgcmF0aGVyIHRoYW4gaGF2ZSB3YWl0X2Zvcl9jb21wbGV0ZQo+Pj4g ZGlnIG91dCBzdGF0ZSB3aXRoIG5vIGxvY2tpbmcuIFRoZSBsYXR0ZXIgaXMgcG90ZW50aWFsbHkg cmFjeSBmb3IKPj4+IGFzeW5jIG9wZXJhdGlvbnMuCj4+Pgo+PiBIaSBEYW5pZWwKPj4gICAgICJp ZiB0aGlzIHBhc3NlZCBpbiBhbiBleHBsaWNpdCBwb2ludGVyIHRvIHN0YXRlLCBvciBhbiBhZGRy ZXNzIHRvIHdhaXQKPj4gZm9yIiwgSSBkb24ndCB1bmRlcnN0YW5kLCBjYW4geW91IHBvaW50IGhv dyBpdCB3b3JrPwo+IEFoLCBPSy4gSSBtZWFuIHRoYXQgcm9ja2NoaXBfY3J0Y193YWl0X2Zvcl91 cGRhdGUgdGFrZXMgYSBkcm1fY3J0Ywo+IHBvaW50ZXIsIGFuZCBlc3RhYmxpc2hlcyB0aGUgc3Rh dGUgZnJvbSB0aGF0IChlLmcuCj4gY3J0Yy0+cHJpbWFyeS0+c3RhdGUpLiBUaGlzIGNhbiBlYXNp bHkgbGVhZCB0byBjb25mdXNpb24gaW4gYXN5bmMKPiBjb250ZXh0cywgYXMgdGhlIHN0YXRlcyBh dHRhY2hlZCB0byBhIGRybV9jcnRjIGFuZCBhIGRybV9wbGFuZSBjYW4KPiBjaGFuZ2UgaGVyZSB3 aGlsZSB5b3Ugd2FpdCBmb3IgaXQuCj4KPiBJdCB3b3VsZCBiZSBiZXR0ZXIgaWYgdGhlIGNhbGwg d2FzOgo+Cj4gZm9yX2VhY2hfcGxhbmVfaW5fc3RhdGUoc3RhdGUsIHBsYW5lLCBwbGFuZV9zdGF0 ZSwgaSkgewo+ICAgICAgaWYgKHBsYW5lLT50eXBlID09IERSTV9QTEFORV9UWVBFX1BSSU1BUlkp Cj4gICAgICAgICAgcm9ja2NoaXBfY3J0Y193YWl0X2Zvcl91cGRhdGUocGxhbmVfc3RhdGUtPmNy dGMsIHBsYW5lX3N0YXRlKTsKPiB9Cj4KPiBUaGlzIHdheSBpdCBpcyB2ZXJ5IGNsZWFyLCBhbmQg dGhlcmUgaXMgbm8gY29uZnVzaW9uIGFzIHRvIHdoaWNoCj4gcmVxdWVzdCB3ZSBhcmUgd2FpdGlu ZyB0byBjb21wbGV0ZS4KPgo+IEluIGdlbmVyYWwsIHVzaW5nIGNydGMtPnN0YXRlIG9yIHBsYW5l LT5zdGF0ZSBpcyBhIHZlcnkgYmFkIGlkZWEsCj4gX2V4Y2VwdF8gaW4gdGhlIGF0b21pY19jaGVj ayBmdW5jdGlvbiB3aGVyZSB5b3UgYXJlIGNhbGN1bGF0aW5nCj4gY2hhbmdlcyAoZS5nLiBpZiAo cGxhbmVfc3RhdGUtPmZiLT5waXRjaGVzWzBdICE9Cj4gcGxhbmUtPnN0YXRlLT5mYi0+cGl0Y2hl c1swXSkgcm9ja2NoaXBfcGxhbmVfc3RhdGUtPnBpdGNoX2NoYW5nZWQgPQo+IHRydWUpLiBBZnRl ciBhdG9taWNfY2hlY2ssIHlvdSBzaG91bGQgYWx3YXlzIHBhc3MgYXJvdW5kIHBvaW50ZXJzIHRv Cj4gdGhlIHBsYW5lIHN0YXRlIGV4cGxpY2l0bHksIGFuZCBhdm9pZCB1c2luZyB0aGUgcG9pbnRl cnMgZnJvbSBkcm1fY3J0Ywo+IGFuZCBkcm1fcGxhbmUuCj4KPiBEb2VzIHRoYXQgaGVscD8KCkhp IERhbmllbAoKU29ycnksIEkgZG9uJ3QgYWN0dWFsbHkgdW5kZXJzdGFuZCB3aHkgY3J0Yy0+c3Rh dGUgb3IgcGxhbmUtPnN0YXRlIGlzIG5vIApyZWNvbW1lbmRlZApleGNlcHQgYXRvbWljX2NoZWNr LCBvbiBhdG9taWNfdXBkYXRlLCB3ZSBuZWVkIHVzZSBwbGFuZS0+c3RhdGUsIGlzIHRoYXQgCmEg cHJvYmxlbT8KCkkgZ3Vlc3MgdGhhdCwgZHJtX2F0b21pY19oZWxwZXJfc3dhcF9zdGF0ZSB3b3Vs ZCByYWNlIHdpdGggYXN5bmMgb3BlcmF0aW9ucywKc28gdXNlIGNydGMtPnN0YXRlIG9uIGFzeW5j IHN0YWNrIGlzIG5vdCBzYWZlLiBpcyBpdCBjb3JyZWN0PwoKSSB0aGluayB3ZSBjYW4gbWFrZSBh c3luY2hyb25vdXMgY29tbWl0IHNlcmlhbGl6ZSBhcyB0ZWdyYSBkcm0gZG9uZSB0byAKYXZvaWQg dGhpcyBwcm9ibGVtOgoKCiAgIDg2ICAgICAgICAgLyogc2VyaWFsaXplIG91dHN0YW5kaW5nIGFz eW5jaHJvbm91cyBjb21taXRzICovCiAgIDg3IG11dGV4X2xvY2soJnRlZ3JhLT5jb21taXQubG9j ayk7CiAgIDg4IGZsdXNoX3dvcmsoJnRlZ3JhLT5jb21taXQud29yayk7Cjg5CiAgIDkwIC8qCiAg IDkxICAgICAgICAgICogVGhpcyBpcyB0aGUgcG9pbnQgb2Ygbm8gcmV0dXJuIC0gZXZlcnl0aGlu ZyBiZWxvdyBuZXZlciAKZmFpbHMgZXhjZXB0CiAgIDkyICAgICAgICAgICogd2hlbiB0aGUgaHcg Z29lcyBib25naGl0cy4gV2hpY2ggbWVhbnMgd2UgY2FuIGNvbW1pdCAKdGhlIG5ldyBzdGF0ZSBv bgogICA5MyAgICAgICAgICAqIHRoZSBzb2Z0d2FyZSBzaWRlIG5vdy4KICAgOTQgKi8KOTUKICAg OTYgICAgICAgICBkcm1fYXRvbWljX2hlbHBlcl9zd2FwX3N0YXRlKGRybSwgc3RhdGUpOwo5Nwog ICA5OCAgICAgICAgIGlmIChhc3luYykKICAgOTkgICAgICAgICAgICAgICAgIHRlZ3JhX2F0b21p Y19zY2hlZHVsZSh0ZWdyYSwgc3RhdGUpOwogIDEwMCBlbHNlCiAgMTAxICAgICAgICAgICAgICAg ICB0ZWdyYV9hdG9taWNfY29tcGxldGUodGVncmEsIHN0YXRlKTsKICAxMDIKICAxMDMgICAgICAg ICBtdXRleF91bmxvY2soJnRlZ3JhLT5jb21taXQubG9jayk7CgoKPgo+Pj4+ICAgICAgICAgICBp ZiAoaXNfeXV2KSB7Cj4+Pj4gICAgICAgICAgICAgICAgICAgLyoKPj4+PiAgICAgICAgICAgICAg ICAgICAgKiBTcmMueDEgY2FuIGJlIG9kZCB3aGVuIGRvIGNsaXAsIGJ1dCB5dXYgcGxhbmUgc3Rh cnQKPj4+PiBwb2ludAo+Pj4+ICAgICAgICAgICAgICAgICAgICAqIG5lZWQgYWxpZ24gd2l0aCAy IHBpeGVsLgo+Pj4+ICAgICAgICAgICAgICAgICAgICAqLwo+Pj4+IC0gICAgICAgICAgICAgICB2 YWwgPSAoc3JjLngxID4+IDE2KSAlIDI7Cj4+Pj4gLSAgICAgICAgICAgICAgIHNyYy54MSArPSB2 YWwgPDwgMTY7Cj4+Pj4gLSAgICAgICAgICAgICAgIHNyYy54MiArPSB2YWwgPDwgMTY7Cj4+Pj4g KyAgICAgICAgICAgICAgIHVpbnQzMl90IHRlbXAgPSAoc3JjLT54MSA+PiAxNikgJSAyOwo+Pj4+ ICsKPj4+PiArICAgICAgICAgICAgICAgc3JjLT54MSArPSB0ZW1wIDw8IDE2Owo+Pj4+ICsgICAg ICAgICAgICAgICBzcmMtPngyICs9IHRlbXAgPDwgMTY7Cj4+Pj4gICAgICAgICAgIH0KPj4+IEkg a25vdyB0aGlzIGlzbid0IG5ldywgYnV0IG1vdmluZyB0aGUgcGxhbmUgYXJvdW5kIGlzIGJhZC4g SWYgdGhlIHVzZXIKPj4+IGdpdmVzIHlvdSBhIHBpeGVsIGJvdW5kYXJ5IHRoYXQgeW91IGNhbid0 IGFjdHVhbGx5IHVzZSwgcGxlYXNlIHJlamVjdAo+Pj4gdGhlIGNvbmZpZ3VyYXRpb24gcmF0aGVy IHRoYW4gc2lsZW50bHkgdHJ5aW5nIHRvIGZpeCBpdCB1cC4KPj4gdGhlIG9yaWdpbiBzcmMueDEg d291bGQgYWxpZ24gd2l0aCAyIHBpeGVsLCBidXQgd2hlbiB3ZSBtb3ZlIHRoZSBkZXN0Cj4+IHdp bmRvdywgYW5kIGRvIGNsaXAgYnkgb3V0cHV0LCB0aGUgc3JjLngxIG1heSBiZSBjbGlwcGVkIHRv IG9kZC4KPj4gcmVnZWN0IHRoaXMgY29uZmlndXJhdGlvbiBtYXkgbGV0IHVzZXIgY29uZnVzZSwg c29tZXRpbWVzIGdvb2QsIHNvbWV0aW1lcwo+PiBiYWQuCj4gRm9yIG1lLCBpdCBpcyBtb3JlIGNv bmZ1c2luZyB3aGVuIHRoZSBkaXNwbGF5IHNob3dzIHNvbWV0aGluZwo+IGRpZmZlcmVudCB0byB3 aGF0IEkgaGF2ZSByZXF1ZXN0ZWQuIEluIHNvbWUgbWVkaWEgdXNlY2FzZXMsIGRvaW5nIHRoaXMK PiBpcyBhIHNob3dzdG9wcGVyIGFuZCB3aWxsIHJlc3VsdCBpbiBwcm9kdWN0cyBmYWlsaW5nIGFj Y2VwdGFuY2UKPiB0ZXN0aW5nLiBVc2Vyc3BhY2UgY2FuIG1ha2UgYSBwb2xpY3kgZGVjaXNpb24g dG8gdHJ5IGRpZmZlcmVudAo+IGFsaWdubWVudHMgdG8gZ2V0IF9zb21ldGhpbmdfIHRvIHNob3cg KGV2ZW4gaWYgaXQncyBub3Qgd2hhdCB3YXMKPiBleHBsaWNpdGx5IHJlcXVlc3RlZCksIGJ1dCBk b2luZyB0aGlzIGluIHRoZSBrZXJuZWwgaXMgaW5hcHByb3ByaWF0ZToKPiBwbGVhc2UganVzdCBy ZWplY3QgaXQsIGFuZCBoYXZlIHVzZXJzcGFjZSBmYWxsIGJhY2sgdG8gYW5vdGhlcgo+IGNvbXBv c2l0aW9uIG1ldGhvZCAoZS5nLiBHTCkgaW4gdGhlc2UgY2FzZXMuCj4KPj4+PiAtc3RhdGljIHZv aWQgdm9wX3BsYW5lX2Rlc3Ryb3koc3RydWN0IGRybV9wbGFuZSAqcGxhbmUpCj4+Pj4gK3N0YXRp YyB2b2lkIHZvcF9hdG9taWNfcGxhbmVfZGVzdHJveV9zdGF0ZShzdHJ1Y3QgZHJtX3BsYW5lICpw bGFuZSwKPj4+PiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgc3Ry dWN0IGRybV9wbGFuZV9zdGF0ZSAqc3RhdGUpCj4+Pj4gICAgewo+Pj4+IC0gICAgICAgdm9wX2Rp c2FibGVfcGxhbmUocGxhbmUpOwo+Pj4+IC0gICAgICAgZHJtX3BsYW5lX2NsZWFudXAocGxhbmUp Owo+Pj4+ICsgICAgICAgc3RydWN0IHZvcF9wbGFuZV9zdGF0ZSAqdm9wX3N0YXRlID0gdG9fdm9w X3BsYW5lX3N0YXRlKHN0YXRlKTsKPj4+PiArCj4+Pj4gKyAgICAgICBpZiAoc3RhdGUtPmZiKQo+ Pj4+ICsgICAgICAgICAgICAgICBkcm1fZnJhbWVidWZmZXJfdW5yZWZlcmVuY2Uoc3RhdGUtPmZi KTsKPj4+PiArCj4+Pj4gKyAgICAgICBrZnJlZSh2b3Bfc3RhdGUpOwo+Pj4+ICAgIH0KPj4+IFlv dSBjYW4gcmVwbGFjZSB0aGlzIGhvb2sgd2l0aCBkcm1fYXRvbWljX2hlbHBlcl9wbGFuZV9kZXN0 cm95X3N0YXRlLgo+Pgo+PiBIbW0sIG9ubHkgY2FuIGhvb2sgd2l0aCBfX2RybV9hdG9taWNfaGVs cGVyX3BsYW5lX2Rlc3Ryb3lfc3RhdGUuCj4gQWggeWVzLCB5b3UncmUgcmlnaHQuIEJ1dCBzdGls bCwgdXNpbmcgdGhhdCB3b3VsZCBiZSBiZXR0ZXIgdGhhbiBkdXBsaWNhdGluZyBpdC4KPgo+PiAg ICAgIENhbiB5b3Ugc2hhcmUgeW91ciBXZXN0b24gZW52aXJvbm1lbnQgdG8gbWUsIEknbSBpbnRl cmVzdGluZyB0byB0ZXN0IGRybQo+PiByb2NrY2hpcCBvbiB3ZXN0b24uCj4gT2YgY291cnNlLiBZ b3UgY2FuIGRvd25sb2FkIFdlc3RvbiBmcm9tIGh0dHA6Ly93YXlsYW5kLmZyZWVkZXNrdG9wLm9y Zwo+IC0gdGhlIG1vc3QgaW50ZXJlc3RpbmcgZGVwZW5kZW5jaWVzIGFyZSBsaWJldmRldiwgbGli aW5wdXQsIGFuZAo+IHdheWxhbmQgaXRzZWxmLiBJZiB5b3UgYXJlIGJ1aWxkaW5nIG5ld2VyIFdl c3RvbiBmcm9tIGdpdCwgeW91J2xsIG5lZWQKPiB0aGUgd2F5bGFuZC1wcm90b2NvbHMgcmVwb3Np dG9yeSBhcyB3ZWxsLCBmcm9tCj4gYW5vbmdpdC5mcmVlZGVza3RvcC5vcmcvZ2l0L3dheWxhbmQv d2F5bGFuZC1wcm90b2NvbHMvLiBQbGVhc2UgbGV0IG1lCj4ga25vdyBwcml2YXRlbHkgaWYgeW91 IG5lZWQgc29tZSBtb3JlIGhlbHAgd2l0aCBidWlsZGluZyB0aGVzZSwgYnV0Cj4gdGhleSBzaG91 bGQgYmUgcXVpdGUgc3RyYWlnaHRmb3J3YXJkLgo+Cj4gQ2hlZXJzLAo+IERhbmllbAo+Cj4KPgoK Ci0tIArvvK1hcmsgWWFvCgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0 b3Aub3JnCmh0dHA6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmkt ZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.yao@rock-chips.com (Mark yao) Date: Fri, 11 Dec 2015 14:26:31 +0800 Subject: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API In-Reply-To: References: <1448940391-23333-1-git-send-email-mark.yao@rock-chips.com> <1448940391-23333-4-git-send-email-mark.yao@rock-chips.com> <565D68F0.3010905@rock-chips.com> Message-ID: <566A6C97.4020207@rock-chips.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2015?12?02? 22:18, Daniel Stone wrote: > Hi Mark, > Thanks for getting back to this. > > On 1 December 2015 at 09:31, Mark yao wrote: >> On 2015?12?01? 16:18, Daniel Stone wrote: >>> On 1 December 2015 at 03:26, Mark Yao wrote: >>>>> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >>>>> + if (!crtc->state->active) >>>>> + continue; >>>>> + >>>>> + rockchip_crtc_wait_for_update(crtc); >>>>> + } >>> I'd be much more comfortable if this passed in an explicit pointer to >>> state, or an address to wait for, rather than have wait_for_complete >>> dig out state with no locking. The latter is potentially racy for >>> async operations. >>> >> Hi Daniel >> "if this passed in an explicit pointer to state, or an address to wait >> for", I don't understand, can you point how it work? > Ah, OK. I mean that rockchip_crtc_wait_for_update takes a drm_crtc > pointer, and establishes the state from that (e.g. > crtc->primary->state). This can easily lead to confusion in async > contexts, as the states attached to a drm_crtc and a drm_plane can > change here while you wait for it. > > It would be better if the call was: > > for_each_plane_in_state(state, plane, plane_state, i) { > if (plane->type == DRM_PLANE_TYPE_PRIMARY) > rockchip_crtc_wait_for_update(plane_state->crtc, plane_state); > } > > This way it is very clear, and there is no confusion as to which > request we are waiting to complete. > > In general, using crtc->state or plane->state is a very bad idea, > _except_ in the atomic_check function where you are calculating > changes (e.g. if (plane_state->fb->pitches[0] != > plane->state->fb->pitches[0]) rockchip_plane_state->pitch_changed = > true). After atomic_check, you should always pass around pointers to > the plane state explicitly, and avoid using the pointers from drm_crtc > and drm_plane. > > Does that help? Hi Daniel Sorry, I don't actually understand why crtc->state or plane->state is no recommended except atomic_check, on atomic_update, we need use plane->state, is that a problem? I guess that, drm_atomic_helper_swap_state would race with async operations, so use crtc->state on async stack is not safe. is it correct? I think we can make asynchronous commit serialize as tegra drm done to avoid this problem: 86 /* serialize outstanding asynchronous commits */ 87 mutex_lock(&tegra->commit.lock); 88 flush_work(&tegra->commit.work); 89 90 /* 91 * This is the point of no return - everything below never fails except 92 * when the hw goes bonghits. Which means we can commit the new state on 93 * the software side now. 94 */ 95 96 drm_atomic_helper_swap_state(drm, state); 97 98 if (async) 99 tegra_atomic_schedule(tegra, state); 100 else 101 tegra_atomic_complete(tegra, state); 102 103 mutex_unlock(&tegra->commit.lock); > >>>> if (is_yuv) { >>>> /* >>>> * Src.x1 can be odd when do clip, but yuv plane start >>>> point >>>> * need align with 2 pixel. >>>> */ >>>> - val = (src.x1 >> 16) % 2; >>>> - src.x1 += val << 16; >>>> - src.x2 += val << 16; >>>> + uint32_t temp = (src->x1 >> 16) % 2; >>>> + >>>> + src->x1 += temp << 16; >>>> + src->x2 += temp << 16; >>>> } >>> I know this isn't new, but moving the plane around is bad. If the user >>> gives you a pixel boundary that you can't actually use, please reject >>> the configuration rather than silently trying to fix it up. >> the origin src.x1 would align with 2 pixel, but when we move the dest >> window, and do clip by output, the src.x1 may be clipped to odd. >> regect this configuration may let user confuse, sometimes good, sometimes >> bad. > For me, it is more confusing when the display shows something > different to what I have requested. In some media usecases, doing this > is a showstopper and will result in products failing acceptance > testing. Userspace can make a policy decision to try different > alignments to get _something_ to show (even if it's not what was > explicitly requested), but doing this in the kernel is inappropriate: > please just reject it, and have userspace fall back to another > composition method (e.g. GL) in these cases. > >>>> -static void vop_plane_destroy(struct drm_plane *plane) >>>> +static void vop_atomic_plane_destroy_state(struct drm_plane *plane, >>>> + struct drm_plane_state *state) >>>> { >>>> - vop_disable_plane(plane); >>>> - drm_plane_cleanup(plane); >>>> + struct vop_plane_state *vop_state = to_vop_plane_state(state); >>>> + >>>> + if (state->fb) >>>> + drm_framebuffer_unreference(state->fb); >>>> + >>>> + kfree(vop_state); >>>> } >>> You can replace this hook with drm_atomic_helper_plane_destroy_state. >> >> Hmm, only can hook with __drm_atomic_helper_plane_destroy_state. > Ah yes, you're right. But still, using that would be better than duplicating it. > >> Can you share your Weston environment to me, I'm interesting to test drm >> rockchip on weston. > Of course. You can download Weston from http://wayland.freedesktop.org > - the most interesting dependencies are libevdev, libinput, and > wayland itself. If you are building newer Weston from git, you'll need > the wayland-protocols repository as well, from > anongit.freedesktop.org/git/wayland/wayland-protocols/. Please let me > know privately if you need some more help with building these, but > they should be quite straightforward. > > Cheers, > Daniel > > > -- ?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 S1754023AbbLKG1D (ORCPT ); Fri, 11 Dec 2015 01:27:03 -0500 Received: from regular2.263xmail.com ([211.157.152.3]:59481 "EHLO regular2.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751666AbbLKG1B (ORCPT ); Fri, 11 Dec 2015 01:27:01 -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: tfiga@chromium.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: mark.yao@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <566A6C97.4020207@rock-chips.com> Date: Fri, 11 Dec 2015 14:26:31 +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: Daniel Stone CC: David Airlie , Heiko Stuebner , dri-devel , "linux-arm-kernel@lists.infradead.org" , linux-rockchip , Linux Kernel Mailing List , Tomasz Figa Subject: Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API References: <1448940391-23333-1-git-send-email-mark.yao@rock-chips.com> <1448940391-23333-4-git-send-email-mark.yao@rock-chips.com> <565D68F0.3010905@rock-chips.com> In-Reply-To: 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 2015年12月02日 22:18, Daniel Stone wrote: > Hi Mark, > Thanks for getting back to this. > > On 1 December 2015 at 09:31, Mark yao wrote: >> On 2015年12月01日 16:18, Daniel Stone wrote: >>> On 1 December 2015 at 03:26, Mark Yao wrote: >>>>> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >>>>> + if (!crtc->state->active) >>>>> + continue; >>>>> + >>>>> + rockchip_crtc_wait_for_update(crtc); >>>>> + } >>> I'd be much more comfortable if this passed in an explicit pointer to >>> state, or an address to wait for, rather than have wait_for_complete >>> dig out state with no locking. The latter is potentially racy for >>> async operations. >>> >> Hi Daniel >> "if this passed in an explicit pointer to state, or an address to wait >> for", I don't understand, can you point how it work? > Ah, OK. I mean that rockchip_crtc_wait_for_update takes a drm_crtc > pointer, and establishes the state from that (e.g. > crtc->primary->state). This can easily lead to confusion in async > contexts, as the states attached to a drm_crtc and a drm_plane can > change here while you wait for it. > > It would be better if the call was: > > for_each_plane_in_state(state, plane, plane_state, i) { > if (plane->type == DRM_PLANE_TYPE_PRIMARY) > rockchip_crtc_wait_for_update(plane_state->crtc, plane_state); > } > > This way it is very clear, and there is no confusion as to which > request we are waiting to complete. > > In general, using crtc->state or plane->state is a very bad idea, > _except_ in the atomic_check function where you are calculating > changes (e.g. if (plane_state->fb->pitches[0] != > plane->state->fb->pitches[0]) rockchip_plane_state->pitch_changed = > true). After atomic_check, you should always pass around pointers to > the plane state explicitly, and avoid using the pointers from drm_crtc > and drm_plane. > > Does that help? Hi Daniel Sorry, I don't actually understand why crtc->state or plane->state is no recommended except atomic_check, on atomic_update, we need use plane->state, is that a problem? I guess that, drm_atomic_helper_swap_state would race with async operations, so use crtc->state on async stack is not safe. is it correct? I think we can make asynchronous commit serialize as tegra drm done to avoid this problem: 86 /* serialize outstanding asynchronous commits */ 87 mutex_lock(&tegra->commit.lock); 88 flush_work(&tegra->commit.work); 89 90 /* 91 * This is the point of no return - everything below never fails except 92 * when the hw goes bonghits. Which means we can commit the new state on 93 * the software side now. 94 */ 95 96 drm_atomic_helper_swap_state(drm, state); 97 98 if (async) 99 tegra_atomic_schedule(tegra, state); 100 else 101 tegra_atomic_complete(tegra, state); 102 103 mutex_unlock(&tegra->commit.lock); > >>>> if (is_yuv) { >>>> /* >>>> * Src.x1 can be odd when do clip, but yuv plane start >>>> point >>>> * need align with 2 pixel. >>>> */ >>>> - val = (src.x1 >> 16) % 2; >>>> - src.x1 += val << 16; >>>> - src.x2 += val << 16; >>>> + uint32_t temp = (src->x1 >> 16) % 2; >>>> + >>>> + src->x1 += temp << 16; >>>> + src->x2 += temp << 16; >>>> } >>> I know this isn't new, but moving the plane around is bad. If the user >>> gives you a pixel boundary that you can't actually use, please reject >>> the configuration rather than silently trying to fix it up. >> the origin src.x1 would align with 2 pixel, but when we move the dest >> window, and do clip by output, the src.x1 may be clipped to odd. >> regect this configuration may let user confuse, sometimes good, sometimes >> bad. > For me, it is more confusing when the display shows something > different to what I have requested. In some media usecases, doing this > is a showstopper and will result in products failing acceptance > testing. Userspace can make a policy decision to try different > alignments to get _something_ to show (even if it's not what was > explicitly requested), but doing this in the kernel is inappropriate: > please just reject it, and have userspace fall back to another > composition method (e.g. GL) in these cases. > >>>> -static void vop_plane_destroy(struct drm_plane *plane) >>>> +static void vop_atomic_plane_destroy_state(struct drm_plane *plane, >>>> + struct drm_plane_state *state) >>>> { >>>> - vop_disable_plane(plane); >>>> - drm_plane_cleanup(plane); >>>> + struct vop_plane_state *vop_state = to_vop_plane_state(state); >>>> + >>>> + if (state->fb) >>>> + drm_framebuffer_unreference(state->fb); >>>> + >>>> + kfree(vop_state); >>>> } >>> You can replace this hook with drm_atomic_helper_plane_destroy_state. >> >> Hmm, only can hook with __drm_atomic_helper_plane_destroy_state. > Ah yes, you're right. But still, using that would be better than duplicating it. > >> Can you share your Weston environment to me, I'm interesting to test drm >> rockchip on weston. > Of course. You can download Weston from http://wayland.freedesktop.org > - the most interesting dependencies are libevdev, libinput, and > wayland itself. If you are building newer Weston from git, you'll need > the wayland-protocols repository as well, from > anongit.freedesktop.org/git/wayland/wayland-protocols/. Please let me > know privately if you need some more help with building these, but > they should be quite straightforward. > > Cheers, > Daniel > > > -- Ｍark Yao