| Message ID | 20190623173743.24088-1-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/3] gpu: host1x: Remove implicit IOMMU backing on client's registration | expand |
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?
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 >
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.
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
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.
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.
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) {
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(+)