| Message ID | 1535034528-11590-1-git-send-email-vgarodia@codeaurora.org |
|---|---|
| Headers | show |
| Series | Venus updates - PIL | expand |
On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > > Add routine to reset the ARM9 and brings it out of reset. Also > abstract the Venus CPU state handling with a new function. This > is in preparation to add PIL functionality in venus driver. > > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/media/platform/qcom/venus/core.h | 2 ++ > drivers/media/platform/qcom/venus/firmware.c | 33 ++++++++++++++++++++++++ > drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ > drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- > drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ > 5 files changed, 57 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 2f02365..dfd5c10 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -98,6 +98,7 @@ struct venus_caps { > * @dev: convenience struct device pointer > * @dev_dec: convenience struct device pointer for decoder device > * @dev_enc: convenience struct device pointer for encoder device > + * @no_tz: a flag that suggests presence of trustzone > * @lock: a lock for this strucure > * @instances: a list_head of all instances > * @insts_count: num of instances > @@ -129,6 +130,7 @@ struct venus_core { > struct device *dev; > struct device *dev_dec; > struct device *dev_enc; > + bool no_tz; > struct mutex lock; > struct list_head instances; > atomic_t insts_count; > diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > index c4a5778..a9d042e 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -22,10 +22,43 @@ > #include <linux/sizes.h> > #include <linux/soc/qcom/mdt_loader.h> > > +#include "core.h" > #include "firmware.h" > +#include "hfi_venus_io.h" > > #define VENUS_PAS_ID 9 > #define VENUS_FW_MEM_SIZE (6 * SZ_1M) This is making a strong assumption about the size of the FW memory region, which in practice is not always true (I had to reduce it to 5MB). How about having this as a member of venus_core, which is initialized in venus_load_fw() from the actual size of the memory region? You could do this as an extra patch that comes before this one.
On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > > Separate firmware loading part into a new function. > > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/media/platform/qcom/venus/core.c | 4 +- > drivers/media/platform/qcom/venus/firmware.c | 55 ++++++++++++++++++---------- > drivers/media/platform/qcom/venus/firmware.h | 2 +- > 3 files changed, 38 insertions(+), 23 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index bb6add9..75b9785 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -84,7 +84,7 @@ static void venus_sys_error_handler(struct work_struct *work) > > pm_runtime_get_sync(core->dev); > > - ret |= venus_boot(core->dev, core->res->fwname); > + ret |= venus_boot(core); > > ret |= hfi_core_resume(core, true); > > @@ -284,7 +284,7 @@ static int venus_probe(struct platform_device *pdev) > if (ret < 0) > goto err_runtime_disable; > > - ret = venus_boot(dev, core->res->fwname); > + ret = venus_boot(core); > if (ret) > goto err_runtime_disable; > > diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > index a9d042e..34224eb 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -60,20 +60,18 @@ int venus_set_hw_state(struct venus_core *core, bool resume) > return 0; > } > > -int venus_boot(struct device *dev, const char *fwname) > +static int venus_load_fw(struct venus_core *core, const char *fwname, > + phys_addr_t *mem_phys, size_t *mem_size) Following the remarks of the previous patch, you would have mem_phys and mem_size as members of venus_core (probably renamed as fw_mem_addr and fw_mem_size). > { > const struct firmware *mdt; > struct device_node *node; > - phys_addr_t mem_phys; > + struct device *dev; > struct resource r; > ssize_t fw_size; > - size_t mem_size; > void *mem_va; > int ret; > > - if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || !qcom_scm_is_available()) > - return -EPROBE_DEFER; !IS_ENABLED(CONFIG_QCOM_MDT_LOADER) is not a condition that can change at runtime, and returning -EPROBE_DEFER in that case seems erroneous to me. Instead, wouldn't it make more sense to make the driver depend on QCOM_MDT_LOADER? > - > + dev = core->dev; > node = of_parse_phandle(dev->of_node, "memory-region", 0); > if (!node) { > dev_err(dev, "no memory-region specified\n"); > @@ -84,16 +82,16 @@ int venus_boot(struct device *dev, const char *fwname) > if (ret) > return ret; > > - mem_phys = r.start; > - mem_size = resource_size(&r); > + *mem_phys = r.start; > + *mem_size = resource_size(&r); > > - if (mem_size < VENUS_FW_MEM_SIZE) > + if (*mem_size < VENUS_FW_MEM_SIZE) > return -EINVAL; > > - mem_va = memremap(r.start, mem_size, MEMREMAP_WC); > + 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); > + &r.start, *mem_size); > return -ENOMEM; > } > > @@ -108,23 +106,40 @@ int venus_boot(struct device *dev, const char *fwname) > goto err_unmap; > } > > - ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys, > - mem_size, NULL); > + if (core->no_tz) > + ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID, > + mem_va, *mem_phys, *mem_size, NULL); > + else > + ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, > + mem_va, *mem_phys, *mem_size, NULL); > > release_firmware(mdt); > > - if (ret) > - goto err_unmap; > - > - ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID); > - if (ret) > - goto err_unmap; > - > err_unmap: > memunmap(mem_va); > return ret; > } > > +int venus_boot(struct venus_core *core) > +{ > + struct device *dev = core->dev; > + phys_addr_t mem_phys; > + size_t mem_size; > + int ret; > + > + if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || > + (!core->no_tz && !qcom_scm_is_available())) > + return -EPROBE_DEFER; Same remark as above here.
On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > > Video hardware is mainly comprised of vcodec subsystem and video > control subsystem. Video control has ARM9 which executes the video > firmware instructions whereas vcodec does the video frame processing. > This change adds support to load the video firmware and bring ARM9 > out of reset for platforms which does not have trustzone. > > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/media/platform/qcom/venus/core.c | 6 +- > drivers/media/platform/qcom/venus/core.h | 7 ++ > drivers/media/platform/qcom/venus/firmware.c | 90 +++++++++++++++++++++++- > drivers/media/platform/qcom/venus/firmware.h | 2 +- > drivers/media/platform/qcom/venus/hfi_venus_io.h | 1 + > 5 files changed, 99 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index 75b9785..393994e 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -76,7 +76,7 @@ static void venus_sys_error_handler(struct work_struct *work) > hfi_core_deinit(core, true); > hfi_destroy(core); > mutex_lock(&core->lock); > - venus_shutdown(core->dev); > + venus_shutdown(core); > > pm_runtime_put_sync(core->dev); > > @@ -323,7 +323,7 @@ static int venus_probe(struct platform_device *pdev) > err_core_deinit: > hfi_core_deinit(core, false); > err_venus_shutdown: > - venus_shutdown(dev); > + venus_shutdown(core); > err_runtime_disable: > pm_runtime_set_suspended(dev); > pm_runtime_disable(dev); > @@ -344,7 +344,7 @@ static int venus_remove(struct platform_device *pdev) > WARN_ON(ret); > > hfi_destroy(core); > - venus_shutdown(dev); > + venus_shutdown(core); > of_platform_depopulate(dev); > > pm_runtime_put_sync(dev); > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index dfd5c10..b2cb359 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -81,6 +81,11 @@ struct venus_caps { > bool valid; /* used only for Venus v1xx */ > }; > > +struct video_firmware { > + struct device *dev; > + struct iommu_domain *iommu_domain; > +}; > + > /** > * struct venus_core - holds core parameters valid for all instances > * > @@ -98,6 +103,7 @@ struct venus_caps { > * @dev: convenience struct device pointer > * @dev_dec: convenience struct device pointer for decoder device > * @dev_enc: convenience struct device pointer for encoder device > + * @fw: a struct for venus firmware info > * @no_tz: a flag that suggests presence of trustzone > * @lock: a lock for this strucure > * @instances: a list_head of all instances > @@ -130,6 +136,7 @@ struct venus_core { > struct device *dev; > struct device *dev_dec; > struct device *dev_enc; > + struct video_firmware fw; Since struct video_firmware is only used here I think you can declare it inline, i.e. struct { struct device *dev; struct iommu_domain *iommu_domain; } fw; This structure is actually a good candidate to hold the firmware memory area start address and size. > bool no_tz; > 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 34224eb..79b3858 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -15,9 +15,11 @@ > #include <linux/device.h> > #include <linux/firmware.h> > #include <linux/kernel.h> > +#include <linux/iommu.h> > #include <linux/io.h> > #include <linux/of.h> > #include <linux/of_address.h> > +#include <linux/platform_device.h> > #include <linux/qcom_scm.h> > #include <linux/sizes.h> > #include <linux/soc/qcom/mdt_loader.h> > @@ -120,6 +122,76 @@ static int venus_load_fw(struct venus_core *core, const char *fwname, > return ret; > } > > +static int venus_boot_no_tz(struct venus_core *core, phys_addr_t mem_phys, > + size_t mem_size) After moving the firmware address and size into venus_core you won't need the last two arguments. > +{ > + struct iommu_domain *iommu_dom; > + struct device *dev; > + int ret; > + > + dev = core->fw.dev; > + if (!dev) > + return -EPROBE_DEFER; > + > + iommu_dom = iommu_domain_alloc(&platform_bus_type); > + if (!iommu_dom) { > + dev_err(dev, "Failed to allocate iommu domain\n"); > + return -ENOMEM; > + } > + > + ret = iommu_attach_device(iommu_dom, dev); > + if (ret) { > + dev_err(dev, "could not attach device\n"); > + goto err_attach; > + } I think like the above belongs more in venus_firmware_init() (introduced in patch 4/4) than here. There is no reason to detach/reattach the iommu if we stop the firmware. > + > + ret = iommu_map(iommu_dom, VENUS_FW_START_ADDR, mem_phys, mem_size, > + IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV); > + if (ret) { > + dev_err(dev, "could not map video firmware region\n"); > + goto err_map; > + } Maybe this too? > + > + core->fw.iommu_domain = iommu_dom; > + venus_reset_cpu(core); > + > + return 0; > + > +err_map: > + iommu_detach_device(iommu_dom, dev); > +err_attach: > + iommu_domain_free(iommu_dom); > + return ret; > +} > + > +static int venus_shutdown_no_tz(struct venus_core *core) > +{ > + struct iommu_domain *iommu; > + size_t unmapped; > + u32 reg; > + struct device *dev = core->fw.dev; > + void __iomem *base = core->base; > + > + /* Assert the reset to ARM9 */ > + reg = readl_relaxed(base + WRAPPER_A9SS_SW_RESET); > + reg |= WRAPPER_A9SS_SW_RESET_BIT; > + writel_relaxed(reg, base + WRAPPER_A9SS_SW_RESET); > + > + /* Make sure reset is asserted before the mapping is removed */ > + mb(); > + > + iommu = core->fw.iommu_domain; > + > + unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE); > + if (unmapped != VENUS_FW_MEM_SIZE) > + dev_err(dev, "failed to unmap firmware\n"); > + > + iommu_detach_device(iommu, dev); > + iommu_domain_free(iommu); Accordingly, this would also be moved into venus_firmware_deinit().
Hi Vikash, On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > > Hello, > > Here is v6 with following comments addressed: > > * 4/4 from earlier series was dropped as .probe was not needed. > * indentation as per checkpatch --strict option. > * tested on Venus v4 hardware. I have tested this series and it seems to be working fine! Thanks for pushing it forward! I have made a few comments inline, but some may be difficult to apply without reorganizing the series a bit. If my explanations are not clear, I can take care of submitting the next spin of this series if you wish.
Hi Alex, On 08/24/2018 10:38 AM, Alexandre Courbot wrote: > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: >> >> Add routine to reset the ARM9 and brings it out of reset. Also >> abstract the Venus CPU state handling with a new function. This >> is in preparation to add PIL functionality in venus driver. >> >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 2 ++ >> drivers/media/platform/qcom/venus/firmware.c | 33 ++++++++++++++++++++++++ >> drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ >> drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- >> drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ >> 5 files changed, 57 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index 2f02365..dfd5c10 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -98,6 +98,7 @@ struct venus_caps { >> * @dev: convenience struct device pointer >> * @dev_dec: convenience struct device pointer for decoder device >> * @dev_enc: convenience struct device pointer for encoder device >> + * @no_tz: a flag that suggests presence of trustzone >> * @lock: a lock for this strucure >> * @instances: a list_head of all instances >> * @insts_count: num of instances >> @@ -129,6 +130,7 @@ struct venus_core { >> struct device *dev; >> struct device *dev_dec; >> struct device *dev_enc; >> + bool no_tz; >> struct mutex lock; >> struct list_head instances; >> atomic_t insts_count; >> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c >> index c4a5778..a9d042e 100644 >> --- a/drivers/media/platform/qcom/venus/firmware.c >> +++ b/drivers/media/platform/qcom/venus/firmware.c >> @@ -22,10 +22,43 @@ >> #include <linux/sizes.h> >> #include <linux/soc/qcom/mdt_loader.h> >> >> +#include "core.h" >> #include "firmware.h" >> +#include "hfi_venus_io.h" >> >> #define VENUS_PAS_ID 9 >> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) > > This is making a strong assumption about the size of the FW memory > region, which in practice is not always true (I had to reduce it to > 5MB). How about having this as a member of venus_core, which is Why you reduced to 5MB? Is there an issue with 6MB or you don't want to waste reserved memory? > initialized in venus_load_fw() from the actual size of the memory > region? You could do this as an extra patch that comes before this > one. > The size is 6MB by historical reasons and they are no more valid, so I think we could safely decrease to 5MB. I could prepare a patch for that.
Hi Alex, On 08/24/2018 10:39 AM, Alexandre Courbot wrote: > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: >> >> Separate firmware loading part into a new function. >> >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >> --- >> drivers/media/platform/qcom/venus/core.c | 4 +- >> drivers/media/platform/qcom/venus/firmware.c | 55 ++++++++++++++++++---------- >> drivers/media/platform/qcom/venus/firmware.h | 2 +- >> 3 files changed, 38 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c >> index bb6add9..75b9785 100644 >> --- a/drivers/media/platform/qcom/venus/core.c >> +++ b/drivers/media/platform/qcom/venus/core.c >> @@ -84,7 +84,7 @@ static void venus_sys_error_handler(struct work_struct *work) >> >> pm_runtime_get_sync(core->dev); >> >> - ret |= venus_boot(core->dev, core->res->fwname); >> + ret |= venus_boot(core); >> >> ret |= hfi_core_resume(core, true); >> >> @@ -284,7 +284,7 @@ static int venus_probe(struct platform_device *pdev) >> if (ret < 0) >> goto err_runtime_disable; >> >> - ret = venus_boot(dev, core->res->fwname); >> + ret = venus_boot(core); >> if (ret) >> goto err_runtime_disable; >> >> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c >> index a9d042e..34224eb 100644 >> --- a/drivers/media/platform/qcom/venus/firmware.c >> +++ b/drivers/media/platform/qcom/venus/firmware.c >> @@ -60,20 +60,18 @@ int venus_set_hw_state(struct venus_core *core, bool resume) >> return 0; >> } >> >> -int venus_boot(struct device *dev, const char *fwname) >> +static int venus_load_fw(struct venus_core *core, const char *fwname, >> + phys_addr_t *mem_phys, size_t *mem_size) > > Following the remarks of the previous patch, you would have mem_phys > and mem_size as members of venus_core (probably renamed as fw_mem_addr > and fw_mem_size). > >> { >> const struct firmware *mdt; >> struct device_node *node; >> - phys_addr_t mem_phys; >> + struct device *dev; >> struct resource r; >> ssize_t fw_size; >> - size_t mem_size; >> void *mem_va; >> int ret; >> >> - if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || !qcom_scm_is_available()) >> - return -EPROBE_DEFER; > > !IS_ENABLED(CONFIG_QCOM_MDT_LOADER) is not a condition that can change > at runtime, and returning -EPROBE_DEFER in that case seems erroneous > to me. Instead, wouldn't it make more sense to make the driver depend > on QCOM_MDT_LOADER? That was made on purpose, for more info git show b8f9bdc151e4a
Hi Alex, On 2018-08-24 13:09, Alexandre Courbot wrote: > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia > <vgarodia@codeaurora.org> wrote: [snip] >> +struct video_firmware { >> + struct device *dev; >> + struct iommu_domain *iommu_domain; >> +}; >> + >> /** >> * struct venus_core - holds core parameters valid for all instances >> * >> @@ -98,6 +103,7 @@ struct venus_caps { >> * @dev: convenience struct device pointer >> * @dev_dec: convenience struct device pointer for decoder device >> * @dev_enc: convenience struct device pointer for encoder device >> + * @fw: a struct for venus firmware info >> * @no_tz: a flag that suggests presence of trustzone >> * @lock: a lock for this strucure >> * @instances: a list_head of all instances >> @@ -130,6 +136,7 @@ struct venus_core { >> struct device *dev; >> struct device *dev_dec; >> struct device *dev_enc; >> + struct video_firmware fw; > > Since struct video_firmware is only used here I think you can declare > it inline, i.e. > > struct { > struct device *dev; > struct iommu_domain *iommu_domain; > } fw; > > This structure is actually a good candidate to hold the firmware > memory area start address and size. I can make it inline. Memory area and size are common parameters populated locally while loading the firmware with or without tz. Firmware struct has info more specific to firmware device. [snip] > >> +{ >> + struct iommu_domain *iommu_dom; >> + struct device *dev; >> + int ret; >> + >> + dev = core->fw.dev; >> + if (!dev) >> + return -EPROBE_DEFER; >> + >> + iommu_dom = iommu_domain_alloc(&platform_bus_type); >> + if (!iommu_dom) { >> + dev_err(dev, "Failed to allocate iommu domain\n"); >> + return -ENOMEM; >> + } >> + >> + ret = iommu_attach_device(iommu_dom, dev); >> + if (ret) { >> + dev_err(dev, "could not attach device\n"); >> + goto err_attach; >> + } > > I think like the above belongs more in venus_firmware_init() > (introduced in patch 4/4) than here. There is no reason to > detach/reattach the iommu if we stop the firmware. Consider the case when we want to reload the firmware during error recovery. Boot and shutdown will be needed in such case without the need to populate the firmware device again. [snip] >> +static int venus_shutdown_no_tz(struct venus_core *core) >> +{ >> + struct iommu_domain *iommu; >> + size_t unmapped; >> + u32 reg; >> + struct device *dev = core->fw.dev; >> + void __iomem *base = core->base; >> + >> + /* Assert the reset to ARM9 */ >> + reg = readl_relaxed(base + WRAPPER_A9SS_SW_RESET); >> + reg |= WRAPPER_A9SS_SW_RESET_BIT; >> + writel_relaxed(reg, base + WRAPPER_A9SS_SW_RESET); >> + >> + /* Make sure reset is asserted before the mapping is removed >> */ >> + mb(); >> + >> + iommu = core->fw.iommu_domain; >> + >> + unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, >> VENUS_FW_MEM_SIZE); >> + if (unmapped != VENUS_FW_MEM_SIZE) >> + dev_err(dev, "failed to unmap firmware\n"); >> + >> + iommu_detach_device(iommu, dev); >> + iommu_domain_free(iommu); > > Accordingly, this would also be moved into venus_firmware_deinit(). Same explanation here as well. Thanks, Vikash
On 2018-08-24 14:27, Stanimir Varbanov wrote: > Hi Alex, > > On 08/24/2018 10:38 AM, Alexandre Courbot wrote: >> On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia >> <vgarodia@codeaurora.org> wrote: >>> [snip] >>> index c4a5778..a9d042e 100644 >>> --- a/drivers/media/platform/qcom/venus/firmware.c >>> +++ b/drivers/media/platform/qcom/venus/firmware.c >>> @@ -22,10 +22,43 @@ >>> #include <linux/sizes.h> >>> #include <linux/soc/qcom/mdt_loader.h> >>> >>> +#include "core.h" >>> #include "firmware.h" >>> +#include "hfi_venus_io.h" >>> >>> #define VENUS_PAS_ID 9 >>> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) >> >> This is making a strong assumption about the size of the FW memory >> region, which in practice is not always true (I had to reduce it to >> 5MB). How about having this as a member of venus_core, which is > > Why you reduced to 5MB? Is there an issue with 6MB or you don't want to > waste reserved memory? > >> initialized in venus_load_fw() from the actual size of the memory >> region? You could do this as an extra patch that comes before this >> one. I would go with existing design than relying on the size specified in the memory-region for venus. size loaded is always taken from DT while the VENUS_FW_MEM_SIZE serves the purpose of sanity check. > The size is 6MB by historical reasons and they are no more valid, so I > think we could safely decrease to 5MB. I could prepare a patch for > that. Thanks Stan. Initial patch in this series had 5MB. We discussed earlier to keep it as is and take it as a separate patch to update from 6MB to 5MB.
On Fri, Aug 24, 2018 at 5:57 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hi Alex, > > On 08/24/2018 10:38 AM, Alexandre Courbot wrote: > > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > >> > >> Add routine to reset the ARM9 and brings it out of reset. Also > >> abstract the Venus CPU state handling with a new function. This > >> is in preparation to add PIL functionality in venus driver. > >> > >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > >> --- > >> drivers/media/platform/qcom/venus/core.h | 2 ++ > >> drivers/media/platform/qcom/venus/firmware.c | 33 ++++++++++++++++++++++++ > >> drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ > >> drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- > >> drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ > >> 5 files changed, 57 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > >> index 2f02365..dfd5c10 100644 > >> --- a/drivers/media/platform/qcom/venus/core.h > >> +++ b/drivers/media/platform/qcom/venus/core.h > >> @@ -98,6 +98,7 @@ struct venus_caps { > >> * @dev: convenience struct device pointer > >> * @dev_dec: convenience struct device pointer for decoder device > >> * @dev_enc: convenience struct device pointer for encoder device > >> + * @no_tz: a flag that suggests presence of trustzone > >> * @lock: a lock for this strucure > >> * @instances: a list_head of all instances > >> * @insts_count: num of instances > >> @@ -129,6 +130,7 @@ struct venus_core { > >> struct device *dev; > >> struct device *dev_dec; > >> struct device *dev_enc; > >> + bool no_tz; > >> struct mutex lock; > >> struct list_head instances; > >> atomic_t insts_count; > >> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > >> index c4a5778..a9d042e 100644 > >> --- a/drivers/media/platform/qcom/venus/firmware.c > >> +++ b/drivers/media/platform/qcom/venus/firmware.c > >> @@ -22,10 +22,43 @@ > >> #include <linux/sizes.h> > >> #include <linux/soc/qcom/mdt_loader.h> > >> > >> +#include "core.h" > >> #include "firmware.h" > >> +#include "hfi_venus_io.h" > >> > >> #define VENUS_PAS_ID 9 > >> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) > > > > This is making a strong assumption about the size of the FW memory > > region, which in practice is not always true (I had to reduce it to > > 5MB). How about having this as a member of venus_core, which is > > Why you reduced to 5MB? Is there an issue with 6MB or you don't want to > waste reserved memory? The DT layout of our board only has 5MB reserved for Venus. > > initialized in venus_load_fw() from the actual size of the memory > > region? You could do this as an extra patch that comes before this > > one. > > > > The size is 6MB by historical reasons and they are no more valid, so I > think we could safely decrease to 5MB. I could prepare a patch for that. Whether we settle with 6MB or 5MB, that size remains arbitrary and not based on the actual firmware size. And __qcom_mdt_load() does check that the firmware fits the memory area. So I don't understand what extra safety is added by ensuring the memory region is larger than a given number of megabytes?
On Fri, Aug 24, 2018 at 9:26 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > > Hi Alex, > > On 2018-08-24 13:09, Alexandre Courbot wrote: > > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia > > <vgarodia@codeaurora.org> wrote: > > [snip] > > >> +struct video_firmware { > >> + struct device *dev; > >> + struct iommu_domain *iommu_domain; > >> +}; > >> + > >> /** > >> * struct venus_core - holds core parameters valid for all instances > >> * > >> @@ -98,6 +103,7 @@ struct venus_caps { > >> * @dev: convenience struct device pointer > >> * @dev_dec: convenience struct device pointer for decoder device > >> * @dev_enc: convenience struct device pointer for encoder device > >> + * @fw: a struct for venus firmware info > >> * @no_tz: a flag that suggests presence of trustzone > >> * @lock: a lock for this strucure > >> * @instances: a list_head of all instances > >> @@ -130,6 +136,7 @@ struct venus_core { > >> struct device *dev; > >> struct device *dev_dec; > >> struct device *dev_enc; > >> + struct video_firmware fw; > > > > Since struct video_firmware is only used here I think you can declare > > it inline, i.e. > > > > struct { > > struct device *dev; > > struct iommu_domain *iommu_domain; > > } fw; > > > > This structure is actually a good candidate to hold the firmware > > memory area start address and size. > > I can make it inline. > Memory area and size are common parameters populated > locally while loading the firmware with or without tz. Firmware struct > has > info more specific to firmware device. > > [snip] > > > > >> +{ > >> + struct iommu_domain *iommu_dom; > >> + struct device *dev; > >> + int ret; > >> + > >> + dev = core->fw.dev; > >> + if (!dev) > >> + return -EPROBE_DEFER; > >> + > >> + iommu_dom = iommu_domain_alloc(&platform_bus_type); > >> + if (!iommu_dom) { > >> + dev_err(dev, "Failed to allocate iommu domain\n"); > >> + return -ENOMEM; > >> + } > >> + > >> + ret = iommu_attach_device(iommu_dom, dev); > >> + if (ret) { > >> + dev_err(dev, "could not attach device\n"); > >> + goto err_attach; > >> + } > > > > I think like the above belongs more in venus_firmware_init() > > (introduced in patch 4/4) than here. There is no reason to > > detach/reattach the iommu if we stop the firmware. > > Consider the case when we want to reload the firmware during error > recovery. > Boot and shutdown will be needed in such case without the need to > populate > the firmware device again. Is there a need to reattach the iommu domain in case of an error?
On Fri, Aug 24, 2018 at 6:01 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hi Alex, > > On 08/24/2018 10:39 AM, Alexandre Courbot wrote: > > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > >> > >> Separate firmware loading part into a new function. > >> > >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > >> --- > >> drivers/media/platform/qcom/venus/core.c | 4 +- > >> drivers/media/platform/qcom/venus/firmware.c | 55 ++++++++++++++++++---------- > >> drivers/media/platform/qcom/venus/firmware.h | 2 +- > >> 3 files changed, 38 insertions(+), 23 deletions(-) > >> > >> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > >> index bb6add9..75b9785 100644 > >> --- a/drivers/media/platform/qcom/venus/core.c > >> +++ b/drivers/media/platform/qcom/venus/core.c > >> @@ -84,7 +84,7 @@ static void venus_sys_error_handler(struct work_struct *work) > >> > >> pm_runtime_get_sync(core->dev); > >> > >> - ret |= venus_boot(core->dev, core->res->fwname); > >> + ret |= venus_boot(core); > >> > >> ret |= hfi_core_resume(core, true); > >> > >> @@ -284,7 +284,7 @@ static int venus_probe(struct platform_device *pdev) > >> if (ret < 0) > >> goto err_runtime_disable; > >> > >> - ret = venus_boot(dev, core->res->fwname); > >> + ret = venus_boot(core); > >> if (ret) > >> goto err_runtime_disable; > >> > >> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > >> index a9d042e..34224eb 100644 > >> --- a/drivers/media/platform/qcom/venus/firmware.c > >> +++ b/drivers/media/platform/qcom/venus/firmware.c > >> @@ -60,20 +60,18 @@ int venus_set_hw_state(struct venus_core *core, bool resume) > >> return 0; > >> } > >> > >> -int venus_boot(struct device *dev, const char *fwname) > >> +static int venus_load_fw(struct venus_core *core, const char *fwname, > >> + phys_addr_t *mem_phys, size_t *mem_size) > > > > Following the remarks of the previous patch, you would have mem_phys > > and mem_size as members of venus_core (probably renamed as fw_mem_addr > > and fw_mem_size). > > > >> { > >> const struct firmware *mdt; > >> struct device_node *node; > >> - phys_addr_t mem_phys; > >> + struct device *dev; > >> struct resource r; > >> ssize_t fw_size; > >> - size_t mem_size; > >> void *mem_va; > >> int ret; > >> > >> - if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || !qcom_scm_is_available()) > >> - return -EPROBE_DEFER; > > > > !IS_ENABLED(CONFIG_QCOM_MDT_LOADER) is not a condition that can change > > at runtime, and returning -EPROBE_DEFER in that case seems erroneous > > to me. Instead, wouldn't it make more sense to make the driver depend > > on QCOM_MDT_LOADER? > > That was made on purpose, for more info git show b8f9bdc151e4a Ah, I see. Still, in the current form it seems like qcom_scm_is_available() is not resolved thanks to a compiler optimization. Wouldn't it be more explicit to do something like #if IS_ENABLED(CONFIG_QCOM_MDT_LOADER) if (!qcom_scm_is_available()) return -EPROBE_DEFER; #endif instead?
On 2018-08-27 08:36, Alexandre Courbot wrote: > On Fri, Aug 24, 2018 at 9:26 PM Vikash Garodia > <vgarodia@codeaurora.org> wrote: >> >> Hi Alex, >> >> On 2018-08-24 13:09, Alexandre Courbot wrote: >> > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia >> > <vgarodia@codeaurora.org> wrote: >> >> [snip] >> >> >> +struct video_firmware { >> >> + struct device *dev; >> >> + struct iommu_domain *iommu_domain; >> >> +}; >> >> + >> >> /** >> >> * struct venus_core - holds core parameters valid for all instances >> >> * >> >> @@ -98,6 +103,7 @@ struct venus_caps { >> >> * @dev: convenience struct device pointer >> >> * @dev_dec: convenience struct device pointer for decoder device >> >> * @dev_enc: convenience struct device pointer for encoder device >> >> + * @fw: a struct for venus firmware info >> >> * @no_tz: a flag that suggests presence of trustzone >> >> * @lock: a lock for this strucure >> >> * @instances: a list_head of all instances >> >> @@ -130,6 +136,7 @@ struct venus_core { >> >> struct device *dev; >> >> struct device *dev_dec; >> >> struct device *dev_enc; >> >> + struct video_firmware fw; >> > >> > Since struct video_firmware is only used here I think you can declare >> > it inline, i.e. >> > >> > struct { >> > struct device *dev; >> > struct iommu_domain *iommu_domain; >> > } fw; >> > >> > This structure is actually a good candidate to hold the firmware >> > memory area start address and size. >> >> I can make it inline. >> Memory area and size are common parameters populated >> locally while loading the firmware with or without tz. Firmware struct >> has >> info more specific to firmware device. >> >> [snip] >> >> > >> >> +{ >> >> + struct iommu_domain *iommu_dom; >> >> + struct device *dev; >> >> + int ret; >> >> + >> >> + dev = core->fw.dev; >> >> + if (!dev) >> >> + return -EPROBE_DEFER; >> >> + >> >> + iommu_dom = iommu_domain_alloc(&platform_bus_type); >> >> + if (!iommu_dom) { >> >> + dev_err(dev, "Failed to allocate iommu domain\n"); >> >> + return -ENOMEM; >> >> + } >> >> + >> >> + ret = iommu_attach_device(iommu_dom, dev); >> >> + if (ret) { >> >> + dev_err(dev, "could not attach device\n"); >> >> + goto err_attach; >> >> + } >> > >> > I think like the above belongs more in venus_firmware_init() >> > (introduced in patch 4/4) than here. There is no reason to >> > detach/reattach the iommu if we stop the firmware. >> >> Consider the case when we want to reload the firmware during error >> recovery. >> Boot and shutdown will be needed in such case without the need to >> populate >> the firmware device again. > > Is there a need to reattach the iommu domain in case of an error? re-attach is not needed. We can have alloc/attach in init and detach/free in deinit. map/reset and unmap/reset can continue to remain in boot and shutdown calls. Let me know if this is good, i can repatch the series.
On Mon, Aug 27, 2018 at 7:56 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > > > On 08/27/2018 06:04 AM, Alexandre Courbot wrote: > > On Fri, Aug 24, 2018 at 5:57 PM Stanimir Varbanov > > <stanimir.varbanov@linaro.org> wrote: > >> > >> Hi Alex, > >> > >> On 08/24/2018 10:38 AM, Alexandre Courbot wrote: > >>> On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > >>>> > >>>> Add routine to reset the ARM9 and brings it out of reset. Also > >>>> abstract the Venus CPU state handling with a new function. This > >>>> is in preparation to add PIL functionality in venus driver. > >>>> > >>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > >>>> --- > >>>> drivers/media/platform/qcom/venus/core.h | 2 ++ > >>>> drivers/media/platform/qcom/venus/firmware.c | 33 ++++++++++++++++++++++++ > >>>> drivers/media/platform/qcom/venus/firmware.h | 11 ++++++++ > >>>> drivers/media/platform/qcom/venus/hfi_venus.c | 13 +++------- > >>>> drivers/media/platform/qcom/venus/hfi_venus_io.h | 7 +++++ > >>>> 5 files changed, 57 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > >>>> index 2f02365..dfd5c10 100644 > >>>> --- a/drivers/media/platform/qcom/venus/core.h > >>>> +++ b/drivers/media/platform/qcom/venus/core.h > >>>> @@ -98,6 +98,7 @@ struct venus_caps { > >>>> * @dev: convenience struct device pointer > >>>> * @dev_dec: convenience struct device pointer for decoder device > >>>> * @dev_enc: convenience struct device pointer for encoder device > >>>> + * @no_tz: a flag that suggests presence of trustzone > >>>> * @lock: a lock for this strucure > >>>> * @instances: a list_head of all instances > >>>> * @insts_count: num of instances > >>>> @@ -129,6 +130,7 @@ struct venus_core { > >>>> struct device *dev; > >>>> struct device *dev_dec; > >>>> struct device *dev_enc; > >>>> + bool no_tz; > >>>> struct mutex lock; > >>>> struct list_head instances; > >>>> atomic_t insts_count; > >>>> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > >>>> index c4a5778..a9d042e 100644 > >>>> --- a/drivers/media/platform/qcom/venus/firmware.c > >>>> +++ b/drivers/media/platform/qcom/venus/firmware.c > >>>> @@ -22,10 +22,43 @@ > >>>> #include <linux/sizes.h> > >>>> #include <linux/soc/qcom/mdt_loader.h> > >>>> > >>>> +#include "core.h" > >>>> #include "firmware.h" > >>>> +#include "hfi_venus_io.h" > >>>> > >>>> #define VENUS_PAS_ID 9 > >>>> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) > >>> > >>> This is making a strong assumption about the size of the FW memory > >>> region, which in practice is not always true (I had to reduce it to > >>> 5MB). How about having this as a member of venus_core, which is > >> > >> Why you reduced to 5MB? Is there an issue with 6MB or you don't want to > >> waste reserved memory? > > > > The DT layout of our board only has 5MB reserved for Venus. > > > >>> initialized in venus_load_fw() from the actual size of the memory > >>> region? You could do this as an extra patch that comes before this > >>> one. > >>> > >> > >> The size is 6MB by historical reasons and they are no more valid, so I > >> think we could safely decrease to 5MB. I could prepare a patch for that. > > > > Whether we settle with 6MB or 5MB, that size remains arbitrary and not > > based on the actual firmware size. And __qcom_mdt_load() does check > > If we go with 5MB it will not be arbitrary, cause all firmware we have > support to are expecting that size of memory. > > > that the firmware fits the memory area. So I don't understand what > > extra safety is added by ensuring the memory region is larger than a > > given number of megabytes? > > > > OK, is that fine for you? Drop size and check does memory region size is > big enough to contain the firmware. > > diff --git a/drivers/media/platform/qcom/venus/firmware.c > b/driver/media/platform/qcom/venus/firmware.c > index c4a577848dd7..9bf0d21e02d4 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -25,7 +25,6 @@ > #include "firmware.h" > > #define VENUS_PAS_ID 9 > -#define VENUS_FW_MEM_SIZE (6 * SZ_1M) > > int venus_boot(struct device *dev, const char *fwname) > { > @@ -54,25 +53,28 @@ int venus_boot(struct device *dev, const char *fwname) > mem_phys = r.start; > mem_size = resource_size(&r); > > - if (mem_size < VENUS_FW_MEM_SIZE) > - return -EINVAL; > - > - 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; > + return ret; > > fw_size = qcom_mdt_get_size(mdt); > if (fw_size < 0) { > ret = fw_size; > release_firmware(mdt); > - goto err_unmap; > + return ret; > + } > + > + if (mem_size < fw_size) { > + release_firmware(mdt); > + return -EINVAL; > + } > + > + mem_va = memremap(r.start, mem_size, MEMREMAP_WC); > + if (!mem_va) { > + release_firmware(mdt); > + dev_err(dev, "unable to map memory region: %pa+%zx\n", > + &r.start, mem_size); > + return -ENOMEM; > } That looks reasonable to me, at least if the firmware does not require extra memory beyond its reported size. But you know the firmware better, so your call! :)
On Mon, Aug 27, 2018 at 9:49 PM Vikash Garodia <vgarodia@codeaurora.org> wrote: > > On 2018-08-27 08:36, Alexandre Courbot wrote: > > On Fri, Aug 24, 2018 at 9:26 PM Vikash Garodia > > <vgarodia@codeaurora.org> wrote: > >> > >> Hi Alex, > >> > >> On 2018-08-24 13:09, Alexandre Courbot wrote: > >> > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia > >> > <vgarodia@codeaurora.org> wrote: > >> > >> [snip] > >> > >> >> +struct video_firmware { > >> >> + struct device *dev; > >> >> + struct iommu_domain *iommu_domain; > >> >> +}; > >> >> + > >> >> /** > >> >> * struct venus_core - holds core parameters valid for all instances > >> >> * > >> >> @@ -98,6 +103,7 @@ struct venus_caps { > >> >> * @dev: convenience struct device pointer > >> >> * @dev_dec: convenience struct device pointer for decoder device > >> >> * @dev_enc: convenience struct device pointer for encoder device > >> >> + * @fw: a struct for venus firmware info > >> >> * @no_tz: a flag that suggests presence of trustzone > >> >> * @lock: a lock for this strucure > >> >> * @instances: a list_head of all instances > >> >> @@ -130,6 +136,7 @@ struct venus_core { > >> >> struct device *dev; > >> >> struct device *dev_dec; > >> >> struct device *dev_enc; > >> >> + struct video_firmware fw; > >> > > >> > Since struct video_firmware is only used here I think you can declare > >> > it inline, i.e. > >> > > >> > struct { > >> > struct device *dev; > >> > struct iommu_domain *iommu_domain; > >> > } fw; > >> > > >> > This structure is actually a good candidate to hold the firmware > >> > memory area start address and size. > >> > >> I can make it inline. > >> Memory area and size are common parameters populated > >> locally while loading the firmware with or without tz. Firmware struct > >> has > >> info more specific to firmware device. > >> > >> [snip] > >> > >> > > >> >> +{ > >> >> + struct iommu_domain *iommu_dom; > >> >> + struct device *dev; > >> >> + int ret; > >> >> + > >> >> + dev = core->fw.dev; > >> >> + if (!dev) > >> >> + return -EPROBE_DEFER; > >> >> + > >> >> + iommu_dom = iommu_domain_alloc(&platform_bus_type); > >> >> + if (!iommu_dom) { > >> >> + dev_err(dev, "Failed to allocate iommu domain\n"); > >> >> + return -ENOMEM; > >> >> + } > >> >> + > >> >> + ret = iommu_attach_device(iommu_dom, dev); > >> >> + if (ret) { > >> >> + dev_err(dev, "could not attach device\n"); > >> >> + goto err_attach; > >> >> + } > >> > > >> > I think like the above belongs more in venus_firmware_init() > >> > (introduced in patch 4/4) than here. There is no reason to > >> > detach/reattach the iommu if we stop the firmware. > >> > >> Consider the case when we want to reload the firmware during error > >> recovery. > >> Boot and shutdown will be needed in such case without the need to > >> populate > >> the firmware device again. > > > > Is there a need to reattach the iommu domain in case of an error? > > re-attach is not needed. We can have alloc/attach in init and > detach/free in deinit. > map/reset and unmap/reset can continue to remain in boot and shutdown > calls. Let me > know if this is good, i can repatch the series. Yeah, the idea is to avoid repeating operations that do not need to be. Thanks!