| Message ID | 20210825005840.170796-6-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: > Both functions are pretty much equal, with minor changes that can be > handled by a single parameter. > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_drrs.c | 82 +++++++++-------------- > 1 file changed, 32 insertions(+), 50 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c > index 5eb5033242575..8583da4e82434 100644 > --- a/drivers/gpu/drm/i915/display/intel_drrs.c > +++ b/drivers/gpu/drm/i915/display/intel_drrs.c > @@ -312,18 +312,9 @@ static void intel_drrs_downclock_work(struct work_struct *work) > mutex_unlock(&dev_priv->drrs.mutex); > } > > -/** > - * intel_drrs_invalidate - Disable Idleness DRRS > - * @dev_priv: i915 device > - * @frontbuffer_bits: frontbuffer plane tracking bits > - * > - * This function gets called everytime rendering on the given planes start. > - * Hence DRRS needs to be Upclocked, i.e. (LOW_RR -> HIGH_RR). > - * > - * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits. > - */ > -void intel_drrs_invalidate(struct drm_i915_private *dev_priv, > - unsigned int frontbuffer_bits) > +static void intel_drrs_frontbuffer_update(struct drm_i915_private *dev_priv, > + unsigned int frontbuffer_bits, > + bool invalidate) > { > struct intel_dp *intel_dp; > struct drm_crtc *crtc; > @@ -346,16 +337,42 @@ void intel_drrs_invalidate(struct drm_i915_private *dev_priv, > pipe = to_intel_crtc(crtc)->pipe; > > frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); > - dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits; > + if (invalidate) > + dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits; > + else > + dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits; > > - /* invalidate means busy screen hence upclock */ > + /* flush/invalidate means busy screen hence upclock */ > if (frontbuffer_bits) > intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, > DRRS_HIGH_RR); > > + /* > + * flush also means no more activity hence schedule downclock, if all > + * other fbs are quiescent too > + */ > + if (!dev_priv->drrs.busy_frontbuffer_bits) > + schedule_delayed_work(&dev_priv->drrs.work, > + msecs_to_jiffies(1000)); This line of code was previously only used in the intel_drrs_flush() function, In the case of the intel_drrs_invalidate() call scenario, it is not confirmed whether dev_priv->drrs.busy_frontbuffer_bits is always true to me. If it's not always true, it looks like you also need to check for "not invalidate". Please ignore this comment if I'm wrong. > mutex_unlock(&dev_priv->drrs.mutex); > } > > +/** > + * intel_drrs_invalidate - Disable Idleness DRRS > + * @dev_priv: i915 device > + * @frontbuffer_bits: frontbuffer plane tracking bits > + * > + * This function gets called everytime rendering on the given planes start. > + * Hence DRRS needs to be Upclocked, i.e. (LOW_RR -> HIGH_RR). > + * > + * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits. > + */ > +void intel_drrs_invalidate(struct drm_i915_private *dev_priv, > + unsigned int frontbuffer_bits) > +{ > + intel_drrs_frontbuffer_update(dev_priv, frontbuffer_bits, true); > +} > + > /** > * intel_drrs_flush - Restart Idleness DRRS > * @dev_priv: i915 device > @@ -371,42 +388,7 @@ void intel_drrs_invalidate(struct drm_i915_private *dev_priv, > void intel_drrs_flush(struct drm_i915_private *dev_priv, > unsigned int frontbuffer_bits) > { > - struct intel_dp *intel_dp; > - struct drm_crtc *crtc; > - enum pipe pipe; > - > - if (dev_priv->drrs.type == DRRS_NOT_SUPPORTED) > - return; > - > - cancel_delayed_work(&dev_priv->drrs.work); > - > - mutex_lock(&dev_priv->drrs.mutex); > - > - intel_dp = dev_priv->drrs.dp; > - if (!intel_dp) { > - mutex_unlock(&dev_priv->drrs.mutex); > - return; > - } > - > - crtc = dp_to_dig_port(intel_dp)->base.base.crtc; > - pipe = to_intel_crtc(crtc)->pipe; > - > - frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); > - dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits; > - > - /* flush means busy screen hence upclock */ > - if (frontbuffer_bits) > - intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, > - DRRS_HIGH_RR); > - > - /* > - * flush also means no more activity hence schedule downclock, if all > - * other fbs are quiescent too > - */ > - if (!dev_priv->drrs.busy_frontbuffer_bits) > - schedule_delayed_work(&dev_priv->drrs.work, > - msecs_to_jiffies(1000)); > - mutex_unlock(&dev_priv->drrs.mutex); > + intel_drrs_frontbuffer_update(dev_priv, frontbuffer_bits, false); > } > > /** >
On Thu, 2021-09-02 at 18:57 +0300, Gwan-gyeong Mun wrote: > > On 8/25/21 3:58 AM, José Roberto de Souza wrote: > > Both functions are pretty much equal, with minor changes that can be > > handled by a single parameter. > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_drrs.c | 82 +++++++++-------------- > > 1 file changed, 32 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c > > index 5eb5033242575..8583da4e82434 100644 > > --- a/drivers/gpu/drm/i915/display/intel_drrs.c > > +++ b/drivers/gpu/drm/i915/display/intel_drrs.c > > @@ -312,18 +312,9 @@ static void intel_drrs_downclock_work(struct work_struct *work) > > mutex_unlock(&dev_priv->drrs.mutex); > > } > > > > -/** > > - * intel_drrs_invalidate - Disable Idleness DRRS > > - * @dev_priv: i915 device > > - * @frontbuffer_bits: frontbuffer plane tracking bits > > - * > > - * This function gets called everytime rendering on the given planes start. > > - * Hence DRRS needs to be Upclocked, i.e. (LOW_RR -> HIGH_RR). > > - * > > - * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits. > > - */ > > -void intel_drrs_invalidate(struct drm_i915_private *dev_priv, > > - unsigned int frontbuffer_bits) > > +static void intel_drrs_frontbuffer_update(struct drm_i915_private *dev_priv, > > + unsigned int frontbuffer_bits, > > + bool invalidate) > > { > > struct intel_dp *intel_dp; > > struct drm_crtc *crtc; > > @@ -346,16 +337,42 @@ void intel_drrs_invalidate(struct drm_i915_private *dev_priv, > > pipe = to_intel_crtc(crtc)->pipe; > > > > frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); > > - dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits; > > + if (invalidate) > > + dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits; > > + else > > + dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits; > > > > - /* invalidate means busy screen hence upclock */ > > + /* flush/invalidate means busy screen hence upclock */ > > if (frontbuffer_bits) > > intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, > > DRRS_HIGH_RR); > > > > + /* > > + * flush also means no more activity hence schedule downclock, if all > > + * other fbs are quiescent too > > + */ > > + if (!dev_priv->drrs.busy_frontbuffer_bits) > > + schedule_delayed_work(&dev_priv->drrs.work, > > + msecs_to_jiffies(1000)); > This line of code was previously only used in the intel_drrs_flush() > function, > In the case of the intel_drrs_invalidate() call scenario, it is not > confirmed whether dev_priv->drrs.busy_frontbuffer_bits is always true to > me. If it's not always true, it looks like you also need to check for > "not invalidate". > Please ignore this comment if I'm wrong. Huum yeah if there is a invalidate in another pipe that could delay the work to be executed or execute it without needing it. Will change it to: if (!invalidate && !dev_priv->drrs.busy_frontbuffer_bit) It should handle this cases above. Thanks for the catch again. > > > mutex_unlock(&dev_priv->drrs.mutex); > > } > > > > +/** > > + * intel_drrs_invalidate - Disable Idleness DRRS > > + * @dev_priv: i915 device > > + * @frontbuffer_bits: frontbuffer plane tracking bits > > + * > > + * This function gets called everytime rendering on the given planes start. > > + * Hence DRRS needs to be Upclocked, i.e. (LOW_RR -> HIGH_RR). > > + * > > + * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits. > > + */ > > +void intel_drrs_invalidate(struct drm_i915_private *dev_priv, > > + unsigned int frontbuffer_bits) > > +{ > > + intel_drrs_frontbuffer_update(dev_priv, frontbuffer_bits, true); > > +} > > + > > /** > > * intel_drrs_flush - Restart Idleness DRRS > > * @dev_priv: i915 device > > @@ -371,42 +388,7 @@ void intel_drrs_invalidate(struct drm_i915_private *dev_priv, > > void intel_drrs_flush(struct drm_i915_private *dev_priv, > > unsigned int frontbuffer_bits) > > { > > - struct intel_dp *intel_dp; > > - struct drm_crtc *crtc; > > - enum pipe pipe; > > - > > - if (dev_priv->drrs.type == DRRS_NOT_SUPPORTED) > > - return; > > - > > - cancel_delayed_work(&dev_priv->drrs.work); > > - > > - mutex_lock(&dev_priv->drrs.mutex); > > - > > - intel_dp = dev_priv->drrs.dp; > > - if (!intel_dp) { > > - mutex_unlock(&dev_priv->drrs.mutex); > > - return; > > - } > > - > > - crtc = dp_to_dig_port(intel_dp)->base.base.crtc; > > - pipe = to_intel_crtc(crtc)->pipe; > > - > > - frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); > > - dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits; > > - > > - /* flush means busy screen hence upclock */ > > - if (frontbuffer_bits) > > - intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, > > - DRRS_HIGH_RR); > > - > > - /* > > - * flush also means no more activity hence schedule downclock, if all > > - * other fbs are quiescent too > > - */ > > - if (!dev_priv->drrs.busy_frontbuffer_bits) > > - schedule_delayed_work(&dev_priv->drrs.work, > > - msecs_to_jiffies(1000)); > > - mutex_unlock(&dev_priv->drrs.mutex); > > + intel_drrs_frontbuffer_update(dev_priv, frontbuffer_bits, false); > > } > > > > /** > >
diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c index 5eb5033242575..8583da4e82434 100644 --- a/drivers/gpu/drm/i915/display/intel_drrs.c +++ b/drivers/gpu/drm/i915/display/intel_drrs.c @@ -312,18 +312,9 @@ static void intel_drrs_downclock_work(struct work_struct *work) mutex_unlock(&dev_priv->drrs.mutex); } -/** - * intel_drrs_invalidate - Disable Idleness DRRS - * @dev_priv: i915 device - * @frontbuffer_bits: frontbuffer plane tracking bits - * - * This function gets called everytime rendering on the given planes start. - * Hence DRRS needs to be Upclocked, i.e. (LOW_RR -> HIGH_RR). - * - * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits. - */ -void intel_drrs_invalidate(struct drm_i915_private *dev_priv, - unsigned int frontbuffer_bits) +static void intel_drrs_frontbuffer_update(struct drm_i915_private *dev_priv, + unsigned int frontbuffer_bits, + bool invalidate) { struct intel_dp *intel_dp; struct drm_crtc *crtc; @@ -346,16 +337,42 @@ void intel_drrs_invalidate(struct drm_i915_private *dev_priv, pipe = to_intel_crtc(crtc)->pipe; frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); - dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits; + if (invalidate) + dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits; + else + dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits; - /* invalidate means busy screen hence upclock */ + /* flush/invalidate means busy screen hence upclock */ if (frontbuffer_bits) intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, DRRS_HIGH_RR); + /* + * flush also means no more activity hence schedule downclock, if all + * other fbs are quiescent too + */ + if (!dev_priv->drrs.busy_frontbuffer_bits) + schedule_delayed_work(&dev_priv->drrs.work, + msecs_to_jiffies(1000)); mutex_unlock(&dev_priv->drrs.mutex); } +/** + * intel_drrs_invalidate - Disable Idleness DRRS + * @dev_priv: i915 device + * @frontbuffer_bits: frontbuffer plane tracking bits + * + * This function gets called everytime rendering on the given planes start. + * Hence DRRS needs to be Upclocked, i.e. (LOW_RR -> HIGH_RR). + * + * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits. + */ +void intel_drrs_invalidate(struct drm_i915_private *dev_priv, + unsigned int frontbuffer_bits) +{ + intel_drrs_frontbuffer_update(dev_priv, frontbuffer_bits, true); +} + /** * intel_drrs_flush - Restart Idleness DRRS * @dev_priv: i915 device @@ -371,42 +388,7 @@ void intel_drrs_invalidate(struct drm_i915_private *dev_priv, void intel_drrs_flush(struct drm_i915_private *dev_priv, unsigned int frontbuffer_bits) { - struct intel_dp *intel_dp; - struct drm_crtc *crtc; - enum pipe pipe; - - if (dev_priv->drrs.type == DRRS_NOT_SUPPORTED) - return; - - cancel_delayed_work(&dev_priv->drrs.work); - - mutex_lock(&dev_priv->drrs.mutex); - - intel_dp = dev_priv->drrs.dp; - if (!intel_dp) { - mutex_unlock(&dev_priv->drrs.mutex); - return; - } - - crtc = dp_to_dig_port(intel_dp)->base.base.crtc; - pipe = to_intel_crtc(crtc)->pipe; - - frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); - dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits; - - /* flush means busy screen hence upclock */ - if (frontbuffer_bits) - intel_drrs_set_state(dev_priv, to_intel_crtc(crtc)->config, - DRRS_HIGH_RR); - - /* - * flush also means no more activity hence schedule downclock, if all - * other fbs are quiescent too - */ - if (!dev_priv->drrs.busy_frontbuffer_bits) - schedule_delayed_work(&dev_priv->drrs.work, - msecs_to_jiffies(1000)); - mutex_unlock(&dev_priv->drrs.mutex); + intel_drrs_frontbuffer_update(dev_priv, frontbuffer_bits, false); } /**
Both functions are pretty much equal, with minor changes that can be handled by a single parameter. Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_drrs.c | 82 +++++++++-------------- 1 file changed, 32 insertions(+), 50 deletions(-)