linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Venus various fixes
@ 2019-01-09  8:46 Stanimir Varbanov
  2019-01-09  8:46 ` [PATCH 1/4] venus: firmware: check fw size against DT memory region size Stanimir Varbanov
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Stanimir Varbanov @ 2019-01-09  8:46 UTC (permalink / raw)
  To: linux-media
  Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam,
	Stanimir Varbanov

Hello,

Here are four various fixes for venus driver.

Comments are welcome!

regards,
Stan

Stanimir Varbanov (4):
  venus: firmware: check fw size against DT memory region size
  venus: core: corect maximum hardware load for sdm845
  venus: core: correct frequency table for sdm845
  venus: helpers: drop setting of timestap invalid flag

 drivers/media/platform/qcom/venus/core.c     | 12 +++--
 drivers/media/platform/qcom/venus/core.h     |  1 +
 drivers/media/platform/qcom/venus/firmware.c | 54 +++++++++++---------
 drivers/media/platform/qcom/venus/helpers.c  |  3 --
 4 files changed, 38 insertions(+), 32 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/4] venus: firmware: check fw size against DT memory region size
  2019-01-09  8:46 [PATCH 0/4] Venus various fixes Stanimir Varbanov
@ 2019-01-09  8:46 ` Stanimir Varbanov
  2019-01-18 16:20   ` Stanimir Varbanov
  2019-01-23  6:10   ` Alexandre Courbot
  2019-01-09  8:46 ` [PATCH 2/4] venus: core: corect maximum hardware load for sdm845 Stanimir Varbanov
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 10+ messages in thread
From: Stanimir Varbanov @ 2019-01-09  8:46 UTC (permalink / raw)
  To: linux-media
  Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam,
	Stanimir Varbanov

By historical reasons we defined firmware memory size to be 6MB even
that the firmware size for all supported Venus versions is 5MBs. Correct
that by compare the required firmware size returned from mdt loader and
the one provided by DT reserved memory region. We proceed further if the
required firmware size is smaller than provided by DT memory region.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.h     |  1 +
 drivers/media/platform/qcom/venus/firmware.c | 54 +++++++++++---------
 2 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 6382cea29185..79c7e816c706 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -134,6 +134,7 @@ struct venus_core {
 	struct video_firmware {
 		struct device *dev;
 		struct iommu_domain *iommu_domain;
+		size_t mapped_mem_size;
 	} fw;
 	struct mutex lock;
 	struct list_head instances;
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index c29acfd70c1b..6b509ffd022a 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -35,14 +35,15 @@
 
 static void venus_reset_cpu(struct venus_core *core)
 {
+	u32 fw_size = core->fw.mapped_mem_size;
 	void __iomem *base = core->base;
 
 	writel(0, base + WRAPPER_FW_START_ADDR);
-	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR);
+	writel(fw_size, base + WRAPPER_FW_END_ADDR);
 	writel(0, base + WRAPPER_CPA_START_ADDR);
-	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR);
-	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_START_ADDR);
-	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_END_ADDR);
+	writel(fw_size, base + WRAPPER_CPA_END_ADDR);
+	writel(fw_size, base + WRAPPER_NONPIX_START_ADDR);
+	writel(fw_size, base + WRAPPER_NONPIX_END_ADDR);
 	writel(0x0, base + WRAPPER_CPU_CGC_DIS);
 	writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG);
 
@@ -74,6 +75,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
 	void *mem_va;
 	int ret;
 
+	*mem_phys = 0;
+	*mem_size = 0;
+
 	dev = core->dev;
 	node = of_parse_phandle(dev->of_node, "memory-region", 0);
 	if (!node) {
@@ -85,28 +89,30 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
 	if (ret)
 		return ret;
 
+	ret = request_firmware(&mdt, fwname, dev);
+	if (ret < 0)
+		return ret;
+
+	fw_size = qcom_mdt_get_size(mdt);
+	if (fw_size < 0) {
+		ret = fw_size;
+		goto err_release_fw;
+	}
+
 	*mem_phys = r.start;
 	*mem_size = resource_size(&r);
 
-	if (*mem_size < VENUS_FW_MEM_SIZE)
-		return -EINVAL;
+	if (*mem_size < fw_size || fw_size > VENUS_FW_MEM_SIZE) {
+		ret = -EINVAL;
+		goto err_release_fw;
+	}
 
 	mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
 	if (!mem_va) {
 		dev_err(dev, "unable to map memory region: %pa+%zx\n",
 			&r.start, *mem_size);
-		return -ENOMEM;
-	}
-
-	ret = request_firmware(&mdt, fwname, dev);
-	if (ret < 0)
-		goto err_unmap;
-
-	fw_size = qcom_mdt_get_size(mdt);
-	if (fw_size < 0) {
-		ret = fw_size;
-		release_firmware(mdt);
-		goto err_unmap;
+		ret = -ENOMEM;
+		goto err_release_fw;
 	}
 
 	if (core->use_tz)
@@ -116,10 +122,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
 		ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
 					    mem_va, *mem_phys, *mem_size, NULL);
 
-	release_firmware(mdt);
-
-err_unmap:
 	memunmap(mem_va);
+err_release_fw:
+	release_firmware(mdt);
 	return ret;
 }
 
@@ -135,6 +140,7 @@ static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
 		return -EPROBE_DEFER;
 
 	iommu = core->fw.iommu_domain;
+	core->fw.mapped_mem_size = mem_size;
 
 	ret = iommu_map(iommu, VENUS_FW_START_ADDR, mem_phys, mem_size,
 			IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV);
@@ -151,7 +157,7 @@ static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
 static int venus_shutdown_no_tz(struct venus_core *core)
 {
 	struct iommu_domain *iommu;
-	size_t unmapped;
+	size_t unmapped, mapped = core->fw.mapped_mem_size;
 	u32 reg;
 	struct device *dev = core->fw.dev;
 	void __iomem *base = core->base;
@@ -166,8 +172,8 @@ static int venus_shutdown_no_tz(struct venus_core *core)
 
 	iommu = core->fw.iommu_domain;
 
-	unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE);
-	if (unmapped != VENUS_FW_MEM_SIZE)
+	unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
+	if (unmapped != mapped)
 		dev_err(dev, "failed to unmap firmware\n");
 
 	return 0;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/4] venus: core: corect maximum hardware load for sdm845
  2019-01-09  8:46 [PATCH 0/4] Venus various fixes Stanimir Varbanov
  2019-01-09  8:46 ` [PATCH 1/4] venus: firmware: check fw size against DT memory region size Stanimir Varbanov
@ 2019-01-09  8:46 ` Stanimir Varbanov
  2019-01-23  6:09   ` Alexandre Courbot
  2019-01-09  8:46 ` [PATCH 3/4] venus: core: correct frequency table " Stanimir Varbanov
  2019-01-09  8:46 ` [PATCH 4/4] venus: helpers: drop setting of timestap invalid flag Stanimir Varbanov
  3 siblings, 1 reply; 10+ messages in thread
From: Stanimir Varbanov @ 2019-01-09  8:46 UTC (permalink / raw)
  To: linux-media
  Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam,
	Stanimir Varbanov

This corects maximum hardware load constant in per SoC resources
for sdm845 aka Venus v4.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index cb411eb85ee4..d95185ea32c3 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -455,7 +455,7 @@ static const struct venus_resources msm8996_res = {
 	.reg_tbl_size = ARRAY_SIZE(msm8996_reg_preset),
 	.clks = {"core", "iface", "bus", "mbus" },
 	.clks_num = 4,
-	.max_load = 2563200,
+	.max_load = 3110400,	/* 4096x2160@90 */
 	.hfi_version = HFI_VERSION_3XX,
 	.vmem_id = VIDC_RESOURCE_NONE,
 	.vmem_size = 0,
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/4] venus: core: correct frequency table for sdm845
  2019-01-09  8:46 [PATCH 0/4] Venus various fixes Stanimir Varbanov
  2019-01-09  8:46 ` [PATCH 1/4] venus: firmware: check fw size against DT memory region size Stanimir Varbanov
  2019-01-09  8:46 ` [PATCH 2/4] venus: core: corect maximum hardware load for sdm845 Stanimir Varbanov
@ 2019-01-09  8:46 ` Stanimir Varbanov
  2019-01-09  8:46 ` [PATCH 4/4] venus: helpers: drop setting of timestap invalid flag Stanimir Varbanov
  3 siblings, 0 replies; 10+ messages in thread
From: Stanimir Varbanov @ 2019-01-09  8:46 UTC (permalink / raw)
  To: linux-media
  Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam,
	Stanimir Varbanov

This corrects clock frequency table rates to be in sync
with video clock controller frequency table.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index d95185ea32c3..739366744e0f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -465,10 +465,12 @@ static const struct venus_resources msm8996_res = {
 };
 
 static const struct freq_tbl sdm845_freq_table[] = {
-	{ 1944000, 380000000 },	/* 4k UHD @ 60 */
-	{  972000, 320000000 },	/* 4k UHD @ 30 */
-	{  489600, 200000000 },	/* 1080p @ 60 */
-	{  244800, 100000000 },	/* 1080p @ 30 */
+	{ 3110400, 533000000 },	/* 4096x2160@90 */
+	{ 2073600, 444000000 },	/* 4096x2160@60 */
+	{ 1944000, 404000000 },	/* 3840x2160@60 */
+	{  972000, 330000000 },	/* 3840x2160@30 */
+	{  489600, 200000000 },	/* 1920x1080@60 */
+	{  244800, 100000000 },	/* 1920x1080@30 */
 };
 
 static const struct venus_resources sdm845_res = {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/4] venus: helpers: drop setting of timestap invalid flag
  2019-01-09  8:46 [PATCH 0/4] Venus various fixes Stanimir Varbanov
                   ` (2 preceding siblings ...)
  2019-01-09  8:46 ` [PATCH 3/4] venus: core: correct frequency table " Stanimir Varbanov
@ 2019-01-09  8:46 ` Stanimir Varbanov
  2019-01-23  6:09   ` Alexandre Courbot
  3 siblings, 1 reply; 10+ messages in thread
From: Stanimir Varbanov @ 2019-01-09  8:46 UTC (permalink / raw)
  To: linux-media
  Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam,
	Stanimir Varbanov

The zero timestap is really a valid so not sure why I discarded it. Fix
that mistake by drop the code which checks for zero timestamp.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/helpers.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index e436385bc5ab..5cad601d4c57 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -439,9 +439,6 @@ session_process_buf(struct venus_inst *inst, struct vb2_v4l2_buffer *vbuf)
 	fdata.flags = 0;
 	fdata.clnt_data = vbuf->vb2_buf.index;
 
-	if (!fdata.timestamp)
-		fdata.flags |= HFI_BUFFERFLAG_TIMESTAMPINVALID;
-
 	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		fdata.buffer_type = HFI_BUFFER_INPUT;
 		fdata.filled_len = vb2_get_plane_payload(vb, 0);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] venus: firmware: check fw size against DT memory region size
  2019-01-09  8:46 ` [PATCH 1/4] venus: firmware: check fw size against DT memory region size Stanimir Varbanov
@ 2019-01-18 16:20   ` Stanimir Varbanov
  2019-01-23  6:10   ` Alexandre Courbot
  1 sibling, 0 replies; 10+ messages in thread
From: Stanimir Varbanov @ 2019-01-18 16:20 UTC (permalink / raw)
  To: linux-media
  Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam

Hi Alex,

If you have time please review this patch, I'd like to send a pull
request for this series.

On 1/9/19 10:46 AM, Stanimir Varbanov wrote:
> By historical reasons we defined firmware memory size to be 6MB even
> that the firmware size for all supported Venus versions is 5MBs. Correct
> that by compare the required firmware size returned from mdt loader and
> the one provided by DT reserved memory region. We proceed further if the
> required firmware size is smaller than provided by DT memory region.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.h     |  1 +
>  drivers/media/platform/qcom/venus/firmware.c | 54 +++++++++++---------
>  2 files changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 6382cea29185..79c7e816c706 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -134,6 +134,7 @@ struct venus_core {
>  	struct video_firmware {
>  		struct device *dev;
>  		struct iommu_domain *iommu_domain;
> +		size_t mapped_mem_size;
>  	} fw;
>  	struct mutex lock;
>  	struct list_head instances;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index c29acfd70c1b..6b509ffd022a 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -35,14 +35,15 @@
>  
>  static void venus_reset_cpu(struct venus_core *core)
>  {
> +	u32 fw_size = core->fw.mapped_mem_size;
>  	void __iomem *base = core->base;
>  
>  	writel(0, base + WRAPPER_FW_START_ADDR);
> -	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR);
> +	writel(fw_size, base + WRAPPER_FW_END_ADDR);
>  	writel(0, base + WRAPPER_CPA_START_ADDR);
> -	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR);
> -	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_START_ADDR);
> -	writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_END_ADDR);
> +	writel(fw_size, base + WRAPPER_CPA_END_ADDR);
> +	writel(fw_size, base + WRAPPER_NONPIX_START_ADDR);
> +	writel(fw_size, base + WRAPPER_NONPIX_END_ADDR);
>  	writel(0x0, base + WRAPPER_CPU_CGC_DIS);
>  	writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG);
>  
> @@ -74,6 +75,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  	void *mem_va;
>  	int ret;
>  
> +	*mem_phys = 0;
> +	*mem_size = 0;
> +
>  	dev = core->dev;
>  	node = of_parse_phandle(dev->of_node, "memory-region", 0);
>  	if (!node) {
> @@ -85,28 +89,30 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  	if (ret)
>  		return ret;
>  
> +	ret = request_firmware(&mdt, fwname, dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	fw_size = qcom_mdt_get_size(mdt);
> +	if (fw_size < 0) {
> +		ret = fw_size;
> +		goto err_release_fw;
> +	}
> +
>  	*mem_phys = r.start;
>  	*mem_size = resource_size(&r);
>  
> -	if (*mem_size < VENUS_FW_MEM_SIZE)
> -		return -EINVAL;
> +	if (*mem_size < fw_size || fw_size > VENUS_FW_MEM_SIZE) {
> +		ret = -EINVAL;
> +		goto err_release_fw;
> +	}
>  
>  	mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
>  	if (!mem_va) {
>  		dev_err(dev, "unable to map memory region: %pa+%zx\n",
>  			&r.start, *mem_size);
> -		return -ENOMEM;
> -	}
> -
> -	ret = request_firmware(&mdt, fwname, dev);
> -	if (ret < 0)
> -		goto err_unmap;
> -
> -	fw_size = qcom_mdt_get_size(mdt);
> -	if (fw_size < 0) {
> -		ret = fw_size;
> -		release_firmware(mdt);
> -		goto err_unmap;
> +		ret = -ENOMEM;
> +		goto err_release_fw;
>  	}
>  
>  	if (core->use_tz)
> @@ -116,10 +122,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  		ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
>  					    mem_va, *mem_phys, *mem_size, NULL);
>  
> -	release_firmware(mdt);
> -
> -err_unmap:
>  	memunmap(mem_va);
> +err_release_fw:
> +	release_firmware(mdt);
>  	return ret;
>  }
>  
> @@ -135,6 +140,7 @@ static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
>  		return -EPROBE_DEFER;
>  
>  	iommu = core->fw.iommu_domain;
> +	core->fw.mapped_mem_size = mem_size;
>  
>  	ret = iommu_map(iommu, VENUS_FW_START_ADDR, mem_phys, mem_size,
>  			IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV);
> @@ -151,7 +157,7 @@ static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
>  static int venus_shutdown_no_tz(struct venus_core *core)
>  {
>  	struct iommu_domain *iommu;
> -	size_t unmapped;
> +	size_t unmapped, mapped = core->fw.mapped_mem_size;
>  	u32 reg;
>  	struct device *dev = core->fw.dev;
>  	void __iomem *base = core->base;
> @@ -166,8 +172,8 @@ static int venus_shutdown_no_tz(struct venus_core *core)
>  
>  	iommu = core->fw.iommu_domain;
>  
> -	unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE);
> -	if (unmapped != VENUS_FW_MEM_SIZE)
> +	unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
> +	if (unmapped != mapped)
>  		dev_err(dev, "failed to unmap firmware\n");
>  
>  	return 0;
> 

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/4] venus: core: corect maximum hardware load for sdm845
  2019-01-09  8:46 ` [PATCH 2/4] venus: core: corect maximum hardware load for sdm845 Stanimir Varbanov
@ 2019-01-23  6:09   ` Alexandre Courbot
  0 siblings, 0 replies; 10+ messages in thread
From: Alexandre Courbot @ 2019-01-23  6:09 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil,
	LKML, linux-arm-msm, Vikash Garodia, Tomasz Figa, Malathi Gottam

On Wed, Jan 9, 2019 at 5:46 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> This corects maximum hardware load constant in per SoC resources

s/corect/correct. Same typo is present in patch title.


> for sdm845 aka Venus v4.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index cb411eb85ee4..d95185ea32c3 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -455,7 +455,7 @@ static const struct venus_resources msm8996_res = {
>         .reg_tbl_size = ARRAY_SIZE(msm8996_reg_preset),
>         .clks = {"core", "iface", "bus", "mbus" },
>         .clks_num = 4,
> -       .max_load = 2563200,
> +       .max_load = 3110400,    /* 4096x2160@90 */
>         .hfi_version = HFI_VERSION_3XX,
>         .vmem_id = VIDC_RESOURCE_NONE,
>         .vmem_size = 0,
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] venus: helpers: drop setting of timestap invalid flag
  2019-01-09  8:46 ` [PATCH 4/4] venus: helpers: drop setting of timestap invalid flag Stanimir Varbanov
@ 2019-01-23  6:09   ` Alexandre Courbot
  0 siblings, 0 replies; 10+ messages in thread
From: Alexandre Courbot @ 2019-01-23  6:09 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil,
	LKML, linux-arm-msm, Vikash Garodia, Tomasz Figa, Malathi Gottam

On Wed, Jan 9, 2019 at 5:46 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> The zero timestap is really a valid so not sure why I discarded it. Fix

s/timestap/timestamp, also in patch title.

> that mistake by drop the code which checks for zero timestamp.

s/drop/dropping


>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index e436385bc5ab..5cad601d4c57 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -439,9 +439,6 @@ session_process_buf(struct venus_inst *inst, struct vb2_v4l2_buffer *vbuf)
>         fdata.flags = 0;
>         fdata.clnt_data = vbuf->vb2_buf.index;
>
> -       if (!fdata.timestamp)
> -               fdata.flags |= HFI_BUFFERFLAG_TIMESTAMPINVALID;
> -
>         if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>                 fdata.buffer_type = HFI_BUFFER_INPUT;
>                 fdata.filled_len = vb2_get_plane_payload(vb, 0);
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] venus: firmware: check fw size against DT memory region size
  2019-01-09  8:46 ` [PATCH 1/4] venus: firmware: check fw size against DT memory region size Stanimir Varbanov
  2019-01-18 16:20   ` Stanimir Varbanov
@ 2019-01-23  6:10   ` Alexandre Courbot
  2019-01-23 10:05     ` Stanimir Varbanov
  1 sibling, 1 reply; 10+ messages in thread
From: Alexandre Courbot @ 2019-01-23  6:10 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil,
	LKML, linux-arm-msm, Vikash Garodia, Tomasz Figa, Malathi Gottam

Sorry for the delayed review! >_<

On Wed, Jan 9, 2019 at 5:46 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> By historical reasons we defined firmware memory size to be 6MB even
> that the firmware size for all supported Venus versions is 5MBs. Correct
> that by compare the required firmware size returned from mdt loader and
> the one provided by DT reserved memory region. We proceed further if the
> required firmware size is smaller than provided by DT memory region.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.h     |  1 +
>  drivers/media/platform/qcom/venus/firmware.c | 54 +++++++++++---------
>  2 files changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 6382cea29185..79c7e816c706 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -134,6 +134,7 @@ struct venus_core {
>         struct video_firmware {
>                 struct device *dev;
>                 struct iommu_domain *iommu_domain;
> +               size_t mapped_mem_size;
>         } fw;
>         struct mutex lock;
>         struct list_head instances;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index c29acfd70c1b..6b509ffd022a 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -35,14 +35,15 @@
>
>  static void venus_reset_cpu(struct venus_core *core)
>  {
> +       u32 fw_size = core->fw.mapped_mem_size;
>         void __iomem *base = core->base;
>
>         writel(0, base + WRAPPER_FW_START_ADDR);
> -       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR);
> +       writel(fw_size, base + WRAPPER_FW_END_ADDR);
>         writel(0, base + WRAPPER_CPA_START_ADDR);
> -       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR);
> -       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_START_ADDR);
> -       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_END_ADDR);
> +       writel(fw_size, base + WRAPPER_CPA_END_ADDR);
> +       writel(fw_size, base + WRAPPER_NONPIX_START_ADDR);
> +       writel(fw_size, base + WRAPPER_NONPIX_END_ADDR);
>         writel(0x0, base + WRAPPER_CPU_CGC_DIS);
>         writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG);
>
> @@ -74,6 +75,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>         void *mem_va;
>         int ret;
>
> +       *mem_phys = 0;
> +       *mem_size = 0;
> +
>         dev = core->dev;
>         node = of_parse_phandle(dev->of_node, "memory-region", 0);
>         if (!node) {
> @@ -85,28 +89,30 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>         if (ret)
>                 return ret;
>
> +       ret = request_firmware(&mdt, fwname, dev);
> +       if (ret < 0)
> +               return ret;
> +
> +       fw_size = qcom_mdt_get_size(mdt);
> +       if (fw_size < 0) {
> +               ret = fw_size;
> +               goto err_release_fw;
> +       }
> +
>         *mem_phys = r.start;
>         *mem_size = resource_size(&r);
>
> -       if (*mem_size < VENUS_FW_MEM_SIZE)
> -               return -EINVAL;
> +       if (*mem_size < fw_size || fw_size > VENUS_FW_MEM_SIZE) {

Do we still need to check for fw_size > VENUS_FW_MEM_SIZE ? If we
don't then we can remove the definition of VENUS_FW_MEM_SIZE
altogether.

> +               ret = -EINVAL;
> +               goto err_release_fw;
> +       }
>
>         mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
>         if (!mem_va) {
>                 dev_err(dev, "unable to map memory region: %pa+%zx\n",
>                         &r.start, *mem_size);
> -               return -ENOMEM;
> -       }
> -
> -       ret = request_firmware(&mdt, fwname, dev);
> -       if (ret < 0)
> -               goto err_unmap;
> -
> -       fw_size = qcom_mdt_get_size(mdt);
> -       if (fw_size < 0) {
> -               ret = fw_size;
> -               release_firmware(mdt);
> -               goto err_unmap;
> +               ret = -ENOMEM;
> +               goto err_release_fw;
>         }
>
>         if (core->use_tz)
> @@ -116,10 +122,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>                 ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
>                                             mem_va, *mem_phys, *mem_size, NULL);
>
> -       release_firmware(mdt);
> -
> -err_unmap:
>         memunmap(mem_va);
> +err_release_fw:
> +       release_firmware(mdt);
>         return ret;
>  }
>
> @@ -135,6 +140,7 @@ static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
>                 return -EPROBE_DEFER;
>
>         iommu = core->fw.iommu_domain;
> +       core->fw.mapped_mem_size = mem_size;
>
>         ret = iommu_map(iommu, VENUS_FW_START_ADDR, mem_phys, mem_size,
>                         IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV);
> @@ -151,7 +157,7 @@ static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
>  static int venus_shutdown_no_tz(struct venus_core *core)
>  {
>         struct iommu_domain *iommu;
> -       size_t unmapped;
> +       size_t unmapped, mapped = core->fw.mapped_mem_size;

mapped should probably be const here.

With these minor comments:

Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Tested-by: Alexandre Courbot <acourbot@chromium.org>

For the 4 patches in this series. I could see the improvement in
decoder performance introduced by patches 2 and 3, thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] venus: firmware: check fw size against DT memory region size
  2019-01-23  6:10   ` Alexandre Courbot
@ 2019-01-23 10:05     ` Stanimir Varbanov
  0 siblings, 0 replies; 10+ messages in thread
From: Stanimir Varbanov @ 2019-01-23 10:05 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil,
	LKML, linux-arm-msm, Vikash Garodia, Tomasz Figa, Malathi Gottam

Hi Alex,

Thanks for the review!

On 1/23/19 8:10 AM, Alexandre Courbot wrote:
> Sorry for the delayed review! >_<
> 
> On Wed, Jan 9, 2019 at 5:46 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> By historical reasons we defined firmware memory size to be 6MB even
>> that the firmware size for all supported Venus versions is 5MBs. Correct
>> that by compare the required firmware size returned from mdt loader and
>> the one provided by DT reserved memory region. We proceed further if the
>> required firmware size is smaller than provided by DT memory region.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/core.h     |  1 +
>>  drivers/media/platform/qcom/venus/firmware.c | 54 +++++++++++---------
>>  2 files changed, 31 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> index 6382cea29185..79c7e816c706 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -134,6 +134,7 @@ struct venus_core {
>>         struct video_firmware {
>>                 struct device *dev;
>>                 struct iommu_domain *iommu_domain;
>> +               size_t mapped_mem_size;
>>         } fw;
>>         struct mutex lock;
>>         struct list_head instances;
>> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
>> index c29acfd70c1b..6b509ffd022a 100644
>> --- a/drivers/media/platform/qcom/venus/firmware.c
>> +++ b/drivers/media/platform/qcom/venus/firmware.c
>> @@ -35,14 +35,15 @@
>>
>>  static void venus_reset_cpu(struct venus_core *core)
>>  {
>> +       u32 fw_size = core->fw.mapped_mem_size;
>>         void __iomem *base = core->base;
>>
>>         writel(0, base + WRAPPER_FW_START_ADDR);
>> -       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_FW_END_ADDR);
>> +       writel(fw_size, base + WRAPPER_FW_END_ADDR);
>>         writel(0, base + WRAPPER_CPA_START_ADDR);
>> -       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_CPA_END_ADDR);
>> -       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_START_ADDR);
>> -       writel(VENUS_FW_MEM_SIZE, base + WRAPPER_NONPIX_END_ADDR);
>> +       writel(fw_size, base + WRAPPER_CPA_END_ADDR);
>> +       writel(fw_size, base + WRAPPER_NONPIX_START_ADDR);
>> +       writel(fw_size, base + WRAPPER_NONPIX_END_ADDR);
>>         writel(0x0, base + WRAPPER_CPU_CGC_DIS);
>>         writel(0x0, base + WRAPPER_CPU_CLOCK_CONFIG);
>>
>> @@ -74,6 +75,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>>         void *mem_va;
>>         int ret;
>>
>> +       *mem_phys = 0;
>> +       *mem_size = 0;
>> +
>>         dev = core->dev;
>>         node = of_parse_phandle(dev->of_node, "memory-region", 0);
>>         if (!node) {
>> @@ -85,28 +89,30 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>>         if (ret)
>>                 return ret;
>>
>> +       ret = request_firmware(&mdt, fwname, dev);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       fw_size = qcom_mdt_get_size(mdt);
>> +       if (fw_size < 0) {
>> +               ret = fw_size;
>> +               goto err_release_fw;
>> +       }
>> +
>>         *mem_phys = r.start;
>>         *mem_size = resource_size(&r);
>>
>> -       if (*mem_size < VENUS_FW_MEM_SIZE)
>> -               return -EINVAL;
>> +       if (*mem_size < fw_size || fw_size > VENUS_FW_MEM_SIZE) {
> 
> Do we still need to check for fw_size > VENUS_FW_MEM_SIZE ? If we
> don't then we can remove the definition of VENUS_FW_MEM_SIZE
> altogether.

I know, it is a bit paranoid but I'd want to avoid if someone set
unreasonable memory size. So I'd like to have some sanitized firmware
region size in the driver.

> 
>> +               ret = -EINVAL;
>> +               goto err_release_fw;
>> +       }
>>
>>         mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
>>         if (!mem_va) {
>>                 dev_err(dev, "unable to map memory region: %pa+%zx\n",
>>                         &r.start, *mem_size);
>> -               return -ENOMEM;
>> -       }
>> -
>> -       ret = request_firmware(&mdt, fwname, dev);
>> -       if (ret < 0)
>> -               goto err_unmap;
>> -
>> -       fw_size = qcom_mdt_get_size(mdt);
>> -       if (fw_size < 0) {
>> -               ret = fw_size;
>> -               release_firmware(mdt);
>> -               goto err_unmap;
>> +               ret = -ENOMEM;
>> +               goto err_release_fw;
>>         }
>>
>>         if (core->use_tz)
>> @@ -116,10 +122,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>>                 ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
>>                                             mem_va, *mem_phys, *mem_size, NULL);
>>
>> -       release_firmware(mdt);
>> -
>> -err_unmap:
>>         memunmap(mem_va);
>> +err_release_fw:
>> +       release_firmware(mdt);
>>         return ret;
>>  }
>>
>> @@ -135,6 +140,7 @@ static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
>>                 return -EPROBE_DEFER;
>>
>>         iommu = core->fw.iommu_domain;
>> +       core->fw.mapped_mem_size = mem_size;
>>
>>         ret = iommu_map(iommu, VENUS_FW_START_ADDR, mem_phys, mem_size,
>>                         IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV);
>> @@ -151,7 +157,7 @@ static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys,
>>  static int venus_shutdown_no_tz(struct venus_core *core)
>>  {
>>         struct iommu_domain *iommu;
>> -       size_t unmapped;
>> +       size_t unmapped, mapped = core->fw.mapped_mem_size;
> 
> mapped should probably be const here.

Ok.

> 
> With these minor comments:
> 
> Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
> Tested-by: Alexandre Courbot <acourbot@chromium.org>
> 
> For the 4 patches in this series. I could see the improvement in
> decoder performance introduced by patches 2 and 3, thanks!
> 

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-01-23 10:06 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-09  8:46 [PATCH 0/4] Venus various fixes Stanimir Varbanov
2019-01-09  8:46 ` [PATCH 1/4] venus: firmware: check fw size against DT memory region size Stanimir Varbanov
2019-01-18 16:20   ` Stanimir Varbanov
2019-01-23  6:10   ` Alexandre Courbot
2019-01-23 10:05     ` Stanimir Varbanov
2019-01-09  8:46 ` [PATCH 2/4] venus: core: corect maximum hardware load for sdm845 Stanimir Varbanov
2019-01-23  6:09   ` Alexandre Courbot
2019-01-09  8:46 ` [PATCH 3/4] venus: core: correct frequency table " Stanimir Varbanov
2019-01-09  8:46 ` [PATCH 4/4] venus: helpers: drop setting of timestap invalid flag Stanimir Varbanov
2019-01-23  6:09   ` Alexandre Courbot

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).