Re: [PATCH 48/62] drm/i915/bdw: Add Broadwell display FIFO limits

Jani Nikula
2013-11-04T09:38:03Z

On Sun, 03 Nov 2013, Ben Widawsky <benjamin.widawsky@intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Broadwell has bigger display FIFOs than Haswell. Otherwise the
> two are very similar.
>
> v2: Fix FBC WM_LP shift for BDW
>
> v3: Rebase on top of the big Haswell wm rework.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> (v2)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++---------
> 2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6f834b3..2a65f92 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3366,6 +3366,7 @@
> #define WM1_LP_LATENCY_MASK (0x7f<<24)
> #define WM1_LP_FBC_MASK (0xf<<20)
> #define WM1_LP_FBC_SHIFT 20
> +#define WM1_LP_FBC_SHIFT_BDW 19
> #define WM1_LP_SR_MASK (0x7ff<<8)

Meh, the above _MASKs would need some love too. WM1_LP_SR_MASK is wrong
for HSW already I think. But nobody's using them, so not a blocker for
this patch.

> #define WM1_LP_SR_SHIFT 8
> #define WM1_LP_CURSOR_MASK (0xff)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 68dc363..6d14182 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2291,7 +2291,9 @@ static uint32_t ilk_compute_fbc_wm(const struct hsw_pipe_wm_parameters *params,
>
> static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
> {
> - if (INTEL_INFO(dev)->gen >= 7)
> + if (INTEL_INFO(dev)->gen >= 8)
> + return 3072;

It's probably just me, but I couldn't find this in the spec, so can't
verify. Apart from that,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

...but read on, some non-blocking bikeshedding below.

> + else if (INTEL_INFO(dev)->gen >= 7)
> return 768;
> else
> return 512;
> @@ -2336,7 +2338,9 @@ static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
> }
>
> /* clamp to max that the registers can hold */
> - if (INTEL_INFO(dev)->gen >= 7)
> + if (INTEL_INFO(dev)->gen >= 8)
> + max = level == 0 ? 255 : 2047;

Not having looked at the WM stuff before, it took me a while to find the
registers and check the maximums. Which made me wonder why we don't fix
the masks and use them here, so it would be bloody obvious what we're
referring to?

if (level)
max = WM1_LP_SR_MASK_BDW >> WM1_LP_SR_SHIFT_BDW;
else
max = WM0_PIPE_PLANE_MASK_BDW >> WM0_PIPE_PLANE_SHIFT;

> + else if (INTEL_INFO(dev)->gen >= 7)
> /* IVB/HSW primary/sprite plane watermarks */
> max = level == 0 ? 127 : 1023;
> else if (!is_sprite)
> @@ -2366,10 +2370,13 @@ static unsigned int ilk_cursor_wm_max(const struct drm_device *dev,
> }
>
> /* Calculate the maximum FBC watermark */
> -static unsigned int ilk_fbc_wm_max(void)
> +static unsigned int ilk_fbc_wm_max(struct drm_device *dev)
> {
> /* max that registers can hold */
> - return 15;
> + if (INTEL_INFO(dev)->gen >= 8)
> + return 31;
> + else
> + return 15;
> }
>
> static void ilk_compute_wm_maximums(struct drm_device *dev,
> @@ -2381,7 +2388,7 @@ static void ilk_compute_wm_maximums(struct drm_device *dev,
> max->pri = ilk_plane_wm_max(dev, level, config, ddb_partitioning, false);
> max->spr = ilk_plane_wm_max(dev, level, config, ddb_partitioning, true);
> max->cur = ilk_cursor_wm_max(dev, level, config);
> - max->fbc = ilk_fbc_wm_max();
> + max->fbc = ilk_fbc_wm_max(dev);
> }
>
> static bool ilk_validate_wm_level(int level,
> @@ -2722,10 +2729,18 @@ static void hsw_compute_wm_results(struct drm_device *dev,
> if (!r->enable)
> break;
>
> - results->wm_lp[wm_lp - 1] = HSW_WM_LP_VAL(level * 2,
> - r->fbc_val,
> - r->pri_val,
> - r->cur_val);

This leaves HSW_WM_LP_VAL() macro unused.

> + results->wm_lp[wm_lp - 1] = WM3_LP_EN |
> + ((level * 2) << WM1_LP_LATENCY_SHIFT) |
> + (r->pri_val << WM1_LP_SR_SHIFT) |
> + r->cur_val;
> +
> + if (INTEL_INFO(dev)->gen >= 8)
> + results->wm_lp[wm_lp - 1] |=
> + r->fbc_val << WM1_LP_FBC_SHIFT_BDW;
> + else
> + results->wm_lp[wm_lp - 1] |=
> + r->fbc_val << WM1_LP_FBC_SHIFT;
> +
> results->wm_lp_spr[wm_lp - 1] = r->spr_val;
> }
>
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org