From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: [PATCH v3 06/11] staging/android: turn fence_info into a __u64 pointer Date: Thu, 4 Feb 2016 10:57:15 +0100 Message-ID: <56B3207B.50100@linux.intel.com> References: <1454505940-18094-1-git-send-email-gustavo@padovan.org> <1454505940-18094-7-git-send-email-gustavo@padovan.org> <56B2111A.8080103@linux.intel.com> <20160203200957.GG2808@joana> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id C40386E84F for ; Thu, 4 Feb 2016 01:57:19 -0800 (PST) In-Reply-To: <20160203200957.GG2808@joana> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Gustavo Padovan , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, dri-devel@lists.freedesktop.org, Daniel Stone , =?UTF-8?Q?Arve_Hj=c3=b8nnev=c3=a5g?= , Riley Andrews , Daniel Vetter , Rob Clark , Greg Hackmann , John Harrison , Gustavo Padovan List-Id: dri-devel@lists.freedesktop.org T3AgMDMtMDItMTYgb20gMjE6MDkgc2NocmVlZiBHdXN0YXZvIFBhZG92YW46Cj4gSGkgTWFhcnRl biwKPgo+IDIwMTYtMDItMDMgTWFhcnRlbiBMYW5raG9yc3QgPG1hYXJ0ZW4ubGFua2hvcnN0QGxp bnV4LmludGVsLmNvbT46Cj4KPj4gT3AgMDMtMDItMTYgb20gMTQ6MjUgc2NocmVlZiBHdXN0YXZv IFBhZG92YW46Cj4+PiBGcm9tOiBHdXN0YXZvIFBhZG92YW4gPGd1c3Rhdm8ucGFkb3ZhbkBjb2xs YWJvcmEuY28udWs+Cj4+Pgo+Pj4gVHVybiBzeW5jX2ZlbmNlX2luZm8gaW50byBfX3U2NCB0eXBl IGVuYWJsZSB1cyB0byBleHRlbmQgdGhlIHN0cnVjdCBpbiB0aGUKPj4+IGZ1dHVyZSB3aXRob3V0 IGJyZWFraW5nIHRoZSBBQkkuCj4+Pgo+Pj4gdjI6IHVzZSB0eXBlIF9fdTY0IGZvciBmZW5jZV9p bmZvCj4+Pgo+Pj4gdjM6IGZpeCBjb21taXQgbWVzc2FnZSB0byByZWZsZWN0IHRoZSB2MiBjaGFu Z2UKPj4+Cj4+PiBTaWduZWQtb2ZmLWJ5OiBHdXN0YXZvIFBhZG92YW4gPGd1c3Rhdm8ucGFkb3Zh bkBjb2xsYWJvcmEuY28udWs+Cj4+PiAtLS0KPj4+ICBkcml2ZXJzL3N0YWdpbmcvYW5kcm9pZC9z eW5jLmMgICAgICB8IDIgKy0KPj4+ICBkcml2ZXJzL3N0YWdpbmcvYW5kcm9pZC91YXBpL3N5bmMu aCB8IDIgKy0KPj4+ICAyIGZpbGVzIGNoYW5nZWQsIDIgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlv bnMoLSkKPj4+Cj4+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9zdGFnaW5nL2FuZHJvaWQvc3luYy5j IGIvZHJpdmVycy9zdGFnaW5nL2FuZHJvaWQvc3luYy5jCj4+PiBpbmRleCAyYWIwYzIwLi44NDI1 NDU3IDEwMDY0NAo+Pj4gLS0tIGEvZHJpdmVycy9zdGFnaW5nL2FuZHJvaWQvc3luYy5jCj4+PiAr KysgYi9kcml2ZXJzL3N0YWdpbmcvYW5kcm9pZC9zeW5jLmMKPj4+IEBAIC01MjUsNyArNTI1LDcg QEAgc3RhdGljIGxvbmcgc3luY19maWxlX2lvY3RsX2ZlbmNlX2luZm8oc3RydWN0IHN5bmNfZmls ZSAqc3luY19maWxlLAo+Pj4gIAlpZiAoaW5mby0+c3RhdHVzID49IDApCj4+PiAgCQlpbmZvLT5z dGF0dXMgPSAhaW5mby0+c3RhdHVzOwo+Pj4gIAo+Pj4gLQlsZW4gPSBzaXplb2Yoc3RydWN0IHN5 bmNfZmlsZV9pbmZvKTsKPj4+ICsJbGVuID0gc2l6ZW9mKHN0cnVjdCBzeW5jX2ZpbGVfaW5mbykg LSBzaXplb2YoX191NjQpOwo+Pj4gIAo+Pj4gIAlmb3IgKGkgPSAwOyBpIDwgc3luY19maWxlLT5u dW1fZmVuY2VzOyArK2kpIHsKPj4+ICAJCXN0cnVjdCBmZW5jZSAqZmVuY2UgPSBzeW5jX2ZpbGUt PmNic1tpXS5mZW5jZTsKPj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3N0YWdpbmcvYW5kcm9pZC91 YXBpL3N5bmMuaCBiL2RyaXZlcnMvc3RhZ2luZy9hbmRyb2lkL3VhcGkvc3luYy5oCj4+PiBpbmRl eCBhMGNmMzU3Li5lNjQ5OTUzIDEwMDY0NAo+Pj4gLS0tIGEvZHJpdmVycy9zdGFnaW5nL2FuZHJv aWQvdWFwaS9zeW5jLmgKPj4+ICsrKyBiL2RyaXZlcnMvc3RhZ2luZy9hbmRyb2lkL3VhcGkvc3lu Yy5oCj4+PiBAQCAtNTQsNyArNTQsNyBAQCBzdHJ1Y3Qgc3luY19maWxlX2luZm8gewo+Pj4gIAlj aGFyCW5hbWVbMzJdOwo+Pj4gIAlfX3MzMglzdGF0dXM7Cj4+PiAgCj4+PiAtCV9fdTgJc3luY19m ZW5jZV9pbmZvWzBdOwo+Pj4gKwlfX3U2NAlzeW5jX2ZlbmNlX2luZm87Cj4+PiAgfTsKPj4+ICAK Pj4+ICAjZGVmaW5lIFNZTkNfSU9DX01BR0lDCQknPicKPj4gVGhpcyBzdGlsbCBkb2Vzbid0IGRv IHdoYXQgeW91IGV4cGVjdCBpdCB0by4KPj4KPj4gSSB0aGluayB0aGlzIGlzIHdoYXQgeW91IHdh bnQgaXMgZm9yIHVzZXJzcGFjZSB0byBkbzoKPj4KPj4gc3RydWN0IHN5bmNfZmlsZV9pbmZvIGlu Zm87Cj4+Cj4+IGluZm8uZmxhZ3MgPSBpbmZvLm51bV9mZW5jZXMgPSAwOwo+PiBpb2N0bChmZCwg U1lOQ19JT0NfRkVOQ0VfSU5GTywgJmluZm8pOwo+PiBpZiAoaW5mby5udW1fZmVuY2VzKSB7Cj4+ IGluZm8uc3luY19mZW5jZV9pbmZvID0gKHVpbnRwdHIpa2NhbGxvYyhpbmZvLm51bV9mZW5jZXMs IHNpemVvZihzdHJ1Y3Qgc3luY19mZW5jZV9pbmZvKSk7Cj4+IGlvY3RsKGZkLCBTWU5DX0lPQ19G RU5DRV9JTkZPLCAmaW5mbyk7Cj4+IH0KPj4KPj4gTWF5YmUgdXNlcnNwYWNlIGNvdWxkIHByZWFs bG9jYXRlIHRoZSBtYXggaW4gYWR2YW5jZSBhbmQgc2V0IG51bV9mZW5jZXMgaGlnaGVyLAo+Pgo+ PiBrZXJuZWwgd291bGQgZG8gc29tZXRoaW5nIGxpa2U6Cj4+Cj4+IG51bV9mZW5jZXMgPSBtaW4o aW5mby5udW1fZmVuY2VzLCBzeW5jLT5udW1fZmVuY2VzKTsKPj4gc3RydWN0IHN5bmNfZmVuY2Vf aW5mbyBhcnJheVtudW1fZmVuY2VzXTsKPj4KPj4gaW5mby5udW1fZmVuY2VzID0gc3luYy0+bnVt X2ZlbmNlczsKPj4gaWYgKG51bV9mZW5jZXMgJiYKPj4gICAgIGNvcHlfdG9fdXNlcigodm9pZCAq IF9fdXNlcikodW5zaWduZWQgbG9uZylpbmZvLnN5bmNfZmVuY2VfaW5mbywgYXJyYXksIG51bV9m ZW5jZXMgICogc2l6ZW9mKGFycmF5KSkpCj4+ICByZXR1cm4gLUVGQVVMVDsKPiBJZiB3ZSBhcmUg Z29pbmcgdG8gY2FsbCBJT0NUTCB0d2ljZSBJIHdvdWxkIGFjdHVhbGx5IGhhdmUgYSBuZXcgSU9D VEwgb25seQo+IHRvIGZldGNoIHN5bmNfZmVuY2VfaW5mby4KPgo+IEZpcnN0IHdlIHdvdWxkIGNh bGwKPgo+IGlvY3RsKGZkLCBTWU5DX0lPQ19GSUxFX0lORk8sICZpbmZvKTsKPgo+IHdoZXJlIGlu Zm8gaXM6Cj4KPiBzdHJ1Y3Qgc3luY19maWxlX2luZm8geyAgICAKPiAgICAgICAgIGNoYXIgICAg bmFtZVszMl07ICAKPiAgICAgICAgIF9fczMyICAgc3RhdHVzOyAgICAKPiAgICAgICAgIF9fdTMy ICAgZmxhZ3M7ICAgICAKPiAgICAgICAgIF9fdTMyICAgbnVtX2ZlbmNlczsKPiB9Owo+Cj4gdGhl biB3ZSB3b3VsZCBhbGxvY2F0ZSBhIGJ1ZmZlciB3aXRoCj4KPiBzaXplID0gaW5mby5udW1fZmVu Y2VzICogc2l6ZW9mKHN0cnVjdCBzeW5jX2ZlbmNlX2luZm8pCj4KPiBhbmQgY2FsbCB0aGUgbmV3 IGlvY3RsCj4KPiBpb2N0bChmZCwgU1lOQ19JT0NfU1lOQ19GRU5DRV9JTkZPLCBzeW5jX2ZlbmNl X2luZm8pOwo+Cj4gVGhpcyBsb29rcyBsaWtlIGEgY2xlYW5lciBzb2x1dGlvbiBhbmQgZG9lc24n dCBicmVhayBBQkkuIFdoYXQgZG8geW91Cj4gdGhpbms/CkkgdGhpbmsgaXQncyBnb29kIHRhc3Rl IHRoYXQgdXNlcnNwYWNlIHNwZWNpZmllcyB0aGUgc2l6ZSBvZiB0aGUgYnVmZmVyIGl0IHBhc3Nl cywgc28gZm9ybWVyIGZlZWxzIG1vcmUgY2xlYW4gdG8gbWUsCnNpbmNlIHlvdSBuZWVkIHRvIHBh c3MgbnVtX2ZlbmNlcyBhbnl3YXkuCkJ1dCBEYW5pZWwga25vd3MgbW9yZSBhYm91dCBkZXNpZ25p bmcgaW9jdGwncyB0aGFuIEkgZG8sIHNvIGZvciBleGFjdCBiZWhhdmlvciBpdCdzIGJlc3QgdG8g YXNrIGhpbS4KCn5NYWFydGVuCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNr dG9wLm9yZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJp LWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755740AbcBDJ5e (ORCPT ); Thu, 4 Feb 2016 04:57:34 -0500 Received: from mga04.intel.com ([192.55.52.120]:14280 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754580AbcBDJ5T (ORCPT ); Thu, 4 Feb 2016 04:57:19 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,395,1449561600"; d="scan'208";a="896171952" Subject: Re: [PATCH v3 06/11] staging/android: turn fence_info into a __u64 pointer To: Gustavo Padovan , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, dri-devel@lists.freedesktop.org, Daniel Stone , =?UTF-8?Q?Arve_Hj=c3=b8nnev=c3=a5g?= , Riley Andrews , Daniel Vetter , Rob Clark , Greg Hackmann , John Harrison , Gustavo Padovan References: <1454505940-18094-1-git-send-email-gustavo@padovan.org> <1454505940-18094-7-git-send-email-gustavo@padovan.org> <56B2111A.8080103@linux.intel.com> <20160203200957.GG2808@joana> From: Maarten Lankhorst Message-ID: <56B3207B.50100@linux.intel.com> Date: Thu, 4 Feb 2016 10:57:15 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20160203200957.GG2808@joana> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Op 03-02-16 om 21:09 schreef Gustavo Padovan: > Hi Maarten, > > 2016-02-03 Maarten Lankhorst : > >> Op 03-02-16 om 14:25 schreef Gustavo Padovan: >>> From: Gustavo Padovan >>> >>> Turn sync_fence_info into __u64 type enable us to extend the struct in the >>> future without breaking the ABI. >>> >>> v2: use type __u64 for fence_info >>> >>> v3: fix commit message to reflect the v2 change >>> >>> Signed-off-by: Gustavo Padovan >>> --- >>> drivers/staging/android/sync.c | 2 +- >>> drivers/staging/android/uapi/sync.h | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c >>> index 2ab0c20..8425457 100644 >>> --- a/drivers/staging/android/sync.c >>> +++ b/drivers/staging/android/sync.c >>> @@ -525,7 +525,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, >>> if (info->status >= 0) >>> info->status = !info->status; >>> >>> - len = sizeof(struct sync_file_info); >>> + len = sizeof(struct sync_file_info) - sizeof(__u64); >>> >>> for (i = 0; i < sync_file->num_fences; ++i) { >>> struct fence *fence = sync_file->cbs[i].fence; >>> diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h >>> index a0cf357..e649953 100644 >>> --- a/drivers/staging/android/uapi/sync.h >>> +++ b/drivers/staging/android/uapi/sync.h >>> @@ -54,7 +54,7 @@ struct sync_file_info { >>> char name[32]; >>> __s32 status; >>> >>> - __u8 sync_fence_info[0]; >>> + __u64 sync_fence_info; >>> }; >>> >>> #define SYNC_IOC_MAGIC '>' >> This still doesn't do what you expect it to. >> >> I think this is what you want is for userspace to do: >> >> struct sync_file_info info; >> >> info.flags = info.num_fences = 0; >> ioctl(fd, SYNC_IOC_FENCE_INFO, &info); >> if (info.num_fences) { >> info.sync_fence_info = (uintptr)kcalloc(info.num_fences, sizeof(struct sync_fence_info)); >> ioctl(fd, SYNC_IOC_FENCE_INFO, &info); >> } >> >> Maybe userspace could preallocate the max in advance and set num_fences higher, >> >> kernel would do something like: >> >> num_fences = min(info.num_fences, sync->num_fences); >> struct sync_fence_info array[num_fences]; >> >> info.num_fences = sync->num_fences; >> if (num_fences && >> copy_to_user((void * __user)(unsigned long)info.sync_fence_info, array, num_fences * sizeof(array))) >> return -EFAULT; > If we are going to call IOCTL twice I would actually have a new IOCTL only > to fetch sync_fence_info. > > First we would call > > ioctl(fd, SYNC_IOC_FILE_INFO, &info); > > where info is: > > struct sync_file_info { > char name[32]; > __s32 status; > __u32 flags; > __u32 num_fences; > }; > > then we would allocate a buffer with > > size = info.num_fences * sizeof(struct sync_fence_info) > > and call the new ioctl > > ioctl(fd, SYNC_IOC_SYNC_FENCE_INFO, sync_fence_info); > > This looks like a cleaner solution and doesn't break ABI. What do you > think? I think it's good taste that userspace specifies the size of the buffer it passes, so former feels more clean to me, since you need to pass num_fences anyway. But Daniel knows more about designing ioctl's than I do, so for exact behavior it's best to ask him. ~Maarten