Date: Tue, 06 Dec 2005 19:41:03 +0100
Message-ID: <s5hiru2jcg0.wl%tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Christoph Hellwig <hch@infradead.org>
Cc: Andrew Morton <akpm@osdl.org>, jayakumar.alsa@gmail.com, perex@suse.cz,
       mj@ucw.cz, alsa-devel@lists.sourceforge.net,
       linux-kernel@vger.kernel.org
Subject: Re: [Alsa-devel] Re: [PATCH 2.6.13.1 1/1] CS5535 AUDIO ALSA driver
In-Reply-To: <20051206174203.GA24569@infradead.org>
References: <200509190639.j8J6dIM4007948@localhost.localdomain>
	<20050920152830.7ef6733b.akpm@osdl.org>
	<47f5dce305092017031a2ba375@mail.gmail.com>
	<20050920172309.626db866.akpm@osdl.org>
	<20050921083109.GA27254@infradead.org>
	<s5hpsqsdn6g.wl%tiwai@suse.de>
	<20051206174203.GA24569@infradead.org>
User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.7 (=?ISO-8859-4?Q?Sanj=F2?=) APEL/10.6 MULE XEmacs/21.5 (beta21)
 (corn) (+CVS-20050720) (i386-suse-linux)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 7019
Lines: 259

At Tue, 6 Dec 2005 17:42:03 +0000,
Christoph Hellwig wrote:
> 
> On Thu, Sep 29, 2005 at 03:21:43PM +0200, Takashi Iwai wrote:
> > At Wed, 21 Sep 2005 09:31:09 +0100,
> > Christoph Hellwig wrote:
> > > 
> > > On Tue, Sep 20, 2005 at 05:23:09PM -0700, Andrew Morton wrote:
> > > > > I'm not sure what to do. I'd be happy to take them out. But I woudn't
> > > > > mind leaving them in if that's what alsa convention is.
> > > > 
> > > > I'd be inclined to stick with the alsa style.  That's just an fyi if you
> > > > plan on working in other places.
> > > 
> > > I'd _really_ prefer to fix all of alsa.  Alsa is so full of wrappers
> > > and multiple names for the same thing that's it's almost impossibly
> > > for a normal kernel developer to fix anythign in there.
> > 
> > Oh I guess you're referring to the messy memory stuff in ALSA code?
> > 
> > (Or you mean about the foo_t style?  Then it would be easy to fix, and
> >  I'd appreciate if someone takes this job :)
> > 
> > Basically I agree with all your suggestions - hacks have been there
> > simply as workarounds.  They should be removed.
> > 
> > The following are in WIP on my local tree:
> > 
> > - Merge of dma_alloc_coherent() hacks for pages <32bit to kernel core
> >   (for i386 and ppc)
> 
> Did this get out?

Not yet.  It's pending because I had other big rewrites beforehand.
I'll prepare a patch for this soon later.


> > - PageReserve and mmaps:
> >   dma_mmap_coherent() will be used in all architectures instead of
> >   vma nopage callback.  Of course, dma_mmap_coherent() should be
> >   ported to all archs.
> 
> What about just making the i386 one Russell posted generic?  It shouldn't
> be any less broken than what we do now and every architecture that needs
> something more specific can implements it.

Yes, it's a good starting point, indeed.


> > - Removal of the common allocator in sound/core/memalloc.c
> >   Instead, each driver has alloc_buffer() and free_buffer()
> >   callbacks.
> 
> this isn't in mainline yet, is it?

Not yet, too.  I'm waiting for stabilization of the recent changes on
mm before this clean-up action.  Also, still considering the best
solution...


> > - The "right" way to allocate/mmap coherent SG-buffers.
> 
> How do you get coherent SG-buffers?  The only way to get coherent dma
> allocations is dma_alloc_coheren (and the pci variant), but that's not
> handing back an SG list.  But even if you build one yourself it should
> be handled by dma_mmap_coherent.

I thought of creating an API for coherent SG-buffers, but it looks
ineffective on some architectures to have large number of
non-contiguous coherent pages.  Each call of dma_alloc_coherent()
involves kmalloc of another record for a consistent VM region.

So, currently I gave up it and rewrote the stuff in an easier style,
using dma_map_sg() + dma_sync_sg_*() for pages allocated via
__vmalloc().  The question is whether mmap of non-coherent buffer may
influence on the behavior of user-space apps on architectures like ARM
or SPARC.  If it really matters, we can disable mmap for such
architectures, as a safe solution :)


> > - A common API for mmap vmalloc buffers.
> 
> What API do you think about?  If you do alloc_page yourself and use
> vmap everything gets much much easier.  And with the new vm_install_page
> this becomes pretty easy:
> 
> init_routine()
> {
> 
> 	...
> 	for (i = 0; i < NR_PAGES; i++) {
> 		pages[i] = alloc_page(GFP_KERNEL);
> 		...
> 	}
> 
> 	dma_buffer = vmap(pages, ...);
> }
> 
> dma_map()
> {
> 	for (i = 0; i < NR_PAGES; i++) {
> 		dma_map_page(pages[i, ....);
> 		...
> 	}
> }
> 
> mmap()
> {
> 	for (i = 0; i < NR_PAGES; i++) {
> 		vm_insert_page(..., pages[i], ..);
> 		...
> 	}
> }
> 
> maybe some tiny helpers that encapsulate the loop over the pages would
> be nice, but we shouldn't need much of a new API here.

My current version has the helper functions as below.  This is
intended for the generic SG-buffers, and uses struct scatterlist.
It looks a bit more complicated than your example above as
consequence.

Any comments are appreciated.


Thanks,

Takashi

================

struct snd_sg_buf {
	struct scatterlist *sglist;
	int nelems;
	int nmaps;
	int direction;
};

static void *do_alloc_sgbuf(struct device *dev, struct snd_sg_buf *sg,
			    gfp_t gfp)
{
	void *vptr;
	struct scatterlist *sgl;
	int i;

	vptr = __vmalloc(sg->nelems << PAGE_SHIFT, gfp, PAGE_KERNEL);
	if (! vptr)
		return NULL;
	
	/* set up sg list */
	sgl = sg->sglist;
	memset(sgl, 0, sg->nelems * sizeof(*sgl));
	for (i = 0; i < sg->nelems; i++, sgl++) {
		sgl->page = vmalloc_to_page(vptr + (i << PAGE_SHIFT));
		sgl->offset = 0;
		sgl->length = PAGE_SIZE;
	}

	if (! dev)
		return vptr;

	sg->nmaps = dma_map_sg(dev, sg->sglist, sg->nelems, sg->direction);
	if (! sg->nmaps) {
		vfree(vptr);
		return NULL;
	}
	if (dev->dma_mask && *dev->dma_mask &&
	    *dev->dma_mask < DMA_32BIT_MASK) {
		/* check whether all pages are in the given dma mask */
		dma_addr_t rmask = ~(*dev->dma_mask);
		sgl = sg->sglist;
		for (i = 0; i < sg->nmaps; i++, sgl++) {
			if (sg_dma_address(sgl) & rmask) {
				dma_unmap_sg(dev, sg->sglist, sg->nelems,
					     sg->direction);
				vfree(vptr);
				return NULL;
			}
		}
	}
	return vptr;
}

/*
 * allocate an sg buffer and map it
 */
void *snd_dma_alloc_sgbuf(struct device *dev, size_t size, int direction,
			 struct snd_sg_buf **sg_ret)
{
	struct snd_sg_buf *sg;
	void *vptr;

	sg = kzalloc(sizeof(*sg), GFP_KERNEL);
	if (! sg)
		return NULL;

	size = PAGE_ALIGN(size);
	sg->nelems = size >> PAGE_SHIFT;
	sg->direction = direction;
	sg->sglist = kmalloc(sg->nelems * sizeof(struct scatterlist), GFP_KERNEL);
	if (! sg->sglist) {
		kfree(sg);
		return NULL;
	}

	if ((vptr = do_alloc_sgbuf(dev, sg, GFP_KERNEL)) == NULL &&
	    (vptr = do_alloc_sgbuf(dev, sg, GFP_DMA)) == NULL) {
		kfree(sg->sglist);
		kfree(sg);
		return NULL;
	}

	*sg_ret = sg;
	return vptr;
}

/*
 * unmap and release the sg buffer
 */
void snd_dma_free_sgbuf(struct device *dev, void *vptr, struct snd_sg_buf *sg)
{
	if (sg->nmaps)
		dma_unmap_sg(dev, sg->sglist, sg->nelems, sg->direction);
	vfree(vptr);
	kfree(sg->sglist);
	kfree(sg);
}

/*
 * return the bus address at the corresponding offset
 */
dma_addr_t snd_sgbuf_get_addr(struct snd_sg_buf *sg, size_t offset)
{
	struct scatterlist *sgl;
	int i;

	if (offset >= (sg->nelems << PAGE_SHIFT))
		return 0;

	/* short path - no compounds */
	if (sg->nelems == sg->nmaps)
		return sg_dma_address(&sg->sglist[offset >> PAGE_SHIFT])
			+ offset % PAGE_SIZE;

	/* look for the corresponding entry... should be optimized */
	sgl = sg->sglist;
	for (i = 0; i < sg->nmaps; i++, sgl++) {
		if (sg_dma_len(sgl) > offset)
			return sg_dma_address(sgl) + offset;
		offset -= sg_dma_len(sgl);
	}
	return 0;
}

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/