Message ID | 20210825005840.170796-5-jose.souza@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Drop frontbuffer rendering support from Skylake and newer | expand |
On 8/25/21 3:58 AM, José Roberto de Souza wrote: > It started as a code style fix for the lines above 100 col but it > turned out to simplifications to intel_drrs_set_state(). > Now it receives the desired refresh rate type, high or low. > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_drrs.c | 60 ++++++++--------------- > 1 file changed, 21 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c > index 1aa9793521158..5eb5033242575 100644 > --- a/drivers/gpu/drm/i915/display/intel_drrs.c > +++ b/drivers/gpu/drm/i915/display/intel_drrs.c > @@ -91,7 +91,7 @@ intel_drrs_compute_config(struct intel_dp *intel_dp, > * intel_drrs_set_state - program registers for RR switch to take effect > * @dev_priv: i915 device > * @crtc_state: a pointer to the active intel_crtc_state > - * @refresh_rate: RR to be programmed > + * @refresh_type: high or low refresh rate to be programmed > * > * This function gets called when refresh rate (RR) has to be changed from > * one frequency to another. Switches can be between high and low RR > @@ -102,19 +102,13 @@ intel_drrs_compute_config(struct intel_dp *intel_dp, > */ > static void intel_drrs_set_state(struct drm_i915_private *dev_priv, > const struct intel_crtc_state *crtc_state, > - int refresh_rate) > + enum drrs_refresh_rate_type refresh_type) > { > struct intel_dp *intel_dp = dev_priv->drrs.dp; > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > - enum drrs_refresh_rate_type index = DRRS_HIGH_RR; > + struct drm_display_mode *mode; > > - if (refresh_rate <= 0) { > - drm_dbg_kms(&dev_priv->drm, > - "Refresh rate should be positive non-zero.\n"); > - return; > - } > - > - if (intel_dp == NULL) { > + if (!intel_dp) { > drm_dbg_kms(&dev_priv->drm, "DRRS not supported.\n"); > return; > } > @@ -130,15 +124,8 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, > return; > } > > - if (drm_mode_vrefresh(intel_dp->attached_connector->panel.downclock_mode) == > - refresh_rate) > - index = DRRS_LOW_RR; > - > - if (index == dev_priv->drrs.refresh_rate_type) { > - drm_dbg_kms(&dev_priv->drm, > - "DRRS requested for previously set RR...ignoring\n"); > + if (refresh_type == dev_priv->drrs.refresh_rate_type) > return; > - } > > if (!crtc_state->hw.active) { > drm_dbg_kms(&dev_priv->drm, > @@ -147,7 +134,7 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, > } > > if (DISPLAY_VER(dev_priv) >= 8 && !IS_CHERRYVIEW(dev_priv)) { > - switch (index) { > + switch (refresh_type) { > case DRRS_HIGH_RR: > intel_dp_set_m_n(crtc_state, M1_N1); > break; > @@ -164,7 +151,7 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, > u32 val; > > val = intel_de_read(dev_priv, reg); > - if (index > DRRS_HIGH_RR) { > + if (refresh_type == DRRS_LOW_RR) { > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > val |= PIPECONF_EDP_RR_MODE_SWITCH_VLV; > else > @@ -178,10 +165,14 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, > intel_de_write(dev_priv, reg, val); > } > > - dev_priv->drrs.refresh_rate_type = index; > + dev_priv->drrs.refresh_rate_type = refresh_type; > > + if (refresh_type == DRRS_LOW_RR) > + mode = intel_dp->attached_connector->panel.fixed_mode; > + else > + mode = intel_dp->attached_connector->panel.downclock_mode; For DRRS_LOW_RR refresh_type, panel.downclock_mode has to be used, and for DRR_HIGH_RR, panel.fixed_mode has to be used. It should be modified as follows. if (refresh_type == DRRS_LOW_RR) mode = intel_dp->attached_connector->panel.downclock_mode; else mode = intel_dp->attached_connector->panel.fixed_mode; Except for this, the rest of the code looks good to me. > drm_dbg_kms(&dev_priv->drm, "eDP Refresh Rate set to : %dHz\n", > - refresh_rate); > + drm_mode_vrefresh(mode)); > } > > static void > @@ -229,13 +220,7 @@ intel_drrs_disable_locked(struct intel_dp *intel_dp, > { > 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_drrs_set_state(dev_priv, crtc_state, refresh); > - } > - > + intel_drrs_set_state(dev_priv, crtc_state, DRRS_HIGH_RR); > dev_priv->drrs.dp = NULL; > } > > @@ -303,6 +288,7 @@ static void intel_drrs_downclock_work(struct work_struct *work) > struct drm_i915_private *dev_priv = > container_of(work, typeof(*dev_priv), drrs.work.work); > struct intel_dp *intel_dp; > + struct drm_crtc *crtc; > > mutex_lock(&dev_priv->drrs.mutex); > > @@ -319,12 +305,8 @@ static void intel_drrs_downclock_work(struct work_struct *work) > if (dev_priv->drrs.busy_frontbuffer_bits) > goto unlock; > > - if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR) { > - struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc; > - > - intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, > - drm_mode_vrefresh(intel_dp->attached_connector->panel.downclock_mode)); > - } > + crtc = dp_to_dig_port(intel_dp)->base.base.crtc; > + intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, DRRS_LOW_RR); > > unlock: > mutex_unlock(&dev_priv->drrs.mutex); > @@ -367,9 +349,9 @@ void intel_drrs_invalidate(struct drm_i915_private *dev_priv, > dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits; > > /* invalidate means busy screen hence upclock */ > - if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) > + if (frontbuffer_bits) > intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, > - drm_mode_vrefresh(intel_dp->attached_connector->panel.fixed_mode)); > + DRRS_HIGH_RR); > > mutex_unlock(&dev_priv->drrs.mutex); > } > @@ -413,9 +395,9 @@ void intel_drrs_flush(struct drm_i915_private *dev_priv, > dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits; > > /* flush means busy screen hence upclock */ > - if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) > + if (frontbuffer_bits) > intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, > - drm_mode_vrefresh(intel_dp->attached_connector->panel.fixed_mode)); > + DRRS_HIGH_RR); > > /* > * flush also means no more activity hence schedule downclock, if all >
On Thu, 2021-09-02 at 18:15 +0300, Gwan-gyeong Mun wrote: > > On 8/25/21 3:58 AM, José Roberto de Souza wrote: > > It started as a code style fix for the lines above 100 col but it > > turned out to simplifications to intel_drrs_set_state(). > > Now it receives the desired refresh rate type, high or low. > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_drrs.c | 60 ++++++++--------------- > > 1 file changed, 21 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c > > index 1aa9793521158..5eb5033242575 100644 > > --- a/drivers/gpu/drm/i915/display/intel_drrs.c > > +++ b/drivers/gpu/drm/i915/display/intel_drrs.c > > @@ -91,7 +91,7 @@ intel_drrs_compute_config(struct intel_dp *intel_dp, > > * intel_drrs_set_state - program registers for RR switch to take effect > > * @dev_priv: i915 device > > * @crtc_state: a pointer to the active intel_crtc_state > > - * @refresh_rate: RR to be programmed > > + * @refresh_type: high or low refresh rate to be programmed > > * > > * This function gets called when refresh rate (RR) has to be changed from > > * one frequency to another. Switches can be between high and low RR > > @@ -102,19 +102,13 @@ intel_drrs_compute_config(struct intel_dp *intel_dp, > > */ > > static void intel_drrs_set_state(struct drm_i915_private *dev_priv, > > const struct intel_crtc_state *crtc_state, > > - int refresh_rate) > > + enum drrs_refresh_rate_type refresh_type) > > { > > struct intel_dp *intel_dp = dev_priv->drrs.dp; > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > - enum drrs_refresh_rate_type index = DRRS_HIGH_RR; > > + struct drm_display_mode *mode; > > > > - if (refresh_rate <= 0) { > > - drm_dbg_kms(&dev_priv->drm, > > - "Refresh rate should be positive non-zero.\n"); > > - return; > > - } > > - > > - if (intel_dp == NULL) { > > + if (!intel_dp) { > > drm_dbg_kms(&dev_priv->drm, "DRRS not supported.\n"); > > return; > > } > > @@ -130,15 +124,8 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, > > return; > > } > > > > - if (drm_mode_vrefresh(intel_dp->attached_connector->panel.downclock_mode) == > > - refresh_rate) > > - index = DRRS_LOW_RR; > > - > > - if (index == dev_priv->drrs.refresh_rate_type) { > > - drm_dbg_kms(&dev_priv->drm, > > - "DRRS requested for previously set RR...ignoring\n"); > > + if (refresh_type == dev_priv->drrs.refresh_rate_type) > > return; > > - } > > > > if (!crtc_state->hw.active) { > > drm_dbg_kms(&dev_priv->drm, > > @@ -147,7 +134,7 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, > > } > > > > if (DISPLAY_VER(dev_priv) >= 8 && !IS_CHERRYVIEW(dev_priv)) { > > - switch (index) { > > + switch (refresh_type) { > > case DRRS_HIGH_RR: > > intel_dp_set_m_n(crtc_state, M1_N1); > > break; > > @@ -164,7 +151,7 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, > > u32 val; > > > > val = intel_de_read(dev_priv, reg); > > - if (index > DRRS_HIGH_RR) { > > + if (refresh_type == DRRS_LOW_RR) { > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > val |= PIPECONF_EDP_RR_MODE_SWITCH_VLV; > > else > > @@ -178,10 +165,14 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, > > intel_de_write(dev_priv, reg, val); > > } > > > > - dev_priv->drrs.refresh_rate_type = index; > > + dev_priv->drrs.refresh_rate_type = refresh_type; > > > > + if (refresh_type == DRRS_LOW_RR) > > + mode = intel_dp->attached_connector->panel.fixed_mode; > > + else > > + mode = intel_dp->attached_connector->panel.downclock_mode; > For DRRS_LOW_RR refresh_type, panel.downclock_mode has to be used, and > for DRR_HIGH_RR, panel.fixed_mode has to be used. > It should be modified as follows. > if (refresh_type == DRRS_LOW_RR) > mode = intel_dp->attached_connector->panel.downclock_mode; > else > mode = intel_dp->attached_connector->panel.fixed_mode; > > Except for this, the rest of the code looks good to me. Thanks for catching this, will fix it. > > > > drm_dbg_kms(&dev_priv->drm, "eDP Refresh Rate set to : %dHz\n", > > - refresh_rate); > > + drm_mode_vrefresh(mode)); > > } > > > > static void > > @@ -229,13 +220,7 @@ intel_drrs_disable_locked(struct intel_dp *intel_dp, > > { > > 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_drrs_set_state(dev_priv, crtc_state, refresh); > > - } > > - > > + intel_drrs_set_state(dev_priv, crtc_state, DRRS_HIGH_RR); > > dev_priv->drrs.dp = NULL; > > } > > > > @@ -303,6 +288,7 @@ static void intel_drrs_downclock_work(struct work_struct *work) > > struct drm_i915_private *dev_priv = > > container_of(work, typeof(*dev_priv), drrs.work.work); > > struct intel_dp *intel_dp; > > + struct drm_crtc *crtc; > > > > mutex_lock(&dev_priv->drrs.mutex); > > > > @@ -319,12 +305,8 @@ static void intel_drrs_downclock_work(struct work_struct *work) > > if (dev_priv->drrs.busy_frontbuffer_bits) > > goto unlock; > > > > - if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR) { > > - struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc; > > - > > - intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, > > - drm_mode_vrefresh(intel_dp->attached_connector->panel.downclock_mode)); > > - } > > + crtc = dp_to_dig_port(intel_dp)->base.base.crtc; > > + intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, DRRS_LOW_RR); > > > > unlock: > > mutex_unlock(&dev_priv->drrs.mutex); > > @@ -367,9 +349,9 @@ void intel_drrs_invalidate(struct drm_i915_private *dev_priv, > > dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits; > > > > /* invalidate means busy screen hence upclock */ > > - if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) > > + if (frontbuffer_bits) > > intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, > > - drm_mode_vrefresh(intel_dp->attached_connector->panel.fixed_mode)); > > + DRRS_HIGH_RR); > > > > mutex_unlock(&dev_priv->drrs.mutex); > > } > > @@ -413,9 +395,9 @@ void intel_drrs_flush(struct drm_i915_private *dev_priv, > > dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits; > > > > /* flush means busy screen hence upclock */ > > - if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) > > + if (frontbuffer_bits) > > intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, > > - drm_mode_vrefresh(intel_dp->attached_connector->panel.fixed_mode)); > > + DRRS_HIGH_RR); > > > > /* > > * flush also means no more activity hence schedule downclock, if all > >
diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c index 1aa9793521158..5eb5033242575 100644 --- a/drivers/gpu/drm/i915/display/intel_drrs.c +++ b/drivers/gpu/drm/i915/display/intel_drrs.c @@ -91,7 +91,7 @@ intel_drrs_compute_config(struct intel_dp *intel_dp, * intel_drrs_set_state - program registers for RR switch to take effect * @dev_priv: i915 device * @crtc_state: a pointer to the active intel_crtc_state - * @refresh_rate: RR to be programmed + * @refresh_type: high or low refresh rate to be programmed * * This function gets called when refresh rate (RR) has to be changed from * one frequency to another. Switches can be between high and low RR @@ -102,19 +102,13 @@ intel_drrs_compute_config(struct intel_dp *intel_dp, */ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, const struct intel_crtc_state *crtc_state, - int refresh_rate) + enum drrs_refresh_rate_type refresh_type) { struct intel_dp *intel_dp = dev_priv->drrs.dp; struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); - enum drrs_refresh_rate_type index = DRRS_HIGH_RR; + struct drm_display_mode *mode; - if (refresh_rate <= 0) { - drm_dbg_kms(&dev_priv->drm, - "Refresh rate should be positive non-zero.\n"); - return; - } - - if (intel_dp == NULL) { + if (!intel_dp) { drm_dbg_kms(&dev_priv->drm, "DRRS not supported.\n"); return; } @@ -130,15 +124,8 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, return; } - if (drm_mode_vrefresh(intel_dp->attached_connector->panel.downclock_mode) == - refresh_rate) - index = DRRS_LOW_RR; - - if (index == dev_priv->drrs.refresh_rate_type) { - drm_dbg_kms(&dev_priv->drm, - "DRRS requested for previously set RR...ignoring\n"); + if (refresh_type == dev_priv->drrs.refresh_rate_type) return; - } if (!crtc_state->hw.active) { drm_dbg_kms(&dev_priv->drm, @@ -147,7 +134,7 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, } if (DISPLAY_VER(dev_priv) >= 8 && !IS_CHERRYVIEW(dev_priv)) { - switch (index) { + switch (refresh_type) { case DRRS_HIGH_RR: intel_dp_set_m_n(crtc_state, M1_N1); break; @@ -164,7 +151,7 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, u32 val; val = intel_de_read(dev_priv, reg); - if (index > DRRS_HIGH_RR) { + if (refresh_type == DRRS_LOW_RR) { if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) val |= PIPECONF_EDP_RR_MODE_SWITCH_VLV; else @@ -178,10 +165,14 @@ static void intel_drrs_set_state(struct drm_i915_private *dev_priv, intel_de_write(dev_priv, reg, val); } - dev_priv->drrs.refresh_rate_type = index; + dev_priv->drrs.refresh_rate_type = refresh_type; + if (refresh_type == DRRS_LOW_RR) + mode = intel_dp->attached_connector->panel.fixed_mode; + else + mode = intel_dp->attached_connector->panel.downclock_mode; drm_dbg_kms(&dev_priv->drm, "eDP Refresh Rate set to : %dHz\n", - refresh_rate); + drm_mode_vrefresh(mode)); } static void @@ -229,13 +220,7 @@ intel_drrs_disable_locked(struct intel_dp *intel_dp, { 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_drrs_set_state(dev_priv, crtc_state, refresh); - } - + intel_drrs_set_state(dev_priv, crtc_state, DRRS_HIGH_RR); dev_priv->drrs.dp = NULL; } @@ -303,6 +288,7 @@ static void intel_drrs_downclock_work(struct work_struct *work) struct drm_i915_private *dev_priv = container_of(work, typeof(*dev_priv), drrs.work.work); struct intel_dp *intel_dp; + struct drm_crtc *crtc; mutex_lock(&dev_priv->drrs.mutex); @@ -319,12 +305,8 @@ static void intel_drrs_downclock_work(struct work_struct *work) if (dev_priv->drrs.busy_frontbuffer_bits) goto unlock; - if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR) { - struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc; - - intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, - drm_mode_vrefresh(intel_dp->attached_connector->panel.downclock_mode)); - } + crtc = dp_to_dig_port(intel_dp)->base.base.crtc; + intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, DRRS_LOW_RR); unlock: mutex_unlock(&dev_priv->drrs.mutex); @@ -367,9 +349,9 @@ void intel_drrs_invalidate(struct drm_i915_private *dev_priv, dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits; /* invalidate means busy screen hence upclock */ - if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) + if (frontbuffer_bits) intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, - drm_mode_vrefresh(intel_dp->attached_connector->panel.fixed_mode)); + DRRS_HIGH_RR); mutex_unlock(&dev_priv->drrs.mutex); } @@ -413,9 +395,9 @@ void intel_drrs_flush(struct drm_i915_private *dev_priv, dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits; /* flush means busy screen hence upclock */ - if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) + if (frontbuffer_bits) intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, - drm_mode_vrefresh(intel_dp->attached_connector->panel.fixed_mode)); + DRRS_HIGH_RR); /* * flush also means no more activity hence schedule downclock, if all
It started as a code style fix for the lines above 100 col but it turned out to simplifications to intel_drrs_set_state(). Now it receives the desired refresh rate type, high or low. Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_drrs.c | 60 ++++++++--------------- 1 file changed, 21 insertions(+), 39 deletions(-)