by Lucas Stach

[permalink] [raw]

Subject: Re: [RFC v2 8/8] drm: tegra: Add gr2d device

Am Dienstag, den 27.11.2012, 10:45 +0200 schrieb Terje Bergström:
> On 27.11.2012 10:32, Dave Airlie wrote:
> > On Tue, Nov 27, 2012 at 8:16 AM, Terje Bergström <[email protected]> wrote:
> >> Thanks for the pointer, I looked at exynos code. It indeed checks the
> >> registers written to, but it doesn't prevent overrun by checking sizes
> >> of buffers and compare against requests.
> > They probably need to add that, its not as important as the base
> > addresses, unless it takes negative strides, generally base addresses
> > means you can target current->uid quite easily!
>
> Ok. We'll implement the firewall, unless we come up with even a better
> choice.
>
> >> If this is the way to go, I'll put the firewall behind a Kconfig flag so
> >> that system integrator can decide if his system needs it.
> > We don't generally make security like this optional :-)
> >
> > If you do that you should restrict the drm device to root users only,
> > and never let a user with a browser anywhere near it.
>
Personally I would never trust any binary, but that's just my personal
opinion.

But I'm in favour of having the command stream checking optional, simply
backed by the fact that we are likely to use the same 2D driver
infrastructure for Tegra 2 and 3. On Tegra 3 we can most likely go
without in-depth command stream checking as the graphics core there sits
behind the IOMMU, which can provide an appropriate level of security.

Regards,
Lucas

2012-11-27 10:37:52

On Mon, Nov 26, 2012 at 03:19:07PM +0200, Terje Bergstrom wrote:
[...]
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index fb9a14e..94c861b 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -2463,4 +2463,6 @@ config FB_SH_MOBILE_MERAM
> Up to 4 memory channels can be configured, allowing 4 RGB or
> 2 YCbCr framebuffers to be configured.
>
> +source "drivers/video/tegra/host/Kconfig"
> +

This could be problematic. Since drivers/video and drivers/gpu/drm are
separate trees, this would entail a continuous burden on keeping both
trees synchronized. While I realize that eventually it might be better
to put the host1x driver in a separate place to accomodate for its use
by other subsystems, I'm not sure moving it here right away is the best
approach.

I'm not sure drivers/video is the best location either. Perhaps
drivers/bus would be better? Or maybe we need a new subdirectory for
this kind of device.

> diff --git a/drivers/video/tegra/host/chip_support.c b/drivers/video/tegra/host/chip_support.c
> new file mode 100644
> index 0000000..5a44147
> --- /dev/null
> +++ b/drivers/video/tegra/host/chip_support.c
> @@ -0,0 +1,48 @@
> +/*
> + * drivers/video/tegra/host/chip_support.c

I think the general nowadays is to no longer use filenames in comments.

[...]
> +struct nvhost_chip_support *nvhost_chip_ops;
> +
> +struct nvhost_chip_support *nvhost_get_chip_ops(void)
> +{
> + return nvhost_chip_ops;
> +}

This seems like it should be more tightly coupled to the host1x device.
And it shouldn't be a global variable.

> +
> +int nvhost_init_chip_support(struct nvhost_master *host)
> +{
> + if (nvhost_chip_ops == NULL) {
> + nvhost_chip_ops = kzalloc(sizeof(*nvhost_chip_ops), GFP_KERNEL);
> + if (nvhost_chip_ops == NULL) {
> + pr_err("%s: Cannot allocate nvhost_chip_support\n",
> + __func__);
> + return -ENOMEM;
> + }
> + }
> +
> + nvhost_init_host1x01_support(host, nvhost_chip_ops);
> + return 0;
> +}

We also don't need this. This should really be done by the central
host1x device's initialization.

> diff --git a/drivers/video/tegra/host/chip_support.h b/drivers/video/tegra/host/chip_support.h
[...]
> +struct output;

What's this? It doesn't seem to be used anywhere.

> +struct nvhost_master;

Why do you suffix this with _master? The whole point of host1x is to be
the "master" so you can just as well call it nvhost, right? Ideally
you'd call it host1x, but I'm repeating myself. =)

> +struct nvhost_syncpt;
> +struct platform_device;
> +
> +struct nvhost_syncpt_ops {
> + void (*reset)(struct nvhost_syncpt *, u32 id);
> + void (*reset_wait_base)(struct nvhost_syncpt *, u32 id);
> + void (*read_wait_base)(struct nvhost_syncpt *, u32 id);
> + u32 (*update_min)(struct nvhost_syncpt *, u32 id);
> + void (*cpu_incr)(struct nvhost_syncpt *, u32 id);
> + void (*debug)(struct nvhost_syncpt *);
> + const char * (*name)(struct nvhost_syncpt *, u32 id);
> +};

Why are these even defined as ops structure? Tegra20 and Tegra30 seem to
be compatible when it comes to handling syncpoints. I thought they would
even be compatible in all other aspects as well, so why even have this?

> +
> +struct nvhost_chip_support {
> + const char *soc_name;
> + struct nvhost_syncpt_ops syncpt;
> +};
> +
> +struct nvhost_chip_support *nvhost_get_chip_ops(void);
> +
> +#define syncpt_op() (nvhost_get_chip_ops()->syncpt)

You really shouldn't be doing this, but rather use explicit accesses for
these structures. If you're design doesn't scatter these definitions
across several files then it isn't difficult to obtain the correct
pointers and you don't need these "shortcuts".

> diff --git a/drivers/video/tegra/host/dev.c b/drivers/video/tegra/host/dev.c
[...]
> +u32 host1x_syncpt_incr_max(u32 id, u32 incrs)
> +{
> + struct nvhost_syncpt *sp = &nvhost->syncpt;
> + return nvhost_syncpt_incr_max(sp, id, incrs);
> +}
> +EXPORT_SYMBOL(host1x_syncpt_incr_max);

This API looks odd. Should syncpoints not be considered as regular
resources, much like interrupts? In that case it would be easier to
abstract them away behind an opaque type. It looks like you already use
the struct nvhost_syncpt to refer to the set of syncpoints associated
with a host1x device.

How about you use nvhost/host1x_syncpt to refer to individual syncpoints
instead. You could export an array of those from your host1x device and
implement a basic resource allocation mechanism on top, similar to how
other resources are handled in the kernel.

So a host1x client device could call host1x_request_syncpt() to allocate
a syncpoint from it's host1x parent dynamically along with passing a
name and a syncpoint handler to it.

> +
> +void host1x_syncpt_incr(u32 id)
> +{
> + struct nvhost_syncpt *sp = &nvhost->syncpt;
> + nvhost_syncpt_incr(sp, id);
> +}
> +EXPORT_SYMBOL(host1x_syncpt_incr);

Similarly, instead of passing an integer here, host1x clients would pass
a pointer to the requested syncpoint instead.

> +bool host1x_powered(struct platform_device *dev)
> +{
[...]
> +}
> +EXPORT_SYMBOL(host1x_powered);
> +
> +void host1x_busy(struct platform_device *dev)
> +{
[...]
> +}
> +EXPORT_SYMBOL(host1x_busy);
> +
> +void host1x_idle(struct platform_device *dev)
> +{
[...]
> +}
> +EXPORT_SYMBOL(host1x_idle);

These look like a reimplementation of the runtime power-management
framework.

> diff --git a/drivers/video/tegra/host/host1x/host1x.c b/drivers/video/tegra/host/host1x/host1x.c
[...]
> +struct nvhost_master *nvhost;

Bad habbit. I know that this is a popular shortcut. However this also
leads to very bad designs because you're allowed to reuse this pointer
from wherever you like.

When I wrote the tegra-drm code I explicitly made sure to not use any
such global variable. In the end it forces you to clean up the driver
design.

As a bonus you automatically get support for any number of host1x
devices on the same SoC. Now you will probably tell me that this is
never going to happen. People also used to think that a computers would
never use more than a single CPU...

> +static void power_on_host(struct platform_device *dev)
> +{
> + struct nvhost_master *host = nvhost_get_private_data(dev);
> +
> + nvhost_syncpt_reset(&host->syncpt);
> +}
> +
> +static int power_off_host(struct platform_device *dev)
> +{
> + struct nvhost_master *host = nvhost_get_private_data(dev);
> +
> + nvhost_syncpt_save(&host->syncpt);
> + return 0;
> +}

These seem like possible candidates for runtime PM.

> +
> +static void nvhost_free_resources(struct nvhost_master *host)
> +{
> +}

This should be removed since it's empty.

> +
> +static int __devinit nvhost_alloc_resources(struct nvhost_master *host)
> +{
> + int err;
> +
> + err = nvhost_init_chip_support(host);
> + if (err)
> + return err;
> +
> + return 0;
> +}

Again, this chip support concept is not useful, so this function can go
away as well. Also nvhost_init_chip_support() doesn't allocate any
resources so it shouldn't be called from this function in the first
place.

> +
> +static int __devinit nvhost_probe(struct platform_device *dev)
> +{
> + struct nvhost_master *host;
> + struct resource *regs, *intr0, *intr1;
> + int i, err;
> + struct nvhost_device_data *pdata =
> + (struct nvhost_device_data *)dev->dev.platform_data;

Platform data should not be used. Tegra is DT only.

> + regs = platform_get_resource(dev, IORESOURCE_MEM, 0);
> + intr0 = platform_get_resource(dev, IORESOURCE_IRQ, 0);
> + intr1 = platform_get_resource(dev, IORESOURCE_IRQ, 1);
> +
> + if (!regs || !intr0 || !intr1) {

I prefer to have these checked for explicitly, one by one for
readability and potentially more useful diagnostics.

Also you should be using platform_get_irq() for interrupts. Furthermore
the host1x DT node (and the TRM) name the interrupts "syncpt" and
"general", so maybe those would be more useful variable names than
"intr0" and "intr1".

But since you don't use them anyway they shouldn't be part of this
patch.

> + host = devm_kzalloc(&dev->dev, sizeof(*host), GFP_KERNEL);
> + if (!host)
> + return -ENOMEM;
> +
> + nvhost = host;
> +
> + host->dev = dev;
> +
> + /* Copy host1x parameters. The private_data gets replaced
> + * by nvhost_master later */

Multiline comments should be in this format:

/*
* foo
*/

> + memcpy(&host->info, pdata->private_data,
> + sizeof(struct host1x_device_info));

I don't think passing data in this way shouldn't be necessary as
discussed in the subthread on the Tegra AUXDATA.

> +
> + pdata->finalize_poweron = power_on_host;
> + pdata->prepare_poweroff = power_off_host;
> +
> + pdata->pdev = dev;
> +
> + /* set common host1x device data */
> + platform_set_drvdata(dev, pdata);
> +
> + /* set private host1x device data */
> + nvhost_set_private_data(dev, host);
> +
> + host->aperture = devm_request_and_ioremap(&dev->dev, regs);
> + if (!host->aperture) {

aperture is confusing as it is typically used for GTT-type memory
regions, so it may be mistaken for the GART found on Tegra 2. Why not
call it "regs" instead?

> + dev_err(&dev->dev, "failed to remap host registers\n");

This is unnecessary. devm_request_and_ioremap() already prints an error
message on failure.

> + for (i = 0; i < pdata->num_clks; i++)
> + clk_prepare_enable(pdata->clk[i]);
> + nvhost_syncpt_reset(&host->syncpt);
> + for (i = 0; i < pdata->num_clks; i++)
> + clk_disable_unprepare(pdata->clk[i]);

Stephen already hinted at this when discussing the AUXDATA. You should
explicitly request the clocks.

> +static int __exit nvhost_remove(struct platform_device *dev)

This should really be __devexit to allow the driver to be built as a
module. However, __dev* are deprecated and in the process of being
removed so you can just drop __exit as well.

> +static struct of_device_id host1x_match[] __devinitdata = {

__devinitdata can be dropped.

> + { .compatible = "nvidia,tegra20-host1x", },
> + { .compatible = "nvidia,tegra30-host1x", },
> + { },
> +};
> +
> +static struct platform_driver platform_driver = {
> + .probe = nvhost_probe,
> + .remove = __exit_p(nvhost_remove),

__exit_p also.

> + .suspend = nvhost_suspend,
> + .resume = nvhost_resume,
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = DRIVER_NAME,
> + .of_match_table = of_match_ptr(host1x_match),

No need for of_match_ptr().

> +static int __init nvhost_mod_init(void)
> +{
> + return platform_driver_register(&platform_driver);
> +}
> +
> +static void __exit nvhost_mod_exit(void)
> +{
> + platform_driver_unregister(&platform_driver);
> +}
> +
> +module_init(nvhost_mod_init);
> +module_exit(nvhost_mod_exit);

Use module_platform_driver().

> diff --git a/drivers/video/tegra/host/host1x/host1x.h b/drivers/video/tegra/host/host1x/host1x.h
[...]
> +#define TRACE_MAX_LENGTH 128U
> +#define IFACE_NAME "nvhost"

None of these seem to be used.

> +static inline void *nvhost_get_private_data(struct platform_device *_dev)
> +{
> + struct nvhost_device_data *pdata =
> + (struct nvhost_device_data *)platform_get_drvdata(_dev);
> + WARN_ON(!pdata);
> + return (pdata && pdata->private_data) ? pdata->private_data : NULL;
> +}
> +
> +static inline void nvhost_set_private_data(struct platform_device *_dev,
> + void *priv_data)
> +{
> + struct nvhost_device_data *pdata =
> + (struct nvhost_device_data *)platform_get_drvdata(_dev);
> + WARN_ON(!pdata);
> + if (pdata)
> + pdata->private_data = priv_data;
> +}

You should need none of these. Instead put all the data you need into
you struct host1x and associate that with the platform device using
platform_set_drvdata().

> +static inline
> +struct nvhost_master *nvhost_get_host(struct platform_device *_dev)
> +{
> + struct platform_device *pdev;
> +
> + if (_dev->dev.parent) {
> + pdev = to_platform_device(_dev->dev.parent);
> + return nvhost_get_private_data(pdev);
> + } else
> + return nvhost_get_private_data(_dev);
> +}
> +
> +static inline
> +struct platform_device *nvhost_get_parent(struct platform_device *_dev)
> +{
> + return _dev->dev.parent ? to_platform_device(_dev->dev.parent) : NULL;
> +}

These don't seem to be used.

> diff --git a/drivers/video/tegra/host/host1x/host1x01.c b/drivers/video/tegra/host/host1x/host1x01.c
> new file mode 100644
> index 0000000..d53302d
> --- /dev/null
> +++ b/drivers/video/tegra/host/host1x/host1x01.c
> @@ -0,0 +1,37 @@
> +/*
> + * drivers/video/tegra/host/host1x01.c
> + *
> + * Host1x init for T20 and T30 Architecture Chips
> + *
> + * Copyright (c) 2011-2012, NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/nvhost.h>
> +
> +#include "host1x/host1x01.h"
> +#include "host1x/host1x.h"
> +#include "host1x/host1x01_hardware.h"
> +#include "chip_support.h"
> +
> +#include "host1x/host1x_syncpt.c"
> +
> +int nvhost_init_host1x01_support(struct nvhost_master *host,
> + struct nvhost_chip_support *op)
> +{
> + host->sync_aperture = host->aperture + HOST1X_CHANNEL_SYNC_REG_BASE;

Usually you don't keep separate variables for subregions. This can
equally well be done with just adding a corresponding offset.

Then again, I already said that this whole chip support concept is
unnecessary and can be dropped.

> diff --git a/drivers/video/tegra/host/host1x/host1x_syncpt.c b/drivers/video/tegra/host/host1x/host1x_syncpt.c
[...]
> +/**
> + * Write the current syncpoint value back to hw.
> + */
> +static void host1x_syncpt_reset(struct nvhost_syncpt *sp, u32 id)
> +{
> + struct nvhost_master *dev = syncpt_to_dev(sp);
> + int min = nvhost_syncpt_read_min(sp, id);
> + writel(min, dev->sync_aperture + (host1x_sync_syncpt_0_r() + id * 4));
> +}

Again, better to represent individual syncpoints with opaque pointers
and dereference them here. Obviously this file will need access to the
structure definition.

> +static void host1x_syncpt_debug(struct nvhost_syncpt *sp)
> +{
> + u32 i;
> + for (i = 0; i < nvhost_syncpt_nb_pts(sp); i++) {
> + u32 max = nvhost_syncpt_read_max(sp, i);
> + u32 min = nvhost_syncpt_update_min(sp, i);
> + if (!max && !min)
> + continue;
> + dev_info(&syncpt_to_dev(sp)->dev->dev,
> + "id %d (%s) min %d max %d\n",
> + i, syncpt_op().name(sp, i),
> + min, max);
> +
> + }
> +
> + for (i = 0; i < nvhost_syncpt_nb_bases(sp); i++) {
> + u32 base_val;
> + host1x_syncpt_read_wait_base(sp, i);
> + base_val = sp->base_val[i];
> + if (base_val)
> + dev_info(&syncpt_to_dev(sp)->dev->dev,
> + "waitbase id %d val %d\n",
> + i, base_val);
> +
> + }
> +}

This should probably be integrated with debugfs.

> diff --git a/drivers/video/tegra/host/host1x/hw_host1x01_sync.h b/drivers/video/tegra/host/host1x/hw_host1x01_sync.h

Autogenerated files are generally not acceptable. And I already
mentioned before that you should be using #define instead of static
inline functions for register and bit definitions.

> diff --git a/drivers/video/tegra/host/nvhost_acm.c b/drivers/video/tegra/host/nvhost_acm.c
[...]

This whole file largely looks like a reimplementation of runtime PM. You
should investigate if you can't reuse the existing infrastructure.

> + /* Init the power sysfs attributes for this device */
> + pdata->power_attrib = devm_kzalloc(&dev->dev,
> + sizeof(struct nvhost_device_power_attr),
> + GFP_KERNEL);
> + if (!pdata->power_attrib) {
> + dev_err(&dev->dev, "Unable to allocate sysfs attributes\n");
> + return -ENOMEM;
> + }
> + pdata->power_attrib->ndev = dev;
> +
> + pdata->power_kobj = kobject_create_and_add("acm", &dev->dev.kobj);
> + if (!pdata->power_kobj) {
> + dev_err(&dev->dev, "Could not add dir 'power'\n");
> + err = -EIO;
> + goto fail_attrib_alloc;
> + }
> +
> + attr = &pdata->power_attrib->power_attr[NVHOST_POWER_SYSFS_ATTRIB_CLOCKGATE_DELAY];
> + attr->attr.name = "clockgate_delay";
> + attr->attr.mode = S_IWUSR | S_IRUGO;
> + attr->show = clockgate_delay_show;
> + attr->store = clockgate_delay_store;
> + if (sysfs_create_file(pdata->power_kobj, &attr->attr)) {
> + dev_err(&dev->dev, "Could not create sysfs attribute clockgate_delay\n");
> + err = -EIO;
> + goto fail_clockdelay;
> + }
> +
> + attr = &pdata->power_attrib->power_attr[NVHOST_POWER_SYSFS_ATTRIB_POWERGATE_DELAY];
> + attr->attr.name = "powergate_delay";
> + attr->attr.mode = S_IWUSR | S_IRUGO;
> + attr->show = powergate_delay_show;
> + attr->store = powergate_delay_store;
> + if (sysfs_create_file(pdata->power_kobj, &attr->attr)) {
> + dev_err(&dev->dev, "Could not create sysfs attribute powergate_delay\n");
> + err = -EIO;
> + goto fail_powergatedelay;
> + }
> +
> + attr = &pdata->power_attrib->power_attr[NVHOST_POWER_SYSFS_ATTRIB_REFCOUNT];
> + attr->attr.name = "refcount";
> + attr->attr.mode = S_IRUGO;
> + attr->show = refcount_show;
> + if (sysfs_create_file(pdata->power_kobj, &attr->attr)) {
> + dev_err(&dev->dev, "Could not create sysfs attribute refcount\n");
> + err = -EIO;
> + goto fail_refcount;
> + }

This is a very funky way of creating sysfs attributes. What you probably
want here are device attributes. See Documentation/filesystems/sysfs.txt
and include/linux/sysfs.h.

But if you can replace this by runtime PM, you'll get similar
functionality for free anyway.

> diff --git a/drivers/video/tegra/host/nvhost_syncpt.c b/drivers/video/tegra/host/nvhost_syncpt.c
[...]
> +/**
> + * Returns true if syncpoint is expired, false if we may need to wait
> + */
> +bool nvhost_syncpt_is_expired(
> + struct nvhost_syncpt *sp,
> + u32 id,
> + u32 thresh)
> +{
> + u32 current_val;
> + u32 future_val;
> + smp_rmb();

What do you need a read memory barrier for?

> +/* Displays the current value of the sync point via sysfs */
> +static ssize_t syncpt_min_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct nvhost_syncpt_attr *syncpt_attr =
> + container_of(attr, struct nvhost_syncpt_attr, attr);
> +
> + return snprintf(buf, PAGE_SIZE, "%u",
> + nvhost_syncpt_read(&syncpt_attr->host->syncpt,
> + syncpt_attr->id));
> +}
> +
> +static ssize_t syncpt_max_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct nvhost_syncpt_attr *syncpt_attr =
> + container_of(attr, struct nvhost_syncpt_attr, attr);
> +
> + return snprintf(buf, PAGE_SIZE, "%u",
> + nvhost_syncpt_read_max(&syncpt_attr->host->syncpt,
> + syncpt_attr->id));
> +}

This looks like it belongs in debugfs.

> +int nvhost_syncpt_init(struct platform_device *dev,
> + struct nvhost_syncpt *sp)
> +{
> + int i;
> + struct nvhost_master *host = syncpt_to_dev(sp);
> + int err = 0;
> +
> + /* Allocate structs for min, max and base values */
> + sp->min_val = kzalloc(sizeof(atomic_t) * nvhost_syncpt_nb_pts(sp),
> + GFP_KERNEL);
> + sp->max_val = kzalloc(sizeof(atomic_t) * nvhost_syncpt_nb_pts(sp),
> + GFP_KERNEL);
> + sp->base_val = kzalloc(sizeof(u32) * nvhost_syncpt_nb_bases(sp),
> + GFP_KERNEL);
> + sp->lock_counts =
> + kzalloc(sizeof(atomic_t) * nvhost_syncpt_nb_mlocks(sp),
> + GFP_KERNEL);

Again, I really think that syncpoints should be objects with
corresponding attributes instead of keeping them in these arrays.

> diff --git a/include/linux/nvhost.h b/include/linux/nvhost.h
[...]
> +struct host1x_device_info {
> + int nb_channels; /* host1x: num channels supported */
> + int nb_pts; /* host1x: num syncpoints supported */
> + int nb_bases; /* host1x: num syncpoints supported */
> + u32 client_managed; /* host1x: client managed syncpts */
> + int nb_mlocks; /* host1x: number of mlocks */
> + const char **syncpt_names; /* names of sync points */
> +};
> +
> +struct nvhost_device_data {
> + int version; /* ip version number of device */
> + int id; /* Separates clients of same hw */
> + int index; /* Hardware channel number */
> + void __iomem *aperture; /* Iomem mapped to kernel */
> +
> + u32 syncpts; /* Bitfield of sync points used */
> + u32 modulemutexes; /* Bit field of module mutexes */
> +
> + u32 class; /* Device class */
> + bool serialize; /* Serialize submits in the channel */
> +
> + int powergate_ids[NVHOST_MODULE_MAX_POWERGATE_IDS];
> + bool can_powergate; /* True if module can be power gated */
> + int clockgate_delay;/* Delay before clock gated */
> + int powergate_delay;/* Delay before power gated */
> + struct nvhost_clock clocks[NVHOST_MODULE_MAX_CLOCKS];/* Clock names */
> +
> + struct delayed_work powerstate_down;/* Power state management */
> + int num_clks; /* Number of clocks opened for dev */
> + struct clk *clk[NVHOST_MODULE_MAX_CLOCKS];
> + struct mutex lock; /* Power management lock */
> + int powerstate; /* Current power state */
> + int refcount; /* Number of tasks active */
> + wait_queue_head_t idle_wq; /* Work queue for idle */
> +
> + struct nvhost_channel *channel; /* Channel assigned for the module */
> + struct kobject *power_kobj; /* kobj to hold power sysfs entries */
> + struct nvhost_device_power_attr *power_attrib; /* sysfs attributes */
> + struct dentry *debugfs; /* debugfs directory */
> +
> + void *private_data; /* private platform data */
> + struct platform_device *pdev; /* owner platform_device */
> +
> + /* Finalize power on. Can be used for context restore. */
> + void (*finalize_poweron)(struct platform_device *dev);
> +
> + /* Device is busy. */
> + void (*busy)(struct platform_device *);
> +
> + /* Device is idle. */
> + void (*idle)(struct platform_device *);
> +
> + /* Device is going to be suspended */
> + void (*suspend_ndev)(struct platform_device *);
> +
> + /* Device is initialized */
> + void (*init)(struct platform_device *dev);
> +
> + /* Device is de-initialized. */
> + void (*deinit)(struct platform_device *dev);
> +
> + /* Preparing for power off. Used for context save. */
> + int (*prepare_poweroff)(struct platform_device *dev);
> +
> + /* Clock gating callbacks */
> + int (*prepare_clockoff)(struct platform_device *dev);
> + void (*finalize_clockon)(struct platform_device *dev);
> +};

A lot of this can be removed if you use existing infrastructure and
simplify the design a bit. Most of it can probably move into the main
struct host1x to avoid needless indirections that make the code hard to
follow and maintain.

Thierry

Attachments:

(No filename) (22.12 kB)
(No filename) (836.00 B)
Download all attachments

2012-11-29 07:37:18

by Mark Zhang

[permalink] [raw]

Subject: Re: [RFC,v2,8/8] drm: tegra: Add gr2d device

On 11/26/2012 09:19 PM, Terje Bergström <[email protected]> wrote:
> Add client driver for 2D device.
>
> Signed-off-by: Arto Merilainen <[email protected]>
> Signed-off-by: Terje Bergstrom <[email protected]>
>
> ---
>
[...]
> +
> +static int
> +tegra_drm_ioctl_open_channel(struct drm_device *drm, void *data,
> + struct drm_file *file_priv)
> +{
> + struct tegra_drm_open_channel_args *args = data;
> + struct tegra_drm_client *client;
> + struct tegra_drm_context *context;
> + struct tegra_drm_fpriv *fpriv = tegra_drm_fpriv(file_priv);
> + int err = 0;
> +
> + dev_dbg(drm->dev, "> %s(fpriv=%p, class=%x)\n", __func__,
> + fpriv, args->class);
> +
> + context = kzalloc(sizeof(*context), GFP_KERNEL);
> + if (!context) {
> + err = -ENOMEM;
> + goto out;

Change to "return -ENOMEM". Otherwise the NULL "context" will be kfree.

> + }
> +
> + list_for_each_entry(client, &tegra_drm_subdrv_list, list) {
> + if (client->class == args->class) {
> + dev_dbg(drm->dev, "opening client %x\n", args->class);
> + context->client = client;
> + err = client->ops->open_channel(client, context);
> + if (err)
> + goto out;
> +
> + dev_dbg(drm->dev, "context %p\n", context);
> + list_add(&context->list, &fpriv->contexts);
> + args->context = context;
> + goto out;
> + }
> + }
> + err = -ENODEV;
> +
> +out:
> + if (err)
> + kfree(context);
> +
> + dev_dbg(drm->dev, "< %s() = %d\n", __func__, err);
> + return err;
> +}
> +
> +static int
> +tegra_drm_ioctl_close_channel(struct drm_device *drm, void *data,
> + struct drm_file *file_priv)
> +{
> + struct tegra_drm_open_channel_args *args = data;
> + struct tegra_drm_context *context;
> + struct tegra_drm_fpriv *fpriv = tegra_drm_fpriv(file_priv);
> + int err = 0;
> +
> + dev_dbg(drm->dev, "> %s(fpriv=%p)\n", __func__, fpriv);
> + list_for_each_entry(context, &fpriv->contexts, list) {

Consider "list_for_each_entry_safe" cause you remove list members during
the loop.

> + if (context == args->context) {
> + context->client->ops->close_channel(context);
> + list_del(&context->list);
> + kfree(context);
> + goto out;
> + }
> + }
> + err = -EINVAL;
> +
> +out:
> + dev_dbg(drm->dev, "< %s() = %d\n", __func__, err);
> + return err;
> +}
> +
[...]
> +
> +static int gr2d_submit(struct tegra_drm_context *context,
> + struct tegra_drm_submit_args *args)

I'm still in the middle of code reading of job submit, so I'll get you
back if I find something.

[...]
> +
> +static struct of_device_id gr2d_match[] __devinitdata = {
> + { .compatible = "nvidia,tegra20-gr2d", },
> + { .compatible = "nvidia,tegra30-gr2d", },

Just as swarren mentioned, you'd better place "nvidia,tegra30-gr2d" in
the front of "nvidia,tegra20-gr2d"...

[...]
> +
> +#define DRM_TEGRA_GEM_CREATE 0x00
> +
> +#define DRM_IOCTL_TEGRA_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + \
> + DRM_TEGRA_GEM_CREATE, struct tegra_gem_create)
> +

Just a very minor suggestion: could you put them at the end of this
file? I mean, stay with other IOCTLs like SYNCPT_READ, SYNCPT_WAIT...

[...]
> +
> +#define DRM_TEGRA_DRM_SYNCPT_READ 0x01
> +#define DRM_TEGRA_DRM_SYNCPT_INCR 0x02
> +#define DRM_TEGRA_DRM_SYNCPT_WAIT 0x03
> +#define DRM_TEGRA_DRM_OPEN_CHANNEL 0x04
> +#define DRM_TEGRA_DRM_CLOSE_CHANNEL 0x05
> +#define DRM_TEGRA_DRM_GET_SYNCPOINTS 0x06
> +#define DRM_TEGRA_DRM_GET_MODMUTEXES 0x07
> +#define DRM_TEGRA_DRM_SUBMIT 0x08
> +
> +#define DRM_IOCTL_TEGRA_DRM_SYNCPT_READ DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_SYNCPT_READ, struct tegra_drm_syncpt_read_args)
> +#define DRM_IOCTL_TEGRA_DRM_SYNCPT_INCR DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_SYNCPT_INCR, struct tegra_drm_syncpt_incr_args)
> +#define DRM_IOCTL_TEGRA_DRM_SYNCPT_WAIT DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_SYNCPT_WAIT, struct tegra_drm_syncpt_wait_args)
> +#define DRM_IOCTL_TEGRA_DRM_OPEN_CHANNEL DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_OPEN_CHANNEL, struct tegra_drm_open_channel_args)
> +#define DRM_IOCTL_TEGRA_DRM_CLOSE_CHANNEL DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_CLOSE_CHANNEL, struct tegra_drm_open_channel_args)
> +#define DRM_IOCTL_TEGRA_DRM_GET_SYNCPOINTS DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_GET_SYNCPOINTS, struct tegra_drm_get_channel_param_args)
> +#define DRM_IOCTL_TEGRA_DRM_GET_MODMUTEXES DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_GET_MODMUTEXES, struct tegra_drm_get_channel_param_args)
> +#define DRM_IOCTL_TEGRA_DRM_SUBMIT DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_DRM_SUBMIT, struct tegra_drm_submit_args)
> +
> +#endif
>

2012-11-29 08:14:05

by Terje Bergstrom

[permalink] [raw]

Subject: Re: [RFC v2 8/8] drm: tegra: Add gr2d device

On 28.11.2012 20:46, Lucas Stach wrote:
> Am Mittwoch, den 28.11.2012, 18:23 +0200 schrieb Terje Bergström:
>> Sorry. I promised in another thread a write-up explaining the design. I
>> still owe you guys that.
> That would be really nice to have. I'm also particularly interested in
> how you plan to do synchronization of command streams to different
> engines working together, if that's not too much to ask for now. Like
> userspace uploading a texture in a buffer, 2D engine doing mipmap
> generation, 3D engine using mipmapped texture.

I can briefly explain (and then copy-paste to a coherent text once I get
to it) how inter-engine synchronization is done. It's not specifically
for 2D or 3D, but generic to any host1x client.

Sync point register is a counter that can only increase. It starts from
0 and is incremented by a host1x client or CPU. host1x can freeze a
channel until a sync point value is reached, and it can trigger an
interrupt upon reaching a threshold. On Tegra2 and Tegra3 we have 32
sync points.

host1x clients all implement a method for incrementing a sync point
based on a condition, and on all of them (well, not entirely true) the
register is number 0. The most used condition is op_done, telling the
client to increment sync point once the previous operations are done.

In kernel, we keep track of the active range of sync point values, i.e.
ones we expect to be reached with the active jobs. Active range's
minimum is the current value read from hw and shadowed in memory. At job
submit time, kernel increments the maximum by the number of sync points
the stream announces it will perform. After performing the increment, we
have a number, which the sync point is supposed to reach at the end of
submit. That number is the the fence and it is recorded in kernel and
returned to user space.

So, when user space flushes, it receives a fence. It can insert the
fence into another command stream as parameter to a host1x channel wait.
This makes that channel freeze until an operation in another channel is
finished. That's how different host1x clients can synchronize without
using CPU.

Kernel's sync point wait essentially puts the process into sleep until
host1x sends an interrupt and we determine the value that a process is
waiting for, has been reached.

On top of this, we guard against wrapping issues by nulling out any sync
point waits (CPU or inside stream) that are waiting for values outside
the active range, and we have a timeout for jobs so that we can kick out
misbehaving command streams.

> Ah yes I see. So if we consider nvhost to be the central entity in
> charge of controlling all host1x clients and tegradrm as the interface
> that happens to bundle display, 2d and 3d engine functionality into it's
> interface we should probably aim for two things:
> 1. Move everything needed by all engines down into nvhost (I especially
> see the allocator falling under this point, I'll explain why this would
> be beneficial a bit later)

Ok. This is almost the current design, except for the allocator.

> 2. Move the exposed DRM interface more in line with other DRM drivers.
> Please take a look at how for example the GEM_EXECBUF ioctl works on
> other drivers to get a feeling of what I'm talking about. Everything
> using the display, 2D and maybe later on the 3D engine should only deal
> with GEM handles. I really don't like the idea of having a single
> userspace application, which uses engines with similar and known
> requirements (DDX) dealing with dma-buf handles or other similar high
> overhead stuff to do the most basic tasks.
> If we move down the allocator into nvhost we can use buffers allocated
> from this to back GEM or V4L2 buffers transparently. The ioctl to
> allocate a GEM buffer shouldn't do much more than wrapping the nvhost
> buffer.

Ok, this is actually what we do downstream. We use dma-buf handles only
for purposes where they're really needed (in fact, none yet), and use
our downstream allocator handles for the rest. I did this, because
benchmarks were showing that memory management overhead shoot through
the roof if I tried doing everything via dma-buf.

We can move support for allocating GEM handles to nvhost, and GEM
handles can be treated just as another memory handle type in nvhost.
tegradrm would then call nvhost for allocation.

> This may also solve your problem with having multiple mappings of the
> same buffer into the very same address space, as nvhost is the single
> instance that manages all host1x client address spaces. If the buffer is
> originating from there you can easily check if it's already mapped. For
> Tegra 3 to do things in an efficient way we likely have to move away
> from dealing with the DMA API to dealing with the IOMMU API, this gets a
> _lot_ easier_ if you have a single point where you manage memory
> allocation and address space.

Yep, this would definitely simplify our IOMMU problem. But, I thought
the canonical way of dealing with device memory is DMA API, and you're
saying that we should just bypass it and call IOMMU directly?

>> Concurrency is handled with sync points. User space will know when a
>> command stream is processed and can be reused by comparing the current
>> sync point value, and the fence that 2D driver returned to user space.
>> User space can have a pool of buffers and can recycle when it knows it
>> can do so. But, this is not enforced by kernel.
>>
> This is the point where we have differ: You have to deal with syncpts in
> kernel anyway, otherwise you don't know when it's safe to destroy a
> buffer. And no, userspace should not have the ability to destroy a
> buffer itself, userspace should always just be able to free it's
> reference to the buffer. Remember: never trust the userspace. And if you
> are dealing with syncpts in kernel anyway, you can just go ahead and
> enforce some sane concurrency rules. There may be some corener cases
> related to userspace suballocating a kernel buffer, which might need
> some more thought still, but that's not a valid excuse to not do any
> concurrency validation in kernel.

nvhost is already dealing with sync points, and protecting memory from
being freed if it's used. We use refcounting to do that. When a job is
sent to hw, we get reference to all memory (command stream & surfaces).
When job is done (fence reached), nvhost unreferences them. User space
can free the memory it has allocated, but kernel would hold on to it
until it's safe to actually release the memory.

>> The difference with your proposal and what I posted is the level of
>> control user space has over its command stream management. But as said,
>> 2D streams are so short that my guess is that there's not too much
>> penalty copying it to kernel managed host1x push buffer directly instead
>> of inserting a GATHER reference.
>>
> This an implementation detail. Whether you shoot down the old pushbuf
> mapping and insert a new one pointing to free backing memory (which may
> be the way to go for 3D) or do an immediate copy of the channel pushbuf
> contents to the host1x pushbuf (which may be beneficial for very small
> pushs) is all the same. Both methods implicitly guarantee that the
> memory mapped by userspace always points to a location the CPU can write
> to without interfering with the GPU.

Ok. Based on this, I propose the way to go for cases without IOMMU
support and all Tegra20 cases (as Tegra20's GART can't provide memory
protection) is to copy the stream to host1x push buffer. In Tegra30 with
IOMMU support we can just reference the buffer. This way we don't have
to do expensive MMU operations.

> I really enjoyed the discussion so far and hope we can get to the point
> where we have a nice design/interface, working together.

Thanks. I don't have a strict deadline for upstreaming, so I'm fine
continuing with discussion until we're settled, and then doing the
needed changes.

My goal is to end up with something that we can take advantage of in
upstream with tegradrm and 2D, but also downstream with the rest of the
downstream stack. That way there's no technical barrier to us moving in
downstream to use the upstream code.

Terje

2012-11-29 08:44:10

2012-11-29 12:14:40

Just replying to part of your mail.

On 30.11.2012 09:22, Thierry Reding wrote:
> Actually for the display controller we want just a notification when the
> VBLANK happens. I'm not sure if we want to do that with syncpoints at
> all since it works quite well using regular interrupts.

VBLANK isn't actually a very good example of dc's use of sync points.
That can easily be done with regular interrupts, as you mention.

More important is when we have double buffering enabled. When you draw
something to a surface, and flip it to display, you want DC to notify
when the flip has been done and rendering can continue to the back buffer.

So, what you can do is return a fence from DC when initiating a flip,
and place that fence into 2D stream as a host wait so that 2D will
patiently wait for buffer to become free before it renders.

> What I'm proposing is to leave it up to each host1x client how they want
> to handle this. For display controllers it may be enough to have their
> callback run in interrupt context but other clients may need to do more
> work so they can queue it themselves.

DC doesn't need to worry about host1x interrupts at all. It's all
internal to the host1x driver, so we're now just talking about the
internal implementation of host1x.

We have two scenarios for the syncpt interrupts. One is that a job got
finished and we need to clean up the queue and free up resources. This
must be done in threads. Other is releasing a thread that is blocked by
a syncpt wait.

It's simpler if both of these are handled with the same infrastructure,
and we've shown that latency is very good even if we handle all events
in a thread.

> I know that this looks like it might be more work, but if it turns out
> that many drivers need to do the exact same thing, that functionality
> can be factored out into a helper. But it may just as well turn out that
> the requirements for each module are slightly different that forcing a
> workqueue on them could result in ugly workarounds because it doesn't
> quite work for them.

This is just driver internal, so there's no need for other drivers to
access this part.

> If we move responsibility of managing the workqueue out of host1x as I
> proposed above, maybe a lot of this code can be removed. Maybe you can
> explain a bit what they are used for exactly in your write-up.

It's going to be a big bad boy. :-)

Terje

2012-11-30 07:40:52

by Terje Bergstrom

[permalink] [raw]

Subject: Re: [RFC v2 8/8] drm: tegra: Add gr2d device

On 29.11.2012 14:14, Thierry Reding wrote:
> On Thu, Nov 29, 2012 at 10:09:13AM +0100, Lucas Stach wrote:
>> This way you would also be able to construct different handles (like GEM
>> obj or V4L2 buffers) from the same backing nvhost object. Note that I'm
>> not sure how useful this would be, but it seems like a reasonable design
>> to me being able to do so.
>
> Wouldn't that be useful for sharing buffers between DRM and V4L2 using
> dma-buf? I'm not very familiar with how exactly importing and exporting
> work with dma-buf, so maybe I need to read up some more.

I would still preserve the dma-buf support, for exactly this purpose.

Terje

2012-11-30 07:47:05

On Fri, Nov 30, 2012 at 10:56:39AM +0200, Terje Bergström wrote:
> On 29.11.2012 13:47, Thierry Reding wrote:
> > On Thu, Nov 29, 2012 at 12:21:04PM +0200, Terje Bergström wrote:
> >> Tegra20 and Tegra30 are compatible, but future chips are not. I was
> >> hoping we would be ready in upstream kernel for future chips.
> >
> > I think we should ignore that problem for now. Generally planning for
> > any possible combination of incompatibilities leads to overgeneralized
> > designs that require precisely these kinds of indirections.
> >
> > Once some documentation for Tegra 40 materializes we can start thinking
> > about how to encapsulate the incompatible code.
>
> I think here our perspectives differ a lot. That is natural considering
> the company I work for and company you work for, so let's try to sync
> the perspective.
>
> In my reality, whatever is in market is old news and I barely work on
> them anymore. Upstreaming activity is the exception. 90% of my time is
> spent dealing with future chips which I know cannot be handled without
> this split to logical and physical driver parts.
>
> For you, Tegra2 and Tegra3 are the reality.

To be fair, Tegra2 and Tegra3 are the reality for *everyone* *outside*
NVIDIA.

It's great that you spend most of your time working with future chips,
but unless you submit the code for inclusion or review nobody upstream
needs to be concerned about the implications. Most people don't have
time to waste so we naturally try to keep the maintenance burden to a
minimum.

The above implies that when you submit code it shouldn't contain pieces
that prepare for possible future extensions which may or may not be
submitted (the exception being if such changes are part of a series
where subsequent patches actually use them). The outcome is that the
amount of cruft in the mainline kernel is kept to a minimum. And that's
a very good thing.

> If we move nvhost in upstream a bit incompatible, that's fine, like
> ripping out features or adding new new stuff, like a new memory type.
> All of this I can support with a good diff tool to get all the patches
> flowing between upstream and downstream.
>
> If we do fundamental changes that prevent bringing the code back to
> downstream, like removing this abstraction, the whole process of
> upstream and downstream converging hits a brick wall. We wouldn't have
> proper continuing co-operation, but just pushing code out and being done
> with it.

Generally upstream doesn't concern itself with downstream. However we
still willingly accept code that is submitted for upstream inclusion
independent of where it comes from. The only requirements are that the
code conforms to the established standards and has gone through an
appropriate amount of review. Downstream maintenance is up to you. If
you need to maintain code that doesn't meet the above requirements or
that you don't want to submit or haven't got around to yet that's your
problem.

If you're serious about wanting to derive your downstream kernel from a
mainline kernel, then the only realistic way for you to reduce your
amount of work is to push your code upstream. And typically the earlier
you do so, the better.

> >> I could move this to debug.c, but it's debugging aid when a command
> >> stream is misbehaving and it spews this to UART when sync point wait is
> >> timing out. So not debugfs stuff.
> >
> > Okay, in that case it should stay in. Perhaps convert dev_info() to
> > dev_dbg(). Perhaps wrapping it in some #ifdef CONFIG_TEGRA_HOST1X_DEBUG
> > guards would also be useful. Maybe not.
>
> I could do that for upstream. In downstream it cannot depend on DEBUG
> flag, as these spews are an important part of how we debug problems with
> customer devices and the DEBUG flag is never on in customer builds.

So I've just looked through these patches once more and I can't find
where this functionality is actually used. The host1x_syncpt_debug()
function is assigned to the nvhost_syncpt_ops.debug member, which in
turn is only used by nvhost_syncpt_debug(). The latter, however is
never used (not even by the debug infrastructure introduced in patch
4).

> >> I like static inline because I get the benefit of compiler type
> >> checking, and gcov shows me which register definitions have been used in
> >> different tests.
> >
> > Type checking shouldn't be necessary for simple defines. And I wasn't
> > aware that you could get the Linux kernel to write out data to be fed to
> > gcov.
> >
> >> #defines are always messy and I pretty much hate them. But if the
> >> general request is to use #define's, even though I don't agree, I can
> >> accommodate. It's simple to write a sed script to do the conversion.
> >
> > There are a lot of opportunities to abuse #defines but they are harmless
> > for register definitions. The Linux kernel is full of them and I haven't
> > yet seen any code that uses static inline functions for this purpose.
>
> My problem is just that I know that the code generated is the same. What
> we're talking about is that should we let the preprocessor or compiler
> take care of this.
>
> My take is that using preprocessor is not wise - it's the last resort if
> there's no other proper way of doing things. Preprocessor requires all
> sorts of extra parenthesis to protect against its deficiencies, and it
> it merely a tool to do search-and-replace. Even multi-line needs special
> treatment.

Okay, so what you're saying here is that a huge number of people haven't
been wise in using the preprocessor for register definitions all these
years. That's a pretty bold statement. Now I obviously haven't looked at
every single line in the kernel, but I have never come across this usage
for static inline functions used for this. So, to be honest, I don't
think this is really up for discussion. Of course if you come up with an
example where this is done in a similar way I could be persuaded
otherwise.

> > What you need to consider as well is that many people that work with the
> > Linux kernel expect code to be in a certain style. Register accesses of
> > the form
> >
> > writel(value, base + OFFSET);
> >
> > are very common and expected to look a certain way, so if you write code
> > that doesn't comply with these guidelines you make it extra hard for
> > people to read the code. And that'll cost extra time, which people don't
> > usually have in excess.
>
> But this has nothing to do with static inline vs. #define anymore, right?

Of course it has. With the way you've chosen to define registers the
code will look like this:

writel(value, base + offset_r())

Maybe it's just me, but when I read code like that I need additional
time to parse it as opposed to the canonical form.

> > Maybe you can explain the usefulness of this some more. Why would it be
> > easier to look at them in sysfs than in debugfs? You could be providing
> > a simple list of syncpoints along with min/max, name, requested status,
> > etc. in debugfs and it should be as easy to parse for both humans and
> > machines as sysfs. I don't think IOCTLs would be any gain as they tend
> > to have higher ABI stability requirements than debugfs (which doesn't
> > have very strong requirements) or sysfs (which is often considered as a
> > public ABI as well and therefore needs to be stable).
>
> debugfs is just a debugging tool, and user space cannot rely on it. Only
> developers can rely on existence of debugfs, as they have the means to
> enable it.
>
> sysfs is a place for actual APIs as you mention, and user space can rely
> on them as proper APIs. That's what the values were exported for.

But I don't see how that's relevant here. Let me quote what you said
originally:

> This is actually the only interface to read the max value to user space,
> which can be useful for doing some comparisons that take wrapping into
> account. But we could just add IOCTLs and remove the sysfs entries.

To me that sounded like it was only used for debugging purposes. If you
actually need to access this from a userspace driver then, as opposed to
what I said earlier, this should be handled by some IOCTL.

Thierry

Attachments:

(No filename) (7.96 kB)
(No filename) (836.00 B)
Download all attachments