[4/5] drm/i915: PSR VLV: Add single frame update.

Submitted by Rodrigo Vivi on April 10, 2015, 6:15 p.m.

Details

Message ID 1428689711-3541-4-git-send-email-rodrigo.vivi@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Rodrigo Vivi April 10, 2015, 6:15 p.m.
According to spec: "In PSR HW or SW mode, SW set this bit before writing
registers for a flip. It will be self-clear when it gets to the PSR
active state."

Some versions of spec mention that this is needed when in
"Persistent mode" but define it as same as "SW mode". Since this
fix the page flip case let's assume this is exactly what we need.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h         |  1 +
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 ++
 drivers/gpu/drm/i915/intel_psr.c         | 42 ++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7a0aa24..9c5d1cd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1220,6 +1220,7 @@  void intel_psr_invalidate(struct drm_device *dev,
 void intel_psr_flush(struct drm_device *dev,
 			 unsigned frontbuffer_bits);
 void intel_psr_init(struct drm_device *dev);
+void intel_psr_single_frame_update(struct drm_device *dev);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index a20cffb..57095f5 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -243,6 +243,8 @@  void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 	/* Remove stale busy bits due to the old buffer. */
 	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_psr_single_frame_update(dev);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 5cd374b..5ee0fa5 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -594,6 +594,48 @@  static void intel_psr_exit(struct drm_device *dev)
 }
 
 /**
+ * intel_psr_single_frame_update - Single Frame Update
+ * @dev: DRM device
+ *
+ * Some platforms support a single frame update feature that is used to
+ * send and update only one frame on Remote Frame Buffer.
+ * So far it is only implemented for Valleyview and Cherryview because
+ * hardware requires this to be done before a page flip.
+ */
+void intel_psr_single_frame_update(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	enum pipe pipe;
+	u32 val;
+
+	/*
+	 * Single frame update is already supported on BDW+ but it requires
+	 * many W/A and it isn't really needed.
+	 */
+	if (!IS_VALLEYVIEW(dev))
+		return;
+
+	mutex_lock(&dev_priv->psr.lock);
+	if (!dev_priv->psr.enabled) {
+		mutex_unlock(&dev_priv->psr.lock);
+		return;
+	}
+
+	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
+	pipe = to_intel_crtc(crtc)->pipe;
+	val = I915_READ(VLV_PSRCTL(pipe));
+
+	/*
+	 * We need to set this bit before writing registers for a flip.
+	 * This bit will be self-clear when it gets to the PSR active state.
+	 */
+	I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
+
+	mutex_unlock(&dev_priv->psr.lock);
+}
+
+/**
  * intel_psr_invalidate - Invalidade PSR
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits

Comments

On Fri, Apr 10, 2015 at 11:15:10AM -0700, Rodrigo Vivi wrote:
> According to spec: "In PSR HW or SW mode, SW set this bit before writing
> registers for a flip. It will be self-clear when it gets to the PSR
> active state."
> 
> Some versions of spec mention that this is needed when in
> "Persistent mode" but define it as same as "SW mode". Since this
> fix the page flip case let's assume this is exactly what we need.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Merged up to this one to dinq, will wait with the final enable until the
bug Matthew reported is a bit clearer.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_drv.h         |  1 +
>  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 ++
>  drivers/gpu/drm/i915/intel_psr.c         | 42 ++++++++++++++++++++++++++++++++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7a0aa24..9c5d1cd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1220,6 +1220,7 @@ void intel_psr_invalidate(struct drm_device *dev,
>  void intel_psr_flush(struct drm_device *dev,
>  			 unsigned frontbuffer_bits);
>  void intel_psr_init(struct drm_device *dev);
> +void intel_psr_single_frame_update(struct drm_device *dev);
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index a20cffb..57095f5 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -243,6 +243,8 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>  	/* Remove stale busy bits due to the old buffer. */
>  	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
>  	mutex_unlock(&dev_priv->fb_tracking.lock);
> +
> +	intel_psr_single_frame_update(dev);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5cd374b..5ee0fa5 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -594,6 +594,48 @@ static void intel_psr_exit(struct drm_device *dev)
>  }
>  
>  /**
> + * intel_psr_single_frame_update - Single Frame Update
> + * @dev: DRM device
> + *
> + * Some platforms support a single frame update feature that is used to
> + * send and update only one frame on Remote Frame Buffer.
> + * So far it is only implemented for Valleyview and Cherryview because
> + * hardware requires this to be done before a page flip.
> + */
> +void intel_psr_single_frame_update(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	enum pipe pipe;
> +	u32 val;
> +
> +	/*
> +	 * Single frame update is already supported on BDW+ but it requires
> +	 * many W/A and it isn't really needed.
> +	 */
> +	if (!IS_VALLEYVIEW(dev))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	if (!dev_priv->psr.enabled) {
> +		mutex_unlock(&dev_priv->psr.lock);
> +		return;
> +	}
> +
> +	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	val = I915_READ(VLV_PSRCTL(pipe));
> +
> +	/*
> +	 * We need to set this bit before writing registers for a flip.
> +	 * This bit will be self-clear when it gets to the PSR active state.
> +	 */
> +	I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +/**
>   * intel_psr_invalidate - Invalidade PSR
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2015-04-14 at 20:04 +0200, Daniel Vetter wrote:
> On Fri, Apr 10, 2015 at 11:15:10AM -0700, Rodrigo Vivi wrote:

> > According to spec: "In PSR HW or SW mode, SW set this bit before writing

> > registers for a flip. It will be self-clear when it gets to the PSR

> > active state."

> > 

> > Some versions of spec mention that this is needed when in

> > "Persistent mode" but define it as same as "SW mode". Since this

> > fix the page flip case let's assume this is exactly what we need.

> > 

> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 

> Merged up to this one to dinq, will wait with the final enable until the

> bug Matthew reported is a bit clearer.


Thank you! I agree it is safe to wait,
but I believe the fbcon roling screen is the ips issue with resolution
and clock.
and the slowness on X typping is something else that is triggered with
some set that powertop --auto-tune uses, because I'm also facing this
even with PSR disabled. I'll investigate that anyway.

So I'm confident Matthew's issues aren't PSR related.

Thanks again,
Rodrigo.


> -Daniel

> 

> > ---

> >  drivers/gpu/drm/i915/intel_drv.h         |  1 +

> >  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 ++

> >  drivers/gpu/drm/i915/intel_psr.c         | 42 ++++++++++++++++++++++++++++++++

> >  3 files changed, 45 insertions(+)

> > 

> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h

> > index 7a0aa24..9c5d1cd 100644

> > --- a/drivers/gpu/drm/i915/intel_drv.h

> > +++ b/drivers/gpu/drm/i915/intel_drv.h

> > @@ -1220,6 +1220,7 @@ void intel_psr_invalidate(struct drm_device *dev,

> >  void intel_psr_flush(struct drm_device *dev,

> >  			 unsigned frontbuffer_bits);

> >  void intel_psr_init(struct drm_device *dev);

> > +void intel_psr_single_frame_update(struct drm_device *dev);

> >  

> >  /* intel_runtime_pm.c */

> >  int intel_power_domains_init(struct drm_i915_private *);

> > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c

> > index a20cffb..57095f5 100644

> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c

> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c

> > @@ -243,6 +243,8 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,

> >  	/* Remove stale busy bits due to the old buffer. */

> >  	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;

> >  	mutex_unlock(&dev_priv->fb_tracking.lock);

> > +

> > +	intel_psr_single_frame_update(dev);

> >  }

> >  

> >  /**

> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c

> > index 5cd374b..5ee0fa5 100644

> > --- a/drivers/gpu/drm/i915/intel_psr.c

> > +++ b/drivers/gpu/drm/i915/intel_psr.c

> > @@ -594,6 +594,48 @@ static void intel_psr_exit(struct drm_device *dev)

> >  }

> >  

> >  /**

> > + * intel_psr_single_frame_update - Single Frame Update

> > + * @dev: DRM device

> > + *

> > + * Some platforms support a single frame update feature that is used to

> > + * send and update only one frame on Remote Frame Buffer.

> > + * So far it is only implemented for Valleyview and Cherryview because

> > + * hardware requires this to be done before a page flip.

> > + */

> > +void intel_psr_single_frame_update(struct drm_device *dev)

> > +{

> > +	struct drm_i915_private *dev_priv = dev->dev_private;

> > +	struct drm_crtc *crtc;

> > +	enum pipe pipe;

> > +	u32 val;

> > +

> > +	/*

> > +	 * Single frame update is already supported on BDW+ but it requires

> > +	 * many W/A and it isn't really needed.

> > +	 */

> > +	if (!IS_VALLEYVIEW(dev))

> > +		return;

> > +

> > +	mutex_lock(&dev_priv->psr.lock);

> > +	if (!dev_priv->psr.enabled) {

> > +		mutex_unlock(&dev_priv->psr.lock);

> > +		return;

> > +	}

> > +

> > +	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;

> > +	pipe = to_intel_crtc(crtc)->pipe;

> > +	val = I915_READ(VLV_PSRCTL(pipe));

> > +

> > +	/*

> > +	 * We need to set this bit before writing registers for a flip.

> > +	 * This bit will be self-clear when it gets to the PSR active state.

> > +	 */

> > +	I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);

> > +

> > +	mutex_unlock(&dev_priv->psr.lock);

> > +}

> > +

> > +/**

> >   * intel_psr_invalidate - Invalidade PSR

> >   * @dev: DRM device

> >   * @frontbuffer_bits: frontbuffer plane tracking bits

> > -- 

> > 2.1.0

> > 

> > _______________________________________________

> > Intel-gfx mailing list

> > Intel-gfx@lists.freedesktop.org

> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

>
On Fri, Apr 10, 2015 at 8:15 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>
>  /**
> + * intel_psr_single_frame_update - Single Frame Update
> + * @dev: DRM device
> + *
> + * Some platforms support a single frame update feature that is used to
> + * send and update only one frame on Remote Frame Buffer.
> + * So far it is only implemented for Valleyview and Cherryview because
> + * hardware requires this to be done before a page flip.
> + */
> +void intel_psr_single_frame_update(struct drm_device *dev)

While reading through frontbuffer code I realized that this function
here doesn't filter flip_prepare events according to the frontbuffer
bitmask. Which means we'll do a single-shot upload even when only
another CRTC changes. Rodrigo, can you please follow-up with a patch
to do that? Similar to how invalidate/flush only do something when
it's about a frontbuffer on the psr CRTC.

Thanks, Daniel