[Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe
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 +++ b/drivers/gpu/drm/amd/amdgpu/dc
Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe
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: > - DRM_ERROR("Unsupported screen form
Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe
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
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
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
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
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, > bypass_lut = true; >
Re: [Intel-gfx] [PATCH] drm: make drm_get_format_name thread-safe
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 > > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > >