lkml.org 
[lkml]   [2018]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] staging: ion: Rework ion_map_dma_buf() to minimize re-mapping
From
Date
On 10/10/2018 04:33 PM, John Stultz wrote:
> Since 4.12, much later narrowed down to commit 2a55e7b5e544
> ("staging: android: ion: Call dma_map_sg for syncing and mapping"),
> we have seen graphics performance issues on the HiKey960.
>
> This was initially confounded by the fact that the out-of-tree
> DRM driver was using HiSi custom ION heap which broke with the
> 4.12 ION abi changes, so there was lots of suspicion that the
> performance problems were due to switching to a somewhat simple
> cma based DRM driver for HiKey960. Additionally, as no
> performance regression was seen w/ the original HiKey board
> (which is SMP, not big.LITTLE as w/ HiKey960), there was some
> thought that the out-of-tree EAS code wasn't quite optimized.
>
> But after chasing a number of other leads, I found that
> reverting the ION code to 4.11-era got the majority of the
> graphics performance back (there may yet be further EAS tweaks
> needed), which lead me to the dma_map_sg change.
>
> In talking w/ Laura and Liam, it was suspected that the extra
> cache operations were causing the trouble. Additionally, I found
> that part of the reason we didn't see this w/ the original
> HiKey board is that its (proprietary blob) GL code uses ion_mmap
> and ion_map_dma_buf is called very rarely, where as with
> HiKey960, the (also proprietary blob) GL code calls
> ion_map_dma_buf much more frequently via the kernel driver.
>
> Anyway, with the cause of the performance regression isolated,
> I've tried to find a way to improve the performance of the
> current code.
>
> This approach, which I've mostly copied from the drm_prime
> implementation is to try to track the direction we're mapping
> the buffers so we can avoid calling dma_map/unmap_sg on every
> ion_map_dma_buf/ion_unmap_dma_buf call, and instead try to do
> the work in attach/detach paths.
>
> I'm not 100% sure of the correctness here, so close review would
> be good, but it gets the performance back to being similar to
> reverting the ION code to the 4.11-era.
>
> Feedback would be greatly appreciated!
>
> Cc: Beata Michalska <Beata.Michalska@arm.com>
> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
> Cc: Anders Pedersen <Anders.Pedersen@arm.com>
> Cc: John Reitan <John.Reitan@arm.com>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Todd Kjos <tkjos@android.com>
> Cc: Martijn Coenen <maco@android.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> drivers/staging/android/ion/ion.c | 36 +++++++++++++++++++++++++++++++-----
> 1 file changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 9907332..a4d7fca 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -199,6 +199,7 @@ struct ion_dma_buf_attachment {
> struct device *dev;
> struct sg_table *table;
> struct list_head list;
> + enum dma_data_direction dir;
> };
>
> static int ion_dma_buf_attach(struct dma_buf *dmabuf,
> @@ -220,6 +221,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf,
>
> a->table = table;
> a->dev = attachment->dev;
> + a->dir = DMA_NONE;
> INIT_LIST_HEAD(&a->list);
>
> attachment->priv = a;
> @@ -236,6 +238,18 @@ static void ion_dma_buf_detatch(struct dma_buf *dmabuf,
> {
> struct ion_dma_buf_attachment *a = attachment->priv;
> struct ion_buffer *buffer = dmabuf->priv;
> + struct sg_table *table;
> +
> + if (!a)
> + return;
> +
> + table = a->table;
> + if (table) {
> + if (a->dir != DMA_NONE)
> + dma_unmap_sg(attachment->dev, table->sgl, table->nents,
> + a->dir);
> + sg_free_table(table);
> + }
>
> free_duped_table(a->table);
> mutex_lock(&buffer->lock);
> @@ -243,6 +257,7 @@ static void ion_dma_buf_detatch(struct dma_buf *dmabuf,
> mutex_unlock(&buffer->lock);
>
> kfree(a);
> + attachment->priv = NULL;
> }
>
> static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
> @@ -251,12 +266,24 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
> struct ion_dma_buf_attachment *a = attachment->priv;
> struct sg_table *table;
>
> - table = a->table;
> + if (WARN_ON(direction == DMA_NONE || !a))
> + return ERR_PTR(-EINVAL);
>
> - if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> - direction))
> - return ERR_PTR(-ENOMEM);
> + if (a->dir == direction)
> + return a->table;
>
> + if (WARN_ON(a->dir != DMA_NONE))
> + return ERR_PTR(-EBUSY);
> +
> + table = a->table;
> + if (!IS_ERR(table)) {
> + if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> + direction)) {
> + table = ERR_PTR(-ENOMEM);
> + } else {
> + a->dir = direction;
> + }
> + }
> return table;
> }
>
> @@ -264,7 +291,6 @@ 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);

This changes the semantics so that the only time a buffer
gets unmapped is on detach. I don't think we want to restrict
Ion to that behavior but I also don't know if anyone else
is relying on that. I thought there might have been some Qualcomm
stuff that did that (Liam? Todd?)

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.

Thanks,
Laura

> }
>
> static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
>

\
 
 \ /
  Last update: 2018-10-12 19:53    [W:0.072 / U:0.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site