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;elseswitch (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