Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[Ville Syrjälä]

On Tue, Mar 21, 2017 at 03:42:53PM +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2017-03-02 at 16:58 +0200, Ville Syrjälä wrote:
> > On Sat, Feb 25, 2017 at 04:31:04PM +0100, Maarten Lankhorst wrote:
> > > Op 24-02-17 om 14:11 schreef Ville Syrjälä:
> > > > On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
> > > > > Op 20-02-17 om 14:38 schreef Ville Syrjälä:
> > > > > > On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote:
> > > > > > > Op 17-02-17 om 16:01 schreef ville.syrj...@linux.intel.com:
> > > > > > > > From: Ville Syrjälä 
> > > > > > > > 
> > > > > > > > In order to make cursor updates actually safe wrt. watermark 
> > > > > > > > programming
> > > > > > > > we have to clear the legacy_cursor_update flag in the atomic 
> > > > > > > > state. That
> > > > > > > > will cause the regular atomic update path to do the necessary 
> > > > > > > > vblank
> > > > > > > > wait after the plane update if needed, otherwise the vblank 
> > > > > > > > wait would
> > > > > > > > be skipped and we'd feed the optimal watermarks to the hardware 
> > > > > > > > before
> > > > > > > > the plane update has actually happened.
> > > > > > > > 
> > > > > > > > To make the slow vs. fast path determination in
> > > > > > > > intel_legacy_cursor_update() a little simpler we can ignore the 
> > > > > > > > actual
> > > > > > > > visibility of the plane (which can only get computed once we've 
> > > > > > > > already
> > > > > > > > chosen out path) and instead we simply check whether the fb is 
> > > > > > > > being
> > > > > > > > set or cleared by the user. This means a fully clipped but 
> > > > > > > > logically
> > > > > > > > visible cursor will be considered visible as far as watermark
> > > > > > > > programming is concerned. We can do that for the cursor since 
> > > > > > > > it's a
> > > > > > > > fixed size plane and the clipped size doesn't play a role in the
> > > > > > > > watermark computation.
> > > > > > > > 
> > > > > > > > This should fix underruns that can occur when the cursor gets
> > > > > > > > enable/disabled or the size gets changed. Hopefully it's good 
> > > > > > > > enough
> > > > > > > > that only pure cursor movement and flips go through unthrottled.
> > > > > > > > 
> > > > > > > > Cc: Maarten Lankhorst 
> > > > > > > > Cc: Daniel Vetter 
> > > > > > > > Cc: Uwe Kleine-König 
> > > > > > > > Reported-by: Uwe Kleine-König 
> > > > > > > > Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow 
> > > > > > > > converting legacy page flip to atomic, v3.")
> > > > > > > > Signed-off-by: Ville Syrjälä 
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_display.c | 29 
> > > > > > > > ++---
> > > > > > > >  drivers/gpu/drm/i915/intel_pm.c  | 20 
> > > > > > > >  2 files changed, 30 insertions(+), 19 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > > > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > index b05d9c85384b..356ac04093e8 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct 
> > > > > > > > drm_device *dev,
> > > > > > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > > int ret = 0;
> > > > > > > >  
> > > > > > > > +   /*
> > > > > > > > +* The intel_legacy_cursor_update() fast path takes care
> > > > > > > > +* of avoiding the vblank waits for simple cursor
> > > > > > > > +* movement and flips. For cursor on/off and size 
> > > > > > > > changes,
> > > > > > > > +* we want to perform the vblank waits so that watermark
> > > > > > > > +* updates happen during the correct frames. Gen9+ have
> > > > > > > > +* double buffered watermarks and so shouldn't need 
> > > > > > > > this.
> > > > > > > > +*/
> > > > > > > > +   if (INTEL_GEN(dev_priv) < 9)
> > > > > > > > +   state->legacy_cursor_update = false;
> > > > > > > 
> > > > > > > Could we perhaps add a check in ilk_compute_pipe_wm which unsets 
> > > > > > > the legacy_cursor_update flag so we keep things unsynced as much 
> > > > > > > as possible?
> > > > > > 
> > > > > > I'd have to sprinkle that stuff everywhere but the SKL code
> > > > > > eventually. Seems a little pointless when I can just plop it
> > > > > > there.
> > > > > 
> > > > > Ah indeed. Lets hope it doesn't slow things down too much.
> > > > > > > > ret = drm_atomic_helper_setup_commit(state, nonblock);
> > > > > > > > if (ret)
> > > > > > > > return ret;
> > > > > > > > @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct 
> > > > > > > > 

Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[Ander Conselvan De Oliveira]

On Thu, 2017-03-02 at 16:58 +0200, Ville Syrjälä wrote:
> On Sat, Feb 25, 2017 at 04:31:04PM +0100, Maarten Lankhorst wrote:
> > Op 24-02-17 om 14:11 schreef Ville Syrjälä:
> > > On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
> > > > Op 20-02-17 om 14:38 schreef Ville Syrjälä:
> > > > > On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote:
> > > > > > Op 17-02-17 om 16:01 schreef ville.syrj...@linux.intel.com:
> > > > > > > From: Ville Syrjälä 
> > > > > > > 
> > > > > > > In order to make cursor updates actually safe wrt. watermark 
> > > > > > > programming
> > > > > > > we have to clear the legacy_cursor_update flag in the atomic 
> > > > > > > state. That
> > > > > > > will cause the regular atomic update path to do the necessary 
> > > > > > > vblank
> > > > > > > wait after the plane update if needed, otherwise the vblank wait 
> > > > > > > would
> > > > > > > be skipped and we'd feed the optimal watermarks to the hardware 
> > > > > > > before
> > > > > > > the plane update has actually happened.
> > > > > > > 
> > > > > > > To make the slow vs. fast path determination in
> > > > > > > intel_legacy_cursor_update() a little simpler we can ignore the 
> > > > > > > actual
> > > > > > > visibility of the plane (which can only get computed once we've 
> > > > > > > already
> > > > > > > chosen out path) and instead we simply check whether the fb is 
> > > > > > > being
> > > > > > > set or cleared by the user. This means a fully clipped but 
> > > > > > > logically
> > > > > > > visible cursor will be considered visible as far as watermark
> > > > > > > programming is concerned. We can do that for the cursor since 
> > > > > > > it's a
> > > > > > > fixed size plane and the clipped size doesn't play a role in the
> > > > > > > watermark computation.
> > > > > > > 
> > > > > > > This should fix underruns that can occur when the cursor gets
> > > > > > > enable/disabled or the size gets changed. Hopefully it's good 
> > > > > > > enough
> > > > > > > that only pure cursor movement and flips go through unthrottled.
> > > > > > > 
> > > > > > > Cc: Maarten Lankhorst 
> > > > > > > Cc: Daniel Vetter 
> > > > > > > Cc: Uwe Kleine-König 
> > > > > > > Reported-by: Uwe Kleine-König 
> > > > > > > Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow 
> > > > > > > converting legacy page flip to atomic, v3.")
> > > > > > > Signed-off-by: Ville Syrjälä 
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_display.c | 29 
> > > > > > > ++---
> > > > > > >  drivers/gpu/drm/i915/intel_pm.c  | 20 
> > > > > > >  2 files changed, 30 insertions(+), 19 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > index b05d9c85384b..356ac04093e8 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct 
> > > > > > > drm_device *dev,
> > > > > > >   struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > >   int ret = 0;
> > > > > > >  
> > > > > > > + /*
> > > > > > > +  * The intel_legacy_cursor_update() fast path takes care
> > > > > > > +  * of avoiding the vblank waits for simple cursor
> > > > > > > +  * movement and flips. For cursor on/off and size changes,
> > > > > > > +  * we want to perform the vblank waits so that watermark
> > > > > > > +  * updates happen during the correct frames. Gen9+ have
> > > > > > > +  * double buffered watermarks and so shouldn't need this.
> > > > > > > +  */
> > > > > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > > > > + state->legacy_cursor_update = false;
> > > > > > 
> > > > > > Could we perhaps add a check in ilk_compute_pipe_wm which unsets 
> > > > > > the legacy_cursor_update flag so we keep things unsynced as much as 
> > > > > > possible?
> > > > > 
> > > > > I'd have to sprinkle that stuff everywhere but the SKL code
> > > > > eventually. Seems a little pointless when I can just plop it
> > > > > there.
> > > > 
> > > > Ah indeed. Lets hope it doesn't slow things down too much.
> > > > > > >   ret = drm_atomic_helper_setup_commit(state, nonblock);
> > > > > > >   if (ret)
> > > > > > >   return ret;
> > > > > > > @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct 
> > > > > > > drm_plane *plane,
> > > > > > >   old_plane_state->src_h != src_h ||
> > > > > > >   old_plane_state->crtc_w != crtc_w ||
> > > > > > >   old_plane_state->crtc_h != crtc_h ||
> > > > > > > - !old_plane_state->visible ||
> > > > > > > - old_plane_state->fb->modifier != fb->modifier)
> > > > > > > + !old_plane_state->fb != !fb)
> > > > > > >   goto 

Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[Ville Syrjälä]

On Sat, Feb 25, 2017 at 04:31:04PM +0100, Maarten Lankhorst wrote:
> Op 24-02-17 om 14:11 schreef Ville Syrjälä:
> > On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
> >> Op 20-02-17 om 14:38 schreef Ville Syrjälä:
> >>> On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote:
>  Op 17-02-17 om 16:01 schreef ville.syrj...@linux.intel.com:
> > From: Ville Syrjälä 
> >
> > In order to make cursor updates actually safe wrt. watermark programming
> > we have to clear the legacy_cursor_update flag in the atomic state. That
> > will cause the regular atomic update path to do the necessary vblank
> > wait after the plane update if needed, otherwise the vblank wait would
> > be skipped and we'd feed the optimal watermarks to the hardware before
> > the plane update has actually happened.
> >
> > To make the slow vs. fast path determination in
> > intel_legacy_cursor_update() a little simpler we can ignore the actual
> > visibility of the plane (which can only get computed once we've already
> > chosen out path) and instead we simply check whether the fb is being
> > set or cleared by the user. This means a fully clipped but logically
> > visible cursor will be considered visible as far as watermark
> > programming is concerned. We can do that for the cursor since it's a
> > fixed size plane and the clipped size doesn't play a role in the
> > watermark computation.
> >
> > This should fix underruns that can occur when the cursor gets
> > enable/disabled or the size gets changed. Hopefully it's good enough
> > that only pure cursor movement and flips go through unthrottled.
> >
> > Cc: Maarten Lankhorst 
> > Cc: Daniel Vetter 
> > Cc: Uwe Kleine-König 
> > Reported-by: Uwe Kleine-König 
> > Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting 
> > legacy page flip to atomic, v3.")
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 29 ++---
> >  drivers/gpu/drm/i915/intel_pm.c  | 20 
> >  2 files changed, 30 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index b05d9c85384b..356ac04093e8 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct 
> > drm_device *dev,
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > int ret = 0;
> >  
> > +   /*
> > +* The intel_legacy_cursor_update() fast path takes care
> > +* of avoiding the vblank waits for simple cursor
> > +* movement and flips. For cursor on/off and size changes,
> > +* we want to perform the vblank waits so that watermark
> > +* updates happen during the correct frames. Gen9+ have
> > +* double buffered watermarks and so shouldn't need this.
> > +*/
> > +   if (INTEL_GEN(dev_priv) < 9)
> > +   state->legacy_cursor_update = false;
>  Could we perhaps add a check in ilk_compute_pipe_wm which unsets the 
>  legacy_cursor_update flag so we keep things unsynced as much as possible?
> >>> I'd have to sprinkle that stuff everywhere but the SKL code
> >>> eventually. Seems a little pointless when I can just plop it
> >>> there.
> >> Ah indeed. Lets hope it doesn't slow things down too much.
> > ret = drm_atomic_helper_setup_commit(state, nonblock);
> > if (ret)
> > return ret;
> > @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane 
> > *plane,
> > old_plane_state->src_h != src_h ||
> > old_plane_state->crtc_w != crtc_w ||
> > old_plane_state->crtc_h != crtc_h ||
> > -   !old_plane_state->visible ||
> > -   old_plane_state->fb->modifier != fb->modifier)
> > +   !old_plane_state->fb != !fb)
> > goto slow;
> >  
> > new_plane_state = intel_plane_duplicate_state(plane);
> > @@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane 
> > *plane,
> > if (ret)
> > goto out_free;
> >  
> > -   /* Visibility changed, must take slowpath. */
> > -   if (!new_plane_state->visible)
> > -   goto slow_free;
> > -
> > ret = mutex_lock_interruptible(_priv->drm.struct_mutex);
> > if (ret)
> > goto out_free;
>  Those 2 hunks are needed. If you move the 

Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[Maarten Lankhorst]

Op 24-02-17 om 14:11 schreef Ville Syrjälä:
> On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
>> Op 20-02-17 om 14:38 schreef Ville Syrjälä:
>>> On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote:
 Op 17-02-17 om 16:01 schreef ville.syrj...@linux.intel.com:
> From: Ville Syrjälä 
>
> In order to make cursor updates actually safe wrt. watermark programming
> we have to clear the legacy_cursor_update flag in the atomic state. That
> will cause the regular atomic update path to do the necessary vblank
> wait after the plane update if needed, otherwise the vblank wait would
> be skipped and we'd feed the optimal watermarks to the hardware before
> the plane update has actually happened.
>
> To make the slow vs. fast path determination in
> intel_legacy_cursor_update() a little simpler we can ignore the actual
> visibility of the plane (which can only get computed once we've already
> chosen out path) and instead we simply check whether the fb is being
> set or cleared by the user. This means a fully clipped but logically
> visible cursor will be considered visible as far as watermark
> programming is concerned. We can do that for the cursor since it's a
> fixed size plane and the clipped size doesn't play a role in the
> watermark computation.
>
> This should fix underruns that can occur when the cursor gets
> enable/disabled or the size gets changed. Hopefully it's good enough
> that only pure cursor movement and flips go through unthrottled.
>
> Cc: Maarten Lankhorst 
> Cc: Daniel Vetter 
> Cc: Uwe Kleine-König 
> Reported-by: Uwe Kleine-König 
> Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting 
> legacy page flip to atomic, v3.")
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 29 ++---
>  drivers/gpu/drm/i915/intel_pm.c  | 20 
>  2 files changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b05d9c85384b..356ac04093e8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct drm_device 
> *dev,
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   int ret = 0;
>  
> + /*
> +  * The intel_legacy_cursor_update() fast path takes care
> +  * of avoiding the vblank waits for simple cursor
> +  * movement and flips. For cursor on/off and size changes,
> +  * we want to perform the vblank waits so that watermark
> +  * updates happen during the correct frames. Gen9+ have
> +  * double buffered watermarks and so shouldn't need this.
> +  */
> + if (INTEL_GEN(dev_priv) < 9)
> + state->legacy_cursor_update = false;
 Could we perhaps add a check in ilk_compute_pipe_wm which unsets the 
 legacy_cursor_update flag so we keep things unsynced as much as possible?
>>> I'd have to sprinkle that stuff everywhere but the SKL code
>>> eventually. Seems a little pointless when I can just plop it
>>> there.
>> Ah indeed. Lets hope it doesn't slow things down too much.
>   ret = drm_atomic_helper_setup_commit(state, nonblock);
>   if (ret)
>   return ret;
> @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane 
> *plane,
>   old_plane_state->src_h != src_h ||
>   old_plane_state->crtc_w != crtc_w ||
>   old_plane_state->crtc_h != crtc_h ||
> - !old_plane_state->visible ||
> - old_plane_state->fb->modifier != fb->modifier)
> + !old_plane_state->fb != !fb)
>   goto slow;
>  
>   new_plane_state = intel_plane_duplicate_state(plane);
> @@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane 
> *plane,
>   if (ret)
>   goto out_free;
>  
> - /* Visibility changed, must take slowpath. */
> - if (!new_plane_state->visible)
> - goto slow_free;
> -
>   ret = mutex_lock_interruptible(_priv->drm.struct_mutex);
>   if (ret)
>   goto out_free;
 Those 2 hunks are needed. If you move the cursor out of the visible area 
 or back you need to update.
>>> No. I changed the wm code to consider a non-visible but logicall active
>>> cursor as needing proper watermarks. That avoids needing this fallback
>>> path here.
>> Ah indeed. But one thing you dropped is the fb modifier check.
>> I suppose there's no harm with no support for using prite planes as cursor 
>> plane yet, but might be nice to keep it in.
> We'd have 

Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[Ville Syrjälä]

On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote:
> Op 20-02-17 om 14:38 schreef Ville Syrjälä:
> > On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote:
> >> Op 17-02-17 om 16:01 schreef ville.syrj...@linux.intel.com:
> >>> From: Ville Syrjälä 
> >>>
> >>> In order to make cursor updates actually safe wrt. watermark programming
> >>> we have to clear the legacy_cursor_update flag in the atomic state. That
> >>> will cause the regular atomic update path to do the necessary vblank
> >>> wait after the plane update if needed, otherwise the vblank wait would
> >>> be skipped and we'd feed the optimal watermarks to the hardware before
> >>> the plane update has actually happened.
> >>>
> >>> To make the slow vs. fast path determination in
> >>> intel_legacy_cursor_update() a little simpler we can ignore the actual
> >>> visibility of the plane (which can only get computed once we've already
> >>> chosen out path) and instead we simply check whether the fb is being
> >>> set or cleared by the user. This means a fully clipped but logically
> >>> visible cursor will be considered visible as far as watermark
> >>> programming is concerned. We can do that for the cursor since it's a
> >>> fixed size plane and the clipped size doesn't play a role in the
> >>> watermark computation.
> >>>
> >>> This should fix underruns that can occur when the cursor gets
> >>> enable/disabled or the size gets changed. Hopefully it's good enough
> >>> that only pure cursor movement and flips go through unthrottled.
> >>>
> >>> Cc: Maarten Lankhorst 
> >>> Cc: Daniel Vetter 
> >>> Cc: Uwe Kleine-König 
> >>> Reported-by: Uwe Kleine-König 
> >>> Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting 
> >>> legacy page flip to atomic, v3.")
> >>> Signed-off-by: Ville Syrjälä 
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_display.c | 29 ++---
> >>>  drivers/gpu/drm/i915/intel_pm.c  | 20 
> >>>  2 files changed, 30 insertions(+), 19 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> >>> b/drivers/gpu/drm/i915/intel_display.c
> >>> index b05d9c85384b..356ac04093e8 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct drm_device 
> >>> *dev,
> >>>   struct drm_i915_private *dev_priv = to_i915(dev);
> >>>   int ret = 0;
> >>>  
> >>> + /*
> >>> +  * The intel_legacy_cursor_update() fast path takes care
> >>> +  * of avoiding the vblank waits for simple cursor
> >>> +  * movement and flips. For cursor on/off and size changes,
> >>> +  * we want to perform the vblank waits so that watermark
> >>> +  * updates happen during the correct frames. Gen9+ have
> >>> +  * double buffered watermarks and so shouldn't need this.
> >>> +  */
> >>> + if (INTEL_GEN(dev_priv) < 9)
> >>> + state->legacy_cursor_update = false;
> >> Could we perhaps add a check in ilk_compute_pipe_wm which unsets the 
> >> legacy_cursor_update flag so we keep things unsynced as much as possible?
> > I'd have to sprinkle that stuff everywhere but the SKL code
> > eventually. Seems a little pointless when I can just plop it
> > there.
> Ah indeed. Lets hope it doesn't slow things down too much.
> >>>   ret = drm_atomic_helper_setup_commit(state, nonblock);
> >>>   if (ret)
> >>>   return ret;
> >>> @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane 
> >>> *plane,
> >>>   old_plane_state->src_h != src_h ||
> >>>   old_plane_state->crtc_w != crtc_w ||
> >>>   old_plane_state->crtc_h != crtc_h ||
> >>> - !old_plane_state->visible ||
> >>> - old_plane_state->fb->modifier != fb->modifier)
> >>> + !old_plane_state->fb != !fb)
> >>>   goto slow;
> >>>  
> >>>   new_plane_state = intel_plane_duplicate_state(plane);
> >>> @@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane 
> >>> *plane,
> >>>   if (ret)
> >>>   goto out_free;
> >>>  
> >>> - /* Visibility changed, must take slowpath. */
> >>> - if (!new_plane_state->visible)
> >>> - goto slow_free;
> >>> -
> >>>   ret = mutex_lock_interruptible(_priv->drm.struct_mutex);
> >>>   if (ret)
> >>>   goto out_free;
> >> Those 2 hunks are needed. If you move the cursor out of the visible area 
> >> or back you need to update.
> > No. I changed the wm code to consider a non-visible but logicall active
> > cursor as needing proper watermarks. That avoids needing this fallback
> > path here.
> Ah indeed. But one thing you dropped is the fb modifier check.
> I suppose there's no harm with no support for using prite planes as cursor 
> plane yet, but might be nice to keep it in.

We'd have bigger problems than the modifier if we want to use a 

Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[Maarten Lankhorst]

Op 20-02-17 om 14:38 schreef Ville Syrjälä:
> On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote:
>> Op 17-02-17 om 16:01 schreef ville.syrj...@linux.intel.com:
>>> From: Ville Syrjälä 
>>>
>>> In order to make cursor updates actually safe wrt. watermark programming
>>> we have to clear the legacy_cursor_update flag in the atomic state. That
>>> will cause the regular atomic update path to do the necessary vblank
>>> wait after the plane update if needed, otherwise the vblank wait would
>>> be skipped and we'd feed the optimal watermarks to the hardware before
>>> the plane update has actually happened.
>>>
>>> To make the slow vs. fast path determination in
>>> intel_legacy_cursor_update() a little simpler we can ignore the actual
>>> visibility of the plane (which can only get computed once we've already
>>> chosen out path) and instead we simply check whether the fb is being
>>> set or cleared by the user. This means a fully clipped but logically
>>> visible cursor will be considered visible as far as watermark
>>> programming is concerned. We can do that for the cursor since it's a
>>> fixed size plane and the clipped size doesn't play a role in the
>>> watermark computation.
>>>
>>> This should fix underruns that can occur when the cursor gets
>>> enable/disabled or the size gets changed. Hopefully it's good enough
>>> that only pure cursor movement and flips go through unthrottled.
>>>
>>> Cc: Maarten Lankhorst 
>>> Cc: Daniel Vetter 
>>> Cc: Uwe Kleine-König 
>>> Reported-by: Uwe Kleine-König 
>>> Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting 
>>> legacy page flip to atomic, v3.")
>>> Signed-off-by: Ville Syrjälä 
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 29 ++---
>>>  drivers/gpu/drm/i915/intel_pm.c  | 20 
>>>  2 files changed, 30 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index b05d9c85384b..356ac04093e8 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct drm_device 
>>> *dev,
>>> struct drm_i915_private *dev_priv = to_i915(dev);
>>> int ret = 0;
>>>  
>>> +   /*
>>> +* The intel_legacy_cursor_update() fast path takes care
>>> +* of avoiding the vblank waits for simple cursor
>>> +* movement and flips. For cursor on/off and size changes,
>>> +* we want to perform the vblank waits so that watermark
>>> +* updates happen during the correct frames. Gen9+ have
>>> +* double buffered watermarks and so shouldn't need this.
>>> +*/
>>> +   if (INTEL_GEN(dev_priv) < 9)
>>> +   state->legacy_cursor_update = false;
>> Could we perhaps add a check in ilk_compute_pipe_wm which unsets the 
>> legacy_cursor_update flag so we keep things unsynced as much as possible?
> I'd have to sprinkle that stuff everywhere but the SKL code
> eventually. Seems a little pointless when I can just plop it
> there.
Ah indeed. Lets hope it doesn't slow things down too much.
>>> ret = drm_atomic_helper_setup_commit(state, nonblock);
>>> if (ret)
>>> return ret;
>>> @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>> old_plane_state->src_h != src_h ||
>>> old_plane_state->crtc_w != crtc_w ||
>>> old_plane_state->crtc_h != crtc_h ||
>>> -   !old_plane_state->visible ||
>>> -   old_plane_state->fb->modifier != fb->modifier)
>>> +   !old_plane_state->fb != !fb)
>>> goto slow;
>>>  
>>> new_plane_state = intel_plane_duplicate_state(plane);
>>> @@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>> if (ret)
>>> goto out_free;
>>>  
>>> -   /* Visibility changed, must take slowpath. */
>>> -   if (!new_plane_state->visible)
>>> -   goto slow_free;
>>> -
>>> ret = mutex_lock_interruptible(_priv->drm.struct_mutex);
>>> if (ret)
>>> goto out_free;
>> Those 2 hunks are needed. If you move the cursor out of the visible area or 
>> back you need to update.
> No. I changed the wm code to consider a non-visible but logicall active
> cursor as needing proper watermarks. That avoids needing this fallback
> path here.
Ah indeed. But one thing you dropped is the fb modifier check.
I suppose there's no harm with no support for using prite planes as cursor 
plane yet, but might be nice to keep it in.

Cc'ing Ristovski for testing the patch. :)
>
>> Easiest way to trigger a bug is load a 256 width cursor out of the visible 
>> area, move it back in and you get
>> a fifo underrun.
>>
>> Why is switching fb's synced?
> It is not.
>
>> Identical sized fb should be unsynced if 

Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[Ville Syrjälä]

On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote:
> Op 17-02-17 om 16:01 schreef ville.syrj...@linux.intel.com:
> > From: Ville Syrjälä 
> >
> > In order to make cursor updates actually safe wrt. watermark programming
> > we have to clear the legacy_cursor_update flag in the atomic state. That
> > will cause the regular atomic update path to do the necessary vblank
> > wait after the plane update if needed, otherwise the vblank wait would
> > be skipped and we'd feed the optimal watermarks to the hardware before
> > the plane update has actually happened.
> >
> > To make the slow vs. fast path determination in
> > intel_legacy_cursor_update() a little simpler we can ignore the actual
> > visibility of the plane (which can only get computed once we've already
> > chosen out path) and instead we simply check whether the fb is being
> > set or cleared by the user. This means a fully clipped but logically
> > visible cursor will be considered visible as far as watermark
> > programming is concerned. We can do that for the cursor since it's a
> > fixed size plane and the clipped size doesn't play a role in the
> > watermark computation.
> >
> > This should fix underruns that can occur when the cursor gets
> > enable/disabled or the size gets changed. Hopefully it's good enough
> > that only pure cursor movement and flips go through unthrottled.
> >
> > Cc: Maarten Lankhorst 
> > Cc: Daniel Vetter 
> > Cc: Uwe Kleine-König 
> > Reported-by: Uwe Kleine-König 
> > Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting 
> > legacy page flip to atomic, v3.")
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 29 ++---
> >  drivers/gpu/drm/i915/intel_pm.c  | 20 
> >  2 files changed, 30 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index b05d9c85384b..356ac04093e8 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct drm_device 
> > *dev,
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > int ret = 0;
> >  
> > +   /*
> > +* The intel_legacy_cursor_update() fast path takes care
> > +* of avoiding the vblank waits for simple cursor
> > +* movement and flips. For cursor on/off and size changes,
> > +* we want to perform the vblank waits so that watermark
> > +* updates happen during the correct frames. Gen9+ have
> > +* double buffered watermarks and so shouldn't need this.
> > +*/
> > +   if (INTEL_GEN(dev_priv) < 9)
> > +   state->legacy_cursor_update = false;
> Could we perhaps add a check in ilk_compute_pipe_wm which unsets the 
> legacy_cursor_update flag so we keep things unsynced as much as possible?

I'd have to sprinkle that stuff everywhere but the SKL code
eventually. Seems a little pointless when I can just plop it
there.

> > ret = drm_atomic_helper_setup_commit(state, nonblock);
> > if (ret)
> > return ret;
> > @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > old_plane_state->src_h != src_h ||
> > old_plane_state->crtc_w != crtc_w ||
> > old_plane_state->crtc_h != crtc_h ||
> > -   !old_plane_state->visible ||
> > -   old_plane_state->fb->modifier != fb->modifier)
> > +   !old_plane_state->fb != !fb)
> > goto slow;
> >  
> > new_plane_state = intel_plane_duplicate_state(plane);
> > @@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > if (ret)
> > goto out_free;
> >  
> > -   /* Visibility changed, must take slowpath. */
> > -   if (!new_plane_state->visible)
> > -   goto slow_free;
> > -
> > ret = mutex_lock_interruptible(_priv->drm.struct_mutex);
> > if (ret)
> > goto out_free;
> Those 2 hunks are needed. If you move the cursor out of the visible area or 
> back you need to update.

No. I changed the wm code to consider a non-visible but logicall active
cursor as needing proper watermarks. That avoids needing this fallback
path here.

> Easiest way to trigger a bug is load a 256 width cursor out of the visible 
> area, move it back in and you get
> a fifo underrun.
> 
> Why is switching fb's synced?

It is not.

> Identical sized fb should be unsynced if possible.
>
> > @@ -13522,9 +13528,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > new_plane_state->fb = old_fb;
> > to_intel_plane_state(new_plane_state)->vma = old_vma;
> >  
> > -   intel_plane->update_plane(plane,
> > - to_intel_crtc_state(crtc->state),
> > - 

Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[Maarten Lankhorst]

Op 17-02-17 om 16:01 schreef ville.syrj...@linux.intel.com:
> From: Ville Syrjälä 
>
> In order to make cursor updates actually safe wrt. watermark programming
> we have to clear the legacy_cursor_update flag in the atomic state. That
> will cause the regular atomic update path to do the necessary vblank
> wait after the plane update if needed, otherwise the vblank wait would
> be skipped and we'd feed the optimal watermarks to the hardware before
> the plane update has actually happened.
>
> To make the slow vs. fast path determination in
> intel_legacy_cursor_update() a little simpler we can ignore the actual
> visibility of the plane (which can only get computed once we've already
> chosen out path) and instead we simply check whether the fb is being
> set or cleared by the user. This means a fully clipped but logically
> visible cursor will be considered visible as far as watermark
> programming is concerned. We can do that for the cursor since it's a
> fixed size plane and the clipped size doesn't play a role in the
> watermark computation.
>
> This should fix underruns that can occur when the cursor gets
> enable/disabled or the size gets changed. Hopefully it's good enough
> that only pure cursor movement and flips go through unthrottled.
>
> Cc: Maarten Lankhorst 
> Cc: Daniel Vetter 
> Cc: Uwe Kleine-König 
> Reported-by: Uwe Kleine-König 
> Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting legacy 
> page flip to atomic, v3.")
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 29 ++---
>  drivers/gpu/drm/i915/intel_pm.c  | 20 
>  2 files changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b05d9c85384b..356ac04093e8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct drm_device 
> *dev,
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   int ret = 0;
>  
> + /*
> +  * The intel_legacy_cursor_update() fast path takes care
> +  * of avoiding the vblank waits for simple cursor
> +  * movement and flips. For cursor on/off and size changes,
> +  * we want to perform the vblank waits so that watermark
> +  * updates happen during the correct frames. Gen9+ have
> +  * double buffered watermarks and so shouldn't need this.
> +  */
> + if (INTEL_GEN(dev_priv) < 9)
> + state->legacy_cursor_update = false;
Could we perhaps add a check in ilk_compute_pipe_wm which unsets the 
legacy_cursor_update flag so we keep things unsynced as much as possible?
>   ret = drm_atomic_helper_setup_commit(state, nonblock);
>   if (ret)
>   return ret;
> @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>   old_plane_state->src_h != src_h ||
>   old_plane_state->crtc_w != crtc_w ||
>   old_plane_state->crtc_h != crtc_h ||
> - !old_plane_state->visible ||
> - old_plane_state->fb->modifier != fb->modifier)
> + !old_plane_state->fb != !fb)
>   goto slow;
>  
>   new_plane_state = intel_plane_duplicate_state(plane);
> @@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>   if (ret)
>   goto out_free;
>  
> - /* Visibility changed, must take slowpath. */
> - if (!new_plane_state->visible)
> - goto slow_free;
> -
>   ret = mutex_lock_interruptible(_priv->drm.struct_mutex);
>   if (ret)
>   goto out_free;
Those 2 hunks are needed. If you move the cursor out of the visible area or 
back you need to update.
Easiest way to trigger a bug is load a 256 width cursor out of the visible 
area, move it back in and you get
a fifo underrun.

Why is switching fb's synced? Identical sized fb should be unsynced if possible.
> @@ -13522,9 +13528,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>   new_plane_state->fb = old_fb;
>   to_intel_plane_state(new_plane_state)->vma = old_vma;
>  
> - intel_plane->update_plane(plane,
> -   to_intel_crtc_state(crtc->state),
> -   to_intel_plane_state(plane->state));
> + if (plane->state->visible)
> + intel_plane->update_plane(plane,
> +   to_intel_crtc_state(crtc->state),
> +   to_intel_plane_state(plane->state));
> + else
> + intel_plane->disable_plane(plane, crtc);
>  
>   intel_cleanup_plane_fb(plane, new_plane_state);
>  
> @@ -13534,8 +13543,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>   

Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 09:04:44PM +0100, Uwe Kleine-König wrote:
> Hello Ville,
> 
> On Fri, Feb 17, 2017 at 05:01:59PM +0200, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > In order to make cursor updates actually safe wrt. watermark programming
> > we have to clear the legacy_cursor_update flag in the atomic state. That
> > will cause the regular atomic update path to do the necessary vblank
> > wait after the plane update if needed, otherwise the vblank wait would
> > be skipped and we'd feed the optimal watermarks to the hardware before
> > the plane update has actually happened.
> > 
> > [...]
> > 
> > Cc: Maarten Lankhorst 
> > Cc: Daniel Vetter 
> > Cc: Uwe Kleine-König 
> > Reported-by: Uwe Kleine-König 
> > Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting 
> > legacy page flip to atomic, v3.")
> > Signed-off-by: Ville Syrjälä 
> 
> Is this supposed to fix
> https://bugs.freedesktop.org/show_bug.cgi?id=98742 ? If so, the Fixes:
> line seems wrong because f79f26921ee1 isn't in 4.9 where I see the
> issue.
> 
> If I want to fix 4.9---my ultimate goal is to fix the kernel that will
> go into the next stable release---I have to cherry-pick f79f26921ee1
> first, I assume?

Argh. The fact is that it has actually been broken since forever,
but I suppose *something* must have been masking the problem for
you on earlier kernels, well, assuming you didn't actually hit the
problem before 4.9.

You can give backporting the custom legacy cursor path a try,
but I'm not sure how badly it depends on other things. So not
sure a backport is entirely trivial. Maarten, any thoughts?

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[Uwe Kleine-König]

Hello Ville,

On Fri, Feb 17, 2017 at 05:01:59PM +0200, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> In order to make cursor updates actually safe wrt. watermark programming
> we have to clear the legacy_cursor_update flag in the atomic state. That
> will cause the regular atomic update path to do the necessary vblank
> wait after the plane update if needed, otherwise the vblank wait would
> be skipped and we'd feed the optimal watermarks to the hardware before
> the plane update has actually happened.
> 
> [...]
> 
> Cc: Maarten Lankhorst 
> Cc: Daniel Vetter 
> Cc: Uwe Kleine-König 
> Reported-by: Uwe Kleine-König 
> Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting legacy 
> page flip to atomic, v3.")
> Signed-off-by: Ville Syrjälä 

Is this supposed to fix
https://bugs.freedesktop.org/show_bug.cgi?id=98742 ? If so, the Fixes:
line seems wrong because f79f26921ee1 isn't in 4.9 where I see the
issue.

If I want to fix 4.9---my ultimate goal is to fix the kernel that will
go into the next stable release---I have to cherry-pick f79f26921ee1
first, I assume?

Best regards
Uwe


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW

[ville . syrjala]

From: Ville Syrjälä 

In order to make cursor updates actually safe wrt. watermark programming
we have to clear the legacy_cursor_update flag in the atomic state. That
will cause the regular atomic update path to do the necessary vblank
wait after the plane update if needed, otherwise the vblank wait would
be skipped and we'd feed the optimal watermarks to the hardware before
the plane update has actually happened.

To make the slow vs. fast path determination in
intel_legacy_cursor_update() a little simpler we can ignore the actual
visibility of the plane (which can only get computed once we've already
chosen out path) and instead we simply check whether the fb is being
set or cleared by the user. This means a fully clipped but logically
visible cursor will be considered visible as far as watermark
programming is concerned. We can do that for the cursor since it's a
fixed size plane and the clipped size doesn't play a role in the
watermark computation.

This should fix underruns that can occur when the cursor gets
enable/disabled or the size gets changed. Hopefully it's good enough
that only pure cursor movement and flips go through unthrottled.

Cc: Maarten Lankhorst 
Cc: Daniel Vetter 
Cc: Uwe Kleine-König 
Reported-by: Uwe Kleine-König 
Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting legacy 
page flip to atomic, v3.")
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_display.c | 29 ++---
 drivers/gpu/drm/i915/intel_pm.c  | 20 
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index b05d9c85384b..356ac04093e8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct drm_device *dev,
struct drm_i915_private *dev_priv = to_i915(dev);
int ret = 0;
 
+   /*
+* The intel_legacy_cursor_update() fast path takes care
+* of avoiding the vblank waits for simple cursor
+* movement and flips. For cursor on/off and size changes,
+* we want to perform the vblank waits so that watermark
+* updates happen during the correct frames. Gen9+ have
+* double buffered watermarks and so shouldn't need this.
+*/
+   if (INTEL_GEN(dev_priv) < 9)
+   state->legacy_cursor_update = false;
+
ret = drm_atomic_helper_setup_commit(state, nonblock);
if (ret)
return ret;
@@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
old_plane_state->src_h != src_h ||
old_plane_state->crtc_w != crtc_w ||
old_plane_state->crtc_h != crtc_h ||
-   !old_plane_state->visible ||
-   old_plane_state->fb->modifier != fb->modifier)
+   !old_plane_state->fb != !fb)
goto slow;
 
new_plane_state = intel_plane_duplicate_state(plane);
@@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
if (ret)
goto out_free;
 
-   /* Visibility changed, must take slowpath. */
-   if (!new_plane_state->visible)
-   goto slow_free;
-
ret = mutex_lock_interruptible(_priv->drm.struct_mutex);
if (ret)
goto out_free;
@@ -13522,9 +13528,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
new_plane_state->fb = old_fb;
to_intel_plane_state(new_plane_state)->vma = old_vma;
 
-   intel_plane->update_plane(plane,
- to_intel_crtc_state(crtc->state),
- to_intel_plane_state(plane->state));
+   if (plane->state->visible)
+   intel_plane->update_plane(plane,
+ to_intel_crtc_state(crtc->state),
+ to_intel_plane_state(plane->state));
+   else
+   intel_plane->disable_plane(plane, crtc);
 
intel_cleanup_plane_fb(plane, new_plane_state);
 
@@ -13534,8 +13543,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
intel_plane_destroy_state(plane, new_plane_state);
return ret;
 
-slow_free:
-   intel_plane_destroy_state(plane, new_plane_state);
 slow:
return drm_atomic_helper_update_plane(plane, crtc, fb,
  crtc_x, crtc_y, crtc_w, crtc_h,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fe243c65de1a..4de8c40acc7e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1831,20 +1831,24 @@ static uint32_t ilk_compute_cur_wm(const struct 
intel_crtc_state *cstate,