* [PATCH 1/3] [media] venus: mark PM functions as __maybe_unused @ 2017-06-27 15:02 Arnd Bergmann 2017-06-27 15:02 ` [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations Arnd Bergmann ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Arnd Bergmann @ 2017-06-27 15:02 UTC (permalink / raw) To: Stanimir Varbanov, Mauro Carvalho Chehab Cc: Arnd Bergmann, Hans Verkuil, Colin Ian King, linux-media, linux-arm-msm, linux-kernel Without PM support, gcc warns about two unused functions: platform/qcom/venus/core.c:146:13: error: 'venus_clks_disable' defined but not used [-Werror=unused-function] platform/qcom/venus/core.c:126:12: error: 'venus_clks_enable' defined but not used [-Werror=unused-function] The problem as usual are incorrect #ifdefs, so the easiest fix is to do away with the #ifdef completely and mark the suspend/resume handlers as __maybe_unused, which they are. Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/media/platform/qcom/venus/core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c index d8cbe8549d97..47f79637938c 100644 --- a/drivers/media/platform/qcom/venus/core.c +++ b/drivers/media/platform/qcom/venus/core.c @@ -270,8 +270,7 @@ static int venus_remove(struct platform_device *pdev) return ret; } -#ifdef CONFIG_PM -static int venus_runtime_suspend(struct device *dev) +static __maybe_unused int venus_runtime_suspend(struct device *dev) { struct venus_core *core = dev_get_drvdata(dev); int ret; @@ -283,7 +282,7 @@ static int venus_runtime_suspend(struct device *dev) return ret; } -static int venus_runtime_resume(struct device *dev) +static __maybe_unused int venus_runtime_resume(struct device *dev) { struct venus_core *core = dev_get_drvdata(dev); int ret; @@ -302,7 +301,6 @@ static int venus_runtime_resume(struct device *dev) venus_clks_disable(core); return ret; } -#endif static const struct dev_pm_ops venus_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, -- 2.9.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations 2017-06-27 15:02 [PATCH 1/3] [media] venus: mark PM functions as __maybe_unused Arnd Bergmann @ 2017-06-27 15:02 ` Arnd Bergmann 2017-06-27 19:39 ` Stanimir Varbanov 2017-06-27 15:02 ` [PATCH 3/3] [media] venus: fix compile-test build on non-qcom ARM platform Arnd Bergmann 2017-06-27 19:41 ` [PATCH 1/3] [media] venus: mark PM functions as __maybe_unused Stanimir Varbanov 2 siblings, 1 reply; 8+ messages in thread From: Arnd Bergmann @ 2017-06-27 15:02 UTC (permalink / raw) To: Stanimir Varbanov, Mauro Carvalho Chehab Cc: Arnd Bergmann, Hans Verkuil, linux-media, linux-arm-msm, linux-kernel In venus_boot(), we pass a pointer to a phys_addr_t into dmam_alloc_coherent, which the compiler warns about: platform/qcom/venus/firmware.c: In function 'venus_boot': platform/qcom/venus/firmware.c:63:49: error: passing argument 3 of 'dmam_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types] The returned DMA address is later passed on to a function that takes a phys_addr_t, so it's clearly wrong to use the DMA mapping interface here: the memory may be uncached, or the address may be completely wrong if there is an IOMMU connected to the device. My interpretation is that using dmam_alloc_coherent() had two purposes: a) get a chunk of consecutive memory that may be larger than the limit for kmalloc() b) use the devres infrastructure to simplify the unwinding in the error case. I think ideally we'd use a devres-based version of alloc_pages_exact() here, but since that doesn't exist, let's use devm_get_free_pages() instead. This wastes a little memory as the size gets rounded up to a power of two, but is otherwise harmless. If we want to save memory here, calling devm_free_pages() to release the memory once it is no longer needed is probably better anyway. Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- The same problem exists in the drm driver, as of commit 7c65817e6d38 ("drm/msm: gpu: Enable zap shader for A5XX"), and I submitted the same patch for that already. --- drivers/media/platform/qcom/venus/firmware.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c index 1b1a4f355918..76edb9f60311 100644 --- a/drivers/media/platform/qcom/venus/firmware.c +++ b/drivers/media/platform/qcom/venus/firmware.c @@ -60,11 +60,13 @@ int venus_boot(struct device *parent, struct device *fw_dev, const char *fwname) mem_size = VENUS_FW_MEM_SIZE; - mem_va = dmam_alloc_coherent(fw_dev, mem_size, &mem_phys, GFP_KERNEL); + mem_va = (void *)devm_get_free_pages(parent, GFP_KERNEL, + get_order(mem_size)); if (!mem_va) { ret = -ENOMEM; goto err_unreg_device; } + mem_phys = virt_to_phys(mem_va); ret = request_firmware(&mdt, fwname, fw_dev); if (ret < 0) -- 2.9.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations 2017-06-27 15:02 ` [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations Arnd Bergmann @ 2017-06-27 19:39 ` Stanimir Varbanov 2017-06-27 20:15 ` Arnd Bergmann 0 siblings, 1 reply; 8+ messages in thread From: Stanimir Varbanov @ 2017-06-27 19:39 UTC (permalink / raw) To: Arnd Bergmann, Stanimir Varbanov, Mauro Carvalho Chehab Cc: Hans Verkuil, linux-media, linux-arm-msm, linux-kernel Hi Arnd, On 27.06.2017 18:02, Arnd Bergmann wrote: > In venus_boot(), we pass a pointer to a phys_addr_t > into dmam_alloc_coherent, which the compiler warns about: > > platform/qcom/venus/firmware.c: In function 'venus_boot': > platform/qcom/venus/firmware.c:63:49: error: passing argument 3 of 'dmam_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types] > > The returned DMA address is later passed on to a function that > takes a phys_addr_t, so it's clearly wrong to use the DMA > mapping interface here: the memory may be uncached, or the > address may be completely wrong if there is an IOMMU connected > to the device. > > My interpretation is that using dmam_alloc_coherent() had two > purposes: > > a) get a chunk of consecutive memory that may be larger than > the limit for kmalloc() > > b) use the devres infrastructure to simplify the unwinding > in the error case. The intension here is to use per-device memory which is removed from kernel allocator, that memory is used by remote processor (Venus) for its code section and system memory, the memory must not be mapped to kernel to avoid any cache issues. As the memory in subject is reserved per-device memory the only legal way to allocate it is by dmam_alloc_coherent() -> dma_alloc_from_coherent(). For me the confusion comes from phys_addr_t which is passed to qcom_mdt_load() and then the address passed to qcom_scm_pas_mem_setup() which probably protects that physical memory. And the tz really expects physical address. The only solution I see is by casting dma_addr_t to phys_addr_t. Yes it is ugly but what is proper solution then? > > I think ideally we'd use a devres-based version of > alloc_pages_exact() here, but since that doesn't exist, > let's use devm_get_free_pages() instead. This wastes a little > memory as the size gets rounded up to a power of two, but > is otherwise harmless. If we want to save memory here, calling > devm_free_pages() to release the memory once it is no longer > needed is probably better anyway. > > Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > The same problem exists in the drm driver, as of commit 7c65817e6d38 > ("drm/msm: gpu: Enable zap shader for A5XX"), and I submitted the > same patch for that already. > --- > drivers/media/platform/qcom/venus/firmware.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > index 1b1a4f355918..76edb9f60311 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -60,11 +60,13 @@ int venus_boot(struct device *parent, struct device *fw_dev, const char *fwname) > > mem_size = VENUS_FW_MEM_SIZE; > > - mem_va = dmam_alloc_coherent(fw_dev, mem_size, &mem_phys, GFP_KERNEL); > + mem_va = (void *)devm_get_free_pages(parent, GFP_KERNEL, > + get_order(mem_size)); > if (!mem_va) { > ret = -ENOMEM; > goto err_unreg_device; > } > + mem_phys = virt_to_phys(mem_va); > > ret = request_firmware(&mdt, fwname, fw_dev); > if (ret < 0) > regards, Stan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations 2017-06-27 19:39 ` Stanimir Varbanov @ 2017-06-27 20:15 ` Arnd Bergmann 0 siblings, 0 replies; 8+ messages in thread From: Arnd Bergmann @ 2017-06-27 20:15 UTC (permalink / raw) To: Stanimir Varbanov Cc: Mauro Carvalho Chehab, Hans Verkuil, Linux Media Mailing List, linux-arm-msm, Linux Kernel Mailing List On Tue, Jun 27, 2017 at 9:39 PM, Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > Hi Arnd, > > On 27.06.2017 18:02, Arnd Bergmann wrote: >> >> In venus_boot(), we pass a pointer to a phys_addr_t >> into dmam_alloc_coherent, which the compiler warns about: >> >> platform/qcom/venus/firmware.c: In function 'venus_boot': >> platform/qcom/venus/firmware.c:63:49: error: passing argument 3 of >> 'dmam_alloc_coherent' from incompatible pointer type >> [-Werror=incompatible-pointer-types] >> >> The returned DMA address is later passed on to a function that >> takes a phys_addr_t, so it's clearly wrong to use the DMA >> mapping interface here: the memory may be uncached, or the >> address may be completely wrong if there is an IOMMU connected >> to the device. >> >> My interpretation is that using dmam_alloc_coherent() had two >> purposes: >> >> a) get a chunk of consecutive memory that may be larger than >> the limit for kmalloc() >> >> b) use the devres infrastructure to simplify the unwinding >> in the error case. > > > The intension here is to use per-device memory which is removed from kernel > allocator, that memory is used by remote processor (Venus) for its code > section and system memory, the memory must not be mapped to kernel to avoid > any cache issues. > > As the memory in subject is reserved per-device memory the only legal way to > allocate it is by dmam_alloc_coherent() -> dma_alloc_from_coherent(). > > For me the confusion comes from phys_addr_t which is passed to > qcom_mdt_load() and then the address passed to qcom_scm_pas_mem_setup() > which probably protects that physical memory. And the tz really expects > physical address. > > The only solution I see is by casting dma_addr_t to phys_addr_t. Yes it is > ugly but what is proper solution then? If you actually have a separate remote processor that accesses this memory, then qcom_mdt_load() is the wrong interface, as it takes a physical address, and we need to introduce another interface that can take a DMA address relative to a particular device. You cannot cast between the two types because phys_addr_t is an address as seen from the CPU, and dma_addr_t is seen by a particular device, and can only be used together with that device pointer. It looks like the pointer gets passed down to qcom_scm_call(dev, QCOM_SCM_SVC_PIL, QCOM_SCM_PAS_MEM_SETUP_CMD, ...), which in turn takes a 32-bit address, suggesting that this is indeed a dma address for that device (possibly going through an IOMMU), so maybe it just needs to all be changed to dma_addr_t. Is there any official documentation for qcom_scm_call() that clarifies what address space the arguments are in? Arnd ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] [media] venus: fix compile-test build on non-qcom ARM platform 2017-06-27 15:02 [PATCH 1/3] [media] venus: mark PM functions as __maybe_unused Arnd Bergmann 2017-06-27 15:02 ` [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations Arnd Bergmann @ 2017-06-27 15:02 ` Arnd Bergmann 2017-06-27 19:45 ` Stanimir Varbanov 2017-06-27 19:41 ` [PATCH 1/3] [media] venus: mark PM functions as __maybe_unused Stanimir Varbanov 2 siblings, 1 reply; 8+ messages in thread From: Arnd Bergmann @ 2017-06-27 15:02 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Arnd Bergmann, linux-media, linux-kernel If QCOM_MDT_LOADER is enabled, but ARCH_QCOM is not, we run into a build error: ERROR: "qcom_mdt_load" [drivers/media/platform/qcom/venus/venus-core.ko] undefined! ERROR: "qcom_mdt_get_size" [drivers/media/platform/qcom/venus/venus-core.ko] undefined! This changes the 'select' statement again, so we only try to enable those symbols when the drivers will actually get built. Fixes: 76724b30f222 ("[media] media: venus: enable building with COMPILE_TEST") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/media/platform/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index cb2f31cd0088..635c53e61f8a 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -475,8 +475,8 @@ config VIDEO_QCOM_VENUS tristate "Qualcomm Venus V4L2 encoder/decoder driver" depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA depends on (ARCH_QCOM && IOMMU_DMA) || COMPILE_TEST - select QCOM_MDT_LOADER if (ARM || ARM64) - select QCOM_SCM if (ARM || ARM64) + select QCOM_MDT_LOADER if ARCH_QCOM + select QCOM_SCM if ARCH_QCOM select VIDEOBUF2_DMA_SG select V4L2_MEM2MEM_DEV ---help--- -- 2.9.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] [media] venus: fix compile-test build on non-qcom ARM platform 2017-06-27 15:02 ` [PATCH 3/3] [media] venus: fix compile-test build on non-qcom ARM platform Arnd Bergmann @ 2017-06-27 19:45 ` Stanimir Varbanov 2017-06-28 20:25 ` Arnd Bergmann 0 siblings, 1 reply; 8+ messages in thread From: Stanimir Varbanov @ 2017-06-27 19:45 UTC (permalink / raw) To: Arnd Bergmann, Mauro Carvalho Chehab; +Cc: linux-media, linux-kernel Hi Arnd, On 27.06.2017 18:02, Arnd Bergmann wrote: > If QCOM_MDT_LOADER is enabled, but ARCH_QCOM is not, we run into > a build error: > > ERROR: "qcom_mdt_load" [drivers/media/platform/qcom/venus/venus-core.ko] undefined! > ERROR: "qcom_mdt_get_size" [drivers/media/platform/qcom/venus/venus-core.ko] undefined! Ahh, thanks for the fix, these two will also pursuing me in my dreams. > > This changes the 'select' statement again, so we only try to enable > those symbols when the drivers will actually get built. > > Fixes: 76724b30f222 ("[media] media: venus: enable building with COMPILE_TEST") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Reviewed-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/Kconfig | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > index cb2f31cd0088..635c53e61f8a 100644 > --- a/drivers/media/platform/Kconfig > +++ b/drivers/media/platform/Kconfig > @@ -475,8 +475,8 @@ config VIDEO_QCOM_VENUS > tristate "Qualcomm Venus V4L2 encoder/decoder driver" > depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA > depends on (ARCH_QCOM && IOMMU_DMA) || COMPILE_TEST > - select QCOM_MDT_LOADER if (ARM || ARM64) > - select QCOM_SCM if (ARM || ARM64) > + select QCOM_MDT_LOADER if ARCH_QCOM > + select QCOM_SCM if ARCH_QCOM > select VIDEOBUF2_DMA_SG > select V4L2_MEM2MEM_DEV > ---help--- > regards, Stan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] [media] venus: fix compile-test build on non-qcom ARM platform 2017-06-27 19:45 ` Stanimir Varbanov @ 2017-06-28 20:25 ` Arnd Bergmann 0 siblings, 0 replies; 8+ messages in thread From: Arnd Bergmann @ 2017-06-28 20:25 UTC (permalink / raw) To: Stanimir Varbanov Cc: Mauro Carvalho Chehab, Linux Media Mailing List, Linux Kernel Mailing List On Tue, Jun 27, 2017 at 9:45 PM, Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > Hi Arnd, > > On 27.06.2017 18:02, Arnd Bergmann wrote: >> >> If QCOM_MDT_LOADER is enabled, but ARCH_QCOM is not, we run into >> a build error: >> >> ERROR: "qcom_mdt_load" [drivers/media/platform/qcom/venus/venus-core.ko] >> undefined! >> ERROR: "qcom_mdt_get_size" >> [drivers/media/platform/qcom/venus/venus-core.ko] undefined! > > > Ahh, thanks for the fix, these two will also pursuing me in my dreams. I just came after me as well, as I hit another corner case, we need this fixup on top, I'll send a replacement: Subject: [PATCH] fixup! [media] venus: fix compile-test build on non-qcom ARM platform Signed-off-by: Arnd Bergmann <arnd@arndb.de> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c index 76edb9f60311..3794b9e3250b 100644 --- a/drivers/media/platform/qcom/venus/firmware.c +++ b/drivers/media/platform/qcom/venus/firmware.c @@ -40,7 +40,7 @@ int venus_boot(struct device *parent, struct device *fw_dev, const char *fwname) void *mem_va; int ret; - if (!qcom_scm_is_available()) + if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || !qcom_scm_is_available()) return -EPROBE_DEFER; fw_dev->parent = parent; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] [media] venus: mark PM functions as __maybe_unused 2017-06-27 15:02 [PATCH 1/3] [media] venus: mark PM functions as __maybe_unused Arnd Bergmann 2017-06-27 15:02 ` [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations Arnd Bergmann 2017-06-27 15:02 ` [PATCH 3/3] [media] venus: fix compile-test build on non-qcom ARM platform Arnd Bergmann @ 2017-06-27 19:41 ` Stanimir Varbanov 2 siblings, 0 replies; 8+ messages in thread From: Stanimir Varbanov @ 2017-06-27 19:41 UTC (permalink / raw) To: Arnd Bergmann, Stanimir Varbanov, Mauro Carvalho Chehab Cc: Hans Verkuil, Colin Ian King, linux-media, linux-arm-msm, linux-kernel Hi Arnd, Thanks for the catch! On 27.06.2017 18:02, Arnd Bergmann wrote: > Without PM support, gcc warns about two unused functions: > > platform/qcom/venus/core.c:146:13: error: 'venus_clks_disable' defined but not used [-Werror=unused-function] > platform/qcom/venus/core.c:126:12: error: 'venus_clks_enable' defined but not used [-Werror=unused-function] > > The problem as usual are incorrect #ifdefs, so the easiest fix > is to do away with the #ifdef completely and mark the suspend/resume > handlers as __maybe_unused, which they are. > > Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Reviewed-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/core.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index d8cbe8549d97..47f79637938c 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -270,8 +270,7 @@ static int venus_remove(struct platform_device *pdev) > return ret; > } > > -#ifdef CONFIG_PM > -static int venus_runtime_suspend(struct device *dev) > +static __maybe_unused int venus_runtime_suspend(struct device *dev) > { > struct venus_core *core = dev_get_drvdata(dev); > int ret; > @@ -283,7 +282,7 @@ static int venus_runtime_suspend(struct device *dev) > return ret; > } > > -static int venus_runtime_resume(struct device *dev) > +static __maybe_unused int venus_runtime_resume(struct device *dev) > { > struct venus_core *core = dev_get_drvdata(dev); > int ret; > @@ -302,7 +301,6 @@ static int venus_runtime_resume(struct device *dev) > venus_clks_disable(core); > return ret; > } > -#endif > > static const struct dev_pm_ops venus_pm_ops = { > SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-28 20:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-27 15:02 [PATCH 1/3] [media] venus: mark PM functions as __maybe_unused Arnd Bergmann 2017-06-27 15:02 ` [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations Arnd Bergmann 2017-06-27 19:39 ` Stanimir Varbanov 2017-06-27 20:15 ` Arnd Bergmann 2017-06-27 15:02 ` [PATCH 3/3] [media] venus: fix compile-test build on non-qcom ARM platform Arnd Bergmann 2017-06-27 19:45 ` Stanimir Varbanov 2017-06-28 20:25 ` Arnd Bergmann 2017-06-27 19:41 ` [PATCH 1/3] [media] venus: mark PM functions as __maybe_unused Stanimir Varbanov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).