Messages in this thread | | | From | Vivek Gautam <> | Date | Thu, 29 Nov 2018 15:13:25 +0530 | Subject | Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg* |
| |
Hi Tomasz,
On Wed, Nov 28, 2018 at 8:39 AM Tomasz Figa <tfiga@chromium.org> wrote: > > Hi Vivek, > > On Tue, Nov 27, 2018 at 6:37 AM Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: > > > > dma_map_sg() expects a DMA domain. However, the drm devices > > have been traditionally using unmanaged iommu domain which > > is non-dma type. Using dma mapping APIs with that domain is bad. > > > > Replace dma_map_sg() calls with dma_sync_sg_for_device{|cpu}() > > to do the cache maintenance. > > > > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > > Suggested-by: Tomasz Figa <tfiga@chromium.org> > > Cc: Rob Clark <robdclark@gmail.com> > > Cc: Jordan Crouse <jcrouse@codeaurora.org> > > Cc: Sean Paul <seanpaul@chromium.org> > > --- > > > > Changes since v1: > > - Addressed Jordan's and Tomasz's comments for > > - moving sg dma addresses preparation out of the coditional > > check to the main path so we do it irrespective of whether > > the buffer is cached or uncached. > > - Enhance the comment to explain this dma addresses prepartion. > > > > Thanks for the patch. Some comments inline. > > > drivers/gpu/drm/msm/msm_gem.c | 31 ++++++++++++++++++++++--------- > > 1 file changed, 22 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > > index 00c795ced02c..1811ac23a31c 100644 > > --- a/drivers/gpu/drm/msm/msm_gem.c > > +++ b/drivers/gpu/drm/msm/msm_gem.c > > @@ -81,6 +81,8 @@ static struct page **get_pages(struct drm_gem_object *obj) > > struct drm_device *dev = obj->dev; > > struct page **p; > > int npages = obj->size >> PAGE_SHIFT; > > + struct scatterlist *s; > > + int i; > > > > if (use_pages(obj)) > > p = drm_gem_get_pages(obj); > > @@ -104,12 +106,21 @@ static struct page **get_pages(struct drm_gem_object *obj) > > return ptr; > > } > > > > - /* For non-cached buffers, ensure the new pages are clean > > + /* > > + * dma_sync_sg_*() flush the physical pages, so point > > + * sg->dma_address to the physical ones for the right behavior. > > The two halves of the sequence don't really relate to each other. An > sglist has the `page` field for the purpose of pointing to physical > pages. The `dma_address` field is for DMA addresses, which are not > equivalent to physical addresses. I'd rewrite it like this;
I guess I was lenient in using the physical pages and dma address words.
> > /* > * Some implementations of the DMA mapping ops expect > * physical addresses of the pages to be stored as DMA > * addresses of the sglist entries. To work around it, > * set them here explicitly. > */
Will update as suggested.
> > + */ > > + for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i) > > + sg_dma_address(s) = sg_phys(s); > > + > > + /* > > + * For non-cached buffers, ensure the new pages are clean > > * because display controller, GPU, etc. are not coherent: > > */ > > - if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED)) > > - dma_map_sg(dev->dev, msm_obj->sgt->sgl, > > - msm_obj->sgt->nents, DMA_BIDIRECTIONAL); > > + if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED)) > > + dma_sync_sg_for_device(dev->dev, msm_obj->sgt->sgl, > > + msm_obj->sgt->nents, > > + DMA_TO_DEVICE); > > Why changing from DMA_BIDIRECTIONAL?
Yea, I went back and checked that we wanted to do this for both. Will keep DMA_BIDIRECTIONAL intact.
> > > } > > > > return msm_obj->pages; > > @@ -133,14 +144,16 @@ static void put_pages(struct drm_gem_object *obj) > > > > if (msm_obj->pages) { > > if (msm_obj->sgt) { > > - /* For non-cached buffers, ensure the new > > + /* > > + * For non-cached buffers, ensure the new > > * pages are clean because display controller, > > * GPU, etc. are not coherent: > > */ > > - if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED)) > > - dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl, > > - msm_obj->sgt->nents, > > - DMA_BIDIRECTIONAL); > > + if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED)) > > + dma_sync_sg_for_cpu(obj->dev->dev, > > + msm_obj->sgt->sgl, > > + msm_obj->sgt->nents, > > + DMA_FROM_DEVICE); > > Ditto.
Sure, will change this as well.
Thanks & Regards Vivek
> > Best regards, > Tomasz
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
| |