Messages in this thread | | | Subject | Re: [PATCH] staging: ion: Rework ion_map_dma_buf() to minimize re-mapping | From | Laura Abbott <> | Date | Fri, 12 Oct 2018 10:51:41 -0700 |
| |
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) >
| |