linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm: tweak drm_print_bits()
@ 2019-09-19 13:14 Gerd Hoffmann
  2019-09-19 14:10 ` Jani Nikula
  2019-09-20 15:37 ` Thierry Reding
  0 siblings, 2 replies; 3+ messages in thread
From: Gerd Hoffmann @ 2019-09-19 13:14 UTC (permalink / raw)
  To: dri-devel
  Cc: jani.nikula, Gerd Hoffmann, Maarten Lankhorst, Maxime Ripard,
	Sean Paul, David Airlie, Daniel Vetter, open list

There is little reason for the from/to logic, printing a subset of
the bits can be done by simply shifting/masking value if needed.

Also use for_each_set_bit().

Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_print.h              |  5 ++---
 drivers/gpu/drm/drm_gem_ttm_helper.c |  4 ++--
 drivers/gpu/drm/drm_print.c          | 20 +++++++++-----------
 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 12d4916254b4..5b9947d157f4 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -89,9 +89,8 @@ __printf(2, 3)
 void drm_printf(struct drm_printer *p, const char *f, ...);
 void drm_puts(struct drm_printer *p, const char *str);
 void drm_print_regset32(struct drm_printer *p, struct debugfs_regset32 *regset);
-void drm_print_bits(struct drm_printer *p,
-		    unsigned long value, const char *bits[],
-		    unsigned int from, unsigned int to);
+void drm_print_bits(struct drm_printer *p, unsigned long value,
+		    const char * const bits[], int nbits);
 
 __printf(2, 0)
 /**
diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
index 9a4bafcf20df..a534104d8bee 100644
--- a/drivers/gpu/drm/drm_gem_ttm_helper.c
+++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
@@ -23,7 +23,7 @@
 void drm_gem_ttm_print_info(struct drm_printer *p, unsigned int indent,
 			    const struct drm_gem_object *gem)
 {
-	static const char const *plname[] = {
+	static const char * const plname[] = {
 		[ TTM_PL_SYSTEM ] = "system",
 		[ TTM_PL_TT     ] = "tt",
 		[ TTM_PL_VRAM   ] = "vram",
@@ -40,7 +40,7 @@ void drm_gem_ttm_print_info(struct drm_printer *p, unsigned int indent,
 	const struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
 
 	drm_printf_indent(p, indent, "placement=");
-	drm_print_bits(p, bo->mem.placement, plname, 0, ARRAY_SIZE(plname));
+	drm_print_bits(p, bo->mem.placement, plname, ARRAY_SIZE(plname));
 	drm_printf(p, "\n");
 
 	if (bo->mem.bus.is_iomem) {
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index dfa27367ebb8..a495fe3ad909 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -189,28 +189,26 @@ EXPORT_SYMBOL(drm_printf);
  * drm_print_bits - print bits to a &drm_printer stream
  *
  * Print bits (in flag fields for example) in human readable form.
- * The first name in the @bits array is for the bit indexed by @from.
  *
  * @p: the &drm_printer
  * @value: field value.
  * @bits: Array with bit names.
- * @from: start of bit range to print (inclusive).
- * @to: end of bit range to print (exclusive).
+ * @nbits: Size of bit names array.
  */
-void drm_print_bits(struct drm_printer *p,
-		    unsigned long value, const char *bits[],
-		    unsigned int from, unsigned int to)
+void drm_print_bits(struct drm_printer *p, unsigned long value,
+		    const char * const bits[], int nbits)
 {
 	bool first = true;
 	unsigned int i;
 
-	for (i = from; i < to; i++) {
-		if (!(value & (1 << i)))
-			continue;
-		if (WARN_ON_ONCE(!bits[i-from]))
+	if (WARN_ON_ONCE(nbits > sizeof(unsigned long) * 8))
+		nbits = sizeof(unsigned long) * 8;
+
+	for_each_set_bit(i, &value, nbits) {
+		if (WARN_ON_ONCE(!bits[i]))
 			continue;
 		drm_printf(p, "%s%s", first ? "" : ",",
-			   bits[i-from]);
+			   bits[i]);
 		first = false;
 	}
 	if (first)
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] drm: tweak drm_print_bits()
  2019-09-19 13:14 [PATCH] drm: tweak drm_print_bits() Gerd Hoffmann
@ 2019-09-19 14:10 ` Jani Nikula
  2019-09-20 15:37 ` Thierry Reding
  1 sibling, 0 replies; 3+ messages in thread
From: Jani Nikula @ 2019-09-19 14:10 UTC (permalink / raw)
  To: Gerd Hoffmann, dri-devel
  Cc: Gerd Hoffmann, Maarten Lankhorst, Maxime Ripard, Sean Paul,
	David Airlie, Daniel Vetter, open list

On Thu, 19 Sep 2019, Gerd Hoffmann <kraxel@redhat.com> wrote:
> There is little reason for the from/to logic, printing a subset of
> the bits can be done by simply shifting/masking value if needed.
>
> Also use for_each_set_bit().

Oh, I don't know why I stumbled upon and reviewed a patch that had
already been merged. Can't keep track of everything it seems...

>
> Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_print.h              |  5 ++---
>  drivers/gpu/drm/drm_gem_ttm_helper.c |  4 ++--
>  drivers/gpu/drm/drm_print.c          | 20 +++++++++-----------
>  3 files changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 12d4916254b4..5b9947d157f4 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -89,9 +89,8 @@ __printf(2, 3)
>  void drm_printf(struct drm_printer *p, const char *f, ...);
>  void drm_puts(struct drm_printer *p, const char *str);
>  void drm_print_regset32(struct drm_printer *p, struct debugfs_regset32 *regset);
> -void drm_print_bits(struct drm_printer *p,
> -		    unsigned long value, const char *bits[],
> -		    unsigned int from, unsigned int to);
> +void drm_print_bits(struct drm_printer *p, unsigned long value,
> +		    const char * const bits[], int nbits);
>  
>  __printf(2, 0)
>  /**
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> index 9a4bafcf20df..a534104d8bee 100644
> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -23,7 +23,7 @@
>  void drm_gem_ttm_print_info(struct drm_printer *p, unsigned int indent,
>  			    const struct drm_gem_object *gem)
>  {
> -	static const char const *plname[] = {
> +	static const char * const plname[] = {
>  		[ TTM_PL_SYSTEM ] = "system",
>  		[ TTM_PL_TT     ] = "tt",
>  		[ TTM_PL_VRAM   ] = "vram",
> @@ -40,7 +40,7 @@ void drm_gem_ttm_print_info(struct drm_printer *p, unsigned int indent,
>  	const struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>  
>  	drm_printf_indent(p, indent, "placement=");
> -	drm_print_bits(p, bo->mem.placement, plname, 0, ARRAY_SIZE(plname));
> +	drm_print_bits(p, bo->mem.placement, plname, ARRAY_SIZE(plname));
>  	drm_printf(p, "\n");
>  
>  	if (bo->mem.bus.is_iomem) {
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index dfa27367ebb8..a495fe3ad909 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -189,28 +189,26 @@ EXPORT_SYMBOL(drm_printf);
>   * drm_print_bits - print bits to a &drm_printer stream
>   *
>   * Print bits (in flag fields for example) in human readable form.
> - * The first name in the @bits array is for the bit indexed by @from.
>   *
>   * @p: the &drm_printer
>   * @value: field value.
>   * @bits: Array with bit names.
> - * @from: start of bit range to print (inclusive).
> - * @to: end of bit range to print (exclusive).
> + * @nbits: Size of bit names array.
>   */
> -void drm_print_bits(struct drm_printer *p,
> -		    unsigned long value, const char *bits[],
> -		    unsigned int from, unsigned int to)
> +void drm_print_bits(struct drm_printer *p, unsigned long value,
> +		    const char * const bits[], int nbits)
>  {
>  	bool first = true;
>  	unsigned int i;
>  
> -	for (i = from; i < to; i++) {
> -		if (!(value & (1 << i)))
> -			continue;
> -		if (WARN_ON_ONCE(!bits[i-from]))
> +	if (WARN_ON_ONCE(nbits > sizeof(unsigned long) * 8))
> +		nbits = sizeof(unsigned long) * 8;

Might be neater with BITS_PER_TYPE(value) instead of open coding, but
either way,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +
> +	for_each_set_bit(i, &value, nbits) {
> +		if (WARN_ON_ONCE(!bits[i]))
>  			continue;
>  		drm_printf(p, "%s%s", first ? "" : ",",
> -			   bits[i-from]);
> +			   bits[i]);
>  		first = false;
>  	}
>  	if (first)

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] drm: tweak drm_print_bits()
  2019-09-19 13:14 [PATCH] drm: tweak drm_print_bits() Gerd Hoffmann
  2019-09-19 14:10 ` Jani Nikula
@ 2019-09-20 15:37 ` Thierry Reding
  1 sibling, 0 replies; 3+ messages in thread
From: Thierry Reding @ 2019-09-20 15:37 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: dri-devel, Maxime Ripard, open list, David Airlie, Sean Paul

[-- Attachment #1: Type: text/plain, Size: 4132 bytes --]

On Thu, Sep 19, 2019 at 03:14:11PM +0200, Gerd Hoffmann wrote:
> There is little reason for the from/to logic, printing a subset of
> the bits can be done by simply shifting/masking value if needed.
> 
> Also use for_each_set_bit().
> 
> Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_print.h              |  5 ++---
>  drivers/gpu/drm/drm_gem_ttm_helper.c |  4 ++--
>  drivers/gpu/drm/drm_print.c          | 20 +++++++++-----------
>  3 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 12d4916254b4..5b9947d157f4 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -89,9 +89,8 @@ __printf(2, 3)
>  void drm_printf(struct drm_printer *p, const char *f, ...);
>  void drm_puts(struct drm_printer *p, const char *str);
>  void drm_print_regset32(struct drm_printer *p, struct debugfs_regset32 *regset);
> -void drm_print_bits(struct drm_printer *p,
> -		    unsigned long value, const char *bits[],
> -		    unsigned int from, unsigned int to);
> +void drm_print_bits(struct drm_printer *p, unsigned long value,
> +		    const char * const bits[], int nbits);

bitmap_*() APIs that this calls underneath all use unsigned int for
nbits, so might be good to stick with that. After all, this better never
be negative.

Thierry

>  
>  __printf(2, 0)
>  /**
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> index 9a4bafcf20df..a534104d8bee 100644
> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -23,7 +23,7 @@
>  void drm_gem_ttm_print_info(struct drm_printer *p, unsigned int indent,
>  			    const struct drm_gem_object *gem)
>  {
> -	static const char const *plname[] = {
> +	static const char * const plname[] = {
>  		[ TTM_PL_SYSTEM ] = "system",
>  		[ TTM_PL_TT     ] = "tt",
>  		[ TTM_PL_VRAM   ] = "vram",
> @@ -40,7 +40,7 @@ void drm_gem_ttm_print_info(struct drm_printer *p, unsigned int indent,
>  	const struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>  
>  	drm_printf_indent(p, indent, "placement=");
> -	drm_print_bits(p, bo->mem.placement, plname, 0, ARRAY_SIZE(plname));
> +	drm_print_bits(p, bo->mem.placement, plname, ARRAY_SIZE(plname));
>  	drm_printf(p, "\n");
>  
>  	if (bo->mem.bus.is_iomem) {
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index dfa27367ebb8..a495fe3ad909 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -189,28 +189,26 @@ EXPORT_SYMBOL(drm_printf);
>   * drm_print_bits - print bits to a &drm_printer stream
>   *
>   * Print bits (in flag fields for example) in human readable form.
> - * The first name in the @bits array is for the bit indexed by @from.
>   *
>   * @p: the &drm_printer
>   * @value: field value.
>   * @bits: Array with bit names.
> - * @from: start of bit range to print (inclusive).
> - * @to: end of bit range to print (exclusive).
> + * @nbits: Size of bit names array.
>   */
> -void drm_print_bits(struct drm_printer *p,
> -		    unsigned long value, const char *bits[],
> -		    unsigned int from, unsigned int to)
> +void drm_print_bits(struct drm_printer *p, unsigned long value,
> +		    const char * const bits[], int nbits)
>  {
>  	bool first = true;
>  	unsigned int i;
>  
> -	for (i = from; i < to; i++) {
> -		if (!(value & (1 << i)))
> -			continue;
> -		if (WARN_ON_ONCE(!bits[i-from]))
> +	if (WARN_ON_ONCE(nbits > sizeof(unsigned long) * 8))
> +		nbits = sizeof(unsigned long) * 8;
> +
> +	for_each_set_bit(i, &value, nbits) {
> +		if (WARN_ON_ONCE(!bits[i]))
>  			continue;
>  		drm_printf(p, "%s%s", first ? "" : ",",
> -			   bits[i-from]);
> +			   bits[i]);
>  		first = false;
>  	}
>  	if (first)
> -- 
> 2.18.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-20 15:37 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-19 13:14 [PATCH] drm: tweak drm_print_bits() Gerd Hoffmann
2019-09-19 14:10 ` Jani Nikula
2019-09-20 15:37 ` Thierry Reding

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).