On Fri, Nov 9, 2012 at 10:00 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
On Fri, Nov 09, 2012 at 09:18:58AM -0600, Rob Clark wrote:
On Fri, Nov 9, 2012 at 7:59 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
[...]
+static int regs_open(struct inode *inode, struct file *file) +{
return single_open(file, regs_show, inode->i_private);+}
+static const struct file_operations regs_fops = {
.open = regs_open,.read = seq_read,.llseek = seq_lseek,.release = single_release,+};
+static int tegra_dc_debugfs_init(struct tegra_dc *dc, struct dentry *parent) +{
char *name;name = kasprintf(GFP_KERNEL, "dc.%d", dc->pipe);dc->debugfs = debugfs_create_dir(name, parent);kfree(name);debugfs_create_file("regs", 0400, dc->debugfs, dc, ®s_fops);note that drm already has it's own debugfs helpers, see drm_debugfs_create_files() and drm_debugfs_remove_files()
And also see debugfs_init/debugfs_cleanup in 'struct drm_driver'.
You probably want to be using that rather than rolling your own. You can have a look at omapdrm for a quite simple example, or i915 for a more complex example.
I actually tried to make use of those functions, but unfortunately it's not possible to hook it up properly. The reason is that I need to pass in some reference to the tegra_dc structure in order to read the registers, but the DRM debugfs support doesn't allow to do that. Or maybe it can. There's the void *arg argument that I could possibly use. Then again it won't allow the creation of separate directories for each of the display controllers. Or maybe I'm missing something.
yeah, no separate directories.. but you could use the void *arg. It is a bit awkward for dealing with multiple subdevices, we have the same issue w/ omapdrm where dmm is a separate subdevice (and dsi/dpi/hdmi/etc too shortly, as we merge omapdss and omapdrm).
But I guess better handling in drm for subdevices would help a lot of the SoC platforms. Maybe something that I'll give some more thought later after the atomic pageflip/modeset stuff is sorted.
+/* synchronization points */ +#define SYNCPT_VBLANK0 26 +#define SYNCPT_VBLANK1 27
maybe these should be in dc.h? Seems like these are related to the dc hw block?
Yes, they could go into dc.h. This is one of the things that is likely to change at some point as more of the host1x support is added, which is where those syncpts are actually used.
hmm, are these values defined by the hw? They look like register offsets into the DC block?
+int host1x_unregister_client(struct host1x *host1x,
struct host1x_client *client)+{
struct host1x_drm_client *drm, *tmp;int err;list_for_each_entry_safe(drm, tmp, &host1x->drm_active, list) {if (drm->client == client) {err = host1x_drm_exit(host1x);if (err < 0) {dev_err(host1x->dev, "host1x_drm_exit(): %d\n",err);return err;}host1x_remove_drm_client(host1x, drm);break;}}mutex_lock(&host1x->clients_lock);list_del_init(&client->list);mutex_unlock(&host1x->clients_lock);return 0;+}
btw, if I understand the register/unregister client stuff, I think there can be some potential issues. If I understand correctly, you will create the crtc's in register. But unregister doesn't destroy them, drm_mode_config_cleanup() when the container drm device is unloaded does. So if there is any possibility to unregister a client without tearing down everything, you might get into some problems here.
Also, you might be breaking some assumptions about when the crtc's are created.. at least if there is some possibility for userspace to sneak in and do a getresources ioctl between the first and second client. That might be easy enough to solve by adding some event for userspace to get notified that it should getresources again. The unregister is what I worry about more.
In general drm isn't set up to well for drivers that are a collection of sub-devices. It is probably worth trying to improve this, although I am still a bit skittish about the edge cases, like what happens when you unload a sub-device mid-modeset. The issue comes up again for common panel/display framework.
But for now you might just want to do something to ensure that all the sub-devices are loaded/unloaded together as a whole.
The way that the above is supposed to work is that the DRM relevant host1x clients are kept in a separate list and only if all of them have successfully been probed is the DRM portion initialized. Upon unregistration, as soon as the first of these clients is unregistered, all of the DRM stuff is torn down.
ahh, ok, I guess if DRM is torn down on first unregister, then you shouldn't be hitting issues. I wasn't sure if the intention was to be able to load/unload clients independently (such as building them as separate modules eventually)
BR, -R
I don't believe there's an issue here. It's precisely what I've been testing for a while, always making sure that when built as a module it can properly be unloaded.
That said it probably won't matter very much since on Tegra all drivers are usually builtin, so none of this may even be used in the end.
Thanks for the quick review.
Thierry