Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941814AbcJYPHV (ORCPT ); Tue, 25 Oct 2016 11:07:21 -0400 Received: from gw.crowfest.net ([52.42.241.221]:48140 "EHLO gw.crowfest.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S941820AbcJYPHP (ORCPT ); Tue, 25 Oct 2016 11:07:15 -0400 X-Greylist: delayed 390 seconds by postgrey-1.27 at vger.kernel.org; Tue, 25 Oct 2016 11:07:15 EDT Message-ID: <1477408034.4618.5.camel@crowfest.net> Subject: Re: [PATCH 2/2] staging: vc04_services: Replace dmac_map_area with dmac_map_sg From: Michael Zoran To: Eric Anholt , gregkh@linuxfoundation.org Cc: daniels@collabora.com, noralf@tronnes.org, popcornmix@gmail.com, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Date: Tue, 25 Oct 2016 08:07:14 -0700 In-Reply-To: <1477407643.4618.3.camel@crowfest.net> References: <20161024052932.28259-1-mzoran@crowfest.net> <878ttdslfb.fsf@eliezer.anholt.net> <1477407643.4618.3.camel@crowfest.net> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7028 Lines: 209 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: > > > > >  */ > > >   > > >  static int > > >  create_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) > > >  { > > >   PAGELIST_T *pagelist; > > >   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; > > >          unsigned long *need_release; > > > + size_t pagelist_size; > > > + struct scatterlist *scatterlist; > > > + int dma_buffers; > > > + int dir; > > >   > > > - offset = (unsigned int)buf & (PAGE_SIZE - 1); > > > + offset = ((unsigned int)(unsigned long)buf & (PAGE_SIZE > > > - > > > 1)); > > >   num_pages = (count + offset + PAGE_SIZE - 1) / > > > PAGE_SIZE; > > >   > > > + pagelist_size = sizeof(PAGELIST_T) + > > > + (num_pages * sizeof(unsigned long)) + > > > + sizeof(unsigned long) + > > > + (num_pages * sizeof(pages[0]) + > > > + (num_pages * sizeof(struct > > > scatterlist))); > > > + > > >   *ppagelist = NULL; > > >   > > > + dir = (type == PAGELIST_WRITE) ? DMA_TO_DEVICE : > > > DMA_FROM_DEVICE; > > > + > > >   /* Allocate enough storage to hold the page pointers and > > > the page > > >   ** list > > >   */ > > > - pagelist = kmalloc(sizeof(PAGELIST_T) + > > > -                           (num_pages * sizeof(unsigned long)) + > > > -                           sizeof(unsigned long) + > > > -                           (num_pages * sizeof(pages[0])), > > > -                           GFP_KERNEL); > > > + pagelist = dma_zalloc_coherent(g_dev, > > > +        pagelist_size, > > > +        dma_addr, > > > +        GFP_KERNEL); > > >   > > >   vchiq_log_trace(vchiq_arm_log_level, "create_pagelist - > > > %pK", > > >   pagelist); > > > @@ -394,10 +399,9 @@ create_pagelist(char __user *buf, size_t > > > count, unsigned short type, > > >   addrs = pagelist->addrs; > > >          need_release = (unsigned long *)(addrs + num_pages); > > >   pages = (struct page **)(addrs + num_pages + 1); > > > + scatterlist = (struct scatterlist *)(pages + num_pages); > > >   > > >   if (is_vmalloc_addr(buf)) { > > > - int dir = (type == PAGELIST_WRITE) ? > > > - DMA_TO_DEVICE : DMA_FROM_DEVICE; > > >   unsigned long length = count; > > >   unsigned int off = offset; > > >   > > > @@ -410,7 +414,6 @@ create_pagelist(char __user *buf, size_t > > > count, > > > unsigned short type, > > >   if (bytes > length) > > >   bytes = length; > > >   pages[actual_pages] = pg; > > > - dmac_map_area(page_address(pg) + off, > > > bytes, dir); > > >   length -= bytes; > > >   off = 0; > > >   } > > > @@ -438,7 +441,8 @@ create_pagelist(char __user *buf, size_t > > > count, > > > unsigned short type, > > >   actual_pages--; > > >   put_page(pages[actual_pages]); > > >   } > > > - kfree(pagelist); > > > + dma_free_coherent(g_dev, pagelist_size, > > > +   pagelist, *dma_addr); > > >   if (actual_pages == 0) > > >   actual_pages = -ENOMEM; > > >   return actual_pages; > > > @@ -450,29 +454,38 @@ create_pagelist(char __user *buf, size_t > > > count, unsigned short type, > > >   pagelist->type = type; > > >   pagelist->offset = offset; > > >   > > > - /* Group the pages into runs of contiguous pages */ > > > - > > > - base_addr = VCHIQ_ARM_ADDRESS(page_address(pages[0])); > > > - next_addr = base_addr + PAGE_SIZE; > > > - addridx = 0; > > > - run = 0; > > > - > > > - for (i = 1; i < num_pages; i++) { > > > - addr = > > > VCHIQ_ARM_ADDRESS(page_address(pages[i])); > > > - if ((addr == next_addr) && (run < (PAGE_SIZE - > > > 1))) { > > > - next_addr += PAGE_SIZE; > > > - run++; > > > - } else { > > > - addrs[addridx] = (unsigned > > > long)base_addr > > > + run; > > > - addridx++; > > > - base_addr = addr; > > > - next_addr = addr + PAGE_SIZE; > > > - run = 0; > > > - } > > > + for (i = 0; i < num_pages; i++) > > > + sg_set_page(scatterlist + i, pages[i], > > > PAGE_SIZE, > > > 0); > > > + > > > + dma_buffers = dma_map_sg(g_dev, > > > +  scatterlist, > > > +  num_pages, > > > +  dir); > > > + > > > + if (dma_buffers == 0) { > > > + dma_free_coherent(g_dev, pagelist_size, > > > +   pagelist, *dma_addr); > > > + > > > + return -EINTR; > > >   } > > >   > > > - addrs[addridx] = (unsigned long)base_addr + run; > > > - addridx++; > > > + k = 0; > > > + for (i = 0; i < dma_buffers;) { > > > + u32 section_length = 0; > > > + > > > + for (j = i + 1; j < dma_buffers; j++) { > > > + if (sg_dma_address(scatterlist + j) != > > > +     sg_dma_address(scatterlist + j - 1) > > > + > > > PAGE_SIZE) { > > > + break; > > > + } > > > + section_length++; > > > + } > > > + > > > + addrs[k] = (u32)sg_dma_address(scatterlist + i) > > > | > > > + section_length; > > > > It looks like scatterlist may not be just an array, so I think this > > whole loop wants to be something like: > > > > for_each_sg(scatterlist, sg, num_pages, i) { > > u32 len = sg_dma_len(sg); > >         u32 addr = sg_dma_address(sg); > > > >         if (k > 0 && addrs[k - 1] & PAGE_MASK + > >             (addrs[k - 1] & ~PAGE_MASK) << PAGE_SHIFT) == addr) { > >          addrs[k - 1] += len; > >         } else { > >          addrs[k++] = addr | len; > >         } > > } > > > > Note the use of sg_dma_len(), since sg entries may be more than one > > page.  I don't think any merging is actually happening on our > > platform > > currently, thus why I left the inner loop. > > > > Thanks for taking on doing this conversion!  This code's going to > > be > > so > > much nicer when you're done. > > Thanks for looking at this.   > > 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. > > 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.  So I'm also a bit confused about the need for > handling scatterlists that are not page aligned or a multiple of > pages. > > 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.