diff mbox series

[1/4] venus: firmware: check fw size against DT memory region size

Message ID 20190109084616.17162-2-stanimir.varbanov@linaro.org (mailing list archive)
State New, archived
Headers show
Series Venus various fixes | expand

Commit Message

Stanimir Varbanov Jan. 9, 2019, 8:46 a.m. UTC
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(-)

Comments

Stanimir Varbanov Jan. 18, 2019, 4:20 p.m. UTC | #1
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;
>
Alexandre Courbot Jan. 23, 2019, 6:10 a.m. UTC | #2
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!
Stanimir Varbanov Jan. 23, 2019, 10:05 a.m. UTC | #3
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!
>
diff mbox series

Patch

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;