diff mbox series

[09/12] drm/i915: Disable VRR during seamless M/N changes

Message ID 20230901130440.2085-10-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: VRR, LRR, and M/N stuff | expand

Commit Message

Ville Syrjala Sept. 1, 2023, 1:04 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make life less confusing by making sure VRR is disabled whenever
we do any drastic changes to the display timings, such as seamless
M/N changes.

Cc: Manasi Navare <navaremanasi@chromium.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Manasi Navare Sept. 7, 2023, 6:49 p.m. UTC | #1 
Hi Ville,

Since we are always disabling when update_m_n, that means if in gaming
mode if VRR enable is requested by userspace, it cannot
be enabled if update_m_n or dual refresh mode is enabled and say we
have downclocked from 120Hz - 60Hz?

Doesnt this contradict the purpose of this series to try and do VRR
update params in fastset because we want VRR range to be
correctly reflected when in dual refresh mode when we downclock from
120-60hz in gaming use case with VRR?

Am I missing something here?

Regards
Manasi

On Fri, Sep 1, 2023 at 6:05 AM Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
>
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make life less confusing by making sure VRR is disabled whenever
> we do any drastic changes to the display timings, such as seamless
> M/N changes.
>
> Cc: Manasi Navare <navaremanasi@chromium.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c20eaf0e7a91..cbbee303cd00 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -916,13 +916,15 @@ static bool planes_disabling(const struct intel_crtc_state *old_crtc_state,
>  static bool vrr_enabling(const struct intel_crtc_state *old_crtc_state,
>                          const struct intel_crtc_state *new_crtc_state)
>  {
> -       return is_enabling(vrr.enable, old_crtc_state, new_crtc_state);
> +       return is_enabling(vrr.enable, old_crtc_state, new_crtc_state) ||
> +               (new_crtc_state->vrr.enable && new_crtc_state->update_m_n);
>  }
>
>  static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
>                           const struct intel_crtc_state *new_crtc_state)
>  {
> -       return is_disabling(vrr.enable, old_crtc_state, new_crtc_state);
> +       return is_disabling(vrr.enable, old_crtc_state, new_crtc_state) ||
> +               (old_crtc_state->vrr.enable && new_crtc_state->update_m_n);
>  }
>
>  #undef is_disabling
> --
> 2.41.0
>
Ville Syrjala Sept. 8, 2023, 5:53 a.m. UTC | #2 
On Thu, Sep 07, 2023 at 11:49:10AM -0700, Manasi Navare wrote:
> Hi Ville,
> 
> Since we are always disabling when update_m_n, that means if in gaming
> mode if VRR enable is requested by userspace, it cannot
> be enabled if update_m_n or dual refresh mode is enabled and say we
> have downclocked from 120Hz - 60Hz?

No, it just means if you have VRR already enabled and want to do
a M/N change VRR gets temporarily disabled and re-enabled during
the commit.

> 
> Doesnt this contradict the purpose of this series to try and do VRR
> update params in fastset because we want VRR range to be
> correctly reflected when in dual refresh mode when we downclock from
> 120-60hz in gaming use case with VRR?
> 
> Am I missing something here?
> 
> Regards
> Manasi
> 
> On Fri, Sep 1, 2023 at 6:05 AM Ville Syrjala
> <ville.syrjala@linux.intel.com> wrote:
> >
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Make life less confusing by making sure VRR is disabled whenever
> > we do any drastic changes to the display timings, such as seamless
> > M/N changes.
> >
> > Cc: Manasi Navare <navaremanasi@chromium.org>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index c20eaf0e7a91..cbbee303cd00 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -916,13 +916,15 @@ static bool planes_disabling(const struct intel_crtc_state *old_crtc_state,
> >  static bool vrr_enabling(const struct intel_crtc_state *old_crtc_state,
> >                          const struct intel_crtc_state *new_crtc_state)
> >  {
> > -       return is_enabling(vrr.enable, old_crtc_state, new_crtc_state);
> > +       return is_enabling(vrr.enable, old_crtc_state, new_crtc_state) ||
> > +               (new_crtc_state->vrr.enable && new_crtc_state->update_m_n);
> >  }
> >
> >  static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
> >                           const struct intel_crtc_state *new_crtc_state)
> >  {
> > -       return is_disabling(vrr.enable, old_crtc_state, new_crtc_state);
> > +       return is_disabling(vrr.enable, old_crtc_state, new_crtc_state) ||
> > +               (old_crtc_state->vrr.enable && new_crtc_state->update_m_n);
> >  }
> >
> >  #undef is_disabling
> > --
> > 2.41.0
> >
Manasi Navare Sept. 8, 2023, 11:29 p.m. UTC | #3 
On Thu, Sep 7, 2023 at 10:54 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Thu, Sep 07, 2023 at 11:49:10AM -0700, Manasi Navare wrote:
> > Hi Ville,
> >
> > Since we are always disabling when update_m_n, that means if in gaming
> > mode if VRR enable is requested by userspace, it cannot
> > be enabled if update_m_n or dual refresh mode is enabled and say we
> > have downclocked from 120Hz - 60Hz?
>
> No, it just means if you have VRR already enabled and want to do
> a M/N change VRR gets temporarily disabled and re-enabled during
> the commit.

Okay sounds good.

Reviewed-by: Manasi Navare <navaremanasi@chromium.org>

Manasi

>
> >
> > Doesnt this contradict the purpose of this series to try and do VRR
> > update params in fastset because we want VRR range to be
> > correctly reflected when in dual refresh mode when we downclock from
> > 120-60hz in gaming use case with VRR?
> >
> > Am I missing something here?
> >
> > Regards
> > Manasi
> >
> > On Fri, Sep 1, 2023 at 6:05 AM Ville Syrjala
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Make life less confusing by making sure VRR is disabled whenever
> > > we do any drastic changes to the display timings, such as seamless
> > > M/N changes.
> > >
> > > Cc: Manasi Navare <navaremanasi@chromium.org>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index c20eaf0e7a91..cbbee303cd00 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -916,13 +916,15 @@ static bool planes_disabling(const struct intel_crtc_state *old_crtc_state,
> > >  static bool vrr_enabling(const struct intel_crtc_state *old_crtc_state,
> > >                          const struct intel_crtc_state *new_crtc_state)
> > >  {
> > > -       return is_enabling(vrr.enable, old_crtc_state, new_crtc_state);
> > > +       return is_enabling(vrr.enable, old_crtc_state, new_crtc_state) ||
> > > +               (new_crtc_state->vrr.enable && new_crtc_state->update_m_n);
> > >  }
> > >
> > >  static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
> > >                           const struct intel_crtc_state *new_crtc_state)
> > >  {
> > > -       return is_disabling(vrr.enable, old_crtc_state, new_crtc_state);
> > > +       return is_disabling(vrr.enable, old_crtc_state, new_crtc_state) ||
> > > +               (old_crtc_state->vrr.enable && new_crtc_state->update_m_n);
> > >  }
> > >
> > >  #undef is_disabling
> > > --
> > > 2.41.0
> > >
>
> --
> Ville Syrjälä
> Intel
Golani, Mitulkumar Ajitkumar Sept. 11, 2023, 5:46 p.m. UTC | #4 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: 01 September 2023 18:35
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 09/12] drm/i915: Disable VRR during seamless
> M/N changes
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Make life less confusing by making sure VRR is disabled whenever we do any
> drastic changes to the display timings, such as seamless M/N changes.
> 
> Cc: Manasi Navare <navaremanasi@chromium.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index c20eaf0e7a91..cbbee303cd00 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -916,13 +916,15 @@ static bool planes_disabling(const struct
> intel_crtc_state *old_crtc_state,  static bool vrr_enabling(const struct
> intel_crtc_state *old_crtc_state,
>  			 const struct intel_crtc_state *new_crtc_state)  {
> -	return is_enabling(vrr.enable, old_crtc_state, new_crtc_state);
> +	return is_enabling(vrr.enable, old_crtc_state, new_crtc_state) ||
> +		(new_crtc_state->vrr.enable && new_crtc_state-
> >update_m_n);
>  }
> 
>  static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
>  			  const struct intel_crtc_state *new_crtc_state)  {
> -	return is_disabling(vrr.enable, old_crtc_state, new_crtc_state);
> +	return is_disabling(vrr.enable, old_crtc_state, new_crtc_state) ||
> +		(old_crtc_state->vrr.enable && new_crtc_state-
> >update_m_n);
>  }

It seems when VRR is already enabled and during seamless M/N changes, disabled VRR and
enabled back again.

Change LGTM.
Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>

> 
>  #undef is_disabling
> --
> 2.41.0
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c20eaf0e7a91..cbbee303cd00 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -916,13 +916,15 @@  static bool planes_disabling(const struct intel_crtc_state *old_crtc_state,
 static bool vrr_enabling(const struct intel_crtc_state *old_crtc_state,
 			 const struct intel_crtc_state *new_crtc_state)
 {
-	return is_enabling(vrr.enable, old_crtc_state, new_crtc_state);
+	return is_enabling(vrr.enable, old_crtc_state, new_crtc_state) ||
+		(new_crtc_state->vrr.enable && new_crtc_state->update_m_n);
 }
 
 static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
 			  const struct intel_crtc_state *new_crtc_state)
 {
-	return is_disabling(vrr.enable, old_crtc_state, new_crtc_state);
+	return is_disabling(vrr.enable, old_crtc_state, new_crtc_state) ||
+		(old_crtc_state->vrr.enable && new_crtc_state->update_m_n);
 }
 
 #undef is_disabling