| Message ID | 20200820172353.15773-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Compute has_drrs after compute has_psr | expand |
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@intel.com> > Cc: Hariom Pandey <hariom.pandey@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > 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 > + > + 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. > 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. with all of above, Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com> > + 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com > > > > > Cc: Hariom Pandey < > > hariom.pandey@intel.com > > > > > Signed-off-by: José Roberto de Souza < > > jose.souza@intel.com > > > > > --- > > 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@intel.com > > > > + 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@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
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; + + 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; 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"); + 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) {
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@intel.com> Cc: Hariom Pandey <hariom.pandey@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 52 +++++++++++++++---------- 1 file changed, 31 insertions(+), 21 deletions(-)