lkml.org 
[lkml]   [2016]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/2] staging: vc04_services: Replace dmac_map_area with dmac_map_sg
    From
    Date
    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.





    \
     
     \ /
      Last update: 2016-10-25 17:10    [W:3.399 / U:0.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site