Re: [Intel-gfx] [RFC 3/8] drm/i915/ivb: Move WaCxSRDisabledForSpriteScaling w/a to atomic check

2015-10-06 Thread Daniel Vetter 
On Fri, Oct 02, 2015 at 06:03:57PM +0200, Egbert Eich wrote:
> Regarding commit 7809e5ae35b9d8d0710f0874b2e3f10be144e38b
> 
> On Wed, Jul 01, 2015 at 07:25:56PM -0700, Matt Roper wrote:
> > Determine whether we need to apply this workaround at atomic check time
> > and just set a flag that will be used by the main watermark update
> > routine.
> > 
> > Moving this workaround into the atomic framework reduces
> > ilk_update_sprite_wm() to just a standard watermark update, so drop it
> > completely and just ensure that ilk_update_wm() is called whenever a
> > sprite plane is updated in a way that would affect watermarks.
> > 
> > Signed-off-by: Matt Roper 
> 
> 
> This patch causes intel_update_watermarks() to be called much more
> frequently although the watermark values don't change. 
> The change responsible for this is:
> 
> > index 5de1ded..46ef981 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11560,23 +11560,38 @@ retry:
> >  static bool intel_wm_need_update(struct drm_plane *plane,
> >  struct drm_plane_state *state)
> >  {
> > -   /* Update watermarks on tiling changes. */
> > +   struct intel_plane_state *new = to_intel_plane_state(state);
> > +   struct intel_plane_state *cur = to_intel_plane_state(plane->state);
> > +
> > +   /* Update watermarks on tiling or size changes. */
> > if (!plane->state->fb || !state->fb ||
> > plane->state->fb->modifier[0] != state->fb->modifier[0] ||
> > -   plane->state->rotation != state->rotation)
> > -   return true;
> > -
> 
> > -   if (plane->state->crtc_w != state->crtc_w)
> 
> A quick look thru intel_pm.c reveals that this is  relevant for
> WM caluclations for gen=4 and any chipsets for which is_g4x is true.
> Should this really be removed?
> 
> > +   plane->state->rotation != state->rotation ||
> 
> > +   drm_rect_width(>src) != drm_rect_width(>src) ||
> > +   drm_rect_height(>src) != drm_rect_height(>src) ||
> > +   drm_rect_width(>dst) != drm_rect_width(>dst) ||
> > +   drm_rect_height(>dst) != drm_rect_height(>dst))
> 
> these values seem to be used only for watermark calculations on gen < 9 when
> HAS_PCH_SPLIT() is true.
> 
> Still these are responsible for most of the watermark recalculations (when 
> the mouse
> cursor is moved towards the edge for example). On the system I'm looking at 
> the moment
> (Q35) changes in these values don't change the WMs.
> 
> Since WM calculation is very chip generation specific and differs 
> considerably between
> generations, wouldn't it make sense to either have chipset specific functions 
> to determin
> whether a recalculation is needed - or even perfrom this in the update_wm() 
> function
> itself?

The chipset-specific functions to recalc wm values should check for any
real changes and no-op out if none of the registers have changed. The idea
is that wm calculations are really complex and trying to keep both the "do
we need to recalc" and the actual calculations perfectly in sync is a hard
problem. Hence why we recalc aggressively to avoid hard-to-find bugs where
wm values are stale. So overhead should be just a bit of wasted cpu time.
Do you see bad things happening because of all these recalculations?

I guess if it's real trouble we can make a special case for cursors on
pre-gen9 on plane window changes.
-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

Re: [Intel-gfx] [RFC 3/8] drm/i915/ivb: Move WaCxSRDisabledForSpriteScaling w/a to atomic check

2015-10-02 Thread Egbert Eich 
Regarding commit 7809e5ae35b9d8d0710f0874b2e3f10be144e38b

On Wed, Jul 01, 2015 at 07:25:56PM -0700, Matt Roper wrote:
> Determine whether we need to apply this workaround at atomic check time
> and just set a flag that will be used by the main watermark update
> routine.
> 
> Moving this workaround into the atomic framework reduces
> ilk_update_sprite_wm() to just a standard watermark update, so drop it
> completely and just ensure that ilk_update_wm() is called whenever a
> sprite plane is updated in a way that would affect watermarks.
> 
> Signed-off-by: Matt Roper 


This patch causes intel_update_watermarks() to be called much more
frequently although the watermark values don't change. 
The change responsible for this is:

> index 5de1ded..46ef981 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11560,23 +11560,38 @@ retry:
>  static bool intel_wm_need_update(struct drm_plane *plane,
>struct drm_plane_state *state)
>  {
> - /* Update watermarks on tiling changes. */
> + struct intel_plane_state *new = to_intel_plane_state(state);
> + struct intel_plane_state *cur = to_intel_plane_state(plane->state);
> +
> + /* Update watermarks on tiling or size changes. */
>   if (!plane->state->fb || !state->fb ||
>   plane->state->fb->modifier[0] != state->fb->modifier[0] ||
> - plane->state->rotation != state->rotation)
> - return true;
> -

> - if (plane->state->crtc_w != state->crtc_w)

A quick look thru intel_pm.c reveals that this is  relevant for
WM caluclations for gen=4 and any chipsets for which is_g4x is true.
Should this really be removed?

> + plane->state->rotation != state->rotation ||

> + drm_rect_width(>src) != drm_rect_width(>src) ||
> + drm_rect_height(>src) != drm_rect_height(>src) ||
> + drm_rect_width(>dst) != drm_rect_width(>dst) ||
> + drm_rect_height(>dst) != drm_rect_height(>dst))

these values seem to be used only for watermark calculations on gen < 9 when
HAS_PCH_SPLIT() is true.

Still these are responsible for most of the watermark recalculations (when the 
mouse
cursor is moved towards the edge for example). On the system I'm looking at the 
moment
(Q35) changes in these values don't change the WMs.

Since WM calculation is very chip generation specific and differs considerably 
between
generations, wouldn't it make sense to either have chipset specific functions 
to determin
whether a recalculation is needed - or even perfrom this in the update_wm() 
function
itself?

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [RFC 3/8] drm/i915/ivb: Move WaCxSRDisabledForSpriteScaling w/a to atomic check

2015-07-01 Thread Matt Roper 
Determine whether we need to apply this workaround at atomic check time
and just set a flag that will be used by the main watermark update
routine.

Moving this workaround into the atomic framework reduces
ilk_update_sprite_wm() to just a standard watermark update, so drop it
completely and just ensure that ilk_update_wm() is called whenever a
sprite plane is updated in a way that would affect watermarks.

Signed-off-by: Matt Roper matthew.d.ro...@intel.com
---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 40 +---
 drivers/gpu/drm/i915/intel_drv.h |  3 +++
 drivers/gpu/drm/i915/intel_pm.c  | 35 +++
 drivers/gpu/drm/i915/intel_sprite.c  |  8 
 5 files changed, 49 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c 
b/drivers/gpu/drm/i915/intel_atomic.c
index 0aeced8..6673516 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -230,6 +230,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
__drm_atomic_helper_crtc_duplicate_state(crtc, crtc_state-base);
 
crtc_state-base.crtc = crtc;
+   crtc_state-disable_lp_wm = false;
 
return crtc_state-base;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 5de1ded..46ef981 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11560,23 +11560,38 @@ retry:
 static bool intel_wm_need_update(struct drm_plane *plane,
 struct drm_plane_state *state)
 {
-   /* Update watermarks on tiling changes. */
+   struct intel_plane_state *new = to_intel_plane_state(state);
+   struct intel_plane_state *cur = to_intel_plane_state(plane-state);
+
+   /* Update watermarks on tiling or size changes. */
if (!plane-state-fb || !state-fb ||
plane-state-fb-modifier[0] != state-fb-modifier[0] ||
-   plane-state-rotation != state-rotation)
-   return true;
-
-   if (plane-state-crtc_w != state-crtc_w)
+   plane-state-rotation != state-rotation ||
+   drm_rect_width(new-src) != drm_rect_width(cur-src) ||
+   drm_rect_height(new-src) != drm_rect_height(cur-src) ||
+   drm_rect_width(new-dst) != drm_rect_width(cur-dst) ||
+   drm_rect_height(new-dst) != drm_rect_height(cur-dst))
return true;
 
return false;
 }
 
+static bool needs_scaling(struct intel_plane_state *state)
+{
+   int src_w = drm_rect_width(state-src)  16;
+   int src_h = drm_rect_height(state-src)  16;
+   int dst_w = drm_rect_width(state-dst);
+   int dst_h = drm_rect_height(state-dst);
+
+   return (src_w != dst_w || src_h != dst_h);
+}
+
 int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
struct drm_plane_state *plane_state)
 {
struct drm_crtc *crtc = crtc_state-crtc;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+   struct intel_crtc_state *cstate = to_intel_crtc_state(crtc_state);
struct drm_plane *plane = plane_state-plane;
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -11587,7 +11602,6 @@ int intel_plane_atomic_calc_changes(struct 
drm_crtc_state *crtc_state,
bool mode_changed = needs_modeset(crtc_state);
bool was_crtc_enabled = crtc-state-active;
bool is_crtc_enabled = crtc_state-active;
-
bool turn_off, turn_on, visible, was_visible;
struct drm_framebuffer *fb = plane_state-fb;
 
@@ -11705,11 +11719,23 @@ int intel_plane_atomic_calc_changes(struct 
drm_crtc_state *crtc_state,
case DRM_PLANE_TYPE_CURSOR:
break;
case DRM_PLANE_TYPE_OVERLAY:
-   if (turn_off  !mode_changed) {
+   /*
+* WaCxSRDisabledForSpriteScaling:ivb
+*
+* atomic.update_wm was already set above, so this flag will
+* take effect when we commit and program watermarks.
+*/
+   if (IS_IVYBRIDGE(dev) 
+   needs_scaling(to_intel_plane_state(plane_state)) 
+   !needs_scaling(old_plane_state)) {
+   cstate-disable_lp_wm = true;
+   } else if (turn_off  !mode_changed) {
intel_crtc-atomic.wait_vblank = true;
intel_crtc-atomic.update_sprite_watermarks |=
1  i;
}
+
+   break;
}
return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9ffacc0..cdc7d6d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -460,6 +460,9 @@ struct intel_crtc_state {
 
/* w/a for waiting 2 vblanks during crtc