Re: [Intel-gfx] [PATCH 09/11] drm/i915/display/vrr: Disable VRR in modeset disable path

["Navare, Manasi" <manasi.d.navare@intel.com>]

On Tue, Nov 10, 2020 at 01:01:09PM +0200, Jani Nikula wrote:
> On Thu, 22 Oct 2020, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > This patch disables the VRR enable and VRR PUSH
> > bits in the HW during commit modeset disable sequence.
> >
> > Thsi disable will happen when the port is disabled
> > or when the userspace sets VRR prop to false and
> > requests to disable VRR.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c |  2 ++
> >  drivers/gpu/drm/i915/display/intel_vrr.c | 22 ++++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_vrr.h |  1 +
> >  3 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 391c51979334..565155af3fb9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3819,6 +3819,8 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,
> >  
> >  		intel_disable_pipe(old_crtc_state);
> >  
> > +		intel_vrr_disable(old_crtc_state);
> > +
> >  		intel_ddi_disable_transcoder_func(old_crtc_state);
> >  
> >  		intel_dsc_disable(old_crtc_state);
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index ec1ce88e869c..5075ecb9b5a7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -119,3 +119,25 @@ void intel_vrr_send_push(const struct intel_crtc_state *crtc_state)
> >  		pipe_name(pipe));
> >  }
> >  
> > +void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> 
> Haven't commented on all patches, but please use i915 instead of
> dev_priv for new code, throughout.
> 
> > +	enum pipe pipe = crtc->pipe;
> > +	u32 trans_vrr_ctl = 0, trans_push = 0;
> 
> Unnecessary initializations, and in fact unnecessary variables with
> intel_de_rmw.
>

Okay yes will try using the intel_de_rmw here and use the (VRR_CTL_FLIP_LINE_EN | VRR_CTL_VRR_ENABLE) directly in the clear field
 
> > +
> > +	if (!old_crtc_state->vrr.enable)
> > +		return;
> > +
> > +	trans_vrr_ctl = intel_de_read(dev_priv, TRANS_VRR_CTL(pipe));
> > +	trans_vrr_ctl &= ~(VRR_CTL_FLIP_LINE_EN | VRR_CTL_VRR_ENABLE);
> > +	intel_de_write(dev_priv, TRANS_VRR_CTL(pipe), trans_vrr_ctl);
> > +
> > +	trans_push = intel_de_read(dev_priv, TRANS_PUSH(pipe));
> > +	trans_push &= ~TRANS_PUSH_EN;
> > +	intel_de_write(dev_priv, TRANS_PUSH(pipe), trans_push);
> 
> Please use intel_de_rmw for both.
> 
> > +
> > +	drm_dbg(&dev_priv->drm, "Disabling VRR on Pipe (%c)\n",
> > +		pipe_name(pipe));
> 
> drm_dbg_kms, "pipe %c" is the convention.

Okay will correct it

Thanks for the above feedback I will fix them in the next rev

Manasi


> 
> > +}
> > +
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
> > index a6b78e1676cb..8c6fd2d1bee5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> > @@ -20,5 +20,6 @@ void intel_vrr_compute_config(struct intel_dp *intel_dp,
> >  void intel_vrr_enable(struct intel_encoder *encoder,
> >  		      const struct intel_crtc_state *crtc_state);
> >  void intel_vrr_send_push(const struct intel_crtc_state *crtc_state);
> > +void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state);
> >  
> >  #endif /* __INTEL_VRR_H__ */
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx