Re: [PATCH 1/2] drm: Add NVIDIA Tegra20 support

[Thierry Reding]

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@xxxxxxxxxxxxxxxxx> 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, &regs_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.

> > +static const struct file_operations tegra_drm_fops = {
> > +       .owner = THIS_MODULE,
> > +       .open = drm_open,
> > +       .release = drm_release,
> > +       .unlocked_ioctl = drm_ioctl,
> > +       .mmap = drm_gem_cma_mmap,
> > +       .poll = drm_poll,
> > +       .fasync = drm_fasync,
> > +       .read = drm_read,
> > +#ifdef CONFIG_COMPAT
> > +       .compat_ioctl = tegra_compat_ioctl,
> 
> is tegra_compat_ioctl() supposed to be in this patch?  Anyways, I
> guess you probably just want to use drm_compat_ioctl(), and make sure
> when you start adding any tegra specific ioctls that they are defined
> properly so compat isn't needed when we get to armv8..

Yeah, I overlooked that. I'll fix it.

> > +/* 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.

> > +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.

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

Attachment: pgp00000.pgp
Description: PGP signature