Re: [PATCH v4 2/2] dma: add IOMMU static calls with clear default ops

[Leon Romanovsky]

On Tue, Sep 10, 2024 at 03:01:05PM -0400, Nícolas F. R. A. Prado wrote:
> On Wed, Jul 24, 2024 at 09:04:49PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxx>
> > 
> > Most of the arch DMA ops (which often, but not always, involve
> > some sort of IOMMU) are using the same DMA operations, but for all
> > modern platforms dma-iommu implementation is really matters.
> > 
> > So let's make sure to call them directly without need to perform
> > function pointers dereference.
> > 
> > During system initialization, the arch can set its own DMA and in such
> > case, the default DMA operations will be overridden.
> > 
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leon@xxxxxxxxxx>
> 
> Hi,
> 
> KernelCI has identified another regression originating from this patch. It
> affects the same platforms:
> * sc7180-trogdor-kingoftown
> * sc7180-trogdor-lazor-limozeen
> 
> But this time the issue is that the venus video codecs are failing to probe as
> indicated by the DT kselftest:
> 
>   not ok 184 /soc@0/video-codec@aa00000
>   ok 185 /soc@0/video-codec@aa00000/opp-table # SKIP
>   not ok 186 /soc@0/video-codec@aa00000/video-decoder
>   not ok 187 /soc@0/video-codec@aa00000/video-encoder
> 
> The kernel logs show the error:
> 
>   qcom-venus aa00000.video-codec: probe with driver qcom-venus failed with error -5
> 
> A quick ftrace run showed that the error comes from dma_set_mask_and_coherent()
> in venus_probe():
> 
>   7)               |  venus_probe() {
>   ...
>   7)               |    dma_set_mask() {
>   7)               |      dma_supported() {
>   7)   0.989 us    |        dma_direct_supported(); /* = 0x0 */
>   7)   2.864 us    |      } /* dma_supported = 0x0 */
>   7)   4.636 us    |    } /* dma_set_mask = -5 */
> 
> For comparison, here is the ftrace run with the commit reverted:
> 
>   7)               |  venus_probe() {
>   ...
>   7)   1.093 us    |    dma_set_mask(); /* = 0x0 */
>   7)   1.041 us    |    dma_set_coherent_mask(); /* = 0x0 */
> 
> The issue is still present as of next-20240909 and reverting this commit fixes
> it.
> 
> Happy to provide any other details necessary.

Thanks for the report, I'm looking into it. However, it is unclear to me
why my patch is causing this issue. The change in dma_supported() should
produce WARN_ON [1] if new path is taken, otherwise, we return to
previous behavior.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/dma/mapping.c#n822

> 
> Please add
> Reported-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx> #KernelCI
> when fixing this.
> 
> #regzbot introduced: next-20240822..20240823
> #regzbot title: Venus codec probe regression for sc7180 platforms in dma_set_mask_and_coherent()
> 
> Thanks,
> Nícolas