Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
On Fri, Jun 23, 2023 at 04:38:34AM +0800, Sui Jingfeng wrote: > On 2023/6/8 15:39, Maxime Ripard wrote: > > On Thu, Jun 08, 2023 at 01:18:38AM +0800, Sui Jingfeng wrote: > > > Hi, > > > > > > On 2023/6/8 00:12, Paul Cercueil wrote: > > > > Hi Sui, > > > > > > > > Le mercredi 07 juin 2023 à 22:38 +0800, Sui Jingfeng a écrit : > > > > > Hi, welcome to discussion. > > > > > > > > > > > > > > > I have limited skills in manipulating English. > > > > > > > > > > It may not express what I'm really means in the short time. > > > > > > > > > > Part of word in the sentence may not as accurate as your. > > > > > > > > > > Well, please don't misunderstand, I'm not doing the rude to you. > > > > No problem. > > > > > > > > > I will explain it with more details. > > > > > > > > > > See below: > > > > > > > > > > > > > > > On 2023/6/7 20:09, Paul Cercueil wrote: > > > > > > Hi Sui, > > > > > > > > > > > > Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > On 2023/6/7 17:36, Paul Cercueil wrote: > > > > > > > > Hi Sui, > > > > > > > > > > > > > > > > Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : > > > > > > > > > The single map_noncoherent member of struct > > > > > > > > > drm_gem_dma_object > > > > > > > > > may > > > > > > > > > not > > > > > > > > > sufficient for describing the backing memory of the GEM > > > > > > > > > buffer > > > > > > > > > object. > > > > > > > > > > > > > > > > > > Especially on dma-coherent systems, the backing memory is > > > > > > > > > both > > > > > > > > > cached > > > > > > > > > coherent for multi-core CPUs and dma-coherent for peripheral > > > > > > > > > device. > > > > > > > > > Say architectures like X86-64, LoongArch64, Loongson Mips64, > > > > > > > > > etc. > > > > > > > > > > > > > > > > > > Whether a peripheral device is dma-coherent or not can be > > > > > > > > > implementation-dependent. The single map_noncoherent option > > > > > > > > > is > > > > > > > > > not > > > > > > > > > enough > > > > > > > > > to reflect real hardware anymore. For example, the Loongson > > > > > > > > > LS3A4000 > > > > > > > > > CPU > > > > > > > > > and LS2K2000/LS2K1000 SoC, peripheral device of such hardware > > > > > > > > > platform > > > > > > > > > allways snoop CPU's cache. Doing the allocation with > > > > > > > > > dma_alloc_coherent > > > > > > > > > function is preferred. The return buffer is cached, it should > > > > > > > > > not > > > > > > > > > using > > > > > > > > > the default write-combine mapping. While with the current > > > > > > > > > implement, > > > > > > > > > there > > > > > > > > > no way to tell the drm core to reflect this. > > > > > > > > > > > > > > > > > > This patch adds cached and coherent members to struct > > > > > > > > > drm_gem_dma_object. > > > > > > > > > which allow driver implements to inform the core. Introducing > > > > > > > > > new > > > > > > > > > mappings > > > > > > > > > while keeping the original default behavior unchanged. > > > > > > > > Did you try to simply set the "dma-coherent" property to the > > > > > > > > device's > > > > > > > > node? > > > > > > > But this approach can only be applied for the device driver with > > > > > > > DT > > > > > > > support. > > > > > > > > > > > > > > X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically > > > > > > > do > > > > > > > not > > > > > > > have DT support. > > > > > > > > > > > > > > They using ACPI to pass parameter from the firmware to Linux > > > > > > > kernel. > > > > > > > > > > > > > > You approach will lost the effectiveness on such a case. > > > > > > Well, I don't really know how ACPI handles it - but it should just > > > > > > be a > > > > > > matter of setting dev->dma_coherent. That's basically what the DT > > > > > > code > > > > > > does. > > > > > > > > > > > > Some MIPS boards set it in their setup code for instance. > > > > > > > > > > > This is a *strategy*, not a *mechanism*. > > > > > > > > > > In this case, DT is just used to describing the hardware. > > > > > > > > > > (It is actually a hardware feature describing language, the > > > > > granularity > > > > > is large) > > > > > > > > > > It does not changing the state of the hardware. > > > > > > > > > > It's your platform firmware or kernel setting up code who actually do > > > > > such a things. > > > > > > > > > > > > > > > It's just that it works on *one* platform, it does not guarantee it > > > > > will > > > > > works on others. > > > > If you add the "dma-coherent" property in a device node in DT, you > > > > effectively specify that the device is DMA-coherent; so you describe > > > > the hardware, which is what DT is for, and you are not changing the > > > > state of the hardware. > > > > > > > > Note that some MIPS platforms (arch/mips/alchemy/common/setup.c) > > > > default to DMA-coherent mapping; I believe you could do something > > > > similar with your Loongson LS3A4000
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi, On 2023/6/8 15:39, Maxime Ripard wrote: On Thu, Jun 08, 2023 at 01:18:38AM +0800, Sui Jingfeng wrote: Hi, On 2023/6/8 00:12, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 22:38 +0800, Sui Jingfeng a écrit : Hi, welcome to discussion. I have limited skills in manipulating English. It may not express what I'm really means in the short time. Part of word in the sentence may not as accurate as your. Well, please don't misunderstand, I'm not doing the rude to you. No problem. I will explain it with more details. See below: On 2023/6/7 20:09, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : Hi, On 2023/6/7 17:36, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : The single map_noncoherent member of struct drm_gem_dma_object may not sufficient for describing the backing memory of the GEM buffer object. Especially on dma-coherent systems, the backing memory is both cached coherent for multi-core CPUs and dma-coherent for peripheral device. Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. Whether a peripheral device is dma-coherent or not can be implementation-dependent. The single map_noncoherent option is not enough to reflect real hardware anymore. For example, the Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC, peripheral device of such hardware platform allways snoop CPU's cache. Doing the allocation with dma_alloc_coherent function is preferred. The return buffer is cached, it should not using the default write-combine mapping. While with the current implement, there no way to tell the drm core to reflect this. This patch adds cached and coherent members to struct drm_gem_dma_object. which allow driver implements to inform the core. Introducing new mappings while keeping the original default behavior unchanged. Did you try to simply set the "dma-coherent" property to the device's node? But this approach can only be applied for the device driver with DT support. X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not have DT support. They using ACPI to pass parameter from the firmware to Linux kernel. You approach will lost the effectiveness on such a case. Well, I don't really know how ACPI handles it - but it should just be a matter of setting dev->dma_coherent. That's basically what the DT code does. Some MIPS boards set it in their setup code for instance. This is a *strategy*, not a *mechanism*. In this case, DT is just used to describing the hardware. (It is actually a hardware feature describing language, the granularity is large) It does not changing the state of the hardware. It's your platform firmware or kernel setting up code who actually do such a things. It's just that it works on *one* platform, it does not guarantee it will works on others. If you add the "dma-coherent" property in a device node in DT, you effectively specify that the device is DMA-coherent; so you describe the hardware, which is what DT is for, and you are not changing the state of the hardware. Note that some MIPS platforms (arch/mips/alchemy/common/setup.c) default to DMA-coherent mapping; I believe you could do something similar with your Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC. The preblem is that device driver can have various demand. It probably want to create different kind of buffers for different thing simultaneously. Say, one allocated with dma_alloc_coherent for command buffer or dma descriptor another one allocated with dma_alloc_wc for uploading shader etc. also has the third one allocated with dma_alloc_noncoherent() for doing some else. And it will work just fine. struct device dma_coherent, or DT's dma-coherent property define that the device doesn't need any kind of cache maintenance, ever. If it's missing, we need to perform cache maintenance to keep coherency. dma_alloc_* functions provide guarantees to the driver. With dma_alloc_wc and dma_alloc_coherent, the buffer is coherent, and thus you don't need to perform cache maintenance operations by hand in the driver. With dma_alloc_noncoherent, the buffer is non-coherent and the driver needs to perform them when relevant. How those buffers are created is platform specific, but the guarantees provided *to the driver* are always there. A buffer allocated with dma_alloc_coherent might be provided by different means (at the hardware level with a coherency unit, by mapping it non-cacheable), but as far as the driver is concerned it's always going to be coherent. Similarly, a driver using dma_alloc_noncoherent will always require cache maintenance operations to use the API properly, even if the hardware provides coherency (in which case, those operations will be nop). So, yeah, like I was saying in the other mail, it looks like you're confusing a bunch of things. dma_alloc_* functions are about the driver expectations and guarantees. DT's dma-coherent property is
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi, On 2023/6/8 00:12, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 22:38 +0800, Sui Jingfeng a écrit : Hi, welcome to discussion. I have limited skills in manipulating English. It may not express what I'm really means in the short time. Part of word in the sentence may not as accurate as your. Well, please don't misunderstand, I'm not doing the rude to you. No problem. I will explain it with more details. See below: On 2023/6/7 20:09, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : Hi, On 2023/6/7 17:36, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : The single map_noncoherent member of struct drm_gem_dma_object may not sufficient for describing the backing memory of the GEM buffer object. Especially on dma-coherent systems, the backing memory is both cached coherent for multi-core CPUs and dma-coherent for peripheral device. Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. Whether a peripheral device is dma-coherent or not can be implementation-dependent. The single map_noncoherent option is not enough to reflect real hardware anymore. For example, the Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC, peripheral device of such hardware platform allways snoop CPU's cache. Doing the allocation with dma_alloc_coherent function is preferred. The return buffer is cached, it should not using the default write-combine mapping. While with the current implement, there no way to tell the drm core to reflect this. This patch adds cached and coherent members to struct drm_gem_dma_object. which allow driver implements to inform the core. Introducing new mappings while keeping the original default behavior unchanged. Did you try to simply set the "dma-coherent" property to the device's node? But this approach can only be applied for the device driver with DT support. X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not have DT support. They using ACPI to pass parameter from the firmware to Linux kernel. You approach will lost the effectiveness on such a case. Well, I don't really know how ACPI handles it - but it should just be a matter of setting dev->dma_coherent. That's basically what the DT code does. Some MIPS boards set it in their setup code for instance. This is a *strategy*, not a *mechanism*. In this case, DT is just used to describing the hardware. (It is actually a hardware feature describing language, the granularity is large) It does not changing the state of the hardware. It's your platform firmware or kernel setting up code who actually do such a things. It's just that it works on *one* platform, it does not guarantee it will works on others. If you add the "dma-coherent" property in a device node in DT, you effectively specify that the device is DMA-coherent; so you describe the hardware, which is what DT is for, and you are not changing the state of the hardware. Note that some MIPS platforms (arch/mips/alchemy/common/setup.c) default to DMA-coherent mapping; I believe you could do something similar with your Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC. While my patch is trying to create a *mechanism* which could probably works on all platform. It is based the patch you have already commuted. Thanks for your excellent contribution. From what I understand if you add that property then Linux will use DMA coherent memory even though you use dma_alloc_noncoherent() and the sync_single_for_cpu() / sync_single_for_device() are then NOPs. Please do not mitigate the problems with confusing method. This approach not only tend to generate confusion but also implement-dependent and arch-dependent. It's definitely problematic. How does the dma_alloc_coherent/dma_alloc_noncoherent is a ARCH specific thing. Dependent on how does the arch_dma_ops is implemented. The definition of the coherent on different ARCH has different meanings. The definition of the wirte-combine on different ARCH has different meanings. The wirte-combine(uncache acceleration) on mips is non dma- coherent. It is dma-coherent on Ingenic SoCs. It is dma-coherent ? How does it achieve it? As far as I know, there is a write buffer within the mips cpu. typically 64 byte, but it is not cache. It will gather the CPU write access, When a peripheral device do the DMA, how does you platform guarantee the data in the CPU write buffer has been already arrived at (or flushed out to) the system RAM? Does the peripheral device snoop the CPU's write buffer, or it need manually flush the write buffer with SYNC instruction? I believe the DMA flushes the write buffer? I don't actually know the details, it would be something to ask to Ingenic. But on arm, It seem that wirte-combine is coherent. (guaranteed by arch implement). I also heard using dma_alloc_coherent to allocation the buffer for the non-coherent doesn't hurt, but the
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
On Thu, Jun 08, 2023 at 04:17:52PM +0800, Sui Jingfeng wrote: > Hi, > > On 2023/6/8 15:39, Maxime Ripard wrote: > > On Thu, Jun 08, 2023 at 01:18:38AM +0800, Sui Jingfeng wrote: > > > Hi, > > > > > > On 2023/6/8 00:12, Paul Cercueil wrote: > > > > Hi Sui, > > > > > > > > Le mercredi 07 juin 2023 à 22:38 +0800, Sui Jingfeng a écrit : > > > > > Hi, welcome to discussion. > > > > > > > > > > > > > > > I have limited skills in manipulating English. > > > > > > > > > > It may not express what I'm really means in the short time. > > > > > > > > > > Part of word in the sentence may not as accurate as your. > > > > > > > > > > Well, please don't misunderstand, I'm not doing the rude to you. > > > > No problem. > > > > > > > > > I will explain it with more details. > > > > > > > > > > See below: > > > > > > > > > > > > > > > On 2023/6/7 20:09, Paul Cercueil wrote: > > > > > > Hi Sui, > > > > > > > > > > > > Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > On 2023/6/7 17:36, Paul Cercueil wrote: > > > > > > > > Hi Sui, > > > > > > > > > > > > > > > > Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : > > > > > > > > > The single map_noncoherent member of struct > > > > > > > > > drm_gem_dma_object > > > > > > > > > may > > > > > > > > > not > > > > > > > > > sufficient for describing the backing memory of the GEM > > > > > > > > > buffer > > > > > > > > > object. > > > > > > > > > > > > > > > > > > Especially on dma-coherent systems, the backing memory is > > > > > > > > > both > > > > > > > > > cached > > > > > > > > > coherent for multi-core CPUs and dma-coherent for peripheral > > > > > > > > > device. > > > > > > > > > Say architectures like X86-64, LoongArch64, Loongson Mips64, > > > > > > > > > etc. > > > > > > > > > > > > > > > > > > Whether a peripheral device is dma-coherent or not can be > > > > > > > > > implementation-dependent. The single map_noncoherent option > > > > > > > > > is > > > > > > > > > not > > > > > > > > > enough > > > > > > > > > to reflect real hardware anymore. For example, the Loongson > > > > > > > > > LS3A4000 > > > > > > > > > CPU > > > > > > > > > and LS2K2000/LS2K1000 SoC, peripheral device of such hardware > > > > > > > > > platform > > > > > > > > > allways snoop CPU's cache. Doing the allocation with > > > > > > > > > dma_alloc_coherent > > > > > > > > > function is preferred. The return buffer is cached, it should > > > > > > > > > not > > > > > > > > > using > > > > > > > > > the default write-combine mapping. While with the current > > > > > > > > > implement, > > > > > > > > > there > > > > > > > > > no way to tell the drm core to reflect this. > > > > > > > > > > > > > > > > > > This patch adds cached and coherent members to struct > > > > > > > > > drm_gem_dma_object. > > > > > > > > > which allow driver implements to inform the core. Introducing > > > > > > > > > new > > > > > > > > > mappings > > > > > > > > > while keeping the original default behavior unchanged. > > > > > > > > Did you try to simply set the "dma-coherent" property to the > > > > > > > > device's > > > > > > > > node? > > > > > > > But this approach can only be applied for the device driver with > > > > > > > DT > > > > > > > support. > > > > > > > > > > > > > > X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically > > > > > > > do > > > > > > > not > > > > > > > have DT support. > > > > > > > > > > > > > > They using ACPI to pass parameter from the firmware to Linux > > > > > > > kernel. > > > > > > > > > > > > > > You approach will lost the effectiveness on such a case. > > > > > > Well, I don't really know how ACPI handles it - but it should just > > > > > > be a > > > > > > matter of setting dev->dma_coherent. That's basically what the DT > > > > > > code > > > > > > does. > > > > > > > > > > > > Some MIPS boards set it in their setup code for instance. > > > > > > > > > > > This is a *strategy*, not a *mechanism*. > > > > > > > > > > In this case, DT is just used to describing the hardware. > > > > > > > > > > (It is actually a hardware feature describing language, the > > > > > granularity > > > > > is large) > > > > > > > > > > It does not changing the state of the hardware. > > > > > > > > > > It's your platform firmware or kernel setting up code who actually do > > > > > such a things. > > > > > > > > > > > > > > > It's just that it works on *one* platform, it does not guarantee it > > > > > will > > > > > works on others. > > > > If you add the "dma-coherent" property in a device node in DT, you > > > > effectively specify that the device is DMA-coherent; so you describe > > > > the hardware, which is what DT is for, and you are not changing the > > > > state of the hardware. > > > > > > > > Note that some MIPS platforms (arch/mips/alchemy/common/setup.c) > > > > default to DMA-coherent mapping; I believe you could do something > > > > similar with your Loongson
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi, On 2023/6/8 15:39, Maxime Ripard wrote: On Thu, Jun 08, 2023 at 01:18:38AM +0800, Sui Jingfeng wrote: Hi, On 2023/6/8 00:12, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 22:38 +0800, Sui Jingfeng a écrit : Hi, welcome to discussion. I have limited skills in manipulating English. It may not express what I'm really means in the short time. Part of word in the sentence may not as accurate as your. Well, please don't misunderstand, I'm not doing the rude to you. No problem. I will explain it with more details. See below: On 2023/6/7 20:09, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : Hi, On 2023/6/7 17:36, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : The single map_noncoherent member of struct drm_gem_dma_object may not sufficient for describing the backing memory of the GEM buffer object. Especially on dma-coherent systems, the backing memory is both cached coherent for multi-core CPUs and dma-coherent for peripheral device. Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. Whether a peripheral device is dma-coherent or not can be implementation-dependent. The single map_noncoherent option is not enough to reflect real hardware anymore. For example, the Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC, peripheral device of such hardware platform allways snoop CPU's cache. Doing the allocation with dma_alloc_coherent function is preferred. The return buffer is cached, it should not using the default write-combine mapping. While with the current implement, there no way to tell the drm core to reflect this. This patch adds cached and coherent members to struct drm_gem_dma_object. which allow driver implements to inform the core. Introducing new mappings while keeping the original default behavior unchanged. Did you try to simply set the "dma-coherent" property to the device's node? But this approach can only be applied for the device driver with DT support. X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not have DT support. They using ACPI to pass parameter from the firmware to Linux kernel. You approach will lost the effectiveness on such a case. Well, I don't really know how ACPI handles it - but it should just be a matter of setting dev->dma_coherent. That's basically what the DT code does. Some MIPS boards set it in their setup code for instance. This is a *strategy*, not a *mechanism*. In this case, DT is just used to describing the hardware. (It is actually a hardware feature describing language, the granularity is large) It does not changing the state of the hardware. It's your platform firmware or kernel setting up code who actually do such a things. It's just that it works on *one* platform, it does not guarantee it will works on others. If you add the "dma-coherent" property in a device node in DT, you effectively specify that the device is DMA-coherent; so you describe the hardware, which is what DT is for, and you are not changing the state of the hardware. Note that some MIPS platforms (arch/mips/alchemy/common/setup.c) default to DMA-coherent mapping; I believe you could do something similar with your Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC. The preblem is that device driver can have various demand. It probably want to create different kind of buffers for different thing simultaneously. Say, one allocated with dma_alloc_coherent for command buffer or dma descriptor another one allocated with dma_alloc_wc for uploading shader etc. also has the third one allocated with dma_alloc_noncoherent() for doing some else. And it will work just fine. struct device dma_coherent, or DT's dma-coherent property define that the device doesn't need any kind of cache maintenance, ever. If it's missing, we need to perform cache maintenance to keep coherency. dma_alloc_* functions provide guarantees to the driver. With dma_alloc_wc and dma_alloc_coherent, the buffer is coherent, and thus you don't need to perform cache maintenance operations by hand in the driver. BO returned by dma_alloc_wc() doesn't works on some platform. This may only guarantee for the CPU side. There is no guarantee for the GPU side. For example, the GPU always snoop CPU's cache. The GPU fetch data from the CPU's cache if hit. if not hit, the GPU fetch the data from the system RAM. But when call dma_alloc_wc(), the BO at cpu side is marked as write combine property. The write buffer within the CPU will gather the CPU side write access. This is to say, there may have some data reside(stall) in the write buffer. while the GPU will fetch data from the system RAM or CPU's cache. the GPU will fetch wrong data. This is the condition for our hardware, I don't know how does the ARM platform guarantee the coherency in this case. If it relay on software to guarantee, then it is still non hardware maintained coherency.
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
On Thu, Jun 08, 2023 at 01:18:38AM +0800, Sui Jingfeng wrote: > Hi, > > On 2023/6/8 00:12, Paul Cercueil wrote: > > Hi Sui, > > > > Le mercredi 07 juin 2023 à 22:38 +0800, Sui Jingfeng a écrit : > > > Hi, welcome to discussion. > > > > > > > > > I have limited skills in manipulating English. > > > > > > It may not express what I'm really means in the short time. > > > > > > Part of word in the sentence may not as accurate as your. > > > > > > Well, please don't misunderstand, I'm not doing the rude to you. > > No problem. > > > > > I will explain it with more details. > > > > > > See below: > > > > > > > > > On 2023/6/7 20:09, Paul Cercueil wrote: > > > > Hi Sui, > > > > > > > > Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : > > > > > Hi, > > > > > > > > > > > > > > > On 2023/6/7 17:36, Paul Cercueil wrote: > > > > > > Hi Sui, > > > > > > > > > > > > Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : > > > > > > > The single map_noncoherent member of struct > > > > > > > drm_gem_dma_object > > > > > > > may > > > > > > > not > > > > > > > sufficient for describing the backing memory of the GEM > > > > > > > buffer > > > > > > > object. > > > > > > > > > > > > > > Especially on dma-coherent systems, the backing memory is > > > > > > > both > > > > > > > cached > > > > > > > coherent for multi-core CPUs and dma-coherent for peripheral > > > > > > > device. > > > > > > > Say architectures like X86-64, LoongArch64, Loongson Mips64, > > > > > > > etc. > > > > > > > > > > > > > > Whether a peripheral device is dma-coherent or not can be > > > > > > > implementation-dependent. The single map_noncoherent option > > > > > > > is > > > > > > > not > > > > > > > enough > > > > > > > to reflect real hardware anymore. For example, the Loongson > > > > > > > LS3A4000 > > > > > > > CPU > > > > > > > and LS2K2000/LS2K1000 SoC, peripheral device of such hardware > > > > > > > platform > > > > > > > allways snoop CPU's cache. Doing the allocation with > > > > > > > dma_alloc_coherent > > > > > > > function is preferred. The return buffer is cached, it should > > > > > > > not > > > > > > > using > > > > > > > the default write-combine mapping. While with the current > > > > > > > implement, > > > > > > > there > > > > > > > no way to tell the drm core to reflect this. > > > > > > > > > > > > > > This patch adds cached and coherent members to struct > > > > > > > drm_gem_dma_object. > > > > > > > which allow driver implements to inform the core. Introducing > > > > > > > new > > > > > > > mappings > > > > > > > while keeping the original default behavior unchanged. > > > > > > Did you try to simply set the "dma-coherent" property to the > > > > > > device's > > > > > > node? > > > > > But this approach can only be applied for the device driver with > > > > > DT > > > > > support. > > > > > > > > > > X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically > > > > > do > > > > > not > > > > > have DT support. > > > > > > > > > > They using ACPI to pass parameter from the firmware to Linux > > > > > kernel. > > > > > > > > > > You approach will lost the effectiveness on such a case. > > > > Well, I don't really know how ACPI handles it - but it should just > > > > be a > > > > matter of setting dev->dma_coherent. That's basically what the DT > > > > code > > > > does. > > > > > > > > Some MIPS boards set it in their setup code for instance. > > > > > > > This is a *strategy*, not a *mechanism*. > > > > > > In this case, DT is just used to describing the hardware. > > > > > > (It is actually a hardware feature describing language, the > > > granularity > > > is large) > > > > > > It does not changing the state of the hardware. > > > > > > It's your platform firmware or kernel setting up code who actually do > > > such a things. > > > > > > > > > It's just that it works on *one* platform, it does not guarantee it > > > will > > > works on others. > > If you add the "dma-coherent" property in a device node in DT, you > > effectively specify that the device is DMA-coherent; so you describe > > the hardware, which is what DT is for, and you are not changing the > > state of the hardware. > > > > Note that some MIPS platforms (arch/mips/alchemy/common/setup.c) > > default to DMA-coherent mapping; I believe you could do something > > similar with your Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC. > > > The preblem is that device driver can have various demand. > > It probably want to create different kind of buffers for different thing > simultaneously. > > Say, one allocated with dma_alloc_coherent for command buffer or dma > descriptor > > another one allocated with dma_alloc_wc for uploading shader etc. > > also has the third one allocated with dma_alloc_noncoherent() for doing some > else. And it will work just fine. struct device dma_coherent, or DT's dma-coherent property define that the device doesn't need any kind of cache maintenance, ever. If
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi, On 2023/6/7 20:19, Maxime Ripard wrote: On Wed, Jun 07, 2023 at 06:30:01PM +0800, Sui Jingfeng wrote: On 2023/6/7 17:36, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : The single map_noncoherent member of struct drm_gem_dma_object may not sufficient for describing the backing memory of the GEM buffer object. Especially on dma-coherent systems, the backing memory is both cached coherent for multi-core CPUs and dma-coherent for peripheral device. Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. Whether a peripheral device is dma-coherent or not can be implementation-dependent. The single map_noncoherent option is not enough to reflect real hardware anymore. For example, the Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC, peripheral device of such hardware platform allways snoop CPU's cache. Doing the allocation with dma_alloc_coherent function is preferred. The return buffer is cached, it should not using the default write-combine mapping. While with the current implement, there no way to tell the drm core to reflect this. This patch adds cached and coherent members to struct drm_gem_dma_object. which allow driver implements to inform the core. Introducing new mappings while keeping the original default behavior unchanged. Did you try to simply set the "dma-coherent" property to the device's node? But this approach can only be applied for the device driver with DT support. X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not have DT support. They using ACPI to pass parameter from the firmware to Linux kernel. You approach will lost the effectiveness on such a case. Not really, no. All DT support is doing is setting some generic device parameter based on that property, but the infrastructure is very much generic and can be used on systems without a DT. From what I understand if you add that property then Linux will use DMA coherent memory even though you use dma_alloc_noncoherent() and the sync_single_for_cpu() / sync_single_for_device() are then NOPs. > Please do not mitigate the problems with confusing method. It's not a confusing method, it's one of the two main API to deal with DMA buffers. And you might disagree with Paul but there's no need to be rude about it. This approach not only tend to generate confusion but also implement-dependent and arch-dependent. It's definitely problematic. How does the dma_alloc_coherent/dma_alloc_noncoherent is a ARCH specific thing. Dependent on how does the arch_dma_ops is implemented. The definition of the coherent on different ARCH has different meanings. The definition of the wirte-combine on different ARCH has different meanings. The wirte-combine(uncache acceleration) on mips is non dma-coherent. Then MIPS breaks the DMA allocation semantics. A buffer allocated with dma_alloc_wc is supposed to be coherent. But on arm, It seem that wirte-combine is coherent. (guaranteed by arch implement). I also heard using dma_alloc_coherent to allocation the buffer for the non-coherent doesn't hurt, but the reverse is not true. But please do not create confusion. software composite is faster because better cacheusing rate and cache is faster to read. It is faster because it is cached, not because it is non-coherent. non-coherent is arch thing and/or driver-side thing, it is a side effect of using the cached mapping. Honestly, it's not clear to me what your point or issue is. Going back to the description in your commit log, you mention that you want to support multiple hardware that might or might not be DMA coherent, and thus you want to allocate a buffer with different attributes depending on that? Like, you say that the LS3A4000 has a coherency unit and thus doing the allocation with dma_alloc_coherent is preferred. Preferred to what? A WC buffer? Why? A WC buffer is a coherent buffer that is allowed to cache writes. It doesn't have to, and worst case scenario you're inexactly the same case than a dma_alloc_coherent buffer. It should left to driver to handle such a side effect. The device driver know their device, so its the device driver's responsibility to maintain the coherency. Not really, no. Some driver are used across multiple SoCs and multiple arch. It doesn't make any sense to encode this in the driver... which is why it's in the DT in the first place, and abstracted away by the DMA API. Like, do you really expect the amdgpu driver to know the DMA attributes it needs to allocate a buffer from when running from a RaspberryPi? On loongson platform, we don't need to call drm_fb_dma_sync_non_coherent() function, Its already guaranteed by hardware. And mostly guaranteed by dma_alloc_coherent. And if you wanted to call it anyway, it would be a nop if the device is declared as coherent already. I think you're thinking about this backward. A buffer has mapping attributes, and a device has hardware properties. The driver (ie,
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi, On 2023/6/8 00:12, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 22:38 +0800, Sui Jingfeng a écrit : Hi, welcome to discussion. I have limited skills in manipulating English. It may not express what I'm really means in the short time. Part of word in the sentence may not as accurate as your. Well, please don't misunderstand, I'm not doing the rude to you. No problem. I will explain it with more details. See below: On 2023/6/7 20:09, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : Hi, On 2023/6/7 17:36, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : The single map_noncoherent member of struct drm_gem_dma_object may not sufficient for describing the backing memory of the GEM buffer object. Especially on dma-coherent systems, the backing memory is both cached coherent for multi-core CPUs and dma-coherent for peripheral device. Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. Whether a peripheral device is dma-coherent or not can be implementation-dependent. The single map_noncoherent option is not enough to reflect real hardware anymore. For example, the Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC, peripheral device of such hardware platform allways snoop CPU's cache. Doing the allocation with dma_alloc_coherent function is preferred. The return buffer is cached, it should not using the default write-combine mapping. While with the current implement, there no way to tell the drm core to reflect this. This patch adds cached and coherent members to struct drm_gem_dma_object. which allow driver implements to inform the core. Introducing new mappings while keeping the original default behavior unchanged. Did you try to simply set the "dma-coherent" property to the device's node? But this approach can only be applied for the device driver with DT support. X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not have DT support. They using ACPI to pass parameter from the firmware to Linux kernel. You approach will lost the effectiveness on such a case. Well, I don't really know how ACPI handles it - but it should just be a matter of setting dev->dma_coherent. That's basically what the DT code does. Some MIPS boards set it in their setup code for instance. This is a *strategy*, not a *mechanism*. In this case, DT is just used to describing the hardware. (It is actually a hardware feature describing language, the granularity is large) It does not changing the state of the hardware. It's your platform firmware or kernel setting up code who actually do such a things. It's just that it works on *one* platform, it does not guarantee it will works on others. If you add the "dma-coherent" property in a device node in DT, you effectively specify that the device is DMA-coherent; so you describe the hardware, which is what DT is for, and you are not changing the state of the hardware. Note that some MIPS platforms (arch/mips/alchemy/common/setup.c) default to DMA-coherent mapping; I believe you could do something similar with your Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC. The preblem is that device driver can have various demand. It probably want to create different kind of buffers for different thing simultaneously. Say, one allocated with dma_alloc_coherent for command buffer or dma descriptor another one allocated with dma_alloc_wc for uploading shader etc. also has the third one allocated with dma_alloc_noncoherent() for doing some else. Simple setting by DT or firmware which override all allocation is not what we want. My patch is toward the drm core, leave the choice to the device drivers. How does the device driver fetch hardware descriptions is the device driver's thing. either via DT, or ACPI, kernel cmd or hard-code. Its device drivers policy. My patch do not require the platform make the decision for the device driver. Nor does it depend on DT. Your approaches are neither sufficient nor necessary. It gives the freedom to the the device driver. Device driver has full control over the buffer allocation. For our hardware, It don't use DT on some application scene. Out hardware is dma-coherent and cached coherent. We don't want a dma-coherent buffer attached with the name of "map_noncoherent". While my patch is trying to create a *mechanism* which could probably works on all platform. It is based the patch you have already commuted. Thanks for your excellent contribution. From what I understand if you add that property then Linux will use DMA coherent memory even though you use dma_alloc_noncoherent() and the sync_single_for_cpu() / sync_single_for_device() are then NOPs. Please do not mitigate the problems with confusing method. This approach not only tend to generate confusion but also implement-dependent and arch-dependent. It's definitely problematic. How does the
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi Sui, Le mercredi 07 juin 2023 à 22:38 +0800, Sui Jingfeng a écrit : > Hi, welcome to discussion. > > > I have limited skills in manipulating English. > > It may not express what I'm really means in the short time. > > Part of word in the sentence may not as accurate as your. > > Well, please don't misunderstand, I'm not doing the rude to you. No problem. > > I will explain it with more details. > > See below: > > > On 2023/6/7 20:09, Paul Cercueil wrote: > > Hi Sui, > > > > Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : > > > Hi, > > > > > > > > > On 2023/6/7 17:36, Paul Cercueil wrote: > > > > Hi Sui, > > > > > > > > Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : > > > > > The single map_noncoherent member of struct > > > > > drm_gem_dma_object > > > > > may > > > > > not > > > > > sufficient for describing the backing memory of the GEM > > > > > buffer > > > > > object. > > > > > > > > > > Especially on dma-coherent systems, the backing memory is > > > > > both > > > > > cached > > > > > coherent for multi-core CPUs and dma-coherent for peripheral > > > > > device. > > > > > Say architectures like X86-64, LoongArch64, Loongson Mips64, > > > > > etc. > > > > > > > > > > Whether a peripheral device is dma-coherent or not can be > > > > > implementation-dependent. The single map_noncoherent option > > > > > is > > > > > not > > > > > enough > > > > > to reflect real hardware anymore. For example, the Loongson > > > > > LS3A4000 > > > > > CPU > > > > > and LS2K2000/LS2K1000 SoC, peripheral device of such hardware > > > > > platform > > > > > allways snoop CPU's cache. Doing the allocation with > > > > > dma_alloc_coherent > > > > > function is preferred. The return buffer is cached, it should > > > > > not > > > > > using > > > > > the default write-combine mapping. While with the current > > > > > implement, > > > > > there > > > > > no way to tell the drm core to reflect this. > > > > > > > > > > This patch adds cached and coherent members to struct > > > > > drm_gem_dma_object. > > > > > which allow driver implements to inform the core. Introducing > > > > > new > > > > > mappings > > > > > while keeping the original default behavior unchanged. > > > > Did you try to simply set the "dma-coherent" property to the > > > > device's > > > > node? > > > But this approach can only be applied for the device driver with > > > DT > > > support. > > > > > > X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically > > > do > > > not > > > have DT support. > > > > > > They using ACPI to pass parameter from the firmware to Linux > > > kernel. > > > > > > You approach will lost the effectiveness on such a case. > > Well, I don't really know how ACPI handles it - but it should just > > be a > > matter of setting dev->dma_coherent. That's basically what the DT > > code > > does. > > > > Some MIPS boards set it in their setup code for instance. > > > This is a *strategy*, not a *mechanism*. > > In this case, DT is just used to describing the hardware. > > (It is actually a hardware feature describing language, the > granularity > is large) > > It does not changing the state of the hardware. > > It's your platform firmware or kernel setting up code who actually do > such a things. > > > It's just that it works on *one* platform, it does not guarantee it > will > works on others. If you add the "dma-coherent" property in a device node in DT, you effectively specify that the device is DMA-coherent; so you describe the hardware, which is what DT is for, and you are not changing the state of the hardware. Note that some MIPS platforms (arch/mips/alchemy/common/setup.c) default to DMA-coherent mapping; I believe you could do something similar with your Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC. > While my patch is trying to create a *mechanism* which could probably > > works on all platform. > > > It is based the patch you have already commuted. > > Thanks for your excellent contribution. > > > > > > From what I understand if you add that property then Linux > > > > will > > > > use DMA > > > > coherent memory even though you use dma_alloc_noncoherent() and > > > > the > > > > sync_single_for_cpu() / sync_single_for_device() are then NOPs. > > > Please do not mitigate the problems with confusing method. > > > > > > > > > This approach not only tend to generate confusion but also > > > implement-dependent > > > > > > and arch-dependent. It's definitely problematic. > > > > > > > > > How does the dma_alloc_coherent/dma_alloc_noncoherent is a ARCH > > > specific > > > thing. > > > > > > Dependent on how does the arch_dma_ops is implemented. > > > > > > > > > The definition of the coherent on different ARCH has different > > > meanings. > > > > > > The definition of the wirte-combine on different ARCH has > > > different > > > meanings. > > > > > > > > > The wirte-combine(uncache acceleration) on mips is non dma- > > >
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi, welcome to discussion. I have limited skills in manipulating English. It may not express what I'm really means in the short time. Part of word in the sentence may not as accurate as your. Well, please don't misunderstand, I'm not doing the rude to you. I will explain it with more details. See below: On 2023/6/7 20:09, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : Hi, On 2023/6/7 17:36, Paul Cercueil wrote: Hi Sui, Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : The single map_noncoherent member of struct drm_gem_dma_object may not sufficient for describing the backing memory of the GEM buffer object. Especially on dma-coherent systems, the backing memory is both cached coherent for multi-core CPUs and dma-coherent for peripheral device. Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. Whether a peripheral device is dma-coherent or not can be implementation-dependent. The single map_noncoherent option is not enough to reflect real hardware anymore. For example, the Loongson LS3A4000 CPU and LS2K2000/LS2K1000 SoC, peripheral device of such hardware platform allways snoop CPU's cache. Doing the allocation with dma_alloc_coherent function is preferred. The return buffer is cached, it should not using the default write-combine mapping. While with the current implement, there no way to tell the drm core to reflect this. This patch adds cached and coherent members to struct drm_gem_dma_object. which allow driver implements to inform the core. Introducing new mappings while keeping the original default behavior unchanged. Did you try to simply set the "dma-coherent" property to the device's node? But this approach can only be applied for the device driver with DT support. X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not have DT support. They using ACPI to pass parameter from the firmware to Linux kernel. You approach will lost the effectiveness on such a case. Well, I don't really know how ACPI handles it - but it should just be a matter of setting dev->dma_coherent. That's basically what the DT code does. Some MIPS boards set it in their setup code for instance. This is a *strategy*, not a *mechanism*. In this case, DT is just used to describing the hardware. (It is actually a hardware feature describing language, the granularity is large) It does not changing the state of the hardware. It's your platform firmware or kernel setting up code who actually do such a things. It's just that it works on *one* platform, it does not guarantee it will works on others. While my patch is trying to create a *mechanism* which could probably works on all platform. It is based the patch you have already commuted. Thanks for your excellent contribution. From what I understand if you add that property then Linux will use DMA coherent memory even though you use dma_alloc_noncoherent() and the sync_single_for_cpu() / sync_single_for_device() are then NOPs. Please do not mitigate the problems with confusing method. This approach not only tend to generate confusion but also implement-dependent and arch-dependent. It's definitely problematic. How does the dma_alloc_coherent/dma_alloc_noncoherent is a ARCH specific thing. Dependent on how does the arch_dma_ops is implemented. The definition of the coherent on different ARCH has different meanings. The definition of the wirte-combine on different ARCH has different meanings. The wirte-combine(uncache acceleration) on mips is non dma-coherent. It is dma-coherent on Ingenic SoCs. It is dma-coherent ? How does it achieve it? As far as I know, there is a write buffer within the mips cpu. typically 64 byte, but it is not cache. It will gather the CPU write access, When a peripheral device do the DMA, how does you platform guarantee the data in the CPU write buffer has been already arrived at (or flushed out to) the system RAM? Does the peripheral device snoop the CPU's write buffer, or it need manually flush the write buffer with SYNC instruction? But on arm, It seem that wirte-combine is coherent. (guaranteed by arch implement). I also heard using dma_alloc_coherent to allocation the buffer for the non-coherent doesn't hurt, but the reverse is not true. But please do not create confusion. software composite is faster because better cacheusing rate and cache is faster to read. It is faster because it is cached, not because it is non-coherent. non-coherent is arch thing and/or driver-side thing, it is a side effect of using the cached mapping. Yes, I know that. It should left to driver to handle such a side effect. The device driver know their device, so its the device driver's responsibility to maintain the coherency. On loongson platform, we don't need to call drm_fb_dma_sync_non_coherent() function, Its already guaranteed by hardware. I understand. What I'm saying, is that you
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
On Wed, Jun 07, 2023 at 06:30:01PM +0800, Sui Jingfeng wrote: > On 2023/6/7 17:36, Paul Cercueil wrote: > > Hi Sui, > > > > Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : > > > The single map_noncoherent member of struct drm_gem_dma_object may > > > not > > > sufficient for describing the backing memory of the GEM buffer > > > object. > > > > > > Especially on dma-coherent systems, the backing memory is both cached > > > coherent for multi-core CPUs and dma-coherent for peripheral device. > > > Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. > > > > > > Whether a peripheral device is dma-coherent or not can be > > > implementation-dependent. The single map_noncoherent option is not > > > enough > > > to reflect real hardware anymore. For example, the Loongson LS3A4000 > > > CPU > > > and LS2K2000/LS2K1000 SoC, peripheral device of such hardware > > > platform > > > allways snoop CPU's cache. Doing the allocation with > > > dma_alloc_coherent > > > function is preferred. The return buffer is cached, it should not > > > using > > > the default write-combine mapping. While with the current implement, > > > there > > > no way to tell the drm core to reflect this. > > > > > > This patch adds cached and coherent members to struct > > > drm_gem_dma_object. > > > which allow driver implements to inform the core. Introducing new > > > mappings > > > while keeping the original default behavior unchanged. > > Did you try to simply set the "dma-coherent" property to the device's > > node? > > But this approach can only be applied for the device driver with DT support. > > X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not > have DT support. > > They using ACPI to pass parameter from the firmware to Linux kernel. > > You approach will lost the effectiveness on such a case. Not really, no. All DT support is doing is setting some generic device parameter based on that property, but the infrastructure is very much generic and can be used on systems without a DT. > > From what I understand if you add that property then Linux will use DMA > > coherent memory even though you use dma_alloc_noncoherent() and the > > sync_single_for_cpu() / sync_single_for_device() are then NOPs. > > Please do not mitigate the problems with confusing method. It's not a confusing method, it's one of the two main API to deal with DMA buffers. And you might disagree with Paul but there's no need to be rude about it. > This approach not only tend to generate confusion but also > implement-dependent and arch-dependent. It's definitely problematic. > > > How does the dma_alloc_coherent/dma_alloc_noncoherent is a ARCH specific > thing. > > Dependent on how does the arch_dma_ops is implemented. > > > The definition of the coherent on different ARCH has different meanings. > > The definition of the wirte-combine on different ARCH has different > meanings. > > > The wirte-combine(uncache acceleration) on mips is non dma-coherent. Then MIPS breaks the DMA allocation semantics. A buffer allocated with dma_alloc_wc is supposed to be coherent. > But on arm, It seem that wirte-combine is coherent. (guaranteed by arch > implement). > > > I also heard using dma_alloc_coherent to allocation the buffer for the > non-coherent doesn't hurt, but the reverse is not true. > > > But please do not create confusion. > > software composite is faster because better cacheusing rate and > > cache is faster to read. > > It is faster because it is cached, not because it is non-coherent. > > non-coherent is arch thing and/or driver-side thing, > > it is a side effect of using the cached mapping. Honestly, it's not clear to me what your point or issue is. Going back to the description in your commit log, you mention that you want to support multiple hardware that might or might not be DMA coherent, and thus you want to allocate a buffer with different attributes depending on that? Like, you say that the LS3A4000 has a coherency unit and thus doing the allocation with dma_alloc_coherent is preferred. Preferred to what? A WC buffer? Why? A WC buffer is a coherent buffer that is allowed to cache writes. It doesn't have to, and worst case scenario you're inexactly the same case than a dma_alloc_coherent buffer. > It should left to driver to handle such a side effect. The device > driver know their device, so its the device driver's responsibility to > maintain the coherency. Not really, no. Some driver are used across multiple SoCs and multiple arch. It doesn't make any sense to encode this in the driver... which is why it's in the DT in the first place, and abstracted away by the DMA API. Like, do you really expect the amdgpu driver to know the DMA attributes it needs to allocate a buffer from when running from a RaspberryPi? > On loongson platform, we don't need to call > drm_fb_dma_sync_non_coherent() function, Its already guaranteed by > hardware. And mostly guaranteed by
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi Sui, Le mercredi 07 juin 2023 à 18:30 +0800, Sui Jingfeng a écrit : > Hi, > > > On 2023/6/7 17:36, Paul Cercueil wrote: > > Hi Sui, > > > > Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit : > > > The single map_noncoherent member of struct drm_gem_dma_object > > > may > > > not > > > sufficient for describing the backing memory of the GEM buffer > > > object. > > > > > > Especially on dma-coherent systems, the backing memory is both > > > cached > > > coherent for multi-core CPUs and dma-coherent for peripheral > > > device. > > > Say architectures like X86-64, LoongArch64, Loongson Mips64, etc. > > > > > > Whether a peripheral device is dma-coherent or not can be > > > implementation-dependent. The single map_noncoherent option is > > > not > > > enough > > > to reflect real hardware anymore. For example, the Loongson > > > LS3A4000 > > > CPU > > > and LS2K2000/LS2K1000 SoC, peripheral device of such hardware > > > platform > > > allways snoop CPU's cache. Doing the allocation with > > > dma_alloc_coherent > > > function is preferred. The return buffer is cached, it should not > > > using > > > the default write-combine mapping. While with the current > > > implement, > > > there > > > no way to tell the drm core to reflect this. > > > > > > This patch adds cached and coherent members to struct > > > drm_gem_dma_object. > > > which allow driver implements to inform the core. Introducing new > > > mappings > > > while keeping the original default behavior unchanged. > > Did you try to simply set the "dma-coherent" property to the > > device's > > node? > > But this approach can only be applied for the device driver with DT > support. > > X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do > not > have DT support. > > They using ACPI to pass parameter from the firmware to Linux kernel. > > You approach will lost the effectiveness on such a case. Well, I don't really know how ACPI handles it - but it should just be a matter of setting dev->dma_coherent. That's basically what the DT code does. Some MIPS boards set it in their setup code for instance. > > From what I understand if you add that property then Linux will > > use DMA > > coherent memory even though you use dma_alloc_noncoherent() and the > > sync_single_for_cpu() / sync_single_for_device() are then NOPs. > > Please do not mitigate the problems with confusing method. > > > This approach not only tend to generate confusion but also > implement-dependent > > and arch-dependent. It's definitely problematic. > > > How does the dma_alloc_coherent/dma_alloc_noncoherent is a ARCH > specific > thing. > > Dependent on how does the arch_dma_ops is implemented. > > > The definition of the coherent on different ARCH has different > meanings. > > The definition of the wirte-combine on different ARCH has different > meanings. > > > The wirte-combine(uncache acceleration) on mips is non dma-coherent. It is dma-coherent on Ingenic SoCs. > > But on arm, It seem that wirte-combine is coherent. (guaranteed by > arch > implement). > > > I also heard using dma_alloc_coherent to allocation the buffer for > the > non-coherent doesn't hurt, but the reverse is not true. > > > But please do not create confusion. > > software composite is faster because better cacheusing rate and > > cache is faster to read. > > It is faster because it is cached, not because it is non-coherent. > > non-coherent is arch thing and/or driver-side thing, > > it is a side effect of using the cached mapping. Yes, I know that. > > > It should left to driver to handle such a side effect. The device > driver > > know their device, so its the device driver's responsibility to > maintain > the coherency. On loongson platform, we don't need to call > drm_fb_dma_sync_non_coherent() function, Its already guaranteed by > hardware. I understand. What I'm saying, is that you should be able to set dma_obj->map_noncoherent (which would arguably be better named "map_cached", but that's a different problem). Then the GEM code would end up calling dma_alloc_noncoherent(), which will give you *cached* memory. Then as long as dev->dma_coherent = true, drm_fb_dma_sync_non_coherent() should be a NOP - so you wouldn't pointlessly sync/invalidate the caches. And I disagree with you, the driver shouldn't handle such things. The fact that it is better to use cached memory or uncached with write- combine really is platform-specific and not something that the driver should be aware of. Cheers, -Paul > > > > Cheers, > > -Paul > > > > > Signed-off-by: Sui Jingfeng > > > --- > > > drivers/gpu/drm/drm_fb_dma_helper.c | 11 +-- > > > drivers/gpu/drm/drm_fbdev_dma.c | 2 +- > > > drivers/gpu/drm/drm_gem_dma_helper.c | 20 > > > > > > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 5 - > > > drivers/gpu/drm/rcar-du/Kconfig | 2 -- > > >
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi,
On 2023/6/7 17:36, Paul Cercueil wrote:
Hi Sui,
Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit :
The single map_noncoherent member of struct drm_gem_dma_object may
not
sufficient for describing the backing memory of the GEM buffer
object.
Especially on dma-coherent systems, the backing memory is both cached
coherent for multi-core CPUs and dma-coherent for peripheral device.
Say architectures like X86-64, LoongArch64, Loongson Mips64, etc.
Whether a peripheral device is dma-coherent or not can be
implementation-dependent. The single map_noncoherent option is not
enough
to reflect real hardware anymore. For example, the Loongson LS3A4000
CPU
and LS2K2000/LS2K1000 SoC, peripheral device of such hardware
platform
allways snoop CPU's cache. Doing the allocation with
dma_alloc_coherent
function is preferred. The return buffer is cached, it should not
using
the default write-combine mapping. While with the current implement,
there
no way to tell the drm core to reflect this.
This patch adds cached and coherent members to struct
drm_gem_dma_object.
which allow driver implements to inform the core. Introducing new
mappings
while keeping the original default behavior unchanged.
Did you try to simply set the "dma-coherent" property to the device's
node?
But this approach can only be applied for the device driver with DT support.
X86-64, Loongson ls3a4000 mips64, Loongson ls3a5000 CPU typically do not
have DT support.
They using ACPI to pass parameter from the firmware to Linux kernel.
You approach will lost the effectiveness on such a case.
From what I understand if you add that property then Linux will use DMA
coherent memory even though you use dma_alloc_noncoherent() and the
sync_single_for_cpu() / sync_single_for_device() are then NOPs.
Please do not mitigate the problems with confusing method.
This approach not only tend to generate confusion but also
implement-dependent
and arch-dependent. It's definitely problematic.
How does the dma_alloc_coherent/dma_alloc_noncoherent is a ARCH specific
thing.
Dependent on how does the arch_dma_ops is implemented.
The definition of the coherent on different ARCH has different meanings.
The definition of the wirte-combine on different ARCH has different
meanings.
The wirte-combine(uncache acceleration) on mips is non dma-coherent.
But on arm, It seem that wirte-combine is coherent. (guaranteed by arch
implement).
I also heard using dma_alloc_coherent to allocation the buffer for the
non-coherent doesn't hurt, but the reverse is not true.
But please do not create confusion.
software composite is faster because better cacheusing rate and
cache is faster to read.
It is faster because it is cached, not because it is non-coherent.
non-coherent is arch thing and/or driver-side thing,
it is a side effect of using the cached mapping.
It should left to driver to handle such a side effect. The device driver
know their device, so its the device driver's responsibility to maintain
the coherency. On loongson platform, we don't need to call
drm_fb_dma_sync_non_coherent() function, Its already guaranteed by hardware.
Cheers,
-Paul
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/drm_fb_dma_helper.c | 11 +--
drivers/gpu/drm/drm_fbdev_dma.c | 2 +-
drivers/gpu/drm/drm_gem_dma_helper.c | 20
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 5 -
drivers/gpu/drm/rcar-du/Kconfig | 2 --
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 4 +++-
include/drm/drm_gem_dma_helper.h | 7 +--
7 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c
b/drivers/gpu/drm/drm_fb_dma_helper.c
index 3b535ad1b07c..93ff05041192 100644
--- a/drivers/gpu/drm/drm_fb_dma_helper.c
+++ b/drivers/gpu/drm/drm_fb_dma_helper.c
@@ -106,16 +106,15 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct
drm_framebuffer *fb,
EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
/**
- * drm_fb_dma_sync_non_coherent - Sync GEM object to non-coherent
backing
- * memory
+ * drm_fb_dma_sync_non_coherent - Sync GEM object to cached backing
memory
* @drm: DRM device
* @old_state: Old plane state
* @state: New plane state
*
* This function can be used by drivers that use damage clips and
have
- * DMA GEM objects backed by non-coherent memory. Calling this
function
- * in a plane's .atomic_update ensures that all the data in the
backing
- * memory have been written to RAM.
+ * DMA GEM objects backed by cached memory. Calling this function in
a
+ * plane's .atomic_update ensures that all the data in the backing
memory
+ * have been written to RAM.
*/
void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
struct drm_plane_state *old_state,
@@ -131,7 +130,7 @@ void drm_fb_dma_sync_non_coherent(struct
drm_device *drm,
for (i = 0; i < finfo->num_planes; i++) {
Re: [PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
Hi Sui,
Le mercredi 07 juin 2023 à 13:30 +0800, Sui Jingfeng a écrit :
> The single map_noncoherent member of struct drm_gem_dma_object may
> not
> sufficient for describing the backing memory of the GEM buffer
> object.
>
> Especially on dma-coherent systems, the backing memory is both cached
> coherent for multi-core CPUs and dma-coherent for peripheral device.
> Say architectures like X86-64, LoongArch64, Loongson Mips64, etc.
>
> Whether a peripheral device is dma-coherent or not can be
> implementation-dependent. The single map_noncoherent option is not
> enough
> to reflect real hardware anymore. For example, the Loongson LS3A4000
> CPU
> and LS2K2000/LS2K1000 SoC, peripheral device of such hardware
> platform
> allways snoop CPU's cache. Doing the allocation with
> dma_alloc_coherent
> function is preferred. The return buffer is cached, it should not
> using
> the default write-combine mapping. While with the current implement,
> there
> no way to tell the drm core to reflect this.
>
> This patch adds cached and coherent members to struct
> drm_gem_dma_object.
> which allow driver implements to inform the core. Introducing new
> mappings
> while keeping the original default behavior unchanged.
Did you try to simply set the "dma-coherent" property to the device's
node?
From what I understand if you add that property then Linux will use DMA
coherent memory even though you use dma_alloc_noncoherent() and the
sync_single_for_cpu() / sync_single_for_device() are then NOPs.
Cheers,
-Paul
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/drm_fb_dma_helper.c | 11 +--
> drivers/gpu/drm/drm_fbdev_dma.c | 2 +-
> drivers/gpu/drm/drm_gem_dma_helper.c | 20
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 5 -
> drivers/gpu/drm/rcar-du/Kconfig | 2 --
> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 4 +++-
> include/drm/drm_gem_dma_helper.h | 7 +--
> 7 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c
> b/drivers/gpu/drm/drm_fb_dma_helper.c
> index 3b535ad1b07c..93ff05041192 100644
> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> @@ -106,16 +106,15 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct
> drm_framebuffer *fb,
> EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
>
> /**
> - * drm_fb_dma_sync_non_coherent - Sync GEM object to non-coherent
> backing
> - * memory
> + * drm_fb_dma_sync_non_coherent - Sync GEM object to cached backing
> memory
> * @drm: DRM device
> * @old_state: Old plane state
> * @state: New plane state
> *
> * This function can be used by drivers that use damage clips and
> have
> - * DMA GEM objects backed by non-coherent memory. Calling this
> function
> - * in a plane's .atomic_update ensures that all the data in the
> backing
> - * memory have been written to RAM.
> + * DMA GEM objects backed by cached memory. Calling this function in
> a
> + * plane's .atomic_update ensures that all the data in the backing
> memory
> + * have been written to RAM.
> */
> void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
> struct drm_plane_state *old_state,
> @@ -131,7 +130,7 @@ void drm_fb_dma_sync_non_coherent(struct
> drm_device *drm,
>
> for (i = 0; i < finfo->num_planes; i++) {
> dma_obj = drm_fb_dma_get_gem_obj(state->fb, i);
> - if (!dma_obj->map_noncoherent)
> + if (dma_obj->cached && dma_obj->coherent)
> continue;
>
> daddr = drm_fb_dma_get_gem_addr(state->fb, state, i);
> diff --git a/drivers/gpu/drm/drm_fbdev_dma.c
> b/drivers/gpu/drm/drm_fbdev_dma.c
> index d86773fa8ab0..49fe9b284cc8 100644
> --- a/drivers/gpu/drm/drm_fbdev_dma.c
> +++ b/drivers/gpu/drm/drm_fbdev_dma.c
> @@ -131,7 +131,7 @@ static int drm_fbdev_dma_helper_fb_probe(struct
> drm_fb_helper *fb_helper,
>
> /* screen */
> info->flags |= FBINFO_VIRTFB; /* system memory */
> - if (dma_obj->map_noncoherent)
> + if (dma_obj->cached)
> info->flags |= FBINFO_READS_FAST; /* signal caching
> */
> info->screen_size = sizes->surface_height * fb->pitches[0];
> info->screen_buffer = map.vaddr;
> diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c
> b/drivers/gpu/drm/drm_gem_dma_helper.c
> index 870b90b78bc4..dec1d512bdf1 100644
> --- a/drivers/gpu/drm/drm_gem_dma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_dma_helper.c
> @@ -93,7 +93,11 @@ __drm_gem_dma_create(struct drm_device *drm,
> size_t size, bool private)
> drm_gem_private_object_init(drm, gem_obj, size);
>
> /* Always use writecombine for dma-buf mappings */
> - dma_obj->map_noncoherent = false;
> + /* FIXME: This is not always true, on some dma
> coherent system,
> + * cached mappings should be preferred over
>
[PATCH] drm: gem: add an option for supporting the dma-coherent hardware.
The single map_noncoherent member of struct drm_gem_dma_object may not
sufficient for describing the backing memory of the GEM buffer object.
Especially on dma-coherent systems, the backing memory is both cached
coherent for multi-core CPUs and dma-coherent for peripheral device.
Say architectures like X86-64, LoongArch64, Loongson Mips64, etc.
Whether a peripheral device is dma-coherent or not can be
implementation-dependent. The single map_noncoherent option is not enough
to reflect real hardware anymore. For example, the Loongson LS3A4000 CPU
and LS2K2000/LS2K1000 SoC, peripheral device of such hardware platform
allways snoop CPU's cache. Doing the allocation with dma_alloc_coherent
function is preferred. The return buffer is cached, it should not using
the default write-combine mapping. While with the current implement, there
no way to tell the drm core to reflect this.
This patch adds cached and coherent members to struct drm_gem_dma_object.
which allow driver implements to inform the core. Introducing new mappings
while keeping the original default behavior unchanged.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/drm_fb_dma_helper.c | 11 +--
drivers/gpu/drm/drm_fbdev_dma.c | 2 +-
drivers/gpu/drm/drm_gem_dma_helper.c | 20
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 5 -
drivers/gpu/drm/rcar-du/Kconfig | 2 --
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 4 +++-
include/drm/drm_gem_dma_helper.h | 7 +--
7 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c
b/drivers/gpu/drm/drm_fb_dma_helper.c
index 3b535ad1b07c..93ff05041192 100644
--- a/drivers/gpu/drm/drm_fb_dma_helper.c
+++ b/drivers/gpu/drm/drm_fb_dma_helper.c
@@ -106,16 +106,15 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer
*fb,
EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
/**
- * drm_fb_dma_sync_non_coherent - Sync GEM object to non-coherent backing
- * memory
+ * drm_fb_dma_sync_non_coherent - Sync GEM object to cached backing memory
* @drm: DRM device
* @old_state: Old plane state
* @state: New plane state
*
* This function can be used by drivers that use damage clips and have
- * DMA GEM objects backed by non-coherent memory. Calling this function
- * in a plane's .atomic_update ensures that all the data in the backing
- * memory have been written to RAM.
+ * DMA GEM objects backed by cached memory. Calling this function in a
+ * plane's .atomic_update ensures that all the data in the backing memory
+ * have been written to RAM.
*/
void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
struct drm_plane_state *old_state,
@@ -131,7 +130,7 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
for (i = 0; i < finfo->num_planes; i++) {
dma_obj = drm_fb_dma_get_gem_obj(state->fb, i);
- if (!dma_obj->map_noncoherent)
+ if (dma_obj->cached && dma_obj->coherent)
continue;
daddr = drm_fb_dma_get_gem_addr(state->fb, state, i);
diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
index d86773fa8ab0..49fe9b284cc8 100644
--- a/drivers/gpu/drm/drm_fbdev_dma.c
+++ b/drivers/gpu/drm/drm_fbdev_dma.c
@@ -131,7 +131,7 @@ static int drm_fbdev_dma_helper_fb_probe(struct
drm_fb_helper *fb_helper,
/* screen */
info->flags |= FBINFO_VIRTFB; /* system memory */
- if (dma_obj->map_noncoherent)
+ if (dma_obj->cached)
info->flags |= FBINFO_READS_FAST; /* signal caching */
info->screen_size = sizes->surface_height * fb->pitches[0];
info->screen_buffer = map.vaddr;
diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c
b/drivers/gpu/drm/drm_gem_dma_helper.c
index 870b90b78bc4..dec1d512bdf1 100644
--- a/drivers/gpu/drm/drm_gem_dma_helper.c
+++ b/drivers/gpu/drm/drm_gem_dma_helper.c
@@ -93,7 +93,11 @@ __drm_gem_dma_create(struct drm_device *drm, size_t size,
bool private)
drm_gem_private_object_init(drm, gem_obj, size);
/* Always use writecombine for dma-buf mappings */
- dma_obj->map_noncoherent = false;
+ /* FIXME: This is not always true, on some dma coherent system,
+* cached mappings should be preferred over writecombine
+*/
+ dma_obj->cached = false;
+ dma_obj->coherent = false;
} else {
ret = drm_gem_object_init(drm, gem_obj, size);
}
@@ -143,7 +147,11 @@ struct drm_gem_dma_object *drm_gem_dma_create(struct
drm_device *drm,
if (IS_ERR(dma_obj))
return dma_obj;
- if (dma_obj->map_noncoherent) {
+ if (dma_obj->cached && dma_obj->coherent) {
+ dma_obj->vaddr = dma_alloc_coherent(drm->dev, size,
+ _obj->dma_addr,
+
