Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759360AbcJYQRD (ORCPT ); Tue, 25 Oct 2016 12:17:03 -0400 Received: from anholt.net ([50.246.234.109]:56095 "EHLO anholt.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759277AbcJYQRB (ORCPT ); Tue, 25 Oct 2016 12:17:01 -0400 From: Eric Anholt To: Michael Zoran , gregkh@linuxfoundation.org Cc: daniels@collabora.com, noralf@tronnes.org, popcornmix@gmail.com, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] staging: vc04_services: Replace dmac_map_area with dmac_map_sg In-Reply-To: <1477408034.4618.5.camel@crowfest.net> References: <20161024052932.28259-1-mzoran@crowfest.net> <878ttdslfb.fsf@eliezer.anholt.net> <1477407643.4618.3.camel@crowfest.net> <1477408034.4618.5.camel@crowfest.net> User-Agent: Notmuch/0.22.2+1~gb0bcfaa (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Tue, 25 Oct 2016 09:16:51 -0700 Message-ID: <87lgxco13g.fsf@eliezer.anholt.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9990 Lines: 261 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Michael Zoran writes: > On Tue, 2016-10-25 at 08:00 -0700, Michael Zoran wrote: >> On Mon, 2016-10-24 at 10:31 -0700, Eric Anholt wrote: >> > mzoran@crowfest.net writes: >> >=20 >> > > =C2=A0*/ >> > > =C2=A0 >> > > =C2=A0static int >> > > =C2=A0create_pagelist(char __user *buf, size_t count, unsigned short >> > > type, >> > > - struct task_struct *task, PAGELIST_T ** ppagelist) >> > > + struct task_struct *task, PAGELIST_T >> > > **ppagelist, >> > > + dma_addr_t *dma_addr) >> > > =C2=A0{ >> > > =C2=A0 PAGELIST_T *pagelist; >> > > =C2=A0 struct page **pages; >> > > - unsigned long *addrs; >> > > - unsigned int num_pages, offset, i; >> > > - char *addr, *base_addr, *next_addr; >> > > - int run, addridx, actual_pages; >> > > + u32 *addrs; >> > > + unsigned int num_pages, offset, i, j, k; >> > > + int actual_pages; >> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned long = *need_release; >> > > + size_t pagelist_size; >> > > + struct scatterlist *scatterlist; >> > > + int dma_buffers; >> > > + int dir; >> > > =C2=A0 >> > > - offset =3D (unsigned int)buf & (PAGE_SIZE - 1); >> > > + offset =3D ((unsigned int)(unsigned long)buf & (PAGE_SIZE >> > > - >> > > 1)); >> > > =C2=A0 num_pages =3D (count + offset + PAGE_SIZE - 1) / >> > > PAGE_SIZE; >> > > =C2=A0 >> > > + pagelist_size =3D sizeof(PAGELIST_T) + >> > > + (num_pages * sizeof(unsigned long)) + >> > > + sizeof(unsigned long) + >> > > + (num_pages * sizeof(pages[0]) + >> > > + (num_pages * sizeof(struct >> > > scatterlist))); >> > > + >> > > =C2=A0 *ppagelist =3D NULL; >> > > =C2=A0 >> > > + dir =3D (type =3D=3D PAGELIST_WRITE) ? DMA_TO_DEVICE : >> > > DMA_FROM_DEVICE; >> > > + >> > > =C2=A0 /* Allocate enough storage to hold the page pointers and >> > > the page >> > > =C2=A0 ** list >> > > =C2=A0 */ >> > > - pagelist =3D kmalloc(sizeof(PAGELIST_T) + >> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0(num_pages * sizeof(unsigned long)) + >> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0sizeof(unsigned long) + >> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0(num_pages * sizeof(pages[0])), >> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0GFP_KERNEL); >> > > + pagelist =3D dma_zalloc_coherent(g_dev, >> > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pagelist_size, >> > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0dma_addr, >> > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0GFP_KERNEL); >> > > =C2=A0 >> > > =C2=A0 vchiq_log_trace(vchiq_arm_log_level, "create_pagelist - >> > > %pK", >> > > =C2=A0 pagelist); >> > > @@ -394,10 +399,9 @@ create_pagelist(char __user *buf, size_t >> > > count, unsigned short type, >> > > =C2=A0 addrs =3D pagelist->addrs; >> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0need_release = =3D (unsigned long *)(addrs + num_pages); >> > > =C2=A0 pages =3D (struct page **)(addrs + num_pages + 1); >> > > + scatterlist =3D (struct scatterlist *)(pages + num_pages); >> > > =C2=A0 >> > > =C2=A0 if (is_vmalloc_addr(buf)) { >> > > - int dir =3D (type =3D=3D PAGELIST_WRITE) ? >> > > - DMA_TO_DEVICE : DMA_FROM_DEVICE; >> > > =C2=A0 unsigned long length =3D count; >> > > =C2=A0 unsigned int off =3D offset; >> > > =C2=A0 >> > > @@ -410,7 +414,6 @@ create_pagelist(char __user *buf, size_t >> > > count, >> > > unsigned short type, >> > > =C2=A0 if (bytes > length) >> > > =C2=A0 bytes =3D length; >> > > =C2=A0 pages[actual_pages] =3D pg; >> > > - dmac_map_area(page_address(pg) + off, >> > > bytes, dir); >> > > =C2=A0 length -=3D bytes; >> > > =C2=A0 off =3D 0; >> > > =C2=A0 } >> > > @@ -438,7 +441,8 @@ create_pagelist(char __user *buf, size_t >> > > count, >> > > unsigned short type, >> > > =C2=A0 actual_pages--; >> > > =C2=A0 put_page(pages[actual_pages]); >> > > =C2=A0 } >> > > - kfree(pagelist); >> > > + dma_free_coherent(g_dev, pagelist_size, >> > > + =C2=A0=C2=A0pagelist, *dma_addr); >> > > =C2=A0 if (actual_pages =3D=3D 0) >> > > =C2=A0 actual_pages =3D -ENOMEM; >> > > =C2=A0 return actual_pages; >> > > @@ -450,29 +454,38 @@ create_pagelist(char __user *buf, size_t >> > > count, unsigned short type, >> > > =C2=A0 pagelist->type =3D type; >> > > =C2=A0 pagelist->offset =3D offset; >> > > =C2=A0 >> > > - /* Group the pages into runs of contiguous pages */ >> > > - >> > > - base_addr =3D VCHIQ_ARM_ADDRESS(page_address(pages[0])); >> > > - next_addr =3D base_addr + PAGE_SIZE; >> > > - addridx =3D 0; >> > > - run =3D 0; >> > > - >> > > - for (i =3D 1; i < num_pages; i++) { >> > > - addr =3D >> > > VCHIQ_ARM_ADDRESS(page_address(pages[i])); >> > > - if ((addr =3D=3D next_addr) && (run < (PAGE_SIZE - >> > > 1))) { >> > > - next_addr +=3D PAGE_SIZE; >> > > - run++; >> > > - } else { >> > > - addrs[addridx] =3D (unsigned >> > > long)base_addr >> > > + run; >> > > - addridx++; >> > > - base_addr =3D addr; >> > > - next_addr =3D addr + PAGE_SIZE; >> > > - run =3D 0; >> > > - } >> > > + for (i =3D 0; i < num_pages; i++) >> > > + sg_set_page(scatterlist + i, pages[i], >> > > PAGE_SIZE, >> > > 0); >> > > + >> > > + dma_buffers =3D dma_map_sg(g_dev, >> > > + =C2=A0scatterlist, >> > > + =C2=A0num_pages, >> > > + =C2=A0dir); >> > > + >> > > + if (dma_buffers =3D=3D 0) { >> > > + dma_free_coherent(g_dev, pagelist_size, >> > > + =C2=A0=C2=A0pagelist, *dma_addr); >> > > + >> > > + return -EINTR; >> > > =C2=A0 } >> > > =C2=A0 >> > > - addrs[addridx] =3D (unsigned long)base_addr + run; >> > > - addridx++; >> > > + k =3D 0; >> > > + for (i =3D 0; i < dma_buffers;) { >> > > + u32 section_length =3D 0; >> > > + >> > > + for (j =3D i + 1; j < dma_buffers; j++) { >> > > + if (sg_dma_address(scatterlist + j) !=3D >> > > + =C2=A0=C2=A0=C2=A0=C2=A0sg_dma_address(scatterlist + j - 1) >> > > + >> > > PAGE_SIZE) { >> > > + break; >> > > + } >> > > + section_length++; >> > > + } >> > > + >> > > + addrs[k] =3D (u32)sg_dma_address(scatterlist + i) >> > > | >> > > + section_length; >> >=20 >> > It looks like scatterlist may not be just an array, so I think this >> > whole loop wants to be something like: >> >=20 >> > for_each_sg(scatterlist, sg, num_pages, i) { >> > u32 len =3D sg_dma_len(sg); >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0u32 addr =3D sg_dma_ad= dress(sg); >> >=20 >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (k > 0 && addrs[k -= 1] & PAGE_MASK + >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0(addrs[k - 1] & ~PAGE_MASK) << PAGE_SHIFT) =3D=3D addr) { >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 addrs[k - 1] +=3D len; >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else { >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 addrs[k++] =3D addr |= len; >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} >> > } >> >=20 >> > Note the use of sg_dma_len(), since sg entries may be more than one >> > page.=C2=A0=C2=A0I don't think any merging is actually happening on our >> > platform >> > currently, thus why I left the inner loop. >> >=20 >> > Thanks for taking on doing this conversion!=C2=A0=C2=A0This code's goi= ng to >> > be >> > so >> > much nicer when you're done. >>=20 >> Thanks for looking at this. =C2=A0 >>=20 >> While I understand the use of sg_dma_len, I don't understand totally >> the need for "for_each_sg". The scatterlist can be part of a scatter >> table with all kinds of complex chaining, but in this case it is >> directly allocated as an array at the top of the function. >>=20 >> Also note that the addrs[] list is passed to the firmware and it >> requires all the items of the list be paged aligned and be a multiple >> of the page size.=C2=A0=C2=A0So I'm also a bit confused about the need f= or >> handling scatterlists that are not page aligned or a multiple of >> pages. I'm sure we can assume that sg_dma_map is going to give us back page-aligned mappings, because we only asked for page aligned mappings. I'm just making suggestions that follow what is describeed in DMA-API-HOWTO.txt here. Following the documentation seems like a good idea unless there's a reason not to. > Sorry, but I forgot to add that the addrs list is a address anded with > a page count, not a byte count. The example you have sent appears at > a glance to look like it is anding a byte count with the address. Looking at the type of the field being read, it looked like a page count to me, but I was unsure and the header didn't clarify. Looking again, it must be bytes, so shifting would be necessary. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCgAGBQJYD4VzAAoJELXWKTbR/J7okR4P/2yzg155q8EZrWc+V5wvGdYa M5W+aiUGVOMHOE5+gY20qoXif6jXQ+/t8kP9bNbOkKt4qTrTWRzt6LxgTVawb8pE LUotQPZV6SGyngv6pK9OXJS/ORg0sLnXJWjd9qPdOWXVR4ZAYrzOALrVKi5wc2e2 o45QzORBzixP6/vikm8X+KgH9G4pMy07qa4LS0DpwtKNgxwo4misQ6KST4NdRUbK eigUgMO8sDTrgRGG2ThvOmPAkkUlDm3kAYMeAoyvxmJYQ7pIN/M1LVTk8KMc30r2 YFFbpyXkRjP7nT99OiGUzq+F1Hi5CcF7BVvXKc+O4Y4lHD1pOaB4zUHT7815K3Xp G3V4UQRfwDd2Zt/nN0HIqNsR5/BYn3P+pmZMqkAycjFGw5Dt5ZAr+JseQL6DSUI+ YfedEJJ2NiMnTRXMleDzuhLLI7PwiDB4g+j2Wb37DufeL2LdzTSJo/5XgxyzFtbr jyeYabwos5CNw/4vEMEsVoyzK63jW9w/7WokjwVoqfEdpTHtImt37i/sPuzSeHV6 Er+Z1pjeEFJpciRV2zFGVrHaaA6H2kpC485ZkEAB0JgZcBD2r6GoSoffFIkLw7/q p+iYSt3avxFtr+Fv3GkSeEl4idvoutjX+kTQWpSYoUz7R3OQb6TC3yPVlallqyeI zAJIXwWWX2gy+qvHdz6r =gSDW -----END PGP SIGNATURE----- --=-=-=--