Re: [Intel-gfx] [PATCH 7/9] drm/i915: Do DRRS disable/enable during pre/post_plane_update()

[Ville Syrjälä]

On Tue, Mar 15, 2022 at 06:48:12PM +, Souza, Jose wrote:
> On Tue, 2022-03-15 at 15:27 +0200, Ville Syrjala wrote:
> > @@ -217,13 +216,12 @@ void intel_drrs_enable(const struct intel_crtc_state 
> > *crtc_state)
> >  void intel_drrs_disable(const struct intel_crtc_state *old_crtc_state)
> >  {
> > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > -   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  
> > if (!old_crtc_state->has_drrs)
> > return;
> >  
> > -   drm_dbg_kms(_priv->drm, "[CRTC:%d:%s] Disabling DRRS\n",
> > -   crtc->base.base.id, crtc->base.name);
> > +   if (!old_crtc_state->hw.active)
> > +   return;
> 
> 
> Changes looks good but now there will not be any DRRS debug message, can you 
> at least add debug message in intel_drrs_compute_config() when DRRS is
> allowed?

I think we should just add it to the crtc state dump. It's sort of there
already by way of non-zero M2/N2, but I guess having a more explicit debug
for it wouldn't hurt.

> 
> With that:
> 
> Reviewed-by: José Roberto de Souza 

Thanks.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 7/9] drm/i915: Do DRRS disable/enable during pre/post_plane_update()

[Souza, Jose]

On Tue, 2022-03-15 at 15:27 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Let's just do a full DRRS disable/enable across all pipe updates.
> This guarantees that the DRRS work doesn't interfere with anything
> while the atomic commit is busy reprogramming the pipe.
> 
> Needed so that we can start reprogramming M/N seamlessly during
> fastsets whenever possible. Also avoids the pre-bdw DRRS PIPECONF
> rmw racing with the potential PIPECONF write from the atomic
> commit (eg. due to GAMMA_MODE changes).
> 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c |  4 --
>  drivers/gpu/drm/i915/display/intel_display.c |  8 ++--
>  drivers/gpu/drm/i915/display/intel_drrs.c| 40 ++--
>  drivers/gpu/drm/i915/display/intel_drrs.h|  3 --
>  4 files changed, 7 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index e2b297d2c295..dc208df829f1 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -45,7 +45,6 @@
>  #include "intel_dp_link_training.h"
>  #include "intel_dp_mst.h"
>  #include "intel_dpio_phy.h"
> -#include "intel_drrs.h"
>  #include "intel_dsi.h"
>  #include "intel_fdi.h"
>  #include "intel_fifo_underrun.h"
> @@ -3010,12 +3009,9 @@ static void intel_ddi_update_pipe_dp(struct 
> intel_atomic_state *state,
>const struct intel_crtc_state *crtc_state,
>const struct drm_connector_state 
> *conn_state)
>  {
> - struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -
>   intel_ddi_set_dp_msa(crtc_state, conn_state);
>  
>   intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
> - intel_drrs_update(state, crtc);
>  
>   intel_backlight_update(state, encoder, crtc_state, conn_state);
>   drm_connector_update_privacy_screen(conn_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> b/drivers/gpu/drm/i915/display/intel_display.c
> index eb49973621f0..86fc8ddd0b8f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1229,7 +1229,6 @@ static void intel_post_plane_update(struct 
> intel_atomic_state *state,
>  
>   hsw_ips_post_update(state, crtc);
>   intel_fbc_post_update(state, crtc);
> - intel_drrs_page_flip(crtc);
>  
>   if (needs_async_flip_vtd_wa(old_crtc_state) &&
>   !needs_async_flip_vtd_wa(new_crtc_state))
> @@ -1247,6 +1246,7 @@ static void intel_post_plane_update(struct 
> intel_atomic_state *state,
>   !needs_cursorclk_wa(new_crtc_state))
>   icl_wa_cursorclkgating(dev_priv, pipe, false);
>  
> + intel_drrs_enable(new_crtc_state);
>  }
>  
>  static void intel_crtc_enable_flip_done(struct intel_atomic_state *state,
> @@ -1324,6 +1324,8 @@ static void intel_pre_plane_update(struct 
> intel_atomic_state *state,
>   intel_atomic_get_new_crtc_state(state, crtc);
>   enum pipe pipe = crtc->pipe;
>  
> + intel_drrs_disable(old_crtc_state);
> +
>   intel_psr_pre_plane_update(state, crtc);
>  
>   if (hsw_ips_pre_update(state, crtc))
> @@ -8127,8 +8129,6 @@ static void intel_enable_crtc(struct intel_atomic_state 
> *state,
>   if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>   return;
>  
> - intel_drrs_enable(new_crtc_state);
> -
>   /* vblanks work again, re-enable pipe CRC. */
>   intel_crtc_enable_pipe_crc(crtc);
>  }
> @@ -8198,8 +8198,6 @@ static void intel_old_crtc_state_disables(struct 
> intel_atomic_state *state,
>*/
>   intel_crtc_disable_pipe_crc(crtc);
>  
> - intel_drrs_disable(old_crtc_state);
> -
>   dev_priv->display->crtc_disable(state, crtc);
>   crtc->active = false;
>   intel_fbc_disable(crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c 
> b/drivers/gpu/drm/i915/display/intel_drrs.c
> index 8f9e0fde0c5a..44c9af8f8b9b 100644
> --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> @@ -189,13 +189,12 @@ static unsigned int intel_drrs_frontbuffer_bits(const 
> struct intel_crtc_state *c
>  void intel_drrs_enable(const struct intel_crtc_state *crtc_state)
>  {
>   struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  
>   if (!crtc_state->has_drrs)
>   return;
>  
> - drm_dbg_kms(_priv->drm, "[CRTC:%d:%s] Enabling DRRS\n",
> - crtc->base.base.id, crtc->base.name);
> + if (!crtc_state->hw.active)
> + return;
>  
>   mutex_lock(>drrs.mutex);
>  
> @@ -217,13 +216,12 @@ void intel_drrs_enable(const struct intel_crtc_state 
> *crtc_state)
>  void intel_drrs_disable(const struct intel_crtc_state *old_crtc_state)
>  {
>   struct intel_crtc *crtc = 

[Intel-gfx] [PATCH 7/9] drm/i915: Do DRRS disable/enable during pre/post_plane_update()

[Ville Syrjala]

From: Ville Syrjälä 

Let's just do a full DRRS disable/enable across all pipe updates.
This guarantees that the DRRS work doesn't interfere with anything
while the atomic commit is busy reprogramming the pipe.

Needed so that we can start reprogramming M/N seamlessly during
fastsets whenever possible. Also avoids the pre-bdw DRRS PIPECONF
rmw racing with the potential PIPECONF write from the atomic
commit (eg. due to GAMMA_MODE changes).

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_ddi.c |  4 --
 drivers/gpu/drm/i915/display/intel_display.c |  8 ++--
 drivers/gpu/drm/i915/display/intel_drrs.c| 40 ++--
 drivers/gpu/drm/i915/display/intel_drrs.h|  3 --
 4 files changed, 7 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
b/drivers/gpu/drm/i915/display/intel_ddi.c
index e2b297d2c295..dc208df829f1 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -45,7 +45,6 @@
 #include "intel_dp_link_training.h"
 #include "intel_dp_mst.h"
 #include "intel_dpio_phy.h"
-#include "intel_drrs.h"
 #include "intel_dsi.h"
 #include "intel_fdi.h"
 #include "intel_fifo_underrun.h"
@@ -3010,12 +3009,9 @@ static void intel_ddi_update_pipe_dp(struct 
intel_atomic_state *state,
 const struct intel_crtc_state *crtc_state,
 const struct drm_connector_state 
*conn_state)
 {
-   struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
-
intel_ddi_set_dp_msa(crtc_state, conn_state);
 
intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
-   intel_drrs_update(state, crtc);
 
intel_backlight_update(state, encoder, crtc_state, conn_state);
drm_connector_update_privacy_screen(conn_state);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index eb49973621f0..86fc8ddd0b8f 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1229,7 +1229,6 @@ static void intel_post_plane_update(struct 
intel_atomic_state *state,
 
hsw_ips_post_update(state, crtc);
intel_fbc_post_update(state, crtc);
-   intel_drrs_page_flip(crtc);
 
if (needs_async_flip_vtd_wa(old_crtc_state) &&
!needs_async_flip_vtd_wa(new_crtc_state))
@@ -1247,6 +1246,7 @@ static void intel_post_plane_update(struct 
intel_atomic_state *state,
!needs_cursorclk_wa(new_crtc_state))
icl_wa_cursorclkgating(dev_priv, pipe, false);
 
+   intel_drrs_enable(new_crtc_state);
 }
 
 static void intel_crtc_enable_flip_done(struct intel_atomic_state *state,
@@ -1324,6 +1324,8 @@ static void intel_pre_plane_update(struct 
intel_atomic_state *state,
intel_atomic_get_new_crtc_state(state, crtc);
enum pipe pipe = crtc->pipe;
 
+   intel_drrs_disable(old_crtc_state);
+
intel_psr_pre_plane_update(state, crtc);
 
if (hsw_ips_pre_update(state, crtc))
@@ -8127,8 +8129,6 @@ static void intel_enable_crtc(struct intel_atomic_state 
*state,
if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
return;
 
-   intel_drrs_enable(new_crtc_state);
-
/* vblanks work again, re-enable pipe CRC. */
intel_crtc_enable_pipe_crc(crtc);
 }
@@ -8198,8 +8198,6 @@ static void intel_old_crtc_state_disables(struct 
intel_atomic_state *state,
 */
intel_crtc_disable_pipe_crc(crtc);
 
-   intel_drrs_disable(old_crtc_state);
-
dev_priv->display->crtc_disable(state, crtc);
crtc->active = false;
intel_fbc_disable(crtc);
diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c 
b/drivers/gpu/drm/i915/display/intel_drrs.c
index 8f9e0fde0c5a..44c9af8f8b9b 100644
--- a/drivers/gpu/drm/i915/display/intel_drrs.c
+++ b/drivers/gpu/drm/i915/display/intel_drrs.c
@@ -189,13 +189,12 @@ static unsigned int intel_drrs_frontbuffer_bits(const 
struct intel_crtc_state *c
 void intel_drrs_enable(const struct intel_crtc_state *crtc_state)
 {
struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
-   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
if (!crtc_state->has_drrs)
return;
 
-   drm_dbg_kms(_priv->drm, "[CRTC:%d:%s] Enabling DRRS\n",
-   crtc->base.base.id, crtc->base.name);
+   if (!crtc_state->hw.active)
+   return;
 
mutex_lock(>drrs.mutex);
 
@@ -217,13 +216,12 @@ void intel_drrs_enable(const struct intel_crtc_state 
*crtc_state)
 void intel_drrs_disable(const struct intel_crtc_state *old_crtc_state)
 {
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
-   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
if (!old_crtc_state->has_drrs)
return;
 
-   drm_dbg_kms(_priv->drm, "[CRTC:%d:%s] Disabling