[PATCH] drm/i915: Expose LLC size to user space

[PATCH] drm/i915: Expose LLC size to user space

[Ben Widawsky]

The algorithm/information was originally written by Chad, though I
changed the control flow, and I think his original code had a couple of
bugs, though I didn't look very hard before rewriting. That could have
also been different interpretations of the spec.

The excellent comments remain entirely copied from Chad's code.

I've tested this on two platforms, and it seems to perform how I want.

CC: Chad Versace <chad.versace@linux.intel.com>
CC: Bryan Bell <bryan.j.bell@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |  2 +-
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_gem.c | 53 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0e22142..377949e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -974,7 +974,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = 1;
 		break;
 	case I915_PARAM_HAS_LLC:
-		value = HAS_LLC(dev);
+		value = dev_priv->llc_size;
 		break;
 	case I915_PARAM_HAS_ALIASING_PPGTT:
 		value = dev_priv->mm.aliasing_ppgtt ? 1 : 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c8d6104..43a549d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1187,6 +1187,8 @@ typedef struct drm_i915_private {
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
+
+	size_t llc_size;
 } drm_i915_private_t;
 
 /* Iterate over initialised rings */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index af61be8..a070686 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4282,6 +4282,57 @@ i915_gem_lastclose(struct drm_device *dev)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
 }
 
+/**
+ * Return the size, in bytes, of the CPU L3 cache size. If the CPU has no L3
+ * cache, or if an error occurs in obtaining the cache size, then return 0.
+ * From "Intel Processor Identification and the CPUID Instruction > 5.15
+ * Deterministic Cache Parmaeters (Function 04h)":
+ *    When EAX is initialized to a value of 4, the CPUID instruction returns
+ *    deterministic cache information in the EAX, EBX, ECX and EDX registers.
+ *    This function requires ECX be initialized with an index which indicates
+ *    which cache to return information about. The OS is expected to call this
+ *    function (CPUID.4) with ECX = 0, 1, 2, until EAX[4:0] == 0, indicating no
+ *    more caches. The order in which the caches are returned is not specified
+ *    and may change at Intel's discretion.
+ *
+ * Equation 5-4. Calculating the Cache Size in bytes:
+ *          = (Ways +1) � (Partitions +1) � (Line Size +1) � (Sets +1)
+ *          = (EBX[31:22] +1) � (EBX[21:12] +1) � (EBX[11:0] +1 � (ECX + 1)
+ */
+static size_t get_llc_size(struct drm_device *dev)
+{
+	u8 cnt = 0;
+	unsigned int eax, ebx, ecx, edx;
+
+	if (!HAS_LLC(dev))
+		return 0;
+
+	do {
+		uint32_t cache_level;
+		uint32_t associativity, line_partitions, line_size, sets;
+
+		eax = 4;
+		ecx = cnt;
+		__cpuid(&eax, &ebx, &ecx, &edx);
+
+		cache_level = (eax >> 5) & 0x7;
+		if (cache_level != 3)
+			continue;
+
+		associativity = ((ebx >> 22) & 0x3ff) + 1;
+		line_partitions = ((ebx >> 12) & 0x3ff) + 1;
+		line_size = (ebx & 0xfff) + 1;
+		sets = ecx + 1;
+
+		return associativity * line_partitions * line_size * sets;
+	} while (eax & 0x1f && ++cnt);
+
+	/* Let user space know we have LLC, but we can't figure it out */
+	DRM_DEBUG_DRIVER("Couldn't find LLC size. Bug?\n");
+	return 1;
+}
+
+
 static void
 init_ring_lists(struct intel_ring_buffer *ring)
 {
@@ -4333,6 +4384,8 @@ i915_gem_load(struct drm_device *dev)
 	else
 		dev_priv->num_fence_regs = 8;
 
+	dev_priv->llc_size = get_llc_size(dev);
+
 	/* Initialize fence registers to zero */
 	INIT_LIST_HEAD(&dev_priv->mm.fence_list);
 	i915_gem_restore_fences(dev);
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] intel_get_llc_size: Small tool to query LLC size

[Ben Widawsky]

CC: Chad Versace <chad.versace@linux.intel.com>
CC: Bryan Bell <bryan.j.bell@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tools/Makefile.am          |  1 +
 tools/intel_get_llc_size.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
 create mode 100644 tools/intel_get_llc_size.c

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 2519169..a064b65 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -9,6 +9,7 @@ bin_PROGRAMS = 				\
 	intel_bios_dumper 		\
 	intel_bios_reader 		\
 	intel_error_decode 		\
+	intel_get_llc_size 		\
 	intel_gpu_top 			\
 	intel_gpu_time 			\
 	intel_gtt 			\
diff --git a/tools/intel_get_llc_size.c b/tools/intel_get_llc_size.c
new file mode 100644
index 0000000..bd384d2
--- /dev/null
+++ b/tools/intel_get_llc_size.c
@@ -0,0 +1,57 @@
+/*
+ * Copyright © 2013 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.
+ *
+ */
+
+#include <sys/ioctl.h>
+#include "drmtest.h"
+#include "i915_drm.h"
+
+static int get_llc_size(int fd)
+{
+	struct drm_i915_getparam gp;
+	int size;
+
+	gp.param = I915_PARAM_HAS_LLC;
+	gp.value = &size;
+
+	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
+		return 0;
+
+	return size;
+}
+
+int main(int argc, char **argv)
+{
+	int size, fd = drm_open_any();
+
+	size = get_llc_size(fd);
+
+	if (size == 0)
+		fprintf(stdout, "Doesn't have LLC\n");
+	else if (size == 1)
+		fprintf(stdout, "Kernel is too old to determine LLC size\n");
+	else
+		fprintf(stdout, "LLC size = %dK\n", size>>10);
+
+	return 0;
+}
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] intel_get_llc_size: Small tool to query LLC size

[Daniel Vetter]

On Tue, Jul 09, 2013 at 07:58:03PM -0700, Ben Widawsky wrote:
> CC: Chad Versace <chad.versace@linux.intel.com>
> CC: Bryan Bell <bryan.j.bell@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

So I think we should run this from igt and check its return value. And
since we've had a few bugs with other (currently untested) igt tools, can
you please add a new igt_tools testcase which just runs those? I'm
thinking of intel_reg_dumper and intel_reg_read (with some render ring
register that exists everywhere) on top of running intel_get_llc_size
here.

And please also add eLLC size querying.
-Daniel

> ---
>  tools/Makefile.am          |  1 +
>  tools/intel_get_llc_size.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
>  create mode 100644 tools/intel_get_llc_size.c
> 
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index 2519169..a064b65 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -9,6 +9,7 @@ bin_PROGRAMS = 				\
>  	intel_bios_dumper 		\
>  	intel_bios_reader 		\
>  	intel_error_decode 		\
> +	intel_get_llc_size 		\
>  	intel_gpu_top 			\
>  	intel_gpu_time 			\
>  	intel_gtt 			\
> diff --git a/tools/intel_get_llc_size.c b/tools/intel_get_llc_size.c
> new file mode 100644
> index 0000000..bd384d2
> --- /dev/null
> +++ b/tools/intel_get_llc_size.c
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright © 2013 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.
> + *
> + */
> +
> +#include <sys/ioctl.h>
> +#include "drmtest.h"
> +#include "i915_drm.h"
> +
> +static int get_llc_size(int fd)
> +{
> +	struct drm_i915_getparam gp;
> +	int size;
> +
> +	gp.param = I915_PARAM_HAS_LLC;
> +	gp.value = &size;
> +
> +	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> +		return 0;
> +
> +	return size;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int size, fd = drm_open_any();
> +
> +	size = get_llc_size(fd);
> +
> +	if (size == 0)
> +		fprintf(stdout, "Doesn't have LLC\n");
> +	else if (size == 1)
> +		fprintf(stdout, "Kernel is too old to determine LLC size\n");
> +	else
> +		fprintf(stdout, "LLC size = %dK\n", size>>10);
> +
> +	return 0;
> +}
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Expose LLC size to user space

[Chris Wilson]

On Tue, Jul 09, 2013 at 07:58:02PM -0700, Ben Widawsky wrote:
> The algorithm/information was originally written by Chad, though I
> changed the control flow, and I think his original code had a couple of
> bugs, though I didn't look very hard before rewriting. That could have
> also been different interpretations of the spec.

Just a cpuid query that can already be done more simply from userspace
(i.e. with no syscalls)? I was expecting a lot more magic.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Expose LLC size to user space

[Ben Widawsky]

On Wed, Jul 10, 2013 at 09:59:01AM +0100, Chris Wilson wrote:
> On Tue, Jul 09, 2013 at 07:58:02PM -0700, Ben Widawsky wrote:
> > The algorithm/information was originally written by Chad, though I
> > changed the control flow, and I think his original code had a couple of
> > bugs, though I didn't look very hard before rewriting. That could have
> > also been different interpretations of the spec.
> 
> Just a cpuid query that can already be done more simply from userspace
> (i.e. with no syscalls)? I was expecting a lot more magic.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

Chad wrote this originally for mesa. And yes, it's doable from
userspace. Chatting with Daniel, we thought maybe other GPU clients
might want this info, and so a central place to put the code would be
nice, in case there are quirks or anything like that (I've had a
particularly hard time figuring out if Xeon really has L3 or not).

So what's a central place? Libdrm, everybody uses that right? You can
read /proc/cpuinfo as far as I know, but then you still need to query
the HAS_LLC getparam to figure out what kind of L3 (or do your own
chipset ID check).

In addition to the centrality argument, I noticed while poking around
cpuid in the kernel that it is a vitalized function. I'm not sure what
the purpose of this is, but it's something you can't fake in userspace.


-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] intel_get_llc_size: Small tool to query LLC size

[Ben Widawsky]

On Wed, Jul 10, 2013 at 08:34:18AM +0200, Daniel Vetter wrote:
> On Tue, Jul 09, 2013 at 07:58:03PM -0700, Ben Widawsky wrote:
> > CC: Chad Versace <chad.versace@linux.intel.com>
> > CC: Bryan Bell <bryan.j.bell@intel.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> So I think we should run this from igt and check its return value. And
> since we've had a few bugs with other (currently untested) igt tools, can
> you please add a new igt_tools testcase which just runs those? I'm
> thinking of intel_reg_dumper and intel_reg_read (with some render ring
> register that exists everywhere) on top of running intel_get_llc_size
> here.

What would you like exactly for the check? Just to make sure platforms
that should have LLC return a value > 1?

As for testing the other tools, I personally want no involvement with
intel_reg_dumper, since I feel maintaining it is a losing effort and
would rather see effort put into quick_dump. I'll write tests for
reg_read/reg_write though.

> And please also add eLLC size querying.

When I see review and signs of those getting merged, I'll gladly do
that. For simple, potentially very useful patches, they're sitting
around doing nothing for an awfully long time. And, I can no longer
easily test them. So poop.

> -Daniel
> 
> > ---
> >  tools/Makefile.am          |  1 +
> >  tools/intel_get_llc_size.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 58 insertions(+)
> >  create mode 100644 tools/intel_get_llc_size.c
> > 
> > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > index 2519169..a064b65 100644
> > --- a/tools/Makefile.am
> > +++ b/tools/Makefile.am
> > @@ -9,6 +9,7 @@ bin_PROGRAMS = 				\
> >  	intel_bios_dumper 		\
> >  	intel_bios_reader 		\
> >  	intel_error_decode 		\
> > +	intel_get_llc_size 		\
> >  	intel_gpu_top 			\
> >  	intel_gpu_time 			\
> >  	intel_gtt 			\
> > diff --git a/tools/intel_get_llc_size.c b/tools/intel_get_llc_size.c
> > new file mode 100644
> > index 0000000..bd384d2
> > --- /dev/null
> > +++ b/tools/intel_get_llc_size.c
> > @@ -0,0 +1,57 @@
> > +/*
> > + * Copyright © 2013 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.
> > + *
> > + */
> > +
> > +#include <sys/ioctl.h>
> > +#include "drmtest.h"
> > +#include "i915_drm.h"
> > +
> > +static int get_llc_size(int fd)
> > +{
> > +	struct drm_i915_getparam gp;
> > +	int size;
> > +
> > +	gp.param = I915_PARAM_HAS_LLC;
> > +	gp.value = &size;
> > +
> > +	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> > +		return 0;
> > +
> > +	return size;
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	int size, fd = drm_open_any();
> > +
> > +	size = get_llc_size(fd);
> > +
> > +	if (size == 0)
> > +		fprintf(stdout, "Doesn't have LLC\n");
> > +	else if (size == 1)
> > +		fprintf(stdout, "Kernel is too old to determine LLC size\n");
> > +	else
> > +		fprintf(stdout, "LLC size = %dK\n", size>>10);
> > +
> > +	return 0;
> > +}
> > -- 
> > 1.8.3.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Expose LLC size to user space

[Bell, Bryan J]

Looks good,
Can you update the comment? The pdf "Intel Processor Identification and the CPUID Instruction" is no longer available and the description of
the CPUID instruction is now in the "Intel 64 and IA32
Architectures Developer's Manual: Vol. 2A -> section 3.2 -> CPUID"

-----Original Message-----
From: Ben Widawsky [mailto:ben@bwidawsk.net] 
Sent: Tuesday, July 09, 2013 7:58 PM
To: Intel GFX
Cc: Ben Widawsky; Chad Versace; Bell, Bryan J
Subject: [PATCH] drm/i915: Expose LLC size to user space

The algorithm/information was originally written by Chad, though I changed the control flow, and I think his original code had a couple of bugs, though I didn't look very hard before rewriting. That could have also been different interpretations of the spec.

The excellent comments remain entirely copied from Chad's code.

I've tested this on two platforms, and it seems to perform how I want.

CC: Chad Versace <chad.versace@linux.intel.com>
CC: Bryan Bell <bryan.j.bell@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |  2 +-  drivers/gpu/drm/i915/i915_drv.h |  2 ++  drivers/gpu/drm/i915/i915_gem.c | 53 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 0e22142..377949e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -974,7 +974,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = 1;
 		break;
 	case I915_PARAM_HAS_LLC:
-		value = HAS_LLC(dev);
+		value = dev_priv->llc_size;
 		break;
 	case I915_PARAM_HAS_ALIASING_PPGTT:
 		value = dev_priv->mm.aliasing_ppgtt ? 1 : 0; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c8d6104..43a549d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1187,6 +1187,8 @@ typedef struct drm_i915_private {
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
+
+	size_t llc_size;
 } drm_i915_private_t;
 
 /* Iterate over initialised rings */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index af61be8..a070686 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4282,6 +4282,57 @@ i915_gem_lastclose(struct drm_device *dev)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);  }
 
+/**
+ * Return the size, in bytes, of the CPU L3 cache size. If the CPU has 
+no L3
+ * cache, or if an error occurs in obtaining the cache size, then return 0.
+ * From "Intel Processor Identification and the CPUID Instruction > 
+5.15
+ * Deterministic Cache Parmaeters (Function 04h)":
+ *    When EAX is initialized to a value of 4, the CPUID instruction returns
+ *    deterministic cache information in the EAX, EBX, ECX and EDX registers.
+ *    This function requires ECX be initialized with an index which indicates
+ *    which cache to return information about. The OS is expected to call this
+ *    function (CPUID.4) with ECX = 0, 1, 2, until EAX[4:0] == 0, indicating no
+ *    more caches. The order in which the caches are returned is not specified
+ *    and may change at Intel's discretion.
+ *
+ * Equation 5-4. Calculating the Cache Size in bytes:
+ *          = (Ways +1)   (Partitions +1)   (Line Size +1)   (Sets +1)
+ *          = (EBX[31:22] +1)   (EBX[21:12] +1)   (EBX[11:0] +1   (ECX + 1)
+ */
+static size_t get_llc_size(struct drm_device *dev) {
+	u8 cnt = 0;
+	unsigned int eax, ebx, ecx, edx;
+
+	if (!HAS_LLC(dev))
+		return 0;
+
+	do {
+		uint32_t cache_level;
+		uint32_t associativity, line_partitions, line_size, sets;
+
+		eax = 4;
+		ecx = cnt;
+		__cpuid(&eax, &ebx, &ecx, &edx);
+
+		cache_level = (eax >> 5) & 0x7;
+		if (cache_level != 3)
+			continue;
+
+		associativity = ((ebx >> 22) & 0x3ff) + 1;
+		line_partitions = ((ebx >> 12) & 0x3ff) + 1;
+		line_size = (ebx & 0xfff) + 1;
+		sets = ecx + 1;
+
+		return associativity * line_partitions * line_size * sets;
+	} while (eax & 0x1f && ++cnt);
+
+	/* Let user space know we have LLC, but we can't figure it out */
+	DRM_DEBUG_DRIVER("Couldn't find LLC size. Bug?\n");
+	return 1;
+}
+
+
 static void
 init_ring_lists(struct intel_ring_buffer *ring)  { @@ -4333,6 +4384,8 @@ i915_gem_load(struct drm_device *dev)
 	else
 		dev_priv->num_fence_regs = 8;
 
+	dev_priv->llc_size = get_llc_size(dev);
+
 	/* Initialize fence registers to zero */
 	INIT_LIST_HEAD(&dev_priv->mm.fence_list);
 	i915_gem_restore_fences(dev);
--
1.8.3.2

Re: [PATCH] drm/i915: Expose LLC size to user space

[Ben Widawsky]

On Wed, Jul 10, 2013 at 09:40:18AM -0700, Ben Widawsky wrote:
> On Wed, Jul 10, 2013 at 09:59:01AM +0100, Chris Wilson wrote:
> > On Tue, Jul 09, 2013 at 07:58:02PM -0700, Ben Widawsky wrote:
> > > The algorithm/information was originally written by Chad, though I
> > > changed the control flow, and I think his original code had a couple of
> > > bugs, though I didn't look very hard before rewriting. That could have
> > > also been different interpretations of the spec.
> > 
> > Just a cpuid query that can already be done more simply from userspace
> > (i.e. with no syscalls)? I was expecting a lot more magic.
> > -Chris
> > 
> > -- 
> > Chris Wilson, Intel Open Source Technology Centre
> 
> Chad wrote this originally for mesa. And yes, it's doable from
> userspace. Chatting with Daniel, we thought maybe other GPU clients
> might want this info, and so a central place to put the code would be
> nice, in case there are quirks or anything like that (I've had a
> particularly hard time figuring out if Xeon really has L3 or not).
> 
> So what's a central place? Libdrm, everybody uses that right? You can
> read /proc/cpuinfo as far as I know, but then you still need to query
> the HAS_LLC getparam to figure out what kind of L3 (or do your own
> chipset ID check).
> 
> In addition to the centrality argument, I noticed while poking around
> cpuid in the kernel that it is a vitalized function. I'm not sure what
That's the last time I let vim tell me I spelled "virtualized" wrong:

cpuid in the kernel that it is a virtualized function
>
> the purpose of this is, but it's something you can't fake in userspace.
> 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] intel_get_llc_size: Small tool to query LLC size

[Daniel Vetter]

On Wed, Jul 10, 2013 at 09:58:38AM -0700, Ben Widawsky wrote:
> On Wed, Jul 10, 2013 at 08:34:18AM +0200, Daniel Vetter wrote:
> > On Tue, Jul 09, 2013 at 07:58:03PM -0700, Ben Widawsky wrote:
> > > CC: Chad Versace <chad.versace@linux.intel.com>
> > > CC: Bryan Bell <bryan.j.bell@intel.com>
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > So I think we should run this from igt and check its return value. And
> > since we've had a few bugs with other (currently untested) igt tools, can
> > you please add a new igt_tools testcase which just runs those? I'm
> > thinking of intel_reg_dumper and intel_reg_read (with some render ring
> > register that exists everywhere) on top of running intel_get_llc_size
> > here.
> 
> What would you like exactly for the check? Just to make sure platforms
> that should have LLC return a value > 1?

Just check that it runs without an non-zero exit code.

> As for testing the other tools, I personally want no involvement with
> intel_reg_dumper, since I feel maintaining it is a losing effort and
> would rather see effort put into quick_dump. I'll write tests for
> reg_read/reg_write though.

Again just checking whether the stuff runs without non-zero exit code. I
guess the same holds for quick_dump. As long as we still use reg_dumper
for debugging regressions I'd would be imo really good to have a little
bit of assurance that it's not completely broken.

Cheers, Daniel

> > And please also add eLLC size querying.
> 
> When I see review and signs of those getting merged, I'll gladly do
> that. For simple, potentially very useful patches, they're sitting
> around doing nothing for an awfully long time. And, I can no longer
> easily test them. So poop.
> 
> > -Daniel
> > 
> > > ---
> > >  tools/Makefile.am          |  1 +
> > >  tools/intel_get_llc_size.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 58 insertions(+)
> > >  create mode 100644 tools/intel_get_llc_size.c
> > > 
> > > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > > index 2519169..a064b65 100644
> > > --- a/tools/Makefile.am
> > > +++ b/tools/Makefile.am
> > > @@ -9,6 +9,7 @@ bin_PROGRAMS = 				\
> > >  	intel_bios_dumper 		\
> > >  	intel_bios_reader 		\
> > >  	intel_error_decode 		\
> > > +	intel_get_llc_size 		\
> > >  	intel_gpu_top 			\
> > >  	intel_gpu_time 			\
> > >  	intel_gtt 			\
> > > diff --git a/tools/intel_get_llc_size.c b/tools/intel_get_llc_size.c
> > > new file mode 100644
> > > index 0000000..bd384d2
> > > --- /dev/null
> > > +++ b/tools/intel_get_llc_size.c
> > > @@ -0,0 +1,57 @@
> > > +/*
> > > + * Copyright © 2013 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.
> > > + *
> > > + */
> > > +
> > > +#include <sys/ioctl.h>
> > > +#include "drmtest.h"
> > > +#include "i915_drm.h"
> > > +
> > > +static int get_llc_size(int fd)
> > > +{
> > > +	struct drm_i915_getparam gp;
> > > +	int size;
> > > +
> > > +	gp.param = I915_PARAM_HAS_LLC;
> > > +	gp.value = &size;
> > > +
> > > +	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> > > +		return 0;
> > > +
> > > +	return size;
> > > +}
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > +	int size, fd = drm_open_any();
> > > +
> > > +	size = get_llc_size(fd);
> > > +
> > > +	if (size == 0)
> > > +		fprintf(stdout, "Doesn't have LLC\n");
> > > +	else if (size == 1)
> > > +		fprintf(stdout, "Kernel is too old to determine LLC size\n");
> > > +	else
> > > +		fprintf(stdout, "LLC size = %dK\n", size>>10);
> > > +
> > > +	return 0;
> > > +}
> > > -- 
> > > 1.8.3.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] intel_get_llc_size: Small tool to query LLC size

[Ben Widawsky]

On Wed, Jul 10, 2013 at 07:15:43PM +0200, Daniel Vetter wrote:
> On Wed, Jul 10, 2013 at 09:58:38AM -0700, Ben Widawsky wrote:
> > On Wed, Jul 10, 2013 at 08:34:18AM +0200, Daniel Vetter wrote:
> > > On Tue, Jul 09, 2013 at 07:58:03PM -0700, Ben Widawsky wrote:
> > > > CC: Chad Versace <chad.versace@linux.intel.com>
> > > > CC: Bryan Bell <bryan.j.bell@intel.com>
> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > 
> > > So I think we should run this from igt and check its return value. And
> > > since we've had a few bugs with other (currently untested) igt tools, can
> > > you please add a new igt_tools testcase which just runs those? I'm
> > > thinking of intel_reg_dumper and intel_reg_read (with some render ring
> > > register that exists everywhere) on top of running intel_get_llc_size
> > > here.
> > 
> > What would you like exactly for the check? Just to make sure platforms
> > that should have LLC return a value > 1?
> 
> Just check that it runs without an non-zero exit code.

That doesn't really check the interface works as advertised though. Same
for the below.

> 
> > As for testing the other tools, I personally want no involvement with
> > intel_reg_dumper, since I feel maintaining it is a losing effort and
> > would rather see effort put into quick_dump. I'll write tests for
> > reg_read/reg_write though.
> 
> Again just checking whether the stuff runs without non-zero exit code. I
> guess the same holds for quick_dump. As long as we still use reg_dumper
> for debugging regressions I'd would be imo really good to have a little
> bit of assurance that it's not completely broken.
> 
> Cheers, Daniel

That's fine... just asking you to volunteer someone else who actually
uses that tool for it. I never use reg_dumper, and don't ask others to
use it.

> 
> > > And please also add eLLC size querying.
> > 
> > When I see review and signs of those getting merged, I'll gladly do
> > that. For simple, potentially very useful patches, they're sitting
> > around doing nothing for an awfully long time. And, I can no longer
> > easily test them. So poop.
> > 
> > > -Daniel
> > > 
> > > > ---
> > > >  tools/Makefile.am          |  1 +
> > > >  tools/intel_get_llc_size.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 58 insertions(+)
> > > >  create mode 100644 tools/intel_get_llc_size.c
> > > > 
> > > > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > > > index 2519169..a064b65 100644
> > > > --- a/tools/Makefile.am
> > > > +++ b/tools/Makefile.am
> > > > @@ -9,6 +9,7 @@ bin_PROGRAMS = 				\
> > > >  	intel_bios_dumper 		\
> > > >  	intel_bios_reader 		\
> > > >  	intel_error_decode 		\
> > > > +	intel_get_llc_size 		\
> > > >  	intel_gpu_top 			\
> > > >  	intel_gpu_time 			\
> > > >  	intel_gtt 			\
> > > > diff --git a/tools/intel_get_llc_size.c b/tools/intel_get_llc_size.c
> > > > new file mode 100644
> > > > index 0000000..bd384d2
> > > > --- /dev/null
> > > > +++ b/tools/intel_get_llc_size.c
> > > > @@ -0,0 +1,57 @@
> > > > +/*
> > > > + * Copyright © 2013 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.
> > > > + *
> > > > + */
> > > > +
> > > > +#include <sys/ioctl.h>
> > > > +#include "drmtest.h"
> > > > +#include "i915_drm.h"
> > > > +
> > > > +static int get_llc_size(int fd)
> > > > +{
> > > > +	struct drm_i915_getparam gp;
> > > > +	int size;
> > > > +
> > > > +	gp.param = I915_PARAM_HAS_LLC;
> > > > +	gp.value = &size;
> > > > +
> > > > +	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> > > > +		return 0;
> > > > +
> > > > +	return size;
> > > > +}
> > > > +
> > > > +int main(int argc, char **argv)
> > > > +{
> > > > +	int size, fd = drm_open_any();
> > > > +
> > > > +	size = get_llc_size(fd);
> > > > +
> > > > +	if (size == 0)
> > > > +		fprintf(stdout, "Doesn't have LLC\n");
> > > > +	else if (size == 1)
> > > > +		fprintf(stdout, "Kernel is too old to determine LLC size\n");
> > > > +	else
> > > > +		fprintf(stdout, "LLC size = %dK\n", size>>10);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > -- 
> > > > 1.8.3.2
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > 
> > -- 
> > Ben Widawsky, Intel Open Source Technology Center
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Expose LLC size to user space

[Chris Wilson]

On Wed, Jul 10, 2013 at 09:40:18AM -0700, Ben Widawsky wrote:
> On Wed, Jul 10, 2013 at 09:59:01AM +0100, Chris Wilson wrote:
> > On Tue, Jul 09, 2013 at 07:58:02PM -0700, Ben Widawsky wrote:
> > > The algorithm/information was originally written by Chad, though I
> > > changed the control flow, and I think his original code had a couple of
> > > bugs, though I didn't look very hard before rewriting. That could have
> > > also been different interpretations of the spec.
> > 
> > Just a cpuid query that can already be done more simply from userspace
> > (i.e. with no syscalls)? I was expecting a lot more magic.
> > -Chris
> > 
> > -- 
> > Chris Wilson, Intel Open Source Technology Centre
> 
> Chad wrote this originally for mesa. And yes, it's doable from
> userspace. Chatting with Daniel, we thought maybe other GPU clients
> might want this info, and so a central place to put the code would be
> nice, in case there are quirks or anything like that (I've had a
> particularly hard time figuring out if Xeon really has L3 or not).
> 
> So what's a central place? Libdrm, everybody uses that right? You can
> read /proc/cpuinfo as far as I know, but then you still need to query
> the HAS_LLC getparam to figure out what kind of L3 (or do your own
> chipset ID check).
> 
> In addition to the centrality argument, I noticed while poking around
> cpuid in the kernel that it is a vitalized function. I'm not sure what
> the purpose of this is, but it's something you can't fake in userspace.

In fact, isn't this functionality already part of arch/x86, and isn't the
value already exposed via /proc/cpuinfo?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] intel_get_llc_size: Small tool to query LLC size

[Daniel Vetter]

On Wed, Jul 10, 2013 at 10:24:27AM -0700, Ben Widawsky wrote:
> On Wed, Jul 10, 2013 at 07:15:43PM +0200, Daniel Vetter wrote:
> > On Wed, Jul 10, 2013 at 09:58:38AM -0700, Ben Widawsky wrote:
> > > On Wed, Jul 10, 2013 at 08:34:18AM +0200, Daniel Vetter wrote:
> > > > On Tue, Jul 09, 2013 at 07:58:03PM -0700, Ben Widawsky wrote:
> > > > > CC: Chad Versace <chad.versace@linux.intel.com>
> > > > > CC: Bryan Bell <bryan.j.bell@intel.com>
> > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > 
> > > > So I think we should run this from igt and check its return value. And
> > > > since we've had a few bugs with other (currently untested) igt tools, can
> > > > you please add a new igt_tools testcase which just runs those? I'm
> > > > thinking of intel_reg_dumper and intel_reg_read (with some render ring
> > > > register that exists everywhere) on top of running intel_get_llc_size
> > > > here.
> > > 
> > > What would you like exactly for the check? Just to make sure platforms
> > > that should have LLC return a value > 1?
> > 
> > Just check that it runs without an non-zero exit code.
> 
> That doesn't really check the interface works as advertised though. Same
> for the below.

It checks more than nothing and at least in the case of reg IO breakage in
the past that was good enough to catch things.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Expose LLC size to user space

[Ben Widawsky]

On Wed, Jul 10, 2013 at 06:40:02PM +0100, Chris Wilson wrote:
> On Wed, Jul 10, 2013 at 09:40:18AM -0700, Ben Widawsky wrote:
> > On Wed, Jul 10, 2013 at 09:59:01AM +0100, Chris Wilson wrote:
> > > On Tue, Jul 09, 2013 at 07:58:02PM -0700, Ben Widawsky wrote:
> > > > The algorithm/information was originally written by Chad, though I
> > > > changed the control flow, and I think his original code had a couple of
> > > > bugs, though I didn't look very hard before rewriting. That could have
> > > > also been different interpretations of the spec.
> > > 
> > > Just a cpuid query that can already be done more simply from userspace
> > > (i.e. with no syscalls)? I was expecting a lot more magic.
> > > -Chris
> > > 
> > > -- 
> > > Chris Wilson, Intel Open Source Technology Centre
> > 
> > Chad wrote this originally for mesa. And yes, it's doable from
> > userspace. Chatting with Daniel, we thought maybe other GPU clients
> > might want this info, and so a central place to put the code would be
> > nice, in case there are quirks or anything like that (I've had a
> > particularly hard time figuring out if Xeon really has L3 or not).
> > 
> > So what's a central place? Libdrm, everybody uses that right? You can
> > read /proc/cpuinfo as far as I know, but then you still need to query
> > the HAS_LLC getparam to figure out what kind of L3 (or do your own
> > chipset ID check).
> > 
> > In addition to the centrality argument, I noticed while poking around
> > cpuid in the kernel that it is a vitalized function. I'm not sure what
> > the purpose of this is, but it's something you can't fake in userspace.
> 
> In fact, isn't this functionality already part of arch/x86, and isn't the
> value already exposed via /proc/cpuinfo?
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
>
Yes to the /proc/cpuinfo bit (I said that). If the implication is just
to use that code, I did a quick search and couldn't find it. I can go
back and check again. Assuming it's externally exposed to drivers, I
would rather use that.

We still need the HAS_LLC check to differentiate the L3 though.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Expose LLC size to user space

[Chris Wilson]

On Wed, Jul 10, 2013 at 10:46:47AM -0700, Ben Widawsky wrote:
> On Wed, Jul 10, 2013 at 06:40:02PM +0100, Chris Wilson wrote:
> > On Wed, Jul 10, 2013 at 09:40:18AM -0700, Ben Widawsky wrote:
> > > On Wed, Jul 10, 2013 at 09:59:01AM +0100, Chris Wilson wrote:
> > > > On Tue, Jul 09, 2013 at 07:58:02PM -0700, Ben Widawsky wrote:
> > > > > The algorithm/information was originally written by Chad, though I
> > > > > changed the control flow, and I think his original code had a couple of
> > > > > bugs, though I didn't look very hard before rewriting. That could have
> > > > > also been different interpretations of the spec.
> > > > 
> > > > Just a cpuid query that can already be done more simply from userspace
> > > > (i.e. with no syscalls)? I was expecting a lot more magic.
> > > > -Chris
> > > > 
> > > > -- 
> > > > Chris Wilson, Intel Open Source Technology Centre
> > > 
> > > Chad wrote this originally for mesa. And yes, it's doable from
> > > userspace. Chatting with Daniel, we thought maybe other GPU clients
> > > might want this info, and so a central place to put the code would be
> > > nice, in case there are quirks or anything like that (I've had a
> > > particularly hard time figuring out if Xeon really has L3 or not).
> > > 
> > > So what's a central place? Libdrm, everybody uses that right? You can
> > > read /proc/cpuinfo as far as I know, but then you still need to query
> > > the HAS_LLC getparam to figure out what kind of L3 (or do your own
> > > chipset ID check).
> > > 
> > > In addition to the centrality argument, I noticed while poking around
> > > cpuid in the kernel that it is a vitalized function. I'm not sure what
> > > the purpose of this is, but it's something you can't fake in userspace.
> > 
> > In fact, isn't this functionality already part of arch/x86, and isn't the
> > value already exposed via /proc/cpuinfo?
> > -Chris
> > 
> > -- 
> > Chris Wilson, Intel Open Source Technology Centre
> >
> Yes to the /proc/cpuinfo bit (I said that). If the implication is just
> to use that code, I did a quick search and couldn't find it. I can go
> back and check again. Assuming it's externally exposed to drivers, I
> would rather use that.
> 
> We still need the HAS_LLC check to differentiate the L3 though.

You have to be careful about repurposing PARAM_HAS_LLC though. At the
moment it has a dual meaning that the kernel/gpu supports LLC and that
we default to placing objects in LLC.

If you must expose cache_size through a param, make it a new one and
make it explicit. I would also rather just use a param than parse
/proc/cpuinfo (especially as some paranoid setups prevent access to
/proc/cpuinfo).

But we really should not be calling cpuid from our driver, that just
looks very very ugly, and given cpuid's history will be a maintenance
burden.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Expose LLC size to user space

[Ben Widawsky]

On Wed, Jul 10, 2013 at 07:00:53PM +0100, Chris Wilson wrote:
> On Wed, Jul 10, 2013 at 10:46:47AM -0700, Ben Widawsky wrote:
> > On Wed, Jul 10, 2013 at 06:40:02PM +0100, Chris Wilson wrote:
> > > On Wed, Jul 10, 2013 at 09:40:18AM -0700, Ben Widawsky wrote:
> > > > On Wed, Jul 10, 2013 at 09:59:01AM +0100, Chris Wilson wrote:
> > > > > On Tue, Jul 09, 2013 at 07:58:02PM -0700, Ben Widawsky wrote:
> > > > > > The algorithm/information was originally written by Chad, though I
> > > > > > changed the control flow, and I think his original code had a couple of
> > > > > > bugs, though I didn't look very hard before rewriting. That could have
> > > > > > also been different interpretations of the spec.
> > > > > 
> > > > > Just a cpuid query that can already be done more simply from userspace
> > > > > (i.e. with no syscalls)? I was expecting a lot more magic.
> > > > > -Chris
> > > > > 
> > > > > -- 
> > > > > Chris Wilson, Intel Open Source Technology Centre
> > > > 
> > > > Chad wrote this originally for mesa. And yes, it's doable from
> > > > userspace. Chatting with Daniel, we thought maybe other GPU clients
> > > > might want this info, and so a central place to put the code would be
> > > > nice, in case there are quirks or anything like that (I've had a
> > > > particularly hard time figuring out if Xeon really has L3 or not).
> > > > 
> > > > So what's a central place? Libdrm, everybody uses that right? You can
> > > > read /proc/cpuinfo as far as I know, but then you still need to query
> > > > the HAS_LLC getparam to figure out what kind of L3 (or do your own
> > > > chipset ID check).
> > > > 
> > > > In addition to the centrality argument, I noticed while poking around
> > > > cpuid in the kernel that it is a vitalized function. I'm not sure what
> > > > the purpose of this is, but it's something you can't fake in userspace.
> > > 
> > > In fact, isn't this functionality already part of arch/x86, and isn't the
> > > value already exposed via /proc/cpuinfo?
> > > -Chris
> > > 
> > > -- 
> > > Chris Wilson, Intel Open Source Technology Centre
> > >
> > Yes to the /proc/cpuinfo bit (I said that). If the implication is just
> > to use that code, I did a quick search and couldn't find it. I can go
> > back and check again. Assuming it's externally exposed to drivers, I
> > would rather use that.
> > 
> > We still need the HAS_LLC check to differentiate the L3 though.
> 
> You have to be careful about repurposing PARAM_HAS_LLC though. At the
> moment it has a dual meaning that the kernel/gpu supports LLC and that
> we default to placing objects in LLC.
> 
> If you must expose cache_size through a param, make it a new one and
> make it explicit. I would also rather just use a param than parse
> /proc/cpuinfo (especially as some paranoid setups prevent access to
> /proc/cpuinfo).

I did a search, and I think my change is backward compatible, but if you
want a new param, I don't care.

> 
> But we really should not be calling cpuid from our driver, that just
> looks very very ugly, and given cpuid's history will be a maintenance
> burden.
> -Chris

Okay, let me try again to find where it is parsed by the arch/x86 code.

> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Expose LLC size to user space

[Chad Versace]

On 07/09/2013 07:58 PM, Ben Widawsky wrote:
> The algorithm/information was originally written by Chad, though I
> changed the control flow, and I think his original code had a couple of
> bugs, though I didn't look very hard before rewriting. That could have
> also been different interpretations of the spec.
>
> The excellent comments remain entirely copied from Chad's code.
>
> I've tested this on two platforms, and it seems to perform how I want.
>
> CC: Chad Versace <chad.versace@linux.intel.com>
> CC: Bryan Bell <bryan.j.bell@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>   drivers/gpu/drm/i915/i915_dma.c |  2 +-
>   drivers/gpu/drm/i915/i915_drv.h |  2 ++
>   drivers/gpu/drm/i915/i915_gem.c | 53 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 0e22142..377949e 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -974,7 +974,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
>   		value = 1;
>   		break;
>   	case I915_PARAM_HAS_LLC:
> -		value = HAS_LLC(dev);
> +		value = dev_priv->llc_size;
>   		break;
>   	case I915_PARAM_HAS_ALIASING_PPGTT:
>   		value = dev_priv->mm.aliasing_ppgtt ? 1 : 0;

I would like to see a dedicated param for the llc size. 'has' is always
boolean valued, right?

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c8d6104..43a549d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1187,6 +1187,8 @@ typedef struct drm_i915_private {
>   	/* Old dri1 support infrastructure, beware the dragons ya fools entering
>   	 * here! */
>   	struct i915_dri1_state dri1;
> +
> +	size_t llc_size;
>   } drm_i915_private_t;
>
>   /* Iterate over initialised rings */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index af61be8..a070686 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4282,6 +4282,57 @@ i915_gem_lastclose(struct drm_device *dev)
>   		DRM_ERROR("failed to idle hardware: %d\n", ret);
>   }
>
> +/**
> + * Return the size, in bytes, of the CPU L3 cache size. If the CPU has no L3
> + * cache, or if an error occurs in obtaining the cache size, then return 0.
> + * From "Intel Processor Identification and the CPUID Instruction > 5.15
> + * Deterministic Cache Parmaeters (Function 04h)":
> + *    When EAX is initialized to a value of 4, the CPUID instruction returns
> + *    deterministic cache information in the EAX, EBX, ECX and EDX registers.
> + *    This function requires ECX be initialized with an index which indicates
> + *    which cache to return information about. The OS is expected to call this
> + *    function (CPUID.4) with ECX = 0, 1, 2, until EAX[4:0] == 0, indicating no
> + *    more caches. The order in which the caches are returned is not specified
> + *    and may change at Intel's discretion.
> + *
> + * Equation 5-4. Calculating the Cache Size in bytes:
> + *          = (Ways +1) � (Partitions +1) � (Line Size +1) � (Sets +1)
> + *          = (EBX[31:22] +1) � (EBX[21:12] +1) � (EBX[11:0] +1 � (ECX + 1)
> + */
> +static size_t get_llc_size(struct drm_device *dev)
> +{
> +	u8 cnt = 0;
> +	unsigned int eax, ebx, ecx, edx;
> +
> +	if (!HAS_LLC(dev))
> +		return 0;
> +
> +	do {
> +		uint32_t cache_level;
> +		uint32_t associativity, line_partitions, line_size, sets;
> +
> +		eax = 4;
> +		ecx = cnt;
> +		__cpuid(&eax, &ebx, &ecx, &edx);
> +
> +		cache_level = (eax >> 5) & 0x7;
> +		if (cache_level != 3)
> +			continue;
> +
> +		associativity = ((ebx >> 22) & 0x3ff) + 1;
> +		line_partitions = ((ebx >> 12) & 0x3ff) + 1;
> +		line_size = (ebx & 0xfff) + 1;
> +		sets = ecx + 1;
> +
> +		return associativity * line_partitions * line_size * sets;
> +	} while (eax & 0x1f && ++cnt);
> +
> +	/* Let user space know we have LLC, but we can't figure it out */
> +	DRM_DEBUG_DRIVER("Couldn't find LLC size. Bug?\n");
> +	return 1;
> +}

This function looks good to me, since I wrote most of it ;)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Expose LLC size to user space

[Ben Widawsky]

On Wed, Jul 10, 2013 at 05:16:54PM -0700, Chad Versace wrote:
> On 07/09/2013 07:58 PM, Ben Widawsky wrote:
> >The algorithm/information was originally written by Chad, though I
> >changed the control flow, and I think his original code had a couple of
> >bugs, though I didn't look very hard before rewriting. That could have
> >also been different interpretations of the spec.
> >
> >The excellent comments remain entirely copied from Chad's code.
> >
> >I've tested this on two platforms, and it seems to perform how I want.
> >
> >CC: Chad Versace <chad.versace@linux.intel.com>
> >CC: Bryan Bell <bryan.j.bell@intel.com>
> >Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >---
> >  drivers/gpu/drm/i915/i915_dma.c |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.h |  2 ++
> >  drivers/gpu/drm/i915/i915_gem.c | 53 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 56 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >index 0e22142..377949e 100644
> >--- a/drivers/gpu/drm/i915/i915_dma.c
> >+++ b/drivers/gpu/drm/i915/i915_dma.c
> >@@ -974,7 +974,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
> >  		value = 1;
> >  		break;
> >  	case I915_PARAM_HAS_LLC:
> >-		value = HAS_LLC(dev);
> >+		value = dev_priv->llc_size;
> >  		break;
> >  	case I915_PARAM_HAS_ALIASING_PPGTT:
> >  		value = dev_priv->mm.aliasing_ppgtt ? 1 : 0;
> 
> I would like to see a dedicated param for the llc size. 'has' is always
> boolean valued, right?

Sounds like Chris wants the same anyway, but no. Getparam is always an
int. Just the name is, 'has'

> 
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index c8d6104..43a549d 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -1187,6 +1187,8 @@ typedef struct drm_i915_private {
> >  	/* Old dri1 support infrastructure, beware the dragons ya fools entering
> >  	 * here! */
> >  	struct i915_dri1_state dri1;
> >+
> >+	size_t llc_size;
> >  } drm_i915_private_t;
> >
> >  /* Iterate over initialised rings */
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index af61be8..a070686 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -4282,6 +4282,57 @@ i915_gem_lastclose(struct drm_device *dev)
> >  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> >  }
> >
> >+/**
> >+ * Return the size, in bytes, of the CPU L3 cache size. If the CPU has no L3
> >+ * cache, or if an error occurs in obtaining the cache size, then return 0.
> >+ * From "Intel Processor Identification and the CPUID Instruction > 5.15
> >+ * Deterministic Cache Parmaeters (Function 04h)":
> >+ *    When EAX is initialized to a value of 4, the CPUID instruction returns
> >+ *    deterministic cache information in the EAX, EBX, ECX and EDX registers.
> >+ *    This function requires ECX be initialized with an index which indicates
> >+ *    which cache to return information about. The OS is expected to call this
> >+ *    function (CPUID.4) with ECX = 0, 1, 2, until EAX[4:0] == 0, indicating no
> >+ *    more caches. The order in which the caches are returned is not specified
> >+ *    and may change at Intel's discretion.
> >+ *
> >+ * Equation 5-4. Calculating the Cache Size in bytes:
> >+ *          = (Ways +1) � (Partitions +1) � (Line Size +1) � (Sets +1)
> >+ *          = (EBX[31:22] +1) � (EBX[21:12] +1) � (EBX[11:0] +1 � (ECX + 1)
> >+ */
> >+static size_t get_llc_size(struct drm_device *dev)
> >+{
> >+	u8 cnt = 0;
> >+	unsigned int eax, ebx, ecx, edx;
> >+
> >+	if (!HAS_LLC(dev))
> >+		return 0;
> >+
> >+	do {
> >+		uint32_t cache_level;
> >+		uint32_t associativity, line_partitions, line_size, sets;
> >+
> >+		eax = 4;
> >+		ecx = cnt;
> >+		__cpuid(&eax, &ebx, &ecx, &edx);
> >+
> >+		cache_level = (eax >> 5) & 0x7;
> >+		if (cache_level != 3)
> >+			continue;
> >+
> >+		associativity = ((ebx >> 22) & 0x3ff) + 1;
> >+		line_partitions = ((ebx >> 12) & 0x3ff) + 1;
> >+		line_size = (ebx & 0xfff) + 1;
> >+		sets = ecx + 1;
> >+
> >+		return associativity * line_partitions * line_size * sets;
> >+	} while (eax & 0x1f && ++cnt);
> >+
> >+	/* Let user space know we have LLC, but we can't figure it out */
> >+	DRM_DEBUG_DRIVER("Couldn't find LLC size. Bug?\n");
> >+	return 1;
> >+}
> 
> This function looks good to me, since I wrote most of it ;)
> 

Still arguing over whether or not to try to pull it from the x86 core. I
tried to get some comment from hpa today , but haven't heard anything
yet.

I do agree if we can just take the code from the x86 core it would be
best, but AFAICT I'd need to change some code, or do an almost equal
amount of logic to determine l2 vs. l3. Thinking about it now though, I
suppose I can assume if HAS_LLC, then whatever cache size was obtained
during boot is l3... dunno.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] [v2] drm/i915: Expose LLC size to user space

[Ben Widawsky]

The algorithm/information was originally written by Chad, though I
changed the control flow, and I think his original code had a couple of
bugs, though I didn't look very hard before rewriting. That could have
also been different interpretations of the spec.

I've tested this on two platforms, and it seems to perform how I want.

With this patch is a small tool for igt to query the size. This can be
used as a reference for DRI clients wishing to query the information.

v2: Update name of the SDM location (Bryan)
Dissent: Use a new param instead of reusing HAS_LLC param (Chris, Chad)
Fix unicode multiply symbol (Ben)

CC: Chad Versace <chad.versace@linux.intel.com>
CC: Bryan Bell <bryan.j.bell@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |  3 +++
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_gem.c | 55 +++++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h     |  1 +
 4 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0e22142..1224586 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1000,6 +1000,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 	case I915_PARAM_HAS_EXEC_HANDLE_LUT:
 		value = 1;
 		break;
+	case I915_PARAM_LLC_SIZE:
+		value = dev_priv->llc_size;
+		break;
 	default:
 		DRM_DEBUG("Unknown parameter %d\n", param->param);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c8d6104..43a549d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1187,6 +1187,8 @@ typedef struct drm_i915_private {
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
+
+	size_t llc_size;
 } drm_i915_private_t;
 
 /* Iterate over initialised rings */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index af61be8..629837d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4282,6 +4282,59 @@ i915_gem_lastclose(struct drm_device *dev)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
 }
 
+/**
+ * Return the size, in bytes, of the CPU L3 cache size. If the CPU has no L3
+ * cache, or if an error occurs in obtaining the cache size, then return 0.
+ * From Intel 64 and IA32 Architectures Developer's Manual: Vol. 2A -> section
+ * 3.2 -> CPUID
+ *
+ * Deterministic Cache Parmaeters (Function 04h)":
+ *    When EAX is initialized to a value of 4, the CPUID instruction returns
+ *    deterministic cache information in the EAX, EBX, ECX and EDX registers.
+ *    This function requires ECX be initialized with an index which indicates
+ *    which cache to return information about. The OS is expected to call this
+ *    function (CPUID.4) with ECX = 0, 1, 2, until EAX[4:0] == 0, indicating no
+ *    more caches. The order in which the caches are returned is not specified
+ *    and may change at Intel's discretion.
+ *
+ * Calculating the Cache Size in bytes:
+ *          = (Ways +1) * (Partitions +1) * (Line Size +1) * (Sets +1)
+ *          = (EBX[31:22] +1) * (EBX[21:12] +1) * (EBX[11:0] +1 * (ECX + 1)
+ */
+static size_t get_llc_size(struct drm_device *dev)
+{
+	u8 cnt = 0;
+	unsigned int eax, ebx, ecx, edx;
+
+	if (!HAS_LLC(dev))
+		return 0;
+
+	do {
+		uint32_t cache_level;
+		uint32_t associativity, line_partitions, line_size, sets;
+
+		eax = 4;
+		ecx = cnt;
+		__cpuid(&eax, &ebx, &ecx, &edx);
+
+		cache_level = (eax >> 5) & 0x7;
+		if (cache_level != 3)
+			continue;
+
+		associativity = ((ebx >> 22) & 0x3ff) + 1;
+		line_partitions = ((ebx >> 12) & 0x3ff) + 1;
+		line_size = (ebx & 0xfff) + 1;
+		sets = ecx + 1;
+
+		return associativity * line_partitions * line_size * sets;
+	} while (eax & 0x1f && ++cnt);
+
+	/* Let user space know we have non-zero LLC, we can't figure it out */
+	DRM_DEBUG_DRIVER("Couldn't find LLC size. Bug?\n");
+	return 1;
+}
+
+
 static void
 init_ring_lists(struct intel_ring_buffer *ring)
 {
@@ -4333,6 +4386,8 @@ i915_gem_load(struct drm_device *dev)
 	else
 		dev_priv->num_fence_regs = 8;
 
+	dev_priv->llc_size = get_llc_size(dev);
+
 	/* Initialize fence registers to zero */
 	INIT_LIST_HEAD(&dev_priv->mm.fence_list);
 	i915_gem_restore_fences(dev);
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 923ed7f..c54559e 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -310,6 +310,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_HAS_PINNED_BATCHES	 24
 #define I915_PARAM_HAS_EXEC_NO_RELOC	 25
 #define I915_PARAM_HAS_EXEC_HANDLE_LUT   26
+#define I915_PARAM_LLC_SIZE		 27
 
 typedef struct drm_i915_getparam {
 	int param;
-- 
1.8.3.2

[PATCH 1/2] intel_get_llc_size: Small tool to query LLC size

[Ben Widawsky]

v2: Use the new param

CC: Chad Versace <chad.versace@linux.intel.com>
CC: Bryan Bell <bryan.j.bell@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tools/Makefile.am          |  1 +
 tools/intel_get_llc_size.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100644 tools/intel_get_llc_size.c

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 2519169..a064b65 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -9,6 +9,7 @@ bin_PROGRAMS = 				\
 	intel_bios_dumper 		\
 	intel_bios_reader 		\
 	intel_error_decode 		\
+	intel_get_llc_size 		\
 	intel_gpu_top 			\
 	intel_gpu_time 			\
 	intel_gtt 			\
diff --git a/tools/intel_get_llc_size.c b/tools/intel_get_llc_size.c
new file mode 100644
index 0000000..498e252
--- /dev/null
+++ b/tools/intel_get_llc_size.c
@@ -0,0 +1,58 @@
+/*
+ * Copyright © 2013 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.
+ *
+ */
+
+#include <sys/ioctl.h>
+#include "drmtest.h"
+#include "i915_drm.h"
+
+#define LOCAL__I915_PARAM_LLC_SIZE 27
+static int get_llc_size(int fd)
+{
+	struct drm_i915_getparam gp;
+	int size;
+
+	gp.param = LOCAL__I915_PARAM_LLC_SIZE;
+	gp.value = &size;
+
+	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
+		return 0;
+
+	return size;
+}
+
+int main(int argc, char **argv)
+{
+	int size, fd = drm_open_any();
+
+	size = get_llc_size(fd);
+
+	if (size == 0)
+		fprintf(stdout, "Doesn't have LLC\n");
+	else if (size == 1)
+		fprintf(stdout, "Kernel is too old to determine LLC size\n");
+	else
+		fprintf(stdout, "LLC size = %dK\n", size>>10);
+
+	return 0;
+}
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] tests: Basic tools tester

[Ben Widawsky]

v2: Remove the reg write test because it breaks pre gen5
Add the llc size param test

Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tests/Makefile.am |  1 +
 tests/drm_lib.sh  |  4 ++++
 tests/tools_test  | 21 +++++++++++++++++++++
 3 files changed, 26 insertions(+)
 create mode 100755 tests/tools_test

diff --git a/tests/Makefile.am b/tests/Makefile.am
index a422899..ccc97b8 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -115,6 +115,7 @@ TESTS_scripts_M = \
 
 TESTS_scripts = \
 	test_rte_check \
+	tools_test \
 	debugfs_reader \
 	debugfs_emon_crash \
 	sysfs_l3_parity \
diff --git a/tests/drm_lib.sh b/tests/drm_lib.sh
index 5ca815b..5975b58 100755
--- a/tests/drm_lib.sh
+++ b/tests/drm_lib.sh
@@ -4,6 +4,10 @@ die() {
 	exit 1
 }
 
+do_or_die() {
+	$@ > /dev/null 2>&1 || (echo "FAIL: $@ ($?)" && exit -1)
+}
+
 if [ -d /debug/dri ] ; then
 	debugfs_path=/debug/dri
 fi
diff --git a/tests/tools_test b/tests/tools_test
new file mode 100755
index 0000000..17f7822
--- /dev/null
+++ b/tests/tools_test
@@ -0,0 +1,21 @@
+#!/bin/bash
+# Test some of the most critical tools we have accidentally broken before.
+# TODO: Possibly make tests parse output
+
+whoami | grep -q root || ( echo ERROR: not running as root; exit 1 )
+
+SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )"
+. $SOURCE_DIR/drm_lib.sh
+
+# ARB_MODE has existed for many gens. It differs per gen, but should be safe to
+# read regardless
+do_or_die "$SOURCE_DIR/../tools/intel_reg_read 0x4030"
+
+do_or_die "$SOURCE_DIR/../tools/intel_reg_dumper"
+
+do_or_die "$SOURCE_DIR/../tools/intel_get_llc_size"
+
+# TODO: Add more tests
+
+exit 0
+
-- 
1.8.3.2

Re: [PATCH] [v2] drm/i915: Expose LLC size to user space

[Chris Wilson]

On Thu, Jul 11, 2013 at 11:52:12AM -0700, Ben Widawsky wrote:
> The algorithm/information was originally written by Chad, though I
> changed the control flow, and I think his original code had a couple of
> bugs, though I didn't look very hard before rewriting. That could have
> also been different interpretations of the spec.
> 
> I've tested this on two platforms, and it seems to perform how I want.
> 
> With this patch is a small tool for igt to query the size. This can be
> used as a reference for DRI clients wishing to query the information.
> 
> v2: Update name of the SDM location (Bryan)
> Dissent: Use a new param instead of reusing HAS_LLC param (Chris, Chad)
> Fix unicode multiply symbol (Ben)

Shrug. I still dislike calling it LLC_SIZE since it is actually L3_SIZE.
For very similar reasons, easily finding out L2 size would be useful, as
would L4.

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c8d6104..43a549d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1187,6 +1187,8 @@ typedef struct drm_i915_private {
>  	/* Old dri1 support infrastructure, beware the dragons ya fools entering
>  	 * here! */
>  	struct i915_dri1_state dri1;
> +
> +	size_t llc_size;

Don't put it below the dungeons, it will be tainted.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] [v2] drm/i915: Expose LLC size to user space

[Chad Versace]

On 07/11/2013 11:52 AM, Ben Widawsky wrote:
> The algorithm/information was originally written by Chad, though I
> changed the control flow, and I think his original code had a couple of
> bugs, though I didn't look very hard before rewriting. That could have
> also been different interpretations of the spec.
>
> I've tested this on two platforms, and it seems to perform how I want.
>
> With this patch is a small tool for igt to query the size. This can be
> used as a reference for DRI clients wishing to query the information.
>
> v2: Update name of the SDM location (Bryan)
> Dissent: Use a new param instead of reusing HAS_LLC param (Chris, Chad)
> Fix unicode multiply symbol (Ben)
>
> CC: Chad Versace <chad.versace@linux.intel.com>
> CC: Bryan Bell <bryan.j.bell@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>   drivers/gpu/drm/i915/i915_dma.c |  3 +++
>   drivers/gpu/drm/i915/i915_drv.h |  2 ++
>   drivers/gpu/drm/i915/i915_gem.c | 55 +++++++++++++++++++++++++++++++++++++++++
>   include/uapi/drm/i915_drm.h     |  1 +
>   4 files changed, 61 insertions(+)

As long as this is tested and returns the same as L3 in /proc/cpuinfo, this patch is
Reviewed-by: Chad Versace <chad.versace@linux.intel.com>

Re: [PATCH 1/2] intel_get_llc_size: Small tool to query LLC size

[Chad Versace]

On 07/11/2013 11:53 AM, Ben Widawsky wrote:
> v2: Use the new param
>
> CC: Chad Versace <chad.versace@linux.intel.com>
> CC: Bryan Bell <bryan.j.bell@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>   tools/Makefile.am          |  1 +
>   tools/intel_get_llc_size.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 59 insertions(+)
>   create mode 100644 tools/intel_get_llc_size.c
>
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index 2519169..a064b65 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -9,6 +9,7 @@ bin_PROGRAMS = 				\
>   	intel_bios_dumper 		\
>   	intel_bios_reader 		\
>   	intel_error_decode 		\
> +	intel_get_llc_size 		\
>   	intel_gpu_top 			\
>   	intel_gpu_time 			\
>   	intel_gtt 			\
> diff --git a/tools/intel_get_llc_size.c b/tools/intel_get_llc_size.c
> new file mode 100644
> index 0000000..498e252
> --- /dev/null
> +++ b/tools/intel_get_llc_size.c
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright © 2013 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.
> + *
> + */
> +
> +#include <sys/ioctl.h>
> +#include "drmtest.h"
> +#include "i915_drm.h"
> +
> +#define LOCAL__I915_PARAM_LLC_SIZE 27
> +static int get_llc_size(int fd)
> +{
> +	struct drm_i915_getparam gp;
> +	int size;
> +
> +	gp.param = LOCAL__I915_PARAM_LLC_SIZE;
> +	gp.value = &size;
> +
> +	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> +		return 0;
> +
> +	return size;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int size, fd = drm_open_any();
> +
> +	size = get_llc_size(fd);
> +
> +	if (size == 0)
> +		fprintf(stdout, "Doesn't have LLC\n");
> +	else if (size == 1)
> +		fprintf(stdout, "Kernel is too old to determine LLC size\n");
> +	else
> +		fprintf(stdout, "LLC size = %dK\n", size>>10);

This if-tree looks wrong. If the kernel doesn't know about I915_PARAM_LLC_SIZE,
the ioctl returns -1, and get_llc_size() returns 0, and the if-tree prints the
wrong error.

How about making get_llc_size() return -1 on error? Or am I missing something here?

> +
> +	return 0;
> +}
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] intel_get_llc_size: Small tool to query LLC size

[Ben Widawsky]

On Fri, Jul 12, 2013 at 10:39:51AM -0700, Chad Versace wrote:
> On 07/11/2013 11:53 AM, Ben Widawsky wrote:
> >v2: Use the new param
> >
> >CC: Chad Versace <chad.versace@linux.intel.com>
> >CC: Bryan Bell <bryan.j.bell@intel.com>
> >Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >---
> >  tools/Makefile.am          |  1 +
> >  tools/intel_get_llc_size.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 59 insertions(+)
> >  create mode 100644 tools/intel_get_llc_size.c
> >
> >diff --git a/tools/Makefile.am b/tools/Makefile.am
> >index 2519169..a064b65 100644
> >--- a/tools/Makefile.am
> >+++ b/tools/Makefile.am
> >@@ -9,6 +9,7 @@ bin_PROGRAMS = 				\
> >  	intel_bios_dumper 		\
> >  	intel_bios_reader 		\
> >  	intel_error_decode 		\
> >+	intel_get_llc_size 		\
> >  	intel_gpu_top 			\
> >  	intel_gpu_time 			\
> >  	intel_gtt 			\
> >diff --git a/tools/intel_get_llc_size.c b/tools/intel_get_llc_size.c
> >new file mode 100644
> >index 0000000..498e252
> >--- /dev/null
> >+++ b/tools/intel_get_llc_size.c
> >@@ -0,0 +1,58 @@
> >+/*
> >+ * Copyright © 2013 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.
> >+ *
> >+ */
> >+
> >+#include <sys/ioctl.h>
> >+#include "drmtest.h"
> >+#include "i915_drm.h"
> >+
> >+#define LOCAL__I915_PARAM_LLC_SIZE 27
> >+static int get_llc_size(int fd)
> >+{
> >+	struct drm_i915_getparam gp;
> >+	int size;
> >+
> >+	gp.param = LOCAL__I915_PARAM_LLC_SIZE;
> >+	gp.value = &size;
> >+
> >+	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> >+		return 0;
> >+
> >+	return size;
> >+}
> >+
> >+int main(int argc, char **argv)
> >+{
> >+	int size, fd = drm_open_any();
> >+
> >+	size = get_llc_size(fd);
> >+
> >+	if (size == 0)
> >+		fprintf(stdout, "Doesn't have LLC\n");
> >+	else if (size == 1)
> >+		fprintf(stdout, "Kernel is too old to determine LLC size\n");
> >+	else
> >+		fprintf(stdout, "LLC size = %dK\n", size>>10);
> 
> This if-tree looks wrong. If the kernel doesn't know about I915_PARAM_LLC_SIZE,
> the ioctl returns -1, and get_llc_size() returns 0, and the if-tree prints the
> wrong error.
> 
> How about making get_llc_size() return -1 on error? Or am I missing something here?
>

Looks like the tool needed updating after the last param change and I
missed it. It should have been correct before (though in the case of no
HAS_LLC param it was also bad). Your suggestion is the right thing to do.

If Daniel decides to merge the kernel patch, I will fix this.

> 
> >+
> >+	return 0;
> >+}
> >
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx