On Mon, 2020-08-24 at 14:21 +0530, Anshuman Gupta wrote: > 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 ? Thanks for catching this, changing back to the name in patch 1. > > { > > 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
