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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
Suggested-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
---
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
