Re: [PATCH v2] staging: ion: Add a default struct device for cma heap

[Feng Tang]

Hi Greg,

Thanks for the review!

On Thu, Aug 06, 2015 at 09:54:04PM -0700, Greg Kroah-Hartman wrote:
> On Fri, Aug 07, 2015 at 11:50:13AM +0800, Feng Tang wrote:
> > When trying to use several cma heaps on our platforms,
> > we met a memory issue due to that the several cma_heaps
> > are sharing the same "struct device *".
> > 
> > As in current code base, the normal cma heap creating
> > process is, one platform device is created during boot,
> > and it will sequentially create cma heaps (usually passing
> > its own struct device * as a parameter)
> > 
> > For the multiple cma heaps case, there will be one "struct
> > cma" created for each cma heap, and this "struct cma *" is
> > saved in dev->cma_area. So the single platform device can't
> > meet the requirement here.
> > 
> > This patch adds one default device for each cma heap to avoid
> > sharing the same "struct device", thus fix the issue. And it
> > doesn't break existing code by only using that default device
> > when no "struct device *" is passed in.
> > 
> > Also, since the cma framework has been cleaned up recently,
> > this patch also adds a platform data member to pass the
> > "struct cma*" to ion_cma_heap_create().
> > 
> > Signed-off-by: Feng Tang <feng.tang@xxxxxxxxx>
> > [From CMAâs point of view: ]
> > Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
> > ---
> >  drivers/staging/android/ion/ion.h          |    4 ++++
> >  drivers/staging/android/ion/ion_cma_heap.c |   20 +++++++++++++++++---
> >  2 files changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
> > index 443db84..11336df 100644
> > --- a/drivers/staging/android/ion/ion.h
> > +++ b/drivers/staging/android/ion/ion.h
> > @@ -44,6 +44,9 @@ struct ion_buffer;
> >   * @base:	base address of heap in physical memory if applicable
> >   * @size:	size of the heap in bytes if applicable
> >   * @align:	required alignment in physical memory if applicable
> > + * @cma:	when creating CMA heap, platform device should better also
> > + *		pass the "struct cma *" info, so that the cma buffer request
> > + *		know where to go for the buffer
> >   * @priv:	private info passed from the board file
> >   *
> >   * Provided by the board file.
> > @@ -55,6 +58,7 @@ struct ion_platform_heap {
> >  	ion_phys_addr_t base;
> >  	size_t size;
> >  	ion_phys_addr_t align;
> > +	struct cma *cma;
> >  	void *priv;
> >  };
> >  
> > diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> > index f4211f1..27f218a 100644
> > --- a/drivers/staging/android/ion/ion_cma_heap.c
> > +++ b/drivers/staging/android/ion/ion_cma_heap.c
> > @@ -29,6 +29,7 @@
> >  struct ion_cma_heap {
> >  	struct ion_heap heap;
> >  	struct device *dev;
> > +	struct device default_dma_dev;
> 
> Why do we have 2 struct devices here?

My bad, I haven't make it clear in git log

For cma heap, the ion_buffer creation will actually go to dma allocation
subsystem, for example
	dma_generic_alloc_coherent	(arch/x86/kernel/pci-dma.c)
		dma_alloc_from_contiguous(dev, count, get_order(size))
			cma_alloc(dev_get_cma_area(dev), count, align)
	
while the dev_get_cma_area(dev) == dev->cma_area in most of cma scenario.

so each struct cma_heap needs one "struct dev" to use it here, and its only
use is to simply proivde one dev->cma_area.

As in current code base, the normal cma heap creating process is, one
ion platform device is created during boot, and it will sequentially create
cma heaps (usually passing its own struct device * as a parameter)

On our customer's platform, they wanted to use multiple cma heaps, so
the only one ion device should not be shared to these cma heaps, as their
"dev->cma_area" is different.

My solution is to embed one by-default cma device inside the cma_heap for
this use case. Keeping the "struct device *dev" is simply to be compatible
with current existing ion platform driver, otherwise 

	struct ion_cma_heap {
		struct ion_heap heap;
		struct device default_dma_dev;
	}
removing the "dev" can also meet our needs.

> 
> >  };
> >  
> >  #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap)
> > @@ -180,9 +181,22 @@ struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data)
> >  		return ERR_PTR(-ENOMEM);
> >  
> >  	cma_heap->heap.ops = &ion_cma_ops;
> > -	/* get device from private heaps data, later it will be
> > -	 * used to make the link with reserved CMA memory */
> > -	cma_heap->dev = data->priv;
> > +
> > +	/*
> > +	 * data->priv for cma heap is currently supposed to point
> > +	 * to a "struct device *"
> > +	 */
> > +	if (data->priv) {
> > +		cma_heap->dev = data->priv;
> > +	} else {
> > +		cma_heap->dev = &cma_heap->default_dma_dev;
> 
> That's not good, it's not initialized, or if it is, what's the lifetime
> rules for it now?

As I described above, the dummy struct device is only needed for
dma request, its lifetime is align with the cma_heap itself. 

I created it follow one example in arch/x86/kernel/pci-dma.c

	/* Dummy device used for NULL arguments (normally ISA). */
	struct device x86_dma_fallback_dev = {
		.init_name = "fallback device",
		.coherent_dma_mask = ISA_DMA_BIT_MASK,
		.dma_mask = &x86_dma_fallback_dev.coherent_dma_mask,
	};
	EXPORT_SYMBOL(x86_dma_fallback_dev);

Please let me know if you have any suggestions or concerns, thanks

- Feng

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