On Fri, Aug 24, 2018 at 9:26 PM Vikash Garodia [off-list ref] wrote:

Hi Alex,

On 2018-08-24 13:09, Alexandre Courbot wrote:

quoted

On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia
[off-list ref] wrote:

[snip]

quoted

quoted

+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]

quoted

quoted

+{
+       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?