On Mon, 2016-10-24 at 10:31 -0700, Eric Anholt wrote:
> mzo...@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.



 
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to