Re: [Intel-gfx] [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2.

[Maarten Lankhorst]

Op 27-10-16 om 01:24 schreef Matt Roper:
> On Wed, Oct 26, 2016 at 03:41:37PM +0200, Maarten Lankhorst wrote:
>> The watermark updates for SKL style watermarks are no longer done
>> in the plane callbacks, but are now called in a separate watermark
>> update function that's called during the same vblank evasion,
>> before the plane updates.
>>
>> This also gets rid of the global skl_results, which was required for
>> keeping track of the current atomic commit.
>>
>> Changes since v1:
>> - Move line unwrap to correct patch. (Lyude)
>> - Make sure we don't regress ILK watermarks. (Matt)
>> - Rephrase commit message. (Matt)
>>
> ...
>> @@ -14459,8 +14436,17 @@ static void intel_atomic_commit_tail(struct 
>> drm_atomic_state *state)
>>  intel_check_cpu_fifo_underruns(dev_priv);
>>  intel_check_pch_fifo_underruns(dev_priv);
>>  
>> -if (!crtc->state->active)
>> -intel_update_watermarks(crtc);
>> +if (!crtc->state->active) {
>> +/*
>> + * Make sure we don't call initial_watermarks
>> + * for ILK-style watermark updates.
>> + */
>> +if (HAS_DDI(dev_priv) && 
>> dev_priv->display.initial_watermarks)
> Aren't HSW/BDW DDI platforms?  They still use the ILK-style watermarks,
> so I don't think this is protecting all the platforms it needs to.
>
> Even if that weren't the case, I wouldn't be wild about using HAS_DDI
> here since whether or not a platform has DDI isn't really the reason
> we're programming watermarks differently so the code is a bit confusing
> to the casual reader.
Oh right, completely forgot about that.

Maybe change the check to if dev_priv->atomic_update_watermarks ?

~Maarten
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2.

[Matt Roper]

On Wed, Oct 26, 2016 at 03:41:37PM +0200, Maarten Lankhorst wrote:
> The watermark updates for SKL style watermarks are no longer done
> in the plane callbacks, but are now called in a separate watermark
> update function that's called during the same vblank evasion,
> before the plane updates.
> 
> This also gets rid of the global skl_results, which was required for
> keeping track of the current atomic commit.
> 
> Changes since v1:
> - Move line unwrap to correct patch. (Lyude)
> - Make sure we don't regress ILK watermarks. (Matt)
> - Rephrase commit message. (Matt)
> 
...
> @@ -14459,8 +14436,17 @@ static void intel_atomic_commit_tail(struct 
> drm_atomic_state *state)
>   intel_check_cpu_fifo_underruns(dev_priv);
>   intel_check_pch_fifo_underruns(dev_priv);
>  
> - if (!crtc->state->active)
> - intel_update_watermarks(crtc);
> + if (!crtc->state->active) {
> + /*
> +  * Make sure we don't call initial_watermarks
> +  * for ILK-style watermark updates.
> +  */
> + if (HAS_DDI(dev_priv) && 
> dev_priv->display.initial_watermarks)

Aren't HSW/BDW DDI platforms?  They still use the ILK-style watermarks,
so I don't think this is protecting all the platforms it needs to.

Even if that weren't the case, I wouldn't be wild about using HAS_DDI
here since whether or not a platform has DDI isn't really the reason
we're programming watermarks differently so the code is a bit confusing
to the casual reader.


Matt

> + 
> dev_priv->display.initial_watermarks(intel_state,
> +  
> to_intel_crtc_state(crtc->state));
> + else
> + intel_update_watermarks(crtc);
> + }
>   }
>   }
>  
> @@ -14631,7 +14617,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>   drm_atomic_helper_swap_state(state, true);
>   dev_priv->wm.distrust_bios_wm = false;
> - dev_priv->wm.skl_results = intel_state->wm_results;
>   intel_shared_dpll_commit(state);
>   intel_atomic_track_fbs(state);
>  
> @@ -14939,7 +14924,7 @@ static void intel_begin_crtc_commit(struct drm_crtc 
> *crtc,
>   intel_pipe_update_start(intel_crtc);
>  
>   if (modeset)
> - return;
> + goto out;
>  
>   if (crtc->state->color_mgmt_changed || 
> to_intel_crtc_state(crtc->state)->update_pipe) {
>   intel_color_set_csc(crtc->state);
> @@ -14951,6 +14936,7 @@ static void intel_begin_crtc_commit(struct drm_crtc 
> *crtc,
>   else if (INTEL_GEN(dev_priv) >= 9)
>   skl_detach_scalers(intel_crtc);
>  
> +out:
>   if (dev_priv->display.atomic_evade_watermarks)
>   
> dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state),
>  intel_cstate);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 77a0a73e37b0..2cd7c5fd9ebd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1747,13 +1747,6 @@ bool skl_ddb_allocation_equals(const struct 
> skl_ddb_allocation *old,
>  enum pipe pipe);
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>struct intel_crtc *intel_crtc);
> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -  const struct skl_plane_wm *wm,
> -  const struct skl_ddb_allocation *ddb);
> -void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> - const struct skl_plane_wm *wm,
> - const struct skl_ddb_allocation *ddb,
> - int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fe4bc97ed56a..e2541539967b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4201,26 +4201,35 @@ skl_compute_wm(struct drm_atomic_state *state)
>   return 0;
>  }
>  
> -static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> -   struct intel_crtc_state *cstate)
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state, struct 
> intel_crtc_state *cstate)
>  {
>   struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>   struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>   struct skl_pipe_wm *pipe_wm = >wm.skl.optimal;
> + const struct skl_ddb_allocation *ddb = 

Re: [Intel-gfx] [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2.

[Lyude Paul]

This approach is perfect :).

Reviewed-by: Lyude 

On Wed, 2016-10-26 at 15:41 +0200, Maarten Lankhorst wrote:
> The watermark updates for SKL style watermarks are no longer done
> in the plane callbacks, but are now called in a separate watermark
> update function that's called during the same vblank evasion,
> before the plane updates.
> 
> This also gets rid of the global skl_results, which was required for
> keeping track of the current atomic commit.
> 
> Changes since v1:
> - Move line unwrap to correct patch. (Lyude)
> - Make sure we don't regress ILK watermarks. (Matt)
> - Rephrase commit message. (Matt)
> 
> Cc: Matt Roper 
> Cc: Lyude 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  7 ---
>  drivers/gpu/drm/i915/intel_display.c | 40 
> 
>  drivers/gpu/drm/i915/intel_drv.h |  7 ---
>  drivers/gpu/drm/i915/intel_pm.c  | 35 
> ---
>  drivers/gpu/drm/i915/intel_sprite.c  | 18 
>  5 files changed, 31 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 7a477d6a486e..227993f0e3ff 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2038,13 +2038,6 @@ struct drm_i915_private {
>    */
>   uint16_t skl_latency[8];
>  
> - /*
> -  * The skl_wm_values structure is a bit too big for
> stack
> -  * allocation, so we keep the staging struct where
> we store
> -  * intermediate results here instead.
> -  */
> - struct skl_wm_values skl_results;
> -
>   /* current hardware state */
>   union {
>   struct ilk_wm_values hw;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 592a6ec354a7..e80ecf85768a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3384,9 +3384,6 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> >base.crtc);
>   struct drm_framebuffer *fb = plane_state->base.fb;
> - const struct skl_wm_values *wm = _priv->wm.skl_results;
> - const struct skl_plane_wm *p_wm =
> - _state->wm.skl.optimal.planes[0];
>   int pipe = intel_crtc->pipe;
>   u32 plane_ctl;
>   unsigned int rotation = plane_state->base.rotation;
> @@ -3422,9 +3419,6 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>   intel_crtc->adjusted_x = src_x;
>   intel_crtc->adjusted_y = src_y;
>  
> - if (wm->dirty_pipes & drm_crtc_mask(_crtc->base))
> - skl_write_plane_wm(intel_crtc, p_wm, >ddb, 0);
> -
>   I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>   I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
>   I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> @@ -3457,18 +3451,8 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>   struct drm_device *dev = crtc->dev;
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> - const struct skl_plane_wm *p_wm = 
> >wm.skl.optimal.planes[0];
>   int pipe = intel_crtc->pipe;
>  
> - /*
> -  * We only populate skl_results on watermark updates, and if
> the
> -  * plane's visiblity isn't actually changing neither is its
> watermarks.
> -  */
> - if (!crtc->primary->state->visible)
> - skl_write_plane_wm(intel_crtc, p_wm,
> -    _priv->wm.skl_results.ddb,
> 0);
> -
>   I915_WRITE(PLANE_CTL(pipe, 0), 0);
>   I915_WRITE(PLANE_SURF(pipe, 0), 0);
>   POSTING_READ(PLANE_SURF(pipe, 0));
> @@ -10840,16 +10824,9 @@ static void i9xx_update_cursor(struct
> drm_crtc *crtc, u32 base,
>   struct drm_device *dev = crtc->dev;
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> - const struct skl_wm_values *wm = _priv->wm.skl_results;
> - const struct skl_plane_wm *p_wm =
> - >wm.skl.optimal.planes[PLANE_CURSOR];
>   int pipe = intel_crtc->pipe;
>   uint32_t cntl = 0;
>  
> - if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> drm_crtc_mask(crtc))
> - skl_write_cursor_wm(intel_crtc, p_wm, >ddb);
> -
>   if (plane_state && plane_state->base.visible) {
>   cntl = MCURSOR_GAMMA_ENABLE;
>   switch (plane_state->base.crtc_w) {
> @@ -14459,8 

[Intel-gfx] [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2.

[Maarten Lankhorst]

The watermark updates for SKL style watermarks are no longer done
in the plane callbacks, but are now called in a separate watermark
update function that's called during the same vblank evasion,
before the plane updates.

This also gets rid of the global skl_results, which was required for
keeping track of the current atomic commit.

Changes since v1:
- Move line unwrap to correct patch. (Lyude)
- Make sure we don't regress ILK watermarks. (Matt)
- Rephrase commit message. (Matt)

Cc: Matt Roper 
Cc: Lyude 
Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/i915/i915_drv.h  |  7 ---
 drivers/gpu/drm/i915/intel_display.c | 40 
 drivers/gpu/drm/i915/intel_drv.h |  7 ---
 drivers/gpu/drm/i915/intel_pm.c  | 35 ---
 drivers/gpu/drm/i915/intel_sprite.c  | 18 
 5 files changed, 31 insertions(+), 76 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7a477d6a486e..227993f0e3ff 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2038,13 +2038,6 @@ struct drm_i915_private {
 */
uint16_t skl_latency[8];
 
-   /*
-* The skl_wm_values structure is a bit too big for stack
-* allocation, so we keep the staging struct where we store
-* intermediate results here instead.
-*/
-   struct skl_wm_values skl_results;
-
/* current hardware state */
union {
struct ilk_wm_values hw;
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 592a6ec354a7..e80ecf85768a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3384,9 +3384,6 @@ static void skylake_update_primary_plane(struct drm_plane 
*plane,
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
struct drm_framebuffer *fb = plane_state->base.fb;
-   const struct skl_wm_values *wm = _priv->wm.skl_results;
-   const struct skl_plane_wm *p_wm =
-   _state->wm.skl.optimal.planes[0];
int pipe = intel_crtc->pipe;
u32 plane_ctl;
unsigned int rotation = plane_state->base.rotation;
@@ -3422,9 +3419,6 @@ static void skylake_update_primary_plane(struct drm_plane 
*plane,
intel_crtc->adjusted_x = src_x;
intel_crtc->adjusted_y = src_y;
 
-   if (wm->dirty_pipes & drm_crtc_mask(_crtc->base))
-   skl_write_plane_wm(intel_crtc, p_wm, >ddb, 0);
-
I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
@@ -3457,18 +3451,8 @@ static void skylake_disable_primary_plane(struct 
drm_plane *primary,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-   struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-   const struct skl_plane_wm *p_wm = >wm.skl.optimal.planes[0];
int pipe = intel_crtc->pipe;
 
-   /*
-* We only populate skl_results on watermark updates, and if the
-* plane's visiblity isn't actually changing neither is its watermarks.
-*/
-   if (!crtc->primary->state->visible)
-   skl_write_plane_wm(intel_crtc, p_wm,
-  _priv->wm.skl_results.ddb, 0);
-
I915_WRITE(PLANE_CTL(pipe, 0), 0);
I915_WRITE(PLANE_SURF(pipe, 0), 0);
POSTING_READ(PLANE_SURF(pipe, 0));
@@ -10840,16 +10824,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, 
u32 base,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-   struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-   const struct skl_wm_values *wm = _priv->wm.skl_results;
-   const struct skl_plane_wm *p_wm =
-   >wm.skl.optimal.planes[PLANE_CURSOR];
int pipe = intel_crtc->pipe;
uint32_t cntl = 0;
 
-   if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
-   skl_write_cursor_wm(intel_crtc, p_wm, >ddb);
-
if (plane_state && plane_state->base.visible) {
cntl = MCURSOR_GAMMA_ENABLE;
switch (plane_state->base.crtc_w) {
@@ -14459,8 +14436,17 @@ static void intel_atomic_commit_tail(struct 
drm_atomic_state *state)
intel_check_cpu_fifo_underruns(dev_priv);
intel_check_pch_fifo_underruns(dev_priv);
 
-   if (!crtc->state->active)