| Message ID | 20160815000247.20063-1-eric@engestrom.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
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
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.
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
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
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
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
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; }
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(-)