Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1515809 > unrolled thread

[PATCH v2] drm: move allocation out of drm_get_format_name()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2] drm: move allocation out of drm_get_format_name() Eric Engestrom <eric@engestrom.ch> - 2016-11-07 01:50 +0100
    Re: [PATCH v2] drm: move allocation out of drm_get_format_name() Jani Nikula <jani.nikula@linux.intel.com> - 2016-11-07 09:20 +0100
      Re: [PATCH v2] drm: move allocation out of drm_get_format_name() Eric Engestrom <eric.engestrom@imgtec.com> - 2016-11-07 18:20 +0100
        Re: [PATCH v2] drm: move allocation out of drm_get_format_name() Jani Nikula <jani.nikula@linux.intel.com> - 2016-11-07 18:40 +0100
          Re: [Intel-gfx] [PATCH v2] drm: move allocation out of  drm_get_format_name() Daniel Vetter <daniel@ffwll.ch> - 2016-11-08 11:20 +0100
            [PATCH v3] drm: move allocation out of drm_get_format_name() Eric Engestrom <eric@engestrom.ch> - 2016-11-09 01:20 +0100
              Re: [Intel-gfx] [PATCH v3] drm: move allocation out of  drm_get_format_name() Daniel Vetter <daniel@ffwll.ch> - 2016-11-09 02:10 +0100
                Re: [Intel-gfx] [PATCH v3] drm: move allocation out of  drm_get_format_name() Daniel Vetter <daniel@ffwll.ch> - 2016-11-09 02:20 +0100
                  Re: [Intel-gfx] [PATCH v3] drm: move allocation out of  drm_get_format_name() Eric Engestrom <eric.engestrom@imgtec.com> - 2016-11-09 12:50 +0100
                    Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name() Daniel Vetter <daniel@ffwll.ch> - 2016-11-09 14:20 +0100
                      Re: [Intel-gfx] [PATCH v3] drm: move allocation out of  drm_get_format_name() Eric Engestrom <eric.engestrom@imgtec.com> - 2016-11-09 18:00 +0100
                        Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name() Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-11-10 11:10 +0100
                          Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name() Jani Nikula <jani.nikula@intel.com> - 2016-11-10 11:40 +0100
                            Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name() Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-11-10 12:00 +0100
                              Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name() Jani Nikula <jani.nikula@intel.com> - 2016-11-10 12:10 +0100
                        Re: [Intel-gfx] [PATCH v3] drm: move allocation out of  drm_get_format_name() Daniel Vetter <daniel@ffwll.ch> - 2016-11-11 10:30 +0100
    Re: [PATCH v2] drm: move allocation out of drm_get_format_name() Rob Clark <robdclark@gmail.com> - 2016-11-07 15:50 +0100

#1515809 — [PATCH v2] drm: move allocation out of drm_get_format_name()

Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07

    drm: make drm_get_format_name thread-safe

    Signed-off-by: Eric Engestrom <eric@engestrom.ch>
    [danvet: Clarify that the returned pointer must be freed with
    kfree().]
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cc: Rob Clark <robdclark@gmail.com>
Cc: Christian König <christian.koenig@amd.com>
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Eric Engestrom <eric@engestrom.ch>
---

v2: use single-field struct instead of typedef to let the compiler
    enforce the type (Christian König)

---
 include/drm/drm_fourcc.h                        | 10 +++++-
 drivers/gpu/drm/drm_fourcc.c                    | 14 +++------
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c          |  7 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c          |  7 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c           |  3 +-
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c           |  7 ++---
 drivers/gpu/drm/drm_atomic.c                    |  7 +++--
 drivers/gpu/drm/drm_crtc.c                      |  7 +++--
 drivers/gpu/drm/drm_framebuffer.c               |  7 +++--
 drivers/gpu/drm/drm_modeset_helper.c            |  7 +++--
 drivers/gpu/drm/drm_plane.c                     |  7 +++--
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  7 ++---
 drivers/gpu/drm/i915/i915_debugfs.c             | 10 +++---
 drivers/gpu/drm/i915/intel_atomic_plane.c       |  8 ++---
 drivers/gpu/drm/i915/intel_display.c            | 41 ++++++++++---------------
 drivers/gpu/drm/radeon/atombios_crtc.c          | 14 ++++-----
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c             |  3 +-
 17 files changed, 80 insertions(+), 86 deletions(-)

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index dc0aafa..4b03ca0 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -45,6 +45,14 @@ struct drm_format_info {
 	u8 vsub;
 };
 
+/**
+ * struct drm_format_name_buf - name of a DRM format
+ * @str: string buffer containing the format name
+ */
+struct drm_format_name_buf {
+	char str[32];
+};
+
 const struct drm_format_info *__drm_format_info(u32 format);
 const struct drm_format_info *drm_format_info(u32 format);
 uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
@@ -54,6 +62,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
 int drm_format_vert_chroma_subsampling(uint32_t format);
 int drm_format_plane_width(int width, uint32_t format, int plane);
 int drm_format_plane_height(int height, uint32_t format, int plane);
-char *drm_get_format_name(uint32_t format) __malloc;
+char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
 
 #endif /* __DRM_FOURCC_H__ */
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index cbb8b77..99b0b60 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -79,17 +79,13 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
 EXPORT_SYMBOL(drm_mode_legacy_fb_format);
 
 /**
- * drm_get_format_name - return a string for drm fourcc format
+ * drm_get_format_name - fill a string with a drm fourcc format's name
  * @format: format to compute name of
+ * @buf: caller-supplied buffer
- *
- * Note that the buffer returned by this function is owned by the caller
- * and will need to be freed using kfree().
  */
-char *drm_get_format_name(uint32_t format)
+char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf)
 {
-	char *buf = kmalloc(32, GFP_KERNEL);
-
-	snprintf(buf, 32,
+	snprintf(buf->str, sizeof(buf->str),
 		 "%c%c%c%c %s-endian (0x%08x)",
 		 printable_char(format & 0xff),
 		 printable_char((format >> 8) & 0xff),
@@ -98,7 +94,7 @@ char *drm_get_format_name(uint32_t format)
 		 format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
 		 format);
 
-	return buf;
+	return buf->str;
 }
 EXPORT_SYMBOL(drm_get_format_name);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 199d3f7..2924cdd 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2032,7 +2032,7 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 tmp, viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -2144,9 +2144,8 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
 		bypass_lut = true;
 		break;
 	default:
-		format_name = drm_get_format_name(target_fb->pixel_format);
-		DRM_ERROR("Unsupported screen format %s\n", format_name);
+		DRM_ERROR("Unsupported screen format %s\n",
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index ecd000e..f0e3c5c 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2013,7 +2013,7 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 tmp, viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -2125,9 +2125,8 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
 		bypass_lut = true;
 		break;
 	default:
-		format_name = drm_get_format_name(target_fb->pixel_format);
-		DRM_ERROR("Unsupported screen format %s\n", format_name);
+		DRM_ERROR("Unsupported screen format %s\n",
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 44547f9..5c6b82f 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -1456,6 +1456,7 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -1559,7 +1560,7 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
 		break;
 	default:
 		DRM_ERROR("Unsupported screen format %s\n",
-			  drm_get_format_name(target_fb->pixel_format));
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 979aedf..5aef19d 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -1910,7 +1910,7 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -2015,9 +2015,8 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
 		bypass_lut = true;
 		break;
 	default:
-		format_name = drm_get_format_name(target_fb->pixel_format);
-		DRM_ERROR("Unsupported screen format %s\n", format_name);
+		DRM_ERROR("Unsupported screen format %s\n",
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c32fb3c..f29d3c5 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -835,9 +835,10 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
 	/* Check whether this plane supports the fb pixel format. */
 	ret = drm_plane_check_pixel_format(plane, state->fb->pixel_format);
 	if (ret) {
-		char *format_name = drm_get_format_name(state->fb->pixel_format);
-		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
+		struct drm_format_name_buf format_name;
+		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
+		                 drm_get_format_name(state->fb->pixel_format,
+		                                     &format_name));
-		kfree(format_name);
 		return ret;
 	}
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 13441e2..08ac216 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -827,9 +827,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			ret = drm_plane_check_pixel_format(crtc->primary,
 							   fb->pixel_format);
 			if (ret) {
-				char *format_name = drm_get_format_name(fb->pixel_format);
-				DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
+				struct drm_format_name_buf format_name;
+				DRM_DEBUG_KMS("Invalid pixel format %s\n",
+				              drm_get_format_name(fb->pixel_format,
+				                                  &format_name));
-				kfree(format_name);
 				goto out;
 			}
 		}
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 49fd7db..df6b180 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -133,9 +133,10 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 
 	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
 	if (!info) {
-		char *format_name = drm_get_format_name(r->pixel_format);
-		DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
+		struct drm_format_name_buf format_name;
+		DRM_DEBUG_KMS("bad framebuffer format %s\n",
+		              drm_get_format_name(r->pixel_format,
+		                                  &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
index 2544dfe..2f452b3 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -75,10 +75,11 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
 
 	info = drm_format_info(mode_cmd->pixel_format);
 	if (!info || !info->depth) {
-		char *format_name = drm_get_format_name(mode_cmd->pixel_format);
+		struct drm_format_name_buf format_name;
 
-		DRM_DEBUG_KMS("non-RGB pixel format %s\n", format_name);
+		DRM_DEBUG_KMS("non-RGB pixel format %s\n",
+		              drm_get_format_name(mode_cmd->pixel_format,
+		                                  &format_name));
-		kfree(format_name);
 
 		fb->depth = 0;
 		fb->bits_per_pixel = 0;
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 249c0ae..2ba0c22 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -479,9 +479,10 @@ static int __setplane_internal(struct drm_plane *plane,
 	/* Check whether this plane supports the fb pixel format. */
 	ret = drm_plane_check_pixel_format(plane, fb->pixel_format);
 	if (ret) {
-		char *format_name = drm_get_format_name(fb->pixel_format);
-		DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
+		struct drm_format_name_buf format_name;
+		DRM_DEBUG_KMS("Invalid pixel format %s\n",
+		              drm_get_format_name(fb->pixel_format,
+		                                  &format_name));
-		kfree(format_name);
 		goto out;
 	}
 
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index 7e7a4d4..afc2b5d 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -608,17 +608,16 @@ static void ade_rdma_set(void __iomem *base, struct drm_framebuffer *fb,
 			 u32 ch, u32 y, u32 in_h, u32 fmt)
 {
 	struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, 0);
-	char *format_name;
+	struct drm_format_name_buf format_name;
 	u32 reg_ctrl, reg_addr, reg_size, reg_stride, reg_space, reg_en;
 	u32 stride = fb->pitches[0];
 	u32 addr = (u32)obj->paddr + y * stride;
 
 	DRM_DEBUG_DRIVER("rdma%d: (y=%d, height=%d), stride=%d, paddr=0x%x\n",
 			 ch + 1, y, in_h, stride, (u32)obj->paddr);
-	format_name = drm_get_format_name(fb->pixel_format);
 	DRM_DEBUG_DRIVER("addr=0x%x, fb:%dx%d, pixel_format=%d(%s)\n",
-			 addr, fb->width, fb->height, fmt, format_name);
+			 addr, fb->width, fb->height, fmt,
+			 drm_get_format_name(fb->pixel_format, &format_name));
-	kfree(format_name);
 
 	/* get reg offset */
 	reg_ctrl = RD_CH_CTRL(ch);
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 20638d2..670423e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2971,7 +2971,7 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 		struct drm_plane_state *state;
 		struct drm_plane *plane = &intel_plane->base;
-		char *format_name;
+		struct drm_format_name_buf format_name;
 
 		if (!plane->state) {
 			seq_puts(m, "plane->state is NULL!\n");
@@ -2981,9 +2981,9 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
 		state = plane->state;
 
 		if (state->fb) {
-			format_name = drm_get_format_name(state->fb->pixel_format);
+			drm_get_format_name(state->fb->pixel_format, &format_name);
 		} else {
-			format_name = kstrdup("N/A", GFP_KERNEL);
+			sprintf(format_name.str, "N/A");
 		}
 
 		seq_printf(m, "\t--Plane id %d: type=%s, crtc_pos=%4dx%4d, crtc_size=%4dx%4d, src_pos=%d.%04ux%d.%04u, src_size=%d.%04ux%d.%04u, format=%s, rotation=%s\n",
@@ -2999,10 +2999,8 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
 			   ((state->src_w & 0xffff) * 15625) >> 10,
 			   (state->src_h >> 16),
 			   ((state->src_h & 0xffff) * 15625) >> 10,
-			   format_name,
+			   format_name.str,
 			   plane_rotation(state->rotation));
-
-		kfree(format_name);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index c762ae5..6c76e3d 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -143,7 +143,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
 
 	if (state->fb && drm_rotation_90_or_270(state->rotation)) {
-		char *format_name;
+		struct drm_format_name_buf format_name;
 
 		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
@@ -159,9 +159,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 		switch (state->fb->pixel_format) {
 		case DRM_FORMAT_C8:
 		case DRM_FORMAT_RGB565:
-			format_name = drm_get_format_name(state->fb->pixel_format);
-			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", format_name);
+			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
+			              drm_get_format_name(state->fb->pixel_format,
+			                                  &format_name));
-			kfree(format_name);
 			return -EINVAL;
 
 		default:
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6f8f6ec..8209170 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12871,7 +12871,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 
 	DRM_DEBUG_KMS("planes on this crtc\n");
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
-		char *format_name;
+		struct drm_format_name_buf format_name;
 		intel_plane = to_intel_plane(plane);
 		if (intel_plane->pipe != crtc->pipe)
 			continue;
@@ -12884,12 +12884,11 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 			continue;
 		}
 
-		format_name = drm_get_format_name(fb->pixel_format);
-
 		DRM_DEBUG_KMS("[PLANE:%d:%s] enabled",
 			      plane->base.id, plane->name);
 		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = %s",
-			      fb->base.id, fb->width, fb->height, format_name);
+			      fb->base.id, fb->width, fb->height,
+			      drm_get_format_name(fb->pixel_format, &format_name));
 		DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
 			      state->scaler_id,
 			      state->base.src.x1 >> 16,
@@ -12899,8 +12898,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 			      state->base.dst.x1, state->base.dst.y1,
 			      drm_rect_width(&state->base.dst),
 			      drm_rect_height(&state->base.dst));
-
-		kfree(format_name);
 	}
 }
 
@@ -15749,7 +15746,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
 	unsigned int tiling = i915_gem_object_get_tiling(obj);
 	int ret;
 	u32 pitch_limit, stride_alignment;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
@@ -15840,18 +15837,16 @@ static int intel_framebuffer_init(struct drm_device *dev,
 		break;
 	case DRM_FORMAT_XRGB1555:
 		if (INTEL_INFO(dev)->gen > 3) {
-			format_name = drm_get_format_name(mode_cmd->pixel_format);
-			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+			DRM_DEBUG("unsupported pixel format: %s\n",
+			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			kfree(format_name);
 			return -EINVAL;
 		}
 		break;
 	case DRM_FORMAT_ABGR8888:
 		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
 		    INTEL_INFO(dev)->gen < 9) {
-			format_name = drm_get_format_name(mode_cmd->pixel_format);
-			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+			DRM_DEBUG("unsupported pixel format: %s\n",
+			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			kfree(format_name);
 			return -EINVAL;
 		}
 		break;
@@ -15859,17 +15854,15 @@ static int intel_framebuffer_init(struct drm_device *dev,
 	case DRM_FORMAT_XRGB2101010:
 	case DRM_FORMAT_XBGR2101010:
 		if (INTEL_INFO(dev)->gen < 4) {
-			format_name = drm_get_format_name(mode_cmd->pixel_format);
-			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+			DRM_DEBUG("unsupported pixel format: %s\n",
+			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			kfree(format_name);
 			return -EINVAL;
 		}
 		break;
 	case DRM_FORMAT_ABGR2101010:
 		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
-			format_name = drm_get_format_name(mode_cmd->pixel_format);
-			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+			DRM_DEBUG("unsupported pixel format: %s\n",
+			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			kfree(format_name);
 			return -EINVAL;
 		}
 		break;
@@ -15878,16 +15871,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
 	case DRM_FORMAT_YVYU:
 	case DRM_FORMAT_VYUY:
 		if (INTEL_INFO(dev)->gen < 5) {
-			format_name = drm_get_format_name(mode_cmd->pixel_format);
-			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+			DRM_DEBUG("unsupported pixel format: %s\n",
+			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			kfree(format_name);
 			return -EINVAL;
 		}
 		break;
 	default:
-		format_name = drm_get_format_name(mode_cmd->pixel_format);
-		DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+		DRM_DEBUG("unsupported pixel format: %s\n",
+		          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
index 74f99ba..05f4ebe 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -1156,7 +1156,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 tmp, viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -1260,9 +1260,8 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
 		bypass_lut = true;
 		break;
 	default:
-		format_name = drm_get_format_name(target_fb->pixel_format);
-		DRM_ERROR("Unsupported screen format %s\n", format_name);
+		DRM_ERROR("Unsupported screen format %s\n",
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
@@ -1473,7 +1472,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -1563,9 +1562,8 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
 		bypass_lut = true;
 		break;
 	default:
-		format_name = drm_get_format_name(target_fb->pixel_format);
-		DRM_ERROR("Unsupported screen format %s\n", format_name);
+		DRM_ERROR("Unsupported screen format %s\n",
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index c965514..e3f68cc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -985,8 +985,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct drm_device *dev,
 
 	info = drm_format_info(mode_cmd2->pixel_format);
 	if (!info || !info->depth) {
+		struct drm_format_name_buf format_name;
 		DRM_ERROR("Unsupported framebuffer format %s\n",
-			  drm_get_format_name(mode_cmd2->pixel_format));
+		          drm_get_format_name(mode_cmd2->pixel_format, &format_name));
 		return ERR_PTR(-EINVAL);
 	}
 
-- 
Cheers,
  Eric

[toc] | [next] | [standalone]

#1515898

On Mon, 07 Nov 2016, Eric Engestrom <eric@engestrom.ch> wrote:
> Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
>
>     drm: make drm_get_format_name thread-safe
>
>     Signed-off-by: Eric Engestrom <eric@engestrom.ch>
>     [danvet: Clarify that the returned pointer must be freed with
>     kfree().]
>     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

The Fixes: format is:

Fixes: 90844f00049e ("drm: make drm_get_format_name thread-safe")

But is this a fix, really, or just an improvement? What exactly is the
bug being fixed? The commit message is not sufficient.

> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Christian König <christian.koenig@amd.com>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> ---
>
> v2: use single-field struct instead of typedef to let the compiler
>     enforce the type (Christian König)

Just to make it clear: NAK three times over for v1. This patch is
definitely a better alternative.

>
> ---
>  include/drm/drm_fourcc.h                        | 10 +++++-
>  drivers/gpu/drm/drm_fourcc.c                    | 14 +++------
>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c          |  7 ++---
>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c          |  7 ++---
>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c           |  3 +-
>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c           |  7 ++---
>  drivers/gpu/drm/drm_atomic.c                    |  7 +++--
>  drivers/gpu/drm/drm_crtc.c                      |  7 +++--
>  drivers/gpu/drm/drm_framebuffer.c               |  7 +++--
>  drivers/gpu/drm/drm_modeset_helper.c            |  7 +++--
>  drivers/gpu/drm/drm_plane.c                     |  7 +++--
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  7 ++---
>  drivers/gpu/drm/i915/i915_debugfs.c             | 10 +++---
>  drivers/gpu/drm/i915/intel_atomic_plane.c       |  8 ++---
>  drivers/gpu/drm/i915/intel_display.c            | 41 ++++++++++---------------
>  drivers/gpu/drm/radeon/atombios_crtc.c          | 14 ++++-----
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c             |  3 +-
>  17 files changed, 80 insertions(+), 86 deletions(-)
>
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index dc0aafa..4b03ca0 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -45,6 +45,14 @@ struct drm_format_info {
>  	u8 vsub;
>  };
>  
> +/**
> + * struct drm_format_name_buf - name of a DRM format
> + * @str: string buffer containing the format name
> + */
> +struct drm_format_name_buf {
> +	char str[32];
> +};
> +
>  const struct drm_format_info *__drm_format_info(u32 format);
>  const struct drm_format_info *drm_format_info(u32 format);
>  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
> @@ -54,6 +62,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
>  int drm_format_vert_chroma_subsampling(uint32_t format);
>  int drm_format_plane_width(int width, uint32_t format, int plane);
>  int drm_format_plane_height(int height, uint32_t format, int plane);
> -char *drm_get_format_name(uint32_t format) __malloc;
> +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);

I wonder if it would be better to make that return "const char *". If
the user really wants to look under the hood, there's buf->str. *shrug*

With the commit message improved,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>  
>  #endif /* __DRM_FOURCC_H__ */
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index cbb8b77..99b0b60 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -79,17 +79,13 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
>  EXPORT_SYMBOL(drm_mode_legacy_fb_format);
>  
>  /**
> - * drm_get_format_name - return a string for drm fourcc format
> + * drm_get_format_name - fill a string with a drm fourcc format's name
>   * @format: format to compute name of
> + * @buf: caller-supplied buffer
> - *
> - * Note that the buffer returned by this function is owned by the caller
> - * and will need to be freed using kfree().
>   */
> -char *drm_get_format_name(uint32_t format)
> +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf)
>  {
> -	char *buf = kmalloc(32, GFP_KERNEL);
> -
> -	snprintf(buf, 32,
> +	snprintf(buf->str, sizeof(buf->str),
>  		 "%c%c%c%c %s-endian (0x%08x)",
>  		 printable_char(format & 0xff),
>  		 printable_char((format >> 8) & 0xff),
> @@ -98,7 +94,7 @@ char *drm_get_format_name(uint32_t format)
>  		 format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
>  		 format);
>  
> -	return buf;
> +	return buf->str;
>  }
>  EXPORT_SYMBOL(drm_get_format_name);
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index 199d3f7..2924cdd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2032,7 +2032,7 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 tmp, viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -2144,9 +2144,8 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
>  		bypass_lut = true;
>  		break;
>  	default:
> -		format_name = drm_get_format_name(target_fb->pixel_format);
> -		DRM_ERROR("Unsupported screen format %s\n", format_name);
> +		DRM_ERROR("Unsupported screen format %s\n",
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index ecd000e..f0e3c5c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2013,7 +2013,7 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 tmp, viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -2125,9 +2125,8 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
>  		bypass_lut = true;
>  		break;
>  	default:
> -		format_name = drm_get_format_name(target_fb->pixel_format);
> -		DRM_ERROR("Unsupported screen format %s\n", format_name);
> +		DRM_ERROR("Unsupported screen format %s\n",
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> index 44547f9..5c6b82f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> @@ -1456,6 +1456,7 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -1559,7 +1560,7 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
>  		break;
>  	default:
>  		DRM_ERROR("Unsupported screen format %s\n",
> -			  drm_get_format_name(target_fb->pixel_format));
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> index 979aedf..5aef19d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -1910,7 +1910,7 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -2015,9 +2015,8 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
>  		bypass_lut = true;
>  		break;
>  	default:
> -		format_name = drm_get_format_name(target_fb->pixel_format);
> -		DRM_ERROR("Unsupported screen format %s\n", format_name);
> +		DRM_ERROR("Unsupported screen format %s\n",
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c32fb3c..f29d3c5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -835,9 +835,10 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>  	/* Check whether this plane supports the fb pixel format. */
>  	ret = drm_plane_check_pixel_format(plane, state->fb->pixel_format);
>  	if (ret) {
> -		char *format_name = drm_get_format_name(state->fb->pixel_format);
> -		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
> +		struct drm_format_name_buf format_name;
> +		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
> +		                 drm_get_format_name(state->fb->pixel_format,
> +		                                     &format_name));
> -		kfree(format_name);
>  		return ret;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 13441e2..08ac216 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -827,9 +827,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			ret = drm_plane_check_pixel_format(crtc->primary,
>  							   fb->pixel_format);
>  			if (ret) {
> -				char *format_name = drm_get_format_name(fb->pixel_format);
> -				DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> +				struct drm_format_name_buf format_name;
> +				DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +				              drm_get_format_name(fb->pixel_format,
> +				                                  &format_name));
> -				kfree(format_name);
>  				goto out;
>  			}
>  		}
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 49fd7db..df6b180 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -133,9 +133,10 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>  
>  	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
>  	if (!info) {
> -		char *format_name = drm_get_format_name(r->pixel_format);
> -		DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
> +		struct drm_format_name_buf format_name;
> +		DRM_DEBUG_KMS("bad framebuffer format %s\n",
> +		              drm_get_format_name(r->pixel_format,
> +		                                  &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index 2544dfe..2f452b3 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -75,10 +75,11 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
>  
>  	info = drm_format_info(mode_cmd->pixel_format);
>  	if (!info || !info->depth) {
> -		char *format_name = drm_get_format_name(mode_cmd->pixel_format);
> +		struct drm_format_name_buf format_name;
>  
> -		DRM_DEBUG_KMS("non-RGB pixel format %s\n", format_name);
> +		DRM_DEBUG_KMS("non-RGB pixel format %s\n",
> +		              drm_get_format_name(mode_cmd->pixel_format,
> +		                                  &format_name));
> -		kfree(format_name);
>  
>  		fb->depth = 0;
>  		fb->bits_per_pixel = 0;
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 249c0ae..2ba0c22 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -479,9 +479,10 @@ static int __setplane_internal(struct drm_plane *plane,
>  	/* Check whether this plane supports the fb pixel format. */
>  	ret = drm_plane_check_pixel_format(plane, fb->pixel_format);
>  	if (ret) {
> -		char *format_name = drm_get_format_name(fb->pixel_format);
> -		DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> +		struct drm_format_name_buf format_name;
> +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +		              drm_get_format_name(fb->pixel_format,
> +		                                  &format_name));
> -		kfree(format_name);
>  		goto out;
>  	}
>  
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> index 7e7a4d4..afc2b5d 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> @@ -608,17 +608,16 @@ static void ade_rdma_set(void __iomem *base, struct drm_framebuffer *fb,
>  			 u32 ch, u32 y, u32 in_h, u32 fmt)
>  {
>  	struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, 0);
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  	u32 reg_ctrl, reg_addr, reg_size, reg_stride, reg_space, reg_en;
>  	u32 stride = fb->pitches[0];
>  	u32 addr = (u32)obj->paddr + y * stride;
>  
>  	DRM_DEBUG_DRIVER("rdma%d: (y=%d, height=%d), stride=%d, paddr=0x%x\n",
>  			 ch + 1, y, in_h, stride, (u32)obj->paddr);
> -	format_name = drm_get_format_name(fb->pixel_format);
>  	DRM_DEBUG_DRIVER("addr=0x%x, fb:%dx%d, pixel_format=%d(%s)\n",
> -			 addr, fb->width, fb->height, fmt, format_name);
> +			 addr, fb->width, fb->height, fmt,
> +			 drm_get_format_name(fb->pixel_format, &format_name));
> -	kfree(format_name);
>  
>  	/* get reg offset */
>  	reg_ctrl = RD_CH_CTRL(ch);
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 20638d2..670423e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2971,7 +2971,7 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>  		struct drm_plane_state *state;
>  		struct drm_plane *plane = &intel_plane->base;
> -		char *format_name;
> +		struct drm_format_name_buf format_name;
>  
>  		if (!plane->state) {
>  			seq_puts(m, "plane->state is NULL!\n");
> @@ -2981,9 +2981,9 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
>  		state = plane->state;
>  
>  		if (state->fb) {
> -			format_name = drm_get_format_name(state->fb->pixel_format);
> +			drm_get_format_name(state->fb->pixel_format, &format_name);
>  		} else {
> -			format_name = kstrdup("N/A", GFP_KERNEL);
> +			sprintf(format_name.str, "N/A");
>  		}
>  
>  		seq_printf(m, "\t--Plane id %d: type=%s, crtc_pos=%4dx%4d, crtc_size=%4dx%4d, src_pos=%d.%04ux%d.%04u, src_size=%d.%04ux%d.%04u, format=%s, rotation=%s\n",
> @@ -2999,10 +2999,8 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
>  			   ((state->src_w & 0xffff) * 15625) >> 10,
>  			   (state->src_h >> 16),
>  			   ((state->src_h & 0xffff) * 15625) >> 10,
> -			   format_name,
> +			   format_name.str,
>  			   plane_rotation(state->rotation));
> -
> -		kfree(format_name);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index c762ae5..6c76e3d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -143,7 +143,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>  
>  	if (state->fb && drm_rotation_90_or_270(state->rotation)) {
> -		char *format_name;
> +		struct drm_format_name_buf format_name;
>  
>  		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>  			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> @@ -159,9 +159,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  		switch (state->fb->pixel_format) {
>  		case DRM_FORMAT_C8:
>  		case DRM_FORMAT_RGB565:
> -			format_name = drm_get_format_name(state->fb->pixel_format);
> -			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", format_name);
> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> +			              drm_get_format_name(state->fb->pixel_format,
> +			                                  &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  
>  		default:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6f8f6ec..8209170 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12871,7 +12871,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  
>  	DRM_DEBUG_KMS("planes on this crtc\n");
>  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> -		char *format_name;
> +		struct drm_format_name_buf format_name;
>  		intel_plane = to_intel_plane(plane);
>  		if (intel_plane->pipe != crtc->pipe)
>  			continue;
> @@ -12884,12 +12884,11 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  			continue;
>  		}
>  
> -		format_name = drm_get_format_name(fb->pixel_format);
> -
>  		DRM_DEBUG_KMS("[PLANE:%d:%s] enabled",
>  			      plane->base.id, plane->name);
>  		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = %s",
> -			      fb->base.id, fb->width, fb->height, format_name);
> +			      fb->base.id, fb->width, fb->height,
> +			      drm_get_format_name(fb->pixel_format, &format_name));
>  		DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
>  			      state->scaler_id,
>  			      state->base.src.x1 >> 16,
> @@ -12899,8 +12898,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  			      state->base.dst.x1, state->base.dst.y1,
>  			      drm_rect_width(&state->base.dst),
>  			      drm_rect_height(&state->base.dst));
> -
> -		kfree(format_name);
>  	}
>  }
>  
> @@ -15749,7 +15746,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	unsigned int tiling = i915_gem_object_get_tiling(obj);
>  	int ret;
>  	u32 pitch_limit, stride_alignment;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
> @@ -15840,18 +15837,16 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  		break;
>  	case DRM_FORMAT_XRGB1555:
>  		if (INTEL_INFO(dev)->gen > 3) {
> -			format_name = drm_get_format_name(mode_cmd->pixel_format);
> -			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  		}
>  		break;
>  	case DRM_FORMAT_ABGR8888:
>  		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
>  		    INTEL_INFO(dev)->gen < 9) {
> -			format_name = drm_get_format_name(mode_cmd->pixel_format);
> -			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  		}
>  		break;
> @@ -15859,17 +15854,15 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	case DRM_FORMAT_XRGB2101010:
>  	case DRM_FORMAT_XBGR2101010:
>  		if (INTEL_INFO(dev)->gen < 4) {
> -			format_name = drm_get_format_name(mode_cmd->pixel_format);
> -			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  		}
>  		break;
>  	case DRM_FORMAT_ABGR2101010:
>  		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
> -			format_name = drm_get_format_name(mode_cmd->pixel_format);
> -			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  		}
>  		break;
> @@ -15878,16 +15871,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	case DRM_FORMAT_YVYU:
>  	case DRM_FORMAT_VYUY:
>  		if (INTEL_INFO(dev)->gen < 5) {
> -			format_name = drm_get_format_name(mode_cmd->pixel_format);
> -			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  		}
>  		break;
>  	default:
> -		format_name = drm_get_format_name(mode_cmd->pixel_format);
> -		DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +		DRM_DEBUG("unsupported pixel format: %s\n",
> +		          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
> index 74f99ba..05f4ebe 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -1156,7 +1156,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 tmp, viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -1260,9 +1260,8 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
>  		bypass_lut = true;
>  		break;
>  	default:
> -		format_name = drm_get_format_name(target_fb->pixel_format);
> -		DRM_ERROR("Unsupported screen format %s\n", format_name);
> +		DRM_ERROR("Unsupported screen format %s\n",
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> @@ -1473,7 +1472,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -1563,9 +1562,8 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
>  		bypass_lut = true;
>  		break;
>  	default:
> -		format_name = drm_get_format_name(target_fb->pixel_format);
> -		DRM_ERROR("Unsupported screen format %s\n", format_name);
> +		DRM_ERROR("Unsupported screen format %s\n",
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index c965514..e3f68cc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -985,8 +985,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct drm_device *dev,
>  
>  	info = drm_format_info(mode_cmd2->pixel_format);
>  	if (!info || !info->depth) {
> +		struct drm_format_name_buf format_name;
>  		DRM_ERROR("Unsupported framebuffer format %s\n",
> -			  drm_get_format_name(mode_cmd2->pixel_format));
> +		          drm_get_format_name(mode_cmd2->pixel_format, &format_name));
>  		return ERR_PTR(-EINVAL);
>  	}

-- 
Jani Nikula, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1516346

On Monday, 2016-11-07 10:10:13 +0200, Jani Nikula wrote:
> On Mon, 07 Nov 2016, Eric Engestrom <eric@engestrom.ch> wrote:
> > Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
> >
> >     drm: make drm_get_format_name thread-safe
> >
> >     Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> >     [danvet: Clarify that the returned pointer must be freed with
> >     kfree().]
> >     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> The Fixes: format is:
> 
> Fixes: 90844f00049e ("drm: make drm_get_format_name thread-safe")
> 
> But is this a fix, really, or just an improvement? What exactly is the
> bug being fixed? The commit message is not sufficient.

"The function's behaviour was changed in 90844f00049e, without changing
its signature, causing people to keep using it the old way without
realising they were now leaking memory.
Rob Clark also noticed it was also allocating GFP_KERNEL memory in
atomic contexts, breaking them.

Instead of having to allocate GFP_ATOMIC memory and fixing the callers
to make them cleanup the memory afterwards, let's change the function's
signature by having the caller take care of the memory and passing it to
the function.
The new parameter is a single-field struct in order to enforce the size
of its buffer and help callers to correctly manage their memory."

Does this sound good?

> > @@ -54,6 +62,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
> >  int drm_format_vert_chroma_subsampling(uint32_t format);
> >  int drm_format_plane_width(int width, uint32_t format, int plane);
> >  int drm_format_plane_height(int height, uint32_t format, int plane);
> > -char *drm_get_format_name(uint32_t format) __malloc;
> > +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
> 
> I wonder if it would be better to make that return "const char *". If
> the user really wants to look under the hood, there's buf->str. *shrug*

Good idea, I'll do that in v3 with the proper commit msg and tags. It'll
have to wait another day though, -ENOTIME and all.

> 
> With the commit message improved,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Cheers :)
  Eric

[toc] | [prev] | [next] | [standalone]

#1516374

On Mon, 07 Nov 2016, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> On Monday, 2016-11-07 10:10:13 +0200, Jani Nikula wrote:
>> On Mon, 07 Nov 2016, Eric Engestrom <eric@engestrom.ch> wrote:
>> > Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
>> >
>> >     drm: make drm_get_format_name thread-safe
>> >
>> >     Signed-off-by: Eric Engestrom <eric@engestrom.ch>
>> >     [danvet: Clarify that the returned pointer must be freed with
>> >     kfree().]
>> >     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> 
>> The Fixes: format is:
>> 
>> Fixes: 90844f00049e ("drm: make drm_get_format_name thread-safe")
>> 
>> But is this a fix, really, or just an improvement? What exactly is the
>> bug being fixed? The commit message is not sufficient.
>
> "The function's behaviour was changed in 90844f00049e, without changing
> its signature, causing people to keep using it the old way without
> realising they were now leaking memory.
> Rob Clark also noticed it was also allocating GFP_KERNEL memory in
> atomic contexts, breaking them.
>
> Instead of having to allocate GFP_ATOMIC memory and fixing the callers
> to make them cleanup the memory afterwards, let's change the function's
> signature by having the caller take care of the memory and passing it to
> the function.
> The new parameter is a single-field struct in order to enforce the size
> of its buffer and help callers to correctly manage their memory."
>
> Does this sound good?

It's fine; no need to go overboard. ;)

BR,
Jani.

>
>> > @@ -54,6 +62,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
>> >  int drm_format_vert_chroma_subsampling(uint32_t format);
>> >  int drm_format_plane_width(int width, uint32_t format, int plane);
>> >  int drm_format_plane_height(int height, uint32_t format, int plane);
>> > -char *drm_get_format_name(uint32_t format) __malloc;
>> > +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>> 
>> I wonder if it would be better to make that return "const char *". If
>> the user really wants to look under the hood, there's buf->str. *shrug*
>
> Good idea, I'll do that in v3 with the proper commit msg and tags. It'll
> have to wait another day though, -ENOTIME and all.
>
>> 
>> With the commit message improved,
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
> Cheers :)
>   Eric

-- 
Jani Nikula, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1517029 — Re: [Intel-gfx] [PATCH v2] drm: move allocation out of drm_get_format_name()

On Mon, Nov 07, 2016 at 07:38:25PM +0200, Jani Nikula wrote:
> On Mon, 07 Nov 2016, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> > On Monday, 2016-11-07 10:10:13 +0200, Jani Nikula wrote:
> >> On Mon, 07 Nov 2016, Eric Engestrom <eric@engestrom.ch> wrote:
> >> > Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
> >> >
> >> >     drm: make drm_get_format_name thread-safe
> >> >
> >> >     Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> >> >     [danvet: Clarify that the returned pointer must be freed with
> >> >     kfree().]
> >> >     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> 
> >> The Fixes: format is:
> >> 
> >> Fixes: 90844f00049e ("drm: make drm_get_format_name thread-safe")
> >> 
> >> But is this a fix, really, or just an improvement? What exactly is the
> >> bug being fixed? The commit message is not sufficient.
> >
> > "The function's behaviour was changed in 90844f00049e, without changing
> > its signature, causing people to keep using it the old way without
> > realising they were now leaking memory.
> > Rob Clark also noticed it was also allocating GFP_KERNEL memory in
> > atomic contexts, breaking them.
> >
> > Instead of having to allocate GFP_ATOMIC memory and fixing the callers
> > to make them cleanup the memory afterwards, let's change the function's
> > signature by having the caller take care of the memory and passing it to
> > the function.
> > The new parameter is a single-field struct in order to enforce the size
> > of its buffer and help callers to correctly manage their memory."
> >
> > Does this sound good?
> 
> It's fine; no need to go overboard. ;)

Can you pls resend with that and corrected Fixes and all the acks
collected?
-Daniel

> 
> BR,
> Jani.
> 
> >
> >> > @@ -54,6 +62,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
> >> >  int drm_format_vert_chroma_subsampling(uint32_t format);
> >> >  int drm_format_plane_width(int width, uint32_t format, int plane);
> >> >  int drm_format_plane_height(int height, uint32_t format, int plane);
> >> > -char *drm_get_format_name(uint32_t format) __malloc;
> >> > +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
> >> 
> >> I wonder if it would be better to make that return "const char *". If
> >> the user really wants to look under the hood, there's buf->str. *shrug*
> >
> > Good idea, I'll do that in v3 with the proper commit msg and tags. It'll
> > have to wait another day though, -ENOTIME and all.
> >
> >> 
> >> With the commit message improved,
> >> 
> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> >
> > Cheers :)
> >   Eric
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1517672 — [PATCH v3] drm: move allocation out of drm_get_format_name()

The function's behaviour was changed in 90844f00049e, without changing
its signature, causing people to keep using it the old way without
realising they were now leaking memory.
Rob Clark also noticed it was also allocating GFP_KERNEL memory in
atomic contexts, breaking them.

Instead of having to allocate GFP_ATOMIC memory and fixing the callers
to make them cleanup the memory afterwards, let's change the function's
signature by having the caller take care of the memory and passing it to
the function.
The new parameter is a single-field struct in order to enforce the size
of its buffer and help callers to correctly manage their memory.

Fixes: 90844f00049e ("drm: make drm_get_format_name thread-safe")
Cc: Rob Clark <robdclark@gmail.com>
Cc: Christian König <christian.koenig@amd.com>
Acked-by: Christian König <christian.koenig@amd.com>
Acked-by: Rob Clark <robdclark@gmail.com>
Acked-by: Sinclair Yeh <syeh@vmware.com> (vmwgfx)
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Eric Engestrom <eric@engestrom.ch>
---
v3 - fix "Fixes" tag, replace it with an actual commit message
   - collect ack & r-b

v2 - use single-field struct instead of typedef to let the compiler
     enforce the type (Christian König)
---
 include/drm/drm_fourcc.h                        | 10 +++++-
 drivers/gpu/drm/drm_fourcc.c                    | 14 +++------
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c          |  7 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c          |  7 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c           |  3 +-
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c           |  7 ++---
 drivers/gpu/drm/drm_atomic.c                    |  7 +++--
 drivers/gpu/drm/drm_crtc.c                      |  7 +++--
 drivers/gpu/drm/drm_framebuffer.c               |  7 +++--
 drivers/gpu/drm/drm_modeset_helper.c            |  7 +++--
 drivers/gpu/drm/drm_plane.c                     |  7 +++--
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  7 ++---
 drivers/gpu/drm/i915/i915_debugfs.c             | 10 +++---
 drivers/gpu/drm/i915/intel_atomic_plane.c       |  8 ++---
 drivers/gpu/drm/i915/intel_display.c            | 41 ++++++++++---------------
 drivers/gpu/drm/radeon/atombios_crtc.c          | 14 ++++-----
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c             |  3 +-
 17 files changed, 80 insertions(+), 86 deletions(-)

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index dc0aafa..4b03ca0 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -45,6 +45,14 @@ struct drm_format_info {
 	u8 vsub;
 };
 
+/**
+ * struct drm_format_name_buf - name of a DRM format
+ * @str: string buffer containing the format name
+ */
+struct drm_format_name_buf {
+	char str[32];
+};
+
 const struct drm_format_info *__drm_format_info(u32 format);
 const struct drm_format_info *drm_format_info(u32 format);
 uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
@@ -54,6 +62,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
 int drm_format_vert_chroma_subsampling(uint32_t format);
 int drm_format_plane_width(int width, uint32_t format, int plane);
 int drm_format_plane_height(int height, uint32_t format, int plane);
-char *drm_get_format_name(uint32_t format) __malloc;
+char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
 
 #endif /* __DRM_FOURCC_H__ */
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index cbb8b77..99b0b60 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -79,17 +79,13 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
 EXPORT_SYMBOL(drm_mode_legacy_fb_format);
 
 /**
- * drm_get_format_name - return a string for drm fourcc format
+ * drm_get_format_name - fill a string with a drm fourcc format's name
  * @format: format to compute name of
+ * @buf: caller-supplied buffer
- *
- * Note that the buffer returned by this function is owned by the caller
- * and will need to be freed using kfree().
  */
-char *drm_get_format_name(uint32_t format)
+char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf)
 {
-	char *buf = kmalloc(32, GFP_KERNEL);
-
-	snprintf(buf, 32,
+	snprintf(buf->str, sizeof(buf->str),
 		 "%c%c%c%c %s-endian (0x%08x)",
 		 printable_char(format & 0xff),
 		 printable_char((format >> 8) & 0xff),
@@ -98,7 +94,7 @@ char *drm_get_format_name(uint32_t format)
 		 format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
 		 format);
 
-	return buf;
+	return buf->str;
 }
 EXPORT_SYMBOL(drm_get_format_name);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 199d3f7..2924cdd 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2032,7 +2032,7 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 tmp, viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -2144,9 +2144,8 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
 		bypass_lut = true;
 		break;
 	default:
-		format_name = drm_get_format_name(target_fb->pixel_format);
-		DRM_ERROR("Unsupported screen format %s\n", format_name);
+		DRM_ERROR("Unsupported screen format %s\n",
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index ecd000e..f0e3c5c 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2013,7 +2013,7 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 tmp, viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -2125,9 +2125,8 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
 		bypass_lut = true;
 		break;
 	default:
-		format_name = drm_get_format_name(target_fb->pixel_format);
-		DRM_ERROR("Unsupported screen format %s\n", format_name);
+		DRM_ERROR("Unsupported screen format %s\n",
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 44547f9..5c6b82f 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -1456,6 +1456,7 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -1559,7 +1560,7 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
 		break;
 	default:
 		DRM_ERROR("Unsupported screen format %s\n",
-			  drm_get_format_name(target_fb->pixel_format));
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 979aedf..5aef19d 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -1910,7 +1910,7 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -2015,9 +2015,8 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
 		bypass_lut = true;
 		break;
 	default:
-		format_name = drm_get_format_name(target_fb->pixel_format);
-		DRM_ERROR("Unsupported screen format %s\n", format_name);
+		DRM_ERROR("Unsupported screen format %s\n",
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c32fb3c..f29d3c5 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -835,9 +835,10 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
 	/* Check whether this plane supports the fb pixel format. */
 	ret = drm_plane_check_pixel_format(plane, state->fb->pixel_format);
 	if (ret) {
-		char *format_name = drm_get_format_name(state->fb->pixel_format);
-		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
+		struct drm_format_name_buf format_name;
+		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
+		                 drm_get_format_name(state->fb->pixel_format,
+		                                     &format_name));
-		kfree(format_name);
 		return ret;
 	}
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 13441e2..08ac216 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -827,9 +827,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			ret = drm_plane_check_pixel_format(crtc->primary,
 							   fb->pixel_format);
 			if (ret) {
-				char *format_name = drm_get_format_name(fb->pixel_format);
-				DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
+				struct drm_format_name_buf format_name;
+				DRM_DEBUG_KMS("Invalid pixel format %s\n",
+				              drm_get_format_name(fb->pixel_format,
+				                                  &format_name));
-				kfree(format_name);
 				goto out;
 			}
 		}
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 49fd7db..df6b180 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -133,9 +133,10 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 
 	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
 	if (!info) {
-		char *format_name = drm_get_format_name(r->pixel_format);
-		DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
+		struct drm_format_name_buf format_name;
+		DRM_DEBUG_KMS("bad framebuffer format %s\n",
+		              drm_get_format_name(r->pixel_format,
+		                                  &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
index 2544dfe..2f452b3 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -75,10 +75,11 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
 
 	info = drm_format_info(mode_cmd->pixel_format);
 	if (!info || !info->depth) {
-		char *format_name = drm_get_format_name(mode_cmd->pixel_format);
+		struct drm_format_name_buf format_name;
 
-		DRM_DEBUG_KMS("non-RGB pixel format %s\n", format_name);
+		DRM_DEBUG_KMS("non-RGB pixel format %s\n",
+		              drm_get_format_name(mode_cmd->pixel_format,
+		                                  &format_name));
-		kfree(format_name);
 
 		fb->depth = 0;
 		fb->bits_per_pixel = 0;
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 249c0ae..2ba0c22 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -479,9 +479,10 @@ static int __setplane_internal(struct drm_plane *plane,
 	/* Check whether this plane supports the fb pixel format. */
 	ret = drm_plane_check_pixel_format(plane, fb->pixel_format);
 	if (ret) {
-		char *format_name = drm_get_format_name(fb->pixel_format);
-		DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
+		struct drm_format_name_buf format_name;
+		DRM_DEBUG_KMS("Invalid pixel format %s\n",
+		              drm_get_format_name(fb->pixel_format,
+		                                  &format_name));
-		kfree(format_name);
 		goto out;
 	}
 
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index 7e7a4d4..afc2b5d 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -608,17 +608,16 @@ static void ade_rdma_set(void __iomem *base, struct drm_framebuffer *fb,
 			 u32 ch, u32 y, u32 in_h, u32 fmt)
 {
 	struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, 0);
-	char *format_name;
+	struct drm_format_name_buf format_name;
 	u32 reg_ctrl, reg_addr, reg_size, reg_stride, reg_space, reg_en;
 	u32 stride = fb->pitches[0];
 	u32 addr = (u32)obj->paddr + y * stride;
 
 	DRM_DEBUG_DRIVER("rdma%d: (y=%d, height=%d), stride=%d, paddr=0x%x\n",
 			 ch + 1, y, in_h, stride, (u32)obj->paddr);
-	format_name = drm_get_format_name(fb->pixel_format);
 	DRM_DEBUG_DRIVER("addr=0x%x, fb:%dx%d, pixel_format=%d(%s)\n",
-			 addr, fb->width, fb->height, fmt, format_name);
+			 addr, fb->width, fb->height, fmt,
+			 drm_get_format_name(fb->pixel_format, &format_name));
-	kfree(format_name);
 
 	/* get reg offset */
 	reg_ctrl = RD_CH_CTRL(ch);
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 20638d2..670423e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2971,7 +2971,7 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 		struct drm_plane_state *state;
 		struct drm_plane *plane = &intel_plane->base;
-		char *format_name;
+		struct drm_format_name_buf format_name;
 
 		if (!plane->state) {
 			seq_puts(m, "plane->state is NULL!\n");
@@ -2981,9 +2981,9 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
 		state = plane->state;
 
 		if (state->fb) {
-			format_name = drm_get_format_name(state->fb->pixel_format);
+			drm_get_format_name(state->fb->pixel_format, &format_name);
 		} else {
-			format_name = kstrdup("N/A", GFP_KERNEL);
+			sprintf(format_name.str, "N/A");
 		}
 
 		seq_printf(m, "\t--Plane id %d: type=%s, crtc_pos=%4dx%4d, crtc_size=%4dx%4d, src_pos=%d.%04ux%d.%04u, src_size=%d.%04ux%d.%04u, format=%s, rotation=%s\n",
@@ -2999,10 +2999,8 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
 			   ((state->src_w & 0xffff) * 15625) >> 10,
 			   (state->src_h >> 16),
 			   ((state->src_h & 0xffff) * 15625) >> 10,
-			   format_name,
+			   format_name.str,
 			   plane_rotation(state->rotation));
-
-		kfree(format_name);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index c762ae5..6c76e3d 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -143,7 +143,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
 
 	if (state->fb && drm_rotation_90_or_270(state->rotation)) {
-		char *format_name;
+		struct drm_format_name_buf format_name;
 
 		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
@@ -159,9 +159,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 		switch (state->fb->pixel_format) {
 		case DRM_FORMAT_C8:
 		case DRM_FORMAT_RGB565:
-			format_name = drm_get_format_name(state->fb->pixel_format);
-			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", format_name);
+			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
+			              drm_get_format_name(state->fb->pixel_format,
+			                                  &format_name));
-			kfree(format_name);
 			return -EINVAL;
 
 		default:
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6f8f6ec..8209170 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12871,7 +12871,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 
 	DRM_DEBUG_KMS("planes on this crtc\n");
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
-		char *format_name;
+		struct drm_format_name_buf format_name;
 		intel_plane = to_intel_plane(plane);
 		if (intel_plane->pipe != crtc->pipe)
 			continue;
@@ -12884,12 +12884,11 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 			continue;
 		}
 
-		format_name = drm_get_format_name(fb->pixel_format);
-
 		DRM_DEBUG_KMS("[PLANE:%d:%s] enabled",
 			      plane->base.id, plane->name);
 		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = %s",
-			      fb->base.id, fb->width, fb->height, format_name);
+			      fb->base.id, fb->width, fb->height,
+			      drm_get_format_name(fb->pixel_format, &format_name));
 		DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
 			      state->scaler_id,
 			      state->base.src.x1 >> 16,
@@ -12899,8 +12898,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 			      state->base.dst.x1, state->base.dst.y1,
 			      drm_rect_width(&state->base.dst),
 			      drm_rect_height(&state->base.dst));
-
-		kfree(format_name);
 	}
 }
 
@@ -15749,7 +15746,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
 	unsigned int tiling = i915_gem_object_get_tiling(obj);
 	int ret;
 	u32 pitch_limit, stride_alignment;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
@@ -15840,18 +15837,16 @@ static int intel_framebuffer_init(struct drm_device *dev,
 		break;
 	case DRM_FORMAT_XRGB1555:
 		if (INTEL_INFO(dev)->gen > 3) {
-			format_name = drm_get_format_name(mode_cmd->pixel_format);
-			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+			DRM_DEBUG("unsupported pixel format: %s\n",
+			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			kfree(format_name);
 			return -EINVAL;
 		}
 		break;
 	case DRM_FORMAT_ABGR8888:
 		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
 		    INTEL_INFO(dev)->gen < 9) {
-			format_name = drm_get_format_name(mode_cmd->pixel_format);
-			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+			DRM_DEBUG("unsupported pixel format: %s\n",
+			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			kfree(format_name);
 			return -EINVAL;
 		}
 		break;
@@ -15859,17 +15854,15 @@ static int intel_framebuffer_init(struct drm_device *dev,
 	case DRM_FORMAT_XRGB2101010:
 	case DRM_FORMAT_XBGR2101010:
 		if (INTEL_INFO(dev)->gen < 4) {
-			format_name = drm_get_format_name(mode_cmd->pixel_format);
-			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+			DRM_DEBUG("unsupported pixel format: %s\n",
+			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			kfree(format_name);
 			return -EINVAL;
 		}
 		break;
 	case DRM_FORMAT_ABGR2101010:
 		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
-			format_name = drm_get_format_name(mode_cmd->pixel_format);
-			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+			DRM_DEBUG("unsupported pixel format: %s\n",
+			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			kfree(format_name);
 			return -EINVAL;
 		}
 		break;
@@ -15878,16 +15871,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
 	case DRM_FORMAT_YVYU:
 	case DRM_FORMAT_VYUY:
 		if (INTEL_INFO(dev)->gen < 5) {
-			format_name = drm_get_format_name(mode_cmd->pixel_format);
-			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+			DRM_DEBUG("unsupported pixel format: %s\n",
+			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			kfree(format_name);
 			return -EINVAL;
 		}
 		break;
 	default:
-		format_name = drm_get_format_name(mode_cmd->pixel_format);
-		DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+		DRM_DEBUG("unsupported pixel format: %s\n",
+		          drm_get_format_name(mode_cmd->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
index 74f99ba..05f4ebe 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -1156,7 +1156,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 tmp, viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -1260,9 +1260,8 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
 		bypass_lut = true;
 		break;
 	default:
-		format_name = drm_get_format_name(target_fb->pixel_format);
-		DRM_ERROR("Unsupported screen format %s\n", format_name);
+		DRM_ERROR("Unsupported screen format %s\n",
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
@@ -1473,7 +1472,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
 	u32 viewport_w, viewport_h;
 	int r;
 	bool bypass_lut = false;
-	char *format_name;
+	struct drm_format_name_buf format_name;
 
 	/* no fb bound */
 	if (!atomic && !crtc->primary->fb) {
@@ -1563,9 +1562,8 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
 		bypass_lut = true;
 		break;
 	default:
-		format_name = drm_get_format_name(target_fb->pixel_format);
-		DRM_ERROR("Unsupported screen format %s\n", format_name);
+		DRM_ERROR("Unsupported screen format %s\n",
+		          drm_get_format_name(target_fb->pixel_format, &format_name));
-		kfree(format_name);
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index c965514..e3f68cc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -985,8 +985,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct drm_device *dev,
 
 	info = drm_format_info(mode_cmd2->pixel_format);
 	if (!info || !info->depth) {
+		struct drm_format_name_buf format_name;
 		DRM_ERROR("Unsupported framebuffer format %s\n",
-			  drm_get_format_name(mode_cmd2->pixel_format));
+		          drm_get_format_name(mode_cmd2->pixel_format, &format_name));
 		return ERR_PTR(-EINVAL);
 	}
 
-- 
Cheers,
  Eric

[toc] | [prev] | [next] | [standalone]

#1517701 — Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

On Wed, Nov 09, 2016 at 12:17:52AM +0000, Eric Engestrom wrote:
> The function's behaviour was changed in 90844f00049e, without changing
> its signature, causing people to keep using it the old way without
> realising they were now leaking memory.
> Rob Clark also noticed it was also allocating GFP_KERNEL memory in
> atomic contexts, breaking them.
> 
> Instead of having to allocate GFP_ATOMIC memory and fixing the callers
> to make them cleanup the memory afterwards, let's change the function's
> signature by having the caller take care of the memory and passing it to
> the function.
> The new parameter is a single-field struct in order to enforce the size
> of its buffer and help callers to correctly manage their memory.
> 
> Fixes: 90844f00049e ("drm: make drm_get_format_name thread-safe")
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Christian König <christian.koenig@amd.com>
> Acked-by: Christian König <christian.koenig@amd.com>
> Acked-by: Rob Clark <robdclark@gmail.com>
> Acked-by: Sinclair Yeh <syeh@vmware.com> (vmwgfx)
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> ---
> v3 - fix "Fixes" tag, replace it with an actual commit message
>    - collect ack & r-b
> 
> v2 - use single-field struct instead of typedef to let the compiler
>      enforce the type (Christian König)

Applied to drm-misc, thanks.
-Daniel

> ---
>  include/drm/drm_fourcc.h                        | 10 +++++-
>  drivers/gpu/drm/drm_fourcc.c                    | 14 +++------
>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c          |  7 ++---
>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c          |  7 ++---
>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c           |  3 +-
>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c           |  7 ++---
>  drivers/gpu/drm/drm_atomic.c                    |  7 +++--
>  drivers/gpu/drm/drm_crtc.c                      |  7 +++--
>  drivers/gpu/drm/drm_framebuffer.c               |  7 +++--
>  drivers/gpu/drm/drm_modeset_helper.c            |  7 +++--
>  drivers/gpu/drm/drm_plane.c                     |  7 +++--
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  7 ++---
>  drivers/gpu/drm/i915/i915_debugfs.c             | 10 +++---
>  drivers/gpu/drm/i915/intel_atomic_plane.c       |  8 ++---
>  drivers/gpu/drm/i915/intel_display.c            | 41 ++++++++++---------------
>  drivers/gpu/drm/radeon/atombios_crtc.c          | 14 ++++-----
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c             |  3 +-
>  17 files changed, 80 insertions(+), 86 deletions(-)
> 
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index dc0aafa..4b03ca0 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -45,6 +45,14 @@ struct drm_format_info {
>  	u8 vsub;
>  };
>  
> +/**
> + * struct drm_format_name_buf - name of a DRM format
> + * @str: string buffer containing the format name
> + */
> +struct drm_format_name_buf {
> +	char str[32];
> +};
> +
>  const struct drm_format_info *__drm_format_info(u32 format);
>  const struct drm_format_info *drm_format_info(u32 format);
>  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
> @@ -54,6 +62,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
>  int drm_format_vert_chroma_subsampling(uint32_t format);
>  int drm_format_plane_width(int width, uint32_t format, int plane);
>  int drm_format_plane_height(int height, uint32_t format, int plane);
> -char *drm_get_format_name(uint32_t format) __malloc;
> +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>  
>  #endif /* __DRM_FOURCC_H__ */
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index cbb8b77..99b0b60 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -79,17 +79,13 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
>  EXPORT_SYMBOL(drm_mode_legacy_fb_format);
>  
>  /**
> - * drm_get_format_name - return a string for drm fourcc format
> + * drm_get_format_name - fill a string with a drm fourcc format's name
>   * @format: format to compute name of
> + * @buf: caller-supplied buffer
> - *
> - * Note that the buffer returned by this function is owned by the caller
> - * and will need to be freed using kfree().
>   */
> -char *drm_get_format_name(uint32_t format)
> +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf)
>  {
> -	char *buf = kmalloc(32, GFP_KERNEL);
> -
> -	snprintf(buf, 32,
> +	snprintf(buf->str, sizeof(buf->str),
>  		 "%c%c%c%c %s-endian (0x%08x)",
>  		 printable_char(format & 0xff),
>  		 printable_char((format >> 8) & 0xff),
> @@ -98,7 +94,7 @@ char *drm_get_format_name(uint32_t format)
>  		 format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
>  		 format);
>  
> -	return buf;
> +	return buf->str;
>  }
>  EXPORT_SYMBOL(drm_get_format_name);
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index 199d3f7..2924cdd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2032,7 +2032,7 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 tmp, viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -2144,9 +2144,8 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
>  		bypass_lut = true;
>  		break;
>  	default:
> -		format_name = drm_get_format_name(target_fb->pixel_format);
> -		DRM_ERROR("Unsupported screen format %s\n", format_name);
> +		DRM_ERROR("Unsupported screen format %s\n",
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index ecd000e..f0e3c5c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2013,7 +2013,7 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 tmp, viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -2125,9 +2125,8 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
>  		bypass_lut = true;
>  		break;
>  	default:
> -		format_name = drm_get_format_name(target_fb->pixel_format);
> -		DRM_ERROR("Unsupported screen format %s\n", format_name);
> +		DRM_ERROR("Unsupported screen format %s\n",
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> index 44547f9..5c6b82f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> @@ -1456,6 +1456,7 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -1559,7 +1560,7 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
>  		break;
>  	default:
>  		DRM_ERROR("Unsupported screen format %s\n",
> -			  drm_get_format_name(target_fb->pixel_format));
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> index 979aedf..5aef19d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -1910,7 +1910,7 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -2015,9 +2015,8 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
>  		bypass_lut = true;
>  		break;
>  	default:
> -		format_name = drm_get_format_name(target_fb->pixel_format);
> -		DRM_ERROR("Unsupported screen format %s\n", format_name);
> +		DRM_ERROR("Unsupported screen format %s\n",
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c32fb3c..f29d3c5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -835,9 +835,10 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>  	/* Check whether this plane supports the fb pixel format. */
>  	ret = drm_plane_check_pixel_format(plane, state->fb->pixel_format);
>  	if (ret) {
> -		char *format_name = drm_get_format_name(state->fb->pixel_format);
> -		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
> +		struct drm_format_name_buf format_name;
> +		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
> +		                 drm_get_format_name(state->fb->pixel_format,
> +		                                     &format_name));
> -		kfree(format_name);
>  		return ret;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 13441e2..08ac216 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -827,9 +827,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			ret = drm_plane_check_pixel_format(crtc->primary,
>  							   fb->pixel_format);
>  			if (ret) {
> -				char *format_name = drm_get_format_name(fb->pixel_format);
> -				DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> +				struct drm_format_name_buf format_name;
> +				DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +				              drm_get_format_name(fb->pixel_format,
> +				                                  &format_name));
> -				kfree(format_name);
>  				goto out;
>  			}
>  		}
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 49fd7db..df6b180 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -133,9 +133,10 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>  
>  	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
>  	if (!info) {
> -		char *format_name = drm_get_format_name(r->pixel_format);
> -		DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
> +		struct drm_format_name_buf format_name;
> +		DRM_DEBUG_KMS("bad framebuffer format %s\n",
> +		              drm_get_format_name(r->pixel_format,
> +		                                  &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index 2544dfe..2f452b3 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -75,10 +75,11 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
>  
>  	info = drm_format_info(mode_cmd->pixel_format);
>  	if (!info || !info->depth) {
> -		char *format_name = drm_get_format_name(mode_cmd->pixel_format);
> +		struct drm_format_name_buf format_name;
>  
> -		DRM_DEBUG_KMS("non-RGB pixel format %s\n", format_name);
> +		DRM_DEBUG_KMS("non-RGB pixel format %s\n",
> +		              drm_get_format_name(mode_cmd->pixel_format,
> +		                                  &format_name));
> -		kfree(format_name);
>  
>  		fb->depth = 0;
>  		fb->bits_per_pixel = 0;
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 249c0ae..2ba0c22 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -479,9 +479,10 @@ static int __setplane_internal(struct drm_plane *plane,
>  	/* Check whether this plane supports the fb pixel format. */
>  	ret = drm_plane_check_pixel_format(plane, fb->pixel_format);
>  	if (ret) {
> -		char *format_name = drm_get_format_name(fb->pixel_format);
> -		DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> +		struct drm_format_name_buf format_name;
> +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +		              drm_get_format_name(fb->pixel_format,
> +		                                  &format_name));
> -		kfree(format_name);
>  		goto out;
>  	}
>  
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> index 7e7a4d4..afc2b5d 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> @@ -608,17 +608,16 @@ static void ade_rdma_set(void __iomem *base, struct drm_framebuffer *fb,
>  			 u32 ch, u32 y, u32 in_h, u32 fmt)
>  {
>  	struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, 0);
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  	u32 reg_ctrl, reg_addr, reg_size, reg_stride, reg_space, reg_en;
>  	u32 stride = fb->pitches[0];
>  	u32 addr = (u32)obj->paddr + y * stride;
>  
>  	DRM_DEBUG_DRIVER("rdma%d: (y=%d, height=%d), stride=%d, paddr=0x%x\n",
>  			 ch + 1, y, in_h, stride, (u32)obj->paddr);
> -	format_name = drm_get_format_name(fb->pixel_format);
>  	DRM_DEBUG_DRIVER("addr=0x%x, fb:%dx%d, pixel_format=%d(%s)\n",
> -			 addr, fb->width, fb->height, fmt, format_name);
> +			 addr, fb->width, fb->height, fmt,
> +			 drm_get_format_name(fb->pixel_format, &format_name));
> -	kfree(format_name);
>  
>  	/* get reg offset */
>  	reg_ctrl = RD_CH_CTRL(ch);
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 20638d2..670423e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2971,7 +2971,7 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>  		struct drm_plane_state *state;
>  		struct drm_plane *plane = &intel_plane->base;
> -		char *format_name;
> +		struct drm_format_name_buf format_name;
>  
>  		if (!plane->state) {
>  			seq_puts(m, "plane->state is NULL!\n");
> @@ -2981,9 +2981,9 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
>  		state = plane->state;
>  
>  		if (state->fb) {
> -			format_name = drm_get_format_name(state->fb->pixel_format);
> +			drm_get_format_name(state->fb->pixel_format, &format_name);
>  		} else {
> -			format_name = kstrdup("N/A", GFP_KERNEL);
> +			sprintf(format_name.str, "N/A");
>  		}
>  
>  		seq_printf(m, "\t--Plane id %d: type=%s, crtc_pos=%4dx%4d, crtc_size=%4dx%4d, src_pos=%d.%04ux%d.%04u, src_size=%d.%04ux%d.%04u, format=%s, rotation=%s\n",
> @@ -2999,10 +2999,8 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
>  			   ((state->src_w & 0xffff) * 15625) >> 10,
>  			   (state->src_h >> 16),
>  			   ((state->src_h & 0xffff) * 15625) >> 10,
> -			   format_name,
> +			   format_name.str,
>  			   plane_rotation(state->rotation));
> -
> -		kfree(format_name);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index c762ae5..6c76e3d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -143,7 +143,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>  
>  	if (state->fb && drm_rotation_90_or_270(state->rotation)) {
> -		char *format_name;
> +		struct drm_format_name_buf format_name;
>  
>  		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>  			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> @@ -159,9 +159,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  		switch (state->fb->pixel_format) {
>  		case DRM_FORMAT_C8:
>  		case DRM_FORMAT_RGB565:
> -			format_name = drm_get_format_name(state->fb->pixel_format);
> -			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", format_name);
> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> +			              drm_get_format_name(state->fb->pixel_format,
> +			                                  &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  
>  		default:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6f8f6ec..8209170 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12871,7 +12871,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  
>  	DRM_DEBUG_KMS("planes on this crtc\n");
>  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> -		char *format_name;
> +		struct drm_format_name_buf format_name;
>  		intel_plane = to_intel_plane(plane);
>  		if (intel_plane->pipe != crtc->pipe)
>  			continue;
> @@ -12884,12 +12884,11 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  			continue;
>  		}
>  
> -		format_name = drm_get_format_name(fb->pixel_format);
> -
>  		DRM_DEBUG_KMS("[PLANE:%d:%s] enabled",
>  			      plane->base.id, plane->name);
>  		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = %s",
> -			      fb->base.id, fb->width, fb->height, format_name);
> +			      fb->base.id, fb->width, fb->height,
> +			      drm_get_format_name(fb->pixel_format, &format_name));
>  		DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
>  			      state->scaler_id,
>  			      state->base.src.x1 >> 16,
> @@ -12899,8 +12898,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  			      state->base.dst.x1, state->base.dst.y1,
>  			      drm_rect_width(&state->base.dst),
>  			      drm_rect_height(&state->base.dst));
> -
> -		kfree(format_name);
>  	}
>  }
>  
> @@ -15749,7 +15746,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	unsigned int tiling = i915_gem_object_get_tiling(obj);
>  	int ret;
>  	u32 pitch_limit, stride_alignment;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
> @@ -15840,18 +15837,16 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  		break;
>  	case DRM_FORMAT_XRGB1555:
>  		if (INTEL_INFO(dev)->gen > 3) {
> -			format_name = drm_get_format_name(mode_cmd->pixel_format);
> -			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  		}
>  		break;
>  	case DRM_FORMAT_ABGR8888:
>  		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
>  		    INTEL_INFO(dev)->gen < 9) {
> -			format_name = drm_get_format_name(mode_cmd->pixel_format);
> -			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  		}
>  		break;
> @@ -15859,17 +15854,15 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	case DRM_FORMAT_XRGB2101010:
>  	case DRM_FORMAT_XBGR2101010:
>  		if (INTEL_INFO(dev)->gen < 4) {
> -			format_name = drm_get_format_name(mode_cmd->pixel_format);
> -			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  		}
>  		break;
>  	case DRM_FORMAT_ABGR2101010:
>  		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
> -			format_name = drm_get_format_name(mode_cmd->pixel_format);
> -			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  		}
>  		break;
> @@ -15878,16 +15871,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	case DRM_FORMAT_YVYU:
>  	case DRM_FORMAT_VYUY:
>  		if (INTEL_INFO(dev)->gen < 5) {
> -			format_name = drm_get_format_name(mode_cmd->pixel_format);
> -			DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +			          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -			kfree(format_name);
>  			return -EINVAL;
>  		}
>  		break;
>  	default:
> -		format_name = drm_get_format_name(mode_cmd->pixel_format);
> -		DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +		DRM_DEBUG("unsupported pixel format: %s\n",
> +		          drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
> index 74f99ba..05f4ebe 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -1156,7 +1156,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 tmp, viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -1260,9 +1260,8 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
>  		bypass_lut = true;
>  		break;
>  	default:
> -		format_name = drm_get_format_name(target_fb->pixel_format);
> -		DRM_ERROR("Unsupported screen format %s\n", format_name);
> +		DRM_ERROR("Unsupported screen format %s\n",
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> @@ -1473,7 +1472,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
>  	u32 viewport_w, viewport_h;
>  	int r;
>  	bool bypass_lut = false;
> -	char *format_name;
> +	struct drm_format_name_buf format_name;
>  
>  	/* no fb bound */
>  	if (!atomic && !crtc->primary->fb) {
> @@ -1563,9 +1562,8 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
>  		bypass_lut = true;
>  		break;
>  	default:
> -		format_name = drm_get_format_name(target_fb->pixel_format);
> -		DRM_ERROR("Unsupported screen format %s\n", format_name);
> +		DRM_ERROR("Unsupported screen format %s\n",
> +		          drm_get_format_name(target_fb->pixel_format, &format_name));
> -		kfree(format_name);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index c965514..e3f68cc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -985,8 +985,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct drm_device *dev,
>  
>  	info = drm_format_info(mode_cmd2->pixel_format);
>  	if (!info || !info->depth) {
> +		struct drm_format_name_buf format_name;
>  		DRM_ERROR("Unsupported framebuffer format %s\n",
> -			  drm_get_format_name(mode_cmd2->pixel_format));
> +		          drm_get_format_name(mode_cmd2->pixel_format, &format_name));
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -- 
> Cheers,
>   Eric
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1517706 — Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

On Wed, Nov 09, 2016 at 02:09:16AM +0100, Daniel Vetter wrote:
> On Wed, Nov 09, 2016 at 12:17:52AM +0000, Eric Engestrom wrote:
> > The function's behaviour was changed in 90844f00049e, without changing
> > its signature, causing people to keep using it the old way without
> > realising they were now leaking memory.
> > Rob Clark also noticed it was also allocating GFP_KERNEL memory in
> > atomic contexts, breaking them.
> > 
> > Instead of having to allocate GFP_ATOMIC memory and fixing the callers
> > to make them cleanup the memory afterwards, let's change the function's
> > signature by having the caller take care of the memory and passing it to
> > the function.
> > The new parameter is a single-field struct in order to enforce the size
> > of its buffer and help callers to correctly manage their memory.
> > 
> > Fixes: 90844f00049e ("drm: make drm_get_format_name thread-safe")
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Acked-by: Christian König <christian.koenig@amd.com>
> > Acked-by: Rob Clark <robdclark@gmail.com>
> > Acked-by: Sinclair Yeh <syeh@vmware.com> (vmwgfx)
> > Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> > ---
> > v3 - fix "Fixes" tag, replace it with an actual commit message
> >    - collect ack & r-b
> > 
> > v2 - use single-field struct instead of typedef to let the compiler
> >      enforce the type (Christian König)
> 
> Applied to drm-misc, thanks.

Well, had to drop it again since it didn't compile:


  CC [M]  drivers/gpu/drm/drm_blend.o
drivers/gpu/drm/drm_atomic.c: In function ‘drm_atomic_plane_print_state’:
drivers/gpu/drm/drm_atomic.c:920:5: error: too few arguments to function ‘drm_get_format_name’
     drm_get_format_name(fb->pixel_format));
     ^~~~~~~~~~~~~~~~~~~
In file included from ./include/drm/drmP.h:71:0,
                 from drivers/gpu/drm/drm_atomic.c:29:
./include/drm/drm_fourcc.h:65:7: note: declared here
 char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
       ^~~~~~~~~~~~~~~~~~~

Can you pls rebase onto drm-misc or linux-next or something?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1518086 — Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

On Wednesday, 2016-11-09 02:13:25 +0100, Daniel Vetter wrote:
> On Wed, Nov 09, 2016 at 02:09:16AM +0100, Daniel Vetter wrote:
> > On Wed, Nov 09, 2016 at 12:17:52AM +0000, Eric Engestrom wrote:
> > > The function's behaviour was changed in 90844f00049e, without changing
> > > its signature, causing people to keep using it the old way without
> > > realising they were now leaking memory.
> > > Rob Clark also noticed it was also allocating GFP_KERNEL memory in
> > > atomic contexts, breaking them.
> > > 
> > > Instead of having to allocate GFP_ATOMIC memory and fixing the callers
> > > to make them cleanup the memory afterwards, let's change the function's
> > > signature by having the caller take care of the memory and passing it to
> > > the function.
> > > The new parameter is a single-field struct in order to enforce the size
> > > of its buffer and help callers to correctly manage their memory.
> > > 
> > > Fixes: 90844f00049e ("drm: make drm_get_format_name thread-safe")
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Acked-by: Christian König <christian.koenig@amd.com>
> > > Acked-by: Rob Clark <robdclark@gmail.com>
> > > Acked-by: Sinclair Yeh <syeh@vmware.com> (vmwgfx)
> > > Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> > > ---
> > > v3 - fix "Fixes" tag, replace it with an actual commit message
> > >    - collect ack & r-b
> > > 
> > > v2 - use single-field struct instead of typedef to let the compiler
> > >      enforce the type (Christian König)
> > 
> > Applied to drm-misc, thanks.
> 
> Well, had to drop it again since it didn't compile:
> 
> 
>   CC [M]  drivers/gpu/drm/drm_blend.o
> drivers/gpu/drm/drm_atomic.c: In function ‘drm_atomic_plane_print_state’:
> drivers/gpu/drm/drm_atomic.c:920:5: error: too few arguments to function ‘drm_get_format_name’
>      drm_get_format_name(fb->pixel_format));
>      ^~~~~~~~~~~~~~~~~~~
> In file included from ./include/drm/drmP.h:71:0,
>                  from drivers/gpu/drm/drm_atomic.c:29:
> ./include/drm/drm_fourcc.h:65:7: note: declared here
>  char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>        ^~~~~~~~~~~~~~~~~~~
> 
> Can you pls rebase onto drm-misc or linux-next or something?

That was based on airlied/drm-next (last fetched on Sunday I think),
I can rebase it on drm-misc if it helps, but it seems older than
drm-next.
Should I just rebase on top of current head of drm-next?

[toc] | [prev] | [next] | [standalone]

#1518139 — Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

On Wed, Nov 9, 2016 at 12:42 PM, Eric Engestrom
<eric.engestrom@imgtec.com> wrote:
>> Well, had to drop it again since it didn't compile:
>>
>>
>>   CC [M]  drivers/gpu/drm/drm_blend.o
>> drivers/gpu/drm/drm_atomic.c: In function ‘drm_atomic_plane_print_state’:
>> drivers/gpu/drm/drm_atomic.c:920:5: error: too few arguments to function ‘drm_get_format_name’
>>      drm_get_format_name(fb->pixel_format));
>>      ^~~~~~~~~~~~~~~~~~~
>> In file included from ./include/drm/drmP.h:71:0,
>>                  from drivers/gpu/drm/drm_atomic.c:29:
>> ./include/drm/drm_fourcc.h:65:7: note: declared here
>>  char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>>        ^~~~~~~~~~~~~~~~~~~
>>
>> Can you pls rebase onto drm-misc or linux-next or something?
>
> That was based on airlied/drm-next (last fetched on Sunday I think),
> I can rebase it on drm-misc if it helps, but it seems older than
> drm-next.
> Should I just rebase on top of current head of drm-next?

It needs to be drm-misc (linux-next doesn't have it yet) due to the
new atomic debug work that we just landed. I'm working on drm-tip as a
drm local integration tree to ease pains like these a bit, but that
doesn't really exist yet.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1518310 — Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

On Wednesday, 2016-11-09 14:13:40 +0100, Daniel Vetter wrote:
> On Wed, Nov 9, 2016 at 12:42 PM, Eric Engestrom
> <eric.engestrom@imgtec.com> wrote:
> >> Well, had to drop it again since it didn't compile:
> >>
> >>
> >>   CC [M]  drivers/gpu/drm/drm_blend.o
> >> drivers/gpu/drm/drm_atomic.c: In function ‘drm_atomic_plane_print_state’:
> >> drivers/gpu/drm/drm_atomic.c:920:5: error: too few arguments to function ‘drm_get_format_name’
> >>      drm_get_format_name(fb->pixel_format));
> >>      ^~~~~~~~~~~~~~~~~~~
> >> In file included from ./include/drm/drmP.h:71:0,
> >>                  from drivers/gpu/drm/drm_atomic.c:29:
> >> ./include/drm/drm_fourcc.h:65:7: note: declared here
> >>  char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
> >>        ^~~~~~~~~~~~~~~~~~~
> >>
> >> Can you pls rebase onto drm-misc or linux-next or something?
> >
> > That was based on airlied/drm-next (last fetched on Sunday I think),
> > I can rebase it on drm-misc if it helps, but it seems older than
> > drm-next.
> > Should I just rebase on top of current head of drm-next?
> 
> It needs to be drm-misc (linux-next doesn't have it yet) due to the
> new atomic debug work that we just landed. I'm working on drm-tip as a
> drm local integration tree to ease pains like these a bit, but that
> doesn't really exist yet.

I'm confused as to how the different trees and branches merge back to
Torvalds' tree (I'm interested in particular in drm), and I'm not sure
which branch you want me to rebase on in the drm-misc tree [1],
especially since all of them are older than drm-next [2].

I'll try to rebase on drm-misc-fixes (currently at 4da5caa6a6f82cda3193)
as it sounds about right, but it doesn't apply at all, so it'll take
a little while.

Could you give me a quick explanation or point me to a doc/page that
explains how the various trees and branches get merged?
I googled a bit and found this doc [4] by Jani, but it doesn't mention
drm-misc for instance, so I'm not sure how up-to-date and
non-intel-specific it is.

Looking at this page, something just occurred to me: did you mean
drm-fixes [3], instead of one of the branches on drm-misc?

Cheers,
  Eric

[1] git://anongit.freedesktop.org/drm/drm-misc
[2] git://people.freedesktop.org/~airlied/linux drm-next
[2] git://people.freedesktop.org/~airlied/linux drm-fixes
[3] https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-intel.html

[toc] | [prev] | [next] | [standalone]

#1518827 — Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

Hi Eric,

On Wednesday 09 Nov 2016 16:59:31 Eric Engestrom wrote:
> On Wednesday, 2016-11-09 14:13:40 +0100, Daniel Vetter wrote:
> > On Wed, Nov 9, 2016 at 12:42 PM, Eric Engestrom wrote:
> > >> Well, had to drop it again since it didn't compile:
> > >>   CC [M]  drivers/gpu/drm/drm_blend.o
> > >> 
> > >> drivers/gpu/drm/drm_atomic.c: In function
> > >> ‘drm_atomic_plane_print_state’:
> > >> drivers/gpu/drm/drm_atomic.c:920:5: error: too few arguments to
> > >> function ‘drm_get_format_name’> >> 
> > >>      drm_get_format_name(fb->pixel_format));
> > >>      ^~~~~~~~~~~~~~~~~~~
> > >> 
> > >> In file included from ./include/drm/drmP.h:71:0,
> > >> 
> > >>                  from drivers/gpu/drm/drm_atomic.c:29:
> > >> ./include/drm/drm_fourcc.h:65:7: note: declared here
> > >> 
> > >>  char *drm_get_format_name(uint32_t format, struct drm_format_name_buf
> > >>  *buf);> >>  
> > >>        ^~~~~~~~~~~~~~~~~~~
> > >> 
> > >> Can you pls rebase onto drm-misc or linux-next or something?
> > > 
> > > That was based on airlied/drm-next (last fetched on Sunday I think),
> > > I can rebase it on drm-misc if it helps, but it seems older than
> > > drm-next. Should I just rebase on top of current head of drm-next?
> > 
> > It needs to be drm-misc (linux-next doesn't have it yet) due to the
> > new atomic debug work that we just landed. I'm working on drm-tip as a
> > drm local integration tree to ease pains like these a bit, but that
> > doesn't really exist yet.
> 
> I'm confused as to how the different trees and branches merge back to
> Torvalds' tree (I'm interested in particular in drm), and I'm not sure
> which branch you want me to rebase on in the drm-misc tree [1],
> especially since all of them are older than drm-next [2].
> 
> I'll try to rebase on drm-misc-fixes (currently at 4da5caa6a6f82cda3193)
> as it sounds about right, but it doesn't apply at all, so it'll take
> a little while.

While at it, could you make the function return a const char * ?

By the way, while this is an improvement over the current situation in that it 
fixes the missing kfree() issue, I wonder whether the problem we're trying to 
solve should be addressed at a more global level.

The issue here is that printk can't format the fourcc as a string by itself. 
There's a bunch of places in the kernel where a similar formatting problem 
occurs. In a few occasions it has been solved by extending printk with 
additional format specifiers (such as for MAC/IP addresses, GUIDs, various 
kind of device names, ...). DRM fourccs are probably too DRM specific to be 
worth a format specifier, but I wonder whether we could introduce a new 
specifier that takes a function pointer as a formatting helper. Another 
similarly crazy option would be a format specifier for strings that would free 
the passed pointer after printing it.

> Could you give me a quick explanation or point me to a doc/page that
> explains how the various trees and branches get merged?
> I googled a bit and found this doc [4] by Jani, but it doesn't mention
> drm-misc for instance, so I'm not sure how up-to-date and
> non-intel-specific it is.
> 
> Looking at this page, something just occurred to me: did you mean
> drm-fixes [3], instead of one of the branches on drm-misc?
> 
> Cheers,
>   Eric
> 
> [1] git://anongit.freedesktop.org/drm/drm-misc
> [2] git://people.freedesktop.org/~airlied/linux drm-next
> [2] git://people.freedesktop.org/~airlied/linux drm-fixes
> [3] https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-intel.html

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1518839 — Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

On Thu, 10 Nov 2016, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> Hi Eric,
>
> On Wednesday 09 Nov 2016 16:59:31 Eric Engestrom wrote:
>> On Wednesday, 2016-11-09 14:13:40 +0100, Daniel Vetter wrote:
>> > On Wed, Nov 9, 2016 at 12:42 PM, Eric Engestrom wrote:
>> > >> Well, had to drop it again since it didn't compile:
>> > >>   CC [M]  drivers/gpu/drm/drm_blend.o
>> > >> 
>> > >> drivers/gpu/drm/drm_atomic.c: In function
>> > >> ‘drm_atomic_plane_print_state’:
>> > >> drivers/gpu/drm/drm_atomic.c:920:5: error: too few arguments to
>> > >> function ‘drm_get_format_name’> >> 
>> > >>      drm_get_format_name(fb->pixel_format));
>> > >>      ^~~~~~~~~~~~~~~~~~~
>> > >> 
>> > >> In file included from ./include/drm/drmP.h:71:0,
>> > >> 
>> > >>                  from drivers/gpu/drm/drm_atomic.c:29:
>> > >> ./include/drm/drm_fourcc.h:65:7: note: declared here
>> > >> 
>> > >>  char *drm_get_format_name(uint32_t format, struct drm_format_name_buf
>> > >>  *buf);> >>  
>> > >>        ^~~~~~~~~~~~~~~~~~~
>> > >> 
>> > >> Can you pls rebase onto drm-misc or linux-next or something?
>> > > 
>> > > That was based on airlied/drm-next (last fetched on Sunday I think),
>> > > I can rebase it on drm-misc if it helps, but it seems older than
>> > > drm-next. Should I just rebase on top of current head of drm-next?
>> > 
>> > It needs to be drm-misc (linux-next doesn't have it yet) due to the
>> > new atomic debug work that we just landed. I'm working on drm-tip as a
>> > drm local integration tree to ease pains like these a bit, but that
>> > doesn't really exist yet.
>> 
>> I'm confused as to how the different trees and branches merge back to
>> Torvalds' tree (I'm interested in particular in drm), and I'm not sure
>> which branch you want me to rebase on in the drm-misc tree [1],
>> especially since all of them are older than drm-next [2].
>> 
>> I'll try to rebase on drm-misc-fixes (currently at 4da5caa6a6f82cda3193)
>> as it sounds about right, but it doesn't apply at all, so it'll take
>> a little while.
>
> While at it, could you make the function return a const char * ?

I thought I mentioned that too, though I didn't insist.

> By the way, while this is an improvement over the current situation in that it 
> fixes the missing kfree() issue, I wonder whether the problem we're trying to 
> solve should be addressed at a more global level.

Maybe, but let's not block this one!

> The issue here is that printk can't format the fourcc as a string by itself. 
> There's a bunch of places in the kernel where a similar formatting problem 
> occurs. In a few occasions it has been solved by extending printk with 
> additional format specifiers (such as for MAC/IP addresses, GUIDs, various 
> kind of device names, ...). DRM fourccs are probably too DRM specific to be 
> worth a format specifier, but I wonder whether we could introduce a new 
> specifier that takes a function pointer as a formatting helper. Another 
> similarly crazy option would be a format specifier for strings that would free 
> the passed pointer after printing it.

I think there are too many non-standard format specifiers already. I
can't review the non-standard format strings without looking at
Documentation/prink-formats.txt first. The formatting hook would be a
generic alternative, but that's more than a little scary from the
security standpoint. And what if the hook has to allocate memory? Can't
do that in atomic contexts.

BR,
Jani.


>
>> Could you give me a quick explanation or point me to a doc/page that
>> explains how the various trees and branches get merged?
>> I googled a bit and found this doc [4] by Jani, but it doesn't mention
>> drm-misc for instance, so I'm not sure how up-to-date and
>> non-intel-specific it is.
>> 
>> Looking at this page, something just occurred to me: did you mean
>> drm-fixes [3], instead of one of the branches on drm-misc?
>> 
>> Cheers,
>>   Eric
>> 
>> [1] git://anongit.freedesktop.org/drm/drm-misc
>> [2] git://people.freedesktop.org/~airlied/linux drm-next
>> [2] git://people.freedesktop.org/~airlied/linux drm-fixes
>> [3] https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-intel.html

-- 
Jani Nikula, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1518850 — Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

Hi Jani,

On Thursday 10 Nov 2016 12:30:09 Jani Nikula wrote:
> On Thu, 10 Nov 2016, Laurent Pinchart wrote:
> > On Wednesday 09 Nov 2016 16:59:31 Eric Engestrom wrote:
> >> On Wednesday, 2016-11-09 14:13:40 +0100, Daniel Vetter wrote:
> >>> On Wed, Nov 9, 2016 at 12:42 PM, Eric Engestrom wrote:
> >>>>> Well, had to drop it again since it didn't compile:
> >>>>>   CC [M]  drivers/gpu/drm/drm_blend.o
> >>>>> 
> >>>>> drivers/gpu/drm/drm_atomic.c: In function
> >>>>> ‘drm_atomic_plane_print_state’:
> >>>>> drivers/gpu/drm/drm_atomic.c:920:5: error: too few arguments to
> >>>>> function ‘drm_get_format_name’> >>
> >>>>>      drm_get_format_name(fb->pixel_format));
> >>>>>      ^~~~~~~~~~~~~~~~~~~
> >>>>> In file included from ./include/drm/drmP.h:71:0,
> >>>>>                  from drivers/gpu/drm/drm_atomic.c:29:
> >>>>> ./include/drm/drm_fourcc.h:65:7: note: declared here
> >>>>>  char *drm_get_format_name(uint32_t format, struct
> >>>>>  drm_format_name_buf *buf);
> >>>>>        ^~~~~~~~~~~~~~~~~~~
> >>>>> 
> >>>>> Can you pls rebase onto drm-misc or linux-next or something?
> >>>> 
> >>>> That was based on airlied/drm-next (last fetched on Sunday I think),
> >>>> I can rebase it on drm-misc if it helps, but it seems older than
> >>>> drm-next. Should I just rebase on top of current head of drm-next?
> >>> 
> >>> It needs to be drm-misc (linux-next doesn't have it yet) due to the
> >>> new atomic debug work that we just landed. I'm working on drm-tip as a
> >>> drm local integration tree to ease pains like these a bit, but that
> >>> doesn't really exist yet.
> >> 
> >> I'm confused as to how the different trees and branches merge back to
> >> Torvalds' tree (I'm interested in particular in drm), and I'm not sure
> >> which branch you want me to rebase on in the drm-misc tree [1],
> >> especially since all of them are older than drm-next [2].
> >> 
> >> I'll try to rebase on drm-misc-fixes (currently at 4da5caa6a6f82cda3193)
> >> as it sounds about right, but it doesn't apply at all, so it'll take
> >> a little while.
> > 
> > While at it, could you make the function return a const char * ?
> 
> I thought I mentioned that too, though I didn't insist.

You did, and I think Eric agreed to change that.

> > By the way, while this is an improvement over the current situation in
> > that it fixes the missing kfree() issue, I wonder whether the problem
> > we're trying to solve should be addressed at a more global level.
> 
> Maybe, but let's not block this one!

Sure, that wasn't the intent of my e-mail.

> > The issue here is that printk can't format the fourcc as a string by
> > itself. There's a bunch of places in the kernel where a similar
> > formatting problem occurs. In a few occasions it has been solved by
> > extending printk with additional format specifiers (such as for MAC/IP
> > addresses, GUIDs, various kind of device names, ...). DRM fourccs are
> > probably too DRM specific to be worth a format specifier, but I wonder
> > whether we could introduce a new specifier that takes a function pointer
> > as a formatting helper. Another similarly crazy option would be a format
> > specifier for strings that would free the passed pointer after printing
> > it.
> 
> I think there are too many non-standard format specifiers already. I
> can't review the non-standard format strings without looking at
> Documentation/prink-formats.txt first. The formatting hook would be a
> generic alternative, but that's more than a little scary from the
> security standpoint. And what if the hook has to allocate memory? Can't
> do that in atomic contexts.

There are lots of details to sort out obviously and I don't have an answer to 
all questions yet. I think it would be worth researching this, as the problem 
isn't specific to DRM/KMS.

> >> Could you give me a quick explanation or point me to a doc/page that
> >> explains how the various trees and branches get merged?
> >> I googled a bit and found this doc [4] by Jani, but it doesn't mention
> >> drm-misc for instance, so I'm not sure how up-to-date and
> >> non-intel-specific it is.
> >> 
> >> Looking at this page, something just occurred to me: did you mean
> >> drm-fixes [3], instead of one of the branches on drm-misc?
> >> 
> >> Cheers,
> >> 
> >>   Eric
> >> 
> >> [1] git://anongit.freedesktop.org/drm/drm-misc
> >> [2] git://people.freedesktop.org/~airlied/linux drm-next
> >> [2] git://people.freedesktop.org/~airlied/linux drm-fixes
> >> [3] https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-intel.html

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1518862 — Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

On Thu, 10 Nov 2016, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> Hi Jani,
>
> On Thursday 10 Nov 2016 12:30:09 Jani Nikula wrote:
>> On Thu, 10 Nov 2016, Laurent Pinchart wrote:
>> > The issue here is that printk can't format the fourcc as a string by
>> > itself. There's a bunch of places in the kernel where a similar
>> > formatting problem occurs. In a few occasions it has been solved by
>> > extending printk with additional format specifiers (such as for MAC/IP
>> > addresses, GUIDs, various kind of device names, ...). DRM fourccs are
>> > probably too DRM specific to be worth a format specifier, but I wonder
>> > whether we could introduce a new specifier that takes a function pointer
>> > as a formatting helper. Another similarly crazy option would be a format
>> > specifier for strings that would free the passed pointer after printing
>> > it.
>> 
>> I think there are too many non-standard format specifiers already. I
>> can't review the non-standard format strings without looking at
>> Documentation/prink-formats.txt first. The formatting hook would be a
>> generic alternative, but that's more than a little scary from the
>> security standpoint. And what if the hook has to allocate memory? Can't
>> do that in atomic contexts.
>
> There are lots of details to sort out obviously and I don't have an answer to 
> all questions yet. I think it would be worth researching this, as the problem 
> isn't specific to DRM/KMS.

That's easy to agree to; as much as you didn't mean to shoot down the
patch, I didn't mean to shoot down your idea! :)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1519634 — Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

On Wed, Nov 09, 2016 at 04:59:31PM +0000, Eric Engestrom wrote:
> On Wednesday, 2016-11-09 14:13:40 +0100, Daniel Vetter wrote:
> > On Wed, Nov 9, 2016 at 12:42 PM, Eric Engestrom
> > <eric.engestrom@imgtec.com> wrote:
> > >> Well, had to drop it again since it didn't compile:
> > >>
> > >>
> > >>   CC [M]  drivers/gpu/drm/drm_blend.o
> > >> drivers/gpu/drm/drm_atomic.c: In function ‘drm_atomic_plane_print_state’:
> > >> drivers/gpu/drm/drm_atomic.c:920:5: error: too few arguments to function ‘drm_get_format_name’
> > >>      drm_get_format_name(fb->pixel_format));
> > >>      ^~~~~~~~~~~~~~~~~~~
> > >> In file included from ./include/drm/drmP.h:71:0,
> > >>                  from drivers/gpu/drm/drm_atomic.c:29:
> > >> ./include/drm/drm_fourcc.h:65:7: note: declared here
> > >>  char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
> > >>        ^~~~~~~~~~~~~~~~~~~
> > >>
> > >> Can you pls rebase onto drm-misc or linux-next or something?
> > >
> > > That was based on airlied/drm-next (last fetched on Sunday I think),
> > > I can rebase it on drm-misc if it helps, but it seems older than
> > > drm-next.
> > > Should I just rebase on top of current head of drm-next?
> > 
> > It needs to be drm-misc (linux-next doesn't have it yet) due to the
> > new atomic debug work that we just landed. I'm working on drm-tip as a
> > drm local integration tree to ease pains like these a bit, but that
> > doesn't really exist yet.
> 
> I'm confused as to how the different trees and branches merge back to
> Torvalds' tree (I'm interested in particular in drm), and I'm not sure
> which branch you want me to rebase on in the drm-misc tree [1],
> especially since all of them are older than drm-next [2].

Dave just pulled in all outstanding pull requests, so just basing on top
of his drm-next should be good enough.

> I'll try to rebase on drm-misc-fixes (currently at 4da5caa6a6f82cda3193)
> as it sounds about right, but it doesn't apply at all, so it'll take
> a little while.
> 
> Could you give me a quick explanation or point me to a doc/page that
> explains how the various trees and branches get merged?
> I googled a bit and found this doc [4] by Jani, but it doesn't mention
> drm-misc for instance, so I'm not sure how up-to-date and
> non-intel-specific it is.

We atm don't have it :( drm-misc was kinda just a (very long running)
experiment, but I want to know make it official. Unfortunately the
scripting rework to split out a new drm-misc.git repo is taking a bit
longer. Atm things are still in flux, but I hope that'll settle soon-ish.

> Looking at this page, something just occurred to me: did you mean
> drm-fixes [3], instead of one of the branches on drm-misc?
> 
> Cheers,
>   Eric
> 
> [1] git://anongit.freedesktop.org/drm/drm-misc

Yeah, that's the new location, but atm it's just under testing and it's
not the real drm-misc. That one is in drm-intel.git, in the topic/drm-misc
branch.

> [2] git://people.freedesktop.org/~airlied/linux drm-next
> [2] git://people.freedesktop.org/~airlied/linux drm-fixes
> [3] https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-intel.html

https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html

We need to flesh that out, and maybe feature it a bit more prominently.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1516232

On Sun, Nov 6, 2016 at 7:48 PM, Eric Engestrom <eric@engestrom.ch> wrote:
> Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
>
>     drm: make drm_get_format_name thread-safe
>
>     Signed-off-by: Eric Engestrom <eric@engestrom.ch>
>     [danvet: Clarify that the returned pointer must be freed with
>     kfree().]
>     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Christian König <christian.koenig@amd.com>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Eric Engestrom <eric@engestrom.ch>

Thanks,

Acked-by: Rob Clark <robdclark@gmail.com>

> ---
>
> v2: use single-field struct instead of typedef to let the compiler
>     enforce the type (Christian König)
>
> ---
>  include/drm/drm_fourcc.h                        | 10 +++++-
>  drivers/gpu/drm/drm_fourcc.c                    | 14 +++------
>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c          |  7 ++---
>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c          |  7 ++---
>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c           |  3 +-
>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c           |  7 ++---
>  drivers/gpu/drm/drm_atomic.c                    |  7 +++--
>  drivers/gpu/drm/drm_crtc.c                      |  7 +++--
>  drivers/gpu/drm/drm_framebuffer.c               |  7 +++--
>  drivers/gpu/drm/drm_modeset_helper.c            |  7 +++--
>  drivers/gpu/drm/drm_plane.c                     |  7 +++--
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  7 ++---
>  drivers/gpu/drm/i915/i915_debugfs.c             | 10 +++---
>  drivers/gpu/drm/i915/intel_atomic_plane.c       |  8 ++---
>  drivers/gpu/drm/i915/intel_display.c            | 41 ++++++++++---------------
>  drivers/gpu/drm/radeon/atombios_crtc.c          | 14 ++++-----
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c             |  3 +-
>  17 files changed, 80 insertions(+), 86 deletions(-)
>
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index dc0aafa..4b03ca0 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -45,6 +45,14 @@ struct drm_format_info {
>         u8 vsub;
>  };
>
> +/**
> + * struct drm_format_name_buf - name of a DRM format
> + * @str: string buffer containing the format name
> + */
> +struct drm_format_name_buf {
> +       char str[32];
> +};
> +
>  const struct drm_format_info *__drm_format_info(u32 format);
>  const struct drm_format_info *drm_format_info(u32 format);
>  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
> @@ -54,6 +62,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
>  int drm_format_vert_chroma_subsampling(uint32_t format);
>  int drm_format_plane_width(int width, uint32_t format, int plane);
>  int drm_format_plane_height(int height, uint32_t format, int plane);
> -char *drm_get_format_name(uint32_t format) __malloc;
> +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>
>  #endif /* __DRM_FOURCC_H__ */
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index cbb8b77..99b0b60 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -79,17 +79,13 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
>  EXPORT_SYMBOL(drm_mode_legacy_fb_format);
>
>  /**
> - * drm_get_format_name - return a string for drm fourcc format
> + * drm_get_format_name - fill a string with a drm fourcc format's name
>   * @format: format to compute name of
> + * @buf: caller-supplied buffer
> - *
> - * Note that the buffer returned by this function is owned by the caller
> - * and will need to be freed using kfree().
>   */
> -char *drm_get_format_name(uint32_t format)
> +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf)
>  {
> -       char *buf = kmalloc(32, GFP_KERNEL);
> -
> -       snprintf(buf, 32,
> +       snprintf(buf->str, sizeof(buf->str),
>                  "%c%c%c%c %s-endian (0x%08x)",
>                  printable_char(format & 0xff),
>                  printable_char((format >> 8) & 0xff),
> @@ -98,7 +94,7 @@ char *drm_get_format_name(uint32_t format)
>                  format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
>                  format);
>
> -       return buf;
> +       return buf->str;
>  }
>  EXPORT_SYMBOL(drm_get_format_name);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index 199d3f7..2924cdd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2032,7 +2032,7 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
>         u32 tmp, viewport_w, viewport_h;
>         int r;
>         bool bypass_lut = false;
> -       char *format_name;
> +       struct drm_format_name_buf format_name;
>
>         /* no fb bound */
>         if (!atomic && !crtc->primary->fb) {
> @@ -2144,9 +2144,8 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
>                 bypass_lut = true;
>                 break;
>         default:
> -               format_name = drm_get_format_name(target_fb->pixel_format);
> -               DRM_ERROR("Unsupported screen format %s\n", format_name);
> +               DRM_ERROR("Unsupported screen format %s\n",
> +                         drm_get_format_name(target_fb->pixel_format, &format_name));
> -               kfree(format_name);
>                 return -EINVAL;
>         }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index ecd000e..f0e3c5c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2013,7 +2013,7 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
>         u32 tmp, viewport_w, viewport_h;
>         int r;
>         bool bypass_lut = false;
> -       char *format_name;
> +       struct drm_format_name_buf format_name;
>
>         /* no fb bound */
>         if (!atomic && !crtc->primary->fb) {
> @@ -2125,9 +2125,8 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
>                 bypass_lut = true;
>                 break;
>         default:
> -               format_name = drm_get_format_name(target_fb->pixel_format);
> -               DRM_ERROR("Unsupported screen format %s\n", format_name);
> +               DRM_ERROR("Unsupported screen format %s\n",
> +                         drm_get_format_name(target_fb->pixel_format, &format_name));
> -               kfree(format_name);
>                 return -EINVAL;
>         }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> index 44547f9..5c6b82f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> @@ -1456,6 +1456,7 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
>         u32 viewport_w, viewport_h;
>         int r;
>         bool bypass_lut = false;
> +       struct drm_format_name_buf format_name;
>
>         /* no fb bound */
>         if (!atomic && !crtc->primary->fb) {
> @@ -1559,7 +1560,7 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
>                 break;
>         default:
>                 DRM_ERROR("Unsupported screen format %s\n",
> -                         drm_get_format_name(target_fb->pixel_format));
> +                         drm_get_format_name(target_fb->pixel_format, &format_name));
>                 return -EINVAL;
>         }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> index 979aedf..5aef19d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -1910,7 +1910,7 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
>         u32 viewport_w, viewport_h;
>         int r;
>         bool bypass_lut = false;
> -       char *format_name;
> +       struct drm_format_name_buf format_name;
>
>         /* no fb bound */
>         if (!atomic && !crtc->primary->fb) {
> @@ -2015,9 +2015,8 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
>                 bypass_lut = true;
>                 break;
>         default:
> -               format_name = drm_get_format_name(target_fb->pixel_format);
> -               DRM_ERROR("Unsupported screen format %s\n", format_name);
> +               DRM_ERROR("Unsupported screen format %s\n",
> +                         drm_get_format_name(target_fb->pixel_format, &format_name));
> -               kfree(format_name);
>                 return -EINVAL;
>         }
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c32fb3c..f29d3c5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -835,9 +835,10 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>         /* Check whether this plane supports the fb pixel format. */
>         ret = drm_plane_check_pixel_format(plane, state->fb->pixel_format);
>         if (ret) {
> -               char *format_name = drm_get_format_name(state->fb->pixel_format);
> -               DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
> +               struct drm_format_name_buf format_name;
> +               DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
> +                                drm_get_format_name(state->fb->pixel_format,
> +                                                    &format_name));
> -               kfree(format_name);
>                 return ret;
>         }
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 13441e2..08ac216 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -827,9 +827,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>                         ret = drm_plane_check_pixel_format(crtc->primary,
>                                                            fb->pixel_format);
>                         if (ret) {
> -                               char *format_name = drm_get_format_name(fb->pixel_format);
> -                               DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> +                               struct drm_format_name_buf format_name;
> +                               DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +                                             drm_get_format_name(fb->pixel_format,
> +                                                                 &format_name));
> -                               kfree(format_name);
>                                 goto out;
>                         }
>                 }
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 49fd7db..df6b180 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -133,9 +133,10 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>
>         info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
>         if (!info) {
> -               char *format_name = drm_get_format_name(r->pixel_format);
> -               DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
> +               struct drm_format_name_buf format_name;
> +               DRM_DEBUG_KMS("bad framebuffer format %s\n",
> +                             drm_get_format_name(r->pixel_format,
> +                                                 &format_name));
> -               kfree(format_name);
>                 return -EINVAL;
>         }
>
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index 2544dfe..2f452b3 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -75,10 +75,11 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
>
>         info = drm_format_info(mode_cmd->pixel_format);
>         if (!info || !info->depth) {
> -               char *format_name = drm_get_format_name(mode_cmd->pixel_format);
> +               struct drm_format_name_buf format_name;
>
> -               DRM_DEBUG_KMS("non-RGB pixel format %s\n", format_name);
> +               DRM_DEBUG_KMS("non-RGB pixel format %s\n",
> +                             drm_get_format_name(mode_cmd->pixel_format,
> +                                                 &format_name));
> -               kfree(format_name);
>
>                 fb->depth = 0;
>                 fb->bits_per_pixel = 0;
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 249c0ae..2ba0c22 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -479,9 +479,10 @@ static int __setplane_internal(struct drm_plane *plane,
>         /* Check whether this plane supports the fb pixel format. */
>         ret = drm_plane_check_pixel_format(plane, fb->pixel_format);
>         if (ret) {
> -               char *format_name = drm_get_format_name(fb->pixel_format);
> -               DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> +               struct drm_format_name_buf format_name;
> +               DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +                             drm_get_format_name(fb->pixel_format,
> +                                                 &format_name));
> -               kfree(format_name);
>                 goto out;
>         }
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> index 7e7a4d4..afc2b5d 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> @@ -608,17 +608,16 @@ static void ade_rdma_set(void __iomem *base, struct drm_framebuffer *fb,
>                          u32 ch, u32 y, u32 in_h, u32 fmt)
>  {
>         struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, 0);
> -       char *format_name;
> +       struct drm_format_name_buf format_name;
>         u32 reg_ctrl, reg_addr, reg_size, reg_stride, reg_space, reg_en;
>         u32 stride = fb->pitches[0];
>         u32 addr = (u32)obj->paddr + y * stride;
>
>         DRM_DEBUG_DRIVER("rdma%d: (y=%d, height=%d), stride=%d, paddr=0x%x\n",
>                          ch + 1, y, in_h, stride, (u32)obj->paddr);
> -       format_name = drm_get_format_name(fb->pixel_format);
>         DRM_DEBUG_DRIVER("addr=0x%x, fb:%dx%d, pixel_format=%d(%s)\n",
> -                        addr, fb->width, fb->height, fmt, format_name);
> +                        addr, fb->width, fb->height, fmt,
> +                        drm_get_format_name(fb->pixel_format, &format_name));
> -       kfree(format_name);
>
>         /* get reg offset */
>         reg_ctrl = RD_CH_CTRL(ch);
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 20638d2..670423e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2971,7 +2971,7 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
>         for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>                 struct drm_plane_state *state;
>                 struct drm_plane *plane = &intel_plane->base;
> -               char *format_name;
> +               struct drm_format_name_buf format_name;
>
>                 if (!plane->state) {
>                         seq_puts(m, "plane->state is NULL!\n");
> @@ -2981,9 +2981,9 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
>                 state = plane->state;
>
>                 if (state->fb) {
> -                       format_name = drm_get_format_name(state->fb->pixel_format);
> +                       drm_get_format_name(state->fb->pixel_format, &format_name);
>                 } else {
> -                       format_name = kstrdup("N/A", GFP_KERNEL);
> +                       sprintf(format_name.str, "N/A");
>                 }
>
>                 seq_printf(m, "\t--Plane id %d: type=%s, crtc_pos=%4dx%4d, crtc_size=%4dx%4d, src_pos=%d.%04ux%d.%04u, src_size=%d.%04ux%d.%04u, format=%s, rotation=%s\n",
> @@ -2999,10 +2999,8 @@ static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
>                            ((state->src_w & 0xffff) * 15625) >> 10,
>                            (state->src_h >> 16),
>                            ((state->src_h & 0xffff) * 15625) >> 10,
> -                          format_name,
> +                          format_name.str,
>                            plane_rotation(state->rotation));
> -
> -               kfree(format_name);
>         }
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index c762ae5..6c76e3d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -143,7 +143,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>                 crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>
>         if (state->fb && drm_rotation_90_or_270(state->rotation)) {
> -               char *format_name;
> +               struct drm_format_name_buf format_name;
>
>                 if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>                         state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> @@ -159,9 +159,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>                 switch (state->fb->pixel_format) {
>                 case DRM_FORMAT_C8:
>                 case DRM_FORMAT_RGB565:
> -                       format_name = drm_get_format_name(state->fb->pixel_format);
> -                       DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", format_name);
> +                       DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> +                                     drm_get_format_name(state->fb->pixel_format,
> +                                                         &format_name));
> -                       kfree(format_name);
>                         return -EINVAL;
>
>                 default:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6f8f6ec..8209170 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12871,7 +12871,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>
>         DRM_DEBUG_KMS("planes on this crtc\n");
>         list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> -               char *format_name;
> +               struct drm_format_name_buf format_name;
>                 intel_plane = to_intel_plane(plane);
>                 if (intel_plane->pipe != crtc->pipe)
>                         continue;
> @@ -12884,12 +12884,11 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>                         continue;
>                 }
>
> -               format_name = drm_get_format_name(fb->pixel_format);
> -
>                 DRM_DEBUG_KMS("[PLANE:%d:%s] enabled",
>                               plane->base.id, plane->name);
>                 DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = %s",
> -                             fb->base.id, fb->width, fb->height, format_name);
> +                             fb->base.id, fb->width, fb->height,
> +                             drm_get_format_name(fb->pixel_format, &format_name));
>                 DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
>                               state->scaler_id,
>                               state->base.src.x1 >> 16,
> @@ -12899,8 +12898,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>                               state->base.dst.x1, state->base.dst.y1,
>                               drm_rect_width(&state->base.dst),
>                               drm_rect_height(&state->base.dst));
> -
> -               kfree(format_name);
>         }
>  }
>
> @@ -15749,7 +15746,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
>         unsigned int tiling = i915_gem_object_get_tiling(obj);
>         int ret;
>         u32 pitch_limit, stride_alignment;
> -       char *format_name;
> +       struct drm_format_name_buf format_name;
>
>         WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> @@ -15840,18 +15837,16 @@ static int intel_framebuffer_init(struct drm_device *dev,
>                 break;
>         case DRM_FORMAT_XRGB1555:
>                 if (INTEL_INFO(dev)->gen > 3) {
> -                       format_name = drm_get_format_name(mode_cmd->pixel_format);
> -                       DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +                       DRM_DEBUG("unsupported pixel format: %s\n",
> +                                 drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -                       kfree(format_name);
>                         return -EINVAL;
>                 }
>                 break;
>         case DRM_FORMAT_ABGR8888:
>                 if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
>                     INTEL_INFO(dev)->gen < 9) {
> -                       format_name = drm_get_format_name(mode_cmd->pixel_format);
> -                       DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +                       DRM_DEBUG("unsupported pixel format: %s\n",
> +                                 drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -                       kfree(format_name);
>                         return -EINVAL;
>                 }
>                 break;
> @@ -15859,17 +15854,15 @@ static int intel_framebuffer_init(struct drm_device *dev,
>         case DRM_FORMAT_XRGB2101010:
>         case DRM_FORMAT_XBGR2101010:
>                 if (INTEL_INFO(dev)->gen < 4) {
> -                       format_name = drm_get_format_name(mode_cmd->pixel_format);
> -                       DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +                       DRM_DEBUG("unsupported pixel format: %s\n",
> +                                 drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -                       kfree(format_name);
>                         return -EINVAL;
>                 }
>                 break;
>         case DRM_FORMAT_ABGR2101010:
>                 if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
> -                       format_name = drm_get_format_name(mode_cmd->pixel_format);
> -                       DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +                       DRM_DEBUG("unsupported pixel format: %s\n",
> +                                 drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -                       kfree(format_name);
>                         return -EINVAL;
>                 }
>                 break;
> @@ -15878,16 +15871,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
>         case DRM_FORMAT_YVYU:
>         case DRM_FORMAT_VYUY:
>                 if (INTEL_INFO(dev)->gen < 5) {
> -                       format_name = drm_get_format_name(mode_cmd->pixel_format);
> -                       DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +                       DRM_DEBUG("unsupported pixel format: %s\n",
> +                                 drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -                       kfree(format_name);
>                         return -EINVAL;
>                 }
>                 break;
>         default:
> -               format_name = drm_get_format_name(mode_cmd->pixel_format);
> -               DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> +               DRM_DEBUG("unsupported pixel format: %s\n",
> +                         drm_get_format_name(mode_cmd->pixel_format, &format_name));
> -               kfree(format_name);
>                 return -EINVAL;
>         }
>
> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
> index 74f99ba..05f4ebe 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -1156,7 +1156,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
>         u32 tmp, viewport_w, viewport_h;
>         int r;
>         bool bypass_lut = false;
> -       char *format_name;
> +       struct drm_format_name_buf format_name;
>
>         /* no fb bound */
>         if (!atomic && !crtc->primary->fb) {
> @@ -1260,9 +1260,8 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
>                 bypass_lut = true;
>                 break;
>         default:
> -               format_name = drm_get_format_name(target_fb->pixel_format);
> -               DRM_ERROR("Unsupported screen format %s\n", format_name);
> +               DRM_ERROR("Unsupported screen format %s\n",
> +                         drm_get_format_name(target_fb->pixel_format, &format_name));
> -               kfree(format_name);
>                 return -EINVAL;
>         }
>
> @@ -1473,7 +1472,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
>         u32 viewport_w, viewport_h;
>         int r;
>         bool bypass_lut = false;
> -       char *format_name;
> +       struct drm_format_name_buf format_name;
>
>         /* no fb bound */
>         if (!atomic && !crtc->primary->fb) {
> @@ -1563,9 +1562,8 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
>                 bypass_lut = true;
>                 break;
>         default:
> -               format_name = drm_get_format_name(target_fb->pixel_format);
> -               DRM_ERROR("Unsupported screen format %s\n", format_name);
> +               DRM_ERROR("Unsupported screen format %s\n",
> +                         drm_get_format_name(target_fb->pixel_format, &format_name));
> -               kfree(format_name);
>                 return -EINVAL;
>         }
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index c965514..e3f68cc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -985,8 +985,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct drm_device *dev,
>
>         info = drm_format_info(mode_cmd2->pixel_format);
>         if (!info || !info->depth) {
> +               struct drm_format_name_buf format_name;
>                 DRM_ERROR("Unsupported framebuffer format %s\n",
> -                         drm_get_format_name(mode_cmd2->pixel_format));
> +                         drm_get_format_name(mode_cmd2->pixel_format, &format_name));
>                 return ERR_PTR(-EINVAL);
>         }
>
> --
> Cheers,
>   Eric
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web