| Message ID | 20220106022053.2406748-7-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Scrap iommu_attach/detach_group() interfaces | expand |
On Thu, Jan 06, 2022 at 10:20:51AM +0800, Lu Baolu wrote: > Ordinary drivers should use iommu_attach/detach_device() for domain > attaching and detaching. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > drivers/gpu/host1x/dev.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c > index fbb6447b8659..6e08cb6202cc 100644 > +++ b/drivers/gpu/host1x/dev.c > @@ -265,7 +265,7 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host) > goto put_cache; > } > > - err = iommu_attach_group(host->domain, host->group); > + err = iommu_attach_device(host->domain, host->dev); > if (err) { > if (err == -ENODEV) > err = 0; > @@ -335,7 +335,7 @@ static void host1x_iommu_exit(struct host1x *host) > { > if (host->domain) { > put_iova_domain(&host->iova); > - iommu_detach_group(host->domain, host->group); > + iommu_detach_device(host->domain, host->dev); > > iommu_domain_free(host->domain); > host->domain = NULL; Shouldn't this add the flag to tegra_host1x_driver ? And do like we did in the other tegra stuff and switch to the dma api when !host1x_wants_iommu() ? Jason
On 1/6/22 11:35 PM, Jason Gunthorpe wrote: > On Thu, Jan 06, 2022 at 10:20:51AM +0800, Lu Baolu wrote: >> Ordinary drivers should use iommu_attach/detach_device() for domain >> attaching and detaching. >> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> drivers/gpu/host1x/dev.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c >> index fbb6447b8659..6e08cb6202cc 100644 >> +++ b/drivers/gpu/host1x/dev.c >> @@ -265,7 +265,7 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host) >> goto put_cache; >> } >> >> - err = iommu_attach_group(host->domain, host->group); >> + err = iommu_attach_device(host->domain, host->dev); >> if (err) { >> if (err == -ENODEV) >> err = 0; >> @@ -335,7 +335,7 @@ static void host1x_iommu_exit(struct host1x *host) >> { >> if (host->domain) { >> put_iova_domain(&host->iova); >> - iommu_detach_group(host->domain, host->group); >> + iommu_detach_device(host->domain, host->dev); >> >> iommu_domain_free(host->domain); >> host->domain = NULL; > > Shouldn't this add the flag to tegra_host1x_driver ? This is called for a single driver. The call trace looks like below: static struct platform_driver tegra_host1x_driver = { .driver = { .name = "tegra-host1x", .of_match_table = host1x_of_match, }, .probe = host1x_probe, .remove = host1x_remove, }; host1x_probe(dev) ->host1x_iommu_init(host) //host is a wrapper of dev -->host1x_iommu_attach(host) ---->iommu_group_get(host->dev) iommu_domain_alloc(&platform_bus_type) iommu_attach_group(domain, group); It seems that the existing code only works for singleton group. > > And do like we did in the other tegra stuff and switch to the dma api > when !host1x_wants_iommu() ? > > Jason > Best regards, baolu
On Fri, Jan 07, 2022 at 08:35:34AM +0800, Lu Baolu wrote: > On 1/6/22 11:35 PM, Jason Gunthorpe wrote: > > On Thu, Jan 06, 2022 at 10:20:51AM +0800, Lu Baolu wrote: > > > Ordinary drivers should use iommu_attach/detach_device() for domain > > > attaching and detaching. > > > > > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > > > drivers/gpu/host1x/dev.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c > > > index fbb6447b8659..6e08cb6202cc 100644 > > > +++ b/drivers/gpu/host1x/dev.c > > > @@ -265,7 +265,7 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host) > > > goto put_cache; > > > } > > > - err = iommu_attach_group(host->domain, host->group); > > > + err = iommu_attach_device(host->domain, host->dev); > > > if (err) { > > > if (err == -ENODEV) > > > err = 0; > > > @@ -335,7 +335,7 @@ static void host1x_iommu_exit(struct host1x *host) > > > { > > > if (host->domain) { > > > put_iova_domain(&host->iova); > > > - iommu_detach_group(host->domain, host->group); > > > + iommu_detach_device(host->domain, host->dev); > > > iommu_domain_free(host->domain); > > > host->domain = NULL; > > > > Shouldn't this add the flag to tegra_host1x_driver ? > > This is called for a single driver. The call trace looks like below: > > static struct platform_driver tegra_host1x_driver = { > .driver = { > .name = "tegra-host1x", > .of_match_table = host1x_of_match, > }, > .probe = host1x_probe, > .remove = host1x_remove, > }; > > host1x_probe(dev) > ->host1x_iommu_init(host) //host is a wrapper of dev > iommu_domain_alloc(&platform_bus_type) > iommu_attach_group(domain, group); The main question is if the iommu group is being shared with other drivers, not the call chain for this function. For tegra you have to go look in each entry of the of_match_table: { .compatible = "nvidia,tegra114-host1x", .data = &host1x02_info, }, And find the DTS block: host1x@50000000 { compatible = "nvidia,tegra114-host1x"; reg = <0x50000000 0x00028000>; interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */ <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */ interrupt-names = "syncpt", "host1x"; clocks = <&tegra_car TEGRA114_CLK_HOST1X>; clock-names = "host1x"; resets = <&tegra_car 28>; reset-names = "host1x"; iommus = <&mc TEGRA_SWGROUP_HC>; Then check if any other devices in the DTS use the same 'iommus' which is how the groups are setup. I checked everything and it does look like this is a single device group. Jason
On 1/7/22 8:48 AM, Jason Gunthorpe wrote: > On Fri, Jan 07, 2022 at 08:35:34AM +0800, Lu Baolu wrote: >> On 1/6/22 11:35 PM, Jason Gunthorpe wrote: >>> On Thu, Jan 06, 2022 at 10:20:51AM +0800, Lu Baolu wrote: >>>> Ordinary drivers should use iommu_attach/detach_device() for domain >>>> attaching and detaching. >>>> >>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >>>> drivers/gpu/host1x/dev.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c >>>> index fbb6447b8659..6e08cb6202cc 100644 >>>> +++ b/drivers/gpu/host1x/dev.c >>>> @@ -265,7 +265,7 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host) >>>> goto put_cache; >>>> } >>>> - err = iommu_attach_group(host->domain, host->group); >>>> + err = iommu_attach_device(host->domain, host->dev); >>>> if (err) { >>>> if (err == -ENODEV) >>>> err = 0; >>>> @@ -335,7 +335,7 @@ static void host1x_iommu_exit(struct host1x *host) >>>> { >>>> if (host->domain) { >>>> put_iova_domain(&host->iova); >>>> - iommu_detach_group(host->domain, host->group); >>>> + iommu_detach_device(host->domain, host->dev); >>>> iommu_domain_free(host->domain); >>>> host->domain = NULL; >>> >>> Shouldn't this add the flag to tegra_host1x_driver ? >> >> This is called for a single driver. The call trace looks like below: >> >> static struct platform_driver tegra_host1x_driver = { >> .driver = { >> .name = "tegra-host1x", >> .of_match_table = host1x_of_match, >> }, >> .probe = host1x_probe, >> .remove = host1x_remove, >> }; >> >> host1x_probe(dev) >> ->host1x_iommu_init(host) //host is a wrapper of dev >> iommu_domain_alloc(&platform_bus_type) >> iommu_attach_group(domain, group); > > The main question is if the iommu group is being shared with other > drivers, not the call chain for this function. > > For tegra you have to go look in each entry of the of_match_table: > > { .compatible = "nvidia,tegra114-host1x", .data = &host1x02_info, }, > > And find the DTS block: > > host1x@50000000 { > compatible = "nvidia,tegra114-host1x"; > reg = <0x50000000 0x00028000>; > interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */ > <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */ > interrupt-names = "syncpt", "host1x"; > clocks = <&tegra_car TEGRA114_CLK_HOST1X>; > clock-names = "host1x"; > resets = <&tegra_car 28>; > reset-names = "host1x"; > iommus = <&mc TEGRA_SWGROUP_HC>; > > Then check if any other devices in the DTS use the same 'iommus' which > is how the groups are setup. Yes, exactly. > > I checked everything and it does look like this is a single device > group. Okay, thanks! > > Jason > Best regards, baolu
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c index fbb6447b8659..6e08cb6202cc 100644 --- a/drivers/gpu/host1x/dev.c +++ b/drivers/gpu/host1x/dev.c @@ -265,7 +265,7 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host) goto put_cache; } - err = iommu_attach_group(host->domain, host->group); + err = iommu_attach_device(host->domain, host->dev); if (err) { if (err == -ENODEV) err = 0; @@ -335,7 +335,7 @@ static void host1x_iommu_exit(struct host1x *host) { if (host->domain) { put_iova_domain(&host->iova); - iommu_detach_group(host->domain, host->group); + iommu_detach_device(host->domain, host->dev); iommu_domain_free(host->domain); host->domain = NULL;
Ordinary drivers should use iommu_attach/detach_device() for domain attaching and detaching. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- drivers/gpu/host1x/dev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)