[58/59] drm/todo: Add new debugfs todo
diff mbox series

Message ID 20190614203615.12639-59-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • prime doc polish and ... a few cleanups
Related show

Commit Message

Daniel Vetter June 14, 2019, 8:36 p.m. UTC
Greg is busy already, but maybe he won't do everything ...

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/gpu/todo.rst | 3 +++
 1 file changed, 3 insertions(+)

Comments

Greg Kroah-Hartman June 15, 2019, 6:23 a.m. UTC | #1
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@linuxfoundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/gpu/todo.rst | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Greg Kroah-Hartman June 18, 2019, 3:19 p.m. UTC | #2
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@linuxfoundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@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.

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.

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?

Either way, who can test these changes?  I can't even build the tegra
driver without digging up an arm64 cross-compiler, and can't test it as
I have no hardware at all.

thanks,

greg k-h
Greg Kroah-Hartman June 18, 2019, 3:25 p.m. UTC | #3
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@linuxfoundation.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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.
> 
> 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.
> 
> 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.

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,
+};
+
 #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;
 	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);
+	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, &regs_fops);
 
 	return 0;
-
-free:
-	kfree(sor->debugfs_files);
-	sor->debugfs_files = NULL;
-
-	return err;
 }
 
 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);
 }
 
 static void tegra_sor_connector_reset(struct drm_connector *connector)
Jon Hunter June 18, 2019, 3:37 p.m. UTC | #4
On 18/06/2019 16:19, 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@linuxfoundation.org>
>> Signed-off-by: Daniel Vetter <daniel.vetter@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.
> 
> 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.
> 
> 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?
> 
> Either way, who can test these changes?  I can't even build the tegra
> driver without digging up an arm64 cross-compiler, and can't test it as
> I have no hardware at all.

We can definitely compile and boot test these no problem. In fact
anything that lands in -next we will boot test. However, I can do some
quick sanity if you have something to test.

Thierry may have more specific Tegra DRM tests.

Cheers
Jon
Daniel Vetter June 18, 2019, 5:32 p.m. UTC | #5
On Tue, Jun 18, 2019 at 5:25 PM Greg Kroah-Hartman
<gregkh@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@linuxfoundation.org>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@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.

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.

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

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

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

>         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?

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

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

>  }
>
>  static void tegra_sor_connector_reset(struct drm_connector *connector)

Cheers, Daniel
Greg Kroah-Hartman June 18, 2019, 6:01 p.m. UTC | #6
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@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@linuxfoundation.org>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@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, &regs_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
Daniel Vetter June 18, 2019, 9:46 p.m. UTC | #7
On Tue, Jun 18, 2019 at 08:01:13PM +0200, Greg Kroah-Hartman wrote:
> 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@linuxfoundation.org> wrote:
> > > On Tue, Jun 18, 2019 at 05:19:38PM +0200, Greg Kroah-Hartman wrote:
> > > > 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.

The todo was to have some drm wrappers here for the boilerplate, but after
looking at your version that's not a good idea. So not just making sure
crtcs/connectors have a debugfs directory made for them, but more.

Wrt adding a new directory: debugfs isnt uapi, but there's usually a
massive pile of script relying on them, so it's not nice to shuffle paths
around. Plus the lifetimes match anyway (at least if you don't hotplug
connectors, which tegra doesn't do).
-Daniel
Thierry Reding June 20, 2019, 2:50 p.m. UTC | #8
On Tue, Jun 18, 2019 at 11:46:56PM +0200, Daniel Vetter wrote:
> On Tue, Jun 18, 2019 at 08:01:13PM +0200, Greg Kroah-Hartman wrote:
> > 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@linuxfoundation.org> wrote:
> > > > On Tue, Jun 18, 2019 at 05:19:38PM +0200, Greg Kroah-Hartman wrote:
> > > > > 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.
> 
> The todo was to have some drm wrappers here for the boilerplate, but after
> looking at your version that's not a good idea. So not just making sure
> crtcs/connectors have a debugfs directory made for them, but more.
> 
> Wrt adding a new directory: debugfs isnt uapi, but there's usually a
> massive pile of script relying on them, so it's not nice to shuffle paths
> around. Plus the lifetimes match anyway (at least if you don't hotplug
> connectors, which tegra doesn't do).

So, I think you two already covered everything. From a Tegra perspective
there's not really a need to care about the exact structure of debugfs
because there are only a handful of scripts that use this and they are
not exactly widely distributed. At the same time there's really no need
to add another level of directories, since the connector really is the
SOR, so an sor directory in the connector's directory is just redundant.
Cleaning up and lifetime management aren't issues, really, so there are
no good arguments for adding it, in my opinion.

Historically, the sor.c driver is interesting because it used to be just
plain debugfs calls. Only when I added a second debugfs file I decided
to go with the built-in DRM infrastructure for this and then later went
and converted the first file to it as well, for consistency. I do
remember though that I was very unhappy about the fact that I had to do
all this kmemdup()'ing just to make debugfs files per-instance (the DRM
infrastructure doesn't allow that by default). In retrospect that was a
poor decision and I should've just stuck with debugfs and perhaps just
spin a couple of helpers around that instead.

So I'm happy to see this effort.

Thierry
Thierry Reding June 20, 2019, 2:57 p.m. UTC | #9
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@linuxfoundation.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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.

Actually the reason why the memory allocation is done is because there
can be multiple instances of the display controller. In fact, there's
always at least two (and up to four in later Tegra generations). The DRM
debugfs infrastructure, however, doesn't automatically duplicate the
data for each drm_debugfs_create_files() call and at the same time it
does not allow you to specify driver-private data other than by
embedding it in the drm_info_list structure. So rather than manually
create the drm_info_list for each display controller instance, the code
creates a template that is then duplicated and for which the driver-
private is then set. That way multiple invocations end up with different
data.

This is because of the extra indirection that the DRM debugfs
infrastructure introduces. It's in fact much easier to do this with just
plain debugfs function calls. The only downside is the boilerplate
required to make that happen.

Thierry
Thierry Reding June 20, 2019, 3:11 p.m. UTC | #10
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@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@linuxfoundation.org>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@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.

For the connector drivers we already sit on top of the per-connector
debugfs directories. I think the only reason why we don't do that for
the display controller is because drm_crtc didn't have built-in debugfs
support like the connectors have. It looks like that's no longer true,
though it's been there for a while. I think it'd be good to just move
those over as well.

As for passing struct drm_minor, I think that's mostly unnecessary. As
far as I can tell, we only use drm_minor to get at drm_device, which in
turn we only use to check some features flags, and drm_minor itself is
only used to track the list of files that are being added so that they
can later be removed again. Given that we can just tear down everything
debugfs recursively, I don't think we need any of that.

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

Yeah. That too.

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

Exactly my thoughts. It'd be nice to have some sort of macro to help
bring the boilerplate down a bit.

Thierry

> > 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.
> 
> > +
> >  #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.
> 
> >         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?
> 
> > +       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, &regs_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.

I fully agree with this and all of your points above.

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

And this. Greg, let me know if you need a hand with the patches or if
you want any of these to be build/runtime tested.

Thierry
Thierry Reding June 20, 2019, 3:16 p.m. UTC | #11
On Tue, Jun 18, 2019 at 04:37:16PM +0100, Jon Hunter wrote:
> 
> On 18/06/2019 16:19, 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@linuxfoundation.org>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@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.
> > 
> > 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.
> > 
> > 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?
> > 
> > Either way, who can test these changes?  I can't even build the tegra
> > driver without digging up an arm64 cross-compiler, and can't test it as
> > I have no hardware at all.
> 
> We can definitely compile and boot test these no problem. In fact
> anything that lands in -next we will boot test. However, I can do some
> quick sanity if you have something to test.
> 
> Thierry may have more specific Tegra DRM tests.

We don't have any automated tests for this yet, unfortunately. Let me
work on something. In the meantime I can manually test any of the
patches that Greg sends out. These should be fairly trivial to test.
It's difficult to check for success/failure on something like the
register dump or the CRC, but I think for now we don't really need much
more than just validating that things don't crash when we read one of
these debugfs files.

Thierry

Patch
diff mbox series

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.
+
 Contact: Daniel Vetter
 
 KMS cleanups