Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1200562 > unrolled thread

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] CMA: Don't return a valid cma for non-cma dev Feng Tang <feng.tang@intel.com> - 2015-08-05 11:20 +0200
    Re: [PATCH] CMA: Don't return a valid cma for non-cma dev Michal Nazarewicz <mina86@mina86.com> - 2015-08-05 12:30 +0200
      Re: [PATCH] CMA: Don't return a valid cma for non-cma dev Feng Tang <feng.tang@intel.com> - 2015-08-05 12:40 +0200
      Re: [PATCH] CMA: Don't return a valid cma for non-cma dev Feng Tang <feng.tang@intel.com> - 2015-08-05 12:50 +0200
        Re: [PATCH] CMA: Don't return a valid cma for non-cma dev Michal Nazarewicz <mina86@mina86.com> - 2015-08-05 13:20 +0200
          Re: [PATCH] CMA: Don't return a valid cma for non-cma dev Feng Tang <feng.tang@intel.com> - 2015-08-05 15:20 +0200

#1200562 — Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

On Fri, Jul 31, 2015 at 07:46:30PM +0200, Michal Nazarewicz wrote:
> On Fri, Jul 31 2015, Feng Tang wrote:
> > Maybe I didn't make my problem clear, for our platform, we do need to
> > use cma as we have camera ISP which has no IOMMU, so we cannot set
> > "cma=0".
> 
> Then specify a CMA region for the camera in platform initialisation code
> or device trees or whatever else is the rave nowadays.
> 
> I’m assuming that you have a piece of code (or configuration of some
> sort) that assigns a CMA region to the device (otherwise ‘dev->cma_area’
> would be NULL and your patch would just always get you NULL CMA area).
> Simply create a CMA area there and assign it to the device.

Your suggestion remind me one more thing, that for a system which needs
multiple cma heaps (like for security reason), they may have to share
one struct device *dev, as in ion_cma_heap_create()

struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data)
{
	struct ion_cma_heap *cma_heap;

	cma_heap = kzalloc(sizeof(struct ion_cma_heap), GFP_KERNEL);

	if (!cma_heap)
		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;
	cma_heap->heap.type = ION_HEAP_TYPE_DMA;
	return &cma_heap->heap;
}

Usually the platform data's priv points to the same ion platform device,
so the several heap's dev will be same.

Then when the time comes to allocate for each cma heap, the "dev->cma_area"
may have to be dynamically switched, and casue many syncing trouble.

I'm working on a patch for this. 

Thanks,
Feng


--
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/

[toc] | [next] | [standalone]

#1200631

On Wed, Aug 05 2015, Feng Tang wrote:
> that for a system which needs multiple cma heaps (like for security
> reason), they may have to share one struct device *dev, as in
> ion_cma_heap_create()

If you need several CMA areas to allocate from, create multiple struct
devices.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
--
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/

[toc] | [prev] | [next] | [standalone]

#1200637

On Wed, Aug 05, 2015 at 12:28:03PM +0200, Michal Nazarewicz wrote:
> On Wed, Aug 05 2015, Feng Tang wrote:
> > that for a system which needs multiple cma heaps (like for security
> > reason), they may have to share one struct device *dev, as in
> > ion_cma_heap_create()
> 
> If you need several CMA areas to allocate from, create multiple struct
> devices.

Yes, that's my thought too. The normal cma case for SOCs are one platform
device is created in drivers/staging/android/ion/xxx_ion.c, then that
device's pointer will be transferred into ion_cma_heap_create() inside
of the struct ion_platform_heap data.

And it's not easy to create multiple platform devices. And it may be
better to give each cma_heap one dedicated device in struct ion_cma_heap
which could be used as a parameter for 
	dma_alloc_from_contigous() --> cma_alloc()

Thanks,
Feng
 
> -- 
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
> ..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
> ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
--
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/

[toc] | [prev] | [next] | [standalone]

#1200645

On Wed, Aug 05, 2015 at 12:28:03PM +0200, Michal Nazarewicz wrote:
> On Wed, Aug 05 2015, Feng Tang wrote:
> > that for a system which needs multiple cma heaps (like for security
> > reason), they may have to share one struct device *dev, as in
> > ion_cma_heap_create()
> 
> If you need several CMA areas to allocate from, create multiple struct
> devices.

I've made a quick patch, which works ok on our multiple cma heap cases.

Thanks,
Feng

---
diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
index f4211f1..ee9c5d1 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;
 };
 
 #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap)
@@ -180,9 +181,14 @@ 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;
+
+	cma_heap->dev = &cma_heap->default_dma_dev;
+	cma_heap->dev->coherent_dma_mask = DMA_BIT_MASK(32);
+	cma_heap->dev->dma_mask = &dev->coherent_dma_mask;
+
+	/* data->priv contains a pointer to struct cma */
+	dev_set_cma_area(cma_heap->dev, data->priv);
+
 	cma_heap->heap.type = ION_HEAP_TYPE_DMA;
 	return &cma_heap->heap;
 }


> -- 
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
> ..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
> ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
--
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/

[toc] | [prev] | [next] | [standalone]

#1200656

> On Wed, Aug 05, 2015 at 12:28:03PM +0200, Michal Nazarewicz wrote:
>> If you need several CMA areas to allocate from, create multiple struct
>> devices.

On Wed, Aug 05 2015, Feng Tang wrote:
> I've made a quick patch, which works ok on our multiple cma heap cases.

> ---
> diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> index f4211f1..ee9c5d1 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;

I’m unfamiliar with ION code so cannot comment in great detail, butwhy
do you need dev and default_dma_dev fields?  Just make dev a non-pointer
and use that.

>  };
>  
>  #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap)
> @@ -180,9 +181,14 @@ 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;
> +
> +	cma_heap->dev = &cma_heap->default_dma_dev;
> +	cma_heap->dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +	cma_heap->dev->dma_mask = &dev->coherent_dma_mask;
> +
> +	/* data->priv contains a pointer to struct cma */
> +	dev_set_cma_area(cma_heap->dev, data->priv);

In the previous code, data->priv seemed to be struct device*, but in
this code it is used as struct cma*.

> +
>  	cma_heap->heap.type = ION_HEAP_TYPE_DMA;
>  	return &cma_heap->heap;
>  }

But yeah, in general, from CMA’s point of view, this looks good.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
--
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/

[toc] | [prev] | [next] | [standalone]

#1200721

On Wed, Aug 05, 2015 at 01:15:50PM +0200, Michal Nazarewicz wrote:
> > On Wed, Aug 05, 2015 at 12:28:03PM +0200, Michal Nazarewicz wrote:
> >> If you need several CMA areas to allocate from, create multiple struct
> >> devices.
> 
> On Wed, Aug 05 2015, Feng Tang wrote:
> > I've made a quick patch, which works ok on our multiple cma heap cases.
> 
> > ---
> > diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> > index f4211f1..ee9c5d1 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;
> 
> I’m unfamiliar with ION code so cannot comment in great detail, butwhy
> do you need dev and default_dma_dev fields?  Just make dev a non-pointer
> and use that.

Good point. I've thought about keeping dev to be back compatible
with current code, and only use default_dma_dev when no "dev" is
passed in platform data. But from your other comments, it may
be not necessary to keep the "dev" 

> 
> >  };
> >  
> >  #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap)
> > @@ -180,9 +181,14 @@ 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;
> > +
> > +	cma_heap->dev = &cma_heap->default_dma_dev;
> > +	cma_heap->dev->coherent_dma_mask = DMA_BIT_MASK(32);
> > +	cma_heap->dev->dma_mask = &dev->coherent_dma_mask;
> > +
> > +	/* data->priv contains a pointer to struct cma */
> > +	dev_set_cma_area(cma_heap->dev, data->priv);
> 
> In the previous code, data->priv seemed to be struct device*, but in
> this code it is used as struct cma*.

As we are going to use per cma-heap's own "default_dma_dev",
the "data->priv" should be a pointer to "struct cma*", which
makes more sense, as when a cma heap is created, we'd better
provide a "struct cma *" to tell it where to request cma buffer.

> 
> > +
> >  	cma_heap->heap.type = ION_HEAP_TYPE_DMA;
> >  	return &cma_heap->heap;
> >  }
> 
> But yeah, in general, from CMA’s point of view, this looks good.

Thanks! Will try to clean the code and add more comments, then
send it out for review.

- Feng
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web