[RFC] Move i915 ascii85 functions to linux/ascii85.h

[RFC] Move i915 ascii85 functions to linux/ascii85.h

[Jordan Crouse]

I've been working on a crash dump utility for the drm/msm GPU driver
to get the useful GPU information out after a hang. Taking inspiration
from the i915 driver I thought it was very smart to use ascii85 format
to encode the binary buffers and other bits.  This patch moves the two
very simple functions to encode binaries as ascii85 from the i915
driver to a linux header to be enjoyed by all.

I debated if it was better to move them to drm/ or go all the way and 
obviously I picked all the way, but if the owners and maintainers feel
like this is something we want to keep closer to home then by all means
lets do what feels best.

Suggestions and flames welcome. Coming immediately after will be the drm/msm
stack that uses this in anger.

Jordan Crouse (1):
  include: Move ascii85 functions from i915 to linux/ascii85.h

 drivers/gpu/drm/i915/i915_gpu_error.c | 24 +---------------
 include/linux/ascii85.h               | 52 +++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 23 deletions(-)
 create mode 100644 include/linux/ascii85.h

-- 
1.9.1

[PATCH] include: Move ascii85 functions from i915 to linux/ascii85.h

[Jordan Crouse]

The i915 DRM driver very cleverly used ascii85 encoding for their
GPU state file. Move the encode functions to a general header file to
support other drivers that might be interested in the same
functionality.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 24 +---------------
 include/linux/ascii85.h               | 52 +++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 23 deletions(-)
 create mode 100644 include/linux/ascii85.h

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 48418fb..2588f37 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -31,6 +31,7 @@
 #include <linux/stop_machine.h>
 #include <linux/zlib.h>
 #include <drm/drm_print.h>
+#include <linux/ascii85.h>
 
 #include "i915_drv.h"
 
@@ -501,29 +502,6 @@ void i915_error_printf(struct drm_i915_error_state_buf *e, const char *f, ...)
 	va_end(args);
 }
 
-static int
-ascii85_encode_len(int len)
-{
-	return DIV_ROUND_UP(len, 4);
-}
-
-static bool
-ascii85_encode(u32 in, char *out)
-{
-	int i;
-
-	if (in == 0)
-		return false;
-
-	out[5] = '\0';
-	for (i = 5; i--; ) {
-		out[i] = '!' + in % 85;
-		in /= 85;
-	}
-
-	return true;
-}
-
 static void print_error_obj(struct drm_i915_error_state_buf *m,
 			    struct intel_engine_cs *engine,
 			    const char *name,
diff --git a/include/linux/ascii85.h b/include/linux/ascii85.h
new file mode 100644
index 0000000..7ee39f9
--- /dev/null
+++ b/include/linux/ascii85.h
@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2008 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef _ASCII85_H_
+#define _ASCII85_H_
+
+#include <linux/kernel.h>
+
+static inline int
+ascii85_encode_len(int len)
+{
+	return DIV_ROUND_UP(len, 4);
+}
+
+static inline bool
+ascii85_encode(u32 in, char *out)
+{
+	int i;
+
+	if (in == 0)
+		return false;
+
+	out[5] = '\0';
+	for (i = 5; i--; ) {
+		out[i] = '!' + in % 85;
+		in /= 85;
+	}
+
+	return true;
+}
+
+#endif
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] include: Move ascii85 functions from i915 to linux/ascii85.h

[Chris Wilson]

Quoting Jordan Crouse (2018-01-26 20:59:22)
> The i915 DRM driver very cleverly used ascii85 encoding for their
> GPU state file. Move the encode functions to a general header file to
> support other drivers that might be interested in the same
> functionality.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>  drivers/gpu/drm/i915/i915_gpu_error.c | 24 +---------------
>  include/linux/ascii85.h               | 52 +++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 23 deletions(-)
>  create mode 100644 include/linux/ascii85.h
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 48418fb..2588f37 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -31,6 +31,7 @@
>  #include <linux/stop_machine.h>
>  #include <linux/zlib.h>
>  #include <drm/drm_print.h>
> +#include <linux/ascii85.h>
>  
>  #include "i915_drv.h"
>  
> @@ -501,29 +502,6 @@ void i915_error_printf(struct drm_i915_error_state_buf *e, const char *f, ...)
>         va_end(args);
>  }
>  
> -static int
> -ascii85_encode_len(int len)
> -{
> -       return DIV_ROUND_UP(len, 4);
> -}
> -
> -static bool
> -ascii85_encode(u32 in, char *out)
> -{
> -       int i;
> -
> -       if (in == 0)
> -               return false;
> -
> -       out[5] = '\0';
> -       for (i = 5; i--; ) {
> -               out[i] = '!' + in % 85;
> -               in /= 85;
> -       }
> -
> -       return true;
> -}
> -
>  static void print_error_obj(struct drm_i915_error_state_buf *m,
>                             struct intel_engine_cs *engine,
>                             const char *name,
> diff --git a/include/linux/ascii85.h b/include/linux/ascii85.h
> new file mode 100644
> index 0000000..7ee39f9
> --- /dev/null
> +++ b/include/linux/ascii85.h
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright (c) 2008 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef _ASCII85_H_
> +#define _ASCII85_H_
> +
> +#include <linux/kernel.h>
> +
> +static inline int
> +ascii85_encode_len(int len)
> +{
> +       return DIV_ROUND_UP(len, 4);
> +}

Use longs for generic stuff.

> +
> +static inline bool
> +ascii85_encode(u32 in, char *out)
> +{
> +       int i;
> +
> +       if (in == 0)
> +               return false;
> +
> +       out[5] = '\0';
> +       for (i = 5; i--; ) {
> +               out[i] = '!' + in % 85;
> +               in /= 85;
> +       }
> +
> +       return true;
> +}

I think you'll want to capture the special case 0 == 'z' in the common
routines.

{
	char buf[ASCII85_BUFSZ];
	int i, len;

	len = ascii85_encode_len(PAGE_SIZE);
	for (i = 0; i < len; i++)
		err_puts(m, ascii85_encode(obj->pages[page][i], buf));
}	

Looks reasonable for the caller, so

#define ASCII85_BUFSZ 6

static inline const char *
ascii85_encode(u32 in, char *out)
{
	int i;

	/* check whether out[0] = 'z'; out[1] = '\0'; generates better code */
	if (in == 0)
		return "z";

	out[5] = '\0';
	for (i = 5; i--; ) {
		out[i] = '!' + in % 85;
		in /= 85;
	}

	return out;
}

-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] include: Move ascii85 functions from i915 to linux/ascii85.h

[Chris Wilson]

Quoting Jordan Crouse (2018-01-26 20:59:22)
> The i915 DRM driver very cleverly used ascii85 encoding for their

All gfx drivers must eventually become PostScript.

> GPU state file. Move the encode functions to a general header file to
> support other drivers that might be interested in the same
> functionality.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
> diff --git a/include/linux/ascii85.h b/include/linux/ascii85.h
> new file mode 100644
> index 0000000..7ee39f9
> --- /dev/null
> +++ b/include/linux/ascii85.h
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright (c) 2008 Intel Corporation

Just cut this down to
/*
 * SPDX-License-Identifier: GPL-2.0
 *
 * Copyright (c) 2008 Intel Corporation
 * Copyright (c) 2018 My Name Here
 */

Fortunately ideas themselves are not copyrightable, otherwise Adobe has
a strong claim to ownership.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx