diff mbox

[v2,13/13] iommu/rockchip: Support sharing IOMMU between masters

Message ID 20180116132540.18939-14-jeffy.chen@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeffy Chen Jan. 16, 2018, 1:25 p.m. UTC
There would be some masters sharing the same IOMMU device. Put them in
the same iommu group and share the same iommu domain.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v2: None

 drivers/iommu/rockchip-iommu.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

Comments

Robin Murphy Jan. 17, 2018, 1 p.m. UTC | #1
On 16/01/18 13:25, Jeffy Chen wrote:
> There would be some masters sharing the same IOMMU device. Put them in
> the same iommu group and share the same iommu domain.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v2: None
> 
>   drivers/iommu/rockchip-iommu.c | 39 +++++++++++++++++++++++++++++++--------
>   1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index c8de1456a016..6c316dd0dd2d 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -100,11 +100,13 @@ struct rk_iommu {
>   	struct iommu_device iommu;
>   	struct list_head node; /* entry in rk_iommu_domain.iommus */
>   	struct iommu_domain *domain; /* domain to which iommu is attached */
> +	struct iommu_group *group;
>   	struct mutex pm_mutex; /* serializes power transitions */
>   };
>   
>   struct rk_iommudata {
>   	struct device_link *link; /* runtime PM link from IOMMU to master */
> +	struct iommu_domain *domain; /* domain to which device is attached */

I don't see why this is needed - for example, mtk_iommu does the same 
thing without needing to track the active domain in more than one place.

Fundamentally, for this kind of IOMMU without the notion of multiple 
translation contexts, the logic should look like:

	iommudrv_attach_device(dev, domain) {
		iommu = dev_get_iommu(dev);
		if (iommu->curr_domain != domain) {
			update_hw_state(iommu, domain);
			iommu->curr_domain = domain;
		}
	}

which I think is essentially what you have anyway. When a group contains 
multiple devices, you update the IOMMU state for the first device, then 
calls for subsequent devices in the group do nothing since they see the 
IOMMU state is already up-to-date with the new domain.

Robin.

>   	struct rk_iommu *iommu;
>   };
>   
> @@ -964,6 +966,7 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
>   {
>   	struct rk_iommu *iommu;
>   	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
> +	struct rk_iommudata *data = dev->archdata.iommu;
>   	unsigned long flags;
>   
>   	/* Allow 'virtual devices' (eg drm) to detach from domain */
> @@ -971,6 +974,12 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
>   	if (!iommu)
>   		return;
>   
> +	/* device already detached */
> +	if (data->domain != domain)
> +		return;
> +
> +	data->domain = NULL;
> +
>   	dev_dbg(dev, "Detaching from iommu domain\n");
>   
>   	/* iommu already detached */
> @@ -994,6 +1003,7 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
>   {
>   	struct rk_iommu *iommu;
>   	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
> +	struct rk_iommudata *data = dev->archdata.iommu;
>   	unsigned long flags;
>   	int ret;
>   
> @@ -1005,15 +1015,21 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
>   	if (!iommu)
>   		return 0;
>   
> +	/* device already attached */
> +	if (data->domain == domain)
> +		return 0;
> +
> +	if (data->domain)
> +		rk_iommu_detach_device(data->domain, dev);
> +
> +	data->domain = domain;
> +
>   	dev_dbg(dev, "Attaching to iommu domain\n");
>   
>   	/* iommu already attached */
>   	if (iommu->domain == domain)
>   		return 0;
>   
> -	if (iommu->domain)
> -		rk_iommu_detach_device(iommu->domain, dev);
> -
>   	iommu->domain = domain;
>   
>   	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
> @@ -1150,13 +1166,11 @@ static void rk_iommu_remove_device(struct device *dev)
>   
>   static struct iommu_group *rk_iommu_device_group(struct device *dev)
>   {
> -	struct iommu_group *group;
> +	struct rk_iommu *iommu;
>   
> -	group = iommu_group_get(dev);
> -	if (!group)
> -		group = iommu_group_alloc();
> +	iommu = rk_iommu_from_dev(dev);
>   
> -	return group;
> +	return iommu ? iommu->group : NULL;
>   }
>   
>   static int rk_iommu_of_xlate(struct device *dev,
> @@ -1263,6 +1277,12 @@ static int rk_iommu_probe(struct platform_device *pdev)
>   	if (err)
>   		goto err_remove_sysfs;
>   
> +	iommu->group = iommu_group_alloc();
> +	if (IS_ERR(iommu->group)) {
> +		err = PTR_ERR(iommu->group);
> +		goto err_unreg_iommu;
> +	}
> +
>   	/*
>   	 * Use the first registered IOMMU device for domain to use with DMA
>   	 * API, since a domain might not physically correspond to a single
> @@ -1276,6 +1296,8 @@ static int rk_iommu_probe(struct platform_device *pdev)
>   	pm_runtime_enable(dev);
>   
>   	return 0;
> +err_unreg_iommu:
> +	iommu_device_unregister(&iommu->iommu);
>   err_remove_sysfs:
>   	iommu_device_sysfs_remove(&iommu->iommu);
>   err_put_clocks:
> @@ -1289,6 +1311,7 @@ static int rk_iommu_remove(struct platform_device *pdev)
>   
>   	pm_runtime_disable(&pdev->dev);
>   
> +	iommu_group_put(iommu->group);
>   	iommu_device_unregister(&iommu->iommu);
>   	iommu_device_sysfs_remove(&iommu->iommu);
>   	rk_iommu_put_clocks(iommu);
>
Jeffy Chen Jan. 17, 2018, 1:32 p.m. UTC | #2
Hi Robin,

On 01/17/2018 09:00 PM, Robin Murphy wrote:
> On 16/01/18 13:25, Jeffy Chen wrote:
>> There would be some masters sharing the same IOMMU device. Put them in
>> the same iommu group and share the same iommu domain.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>> Changes in v2: None
>>
>>   drivers/iommu/rockchip-iommu.c | 39
>> +++++++++++++++++++++++++++++++--------
>>   1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/rockchip-iommu.c
>> b/drivers/iommu/rockchip-iommu.c
>> index c8de1456a016..6c316dd0dd2d 100644
>> --- a/drivers/iommu/rockchip-iommu.c
>> +++ b/drivers/iommu/rockchip-iommu.c
>> @@ -100,11 +100,13 @@ struct rk_iommu {
>>       struct iommu_device iommu;
>>       struct list_head node; /* entry in rk_iommu_domain.iommus */
>>       struct iommu_domain *domain; /* domain to which iommu is
>> attached */
>> +    struct iommu_group *group;
>>       struct mutex pm_mutex; /* serializes power transitions */
>>   };
>>   struct rk_iommudata {
>>       struct device_link *link; /* runtime PM link from IOMMU to
>> master */
>> +    struct iommu_domain *domain; /* domain to which device is
>> attached */
>
> I don't see why this is needed - for example, mtk_iommu does the same
> thing without needing to track the active domain in more than one place.
>
> Fundamentally, for this kind of IOMMU without the notion of multiple
> translation contexts, the logic should look like:
>
>      iommudrv_attach_device(dev, domain) {
>          iommu = dev_get_iommu(dev);
>          if (iommu->curr_domain != domain) {
>              update_hw_state(iommu, domain);
>              iommu->curr_domain = domain;
>          }
>      }
>
> which I think is essentially what you have anyway. When a group contains
> multiple devices, you update the IOMMU state for the first device, then
> calls for subsequent devices in the group do nothing since they see the
> IOMMU state is already up-to-date with the new domain.
>
right, will remove it.
> Robin.
diff mbox

Patch

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index c8de1456a016..6c316dd0dd2d 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -100,11 +100,13 @@  struct rk_iommu {
 	struct iommu_device iommu;
 	struct list_head node; /* entry in rk_iommu_domain.iommus */
 	struct iommu_domain *domain; /* domain to which iommu is attached */
+	struct iommu_group *group;
 	struct mutex pm_mutex; /* serializes power transitions */
 };
 
 struct rk_iommudata {
 	struct device_link *link; /* runtime PM link from IOMMU to master */
+	struct iommu_domain *domain; /* domain to which device is attached */
 	struct rk_iommu *iommu;
 };
 
@@ -964,6 +966,7 @@  static void rk_iommu_detach_device(struct iommu_domain *domain,
 {
 	struct rk_iommu *iommu;
 	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
+	struct rk_iommudata *data = dev->archdata.iommu;
 	unsigned long flags;
 
 	/* Allow 'virtual devices' (eg drm) to detach from domain */
@@ -971,6 +974,12 @@  static void rk_iommu_detach_device(struct iommu_domain *domain,
 	if (!iommu)
 		return;
 
+	/* device already detached */
+	if (data->domain != domain)
+		return;
+
+	data->domain = NULL;
+
 	dev_dbg(dev, "Detaching from iommu domain\n");
 
 	/* iommu already detached */
@@ -994,6 +1003,7 @@  static int rk_iommu_attach_device(struct iommu_domain *domain,
 {
 	struct rk_iommu *iommu;
 	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
+	struct rk_iommudata *data = dev->archdata.iommu;
 	unsigned long flags;
 	int ret;
 
@@ -1005,15 +1015,21 @@  static int rk_iommu_attach_device(struct iommu_domain *domain,
 	if (!iommu)
 		return 0;
 
+	/* device already attached */
+	if (data->domain == domain)
+		return 0;
+
+	if (data->domain)
+		rk_iommu_detach_device(data->domain, dev);
+
+	data->domain = domain;
+
 	dev_dbg(dev, "Attaching to iommu domain\n");
 
 	/* iommu already attached */
 	if (iommu->domain == domain)
 		return 0;
 
-	if (iommu->domain)
-		rk_iommu_detach_device(iommu->domain, dev);
-
 	iommu->domain = domain;
 
 	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
@@ -1150,13 +1166,11 @@  static void rk_iommu_remove_device(struct device *dev)
 
 static struct iommu_group *rk_iommu_device_group(struct device *dev)
 {
-	struct iommu_group *group;
+	struct rk_iommu *iommu;
 
-	group = iommu_group_get(dev);
-	if (!group)
-		group = iommu_group_alloc();
+	iommu = rk_iommu_from_dev(dev);
 
-	return group;
+	return iommu ? iommu->group : NULL;
 }
 
 static int rk_iommu_of_xlate(struct device *dev,
@@ -1263,6 +1277,12 @@  static int rk_iommu_probe(struct platform_device *pdev)
 	if (err)
 		goto err_remove_sysfs;
 
+	iommu->group = iommu_group_alloc();
+	if (IS_ERR(iommu->group)) {
+		err = PTR_ERR(iommu->group);
+		goto err_unreg_iommu;
+	}
+
 	/*
 	 * Use the first registered IOMMU device for domain to use with DMA
 	 * API, since a domain might not physically correspond to a single
@@ -1276,6 +1296,8 @@  static int rk_iommu_probe(struct platform_device *pdev)
 	pm_runtime_enable(dev);
 
 	return 0;
+err_unreg_iommu:
+	iommu_device_unregister(&iommu->iommu);
 err_remove_sysfs:
 	iommu_device_sysfs_remove(&iommu->iommu);
 err_put_clocks:
@@ -1289,6 +1311,7 @@  static int rk_iommu_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
+	iommu_group_put(iommu->group);
 	iommu_device_unregister(&iommu->iommu);
 	iommu_device_sysfs_remove(&iommu->iommu);
 	rk_iommu_put_clocks(iommu);