From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH] drm: gem_cma_helper.c: Allow importing of contiguous scatterlists with nents > 1 Date: Fri, 10 Nov 2017 06:26:54 +0200 Message-ID: <2213365.Z1vsgl9bDt@avalon> References: <20171101141419.3180-1-Liviu.Dudau@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [IPv6:2001:4b98:dc2:45:216:3eff:febb:480d]) by gabe.freedesktop.org (Postfix) with ESMTPS id 22B686E9E5 for ; Fri, 10 Nov 2017 04:26:50 +0000 (UTC) In-Reply-To: <20171101141419.3180-1-Liviu.Dudau@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: dri-devel@lists.freedesktop.org Cc: David Airlie , Liviu Dudau , LKML , Mali DP Maintainers , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org SGkgTGl2aXUsCgpUaGFuayB5b3UgZm9yIHRoZSBwYXRjaC4KCk9uIFdlZG5lc2RheSwgMSBOb3Zl bWJlciAyMDE3IDE2OjE0OjE5IEVFVCBMaXZpdSBEdWRhdSB3cm90ZToKPiBkcm1fZ2VtX2NtYV9w cmltZV9pbXBvcnRfc2dfdGFibGUoKSB3aWxsIGZhaWwgaWYgdGhlIG51bWJlciBvZiBlbnRyaWVz Cj4gaW4gdGhlIHNnX3RhYmxlID4gMS4gSG93ZXZlciwgeW91IGNhbiBoYXZlIGEgZGV2aWNlIHRo YXQgdXNlcyBhbiBJT01NVQo+IGVuZ2luZSBhbmQgY2FuIG1hcCBhIGRpc2NvbnRpZ3VvdXMgYnVm ZmVyIHdpdGggbXVsdGlwbGUgZW50cmllcyB0aGF0Cj4gaGF2ZSBjb25zZWN1dGl2ZSBzZ19kbWFf YWRkcmVzc2VzLCBlZmZlY3RpdmVseSBtYWtpbmcgaXQgY29udGlndW91cy4KPiBBbGxvdyBmb3Ig dGhhdCBzY2VuYXJpbyBieSB0ZXN0aW5nIHRoZSBlbnRyaWVzIGluIHRoZSBzZ190YWJsZSBmb3IK PiBjb250aWd1b3VzIGNvdmVyYWdlLgo+IAo+IFJldmlld2VkLWJ5OiBCcmlhbiBTdGFya2V5IDxi cmlhbi5zdGFya2V5QGFybS5jb20+Cj4gU2lnbmVkLW9mZi1ieTogTGl2aXUgRHVkYXUgPGxpdml1 LmR1ZGF1QGFybS5jb20+Cj4gLS0tCj4gCj4gSGksCj4gCj4gVGhpcyBwYXRjaCBpcyB0aGUgb25s eSBjaGFuZ2UgSSBuZWVkIGluIG9yZGVyIHRvIGJlIGFibGUgdG8gdXNlIGV4aXN0aW5nCj4gSU9N TVUgZG9tYWluIGluZnJhc3RydWN0dXJlIHdpdGggdGhlIE1hbGkgRFAgZHJpdmVyLiBJIGhhdmUg dGVzdGVkIHRoZQo+IHBhdGNoIGFuZCBJIGtub3cgaXQgd29ya3MgY29ycmVjdGx5IGZvciBteSBz ZXR1cCwgYnV0IEkgd291bGQgbGlrZSB0byBnZXQKPiBzb21lIGNvbW1lbnRzIG9uIHdoZXRoZXIg SSBhbSBvbiB0aGUgcmlnaHQgcGF0aCBvciBpZiBDTUEgcmVhbGx5IHdhbnRzIHRvCj4gc2VlIGFu IHNnX3RhYmxlIHdpdGggb25seSBvbmUgZW50cnkuCgpDTUEsIGFzIHRoZSBtZW1vcnkgYWxsb2Nh dG9yLCBkb2Vzbid0IGNhcmUgYXMgaXQgZG9lc24ndCBldmVuIHNlZSB0aGUgc2cgCnRhYmxlLiBU aGUgZHJtX2dlbV9jbWFfaGVscGVyIGlzIGJhZGx5IG5hbWVkIGFzIGl0IGRvZXNuJ3QgZGVwZW5k IG9uIENNQSwgaXQgCnNob3VsZCBoYXZlIGJlZW4gY2FsbGVkIGRybV9nZW1fZG1hX2NvbnRpZ19o ZWxwZXIgb3Igc29tZXRoaW5nIHNpbWlsYXIuCgpUaGUgYXNzdW1wdGlvbiBhdCB0aGUgYmFzZSBv ZiB0aGF0IGhlbHBlciBsaWJyYXJ5IGlzIHRoYXQgdGhlIG1lbW9yeSBpcyBETUEgCmNvbnRpZ3Vv dXMuIFlvdXIgcGF0Y2ggZ3VhcmFudGVlcyB0aGF0LCBzbyBpdCBzaG91bGQgYmUgZmluZS4gSSd2 ZSBxdWlja2x5IApjaGVja2VkIHRoZSBkcml2ZXJzIHVzaW5nIGRybV9nZW1fY21hX3ByaW1lX2lt cG9ydF9zZ190YWJsZSBhbmQgbm9uZSBvZiB0aGVtIAp1c2UgY21hX29iai0+c2d0LCBzbyBJIHRo aW5rIHRoZXJlJ3Mgbm8gcmlzayBvZiBicmVha2FnZS4gSG93ZXZlciwgSSB3b3VsZCAKcHJlZmVy IGlmIHlvdSB1cGRhdGVkIHRoZSBkcm1fZ2VtX2NtYV9vYmplY3Qgc3RydWN0dXJlIGRvY3VtZW50 YXRpb24gdG8gCmV4cGxpY2l0bHkgc3RhdGUgdGhhdCB0aGUgc2d0IGNhbiBjb250YWluIG11bHRp cGxlIGVudHJpZXMgYnV0IHRoYXQgdGhvc2UgCmVudHJpZXMgYXJlIGd1YXJhbnRlZWQgdG8gaGF2 ZSBjb250aWd1b3VzIERNQSBhZGRyZXNzZXMuCgpXaXRoIHRoZSBkb2N1bWVudGF0aW9uIHVwZGF0 ZSwKClJldmlld2VkLWJ5OiBMYXVyZW50IFBpbmNoYXJ0IDxsYXVyZW50LnBpbmNoYXJ0QGlkZWFz b25ib2FyZC5jb20+Cgo+ICBkcml2ZXJzL2dwdS9kcm0vZHJtX2dlbV9jbWFfaGVscGVyLmMgfCAy MiArKysrKysrKysrKysrKysrKysrKy0tCj4gIDEgZmlsZSBjaGFuZ2VkLCAyMCBpbnNlcnRpb25z KCspLCAyIGRlbGV0aW9ucygtKQo+IAo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vZHJt X2dlbV9jbWFfaGVscGVyLmMKPiBiL2RyaXZlcnMvZ3B1L2RybS9kcm1fZ2VtX2NtYV9oZWxwZXIu YyBpbmRleCAwMjBlNzY2OGRmYWJhLi40M2IxNzkyMTIwNTJkCj4gMTAwNjQ0Cj4gLS0tIGEvZHJp dmVycy9ncHUvZHJtL2RybV9nZW1fY21hX2hlbHBlci5jCj4gKysrIGIvZHJpdmVycy9ncHUvZHJt L2RybV9nZW1fY21hX2hlbHBlci5jCj4gQEAgLTQ4Miw4ICs0ODIsMjYgQEAgZHJtX2dlbV9jbWFf cHJpbWVfaW1wb3J0X3NnX3RhYmxlKHN0cnVjdCBkcm1fZGV2aWNlCj4gKmRldiwgewo+ICAJc3Ry dWN0IGRybV9nZW1fY21hX29iamVjdCAqY21hX29iajsKPiAKPiAtCWlmIChzZ3QtPm5lbnRzICE9 IDEpCj4gLQkJcmV0dXJuIEVSUl9QVFIoLUVJTlZBTCk7Cj4gKwlpZiAoc2d0LT5uZW50cyAhPSAx KSB7Cj4gKwkJLyogY2hlY2sgaWYgdGhlIGVudHJpZXMgaW4gdGhlIHNnX3RhYmxlIGFyZSBjb250 aWd1b3VzICovCj4gKwkJZG1hX2FkZHJfdCBuZXh0X2FkZHIgPSBzZ19kbWFfYWRkcmVzcyhzZ3Qt PnNnbCk7Cj4gKwkJc3RydWN0IHNjYXR0ZXJsaXN0ICpzOwo+ICsJCXVuc2lnbmVkIGludCBpOwo+ ICsKPiArCQlmb3JfZWFjaF9zZyhzZ3QtPnNnbCwgcywgc2d0LT5uZW50cywgaSkgewo+ICsJCQkv Kgo+ICsJCQkgKiBzZ19kbWFfYWRkcmVzcyhzKSBpcyBvbmx5IHZhbGlkIGZvciBlbnRyaWVzCj4g KwkJCSAqIHRoYXQgaGF2ZSBzZ19kbWFfbGVuKHMpICE9IDAKPiArCQkJICovCj4gKwkJCWlmICgh c2dfZG1hX2xlbihzKSkKPiArCQkJCWNvbnRpbnVlOwo+ICsKPiArCQkJaWYgKHNnX2RtYV9hZGRy ZXNzKHMpICE9IG5leHRfYWRkcikKPiArCQkJCXJldHVybiBFUlJfUFRSKC1FSU5WQUwpOwo+ICsK PiArCQkJbmV4dF9hZGRyID0gc2dfZG1hX2FkZHJlc3MocykgKyBzZ19kbWFfbGVuKHMpOwo+ICsJ CX0KPiArCX0KPiAKPiAgCS8qIENyZWF0ZSBhIENNQSBHRU0gYnVmZmVyLiAqLwo+ICAJY21hX29i aiA9IF9fZHJtX2dlbV9jbWFfY3JlYXRlKGRldiwgYXR0YWNoLT5kbWFidWYtPnNpemUpOwoKLS0g ClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0 cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9s aXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755906AbdKJE0u (ORCPT ); Thu, 9 Nov 2017 23:26:50 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:34373 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755232AbdKJE0t (ORCPT ); Thu, 9 Nov 2017 23:26:49 -0500 From: Laurent Pinchart To: dri-devel@lists.freedesktop.org Cc: Liviu Dudau , Brian Starkey , David Airlie , Liviu Dudau , LKML , Mali DP Maintainers , Daniel Vetter Subject: Re: [PATCH] drm: gem_cma_helper.c: Allow importing of contiguous scatterlists with nents > 1 Date: Fri, 10 Nov 2017 06:26:54 +0200 Message-ID: <2213365.Z1vsgl9bDt@avalon> Organization: Ideas on Board Oy In-Reply-To: <20171101141419.3180-1-Liviu.Dudau@arm.com> References: <20171101141419.3180-1-Liviu.Dudau@arm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Liviu, Thank you for the patch. On Wednesday, 1 November 2017 16:14:19 EET Liviu Dudau wrote: > drm_gem_cma_prime_import_sg_table() will fail if the number of entries > in the sg_table > 1. However, you can have a device that uses an IOMMU > engine and can map a discontiguous buffer with multiple entries that > have consecutive sg_dma_addresses, effectively making it contiguous. > Allow for that scenario by testing the entries in the sg_table for > contiguous coverage. > > Reviewed-by: Brian Starkey > Signed-off-by: Liviu Dudau > --- > > Hi, > > This patch is the only change I need in order to be able to use existing > IOMMU domain infrastructure with the Mali DP driver. I have tested the > patch and I know it works correctly for my setup, but I would like to get > some comments on whether I am on the right path or if CMA really wants to > see an sg_table with only one entry. CMA, as the memory allocator, doesn't care as it doesn't even see the sg table. The drm_gem_cma_helper is badly named as it doesn't depend on CMA, it should have been called drm_gem_dma_contig_helper or something similar. The assumption at the base of that helper library is that the memory is DMA contiguous. Your patch guarantees that, so it should be fine. I've quickly checked the drivers using drm_gem_cma_prime_import_sg_table and none of them use cma_obj->sgt, so I think there's no risk of breakage. However, I would prefer if you updated the drm_gem_cma_object structure documentation to explicitly state that the sgt can contain multiple entries but that those entries are guaranteed to have contiguous DMA addresses. With the documentation update, Reviewed-by: Laurent Pinchart > drivers/gpu/drm/drm_gem_cma_helper.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c > b/drivers/gpu/drm/drm_gem_cma_helper.c index 020e7668dfaba..43b179212052d > 100644 > --- a/drivers/gpu/drm/drm_gem_cma_helper.c > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c > @@ -482,8 +482,26 @@ drm_gem_cma_prime_import_sg_table(struct drm_device > *dev, { > struct drm_gem_cma_object *cma_obj; > > - if (sgt->nents != 1) > - return ERR_PTR(-EINVAL); > + if (sgt->nents != 1) { > + /* check if the entries in the sg_table are contiguous */ > + dma_addr_t next_addr = sg_dma_address(sgt->sgl); > + struct scatterlist *s; > + unsigned int i; > + > + for_each_sg(sgt->sgl, s, sgt->nents, i) { > + /* > + * sg_dma_address(s) is only valid for entries > + * that have sg_dma_len(s) != 0 > + */ > + if (!sg_dma_len(s)) > + continue; > + > + if (sg_dma_address(s) != next_addr) > + return ERR_PTR(-EINVAL); > + > + next_addr = sg_dma_address(s) + sg_dma_len(s); > + } > + } > > /* Create a CMA GEM buffer. */ > cma_obj = __drm_gem_cma_create(dev, attach->dmabuf->size); -- Regards, Laurent Pinchart