All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: "Eric Engestrom" <eric@engestrom.ch>,
	"Tom St Denis" <tom.stdenis@amd.com>,
	"Archit Taneja" <architt@codeaurora.org>,
	"Flora Cui" <Flora.Cui@amd.com>,
	"David Airlie" <airlied@linux.ie>,
	"Intel Graphics Development" <intel-gfx@lists.freedesktop.org>,
	"Michel Dänzer" <michel.daenzer@amd.com>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Xinliang Liu" <z.liuxinliang@hisilicon.com>,
	"David Zhang" <david1.zhang@amd.com>,
	"Wei Yongjun" <yongjun_wei@trendmicro.com.cn>,
	"Vitaly Prosyak" <vitaly.prosyak@amd.com>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Junwei Zhang" <Jerry.Zhang@amd.com>,
	"Alex Deucher" <alexander.deucher@amd.com>,
	"Xinwei Kong" <kong.kongxinwei@hisilicon.com>,
	"Gustavo Padovan" <gustavo.padovan@collabora.co.uk>,
	"Christian König" <christian.koenig@amd.com>
Subject: Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe
Date: Tue, 8 Nov 2016 09:43:34 +0100	[thread overview]
Message-ID: <20161108084334.i4avrqu5shvhlanu@phenom.ffwll.local> (raw)
In-Reply-To: <CAF6AEGtacCXixzEvDCJEfEgW=0UhyFffob1_r_GeZ2_epEQBCA@mail.gmail.com>

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

WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: "Eric Engestrom" <eric@engestrom.ch>,
	"Tom St Denis" <tom.stdenis@amd.com>,
	"Archit Taneja" <architt@codeaurora.org>,
	"Flora Cui" <Flora.Cui@amd.com>,
	"David Airlie" <airlied@linux.ie>,
	"Intel Graphics Development" <intel-gfx@lists.freedesktop.org>,
	"Michel Dänzer" <michel.daenzer@amd.com>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Xinliang Liu" <z.liuxinliang@hisilicon.com>,
	"David Zhang" <david1.zhang@amd.com>,
	"Wei Yongjun" <yongjun_wei@trendmicro.com.cn>,
	"Vitaly Prosyak" <vitaly.prosyak@amd.com>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Junwei Zhang" <Jerry.Zhang@amd.com>,
	"Alex Deucher" <alexander.deucher@amd.com>,
	"Xinwei Kong" <kong.kongxinwei@hisilicon.com>
Subject: Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe
Date: Tue, 8 Nov 2016 09:43:34 +0100	[thread overview]
Message-ID: <20161108084334.i4avrqu5shvhlanu@phenom.ffwll.local> (raw)
In-Reply-To: <CAF6AEGtacCXixzEvDCJEfEgW=0UhyFffob1_r_GeZ2_epEQBCA@mail.gmail.com>

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

  reply	other threads:[~2016-11-08  8:43 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-15  0:02 [PATCH] drm: make drm_get_format_name thread-safe Eric Engestrom
2016-08-15  0:02 ` Eric Engestrom
2016-08-15  5:33 ` ✗ Ro.CI.BAT: failure for " Patchwork
2016-08-15  9:54 ` [PATCH] " Jani Nikula
2016-08-15  9:54   ` Jani Nikula
2016-08-15 12:59   ` Eric Engestrom
2016-08-15 12:59     ` Eric Engestrom
2016-08-15 13:13     ` Jani Nikula
2016-08-15 13:52       ` Daniel Vetter
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-15 15:29           ` Eric Engestrom
2016-08-16 11:04           ` Jani Nikula
2016-08-16 11:04             ` Jani Nikula
2016-08-16 12:07             ` Daniel Vetter
2016-08-16 12:07               ` Daniel Vetter
2016-08-15 16:00 ` ✗ Ro.CI.BAT: failure for drm: make drm_get_format_name thread-safe (rev2) Patchwork
2016-11-03 18:52 ` [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe Rob Clark
2016-11-03 18:52   ` Rob Clark
2016-11-08  8:43   ` Daniel Vetter [this message]
2016-11-08  8:43     ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161108084334.i4avrqu5shvhlanu@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=Flora.Cui@amd.com \
    --cc=Jerry.Zhang@amd.com \
    --cc=airlied@linux.ie \
    --cc=alexander.deucher@amd.com \
    --cc=architt@codeaurora.org \
    --cc=christian.koenig@amd.com \
    --cc=daniel.vetter@intel.com \
    --cc=david1.zhang@amd.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=eric@engestrom.ch \
    --cc=gustavo.padovan@collabora.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=kong.kongxinwei@hisilicon.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=michel.daenzer@amd.com \
    --cc=robdclark@gmail.com \
    --cc=tom.stdenis@amd.com \
    --cc=vitaly.prosyak@amd.com \
    --cc=yongjun_wei@trendmicro.com.cn \
    --cc=z.liuxinliang@hisilicon.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.