Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe

[Daniel Vetter]

On Thu, Nov 03, 2016 at 02:52:00PM -0400, Rob Clark wrote:
> On Sun, Aug 14, 2016 at 8:02 PM, Eric Engestrom  wrote:
> > Signed-off-by: Eric Engestrom 
> > ---
> >
> > 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
> > +++ 

Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe

[Rob Clark]

On Sun, Aug 14, 2016 at 8:02 PM, Eric Engestrom  wrote:
> Signed-off-by: Eric Engestrom 
> ---
>
> 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,
>  

Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe

[Eric Engestrom]

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  wrote:
> > > On Mon, Aug 15, 2016 at 12:54:01PM +0300, Jani Nikula wrote:
> > >> On Mon, 15 Aug 2016, Eric Engestrom  wrote:
> > >> > Signed-off-by: Eric Engestrom 
> > >> > ---
> > >> >
> > >> > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe

[Daniel Vetter]

On Mon, Aug 15, 2016 at 04:13:54PM +0300, Jani Nikula wrote:
> On Mon, 15 Aug 2016, Eric Engestrom  wrote:
> > On Mon, Aug 15, 2016 at 12:54:01PM +0300, Jani Nikula wrote:
> >> On Mon, 15 Aug 2016, Eric Engestrom  wrote:
> >> > Signed-off-by: Eric Engestrom 
> >> > ---
> >> >
> >> > I moved the main bits to be the first diffs, shouldn't affect anything
> >> > when applying the patch, but I wanted to ask:
> >> > I don't like the hard-coded `32` the appears in both kmalloc() and
> >> > snprintf(), what do you think? If you don't like it either, what would
> >> > you suggest? Should I #define it?
> >> >
> >> > Second question is about the patch mail itself: should I send this kind
> >> > of patch separated by module, with a note requesting them to be squashed
> >> > when applying? It has to land as a single patch, but for review it might
> >> > be easier if people only see the bits they each care about, as well as
> >> > to collect ack's/r-b's.
> >> >
> >> > Cheers,
> >> >   Eric
> >> >
> >> > ---
> >> >  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c  |  6 ++--
> >> >  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c  |  6 ++--
> >> >  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c   |  6 ++--
> >> >  drivers/gpu/drm/drm_atomic.c|  5 ++--
> >> >  drivers/gpu/drm/drm_crtc.c  | 21 -
> >> >  drivers/gpu/drm/drm_fourcc.c| 17 ++-
> >> >  drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  6 ++--
> >> >  drivers/gpu/drm/i915/i915_debugfs.c | 11 ++-
> >> >  drivers/gpu/drm/i915/intel_atomic_plane.c   |  6 ++--
> >> >  drivers/gpu/drm/i915/intel_display.c| 39 
> >> > -
> >> >  drivers/gpu/drm/radeon/atombios_crtc.c  | 12 +---
> >> >  include/drm/drm_fourcc.h|  2 +-
> >> >  12 files changed, 89 insertions(+), 48 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> >> > index 0645c85..38216a1 100644
> >> > --- a/drivers/gpu/drm/drm_fourcc.c
> >> > +++ b/drivers/gpu/drm/drm_fourcc.c
> >> > @@ -39,16 +39,14 @@ static char printable_char(int c)
> >> >   * drm_get_format_name - return a string for drm fourcc format
> >> >   * @format: format to compute name of
> >> >   *
> >> > - * Note that the buffer used by this function is globally shared and 
> >> > owned by
> >> > - * the function itself.
> >> > - *
> >> > - * FIXME: This isn't really multithreading safe.
> >> > + * Note that the buffer returned by this function is owned by the caller
> >> > + * and will need to be freed.
> >> >   */
> >> >  const char *drm_get_format_name(uint32_t format)
> >> 
> >> I find it surprising that a function that allocates a buffer returns a
> >> const pointer. Some userspace libraries have conventions about the
> >> ownership based on constness.
> >> 
> >> (I also find it suprising that kfree() takes a const pointer; arguably
> >> that call changes the memory.)
> >> 
> >> Is there precedent for this?
> >> 
> >> BR,
> >> Jani.
> >
> > It's not a const pointer, it's a normal pointer to a const char, i.e.
> > you can do as you want with the pointer but you shouldn't change the
> > chars it points to.
> 
> Ermh, that's what I meant even if I was sloppy in my reply. And arguably
> freeing the bytes the pointer points at changes them, albeit subtly. And
> having a function return a pointer to const data is often an indication
> that the ownership of the data isn't transfered, i.e. you're not
> supposed to free it yourself.

I already applied the patch, but yes dropping the const would be a good
hint to callers that they now own that block of memory. Eric, can you pls
follow up with a fix up patch - drm-misc is non-rebasing?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe

[Jani Nikula]

On Mon, 15 Aug 2016, Eric Engestrom  wrote:
> On Mon, Aug 15, 2016 at 12:54:01PM +0300, Jani Nikula wrote:
>> On Mon, 15 Aug 2016, Eric Engestrom  wrote:
>> > Signed-off-by: Eric Engestrom 
>> > ---
>> >
>> > I moved the main bits to be the first diffs, shouldn't affect anything
>> > when applying the patch, but I wanted to ask:
>> > I don't like the hard-coded `32` the appears in both kmalloc() and
>> > snprintf(), what do you think? If you don't like it either, what would
>> > you suggest? Should I #define it?
>> >
>> > Second question is about the patch mail itself: should I send this kind
>> > of patch separated by module, with a note requesting them to be squashed
>> > when applying? It has to land as a single patch, but for review it might
>> > be easier if people only see the bits they each care about, as well as
>> > to collect ack's/r-b's.
>> >
>> > Cheers,
>> >   Eric
>> >
>> > ---
>> >  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c  |  6 ++--
>> >  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c  |  6 ++--
>> >  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c   |  6 ++--
>> >  drivers/gpu/drm/drm_atomic.c|  5 ++--
>> >  drivers/gpu/drm/drm_crtc.c  | 21 -
>> >  drivers/gpu/drm/drm_fourcc.c| 17 ++-
>> >  drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  6 ++--
>> >  drivers/gpu/drm/i915/i915_debugfs.c | 11 ++-
>> >  drivers/gpu/drm/i915/intel_atomic_plane.c   |  6 ++--
>> >  drivers/gpu/drm/i915/intel_display.c| 39 
>> > -
>> >  drivers/gpu/drm/radeon/atombios_crtc.c  | 12 +---
>> >  include/drm/drm_fourcc.h|  2 +-
>> >  12 files changed, 89 insertions(+), 48 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>> > index 0645c85..38216a1 100644
>> > --- a/drivers/gpu/drm/drm_fourcc.c
>> > +++ b/drivers/gpu/drm/drm_fourcc.c
>> > @@ -39,16 +39,14 @@ static char printable_char(int c)
>> >   * drm_get_format_name - return a string for drm fourcc format
>> >   * @format: format to compute name of
>> >   *
>> > - * Note that the buffer used by this function is globally shared and 
>> > owned by
>> > - * the function itself.
>> > - *
>> > - * FIXME: This isn't really multithreading safe.
>> > + * Note that the buffer returned by this function is owned by the caller
>> > + * and will need to be freed.
>> >   */
>> >  const char *drm_get_format_name(uint32_t format)
>> 
>> I find it surprising that a function that allocates a buffer returns a
>> const pointer. Some userspace libraries have conventions about the
>> ownership based on constness.
>> 
>> (I also find it suprising that kfree() takes a const pointer; arguably
>> that call changes the memory.)
>> 
>> Is there precedent for this?
>> 
>> BR,
>> Jani.
>
> It's not a const pointer, it's a normal pointer to a const char, i.e.
> you can do as you want with the pointer but you shouldn't change the
> chars it points to.

Ermh, that's what I meant even if I was sloppy in my reply. And arguably
freeing the bytes the pointer points at changes them, albeit subtly. And
having a function return a pointer to const data is often an indication
that the ownership of the data isn't transfered, i.e. you're not
supposed to free it yourself.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe

[Eric Engestrom]

On Mon, Aug 15, 2016 at 12:54:01PM +0300, Jani Nikula wrote:
> On Mon, 15 Aug 2016, Eric Engestrom  wrote:
> > Signed-off-by: Eric Engestrom 
> > ---
> >
> > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe

[Jani Nikula]

On Mon, 15 Aug 2016, Eric Engestrom  wrote:
> Signed-off-by: Eric Engestrom 
> ---
>
> 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:
> - 

[Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe

[Eric Engestrom]

Signed-off-by: Eric Engestrom 
---

I moved the main bits to be the first diffs, shouldn't affect anything
when applying the patch, but I wanted to ask:
I don't like the hard-coded `32` the appears in both kmalloc() and
snprintf(), what do you think? If you don't like it either, what would
you suggest? Should I #define it?

Second question is about the patch mail itself: should I send this kind
of patch separated by module, with a note requesting them to be squashed
when applying? It has to land as a single patch, but for review it might
be easier if people only see the bits they each care about, as well as
to collect ack's/r-b's.

Cheers,
  Eric

---
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c  |  6 ++--
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c  |  6 ++--
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c   |  6 ++--
 drivers/gpu/drm/drm_atomic.c|  5 ++--
 drivers/gpu/drm/drm_crtc.c  | 21 -
 drivers/gpu/drm/drm_fourcc.c| 17 ++-
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  6 ++--
 drivers/gpu/drm/i915/i915_debugfs.c | 11 ++-
 drivers/gpu/drm/i915/intel_atomic_plane.c   |  6 ++--
 drivers/gpu/drm/i915/intel_display.c| 39 -
 drivers/gpu/drm/radeon/atombios_crtc.c  | 12 +---
 include/drm/drm_fourcc.h|  2 +-
 12 files changed, 89 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 0645c85..38216a1 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -39,16 +39,14 @@ static char printable_char(int c)
  * drm_get_format_name - return a string for drm fourcc format
  * @format: format to compute name of
  *
- * Note that the buffer used by this function is globally shared and owned by
- * the function itself.
- *
- * FIXME: This isn't really multithreading safe.
+ * Note that the buffer returned by this function is owned by the caller
+ * and will need to be freed.
  */
 const char *drm_get_format_name(uint32_t format)
 {
-   static char buf[32];
+   char *buf = kmalloc(32, GFP_KERNEL);
 
-   snprintf(buf, sizeof(buf),
+   snprintf(buf, 32,
 "%c%c%c%c %s-endian (0x%08x)",
 printable_char(format & 0xff),
 printable_char((format >> 8) & 0xff),
@@ -73,6 +71,8 @@ EXPORT_SYMBOL(drm_get_format_name);
 void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
  int *bpp)
 {
+   const char *format_name;
+
switch (format) {
case DRM_FORMAT_C8:
case DRM_FORMAT_RGB332:
@@ -127,8 +127,9 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int 
*depth,
*bpp = 32;
break;
default:
-   DRM_DEBUG_KMS("unsupported pixel format %s\n",
- drm_get_format_name(format));
+   format_name = drm_get_format_name(format);
+   DRM_DEBUG_KMS("unsupported pixel format %s\n", format_name);
+   kfree(format_name);
*depth = 0;
*bpp = 0;
break;
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 7f90a39..030d22d 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -32,6 +32,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
 int drm_format_vert_chroma_subsampling(uint32_t format);
 int drm_format_plane_width(int width, uint32_t format, int plane);
 int drm_format_plane_height(int height, uint32_t format, int plane);
-const char *drm_get_format_name(uint32_t format);
+const char *drm_get_format_name(uint32_t format) __malloc;
 
 #endif /* __DRM_FOURCC_H__ */
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c 
b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index c1b04e9..0bf8959 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2071,6 +2071,7 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc 
*crtc,
u32 tmp, viewport_w, viewport_h;
int r;
bool bypass_lut = false;
+   const char *format_name;
 
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -2182,8 +2183,9 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc 
*crtc,
bypass_lut = true;
break;
default:
-   DRM_ERROR("Unsupported screen format %s\n",
-   drm_get_format_name(target_fb->pixel_format));
+   format_name = drm_get_format_name(target_fb->pixel_format);
+   DRM_ERROR("Unsupported screen format %s\n", format_name);
+   kfree(format_name);
return -EINVAL;
}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c 
b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index d4bf133..1558a97 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++