Re: [PATCH] staging: ion: Rework ion_map_dma_buf() to minimize re-mapping

[Laura Abbott <labbott@redhat.com>]

On 10/17/2018 07:53 PM, John Stultz wrote:
> On Fri, Oct 12, 2018 at 10:51 AM, Laura Abbott <labbott@redhat.com> wrote:
>>
>> I suspect most of the cost of the dma_map/dma_unmap is from the
>> cache flushing and not the actual mapping operations. If this
>> is the case, another option might be to figure out how to
>> incorporate dma_attrs so drivers can use DMA_ATTR_SKIP_CPU_SYNC
>> to decide when they actually want to sync.
> 
> So just to confirm on this point, I basically tested the following
> change (whitespace corrupt, sorry):
> 
> diff --git a/drivers/staging/android/ion/ion.c
> b/drivers/staging/android/ion/ion.c
> index 24cb666..e76b0e2 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -273,8 +273,8 @@ static struct sg_table *ion_map_dma_buf(struct
> dma_buf_attachment *attachment,
> 
>          table = a->table;
> 
> -       if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> -                       direction))
> +       if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> +                             direction,  DMA_ATTR_SKIP_CPU_SYNC))
>                  return ERR_PTR(-ENOMEM);
> 
>          return table;
> @@ -284,7 +284,7 @@ static void ion_unmap_dma_buf(struct
> dma_buf_attachment *attachment,
>                                struct sg_table *table,
>                                enum dma_data_direction direction)
>   {
> -       dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
> +       dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents,
> direction, DMA_ATTR_SKIP_CPU_SYNC);
>   }
> 
>   static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> 
> 
> And indeed, that performed similarly to the pre 4.12 ION code (and it
> also had some of the same image caching error garbage we've seen w/
> 4.9 era kernels, which my earlier patch didn't have).
> 
> So yes, it seems having some way to conditionally skip cpu sync would
> be good.  Though I'm not sure what sort of interface to using this you
> might have in mind?
> 

I hadn't quite gotten anything fully formed but I think solving this
will require changes at the dma-buf layer. One idea I had was allowing
dma_attrs to be set per attachment, the other was extending the
dma_buf_map_attachment to take an attrs argument. I'm at OSSEU this
week but I didn't want to forget about this. I'll see if I can turn
this into a more coherent proposal unless you get to it first.

Thanks,
Laura

> thanks
> -john
>