Re: [PATCH v3] drm/i915 : Added Programming of the MOCS

From: Daniel Vetter <daniel@ffwll.ch>
To: Peter Antoine <peter.antoine@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3] drm/i915 : Added Programming of the MOCS
Date: Mon, 15 Jun 2015 14:51:35 +0200	[thread overview]
Message-ID: <20150615125135.GQ8341@phenom.ffwll.local> (raw)
In-Reply-To: <1433923936-20014-1-git-send-email-peter.antoine@intel.com>

On Wed, Jun 10, 2015 at 09:12:16AM +0100, Peter Antoine wrote:
> This change adds the programming of the MOCS registers to the gen 9+
> platforms. This change set programs the MOCS register values to a set
> of values that are defined to be optimal.
> 
> It creates a fixed register set that is programmed across the different
> engines so that all engines have the same table. This is done as the
> main RCS context only holds the registers for itself and the shared
> L3 values. By trying to keep the registers consistent across the
> different engines it should make the programming for the registers
> consistent.
> 
> v2:
> -'static const' for private data structures and style changes.(Matt Turner)
> v3:
> - Make the tables "slightly" more readable. (Damien Lespiau)
> - Updated tables fix performance regression.
> 
> 
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile     |   3 +-
>  drivers/gpu/drm/i915/i915_reg.h   |   9 ++
>  drivers/gpu/drm/i915/intel_lrc.c  |  68 ++++++++++
>  drivers/gpu/drm/i915/intel_mocs.c | 252 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_mocs.h | 119 ++++++++++++++++++
>  5 files changed, 450 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_mocs.c
>  create mode 100644 drivers/gpu/drm/i915/intel_mocs.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index b7ddf48..cd7b910 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -36,7 +36,8 @@ i915-y += i915_cmd_parser.o \
>  	  i915_trace_points.o \
>  	  intel_lrc.o \
>  	  intel_ringbuffer.o \
> -	  intel_uncore.o
> +	  intel_uncore.o \
> +	  intel_mocs.o
>  
>  # autogenerated null render state
>  i915-y += intel_renderstate_gen6.o \
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7213224..3a435b5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7829,4 +7829,13 @@ enum skl_disp_power_wells {
>  #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
>  #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
>  
> +/* MOCS (Memory Object Control State) registers */
> +#define GEN9_LNCFCMOCS0		(0xB020)	/* L3 Cache Control base */
> +
> +#define GEN9_GFX_MOCS_0		(0xc800)	/* Graphics MOCS base register*/
> +#define GEN9_MFX0_MOCS_0	(0xc900)	/* Media 0 MOCS base register*/
> +#define GEN9_MFX1_MOCS_0	(0xcA00)	/* Media 1 MOCS base register*/
> +#define GEN9_VEBOX_MOCS_0	(0xcB00)	/* Video MOCS base register*/
> +#define GEN9_BLT_MOCS_0		(0xcc00)	/* Blitter MOCS base register*/
> +
>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 9f5485d..c875569 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -135,6 +135,7 @@
>  #include <drm/drmP.h>
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
> +#include "intel_mocs.h"
>  
>  #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
>  #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
> @@ -1370,6 +1371,67 @@ out:
>  	return ret;
>  }
>  
> +/*
> + * i915_gem_program_mocs() - program the MOCS register.
> + *
> + * ring:	The ring that the programming batch will be run in.
> + * ctx:		The intel_context to be used.
> + *
> + * This function will emit a batch buffer with the values required for
> + * programming the MOCS register values for all the currenly supported
> + * rings.
> + *
> + * Return: 0 on success, otherwise the error status.
> + */
> +static int i915_gem_program_mocs(struct intel_engine_cs *ring,
> +			  struct intel_context *ctx)

If you go with a separate source file then imo this would be the function
to move there. As a rule of thumb if your new C file is full of non-static
functions used somewhere else and you need plenty of header definitions to
make it all work then you've split things in a useless way. Separate
source files are only useful if you actually manage to encapsulate things
a bit. If that encapsulation isn't given then bothering with all the
kerneldoc isn't worth it eithe since due to the tight coupling it'll
outdate extremely fast.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx