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

Ville Syrjälä
2014-09-10T18:04:08Z

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