| Message ID | 1476489353-16261-5-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 14, 2016 at 07:55:51PM -0400, Rob Clark wrote: > Signed-off-by: Rob Clark <robdclark@gmail.com> General comment: Some drivers (and I think this would be useful in general) also dump state into debugfs with seq_file. And we have a bunch of other places where unfortunately we duplicate dumping functions between dmesg and seq_file (e.g. drm_mm.c). Fancy idea: Could we create a seq_file that dumps to dmesg and so reuse one for all? Before we have to add atomic_print_state and atomic_dump_state for dmesg vs. seq_file variations ... One kerneldoc nit below. > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 7ffaa35..642d5da 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -133,6 +133,7 @@ struct dma_buf_attachment; > #define DRM_UT_PRIME 0x08 > #define DRM_UT_ATOMIC 0x10 > #define DRM_UT_VBL 0x20 > +#define DRM_UT_STATE 0x40 > > extern __printf(2, 3) > void drm_ut_debug_printk(const char *function_name, > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 856fdf8..d74d47a 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -44,6 +44,7 @@ struct drm_framebuffer; > struct drm_object_properties; > struct drm_file; > struct drm_clip_rect; > +struct drm_print; > struct device_node; > struct fence; > struct edid; > @@ -707,6 +708,18 @@ struct drm_crtc_funcs { > const struct drm_crtc_state *state, > struct drm_property *property, > uint64_t *val); > + > + /** > + * @atomic_print_state: > + * > + * If driver subclasses struct &drm_crtc_state, it should implement > + * this optional hook for printing state. s/printing state/printing additional, driver-private state/ in all of these for clarification. Same below. -Daniel > + * > + * Do not call this directly, use drm_atomic_crtc_print_state() > + * instead. > + */ > + void (*atomic_print_state)(struct drm_print *p, > + const struct drm_crtc_state *state); > }; > > /** > @@ -1132,6 +1145,18 @@ struct drm_connector_funcs { > const struct drm_connector_state *state, > struct drm_property *property, > uint64_t *val); > + > + /** > + * @atomic_print_state: > + * > + * If driver subclasses struct &drm_connector_state, it should > + * implement this optional hook for printing state. > + * > + * Do not call this directly, use drm_atomic_connector_print_state() > + * instead. > + */ > + void (*atomic_print_state)(struct drm_print *p, > + const struct drm_connector_state *state); > }; > > /** > @@ -1623,6 +1648,18 @@ struct drm_plane_funcs { > const struct drm_plane_state *state, > struct drm_property *property, > uint64_t *val); > + > + /** > + * @atomic_print_state: > + * > + * If driver subclasses struct &drm_plane_state, it should implement > + * this optional hook for printing state. > + * > + * Do not call this directly, use drm_atomic_plane_print_state() > + * instead. > + */ > + void (*atomic_print_state)(struct drm_print *p, > + const struct drm_plane_state *state); > }; > > enum drm_plane_type { > -- > 2.7.4 >
On Fri, Oct 14, 2016 at 7:55 PM, Rob Clark <robdclark@gmail.com> wrote: Could you add a commit message detailing what and when the state is dumped? > Signed-off-by: Rob Clark <robdclark@gmail.com> > --- > drivers/gpu/drm/drm_atomic.c | 93 ++++++++++++++++++++++++++++++++++++++++++++ > include/drm/drmP.h | 1 + > include/drm/drm_crtc.h | 37 ++++++++++++++++++ > 3 files changed, 131 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7ebf375..3cc3cb5 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -30,6 +30,7 @@ > #include <drm/drm_atomic.h> > #include <drm/drm_mode.h> > #include <drm/drm_plane_helper.h> > +#include <drm/drm_print.h> > #include <linux/sync_file.h> > > #include "drm_crtc_internal.h" > @@ -608,6 +609,28 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, > return 0; > } > > +static void drm_atomic_crtc_print_state(struct drm_print *p, > + const struct drm_crtc_state *state) > +{ > + struct drm_crtc *crtc = state->crtc; > + > + drm_printf(p, "crtc[%u]: %s\n", crtc->base.id, crtc->name); > + drm_printf(p, "\tenable=%d\n", state->enable); > + drm_printf(p, "\tactive=%d\n", state->active); > + drm_printf(p, "\tplanes_changed=%d\n", state->planes_changed); > + drm_printf(p, "\tmode_changed=%d\n", state->mode_changed); > + drm_printf(p, "\tactive_changed=%d\n", state->active_changed); > + drm_printf(p, "\tconnectors_changed=%d\n", state->connectors_changed); > + drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed); > + drm_printf(p, "\tplane_mask=%x\n", state->plane_mask); > + drm_printf(p, "\tconnector_mask=%x\n", state->connector_mask); > + drm_printf(p, "\tencoder_mask=%x\n", state->encoder_mask); > + drm_printf(p, "\tmode: " DRM_MODE_FMT "\n", DRM_MODE_ARG(&state->mode)); > + > + if (crtc->funcs->atomic_print_state) > + crtc->funcs->atomic_print_state(p, state); > +} > + > /** > * drm_atomic_get_plane_state - get plane state > * @state: global atomic state object > @@ -895,6 +918,38 @@ static int drm_atomic_plane_check(struct drm_plane *plane, > return 0; > } > > +static void drm_atomic_plane_print_state(struct drm_print *p, > + const struct drm_plane_state *state) > +{ > + struct drm_plane *plane = state->plane; > + struct drm_rect src = drm_plane_state_src(state); > + struct drm_rect dest = drm_plane_state_dest(state); > + > + drm_printf(p, "plane[%u]: %s\n", plane->base.id, plane->name); > + drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)"); > + drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0); > + if (state->fb) { > + struct drm_framebuffer *fb = state->fb; > + int i, n = drm_format_num_planes(fb->pixel_format); > + > + drm_printf(p, "\t\tformat=%s\n", > + drm_get_format_name(fb->pixel_format)); > + drm_printf(p, "\t\tsize=%dx%d\n", fb->width, fb->height); > + drm_printf(p, "\t\tlayers:\n"); > + for (i = 0; i < n; i++) { > + drm_printf(p, "\t\t\tpitch[%d]=%u\n", i, fb->pitches[i]); > + drm_printf(p, "\t\t\toffset[%d]=%u\n", i, fb->offsets[i]); > + drm_printf(p, "\t\t\tmodifier[%d]=0x%llx\n", i, fb->modifier[i]); > + } > + } > + drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest)); > + drm_printf(p, "\tsrc-pos=" DRM_RECT_FP_FMT "\n", DRM_RECT_FP_ARG(&src)); > + drm_printf(p, "\trotation=%x\n", state->rotation); > + > + if (plane->funcs->atomic_print_state) > + plane->funcs->atomic_print_state(p, state); > +} > + > /** > * drm_atomic_get_connector_state - get connector state > * @state: global atomic state object > @@ -1010,6 +1065,18 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, > } > EXPORT_SYMBOL(drm_atomic_connector_set_property); > > +static void drm_atomic_connector_print_state(struct drm_print *p, > + const struct drm_connector_state *state) > +{ > + struct drm_connector *connector = state->connector; > + > + drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector->name); > + drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)"); > + > + if (connector->funcs->atomic_print_state) > + connector->funcs->atomic_print_state(p, state); > +} > + > /** > * drm_atomic_connector_get_property - get property value from connector state > * @connector: the drm connector to set a property on > @@ -1475,6 +1542,29 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state) > } > EXPORT_SYMBOL(drm_atomic_nonblocking_commit); > > +static void drm_atomic_print_state(const struct drm_atomic_state *state) > +{ > + struct drm_print p = drm_print_info(); > + struct drm_plane *plane; > + struct drm_plane_state *plane_state; > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > + struct drm_connector *connector; > + struct drm_connector_state *connector_state; > + int i; > + > + DRM_DEBUG_ATOMIC("checking %p\n", state); > + > + for_each_plane_in_state(state, plane, plane_state, i) > + drm_atomic_plane_print_state(&p, plane_state); > + > + for_each_crtc_in_state(state, crtc, crtc_state, i) > + drm_atomic_crtc_print_state(&p, crtc_state); > + > + for_each_connector_in_state(state, connector, connector_state, i) > + drm_atomic_connector_print_state(&p, connector_state); > +} > + > /* > * The big monstor ioctl > */ > @@ -1987,6 +2077,9 @@ retry: > */ > ret = drm_atomic_check_only(state); > } else { > + if (unlikely(drm_debug & DRM_UT_STATE)) > + drm_atomic_print_state(state); > + > if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) > install_out_fence(state, arg->count_out_fences, > fence_state); > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 7ffaa35..642d5da 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -133,6 +133,7 @@ struct dma_buf_attachment; > #define DRM_UT_PRIME 0x08 > #define DRM_UT_ATOMIC 0x10 > #define DRM_UT_VBL 0x20 > +#define DRM_UT_STATE 0x40 > > extern __printf(2, 3) > void drm_ut_debug_printk(const char *function_name, > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 856fdf8..d74d47a 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -44,6 +44,7 @@ struct drm_framebuffer; > struct drm_object_properties; > struct drm_file; > struct drm_clip_rect; > +struct drm_print; > struct device_node; > struct fence; > struct edid; > @@ -707,6 +708,18 @@ struct drm_crtc_funcs { > const struct drm_crtc_state *state, > struct drm_property *property, > uint64_t *val); > + > + /** > + * @atomic_print_state: > + * > + * If driver subclasses struct &drm_crtc_state, it should implement > + * this optional hook for printing state. > + * > + * Do not call this directly, use drm_atomic_crtc_print_state() > + * instead. These "Do not call this directory, use X instead" messages are somewhat misleading, since X is a static function in drm_atomic.c Perhaps you should export drm_atomic_print_state? Sean > + */ > + void (*atomic_print_state)(struct drm_print *p, > + const struct drm_crtc_state *state); > }; > > /** > @@ -1132,6 +1145,18 @@ struct drm_connector_funcs { > const struct drm_connector_state *state, > struct drm_property *property, > uint64_t *val); > + > + /** > + * @atomic_print_state: > + * > + * If driver subclasses struct &drm_connector_state, it should > + * implement this optional hook for printing state. > + * > + * Do not call this directly, use drm_atomic_connector_print_state() > + * instead. > + */ > + void (*atomic_print_state)(struct drm_print *p, > + const struct drm_connector_state *state); > }; > > /** > @@ -1623,6 +1648,18 @@ struct drm_plane_funcs { > const struct drm_plane_state *state, > struct drm_property *property, > uint64_t *val); > + > + /** > + * @atomic_print_state: > + * > + * If driver subclasses struct &drm_plane_state, it should implement > + * this optional hook for printing state. > + * > + * Do not call this directly, use drm_atomic_plane_print_state() > + * instead. > + */ > + void (*atomic_print_state)(struct drm_print *p, > + const struct drm_plane_state *state); > }; > > enum drm_plane_type { > -- > 2.7.4 >
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7ebf375..3cc3cb5 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -30,6 +30,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_mode.h> #include <drm/drm_plane_helper.h> +#include <drm/drm_print.h> #include <linux/sync_file.h> #include "drm_crtc_internal.h" @@ -608,6 +609,28 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, return 0; } +static void drm_atomic_crtc_print_state(struct drm_print *p, + const struct drm_crtc_state *state) +{ + struct drm_crtc *crtc = state->crtc; + + drm_printf(p, "crtc[%u]: %s\n", crtc->base.id, crtc->name); + drm_printf(p, "\tenable=%d\n", state->enable); + drm_printf(p, "\tactive=%d\n", state->active); + drm_printf(p, "\tplanes_changed=%d\n", state->planes_changed); + drm_printf(p, "\tmode_changed=%d\n", state->mode_changed); + drm_printf(p, "\tactive_changed=%d\n", state->active_changed); + drm_printf(p, "\tconnectors_changed=%d\n", state->connectors_changed); + drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed); + drm_printf(p, "\tplane_mask=%x\n", state->plane_mask); + drm_printf(p, "\tconnector_mask=%x\n", state->connector_mask); + drm_printf(p, "\tencoder_mask=%x\n", state->encoder_mask); + drm_printf(p, "\tmode: " DRM_MODE_FMT "\n", DRM_MODE_ARG(&state->mode)); + + if (crtc->funcs->atomic_print_state) + crtc->funcs->atomic_print_state(p, state); +} + /** * drm_atomic_get_plane_state - get plane state * @state: global atomic state object @@ -895,6 +918,38 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return 0; } +static void drm_atomic_plane_print_state(struct drm_print *p, + const struct drm_plane_state *state) +{ + struct drm_plane *plane = state->plane; + struct drm_rect src = drm_plane_state_src(state); + struct drm_rect dest = drm_plane_state_dest(state); + + drm_printf(p, "plane[%u]: %s\n", plane->base.id, plane->name); + drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)"); + drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0); + if (state->fb) { + struct drm_framebuffer *fb = state->fb; + int i, n = drm_format_num_planes(fb->pixel_format); + + drm_printf(p, "\t\tformat=%s\n", + drm_get_format_name(fb->pixel_format)); + drm_printf(p, "\t\tsize=%dx%d\n", fb->width, fb->height); + drm_printf(p, "\t\tlayers:\n"); + for (i = 0; i < n; i++) { + drm_printf(p, "\t\t\tpitch[%d]=%u\n", i, fb->pitches[i]); + drm_printf(p, "\t\t\toffset[%d]=%u\n", i, fb->offsets[i]); + drm_printf(p, "\t\t\tmodifier[%d]=0x%llx\n", i, fb->modifier[i]); + } + } + drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest)); + drm_printf(p, "\tsrc-pos=" DRM_RECT_FP_FMT "\n", DRM_RECT_FP_ARG(&src)); + drm_printf(p, "\trotation=%x\n", state->rotation); + + if (plane->funcs->atomic_print_state) + plane->funcs->atomic_print_state(p, state); +} + /** * drm_atomic_get_connector_state - get connector state * @state: global atomic state object @@ -1010,6 +1065,18 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_atomic_connector_set_property); +static void drm_atomic_connector_print_state(struct drm_print *p, + const struct drm_connector_state *state) +{ + struct drm_connector *connector = state->connector; + + drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector->name); + drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)"); + + if (connector->funcs->atomic_print_state) + connector->funcs->atomic_print_state(p, state); +} + /** * drm_atomic_connector_get_property - get property value from connector state * @connector: the drm connector to set a property on @@ -1475,6 +1542,29 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state) } EXPORT_SYMBOL(drm_atomic_nonblocking_commit); +static void drm_atomic_print_state(const struct drm_atomic_state *state) +{ + struct drm_print p = drm_print_info(); + struct drm_plane *plane; + struct drm_plane_state *plane_state; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + struct drm_connector *connector; + struct drm_connector_state *connector_state; + int i; + + DRM_DEBUG_ATOMIC("checking %p\n", state); + + for_each_plane_in_state(state, plane, plane_state, i) + drm_atomic_plane_print_state(&p, plane_state); + + for_each_crtc_in_state(state, crtc, crtc_state, i) + drm_atomic_crtc_print_state(&p, crtc_state); + + for_each_connector_in_state(state, connector, connector_state, i) + drm_atomic_connector_print_state(&p, connector_state); +} + /* * The big monstor ioctl */ @@ -1987,6 +2077,9 @@ retry: */ ret = drm_atomic_check_only(state); } else { + if (unlikely(drm_debug & DRM_UT_STATE)) + drm_atomic_print_state(state); + if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) install_out_fence(state, arg->count_out_fences, fence_state); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 7ffaa35..642d5da 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -133,6 +133,7 @@ struct dma_buf_attachment; #define DRM_UT_PRIME 0x08 #define DRM_UT_ATOMIC 0x10 #define DRM_UT_VBL 0x20 +#define DRM_UT_STATE 0x40 extern __printf(2, 3) void drm_ut_debug_printk(const char *function_name, diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 856fdf8..d74d47a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -44,6 +44,7 @@ struct drm_framebuffer; struct drm_object_properties; struct drm_file; struct drm_clip_rect; +struct drm_print; struct device_node; struct fence; struct edid; @@ -707,6 +708,18 @@ struct drm_crtc_funcs { const struct drm_crtc_state *state, struct drm_property *property, uint64_t *val); + + /** + * @atomic_print_state: + * + * If driver subclasses struct &drm_crtc_state, it should implement + * this optional hook for printing state. + * + * Do not call this directly, use drm_atomic_crtc_print_state() + * instead. + */ + void (*atomic_print_state)(struct drm_print *p, + const struct drm_crtc_state *state); }; /** @@ -1132,6 +1145,18 @@ struct drm_connector_funcs { const struct drm_connector_state *state, struct drm_property *property, uint64_t *val); + + /** + * @atomic_print_state: + * + * If driver subclasses struct &drm_connector_state, it should + * implement this optional hook for printing state. + * + * Do not call this directly, use drm_atomic_connector_print_state() + * instead. + */ + void (*atomic_print_state)(struct drm_print *p, + const struct drm_connector_state *state); }; /** @@ -1623,6 +1648,18 @@ struct drm_plane_funcs { const struct drm_plane_state *state, struct drm_property *property, uint64_t *val); + + /** + * @atomic_print_state: + * + * If driver subclasses struct &drm_plane_state, it should implement + * this optional hook for printing state. + * + * Do not call this directly, use drm_atomic_plane_print_state() + * instead. + */ + void (*atomic_print_state)(struct drm_print *p, + const struct drm_plane_state *state); }; enum drm_plane_type {
Signed-off-by: Rob Clark <robdclark@gmail.com> --- drivers/gpu/drm/drm_atomic.c | 93 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 1 + include/drm/drm_crtc.h | 37 ++++++++++++++++++ 3 files changed, 131 insertions(+)