On 2018-08-27 08:36, Alexandre Courbot wrote:
On Fri, Aug 24, 2018 at 9:26 PM Vikash Garodia <vgarodia@xxxxxxxxxxxxxx> wrote:Hi Alex, On 2018-08-24 13:09, Alexandre Courbot wrote: > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia > <vgarodia@xxxxxxxxxxxxxx> 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.
