On Mon, Sep 2, 2019 at 11:03 AM Fabio Estevam festevam@gmail.com wrote:

Hi Robin,

On Mon, Sep 2, 2019 at 11:45 AM Robin Murphy robin.murphy@arm.com wrote:

...

Try 0036bc73ccbe - that looks like something that CONFIG_DMA_API_DEBUG should have been screaming about anyway.

Thanks for your suggestion.

I can successfully boot after reverting the following commits:

commit 141db5703c887f46957615cd6616ca28fe4691e0 (HEAD) Author: Fabio Estevam festevam@gmail.com Date: Mon Sep 2 14:58:18 2019 -0300

Revert "drm/msm: stop abusing dma_map/unmap for cache"

This reverts commit 0036bc73ccbe7e600a3468bf8e8879b122252274.

commit fa5b1f620f2984c254877d6049214c39c24c8207 Author: Fabio Estevam festevam@gmail.com Date: Mon Sep 2 14:56:01 2019 -0300

Revert "drm/msm: Use the correct dma_sync calls in msm_gem"

This reverts commit 3de433c5b38af49a5fc7602721e2ab5d39f1e69c.

Rob,

What would be the recommended approach for fixing this?

We need a direct way to handle cache, so we can stop trying to trick DMA API into doing what we want.

Something like this is what I had in mind:

https://patchwork.freedesktop.org/series/65211/

I guess I could respin that. I'm not really sure of any other way to have things working on the different combinations of archs and dma_ops that we have. Lately fixing one has been breaking another.

BR, -R

Hi Jonathan,

On Tue, Sep 3, 2019 at 4:25 PM Jonathan Marek jonathan@marek.ca wrote:

Hi,

I tried this and it works with patches 4+5 from Rob's series and changing gpummu to use sg_phys(sg) instead of sg->dma_address (dma_address isn't set now that dma_map_sg isn't used).

Thanks for testing it. I haven't had a chance to test it yet.

Rob,

I assume your series is targeted to 5.4, correct?

If this is the case, what we should do about the i.MX5 regression on 5.3?

Would a revert of the two commits be acceptable in 5.3 in order to avoid the regression?

Please advise.

Thanks

Hi Rob,

On Tue, Sep 3, 2019 at 9:12 PM Rob Clark robdclark@gmail.com wrote:

In the mean time, it is a bit ugly, but I guess something like this should work:

Yes, this works on a i.MX53 board, thanks:

Tested-by: Fabio Estevam festevam@gmail.com

Is this something you could submit for 5.3?

Thanks

On Wed, Sep 4, 2019 at 11:06 AM Robin Murphy robin.murphy@arm.com wrote:

On 04/09/2019 01:12, Rob Clark wrote:

...

On Tue, Sep 3, 2019 at 12:31 PM Fabio Estevam festevam@gmail.com wrote:

...

Hi Jonathan,

On Tue, Sep 3, 2019 at 4:25 PM Jonathan Marek jonathan@marek.ca wrote:

...

Hi,

I tried this and it works with patches 4+5 from Rob's series and changing gpummu to use sg_phys(sg) instead of sg->dma_address (dma_address isn't set now that dma_map_sg isn't used).

Thanks for testing it. I haven't had a chance to test it yet.

Rob,

I assume your series is targeted to 5.4, correct?

maybe, although Christoph Hellwig didn't seem like a big fan of exposing cache ops, and would rather add a new allocation API for uncached pages.. so I'm not entirely sure what the way forward will be.

TBH, the use of map/unmap looked reasonable in the context of "start/stop using these pages for stuff which may include DMA", so even if it was cheekily ignoring sg->dma_address I'm not sure I'd really consider it "abuse" - in comparison, using sync without a prior map unquestionably violates the API, and means that CONFIG_DMA_API_DEBUG will be rendered useless with false positives if this driver is active while trying to debug something else.

The warning referenced in 0036bc73ccbe represents something being unmapped which didn't match a corresponding map - from what I can make of get_pages()/put_pages() it looks like that would need msm_obj->flags or msm_obj->sgt to change during the lifetime of the object, neither of which sounds like a thing that should legitimately happen. Are you sure this isn't all just hiding a subtle bug elsewhere? After all, if what was being unmapped wasn't right, who says that what's now being synced is?

Correct, msm_obj->flags/sgt should not change.

I reverted the various patches, and went back to the original setup that used dma_{map,unmap}_sg() to reproduce the original issue that prompted the change in the first place. It is a pretty massive flood of splats, which pretty quickly overflowed the dmesg ring buffer, so I might be missing some things, but I'll poke around some more.

The one thing I wonder about, what would happen if the buffer is allocated and dma_map_sg() called before drm/msm attaches it's own iommu_domains, and then dma_unmap_sg() afterwards. We aren't actually ever using the iommu domain that DMA API is creating for the device, so all the extra iommu_map/unmap (and tlb flush) is at best unnecessary. But I'm not sure if it could be having some unintended side effects that cause this sort of problem.

BR, -R

On Thu, Sep 5, 2019 at 12:05 PM Rob Clark robdclark@gmail.com wrote:

On Thu, Sep 5, 2019 at 10:03 AM Rob Clark robdclark@gmail.com wrote:

...

On Wed, Sep 4, 2019 at 11:06 AM Robin Murphy robin.murphy@arm.com wrote:

...

On 04/09/2019 01:12, Rob Clark wrote:

...

On Tue, Sep 3, 2019 at 12:31 PM Fabio Estevam festevam@gmail.com wrote:

...

Hi Jonathan,

On Tue, Sep 3, 2019 at 4:25 PM Jonathan Marek jonathan@marek.ca wrote:

...

Hi,

I tried this and it works with patches 4+5 from Rob's series and changing gpummu to use sg_phys(sg) instead of sg->dma_address (dma_address isn't set now that dma_map_sg isn't used).

Thanks for testing it. I haven't had a chance to test it yet.

Rob,

I assume your series is targeted to 5.4, correct?

maybe, although Christoph Hellwig didn't seem like a big fan of exposing cache ops, and would rather add a new allocation API for uncached pages.. so I'm not entirely sure what the way forward will be.

TBH, the use of map/unmap looked reasonable in the context of "start/stop using these pages for stuff which may include DMA", so even if it was cheekily ignoring sg->dma_address I'm not sure I'd really consider it "abuse" - in comparison, using sync without a prior map unquestionably violates the API, and means that CONFIG_DMA_API_DEBUG will be rendered useless with false positives if this driver is active while trying to debug something else.

The warning referenced in 0036bc73ccbe represents something being unmapped which didn't match a corresponding map - from what I can make of get_pages()/put_pages() it looks like that would need msm_obj->flags or msm_obj->sgt to change during the lifetime of the object, neither of which sounds like a thing that should legitimately happen. Are you sure this isn't all just hiding a subtle bug elsewhere? After all, if what was being unmapped wasn't right, who says that what's now being synced is?

Correct, msm_obj->flags/sgt should not change.

I reverted the various patches, and went back to the original setup that used dma_{map,unmap}_sg() to reproduce the original issue that prompted the change in the first place. It is a pretty massive flood of splats, which pretty quickly overflowed the dmesg ring buffer, so I might be missing some things, but I'll poke around some more.

The one thing I wonder about, what would happen if the buffer is allocated and dma_map_sg() called before drm/msm attaches it's own iommu_domains, and then dma_unmap_sg() afterwards. We aren't actually ever using the iommu domain that DMA API is creating for the device, so all the extra iommu_map/unmap (and tlb flush) is at best unnecessary. But I'm not sure if it could be having some unintended side effects that cause this sort of problem.

it seems like every time (or at least every time we splat), we end up w/ iova=fffffffffffff000 .. which doesn't sound likely to be right. Although from just looking at the dma-iommu.c code, I'm not sure how this happens. And adding some printk's results in enough traces that I can't boot for some reason..

Ok, I see better what is going on.. at least on the kernel that I'm using on the yoga c630 laptop, where I have a patch[1] to skip domain attach. That results in to_smmu_domain(domain)->pgtbl_ops being null, so arm_smmu_map() fails. So we skip __finalise_sg() which sets the sg_dma_address(). Which causes the failure on unmap.

That said, I'm pretty sure I've seen (or had reported) a similar splat (although maybe not so frequent) on devices without that patch (where the bootloader isn't enabling scanout). I'll have to switch over to a different device that doesn't light up display from bootloader, so that I can drop that skip-domain-attach patch

All that said, this would be much easier if I could do the cache operations without all this unneeded iommu stuff. (Not to mention the unnecessary TLB flushes that I suspect are also happening.)

[1] https://patchwork.kernel.org/patch/11038793/

BR, -R

On 05/09/2019 20:05, Rob Clark wrote:

On Thu, Sep 5, 2019 at 10:03 AM Rob Clark robdclark@gmail.com wrote:

...

On Wed, Sep 4, 2019 at 11:06 AM Robin Murphy robin.murphy@arm.com wrote:

...

On 04/09/2019 01:12, Rob Clark wrote:

...

On Tue, Sep 3, 2019 at 12:31 PM Fabio Estevam festevam@gmail.com wrote:

...

Hi Jonathan,

On Tue, Sep 3, 2019 at 4:25 PM Jonathan Marek jonathan@marek.ca wrote:

...

Hi,

I tried this and it works with patches 4+5 from Rob's series and changing gpummu to use sg_phys(sg) instead of sg->dma_address (dma_address isn't set now that dma_map_sg isn't used).

Thanks for testing it. I haven't had a chance to test it yet.

Rob,

I assume your series is targeted to 5.4, correct?

maybe, although Christoph Hellwig didn't seem like a big fan of exposing cache ops, and would rather add a new allocation API for uncached pages.. so I'm not entirely sure what the way forward will be.

TBH, the use of map/unmap looked reasonable in the context of "start/stop using these pages for stuff which may include DMA", so even if it was cheekily ignoring sg->dma_address I'm not sure I'd really consider it "abuse" - in comparison, using sync without a prior map unquestionably violates the API, and means that CONFIG_DMA_API_DEBUG will be rendered useless with false positives if this driver is active while trying to debug something else.

The warning referenced in 0036bc73ccbe represents something being unmapped which didn't match a corresponding map - from what I can make of get_pages()/put_pages() it looks like that would need msm_obj->flags or msm_obj->sgt to change during the lifetime of the object, neither of which sounds like a thing that should legitimately happen. Are you sure this isn't all just hiding a subtle bug elsewhere? After all, if what was being unmapped wasn't right, who says that what's now being synced is?

Correct, msm_obj->flags/sgt should not change.

I reverted the various patches, and went back to the original setup that used dma_{map,unmap}_sg() to reproduce the original issue that prompted the change in the first place. It is a pretty massive flood of splats, which pretty quickly overflowed the dmesg ring buffer, so I might be missing some things, but I'll poke around some more.

The one thing I wonder about, what would happen if the buffer is allocated and dma_map_sg() called before drm/msm attaches it's own iommu_domains, and then dma_unmap_sg() afterwards. We aren't actually ever using the iommu domain that DMA API is creating for the device, so all the extra iommu_map/unmap (and tlb flush) is at best unnecessary. But I'm not sure if it could be having some unintended side effects that cause this sort of problem.

Right, one of the semi-intentional side-effects of 43c5bf11a610 is that iommu-dma no longer interferes with unmanaged domains - it will still go and make its own redundant mappings in the unattached default domain, but as long as the DMA API usage is fundamentally sound then it shouldn't actually get in the way.

it seems like every time (or at least every time we splat), we end up w/ iova=fffffffffffff000 .. which doesn't sound likely to be right. Although from just looking at the dma-iommu.c code, I'm not sure how this happens. And adding some printk's results in enough traces that I can't boot for some reason..

Yeah, that's a bogus IOVA for sure, so regardless of how we actually make Adreno happy it would still be interesting to figure out how it came about. Do you see any WARNs from io-pgtable-arm before the one from __iommu_dma_unmap()?

Robin.