* [PATCH] drm: make drm_get_format_name thread-safe
@ 2016-08-15 0:02 Eric Engestrom
2016-08-15 9:54 ` Jani Nikula
2016-11-03 18:52 ` [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe Rob Clark
0 siblings, 2 replies; 11+ messages in thread
From: Eric Engestrom @ 2016-08-15 0:02 UTC (permalink / raw)
To: linux-kernel
Cc: Eric Engestrom, Alex Deucher, Christian König, David Airlie,
Xinliang Liu, Daniel Vetter, Jani Nikula, Michel Dänzer,
Tom St Denis, Gustavo Padovan, David Zhang, Flora Cui,
Vitaly Prosyak, Junwei Zhang, Xinwei Kong, Archit Taneja,
Wei Yongjun, dri-devel, intel-gfx
Signed-off-by: Eric Engestrom <eric@engestrom.ch>
---
I moved the main bits to be the first diffs, shouldn't affect anything
when applying the patch, but I wanted to ask:
I don't like the hard-coded `32` the appears in both kmalloc() and
snprintf(), what do you think? If you don't like it either, what would
you suggest? Should I #define it?
Second question is about the patch mail itself: should I send this kind
of patch separated by module, with a note requesting them to be squashed
when applying? It has to land as a single patch, but for review it might
be easier if people only see the bits they each care about, as well as
to collect ack's/r-b's.
Cheers,
Eric
---
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++--
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++--
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++--
drivers/gpu/drm/drm_atomic.c | 5 ++--
drivers/gpu/drm/drm_crtc.c | 21 ++++++++-----
drivers/gpu/drm/drm_fourcc.c | 17 ++++++-----
drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 6 ++--
drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-
drivers/gpu/drm/i915/intel_atomic_plane.c | 6 ++--
drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++---------
drivers/gpu/drm/radeon/atombios_crtc.c | 12 +++++---
include/drm/drm_fourcc.h | 2 +-
12 files changed, 89 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 0645c85..38216a1 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -39,16 +39,14 @@ static char printable_char(int c)
* drm_get_format_name - return a string for drm fourcc format
* @format: format to compute name of
*
- * Note that the buffer used by this function is globally shared and owned by
- * the function itself.
- *
- * FIXME: This isn't really multithreading safe.
+ * Note that the buffer returned by this function is owned by the caller
+ * and will need to be freed.
*/
const char *drm_get_format_name(uint32_t format)
{
- static char buf[32];
+ char *buf = kmalloc(32, GFP_KERNEL);
- snprintf(buf, sizeof(buf),
+ snprintf(buf, 32,
"%c%c%c%c %s-endian (0x%08x)",
printable_char(format & 0xff),
printable_char((format >> 8) & 0xff),
@@ -73,6 +71,8 @@ EXPORT_SYMBOL(drm_get_format_name);
void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
int *bpp)
{
+ const char *format_name;
+
switch (format) {
case DRM_FORMAT_C8:
case DRM_FORMAT_RGB332:
@@ -127,8 +127,9 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
*bpp = 32;
break;
default:
- DRM_DEBUG_KMS("unsupported pixel format %s\n",
- drm_get_format_name(format));
+ format_name = drm_get_format_name(format);
+ DRM_DEBUG_KMS("unsupported pixel format %s\n", format_name);
+ kfree(format_name);
*depth = 0;
*bpp = 0;
break;
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 7f90a39..030d22d 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -32,6 +32,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);
-const char *drm_get_format_name(uint32_t format);
+const char *drm_get_format_name(uint32_t format) __malloc;
#endif /* __DRM_FOURCC_H__ */
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index c1b04e9..0bf8959 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2071,6 +2071,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;
+ const char *format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -2182,8 +2183,9 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
bypass_lut = true;
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->pixel_format));
+ format_name = drm_get_format_name(target_fb->pixel_format);
+ DRM_ERROR("Unsupported screen format %s\n", 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 d4bf133..1558a97 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2046,6 +2046,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;
+ const char *format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -2157,8 +2158,9 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
bypass_lut = true;
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->pixel_format));
+ format_name = drm_get_format_name(target_fb->pixel_format);
+ DRM_ERROR("Unsupported screen format %s\n", format_name);
+ kfree(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 4fdfab1..71a0375 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -1952,6 +1952,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;
+ const char *format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -2056,8 +2057,9 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
bypass_lut = true;
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->pixel_format));
+ format_name = drm_get_format_name(target_fb->pixel_format);
+ DRM_ERROR("Unsupported screen format %s\n", format_name);
+ kfree(format_name);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index fa39307..087391f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -837,8 +837,9 @@ 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) {
- DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
- drm_get_format_name(state->fb->pixel_format));
+ const char *format_name = drm_get_format_name(state->fb->pixel_format);
+ DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
+ kfree(format_name);
return ret;
}
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b1dbb60..7da5d33 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2592,8 +2592,9 @@ 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) {
- DRM_DEBUG_KMS("Invalid pixel format %s\n",
- drm_get_format_name(fb->pixel_format));
+ const char *format_name = drm_get_format_name(fb->pixel_format);
+ DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
+ kfree(format_name);
goto out;
}
@@ -2902,8 +2903,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
ret = drm_plane_check_pixel_format(crtc->primary,
fb->pixel_format);
if (ret) {
- DRM_DEBUG_KMS("Invalid pixel format %s\n",
- drm_get_format_name(fb->pixel_format));
+ const char *format_name = drm_get_format_name(fb->pixel_format);
+ DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
+ kfree(format_name);
goto out;
}
}
@@ -3279,6 +3281,7 @@ int drm_mode_addfb(struct drm_device *dev,
static int format_check(const struct drm_mode_fb_cmd2 *r)
{
uint32_t format = r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN;
+ const char *format_name;
switch (format) {
case DRM_FORMAT_C8:
@@ -3343,8 +3346,9 @@ static int format_check(const struct drm_mode_fb_cmd2 *r)
case DRM_FORMAT_YVU444:
return 0;
default:
- DRM_DEBUG_KMS("invalid pixel format %s\n",
- drm_get_format_name(r->pixel_format));
+ format_name = drm_get_format_name(r->pixel_format);
+ DRM_DEBUG_KMS("invalid pixel format %s\n", format_name);
+ kfree(format_name);
return -EINVAL;
}
}
@@ -3355,8 +3359,9 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
ret = format_check(r);
if (ret) {
- DRM_DEBUG_KMS("bad framebuffer format %s\n",
- drm_get_format_name(r->pixel_format));
+ const char *format_name = drm_get_format_name(r->pixel_format);
+ DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
+ kfree(format_name);
return ret;
}
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index c3707d4..ac7fa02 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -608,15 +608,17 @@ 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);
+ const char *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,
- drm_get_format_name(fb->pixel_format));
+ addr, fb->width, fb->height, fmt, 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 844fea7..904720b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3109,6 +3109,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;
+ const char *format_name;
if (!plane->state) {
seq_puts(m, "plane->state is NULL!\n");
@@ -3117,6 +3118,12 @@ 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);
+ } else {
+ format_name = kstrdup("N/A", GFP_KERNEL);
+ }
+
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",
plane->base.id,
plane_type(intel_plane->base.type),
@@ -3130,8 +3137,10 @@ 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,
- state->fb ? drm_get_format_name(state->fb->pixel_format) : "N/A",
+ format_name,
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 7de7721..e06131a 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -157,6 +157,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
if (state->fb && intel_rotation_90_or_270(state->rotation)) {
+ const char *format_name;
if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
@@ -171,8 +172,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
switch (state->fb->pixel_format) {
case DRM_FORMAT_C8:
case DRM_FORMAT_RGB565:
- DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
- drm_get_format_name(state->fb->pixel_format));
+ format_name = drm_get_format_name(state->fb->pixel_format);
+ DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", 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 c457eed..071399b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12282,6 +12282,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) {
+ const char *format_name;
intel_plane = to_intel_plane(plane);
if (intel_plane->pipe != crtc->pipe)
continue;
@@ -12294,11 +12295,12 @@ 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,
- drm_get_format_name(fb->pixel_format));
+ fb->base.id, fb->width, fb->height, format_name);
DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
state->scaler_id,
state->src.x1 >> 16, state->src.y1 >> 16,
@@ -12307,6 +12309,8 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
state->dst.x1, state->dst.y1,
drm_rect_width(&state->dst),
drm_rect_height(&state->dst));
+
+ kfree(format_name);
}
}
@@ -14936,6 +14940,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
unsigned int aligned_height;
int ret;
u32 pitch_limit, stride_alignment;
+ const char *format_name;
WARN_ON(!mutex_is_locked(&dev->struct_mutex));
@@ -15009,16 +15014,18 @@ static int intel_framebuffer_init(struct drm_device *dev,
break;
case DRM_FORMAT_XRGB1555:
if (INTEL_INFO(dev)->gen > 3) {
- DRM_DEBUG("unsupported pixel format: %s\n",
- drm_get_format_name(mode_cmd->pixel_format));
+ format_name = drm_get_format_name(mode_cmd->pixel_format);
+ DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+ kfree(format_name);
return -EINVAL;
}
break;
case DRM_FORMAT_ABGR8888:
if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) &&
INTEL_INFO(dev)->gen < 9) {
- DRM_DEBUG("unsupported pixel format: %s\n",
- drm_get_format_name(mode_cmd->pixel_format));
+ format_name = drm_get_format_name(mode_cmd->pixel_format);
+ DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+ kfree(format_name);
return -EINVAL;
}
break;
@@ -15026,15 +15033,17 @@ static int intel_framebuffer_init(struct drm_device *dev,
case DRM_FORMAT_XRGB2101010:
case DRM_FORMAT_XBGR2101010:
if (INTEL_INFO(dev)->gen < 4) {
- DRM_DEBUG("unsupported pixel format: %s\n",
- drm_get_format_name(mode_cmd->pixel_format));
+ format_name = drm_get_format_name(mode_cmd->pixel_format);
+ DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+ kfree(format_name);
return -EINVAL;
}
break;
case DRM_FORMAT_ABGR2101010:
if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
- DRM_DEBUG("unsupported pixel format: %s\n",
- drm_get_format_name(mode_cmd->pixel_format));
+ format_name = drm_get_format_name(mode_cmd->pixel_format);
+ DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+ kfree(format_name);
return -EINVAL;
}
break;
@@ -15043,14 +15052,16 @@ static int intel_framebuffer_init(struct drm_device *dev,
case DRM_FORMAT_YVYU:
case DRM_FORMAT_VYUY:
if (INTEL_INFO(dev)->gen < 5) {
- DRM_DEBUG("unsupported pixel format: %s\n",
- drm_get_format_name(mode_cmd->pixel_format));
+ format_name = drm_get_format_name(mode_cmd->pixel_format);
+ DRM_DEBUG("unsupported pixel format: %s\n", format_name);
+ kfree(format_name);
return -EINVAL;
}
break;
default:
- DRM_DEBUG("unsupported pixel format: %s\n",
- drm_get_format_name(mode_cmd->pixel_format));
+ format_name = drm_get_format_name(mode_cmd->pixel_format);
+ DRM_DEBUG("unsupported pixel format: %s\n", 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 a97abc8..981ca3f 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -1154,6 +1154,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
u32 tmp, viewport_w, viewport_h;
int r;
bool bypass_lut = false;
+ const char *format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -1257,8 +1258,9 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
bypass_lut = true;
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->pixel_format));
+ format_name = drm_get_format_name(target_fb->pixel_format);
+ DRM_ERROR("Unsupported screen format %s\n", format_name);
+ kfree(format_name);
return -EINVAL;
}
@@ -1469,6 +1471,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
u32 viewport_w, viewport_h;
int r;
bool bypass_lut = false;
+ const char *format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -1558,8 +1561,9 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
bypass_lut = true;
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->pixel_format));
+ format_name = drm_get_format_name(target_fb->pixel_format);
+ DRM_ERROR("Unsupported screen format %s\n", format_name);
+ kfree(format_name);
return -EINVAL;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] drm: make drm_get_format_name thread-safe
2016-08-15 0:02 [PATCH] drm: make drm_get_format_name thread-safe Eric Engestrom
@ 2016-08-15 9:54 ` Jani Nikula
2016-08-15 12:59 ` Eric Engestrom
2016-11-03 18:52 ` [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe Rob Clark
1 sibling, 1 reply; 11+ messages in thread
From: Jani Nikula @ 2016-08-15 9:54 UTC (permalink / raw)
To: Eric Engestrom, linux-kernel
Cc: Eric Engestrom, Alex Deucher, Christian König, David Airlie,
Xinliang Liu, Daniel Vetter, Michel Dänzer, Tom St Denis,
Gustavo Padovan, David Zhang, Flora Cui, Vitaly Prosyak,
Junwei Zhang, Xinwei Kong, Archit Taneja, Wei Yongjun, dri-devel,
intel-gfx
On Mon, 15 Aug 2016, Eric Engestrom <eric@engestrom.ch> wrote:
> Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> ---
>
> I moved the main bits to be the first diffs, shouldn't affect anything
> when applying the patch, but I wanted to ask:
> I don't like the hard-coded `32` the appears in both kmalloc() and
> snprintf(), what do you think? If you don't like it either, what would
> you suggest? Should I #define it?
>
> Second question is about the patch mail itself: should I send this kind
> of patch separated by module, with a note requesting them to be squashed
> when applying? It has to land as a single patch, but for review it might
> be easier if people only see the bits they each care about, as well as
> to collect ack's/r-b's.
>
> Cheers,
> Eric
>
> ---
> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++--
> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++--
> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++--
> drivers/gpu/drm/drm_atomic.c | 5 ++--
> drivers/gpu/drm/drm_crtc.c | 21 ++++++++-----
> drivers/gpu/drm/drm_fourcc.c | 17 ++++++-----
> drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 6 ++--
> drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-
> drivers/gpu/drm/i915/intel_atomic_plane.c | 6 ++--
> drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++---------
> drivers/gpu/drm/radeon/atombios_crtc.c | 12 +++++---
> include/drm/drm_fourcc.h | 2 +-
> 12 files changed, 89 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 0645c85..38216a1 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -39,16 +39,14 @@ static char printable_char(int c)
> * drm_get_format_name - return a string for drm fourcc format
> * @format: format to compute name of
> *
> - * Note that the buffer used by this function is globally shared and owned by
> - * the function itself.
> - *
> - * FIXME: This isn't really multithreading safe.
> + * Note that the buffer returned by this function is owned by the caller
> + * and will need to be freed.
> */
> const char *drm_get_format_name(uint32_t format)
I find it surprising that a function that allocates a buffer returns a
const pointer. Some userspace libraries have conventions about the
ownership based on constness.
(I also find it suprising that kfree() takes a const pointer; arguably
that call changes the memory.)
Is there precedent for this?
BR,
Jani.
> {
> - static char buf[32];
> + char *buf = kmalloc(32, GFP_KERNEL);
>
> - snprintf(buf, sizeof(buf),
> + snprintf(buf, 32,
> "%c%c%c%c %s-endian (0x%08x)",
> printable_char(format & 0xff),
> printable_char((format >> 8) & 0xff),
> @@ -73,6 +71,8 @@ EXPORT_SYMBOL(drm_get_format_name);
> void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> int *bpp)
> {
> + const char *format_name;
> +
> switch (format) {
> case DRM_FORMAT_C8:
> case DRM_FORMAT_RGB332:
> @@ -127,8 +127,9 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> *bpp = 32;
> break;
> default:
> - DRM_DEBUG_KMS("unsupported pixel format %s\n",
> - drm_get_format_name(format));
> + format_name = drm_get_format_name(format);
> + DRM_DEBUG_KMS("unsupported pixel format %s\n", format_name);
> + kfree(format_name);
> *depth = 0;
> *bpp = 0;
> break;
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 7f90a39..030d22d 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -32,6 +32,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);
> -const char *drm_get_format_name(uint32_t format);
> +const char *drm_get_format_name(uint32_t format) __malloc;
>
> #endif /* __DRM_FOURCC_H__ */
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index c1b04e9..0bf8959 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2071,6 +2071,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;
> + const char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -2182,8 +2183,9 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
> bypass_lut = true;
> break;
> default:
> - DRM_ERROR("Unsupported screen format %s\n",
> - drm_get_format_name(target_fb->pixel_format));
> + format_name = drm_get_format_name(target_fb->pixel_format);
> + DRM_ERROR("Unsupported screen format %s\n", 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 d4bf133..1558a97 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2046,6 +2046,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;
> + const char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -2157,8 +2158,9 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
> bypass_lut = true;
> break;
> default:
> - DRM_ERROR("Unsupported screen format %s\n",
> - drm_get_format_name(target_fb->pixel_format));
> + format_name = drm_get_format_name(target_fb->pixel_format);
> + DRM_ERROR("Unsupported screen format %s\n", format_name);
> + kfree(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 4fdfab1..71a0375 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -1952,6 +1952,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;
> + const char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -2056,8 +2057,9 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
> bypass_lut = true;
> break;
> default:
> - DRM_ERROR("Unsupported screen format %s\n",
> - drm_get_format_name(target_fb->pixel_format));
> + format_name = drm_get_format_name(target_fb->pixel_format);
> + DRM_ERROR("Unsupported screen format %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fa39307..087391f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -837,8 +837,9 @@ 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) {
> - DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
> - drm_get_format_name(state->fb->pixel_format));
> + const char *format_name = drm_get_format_name(state->fb->pixel_format);
> + DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
> + kfree(format_name);
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b1dbb60..7da5d33 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2592,8 +2592,9 @@ 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) {
> - DRM_DEBUG_KMS("Invalid pixel format %s\n",
> - drm_get_format_name(fb->pixel_format));
> + const char *format_name = drm_get_format_name(fb->pixel_format);
> + DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> + kfree(format_name);
> goto out;
> }
>
> @@ -2902,8 +2903,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> ret = drm_plane_check_pixel_format(crtc->primary,
> fb->pixel_format);
> if (ret) {
> - DRM_DEBUG_KMS("Invalid pixel format %s\n",
> - drm_get_format_name(fb->pixel_format));
> + const char *format_name = drm_get_format_name(fb->pixel_format);
> + DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> + kfree(format_name);
> goto out;
> }
> }
> @@ -3279,6 +3281,7 @@ int drm_mode_addfb(struct drm_device *dev,
> static int format_check(const struct drm_mode_fb_cmd2 *r)
> {
> uint32_t format = r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN;
> + const char *format_name;
>
> switch (format) {
> case DRM_FORMAT_C8:
> @@ -3343,8 +3346,9 @@ static int format_check(const struct drm_mode_fb_cmd2 *r)
> case DRM_FORMAT_YVU444:
> return 0;
> default:
> - DRM_DEBUG_KMS("invalid pixel format %s\n",
> - drm_get_format_name(r->pixel_format));
> + format_name = drm_get_format_name(r->pixel_format);
> + DRM_DEBUG_KMS("invalid pixel format %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> }
> @@ -3355,8 +3359,9 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>
> ret = format_check(r);
> if (ret) {
> - DRM_DEBUG_KMS("bad framebuffer format %s\n",
> - drm_get_format_name(r->pixel_format));
> + const char *format_name = drm_get_format_name(r->pixel_format);
> + DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
> + kfree(format_name);
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> index c3707d4..ac7fa02 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> @@ -608,15 +608,17 @@ 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);
> + const char *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,
> - drm_get_format_name(fb->pixel_format));
> + addr, fb->width, fb->height, fmt, 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 844fea7..904720b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3109,6 +3109,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;
> + const char *format_name;
>
> if (!plane->state) {
> seq_puts(m, "plane->state is NULL!\n");
> @@ -3117,6 +3118,12 @@ 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);
> + } else {
> + format_name = kstrdup("N/A", GFP_KERNEL);
> + }
> +
> 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",
> plane->base.id,
> plane_type(intel_plane->base.type),
> @@ -3130,8 +3137,10 @@ 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,
> - state->fb ? drm_get_format_name(state->fb->pixel_format) : "N/A",
> + format_name,
> 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 7de7721..e06131a 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -157,6 +157,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>
> if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> + const char *format_name;
> if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> @@ -171,8 +172,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> switch (state->fb->pixel_format) {
> case DRM_FORMAT_C8:
> case DRM_FORMAT_RGB565:
> - DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> - drm_get_format_name(state->fb->pixel_format));
> + format_name = drm_get_format_name(state->fb->pixel_format);
> + DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", 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 c457eed..071399b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12282,6 +12282,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) {
> + const char *format_name;
> intel_plane = to_intel_plane(plane);
> if (intel_plane->pipe != crtc->pipe)
> continue;
> @@ -12294,11 +12295,12 @@ 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,
> - drm_get_format_name(fb->pixel_format));
> + fb->base.id, fb->width, fb->height, format_name);
> DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
> state->scaler_id,
> state->src.x1 >> 16, state->src.y1 >> 16,
> @@ -12307,6 +12309,8 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> state->dst.x1, state->dst.y1,
> drm_rect_width(&state->dst),
> drm_rect_height(&state->dst));
> +
> + kfree(format_name);
> }
> }
>
> @@ -14936,6 +14940,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
> unsigned int aligned_height;
> int ret;
> u32 pitch_limit, stride_alignment;
> + const char *format_name;
>
> WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> @@ -15009,16 +15014,18 @@ static int intel_framebuffer_init(struct drm_device *dev,
> break;
> case DRM_FORMAT_XRGB1555:
> if (INTEL_INFO(dev)->gen > 3) {
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> break;
> case DRM_FORMAT_ABGR8888:
> if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) &&
> INTEL_INFO(dev)->gen < 9) {
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> break;
> @@ -15026,15 +15033,17 @@ static int intel_framebuffer_init(struct drm_device *dev,
> case DRM_FORMAT_XRGB2101010:
> case DRM_FORMAT_XBGR2101010:
> if (INTEL_INFO(dev)->gen < 4) {
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> break;
> case DRM_FORMAT_ABGR2101010:
> if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> break;
> @@ -15043,14 +15052,16 @@ static int intel_framebuffer_init(struct drm_device *dev,
> case DRM_FORMAT_YVYU:
> case DRM_FORMAT_VYUY:
> if (INTEL_INFO(dev)->gen < 5) {
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> break;
> default:
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", 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 a97abc8..981ca3f 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -1154,6 +1154,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
> u32 tmp, viewport_w, viewport_h;
> int r;
> bool bypass_lut = false;
> + const char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -1257,8 +1258,9 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
> bypass_lut = true;
> break;
> default:
> - DRM_ERROR("Unsupported screen format %s\n",
> - drm_get_format_name(target_fb->pixel_format));
> + format_name = drm_get_format_name(target_fb->pixel_format);
> + DRM_ERROR("Unsupported screen format %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
>
> @@ -1469,6 +1471,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
> u32 viewport_w, viewport_h;
> int r;
> bool bypass_lut = false;
> + const char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -1558,8 +1561,9 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
> bypass_lut = true;
> break;
> default:
> - DRM_ERROR("Unsupported screen format %s\n",
> - drm_get_format_name(target_fb->pixel_format));
> + format_name = drm_get_format_name(target_fb->pixel_format);
> + DRM_ERROR("Unsupported screen format %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] drm: make drm_get_format_name thread-safe
2016-08-15 9:54 ` Jani Nikula
@ 2016-08-15 12:59 ` Eric Engestrom
2016-08-15 13:13 ` Jani Nikula
0 siblings, 1 reply; 11+ messages in thread
From: Eric Engestrom @ 2016-08-15 12:59 UTC (permalink / raw)
To: Jani Nikula
Cc: Eric Engestrom, linux-kernel, Tom St Denis, intel-gfx,
Michel Dänzer, Wei Yongjun, dri-devel, Junwei Zhang,
Xinliang Liu, David Zhang, Vitaly Prosyak, Alex Deucher,
Daniel Vetter, Flora Cui, Gustavo Padovan, Christian König
On Mon, Aug 15, 2016 at 12:54:01PM +0300, Jani Nikula wrote:
> On Mon, 15 Aug 2016, Eric Engestrom <eric@engestrom.ch> wrote:
> > Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> > ---
> >
> > I moved the main bits to be the first diffs, shouldn't affect anything
> > when applying the patch, but I wanted to ask:
> > I don't like the hard-coded `32` the appears in both kmalloc() and
> > snprintf(), what do you think? If you don't like it either, what would
> > you suggest? Should I #define it?
> >
> > Second question is about the patch mail itself: should I send this kind
> > of patch separated by module, with a note requesting them to be squashed
> > when applying? It has to land as a single patch, but for review it might
> > be easier if people only see the bits they each care about, as well as
> > to collect ack's/r-b's.
> >
> > Cheers,
> > Eric
> >
> > ---
> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++--
> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++--
> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++--
> > drivers/gpu/drm/drm_atomic.c | 5 ++--
> > drivers/gpu/drm/drm_crtc.c | 21 ++++++++-----
> > drivers/gpu/drm/drm_fourcc.c | 17 ++++++-----
> > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 6 ++--
> > drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-
> > drivers/gpu/drm/i915/intel_atomic_plane.c | 6 ++--
> > drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++---------
> > drivers/gpu/drm/radeon/atombios_crtc.c | 12 +++++---
> > include/drm/drm_fourcc.h | 2 +-
> > 12 files changed, 89 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 0645c85..38216a1 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -39,16 +39,14 @@ static char printable_char(int c)
> > * drm_get_format_name - return a string for drm fourcc format
> > * @format: format to compute name of
> > *
> > - * Note that the buffer used by this function is globally shared and owned by
> > - * the function itself.
> > - *
> > - * FIXME: This isn't really multithreading safe.
> > + * Note that the buffer returned by this function is owned by the caller
> > + * and will need to be freed.
> > */
> > const char *drm_get_format_name(uint32_t format)
>
> I find it surprising that a function that allocates a buffer returns a
> const pointer. Some userspace libraries have conventions about the
> ownership based on constness.
>
> (I also find it suprising that kfree() takes a const pointer; arguably
> that call changes the memory.)
>
> Is there precedent for this?
>
> BR,
> Jani.
It's not a const pointer, it's a normal pointer to a const char, i.e.
you can do as you want with the pointer but you shouldn't change the
chars it points to.
Cheers,
Eric
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: make drm_get_format_name thread-safe
2016-08-15 12:59 ` Eric Engestrom
@ 2016-08-15 13:13 ` Jani Nikula
2016-08-15 13:52 ` Daniel Vetter
0 siblings, 1 reply; 11+ messages in thread
From: Jani Nikula @ 2016-08-15 13:13 UTC (permalink / raw)
To: Eric Engestrom
Cc: Eric Engestrom, linux-kernel, Tom St Denis, intel-gfx,
Michel Dänzer, Wei Yongjun, dri-devel, Junwei Zhang,
Xinliang Liu, David Zhang, Vitaly Prosyak, Alex Deucher,
Daniel Vetter, Flora Cui, Gustavo Padovan, Christian König
On Mon, 15 Aug 2016, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> On Mon, Aug 15, 2016 at 12:54:01PM +0300, Jani Nikula wrote:
>> On Mon, 15 Aug 2016, Eric Engestrom <eric@engestrom.ch> wrote:
>> > Signed-off-by: Eric Engestrom <eric@engestrom.ch>
>> > ---
>> >
>> > I moved the main bits to be the first diffs, shouldn't affect anything
>> > when applying the patch, but I wanted to ask:
>> > I don't like the hard-coded `32` the appears in both kmalloc() and
>> > snprintf(), what do you think? If you don't like it either, what would
>> > you suggest? Should I #define it?
>> >
>> > Second question is about the patch mail itself: should I send this kind
>> > of patch separated by module, with a note requesting them to be squashed
>> > when applying? It has to land as a single patch, but for review it might
>> > be easier if people only see the bits they each care about, as well as
>> > to collect ack's/r-b's.
>> >
>> > Cheers,
>> > Eric
>> >
>> > ---
>> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++--
>> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++--
>> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++--
>> > drivers/gpu/drm/drm_atomic.c | 5 ++--
>> > drivers/gpu/drm/drm_crtc.c | 21 ++++++++-----
>> > drivers/gpu/drm/drm_fourcc.c | 17 ++++++-----
>> > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 6 ++--
>> > drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-
>> > drivers/gpu/drm/i915/intel_atomic_plane.c | 6 ++--
>> > drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++---------
>> > drivers/gpu/drm/radeon/atombios_crtc.c | 12 +++++---
>> > include/drm/drm_fourcc.h | 2 +-
>> > 12 files changed, 89 insertions(+), 48 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>> > index 0645c85..38216a1 100644
>> > --- a/drivers/gpu/drm/drm_fourcc.c
>> > +++ b/drivers/gpu/drm/drm_fourcc.c
>> > @@ -39,16 +39,14 @@ static char printable_char(int c)
>> > * drm_get_format_name - return a string for drm fourcc format
>> > * @format: format to compute name of
>> > *
>> > - * Note that the buffer used by this function is globally shared and owned by
>> > - * the function itself.
>> > - *
>> > - * FIXME: This isn't really multithreading safe.
>> > + * Note that the buffer returned by this function is owned by the caller
>> > + * and will need to be freed.
>> > */
>> > const char *drm_get_format_name(uint32_t format)
>>
>> I find it surprising that a function that allocates a buffer returns a
>> const pointer. Some userspace libraries have conventions about the
>> ownership based on constness.
>>
>> (I also find it suprising that kfree() takes a const pointer; arguably
>> that call changes the memory.)
>>
>> Is there precedent for this?
>>
>> BR,
>> Jani.
>
> It's not a const pointer, it's a normal pointer to a const char, i.e.
> you can do as you want with the pointer but you shouldn't change the
> chars it points to.
Ermh, that's what I meant even if I was sloppy in my reply. And arguably
freeing the bytes the pointer points at changes them, albeit subtly. And
having a function return a pointer to const data is often an indication
that the ownership of the data isn't transfered, i.e. you're not
supposed to free it yourself.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: make drm_get_format_name thread-safe
2016-08-15 13:13 ` Jani Nikula
@ 2016-08-15 13:52 ` Daniel Vetter
2016-08-15 15:07 ` Eric Engestrom
2016-08-15 15:29 ` [FIXUP] drm: remove `const` attribute to hint at caller that they now own the memory Eric Engestrom
0 siblings, 2 replies; 11+ messages in thread
From: Daniel Vetter @ 2016-08-15 13:52 UTC (permalink / raw)
To: Jani Nikula
Cc: Eric Engestrom, Tom St Denis, intel-gfx, Eric Engestrom,
Michel Dänzer, linux-kernel, dri-devel, Xinliang Liu,
David Zhang, Wei Yongjun, Vitaly Prosyak, Daniel Vetter,
Junwei Zhang, Alex Deucher, Flora Cui, Gustavo Padovan,
Christian König
On Mon, Aug 15, 2016 at 04:13:54PM +0300, Jani Nikula wrote:
> On Mon, 15 Aug 2016, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> > On Mon, Aug 15, 2016 at 12:54:01PM +0300, Jani Nikula wrote:
> >> On Mon, 15 Aug 2016, Eric Engestrom <eric@engestrom.ch> wrote:
> >> > Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> >> > ---
> >> >
> >> > I moved the main bits to be the first diffs, shouldn't affect anything
> >> > when applying the patch, but I wanted to ask:
> >> > I don't like the hard-coded `32` the appears in both kmalloc() and
> >> > snprintf(), what do you think? If you don't like it either, what would
> >> > you suggest? Should I #define it?
> >> >
> >> > Second question is about the patch mail itself: should I send this kind
> >> > of patch separated by module, with a note requesting them to be squashed
> >> > when applying? It has to land as a single patch, but for review it might
> >> > be easier if people only see the bits they each care about, as well as
> >> > to collect ack's/r-b's.
> >> >
> >> > Cheers,
> >> > Eric
> >> >
> >> > ---
> >> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++--
> >> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++--
> >> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++--
> >> > drivers/gpu/drm/drm_atomic.c | 5 ++--
> >> > drivers/gpu/drm/drm_crtc.c | 21 ++++++++-----
> >> > drivers/gpu/drm/drm_fourcc.c | 17 ++++++-----
> >> > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 6 ++--
> >> > drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-
> >> > drivers/gpu/drm/i915/intel_atomic_plane.c | 6 ++--
> >> > drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++---------
> >> > drivers/gpu/drm/radeon/atombios_crtc.c | 12 +++++---
> >> > include/drm/drm_fourcc.h | 2 +-
> >> > 12 files changed, 89 insertions(+), 48 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> >> > index 0645c85..38216a1 100644
> >> > --- a/drivers/gpu/drm/drm_fourcc.c
> >> > +++ b/drivers/gpu/drm/drm_fourcc.c
> >> > @@ -39,16 +39,14 @@ static char printable_char(int c)
> >> > * drm_get_format_name - return a string for drm fourcc format
> >> > * @format: format to compute name of
> >> > *
> >> > - * Note that the buffer used by this function is globally shared and owned by
> >> > - * the function itself.
> >> > - *
> >> > - * FIXME: This isn't really multithreading safe.
> >> > + * Note that the buffer returned by this function is owned by the caller
> >> > + * and will need to be freed.
> >> > */
> >> > const char *drm_get_format_name(uint32_t format)
> >>
> >> I find it surprising that a function that allocates a buffer returns a
> >> const pointer. Some userspace libraries have conventions about the
> >> ownership based on constness.
> >>
> >> (I also find it suprising that kfree() takes a const pointer; arguably
> >> that call changes the memory.)
> >>
> >> Is there precedent for this?
> >>
> >> BR,
> >> Jani.
> >
> > It's not a const pointer, it's a normal pointer to a const char, i.e.
> > you can do as you want with the pointer but you shouldn't change the
> > chars it points to.
>
> Ermh, that's what I meant even if I was sloppy in my reply. And arguably
> freeing the bytes the pointer points at changes them, albeit subtly. And
> having a function return a pointer to const data is often an indication
> that the ownership of the data isn't transfered, i.e. you're not
> supposed to free it yourself.
I already applied the patch, but yes dropping the const would be a good
hint to callers that they now own that block of memory. Eric, can you pls
follow up with a fix up patch - drm-misc is non-rebasing?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm: make drm_get_format_name thread-safe
2016-08-15 13:52 ` Daniel Vetter
@ 2016-08-15 15:07 ` Eric Engestrom
2016-08-15 15:29 ` [FIXUP] drm: remove `const` attribute to hint at caller that they now own the memory Eric Engestrom
1 sibling, 0 replies; 11+ messages in thread
From: Eric Engestrom @ 2016-08-15 15:07 UTC (permalink / raw)
To: Jani Nikula, Tom St Denis, intel-gfx, Eric Engestrom,
Michel Dänzer, linux-kernel, dri-devel, Xinliang Liu,
David Zhang, Wei Yongjun, Vitaly Prosyak, Daniel Vetter,
Junwei Zhang, Alex Deucher, Flora Cui, Gustavo Padovan,
Christian König
On Mon, Aug 15, 2016 at 03:52:07PM +0200, Daniel Vetter wrote:
> On Mon, Aug 15, 2016 at 04:13:54PM +0300, Jani Nikula wrote:
> > On Mon, 15 Aug 2016, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> > > On Mon, Aug 15, 2016 at 12:54:01PM +0300, Jani Nikula wrote:
> > >> On Mon, 15 Aug 2016, Eric Engestrom <eric@engestrom.ch> wrote:
> > >> > Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> > >> > ---
> > >> >
> > >> > I moved the main bits to be the first diffs, shouldn't affect anything
> > >> > when applying the patch, but I wanted to ask:
> > >> > I don't like the hard-coded `32` the appears in both kmalloc() and
> > >> > snprintf(), what do you think? If you don't like it either, what would
> > >> > you suggest? Should I #define it?
> > >> >
> > >> > Second question is about the patch mail itself: should I send this kind
> > >> > of patch separated by module, with a note requesting them to be squashed
> > >> > when applying? It has to land as a single patch, but for review it might
> > >> > be easier if people only see the bits they each care about, as well as
> > >> > to collect ack's/r-b's.
> > >> >
> > >> > Cheers,
> > >> > Eric
> > >> >
> > >> > ---
> > >> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++--
> > >> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++--
> > >> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++--
> > >> > drivers/gpu/drm/drm_atomic.c | 5 ++--
> > >> > drivers/gpu/drm/drm_crtc.c | 21 ++++++++-----
> > >> > drivers/gpu/drm/drm_fourcc.c | 17 ++++++-----
> > >> > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 6 ++--
> > >> > drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-
> > >> > drivers/gpu/drm/i915/intel_atomic_plane.c | 6 ++--
> > >> > drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++---------
> > >> > drivers/gpu/drm/radeon/atombios_crtc.c | 12 +++++---
> > >> > include/drm/drm_fourcc.h | 2 +-
> > >> > 12 files changed, 89 insertions(+), 48 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > >> > index 0645c85..38216a1 100644
> > >> > --- a/drivers/gpu/drm/drm_fourcc.c
> > >> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > >> > @@ -39,16 +39,14 @@ static char printable_char(int c)
> > >> > * drm_get_format_name - return a string for drm fourcc format
> > >> > * @format: format to compute name of
> > >> > *
> > >> > - * Note that the buffer used by this function is globally shared and owned by
> > >> > - * the function itself.
> > >> > - *
> > >> > - * FIXME: This isn't really multithreading safe.
> > >> > + * Note that the buffer returned by this function is owned by the caller
> > >> > + * and will need to be freed.
> > >> > */
> > >> > const char *drm_get_format_name(uint32_t format)
> > >>
> > >> I find it surprising that a function that allocates a buffer returns a
> > >> const pointer. Some userspace libraries have conventions about the
> > >> ownership based on constness.
> > >>
> > >> (I also find it suprising that kfree() takes a const pointer; arguably
> > >> that call changes the memory.)
> > >>
> > >> Is there precedent for this?
> > >>
> > >> BR,
> > >> Jani.
> > >
> > > It's not a const pointer, it's a normal pointer to a const char, i.e.
> > > you can do as you want with the pointer but you shouldn't change the
> > > chars it points to.
> >
> > Ermh, that's what I meant even if I was sloppy in my reply. And arguably
> > freeing the bytes the pointer points at changes them, albeit subtly. And
> > having a function return a pointer to const data is often an indication
> > that the ownership of the data isn't transfered, i.e. you're not
> > supposed to free it yourself.
>
> I already applied the patch, but yes dropping the const would be a good
> hint to callers that they now own that block of memory. Eric, can you pls
> follow up with a fix up patch - drm-misc is non-rebasing?
> -Daniel
I didn't know about that convention. I'll send a fixup patch, but it'll
have to wait until tomorrow night. I hope that's not an issue :(
Cheers,
Eric
^ permalink raw reply [flat|nested] 11+ messages in thread
* [FIXUP] drm: remove `const` attribute to hint at caller that they now own the memory
2016-08-15 13:52 ` Daniel Vetter
2016-08-15 15:07 ` Eric Engestrom
@ 2016-08-15 15:29 ` Eric Engestrom
2016-08-16 11:04 ` Jani Nikula
1 sibling, 1 reply; 11+ messages in thread
From: Eric Engestrom @ 2016-08-15 15:29 UTC (permalink / raw)
To: linux-kernel; +Cc: Daniel Vetter, Jani Nikula, dri-devel, intel-gfx
Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
---
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +-
drivers/gpu/drm/drm_atomic.c | 2 +-
drivers/gpu/drm/drm_crtc.c | 8 ++++----
drivers/gpu/drm/drm_fourcc.c | 4 ++--
drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 2 +-
drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
drivers/gpu/drm/i915/intel_atomic_plane.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 4 ++--
drivers/gpu/drm/radeon/atombios_crtc.c | 4 ++--
include/drm/drm_fourcc.h | 2 +-
12 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 0bf8959..741da36 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2071,7 +2071,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;
- const char *format_name;
+ char *format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 1558a97..2282eb6 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2046,7 +2046,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;
- const char *format_name;
+ char *format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 71a0375..8b7ad34 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -1952,7 +1952,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;
- const char *format_name;
+ char *format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 087391f..5cb2e22 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -837,7 +837,7 @@ 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) {
- const char *format_name = drm_get_format_name(state->fb->pixel_format);
+ char *format_name = drm_get_format_name(state->fb->pixel_format);
DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
kfree(format_name);
return ret;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 7da5d33..30ab28b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2592,7 +2592,7 @@ 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) {
- const char *format_name = drm_get_format_name(fb->pixel_format);
+ char *format_name = drm_get_format_name(fb->pixel_format);
DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
kfree(format_name);
goto out;
@@ -2903,7 +2903,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
ret = drm_plane_check_pixel_format(crtc->primary,
fb->pixel_format);
if (ret) {
- const char *format_name = drm_get_format_name(fb->pixel_format);
+ char *format_name = drm_get_format_name(fb->pixel_format);
DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
kfree(format_name);
goto out;
@@ -3281,7 +3281,7 @@ int drm_mode_addfb(struct drm_device *dev,
static int format_check(const struct drm_mode_fb_cmd2 *r)
{
uint32_t format = r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN;
- const char *format_name;
+ char *format_name;
switch (format) {
case DRM_FORMAT_C8:
@@ -3359,7 +3359,7 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
ret = format_check(r);
if (ret) {
- const char *format_name = drm_get_format_name(r->pixel_format);
+ char *format_name = drm_get_format_name(r->pixel_format);
DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
kfree(format_name);
return ret;
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 38216a1..41d06d6 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -42,7 +42,7 @@ static char printable_char(int c)
* Note that the buffer returned by this function is owned by the caller
* and will need to be freed.
*/
-const char *drm_get_format_name(uint32_t format)
+char *drm_get_format_name(uint32_t format)
{
char *buf = kmalloc(32, GFP_KERNEL);
@@ -71,7 +71,7 @@ EXPORT_SYMBOL(drm_get_format_name);
void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
int *bpp)
{
- const char *format_name;
+ char *format_name;
switch (format) {
case DRM_FORMAT_C8:
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index ac7fa02..eaa3df7 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -608,7 +608,7 @@ 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);
- const char *format_name;
+ char *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;
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 904720b..e5cddea 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3109,7 +3109,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;
- const char *format_name;
+ char *format_name;
if (!plane->state) {
seq_puts(m, "plane->state is NULL!\n");
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index e06131a..28fe00d 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -157,7 +157,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
if (state->fb && intel_rotation_90_or_270(state->rotation)) {
- const char *format_name;
+ char *format_name;
if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index aaada15..b1d701f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12264,7 +12264,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) {
- const char *format_name;
+ char *format_name;
intel_plane = to_intel_plane(plane);
if (intel_plane->pipe != crtc->pipe)
continue;
@@ -14922,7 +14922,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
unsigned int aligned_height;
int ret;
u32 pitch_limit, stride_alignment;
- const char *format_name;
+ char *format_name;
WARN_ON(!mutex_is_locked(&dev->struct_mutex));
diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
index 981ca3f..a89c480 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -1154,7 +1154,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
u32 tmp, viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- const char *format_name;
+ char *format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -1471,7 +1471,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
u32 viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- const char *format_name;
+ char *format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 030d22d..b106337 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -32,6 +32,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);
-const char *drm_get_format_name(uint32_t format) __malloc;
+char *drm_get_format_name(uint32_t format) __malloc;
#endif /* __DRM_FOURCC_H__ */
--
2.9.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [FIXUP] drm: remove `const` attribute to hint at caller that they now own the memory
2016-08-15 15:29 ` [FIXUP] drm: remove `const` attribute to hint at caller that they now own the memory Eric Engestrom
@ 2016-08-16 11:04 ` Jani Nikula
2016-08-16 12:07 ` Daniel Vetter
0 siblings, 1 reply; 11+ messages in thread
From: Jani Nikula @ 2016-08-16 11:04 UTC (permalink / raw)
To: Eric Engestrom, linux-kernel; +Cc: Daniel Vetter, dri-devel, intel-gfx
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
On Mon, 15 Aug 2016, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
> ---
> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +-
> drivers/gpu/drm/drm_atomic.c | 2 +-
> drivers/gpu/drm/drm_crtc.c | 8 ++++----
> drivers/gpu/drm/drm_fourcc.c | 4 ++--
> drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 2 +-
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> drivers/gpu/drm/i915/intel_atomic_plane.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 4 ++--
> drivers/gpu/drm/radeon/atombios_crtc.c | 4 ++--
> include/drm/drm_fourcc.h | 2 +-
> 12 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index 0bf8959..741da36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2071,7 +2071,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;
> - const char *format_name;
> + char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index 1558a97..2282eb6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2046,7 +2046,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;
> - const char *format_name;
> + char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> index 71a0375..8b7ad34 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -1952,7 +1952,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;
> - const char *format_name;
> + char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 087391f..5cb2e22 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -837,7 +837,7 @@ 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) {
> - const char *format_name = drm_get_format_name(state->fb->pixel_format);
> + char *format_name = drm_get_format_name(state->fb->pixel_format);
> DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
> kfree(format_name);
> return ret;
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 7da5d33..30ab28b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2592,7 +2592,7 @@ 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) {
> - const char *format_name = drm_get_format_name(fb->pixel_format);
> + char *format_name = drm_get_format_name(fb->pixel_format);
> DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> kfree(format_name);
> goto out;
> @@ -2903,7 +2903,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> ret = drm_plane_check_pixel_format(crtc->primary,
> fb->pixel_format);
> if (ret) {
> - const char *format_name = drm_get_format_name(fb->pixel_format);
> + char *format_name = drm_get_format_name(fb->pixel_format);
> DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> kfree(format_name);
> goto out;
> @@ -3281,7 +3281,7 @@ int drm_mode_addfb(struct drm_device *dev,
> static int format_check(const struct drm_mode_fb_cmd2 *r)
> {
> uint32_t format = r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN;
> - const char *format_name;
> + char *format_name;
>
> switch (format) {
> case DRM_FORMAT_C8:
> @@ -3359,7 +3359,7 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>
> ret = format_check(r);
> if (ret) {
> - const char *format_name = drm_get_format_name(r->pixel_format);
> + char *format_name = drm_get_format_name(r->pixel_format);
> DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
> kfree(format_name);
> return ret;
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 38216a1..41d06d6 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -42,7 +42,7 @@ static char printable_char(int c)
> * Note that the buffer returned by this function is owned by the caller
> * and will need to be freed.
> */
> -const char *drm_get_format_name(uint32_t format)
> +char *drm_get_format_name(uint32_t format)
> {
> char *buf = kmalloc(32, GFP_KERNEL);
>
> @@ -71,7 +71,7 @@ EXPORT_SYMBOL(drm_get_format_name);
> void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> int *bpp)
> {
> - const char *format_name;
> + char *format_name;
>
> switch (format) {
> case DRM_FORMAT_C8:
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> index ac7fa02..eaa3df7 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> @@ -608,7 +608,7 @@ 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);
> - const char *format_name;
> + char *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;
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 904720b..e5cddea 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3109,7 +3109,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;
> - const char *format_name;
> + char *format_name;
>
> if (!plane->state) {
> seq_puts(m, "plane->state is NULL!\n");
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index e06131a..28fe00d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -157,7 +157,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>
> if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> - const char *format_name;
> + char *format_name;
> if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index aaada15..b1d701f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12264,7 +12264,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) {
> - const char *format_name;
> + char *format_name;
> intel_plane = to_intel_plane(plane);
> if (intel_plane->pipe != crtc->pipe)
> continue;
> @@ -14922,7 +14922,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
> unsigned int aligned_height;
> int ret;
> u32 pitch_limit, stride_alignment;
> - const char *format_name;
> + char *format_name;
>
> WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
> index 981ca3f..a89c480 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -1154,7 +1154,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
> u32 tmp, viewport_w, viewport_h;
> int r;
> bool bypass_lut = false;
> - const char *format_name;
> + char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -1471,7 +1471,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
> u32 viewport_w, viewport_h;
> int r;
> bool bypass_lut = false;
> - const char *format_name;
> + char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 030d22d..b106337 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -32,6 +32,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);
> -const char *drm_get_format_name(uint32_t format) __malloc;
> +char *drm_get_format_name(uint32_t format) __malloc;
>
> #endif /* __DRM_FOURCC_H__ */
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [FIXUP] drm: remove `const` attribute to hint at caller that they now own the memory
2016-08-16 11:04 ` Jani Nikula
@ 2016-08-16 12:07 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2016-08-16 12:07 UTC (permalink / raw)
To: Jani Nikula
Cc: Eric Engestrom, linux-kernel, Daniel Vetter, dri-devel, intel-gfx
On Tue, Aug 16, 2016 at 02:04:21PM +0300, Jani Nikula wrote:
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Applied to drm-misc, thanks.
-Daniel
>
>
> On Mon, 15 Aug 2016, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> > Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
> > ---
> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +-
> > drivers/gpu/drm/drm_atomic.c | 2 +-
> > drivers/gpu/drm/drm_crtc.c | 8 ++++----
> > drivers/gpu/drm/drm_fourcc.c | 4 ++--
> > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 2 +-
> > drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> > drivers/gpu/drm/i915/intel_atomic_plane.c | 2 +-
> > drivers/gpu/drm/i915/intel_display.c | 4 ++--
> > drivers/gpu/drm/radeon/atombios_crtc.c | 4 ++--
> > include/drm/drm_fourcc.h | 2 +-
> > 12 files changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > index 0bf8959..741da36 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > @@ -2071,7 +2071,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;
> > - const char *format_name;
> > + char *format_name;
> >
> > /* no fb bound */
> > if (!atomic && !crtc->primary->fb) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > index 1558a97..2282eb6 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > @@ -2046,7 +2046,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;
> > - const char *format_name;
> > + char *format_name;
> >
> > /* no fb bound */
> > if (!atomic && !crtc->primary->fb) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> > index 71a0375..8b7ad34 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> > @@ -1952,7 +1952,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;
> > - const char *format_name;
> > + char *format_name;
> >
> > /* no fb bound */
> > if (!atomic && !crtc->primary->fb) {
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 087391f..5cb2e22 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -837,7 +837,7 @@ 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) {
> > - const char *format_name = drm_get_format_name(state->fb->pixel_format);
> > + char *format_name = drm_get_format_name(state->fb->pixel_format);
> > DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
> > kfree(format_name);
> > return ret;
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 7da5d33..30ab28b 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -2592,7 +2592,7 @@ 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) {
> > - const char *format_name = drm_get_format_name(fb->pixel_format);
> > + char *format_name = drm_get_format_name(fb->pixel_format);
> > DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> > kfree(format_name);
> > goto out;
> > @@ -2903,7 +2903,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> > ret = drm_plane_check_pixel_format(crtc->primary,
> > fb->pixel_format);
> > if (ret) {
> > - const char *format_name = drm_get_format_name(fb->pixel_format);
> > + char *format_name = drm_get_format_name(fb->pixel_format);
> > DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> > kfree(format_name);
> > goto out;
> > @@ -3281,7 +3281,7 @@ int drm_mode_addfb(struct drm_device *dev,
> > static int format_check(const struct drm_mode_fb_cmd2 *r)
> > {
> > uint32_t format = r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN;
> > - const char *format_name;
> > + char *format_name;
> >
> > switch (format) {
> > case DRM_FORMAT_C8:
> > @@ -3359,7 +3359,7 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> >
> > ret = format_check(r);
> > if (ret) {
> > - const char *format_name = drm_get_format_name(r->pixel_format);
> > + char *format_name = drm_get_format_name(r->pixel_format);
> > DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
> > kfree(format_name);
> > return ret;
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 38216a1..41d06d6 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -42,7 +42,7 @@ static char printable_char(int c)
> > * Note that the buffer returned by this function is owned by the caller
> > * and will need to be freed.
> > */
> > -const char *drm_get_format_name(uint32_t format)
> > +char *drm_get_format_name(uint32_t format)
> > {
> > char *buf = kmalloc(32, GFP_KERNEL);
> >
> > @@ -71,7 +71,7 @@ EXPORT_SYMBOL(drm_get_format_name);
> > void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> > int *bpp)
> > {
> > - const char *format_name;
> > + char *format_name;
> >
> > switch (format) {
> > case DRM_FORMAT_C8:
> > diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> > index ac7fa02..eaa3df7 100644
> > --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> > +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> > @@ -608,7 +608,7 @@ 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);
> > - const char *format_name;
> > + char *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;
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 904720b..e5cddea 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -3109,7 +3109,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;
> > - const char *format_name;
> > + char *format_name;
> >
> > if (!plane->state) {
> > seq_puts(m, "plane->state is NULL!\n");
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index e06131a..28fe00d 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -157,7 +157,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
> >
> > if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> > - const char *format_name;
> > + char *format_name;
> > if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> > state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> > DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index aaada15..b1d701f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12264,7 +12264,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) {
> > - const char *format_name;
> > + char *format_name;
> > intel_plane = to_intel_plane(plane);
> > if (intel_plane->pipe != crtc->pipe)
> > continue;
> > @@ -14922,7 +14922,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
> > unsigned int aligned_height;
> > int ret;
> > u32 pitch_limit, stride_alignment;
> > - const char *format_name;
> > + char *format_name;
> >
> > WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >
> > diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
> > index 981ca3f..a89c480 100644
> > --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> > +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> > @@ -1154,7 +1154,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
> > u32 tmp, viewport_w, viewport_h;
> > int r;
> > bool bypass_lut = false;
> > - const char *format_name;
> > + char *format_name;
> >
> > /* no fb bound */
> > if (!atomic && !crtc->primary->fb) {
> > @@ -1471,7 +1471,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
> > u32 viewport_w, viewport_h;
> > int r;
> > bool bypass_lut = false;
> > - const char *format_name;
> > + char *format_name;
> >
> > /* no fb bound */
> > if (!atomic && !crtc->primary->fb) {
> > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > index 030d22d..b106337 100644
> > --- a/include/drm/drm_fourcc.h
> > +++ b/include/drm/drm_fourcc.h
> > @@ -32,6 +32,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);
> > -const char *drm_get_format_name(uint32_t format) __malloc;
> > +char *drm_get_format_name(uint32_t format) __malloc;
> >
> > #endif /* __DRM_FOURCC_H__ */
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe
2016-08-15 0:02 [PATCH] drm: make drm_get_format_name thread-safe Eric Engestrom
2016-08-15 9:54 ` Jani Nikula
@ 2016-11-03 18:52 ` Rob Clark
2016-11-08 8:43 ` Daniel Vetter
1 sibling, 1 reply; 11+ messages in thread
From: Rob Clark @ 2016-11-03 18:52 UTC (permalink / raw)
To: Eric Engestrom
Cc: Linux Kernel Mailing List, Tom St Denis, Archit Taneja,
David Airlie, dri-devel, Michel Dänzer, Wei Yongjun,
Junwei Zhang, Xinliang Liu, David Zhang, Xinwei Kong,
Vitaly Prosyak, Intel Graphics Development, Alex Deucher,
Daniel Vetter, Flora Cui, Gustavo Padovan, Christian König
On Sun, Aug 14, 2016 at 8:02 PM, Eric Engestrom <eric@engestrom.ch> wrote:
> Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> ---
>
> I moved the main bits to be the first diffs, shouldn't affect anything
> when applying the patch, but I wanted to ask:
> I don't like the hard-coded `32` the appears in both kmalloc() and
> snprintf(), what do you think? If you don't like it either, what would
> you suggest? Should I #define it?
>
> Second question is about the patch mail itself: should I send this kind
> of patch separated by module, with a note requesting them to be squashed
> when applying? It has to land as a single patch, but for review it might
> be easier if people only see the bits they each care about, as well as
> to collect ack's/r-b's.
>
> Cheers,
> Eric
>
> ---
> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++--
> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++--
> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++--
> drivers/gpu/drm/drm_atomic.c | 5 ++--
> drivers/gpu/drm/drm_crtc.c | 21 ++++++++-----
> drivers/gpu/drm/drm_fourcc.c | 17 ++++++-----
> drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 6 ++--
> drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-
> drivers/gpu/drm/i915/intel_atomic_plane.c | 6 ++--
> drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++---------
> drivers/gpu/drm/radeon/atombios_crtc.c | 12 +++++---
> include/drm/drm_fourcc.h | 2 +-
> 12 files changed, 89 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 0645c85..38216a1 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -39,16 +39,14 @@ static char printable_char(int c)
> * drm_get_format_name - return a string for drm fourcc format
> * @format: format to compute name of
> *
> - * Note that the buffer used by this function is globally shared and owned by
> - * the function itself.
> - *
> - * FIXME: This isn't really multithreading safe.
> + * Note that the buffer returned by this function is owned by the caller
> + * and will need to be freed.
> */
> const char *drm_get_format_name(uint32_t format)
> {
> - static char buf[32];
> + char *buf = kmalloc(32, GFP_KERNEL);
hmm, I guess I wasn't paying attention at the time this patch came by,
but unfortunately this makes drm_get_format_name() no longer safe in
atomic contexts :-/
We should probably either revert this or have two variants of
drm_get_format_name()? (ie. one that is not thread safe but is good
enough for debugging)
BR,
-R
> - snprintf(buf, sizeof(buf),
> + snprintf(buf, 32,
> "%c%c%c%c %s-endian (0x%08x)",
> printable_char(format & 0xff),
> printable_char((format >> 8) & 0xff),
> @@ -73,6 +71,8 @@ EXPORT_SYMBOL(drm_get_format_name);
> void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> int *bpp)
> {
> + const char *format_name;
> +
> switch (format) {
> case DRM_FORMAT_C8:
> case DRM_FORMAT_RGB332:
> @@ -127,8 +127,9 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> *bpp = 32;
> break;
> default:
> - DRM_DEBUG_KMS("unsupported pixel format %s\n",
> - drm_get_format_name(format));
> + format_name = drm_get_format_name(format);
> + DRM_DEBUG_KMS("unsupported pixel format %s\n", format_name);
> + kfree(format_name);
> *depth = 0;
> *bpp = 0;
> break;
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 7f90a39..030d22d 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -32,6 +32,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);
> -const char *drm_get_format_name(uint32_t format);
> +const char *drm_get_format_name(uint32_t format) __malloc;
>
> #endif /* __DRM_FOURCC_H__ */
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index c1b04e9..0bf8959 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2071,6 +2071,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;
> + const char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -2182,8 +2183,9 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
> bypass_lut = true;
> break;
> default:
> - DRM_ERROR("Unsupported screen format %s\n",
> - drm_get_format_name(target_fb->pixel_format));
> + format_name = drm_get_format_name(target_fb->pixel_format);
> + DRM_ERROR("Unsupported screen format %s\n", 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 d4bf133..1558a97 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2046,6 +2046,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;
> + const char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -2157,8 +2158,9 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
> bypass_lut = true;
> break;
> default:
> - DRM_ERROR("Unsupported screen format %s\n",
> - drm_get_format_name(target_fb->pixel_format));
> + format_name = drm_get_format_name(target_fb->pixel_format);
> + DRM_ERROR("Unsupported screen format %s\n", format_name);
> + kfree(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 4fdfab1..71a0375 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -1952,6 +1952,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;
> + const char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -2056,8 +2057,9 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
> bypass_lut = true;
> break;
> default:
> - DRM_ERROR("Unsupported screen format %s\n",
> - drm_get_format_name(target_fb->pixel_format));
> + format_name = drm_get_format_name(target_fb->pixel_format);
> + DRM_ERROR("Unsupported screen format %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fa39307..087391f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -837,8 +837,9 @@ 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) {
> - DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
> - drm_get_format_name(state->fb->pixel_format));
> + const char *format_name = drm_get_format_name(state->fb->pixel_format);
> + DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
> + kfree(format_name);
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b1dbb60..7da5d33 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2592,8 +2592,9 @@ 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) {
> - DRM_DEBUG_KMS("Invalid pixel format %s\n",
> - drm_get_format_name(fb->pixel_format));
> + const char *format_name = drm_get_format_name(fb->pixel_format);
> + DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> + kfree(format_name);
> goto out;
> }
>
> @@ -2902,8 +2903,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> ret = drm_plane_check_pixel_format(crtc->primary,
> fb->pixel_format);
> if (ret) {
> - DRM_DEBUG_KMS("Invalid pixel format %s\n",
> - drm_get_format_name(fb->pixel_format));
> + const char *format_name = drm_get_format_name(fb->pixel_format);
> + DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> + kfree(format_name);
> goto out;
> }
> }
> @@ -3279,6 +3281,7 @@ int drm_mode_addfb(struct drm_device *dev,
> static int format_check(const struct drm_mode_fb_cmd2 *r)
> {
> uint32_t format = r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN;
> + const char *format_name;
>
> switch (format) {
> case DRM_FORMAT_C8:
> @@ -3343,8 +3346,9 @@ static int format_check(const struct drm_mode_fb_cmd2 *r)
> case DRM_FORMAT_YVU444:
> return 0;
> default:
> - DRM_DEBUG_KMS("invalid pixel format %s\n",
> - drm_get_format_name(r->pixel_format));
> + format_name = drm_get_format_name(r->pixel_format);
> + DRM_DEBUG_KMS("invalid pixel format %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> }
> @@ -3355,8 +3359,9 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>
> ret = format_check(r);
> if (ret) {
> - DRM_DEBUG_KMS("bad framebuffer format %s\n",
> - drm_get_format_name(r->pixel_format));
> + const char *format_name = drm_get_format_name(r->pixel_format);
> + DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
> + kfree(format_name);
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> index c3707d4..ac7fa02 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> @@ -608,15 +608,17 @@ 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);
> + const char *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,
> - drm_get_format_name(fb->pixel_format));
> + addr, fb->width, fb->height, fmt, 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 844fea7..904720b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3109,6 +3109,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;
> + const char *format_name;
>
> if (!plane->state) {
> seq_puts(m, "plane->state is NULL!\n");
> @@ -3117,6 +3118,12 @@ 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);
> + } else {
> + format_name = kstrdup("N/A", GFP_KERNEL);
> + }
> +
> 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",
> plane->base.id,
> plane_type(intel_plane->base.type),
> @@ -3130,8 +3137,10 @@ 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,
> - state->fb ? drm_get_format_name(state->fb->pixel_format) : "N/A",
> + format_name,
> 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 7de7721..e06131a 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -157,6 +157,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>
> if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> + const char *format_name;
> if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> @@ -171,8 +172,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> switch (state->fb->pixel_format) {
> case DRM_FORMAT_C8:
> case DRM_FORMAT_RGB565:
> - DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> - drm_get_format_name(state->fb->pixel_format));
> + format_name = drm_get_format_name(state->fb->pixel_format);
> + DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", 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 c457eed..071399b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12282,6 +12282,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) {
> + const char *format_name;
> intel_plane = to_intel_plane(plane);
> if (intel_plane->pipe != crtc->pipe)
> continue;
> @@ -12294,11 +12295,12 @@ 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,
> - drm_get_format_name(fb->pixel_format));
> + fb->base.id, fb->width, fb->height, format_name);
> DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
> state->scaler_id,
> state->src.x1 >> 16, state->src.y1 >> 16,
> @@ -12307,6 +12309,8 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> state->dst.x1, state->dst.y1,
> drm_rect_width(&state->dst),
> drm_rect_height(&state->dst));
> +
> + kfree(format_name);
> }
> }
>
> @@ -14936,6 +14940,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
> unsigned int aligned_height;
> int ret;
> u32 pitch_limit, stride_alignment;
> + const char *format_name;
>
> WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> @@ -15009,16 +15014,18 @@ static int intel_framebuffer_init(struct drm_device *dev,
> break;
> case DRM_FORMAT_XRGB1555:
> if (INTEL_INFO(dev)->gen > 3) {
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> break;
> case DRM_FORMAT_ABGR8888:
> if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) &&
> INTEL_INFO(dev)->gen < 9) {
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> break;
> @@ -15026,15 +15033,17 @@ static int intel_framebuffer_init(struct drm_device *dev,
> case DRM_FORMAT_XRGB2101010:
> case DRM_FORMAT_XBGR2101010:
> if (INTEL_INFO(dev)->gen < 4) {
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> break;
> case DRM_FORMAT_ABGR2101010:
> if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> break;
> @@ -15043,14 +15052,16 @@ static int intel_framebuffer_init(struct drm_device *dev,
> case DRM_FORMAT_YVYU:
> case DRM_FORMAT_VYUY:
> if (INTEL_INFO(dev)->gen < 5) {
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
> break;
> default:
> - DRM_DEBUG("unsupported pixel format: %s\n",
> - drm_get_format_name(mode_cmd->pixel_format));
> + format_name = drm_get_format_name(mode_cmd->pixel_format);
> + DRM_DEBUG("unsupported pixel format: %s\n", 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 a97abc8..981ca3f 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -1154,6 +1154,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
> u32 tmp, viewport_w, viewport_h;
> int r;
> bool bypass_lut = false;
> + const char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -1257,8 +1258,9 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
> bypass_lut = true;
> break;
> default:
> - DRM_ERROR("Unsupported screen format %s\n",
> - drm_get_format_name(target_fb->pixel_format));
> + format_name = drm_get_format_name(target_fb->pixel_format);
> + DRM_ERROR("Unsupported screen format %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
>
> @@ -1469,6 +1471,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
> u32 viewport_w, viewport_h;
> int r;
> bool bypass_lut = false;
> + const char *format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -1558,8 +1561,9 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
> bypass_lut = true;
> break;
> default:
> - DRM_ERROR("Unsupported screen format %s\n",
> - drm_get_format_name(target_fb->pixel_format));
> + format_name = drm_get_format_name(target_fb->pixel_format);
> + DRM_ERROR("Unsupported screen format %s\n", format_name);
> + kfree(format_name);
> return -EINVAL;
> }
>
> --
> 2.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe
2016-11-03 18:52 ` [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe Rob Clark
@ 2016-11-08 8:43 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2016-11-08 8:43 UTC (permalink / raw)
To: Rob Clark
Cc: Eric Engestrom, Tom St Denis, Archit Taneja, Flora Cui,
David Airlie, Intel Graphics Development, Michel Dänzer,
Linux Kernel Mailing List, dri-devel, Xinliang Liu, David Zhang,
Wei Yongjun, Vitaly Prosyak, Daniel Vetter, Junwei Zhang,
Alex Deucher, Xinwei Kong, Gustavo Padovan, Christian König
On Thu, Nov 03, 2016 at 02:52:00PM -0400, Rob Clark wrote:
> On Sun, Aug 14, 2016 at 8:02 PM, Eric Engestrom <eric@engestrom.ch> wrote:
> > Signed-off-by: Eric Engestrom <eric@engestrom.ch>
> > ---
> >
> > I moved the main bits to be the first diffs, shouldn't affect anything
> > when applying the patch, but I wanted to ask:
> > I don't like the hard-coded `32` the appears in both kmalloc() and
> > snprintf(), what do you think? If you don't like it either, what would
> > you suggest? Should I #define it?
> >
> > Second question is about the patch mail itself: should I send this kind
> > of patch separated by module, with a note requesting them to be squashed
> > when applying? It has to land as a single patch, but for review it might
> > be easier if people only see the bits they each care about, as well as
> > to collect ack's/r-b's.
> >
> > Cheers,
> > Eric
> >
> > ---
> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++--
> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++--
> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++--
> > drivers/gpu/drm/drm_atomic.c | 5 ++--
> > drivers/gpu/drm/drm_crtc.c | 21 ++++++++-----
> > drivers/gpu/drm/drm_fourcc.c | 17 ++++++-----
> > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 6 ++--
> > drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-
> > drivers/gpu/drm/i915/intel_atomic_plane.c | 6 ++--
> > drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++---------
> > drivers/gpu/drm/radeon/atombios_crtc.c | 12 +++++---
> > include/drm/drm_fourcc.h | 2 +-
> > 12 files changed, 89 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 0645c85..38216a1 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -39,16 +39,14 @@ static char printable_char(int c)
> > * drm_get_format_name - return a string for drm fourcc format
> > * @format: format to compute name of
> > *
> > - * Note that the buffer used by this function is globally shared and owned by
> > - * the function itself.
> > - *
> > - * FIXME: This isn't really multithreading safe.
> > + * Note that the buffer returned by this function is owned by the caller
> > + * and will need to be freed.
> > */
> > const char *drm_get_format_name(uint32_t format)
> > {
> > - static char buf[32];
> > + char *buf = kmalloc(32, GFP_KERNEL);
>
>
> hmm, I guess I wasn't paying attention at the time this patch came by,
> but unfortunately this makes drm_get_format_name() no longer safe in
> atomic contexts :-/
>
> We should probably either revert this or have two variants of
> drm_get_format_name()? (ie. one that is not thread safe but is good
> enough for debugging)
Where do we need that for atomic contexts? I guess worst-case we could do
an auto-GFP trick using drm_can_sleep or something like that.
-Daniel
>
> BR,
> -R
>
> > - snprintf(buf, sizeof(buf),
> > + snprintf(buf, 32,
> > "%c%c%c%c %s-endian (0x%08x)",
> > printable_char(format & 0xff),
> > printable_char((format >> 8) & 0xff),
> > @@ -73,6 +71,8 @@ EXPORT_SYMBOL(drm_get_format_name);
> > void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> > int *bpp)
> > {
> > + const char *format_name;
> > +
> > switch (format) {
> > case DRM_FORMAT_C8:
> > case DRM_FORMAT_RGB332:
> > @@ -127,8 +127,9 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> > *bpp = 32;
> > break;
> > default:
> > - DRM_DEBUG_KMS("unsupported pixel format %s\n",
> > - drm_get_format_name(format));
> > + format_name = drm_get_format_name(format);
> > + DRM_DEBUG_KMS("unsupported pixel format %s\n", format_name);
> > + kfree(format_name);
> > *depth = 0;
> > *bpp = 0;
> > break;
> > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > index 7f90a39..030d22d 100644
> > --- a/include/drm/drm_fourcc.h
> > +++ b/include/drm/drm_fourcc.h
> > @@ -32,6 +32,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);
> > -const char *drm_get_format_name(uint32_t format);
> > +const char *drm_get_format_name(uint32_t format) __malloc;
> >
> > #endif /* __DRM_FOURCC_H__ */
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > index c1b04e9..0bf8959 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > @@ -2071,6 +2071,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;
> > + const char *format_name;
> >
> > /* no fb bound */
> > if (!atomic && !crtc->primary->fb) {
> > @@ -2182,8 +2183,9 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
> > bypass_lut = true;
> > break;
> > default:
> > - DRM_ERROR("Unsupported screen format %s\n",
> > - drm_get_format_name(target_fb->pixel_format));
> > + format_name = drm_get_format_name(target_fb->pixel_format);
> > + DRM_ERROR("Unsupported screen format %s\n", 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 d4bf133..1558a97 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > @@ -2046,6 +2046,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;
> > + const char *format_name;
> >
> > /* no fb bound */
> > if (!atomic && !crtc->primary->fb) {
> > @@ -2157,8 +2158,9 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
> > bypass_lut = true;
> > break;
> > default:
> > - DRM_ERROR("Unsupported screen format %s\n",
> > - drm_get_format_name(target_fb->pixel_format));
> > + format_name = drm_get_format_name(target_fb->pixel_format);
> > + DRM_ERROR("Unsupported screen format %s\n", format_name);
> > + kfree(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 4fdfab1..71a0375 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> > @@ -1952,6 +1952,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;
> > + const char *format_name;
> >
> > /* no fb bound */
> > if (!atomic && !crtc->primary->fb) {
> > @@ -2056,8 +2057,9 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
> > bypass_lut = true;
> > break;
> > default:
> > - DRM_ERROR("Unsupported screen format %s\n",
> > - drm_get_format_name(target_fb->pixel_format));
> > + format_name = drm_get_format_name(target_fb->pixel_format);
> > + DRM_ERROR("Unsupported screen format %s\n", format_name);
> > + kfree(format_name);
> > return -EINVAL;
> > }
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index fa39307..087391f 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -837,8 +837,9 @@ 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) {
> > - DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
> > - drm_get_format_name(state->fb->pixel_format));
> > + const char *format_name = drm_get_format_name(state->fb->pixel_format);
> > + DRM_DEBUG_ATOMIC("Invalid pixel format %s\n", format_name);
> > + kfree(format_name);
> > return ret;
> > }
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index b1dbb60..7da5d33 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -2592,8 +2592,9 @@ 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) {
> > - DRM_DEBUG_KMS("Invalid pixel format %s\n",
> > - drm_get_format_name(fb->pixel_format));
> > + const char *format_name = drm_get_format_name(fb->pixel_format);
> > + DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> > + kfree(format_name);
> > goto out;
> > }
> >
> > @@ -2902,8 +2903,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> > ret = drm_plane_check_pixel_format(crtc->primary,
> > fb->pixel_format);
> > if (ret) {
> > - DRM_DEBUG_KMS("Invalid pixel format %s\n",
> > - drm_get_format_name(fb->pixel_format));
> > + const char *format_name = drm_get_format_name(fb->pixel_format);
> > + DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
> > + kfree(format_name);
> > goto out;
> > }
> > }
> > @@ -3279,6 +3281,7 @@ int drm_mode_addfb(struct drm_device *dev,
> > static int format_check(const struct drm_mode_fb_cmd2 *r)
> > {
> > uint32_t format = r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN;
> > + const char *format_name;
> >
> > switch (format) {
> > case DRM_FORMAT_C8:
> > @@ -3343,8 +3346,9 @@ static int format_check(const struct drm_mode_fb_cmd2 *r)
> > case DRM_FORMAT_YVU444:
> > return 0;
> > default:
> > - DRM_DEBUG_KMS("invalid pixel format %s\n",
> > - drm_get_format_name(r->pixel_format));
> > + format_name = drm_get_format_name(r->pixel_format);
> > + DRM_DEBUG_KMS("invalid pixel format %s\n", format_name);
> > + kfree(format_name);
> > return -EINVAL;
> > }
> > }
> > @@ -3355,8 +3359,9 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> >
> > ret = format_check(r);
> > if (ret) {
> > - DRM_DEBUG_KMS("bad framebuffer format %s\n",
> > - drm_get_format_name(r->pixel_format));
> > + const char *format_name = drm_get_format_name(r->pixel_format);
> > + DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
> > + kfree(format_name);
> > return ret;
> > }
> >
> > diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> > index c3707d4..ac7fa02 100644
> > --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> > +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> > @@ -608,15 +608,17 @@ 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);
> > + const char *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,
> > - drm_get_format_name(fb->pixel_format));
> > + addr, fb->width, fb->height, fmt, 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 844fea7..904720b 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -3109,6 +3109,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;
> > + const char *format_name;
> >
> > if (!plane->state) {
> > seq_puts(m, "plane->state is NULL!\n");
> > @@ -3117,6 +3118,12 @@ 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);
> > + } else {
> > + format_name = kstrdup("N/A", GFP_KERNEL);
> > + }
> > +
> > 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",
> > plane->base.id,
> > plane_type(intel_plane->base.type),
> > @@ -3130,8 +3137,10 @@ 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,
> > - state->fb ? drm_get_format_name(state->fb->pixel_format) : "N/A",
> > + format_name,
> > 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 7de7721..e06131a 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -157,6 +157,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
> >
> > if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> > + const char *format_name;
> > if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> > state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> > DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> > @@ -171,8 +172,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > switch (state->fb->pixel_format) {
> > case DRM_FORMAT_C8:
> > case DRM_FORMAT_RGB565:
> > - DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> > - drm_get_format_name(state->fb->pixel_format));
> > + format_name = drm_get_format_name(state->fb->pixel_format);
> > + DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n", 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 c457eed..071399b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12282,6 +12282,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) {
> > + const char *format_name;
> > intel_plane = to_intel_plane(plane);
> > if (intel_plane->pipe != crtc->pipe)
> > continue;
> > @@ -12294,11 +12295,12 @@ 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,
> > - drm_get_format_name(fb->pixel_format));
> > + fb->base.id, fb->width, fb->height, format_name);
> > DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
> > state->scaler_id,
> > state->src.x1 >> 16, state->src.y1 >> 16,
> > @@ -12307,6 +12309,8 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> > state->dst.x1, state->dst.y1,
> > drm_rect_width(&state->dst),
> > drm_rect_height(&state->dst));
> > +
> > + kfree(format_name);
> > }
> > }
> >
> > @@ -14936,6 +14940,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
> > unsigned int aligned_height;
> > int ret;
> > u32 pitch_limit, stride_alignment;
> > + const char *format_name;
> >
> > WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >
> > @@ -15009,16 +15014,18 @@ static int intel_framebuffer_init(struct drm_device *dev,
> > break;
> > case DRM_FORMAT_XRGB1555:
> > if (INTEL_INFO(dev)->gen > 3) {
> > - DRM_DEBUG("unsupported pixel format: %s\n",
> > - drm_get_format_name(mode_cmd->pixel_format));
> > + format_name = drm_get_format_name(mode_cmd->pixel_format);
> > + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> > + kfree(format_name);
> > return -EINVAL;
> > }
> > break;
> > case DRM_FORMAT_ABGR8888:
> > if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) &&
> > INTEL_INFO(dev)->gen < 9) {
> > - DRM_DEBUG("unsupported pixel format: %s\n",
> > - drm_get_format_name(mode_cmd->pixel_format));
> > + format_name = drm_get_format_name(mode_cmd->pixel_format);
> > + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> > + kfree(format_name);
> > return -EINVAL;
> > }
> > break;
> > @@ -15026,15 +15033,17 @@ static int intel_framebuffer_init(struct drm_device *dev,
> > case DRM_FORMAT_XRGB2101010:
> > case DRM_FORMAT_XBGR2101010:
> > if (INTEL_INFO(dev)->gen < 4) {
> > - DRM_DEBUG("unsupported pixel format: %s\n",
> > - drm_get_format_name(mode_cmd->pixel_format));
> > + format_name = drm_get_format_name(mode_cmd->pixel_format);
> > + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> > + kfree(format_name);
> > return -EINVAL;
> > }
> > break;
> > case DRM_FORMAT_ABGR2101010:
> > if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> > - DRM_DEBUG("unsupported pixel format: %s\n",
> > - drm_get_format_name(mode_cmd->pixel_format));
> > + format_name = drm_get_format_name(mode_cmd->pixel_format);
> > + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> > + kfree(format_name);
> > return -EINVAL;
> > }
> > break;
> > @@ -15043,14 +15052,16 @@ static int intel_framebuffer_init(struct drm_device *dev,
> > case DRM_FORMAT_YVYU:
> > case DRM_FORMAT_VYUY:
> > if (INTEL_INFO(dev)->gen < 5) {
> > - DRM_DEBUG("unsupported pixel format: %s\n",
> > - drm_get_format_name(mode_cmd->pixel_format));
> > + format_name = drm_get_format_name(mode_cmd->pixel_format);
> > + DRM_DEBUG("unsupported pixel format: %s\n", format_name);
> > + kfree(format_name);
> > return -EINVAL;
> > }
> > break;
> > default:
> > - DRM_DEBUG("unsupported pixel format: %s\n",
> > - drm_get_format_name(mode_cmd->pixel_format));
> > + format_name = drm_get_format_name(mode_cmd->pixel_format);
> > + DRM_DEBUG("unsupported pixel format: %s\n", 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 a97abc8..981ca3f 100644
> > --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> > +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> > @@ -1154,6 +1154,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
> > u32 tmp, viewport_w, viewport_h;
> > int r;
> > bool bypass_lut = false;
> > + const char *format_name;
> >
> > /* no fb bound */
> > if (!atomic && !crtc->primary->fb) {
> > @@ -1257,8 +1258,9 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
> > bypass_lut = true;
> > break;
> > default:
> > - DRM_ERROR("Unsupported screen format %s\n",
> > - drm_get_format_name(target_fb->pixel_format));
> > + format_name = drm_get_format_name(target_fb->pixel_format);
> > + DRM_ERROR("Unsupported screen format %s\n", format_name);
> > + kfree(format_name);
> > return -EINVAL;
> > }
> >
> > @@ -1469,6 +1471,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
> > u32 viewport_w, viewport_h;
> > int r;
> > bool bypass_lut = false;
> > + const char *format_name;
> >
> > /* no fb bound */
> > if (!atomic && !crtc->primary->fb) {
> > @@ -1558,8 +1561,9 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
> > bypass_lut = true;
> > break;
> > default:
> > - DRM_ERROR("Unsupported screen format %s\n",
> > - drm_get_format_name(target_fb->pixel_format));
> > + format_name = drm_get_format_name(target_fb->pixel_format);
> > + DRM_ERROR("Unsupported screen format %s\n", format_name);
> > + kfree(format_name);
> > return -EINVAL;
> > }
> >
> > --
> > 2.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-11-08 8:43 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-15 0:02 [PATCH] drm: make drm_get_format_name thread-safe Eric Engestrom
2016-08-15 9:54 ` Jani Nikula
2016-08-15 12:59 ` Eric Engestrom
2016-08-15 13:13 ` Jani Nikula
2016-08-15 13:52 ` Daniel Vetter
2016-08-15 15:07 ` Eric Engestrom
2016-08-15 15:29 ` [FIXUP] drm: remove `const` attribute to hint at caller that they now own the memory Eric Engestrom
2016-08-16 11:04 ` Jani Nikula
2016-08-16 12:07 ` Daniel Vetter
2016-11-03 18:52 ` [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe Rob Clark
2016-11-08 8:43 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).