drm connectors, tegra, and the web they weave (was Re: [PATCH 58/59] drm/todo: Add new debugfs todo)
Greg Kroah-Hartman
gregkh at linuxfoundation.org
Tue Jun 18 18:01:13 UTC 2019
On Tue, Jun 18, 2019 at 07:32:20PM +0200, Daniel Vetter wrote:
> On Tue, Jun 18, 2019 at 5:25 PM Greg Kroah-Hartman
> <gregkh at linuxfoundation.org> wrote:
> > On Tue, Jun 18, 2019 at 05:19:38PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Jun 14, 2019 at 10:36:14PM +0200, Daniel Vetter wrote:
> > > > Greg is busy already, but maybe he won't do everything ...
> > > >
> > > > Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > > ---
> > > > Documentation/gpu/todo.rst | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > > > index 9717540ee28f..026e55c517e1 100644
> > > > --- a/Documentation/gpu/todo.rst
> > > > +++ b/Documentation/gpu/todo.rst
> > > > @@ -375,6 +375,9 @@ There's a bunch of issues with it:
> > > > this (together with the drm_minor->drm_device move) would allow us to remove
> > > > debugfs_init.
> > > >
> > > > +- Drop the return code and error checking from all debugfs functions. Greg KH is
> > > > + working on this already.
> > >
> > >
> > > Part of this work was to try to delete drm_debugfs_remove_files().
> > >
> > > There are only 4 files that currently still call this function:
> > > drivers/gpu/drm/tegra/dc.c
> > > drivers/gpu/drm/tegra/dsi.c
> > > drivers/gpu/drm/tegra/hdmi.c
> > > drivers/gpu/drm/tegra/sor.c
> > >
> > > For dc.c, the driver wants to add debugfs files to the struct drm_crtc
> > > debugfs directory. Which is fine, but it has to do some special memory
> > > allocation to get the debugfs callback to point not to the struct
> > > drm_minor pointer, but rather the drm_crtc structure.
>
> There's already a todo to switch the drm_minor debugfs stuff over to
> drm_device. drm_minor is essentially different uapi flavours (/dev/
> minor nodes, hence the name) sitting on top of the same drm_device.
> Last time I checked all the debugfs files want the drm_device, not the
> minor. I think we even discussed to only register the debugfs files
> for the first minor, and create the other ones as symlinks to the
> first one. But haven't yet gotten around to typing that.
>
> drm_crtc/connector are parts of drm_device with modesetting support,
> so the drm_minor is even worse choice really.
Heh, ok, so the existing code is working around that choice right now,
but that wasn't a good choice, so I'll ignore it :)
> Not exactly sure why we went with this, but probably dates back to the
> *bsd compat layer and a lot of these files hanging out in procfs too
> (we've fixed those mistakes a few years ago, yay!).
>
> > > So, to remove this call, I need to remove this special memory allocation
> > > and to do that, I need to somehow be able to cast from drm_minor back to
> > > the drm_crtc structure being used in this driver. And I can't figure
> > > how they are related at all.
> > >
> > > Any pointers here (pun intended) would be appreciated.
> > >
> > > For the other 3 files, the situation is much the same, but I need to get
> > > from a 'struct drm_minor' pointer to a 'struct drm_connector' pointer.
>
> Ditch the drm_minor, there's no no way to get from that to something
> like drm_connector/crtc, since it's a n:m relationship.
Ok, will do.
>
> > > I could just "open code" a bunch of calls to debugfs_create_file() for
> > > these drivers, which would solve this issue, but in a more "non-drm"
> > > way. Is it worth to just do that instead of overthinking the whole
> > > thing and trying to squish it into the drm "model" of drm debugfs calls?
> >
> > An example of "open coding" this is the patch below for the sor.c
> > driver.
>
> I think open-coding is the way to go here. One of the todos is to
> extend debugfs support for crtc/connectors, but looking at the
> open-coded version we really don't need a drm-flavoured midlayer here.
There already is debugfs support in the code for crtc/connectors, these
files are "hanging" off of those locations already. I'll keep that, but
indent it one more directory so that there's no namespace collisions.
> > Totally untested, not even built, but you should get the idea here.
> >
> > thanks,
> >
> > greg k-h
> >
> > ---------------
> >
> > diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
> > index 5be5a0817dfe..3216221c77c4 100644
> > --- a/drivers/gpu/drm/tegra/sor.c
> > +++ b/drivers/gpu/drm/tegra/sor.c
> > @@ -414,7 +414,8 @@ struct tegra_sor {
> >
> > struct drm_dp_aux *aux;
> >
> > - struct drm_info_list *debugfs_files;
> > + struct dentry *debugfs_root;
> > + struct drm_device *drm;
> >
> > const struct tegra_sor_ops *ops;
> > enum tegra_io_pad pad;
> > @@ -1262,10 +1263,9 @@ static int tegra_sor_crc_wait(struct tegra_sor *sor, unsigned long timeout)
> >
> > static int tegra_sor_show_crc(struct seq_file *s, void *data)
> > {
> > - struct drm_info_node *node = s->private;
> > - struct tegra_sor *sor = node->info_ent->data;
> > + struct tegra_sor *sor = s->private;
> > struct drm_crtc *crtc = sor->output.encoder.crtc;
> > - struct drm_device *drm = node->minor->dev;
> > + struct drm_device *drm = sor->drm;
> > int err = 0;
> > u32 value;
> >
> > @@ -1302,6 +1302,20 @@ static int tegra_sor_show_crc(struct seq_file *s, void *data)
> > return err;
> > }
> >
> > +static int crc_open(struct inode *inode, struct file *file)
> > +{
> > + struct tegra_sor *sor = inode->i_private;
> > + return single_open(file, tegra_sor_show_crc, sor);
> > +}
> > +
> > +static const struct file_operations crc_fops = {
> > + .owner = THIS_MODULE,
> > + .open = crc_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
>
> Hm, is there not a macro to create such simple files with read/write
> ops? At least for sysfs this is a bit less boilerplate iirc.
For "simple" things like single variables, yes, there is.
For more "free-form" text, where you want to use a seq file interface,
this seems to be the "simplest" boiler-plate to create. Actually should
be pretty simple to create a macro for this, as it's pretty trivial (the
drm core already wraps this on its own, so it can be done...)
I'll do that too.
> > #define DEBUGFS_REG32(_name) { .name = #_name, .offset = _name }
> >
> > static const struct debugfs_reg32 tegra_sor_regs[] = {
> > @@ -1424,10 +1438,9 @@ static const struct debugfs_reg32 tegra_sor_regs[] = {
> >
> > static int tegra_sor_show_regs(struct seq_file *s, void *data)
> > {
> > - struct drm_info_node *node = s->private;
> > - struct tegra_sor *sor = node->info_ent->data;
> > + struct tegra_sor *sor = s->private;
> > struct drm_crtc *crtc = sor->output.encoder.crtc;
> > - struct drm_device *drm = node->minor->dev;
> > + struct drm_device *drm = sor->drm;
>
> sor->output.connector.dev should give you this already. And I think
> getting at the drm_device is the only reason we needed the drm_minor
> here at all.
Ah, good, I missed that, should make this code simpler then, thanks!
>
> > unsigned int i;
> > int err = 0;
> >
> > @@ -1450,51 +1463,44 @@ static int tegra_sor_show_regs(struct seq_file *s, void *data)
> > return err;
> > }
> >
> > -static const struct drm_info_list debugfs_files[] = {
> > - { "crc", tegra_sor_show_crc, 0, NULL },
> > - { "regs", tegra_sor_show_regs, 0, NULL },
> > +static int regs_open(struct inode *inode, struct file *file)
> > +{
> > + struct tegra_sor *sor = inode->i_private;
> > + return single_open(file, tegra_sor_show_regs, sor);
> > +}
> > +
> > +static const struct file_operations crc_fops = {
> > + .owner = THIS_MODULE,
> > + .open = crc_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > };
> >
> > static int tegra_sor_late_register(struct drm_connector *connector)
> > {
> > - struct tegra_output *output = connector_to_output(connector);
> > - unsigned int i, count = ARRAY_SIZE(debugfs_files);
> > struct drm_minor *minor = connector->dev->primary;
> > - struct dentry *root = connector->debugfs_entry;
> > + struct tegra_output *output = connector_to_output(connector);
> > struct tegra_sor *sor = to_sor(output);
> > - int err;
> > + struct dentry *root;
> >
> > - sor->debugfs_files = kmemdup(debugfs_files, sizeof(debugfs_files),
> > - GFP_KERNEL);
> > - if (!sor->debugfs_files)
> > - return -ENOMEM;
> > + sor->drm = minor->dev;
> >
> > - for (i = 0; i < count; i++)
> > - sor->debugfs_files[i].data = sor;
> > + root = debugfs_create_dir("sor", connector->debugfs_entry);
>
> Hm I think the old files got created right in the
> drm_connector->debugfs_entry directory?
They did. I was trying to be nice and keep things in their own
directory so I could clean it up. But I guess we want the drm core to
be cleaning things up, I forgot about drm_debugfs_remove_files() being
the main goal to get rid of here :)
> > + sor->debugfs_root = root;
> >
> > - err = drm_debugfs_create_files(sor->debugfs_files, count, root, minor);
> > - if (err < 0)
> > - goto free;
> > + debugfs_create_file("crc", S_IFREG | S_IRUGO, root, sor, &crc_fops);
> > + debugfs_create_file("regs", S_IFREG | S_IRUGO, root, sor, ®s_fops);
> >
> > return 0;
> > -
> > -free:
> > - kfree(sor->debugfs_files);
> > - sor->debugfs_files = NULL;
> > -
> > - return err;
> > }
>
> I think if you can create a debugfs-simple-file macro, this here would
> win hands-down from a boilerplate pov. I like.
Ok, will do.
> > static void tegra_sor_early_unregister(struct drm_connector *connector)
> > {
> > struct tegra_output *output = connector_to_output(connector);
> > - unsigned int count = ARRAY_SIZE(debugfs_files);
> > struct tegra_sor *sor = to_sor(output);
> >
> > - drm_debugfs_remove_files(sor->debugfs_files, count,
> > - connector->dev->primary);
> > - kfree(sor->debugfs_files);
> > - sor->debugfs_files = NULL;
> > + debugfs_remove_recursive(sor->debugfs_root);
>
> Not needed, we tear down everything as part of drm_dev_unregister
> anyway. So you can ditch this.
Wonderful, will do.
thanks for the review.
greg k-h
More information about the dri-devel
mailing list