[Intel-gfx] [PATCH 2/2] drm/i915/drrs: Disable DRRS when needed in fastsets
Souza, Jose
jose.souza at intel.com
Mon Aug 24 17:31:00 UTC 2020
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 at intel.com
> > >
> > Cc: Hariom Pandey <
> > hariom.pandey at intel.com
> > >
> > Signed-off-by: José Roberto de Souza <
> > jose.souza at intel.com
> > >
>
> 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 at lists.freedesktop.org
> >
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
More information about the Intel-gfx
mailing list