diff mbox series

[4/4] staging: android: ion: Support for mapping with dma mapping attributes

Message ID 1547836667-13695-5-git-send-email-lmark@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series ION stability and perf changes | expand

Commit Message

Liam Mark Jan. 18, 2019, 6:37 p.m. UTC
Add support for configuring dma mapping attributes when mapping
and unmapping memory through dma_buf_map_attachment and
dma_buf_unmap_attachment.

For example this will allow ION clients to skip cache maintenance, by
using DMA_ATTR_SKIP_CPU_SYNC, for buffers which are clean and haven't been
accessed by the CPU.

Signed-off-by: Liam Mark <lmark@codeaurora.org>
---
 drivers/staging/android/ion/ion.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Brian Starkey Jan. 21, 2019, 12:19 p.m. UTC | #1
Hi Liam,

On Fri, Jan 18, 2019 at 10:37:47AM -0800, Liam Mark wrote:
> Add support for configuring dma mapping attributes when mapping
> and unmapping memory through dma_buf_map_attachment and
> dma_buf_unmap_attachment.
> 
> For example this will allow ION clients to skip cache maintenance, by
> using DMA_ATTR_SKIP_CPU_SYNC, for buffers which are clean and haven't been
> accessed by the CPU.

How can a client know that the buffer won't be accessed by the CPU in
the future though?

I don't think we can push this decision to clients, because they are
lacking information about what else is going on with the buffer. It
needs to be done by the exporter, IMO.

Thanks,
-Brian

> 
> Signed-off-by: Liam Mark <lmark@codeaurora.org>
> ---
>  drivers/staging/android/ion/ion.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 1fe633a7fdba..0aae845b20ba 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -268,8 +268,8 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
>  	table = a->table;
>  
>  	mutex_lock(&buffer->lock);
> -	if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> -			direction)) {
> +	if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> +			      direction, attachment->dma_map_attrs)) {
>  		mutex_unlock(&buffer->lock);
>  		return ERR_PTR(-ENOMEM);
>  	}
> @@ -287,7 +287,8 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
>  	struct ion_buffer *buffer = attachment->dmabuf->priv;
>  
>  	mutex_lock(&buffer->lock);
> -	dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
> +	dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents, direction,
> +			   attachment->dma_map_attrs);
>  	a->dma_mapped = false;
>  	mutex_unlock(&buffer->lock);
>  }
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
> a Linux Foundation Collaborative Project
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Liam Mark Jan. 22, 2019, 10:37 p.m. UTC | #2
On Mon, 21 Jan 2019, Brian Starkey wrote:

> Hi Liam,
> 
> On Fri, Jan 18, 2019 at 10:37:47AM -0800, Liam Mark wrote:
> > Add support for configuring dma mapping attributes when mapping
> > and unmapping memory through dma_buf_map_attachment and
> > dma_buf_unmap_attachment.
> > 
> > For example this will allow ION clients to skip cache maintenance, by
> > using DMA_ATTR_SKIP_CPU_SYNC, for buffers which are clean and haven't been
> > accessed by the CPU.
> 
> How can a client know that the buffer won't be accessed by the CPU in
> the future though?
> 
Yes, for use cases where you don't if it will be accessed in the future 
then you would only use it to optimize the dma map path, but as I 
mentioned in the other thread there are cases (such as in our Camera) 
where we have complete ownership of buffers and do know if it will be 
accessed in the future.

> I don't think we can push this decision to clients, because they are
> lacking information about what else is going on with the buffer. It
> needs to be done by the exporter, IMO.
> 

I do agree it would be better to handle in the exporter, but in a 
pipelining use case where there might not be any devices attached that 
doesn't seem very doable.

> Thanks,
> -Brian
> 
> > 
> > Signed-off-by: Liam Mark <lmark@codeaurora.org>
> > ---
> >  drivers/staging/android/ion/ion.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> > index 1fe633a7fdba..0aae845b20ba 100644
> > --- a/drivers/staging/android/ion/ion.c
> > +++ b/drivers/staging/android/ion/ion.c
> > @@ -268,8 +268,8 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
> >  	table = a->table;
> >  
> >  	mutex_lock(&buffer->lock);
> > -	if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> > -			direction)) {
> > +	if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> > +			      direction, attachment->dma_map_attrs)) {
> >  		mutex_unlock(&buffer->lock);
> >  		return ERR_PTR(-ENOMEM);
> >  	}
> > @@ -287,7 +287,8 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
> >  	struct ion_buffer *buffer = attachment->dmabuf->priv;
> >  
> >  	mutex_lock(&buffer->lock);
> > -	dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
> > +	dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents, direction,
> > +			   attachment->dma_map_attrs);
> >  	a->dma_mapped = false;
> >  	mutex_unlock(&buffer->lock);
> >  }
> > -- 
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
> > a Linux Foundation Collaborative Project
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
diff mbox series

Patch

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 1fe633a7fdba..0aae845b20ba 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -268,8 +268,8 @@  static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
 	table = a->table;
 
 	mutex_lock(&buffer->lock);
-	if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
-			direction)) {
+	if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
+			      direction, attachment->dma_map_attrs)) {
 		mutex_unlock(&buffer->lock);
 		return ERR_PTR(-ENOMEM);
 	}
@@ -287,7 +287,8 @@  static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
 	struct ion_buffer *buffer = attachment->dmabuf->priv;
 
 	mutex_lock(&buffer->lock);
-	dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
+	dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents, direction,
+			   attachment->dma_map_attrs);
 	a->dma_mapped = false;
 	mutex_unlock(&buffer->lock);
 }