Re: [PATCH v2 3/3] drm/i915/psr: Use more PSR HW tracking.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Mar 13, 2018 at 12:58:59AM +0000, Souza, Jose wrote:
> On Mon, 2018-03-12 at 16:19 -0700, Pandiyan, Dhinakaran wrote:
> > On Mon, 2018-03-12 at 23:16 +0000, Souza, Jose wrote:
> > > What if FBC is disabled? Or FBC can not be activate by any of the
> > > reasons in intel_fbc_can_activate(). The hardware tracking would
> > > never
> > > trigger a PSR exit by it self?!
> > > 
> > 
> > Only frontbuffer tracking is tied to FBC, PSR exit on plane/pipe MMIO
> > writes, vblank enable should happen even without FBC.
> 
> Oh now I got it.
> 
> > 
> > 
> > 
> > > On Tue, 2018-03-06 at 19:34 -0800, Dhinakaran Pandiyan wrote:
> > > > From: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > 
> > > > So far we are using frontbuffer tracking for everything
> > > > and ignoring that PSR has a HW capable HW tracking for many
> > > > modern usages of GPU on Core platforms and newer Atom ones.
> > > > 
> > > > One reason for that is that we were trying to keep same
> > > > infrastructure in place for VLV/CHV than the rest of platforms.
> > > > But also because when this infrastructure was created
> > > > the front-buffer-tracking origin wasn't that good and stable
> > > > how it is today after Paulo reworked it to attend FBC cases.
> > > > 
> > > > However this PSR implementation without HW tracking died
> > > > on gen8LP. And newer platforms are starting to demand more HW
> > > > tracking specially with PSR2 cases in mind.
> > > > 
> > > > By disabling and re-enabling PSR totally every time we believe
> > > > someone is going to change the front buffer content we don't
> > > > allow PSR HW tracking to do this job and specially compromising
> > > > the whole idea of PSR2 case where the HW tracking detect only
> > > > the damaged area and do a partial screen update.
> > > > 
> > > > So, from now on, on the platforms that has hw_tracking let's
> > > > rely more on HW tracking.
> > > > 
> > > > This also is the case in used by other drivers and more validated
> > > > by SV teams. So I hope that this will lead us to less misterious
> > > > bugs.
> > > > 
> > > > v2: Only do this for platform that actually has hw tracking.
> > > > 
> > > > v3 from DK
> > > > Do this only for flips, small gradual changes are better.
> > > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Jim Bride <jim.bride@linux.intel.com>
> > > > Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > >
> 
> Reviewed-by: Jose Roberto de Souza <jose.souza@intel.com>

thanks. pushed to dinq

> 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> > > >  drivers/gpu/drm/i915/intel_drv.h         |  3 ++-
> > > >  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
> > > >  drivers/gpu/drm/i915/intel_psr.c         | 10 +++++++++-
> > > >  4 files changed, 13 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 7eec99d7fad4..6aab929f354c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -772,6 +772,7 @@ struct i915_psr {
> > > >  	bool y_cord_support;
> > > >  	bool colorimetry_support;
> > > >  	bool alpm;
> > > > +	bool has_hw_tracking;
> > > >  
> > > >  	void (*enable_source)(struct intel_dp *,
> > > >  			      const struct intel_crtc_state *);
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 1a7c5addcec3..fdf7dd100dfe 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1873,7 +1873,8 @@ void intel_psr_enable(struct intel_dp
> > > > *intel_dp,
> > > >  void intel_psr_disable(struct intel_dp *intel_dp,
> > > >  		      const struct intel_crtc_state
> > > > *old_crtc_state);
> > > >  void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> > > > -			  unsigned frontbuffer_bits);
> > > > +			  unsigned frontbuffer_bits,
> > > > +			  enum fb_op_origin origin);
> > > >  void intel_psr_flush(struct drm_i915_private *dev_priv,
> > > >  		     unsigned frontbuffer_bits,
> > > >  		     enum fb_op_origin origin);
> > > > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > > b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > > index 3a8d3d06c26a..7fff0a0eceb4 100644
> > > > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > > @@ -80,7 +80,7 @@ void __intel_fb_obj_invalidate(struct
> > > > drm_i915_gem_object *obj,
> > > >  	}
> > > >  
> > > >  	might_sleep();
> > > > -	intel_psr_invalidate(dev_priv, frontbuffer_bits);
> > > > +	intel_psr_invalidate(dev_priv, frontbuffer_bits,
> > > > origin);
> > > >  	intel_edp_drrs_invalidate(dev_priv, frontbuffer_bits);
> > > >  	intel_fbc_invalidate(dev_priv, frontbuffer_bits,
> > > > origin);
> > > >  }
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 05770790a4e9..16b77e19c28e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -948,6 +948,7 @@ void intel_psr_single_frame_update(struct
> > > > drm_i915_private *dev_priv,
> > > >   * intel_psr_invalidate - Invalidade PSR
> > > >   * @dev_priv: i915 device
> > > >   * @frontbuffer_bits: frontbuffer plane tracking bits
> > > > + * @origin: which operation caused the invalidate
> > > >   *
> > > >   * Since the hardware frontbuffer tracking has gaps we need to
> > > > integrate
> > > >   * with the software frontbuffer tracking. This function gets
> > > > called
> > > > every
> > > > @@ -957,7 +958,7 @@ void intel_psr_single_frame_update(struct
> > > > drm_i915_private *dev_priv,
> > > >   * Dirty frontbuffers relevant to PSR are tracked in
> > > > busy_frontbuffer_bits."
> > > >   */
> > > >  void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> > > > -			  unsigned frontbuffer_bits)
> > > > +			  unsigned frontbuffer_bits, enum
> > > > fb_op_origin origin)
> > > >  {
> > > >  	struct drm_crtc *crtc;
> > > >  	enum pipe pipe;
> > > > @@ -965,6 +966,9 @@ void intel_psr_invalidate(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > >  	if (!CAN_PSR(dev_priv))
> > > >  		return;
> > > >  
> > > > +	if (dev_priv->psr.has_hw_tracking && origin ==
> > > > ORIGIN_FLIP)
> > > > +		return;
> > > > +
> > > >  	mutex_lock(&dev_priv->psr.lock);
> > > >  	if (!dev_priv->psr.enabled) {
> > > >  		mutex_unlock(&dev_priv->psr.lock);
> > > > @@ -1005,6 +1009,9 @@ void intel_psr_flush(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > >  	if (!CAN_PSR(dev_priv))
> > > >  		return;
> > > >  
> > > > +	if (dev_priv->psr.has_hw_tracking && origin ==
> > > > ORIGIN_FLIP)
> > > > +		return;
> > > > +
> > > >  	mutex_lock(&dev_priv->psr.lock);
> > > >  	if (!dev_priv->psr.enabled) {
> > > >  		mutex_unlock(&dev_priv->psr.lock);
> > > > @@ -1081,6 +1088,7 @@ void intel_psr_init(struct drm_i915_private
> > > > *dev_priv)
> > > >  		dev_priv->psr.activate = vlv_psr_activate;
> > > >  		dev_priv->psr.setup_vsc = vlv_psr_setup_vsc;
> > > >  	} else {
> > > > +		dev_priv->psr.has_hw_tracking = true;
> > > >  		dev_priv->psr.enable_source =
> > > > hsw_psr_enable_source;
> > > >  		dev_priv->psr.disable_source = hsw_psr_disable;
> > > >  		dev_priv->psr.enable_sink = hsw_psr_enable_sink;
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx