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