On Tue, Oct 24, 2017 at 04:03:46PM -0700, Kristian Høgsberg wrote:
> On Tue, Oct 24, 2017 at 9:39 AM, Noralf Trønnes <noralf@xxxxxxxxxxx> wrote:
> >
> > Den 23.10.2017 23.32, skrev Kristian Høgsberg:
> >>
> >> On Mon, Oct 23, 2017 at 9:47 AM, Noralf Trønnes <noralf@xxxxxxxxxxx>
> >> wrote:
> >>>
> >>> Add debugfs file that dumps info about the framebuffers and its planes.
> >>> Also dump info about any connected gem object(s).
> >>>
> >>> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> >>> ---
> >>> drivers/gpu/drm/drm_debugfs.c | 6 +++++
> >>> drivers/gpu/drm/drm_framebuffer.c | 51
> >>> +++++++++++++++++++++++++++++++++++++++
> >>> drivers/gpu/drm/drm_gem.c | 11 +++++++++
> >>> drivers/gpu/drm/drm_internal.h | 4 +++
> >>> 4 files changed, 72 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..ebdfe2b5689f 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,52 @@ int drm_framebuffer_plane_height(int height,
> >>> return fb_plane_height(height, fb->format, plane);
> >>> }
> >>> EXPORT_SYMBOL(drm_framebuffer_plane_height);
> >>> +
> >>> +#ifdef CONFIG_DEBUG_FS
> >>> +static void drm_framebuffer_print(struct drm_framebuffer *fb,
> >>> + struct drm_printer *p)
> >>> +{
> >>> + unsigned int i;
> >>> +
> >>> + drm_printf(p, "[FB:%d] %dx%d@%4.4s refcount=%d\n", fb->base.id,
> >>> + fb->width, fb->height, (char *)&fb->format->format,
> >>> + drm_framebuffer_read_refcount(fb));
> >>
> >> Could you print the format modifier as well?
> >>
> >>> +
> >>> + for (i = 0; i < fb->format->num_planes; i++) {
> >>> + drm_printf(p, "\t%u: offset=%d pitch=%d",
> >>> + i, fb->offsets[i], fb->pitches[i]);
> >>> + if (fb->obj[i]) {
> >>> + drm_printf(p, " GEM: ");
> >>> + drm_gem_print(fb->obj[i], p);
> >>> + } else {
> >>> + drm_printf(p, "\n");
> >>> + }
> >>> + }
> >>> +}
> >>> +
> >>> +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_framebuffer_print(fb, &p);
> >>> + mutex_unlock(&dev->mode_config.fb_lock);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static const struct drm_info_list drm_framebuffer_debugfs_list[] = {
> >>> + { "framebuffer", drm_framebuffer_info, 0 },
> >>
> >> This is quite useful, thanks. Ultimately, it would be very nice to
> >> have something like i915_display_info, but in a generic way. There's
> >> not much there that's actually i915 specific:
> >
> >
> > How about mimicking drm_state_info(), debugfs file 'state', and create
> > an 'info' file?
> >
> > struct drm_crtc_funcs {
> > /**
> > * @print_info:
> > *
> > * If driver subclasses struct &drm_crtc, it can implement this
> > * optional hook for printing additional driver specific info.
> > * See drm_crtc_print_info().
> > */
> > void (*print_info)(struct drm_printer *p, struct drm_crtc *crtc);
> > }
> >
> > void drm_crtc_print_info(struct drm_printer *p, struct drm_crtc *crtc)
> > {
> > drm_modeset_lock(&crtc->mutex, NULL);
> >
> > drm_printf(p, "[CRTC:%d:%s] ...");
> >
> > if (crtc->funcs->print_info)
> > crtc->funcs->print_info(p, crtc);
> >
> > drm_modeset_unlock(&crtc->mutex);
> > }
> >
> > static int drm_debugfs_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_plane *plane;
> > struct drm_crtc *crtc;
> > struct drm_encoder *encoder;
> > struct drm_connector *connector;
> > struct drm_connector_list_iter conn_iter;
> > struct drm_framebuffer *fb;
> >
> > mutex_lock(&dev->struct_mutex);
> >
> > drm_for_each_plane(plane, dev)
> > drm_plane_print_info(&p, plane);
> >
> > drm_for_each_crtc(crtc, dev)
> > drm_crtc_print_info(&p, crtc);
> >
> > drm_for_each_encoder(encoder, dev)
> > drm_encoder_print_info(&p, encoder);
> >
> > drm_connector_list_iter_begin(dev, &conn_iter);
> > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > drm_for_each_connector_iter(connector, &conn_iter)
> > drm_connector_print_info(&p, connector);
> > drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > drm_connector_list_iter_end(&conn_iter);
> >
> > mutex_lock(&dev->mode_config.fb_lock);
> > drm_for_each_fb(fb, dev)
> > drm_framebuffer_print_info(&p, fb);
> > mutex_unlock(&dev->mode_config.fb_lock);
> >
> > mutex_unlock(&dev->struct_mutex);
> > }
> >
> > static const struct drm_info_list drm_debugfs_list[] = {
> > {"name", drm_name_info, 0},
> > {"clients", drm_clients_info, 0},
> > {"gem_names", drm_gem_name_info, DRIVER_GEM},
> > + {"info", drm_debugfs_info, 0},
> > };
> >
> >
> > Or is it too much info in one single file so it should be split into one
> > file per object: plane, framebuffer, etc. ?
> > Daniel mentioned blob properties, and there are probably more stuff that
> > can be useful for debugging.
>
> Oh oops, my bad, I wasn't aware of 'state'. That goes a long way
> towards what I had in mind with info.
Imo we should just put all the i915 kms printing into the state stuff and
throw our info stuff away. No one got around to that yet unfortunately,
probably needs someone with an interest to make the cross-driver debugging
experience more consistent ... hint, hint :-)
But yeah it's all there, and I suggested to Noralf he integrate with that
to make an even stronger incentive to use the commont state/kms debugfs
stuff.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
