Message-ID: <20130205095403.GF20437@avionic-0098.mockup.avionic-design.de>
Date:	Tue, 5 Feb 2013 10:54:04 +0100
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Terje Bergström <tbergstrom@...dia.com>
Cc:	Arto Merilainen <amerilainen@...dia.com>,
	"airlied@...ux.ie" <airlied@...ux.ie>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv5,RESEND 8/8] drm: tegra: Add gr2d device

On Mon, Feb 04, 2013 at 09:17:45PM -0800, Terje Bergström wrote:
> On 04.02.2013 04:56, Thierry Reding wrote:
> > On Tue, Jan 15, 2013 at 01:44:04PM +0200, Terje Bergstrom wrote:
> >> +{
> >> +     struct host1x *host1x = drm->dev_private;
> >> +     struct tegra_drm_syncpt_read_args *args = data;
> >> +     struct host1x_syncpt *sp =
> >> +             host1x_syncpt_get_bydev(host1x->dev, args->id);
> > 
> > I don't know if we need this, except maybe to work around the problem
> > that we have two different structures named host1x. The _bydev() suffix
> > is misleading because all you really do here is obtain the syncpt from
> > the host1x.
> 
> Yeah, it's actually working around the host1x duplicate naming.
> host1x_syncpt_get takes struct host1x as parameter, but that's different
> host1x than in this code.

So maybe a better way would be to rename the DRM host1x after all. If it
avoids the need for workarounds such as this I think it justifies the
additional churn.

> > Also, how useful is it to create a context? Looking at the gr2d
> > implementation for .open_channel(), it will return the same channel to
> > whichever userspace process requests them. Can you explain why it is
> > necessary at all? From the name I would have expected some kind of
> > context switching to take place when different applications submit
> > requests to the same client, but that doesn't seem to be the case.
> 
> Hardware context switching will be a later submit, and it'll actually
> create a new structure. Hardware context might live longer than the
> process that created it, so they'll need to be separate.

Why would it live longer than the process? Isn't the whole purpose of
the context to keep per-process state? What use is that state if the
process dies?

> We've used the context as a place for storing flags and the reference to
> hardware context. It'd allow also opening channels to multiple devices,
> and context would be used in submit to find out the target device. But
> as hardware context switching is not implemented in this patch set, and
> neither is support for anything but 2D, it's difficult to justify it.
> 
> Perhaps the justification is that this way we can keep the kernel API
> stable even when we add support for hardware contexts and other clients.

We don't need a stable kernel API. But I guess it is fine to keep it if
for no other reason to fill the context returned in the ioctl() with
meaningful data.

> >> diff --git a/drivers/gpu/host1x/drm/drm.h b/drivers/gpu/host1x/drm/drm.h
> > [...]
> >> +struct host1x_drm_fpriv {
> >> +     struct list_head contexts;
> >>  };
> > 
> > Maybe name this host1x_drm_file. fpriv isn't very specific.
> 
> host1x_drm_file sounds a bit odd, because it's not really a file, but a
> private data pointer stored in driver_priv.

The same is true for struct drm_file, which is stored in struct file's
.private_data field. I find it to be very intuitive if the inheritance
is reflected in the structure name. struct host1x_drm_file is host1x'
driver-specific part of struct drm_file.

> >> +static u32 handle_cma_to_host1x(struct drm_device *drm,
> >> +                             struct drm_file *file_priv, u32 gem_handle)
> >> +{
> >> +     struct drm_gem_object *obj;
> >> +     struct drm_gem_cma_object *cma_obj;
> >> +     u32 host1x_handle;
> >> +
> >> +     obj = drm_gem_object_lookup(drm, file_priv, gem_handle);
> >> +     if (!obj)
> >> +             return 0;
> >> +
> >> +     cma_obj = to_drm_gem_cma_obj(obj);
> >> +     host1x_handle = host1x_memmgr_host1x_id(mem_mgr_type_cma, (u32)cma_obj);
> >> +     drm_gem_object_unreference(obj);
> >> +
> >> +     return host1x_handle;
> >> +}
> > 
> > I though we had settled in previous reviews on only having a single
> > allocator and not do the conversion between various types?
> 
> I'll need to agree with Lucas on how to handle this. He intended to make
> a patch to fix this, but he hasn't had time to do that.
> 
> But, I'd still like to keep the possibility open to add dma_buf as
> memory handle type, and fit that into the same API, so there's still a
> need to have the mem_mgr_type abstraction.

I fail to see how dma_buf would require a separate mem_mgr_type. Can we
perhaps postpone this to a later point and just go with CMA as the only
alternative for now until we have an actual working implementation that
we can use this for?

> >> +static int gr2d_submit(struct host1x_drm_context *context,
> >> +             struct tegra_drm_submit_args *args,
> >> +             struct drm_device *drm,
> >> +             struct drm_file *file_priv)
> >> +{
> >> +     struct host1x_job *job;
> >> +     int num_cmdbufs = args->num_cmdbufs;
> >> +     int num_relocs = args->num_relocs;
> >> +     int num_waitchks = args->num_waitchks;
> >> +     struct tegra_drm_cmdbuf __user *cmdbufs =
> >> +             (void * __user)(uintptr_t)args->cmdbufs;
> >> +     struct tegra_drm_reloc __user *relocs =
> >> +             (void * __user)(uintptr_t)args->relocs;
> >> +     struct tegra_drm_waitchk __user *waitchks =
> >> +             (void * __user)(uintptr_t)args->waitchks;
> > 
> > No need for all the uintptr_t casts.
> 
> Will try to remove - but I do remember getting compiler warnings without
> them.

I think you shouldn't even have to cast to void * first. Just cast to
the target type directly. I don't see why the compiler should complain.

> (...)
> > Most of this looks very generic. Can't it be split out into separate
> > functions and reused in other (gr3d) modules?
> 
> That's actually how most of this is downstream. I thought to make
> everything really simple and make it all 2D specific in the first patch
> set, and split into generic when we add support for another device.

Okay, that's fine then.

> >> +static int gr2d_is_addr_reg(struct platform_device *dev, u32 class, u32 reg)
> >> +{
> >> +     int ret;
> >> +
> >> +     if (class == NV_HOST1X_CLASS_ID)
> >> +             ret = reg == 0x2b;
> >> +     else
> >> +             switch (reg) {
> >> +             case 0x1a:
> >> +             case 0x1b:
> >> +             case 0x26:
> >> +             case 0x2b:
> >> +             case 0x2c:
> >> +             case 0x2d:
> >> +             case 0x31:
> >> +             case 0x32:
> >> +             case 0x48:
> >> +             case 0x49:
> >> +             case 0x4a:
> >> +             case 0x4b:
> >> +             case 0x4c:
> >> +                     ret = 1;
> >> +                     break;
> >> +             default:
> >> +                     ret = 0;
> >> +                     break;
> >> +             }
> >> +
> >> +     return ret;
> >> +}
> > 
> > I should probably bite the bullet and read through the (still) huge
> > patch 3 to understand exactly why this is needed.
> 
> That's the security firewall. It walks through each submit, and ensures
> that each register write that writes an address, goes through the host1x
> reloc mechanism. This way user space cannot ask 2D to write to arbitrary
> memory locations.

I see. Can this be made more generic? Perhaps adding a table of valid
registers to the device and use a generic function to iterate over that
instead of having to provide the same function for each client.

> >> +static int __exit gr2d_remove(struct platform_device *dev)
> >> +{
> >> +     struct host1x *host1x =
> >> +             host1x_get_drm_data(to_platform_device(dev->dev.parent));
> >> +     struct gr2d *gr2d = platform_get_drvdata(dev);
> >> +     int err;
> >> +
> >> +     err = host1x_unregister_client(host1x, &gr2d->client);
> >> +     if (err < 0) {
> >> +             dev_err(&dev->dev, "failed to unregister host1x client: %d\n",
> >> +                     err);
> >> +             return err;
> >> +     }
> >> +
> >> +     host1x_syncpt_free(gr2d->syncpt);
> >> +     return 0;
> >> +}
> > 
> > Isn't this missing a host1x_channel_put() or host1x_free_channel()?
> 
> All references should be handled in gr2d_open_channel() and
> gr2d_close_channel(). I think we'd need to ensure all contexts are
> closed at this point.

Yes, that'd work as well. Actually I would assume that all contexts
associated with a given file should be freed when the file is closed.
That way all of this should work pretty much automatically.

> >> +struct tegra_drm_syncpt_wait_args {
> >> +     __u32 id;
> >> +     __u32 thresh;
> >> +     __s32 timeout;
> >> +     __u32 value;
> >> +};
> >> +
> >> +#define DRM_TEGRA_NO_TIMEOUT (-1)
> > 
> > Is this the only reason why timeout is signed? If so maybe a better
> > choice would be __u32 and DRM_TEGRA_NO_TIMEOUT 0xffffffff.
> 
> I believe it is so. In fact we'd need to rename it to something like
> INFINITE_TIMEOUT, because we also have a case of timeout=0, which
> returns immediately, i.e. doesn't have a timeout either.

For timeout == 0 I don't think we need a symbolic name. It is pretty
common for 0 to mean no timeout. But yes, DRM_TEGRA_INFINITE_TIMEOUT
should be okay.

> >> +struct tegra_drm_syncpt_incr {
> >> +     __u32 syncpt_id;
> >> +     __u32 syncpt_incrs;
> >> +};
> > 
> > Maybe the fields would be better named id and incrs. Though I also
> > notice that incrs is never used. I guess that's supposed to be used in
> > the future to allow increments by more than a single value. If so,
> > perhaps value would be a better name.
> 
> It's actually used in the dreaded patch 3, as part of tegra_drm_submit_args.

Okay. The superfluous syncpt_ prefixes should still go away.

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists