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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 12, 2023 at 02:16:22PM +0000, Golani, Mitulkumar Ajitkumar wrote:
> 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>

Thanks for the review. Series pushed.

-- 
Ville Syrjälä
Intel