Messages in this thread | | | Subject | Re: [PATCH 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg* | From | Vivek Gautam <> | Date | Thu, 22 Nov 2018 15:37:54 +0530 |
| |
Hi Tomasz, Jordan,
On 11/21/2018 9:18 AM, Tomasz Figa wrote: > Hi Jordan, Vivek, > > On Wed, Nov 21, 2018 at 12:41 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: >> On Tue, Nov 20, 2018 at 03:24:37PM +0530, Vivek Gautam 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> >>> --- >>> >>> Tested on an MTP sdm845: >>> https://github.com/vivekgautam1/linux/tree/v4.19/sdm845-mtp-display-working >>> >>> drivers/gpu/drm/msm/msm_gem.c | 27 ++++++++++++++++++++------- >>> 1 file changed, 20 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c >>> index 00c795ced02c..d7a7af610803 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); >>> @@ -107,9 +109,19 @@ static struct page **get_pages(struct drm_gem_object *obj) >>> /* 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)) { >>> + /* >>> + * Fake up the SG table so that dma_sync_sg_*() >>> + * can be used to flush the pages associated with it. >>> + */ >> We aren't really faking. The table is real, we are just slightly abusing the >> sg_dma_address() which makes this comment a bit misleading. Instead I would >> probably say something like: >> >> /* dma_sync_sg_* flushes pages using sg_dma_address() so point it at the >> * physical page for the right behavior */ >> >> Or something like that. >> > It's actually quite complicated, but I agree that the comment isn't > very precise. The cases are as follows: > - arm64 iommu_dma_ops use sg_phys() > https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm64/mm/dma-mapping.c#L599 > - swiotlb_dma_ops used on arm64 if no IOMMU is available use > sg->dma_address directly: > https://elixir.bootlin.com/linux/v4.20-rc3/source/kernel/dma/swiotlb.c#L832 > - arm_dma_ops use sg_dma_address(): > https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm/mm/dma-mapping.c#L1130 > - arm iommu_ops use sg_page(): > https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm/mm/dma-mapping.c#L1869 > > Sounds like a mess... Thanks for the review.
Technically with the below assignment we address all of the above. How about an even simpler version of the suggested comment:
/* dma_sync_sg_* flushes physical pages, so point sg->dma_address to * the physical one for the right behavior. */
> >>> + for_each_sg(msm_obj->sgt->sgl, s, >>> + msm_obj->sgt->nents, i) >>> + sg_dma_address(s) = sg_phys(s); >>> + >> I'm wondering - wouldn't we want to do this association for cached buffers to so >> we could sync them correctly in cpu_prep and cpu_fini? Maybe it wouldn't hurt >> to put this association in the main path (obviously the sync should stay inside >> the conditional for uncached buffers). >>
Sure, I will move this out of the conditional check.
> I guess it wouldn't hurt indeed. Note that cpu_prep/fini seem to be > missing the sync call currently.
I can't say I understand the usage of cpu_prep and cpu_fini(). But I can add the necessary support if you can point me in the right direction. Thanks
Best regards Vivek > > P.S. Jordan, not sure if it's my Gmail or your email client, but your > message had all the recipients in a Reply-to header, except you, so > pressing Reply to all in my case led to a message that didn't have you > in recipients anymore... > > Best regards, > Tomasz
| |