On Tue, Nov 10, 2020 at 01:01:09PM +0200, Jani Nikula wrote:
> On Thu, 22 Oct 2020, Manasi Navare <manasi.d.navare@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
