From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH v2 01/11] dma-buf/sync_file: de-stage sync_file Date: Thu, 28 Jan 2016 10:23:55 +0100 Message-ID: <20160128092355.GS11240@phenom.ffwll.local> References: <1453901439-19467-1-git-send-email-gustavo@padovan.org> <1453901439-19467-2-git-send-email-gustavo@padovan.org> <56A8D4A7.1070409@linux.intel.com> <20160127170313.GC3773@joana> <20160127202540.GD3773@joana> <56A9396F.8010803@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-wm0-f65.google.com (mail-wm0-f65.google.com [74.125.82.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6FA156E7C7 for ; Thu, 28 Jan 2016 01:23:48 -0800 (PST) Received: by mail-wm0-f65.google.com with SMTP id r129so2449858wmr.0 for ; Thu, 28 Jan 2016 01:23:48 -0800 (PST) Content-Disposition: inline In-Reply-To: <56A9396F.8010803@google.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Greg Hackmann Cc: devel@driverdev.osuosl.org, Daniel Stone , "Linux-Kernel@Vger. Kernel. Org" , ML dri-devel , Daniel Vetter , Emil Velikov , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Greg Kroah-Hartman , Riley Andrews , Gustavo Padovan , John Harrison List-Id: dri-devel@lists.freedesktop.org T24gV2VkLCBKYW4gMjcsIDIwMTYgYXQgMDE6NDE6MDNQTSAtMDgwMCwgR3JlZyBIYWNrbWFubiB3 cm90ZToKPiBPbiAwMS8yNy8yMDE2IDEyOjI1IFBNLCBHdXN0YXZvIFBhZG92YW4gd3JvdGU6Cj4g Pj4+PklzIHRoZXJlIGEgdmFsdWUgaW4ga2VlcGluZyB0aGUgYWJpIHVuY2hhbmdlZD8KPiA+Pj4+ SWYgbm90LCB0aGVuIERvY3VtZW50YXRpb24vaW9jdGwvYm90Y2hpbmctdXAtaW9jdGxzLnR4dCBp cyB3b3J0aCBhIHJlYWQuCj4gPj4+Cj4gPj4+Tm9uZSBmcm9tIG1lLiBJJ2xsIGxvb2sgd2hlcmUg d2UgY2FuIGltcHJvdmUgdGhlIEFCSS4KPiAKPiBBbmRyb2lkIGhhcyBleGlzdGluZyBjbGllbnRz IG9mIHRoZSBjdXJyZW50IEFCSS4gIFRoYW5rZnVsbHkgdGhleSdyZSBhbGwKPiBjb250YWluZWQg aW4gc3lzdGVtIHNlcnZpY2VzIGxpa2UgU3VyZmFjZUZsaW5nZXIsIHNpbmNlIGVuZC11c2VyIGFw cHMgZG9uJ3QKPiBnZXQgZGlyZWN0IGFjY2VzcyB0byBmZW5jZSBmZHMuCj4gCj4gQXMgbG9uZyB0 aGUgQUJJIGJyZWFrcyBkb24ndCByZW1vdmUgZnVuY3Rpb25hbGl0eSB3ZSBkZXBlbmQgb24sIHdl IGNhbiB3cmFwCj4gYXJvdW5kIHRoZW0gaW4gb3VyIHVzZXJzcGFjZSBsaWJzeW5jLiAgSSdkIHJh dGhlciBub3QgaGF2ZSB0byBkbyB0aGF0LCBidXQKPiBpdCdzIGEgcHJpY2UgSSdtIHdpbGxpbmcg dG8gcGF5IHRvIGdldCB0aGlzIG1vdmVkIG91dCBvZiBzdGFnaW5nLgo+IAo+ID4+ICAtIHN0cnVj dCBzeW5jX2ZpbGVfaW5mb19kYXRhOjpmZW5jZV9pbmZvIGlzIG9mIHR5cGUgX191OCB5ZXQgaXQg aXMgImEKPiA+PmZlbmNlX2luZm8gc3RydWN0IGZvciBldmVyeSBmZW5jZSBpbiB0aGUgc3luY19m aWxlIi4gVGh1cyBzaG91bGRuJ3QKPiA+Pm9uZSB1c2UgInN0cnVjdCBmZW5jZV9pbmZvIiBhcyB0 aGUgdHlwZSA/Cj4gPgo+ID5BZ3JlZWQuIEJ1dCBJJ20gY3VycmVudGx5IHRoaW5raW5nIGlmIHdl IHJlYWxseSBzaG91bGQga2VlcCB0aGlzIGlvY3RsLgo+ID4KPiA+CUd1c3Rhdm8KPiA+Cj4gCj4g SSdtIG5vdCBzZWVpbmcgYW55IGNvbnN1bWVycyBvZiBkcml2ZXJfZGF0YSBpbiBvdXIgdHJlZS4g IE9UT0ggY29tcGxldGVseQo+IGdldHRpbmcgcmlkIG9mIHRoZSBpb2N0bCB3b3VsZCBiZSBhIHBy b2JsZW0sIHNpbmNlIFN1cmZhY2VGbGluZ2VyIGRlcGVuZHMgb24KPiB0aGUgdGltZXN0YW1wIGlu Zm9ybWF0aW9uIGZvciBpdHMgb3duIGJvb2trZWVwaW5nLgoKSWYgd2UgcmVtb3ZlIGRyaXZlcl9k YXRhIChhbmQgbGVuIGlzIHN1cGVyZmxvdXMgdG9vKSwgdGhlbiBJIHRoaW5rIHdlCnNob3VsZCBh bHNvIG1ha2UgdGhlIG1hc3RlciBzdHJ1Y3QgdXNlIGNvbW1vbiBpb2N0bCBwYXR0ZXJuOgotIEFk ZCBhIG51bV9mZW5jZXMgZmllbGQgb3Igc2ltaWxhciB0aGF0IHRoZSBrZXJuZWwgZmlsbHMgb3V0 LgotIE1ha2UgcHRfaW5mbyBhbiBfX3U2NCBwb2ludGVyIGluc3RlYWQgb2YgYSB2YXJpYWJsZS1s ZW5ndGggYXJyYXkgKGFuZAogIGxlbmd0aCkgLSBpb2N0bCBwYXlsb2FkIHNpemVzIGFyZSBzb21l d2hhdCBsaW1pdGVkLgoKVGhpcyB3YXkgdGhlIGludGVyZmFjZSBpcyBmdXR1cmUtcHJvb2ZlZCBm b3IgdHJ1bHkgcGF0YWxvZ2ljYWwgbnVtYmVyIG9mCmZlbmNlcyAod2hpY2ggc3VyZmFjZSBmbGlu Z2VyIHdvbid0IGRvLCBidXQgY291bGQgaGFwcGVuIGluCnNlcnZlci9vcGVuY2wvbWVkaWEgd29y a2xvYWRzIEknZCBpbWFnaW5lKS4KCkFuZCBJIHRoaW5rIGRyaXZlcl9kYXRhIHJlYWxseSBzaG91 bGRuJ3QgYmUgdGhlcmUsIGl0IG1ha2VzIHRoaW5ncwpjb21wbGljYXRlZCB3aXRoIHRoZSBhcnJh eSBvZiB2YXJpYWJsZS1zaXplZCBvYmplY3RzLCBhbmQgZ2VuZXJpYwp1c2Vyc3BhY2UgY2FuJ3Qg cmVhbGx5IHVzZSBpdCAtIGZvciBkZWJ1ZyBvdXRwdXQgd2UgYWxyZWFkeSBoYXZlCm9iai9kcml2 ZXJfbmFtZSBwZXIgZmVuY2UgcG9pbnQsIHdoaWNoIEkgdGhpbmsgaXMgZ29vZCBlbm91Z2guICAK CldvdWxkIHRoYXQgYmUgb2sgZm9yIHlvdSBmcm9tIHRoZSBBbmRyb2lkIHNpZGUgaWYgR3VzdGF2 byBhbHNvIHByb3ZpZGVzIGEKcGF0Y2ggdG8gdXBkYXRlIGxpYnN5bmM/IEkgZG9uJ3QgdGhpbmsg dGhlIEFCSSBpcyBmdW5kYW1lbnRhbGx5IGJyb2tlbiwKYnV0IHRoaXMgbGlnaHQgY2xlYW51cCB3 b3VsZCBiZSBuaWNlLgoKV3J0IGtlZXBpbmcgU1lOQ19XQUlUOiBJIHRoaW5rIHRoYXQncyB0b3Rh bGx5IGZpbmUuIFJlZHVuZGFudCBzaW5jZQpwb2xsaW5nIGlzIHN1cHBvcnRlZCwgYnV0IG5vdCBy ZWFsbHkgYW4gaXNzdWUgaW1vIGVpdGhlci4gSWYgd2UncmUgdG90YWxseQpsYXp5IHdlIGNvdWxk IGltcGxlbWVudCBTWU5DX1dBSVQgaW50ZXJuYWxseSB1c2luZyBwb2xsIGFuZCBzaGF2ZSBvZmYg YQpmZXcgbGluZXMgb2YgdGhlIGltcGxlbWVudGF0aW9uLgotRGFuaWVsCi0tIApEYW5pZWwgVmV0 dGVyClNvZnR3YXJlIEVuZ2luZWVyLCBJbnRlbCBDb3Jwb3JhdGlvbgpodHRwOi8vYmxvZy5mZnds bC5jaApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmkt ZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cDov 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 S966675AbcA1JXy (ORCPT ); Thu, 28 Jan 2016 04:23:54 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:33495 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964994AbcA1JXs (ORCPT ); Thu, 28 Jan 2016 04:23:48 -0500 Date: Thu, 28 Jan 2016 10:23:55 +0100 From: Daniel Vetter To: Greg Hackmann Cc: Gustavo Padovan , Emil Velikov , Maarten Lankhorst , Greg Kroah-Hartman , "Linux-Kernel@Vger. Kernel. Org" , devel@driverdev.osuosl.org, ML dri-devel , Daniel Stone , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Daniel Vetter , Rob Clark , John Harrison , Gustavo Padovan Subject: Re: [PATCH v2 01/11] dma-buf/sync_file: de-stage sync_file Message-ID: <20160128092355.GS11240@phenom.ffwll.local> Mail-Followup-To: Greg Hackmann , Gustavo Padovan , Emil Velikov , Maarten Lankhorst , Greg Kroah-Hartman , "Linux-Kernel@Vger. Kernel. Org" , devel@driverdev.osuosl.org, ML dri-devel , Daniel Stone , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Rob Clark , John Harrison , Gustavo Padovan References: <1453901439-19467-1-git-send-email-gustavo@padovan.org> <1453901439-19467-2-git-send-email-gustavo@padovan.org> <56A8D4A7.1070409@linux.intel.com> <20160127170313.GC3773@joana> <20160127202540.GD3773@joana> <56A9396F.8010803@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56A9396F.8010803@google.com> X-Operating-System: Linux phenom 4.3.0-1-amd64 User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 27, 2016 at 01:41:03PM -0800, Greg Hackmann wrote: > On 01/27/2016 12:25 PM, Gustavo Padovan wrote: > >>>>Is there a value in keeping the abi unchanged? > >>>>If not, then Documentation/ioctl/botching-up-ioctls.txt is worth a read. > >>> > >>>None from me. I'll look where we can improve the ABI. > > Android has existing clients of the current ABI. Thankfully they're all > contained in system services like SurfaceFlinger, since end-user apps don't > get direct access to fence fds. > > As long the ABI breaks don't remove functionality we depend on, we can wrap > around them in our userspace libsync. I'd rather not have to do that, but > it's a price I'm willing to pay to get this moved out of staging. > > >> - struct sync_file_info_data::fence_info is of type __u8 yet it is "a > >>fence_info struct for every fence in the sync_file". Thus shouldn't > >>one use "struct fence_info" as the type ? > > > >Agreed. But I'm currently thinking if we really should keep this ioctl. > > > > Gustavo > > > > I'm not seeing any consumers of driver_data in our tree. OTOH completely > getting rid of the ioctl would be a problem, since SurfaceFlinger depends on > the timestamp information for its own bookkeeping. If we remove driver_data (and len is superflous too), then I think we should also make the master struct use common ioctl pattern: - Add a num_fences field or similar that the kernel fills out. - Make pt_info an __u64 pointer instead of a variable-length array (and length) - ioctl payload sizes are somewhat limited. This way the interface is future-proofed for truly patalogical number of fences (which surface flinger won't do, but could happen in server/opencl/media workloads I'd imagine). And I think driver_data really shouldn't be there, it makes things complicated with the array of variable-sized objects, and generic userspace can't really use it - for debug output we already have obj/driver_name per fence point, which I think is good enough. Would that be ok for you from the Android side if Gustavo also provides a patch to update libsync? I don't think the ABI is fundamentally broken, but this light cleanup would be nice. Wrt keeping SYNC_WAIT: I think that's totally fine. Redundant since polling is supported, but not really an issue imo either. If we're totally lazy we could implement SYNC_WAIT internally using poll and shave off a few lines of the implementation. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch