On Thu, Jul 31, 2014 at 04:53:34AM -0700, Mcaulay, Alistair wrote:
> Hi Jeff,
>
> These patches look like they solve the problem well. I've added some comments
> in amongst the code.
>
> Thanks,
> Alistair.
>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
> jeff.mc...@intel.com
> Sent: Thursday, July 31, 2014 3:00 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 1/2] drm/i915: Slice/Subslice/EU info via GETPARAM
>
> From: Jeff McGee <jeff.mc...@intel.com>
>
> Define a struct to capture information on the device's Slice/Subslice/EU
> (SSEU) configuration. Add this struct to the main device info struct.
> Define a packed bitfield form for the SSEU info and share it with userspace
> via a new GETPARAM option.
>
> Starting with Cherryview, devices may have a varying number of EU for a given
> ID due to creative fusing. The surest way to determine the configuration is
> by reading fuses which is best done in the kernel and communicated to
> userspace. The immediate need from userspace is to determine the number of
> threads of compute work that can be safely submitted.
>
> The definition of SSEU as a new drm/i915 component, with its own header file
> and soon-to-be source file, is in anticipation of lots of upcoming code for
> its management, particularly the power gating functionality.
>
> Signed-off-by: Jeff McGee <jeff.mc...@intel.com>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> drivers/gpu/drm/i915/i915_drv.h | 3 +++
> drivers/gpu/drm/i915/intel_sseu.h | 40
> +++++++++++++++++++++++++++++++++++++++
> include/uapi/drm/i915_drm.h | 18 ++++++++++++++++++
> 4 files changed, 64 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/intel_sseu.h
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c
> b/drivers/gpu/drm/i915/i915_dma.c index 2e7f03a..f581848 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1025,6 +1025,9 @@ static int i915_getparam(struct drm_device *dev, void
> *data,
> case I915_PARAM_CMD_PARSER_VERSION:
> value = i915_cmd_parser_get_version();
> break;
> + case I915_PARAM_SSEU_INFO:
> + value = INTEL_INFO(dev)->sseu.gp_sseu_info;
> + 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 18c9ad8..01adafd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -45,6 +45,7 @@
> #include <linux/intel-iommu.h>
> #include <linux/kref.h>
> #include <linux/pm_qos.h>
> +#include "intel_sseu.h"
>
> /* General customization:
> */
> @@ -562,6 +563,8 @@ struct intel_device_info {
> int trans_offsets[I915_MAX_TRANSCODERS];
> int palette_offsets[I915_MAX_PIPES];
> int cursor_offsets[I915_MAX_PIPES];
> + /* Slice/Subslice/EU info */
> + struct intel_sseu_info sseu;
> };
>
> #undef DEFINE_FLAG
> diff --git a/drivers/gpu/drm/i915/intel_sseu.h
> b/drivers/gpu/drm/i915/intel_sseu.h
> new file mode 100644
> index 0000000..7db7175
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_sseu.h
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright © 2014 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 _INTEL_SSEU_H_
> +#define _INTEL_SSEU_H_
> +
> +struct intel_sseu_info {
> + /* Total slice count */
> + unsigned int slice_cnt;
> + /* Total subslice count */
> + unsigned int subslice_cnt;
> + /* Total execution unit count */
> + unsigned int eu_cnt;
> + /* Thread count per EU */
> + unsigned int threads_per_eu;
> + /* Bit field representation for I915_PARAM_SSEU_INFO */
> + u32 gp_sseu_info;
> +};
> +
> +#endif
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index
> ff57f07..b99c1a2 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -171,6 +171,23 @@ typedef struct _drm_i915_sarea { #define
> I915_BOX_TEXTURE_LOAD 0x8 #define I915_BOX_LOST_CONTEXT 0x10
>
> +/*
> + * Slice/Subslice/EU Info
> + * - Accessed via GETPARAM ioctl option I915_PARAM_SSEU_INFO
> + * - SLICE_CNT: total slice count
> + * - SUBSLICE_CNT: total subslice count
> + * - EU_CNT: total execution unit count
> + * - THREADS_PER_EU: thread count per EU */
> +#define I915_SSEU_INFO_SLICE_CNT_MASK 0xf
> +#define I915_SSEU_INFO_SLICE_CNT_SHIFT 0
> +#define I915_SSEU_INFO_SUBSLICE_CNT_MASK (0x3f<<4)
>
> Why not use the shift defines here?
> #define I915_SSEU_INFO_SUBSLICE_CNT_MASK (0x3f<<
> I915_SSEU_INFO_SUBSLICE_CNT_SHIFT)
> etc
>
The macro names are so long right now (in order to be descriptive) that
doing this will exceed 80 character line length, and so is a readability
issue for some. Otherwise I agree with you. I'll see what I can do with
the names and perhaps ask for a pass on the line length.
-Jeff
> +#define I915_SSEU_INFO_SUBSLICE_CNT_SHIFT 4
> +#define I915_SSEU_INFO_EU_CNT_MASK (0xff<<10)
> +#define I915_SSEU_INFO_EU_CNT_SHIFT 10
> +#define I915_SSEU_INFO_THREADS_PER_EU_MASK (0xf<<18)
> +#define I915_SSEU_INFO_THREADS_PER_EU_SHIFT 18
>
> There are 10 bits left unused here. Is it likely another field could be added
> later?
> If not, the masks could be widened to be more future proof.
>
Yes, I think with power gating, we will need additional flags here. I
feel like the ranges here are pretty future proof: max slice = 15,
max subslice = 63, max eu = 255, max threads per eu = 15. We could
probably dedicate a few more bits to these and still have room for flags.
Any suggestions on what range to increase?
-Jeff
> +
> /* I915 specific ioctls
> * The device specific ioctl range is 0x40 to 0x79.
> */
> @@ -340,6 +357,7 @@ typedef struct drm_i915_irq_wait {
> #define I915_PARAM_HAS_EXEC_HANDLE_LUT 26
> #define I915_PARAM_HAS_WT 27
> #define I915_PARAM_CMD_PARSER_VERSION 28
> +#define I915_PARAM_SSEU_INFO 29
>
> typedef struct drm_i915_getparam {
> int param;
> --
> 2.0.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
