Re: [PATCH 2/2] staging: vc04_services: Replace dmac_map_area with dmac_map_sg

[Michael Zoran]

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@xxxxxxxxxxxx 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.