Re: [Intel-gfx] [PATCH v3 01/11] drm/i915: Add a atomic evasion step to watermark programming, v3.

[Matt Roper]

On Tue, Nov 08, 2016 at 01:55:32PM +0100, Maarten Lankhorst wrote:
> Allow the driver to write watermarks during atomic evasion.
> This will make it possible to write the watermarks in a cleaner
> way on gen9+.
> 
> intel_atomic_state is not used here yet, but will be used when
> we program all watermarks as a separate step during evasion.
> 
> This also writes linetime all the time, while before it was only
> done during plane updates. This looks like this could be a bugfix,
> but I'm not sure what it affects.
> 
> Changes since v1:
> - Add comment about atomic evasion to commit message.
> - Unwrap I915_WRITE call. (Lyude)
> Changes since v2:
> - Rename atomic_evade_watermarks to atomic_update_watermarks. (Ville)
> - Add line wraps where appropriate, fix grammar in commit message. (Matt)

I'm not sure if these minor changes actually got applied or not; the
headline still needs s/a atomic/an atomic/ and there are still several
long lines that could be wrapped (especially the callsites of
initial_watermarks()).

But otherwise the code looks good, so if you clean up the cosmetic
issues, then my r-b still stands.


Matt

> 
> Signed-off-by: Maarten Lankhorst 
> Reviewed-by: Matt Roper 
> Cc: Paulo Zanoni 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  9 +++--
>  drivers/gpu/drm/i915/intel_display.c | 26 +-
>  drivers/gpu/drm/i915/intel_pm.c  | 18 --
>  3 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4735b4177100..00988d716d7e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -487,6 +487,7 @@ struct sdvo_device_mapping {
>  
>  struct intel_connector;
>  struct intel_encoder;
> +struct intel_atomic_state;
>  struct intel_crtc_state;
>  struct intel_initial_plane_config;
>  struct intel_crtc;
> @@ -500,8 +501,12 @@ struct drm_i915_display_funcs {
>   int (*compute_intermediate_wm)(struct drm_device *dev,
>  struct intel_crtc *intel_crtc,
>  struct intel_crtc_state *newstate);
> - void (*initial_watermarks)(struct intel_crtc_state *cstate);
> - void (*optimize_watermarks)(struct intel_crtc_state *cstate);
> + void (*initial_watermarks)(struct intel_atomic_state *state,
> +struct intel_crtc_state *cstate);
> + void (*atomic_update_watermarks)(struct intel_atomic_state *state,
> +  struct intel_crtc_state *cstate);
> + void (*optimize_watermarks)(struct intel_atomic_state *state,
> + struct intel_crtc_state *cstate);
>   int (*compute_global_watermarks)(struct drm_atomic_state *state);
>   void (*update_wm)(struct intel_crtc *crtc);
>   int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 92ab01f33208..66cf29ac9fe4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5169,7 +5169,7 @@ static void intel_pre_plane_update(struct 
> intel_crtc_state *old_crtc_state)
>* us to.
>*/
>   if (dev_priv->display.initial_watermarks != NULL)
> - dev_priv->display.initial_watermarks(pipe_config);
> + 
> dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), 
> pipe_config);
>   else if (pipe_config->update_wm_pre)
>   intel_update_watermarks(crtc);
>  }
> @@ -5383,7 +5383,7 @@ static void ironlake_crtc_enable(struct 
> intel_crtc_state *pipe_config,
>   intel_color_load_luts(_config->base);
>  
>   if (dev_priv->display.initial_watermarks != NULL)
> - dev_priv->display.initial_watermarks(intel_crtc->config);
> + 
> dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), 
> intel_crtc->config);
>   intel_enable_pipe(intel_crtc);
>  
>   if (intel_crtc->config->has_pch_encoder)
> @@ -5489,7 +5489,7 @@ static void haswell_crtc_enable(struct intel_crtc_state 
> *pipe_config,
>   intel_ddi_enable_transcoder_func(crtc);
>  
>   if (dev_priv->display.initial_watermarks != NULL)
> - dev_priv->display.initial_watermarks(pipe_config);
> + 
> dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), 
> pipe_config);
>   else
>   intel_update_watermarks(intel_crtc);
>  
> @@ -14474,7 +14474,7 @@ static void intel_atomic_commit_tail(struct 
> drm_atomic_state *state)
>   intel_cstate = to_intel_crtc_state(crtc->state);
>  
>   if (dev_priv->display.optimize_watermarks)
> - dev_priv->display.optimize_watermarks(intel_cstate);
> + 

[Intel-gfx] [PATCH v3 01/11] drm/i915: Add a atomic evasion step to watermark programming, v3.

[Maarten Lankhorst]

Allow the driver to write watermarks during atomic evasion.
This will make it possible to write the watermarks in a cleaner
way on gen9+.

intel_atomic_state is not used here yet, but will be used when
we program all watermarks as a separate step during evasion.

This also writes linetime all the time, while before it was only
done during plane updates. This looks like this could be a bugfix,
but I'm not sure what it affects.

Changes since v1:
- Add comment about atomic evasion to commit message.
- Unwrap I915_WRITE call. (Lyude)
Changes since v2:
- Rename atomic_evade_watermarks to atomic_update_watermarks. (Ville)
- Add line wraps where appropriate, fix grammar in commit message. (Matt)

Signed-off-by: Maarten Lankhorst 
Reviewed-by: Matt Roper 
Cc: Paulo Zanoni 
---
 drivers/gpu/drm/i915/i915_drv.h  |  9 +++--
 drivers/gpu/drm/i915/intel_display.c | 26 +-
 drivers/gpu/drm/i915/intel_pm.c  | 18 --
 3 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4735b4177100..00988d716d7e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -487,6 +487,7 @@ struct sdvo_device_mapping {
 
 struct intel_connector;
 struct intel_encoder;
+struct intel_atomic_state;
 struct intel_crtc_state;
 struct intel_initial_plane_config;
 struct intel_crtc;
@@ -500,8 +501,12 @@ struct drm_i915_display_funcs {
int (*compute_intermediate_wm)(struct drm_device *dev,
   struct intel_crtc *intel_crtc,
   struct intel_crtc_state *newstate);
-   void (*initial_watermarks)(struct intel_crtc_state *cstate);
-   void (*optimize_watermarks)(struct intel_crtc_state *cstate);
+   void (*initial_watermarks)(struct intel_atomic_state *state,
+  struct intel_crtc_state *cstate);
+   void (*atomic_update_watermarks)(struct intel_atomic_state *state,
+struct intel_crtc_state *cstate);
+   void (*optimize_watermarks)(struct intel_atomic_state *state,
+   struct intel_crtc_state *cstate);
int (*compute_global_watermarks)(struct drm_atomic_state *state);
void (*update_wm)(struct intel_crtc *crtc);
int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 92ab01f33208..66cf29ac9fe4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5169,7 +5169,7 @@ static void intel_pre_plane_update(struct 
intel_crtc_state *old_crtc_state)
 * us to.
 */
if (dev_priv->display.initial_watermarks != NULL)
-   dev_priv->display.initial_watermarks(pipe_config);
+   
dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), 
pipe_config);
else if (pipe_config->update_wm_pre)
intel_update_watermarks(crtc);
 }
@@ -5383,7 +5383,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state 
*pipe_config,
intel_color_load_luts(_config->base);
 
if (dev_priv->display.initial_watermarks != NULL)
-   dev_priv->display.initial_watermarks(intel_crtc->config);
+   
dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), 
intel_crtc->config);
intel_enable_pipe(intel_crtc);
 
if (intel_crtc->config->has_pch_encoder)
@@ -5489,7 +5489,7 @@ static void haswell_crtc_enable(struct intel_crtc_state 
*pipe_config,
intel_ddi_enable_transcoder_func(crtc);
 
if (dev_priv->display.initial_watermarks != NULL)
-   dev_priv->display.initial_watermarks(pipe_config);
+   
dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), 
pipe_config);
else
intel_update_watermarks(intel_crtc);
 
@@ -14474,7 +14474,7 @@ static void intel_atomic_commit_tail(struct 
drm_atomic_state *state)
intel_cstate = to_intel_crtc_state(crtc->state);
 
if (dev_priv->display.optimize_watermarks)
-   dev_priv->display.optimize_watermarks(intel_cstate);
+   dev_priv->display.optimize_watermarks(intel_state, 
intel_cstate);
}
 
for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
@@ -14909,10 +14909,11 @@ static void intel_begin_crtc_commit(struct drm_crtc 
*crtc,
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct intel_crtc_state *intel_cstate =
to_intel_crtc_state(crtc->state);
-   struct intel_crtc_state *old_intel_state =
+   struct intel_crtc_state *old_intel_cstate =
to_intel_crtc_state(old_crtc_state);
+