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
Attachment:
pgpe16i4Y5TML.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
