On 9/20/22 7:05 PM, Sean Anderson wrote:
>
>
> On 9/20/22 6:49 PM, Leo Li wrote:
>>
>>
>>> -----Original Message-----
>>> From: Sean Anderson <sean.anderson(a)seco.com>
>>> Sent: Tuesday, September 20, 2022 11:21 AM
>>> To: Robin Murphy <robin.murphy(a)arm.com>; Oleksij Rempel
>>> <linux(a)rempel-privat.de>; Pengutronix Kernel Team
>>> <kernel(a)pengutronix.de>; linux-i2c(a)vger.kernel.org; linux-arm-kernel
>>> <linux-arm-kernel(a)lists.infradead.org>; Vinod Koul <vkoul(a)kernel.org>;
>>> dmaengine(a)vger.kernel.org; Leo Li <leoyang.li(a)nxp.com>; Laurentiu Tudor
>>> <laurentiu.tudor(a)nxp.com>
>>> Cc: Linux Kernel Mailing List <linux-kernel(a)vger.kernel.org>; dri-
>>> devel(a)lists.freedesktop.org; Christian König <christian.koenig(a)amd.com>;
>>> linaro-mm-sig(a)lists.linaro.org; Shawn Guo <shawnguo(a)kernel.org>; Sumit
>>> Semwal <sumit.semwal(a)linaro.org>; Joy Zou <joy.zou(a)nxp.com>; linux-
>>> media(a)vger.kernel.org
>>> Subject: Re: [BUG] ls1046a: eDMA does not transfer data from I2C
>>>
>>>
>>>
>>> On 9/20/22 11:44 AM, Sean Anderson wrote:
>>> >
>>> >
>>> > On 9/20/22 11:24 AM, Sean Anderson wrote:
>>> >>
>>> >>
>>> >> On 9/20/22 6:07 AM, Robin Murphy wrote:
>>> >>> On 2022-09-19 23:24, Sean Anderson wrote:
>>> >>>> Hi all,
>>> >>>>
>>> >>>> I discovered a bug in either imx_i2c or fsl-edma on the LS1046A
>>> >>>> where no data is read in i2c_imx_dma_read except for the last two
>>> >>>> bytes (which are not read using DMA). This is perhaps best
>>> >>>> illustrated with the following example:
>>> >>>>
>>> >>>> # hexdump -C /sys/bus/nvmem/devices/0-00540/nvmem
>>> >>>> [ 308.914884] i2c i2c-0: ffff000809380000 0x0000000889380000
>>> 0x00000000f5401000 ffff000075401000
>>> >>>> [ 308.923529] src= 2180004 dst=f5401000 attr= 0 soff= 0 nbytes=1
>>> slast= 0
>>> >>>> [ 308.923529] citer= 7e biter= 7e doff= 1 dlast_sga= 0
>>> >>>> [ 308.923529] major_int=1 disable_req=1 enable_sg=0 [ 308.942113]
>>> >>>> fsl-edma 2c00000.edma: vchan 000000001b4371fc: txd
>>> >>>> 00000000d9dd26c5[4]: submitted [ 308.974049] fsl-edma
>>> >>>> 2c00000.edma: txd 00000000d9dd26c5[4]: marked complete [
>>> >>>> 308.981339] i2c i2c-0: ffff000809380000 = [2e 2e 2f 2e 2e 2f 2e 2e
>>> >>>> 2f 64 65 76 69 63 65 73 2f 70 6c 61 74 66 6f 72 6d 2f 73 6f 63 2f 32 31 38 30
>>> 30 30 30 2e 69 32 63 2f 69 32 63 2d 30 2f 30 2d 30 30 35 34 2f 30 2d 30 30 35 34
>>> 30 00 00] [ 309.002226] i2c i2c-0: ffff000075401000 = [2e 2e 2f 2e 2e 2f 2e 2e 2f
>>> 64 65 76 69 63 65 73 2f 70 6c 61 74 66 6f 72 6d 2f 73 6f 63 2f 32 31 38 30 30 30 30
>>> 2e 69 32 63 2f 69 32 63 2d 30 2f 30 2d 30 30 35 34 2f 30 2d 30 30 35 34 30 00 00]
>>> [ 309.024649] i2c i2c-0: ffff000809380080 0x0000000889380080
>>> 0x00000000f5401800 ffff000075401800
>>> >>>> [ 309.033270] src= 2180004 dst=f5401800 attr= 0 soff= 0 nbytes=1
>>> slast= 0
>>> >>>> [ 309.033270] citer= 7e biter= 7e doff= 1 dlast_sga= 0
>>> >>>> [ 309.033270] major_int=1 disable_req=1 enable_sg=0 [ 309.051633]
>>> >>>> fsl-edma 2c00000.edma: vchan 000000001b4371fc: txd
>>> >>>> 00000000d9dd26c5[5]: submitted [ 309.083526] fsl-edma
>>> >>>> 2c00000.edma: txd 00000000d9dd26c5[5]: marked complete [
>>> >>>> 309.090807] i2c i2c-0: ffff000809380080 = [00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00 00 00 00 00 00 00 00 00] [ 309.111694] i2c i2c-0:
>>> >>>> ffff000075401800 = [00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00]
>>> >>>> 00000000 2e 2e 2f 2e 2e 2f 2e 2e 2f 64 65 76 69 63 65 73
>>> >>>> |../../../devices|
>>> >>>> 00000010 2f 70 6c 61 74 66 6f 72 6d 2f 73 6f 63 2f 32 31
>>> >>>> |/platform/soc/21|
>>> >>>> 00000020 38 30 30 30 30 2e 69 32 63 2f 69 32 63 2d 30 2f
>>> >>>> |80000.i2c/i2c-0/|
>>> >>>> 00000030 30 2d 30 30 35 34 2f 30 2d 30 30 35 34 30 00 00
>>> >>>> |0-0054/0-00540..|
>>> >>>> 00000040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> |................|
>>> >>>> *
>>> >>>> 00000070 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ff ff
>>> >>>> |................|
>>> >>>> 00000080 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> |................|
>>> >>>> *
>>> >>>> 000000f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ff 5b
>>> >>>> |...............[|
>>> >>>> 00000100
>>> >>>>
>>> >>>> (patch with my debug prints appended below)
>>> >>>>
>>> >>>> Despite the DMA completing successfully, no data was copied into
>>> >>>> the buffer, leaving the original (now junk) contents. I probed the
>>> >>>> I2C bus with an oscilloscope, and I verified that the transfer did indeed
>>> occur.
>>> >>>> The timing between submission and completion seems reasonable for
>>> >>>> the bus speed (50 kHz for whatever reason).
>>> >>>>
>>> >>>> I had a look over the I2C driver, and nothing looked obviously
>>> >>>> incorrect. If anyone has ideas on what to try, I'm more than willing.
>>> >>>
>>> >>> Is the DMA controller cache-coherent? I see the mainline LS1046A DT
>>> doesn't have a "dma-coherent" property for it, but the behaviour is entirely
>>> consistent with that being wrong - dma_map_single() cleans the cache,
>>> coherent DMA write hits the still-present cache lines, dma_unmap_single()
>>> invalidates the cache, and boom, the data is gone and you read back the
>>> previous content of the buffer that was cleaned out to DRAM beforehand.
>>> >>
>>> >> I've tried both with and without [1] applied. I also tried removing
>>> >> the call to dma_unmap_single, but to no effect.
>>> >
>>> > Actually, I wasn't updating my device tree like I thought...
>>> >
>>> > Turns out I2C works only *without* this patch.
>>> >
>>> > So maybe the eDMA is not coherent?
>>>
>>> It seems like this might be the case. From the reference manual:
>>>
>>> > All transactions from eDMA are tagged as snoop configuration if the
>>> > SCFG_SNPCNFGCR[eDMASNP] bit is set. Refer Snoop Configuration
>>> Register
>>> > (SCFG_SNPCNFGCR) for details.
>>>
>>> But there is no such bit in this register on the LS1046A. On the LS1043A, this
>>> bit does exist, but on the LS1046A it is reserved. I read the register, and
>>> found the bit was 0. Perhaps eDMA was intended to be coherent, but there
>>> was a hardware bug?
>>
>> Thanks for the findings. I don't know the reason why this bit is reserved on LS1046a either. It should have a similar design as LS1043a.
>
> Funnily enough, this bit is not set for the LS1043A either [1]. So maybe
> this is a U-Boot bug? I'll test this tomorrow.
OK, this looks like it fixes things. I'll submit a patch to U-Boot. But
this bit should really be documented in the LS1046A manual as well.
--Sean
> [1] https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/cpu/armv8/fsl-l…
>
>>>
>>> If this is the case, then I think dma-coherent should be left out of the top-
>>> level /soc node. Instead, the qdma, sata, usb, and emmc nodes should have
>>> dma-coherent added.
>>>
>>> Li/Laurentiu, what are your thoughts?
>>
>> Then looks like it is not correct to say all devices on the bus is coherent. But as we have the new "dma-noncoherent" property now and most of the devices are actually coherent, we probably can keep the bus as dma-coherent and mark exceptions with dma-noncoherent?
>
> Neat, I didn't know about that.
>
> Yes, that sounds good. For the moment, only i2c0 uses DMA, so we will
> only need it there. At some point, someone should send a patch enabling
> it for the rest of the I2C devices, as well as the LPUARTs and SPIs.
Hello,
This series moves all drivers to a dynamic dma-buf locking specification.
From now on all dma-buf importers are made responsible for holding
dma-buf's reservation lock around all operations performed over dma-bufs
in accordance to the locking specification. This allows us to utilize
reservation lock more broadly around kernel without fearing of a potential
deadlocks.
This patchset passes all i915 selftests. It was also tested using VirtIO,
Panfrost, Lima, Tegra, udmabuf, AMDGPU and Nouveau drivers. I tested cases
of display+GPU, display+V4L and GPU+V4L dma-buf sharing (where appropriate),
which covers majority of kernel drivers since rest of the drivers share
same or similar code paths.
Changelog:
v5: - Added acks and r-bs that were given to v4.
- Changed i915 preparation patch like was suggested by Michael Ruhl.
The scope of reservation locking is smaller now.
v4: - Added dma_buf_mmap() to the "locking convention" documentation,
which was missed by accident in v3.
- Added acks from Christian König, Tomasz Figa and Hans Verkuil that
they gave to couple v3 patches.
- Dropped the "_unlocked" postfix from function names that don't have
the locked variant, as was requested by Christian König.
- Factored out the per-driver preparations into separate patches
to ease reviewing of the changes, which is now doable without the
global dma-buf functions renaming.
- Factored out the dynamic locking convention enforcements into separate
patches which add the final dma_resv_assert_held(dmabuf->resv) to the
dma-buf API functions.
v3: - Factored out dma_buf_mmap_unlocked() and attachment functions
into aseparate patches, like was suggested by Christian König.
- Corrected and factored out dma-buf locking documentation into
a separate patch, like was suggested by Christian König.
- Intel driver dropped the reservation locking fews days ago from
its BO-release code path, but we need that locking for the imported
GEMs because in the end that code path unmaps the imported GEM.
So I added back the locking needed by the imported GEMs, updating
the "dma-buf attachment locking specification" patch appropriately.
- Tested Nouveau+Intel dma-buf import/export combo.
- Tested udmabuf import to i915/Nouveau/AMDGPU.
- Fixed few places in Etnaviv, Panfrost and Lima drivers that I missed
to switch to locked dma-buf vmapping in the drm/gem: Take reservation
lock for vmap/vunmap operations" patch. In a result invalidated the
Christian's r-b that he gave to v2.
- Added locked dma-buf vmap/vunmap functions that are needed for fixing
vmappping of Etnaviv, Panfrost and Lima drivers mentioned above.
I actually had this change stashed for the drm-shmem shrinker patchset,
but then realized that it's already needed by the dma-buf patches.
Also improved my tests to better cover these code paths.
v2: - Changed locking specification to avoid problems with a cross-driver
ww locking, like was suggested by Christian König. Now the attach/detach
callbacks are invoked without the held lock and exporter should take the
lock.
- Added "locking convention" documentation that explains which dma-buf
functions and callbacks are locked/unlocked for importers and exporters,
which was requested by Christian König.
- Added ack from Tomasz Figa to the V4L patches that he gave to v1.
Dmitry Osipenko (21):
dma-buf: Add unlocked variant of vmapping functions
dma-buf: Add unlocked variant of attachment-mapping functions
drm/gem: Take reservation lock for vmap/vunmap operations
drm/prime: Prepare to dynamic dma-buf locking specification
drm/armada: Prepare to dynamic dma-buf locking specification
drm/i915: Prepare to dynamic dma-buf locking specification
drm/omapdrm: Prepare to dynamic dma-buf locking specification
drm/tegra: Prepare to dynamic dma-buf locking specification
drm/etnaviv: Prepare to dynamic dma-buf locking specification
RDMA/umem: Prepare to dynamic dma-buf locking specification
misc: fastrpc: Prepare to dynamic dma-buf locking specification
xen/gntdev: Prepare to dynamic dma-buf locking specification
media: videobuf2: Prepare to dynamic dma-buf locking specification
media: tegra-vde: Prepare to dynamic dma-buf locking specification
dma-buf: Move dma_buf_vmap() to dynamic locking specification
dma-buf: Move dma_buf_attach() to dynamic locking specification
dma-buf: Move dma_buf_map_attachment() to dynamic locking
specification
dma-buf: Move dma_buf_mmap() to dynamic locking specification
dma-buf: Document dynamic locking convention
media: videobuf2: Stop using internal dma-buf lock
dma-buf: Remove obsoleted internal lock
Documentation/driver-api/dma-buf.rst | 6 +
drivers/dma-buf/dma-buf.c | 211 +++++++++++++++---
drivers/gpu/drm/armada/armada_gem.c | 8 +-
drivers/gpu/drm/drm_client.c | 4 +-
drivers/gpu/drm/drm_gem.c | 24 ++
drivers/gpu/drm/drm_gem_dma_helper.c | 6 +-
drivers/gpu/drm/drm_gem_framebuffer_helper.c | 6 +-
drivers/gpu/drm/drm_gem_ttm_helper.c | 9 +-
drivers/gpu/drm/drm_prime.c | 6 +-
drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_object.c | 14 ++
.../drm/i915/gem/selftests/i915_gem_dmabuf.c | 16 +-
drivers/gpu/drm/lima/lima_sched.c | 4 +-
drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 4 +-
drivers/gpu/drm/panfrost/panfrost_dump.c | 4 +-
drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 6 +-
drivers/gpu/drm/qxl/qxl_object.c | 17 +-
drivers/gpu/drm/qxl/qxl_prime.c | 4 +-
drivers/gpu/drm/tegra/gem.c | 17 +-
drivers/infiniband/core/umem_dmabuf.c | 7 +-
.../common/videobuf2/videobuf2-dma-contig.c | 22 +-
.../media/common/videobuf2/videobuf2-dma-sg.c | 19 +-
.../common/videobuf2/videobuf2-vmalloc.c | 17 +-
.../platform/nvidia/tegra-vde/dmabuf-cache.c | 6 +-
drivers/misc/fastrpc.c | 6 +-
drivers/xen/gntdev-dmabuf.c | 8 +-
include/drm/drm_gem.h | 3 +
include/linux/dma-buf.h | 17 +-
29 files changed, 320 insertions(+), 155 deletions(-)
--
2.37.3
Thanks for pointing this out! It's indeed quite a bug.
Going to send a patch ASAP.
Regards,
Christian.
Am 20.09.22 um 13:47 schrieb Sudip Mukherjee:
> Hi All,
>
> The builds of arm64 allmodconfig with clang failed to build next-20220920
> with the error:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:1190:3: error: variable 'r' is uninitialized when used here [-Werror,-Wuninitialized]
> r |= !amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
> ^
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:1171:7: note: initialize the variable 'r' to silence this warning
> int r;
> ^
> = 0
> 1 error generated.
>
>
> git bisect pointed to c2b08e7a6d27 ("drm/amdgpu: move entity selection and job init earlier during CS")
>
> I will be happy to test any patch or provide any extra log if needed.
>
>
Fence signaling must be enabled to make sure that
the dma_fence_is_signaled() function ever returns true.
Since drivers and implementations sometimes mess this up,
this ensures correct behaviour when DEBUG_WW_MUTEX_SLOWPATH
is used during debugging.
This should make any implementation bugs resulting in not
signaled fences much more obvious.
Arvind Yadav (6):
[PATCH v4 1/6] dma-buf: Remove the signaled bit status check
[PATCH v4 2/6] dma-buf: set signaling bit for the stub fence
[PATCH v4 3/6] dma-buf: Enable signaling on fence for selftests
[PATCH v4 4/6] dma-buf: dma_fence_wait must enable signaling
[PATCH v4 5/6] drm/sched: Use parent fence instead of finished
[PATCH v4 6/6] dma-buf: Check status of enable-signaling bit on debug
drivers/dma-buf/Kconfig | 7 +++++++
drivers/dma-buf/dma-fence.c | 16 ++++++++++------
drivers/dma-buf/st-dma-fence-chain.c | 4 ++++
drivers/dma-buf/st-dma-fence-unwrap.c | 22 ++++++++++++++++++++++
drivers/dma-buf/st-dma-fence.c | 16 ++++++++++++++++
drivers/dma-buf/st-dma-resv.c | 10 ++++++++++
drivers/gpu/drm/scheduler/sched_main.c | 4 ++--
include/linux/dma-fence.h | 5 +++++
8 files changed, 76 insertions(+), 8 deletions(-)
--
2.25.1
Fence signaling must be enabled to make sure that
the dma_fence_is_signaled() function ever returns true.
Since drivers and implementations sometimes mess this up,
this ensures correct behaviour when DEBUG_WW_MUTEX_SLOWPATH
is used during debugging.
This should make any implementation bugs resulting in not
signaled fences much more obvious.
Arvind Yadav (6):
[PATCH v3 1/6] dma-buf: Remove the signaled bit status check
[PATCH v3 2/6] dma-buf: set signaling bit for the stub fence
[PATCH v3 3/6] dma-buf: Enable signaling on fence for selftests
[PATCH v3 4/6] drm/amdgpu: Enable signaling on fence.
[PATCH v3 5/6] drm/sched: Use parent fence instead of finished
[PATCH v3 6/6] dma-buf: Check status of enable-signaling bit on debug
drivers/dma-buf/dma-fence.c | 7 ++++---
drivers/dma-buf/st-dma-fence-chain.c | 4 ++++
drivers/dma-buf/st-dma-fence-unwrap.c | 22 ++++++++++++++++++++++
drivers/dma-buf/st-dma-fence.c | 16 ++++++++++++++++
drivers/dma-buf/st-dma-resv.c | 10 ++++++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 2 ++
drivers/gpu/drm/scheduler/sched_main.c | 4 ++--
include/linux/dma-fence.h | 5 +++++
8 files changed, 65 insertions(+), 5 deletions(-)
--
2.25.1