Re: [Intel-gfx] [PATCH v2 5/8] drm/i915/display: Share code between intel_drrs_flush and intel_drrs_invalidate

["Souza, Jose" <jose.souza@intel.com>]

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);
> >   }
> >   
> >   /**
> >