From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects Date: Wed, 21 Feb 2018 17:07:27 +0200 Message-ID: <20180221150727.GO5453@intel.com> References: <20180221143730.30285-1-robdclark@gmail.com> <20180221143730.30285-2-robdclark@gmail.com> <20180221144919.GN5453@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline 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: Rob Clark Cc: David Airlie , linux-arm-msm , Linux Kernel Mailing List , dri-devel List-Id: linux-arm-msm@vger.kernel.org T24gV2VkLCBGZWIgMjEsIDIwMTggYXQgMDk6NTQ6NDlBTSAtMDUwMCwgUm9iIENsYXJrIHdyb3Rl Ogo+IE9uIFdlZCwgRmViIDIxLCAyMDE4IGF0IDk6NDkgQU0sIFZpbGxlIFN5cmrDpGzDpAo+IDx2 aWxsZS5zeXJqYWxhQGxpbnV4LmludGVsLmNvbT4gd3JvdGU6Cj4gPiBPbiBXZWQsIEZlYiAyMSwg MjAxOCBhdCAwOTozNzoyMUFNIC0wNTAwLCBSb2IgQ2xhcmsgd3JvdGU6Cj4gPj4gRm9sbG93IHRo ZSBzYW1lIHBhdHRlcm4gb2YgbG9ja2luZyBhcyB3aXRoIG90aGVyIHN0YXRlIG9iamVjdHMuICBU aGlzCj4gPj4gYXZvaWRzIGJvaWxlcnBsYXRlIGluIHRoZSBkcml2ZXIuCj4gPgo+ID4gSSdtIG5v dCBzdXJlIHdlIHJlYWxseSB3YW50IHRvIGRvIHRoaXMuIFdoYXQgaWYgdGhlIGRyaXZlciB3YW50 cyBhCj4gPiBjdXN0b20gbG9ja2luZyBzY2hlbWUgZm9yIHRoaXMgc3RhdGU/Cj4gCj4gVGhhdCBz ZWVtcyBsaWtlIHNvbWV0aGluZyB3ZSB3YW50IHRvIGRpc2NvdXJhZ2UsIGllLiBhbGwgdGhlIG1v cmUKPiByZWFzb24gZm9yIHRoaXMgcGF0Y2guCj4gCj4gVGhlcmUgaXMgbm8gcmVhc29uIGRyaXZl cnMgY291bGQgbm90IHNwbGl0IHRoZWlyIGdsb2JhbCBzdGF0ZSBpbnRvCj4gbXVsdGlwbGUgcHJp dmF0ZSBvYmpzJ3MsIGVhY2ggd2l0aCB0aGVpciBvd24gbG9jaywgZm9yIG1vcmUgZmluZQo+IGdy YWluZWQgbG9ja2luZy4gIFRoYXQgaXMgYmFzaWNhbGx5IHRoZSBvbmx5IHZhbGlkIHJlYXNvbiBJ IGNhbiB0aGluawo+IG9mIGZvciAiY3VzdG9tIGxvY2tpbmciLgoKSW4gaTkxNSB3ZSBoYXZlIGF0 IGxlYXN0IG9uZSBjYXNlIHRoYXQgd291bGQgd2FudCBzb21ldGhpbmcgY2xvc2UgdG8gYW4Kcnds b2NrLiBBbnkgY3J0YyBsb2NrIGlzIGVub3VnaCBmb3IgcmVhZCwgbmVlZCBhbGwgb2YgdGhlbSBm b3Igd3JpdGUuClRob3VnaCBpZiB3ZSB3YW50ZWQgdG8gdXNlIHByaXZhdGUgb2JqcyBmb3IgdGhh dCB3ZSBtaWdodCBuZWVkIHRvCmFjdHVhbGx5IG1ha2UgdGhlIHN0YXRlcyByZWZjb3VudGVkIGFz IHdlbGwsIG90aGVyd2lzZSBJIGNhbiBpbWFnaW5lCndlIG1pZ2h0IGxhbmQgaW4gc29tZSB1c2Ut YWZ0ZXItZnJlZSBpc3N1ZXMgb25jZSBhZ2Fpbi4KCk1heWJlIHdlIGNvdWxkIGR1cGxpY2F0ZSB0 aGUgc3RhdGUgaW50byBwZXItY3J0YyBhbmQgZ2xvYmFsIGNvcGllcywgYnV0CnRoZW4gd2UgaGF2 ZSB0byBrZWVwIGFsbCBvZiB0aG9zZSBpbiBzeW5jIHNvbWVob3cgd2hpY2ggZG9lc24ndCBzb3Vu ZApwYXJ0aWN1bGFybHkgcGxlYXNhbnQuCgo+IAo+IChBbmQgb2ZjIGRyaXZlcnMgY291bGQgYWRk IHRoZXJlIG93biBsb2NrcyBpbiBhZGRpdGlvbiB0byB3aGF0IGlzIGRvbmUKPiBieSBjb3JlLCBi dXQgSSdkIHJhdGhlciBsb29rIGF0IHRoYXQgb24gYSBjYXNlIGJ5IGNhc2UgYmFzaXMsIHJhdGhl cgo+IHRoYW4gaXQgYmVpbmcgcGFydCBvZiB0aGUgYm9pbGVycGxhdGUgaW4gZWFjaCBkcml2ZXIu KQo+IAo+IEJSLAo+IC1SCj4gCj4gPj4KPiA+PiBTaWduZWQtb2ZmLWJ5OiBSb2IgQ2xhcmsgPHJv YmRjbGFya0BnbWFpbC5jb20+Cj4gPj4gLS0tCj4gPj4gIGRyaXZlcnMvZ3B1L2RybS9kcm1fYXRv bWljLmMgfCA5ICsrKysrKysrLQo+ID4+ICBpbmNsdWRlL2RybS9kcm1fYXRvbWljLmggICAgIHwg NSArKysrKwo+ID4+ICAyIGZpbGVzIGNoYW5nZWQsIDEzIGluc2VydGlvbnMoKyksIDEgZGVsZXRp b24oLSkKPiA+Pgo+ID4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vZHJtX2F0b21pYy5j IGIvZHJpdmVycy9ncHUvZHJtL2RybV9hdG9taWMuYwo+ID4+IGluZGV4IGZjOGM0ZGE0MDlmZi4u MDA0ZTYyMWFiMzA3IDEwMDY0NAo+ID4+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fYXRvbWlj LmMKPiA+PiArKysgYi9kcml2ZXJzL2dwdS9kcm0vZHJtX2F0b21pYy5jCj4gPj4gQEAgLTEwNzgs NiArMTA3OCw4IEBAIGRybV9hdG9taWNfcHJpdmF0ZV9vYmpfaW5pdChzdHJ1Y3QgZHJtX3ByaXZh dGVfb2JqICpvYmosCj4gPj4gIHsKPiA+PiAgICAgICBtZW1zZXQob2JqLCAwLCBzaXplb2YoKm9i aikpOwo+ID4+Cj4gPj4gKyAgICAgZHJtX21vZGVzZXRfbG9ja19pbml0KCZvYmotPmxvY2spOwo+ ID4+ICsKPiA+PiAgICAgICBvYmotPnN0YXRlID0gc3RhdGU7Cj4gPj4gICAgICAgb2JqLT5mdW5j cyA9IGZ1bmNzOwo+ID4+ICB9Cj4gPj4gQEAgLTEwOTMsNiArMTA5NSw3IEBAIHZvaWQKPiA+PiAg ZHJtX2F0b21pY19wcml2YXRlX29ial9maW5pKHN0cnVjdCBkcm1fcHJpdmF0ZV9vYmogKm9iaikK PiA+PiAgewo+ID4+ICAgICAgIG9iai0+ZnVuY3MtPmF0b21pY19kZXN0cm95X3N0YXRlKG9iaiwg b2JqLT5zdGF0ZSk7Cj4gPj4gKyAgICAgZHJtX21vZGVzZXRfbG9ja19maW5pKCZvYmotPmxvY2sp Owo+ID4+ICB9Cj4gPj4gIEVYUE9SVF9TWU1CT0woZHJtX2F0b21pY19wcml2YXRlX29ial9maW5p KTsKPiA+Pgo+ID4+IEBAIC0xMTEzLDcgKzExMTYsNyBAQCBzdHJ1Y3QgZHJtX3ByaXZhdGVfc3Rh dGUgKgo+ID4+ICBkcm1fYXRvbWljX2dldF9wcml2YXRlX29ial9zdGF0ZShzdHJ1Y3QgZHJtX2F0 b21pY19zdGF0ZSAqc3RhdGUsCj4gPj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHN0 cnVjdCBkcm1fcHJpdmF0ZV9vYmogKm9iaikKPiA+PiAgewo+ID4+IC0gICAgIGludCBpbmRleCwg bnVtX29ianMsIGk7Cj4gPj4gKyAgICAgaW50IGluZGV4LCBudW1fb2JqcywgaSwgcmV0Owo+ID4+ ICAgICAgIHNpemVfdCBzaXplOwo+ID4+ICAgICAgIHN0cnVjdCBfX2RybV9wcml2YXRlX29ianNf c3RhdGUgKmFycjsKPiA+PiAgICAgICBzdHJ1Y3QgZHJtX3ByaXZhdGVfc3RhdGUgKm9ial9zdGF0 ZTsKPiA+PiBAQCAtMTEyMiw2ICsxMTI1LDEwIEBAIGRybV9hdG9taWNfZ2V0X3ByaXZhdGVfb2Jq X3N0YXRlKHN0cnVjdCBkcm1fYXRvbWljX3N0YXRlICpzdGF0ZSwKPiA+PiAgICAgICAgICAgICAg IGlmIChvYmogPT0gc3RhdGUtPnByaXZhdGVfb2Jqc1tpXS5wdHIpCj4gPj4gICAgICAgICAgICAg ICAgICAgICAgIHJldHVybiBzdGF0ZS0+cHJpdmF0ZV9vYmpzW2ldLnN0YXRlOwo+ID4+Cj4gPj4g KyAgICAgcmV0ID0gZHJtX21vZGVzZXRfbG9jaygmb2JqLT5sb2NrLCBzdGF0ZS0+YWNxdWlyZV9j dHgpOwo+ID4+ICsgICAgIGlmIChyZXQpCj4gPj4gKyAgICAgICAgICAgICByZXR1cm4gRVJSX1BU UihyZXQpOwo+ID4+ICsKPiA+PiAgICAgICBudW1fb2JqcyA9IHN0YXRlLT5udW1fcHJpdmF0ZV9v YmpzICsgMTsKPiA+PiAgICAgICBzaXplID0gc2l6ZW9mKCpzdGF0ZS0+cHJpdmF0ZV9vYmpzKSAq IG51bV9vYmpzOwo+ID4+ICAgICAgIGFyciA9IGtyZWFsbG9jKHN0YXRlLT5wcml2YXRlX29ianMs IHNpemUsIEdGUF9LRVJORUwpOwo+ID4+IGRpZmYgLS1naXQgYS9pbmNsdWRlL2RybS9kcm1fYXRv bWljLmggYi9pbmNsdWRlL2RybS9kcm1fYXRvbWljLmgKPiA+PiBpbmRleCAwOTA3NmE2MjU2Mzcu LjlhZTUzYjczYzlkMiAxMDA2NDQKPiA+PiAtLS0gYS9pbmNsdWRlL2RybS9kcm1fYXRvbWljLmgK PiA+PiArKysgYi9pbmNsdWRlL2RybS9kcm1fYXRvbWljLmgKPiA+PiBAQCAtMjE4LDYgKzIxOCwx MSBAQCBzdHJ1Y3QgZHJtX3ByaXZhdGVfc3RhdGVfZnVuY3Mgewo+ID4+ICAgKiAmZHJtX21vZGVz ZXRfbG9jayBpcyByZXF1aXJlZCB0byBkdXBsaWNhdGUgYW5kIHVwZGF0ZSB0aGlzIG9iamVjdCdz IHN0YXRlLgo+ID4+ICAgKi8KPiA+PiAgc3RydWN0IGRybV9wcml2YXRlX29iaiB7Cj4gPj4gKyAg ICAgLyoqCj4gPj4gKyAgICAgICogQGxvY2s6IE1vZGVzZXQgbG9jayB0byBwcm90ZWN0IHRoZSBz dGF0ZSBvYmplY3QuCj4gPj4gKyAgICAgICovCj4gPj4gKyAgICAgc3RydWN0IGRybV9tb2Rlc2V0 X2xvY2sgbG9jazsKPiA+PiArCj4gPj4gICAgICAgLyoqCj4gPj4gICAgICAgICogQHN0YXRlOiBD dXJyZW50IGF0b21pYyBzdGF0ZSBmb3IgdGhpcyBkcml2ZXIgcHJpdmF0ZSBvYmplY3QuCj4gPj4g ICAgICAgICovCj4gPj4gLS0KPiA+PiAyLjE0LjMKPiA+Pgo+ID4+IF9fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCj4gPj4gZHJpLWRldmVsIG1haWxpbmcgbGlz dAo+ID4+IGRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKPiA+PiBodHRwczovL2xpc3Rz LmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo+ID4KPiA+IC0tCj4g PiBWaWxsZSBTeXJqw6Rsw6QKPiA+IEludGVsIE9UQwoKLS0gClZpbGxlIFN5cmrDpGzDpApJbnRl bCBPVEMKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJp LWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBz Oi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965925AbeBUPHc (ORCPT ); Wed, 21 Feb 2018 10:07:32 -0500 Received: from mga04.intel.com ([192.55.52.120]:5178 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932826AbeBUPHb (ORCPT ); Wed, 21 Feb 2018 10:07:31 -0500 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,543,1511856000"; d="scan'208";a="21888900" Date: Wed, 21 Feb 2018 17:07:27 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Rob Clark Cc: dri-devel , David Airlie , linux-arm-msm , Linux Kernel Mailing List Subject: Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects Message-ID: <20180221150727.GO5453@intel.com> References: <20180221143730.30285-1-robdclark@gmail.com> <20180221143730.30285-2-robdclark@gmail.com> <20180221144919.GN5453@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 21, 2018 at 09:54:49AM -0500, Rob Clark wrote: > On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrjälä > wrote: > > On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote: > >> Follow the same pattern of locking as with other state objects. This > >> avoids boilerplate in the driver. > > > > I'm not sure we really want to do this. What if the driver wants a > > custom locking scheme for this state? > > That seems like something we want to discourage, ie. all the more > reason for this patch. > > There is no reason drivers could not split their global state into > multiple private objs's, each with their own lock, for more fine > grained locking. That is basically the only valid reason I can think > of for "custom locking". In i915 we have at least one case that would want something close to an rwlock. Any crtc lock is enough for read, need all of them for write. Though if we wanted to use private objs for that we might need to actually make the states refcounted as well, otherwise I can imagine we might land in some use-after-free issues once again. Maybe we could duplicate the state into per-crtc and global copies, but then we have to keep all of those in sync somehow which doesn't sound particularly pleasant. > > (And ofc drivers could add there own locks in addition to what is done > by core, but I'd rather look at that on a case by case basis, rather > than it being part of the boilerplate in each driver.) > > BR, > -R > > >> > >> Signed-off-by: Rob Clark > >> --- > >> drivers/gpu/drm/drm_atomic.c | 9 ++++++++- > >> include/drm/drm_atomic.h | 5 +++++ > >> 2 files changed, 13 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >> index fc8c4da409ff..004e621ab307 100644 > >> --- a/drivers/gpu/drm/drm_atomic.c > >> +++ b/drivers/gpu/drm/drm_atomic.c > >> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj, > >> { > >> memset(obj, 0, sizeof(*obj)); > >> > >> + drm_modeset_lock_init(&obj->lock); > >> + > >> obj->state = state; > >> obj->funcs = funcs; > >> } > >> @@ -1093,6 +1095,7 @@ void > >> drm_atomic_private_obj_fini(struct drm_private_obj *obj) > >> { > >> obj->funcs->atomic_destroy_state(obj, obj->state); > >> + drm_modeset_lock_fini(&obj->lock); > >> } > >> EXPORT_SYMBOL(drm_atomic_private_obj_fini); > >> > >> @@ -1113,7 +1116,7 @@ struct drm_private_state * > >> drm_atomic_get_private_obj_state(struct drm_atomic_state *state, > >> struct drm_private_obj *obj) > >> { > >> - int index, num_objs, i; > >> + int index, num_objs, i, ret; > >> size_t size; > >> struct __drm_private_objs_state *arr; > >> struct drm_private_state *obj_state; > >> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, > >> if (obj == state->private_objs[i].ptr) > >> return state->private_objs[i].state; > >> > >> + ret = drm_modeset_lock(&obj->lock, state->acquire_ctx); > >> + if (ret) > >> + return ERR_PTR(ret); > >> + > >> num_objs = state->num_private_objs + 1; > >> size = sizeof(*state->private_objs) * num_objs; > >> arr = krealloc(state->private_objs, size, GFP_KERNEL); > >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > >> index 09076a625637..9ae53b73c9d2 100644 > >> --- a/include/drm/drm_atomic.h > >> +++ b/include/drm/drm_atomic.h > >> @@ -218,6 +218,11 @@ struct drm_private_state_funcs { > >> * &drm_modeset_lock is required to duplicate and update this object's state. > >> */ > >> struct drm_private_obj { > >> + /** > >> + * @lock: Modeset lock to protect the state object. > >> + */ > >> + struct drm_modeset_lock lock; > >> + > >> /** > >> * @state: Current atomic state for this driver private object. > >> */ > >> -- > >> 2.14.3 > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Ville Syrjälä > > Intel OTC -- Ville Syrjälä Intel OTC