On Mon, Nov 04, 2013 at 11:39:56AM +0200, Jani Nikula wrote:
> On Sun, 03 Nov 2013, Ben Widawsky <benjamin.widawsky@xxxxxxxxx> wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx> (v2)
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > ---
> > 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.
Looks like it's not spelled out there anymore. But you can figure it out
by looking at the single pipe primary:sprite 1:1 split numbers
(1536 * 2 = 3072) or the multi pipe primary only numbers (1024 * 3 = 3072).
> Apart from that,
>
> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
> ...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;
I just extended the masks to cover all platforms. That makes hw
state readout a bit simpler since I don't need to worry about the
differences between generations there. But that means the masks
don't necessarily correspond to any specific platform, and so we
can't use them here. I could define per-platforms masks too, but
that seems rather pointless since there would be but one user.
>
> > + 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.
Yeah. I should just kill it.
>
> > + 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
