HI Ville, > -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of > Golani, Mitulkumar Ajitkumar > Sent: 12 April 2023 19:45 > To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH v2 5/6] drm/i915/vrr: Relocate VRR > enable/disable > > Hi Ville, > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Sent: 12 April 2023 13:15 > > To: Golani, Mitulkumar Ajitkumar > > <mitulkumar.ajitkumar.golani@xxxxxxxxx> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx> On > > > > > Behalf Of Ville Syrjala > > > > > Sent: 21 March 2023 19:26 > > > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > > > Subject: [PATCH v2 5/6] drm/i915/vrr: Relocate VRR > > > > > enable/disable > > > > > > > > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > > > > 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@xxxxxxxxx>
