[PATCH 0/6] drm: add atomic state logging and debugfs

[PATCH 0/6] drm: add atomic state logging and debugfs

[Rob Clark]

Bit more spiffed out version of the RFC.  Now with Sean's suggestion to
add vfuncs in plane/crtc/connector funcs for drivers that subclass the
various state structs.  Plus Ville's suggestion about helper macros
for printing mode/rect structs (and alignment with how drm_rect printed
the integer and fixed-point rects).  Plus addition of connector state
and debugfs.

Rob Clark (6):
  drm: helper macros to print composite types
  drm: add helper for printing to log or seq_file
  drm: add helpers to go from plane state to drm_rect
  drm/atomic: add new drm_debug bit to dump atomic state before commit
  drm/atomic: add debugfs file to dump out atomic state
  drm/msm/mdp5: add atomic_print_state support

 drivers/gpu/drm/Makefile                    |   3 +-
 drivers/gpu/drm/drm_atomic.c                | 133 ++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_debugfs.c               |   9 ++
 drivers/gpu/drm/drm_modes.c                 |   8 +-
 drivers/gpu/drm/drm_print.c                 |  54 +++++++++++
 drivers/gpu/drm/drm_rect.c                  |  11 +--
 drivers/gpu/drm/drm_simple_kms_helper.c     |  14 +--
 drivers/gpu/drm/i915/intel_atomic_plane.c   |  10 +--
 drivers/gpu/drm/mediatek/mtk_drm_plane.c    |  15 +---
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h     |  12 +++
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c   |  18 +++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  10 +--
 include/drm/drmP.h                          |  22 +++++
 include/drm/drm_atomic.h                    |   4 +
 include/drm/drm_crtc.h                      |  61 +++++++++++++
 include/drm/drm_print.h                     |  62 +++++++++++++
 16 files changed, 387 insertions(+), 59 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_print.c
 create mode 100644 include/drm/drm_print.h

-- 
2.7.4

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 1/6] drm: helper macros to print composite types

[Rob Clark]

I'll want to print things in a similar way in a later patch.  This will
make it easier.

TODO drm_rect_debug_print() doesn't have many call sites, and is kind of
unnecessary now.  Should we just drop it?

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_modes.c |  8 +-------
 drivers/gpu/drm/drm_rect.c  | 11 ++---------
 include/drm/drmP.h          | 21 +++++++++++++++++++++
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index fc5040a..77b0301 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -49,13 +49,7 @@
  */
 void drm_mode_debug_printmodeline(const struct drm_display_mode *mode)
 {
-	DRM_DEBUG_KMS("Modeline %d:\"%s\" %d %d %d %d %d %d %d %d %d %d "
-			"0x%x 0x%x\n",
-		mode->base.id, mode->name, mode->vrefresh, mode->clock,
-		mode->hdisplay, mode->hsync_start,
-		mode->hsync_end, mode->htotal,
-		mode->vdisplay, mode->vsync_start,
-		mode->vsync_end, mode->vtotal, mode->type, mode->flags);
+	DRM_DEBUG_KMS("Modeline " DRM_MODE_FMT "\n", DRM_MODE_ARG(mode));
 }
 EXPORT_SYMBOL(drm_mode_debug_printmodeline);
 
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index a8e2c86..d451112 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -281,17 +281,10 @@ EXPORT_SYMBOL(drm_rect_calc_vscale_relaxed);
  */
 void drm_rect_debug_print(const char *prefix, const struct drm_rect *r, bool fixed_point)
 {
-	int w = drm_rect_width(r);
-	int h = drm_rect_height(r);
-
 	if (fixed_point)
-		DRM_DEBUG_KMS("%s%d.%06ux%d.%06u%+d.%06u%+d.%06u\n", prefix,
-			      w >> 16, ((w & 0xffff) * 15625) >> 10,
-			      h >> 16, ((h & 0xffff) * 15625) >> 10,
-			      r->x1 >> 16, ((r->x1 & 0xffff) * 15625) >> 10,
-			      r->y1 >> 16, ((r->y1 & 0xffff) * 15625) >> 10);
+		DRM_DEBUG_KMS("%s" DRM_RECT_FP_FMT "\n", prefix, DRM_RECT_FP_ARG(r));
 	else
-		DRM_DEBUG_KMS("%s%dx%d%+d%+d\n", prefix, w, h, r->x1, r->y1);
+		DRM_DEBUG_KMS("%s" DRM_RECT_FMT "\n", prefix, DRM_RECT_ARG(r));
 }
 EXPORT_SYMBOL(drm_rect_debug_print);
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 28d341a..7ffaa35 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -231,6 +231,27 @@ void drm_err(const char *format, ...);
 			drm_ut_debug_printk(__func__, fmt, ##args);	\
 	} while (0)
 
+/* Format strings and argument splitters to simplify printing
+ * various "complex" objects
+ */
+#define DRM_MODE_FMT    "%d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x"
+#define DRM_MODE_ARG(m) \
+	(m)->base.id, (m)->name, (m)->vrefresh, (m)->clock, \
+	(m)->hdisplay, (m)->hsync_start, (m)->hsync_end, (m)->htotal, \
+	(m)->vdisplay, (m)->vsync_start, (m)->vsync_end, (m)->vtotal, \
+	(m)->type, (m)->flags
+
+#define DRM_RECT_FMT    "%dx%d%+d%+d"
+#define DRM_RECT_ARG(r) drm_rect_width(r), drm_rect_height(r), (r)->x1, (r)->y1
+
+/* for rect's in fixed-point format: */
+#define DRM_RECT_FP_FMT "%d.%06ux%d.%06u%+d.%06u%+d.%06u"
+#define DRM_RECT_FP_ARG(r) \
+		drm_rect_width(r) >> 16, ((drm_rect_width(r) & 0xffff) * 15625) >> 10, \
+		drm_rect_height(r) >> 16, ((drm_rect_height(r) & 0xffff) * 15625) >> 10, \
+		(r)->x1 >> 16, (((r)->x1 & 0xffff) * 15625) >> 10, \
+		(r)->y1 >> 16, (((r)->y1 & 0xffff) * 15625) >> 10
+
 /*@}*/
 
 /***********************************************************************/
-- 
2.7.4

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 2/6] drm: add helper for printing to log or seq_file

[Rob Clark]

Sometimes it is nice not to duplicate equivalent printk() and
seq_printf() code.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/Makefile    |  3 ++-
 drivers/gpu/drm/drm_print.c | 54 +++++++++++++++++++++++++++++++++++++++
 include/drm/drm_print.h     | 62 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 118 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_print.c
 create mode 100644 include/drm/drm_print.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index e3dba6f..4894ebb 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -12,7 +12,8 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_info.o drm_debugfs.o drm_encoder_slave.o \
 		drm_trace_points.o drm_global.o drm_prime.o \
 		drm_rect.o drm_vma_manager.o drm_flip_work.o \
-		drm_modeset_lock.o drm_atomic.o drm_bridge.o
+		drm_modeset_lock.o drm_atomic.o drm_bridge.o \
+		drm_print.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
new file mode 100644
index 0000000..ddcb437
--- /dev/null
+++ b/drivers/gpu/drm/drm_print.c
@@ -0,0 +1,54 @@
+/*
+ * Copyright (C) 2016 Red Hat
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors:
+ * Rob Clark <robdclark@gmail.com>
+ */
+
+#include <linux/seq_file.h>
+#include <drm/drmP.h>
+#include <drm/drm_print.h>
+
+void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args)
+{
+	seq_vprintf(p->arg, f, args);
+}
+
+void __drm_printfn_info(struct drm_print *p, const char *f, va_list args)
+{
+	const char *hdr = KERN_INFO "[" DRM_NAME "] ";
+	char fmt[strlen(hdr) + strlen(f) + 1];
+
+	memcpy(fmt, hdr, strlen(hdr));
+	memcpy(fmt + strlen(hdr), f, strlen(f));
+	fmt[strlen(hdr) + strlen(f)] = 0;
+
+	vprintk(fmt, args);
+}
+
+void drm_printf(struct drm_print *p, const char *f, ...)
+{
+	va_list args;
+
+	va_start(args, f);
+	p->printfn(p, f, args);
+	va_end(args);
+}
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
new file mode 100644
index 0000000..f294a9a
--- /dev/null
+++ b/include/drm/drm_print.h
@@ -0,0 +1,62 @@
+/*
+ * Copyright (C) 2016 Red Hat
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors:
+ * Rob Clark <robdclark@gmail.com>
+ */
+
+#ifndef DRM_PRINT_H_
+#define DRM_PRINT_H_
+
+#include <stdarg.h>
+#include <linux/seq_file.h>
+
+/* A simple wrapper to abstract seq_file vs printk, so same logging code
+ * does not have to be duplicated.
+ */
+struct drm_print {
+	void (*printfn)(struct drm_print *p, const char *f, va_list args);
+	void *arg;
+};
+
+void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args);
+void __drm_printfn_info(struct drm_print *p, const char *f, va_list args);
+
+void drm_printf(struct drm_print *p, const char *f, ...);
+
+static inline struct drm_print drm_print_seq_file(struct seq_file *f)
+{
+	struct drm_print p = {
+		.printfn = __drm_printfn_seq_file,
+		.arg = f,
+	};
+	return p;
+}
+
+static inline struct drm_print drm_print_info(void)
+{
+	struct drm_print p = {
+		.printfn = __drm_printfn_info,
+	};
+	return p;
+}
+
+#endif /* DRM_PRINT_H_ */
-- 
2.7.4

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 3/6] drm: add helpers to go from plane state to drm_rect

[Rob Clark]

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_simple_kms_helper.c     | 14 ++------------
 drivers/gpu/drm/i915/intel_atomic_plane.c   | 10 ++--------
 drivers/gpu/drm/mediatek/mtk_drm_plane.c    | 15 ++-------------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 10 ++--------
 include/drm/drm_crtc.h                      | 24 ++++++++++++++++++++++++
 5 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 0db36d2..9834fc5 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -73,18 +73,8 @@ static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = {
 static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
 					struct drm_plane_state *plane_state)
 {
-	struct drm_rect src = {
-		.x1 = plane_state->src_x,
-		.y1 = plane_state->src_y,
-		.x2 = plane_state->src_x + plane_state->src_w,
-		.y2 = plane_state->src_y + plane_state->src_h,
-	};
-	struct drm_rect dest = {
-		.x1 = plane_state->crtc_x,
-		.y1 = plane_state->crtc_y,
-		.x2 = plane_state->crtc_x + plane_state->crtc_w,
-		.y2 = plane_state->crtc_y + plane_state->crtc_h,
-	};
+	struct drm_rect src = drm_plane_state_src(plane_state);
+	struct drm_rect dest = drm_plane_state_dest(plane_state);
 	struct drm_rect clip = { 0 };
 	struct drm_simple_display_pipe *pipe;
 	struct drm_crtc_state *crtc_state;
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 7de7721..6682e9b 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -139,14 +139,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	 * we want to keep another copy internal to our driver that we can
 	 * clip/modify ourselves.
 	 */
-	intel_state->src.x1 = state->src_x;
-	intel_state->src.y1 = state->src_y;
-	intel_state->src.x2 = state->src_x + state->src_w;
-	intel_state->src.y2 = state->src_y + state->src_h;
-	intel_state->dst.x1 = state->crtc_x;
-	intel_state->dst.y1 = state->crtc_y;
-	intel_state->dst.x2 = state->crtc_x + state->crtc_w;
-	intel_state->dst.y2 = state->crtc_y + state->crtc_h;
+	intel_state->src = drm_plane_state_src(state);
+	intel_state->dst = drm_plane_state_dest(state);
 
 	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
 	intel_state->clip.x1 = 0;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 3995765..2749e74 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -135,19 +135,8 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_crtc_state *crtc_state;
 	bool visible;
-	struct drm_rect dest = {
-		.x1 = state->crtc_x,
-		.y1 = state->crtc_y,
-		.x2 = state->crtc_x + state->crtc_w,
-		.y2 = state->crtc_y + state->crtc_h,
-	};
-	struct drm_rect src = {
-		/* 16.16 fixed point */
-		.x1 = state->src_x,
-		.y1 = state->src_y,
-		.x2 = state->src_x + state->src_w,
-		.y2 = state->src_y + state->src_h,
-	};
+	struct drm_rect dest = drm_plane_state_dest(state);
+	struct drm_rect src  = drm_plane_state_src(state);  /* 16.16 fixed point */
 	struct drm_rect clip = { 0, };
 
 	if (!fb)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 6255e5b..31384ca 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -610,14 +610,8 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 	if (WARN_ON(!crtc_state))
 		return -EINVAL;
 
-	src->x1 = state->src_x;
-	src->y1 = state->src_y;
-	src->x2 = state->src_x + state->src_w;
-	src->y2 = state->src_y + state->src_h;
-	dest->x1 = state->crtc_x;
-	dest->y1 = state->crtc_y;
-	dest->x2 = state->crtc_x + state->crtc_w;
-	dest->y2 = state->crtc_y + state->crtc_h;
+	*src  = drm_plane_state_src(state);
+	*dest = drm_plane_state_dest(state);
 
 	clip.x1 = 0;
 	clip.y1 = 0;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index b7d67cc..856fdf8 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -36,6 +36,7 @@
 #include <uapi/drm/drm_mode.h>
 #include <uapi/drm/drm_fourcc.h>
 #include <drm/drm_modeset_lock.h>
+#include <drm/drm_rect.h>
 
 struct drm_device;
 struct drm_mode_set;
@@ -1395,6 +1396,29 @@ struct drm_plane_state {
 	struct drm_atomic_state *state;
 };
 
+static inline struct drm_rect
+drm_plane_state_src(const struct drm_plane_state *state)
+{
+	struct drm_rect src = {
+		.x1 = state->src_x,
+		.y1 = state->src_y,
+		.x2 = state->src_x + state->src_w,
+		.y2 = state->src_y + state->src_h,
+	};
+	return src;
+}
+
+static inline struct drm_rect
+drm_plane_state_dest(const struct drm_plane_state *state)
+{
+	struct drm_rect dest = {
+		.x1 = state->crtc_x,
+		.y1 = state->crtc_y,
+		.x2 = state->crtc_x + state->crtc_w,
+		.y2 = state->crtc_y + state->crtc_h,
+	};
+	return dest;
+}
 
 /**
  * struct drm_plane_funcs - driver plane control functions
-- 
2.7.4

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 4/6] drm/atomic: add new drm_debug bit to dump atomic state before commit

[Rob Clark]

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.
+	 */
+	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

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

[PATCH 5/6] drm/atomic: add debugfs file to dump out atomic state

[Rob Clark]

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_atomic.c  | 40 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_debugfs.c |  9 +++++++++
 include/drm/drm_atomic.h      |  4 ++++
 3 files changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3cc3cb5..117d429 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1565,6 +1565,46 @@ static void drm_atomic_print_state(const struct drm_atomic_state *state)
 		drm_atomic_connector_print_state(&p, connector_state);
 }
 
+#ifdef CONFIG_DEBUG_FS
+static int drm_state_info(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+	struct drm_connector *connector;
+	struct drm_print p = drm_print_seq_file(m);
+
+	drm_modeset_lock_all(dev);
+
+	list_for_each_entry(plane, &config->plane_list, head)
+		drm_atomic_plane_print_state(&p, plane->state);
+
+	list_for_each_entry(crtc, &config->crtc_list, head)
+		drm_atomic_crtc_print_state(&p, crtc->state);
+
+	list_for_each_entry(connector, &config->connector_list, head)
+		drm_atomic_connector_print_state(&p, connector->state);
+
+	drm_modeset_unlock_all(dev);
+
+	return 0;
+}
+
+/* any use in debugfs files to dump individual planes/crtc/etc? */
+static const struct drm_info_list drm_atomic_debugfs_list[] = {
+	{"state", drm_state_info, 0},
+};
+
+int drm_atomic_debugfs_init(struct drm_minor *minor)
+{
+	return drm_debugfs_create_files(drm_atomic_debugfs_list,
+			ARRAY_SIZE(drm_atomic_debugfs_list),
+			minor->debugfs_root, minor);
+}
+#endif
+
 /*
  * The big monstor ioctl
  */
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index fa10cef..d8945a9 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -36,6 +36,7 @@
 #include <linux/export.h>
 #include <drm/drmP.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_atomic.h>
 #include "drm_internal.h"
 
 #if defined(CONFIG_DEBUG_FS)
@@ -163,6 +164,14 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 		return ret;
 	}
 
+	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
+		ret = drm_atomic_debugfs_init(minor);
+		if (ret) {
+			DRM_ERROR("Failed to create atomic debugfs files\n");
+			return ret;
+		}
+	}
+
 	if (dev->driver->debugfs_init) {
 		ret = dev->driver->debugfs_init(minor);
 		if (ret) {
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 856a9c8..68422f6 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -181,6 +181,10 @@ int __must_check drm_atomic_check_only(struct drm_atomic_state *state);
 int __must_check drm_atomic_commit(struct drm_atomic_state *state);
 int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);
 
+#ifdef CONFIG_DEBUG_FS
+int drm_atomic_debugfs_init(struct drm_minor *minor);
+#endif
+
 #define for_each_connector_in_state(__state, connector, connector_state, __i) \
 	for ((__i) = 0;							\
 	     (__i) < (__state)->num_connector &&				\
-- 
2.7.4

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 6/6] drm/msm/mdp5: add atomic_print_state support

[Rob Clark]

We subclass drm_plane_state, so add mdp5_plane_atomic_print_state() to
dump out our own driver specific plane state.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h   | 12 ++++++++++++
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 18 +++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
index e4b3fb3..3502711 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
@@ -120,6 +120,18 @@ static inline u32 mdp5_read(struct mdp5_kms *mdp5_kms, u32 reg)
 	return msm_readl(mdp5_kms->mmio + reg);
 }
 
+static inline const char *stage2name(enum mdp_mixer_stage_id stage)
+{
+	static const char *names[] = {
+#define NAME(n) [n] = #n
+		NAME(STAGE_UNUSED), NAME(STAGE_BASE),
+		NAME(STAGE0), NAME(STAGE1), NAME(STAGE2),
+		NAME(STAGE3), NAME(STAGE4), NAME(STAGE6),
+#undef NAME
+	};
+	return names[stage];
+}
+
 static inline const char *pipe2name(enum mdp5_pipe pipe)
 {
 	static const char *names[] = {
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
index b6f1fc66..9f99b4d 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
@@ -16,6 +16,7 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <drm/drm_print.h>
 #include "mdp5_kms.h"
 
 struct mdp5_plane {
@@ -187,6 +188,20 @@ done:
 #undef SET_PROPERTY
 }
 
+static void
+mdp5_plane_atomic_print_state(struct drm_print *p,
+		const struct drm_plane_state *state)
+{
+	struct mdp5_plane_state *pstate = to_mdp5_plane_state(state);
+
+	drm_printf(p, "\tpremultiplied=%u\n", pstate->premultiplied);
+	drm_printf(p, "\tzpos=%u\n", pstate->zpos);
+	drm_printf(p, "\talpha=%u\n", pstate->alpha);
+	drm_printf(p, "\tstage=%s\n", stage2name(pstate->stage));
+	drm_printf(p, "\tmode_changed=%u\n", pstate->mode_changed);
+	drm_printf(p, "\tpending=%u\n", pstate->pending);
+}
+
 static void mdp5_plane_reset(struct drm_plane *plane)
 {
 	struct mdp5_plane_state *mdp5_state;
@@ -250,6 +265,7 @@ static const struct drm_plane_funcs mdp5_plane_funcs = {
 		.reset = mdp5_plane_reset,
 		.atomic_duplicate_state = mdp5_plane_duplicate_state,
 		.atomic_destroy_state = mdp5_plane_destroy_state,
+		.atomic_print_state = mdp5_plane_atomic_print_state,
 };
 
 static int mdp5_plane_prepare_fb(struct drm_plane *plane,
@@ -907,7 +923,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
 	type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
 	ret = drm_universal_plane_init(dev, plane, 0xff, &mdp5_plane_funcs,
 				 mdp5_plane->formats, mdp5_plane->nformats,
-				 type, NULL);
+				 type, "%s", mdp5_plane->name);
 	if (ret)
 		goto fail;
 
-- 
2.7.4

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 0/6] drm: add atomic state logging and debugfs

[Rob Clark]

On Fri, Oct 14, 2016 at 7:55 PM, Rob Clark <robdclark@gmail.com> wrote:
> Bit more spiffed out version of the RFC.  Now with Sean's suggestion to
> add vfuncs in plane/crtc/connector funcs for drivers that subclass the
> various state structs.  Plus Ville's suggestion about helper macros
> for printing mode/rect structs (and alignment with how drm_rect printed
> the integer and fixed-point rects).  Plus addition of connector state
> and debugfs.

and jfyi, full state dump now looks like this:

--------
plane[24]: RGB0
    crtc=crtc-0
    fb=52
        format=XB24 little-endian (0x34324258)
        size=2048x1152
        layers:
            pitch[0]=8192
            offset[0]=0
            modifier[0]=0x0
    crtc-pos=2048x1152+0+0
    src-pos=2048.000000x1152.000000+0.000000+0.000000
    rotation=0
    premultiplied=0
    zpos=1
    alpha=255
    stage=STAGE_BASE
    mode_changed=0
    pending=0
plane[29]: RGB1
    crtc=(null)
    fb=0
    crtc-pos=0x0+0+0
    src-pos=0.000000x0.000000+0.000000+0.000000
    rotation=0
    premultiplied=0
    zpos=1
    alpha=255
    stage=STAGE_UNUSED
    mode_changed=0
    pending=0
plane[31]: VIG0
    crtc=(null)
    fb=0
    crtc-pos=0x0+0+0
    src-pos=0.000000x0.000000+0.000000+0.000000
    rotation=0
    premultiplied=0
    zpos=4
    alpha=255
    stage=STAGE_UNUSED
    mode_changed=0
    pending=0
plane[32]: DMA0
    crtc=(null)
    fb=0
    crtc-pos=0x0+0+0
    src-pos=0.000000x0.000000+0.000000+0.000000
    rotation=0
    premultiplied=0
    zpos=5
    alpha=255
    stage=STAGE_UNUSED
    mode_changed=0
    pending=0
crtc[28]: crtc-0
    enable=1
    active=1
    planes_changed=1
    mode_changed=0
    active_changed=0
    connectors_changed=0
    color_mgmt_changed=0
    plane_mask=1
    connector_mask=1
    encoder_mask=1
    mode: 0:"2048x1152" 60 156750 2048 2096 2128 2208 1152 1155 1160
1185 0x48 0x9
crtc[30]: crtc-1
    enable=0
    active=0
    planes_changed=0
    mode_changed=0
    active_changed=0
    connectors_changed=0
    color_mgmt_changed=0
    plane_mask=0
    connector_mask=0
    encoder_mask=0
    mode: 0:"" 0 0 0 0 0 0 0 0 0 0 0x0 0x0
connector[35]: HDMI-A-1
    crtc=crtc-0
--------

I've added fb parameters, which was another suggestion

BR,
-R

> Rob Clark (6):
>   drm: helper macros to print composite types
>   drm: add helper for printing to log or seq_file
>   drm: add helpers to go from plane state to drm_rect
>   drm/atomic: add new drm_debug bit to dump atomic state before commit
>   drm/atomic: add debugfs file to dump out atomic state
>   drm/msm/mdp5: add atomic_print_state support
>
>  drivers/gpu/drm/Makefile                    |   3 +-
>  drivers/gpu/drm/drm_atomic.c                | 133 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_debugfs.c               |   9 ++
>  drivers/gpu/drm/drm_modes.c                 |   8 +-
>  drivers/gpu/drm/drm_print.c                 |  54 +++++++++++
>  drivers/gpu/drm/drm_rect.c                  |  11 +--
>  drivers/gpu/drm/drm_simple_kms_helper.c     |  14 +--
>  drivers/gpu/drm/i915/intel_atomic_plane.c   |  10 +--
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c    |  15 +---
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h     |  12 +++
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c   |  18 +++-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  10 +--
>  include/drm/drmP.h                          |  22 +++++
>  include/drm/drm_atomic.h                    |   4 +
>  include/drm/drm_crtc.h                      |  61 +++++++++++++
>  include/drm/drm_print.h                     |  62 +++++++++++++
>  16 files changed, 387 insertions(+), 59 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_print.c
>  create mode 100644 include/drm/drm_print.h
>
> --
> 2.7.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm/atomic: add new drm_debug bit to dump atomic state before commit

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] drm: add helper for printing to log or seq_file

[Daniel Vetter]

On Fri, Oct 14, 2016 at 07:55:49PM -0400, Rob Clark wrote:
> Sometimes it is nice not to duplicate equivalent printk() and
> seq_printf() code.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Ok, maybe I should wait for coffee to kick in before commenting on patches
;-)

Two bits:
- kerneldoc for this pls, plus pulling into drm-internals.rst.
- Would be awesome to de-dupe the existing dumping code in drm_mm.c with
  this.

-Daniel

> ---
>  drivers/gpu/drm/Makefile    |  3 ++-
>  drivers/gpu/drm/drm_print.c | 54 +++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_print.h     | 62 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 118 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_print.c
>  create mode 100644 include/drm/drm_print.h
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index e3dba6f..4894ebb 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -12,7 +12,8 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
>  		drm_info.o drm_debugfs.o drm_encoder_slave.o \
>  		drm_trace_points.o drm_global.o drm_prime.o \
>  		drm_rect.o drm_vma_manager.o drm_flip_work.o \
> -		drm_modeset_lock.o drm_atomic.o drm_bridge.o
> +		drm_modeset_lock.o drm_atomic.o drm_bridge.o \
> +		drm_print.o
>  
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> new file mode 100644
> index 0000000..ddcb437
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -0,0 +1,54 @@
> +/*
> + * Copyright (C) 2016 Red Hat
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Rob Clark <robdclark@gmail.com>
> + */
> +
> +#include <linux/seq_file.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_print.h>
> +
> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args)
> +{
> +	seq_vprintf(p->arg, f, args);
> +}
> +
> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args)
> +{
> +	const char *hdr = KERN_INFO "[" DRM_NAME "] ";
> +	char fmt[strlen(hdr) + strlen(f) + 1];
> +
> +	memcpy(fmt, hdr, strlen(hdr));
> +	memcpy(fmt + strlen(hdr), f, strlen(f));
> +	fmt[strlen(hdr) + strlen(f)] = 0;
> +
> +	vprintk(fmt, args);
> +}
> +
> +void drm_printf(struct drm_print *p, const char *f, ...)
> +{
> +	va_list args;
> +
> +	va_start(args, f);
> +	p->printfn(p, f, args);
> +	va_end(args);
> +}
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> new file mode 100644
> index 0000000..f294a9a
> --- /dev/null
> +++ b/include/drm/drm_print.h
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright (C) 2016 Red Hat
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Rob Clark <robdclark@gmail.com>
> + */
> +
> +#ifndef DRM_PRINT_H_
> +#define DRM_PRINT_H_
> +
> +#include <stdarg.h>
> +#include <linux/seq_file.h>
> +
> +/* A simple wrapper to abstract seq_file vs printk, so same logging code
> + * does not have to be duplicated.
> + */
> +struct drm_print {
> +	void (*printfn)(struct drm_print *p, const char *f, va_list args);
> +	void *arg;
> +};
> +
> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args);
> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args);
> +
> +void drm_printf(struct drm_print *p, const char *f, ...);
> +
> +static inline struct drm_print drm_print_seq_file(struct seq_file *f)
> +{
> +	struct drm_print p = {
> +		.printfn = __drm_printfn_seq_file,
> +		.arg = f,
> +	};
> +	return p;
> +}
> +
> +static inline struct drm_print drm_print_info(void)
> +{
> +	struct drm_print p = {
> +		.printfn = __drm_printfn_info,
> +	};
> +	return p;
> +}
> +
> +#endif /* DRM_PRINT_H_ */
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] drm: add helper for printing to log or seq_file

[Rob Clark]

On Mon, Oct 17, 2016 at 2:38 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Oct 14, 2016 at 07:55:49PM -0400, Rob Clark wrote:
>> Sometimes it is nice not to duplicate equivalent printk() and
>> seq_printf() code.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>
> Ok, maybe I should wait for coffee to kick in before commenting on patches
> ;-)
>
> Two bits:
> - kerneldoc for this pls, plus pulling into drm-internals.rst.
> - Would be awesome to de-dupe the existing dumping code in drm_mm.c with
>   this.

I have a couple things in drm/msm that I want to de-dup.. and I guess
most drivers probably do as well..

I'm toying with the idea to convert drm_info_list::show over to take a
drm_print ptr instead of seq_file, then we could just re-use the
debugfs fxns directly without having extra wrappers fxns.. question is
best way to do that in a non-flag-day way..

BR,
-R

> -Daniel
>
>> ---
>>  drivers/gpu/drm/Makefile    |  3 ++-
>>  drivers/gpu/drm/drm_print.c | 54 +++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_print.h     | 62 +++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 118 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/drm_print.c
>>  create mode 100644 include/drm/drm_print.h
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index e3dba6f..4894ebb 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -12,7 +12,8 @@ drm-y       :=      drm_auth.o drm_bufs.o drm_cache.o \
>>               drm_info.o drm_debugfs.o drm_encoder_slave.o \
>>               drm_trace_points.o drm_global.o drm_prime.o \
>>               drm_rect.o drm_vma_manager.o drm_flip_work.o \
>> -             drm_modeset_lock.o drm_atomic.o drm_bridge.o
>> +             drm_modeset_lock.o drm_atomic.o drm_bridge.o \
>> +             drm_print.o
>>
>>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
>> new file mode 100644
>> index 0000000..ddcb437
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_print.c
>> @@ -0,0 +1,54 @@
>> +/*
>> + * Copyright (C) 2016 Red Hat
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + * Rob Clark <robdclark@gmail.com>
>> + */
>> +
>> +#include <linux/seq_file.h>
>> +#include <drm/drmP.h>
>> +#include <drm/drm_print.h>
>> +
>> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args)
>> +{
>> +     seq_vprintf(p->arg, f, args);
>> +}
>> +
>> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args)
>> +{
>> +     const char *hdr = KERN_INFO "[" DRM_NAME "] ";
>> +     char fmt[strlen(hdr) + strlen(f) + 1];
>> +
>> +     memcpy(fmt, hdr, strlen(hdr));
>> +     memcpy(fmt + strlen(hdr), f, strlen(f));
>> +     fmt[strlen(hdr) + strlen(f)] = 0;
>> +
>> +     vprintk(fmt, args);
>> +}
>> +
>> +void drm_printf(struct drm_print *p, const char *f, ...)
>> +{
>> +     va_list args;
>> +
>> +     va_start(args, f);
>> +     p->printfn(p, f, args);
>> +     va_end(args);
>> +}
>> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
>> new file mode 100644
>> index 0000000..f294a9a
>> --- /dev/null
>> +++ b/include/drm/drm_print.h
>> @@ -0,0 +1,62 @@
>> +/*
>> + * Copyright (C) 2016 Red Hat
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + * Rob Clark <robdclark@gmail.com>
>> + */
>> +
>> +#ifndef DRM_PRINT_H_
>> +#define DRM_PRINT_H_
>> +
>> +#include <stdarg.h>
>> +#include <linux/seq_file.h>
>> +
>> +/* A simple wrapper to abstract seq_file vs printk, so same logging code
>> + * does not have to be duplicated.
>> + */
>> +struct drm_print {
>> +     void (*printfn)(struct drm_print *p, const char *f, va_list args);
>> +     void *arg;
>> +};
>> +
>> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args);
>> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args);
>> +
>> +void drm_printf(struct drm_print *p, const char *f, ...);
>> +
>> +static inline struct drm_print drm_print_seq_file(struct seq_file *f)
>> +{
>> +     struct drm_print p = {
>> +             .printfn = __drm_printfn_seq_file,
>> +             .arg = f,
>> +     };
>> +     return p;
>> +}
>> +
>> +static inline struct drm_print drm_print_info(void)
>> +{
>> +     struct drm_print p = {
>> +             .printfn = __drm_printfn_info,
>> +     };
>> +     return p;
>> +}
>> +
>> +#endif /* DRM_PRINT_H_ */
>> --
>> 2.7.4
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] drm: add helper for printing to log or seq_file

[Daniel Vetter]

On Mon, Oct 17, 2016 at 07:59:51AM -0400, Rob Clark wrote:
> On Mon, Oct 17, 2016 at 2:38 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Oct 14, 2016 at 07:55:49PM -0400, Rob Clark wrote:
> >> Sometimes it is nice not to duplicate equivalent printk() and
> >> seq_printf() code.
> >>
> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >
> > Ok, maybe I should wait for coffee to kick in before commenting on patches
> > ;-)
> >
> > Two bits:
> > - kerneldoc for this pls, plus pulling into drm-internals.rst.
> > - Would be awesome to de-dupe the existing dumping code in drm_mm.c with
> >   this.
> 
> I have a couple things in drm/msm that I want to de-dup.. and I guess
> most drivers probably do as well..
> 
> I'm toying with the idea to convert drm_info_list::show over to take a
> drm_print ptr instead of seq_file, then we could just re-use the
> debugfs fxns directly without having extra wrappers fxns.. question is
> best way to do that in a non-flag-day way..

One idea I had in the other reply is to create a seq_file which can dump
to debugfs. Not sure how feasible that is ...
-Daniel

> 
> BR,
> -R
> 
> > -Daniel
> >
> >> ---
> >>  drivers/gpu/drm/Makefile    |  3 ++-
> >>  drivers/gpu/drm/drm_print.c | 54 +++++++++++++++++++++++++++++++++++++++
> >>  include/drm/drm_print.h     | 62 +++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 118 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/gpu/drm/drm_print.c
> >>  create mode 100644 include/drm/drm_print.h
> >>
> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >> index e3dba6f..4894ebb 100644
> >> --- a/drivers/gpu/drm/Makefile
> >> +++ b/drivers/gpu/drm/Makefile
> >> @@ -12,7 +12,8 @@ drm-y       :=      drm_auth.o drm_bufs.o drm_cache.o \
> >>               drm_info.o drm_debugfs.o drm_encoder_slave.o \
> >>               drm_trace_points.o drm_global.o drm_prime.o \
> >>               drm_rect.o drm_vma_manager.o drm_flip_work.o \
> >> -             drm_modeset_lock.o drm_atomic.o drm_bridge.o
> >> +             drm_modeset_lock.o drm_atomic.o drm_bridge.o \
> >> +             drm_print.o
> >>
> >>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
> >>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> >> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> >> new file mode 100644
> >> index 0000000..ddcb437
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/drm_print.c
> >> @@ -0,0 +1,54 @@
> >> +/*
> >> + * Copyright (C) 2016 Red Hat
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining a
> >> + * copy of this software and associated documentation files (the "Software"),
> >> + * to deal in the Software without restriction, including without limitation
> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >> + * and/or sell copies of the Software, and to permit persons to whom the
> >> + * Software is furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice shall be included in
> >> + * all copies or substantial portions of the Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> >> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> >> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> >> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> >> + * OTHER DEALINGS IN THE SOFTWARE.
> >> + *
> >> + * Authors:
> >> + * Rob Clark <robdclark@gmail.com>
> >> + */
> >> +
> >> +#include <linux/seq_file.h>
> >> +#include <drm/drmP.h>
> >> +#include <drm/drm_print.h>
> >> +
> >> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args)
> >> +{
> >> +     seq_vprintf(p->arg, f, args);
> >> +}
> >> +
> >> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args)
> >> +{
> >> +     const char *hdr = KERN_INFO "[" DRM_NAME "] ";
> >> +     char fmt[strlen(hdr) + strlen(f) + 1];
> >> +
> >> +     memcpy(fmt, hdr, strlen(hdr));
> >> +     memcpy(fmt + strlen(hdr), f, strlen(f));
> >> +     fmt[strlen(hdr) + strlen(f)] = 0;
> >> +
> >> +     vprintk(fmt, args);
> >> +}
> >> +
> >> +void drm_printf(struct drm_print *p, const char *f, ...)
> >> +{
> >> +     va_list args;
> >> +
> >> +     va_start(args, f);
> >> +     p->printfn(p, f, args);
> >> +     va_end(args);
> >> +}
> >> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> >> new file mode 100644
> >> index 0000000..f294a9a
> >> --- /dev/null
> >> +++ b/include/drm/drm_print.h
> >> @@ -0,0 +1,62 @@
> >> +/*
> >> + * Copyright (C) 2016 Red Hat
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining a
> >> + * copy of this software and associated documentation files (the "Software"),
> >> + * to deal in the Software without restriction, including without limitation
> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >> + * and/or sell copies of the Software, and to permit persons to whom the
> >> + * Software is furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice shall be included in
> >> + * all copies or substantial portions of the Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> >> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> >> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> >> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> >> + * OTHER DEALINGS IN THE SOFTWARE.
> >> + *
> >> + * Authors:
> >> + * Rob Clark <robdclark@gmail.com>
> >> + */
> >> +
> >> +#ifndef DRM_PRINT_H_
> >> +#define DRM_PRINT_H_
> >> +
> >> +#include <stdarg.h>
> >> +#include <linux/seq_file.h>
> >> +
> >> +/* A simple wrapper to abstract seq_file vs printk, so same logging code
> >> + * does not have to be duplicated.
> >> + */
> >> +struct drm_print {
> >> +     void (*printfn)(struct drm_print *p, const char *f, va_list args);
> >> +     void *arg;
> >> +};
> >> +
> >> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args);
> >> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args);
> >> +
> >> +void drm_printf(struct drm_print *p, const char *f, ...);
> >> +
> >> +static inline struct drm_print drm_print_seq_file(struct seq_file *f)
> >> +{
> >> +     struct drm_print p = {
> >> +             .printfn = __drm_printfn_seq_file,
> >> +             .arg = f,
> >> +     };
> >> +     return p;
> >> +}
> >> +
> >> +static inline struct drm_print drm_print_info(void)
> >> +{
> >> +     struct drm_print p = {
> >> +             .printfn = __drm_printfn_info,
> >> +     };
> >> +     return p;
> >> +}
> >> +
> >> +#endif /* DRM_PRINT_H_ */
> >> --
> >> 2.7.4
> >>
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] drm: add helper for printing to log or seq_file

[Rob Clark]

On Mon, Oct 17, 2016 at 8:30 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Oct 17, 2016 at 07:59:51AM -0400, Rob Clark wrote:
>> On Mon, Oct 17, 2016 at 2:38 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Fri, Oct 14, 2016 at 07:55:49PM -0400, Rob Clark wrote:
>> >> Sometimes it is nice not to duplicate equivalent printk() and
>> >> seq_printf() code.
>> >>
>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> >
>> > Ok, maybe I should wait for coffee to kick in before commenting on patches
>> > ;-)
>> >
>> > Two bits:
>> > - kerneldoc for this pls, plus pulling into drm-internals.rst.
>> > - Would be awesome to de-dupe the existing dumping code in drm_mm.c with
>> >   this.
>>
>> I have a couple things in drm/msm that I want to de-dup.. and I guess
>> most drivers probably do as well..
>>
>> I'm toying with the idea to convert drm_info_list::show over to take a
>> drm_print ptr instead of seq_file, then we could just re-use the
>> debugfs fxns directly without having extra wrappers fxns.. question is
>> best way to do that in a non-flag-day way..
>
> One idea I had in the other reply is to create a seq_file which can dump
> to debugfs. Not sure how feasible that is ...

that was my first approach..  but seq_file carries a lot of baggage..

BR,
-R

> -Daniel
>
>>
>> BR,
>> -R
>>
>> > -Daniel
>> >
>> >> ---
>> >>  drivers/gpu/drm/Makefile    |  3 ++-
>> >>  drivers/gpu/drm/drm_print.c | 54 +++++++++++++++++++++++++++++++++++++++
>> >>  include/drm/drm_print.h     | 62 +++++++++++++++++++++++++++++++++++++++++++++
>> >>  3 files changed, 118 insertions(+), 1 deletion(-)
>> >>  create mode 100644 drivers/gpu/drm/drm_print.c
>> >>  create mode 100644 include/drm/drm_print.h
>> >>
>> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> >> index e3dba6f..4894ebb 100644
>> >> --- a/drivers/gpu/drm/Makefile
>> >> +++ b/drivers/gpu/drm/Makefile
>> >> @@ -12,7 +12,8 @@ drm-y       :=      drm_auth.o drm_bufs.o drm_cache.o \
>> >>               drm_info.o drm_debugfs.o drm_encoder_slave.o \
>> >>               drm_trace_points.o drm_global.o drm_prime.o \
>> >>               drm_rect.o drm_vma_manager.o drm_flip_work.o \
>> >> -             drm_modeset_lock.o drm_atomic.o drm_bridge.o
>> >> +             drm_modeset_lock.o drm_atomic.o drm_bridge.o \
>> >> +             drm_print.o
>> >>
>> >>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>> >>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>> >> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
>> >> new file mode 100644
>> >> index 0000000..ddcb437
>> >> --- /dev/null
>> >> +++ b/drivers/gpu/drm/drm_print.c
>> >> @@ -0,0 +1,54 @@
>> >> +/*
>> >> + * Copyright (C) 2016 Red Hat
>> >> + *
>> >> + * Permission is hereby granted, free of charge, to any person obtaining a
>> >> + * copy of this software and associated documentation files (the "Software"),
>> >> + * to deal in the Software without restriction, including without limitation
>> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> >> + * and/or sell copies of the Software, and to permit persons to whom the
>> >> + * Software is furnished to do so, subject to the following conditions:
>> >> + *
>> >> + * The above copyright notice and this permission notice shall be included in
>> >> + * all copies or substantial portions of the Software.
>> >> + *
>> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> >> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> >> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> >> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> >> + * OTHER DEALINGS IN THE SOFTWARE.
>> >> + *
>> >> + * Authors:
>> >> + * Rob Clark <robdclark@gmail.com>
>> >> + */
>> >> +
>> >> +#include <linux/seq_file.h>
>> >> +#include <drm/drmP.h>
>> >> +#include <drm/drm_print.h>
>> >> +
>> >> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args)
>> >> +{
>> >> +     seq_vprintf(p->arg, f, args);
>> >> +}
>> >> +
>> >> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args)
>> >> +{
>> >> +     const char *hdr = KERN_INFO "[" DRM_NAME "] ";
>> >> +     char fmt[strlen(hdr) + strlen(f) + 1];
>> >> +
>> >> +     memcpy(fmt, hdr, strlen(hdr));
>> >> +     memcpy(fmt + strlen(hdr), f, strlen(f));
>> >> +     fmt[strlen(hdr) + strlen(f)] = 0;
>> >> +
>> >> +     vprintk(fmt, args);
>> >> +}
>> >> +
>> >> +void drm_printf(struct drm_print *p, const char *f, ...)
>> >> +{
>> >> +     va_list args;
>> >> +
>> >> +     va_start(args, f);
>> >> +     p->printfn(p, f, args);
>> >> +     va_end(args);
>> >> +}
>> >> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
>> >> new file mode 100644
>> >> index 0000000..f294a9a
>> >> --- /dev/null
>> >> +++ b/include/drm/drm_print.h
>> >> @@ -0,0 +1,62 @@
>> >> +/*
>> >> + * Copyright (C) 2016 Red Hat
>> >> + *
>> >> + * Permission is hereby granted, free of charge, to any person obtaining a
>> >> + * copy of this software and associated documentation files (the "Software"),
>> >> + * to deal in the Software without restriction, including without limitation
>> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> >> + * and/or sell copies of the Software, and to permit persons to whom the
>> >> + * Software is furnished to do so, subject to the following conditions:
>> >> + *
>> >> + * The above copyright notice and this permission notice shall be included in
>> >> + * all copies or substantial portions of the Software.
>> >> + *
>> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> >> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> >> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> >> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> >> + * OTHER DEALINGS IN THE SOFTWARE.
>> >> + *
>> >> + * Authors:
>> >> + * Rob Clark <robdclark@gmail.com>
>> >> + */
>> >> +
>> >> +#ifndef DRM_PRINT_H_
>> >> +#define DRM_PRINT_H_
>> >> +
>> >> +#include <stdarg.h>
>> >> +#include <linux/seq_file.h>
>> >> +
>> >> +/* A simple wrapper to abstract seq_file vs printk, so same logging code
>> >> + * does not have to be duplicated.
>> >> + */
>> >> +struct drm_print {
>> >> +     void (*printfn)(struct drm_print *p, const char *f, va_list args);
>> >> +     void *arg;
>> >> +};
>> >> +
>> >> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args);
>> >> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args);
>> >> +
>> >> +void drm_printf(struct drm_print *p, const char *f, ...);
>> >> +
>> >> +static inline struct drm_print drm_print_seq_file(struct seq_file *f)
>> >> +{
>> >> +     struct drm_print p = {
>> >> +             .printfn = __drm_printfn_seq_file,
>> >> +             .arg = f,
>> >> +     };
>> >> +     return p;
>> >> +}
>> >> +
>> >> +static inline struct drm_print drm_print_info(void)
>> >> +{
>> >> +     struct drm_print p = {
>> >> +             .printfn = __drm_printfn_info,
>> >> +     };
>> >> +     return p;
>> >> +}
>> >> +
>> >> +#endif /* DRM_PRINT_H_ */
>> >> --
>> >> 2.7.4
>> >>
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm: helper macros to print composite types

[Sean Paul]

On Fri, Oct 14, 2016 at 7:55 PM, Rob Clark <robdclark@gmail.com> wrote:
> I'll want to print things in a similar way in a later patch.  This will
> make it easier.
>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> TODO drm_rect_debug_print() doesn't have many call sites, and is kind of
> unnecessary now.  Should we just drop it?
>

yeah, seems like you could get the same info from the state dumps.

> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_modes.c |  8 +-------
>  drivers/gpu/drm/drm_rect.c  | 11 ++---------
>  include/drm/drmP.h          | 21 +++++++++++++++++++++
>  3 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index fc5040a..77b0301 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -49,13 +49,7 @@
>   */
>  void drm_mode_debug_printmodeline(const struct drm_display_mode *mode)
>  {
> -       DRM_DEBUG_KMS("Modeline %d:\"%s\" %d %d %d %d %d %d %d %d %d %d "
> -                       "0x%x 0x%x\n",
> -               mode->base.id, mode->name, mode->vrefresh, mode->clock,
> -               mode->hdisplay, mode->hsync_start,
> -               mode->hsync_end, mode->htotal,
> -               mode->vdisplay, mode->vsync_start,
> -               mode->vsync_end, mode->vtotal, mode->type, mode->flags);
> +       DRM_DEBUG_KMS("Modeline " DRM_MODE_FMT "\n", DRM_MODE_ARG(mode));
>  }
>  EXPORT_SYMBOL(drm_mode_debug_printmodeline);
>
> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> index a8e2c86..d451112 100644
> --- a/drivers/gpu/drm/drm_rect.c
> +++ b/drivers/gpu/drm/drm_rect.c
> @@ -281,17 +281,10 @@ EXPORT_SYMBOL(drm_rect_calc_vscale_relaxed);
>   */
>  void drm_rect_debug_print(const char *prefix, const struct drm_rect *r, bool fixed_point)
>  {
> -       int w = drm_rect_width(r);
> -       int h = drm_rect_height(r);
> -
>         if (fixed_point)
> -               DRM_DEBUG_KMS("%s%d.%06ux%d.%06u%+d.%06u%+d.%06u\n", prefix,
> -                             w >> 16, ((w & 0xffff) * 15625) >> 10,
> -                             h >> 16, ((h & 0xffff) * 15625) >> 10,
> -                             r->x1 >> 16, ((r->x1 & 0xffff) * 15625) >> 10,
> -                             r->y1 >> 16, ((r->y1 & 0xffff) * 15625) >> 10);
> +               DRM_DEBUG_KMS("%s" DRM_RECT_FP_FMT "\n", prefix, DRM_RECT_FP_ARG(r));
>         else
> -               DRM_DEBUG_KMS("%s%dx%d%+d%+d\n", prefix, w, h, r->x1, r->y1);
> +               DRM_DEBUG_KMS("%s" DRM_RECT_FMT "\n", prefix, DRM_RECT_ARG(r));
>  }
>  EXPORT_SYMBOL(drm_rect_debug_print);
>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 28d341a..7ffaa35 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -231,6 +231,27 @@ void drm_err(const char *format, ...);
>                         drm_ut_debug_printk(__func__, fmt, ##args);     \
>         } while (0)
>
> +/* Format strings and argument splitters to simplify printing
> + * various "complex" objects
> + */
> +#define DRM_MODE_FMT    "%d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x"
> +#define DRM_MODE_ARG(m) \
> +       (m)->base.id, (m)->name, (m)->vrefresh, (m)->clock, \
> +       (m)->hdisplay, (m)->hsync_start, (m)->hsync_end, (m)->htotal, \
> +       (m)->vdisplay, (m)->vsync_start, (m)->vsync_end, (m)->vtotal, \
> +       (m)->type, (m)->flags
> +
> +#define DRM_RECT_FMT    "%dx%d%+d%+d"
> +#define DRM_RECT_ARG(r) drm_rect_width(r), drm_rect_height(r), (r)->x1, (r)->y1
> +
> +/* for rect's in fixed-point format: */
> +#define DRM_RECT_FP_FMT "%d.%06ux%d.%06u%+d.%06u%+d.%06u"
> +#define DRM_RECT_FP_ARG(r) \
> +               drm_rect_width(r) >> 16, ((drm_rect_width(r) & 0xffff) * 15625) >> 10, \
> +               drm_rect_height(r) >> 16, ((drm_rect_height(r) & 0xffff) * 15625) >> 10, \
> +               (r)->x1 >> 16, (((r)->x1 & 0xffff) * 15625) >> 10, \
> +               (r)->y1 >> 16, (((r)->y1 & 0xffff) * 15625) >> 10
> +
>  /*@}*/
>
>  /***********************************************************************/
> --
> 2.7.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] drm: add helper for printing to log or seq_file

[Sean Paul]

On Fri, Oct 14, 2016 at 7:55 PM, Rob Clark <robdclark@gmail.com> wrote:
> Sometimes it is nice not to duplicate equivalent printk() and
> seq_printf() code.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/Makefile    |  3 ++-
>  drivers/gpu/drm/drm_print.c | 54 +++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_print.h     | 62 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 118 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_print.c
>  create mode 100644 include/drm/drm_print.h
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index e3dba6f..4894ebb 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -12,7 +12,8 @@ drm-y       :=        drm_auth.o drm_bufs.o drm_cache.o \
>                 drm_info.o drm_debugfs.o drm_encoder_slave.o \
>                 drm_trace_points.o drm_global.o drm_prime.o \
>                 drm_rect.o drm_vma_manager.o drm_flip_work.o \
> -               drm_modeset_lock.o drm_atomic.o drm_bridge.o
> +               drm_modeset_lock.o drm_atomic.o drm_bridge.o \
> +               drm_print.o
>
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> new file mode 100644
> index 0000000..ddcb437
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -0,0 +1,54 @@
> +/*
> + * Copyright (C) 2016 Red Hat
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Rob Clark <robdclark@gmail.com>
> + */
> +
> +#include <linux/seq_file.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_print.h>
> +
> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args)
> +{
> +       seq_vprintf(p->arg, f, args);
> +}
> +
> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args)
> +{

I think you can re-use drm_printk here:

drm_printk(KERN_INFO, DRM_UT_NONE, f, args)

> +       const char *hdr = KERN_INFO "[" DRM_NAME "] ";
> +       char fmt[strlen(hdr) + strlen(f) + 1];
> +
> +       memcpy(fmt, hdr, strlen(hdr));
> +       memcpy(fmt + strlen(hdr), f, strlen(f));
> +       fmt[strlen(hdr) + strlen(f)] = 0;
> +
> +       vprintk(fmt, args);
> +}
> +
> +void drm_printf(struct drm_print *p, const char *f, ...)
> +{
> +       va_list args;
> +
> +       va_start(args, f);
> +       p->printfn(p, f, args);
> +       va_end(args);
> +}
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> new file mode 100644
> index 0000000..f294a9a
> --- /dev/null
> +++ b/include/drm/drm_print.h
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright (C) 2016 Red Hat
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Rob Clark <robdclark@gmail.com>
> + */
> +
> +#ifndef DRM_PRINT_H_
> +#define DRM_PRINT_H_
> +
> +#include <stdarg.h>
> +#include <linux/seq_file.h>
> +
> +/* A simple wrapper to abstract seq_file vs printk, so same logging code
> + * does not have to be duplicated.
> + */
> +struct drm_print {
> +       void (*printfn)(struct drm_print *p, const char *f, va_list args);
> +       void *arg;
> +};
> +
> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args);
> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args);
> +
> +void drm_printf(struct drm_print *p, const char *f, ...);
> +

Is it worthwhile making _dev_ variants now?

Sean


> +static inline struct drm_print drm_print_seq_file(struct seq_file *f)
> +{
> +       struct drm_print p = {
> +               .printfn = __drm_printfn_seq_file,
> +               .arg = f,
> +       };
> +       return p;
> +}
> +
> +static inline struct drm_print drm_print_info(void)
> +{
> +       struct drm_print p = {
> +               .printfn = __drm_printfn_info,
> +       };
> +       return p;
> +}
> +
> +#endif /* DRM_PRINT_H_ */
> --
> 2.7.4
>
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 3/6] drm: add helpers to go from plane state to drm_rect

[Sean Paul]

On Fri, Oct 14, 2016 at 7:55 PM, Rob Clark <robdclark@gmail.com> wrote:

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c     | 14 ++------------
>  drivers/gpu/drm/i915/intel_atomic_plane.c   | 10 ++--------
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c    | 15 ++-------------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 10 ++--------
>  include/drm/drm_crtc.h                      | 24 ++++++++++++++++++++++++
>  5 files changed, 32 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 0db36d2..9834fc5 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -73,18 +73,8 @@ static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = {
>  static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
>                                         struct drm_plane_state *plane_state)
>  {
> -       struct drm_rect src = {
> -               .x1 = plane_state->src_x,
> -               .y1 = plane_state->src_y,
> -               .x2 = plane_state->src_x + plane_state->src_w,
> -               .y2 = plane_state->src_y + plane_state->src_h,
> -       };
> -       struct drm_rect dest = {
> -               .x1 = plane_state->crtc_x,
> -               .y1 = plane_state->crtc_y,
> -               .x2 = plane_state->crtc_x + plane_state->crtc_w,
> -               .y2 = plane_state->crtc_y + plane_state->crtc_h,
> -       };
> +       struct drm_rect src = drm_plane_state_src(plane_state);
> +       struct drm_rect dest = drm_plane_state_dest(plane_state);
>         struct drm_rect clip = { 0 };
>         struct drm_simple_display_pipe *pipe;
>         struct drm_crtc_state *crtc_state;
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 7de7721..6682e9b 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -139,14 +139,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>          * we want to keep another copy internal to our driver that we can
>          * clip/modify ourselves.
>          */
> -       intel_state->src.x1 = state->src_x;
> -       intel_state->src.y1 = state->src_y;
> -       intel_state->src.x2 = state->src_x + state->src_w;
> -       intel_state->src.y2 = state->src_y + state->src_h;
> -       intel_state->dst.x1 = state->crtc_x;
> -       intel_state->dst.y1 = state->crtc_y;
> -       intel_state->dst.x2 = state->crtc_x + state->crtc_w;
> -       intel_state->dst.y2 = state->crtc_y + state->crtc_h;
> +       intel_state->src = drm_plane_state_src(state);
> +       intel_state->dst = drm_plane_state_dest(state);
>
>         /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>         intel_state->clip.x1 = 0;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 3995765..2749e74 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -135,19 +135,8 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
>         struct drm_framebuffer *fb = state->fb;
>         struct drm_crtc_state *crtc_state;
>         bool visible;
> -       struct drm_rect dest = {
> -               .x1 = state->crtc_x,
> -               .y1 = state->crtc_y,
> -               .x2 = state->crtc_x + state->crtc_w,
> -               .y2 = state->crtc_y + state->crtc_h,
> -       };
> -       struct drm_rect src = {
> -               /* 16.16 fixed point */
> -               .x1 = state->src_x,
> -               .y1 = state->src_y,
> -               .x2 = state->src_x + state->src_w,
> -               .y2 = state->src_y + state->src_h,
> -       };
> +       struct drm_rect dest = drm_plane_state_dest(state);
> +       struct drm_rect src  = drm_plane_state_src(state);  /* 16.16 fixed point */
>         struct drm_rect clip = { 0, };
>
>         if (!fb)
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 6255e5b..31384ca 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -610,14 +610,8 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
>         if (WARN_ON(!crtc_state))
>                 return -EINVAL;
>
> -       src->x1 = state->src_x;
> -       src->y1 = state->src_y;
> -       src->x2 = state->src_x + state->src_w;
> -       src->y2 = state->src_y + state->src_h;
> -       dest->x1 = state->crtc_x;
> -       dest->y1 = state->crtc_y;
> -       dest->x2 = state->crtc_x + state->crtc_w;
> -       dest->y2 = state->crtc_y + state->crtc_h;
> +       *src  = drm_plane_state_src(state);
> +       *dest = drm_plane_state_dest(state);
>
>         clip.x1 = 0;
>         clip.y1 = 0;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b7d67cc..856fdf8 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -36,6 +36,7 @@
>  #include <uapi/drm/drm_mode.h>
>  #include <uapi/drm/drm_fourcc.h>
>  #include <drm/drm_modeset_lock.h>
> +#include <drm/drm_rect.h>
>
>  struct drm_device;
>  struct drm_mode_set;
> @@ -1395,6 +1396,29 @@ struct drm_plane_state {
>         struct drm_atomic_state *state;
>  };
>
> +static inline struct drm_rect
> +drm_plane_state_src(const struct drm_plane_state *state)
> +{
> +       struct drm_rect src = {
> +               .x1 = state->src_x,
> +               .y1 = state->src_y,
> +               .x2 = state->src_x + state->src_w,
> +               .y2 = state->src_y + state->src_h,
> +       };
> +       return src;
> +}
> +
> +static inline struct drm_rect
> +drm_plane_state_dest(const struct drm_plane_state *state)
> +{
> +       struct drm_rect dest = {
> +               .x1 = state->crtc_x,
> +               .y1 = state->crtc_y,
> +               .x2 = state->crtc_x + state->crtc_w,
> +               .y2 = state->crtc_y + state->crtc_h,
> +       };
> +       return dest;
> +}
>
>  /**
>   * struct drm_plane_funcs - driver plane control functions
> --
> 2.7.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm/atomic: add new drm_debug bit to dump atomic state before commit

[Sean Paul]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/atomic: add debugfs file to dump out atomic state

[Sean Paul]

On Fri, Oct 14, 2016 at 7:55 PM, Rob Clark <robdclark@gmail.com> wrote:

A short commit message detailing the change and info printed would
probably be helpful.


Aside from that,

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic.c  | 40 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_debugfs.c |  9 +++++++++
>  include/drm/drm_atomic.h      |  4 ++++
>  3 files changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3cc3cb5..117d429 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1565,6 +1565,46 @@ static void drm_atomic_print_state(const struct drm_atomic_state *state)
>                 drm_atomic_connector_print_state(&p, connector_state);
>  }
>
> +#ifdef CONFIG_DEBUG_FS
> +static int drm_state_info(struct seq_file *m, void *data)
> +{
> +       struct drm_info_node *node = (struct drm_info_node *) m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct drm_mode_config *config = &dev->mode_config;
> +       struct drm_plane *plane;
> +       struct drm_crtc *crtc;
> +       struct drm_connector *connector;
> +       struct drm_print p = drm_print_seq_file(m);
> +
> +       drm_modeset_lock_all(dev);
> +
> +       list_for_each_entry(plane, &config->plane_list, head)
> +               drm_atomic_plane_print_state(&p, plane->state);
> +
> +       list_for_each_entry(crtc, &config->crtc_list, head)
> +               drm_atomic_crtc_print_state(&p, crtc->state);
> +
> +       list_for_each_entry(connector, &config->connector_list, head)
> +               drm_atomic_connector_print_state(&p, connector->state);
> +
> +       drm_modeset_unlock_all(dev);
> +
> +       return 0;
> +}
> +
> +/* any use in debugfs files to dump individual planes/crtc/etc? */
> +static const struct drm_info_list drm_atomic_debugfs_list[] = {
> +       {"state", drm_state_info, 0},
> +};
> +
> +int drm_atomic_debugfs_init(struct drm_minor *minor)
> +{
> +       return drm_debugfs_create_files(drm_atomic_debugfs_list,
> +                       ARRAY_SIZE(drm_atomic_debugfs_list),
> +                       minor->debugfs_root, minor);
> +}
> +#endif
> +
>  /*
>   * The big monstor ioctl
>   */
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index fa10cef..d8945a9 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -36,6 +36,7 @@
>  #include <linux/export.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_atomic.h>
>  #include "drm_internal.h"
>
>  #if defined(CONFIG_DEBUG_FS)
> @@ -163,6 +164,14 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>                 return ret;
>         }
>
> +       if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> +               ret = drm_atomic_debugfs_init(minor);
> +               if (ret) {
> +                       DRM_ERROR("Failed to create atomic debugfs files\n");
> +                       return ret;
> +               }
> +       }
> +
>         if (dev->driver->debugfs_init) {
>                 ret = dev->driver->debugfs_init(minor);
>                 if (ret) {
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 856a9c8..68422f6 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -181,6 +181,10 @@ int __must_check drm_atomic_check_only(struct drm_atomic_state *state);
>  int __must_check drm_atomic_commit(struct drm_atomic_state *state);
>  int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);
>
> +#ifdef CONFIG_DEBUG_FS
> +int drm_atomic_debugfs_init(struct drm_minor *minor);
> +#endif
> +
>  #define for_each_connector_in_state(__state, connector, connector_state, __i) \
>         for ((__i) = 0;                                                 \
>              (__i) < (__state)->num_connector &&                                \
> --
> 2.7.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm/msm/mdp5: add atomic_print_state support

[Sean Paul]

On Fri, Oct 14, 2016 at 7:55 PM, Rob Clark <robdclark@gmail.com> wrote:
> We subclass drm_plane_state, so add mdp5_plane_atomic_print_state() to
> dump out our own driver specific plane state.
>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h   | 12 ++++++++++++
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 18 +++++++++++++++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> index e4b3fb3..3502711 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> @@ -120,6 +120,18 @@ static inline u32 mdp5_read(struct mdp5_kms *mdp5_kms, u32 reg)
>         return msm_readl(mdp5_kms->mmio + reg);
>  }
>
> +static inline const char *stage2name(enum mdp_mixer_stage_id stage)
> +{
> +       static const char *names[] = {
> +#define NAME(n) [n] = #n
> +               NAME(STAGE_UNUSED), NAME(STAGE_BASE),
> +               NAME(STAGE0), NAME(STAGE1), NAME(STAGE2),
> +               NAME(STAGE3), NAME(STAGE4), NAME(STAGE6),
> +#undef NAME
> +       };
> +       return names[stage];
> +}
> +
>  static inline const char *pipe2name(enum mdp5_pipe pipe)
>  {
>         static const char *names[] = {
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> index b6f1fc66..9f99b4d 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> @@ -16,6 +16,7 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>
> +#include <drm/drm_print.h>
>  #include "mdp5_kms.h"
>
>  struct mdp5_plane {
> @@ -187,6 +188,20 @@ done:
>  #undef SET_PROPERTY
>  }
>
> +static void
> +mdp5_plane_atomic_print_state(struct drm_print *p,
> +               const struct drm_plane_state *state)
> +{
> +       struct mdp5_plane_state *pstate = to_mdp5_plane_state(state);
> +
> +       drm_printf(p, "\tpremultiplied=%u\n", pstate->premultiplied);
> +       drm_printf(p, "\tzpos=%u\n", pstate->zpos);
> +       drm_printf(p, "\talpha=%u\n", pstate->alpha);
> +       drm_printf(p, "\tstage=%s\n", stage2name(pstate->stage));
> +       drm_printf(p, "\tmode_changed=%u\n", pstate->mode_changed);
> +       drm_printf(p, "\tpending=%u\n", pstate->pending);
> +}
> +
>  static void mdp5_plane_reset(struct drm_plane *plane)
>  {
>         struct mdp5_plane_state *mdp5_state;
> @@ -250,6 +265,7 @@ static const struct drm_plane_funcs mdp5_plane_funcs = {
>                 .reset = mdp5_plane_reset,
>                 .atomic_duplicate_state = mdp5_plane_duplicate_state,
>                 .atomic_destroy_state = mdp5_plane_destroy_state,
> +               .atomic_print_state = mdp5_plane_atomic_print_state,
>  };
>
>  static int mdp5_plane_prepare_fb(struct drm_plane *plane,
> @@ -907,7 +923,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
>         type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
>         ret = drm_universal_plane_init(dev, plane, 0xff, &mdp5_plane_funcs,
>                                  mdp5_plane->formats, mdp5_plane->nformats,
> -                                type, NULL);
> +                                type, "%s", mdp5_plane->name);
>         if (ret)
>                 goto fail;
>
> --
> 2.7.4
>
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 2/6] drm: add helper for printing to log or seq_file

[Rob Clark]

On Mon, Oct 17, 2016 at 1:00 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Fri, Oct 14, 2016 at 7:55 PM, Rob Clark <robdclark@gmail.com> wrote:
>> Sometimes it is nice not to duplicate equivalent printk() and
>> seq_printf() code.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>>  drivers/gpu/drm/Makefile    |  3 ++-
>>  drivers/gpu/drm/drm_print.c | 54 +++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_print.h     | 62 +++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 118 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/drm_print.c
>>  create mode 100644 include/drm/drm_print.h
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index e3dba6f..4894ebb 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -12,7 +12,8 @@ drm-y       :=        drm_auth.o drm_bufs.o drm_cache.o \
>>                 drm_info.o drm_debugfs.o drm_encoder_slave.o \
>>                 drm_trace_points.o drm_global.o drm_prime.o \
>>                 drm_rect.o drm_vma_manager.o drm_flip_work.o \
>> -               drm_modeset_lock.o drm_atomic.o drm_bridge.o
>> +               drm_modeset_lock.o drm_atomic.o drm_bridge.o \
>> +               drm_print.o
>>
>>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
>> new file mode 100644
>> index 0000000..ddcb437
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_print.c
>> @@ -0,0 +1,54 @@
>> +/*
>> + * Copyright (C) 2016 Red Hat
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + * Rob Clark <robdclark@gmail.com>
>> + */
>> +
>> +#include <linux/seq_file.h>
>> +#include <drm/drmP.h>
>> +#include <drm/drm_print.h>
>> +
>> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args)
>> +{
>> +       seq_vprintf(p->arg, f, args);
>> +}
>> +
>> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args)
>> +{
>
> I think you can re-use drm_printk here:
>
> drm_printk(KERN_INFO, DRM_UT_NONE, f, args)

fwiw, I looked at drm_printk().. but kinda don't want the fxn name in
the output.  However the va_format trick is neat.  I think I'll use
that plus dev_printk().

BR,
-R

>
>> +       const char *hdr = KERN_INFO "[" DRM_NAME "] ";
>> +       char fmt[strlen(hdr) + strlen(f) + 1];
>> +
>> +       memcpy(fmt, hdr, strlen(hdr));
>> +       memcpy(fmt + strlen(hdr), f, strlen(f));
>> +       fmt[strlen(hdr) + strlen(f)] = 0;
>> +
>> +       vprintk(fmt, args);
>> +}
>> +
>> +void drm_printf(struct drm_print *p, const char *f, ...)
>> +{
>> +       va_list args;
>> +
>> +       va_start(args, f);
>> +       p->printfn(p, f, args);
>> +       va_end(args);
>> +}
>> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
>> new file mode 100644
>> index 0000000..f294a9a
>> --- /dev/null
>> +++ b/include/drm/drm_print.h
>> @@ -0,0 +1,62 @@
>> +/*
>> + * Copyright (C) 2016 Red Hat
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + * Rob Clark <robdclark@gmail.com>
>> + */
>> +
>> +#ifndef DRM_PRINT_H_
>> +#define DRM_PRINT_H_
>> +
>> +#include <stdarg.h>
>> +#include <linux/seq_file.h>
>> +
>> +/* A simple wrapper to abstract seq_file vs printk, so same logging code
>> + * does not have to be duplicated.
>> + */
>> +struct drm_print {
>> +       void (*printfn)(struct drm_print *p, const char *f, va_list args);
>> +       void *arg;
>> +};
>> +
>> +void __drm_printfn_seq_file(struct drm_print *p, const char *f, va_list args);
>> +void __drm_printfn_info(struct drm_print *p, const char *f, va_list args);
>> +
>> +void drm_printf(struct drm_print *p, const char *f, ...);
>> +
>
> Is it worthwhile making _dev_ variants now?
>
> Sean
>
>
>> +static inline struct drm_print drm_print_seq_file(struct seq_file *f)
>> +{
>> +       struct drm_print p = {
>> +               .printfn = __drm_printfn_seq_file,
>> +               .arg = f,
>> +       };
>> +       return p;
>> +}
>> +
>> +static inline struct drm_print drm_print_info(void)
>> +{
>> +       struct drm_print p = {
>> +               .printfn = __drm_printfn_info,
>> +       };
>> +       return p;
>> +}
>> +
>> +#endif /* DRM_PRINT_H_ */
>> --
>> 2.7.4
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel