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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

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

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel