subject:"\[Intel\-gfx\] \[RFC 2\/3\] drm\/i915\: Add Enable\/Disable for CMRR based on VRR state"

[Intel-gfx] [RFC 2/3] drm/i915: Add Enable/Disable for CMRR based on VRR state

2023-11-15 Thread Mitul Golani

Add CMRR/Fixed Average Vtotal mode enable and disable
functions based on change in VRR mode of operation.
When Adaptive Sync Vtotal is enabled, Fixed Average Vtotal
mode is disabled and vice versa. With this commit setting
the stage for subsequent CMRR enablement.

--v2:
- Check pipe active state in cmrr enabling. [Jani]
- Remove usage of bitwise OR on booleans. [Jani]
- Revert unrelated changes. [Jani]
- Update intel_vrr_enable, vrr and cmrr enable conditions. [Jani]
- Simplify whole if-ladder in intel_vrr_enable. [Jani]
- Revert patch restructuring mistakes in intel_vrr_get_config. [Jani]

--v3:
- Check pipe active state in cmrr disabling. (Similar to enable case).[Jani]
- Correct messed up condition in intel_vrr_enable while fixing rev2. [Jani]

Signed-off-by: Mitul Golani 
---
 .../drm/i915/display/intel_crtc_state_dump.c  |  4 ++-
 drivers/gpu/drm/i915/display/intel_display.c  | 30 ---
 drivers/gpu/drm/i915/display/intel_vrr.c  | 28 -
 3 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c 
b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
index 2d15e82c0b3d..908a4c4ccb00 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
@@ -299,7 +299,9 @@ void intel_crtc_state_dump(const struct intel_crtc_state 
*pipe_config,
intel_dump_buffer(i915, "ELD: ", pipe_config->eld,
  drm_eld_size(pipe_config->eld));
 
-   drm_dbg_kms(>drm, "vrr: %s, vmin: %d, vmax: %d, pipeline full: 
%d, guardband: %d flipline: %d, vmin vblank: %d, vmax vblank: %d\n",
+   drm_dbg_kms(>drm,
+   "cmrr: %s, vrr: %s, vmin: %d, vmax: %d, pipeline full: %d, 
guardband: %d, flipline: %d, vmin vblank: %d, vmax vblank: %d\n",
+   str_yes_no(pipe_config->cmrr.enable),
str_yes_no(pipe_config->vrr.enable),
pipe_config->vrr.vmin, pipe_config->vrr.vmax,
pipe_config->vrr.pipeline_full, pipe_config->vrr.guardband,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index f99d2de840bc..31ee7984a5fd 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -937,6 +937,15 @@ static bool vrr_enabling(const struct intel_crtc_state 
*old_crtc_state,
  vrr_params_changed(old_crtc_state, new_crtc_state)));
 }
 
+static bool cmrr_enabling(const struct intel_crtc_state *old_crtc_state,
+ const struct intel_crtc_state *new_crtc_state)
+{
+   if (!new_crtc_state->hw.active)
+   return false;
+
+   return is_enabling(cmrr.enable, old_crtc_state, new_crtc_state);
+}
+
 static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
  const struct intel_crtc_state *new_crtc_state)
 {
@@ -946,6 +955,15 @@ static bool vrr_disabling(const struct intel_crtc_state 
*old_crtc_state,
  vrr_params_changed(old_crtc_state, new_crtc_state)));
 }
 
+static bool cmrr_disabling(const struct intel_crtc_state *old_crtc_state,
+  const struct intel_crtc_state *new_crtc_state)
+{
+   if (!old_crtc_state->hw.active)
+   return false;
+
+   return is_disabling(cmrr.enable, old_crtc_state, new_crtc_state);
+}
+
 #undef is_disabling
 #undef is_enabling
 
@@ -1064,7 +1082,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;
 
-   if (vrr_disabling(old_crtc_state, new_crtc_state)) {
+   if (vrr_disabling(old_crtc_state, new_crtc_state) ||
+   cmrr_disabling(old_crtc_state, new_crtc_state)) {
intel_vrr_disable(old_crtc_state);
intel_crtc_update_active_timings(old_crtc_state, false);
}
@@ -6754,7 +6773,8 @@ static void commit_pipe_post_planes(struct 
intel_atomic_state *state,
!intel_crtc_needs_modeset(new_crtc_state))
skl_detach_scalers(new_crtc_state);
 
-   if (vrr_enabling(old_crtc_state, new_crtc_state))
+   if (vrr_enabling(old_crtc_state, new_crtc_state) ||
+   cmrr_enabling(old_crtc_state, new_crtc_state))
intel_vrr_enable(new_crtc_state);
 }
 
@@ -6851,9 +6871,11 @@ static void intel_update_crtc(struct intel_atomic_state 
*state,
 * FIXME Should be synchronized with the start of vblank somehow...
 */
if (vrr_enabling(old_crtc_state, new_crtc_state) ||
-   new_crtc_state->update_m_n || new_crtc_state->update_lrr)
+   new_crtc_state->update_m_n || new_crtc_state->update_lrr ||
+   cmrr_enabling(old_crtc_state, new_crtc_state))
intel_crtc_update_active_timings(new_crtc_state,
-

Re: [Intel-gfx] [RFC 2/3] drm/i915: Add Enable/Disable for CMRR based on VRR state

2023-11-15 Thread Golani, Mitulkumar Ajitkumar

Thanks @Jani Nikula

Addressed all review comments.

Regards,
Mitul

> -Original Message-
> From: Jani Nikula 
> Sent: Wednesday, November 15, 2023 2:25 PM
> To: Golani, Mitulkumar Ajitkumar ;
> intel-gfx@lists.freedesktop.org
> Cc: Syrjala, Ville 
> Subject: Re: [Intel-gfx] [RFC 2/3] drm/i915: Add Enable/Disable for CMRR
> based on VRR state
> 
> On Wed, 15 Nov 2023, Mitul Golani
>  wrote:
> > Add CMRR/Fixed Average Vtotal mode enable and disable functions based
> > on change in VRR mode of operation.
> > When Adaptive Sync Vtotal is enabled, Fixed Average Vtotal mode is
> > disabled and vice versa. With this commit setting the stage for
> > subsequent CMRR enablement.
> >
> > Signed-off-by: Mitul Golani 
> > ---
> >  .../drm/i915/display/intel_crtc_state_dump.c  |  4 +-
> > drivers/gpu/drm/i915/display/intel_display.c  | 24 ++--
> >  drivers/gpu/drm/i915/display/intel_vrr.c  | 37 +--
> >  3 files changed, 48 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > index 2d15e82c0b3d..908a4c4ccb00 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > @@ -299,7 +299,9 @@ void intel_crtc_state_dump(const struct
> intel_crtc_state *pipe_config,
> > intel_dump_buffer(i915, "ELD: ", pipe_config->eld,
> >   drm_eld_size(pipe_config->eld));
> >
> > -   drm_dbg_kms(>drm, "vrr: %s, vmin: %d, vmax: %d, pipeline
> full: %d, guardband: %d flipline: %d, vmin vblank: %d, vmax vblank: %d\n",
> > +   drm_dbg_kms(>drm,
> > +   "cmrr: %s, vrr: %s, vmin: %d, vmax: %d, pipeline full: %d,
> guardband: %d, flipline: %d, vmin vblank: %d, vmax vblank: %d\n",
> > +   str_yes_no(pipe_config->cmrr.enable),
> > str_yes_no(pipe_config->vrr.enable),
> > pipe_config->vrr.vmin, pipe_config->vrr.vmax,
> > pipe_config->vrr.pipeline_full, pipe_config-
> >vrr.guardband,
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index f99d2de840bc..f5a69309e65a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -937,6 +937,12 @@ static bool vrr_enabling(const struct
> intel_crtc_state *old_crtc_state,
> >   vrr_params_changed(old_crtc_state, new_crtc_state)));  }
> >
> > +static bool cmrr_enabling(const struct intel_crtc_state *old_crtc_state,
> > + const struct intel_crtc_state *new_crtc_state) {
> > +   return is_enabling(cmrr.enable, old_crtc_state, new_crtc_state); }
> > +
> >  static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
> >   const struct intel_crtc_state *new_crtc_state)  { @@
> -946,6
> > +952,12 @@ static bool vrr_disabling(const struct intel_crtc_state
> *old_crtc_state,
> >   vrr_params_changed(old_crtc_state, new_crtc_state)));  }
> >
> > +static bool cmrr_disabling(const struct intel_crtc_state *old_crtc_state,
> > +  const struct intel_crtc_state *new_crtc_state) {
> > +   return is_disabling(cmrr.enable, old_crtc_state, new_crtc_state); }
> > +
> 
> See
> https://patchwork.freedesktop.org/patch/msgid/20231106211915.13406-2-
> ville.syrj...@linux.intel.com
> 
> >  #undef is_disabling
> >  #undef is_enabling
> >
> > @@ -1064,7 +1076,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;
> >
> > -   if (vrr_disabling(old_crtc_state, new_crtc_state)) {
> > +   if (vrr_disabling(old_crtc_state, new_crtc_state) ||
> > +   cmrr_disabling(old_crtc_state, new_crtc_state)) {
> > intel_vrr_disable(old_crtc_state);
> > intel_crtc_update_active_timings(old_crtc_state, false);
> > }
> > @@ -6754,7 +6767,8 @@ static void commit_pipe_post_planes(struct
> intel_atomic_state *state,
> > !intel_crtc_needs_modeset(new_crtc_state))
> > skl_detach_scalers(new_crtc_state);
> >
> > -   if (vrr_enabling(old_crtc_state, new_crtc_state))
> > +   if (vrr_enabling(old_crtc_state, new_crtc_state) ||
> > +   cmrr_enabling(old_crtc_state, new_crtc_state))
> >

Re: [Intel-gfx] [RFC 2/3] drm/i915: Add Enable/Disable for CMRR based on VRR state

2023-11-15 Thread Jani Nikula

On Wed, 15 Nov 2023, Mitul Golani  wrote:
> Add CMRR/Fixed Average Vtotal mode enable and disable
> functions based on change in VRR mode of operation.
> When Adaptive Sync Vtotal is enabled, Fixed Average Vtotal
> mode is disabled and vice versa. With this commit setting
> the stage for subsequent CMRR enablement.
>
> Signed-off-by: Mitul Golani 
> ---
>  .../drm/i915/display/intel_crtc_state_dump.c  |  4 +-
>  drivers/gpu/drm/i915/display/intel_display.c  | 24 ++--
>  drivers/gpu/drm/i915/display/intel_vrr.c  | 37 +--
>  3 files changed, 48 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c 
> b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> index 2d15e82c0b3d..908a4c4ccb00 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> @@ -299,7 +299,9 @@ void intel_crtc_state_dump(const struct intel_crtc_state 
> *pipe_config,
>   intel_dump_buffer(i915, "ELD: ", pipe_config->eld,
> drm_eld_size(pipe_config->eld));
>  
> - drm_dbg_kms(>drm, "vrr: %s, vmin: %d, vmax: %d, pipeline full: 
> %d, guardband: %d flipline: %d, vmin vblank: %d, vmax vblank: %d\n",
> + drm_dbg_kms(>drm,
> + "cmrr: %s, vrr: %s, vmin: %d, vmax: %d, pipeline full: %d, 
> guardband: %d, flipline: %d, vmin vblank: %d, vmax vblank: %d\n",
> + str_yes_no(pipe_config->cmrr.enable),
>   str_yes_no(pipe_config->vrr.enable),
>   pipe_config->vrr.vmin, pipe_config->vrr.vmax,
>   pipe_config->vrr.pipeline_full, pipe_config->vrr.guardband,
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> b/drivers/gpu/drm/i915/display/intel_display.c
> index f99d2de840bc..f5a69309e65a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -937,6 +937,12 @@ static bool vrr_enabling(const struct intel_crtc_state 
> *old_crtc_state,
> vrr_params_changed(old_crtc_state, new_crtc_state)));
>  }
>  
> +static bool cmrr_enabling(const struct intel_crtc_state *old_crtc_state,
> +   const struct intel_crtc_state *new_crtc_state)
> +{
> + return is_enabling(cmrr.enable, old_crtc_state, new_crtc_state);
> +}
> +
>  static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
> const struct intel_crtc_state *new_crtc_state)
>  {
> @@ -946,6 +952,12 @@ static bool vrr_disabling(const struct intel_crtc_state 
> *old_crtc_state,
> vrr_params_changed(old_crtc_state, new_crtc_state)));
>  }
>  
> +static bool cmrr_disabling(const struct intel_crtc_state *old_crtc_state,
> +const struct intel_crtc_state *new_crtc_state)
> +{
> + return is_disabling(cmrr.enable, old_crtc_state, new_crtc_state);
> +}
> +

See 
https://patchwork.freedesktop.org/patch/msgid/20231106211915.13406-2-ville.syrj...@linux.intel.com

>  #undef is_disabling
>  #undef is_enabling
>  
> @@ -1064,7 +1076,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;
>  
> - if (vrr_disabling(old_crtc_state, new_crtc_state)) {
> + if (vrr_disabling(old_crtc_state, new_crtc_state) ||
> + cmrr_disabling(old_crtc_state, new_crtc_state)) {
>   intel_vrr_disable(old_crtc_state);
>   intel_crtc_update_active_timings(old_crtc_state, false);
>   }
> @@ -6754,7 +6767,8 @@ static void commit_pipe_post_planes(struct 
> intel_atomic_state *state,
>   !intel_crtc_needs_modeset(new_crtc_state))
>   skl_detach_scalers(new_crtc_state);
>  
> - if (vrr_enabling(old_crtc_state, new_crtc_state))
> + if (vrr_enabling(old_crtc_state, new_crtc_state) ||
> + cmrr_enabling(old_crtc_state, new_crtc_state))
>   intel_vrr_enable(new_crtc_state);
>  }
>  
> @@ -6851,9 +6865,11 @@ static void intel_update_crtc(struct 
> intel_atomic_state *state,
>* FIXME Should be synchronized with the start of vblank somehow...
>*/
>   if (vrr_enabling(old_crtc_state, new_crtc_state) ||
> - new_crtc_state->update_m_n || new_crtc_state->update_lrr)
> + new_crtc_state->update_m_n || new_crtc_state->update_lrr ||
> + cmrr_enabling(old_crtc_state, new_crtc_state))
>   intel_crtc_update_active_timings(new_crtc_state,
> -  new_crtc_state->vrr.enable);
> +  new_crtc_state->vrr.enable |
> +  new_crtc_state->cmrr.enable);

Please don't use bitwise OR on booleans.

>  
>   /*
>* We usually enable FIFO underrun interrupts as part of the
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c

[Intel-gfx] [RFC 2/3] drm/i915: Add Enable/Disable for CMRR based on VRR state

2023-11-14 Thread Mitul Golani

Add CMRR/Fixed Average Vtotal mode enable and disable
functions based on change in VRR mode of operation.
When Adaptive Sync Vtotal is enabled, Fixed Average Vtotal
mode is disabled and vice versa. With this commit setting
the stage for subsequent CMRR enablement.

Signed-off-by: Mitul Golani 
---
 .../drm/i915/display/intel_crtc_state_dump.c  |  4 +-
 drivers/gpu/drm/i915/display/intel_display.c  | 24 ++--
 drivers/gpu/drm/i915/display/intel_vrr.c  | 37 +--
 3 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c 
b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
index 2d15e82c0b3d..908a4c4ccb00 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
@@ -299,7 +299,9 @@ void intel_crtc_state_dump(const struct intel_crtc_state 
*pipe_config,
intel_dump_buffer(i915, "ELD: ", pipe_config->eld,
  drm_eld_size(pipe_config->eld));
 
-   drm_dbg_kms(>drm, "vrr: %s, vmin: %d, vmax: %d, pipeline full: 
%d, guardband: %d flipline: %d, vmin vblank: %d, vmax vblank: %d\n",
+   drm_dbg_kms(>drm,
+   "cmrr: %s, vrr: %s, vmin: %d, vmax: %d, pipeline full: %d, 
guardband: %d, flipline: %d, vmin vblank: %d, vmax vblank: %d\n",
+   str_yes_no(pipe_config->cmrr.enable),
str_yes_no(pipe_config->vrr.enable),
pipe_config->vrr.vmin, pipe_config->vrr.vmax,
pipe_config->vrr.pipeline_full, pipe_config->vrr.guardband,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index f99d2de840bc..f5a69309e65a 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -937,6 +937,12 @@ static bool vrr_enabling(const struct intel_crtc_state 
*old_crtc_state,
  vrr_params_changed(old_crtc_state, new_crtc_state)));
 }
 
+static bool cmrr_enabling(const struct intel_crtc_state *old_crtc_state,
+ const struct intel_crtc_state *new_crtc_state)
+{
+   return is_enabling(cmrr.enable, old_crtc_state, new_crtc_state);
+}
+
 static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
  const struct intel_crtc_state *new_crtc_state)
 {
@@ -946,6 +952,12 @@ static bool vrr_disabling(const struct intel_crtc_state 
*old_crtc_state,
  vrr_params_changed(old_crtc_state, new_crtc_state)));
 }
 
+static bool cmrr_disabling(const struct intel_crtc_state *old_crtc_state,
+  const struct intel_crtc_state *new_crtc_state)
+{
+   return is_disabling(cmrr.enable, old_crtc_state, new_crtc_state);
+}
+
 #undef is_disabling
 #undef is_enabling
 
@@ -1064,7 +1076,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;
 
-   if (vrr_disabling(old_crtc_state, new_crtc_state)) {
+   if (vrr_disabling(old_crtc_state, new_crtc_state) ||
+   cmrr_disabling(old_crtc_state, new_crtc_state)) {
intel_vrr_disable(old_crtc_state);
intel_crtc_update_active_timings(old_crtc_state, false);
}
@@ -6754,7 +6767,8 @@ static void commit_pipe_post_planes(struct 
intel_atomic_state *state,
!intel_crtc_needs_modeset(new_crtc_state))
skl_detach_scalers(new_crtc_state);
 
-   if (vrr_enabling(old_crtc_state, new_crtc_state))
+   if (vrr_enabling(old_crtc_state, new_crtc_state) ||
+   cmrr_enabling(old_crtc_state, new_crtc_state))
intel_vrr_enable(new_crtc_state);
 }
 
@@ -6851,9 +6865,11 @@ static void intel_update_crtc(struct intel_atomic_state 
*state,
 * FIXME Should be synchronized with the start of vblank somehow...
 */
if (vrr_enabling(old_crtc_state, new_crtc_state) ||
-   new_crtc_state->update_m_n || new_crtc_state->update_lrr)
+   new_crtc_state->update_m_n || new_crtc_state->update_lrr ||
+   cmrr_enabling(old_crtc_state, new_crtc_state))
intel_crtc_update_active_timings(new_crtc_state,
-new_crtc_state->vrr.enable);
+new_crtc_state->vrr.enable |
+new_crtc_state->cmrr.enable);
 
/*
 * We usually enable FIFO underrun interrupts as part of the
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c 
b/drivers/gpu/drm/i915/display/intel_vrr.c
index 4aeccbbf1d2a..1e33661062b3 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -224,7 +224,7 @@ void intel_vrr_send_push(const struct intel_crtc_state 
*crtc_state)
struct drm_i915_private *dev_priv =

[Intel-gfx] [RFC 2/3] drm/i915: Add Enable/Disable for CMRR based on VRR state

Re: [Intel-gfx] [RFC 2/3] drm/i915: Add Enable/Disable for CMRR based on VRR state

Re: [Intel-gfx] [RFC 2/3] drm/i915: Add Enable/Disable for CMRR based on VRR state

[Intel-gfx] [RFC 2/3] drm/i915: Add Enable/Disable for CMRR based on VRR state

4 matches

Site Navigation

Mail list logo

Footer information