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 18:36:54 +0200 Message-ID: <20180221163654.GS5453@intel.com> References: <20180221143730.30285-1-robdclark@gmail.com> <20180221143730.30285-2-robdclark@gmail.com> <20180221144919.GN5453@intel.com> <20180221150727.GO5453@intel.com> <20180221152720.GP5453@intel.com> <20180221155410.GQ5453@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 T24gV2VkLCBGZWIgMjEsIDIwMTggYXQgMTE6MTc6MjFBTSAtMDUwMCwgUm9iIENsYXJrIHdyb3Rl Ogo+IE9uIFdlZCwgRmViIDIxLCAyMDE4IGF0IDEwOjU0IEFNLCBWaWxsZSBTeXJqw6Rsw6QKPiA8 dmlsbGUuc3lyamFsYUBsaW51eC5pbnRlbC5jb20+IHdyb3RlOgo+ID4gT24gV2VkLCBGZWIgMjEs IDIwMTggYXQgMTA6MzY6MDZBTSAtMDUwMCwgUm9iIENsYXJrIHdyb3RlOgo+ID4+IE9uIFdlZCwg RmViIDIxLCAyMDE4IGF0IDEwOjI3IEFNLCBWaWxsZSBTeXJqw6Rsw6QKPiA+PiA8dmlsbGUuc3ly amFsYUBsaW51eC5pbnRlbC5jb20+IHdyb3RlOgo+ID4+ID4gT24gV2VkLCBGZWIgMjEsIDIwMTgg YXQgMTA6MjA6MDNBTSAtMDUwMCwgUm9iIENsYXJrIHdyb3RlOgo+ID4+ID4+IE9uIFdlZCwgRmVi IDIxLCAyMDE4IGF0IDEwOjA3IEFNLCBWaWxsZSBTeXJqw6Rsw6QKPiA+PiA+PiA8dmlsbGUuc3ly amFsYUBsaW51eC5pbnRlbC5jb20+IHdyb3RlOgo+ID4+ID4+ID4gT24gV2VkLCBGZWIgMjEsIDIw MTggYXQgMDk6NTQ6NDlBTSAtMDUwMCwgUm9iIENsYXJrIHdyb3RlOgo+ID4+ID4+ID4+IE9uIFdl ZCwgRmViIDIxLCAyMDE4IGF0IDk6NDkgQU0sIFZpbGxlIFN5cmrDpGzDpAo+ID4+ID4+ID4+IDx2 aWxsZS5zeXJqYWxhQGxpbnV4LmludGVsLmNvbT4gd3JvdGU6Cj4gPj4gPj4gPj4gPiBPbiBXZWQs IEZlYiAyMSwgMjAxOCBhdCAwOTozNzoyMUFNIC0wNTAwLCBSb2IgQ2xhcmsgd3JvdGU6Cj4gPj4g Pj4gPj4gPj4gRm9sbG93IHRoZSBzYW1lIHBhdHRlcm4gb2YgbG9ja2luZyBhcyB3aXRoIG90aGVy IHN0YXRlIG9iamVjdHMuICBUaGlzCj4gPj4gPj4gPj4gPj4gYXZvaWRzIGJvaWxlcnBsYXRlIGlu IHRoZSBkcml2ZXIuCj4gPj4gPj4gPj4gPgo+ID4+ID4+ID4+ID4gSSdtIG5vdCBzdXJlIHdlIHJl YWxseSB3YW50IHRvIGRvIHRoaXMuIFdoYXQgaWYgdGhlIGRyaXZlciB3YW50cyBhCj4gPj4gPj4g Pj4gPiBjdXN0b20gbG9ja2luZyBzY2hlbWUgZm9yIHRoaXMgc3RhdGU/Cj4gPj4gPj4gPj4KPiA+ PiA+PiA+PiBUaGF0IHNlZW1zIGxpa2Ugc29tZXRoaW5nIHdlIHdhbnQgdG8gZGlzY291cmFnZSwg aWUuIGFsbCB0aGUgbW9yZQo+ID4+ID4+ID4+IHJlYXNvbiBmb3IgdGhpcyBwYXRjaC4KPiA+PiA+ PiA+Pgo+ID4+ID4+ID4+IFRoZXJlIGlzIG5vIHJlYXNvbiBkcml2ZXJzIGNvdWxkIG5vdCBzcGxp dCB0aGVpciBnbG9iYWwgc3RhdGUgaW50bwo+ID4+ID4+ID4+IG11bHRpcGxlIHByaXZhdGUgb2Jq cydzLCBlYWNoIHdpdGggdGhlaXIgb3duIGxvY2ssIGZvciBtb3JlIGZpbmUKPiA+PiA+PiA+PiBn cmFpbmVkIGxvY2tpbmcuICBUaGF0IGlzIGJhc2ljYWxseSB0aGUgb25seSB2YWxpZCByZWFzb24g SSBjYW4gdGhpbmsKPiA+PiA+PiA+PiBvZiBmb3IgImN1c3RvbSBsb2NraW5nIi4KPiA+PiA+PiA+ Cj4gPj4gPj4gPiBJbiBpOTE1IHdlIGhhdmUgYXQgbGVhc3Qgb25lIGNhc2UgdGhhdCB3b3VsZCB3 YW50IHNvbWV0aGluZyBjbG9zZSB0byBhbgo+ID4+ID4+ID4gcndsb2NrLiBBbnkgY3J0YyBsb2Nr IGlzIGVub3VnaCBmb3IgcmVhZCwgbmVlZCBhbGwgb2YgdGhlbSBmb3Igd3JpdGUuCj4gPj4gPj4g PiBUaG91Z2ggaWYgd2Ugd2FudGVkIHRvIHVzZSBwcml2YXRlIG9ianMgZm9yIHRoYXQgd2UgbWln aHQgbmVlZCB0bwo+ID4+ID4+ID4gYWN0dWFsbHkgbWFrZSB0aGUgc3RhdGVzIHJlZmNvdW50ZWQg YXMgd2VsbCwgb3RoZXJ3aXNlIEkgY2FuIGltYWdpbmUKPiA+PiA+PiA+IHdlIG1pZ2h0IGxhbmQg aW4gc29tZSB1c2UtYWZ0ZXItZnJlZSBpc3N1ZXMgb25jZSBhZ2Fpbi4KPiA+PiA+PiA+Cj4gPj4g Pj4gPiBNYXliZSB3ZSBjb3VsZCBkdXBsaWNhdGUgdGhlIHN0YXRlIGludG8gcGVyLWNydGMgYW5k IGdsb2JhbCBjb3BpZXMsIGJ1dAo+ID4+ID4+ID4gdGhlbiB3ZSBoYXZlIHRvIGtlZXAgYWxsIG9m IHRob3NlIGluIHN5bmMgc29tZWhvdyB3aGljaCBkb2Vzbid0IHNvdW5kCj4gPj4gPj4gPiBwYXJ0 aWN1bGFybHkgcGxlYXNhbnQuCj4gPj4gPj4KPiA+PiA+PiBPciBqdXN0IGtlZXAgeW91ciBvd24g ZHJpdmVyIGxvY2sgZm9yIHJlYWQsIGFuZCB1c2UgdGhhdCBwbHVzIHRoZSBjb3JlCj4gPj4gPj4g bW9kZXNldCBsb2NrIGZvciB3cml0ZT8KPiA+PiA+Cj4gPj4gPiBJZiB3ZSBjYW4ndCBhZGQgdGhl IHByaXZhdGUgb2JqIHRvIHRoZSBzdGF0ZSB3ZSBjYW4ndCByZWFsbHkgdXNlIGl0Lgo+ID4+ID4K PiA+Pgo+ID4+IEknbSBub3Qgc3VyZSB3aHkgdGhhdCBpcyBzdHJpY3RseSB0cnVlICh0aGF0IHlv dSBuZWVkIHRvIGFkZCBpdCB0byB0aGUKPiA+PiBzdGF0ZSBpZiBmb3IgcmVhZC1vbmx5KSwgc2lu Y2UgeW91J2QgYmUgZ3VhcmRpbmcgaXQgd2l0aCB5b3VyIG93bgo+ID4+IGRyaXZlciByZWFkLWxv Y2sgeW91IGNhbiBqdXN0IHByaXYtPmZvb19zdGF0ZS0+YmFyLgo+ID4+Cj4gPj4gU2luY2UgaXQg aXMgcmVhZC1vbmx5IGFjY2VzcywgdGhlcmUgaXMgbm8gcm9sbC1iYWNrIHRvIHdvcnJ5IGFib3V0 IGZvcgo+ID4+IHRlc3Qtb25seSBvciBmYWlsZWQgYXRvbWljX2NoZWNrKClzLi4KPiA+Cj4gPiBU aGF0IHdvdWxkIGJlIHN1cGVyIHVnbHkuIFdlIHdhbnQgdG8gYWNjZXNzIHRoZSBpbmZvcm1hdGlv biB0aGUgc2FtZQo+ID4gd2F5IHdoZXRoZXIgaXQgaGFzIGJlZW4gbW9kaWZpZWQgb3Igbm90Lgo+ IAo+IFdlbGwsIEkgbWVhbiB0aGUgd2hvbGUgaWRlYSBvZiB3aGF0IHlvdSB3YW50IHRvIGRvIHNl ZW1zIGEgYml0IHN1cGVyLXVnbHkgOy0pCj4gCj4gSSBtZWFuLCBpbiBtZHA1IHRoZSBhc3NpZ25l ZCBnbG9iYWwgcmVzb3VyY2VzIGdvIGluIHBsYW5lL2NydGMgc3RhdGUsCj4gYW5kIHRyYWNraW5n IG9mIHdoYXQgaXMgYXNzaWduZWQgdG8gd2hpY2ggcGxhbmUvY3J0YyBpcyBpbiBnbG9iYWwKPiBz dGF0ZSwgc28gaXQgZml0cyBuaWNlbHkgaW4gdGhlIGN1cnJlbnQgbG9ja2luZyBtb2RlbC4gIEZv ciBpOTE1LCBJJ20KPiBub3QgcXVpdGUgc3VyZSB3aGF0IGlzIHRoZSBnbG9iYWwgc3RhdGUgeW91 IGFyZSBjb25jZXJuZWQgYWJvdXQsIHNvIGl0Cj4gaXMgYSBiaXQgaGFyZCB0byB0YWxrIGFib3V0 IHRoZSBiZXN0IHNvbHV0aW9uIGluIHRoZSBhYnN0cmFjdC4gIE1heWJlCj4gdGhlIGJldHRlciBv cHRpb24gaXMgdG8gdGVhY2ggbW9kZXNldC1sb2NrIGhvdyB0byBiZSBhIHJ3bG9jayBpbnN0ZWFk PwoKVGhlIHRoaW5nIEknbSB0aGlua2luZyBpcyB0aGUgY29yZSBkaXNwbGF5IGNsb2NrIChjZGNs aykgZnJlcXVlbmN5IHdoaWNoCndlIG5lZWQgdG8gY29uc3VsdCB3aGVuZXZlciBjb21wdXRpbmcg cGxhbmUgc3RhdGVzIGFuZCB3aGF0bm90LiBXZSBkb24ndAp3YW50IGEgbW9kZXNldCBvbiBvbmUg Y3J0YyB0byBibG9jayBhIHBsYW5lIHVwZGF0ZSBvbiBhbm90aGVyIGNydGMKdW5sZXNzIHdlIGFj dHVhbGx5IGhhdmUgdG8gYnVtcCB0aGUgY2RjbGsgKHdoaWNoIHdvdWxkIGdlbmVyYWxseSByZXF1 aXJlCmFsbCBjcnRjcyB0byB1bmRlcmdvIGEgZnVsbCBtb2Rlc2V0KS4gU2VlbXMgbGlrZSBhIGdl bmVyYWxseSB1c2VmdWwKcGF0dGVybiB0byBtZS4KCi0tIApWaWxsZSBTeXJqw6Rsw6QKSW50ZWwg T1RDCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1k ZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczov L2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936096AbeBUQg7 (ORCPT ); Wed, 21 Feb 2018 11:36:59 -0500 Received: from mga14.intel.com ([192.55.52.115]:3497 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932674AbeBUQg6 (ORCPT ); Wed, 21 Feb 2018 11:36:58 -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,545,1511856000"; d="scan'208";a="32490339" Date: Wed, 21 Feb 2018 18:36:54 +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: <20180221163654.GS5453@intel.com> References: <20180221143730.30285-1-robdclark@gmail.com> <20180221143730.30285-2-robdclark@gmail.com> <20180221144919.GN5453@intel.com> <20180221150727.GO5453@intel.com> <20180221152720.GP5453@intel.com> <20180221155410.GQ5453@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 11:17:21AM -0500, Rob Clark wrote: > On Wed, Feb 21, 2018 at 10:54 AM, Ville Syrjälä > wrote: > > On Wed, Feb 21, 2018 at 10:36:06AM -0500, Rob Clark wrote: > >> On Wed, Feb 21, 2018 at 10:27 AM, Ville Syrjälä > >> wrote: > >> > On Wed, Feb 21, 2018 at 10:20:03AM -0500, Rob Clark wrote: > >> >> On Wed, Feb 21, 2018 at 10:07 AM, Ville Syrjälä > >> >> wrote: > >> >> > 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. > >> >> > >> >> Or just keep your own driver lock for read, and use that plus the core > >> >> modeset lock for write? > >> > > >> > If we can't add the private obj to the state we can't really use it. > >> > > >> > >> I'm not sure why that is strictly true (that you need to add it to the > >> state if for read-only), since you'd be guarding it with your own > >> driver read-lock you can just priv->foo_state->bar. > >> > >> Since it is read-only access, there is no roll-back to worry about for > >> test-only or failed atomic_check()s.. > > > > That would be super ugly. We want to access the information the same > > way whether it has been modified or not. > > Well, I mean the whole idea of what you want to do seems a bit super-ugly ;-) > > I mean, in mdp5 the assigned global resources go in plane/crtc state, > and tracking of what is assigned to which plane/crtc is in global > state, so it fits nicely in the current locking model. For i915, I'm > not quite sure what is the global state you are concerned about, so it > is a bit hard to talk about the best solution in the abstract. Maybe > the better option is to teach modeset-lock how to be a rwlock instead? The thing I'm thinking is the core display clock (cdclk) frequency which we need to consult whenever computing plane states and whatnot. We don't want a modeset on one crtc to block a plane update on another crtc unless we actually have to bump the cdclk (which would generally require all crtcs to undergo a full modeset). Seems like a generally useful pattern to me. -- Ville Syrjälä Intel OTC