Re: [Intel-gfx] [PATCH v2 5/6] drm/i915/vrr: Relocate VRR enable/disable

["Golani, Mitulkumar Ajitkumar" <mitulkumar.ajitkumar.golani@intel.com>]

HI Ville,

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Golani, Mitulkumar Ajitkumar
> Sent: 12 April 2023 19:45
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 5/6] drm/i915/vrr: Relocate VRR
> enable/disable
> 
> Hi Ville,
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: 12 April 2023 13:15
> > To: Golani, Mitulkumar Ajitkumar
> > <mitulkumar.ajitkumar.golani@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH v2 5/6] drm/i915/vrr: Relocate VRR
> > enable/disable
> >
> > On Tue, Apr 11, 2023 at 08:41:11AM +0300, Ville Syrjälä wrote:
> > > On Mon, Mar 27, 2023 at 08:05:49PM +0000, Golani, Mitulkumar
> > > Ajitkumar
> > wrote:
> > > > Hi Ville,
> > > >
> > > > > -----Original Message-----
> > > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On
> > > > > Behalf Of Ville Syrjala
> > > > > Sent: 21 March 2023 19:26
> > > > > To: intel-gfx@lists.freedesktop.org
> > > > > Subject: [Intel-gfx] [PATCH v2 5/6] drm/i915/vrr: Relocate VRR
> > > > > enable/disable
> > > > >
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Move VRR enabling/disabling into a place where it also works for
> > fastsets.
> > > > >
> > > > > With this we always start the transcoder up in non-VRR mode.
> > > > > Granted  we already did that but for a very short period of time.
> > > > > But now that we might end up doing a bit more with the
> > > > > transcoder in non-VRR mode it seems prudent to also update the
> > > > > active timings as the transcoder changes its operating mode.
> > > > >
> > > > > crtc_state->vrr.enable still tracks whether VRR is actually
> > > > > enabled or not, but now we configure all the other VRR timing
> > > > > registers whenever VRR is possible (whether we actually enable
> > > > > it or not). crtc_state->vrr.flipline can now serve as our "is VRR
> possible"
> > bit of state.
> > > >
> > > > Understood the change. I was thinking if it is possible to make
> > > > distinguish between is VRR "possible" and is VRR "enabled" by
> > > > adding a new param ? Although changes looks good to me but using
> > > > Flipline
> > value as "is VRR Possible" makes it bit confusing.
> > >
> > > I suppose we could think about adding a knob for it. It would just
> > > reflect the flipline enable bit state in the current scheme.
> >
> > After further pondering I think I'm leaning towards just adding a tiny
> > intel_vrr_possible()/etc. helper instead of adding a separate knob for
> > this into the crtc state. That seems like somehting we can trivially
> > do as a followup.
> 
> Yes. That also sounds good.
> 
> Regards,
> Mitul
> >
> > >
> > > Another thing I was pondering is whether we should even care about
> > > this in intel_dp_prepare_link_train() or if we should just set the
> > > MSA ingore bit any time we have a VRR capable display. But I suppose
> > > that could have some implicatations eg. for interlaces displays modes.
> 
> In that case to avoid implications should we add additional check for
> vrr.enable as well ?
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> >
> > --
> > Ville Syrjälä
> > Intel

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