| Message ID | 20201022222709.29386-8-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Thu, 22 Oct 2020, Manasi Navare <manasi.d.navare@intel.com> wrote: > This patch computes the VRR parameters from VRR crtc states > and configures them in VRR registers during CRTC enable in > the modeset enable sequence. > > 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 | 5 ++++ > drivers/gpu/drm/i915/display/intel_vrr.c | 38 ++++++++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_vrr.h | 2 ++ > 3 files changed, 45 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 09811be08cfe..391c51979334 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -49,6 +49,7 @@ > #include "intel_sprite.h" > #include "intel_tc.h" > #include "intel_vdsc.h" > +#include "intel_vrr.h" > > struct ddi_buf_trans { > u32 trans1; /* balance leg enable, de-emph level */ > @@ -4046,6 +4047,10 @@ static void intel_enable_ddi(struct intel_atomic_state *state, > > intel_ddi_enable_transcoder_func(encoder, crtc_state); > > + /* Enable VRR if requested through CRTC property */ I don't think the comment is helpful, really. > + if (crtc_state->vrr.enable) > + intel_vrr_enable(encoder, crtc_state); In the disable path you check the vrr.enable within the function. Perhaps we should do the same here, i.e. call vrr enable unconditionally and have it early return if not requested. > + > intel_enable_pipe(crtc_state); > > intel_crtc_vblank_on(crtc_state); > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c > index 56114f535f94..7f1353bac583 100644 > --- a/drivers/gpu/drm/i915/display/intel_vrr.c > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > @@ -64,3 +64,41 @@ intel_vrr_compute_config(struct intel_dp *intel_dp, > crtc_state->vrr.vtotalmax); > } > > +void intel_vrr_enable(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > + enum pipe pipe = crtc->pipe; > + const struct drm_display_mode *adjusted_mode = > + &crtc_state->hw.adjusted_mode; > + u32 trans_vrr_ctl = 0, trans_vrr_vmax = 0, trans_vrr_vmin = 0, trans_vrr_flipline = 0, trans_push = 0; > + u16 framestart_to_pipelinefull_linecnt = 0; All the initializations to 0 are unnecessary. > + > + framestart_to_pipelinefull_linecnt = > + min_t(u16, 255, (crtc_state->vrr.vtotalmin - adjusted_mode->crtc_vdisplay)); > + > + trans_vrr_ctl = VRR_CTL_VRR_ENABLE | VRR_CTL_IGN_MAX_SHIFT | > + VRR_CTL_FLIP_LINE_EN | VRR_CTL_LINE_COUNT(framestart_to_pipelinefull_linecnt) | > + VRR_CTL_SW_FULLLINE_COUNT; > + > + /* Programming adjustments for 0 based regs */ > + trans_vrr_vmax = crtc_state->vrr.vtotalmax - 1; > + trans_vrr_vmin = crtc_state->vrr.vtotalmin - 1; > + trans_vrr_flipline = crtc_state->vrr.vtotalmin - 1; > + > + trans_push = TRANS_PUSH_EN; Frankly I'd just throw away the above four temp variables. > + > + intel_de_write(dev_priv, TRANS_VRR_VMIN(pipe), trans_vrr_vmin); > + intel_de_write(dev_priv, TRANS_VRR_VMAX(pipe), trans_vrr_vmax); > + intel_de_write(dev_priv, TRANS_VRR_CTL(pipe), trans_vrr_ctl); > + intel_de_write(dev_priv, TRANS_VRR_FLIPLINE(pipe), trans_vrr_flipline); > + intel_de_write(dev_priv, TRANS_PUSH(pipe), trans_push); > + > + drm_dbg(&dev_priv->drm, "Enabling VRR on pipe (%c)\n", pipe_name(pipe)); drm_dbg_kms. "pipe %c" is the convention, not "pipe (%c)". > + drm_dbg(&dev_priv->drm, "VRR Parameters: Vblank - Min = %d, Max = %d Flipline Count = %d, CTL Reg = 0x%08x, TRANS PUSH reg = 0x%08x", > + crtc_state->vrr.vtotalmin, crtc_state->vrr.vtotalmax, > + crtc_state->vrr.vtotalmin, trans_vrr_ctl, > + trans_push); > +} > + > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h > index 1e6fe8fe92ec..05d982d6fbae 100644 > --- a/drivers/gpu/drm/i915/display/intel_vrr.h > +++ b/drivers/gpu/drm/i915/display/intel_vrr.h > @@ -17,5 +17,7 @@ struct intel_dp; > bool intel_is_vrr_capable(struct drm_connector *connector); > void intel_vrr_compute_config(struct intel_dp *intel_dp, > struct intel_crtc_state *crtc_state); > +void intel_vrr_enable(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state); > > #endif /* __INTEL_VRR_H__ */
On Tue, Nov 10, 2020 at 12:56:40PM +0200, Jani Nikula wrote: > On Thu, 22 Oct 2020, Manasi Navare <manasi.d.navare@intel.com> wrote: > > This patch computes the VRR parameters from VRR crtc states > > and configures them in VRR registers during CRTC enable in > > the modeset enable sequence. > > > > 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 | 5 ++++ > > drivers/gpu/drm/i915/display/intel_vrr.c | 38 ++++++++++++++++++++++++ > > drivers/gpu/drm/i915/display/intel_vrr.h | 2 ++ > > 3 files changed, 45 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 09811be08cfe..391c51979334 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -49,6 +49,7 @@ > > #include "intel_sprite.h" > > #include "intel_tc.h" > > #include "intel_vdsc.h" > > +#include "intel_vrr.h" > > > > struct ddi_buf_trans { > > u32 trans1; /* balance leg enable, de-emph level */ > > @@ -4046,6 +4047,10 @@ static void intel_enable_ddi(struct intel_atomic_state *state, > > > > intel_ddi_enable_transcoder_func(encoder, crtc_state); > > > > + /* Enable VRR if requested through CRTC property */ > > I don't think the comment is helpful, really. Yes will remove > > > + if (crtc_state->vrr.enable) > > + intel_vrr_enable(encoder, crtc_state); > > In the disable path you check the vrr.enable within the > function. Perhaps we should do the same here, i.e. call vrr enable > unconditionally and have it early return if not requested. Okay will do > > > + > > intel_enable_pipe(crtc_state); > > > > intel_crtc_vblank_on(crtc_state); > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c > > index 56114f535f94..7f1353bac583 100644 > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > > @@ -64,3 +64,41 @@ intel_vrr_compute_config(struct intel_dp *intel_dp, > > crtc_state->vrr.vtotalmax); > > } > > > > +void intel_vrr_enable(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > + enum pipe pipe = crtc->pipe; > > + const struct drm_display_mode *adjusted_mode = > > + &crtc_state->hw.adjusted_mode; > > + u32 trans_vrr_ctl = 0, trans_vrr_vmax = 0, trans_vrr_vmin = 0, trans_vrr_flipline = 0, trans_push = 0; > > + u16 framestart_to_pipelinefull_linecnt = 0; > > All the initializations to 0 are unnecessary. Will remove > > > + > > + framestart_to_pipelinefull_linecnt = > > + min_t(u16, 255, (crtc_state->vrr.vtotalmin - adjusted_mode->crtc_vdisplay)); > > + > > + trans_vrr_ctl = VRR_CTL_VRR_ENABLE | VRR_CTL_IGN_MAX_SHIFT | > > + VRR_CTL_FLIP_LINE_EN | VRR_CTL_LINE_COUNT(framestart_to_pipelinefull_linecnt) | > > + VRR_CTL_SW_FULLLINE_COUNT; > > + > > + /* Programming adjustments for 0 based regs */ > > + trans_vrr_vmax = crtc_state->vrr.vtotalmax - 1; > > + trans_vrr_vmin = crtc_state->vrr.vtotalmin - 1; > > + trans_vrr_flipline = crtc_state->vrr.vtotalmin - 1; > > + > > + trans_push = TRANS_PUSH_EN; > > Frankly I'd just throw away the above four temp variables. Okay and use them directly in de_write ? > > > + > > + intel_de_write(dev_priv, TRANS_VRR_VMIN(pipe), trans_vrr_vmin); > > + intel_de_write(dev_priv, TRANS_VRR_VMAX(pipe), trans_vrr_vmax); > > + intel_de_write(dev_priv, TRANS_VRR_CTL(pipe), trans_vrr_ctl); > > + intel_de_write(dev_priv, TRANS_VRR_FLIPLINE(pipe), trans_vrr_flipline); > > + intel_de_write(dev_priv, TRANS_PUSH(pipe), trans_push); > > + > > + drm_dbg(&dev_priv->drm, "Enabling VRR on pipe (%c)\n", pipe_name(pipe)); > > drm_dbg_kms. "pipe %c" is the convention, not "pipe (%c)". Got it will fix this Thanks for the inputs Manasi > > > + drm_dbg(&dev_priv->drm, "VRR Parameters: Vblank - Min = %d, Max = %d Flipline Count = %d, CTL Reg = 0x%08x, TRANS PUSH reg = 0x%08x", > > + crtc_state->vrr.vtotalmin, crtc_state->vrr.vtotalmax, > > + crtc_state->vrr.vtotalmin, trans_vrr_ctl, > > + trans_push); > > +} > > + > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h > > index 1e6fe8fe92ec..05d982d6fbae 100644 > > --- a/drivers/gpu/drm/i915/display/intel_vrr.h > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.h > > @@ -17,5 +17,7 @@ struct intel_dp; > > bool intel_is_vrr_capable(struct drm_connector *connector); > > void intel_vrr_compute_config(struct intel_dp *intel_dp, > > struct intel_crtc_state *crtc_state); > > +void intel_vrr_enable(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state); > > > > #endif /* __INTEL_VRR_H__ */ > > -- > Jani Nikula, Intel Open Source Graphics Center
On Tue, 01 Dec 2020, "Navare, Manasi" <manasi.d.navare@intel.com> wrote: > On Tue, Nov 10, 2020 at 12:56:40PM +0200, Jani Nikula wrote: >> On Thu, 22 Oct 2020, Manasi Navare <manasi.d.navare@intel.com> wrote: >> > + /* Programming adjustments for 0 based regs */ >> > + trans_vrr_vmax = crtc_state->vrr.vtotalmax - 1; >> > + trans_vrr_vmin = crtc_state->vrr.vtotalmin - 1; >> > + trans_vrr_flipline = crtc_state->vrr.vtotalmin - 1; >> > + >> > + trans_push = TRANS_PUSH_EN; >> >> Frankly I'd just throw away the above four temp variables. > > Okay and use them directly in de_write ? Yes. BR, Jani.
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 09811be08cfe..391c51979334 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -49,6 +49,7 @@ #include "intel_sprite.h" #include "intel_tc.h" #include "intel_vdsc.h" +#include "intel_vrr.h" struct ddi_buf_trans { u32 trans1; /* balance leg enable, de-emph level */ @@ -4046,6 +4047,10 @@ static void intel_enable_ddi(struct intel_atomic_state *state, intel_ddi_enable_transcoder_func(encoder, crtc_state); + /* Enable VRR if requested through CRTC property */ + if (crtc_state->vrr.enable) + intel_vrr_enable(encoder, crtc_state); + intel_enable_pipe(crtc_state); intel_crtc_vblank_on(crtc_state); diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c index 56114f535f94..7f1353bac583 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.c +++ b/drivers/gpu/drm/i915/display/intel_vrr.c @@ -64,3 +64,41 @@ intel_vrr_compute_config(struct intel_dp *intel_dp, crtc_state->vrr.vtotalmax); } +void intel_vrr_enable(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); + enum pipe pipe = crtc->pipe; + const struct drm_display_mode *adjusted_mode = + &crtc_state->hw.adjusted_mode; + u32 trans_vrr_ctl = 0, trans_vrr_vmax = 0, trans_vrr_vmin = 0, trans_vrr_flipline = 0, trans_push = 0; + u16 framestart_to_pipelinefull_linecnt = 0; + + framestart_to_pipelinefull_linecnt = + min_t(u16, 255, (crtc_state->vrr.vtotalmin - adjusted_mode->crtc_vdisplay)); + + trans_vrr_ctl = VRR_CTL_VRR_ENABLE | VRR_CTL_IGN_MAX_SHIFT | + VRR_CTL_FLIP_LINE_EN | VRR_CTL_LINE_COUNT(framestart_to_pipelinefull_linecnt) | + VRR_CTL_SW_FULLLINE_COUNT; + + /* Programming adjustments for 0 based regs */ + trans_vrr_vmax = crtc_state->vrr.vtotalmax - 1; + trans_vrr_vmin = crtc_state->vrr.vtotalmin - 1; + trans_vrr_flipline = crtc_state->vrr.vtotalmin - 1; + + trans_push = TRANS_PUSH_EN; + + intel_de_write(dev_priv, TRANS_VRR_VMIN(pipe), trans_vrr_vmin); + intel_de_write(dev_priv, TRANS_VRR_VMAX(pipe), trans_vrr_vmax); + intel_de_write(dev_priv, TRANS_VRR_CTL(pipe), trans_vrr_ctl); + intel_de_write(dev_priv, TRANS_VRR_FLIPLINE(pipe), trans_vrr_flipline); + intel_de_write(dev_priv, TRANS_PUSH(pipe), trans_push); + + drm_dbg(&dev_priv->drm, "Enabling VRR on pipe (%c)\n", pipe_name(pipe)); + drm_dbg(&dev_priv->drm, "VRR Parameters: Vblank - Min = %d, Max = %d Flipline Count = %d, CTL Reg = 0x%08x, TRANS PUSH reg = 0x%08x", + crtc_state->vrr.vtotalmin, crtc_state->vrr.vtotalmax, + crtc_state->vrr.vtotalmin, trans_vrr_ctl, + trans_push); +} + diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h index 1e6fe8fe92ec..05d982d6fbae 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.h +++ b/drivers/gpu/drm/i915/display/intel_vrr.h @@ -17,5 +17,7 @@ struct intel_dp; bool intel_is_vrr_capable(struct drm_connector *connector); void intel_vrr_compute_config(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state); +void intel_vrr_enable(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state); #endif /* __INTEL_VRR_H__ */
This patch computes the VRR parameters from VRR crtc states and configures them in VRR registers during CRTC enable in the modeset enable sequence. 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 | 5 ++++ drivers/gpu/drm/i915/display/intel_vrr.c | 38 ++++++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_vrr.h | 2 ++ 3 files changed, 45 insertions(+)