-----Original Message----- From: Christoph Hellwig [mailto:hch@lst.de] Sent: Sunday, November 15, 2020 9:45 PM To: Song Bao Hua (Barry Song) song.bao.hua@hisilicon.com Cc: Christoph Hellwig hch@lst.de; iommu@lists.linux-foundation.org; robin.murphy@arm.com; m.szyprowski@samsung.com; Linuxarm linuxarm@huawei.com; linux-kselftest@vger.kernel.org; xuwei (O) xuwei5@huawei.com; Joerg Roedel joro@8bytes.org; Will Deacon will@kernel.org; Shuah Khan shuah@kernel.org Subject: Re: [PATCH v3 1/2] dma-mapping: add benchmark support for streaming DMA APIs
On Sun, Nov 15, 2020 at 12:11:15AM +0000, Song Bao Hua (Barry Song) wrote:
Checkpatch has changed 80 to 100. That's probably why my local checkpatch
didn't report any warning:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... bdc48fa11e46f867ea4d
I am happy to change them to be less than 80 if you like.
Don't rely on checkpath, is is broken. Look at the codingstyle document.
I think this needs to set a dma mask as behavior for unlimited dma mask vs the default 32-bit one can be very different.
I actually prefer users bind real devices with real dma_mask to test rather
than force to change
the dma_mask in this benchmark.
The mask is set by the driver, not the device. So you need to set when when you bind, real device or not.
Yep while it is a little bit tricky.
Sometimes, it is done by "device" in architectures, e.g. there are lots of dma_mask configuration code in arch/arm/mach-xxx. arch/arm/mach-davinci/da850.c static u64 da850_vpif_dma_mask = DMA_BIT_MASK(32); static struct platform_device da850_vpif_dev = { .name = "vpif", .id = -1, .dev = { .dma_mask = &da850_vpif_dma_mask, .coherent_dma_mask = DMA_BIT_MASK(32), }, .resource = da850_vpif_resource, .num_resources = ARRAY_SIZE(da850_vpif_resource), };
Sometimes, it is done by "of" or "acpi", for example: drivers/acpi/arm64/iort.c void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) { u64 end, mask, dmaaddr = 0, size = 0, offset = 0; int ret;
...
ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); if (!ret) { /* * Limit coherent and dma mask based on size retrieved from * firmware. */ end = dmaaddr + size - 1; mask = DMA_BIT_MASK(ilog2(end) + 1); dev->bus_dma_limit = end; dev->coherent_dma_mask = mask; *dev->dma_mask = mask; } ... }
Sometimes, it is done by "bus", for example, ISA: isa_dev->dev.coherent_dma_mask = DMA_BIT_MASK(24); isa_dev->dev.dma_mask = &isa_dev->dev.coherent_dma_mask;
error = device_register(&isa_dev->dev); if (error) { put_device(&isa_dev->dev); break; }
And in many cases, it is done by driver. On the ARM64 server platform I am testing, actually rarely drivers set dma_mask.
So to make the dma benchmark work on all platforms, it seems it is worth to add a dma_mask_bit parameter. But, in order to avoid breaking the dma_mask of those devices whose dma_mask are set by architectures, acpi and bus, it seems we need to do the below in dma_benchmark:
u64 old_mask;
old_mask = dma_get_mask(dev);
dma_set_mask(dev, &new_mask);
do_map_benchmark();
/* restore old dma_mask so that the dma_mask of the device is not changed due to benchmark when it is bound back to its original driver */ dma_set_mask(dev, &old_mask);
Thanks Barry