Re: [PATCH v4 04/11] drm/framebuffer: Add framebuffer debugfs file

Hi Laurent,

Thank you for the review.

Den 02.11.2017 05.43, skrev Laurent Pinchart:

Hi Noralf,

Thank you for the patch.

On Monday, 30 October 2017 18:29:38 EET Noralf Trønnes wrote:

...

Add debugfs file that dumps info about the framebuffers and its planes. Also dump info about any connected gem object(s).

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Noralf Trønnes noralf@tronnes.org

---

drivers/gpu/drm/drm_debugfs.c | 6 ++++ drivers/gpu/drm/drm_framebuffer.c | 59 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_gem.c | 17 +++++++++++ drivers/gpu/drm/drm_internal.h | 7 +++++ include/drm/drm_drv.h | 11 ++++++++ 5 files changed, 100 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index c1807d5754b2..550f29de6c1f 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -158,6 +158,12 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, } }

• ret = drm_framebuffer_debugfs_init(minor);
• if (ret) {

• DRM_ERROR("Failed to create framebuffer debugfs file\n");
  

• return ret;
  

• }
• if (dev->driver->debugfs_init) { ret = dev->driver->debugfs_init(minor); if (ret) {

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index af279844d7ce..4f7873a1b922 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -25,7 +25,9 @@ #include <drm/drm_auth.h> #include <drm/drm_framebuffer.h> #include <drm/drm_atomic.h> +#include <drm/drm_print.h>

+#include "drm_internal.h" #include "drm_crtc_internal.h"

/** @@ -955,3 +957,60 @@ int drm_framebuffer_plane_height(int height, return fb_plane_height(height, fb->format, plane); } EXPORT_SYMBOL(drm_framebuffer_plane_height);

+void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,

• 		const struct drm_framebuffer *fb)
  

+{

• struct drm_format_name_buf format_name;
• int i;

i is never negative, you can make it an unsigned int.

Yeah, I did that in a previous version, but Ville wanted it to be int. Personally I don't really care as long as I remember to use int when I have a loop that counts down to zero, which has bitten me before.

So based on my experience maybe I should always use plain int in loop indices, too avoid that rare count down case where I continue to use unsigned int for the loop index, me being a limited human being :-)

Noralf.

...

• drm_printf_indent(p, indent, "refcount=%d\n",

• 	  drm_framebuffer_read_refcount(fb));
  

drm_framebuffer_read_refcount() returns an unsigned int, the printk format should be %u.

...

• drm_printf_indent(p, indent, "format=%s\n",

• 	  drm_get_format_name(fb->format->format, &format_name));
  

• drm_printf_indent(p, indent, "modifier=0x%llx\n", fb->modifier);
• drm_printf_indent(p, indent, "size=%dx%d\n", fb->width, fb->height);

Ditto here, %ux%u.

...

• drm_printf_indent(p, indent, "layers:\n");
• for (i = 0; i < fb->format->num_planes; i++) {

• drm_printf_indent(p, indent + 1, "size[%d]=%dx%d\n", i,
  

• 		  drm_framebuffer_plane_width(fb->width, fb, i),
  

• 		  drm_framebuffer_plane_height(fb->height, fb, i));
  

• drm_printf_indent(p, indent + 1, "pitch[%d]=%u\n", i, fb->pitches[i]);
  

• drm_printf_indent(p, indent + 1, "offset[%d]=%u\n", i, fb->offsets[i]);
  

• drm_printf_indent(p, indent + 1, "obj[%d]:%s\n", i,
  

• 		  fb->obj[i] ? "" : "(null)");
  

And in various places here too.

...

• if (fb->obj[i])
  

• 	drm_gem_print_info(p, indent + 2, fb->obj[i]);
  

• }

+}

+#ifdef CONFIG_DEBUG_FS +static int drm_framebuffer_info(struct seq_file *m, void *data) +{

• struct drm_info_node *node = m->private;
• struct drm_device *dev = node->minor->dev;
• struct drm_printer p = drm_seq_file_printer(m);
• struct drm_framebuffer *fb;
• mutex_lock(&dev->mode_config.fb_lock);
• drm_for_each_fb(fb, dev) {

• drm_printf(&p, "framebuffer[%d]:\n", fb->base.id);
  

fb->base.id is an unsigned int, the printk format should be %u.

...

• drm_framebuffer_print_info(&p, 1, fb);
  

• }
• mutex_unlock(&dev->mode_config.fb_lock);
• return 0;

+}

+static const struct drm_info_list drm_framebuffer_debugfs_list[] = {

• { "framebuffer", drm_framebuffer_info, 0 },

+};

+int drm_framebuffer_debugfs_init(struct drm_minor *minor) +{

• return drm_debugfs_create_files(drm_framebuffer_debugfs_list,

• 		ARRAY_SIZE(drm_framebuffer_debugfs_list),
  

• 		minor->debugfs_root, minor);
  

+} +#endif diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 4c84b23d37cc..152bd6210dde 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -40,6 +40,7 @@ #include <drm/drmP.h> #include <drm/drm_vma_manager.h> #include <drm/drm_gem.h> +#include <drm/drm_print.h> #include "drm_internal.h"

/** @file drm_gem.c @@ -1040,3 +1041,19 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) return ret; } EXPORT_SYMBOL(drm_gem_mmap);

+void drm_gem_print_info(struct drm_printer *p, unsigned int indent,

• 	const struct drm_gem_object *obj)
  

+{

• drm_printf_indent(p, indent, "name=%d\n", obj->name);
• drm_printf_indent(p, indent, "refcount=%d\n",

• 	  kref_read(&obj->refcount));
  

kref_read() returns an unsigned int, the printk format should be %u.

...

• drm_printf_indent(p, indent, "start=%08lx\n",

• 	  drm_vma_node_start(&obj->vma_node));
  

• drm_printf_indent(p, indent, "size=%zu\n", obj->size);
• drm_printf_indent(p, indent, "imported=%s\n",

• 	  obj->import_attach ? "yes" : "no");
  

• if (obj->dev->driver->gem_print_info)

• obj->dev->driver->gem_print_info(p, indent, obj);
  

+} diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index fbc3f308fa19..430ce3fe4f3b 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -106,6 +106,8 @@ int drm_gem_open_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); void drm_gem_open(struct drm_device *dev, struct drm_file *file_private); void drm_gem_release(struct drm_device *dev, struct drm_file *file_private); +void drm_gem_print_info(struct drm_printer *p, unsigned int indent,

• 	const struct drm_gem_object *obj);
  
  

  /* drm_debugfs.c drm_debugfs_crc.c */ #if defined(CONFIG_DEBUG_FS)

@@ -173,3 +175,8 @@ int drm_syncobj_reset_ioctl(struct drm_device *dev, void *data, struct drm_file *file_private); int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, struct drm_file *file_private);

+/* drm_framebuffer.c */ +void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,

• 		const struct drm_framebuffer *fb);
  

+int drm_framebuffer_debugfs_init(struct drm_minor *minor); diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 0e90ef24214b..434de999c3ba 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -39,6 +39,7 @@ struct drm_minor; struct dma_buf_attachment; struct drm_display_mode; struct drm_mode_create_dumb; +struct drm_printer;

/* driver capabilities and requirements mask */ #define DRIVER_USE_AGP 0x1 @@ -428,6 +429,16 @@ struct drm_driver { */ void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);

• /**

• * @gem_print_info:
  

• *
  

• * If driver subclasses struct &drm_gem_object, it can implement this
  

• * optional hook for printing additional driver specific info.
  

• * See drm_gem_print_info().
  

Should you also document the meaning of the indent argument ?

...

• */
  

• void (*gem_print_info)(struct drm_printer *p, unsigned int indent,

• 	       const struct drm_gem_object *obj);
  

• /**
  • @gem_create_object: constructor for gem objects

With all this fixed,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com