[PATCH v1 1/3] gpu: host1x: Remove implicit IOMMU backing on client's registration

[PATCH v1 1/3] gpu: host1x: Remove implicit IOMMU backing on client's registration

[Dmitry Osipenko]

On ARM32 we don't want any of the clients device to be backed by the
implicit domain, simply because we can't afford such a waste on older
Tegra SoCs that have very few domains available in total. The recent IOMMU
support addition for the Video Decoder hardware uncovered the problem
that an unfortunate drivers probe order results in the DRM driver probe
failure if CONFIG_ARM_DMA_USE_IOMMU=y due to a shortage of IOMMU domains
caused by the implicit backing. The host1x_client_register() is a common
function that is invoked by all of the relevant DRM drivers during theirs
probe and hence it is convenient to remove the implicit backing there,
resolving the problem.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/host1x/bus.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 742aa9ff21b8..559df3974afb 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -14,6 +14,10 @@
 #include "bus.h"
 #include "dev.h"
 
+#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
+#include <asm/dma-iommu.h>
+#endif
+
 static DEFINE_MUTEX(clients_lock);
 static LIST_HEAD(clients);
 
@@ -710,6 +714,21 @@ int host1x_client_register(struct host1x_client *client)
 	struct host1x *host1x;
 	int err;
 
+#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
+	/*
+	 * The client's driver could be backed by implicit IOMMU mapping
+	 * and we don't want to have that because all of current Tegra
+	 * drivers are managing IOMMU by themselves. This is a convenient
+	 * place for unmapping of the implicit mapping because this function
+	 * is called by all host1x drivers during theirs probe.
+	 */
+	if (client->dev->archdata.mapping) {
+		struct dma_iommu_mapping *mapping =
+			to_dma_iommu_mapping(client->dev);
+		arm_iommu_detach_device(client->dev);
+		arm_iommu_release_mapping(mapping);
+	}
+#endif
 	mutex_lock(&devices_lock);
 
 	list_for_each_entry(host1x, &devices, list) {
-- 
2.22.0

[PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Dmitry Osipenko]

This should should fire up on the DRM's driver module re-loader because
there won't be enough available domains on older Tegra SoCs.

Cc: stable <stable@vger.kernel.org>
Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/dc.c   | 4 ++--
 drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
 drivers/gpu/drm/tegra/drm.h  | 3 ++-
 drivers/gpu/drm/tegra/gr2d.c | 4 ++--
 drivers/gpu/drm/tegra/gr3d.c | 4 ++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index fa505baaaabc..c1b885444d90 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -2388,7 +2388,7 @@ static int tegra_dc_init(struct host1x_client *client)
 	if (!IS_ERR(primary))
 		drm_plane_cleanup(primary);
 
-	host1x_client_iommu_detach(client, dc->group);
+	host1x_client_iommu_detach(client, dc->group, true);
 	host1x_syncpt_free(dc->syncpt);
 
 	return err;
@@ -2412,7 +2412,7 @@ static int tegra_dc_exit(struct host1x_client *client)
 		return err;
 	}
 
-	host1x_client_iommu_detach(client, dc->group);
+	host1x_client_iommu_detach(client, dc->group, true);
 	host1x_syncpt_free(dc->syncpt);
 
 	return 0;
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index d2080bd7d392..f94441457c64 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1120,15 +1120,18 @@ struct iommu_group *host1x_client_iommu_attach(struct host1x_client *client,
 }
 
 void host1x_client_iommu_detach(struct host1x_client *client,
-				struct iommu_group *group)
+				struct iommu_group *group,
+				bool shared)
 {
 	struct drm_device *drm = dev_get_drvdata(client->parent);
 	struct tegra_drm *tegra = drm->dev_private;
 
 	if (group) {
-		if (group == tegra->group) {
+		if (!shared || group == tegra->group) {
 			iommu_detach_group(tegra->domain, group);
-			tegra->group = NULL;
+
+			if (group == tegra->group)
+				tegra->group = NULL;
 		}
 
 		iommu_group_put(group);
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 488f36f00bd8..9f1a3d6f3406 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -107,7 +107,8 @@ int tegra_drm_unregister_client(struct tegra_drm *tegra,
 struct iommu_group *host1x_client_iommu_attach(struct host1x_client *client,
 					       bool shared);
 void host1x_client_iommu_detach(struct host1x_client *client,
-				struct iommu_group *group);
+				struct iommu_group *group,
+				bool shared);
 
 int tegra_drm_init(struct tegra_drm *tegra, struct drm_device *drm);
 int tegra_drm_exit(struct tegra_drm *tegra);
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index 673059fd2fcb..c486e0a05c9d 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -69,7 +69,7 @@ static int gr2d_init(struct host1x_client *client)
 	return 0;
 
 detach:
-	host1x_client_iommu_detach(client, gr2d->group);
+	host1x_client_iommu_detach(client, gr2d->group, false);
 free:
 	host1x_syncpt_free(client->syncpts[0]);
 put:
@@ -89,7 +89,7 @@ static int gr2d_exit(struct host1x_client *client)
 	if (err < 0)
 		return err;
 
-	host1x_client_iommu_detach(client, gr2d->group);
+	host1x_client_iommu_detach(client, gr2d->group, false);
 	host1x_syncpt_free(client->syncpts[0]);
 	host1x_channel_put(gr2d->channel);
 
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 4778ae999668..591bafe455e0 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -79,7 +79,7 @@ static int gr3d_init(struct host1x_client *client)
 	return 0;
 
 detach:
-	host1x_client_iommu_detach(client, gr3d->group);
+	host1x_client_iommu_detach(client, gr3d->group, false);
 free:
 	host1x_syncpt_free(client->syncpts[0]);
 put:
@@ -98,7 +98,7 @@ static int gr3d_exit(struct host1x_client *client)
 	if (err < 0)
 		return err;
 
-	host1x_client_iommu_detach(client, gr3d->group);
+	host1x_client_iommu_detach(client, gr3d->group, false);
 	host1x_syncpt_free(client->syncpts[0]);
 	host1x_channel_put(gr3d->channel);
 
-- 
2.22.0

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5666 bytes --]

On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
> This should should fire up on the DRM's driver module re-loader because
> there won't be enough available domains on older Tegra SoCs.
> 
> Cc: stable <stable@vger.kernel.org>
> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
>  5 files changed, 14 insertions(+), 10 deletions(-)

I think I understand what this is trying to do, but the commit message
does not help at all. So what's really going on here is that we need to
detach the device from the group regardless of whether we're sharing the
group or not, just like we attach groups to the shared domain whether
they share the same group or not.

But in that case, I wonder if it's even worth splitting groups the way
we are right now. Wouldn't it be better to just put all the devices into
the same group and be done with it?

The current code gives me headaches every time I read it, so if we can
just make it so that all the devices under the DRM device share the same
group, this would become a lot easier to deal with. I'm not really
convinced that it makes much sense to keep them on separate domains,
especially given the constraints on the number of domains available on
earlier Tegra devices.

Note that sharing a group will also make it much easier for these to use
the DMA API if it is backed by an IOMMU.

Let me see if I can throw something together to that effect.

Thierry

> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index fa505baaaabc..c1b885444d90 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -2388,7 +2388,7 @@ static int tegra_dc_init(struct host1x_client *client)
>  	if (!IS_ERR(primary))
>  		drm_plane_cleanup(primary);
>  
> -	host1x_client_iommu_detach(client, dc->group);
> +	host1x_client_iommu_detach(client, dc->group, true);
>  	host1x_syncpt_free(dc->syncpt);
>  
>  	return err;
> @@ -2412,7 +2412,7 @@ static int tegra_dc_exit(struct host1x_client *client)
>  		return err;
>  	}
>  
> -	host1x_client_iommu_detach(client, dc->group);
> +	host1x_client_iommu_detach(client, dc->group, true);
>  	host1x_syncpt_free(dc->syncpt);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index d2080bd7d392..f94441457c64 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1120,15 +1120,18 @@ struct iommu_group *host1x_client_iommu_attach(struct host1x_client *client,
>  }
>  
>  void host1x_client_iommu_detach(struct host1x_client *client,
> -				struct iommu_group *group)
> +				struct iommu_group *group,
> +				bool shared)
>  {
>  	struct drm_device *drm = dev_get_drvdata(client->parent);
>  	struct tegra_drm *tegra = drm->dev_private;
>  
>  	if (group) {
> -		if (group == tegra->group) {
> +		if (!shared || group == tegra->group) {
>  			iommu_detach_group(tegra->domain, group);
> -			tegra->group = NULL;
> +
> +			if (group == tegra->group)
> +				tegra->group = NULL;
>  		}
>  
>  		iommu_group_put(group);
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 488f36f00bd8..9f1a3d6f3406 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -107,7 +107,8 @@ int tegra_drm_unregister_client(struct tegra_drm *tegra,
>  struct iommu_group *host1x_client_iommu_attach(struct host1x_client *client,
>  					       bool shared);
>  void host1x_client_iommu_detach(struct host1x_client *client,
> -				struct iommu_group *group);
> +				struct iommu_group *group,
> +				bool shared);
>  
>  int tegra_drm_init(struct tegra_drm *tegra, struct drm_device *drm);
>  int tegra_drm_exit(struct tegra_drm *tegra);
> diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
> index 673059fd2fcb..c486e0a05c9d 100644
> --- a/drivers/gpu/drm/tegra/gr2d.c
> +++ b/drivers/gpu/drm/tegra/gr2d.c
> @@ -69,7 +69,7 @@ static int gr2d_init(struct host1x_client *client)
>  	return 0;
>  
>  detach:
> -	host1x_client_iommu_detach(client, gr2d->group);
> +	host1x_client_iommu_detach(client, gr2d->group, false);
>  free:
>  	host1x_syncpt_free(client->syncpts[0]);
>  put:
> @@ -89,7 +89,7 @@ static int gr2d_exit(struct host1x_client *client)
>  	if (err < 0)
>  		return err;
>  
> -	host1x_client_iommu_detach(client, gr2d->group);
> +	host1x_client_iommu_detach(client, gr2d->group, false);
>  	host1x_syncpt_free(client->syncpts[0]);
>  	host1x_channel_put(gr2d->channel);
>  
> diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
> index 4778ae999668..591bafe455e0 100644
> --- a/drivers/gpu/drm/tegra/gr3d.c
> +++ b/drivers/gpu/drm/tegra/gr3d.c
> @@ -79,7 +79,7 @@ static int gr3d_init(struct host1x_client *client)
>  	return 0;
>  
>  detach:
> -	host1x_client_iommu_detach(client, gr3d->group);
> +	host1x_client_iommu_detach(client, gr3d->group, false);
>  free:
>  	host1x_syncpt_free(client->syncpts[0]);
>  put:
> @@ -98,7 +98,7 @@ static int gr3d_exit(struct host1x_client *client)
>  	if (err < 0)
>  		return err;
>  
> -	host1x_client_iommu_detach(client, gr3d->group);
> +	host1x_client_iommu_detach(client, gr3d->group, false);
>  	host1x_syncpt_free(client->syncpts[0]);
>  	host1x_channel_put(gr3d->channel);
>  
> -- 
> 2.22.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Dmitry Osipenko]

24.10.2019 14:58, Thierry Reding пишет:
> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
>> This should should fire up on the DRM's driver module re-loader because
>> there won't be enough available domains on older Tegra SoCs.
>>
>> Cc: stable <stable@vger.kernel.org>
>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
>>  5 files changed, 14 insertions(+), 10 deletions(-)
> 
> I think I understand what this is trying to do, but the commit message
> does not help at all. So what's really going on here is that we need to
> detach the device from the group regardless of whether we're sharing the
> group or not, just like we attach groups to the shared domain whether
> they share the same group or not.

Yes, the commit's message could be improved.

> But in that case, I wonder if it's even worth splitting groups the way
> we are right now. Wouldn't it be better to just put all the devices into
> the same group and be done with it?
> 
> The current code gives me headaches every time I read it, so if we can
> just make it so that all the devices under the DRM device share the same
> group, this would become a lot easier to deal with. I'm not really
> convinced that it makes much sense to keep them on separate domains,
> especially given the constraints on the number of domains available on
> earlier Tegra devices.
> 
> Note that sharing a group will also make it much easier for these to use
> the DMA API if it is backed by an IOMMU.

Probably I'm blanking on everything about IOMMU now.. could you please
remind me what "IOMMU group" is?

Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
each display controller has its own SWGROUP.. and thus that sharing just
doesn't make any sense, hm.

> Let me see if I can throw something together to that effect.

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2661 bytes --]

On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
> 24.10.2019 14:58, Thierry Reding пишет:
> > On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
> >> This should should fire up on the DRM's driver module re-loader because
> >> there won't be enough available domains on older Tegra SoCs.
> >>
> >> Cc: stable <stable@vger.kernel.org>
> >> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
> >>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
> >>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
> >>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
> >>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
> >>  5 files changed, 14 insertions(+), 10 deletions(-)
> > 
> > I think I understand what this is trying to do, but the commit message
> > does not help at all. So what's really going on here is that we need to
> > detach the device from the group regardless of whether we're sharing the
> > group or not, just like we attach groups to the shared domain whether
> > they share the same group or not.
> 
> Yes, the commit's message could be improved.
> 
> > But in that case, I wonder if it's even worth splitting groups the way
> > we are right now. Wouldn't it be better to just put all the devices into
> > the same group and be done with it?
> > 
> > The current code gives me headaches every time I read it, so if we can
> > just make it so that all the devices under the DRM device share the same
> > group, this would become a lot easier to deal with. I'm not really
> > convinced that it makes much sense to keep them on separate domains,
> > especially given the constraints on the number of domains available on
> > earlier Tegra devices.
> > 
> > Note that sharing a group will also make it much easier for these to use
> > the DMA API if it is backed by an IOMMU.
> 
> Probably I'm blanking on everything about IOMMU now.. could you please
> remind me what "IOMMU group" is?
> 
> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
> each display controller has its own SWGROUP.. and thus that sharing just
> doesn't make any sense, hm.

IOMMU groups are not directly related to SWGROUPs. But by default the
IOMMU framework will share a domain between members of the same IOMMU
group. Seems like that's really what we want here, so that when we do
use the DMA API, all the devices part of the DRM device get attached to
the same IOMMU domain, yet if we don't want to use the DMA API we only
need to detach the one group from the backing.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Dmitry Osipenko]

24.10.2019 16:50, Thierry Reding пишет:
> On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
>> 24.10.2019 14:58, Thierry Reding пишет:
>>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
>>>> This should should fire up on the DRM's driver module re-loader because
>>>> there won't be enough available domains on older Tegra SoCs.
>>>>
>>>> Cc: stable <stable@vger.kernel.org>
>>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
>>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
>>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
>>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
>>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
>>>>  5 files changed, 14 insertions(+), 10 deletions(-)
>>>
>>> I think I understand what this is trying to do, but the commit message
>>> does not help at all. So what's really going on here is that we need to
>>> detach the device from the group regardless of whether we're sharing the
>>> group or not, just like we attach groups to the shared domain whether
>>> they share the same group or not.
>>
>> Yes, the commit's message could be improved.
>>
>>> But in that case, I wonder if it's even worth splitting groups the way
>>> we are right now. Wouldn't it be better to just put all the devices into
>>> the same group and be done with it?
>>>
>>> The current code gives me headaches every time I read it, so if we can
>>> just make it so that all the devices under the DRM device share the same
>>> group, this would become a lot easier to deal with. I'm not really
>>> convinced that it makes much sense to keep them on separate domains,
>>> especially given the constraints on the number of domains available on
>>> earlier Tegra devices.
>>>
>>> Note that sharing a group will also make it much easier for these to use
>>> the DMA API if it is backed by an IOMMU.
>>
>> Probably I'm blanking on everything about IOMMU now.. could you please
>> remind me what "IOMMU group" is?
>>
>> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
>> each display controller has its own SWGROUP.. and thus that sharing just
>> doesn't make any sense, hm.
> 
> IOMMU groups are not directly related to SWGROUPs. But by default the
> IOMMU framework will share a domain between members of the same IOMMU
> group.

Ah, I re-figured out that again. The memory controller drivers are
defining a single "IOMMU group" for both of the display controllers.

> Seems like that's really what we want here, so that when we do
> use the DMA API, all the devices part of the DRM device get attached to
> the same IOMMU domain, yet if we don't want to use the DMA API we only
> need to detach the one group from the backing.

Yes, it should be okay to put all DRM devices into the same group, like
it is done now for the displays. It also should resolve problem with the
domains shortage on T30 since now there are maximum 3 domains in use:
host1x, drm and vde.

I actually just checked that the original problem still exists
and this change solves it as well:

---
diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
index 5a0f6e0a1643..e71096498436 100644
--- a/drivers/memory/tegra/tegra30.c
+++ b/drivers/memory/tegra/tegra30.c
@@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
tegra30_swgroups[] = {
 static const unsigned int tegra30_group_display[] = {
 	TEGRA_SWGROUP_DC,
 	TEGRA_SWGROUP_DCB,
+	TEGRA_SWGROUP_G2,
+	TEGRA_SWGROUP_NV,
+	TEGRA_SWGROUP_NV2,
 };

 static const struct tegra_smmu_group_soc tegra30_groups[] = {
---

Please let me know whether you're going to make a patch or if I should
do it.

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4505 bytes --]

On Thu, Oct 24, 2019 at 06:47:23PM +0300, Dmitry Osipenko wrote:
> 24.10.2019 16:50, Thierry Reding пишет:
> > On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
> >> 24.10.2019 14:58, Thierry Reding пишет:
> >>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
> >>>> This should should fire up on the DRM's driver module re-loader because
> >>>> there won't be enough available domains on older Tegra SoCs.
> >>>>
> >>>> Cc: stable <stable@vger.kernel.org>
> >>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>>> ---
> >>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
> >>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
> >>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
> >>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
> >>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
> >>>>  5 files changed, 14 insertions(+), 10 deletions(-)
> >>>
> >>> I think I understand what this is trying to do, but the commit message
> >>> does not help at all. So what's really going on here is that we need to
> >>> detach the device from the group regardless of whether we're sharing the
> >>> group or not, just like we attach groups to the shared domain whether
> >>> they share the same group or not.
> >>
> >> Yes, the commit's message could be improved.
> >>
> >>> But in that case, I wonder if it's even worth splitting groups the way
> >>> we are right now. Wouldn't it be better to just put all the devices into
> >>> the same group and be done with it?
> >>>
> >>> The current code gives me headaches every time I read it, so if we can
> >>> just make it so that all the devices under the DRM device share the same
> >>> group, this would become a lot easier to deal with. I'm not really
> >>> convinced that it makes much sense to keep them on separate domains,
> >>> especially given the constraints on the number of domains available on
> >>> earlier Tegra devices.
> >>>
> >>> Note that sharing a group will also make it much easier for these to use
> >>> the DMA API if it is backed by an IOMMU.
> >>
> >> Probably I'm blanking on everything about IOMMU now.. could you please
> >> remind me what "IOMMU group" is?
> >>
> >> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
> >> each display controller has its own SWGROUP.. and thus that sharing just
> >> doesn't make any sense, hm.
> > 
> > IOMMU groups are not directly related to SWGROUPs. But by default the
> > IOMMU framework will share a domain between members of the same IOMMU
> > group.
> 
> Ah, I re-figured out that again. The memory controller drivers are
> defining a single "IOMMU group" for both of the display controllers.
> 
> > Seems like that's really what we want here, so that when we do
> > use the DMA API, all the devices part of the DRM device get attached to
> > the same IOMMU domain, yet if we don't want to use the DMA API we only
> > need to detach the one group from the backing.
> 
> Yes, it should be okay to put all DRM devices into the same group, like
> it is done now for the displays. It also should resolve problem with the
> domains shortage on T30 since now there are maximum 3 domains in use:
> host1x, drm and vde.
> 
> I actually just checked that the original problem still exists
> and this change solves it as well:
> 
> ---
> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
> index 5a0f6e0a1643..e71096498436 100644
> --- a/drivers/memory/tegra/tegra30.c
> +++ b/drivers/memory/tegra/tegra30.c
> @@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
> tegra30_swgroups[] = {
>  static const unsigned int tegra30_group_display[] = {
>  	TEGRA_SWGROUP_DC,
>  	TEGRA_SWGROUP_DCB,
> +	TEGRA_SWGROUP_G2,
> +	TEGRA_SWGROUP_NV,
> +	TEGRA_SWGROUP_NV2,
>  };
> 
>  static const struct tegra_smmu_group_soc tegra30_groups[] = {
> ---
> 
> Please let me know whether you're going to make a patch or if I should
> do it.

I've been testing with a similar change and couldn't find any
regressions. I've also made the same modifications for Tegra114 and
Tegra124.

Are you saying that none of these patches are needed anymore? Or do we
still need a patch to fix detaching? I'm thinking that maybe we can
drastrically simplify the detachment now by dropping the shared
parameter altogether.

Let me draft a patch and send out the whole set for testing.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Dmitry Osipenko]

24.10.2019 18:56, Thierry Reding пишет:
> On Thu, Oct 24, 2019 at 06:47:23PM +0300, Dmitry Osipenko wrote:
>> 24.10.2019 16:50, Thierry Reding пишет:
>>> On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
>>>> 24.10.2019 14:58, Thierry Reding пишет:
>>>>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
>>>>>> This should should fire up on the DRM's driver module re-loader because
>>>>>> there won't be enough available domains on older Tegra SoCs.
>>>>>>
>>>>>> Cc: stable <stable@vger.kernel.org>
>>>>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
>>>>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
>>>>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
>>>>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
>>>>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
>>>>>>  5 files changed, 14 insertions(+), 10 deletions(-)
>>>>>
>>>>> I think I understand what this is trying to do, but the commit message
>>>>> does not help at all. So what's really going on here is that we need to
>>>>> detach the device from the group regardless of whether we're sharing the
>>>>> group or not, just like we attach groups to the shared domain whether
>>>>> they share the same group or not.
>>>>
>>>> Yes, the commit's message could be improved.
>>>>
>>>>> But in that case, I wonder if it's even worth splitting groups the way
>>>>> we are right now. Wouldn't it be better to just put all the devices into
>>>>> the same group and be done with it?
>>>>>
>>>>> The current code gives me headaches every time I read it, so if we can
>>>>> just make it so that all the devices under the DRM device share the same
>>>>> group, this would become a lot easier to deal with. I'm not really
>>>>> convinced that it makes much sense to keep them on separate domains,
>>>>> especially given the constraints on the number of domains available on
>>>>> earlier Tegra devices.
>>>>>
>>>>> Note that sharing a group will also make it much easier for these to use
>>>>> the DMA API if it is backed by an IOMMU.
>>>>
>>>> Probably I'm blanking on everything about IOMMU now.. could you please
>>>> remind me what "IOMMU group" is?
>>>>
>>>> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
>>>> each display controller has its own SWGROUP.. and thus that sharing just
>>>> doesn't make any sense, hm.
>>>
>>> IOMMU groups are not directly related to SWGROUPs. But by default the
>>> IOMMU framework will share a domain between members of the same IOMMU
>>> group.
>>
>> Ah, I re-figured out that again. The memory controller drivers are
>> defining a single "IOMMU group" for both of the display controllers.
>>
>>> Seems like that's really what we want here, so that when we do
>>> use the DMA API, all the devices part of the DRM device get attached to
>>> the same IOMMU domain, yet if we don't want to use the DMA API we only
>>> need to detach the one group from the backing.
>>
>> Yes, it should be okay to put all DRM devices into the same group, like
>> it is done now for the displays. It also should resolve problem with the
>> domains shortage on T30 since now there are maximum 3 domains in use:
>> host1x, drm and vde.
>>
>> I actually just checked that the original problem still exists
>> and this change solves it as well:
>>
>> ---
>> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
>> index 5a0f6e0a1643..e71096498436 100644
>> --- a/drivers/memory/tegra/tegra30.c
>> +++ b/drivers/memory/tegra/tegra30.c
>> @@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
>> tegra30_swgroups[] = {
>>  static const unsigned int tegra30_group_display[] = {
>>  	TEGRA_SWGROUP_DC,
>>  	TEGRA_SWGROUP_DCB,
>> +	TEGRA_SWGROUP_G2,
>> +	TEGRA_SWGROUP_NV,
>> +	TEGRA_SWGROUP_NV2,
>>  };
>>
>>  static const struct tegra_smmu_group_soc tegra30_groups[] = {
>> ---
>>
>> Please let me know whether you're going to make a patch or if I should
>> do it.
> 
> I've been testing with a similar change and couldn't find any
> regressions. I've also made the same modifications for Tegra114 and
> Tegra124.
> 
> Are you saying that none of these patches are needed anymore? Or do we
> still need a patch to fix detaching? I'm thinking that maybe we can
> drastrically simplify the detachment now by dropping the shared
> parameter altogether.
> 
> Let me draft a patch and send out the whole set for testing.

Seems it's still not ideal because I noticed this in KMSG:

[    0.703185] Failed to attached device 54200000.dc to IOMMU_mapping
[    0.710404] Failed to attached device 54240000.dc to IOMMU_mapping
[    0.719347] Failed to attached device 54140000.gr2d to IOMMU_mapping
[    0.719569] Failed to attached device 54180000.gr3d to IOMMU_mapping

which comes from the implicit IOMMU backing.

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Dmitry Osipenko]

24.10.2019 18:57, Dmitry Osipenko пишет:
> 24.10.2019 18:56, Thierry Reding пишет:
>> On Thu, Oct 24, 2019 at 06:47:23PM +0300, Dmitry Osipenko wrote:
>>> 24.10.2019 16:50, Thierry Reding пишет:
>>>> On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
>>>>> 24.10.2019 14:58, Thierry Reding пишет:
>>>>>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
>>>>>>> This should should fire up on the DRM's driver module re-loader because
>>>>>>> there won't be enough available domains on older Tegra SoCs.
>>>>>>>
>>>>>>> Cc: stable <stable@vger.kernel.org>
>>>>>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
>>>>>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
>>>>>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
>>>>>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
>>>>>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
>>>>>>>  5 files changed, 14 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> I think I understand what this is trying to do, but the commit message
>>>>>> does not help at all. So what's really going on here is that we need to
>>>>>> detach the device from the group regardless of whether we're sharing the
>>>>>> group or not, just like we attach groups to the shared domain whether
>>>>>> they share the same group or not.
>>>>>
>>>>> Yes, the commit's message could be improved.
>>>>>
>>>>>> But in that case, I wonder if it's even worth splitting groups the way
>>>>>> we are right now. Wouldn't it be better to just put all the devices into
>>>>>> the same group and be done with it?
>>>>>>
>>>>>> The current code gives me headaches every time I read it, so if we can
>>>>>> just make it so that all the devices under the DRM device share the same
>>>>>> group, this would become a lot easier to deal with. I'm not really
>>>>>> convinced that it makes much sense to keep them on separate domains,
>>>>>> especially given the constraints on the number of domains available on
>>>>>> earlier Tegra devices.
>>>>>>
>>>>>> Note that sharing a group will also make it much easier for these to use
>>>>>> the DMA API if it is backed by an IOMMU.
>>>>>
>>>>> Probably I'm blanking on everything about IOMMU now.. could you please
>>>>> remind me what "IOMMU group" is?
>>>>>
>>>>> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
>>>>> each display controller has its own SWGROUP.. and thus that sharing just
>>>>> doesn't make any sense, hm.
>>>>
>>>> IOMMU groups are not directly related to SWGROUPs. But by default the
>>>> IOMMU framework will share a domain between members of the same IOMMU
>>>> group.
>>>
>>> Ah, I re-figured out that again. The memory controller drivers are
>>> defining a single "IOMMU group" for both of the display controllers.
>>>
>>>> Seems like that's really what we want here, so that when we do
>>>> use the DMA API, all the devices part of the DRM device get attached to
>>>> the same IOMMU domain, yet if we don't want to use the DMA API we only
>>>> need to detach the one group from the backing.
>>>
>>> Yes, it should be okay to put all DRM devices into the same group, like
>>> it is done now for the displays. It also should resolve problem with the
>>> domains shortage on T30 since now there are maximum 3 domains in use:
>>> host1x, drm and vde.
>>>
>>> I actually just checked that the original problem still exists
>>> and this change solves it as well:
>>>
>>> ---
>>> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
>>> index 5a0f6e0a1643..e71096498436 100644
>>> --- a/drivers/memory/tegra/tegra30.c
>>> +++ b/drivers/memory/tegra/tegra30.c
>>> @@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
>>> tegra30_swgroups[] = {
>>>  static const unsigned int tegra30_group_display[] = {
>>>  	TEGRA_SWGROUP_DC,
>>>  	TEGRA_SWGROUP_DCB,
>>> +	TEGRA_SWGROUP_G2,
>>> +	TEGRA_SWGROUP_NV,
>>> +	TEGRA_SWGROUP_NV2,
>>>  };
>>>
>>>  static const struct tegra_smmu_group_soc tegra30_groups[] = {
>>> ---
>>>
>>> Please let me know whether you're going to make a patch or if I should
>>> do it.
>>
>> I've been testing with a similar change and couldn't find any
>> regressions. I've also made the same modifications for Tegra114 and
>> Tegra124.
>>
>> Are you saying that none of these patches are needed anymore? Or do we
>> still need a patch to fix detaching? I'm thinking that maybe we can
>> drastrically simplify the detachment now by dropping the shared
>> parameter altogether.
>>
>> Let me draft a patch and send out the whole set for testing.
> 
> Seems it's still not ideal because I noticed this in KMSG:
> 
> [    0.703185] Failed to attached device 54200000.dc to IOMMU_mapping
> [    0.710404] Failed to attached device 54240000.dc to IOMMU_mapping
> [    0.719347] Failed to attached device 54140000.gr2d to IOMMU_mapping
> [    0.719569] Failed to attached device 54180000.gr3d to IOMMU_mapping
> 
> which comes from the implicit IOMMU backing.

And the error comes from here:

https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/iommu/iommu.c#L1655

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Dmitry Osipenko]

24.10.2019 19:09, Dmitry Osipenko пишет:
> 24.10.2019 18:57, Dmitry Osipenko пишет:
>> 24.10.2019 18:56, Thierry Reding пишет:
>>> On Thu, Oct 24, 2019 at 06:47:23PM +0300, Dmitry Osipenko wrote:
>>>> 24.10.2019 16:50, Thierry Reding пишет:
>>>>> On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
>>>>>> 24.10.2019 14:58, Thierry Reding пишет:
>>>>>>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
>>>>>>>> This should should fire up on the DRM's driver module re-loader because
>>>>>>>> there won't be enough available domains on older Tegra SoCs.
>>>>>>>>
>>>>>>>> Cc: stable <stable@vger.kernel.org>
>>>>>>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
>>>>>>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
>>>>>>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
>>>>>>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
>>>>>>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
>>>>>>>>  5 files changed, 14 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> I think I understand what this is trying to do, but the commit message
>>>>>>> does not help at all. So what's really going on here is that we need to
>>>>>>> detach the device from the group regardless of whether we're sharing the
>>>>>>> group or not, just like we attach groups to the shared domain whether
>>>>>>> they share the same group or not.
>>>>>>
>>>>>> Yes, the commit's message could be improved.
>>>>>>
>>>>>>> But in that case, I wonder if it's even worth splitting groups the way
>>>>>>> we are right now. Wouldn't it be better to just put all the devices into
>>>>>>> the same group and be done with it?
>>>>>>>
>>>>>>> The current code gives me headaches every time I read it, so if we can
>>>>>>> just make it so that all the devices under the DRM device share the same
>>>>>>> group, this would become a lot easier to deal with. I'm not really
>>>>>>> convinced that it makes much sense to keep them on separate domains,
>>>>>>> especially given the constraints on the number of domains available on
>>>>>>> earlier Tegra devices.
>>>>>>>
>>>>>>> Note that sharing a group will also make it much easier for these to use
>>>>>>> the DMA API if it is backed by an IOMMU.
>>>>>>
>>>>>> Probably I'm blanking on everything about IOMMU now.. could you please
>>>>>> remind me what "IOMMU group" is?
>>>>>>
>>>>>> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
>>>>>> each display controller has its own SWGROUP.. and thus that sharing just
>>>>>> doesn't make any sense, hm.
>>>>>
>>>>> IOMMU groups are not directly related to SWGROUPs. But by default the
>>>>> IOMMU framework will share a domain between members of the same IOMMU
>>>>> group.
>>>>
>>>> Ah, I re-figured out that again. The memory controller drivers are
>>>> defining a single "IOMMU group" for both of the display controllers.
>>>>
>>>>> Seems like that's really what we want here, so that when we do
>>>>> use the DMA API, all the devices part of the DRM device get attached to
>>>>> the same IOMMU domain, yet if we don't want to use the DMA API we only
>>>>> need to detach the one group from the backing.
>>>>
>>>> Yes, it should be okay to put all DRM devices into the same group, like
>>>> it is done now for the displays. It also should resolve problem with the
>>>> domains shortage on T30 since now there are maximum 3 domains in use:
>>>> host1x, drm and vde.
>>>>
>>>> I actually just checked that the original problem still exists
>>>> and this change solves it as well:
>>>>
>>>> ---
>>>> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
>>>> index 5a0f6e0a1643..e71096498436 100644
>>>> --- a/drivers/memory/tegra/tegra30.c
>>>> +++ b/drivers/memory/tegra/tegra30.c
>>>> @@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
>>>> tegra30_swgroups[] = {
>>>>  static const unsigned int tegra30_group_display[] = {
>>>>  	TEGRA_SWGROUP_DC,
>>>>  	TEGRA_SWGROUP_DCB,
>>>> +	TEGRA_SWGROUP_G2,
>>>> +	TEGRA_SWGROUP_NV,
>>>> +	TEGRA_SWGROUP_NV2,
>>>>  };
>>>>
>>>>  static const struct tegra_smmu_group_soc tegra30_groups[] = {
>>>> ---
>>>>
>>>> Please let me know whether you're going to make a patch or if I should
>>>> do it.
>>>
>>> I've been testing with a similar change and couldn't find any
>>> regressions. I've also made the same modifications for Tegra114 and
>>> Tegra124.
>>>
>>> Are you saying that none of these patches are needed anymore? Or do we
>>> still need a patch to fix detaching? I'm thinking that maybe we can
>>> drastrically simplify the detachment now by dropping the shared
>>> parameter altogether.
>>>
>>> Let me draft a patch and send out the whole set for testing.
>>
>> Seems it's still not ideal because I noticed this in KMSG:
>>
>> [    0.703185] Failed to attached device 54200000.dc to IOMMU_mapping
>> [    0.710404] Failed to attached device 54240000.dc to IOMMU_mapping
>> [    0.719347] Failed to attached device 54140000.gr2d to IOMMU_mapping
>> [    0.719569] Failed to attached device 54180000.gr3d to IOMMU_mapping
>>
>> which comes from the implicit IOMMU backing.
> 
> And the error comes from here:
> 
> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/iommu/iommu.c#L1655

So the detaching still should be needed, but at the moment the ARM32
DMA-mapping code is simply not suitable for the case of having multiple
devices in the same group. I'm wondering whether there are any real
users for the implicit IOMMU backing on ARM32 at all :/

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Dmitry Osipenko]

24.10.2019 19:21, Dmitry Osipenko пишет:
> 24.10.2019 19:09, Dmitry Osipenko пишет:
>> 24.10.2019 18:57, Dmitry Osipenko пишет:
>>> 24.10.2019 18:56, Thierry Reding пишет:
>>>> On Thu, Oct 24, 2019 at 06:47:23PM +0300, Dmitry Osipenko wrote:
>>>>> 24.10.2019 16:50, Thierry Reding пишет:
>>>>>> On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
>>>>>>> 24.10.2019 14:58, Thierry Reding пишет:
>>>>>>>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
>>>>>>>>> This should should fire up on the DRM's driver module re-loader because
>>>>>>>>> there won't be enough available domains on older Tegra SoCs.
>>>>>>>>>
>>>>>>>>> Cc: stable <stable@vger.kernel.org>
>>>>>>>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
>>>>>>>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
>>>>>>>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
>>>>>>>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
>>>>>>>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
>>>>>>>>>  5 files changed, 14 insertions(+), 10 deletions(-)
>>>>>>>>
>>>>>>>> I think I understand what this is trying to do, but the commit message
>>>>>>>> does not help at all. So what's really going on here is that we need to
>>>>>>>> detach the device from the group regardless of whether we're sharing the
>>>>>>>> group or not, just like we attach groups to the shared domain whether
>>>>>>>> they share the same group or not.
>>>>>>>
>>>>>>> Yes, the commit's message could be improved.
>>>>>>>
>>>>>>>> But in that case, I wonder if it's even worth splitting groups the way
>>>>>>>> we are right now. Wouldn't it be better to just put all the devices into
>>>>>>>> the same group and be done with it?
>>>>>>>>
>>>>>>>> The current code gives me headaches every time I read it, so if we can
>>>>>>>> just make it so that all the devices under the DRM device share the same
>>>>>>>> group, this would become a lot easier to deal with. I'm not really
>>>>>>>> convinced that it makes much sense to keep them on separate domains,
>>>>>>>> especially given the constraints on the number of domains available on
>>>>>>>> earlier Tegra devices.
>>>>>>>>
>>>>>>>> Note that sharing a group will also make it much easier for these to use
>>>>>>>> the DMA API if it is backed by an IOMMU.
>>>>>>>
>>>>>>> Probably I'm blanking on everything about IOMMU now.. could you please
>>>>>>> remind me what "IOMMU group" is?
>>>>>>>
>>>>>>> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
>>>>>>> each display controller has its own SWGROUP.. and thus that sharing just
>>>>>>> doesn't make any sense, hm.
>>>>>>
>>>>>> IOMMU groups are not directly related to SWGROUPs. But by default the
>>>>>> IOMMU framework will share a domain between members of the same IOMMU
>>>>>> group.
>>>>>
>>>>> Ah, I re-figured out that again. The memory controller drivers are
>>>>> defining a single "IOMMU group" for both of the display controllers.
>>>>>
>>>>>> Seems like that's really what we want here, so that when we do
>>>>>> use the DMA API, all the devices part of the DRM device get attached to
>>>>>> the same IOMMU domain, yet if we don't want to use the DMA API we only
>>>>>> need to detach the one group from the backing.
>>>>>
>>>>> Yes, it should be okay to put all DRM devices into the same group, like
>>>>> it is done now for the displays. It also should resolve problem with the
>>>>> domains shortage on T30 since now there are maximum 3 domains in use:
>>>>> host1x, drm and vde.
>>>>>
>>>>> I actually just checked that the original problem still exists
>>>>> and this change solves it as well:
>>>>>
>>>>> ---
>>>>> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
>>>>> index 5a0f6e0a1643..e71096498436 100644
>>>>> --- a/drivers/memory/tegra/tegra30.c
>>>>> +++ b/drivers/memory/tegra/tegra30.c
>>>>> @@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
>>>>> tegra30_swgroups[] = {
>>>>>  static const unsigned int tegra30_group_display[] = {
>>>>>  	TEGRA_SWGROUP_DC,
>>>>>  	TEGRA_SWGROUP_DCB,
>>>>> +	TEGRA_SWGROUP_G2,
>>>>> +	TEGRA_SWGROUP_NV,
>>>>> +	TEGRA_SWGROUP_NV2,
>>>>>  };
>>>>>
>>>>>  static const struct tegra_smmu_group_soc tegra30_groups[] = {
>>>>> ---
>>>>>
>>>>> Please let me know whether you're going to make a patch or if I should
>>>>> do it.
>>>>
>>>> I've been testing with a similar change and couldn't find any
>>>> regressions. I've also made the same modifications for Tegra114 and
>>>> Tegra124.
>>>>
>>>> Are you saying that none of these patches are needed anymore? Or do we
>>>> still need a patch to fix detaching? I'm thinking that maybe we can
>>>> drastrically simplify the detachment now by dropping the shared
>>>> parameter altogether.
>>>>
>>>> Let me draft a patch and send out the whole set for testing.
>>>
>>> Seems it's still not ideal because I noticed this in KMSG:
>>>
>>> [    0.703185] Failed to attached device 54200000.dc to IOMMU_mapping
>>> [    0.710404] Failed to attached device 54240000.dc to IOMMU_mapping
>>> [    0.719347] Failed to attached device 54140000.gr2d to IOMMU_mapping
>>> [    0.719569] Failed to attached device 54180000.gr3d to IOMMU_mapping
>>>
>>> which comes from the implicit IOMMU backing.
>>
>> And the error comes from here:
>>
>> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/iommu/iommu.c#L1655
> 
> So the detaching still should be needed, but at the moment the ARM32
> DMA-mapping code is simply not suitable for the case of having multiple
> devices in the same group. I'm wondering whether there are any real
> users for the implicit IOMMU backing on ARM32 at all :/
> 

Apparently the "Failed to attached device 54200000.dc" was always in the
log (I rarely testing the default multi-platform config), it's just the
message is a pr_warn that I wasn't paying attention because it is
colored like pr_info in dmesg :)

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6799 bytes --]

On Thu, Oct 24, 2019 at 07:31:19PM +0300, Dmitry Osipenko wrote:
> 24.10.2019 19:21, Dmitry Osipenko пишет:
> > 24.10.2019 19:09, Dmitry Osipenko пишет:
> >> 24.10.2019 18:57, Dmitry Osipenko пишет:
> >>> 24.10.2019 18:56, Thierry Reding пишет:
> >>>> On Thu, Oct 24, 2019 at 06:47:23PM +0300, Dmitry Osipenko wrote:
> >>>>> 24.10.2019 16:50, Thierry Reding пишет:
> >>>>>> On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
> >>>>>>> 24.10.2019 14:58, Thierry Reding пишет:
> >>>>>>>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
> >>>>>>>>> This should should fire up on the DRM's driver module re-loader because
> >>>>>>>>> there won't be enough available domains on older Tegra SoCs.
> >>>>>>>>>
> >>>>>>>>> Cc: stable <stable@vger.kernel.org>
> >>>>>>>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
> >>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>>>>>>>> ---
> >>>>>>>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
> >>>>>>>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
> >>>>>>>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
> >>>>>>>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
> >>>>>>>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
> >>>>>>>>>  5 files changed, 14 insertions(+), 10 deletions(-)
> >>>>>>>>
> >>>>>>>> I think I understand what this is trying to do, but the commit message
> >>>>>>>> does not help at all. So what's really going on here is that we need to
> >>>>>>>> detach the device from the group regardless of whether we're sharing the
> >>>>>>>> group or not, just like we attach groups to the shared domain whether
> >>>>>>>> they share the same group or not.
> >>>>>>>
> >>>>>>> Yes, the commit's message could be improved.
> >>>>>>>
> >>>>>>>> But in that case, I wonder if it's even worth splitting groups the way
> >>>>>>>> we are right now. Wouldn't it be better to just put all the devices into
> >>>>>>>> the same group and be done with it?
> >>>>>>>>
> >>>>>>>> The current code gives me headaches every time I read it, so if we can
> >>>>>>>> just make it so that all the devices under the DRM device share the same
> >>>>>>>> group, this would become a lot easier to deal with. I'm not really
> >>>>>>>> convinced that it makes much sense to keep them on separate domains,
> >>>>>>>> especially given the constraints on the number of domains available on
> >>>>>>>> earlier Tegra devices.
> >>>>>>>>
> >>>>>>>> Note that sharing a group will also make it much easier for these to use
> >>>>>>>> the DMA API if it is backed by an IOMMU.
> >>>>>>>
> >>>>>>> Probably I'm blanking on everything about IOMMU now.. could you please
> >>>>>>> remind me what "IOMMU group" is?
> >>>>>>>
> >>>>>>> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
> >>>>>>> each display controller has its own SWGROUP.. and thus that sharing just
> >>>>>>> doesn't make any sense, hm.
> >>>>>>
> >>>>>> IOMMU groups are not directly related to SWGROUPs. But by default the
> >>>>>> IOMMU framework will share a domain between members of the same IOMMU
> >>>>>> group.
> >>>>>
> >>>>> Ah, I re-figured out that again. The memory controller drivers are
> >>>>> defining a single "IOMMU group" for both of the display controllers.
> >>>>>
> >>>>>> Seems like that's really what we want here, so that when we do
> >>>>>> use the DMA API, all the devices part of the DRM device get attached to
> >>>>>> the same IOMMU domain, yet if we don't want to use the DMA API we only
> >>>>>> need to detach the one group from the backing.
> >>>>>
> >>>>> Yes, it should be okay to put all DRM devices into the same group, like
> >>>>> it is done now for the displays. It also should resolve problem with the
> >>>>> domains shortage on T30 since now there are maximum 3 domains in use:
> >>>>> host1x, drm and vde.
> >>>>>
> >>>>> I actually just checked that the original problem still exists
> >>>>> and this change solves it as well:
> >>>>>
> >>>>> ---
> >>>>> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
> >>>>> index 5a0f6e0a1643..e71096498436 100644
> >>>>> --- a/drivers/memory/tegra/tegra30.c
> >>>>> +++ b/drivers/memory/tegra/tegra30.c
> >>>>> @@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
> >>>>> tegra30_swgroups[] = {
> >>>>>  static const unsigned int tegra30_group_display[] = {
> >>>>>  	TEGRA_SWGROUP_DC,
> >>>>>  	TEGRA_SWGROUP_DCB,
> >>>>> +	TEGRA_SWGROUP_G2,
> >>>>> +	TEGRA_SWGROUP_NV,
> >>>>> +	TEGRA_SWGROUP_NV2,
> >>>>>  };
> >>>>>
> >>>>>  static const struct tegra_smmu_group_soc tegra30_groups[] = {
> >>>>> ---
> >>>>>
> >>>>> Please let me know whether you're going to make a patch or if I should
> >>>>> do it.
> >>>>
> >>>> I've been testing with a similar change and couldn't find any
> >>>> regressions. I've also made the same modifications for Tegra114 and
> >>>> Tegra124.
> >>>>
> >>>> Are you saying that none of these patches are needed anymore? Or do we
> >>>> still need a patch to fix detaching? I'm thinking that maybe we can
> >>>> drastrically simplify the detachment now by dropping the shared
> >>>> parameter altogether.
> >>>>
> >>>> Let me draft a patch and send out the whole set for testing.
> >>>
> >>> Seems it's still not ideal because I noticed this in KMSG:
> >>>
> >>> [    0.703185] Failed to attached device 54200000.dc to IOMMU_mapping
> >>> [    0.710404] Failed to attached device 54240000.dc to IOMMU_mapping
> >>> [    0.719347] Failed to attached device 54140000.gr2d to IOMMU_mapping
> >>> [    0.719569] Failed to attached device 54180000.gr3d to IOMMU_mapping
> >>>
> >>> which comes from the implicit IOMMU backing.
> >>
> >> And the error comes from here:
> >>
> >> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/iommu/iommu.c#L1655
> > 
> > So the detaching still should be needed, but at the moment the ARM32
> > DMA-mapping code is simply not suitable for the case of having multiple
> > devices in the same group. I'm wondering whether there are any real
> > users for the implicit IOMMU backing on ARM32 at all :/
> > 
> 
> Apparently the "Failed to attached device 54200000.dc" was always in the
> log (I rarely testing the default multi-platform config), it's just the
> message is a pr_warn that I wasn't paying attention because it is
> colored like pr_info in dmesg :)

Yeah, so the above isn't a complete solution. In order to actually use
the DMA API backed by an IOMMU, some additional patches are needed. I
have all of those in a local tree and I've already sent out a couple of
them. It's taking a while because they all need to be applied in small
iterations to make sure things don't break midway.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Dmitry Osipenko]

24.10.2019 20:28, Thierry Reding пишет:
> On Thu, Oct 24, 2019 at 07:31:19PM +0300, Dmitry Osipenko wrote:
>> 24.10.2019 19:21, Dmitry Osipenko пишет:
>>> 24.10.2019 19:09, Dmitry Osipenko пишет:
>>>> 24.10.2019 18:57, Dmitry Osipenko пишет:
>>>>> 24.10.2019 18:56, Thierry Reding пишет:
>>>>>> On Thu, Oct 24, 2019 at 06:47:23PM +0300, Dmitry Osipenko wrote:
>>>>>>> 24.10.2019 16:50, Thierry Reding пишет:
>>>>>>>> On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
>>>>>>>>> 24.10.2019 14:58, Thierry Reding пишет:
>>>>>>>>>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
>>>>>>>>>>> This should should fire up on the DRM's driver module re-loader because
>>>>>>>>>>> there won't be enough available domains on older Tegra SoCs.
>>>>>>>>>>>
>>>>>>>>>>> Cc: stable <stable@vger.kernel.org>
>>>>>>>>>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
>>>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
>>>>>>>>>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
>>>>>>>>>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
>>>>>>>>>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
>>>>>>>>>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
>>>>>>>>>>>  5 files changed, 14 insertions(+), 10 deletions(-)
>>>>>>>>>>
>>>>>>>>>> I think I understand what this is trying to do, but the commit message
>>>>>>>>>> does not help at all. So what's really going on here is that we need to
>>>>>>>>>> detach the device from the group regardless of whether we're sharing the
>>>>>>>>>> group or not, just like we attach groups to the shared domain whether
>>>>>>>>>> they share the same group or not.
>>>>>>>>>
>>>>>>>>> Yes, the commit's message could be improved.
>>>>>>>>>
>>>>>>>>>> But in that case, I wonder if it's even worth splitting groups the way
>>>>>>>>>> we are right now. Wouldn't it be better to just put all the devices into
>>>>>>>>>> the same group and be done with it?
>>>>>>>>>>
>>>>>>>>>> The current code gives me headaches every time I read it, so if we can
>>>>>>>>>> just make it so that all the devices under the DRM device share the same
>>>>>>>>>> group, this would become a lot easier to deal with. I'm not really
>>>>>>>>>> convinced that it makes much sense to keep them on separate domains,
>>>>>>>>>> especially given the constraints on the number of domains available on
>>>>>>>>>> earlier Tegra devices.
>>>>>>>>>>
>>>>>>>>>> Note that sharing a group will also make it much easier for these to use
>>>>>>>>>> the DMA API if it is backed by an IOMMU.
>>>>>>>>>
>>>>>>>>> Probably I'm blanking on everything about IOMMU now.. could you please
>>>>>>>>> remind me what "IOMMU group" is?
>>>>>>>>>
>>>>>>>>> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
>>>>>>>>> each display controller has its own SWGROUP.. and thus that sharing just
>>>>>>>>> doesn't make any sense, hm.
>>>>>>>>
>>>>>>>> IOMMU groups are not directly related to SWGROUPs. But by default the
>>>>>>>> IOMMU framework will share a domain between members of the same IOMMU
>>>>>>>> group.
>>>>>>>
>>>>>>> Ah, I re-figured out that again. The memory controller drivers are
>>>>>>> defining a single "IOMMU group" for both of the display controllers.
>>>>>>>
>>>>>>>> Seems like that's really what we want here, so that when we do
>>>>>>>> use the DMA API, all the devices part of the DRM device get attached to
>>>>>>>> the same IOMMU domain, yet if we don't want to use the DMA API we only
>>>>>>>> need to detach the one group from the backing.
>>>>>>>
>>>>>>> Yes, it should be okay to put all DRM devices into the same group, like
>>>>>>> it is done now for the displays. It also should resolve problem with the
>>>>>>> domains shortage on T30 since now there are maximum 3 domains in use:
>>>>>>> host1x, drm and vde.
>>>>>>>
>>>>>>> I actually just checked that the original problem still exists
>>>>>>> and this change solves it as well:
>>>>>>>
>>>>>>> ---
>>>>>>> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
>>>>>>> index 5a0f6e0a1643..e71096498436 100644
>>>>>>> --- a/drivers/memory/tegra/tegra30.c
>>>>>>> +++ b/drivers/memory/tegra/tegra30.c
>>>>>>> @@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
>>>>>>> tegra30_swgroups[] = {
>>>>>>>  static const unsigned int tegra30_group_display[] = {
>>>>>>>  	TEGRA_SWGROUP_DC,
>>>>>>>  	TEGRA_SWGROUP_DCB,
>>>>>>> +	TEGRA_SWGROUP_G2,
>>>>>>> +	TEGRA_SWGROUP_NV,
>>>>>>> +	TEGRA_SWGROUP_NV2,
>>>>>>>  };
>>>>>>>
>>>>>>>  static const struct tegra_smmu_group_soc tegra30_groups[] = {
>>>>>>> ---
>>>>>>>
>>>>>>> Please let me know whether you're going to make a patch or if I should
>>>>>>> do it.
>>>>>>
>>>>>> I've been testing with a similar change and couldn't find any
>>>>>> regressions. I've also made the same modifications for Tegra114 and
>>>>>> Tegra124.
>>>>>>
>>>>>> Are you saying that none of these patches are needed anymore? Or do we
>>>>>> still need a patch to fix detaching? I'm thinking that maybe we can
>>>>>> drastrically simplify the detachment now by dropping the shared
>>>>>> parameter altogether.
>>>>>>
>>>>>> Let me draft a patch and send out the whole set for testing.
>>>>>
>>>>> Seems it's still not ideal because I noticed this in KMSG:
>>>>>
>>>>> [    0.703185] Failed to attached device 54200000.dc to IOMMU_mapping
>>>>> [    0.710404] Failed to attached device 54240000.dc to IOMMU_mapping
>>>>> [    0.719347] Failed to attached device 54140000.gr2d to IOMMU_mapping
>>>>> [    0.719569] Failed to attached device 54180000.gr3d to IOMMU_mapping
>>>>>
>>>>> which comes from the implicit IOMMU backing.
>>>>
>>>> And the error comes from here:
>>>>
>>>> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/iommu/iommu.c#L1655
>>>
>>> So the detaching still should be needed, but at the moment the ARM32
>>> DMA-mapping code is simply not suitable for the case of having multiple
>>> devices in the same group. I'm wondering whether there are any real
>>> users for the implicit IOMMU backing on ARM32 at all :/
>>>
>>
>> Apparently the "Failed to attached device 54200000.dc" was always in the
>> log (I rarely testing the default multi-platform config), it's just the
>> message is a pr_warn that I wasn't paying attention because it is
>> colored like pr_info in dmesg :)
> 
> Yeah, so the above isn't a complete solution. In order to actually use
> the DMA API backed by an IOMMU, some additional patches are needed. I
> have all of those in a local tree and I've already sent out a couple of
> them. It's taking a while because they all need to be applied in small
> iterations to make sure things don't break midway.

I'd like to have an immediate interim solution.

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7670 bytes --]

On Thu, Oct 24, 2019 at 09:46:58PM +0300, Dmitry Osipenko wrote:
> 24.10.2019 20:28, Thierry Reding пишет:
> > On Thu, Oct 24, 2019 at 07:31:19PM +0300, Dmitry Osipenko wrote:
> >> 24.10.2019 19:21, Dmitry Osipenko пишет:
> >>> 24.10.2019 19:09, Dmitry Osipenko пишет:
> >>>> 24.10.2019 18:57, Dmitry Osipenko пишет:
> >>>>> 24.10.2019 18:56, Thierry Reding пишет:
> >>>>>> On Thu, Oct 24, 2019 at 06:47:23PM +0300, Dmitry Osipenko wrote:
> >>>>>>> 24.10.2019 16:50, Thierry Reding пишет:
> >>>>>>>> On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
> >>>>>>>>> 24.10.2019 14:58, Thierry Reding пишет:
> >>>>>>>>>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
> >>>>>>>>>>> This should should fire up on the DRM's driver module re-loader because
> >>>>>>>>>>> there won't be enough available domains on older Tegra SoCs.
> >>>>>>>>>>>
> >>>>>>>>>>> Cc: stable <stable@vger.kernel.org>
> >>>>>>>>>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
> >>>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
> >>>>>>>>>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
> >>>>>>>>>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
> >>>>>>>>>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
> >>>>>>>>>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
> >>>>>>>>>>>  5 files changed, 14 insertions(+), 10 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> I think I understand what this is trying to do, but the commit message
> >>>>>>>>>> does not help at all. So what's really going on here is that we need to
> >>>>>>>>>> detach the device from the group regardless of whether we're sharing the
> >>>>>>>>>> group or not, just like we attach groups to the shared domain whether
> >>>>>>>>>> they share the same group or not.
> >>>>>>>>>
> >>>>>>>>> Yes, the commit's message could be improved.
> >>>>>>>>>
> >>>>>>>>>> But in that case, I wonder if it's even worth splitting groups the way
> >>>>>>>>>> we are right now. Wouldn't it be better to just put all the devices into
> >>>>>>>>>> the same group and be done with it?
> >>>>>>>>>>
> >>>>>>>>>> The current code gives me headaches every time I read it, so if we can
> >>>>>>>>>> just make it so that all the devices under the DRM device share the same
> >>>>>>>>>> group, this would become a lot easier to deal with. I'm not really
> >>>>>>>>>> convinced that it makes much sense to keep them on separate domains,
> >>>>>>>>>> especially given the constraints on the number of domains available on
> >>>>>>>>>> earlier Tegra devices.
> >>>>>>>>>>
> >>>>>>>>>> Note that sharing a group will also make it much easier for these to use
> >>>>>>>>>> the DMA API if it is backed by an IOMMU.
> >>>>>>>>>
> >>>>>>>>> Probably I'm blanking on everything about IOMMU now.. could you please
> >>>>>>>>> remind me what "IOMMU group" is?
> >>>>>>>>>
> >>>>>>>>> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
> >>>>>>>>> each display controller has its own SWGROUP.. and thus that sharing just
> >>>>>>>>> doesn't make any sense, hm.
> >>>>>>>>
> >>>>>>>> IOMMU groups are not directly related to SWGROUPs. But by default the
> >>>>>>>> IOMMU framework will share a domain between members of the same IOMMU
> >>>>>>>> group.
> >>>>>>>
> >>>>>>> Ah, I re-figured out that again. The memory controller drivers are
> >>>>>>> defining a single "IOMMU group" for both of the display controllers.
> >>>>>>>
> >>>>>>>> Seems like that's really what we want here, so that when we do
> >>>>>>>> use the DMA API, all the devices part of the DRM device get attached to
> >>>>>>>> the same IOMMU domain, yet if we don't want to use the DMA API we only
> >>>>>>>> need to detach the one group from the backing.
> >>>>>>>
> >>>>>>> Yes, it should be okay to put all DRM devices into the same group, like
> >>>>>>> it is done now for the displays. It also should resolve problem with the
> >>>>>>> domains shortage on T30 since now there are maximum 3 domains in use:
> >>>>>>> host1x, drm and vde.
> >>>>>>>
> >>>>>>> I actually just checked that the original problem still exists
> >>>>>>> and this change solves it as well:
> >>>>>>>
> >>>>>>> ---
> >>>>>>> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
> >>>>>>> index 5a0f6e0a1643..e71096498436 100644
> >>>>>>> --- a/drivers/memory/tegra/tegra30.c
> >>>>>>> +++ b/drivers/memory/tegra/tegra30.c
> >>>>>>> @@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
> >>>>>>> tegra30_swgroups[] = {
> >>>>>>>  static const unsigned int tegra30_group_display[] = {
> >>>>>>>  	TEGRA_SWGROUP_DC,
> >>>>>>>  	TEGRA_SWGROUP_DCB,
> >>>>>>> +	TEGRA_SWGROUP_G2,
> >>>>>>> +	TEGRA_SWGROUP_NV,
> >>>>>>> +	TEGRA_SWGROUP_NV2,
> >>>>>>>  };
> >>>>>>>
> >>>>>>>  static const struct tegra_smmu_group_soc tegra30_groups[] = {
> >>>>>>> ---
> >>>>>>>
> >>>>>>> Please let me know whether you're going to make a patch or if I should
> >>>>>>> do it.
> >>>>>>
> >>>>>> I've been testing with a similar change and couldn't find any
> >>>>>> regressions. I've also made the same modifications for Tegra114 and
> >>>>>> Tegra124.
> >>>>>>
> >>>>>> Are you saying that none of these patches are needed anymore? Or do we
> >>>>>> still need a patch to fix detaching? I'm thinking that maybe we can
> >>>>>> drastrically simplify the detachment now by dropping the shared
> >>>>>> parameter altogether.
> >>>>>>
> >>>>>> Let me draft a patch and send out the whole set for testing.
> >>>>>
> >>>>> Seems it's still not ideal because I noticed this in KMSG:
> >>>>>
> >>>>> [    0.703185] Failed to attached device 54200000.dc to IOMMU_mapping
> >>>>> [    0.710404] Failed to attached device 54240000.dc to IOMMU_mapping
> >>>>> [    0.719347] Failed to attached device 54140000.gr2d to IOMMU_mapping
> >>>>> [    0.719569] Failed to attached device 54180000.gr3d to IOMMU_mapping
> >>>>>
> >>>>> which comes from the implicit IOMMU backing.
> >>>>
> >>>> And the error comes from here:
> >>>>
> >>>> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/iommu/iommu.c#L1655
> >>>
> >>> So the detaching still should be needed, but at the moment the ARM32
> >>> DMA-mapping code is simply not suitable for the case of having multiple
> >>> devices in the same group. I'm wondering whether there are any real
> >>> users for the implicit IOMMU backing on ARM32 at all :/
> >>>
> >>
> >> Apparently the "Failed to attached device 54200000.dc" was always in the
> >> log (I rarely testing the default multi-platform config), it's just the
> >> message is a pr_warn that I wasn't paying attention because it is
> >> colored like pr_info in dmesg :)
> > 
> > Yeah, so the above isn't a complete solution. In order to actually use
> > the DMA API backed by an IOMMU, some additional patches are needed. I
> > have all of those in a local tree and I've already sent out a couple of
> > them. It's taking a while because they all need to be applied in small
> > iterations to make sure things don't break midway.
> 
> I'd like to have an immediate interim solution.

To clarify: when I said "isn't a complete solution", what I meant is
that it's not a complete solution to make the implicit IOMMU backing
work with the DMA API. That's what I've got a patch set ready for.

But you said earlier that this change (i.e. putting all DRM devices into
the same IOMMU group) fixes the issue that you were seeing, right? So
that would be an immediate, interim solution, wouldn't it?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Dmitry Osipenko]

25.10.2019 14:48, Thierry Reding пишет:
> On Thu, Oct 24, 2019 at 09:46:58PM +0300, Dmitry Osipenko wrote:
>> 24.10.2019 20:28, Thierry Reding пишет:
>>> On Thu, Oct 24, 2019 at 07:31:19PM +0300, Dmitry Osipenko wrote:
>>>> 24.10.2019 19:21, Dmitry Osipenko пишет:
>>>>> 24.10.2019 19:09, Dmitry Osipenko пишет:
>>>>>> 24.10.2019 18:57, Dmitry Osipenko пишет:
>>>>>>> 24.10.2019 18:56, Thierry Reding пишет:
>>>>>>>> On Thu, Oct 24, 2019 at 06:47:23PM +0300, Dmitry Osipenko wrote:
>>>>>>>>> 24.10.2019 16:50, Thierry Reding пишет:
>>>>>>>>>> On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
>>>>>>>>>>> 24.10.2019 14:58, Thierry Reding пишет:
>>>>>>>>>>>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
>>>>>>>>>>>>> This should should fire up on the DRM's driver module re-loader because
>>>>>>>>>>>>> there won't be enough available domains on older Tegra SoCs.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Cc: stable <stable@vger.kernel.org>
>>>>>>>>>>>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
>>>>>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
>>>>>>>>>>>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
>>>>>>>>>>>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
>>>>>>>>>>>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
>>>>>>>>>>>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
>>>>>>>>>>>>>  5 files changed, 14 insertions(+), 10 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> I think I understand what this is trying to do, but the commit message
>>>>>>>>>>>> does not help at all. So what's really going on here is that we need to
>>>>>>>>>>>> detach the device from the group regardless of whether we're sharing the
>>>>>>>>>>>> group or not, just like we attach groups to the shared domain whether
>>>>>>>>>>>> they share the same group or not.
>>>>>>>>>>>
>>>>>>>>>>> Yes, the commit's message could be improved.
>>>>>>>>>>>
>>>>>>>>>>>> But in that case, I wonder if it's even worth splitting groups the way
>>>>>>>>>>>> we are right now. Wouldn't it be better to just put all the devices into
>>>>>>>>>>>> the same group and be done with it?
>>>>>>>>>>>>
>>>>>>>>>>>> The current code gives me headaches every time I read it, so if we can
>>>>>>>>>>>> just make it so that all the devices under the DRM device share the same
>>>>>>>>>>>> group, this would become a lot easier to deal with. I'm not really
>>>>>>>>>>>> convinced that it makes much sense to keep them on separate domains,
>>>>>>>>>>>> especially given the constraints on the number of domains available on
>>>>>>>>>>>> earlier Tegra devices.
>>>>>>>>>>>>
>>>>>>>>>>>> Note that sharing a group will also make it much easier for these to use
>>>>>>>>>>>> the DMA API if it is backed by an IOMMU.
>>>>>>>>>>>
>>>>>>>>>>> Probably I'm blanking on everything about IOMMU now.. could you please
>>>>>>>>>>> remind me what "IOMMU group" is?
>>>>>>>>>>>
>>>>>>>>>>> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
>>>>>>>>>>> each display controller has its own SWGROUP.. and thus that sharing just
>>>>>>>>>>> doesn't make any sense, hm.
>>>>>>>>>>
>>>>>>>>>> IOMMU groups are not directly related to SWGROUPs. But by default the
>>>>>>>>>> IOMMU framework will share a domain between members of the same IOMMU
>>>>>>>>>> group.
>>>>>>>>>
>>>>>>>>> Ah, I re-figured out that again. The memory controller drivers are
>>>>>>>>> defining a single "IOMMU group" for both of the display controllers.
>>>>>>>>>
>>>>>>>>>> Seems like that's really what we want here, so that when we do
>>>>>>>>>> use the DMA API, all the devices part of the DRM device get attached to
>>>>>>>>>> the same IOMMU domain, yet if we don't want to use the DMA API we only
>>>>>>>>>> need to detach the one group from the backing.
>>>>>>>>>
>>>>>>>>> Yes, it should be okay to put all DRM devices into the same group, like
>>>>>>>>> it is done now for the displays. It also should resolve problem with the
>>>>>>>>> domains shortage on T30 since now there are maximum 3 domains in use:
>>>>>>>>> host1x, drm and vde.
>>>>>>>>>
>>>>>>>>> I actually just checked that the original problem still exists
>>>>>>>>> and this change solves it as well:
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
>>>>>>>>> index 5a0f6e0a1643..e71096498436 100644
>>>>>>>>> --- a/drivers/memory/tegra/tegra30.c
>>>>>>>>> +++ b/drivers/memory/tegra/tegra30.c
>>>>>>>>> @@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
>>>>>>>>> tegra30_swgroups[] = {
>>>>>>>>>  static const unsigned int tegra30_group_display[] = {
>>>>>>>>>  	TEGRA_SWGROUP_DC,
>>>>>>>>>  	TEGRA_SWGROUP_DCB,
>>>>>>>>> +	TEGRA_SWGROUP_G2,
>>>>>>>>> +	TEGRA_SWGROUP_NV,
>>>>>>>>> +	TEGRA_SWGROUP_NV2,
>>>>>>>>>  };
>>>>>>>>>
>>>>>>>>>  static const struct tegra_smmu_group_soc tegra30_groups[] = {
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Please let me know whether you're going to make a patch or if I should
>>>>>>>>> do it.
>>>>>>>>
>>>>>>>> I've been testing with a similar change and couldn't find any
>>>>>>>> regressions. I've also made the same modifications for Tegra114 and
>>>>>>>> Tegra124.
>>>>>>>>
>>>>>>>> Are you saying that none of these patches are needed anymore? Or do we
>>>>>>>> still need a patch to fix detaching? I'm thinking that maybe we can
>>>>>>>> drastrically simplify the detachment now by dropping the shared
>>>>>>>> parameter altogether.
>>>>>>>>
>>>>>>>> Let me draft a patch and send out the whole set for testing.
>>>>>>>
>>>>>>> Seems it's still not ideal because I noticed this in KMSG:
>>>>>>>
>>>>>>> [    0.703185] Failed to attached device 54200000.dc to IOMMU_mapping
>>>>>>> [    0.710404] Failed to attached device 54240000.dc to IOMMU_mapping
>>>>>>> [    0.719347] Failed to attached device 54140000.gr2d to IOMMU_mapping
>>>>>>> [    0.719569] Failed to attached device 54180000.gr3d to IOMMU_mapping
>>>>>>>
>>>>>>> which comes from the implicit IOMMU backing.
>>>>>>
>>>>>> And the error comes from here:
>>>>>>
>>>>>> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/iommu/iommu.c#L1655
>>>>>
>>>>> So the detaching still should be needed, but at the moment the ARM32
>>>>> DMA-mapping code is simply not suitable for the case of having multiple
>>>>> devices in the same group. I'm wondering whether there are any real
>>>>> users for the implicit IOMMU backing on ARM32 at all :/
>>>>>
>>>>
>>>> Apparently the "Failed to attached device 54200000.dc" was always in the
>>>> log (I rarely testing the default multi-platform config), it's just the
>>>> message is a pr_warn that I wasn't paying attention because it is
>>>> colored like pr_info in dmesg :)
>>>
>>> Yeah, so the above isn't a complete solution. In order to actually use
>>> the DMA API backed by an IOMMU, some additional patches are needed. I
>>> have all of those in a local tree and I've already sent out a couple of
>>> them. It's taking a while because they all need to be applied in small
>>> iterations to make sure things don't break midway.
>>
>> I'd like to have an immediate interim solution.
> 
> To clarify: when I said "isn't a complete solution", what I meant is
> that it's not a complete solution to make the implicit IOMMU backing
> work with the DMA API. That's what I've got a patch set ready for.

Okay.

> But you said earlier that this change (i.e. putting all DRM devices into
> the same IOMMU group) fixes the issue that you were seeing, right? So
> that would be an immediate, interim solution, wouldn't it?

Yes, it would.

[PATCH v1 3/3] drm/tegra: vic: Use common helpers to attach/detach from IOMMU domain

[Dmitry Osipenko]

We now have helpers for the domain's attachment, let's use them.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/vic.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
index 982ce37ecde1..baa00da780d0 100644
--- a/drivers/gpu/drm/tegra/vic.c
+++ b/drivers/gpu/drm/tegra/vic.c
@@ -36,7 +36,7 @@ struct vic {
 	void __iomem *regs;
 	struct tegra_drm_client client;
 	struct host1x_channel *channel;
-	struct iommu_domain *domain;
+	struct iommu_group *group;
 	struct device *dev;
 	struct clk *clk;
 	struct reset_control *rst;
@@ -183,21 +183,16 @@ static const struct falcon_ops vic_falcon_ops = {
 static int vic_init(struct host1x_client *client)
 {
 	struct tegra_drm_client *drm = host1x_to_drm_client(client);
-	struct iommu_group *group = iommu_group_get(client->dev);
 	struct drm_device *dev = dev_get_drvdata(client->parent);
 	struct tegra_drm *tegra = dev->dev_private;
 	struct vic *vic = to_vic(drm);
 	int err;
 
-	if (group && tegra->domain) {
-		err = iommu_attach_group(tegra->domain, group);
-		if (err < 0) {
-			dev_err(vic->dev, "failed to attach to domain: %d\n",
-				err);
-			return err;
-		}
-
-		vic->domain = tegra->domain;
+	vic->group = host1x_client_iommu_attach(client, false);
+	if (IS_ERR(vic->group)) {
+		err = PTR_ERR(vic->group);
+		dev_err(client->dev, "failed to attach to domain: %d\n", err);
+		return err;
 	}
 
 	vic->channel = host1x_channel_request(client->dev);
@@ -223,8 +218,7 @@ static int vic_init(struct host1x_client *client)
 free_channel:
 	host1x_channel_put(vic->channel);
 detach:
-	if (group && tegra->domain)
-		iommu_detach_group(tegra->domain, group);
+	host1x_client_iommu_detach(client, vic->group, false);
 
 	return err;
 }
@@ -232,7 +226,6 @@ static int vic_init(struct host1x_client *client)
 static int vic_exit(struct host1x_client *client)
 {
 	struct tegra_drm_client *drm = host1x_to_drm_client(client);
-	struct iommu_group *group = iommu_group_get(client->dev);
 	struct drm_device *dev = dev_get_drvdata(client->parent);
 	struct tegra_drm *tegra = dev->dev_private;
 	struct vic *vic = to_vic(drm);
@@ -244,11 +237,7 @@ static int vic_exit(struct host1x_client *client)
 
 	host1x_syncpt_free(client->syncpts[0]);
 	host1x_channel_put(vic->channel);
-
-	if (vic->domain) {
-		iommu_detach_group(vic->domain, group);
-		vic->domain = NULL;
-	}
+	host1x_client_iommu_detach(client, vic->group, false);
 
 	return 0;
 }
-- 
2.22.0

Re: [PATCH v1 1/3] gpu: host1x: Remove implicit IOMMU backing on client's registration

[Christoph Hellwig]

Don't we have a device tree problem here if there is a domain covering
them?  I though we should only pick up an IOMMU for a given device
if DT explicitly asked for that?

Re: [PATCH v1 1/3] gpu: host1x: Remove implicit IOMMU backing on client's registration

[Dmitry Osipenko]

24.06.2019 10:04, Christoph Hellwig пишет:
> Don't we have a device tree problem here if there is a domain covering
> them?  I though we should only pick up an IOMMU for a given device
> if DT explicitly asked for that?
> 

There is no specific domain that "covering them". The IOMMU domain is allocated
dynamically during of the Tegra DRM's driver initialization (see tegra_drm_load) and
then all of DRM devices are attached to that domain once all of the DRM sub-drivers
are probed successfully. On Tegra SoCs it's up to software (driver) to decide how to
separate hardware devices from each other, in a case of DRM we're putting all the
relevant graphics devices into a single domain. Is it even possible to express IOMMU
domain (not group!) assignments in a device-tree?

Re: [PATCH v1 1/3] gpu: host1x: Remove implicit IOMMU backing on client's registration

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

On Sun, Jun 23, 2019 at 08:37:41PM +0300, Dmitry Osipenko wrote:
> On ARM32 we don't want any of the clients device to be backed by the
> implicit domain, simply because we can't afford such a waste on older
> Tegra SoCs that have very few domains available in total. The recent IOMMU
> support addition for the Video Decoder hardware uncovered the problem
> that an unfortunate drivers probe order results in the DRM driver probe
> failure if CONFIG_ARM_DMA_USE_IOMMU=y due to a shortage of IOMMU domains
> caused by the implicit backing. The host1x_client_register() is a common
> function that is invoked by all of the relevant DRM drivers during theirs
> probe and hence it is convenient to remove the implicit backing there,
> resolving the problem.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/host1x/bus.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

I don't really want to do this in a central place like this. If we
really do need this, why can't we do it in the individual drivers? Also,
we already call host1x_client_iommu_attach() from all the drivers and
that detaches from the IOMMU as well. So I'm not sure I understand why
this is needed.

Thierry

> 
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index 742aa9ff21b8..559df3974afb 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -14,6 +14,10 @@
>  #include "bus.h"
>  #include "dev.h"
>  
> +#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> +#include <asm/dma-iommu.h>
> +#endif
> +
>  static DEFINE_MUTEX(clients_lock);
>  static LIST_HEAD(clients);
>  
> @@ -710,6 +714,21 @@ int host1x_client_register(struct host1x_client *client)
>  	struct host1x *host1x;
>  	int err;
>  
> +#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> +	/*
> +	 * The client's driver could be backed by implicit IOMMU mapping
> +	 * and we don't want to have that because all of current Tegra
> +	 * drivers are managing IOMMU by themselves. This is a convenient
> +	 * place for unmapping of the implicit mapping because this function
> +	 * is called by all host1x drivers during theirs probe.
> +	 */
> +	if (client->dev->archdata.mapping) {
> +		struct dma_iommu_mapping *mapping =
> +			to_dma_iommu_mapping(client->dev);
> +		arm_iommu_detach_device(client->dev);
> +		arm_iommu_release_mapping(mapping);
> +	}
> +#endif
>  	mutex_lock(&devices_lock);
>  
>  	list_for_each_entry(host1x, &devices, list) {
> -- 
> 2.22.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/3] gpu: host1x: Remove implicit IOMMU backing on client's registration

[Dmitry Osipenko]

24.10.2019 14:50, Thierry Reding пишет:
> On Sun, Jun 23, 2019 at 08:37:41PM +0300, Dmitry Osipenko wrote:
>> On ARM32 we don't want any of the clients device to be backed by the
>> implicit domain, simply because we can't afford such a waste on older
>> Tegra SoCs that have very few domains available in total. The recent IOMMU
>> support addition for the Video Decoder hardware uncovered the problem
>> that an unfortunate drivers probe order results in the DRM driver probe
>> failure if CONFIG_ARM_DMA_USE_IOMMU=y due to a shortage of IOMMU domains
>> caused by the implicit backing. The host1x_client_register() is a common
>> function that is invoked by all of the relevant DRM drivers during theirs
>> probe and hence it is convenient to remove the implicit backing there,
>> resolving the problem.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/gpu/host1x/bus.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
> 
> I don't really want to do this in a central place like this. If we
> really do need this, why can't we do it in the individual drivers?

Why do you want to duplicate the same action for each driver instead of
doing it in a single common place?

> we already call host1x_client_iommu_attach() from all the drivers and
> that detaches from the IOMMU as well. So I'm not sure I understand why
> this is needed.

Wish you could ask that couple months ago. I'll try to refresh the details.

Re: [PATCH v1 1/3] gpu: host1x: Remove implicit IOMMU backing on client's registration

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1757 bytes --]

On Thu, Oct 24, 2019 at 04:35:13PM +0300, Dmitry Osipenko wrote:
> 24.10.2019 14:50, Thierry Reding пишет:
> > On Sun, Jun 23, 2019 at 08:37:41PM +0300, Dmitry Osipenko wrote:
> >> On ARM32 we don't want any of the clients device to be backed by the
> >> implicit domain, simply because we can't afford such a waste on older
> >> Tegra SoCs that have very few domains available in total. The recent IOMMU
> >> support addition for the Video Decoder hardware uncovered the problem
> >> that an unfortunate drivers probe order results in the DRM driver probe
> >> failure if CONFIG_ARM_DMA_USE_IOMMU=y due to a shortage of IOMMU domains
> >> caused by the implicit backing. The host1x_client_register() is a common
> >> function that is invoked by all of the relevant DRM drivers during theirs
> >> probe and hence it is convenient to remove the implicit backing there,
> >> resolving the problem.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/gpu/host1x/bus.c | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> > 
> > I don't really want to do this in a central place like this. If we
> > really do need this, why can't we do it in the individual drivers?
> 
> Why do you want to duplicate the same action for each driver instead of
> doing it in a single common place?

I don't mind doing it in a common place in particular, I just don't want
to do this within the host1x bus infrastructure. This is really a policy
decision that should be up to drivers. Consider the case where we had a
different host1x driver (for V4L2 for example) that would actually want
to use the DMA API. In that case we may want to detach in the DRM driver
but not the V4L2 driver.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/3] gpu: host1x: Remove implicit IOMMU backing on client's registration

[Dmitry Osipenko]

24.10.2019 16:47, Thierry Reding пишет:
> On Thu, Oct 24, 2019 at 04:35:13PM +0300, Dmitry Osipenko wrote:
>> 24.10.2019 14:50, Thierry Reding пишет:
>>> On Sun, Jun 23, 2019 at 08:37:41PM +0300, Dmitry Osipenko wrote:
>>>> On ARM32 we don't want any of the clients device to be backed by the
>>>> implicit domain, simply because we can't afford such a waste on older
>>>> Tegra SoCs that have very few domains available in total. The recent IOMMU
>>>> support addition for the Video Decoder hardware uncovered the problem
>>>> that an unfortunate drivers probe order results in the DRM driver probe
>>>> failure if CONFIG_ARM_DMA_USE_IOMMU=y due to a shortage of IOMMU domains
>>>> caused by the implicit backing. The host1x_client_register() is a common
>>>> function that is invoked by all of the relevant DRM drivers during theirs
>>>> probe and hence it is convenient to remove the implicit backing there,
>>>> resolving the problem.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/gpu/host1x/bus.c | 19 +++++++++++++++++++
>>>>  1 file changed, 19 insertions(+)
>>>
>>> I don't really want to do this in a central place like this. If we
>>> really do need this, why can't we do it in the individual drivers?
>>
>> Why do you want to duplicate the same action for each driver instead of
>> doing it in a single common place?
> 
> I don't mind doing it in a common place in particular, I just don't want
> to do this within the host1x bus infrastructure. This is really a policy
> decision that should be up to drivers. Consider the case where we had a
> different host1x driver (for V4L2 for example) that would actually want
> to use the DMA API. In that case we may want to detach in the DRM driver
> but not the V4L2 driver.

Okay.

Re: [PATCH v1 1/3] gpu: host1x: Remove implicit IOMMU backing on client's registration

[Dmitry Osipenko]

24.10.2019 16:35, Dmitry Osipenko пишет:
> 24.10.2019 14:50, Thierry Reding пишет:
>> On Sun, Jun 23, 2019 at 08:37:41PM +0300, Dmitry Osipenko wrote:
>>> On ARM32 we don't want any of the clients device to be backed by the
>>> implicit domain, simply because we can't afford such a waste on older
>>> Tegra SoCs that have very few domains available in total. The recent IOMMU
>>> support addition for the Video Decoder hardware uncovered the problem
>>> that an unfortunate drivers probe order results in the DRM driver probe
>>> failure if CONFIG_ARM_DMA_USE_IOMMU=y due to a shortage of IOMMU domains
>>> caused by the implicit backing. The host1x_client_register() is a common
>>> function that is invoked by all of the relevant DRM drivers during theirs
>>> probe and hence it is convenient to remove the implicit backing there,
>>> resolving the problem.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/gpu/host1x/bus.c | 19 +++++++++++++++++++
>>>  1 file changed, 19 insertions(+)
>>
>> I don't really want to do this in a central place like this. If we
>> really do need this, why can't we do it in the individual drivers?
> 
> Why do you want to duplicate the same action for each driver instead of
> doing it in a single common place?
> 
>> we already call host1x_client_iommu_attach() from all the drivers and
>> that detaches from the IOMMU as well. So I'm not sure I understand why
>> this is needed.
> 
> Wish you could ask that couple months ago. I'll try to refresh the details.
> 

In kernel's boot order:

1) Host1x is attached to exclusive implicit domain
2) Host1x is detached from the implicit domain and attached to explicit
3) Both DC's fail to attach to implicit domain because DMA-mapping code
doesn't take into account multiple devices in the same group.
4) GR2D is attached to exclusive implicit domain
5) GR3D is attached to exclusive implicit domain
6) VDE is attached to exclusive implicit domain
7) VDE is detached from the implicit domain and attached to explicit
8) DC client is trying to attach to DRM domain in
host1x_client_iommu_attach() and that fails because tegra-smmu code
allocates ASID in tegra_smmu_attach_dev()->tegra_smmu_as_prepare() (i.e.
on IOMMU group's attaching) and all ASIDs are already busy.

Hence if DRM sub-device drivers are detaching from the implicit domain
on probe, then the problem is getting worked around because there are
available ASIDs at the time of host1x clients initialization.