2014-06-27 11:01:16

On Sun, Jul 06, 2014 at 08:17:22PM +0200, Arnd Bergmann wrote:
> On Friday 04 July 2014, Will Deacon wrote:
> > On Fri, Jul 04, 2014 at 02:47:10PM +0100, Thierry Reding wrote:
> > > On Fri, Jul 04, 2014 at 01:05:30PM +0200, Joerg Roedel wrote:
> > > > On Thu, Jun 26, 2014 at 10:49:41PM +0200, Thierry Reding wrote:
> > > > > Add an IOMMU device registry for drivers to register with and implement
> > > > > a method for users of the IOMMU API to attach to an IOMMU device. This
> > > > > allows to support deferred probing and gives the IOMMU API a convenient
> > > > > hook to perform early initialization of a device if necessary.
> > > >
> > > > Can you elaborate on why exactly you need this? The IOMMU-API is
> > > > designed to hide any details from the user about the available IOMMUs in
> > > > the system and which IOMMU handles which device. This looks like it is
> > > > going in a completly different direction from that.
> > >
> > > I need this primarily to properly serialize device probing order.
> > > Without it the IOMMU may be probed later than its clients, in which case
> > > the client drivers will assume that there is no IOMMU (iommu_present()
> > > for the parent bus fails).
> >
> > I can also vouch for needing a solution to this problem. The ARM SMMU (and
> > I think others) rely on initcall ordering rather than the driver probing
> > model to ensure the IOMMU is probed before any of its masters.
>
> I think it would be best to attach platform devices to IOMMUs from the
> of_dma_configure() we just introduced. That still requires handling
> IOMMUs special though, and I don't know how we should best deal
> with that. It would not be too hard to scan for IOMMUs in DT first
> and register them all in a way that we can later look them up
> by phandle, but that would break down if we ever get nested IOMMUs.

But even for nested IOMMUs each will have an associated device node, so
we could scan the tree up front. But given that it only solves the
problem partially I don't think that's a big advantage.

> Another possibility might be to register all devices as we do today,
> including IOMMU devices, but return -EPROBE_DEFER from
> platform_drv_probe() before we call into the driver's probe function
> if the IOMMU has not been set up at that point.

Right, Hiroshi already proposed a patch for that, but it was more or
less NAK'ed because people didn't want to have that functionality in the
device driver core.

> For PCI devices, we need a different way of dealing with the IOMMUs,
> some generic PCI code needs to be added to attach the correct IOMMU
> to a newly added PCI device based on how the host bridge is configured.

I'm curious. Without device tree, how do we find out what IOMMU a device
is connected to? Will it always be an ancestor of the device in the PCI
hierarchy?

> We can probably for now get away with not worrying about any bus type
> other than platform, amba or PCI: we don't use any other DMA master
> capable bus on ARM, and other architectures can probably rely on
> having only a single IOMMU implementation in the system.

Neither of the above proposals will work for cases where more than a
single IOMMU exists in the system. Currently we can only register one
IOMMU per bus and if we try to register a second IOMMU it will fail
(bus_set_iommu() returns -EBUSY).

Also, struct bus_type has only a pointer to a struct iommu_ops, but no
associated context. Hence my proposal, which I only posted partially
here since it didn't seem immediately relevant. But I guess to better
illustrate how I envisioned this to work, here goes:

The idea was to allow each device to have zero or more master on zero or
more IOMMUs. That's as general a case as it gets. Now to make this work
we'd need something like this:

struct iommu_master {
struct device *dev; /* the master device */
struct iommu *iommu; /* the IOMMU that dev masters */
struct list_head list; /* link in a list of all master
interfaces of dev */
};

Then we could store a list in struct device:

struct device {
...
struct list_head iommu_masters;
...
};

It was already mentioned in other threads that if a device does indeed
have more than one master interface, then it needs to control access to
them explicitly via the IOMMU API. Since we only have an API to allocate
an IRQ domain (which automatically forwards calls to the global IOMMU)
we'd need something new, such as:

master = iommu_get(dev, "foo");

or

master = iommu_get(dev, 0);

Or whichever variant we prefer. That could return a pointer to a struct
iommu_master, which could then be used to obtain a domain, like so:

domain = iommu_master_alloc_domain(master);

To make that work, as far as I can tell only very minimal changes would
have to be done to iommu_ops. Most of the functions take a pointer to a
struct iommu_domain anyway, we could extend it with a reference to the
parent of a domain. For that we'll need a structure that represents the
IOMMU device's context (which is what this patch introduces as struct
iommu).

The only functions in struct iommu_ops that deal with an IOMMU directly
are .add_device(), .remove_device() and .device_group(), although they
may become obsolete with the new APIs. Currently .add_device() and
.remove_device() are only used to register devices from a bus notifier
and that would be replaced by something more explicit like above. As for
device_group(), I don't see it being used at all currently.

Now for DMA mapping API integration we could make that use the first (or
only) IOMMU device registered. Perhaps we could even reject using this
layer of integration for multi-master devices, since it would be
difficult to tell whether or not the selected device is the correct one.

We still have the option to handle things mostly transparently with the
above by moving calls to iommu_get() into the core. But we also gain the
flexibility to work with multiple IOMMU contexts explicitly if required.

Thierry

Attachments:

(No filename) (5.84 kB)
(No filename) (819.00 B)
Download all attachments

2014-10-01 15:54:35

by Sean Paul

On Tue, Sep 30, 2014 at 02:48:35PM -0400, Sean Paul wrote:
> On Thu, Jun 26, 2014 at 4:49 PM, Thierry Reding
> <[email protected]> wrote:
> > From: Thierry Reding <[email protected]>
> >
> > When an IOMMU device is available on the platform bus, allocate an IOMMU
> > domain and attach the display controllers to it. The display controllers
> > can then scan out non-contiguous buffers by mapping them through the
> > IOMMU.
> >
>
> Hi Thierry,
> A few comments from Stéphane and myself that came up while we were
> reviewing this for our tree.

I just realized that I hadn't integrated these comments completely yet,
but I've done so now in my local tree. I'm running a couple of tests to
verify that it's all handled correctly.

> > Signed-off-by: Thierry Reding <[email protected]>
> > ---
> > drivers/gpu/drm/tegra/dc.c | 21 ++++
> > drivers/gpu/drm/tegra/drm.c | 17 ++++
> > drivers/gpu/drm/tegra/drm.h | 3 +
> > drivers/gpu/drm/tegra/fb.c | 16 ++-
> > drivers/gpu/drm/tegra/gem.c | 236 +++++++++++++++++++++++++++++++++++++++-----
> > drivers/gpu/drm/tegra/gem.h | 4 +
> > 6 files changed, 273 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index afcca04f5367..0f7452d04811 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -9,6 +9,7 @@
> >
> > #include <linux/clk.h>
> > #include <linux/debugfs.h>
> > +#include <linux/iommu.h>
> > #include <linux/reset.h>
> >
> > #include "dc.h"
> > @@ -1283,8 +1284,18 @@ static int tegra_dc_init(struct host1x_client *client)
> > {
> > struct drm_device *drm = dev_get_drvdata(client->parent);
> > struct tegra_dc *dc = host1x_client_to_dc(client);
> > + struct tegra_drm *tegra = drm->dev_private;
> > int err;
> >
> > + if (tegra->domain) {
> > + err = iommu_attach_device(tegra->domain, dc->dev);
> > + if (err < 0) {
> > + dev_err(dc->dev, "failed to attach to IOMMU: %d\n",
> > + err);
> > + return err;
> > + }
>
> [from Stéphane]
>
> shouldn't we call detach in the error paths below?

This was mostly rewritten for universal plane support, but I've made
sure that the DC properly detaches from the IOMMU in case of failure
during the code below.

> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
[...]
> > @@ -8,6 +8,7 @@
> > */
> >
> > #include <linux/host1x.h>
> > +#include <linux/iommu.h>
> >
> > #include "drm.h"
> > #include "gem.h"
> > @@ -33,6 +34,16 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
> > if (!tegra)
> > return -ENOMEM;
> >
> > + if (iommu_present(&platform_bus_type)) {
> > + tegra->domain = iommu_domain_alloc(&platform_bus_type);
> > + if (IS_ERR(tegra->domain)) {
> > + kfree(tegra);
> > + return PTR_ERR(tegra->domain);
> > + }
> > +
> > + drm_mm_init(&tegra->mm, 0, SZ_2G);
>
>
> [from Stéphane]:
>
> none of these are freed in the error path below (iommu_domain_free and
> drm_mm_takedown)
>
> also |tegra| isn't freed either?

None of the resources were actually being cleaned up, but I think I have
it all handled properly now.

> > @@ -108,22 +240,33 @@ struct tegra_bo *tegra_bo_create(struct drm_device *drm, unsigned int size,
> > host1x_bo_init(&bo->base, &tegra_bo_ops);
> > size = round_up(size, PAGE_SIZE);
> >
> > - bo->vaddr = dma_alloc_writecombine(drm->dev, size, &bo->paddr,
> > - GFP_KERNEL | __GFP_NOWARN);
> > - if (!bo->vaddr) {
> > - dev_err(drm->dev, "failed to allocate buffer with size %u\n",
> > - size);
> > - err = -ENOMEM;
> > - goto err_dma;
> > - }
> > -
> > err = drm_gem_object_init(drm, &bo->gem, size);
> > if (err)
> > - goto err_init;
> > + goto free;
> >
> > err = drm_gem_create_mmap_offset(&bo->gem);
>
> We need to call drm_gem_free_mmap_offset if one of the calls below
> fails, otherwise we'll try to free the mmap_offset on an already
> destroyed bo.

drm_gem_object_release() (below) already calls drm_gem_free_mmap_offset()
for us implicitly.

Thierry

Attachments:

(No filename) (4.35 kB)
(No filename) (819.00 B)
Download all attachments