On 2020-08-20 at 22:53:53 +0530, José Roberto de Souza wrote: > Changes in the configuration could cause PSR to be compatible and > enabled so driver must also be able to disable DRRS when doing > fastsets. > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/209 > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/173 > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/209 > Cc: Srinivas K <srinivasx.k@xxxxxxxxx> > Cc: Hariom Pandey <hariom.pandey@xxxxxxxxx> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> overall patch looks goood to me, > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 2 +- > drivers/gpu/drm/i915/display/intel_dp.c | 84 +++++++++++++++++++----- > drivers/gpu/drm/i915/display/intel_dp.h | 2 + > 3 files changed, 71 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index de5b216561d8..ff05a852417c 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -4012,7 +4012,7 @@ static void intel_ddi_update_pipe_dp(struct intel_atomic_state *state, > > intel_psr_update(intel_dp, crtc_state, conn_state); > intel_dp_set_infoframes(encoder, true, crtc_state, conn_state); > - intel_edp_drrs_enable(intel_dp, crtc_state); > + intel_edp_drrs_update(intel_dp, crtc_state); > > intel_panel_update_backlight(state, encoder, crtc_state, conn_state); > } > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 3bf50b1ae983..de2c9851395d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -2576,9 +2576,9 @@ intel_dp_compute_hdr_metadata_infoframe_sdp(struct intel_dp *intel_dp, > } > > static void > -intel_dp_drrs_compute_config(struct intel_dp *intel_dp, > - struct intel_crtc_state *pipe_config, > - int output_bpp, bool constant_n) > +intel_dp_compute_drrs(struct intel_dp *intel_dp, > + struct intel_crtc_state *pipe_config, > + int output_bpp, bool constant_n) intel_dp_drrs_compute_config looks a better name which u have introduced in patch 1 of this series. any reason to change the function name in second patch ? > { > struct intel_connector *intel_connector = intel_dp->attached_connector; > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > @@ -2586,8 +2586,8 @@ intel_dp_drrs_compute_config(struct intel_dp *intel_dp, > /* > * DRRS and PSR can't be enable together, so giving preference to PSR > * as it allows more power-savings by complete shutting down display, > - * so to guarantee this, intel_dp_drrs_compute_config() must be called > - * after intel_psr_compute_config(). > + * so to guarantee this, intel_dp_compute_drrs() must be called after > + * intel_psr_compute_config(). > */ > if (pipe_config->has_psr) > return; > @@ -2688,8 +2688,7 @@ intel_dp_compute_config(struct intel_encoder *encoder, > intel_dp_set_clock(encoder, pipe_config); > > intel_psr_compute_config(intel_dp, pipe_config); > - intel_dp_drrs_compute_config(intel_dp, pipe_config, output_bpp, > - constant_n); > + intel_dp_compute_drrs(intel_dp, pipe_config, output_bpp, constant_n); > intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state); > intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp, pipe_config, conn_state); > > @@ -7736,6 +7735,15 @@ static void intel_dp_set_drrs_state(struct drm_i915_private *dev_priv, > refresh_rate); > } > > +static void > +intel_edp_drrs_enable_locked(struct intel_dp *intel_dp) > +{ > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + > + dev_priv->drrs.busy_frontbuffer_bits = 0; > + dev_priv->drrs.dp = intel_dp; > +} > + > /** > * intel_edp_drrs_enable - init drrs struct if supported > * @intel_dp: DP struct > @@ -7752,19 +7760,34 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp, > return; > > mutex_lock(&dev_priv->drrs.mutex); > + > if (dev_priv->drrs.dp) { > - drm_dbg_kms(&dev_priv->drm, "DRRS already enabled\n"); > + drm_warn(&dev_priv->drm, "DRRS already enabled\n"); > goto unlock; > } > > - dev_priv->drrs.busy_frontbuffer_bits = 0; > - > - dev_priv->drrs.dp = intel_dp; > + intel_edp_drrs_enable_locked(intel_dp); > > unlock: > mutex_unlock(&dev_priv->drrs.mutex); > } > > +static void > +intel_edp_drrs_disable_locked(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + > + if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) { > + int refresh; > + > + refresh = drm_mode_vrefresh(intel_dp->attached_connector->panel.fixed_mode); > + intel_dp_set_drrs_state(dev_priv, crtc_state, refresh); > + } > + > + dev_priv->drrs.dp = NULL; > +} > + > /** > * intel_edp_drrs_disable - Disable DRRS > * @intel_dp: DP struct > @@ -7785,16 +7808,45 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp, > return; > } > > - if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) > - intel_dp_set_drrs_state(dev_priv, old_crtc_state, > - drm_mode_vrefresh(intel_dp->attached_connector->panel.fixed_mode)); > - > - dev_priv->drrs.dp = NULL; > + intel_edp_drrs_disable_locked(intel_dp, old_crtc_state); > mutex_unlock(&dev_priv->drrs.mutex); > > cancel_delayed_work_sync(&dev_priv->drrs.work); > } > > +/** > + * intel_edp_drrs_update - Update DRRS state > + * @intel_dp: Intel DP > + * @crtc_state: new CRTC state > + * > + * This function will update DRRS states, disabling or enabling DRRS when > + * executing fastsets. For full modeset, intel_edp_drrs_disable() and > + * intel_edp_drrs_enable() should be called instead. > + */ > +void > +intel_edp_drrs_update(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + > + if (dev_priv->drrs.type != SEAMLESS_DRRS_SUPPORT) > + return; > + > + mutex_lock(&dev_priv->drrs.mutex); > + > + /* New state matches current one? */ > + if (crtc_state->has_drrs == !!dev_priv->drrs.dp) > + goto unlock; > + > + if (crtc_state->has_drrs) > + intel_edp_drrs_enable_locked(intel_dp); > + else > + intel_edp_drrs_disable_locked(intel_dp, crtc_state); > + > +unlock: > + mutex_unlock(&dev_priv->drrs.mutex); > +} > + > static void intel_edp_drrs_downclock_work(struct work_struct *work) > { > struct drm_i915_private *dev_priv = > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h > index b901ab850cbd..057b2c152cbd 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.h > +++ b/drivers/gpu/drm/i915/display/intel_dp.h > @@ -81,6 +81,8 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state); > void intel_edp_drrs_disable(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state); > +void intel_edp_drrs_update(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state); > void intel_edp_drrs_invalidate(struct drm_i915_private *dev_priv, > unsigned int frontbuffer_bits); > void intel_edp_drrs_flush(struct drm_i915_private *dev_priv, > -- > 2.28.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
