On Mon, 2020-08-24 at 16:24 +0530, Anshuman Gupta wrote:
> On 2020-08-20 at 10:23:52 -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@xxxxxxxxx
> > >
> > Cc: Hariom Pandey <
> > hariom.pandey@xxxxxxxxx
> > >
> > Signed-off-by: José Roberto de Souza <
> > jose.souza@xxxxxxxxx
> > >
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 52 +++++++++++++++----------
> > 1 file changed, 31 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 79c27f91f42c..3bf50b1ae983 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;
>
> In cases when psr is supported but is not enabled due to any psr error, drrs can be useful.
> AFAIU has_psr is false in cases psr is disabled due any PSR error, this should be fine for such case
sink_not_reliable is checked in intel_psr_compute_config() so after a PSR error in the next time fastset(in the next patch) DRRS will be enabled.
> > +
> > + 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;
>
> May be u can retain it for better readability.
The whole struct is zeroed, other features are not set false if not supported.
> > 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,8 @@ 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");
>
> IMHO this print is useful.
Okay but will change the message to DRRS enabled, we already know that the panel support DRRS in intel_dp_drrs_init().
> with all of above,
> Reviewed-by: Anshuman Gupta <
> anshuman.gupta@xxxxxxxxx
> >
> > + if (!crtc_state->has_drrs)
> > return;
> > - }
> > -
> > - if (dev_priv->psr.enabled) {
> > - drm_dbg_kms(&dev_priv->drm,
> > - "PSR enabled. Not enabling DRRS.\n");
> > - return;
> > - }
> >
> > mutex_lock(&dev_priv->drrs.mutex);
> > if (dev_priv->drrs.dp) {
> > --
> > 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
