Re: [PATCH 44/89] drm/i915/skl: Register definitions and macros for SKL Watermark regs

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 04, 2014 at 12:27:10PM +0100, Damien Lespiau wrote:
> From: Pradeep Bhat <pradeep.bhat@intel.com>
> 
> This patch defines SKL specific PLANE_WM Watermark registers. It also
> defines macros to get the addresses of different LP levels within a pipe.
> 
> v2: Reworked the register definitions and associated macros to make it more
>     generic and be able to use for_each_pipe in values computation.
>     Incorporated Damien's review comments and indentation.
> 
> v3: Added default values for lines and blocks. Provided mask for blocks.
> 
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bc55990..9fbce2c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4066,6 +4066,43 @@ enum punit_power_well {
>  #define I965_CURSOR_MAX_WM	32
>  #define I965_CURSOR_DFT_WM	8
>  
> +/* Watermark register definitions for SKL */
> +#define CUR_WM_A_0		0x70140
> +#define CUR_WM_B_0		0x71140
> +#define PLANE_WM_1_A_0		0x70240
> +#define PLANE_WM_1_B_0		0x71240
> +#define PLANE_WM_2_A_0		0x70340
> +#define PLANE_WM_2_B_0		0x71340
> +#define PLANE_WM_TRANS_1_A_0	0x70268
> +#define PLANE_WM_TRANS_1_B_0	0x71268
> +#define PLANE_WM_TRANS_2_A_0	0x70368
> +#define PLANE_WM_TRANS_2_B_0	0x71368
> +#define CUR_WM_TRANS_A_0	0x70168
> +#define CUR_WM_TRANS_B_0	0x71168
> +#define   PLANE_WM_EN		(1 << 31)
> +#define   PLANE_WM_LINES_SHIFT	14
> +#define   PLANE_WM_LINES_MASK	0x1f
> +#define   PLANE_WM_BLOCKS_MASK	0x3ff
> +#define   PLANE_WM_LINES_DEFAULT	0x1
> +#define   PLANE_WM_BLOCKS_DEFAULT	0x7
> +

These numbers seemed to make sense when I tried to compare with the
spec.

> +#define CUR_WM_0(pipe) _PIPE(pipe, CUR_WM_A_0, CUR_WM_B_0)
> +#define CUR_WM(pipe, level) (CUR_WM_0(pipe) + ((4) * (level)))
> +#define CUR_WM_TRANS(pipe) _PIPE(pipe, CUR_WM_TRANS_A_0, CUR_WM_TRANS_B_0)
> +
> +#define PLANE_WM_1(pipe) _PIPE(pipe, PLANE_WM_1_A_0, PLANE_WM_1_B_0)
> +#define PLANE_WM_2(pipe) _PIPE(pipe, PLANE_WM_2_A_0, PLANE_WM_2_B_0)
> +#define PLANE_WM_BASE(pipe, plane)	\
> +			_PLANE(plane, PLANE_WM_1(pipe), PLANE_WM_2(pipe))
> +#define PLANE_WM(pipe, plane, level)	\
> +			(PLANE_WM_BASE(pipe, plane) + ((4) * (level)))
> +#define PLANE_WM_TRANS_1(pipe)		\
> +			_PIPE(pipe, PLANE_WM_TRANS_1_A_0, PLANE_WM_TRANS_1_B_0)
> +#define PLANE_WM_TRANS_2(pipe)		\
> +			_PIPE(pipe, PLANE_WM_TRANS_2_A_0, PLANE_WM_TRANS_2_B_0)
> +#define PLANE_WM_TRANS(pipe, plane)	\
> +		_PLANE(plane, PLANE_WM_TRANS_1(pipe), PLANE_WM_TRANS_2(pipe))

I must admit to my eyes glazing over when trying to parse these macros.
Not sure if a bit less redirection might help here. Anyway I plugged them
into a small test program and the resulting register offsets were all good.

Just one small nit: if the intermediate macros aren't supposed to be used
by anywhere outside this file then they might be prefixed with _ to make
it clear they're internal.

Either way:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
>  /* define the Watermark register on Ironlake */
>  #define WM0_PIPEA_ILK		0x45100
>  #define  WM0_PIPE_PLANE_MASK	(0xffff<<16)
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC