Message-ID: <1477413053.6622.1.camel@crowfest.net>
Subject: Re: [PATCH 2/2] staging: vc04_services: Replace dmac_map_area with
 dmac_map_sg
From: Michael Zoran <mzoran@crowfest.net>
To: Eric Anholt <eric@anholt.net>, 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 09:30:53 -0700
In-Reply-To: <87lgxco13g.fsf@eliezer.anholt.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> <87lgxco13g.fsf@eliezer.anholt.net>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 9845
Lines: 286

On Tue, 2016-10-25 at 09:16 -0700, Eric Anholt wrote:
> Michael Zoran <mzoran@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:
> > > > 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.
> 
> 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.

Please see the pagelist_struct structure.  It contains a comment
that the addrs field is in pages.

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h

typedef struct pagelist_struct {
	unsigned long length;
	unsigned short type;
	unsigned short offset;
	unsigned long addrs[1];	/* N.B. 12 LSBs hold the number
of following
				   pages at consecutive addresses. */
} PAGELIST_T;

If the DMA api can return lengths that aren't pages lengths, then I
don't think this is really a good API to be using especially since the
firmware requires all lengths to be pages.  

It might be good to reference the original code that is currently
checked into the stagging tree.  It also strongly suggests the length
is in pages. 

I tested the changes with vchiq_test and added some temp print outs and
I know that count in pages works and that for larger messages sizes the
changes I sent is combining pages at consecutive addresses.  Sometimes
alot of pages are being combined.

I wish meld or something similar had a way to load a diff so that it's
easier to review these patches.  But I digress...