[Intel-gfx] [PATCH v3 1/3] drm/i915/display: Compute has_drrs after compute has_psr
Souza, Jose
jose.souza at intel.com
Thu Aug 27 16:51:57 UTC 2020
On Thu, 2020-08-27 at 17:39 +0530, Anshuman Gupta wrote:
> On 2020-08-26 at 22:17:38 +0530, Souza, Jose wrote:
> > On Wed, 2020-08-26 at 12:56 +0530, Anshuman Gupta wrote:
> > > On 2020-08-25 at 10:13:29 -0700, José Roberto de Souza wrote:
> > > > 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, it should compute DRRS state after compute PSR.
> > > >
> > > > Cc: Srinivas K <
> > > > srinivasx.k at intel.com
> > > >
> > > >
> > > > Cc: Hariom Pandey <
> > > > hariom.pandey at intel.com
> > > >
> > > >
> > > > Reviewed-by: Anshuman Gupta <
> > > > anshuman.gupta at intel.com
> > > >
> > > >
> > > > Signed-off-by: José Roberto de Souza <
> > > > jose.souza at intel.com
> > > >
> > > >
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_dp.c | 52 +++++++++++++++----------
> > > > 1 file changed, 32 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 79c27f91f42c..a08d03c61b02 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -2575,6 +2575,34 @@ intel_dp_compute_hdr_metadata_infoframe_sdp(struct intel_dp *intel_dp,
> > > > intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_GAMUT_METADATA);
> > > > }
> > > >
> > > > +static void
> > > > +intel_dp_drrs_compute_config(struct intel_dp *intel_dp,
> > > > + struct intel_crtc_state *pipe_config,
> > > > + int output_bpp, bool constant_n)
> > > > +{
> > > > + struct intel_connector *intel_connector = intel_dp->attached_connector;
> > > > + struct drm_i915_private *dev_priv = dp_to_i915(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().
> > > > + */
> > > > + if (pipe_config->has_psr)
> > > > + return;
> > > > +
> > > > + if (!intel_connector->panel.downclock_mode ||
> > > > + dev_priv->drrs.type != SEAMLESS_DRRS_SUPPORT)
> > > > + return;
> > > > +
> > > > + pipe_config->has_drrs = true;
> > > > + intel_link_compute_m_n(output_bpp, pipe_config->lane_count,
> > > > + intel_connector->panel.downclock_mode->clock,
> > > > + pipe_config->port_clock, &pipe_config->dp_m2_n2,
> > > > + constant_n, pipe_config->fec_enable);
> > > > +}
> > > > +
> > > > int
> > > > intel_dp_compute_config(struct intel_encoder *encoder,
> > > > struct intel_crtc_state *pipe_config,
> > > > @@ -2605,7 +2633,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - pipe_config->has_drrs = false;
> > >
> > > IMHO this assignment is required, i was thinking a case, when a crtc is attached to more than
> > > one connector, suppose first eDP connector supports DRRS from panel.downclock_mode and
> > > drrs.type but another DP connector won't support it in that case has_drrs will be still
> > > true.
> > > Please correct me if i am wrong here.
> >
> > i915 only supports one connector per pipe/CRTC, if that was the case all other flags in intel_crtc_state would also have the same behaviour as
> > has_drrs.
>
> Actually once on kabylake i had witnessed this use case when a CRTC was attached to multiple connectors, AFAIU theortically also
> it seems possible as intel_modeset_pipe_config iterate for each connector in state for a given crtc i.e (connector_state->crtc == crtc)
> and call encoder->compute_config().
> If you are sure above case can't happen for drrs you can use my RB.
> Applicable for [PATCH 3/3] of this series,
> Reviewed-by: Anshuman Gupta <
> anshuman.gupta at intel.com
PSR would have a lot of issues if such scenario was possible.Patches pushed, thanks for the review.
> >
> > > Thanks,
> > > Anshuman Gupta.
> > > > if (!intel_dp_port_has_audio(dev_priv, port))
> > > > pipe_config->has_audio = false;
> > > > else if (intel_conn_state->force_audio == HDMI_AUDIO_AUTO)
> > > > @@ -2657,21 +2684,12 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > > > &pipe_config->dp_m_n,
> > > > constant_n, pipe_config->fec_enable);
> > > >
> > > > - if (intel_connector->panel.downclock_mode != NULL &&
> > > > - dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
> > > > - pipe_config->has_drrs = true;
> > > > - intel_link_compute_m_n(output_bpp,
> > > > - pipe_config->lane_count,
> > > > - intel_connector->panel.downclock_mode->clock,
> > > > - pipe_config->port_clock,
> > > > - &pipe_config->dp_m2_n2,
> > > > - constant_n, pipe_config->fec_enable);
> > > > - }
> > > > -
> > > > if (!HAS_DDI(dev_priv))
> > > > 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_vsc_sdp(intel_dp, pipe_config, conn_state);
> > > > intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp, pipe_config, conn_state);
> > > >
> > > > @@ -7730,16 +7748,10 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp,
> > > > {
> > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > >
> > > > - if (!crtc_state->has_drrs) {
> > > > - drm_dbg_kms(&dev_priv->drm, "Panel doesn't support DRRS\n");
> > > > + if (!crtc_state->has_drrs)
> > > > return;
> > > > - }
> > > >
> > > > - if (dev_priv->psr.enabled) {
> > > > - drm_dbg_kms(&dev_priv->drm,
> > > > - "PSR enabled. Not enabling DRRS.\n");
> > > > - return;
> > > > - }
> > > > + drm_dbg_kms(&dev_priv->drm, "Enabling DRRS\n");
> > > >
> > > > mutex_lock(&dev_priv->drrs.mutex);
> > > > if (dev_priv->drrs.dp) {
> > > > --
> > > > 2.28.0
> > > >
More information about the Intel-gfx
mailing list