Michael Zoran <mzo...@crowfest.net> 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:
>> > 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.

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.

Attachment: signature.asc
Description: PGP signature

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

Reply via email to