Re: [Intel-gfx] [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Maarten Lankhorst]

Op 20-10-16 om 19:51 schreef Paulo Zanoni:
> Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu:
>> Allow the driver to write watermarks during atomic evasion.
>> This will make it possible to write the watermarks in a cleaner
>> way on gen9+.
>>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |  6 --
>>  drivers/gpu/drm/i915/intel_display.c | 18 --
>>  drivers/gpu/drm/i915/intel_pm.c  | 19 +--
>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index f65ccf9b0bea..09588c58148f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -484,6 +484,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;
>> @@ -497,8 +498,9 @@ 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_evade_watermarks)(struct intel_atomic_state
>> *state, struct intel_crtc_state *cstate);
>> +void (*optimize_watermarks)(struct intel_atomic_state
>> *state, struct intel_crtc_state *cstate);
> Can't we just get intel_atomic_state from intel_crtc_state?  Why pass
> both?
No we can't. After the swap we can only obtain it from old_crtc_state, not the 
new state passed here.
>
>>  int (*compute_global_watermarks)(struct drm_atomic_state
>> *state);
>>  void (*update_wm)(struct drm_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 55f8ec8c76ae..23d8c72dade3 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5160,7 +5160,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(>base);
>>  }
>> @@ -5374,7 +5374,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)
>> @@ -5480,7 +5480,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(crtc);
>>  
>> @@ -14503,7 +14503,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) {
>> @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct
>> drm_crtc *crtc,
>>  struct intel_crtc_state *old_intel_state =
>>  to_intel_crtc_state(old_crtc_state);
>>  bool modeset = needs_modeset(crtc->state);
>> -enum pipe pipe = intel_crtc->pipe;
>>  
>>  /* Perform vblank evasion around commit operation */
>>  intel_pipe_update_start(intel_crtc);
>> @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct
>> drm_crtc *crtc,
>>  
>>  if (intel_cstate->update_pipe)
>>  intel_update_pipe_config(intel_crtc,
>> old_intel_state);
>> -else if (INTEL_GEN(dev_priv) >= 9) {
>> +else if (INTEL_GEN(dev_priv) >= 9)
>>  

Re: [Intel-gfx] [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Paulo Zanoni]

Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu:
> Allow the driver to write watermarks during atomic evasion.
> This will make it possible to write the watermarks in a cleaner
> way on gen9+.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  6 --
>  drivers/gpu/drm/i915/intel_display.c | 18 --
>  drivers/gpu/drm/i915/intel_pm.c  | 19 +--
>  3 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index f65ccf9b0bea..09588c58148f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -484,6 +484,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;
> @@ -497,8 +498,9 @@ 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_evade_watermarks)(struct intel_atomic_state
> *state, struct intel_crtc_state *cstate);
> + void (*optimize_watermarks)(struct intel_atomic_state
> *state, struct intel_crtc_state *cstate);

Can't we just get intel_atomic_state from intel_crtc_state?  Why pass
both?


>   int (*compute_global_watermarks)(struct drm_atomic_state
> *state);
>   void (*update_wm)(struct drm_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 55f8ec8c76ae..23d8c72dade3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5160,7 +5160,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(>base);
>  }
> @@ -5374,7 +5374,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)
> @@ -5480,7 +5480,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(crtc);
>  
> @@ -14503,7 +14503,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) {
> @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>   struct intel_crtc_state *old_intel_state =
>   to_intel_crtc_state(old_crtc_state);
>   bool modeset = needs_modeset(crtc->state);
> - enum pipe pipe = intel_crtc->pipe;
>  
>   /* Perform vblank evasion around commit operation */
>   intel_pipe_update_start(intel_crtc);
> @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  
>   if (intel_cstate->update_pipe)
>   intel_update_pipe_config(intel_crtc,
> old_intel_state);
> - else if (INTEL_GEN(dev_priv) >= 9) {
> + else if (INTEL_GEN(dev_priv) >= 9)
>   skl_detach_scalers(intel_crtc);
>  
> - I915_WRITE(PIPE_WM_LINETIME(pipe),
> -    intel_cstate->wm.skl.optimal.linetime);
> - }
> + if (dev_priv->display.atomic_evade_watermarks)
> + 

Re: [Intel-gfx] [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Maarten Lankhorst]

Hey,

Op 20-10-16 om 01:15 schreef Matt Roper:
> On Wed, Oct 12, 2016 at 03:28:18PM +0200, 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+.
>>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |  6 --
>>  drivers/gpu/drm/i915/intel_display.c | 18 --
>>  drivers/gpu/drm/i915/intel_pm.c  | 19 +--
>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index f65ccf9b0bea..09588c58148f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -484,6 +484,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;
>> @@ -497,8 +498,9 @@ 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_evade_watermarks)(struct intel_atomic_state *state, 
>> struct intel_crtc_state *cstate);
>> +void (*optimize_watermarks)(struct intel_atomic_state *state, struct 
>> intel_crtc_state *cstate);
> initial_watermarks() and optimize_watermarks() are currently only used
> on ILK (and possibly by in-development VLV/CHV patches that Ville is
> working on?).  As far as I can see, the top-level state that we add as a
> parameter here doesn't actually get used in the implementations.  Are
> you adding it to just make them more similar to the signature of the new
> atomic_evade_watermarks vfunc or did you have something else in mind?
>
> I'd also suggest adding a brief comment to your new skl_evade_crtc_wm()
> function that indicates that nearly all of the gen9 watermark values are
> per-plane values that get written as part of the general plane update in
> skylake_update_primary_plane and/or skl_update_plane.  Given that those
> two functions are located in other files that may help clarify to future
> developers why this function appears so trivial.

We don't completely use intel_atomic_state here yet, patch 7 uses it for the
ddb allocation and because initial_watermarks ends up calling
.atomic_evade_watermarks().

I added intel_atomic_state to all callbacks to keep the function signature
identical. It looked better to me to put the function signature in a small
change, and then the behavioral change in patch 7 separately, for easier
bisection.

~Maarten

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

Re: [Intel-gfx] [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Matt Roper]

On Wed, Oct 19, 2016 at 04:15:02PM -0700, Matt Roper wrote:
> On Wed, Oct 12, 2016 at 03:28:18PM +0200, 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+.
> > 
> > Signed-off-by: Maarten Lankhorst 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  6 --
> >  drivers/gpu/drm/i915/intel_display.c | 18 --
> >  drivers/gpu/drm/i915/intel_pm.c  | 19 +--
> >  3 files changed, 29 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index f65ccf9b0bea..09588c58148f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -484,6 +484,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;
> > @@ -497,8 +498,9 @@ 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_evade_watermarks)(struct intel_atomic_state *state, 
> > struct intel_crtc_state *cstate);
> > +   void (*optimize_watermarks)(struct intel_atomic_state *state, struct 
> > intel_crtc_state *cstate);
> 
> initial_watermarks() and optimize_watermarks() are currently only used
> on ILK (and possibly by in-development VLV/CHV patches that Ville is
> working on?).  As far as I can see, the top-level state that we add as a
> parameter here doesn't actually get used in the implementations.  Are
> you adding it to just make them more similar to the signature of the new
> atomic_evade_watermarks vfunc or did you have something else in mind?

For that matter, it doesn't look like it's really used in
atomic_evade_watermarks either except to grab dev_priv (which we can
obtain in other ways).


Matt

> 
> I'd also suggest adding a brief comment to your new skl_evade_crtc_wm()
> function that indicates that nearly all of the gen9 watermark values are
> per-plane values that get written as part of the general plane update in
> skylake_update_primary_plane and/or skl_update_plane.  Given that those
> two functions are located in other files that may help clarify to future
> developers why this function appears so trivial.
> 
> 
> Matt
> 
> > int (*compute_global_watermarks)(struct drm_atomic_state *state);
> > void (*update_wm)(struct drm_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 55f8ec8c76ae..23d8c72dade3 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5160,7 +5160,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(>base);
> >  }
> > @@ -5374,7 +5374,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)
> > @@ -5480,7 +5480,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(crtc);
> >  
> > @@ -14503,7 +14503,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)
> > -   

Re: [Intel-gfx] [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:18PM +0200, 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+.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  6 --
>  drivers/gpu/drm/i915/intel_display.c | 18 --
>  drivers/gpu/drm/i915/intel_pm.c  | 19 +--
>  3 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f65ccf9b0bea..09588c58148f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -484,6 +484,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;
> @@ -497,8 +498,9 @@ 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_evade_watermarks)(struct intel_atomic_state *state, 
> struct intel_crtc_state *cstate);
> + void (*optimize_watermarks)(struct intel_atomic_state *state, struct 
> intel_crtc_state *cstate);

initial_watermarks() and optimize_watermarks() are currently only used
on ILK (and possibly by in-development VLV/CHV patches that Ville is
working on?).  As far as I can see, the top-level state that we add as a
parameter here doesn't actually get used in the implementations.  Are
you adding it to just make them more similar to the signature of the new
atomic_evade_watermarks vfunc or did you have something else in mind?

I'd also suggest adding a brief comment to your new skl_evade_crtc_wm()
function that indicates that nearly all of the gen9 watermark values are
per-plane values that get written as part of the general plane update in
skylake_update_primary_plane and/or skl_update_plane.  Given that those
two functions are located in other files that may help clarify to future
developers why this function appears so trivial.


Matt

>   int (*compute_global_watermarks)(struct drm_atomic_state *state);
>   void (*update_wm)(struct drm_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 55f8ec8c76ae..23d8c72dade3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5160,7 +5160,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(>base);
>  }
> @@ -5374,7 +5374,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)
> @@ -5480,7 +5480,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(crtc);
>  
> @@ -14503,7 +14503,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) {
> @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct drm_crtc 
> *crtc,
>   struct intel_crtc_state *old_intel_state =
>   

[Intel-gfx] [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[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+.

Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/i915/i915_drv.h  |  6 --
 drivers/gpu/drm/i915/intel_display.c | 18 --
 drivers/gpu/drm/i915/intel_pm.c  | 19 +--
 3 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f65ccf9b0bea..09588c58148f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -484,6 +484,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;
@@ -497,8 +498,9 @@ 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_evade_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 drm_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 55f8ec8c76ae..23d8c72dade3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5160,7 +5160,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(>base);
 }
@@ -5374,7 +5374,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)
@@ -5480,7 +5480,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(crtc);
 
@@ -14503,7 +14503,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) {
@@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct drm_crtc 
*crtc,
struct intel_crtc_state *old_intel_state =
to_intel_crtc_state(old_crtc_state);
bool modeset = needs_modeset(crtc->state);
-   enum pipe pipe = intel_crtc->pipe;
 
/* Perform vblank evasion around commit operation */
intel_pipe_update_start(intel_crtc);
@@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct drm_crtc 
*crtc,
 
if (intel_cstate->update_pipe)
intel_update_pipe_config(intel_crtc, old_intel_state);
-   else if (INTEL_GEN(dev_priv) >= 9) {
+   else if (INTEL_GEN(dev_priv) >= 9)
skl_detach_scalers(intel_crtc);
 
-   I915_WRITE(PIPE_WM_LINETIME(pipe),
-  intel_cstate->wm.skl.optimal.linetime);
-   }
+   if (dev_priv->display.atomic_evade_watermarks)
+   
dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state),
 intel_cstate);
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc,
@@ -16388,7 +16386,7 @@ retry:
struct intel_crtc_state *cs =