[PATCH 0/3] CHV vblank failures when PSR is active

[PATCH 0/3] CHV vblank failures when PSR is active

[Dhinakaran Pandiyan]

IGT vblank tests fail on CHV by timing out on VBIs if PSR is enabled. We
do not get VBIs as the source timing generation is disabled when PSR is
active. The first two patches written by Rodrigo add drm hooks. Patch 3
deactivates PSR when VBI are needed.

[PATCH 1/3] drm: Add vblank prepare and unprepare hooks.
[PATCH 2/3] drm/i915: Move drm_crtc_vblank_get out of disabled
[PATCH 3/3] drm/i915/psr: Do not activate PSR when vblank interrupts
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm: Add vblank prepare and unprepare hooks.

[Dhinakaran Pandiyan]

From: Rodrigo Vivi <rodrigo.vivi@intel.com>

This will allow drivers to control specific power saving
feature and power domains when dealing with vblanks.

Vblanks code are protected by spin_locks where we can't
have anything that can sleep. While power saving features
and power domain code have mutexes to control the states.

Mutex can sleep so they cannot be used inside spin lock areas.
So the easiest way to deal with them currently is to add these
prepare hook for pre enabling vblanks
and unprepare one for post disabling them.

Let's introduce this optional prepare and unprepare
hooks so drivers can deal with cases like this and any other
case that should require sleeping codes interacting with vblanks.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/drm_irq.c | 28 +++++++++++++++++++++++++++-
 include/drm/drmP.h        | 30 ++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index d3124b6..c833a5d 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -411,6 +411,8 @@ void drm_vblank_cleanup(struct drm_device *dev)
 			drm_core_check_feature(dev, DRIVER_MODESET));
 
 		del_timer_sync(&vblank->disable_timer);
+
+		flush_work(&vblank->unprepare.work);
 	}
 
 	kfree(dev->vblank);
@@ -419,6 +421,20 @@ void drm_vblank_cleanup(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_vblank_cleanup);
 
+static void drm_vblank_unprepare_work_fn(struct work_struct *work)
+{
+	struct drm_vblank_crtc *vblank;
+	struct drm_device *dev;
+
+	vblank = container_of(work, typeof(*vblank), unprepare.work);
+	dev = vblank->dev;
+
+	do {
+		if (dev->driver->unprepare_vblank)
+			dev->driver->unprepare_vblank(dev, vblank->pipe);
+	} while (!atomic_dec_and_test(&vblank->unprepare.counter));
+}
+
 /**
  * drm_vblank_init - initialize vblank support
  * @dev: DRM device
@@ -451,6 +467,8 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs)
 		init_waitqueue_head(&vblank->queue);
 		setup_timer(&vblank->disable_timer, vblank_disable_fn,
 			    (unsigned long)vblank);
+		INIT_WORK(&vblank->unprepare.work,
+			  drm_vblank_unprepare_work_fn);
 	}
 
 	DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");
@@ -1241,6 +1259,9 @@ int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 	if (WARN_ON(pipe >= dev->num_crtcs))
 		return -EINVAL;
 
+	if (dev->driver->prepare_vblank)
+		dev->driver->prepare_vblank(dev, pipe);
+
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	/* Going from 0->1 means we have to enable interrupts again */
 	if (atomic_add_return(1, &vblank->refcount) == 1) {
@@ -1253,6 +1274,9 @@ int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 	}
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 
+	if (ret != 0 && dev->driver->unprepare_vblank)
+		dev->driver->unprepare_vblank(dev, pipe);
+
 	return ret;
 }
 EXPORT_SYMBOL(drm_vblank_get);
@@ -1305,6 +1329,9 @@ void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
 			mod_timer(&vblank->disable_timer,
 				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
 	}
+
+	atomic_inc(&vblank->unprepare.counter);
+	schedule_work(&vblank->unprepare.work);
 }
 EXPORT_SYMBOL(drm_vblank_put);
 
@@ -1740,7 +1767,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 	}
 
 	spin_unlock_irqrestore(&dev->event_lock, flags);
-
 	return 0;
 
 err_unlock:
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index ed89038..544c65f 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -447,6 +447,30 @@ struct drm_driver {
 	u32 (*get_vblank_counter) (struct drm_device *dev, unsigned int pipe);
 
 	/**
+	 * prepare_vblank - Optional prepare vblank hook.
+	 * @dev: DRM device
+	 * @pipe: counter to fetch
+	 *
+	 * Drivers that need to handle any kind of mutex or any other sleeping
+	 * code in combination with vblanks need to implement this hook
+	 * that will be called before drm_vblank_get spin_lock gets.
+	 */
+	void (*prepare_vblank) (struct drm_device *dev, unsigned int pipe);
+
+	/**
+	 * unprepare_vblank - Optional unprepare vblank hook.
+	 * @dev: DRM device
+	 * @pipe: counter to fetch
+	 *
+	 * Drivers that need to handle any kind of mutex or any other sleeping
+	 * code in combination with vblanks need to implement this hook
+	 * that will be called in a work queue to be executed after spin lock
+	 * areas of drm_vblank_put.
+	 */
+	void (*unprepare_vblank) (struct drm_device *dev, unsigned int pipe);
+
+
+	/**
 	 * enable_vblank - enable vblank interrupt events
 	 * @dev: DRM device
 	 * @pipe: which irq to enable
@@ -720,6 +744,11 @@ struct drm_pending_vblank_event {
 	struct drm_event_vblank event;
 };
 
+struct drm_vblank_unprepare {
+	struct work_struct work;	/* Post disable worker */
+	atomic_t counter;		/* Number of vblanks handled */
+};
+
 struct drm_vblank_crtc {
 	struct drm_device *dev;		/* pointer to the drm_device */
 	wait_queue_head_t queue;	/**< VBLANK wait queue */
@@ -740,6 +769,7 @@ struct drm_vblank_crtc {
 	int linedur_ns;			/* line duration in ns */
 	bool enabled;			/* so we don't call enable more than
 					   once per disable */
+	struct drm_vblank_unprepare unprepare;  /* Unprepare work helper */
 };
 
 /**
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

REBASED: [PATCH 1/3] drm: Add vblank prepare and unprepare hooks.

[Dhinakaran Pandiyan]

From: Rodrigo Vivi <rodrigo.vivi@intel.com>

This will allow drivers to control specific power saving
feature and power domains when dealing with vblanks.

Vblanks code are protected by spin_locks where we can't
have anything that can sleep. While power saving features
and power domain code have mutexes to control the states.

Mutex can sleep so they cannot be used inside spin lock areas.
So the easiest way to deal with them currently is to add these
prepare hook for pre enabling vblanks
and unprepare one for post disabling them.

Let's introduce this optional prepare and unprepare
hooks so drivers can deal with cases like this and any other
case that should require sleeping codes interacting with vblanks.

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

 drivers/gpu/drm/drm_irq.c | 28 +++++++++++++++++++++++++++-
 include/drm/drmP.h        | 30 ++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 5a773e4..bdf01cd 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -347,6 +347,8 @@ void drm_vblank_cleanup(struct drm_device *dev)
 			drm_core_check_feature(dev, DRIVER_MODESET));
 
 		del_timer_sync(&vblank->disable_timer);
+
+		flush_work(&vblank->unprepare.work);
 	}
 
 	kfree(dev->vblank);
@@ -355,6 +357,20 @@ void drm_vblank_cleanup(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_vblank_cleanup);
 
+static void drm_vblank_unprepare_work_fn(struct work_struct *work)
+{
+	struct drm_vblank_crtc *vblank;
+	struct drm_device *dev;
+
+	vblank = container_of(work, typeof(*vblank), unprepare.work);
+	dev = vblank->dev;
+
+	do {
+		if (dev->driver->unprepare_vblank)
+			dev->driver->unprepare_vblank(dev, vblank->pipe);
+	} while (!atomic_dec_and_test(&vblank->unprepare.counter));
+}
+
 /**
  * drm_vblank_init - initialize vblank support
  * @dev: DRM device
@@ -388,6 +404,8 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs)
 		setup_timer(&vblank->disable_timer, vblank_disable_fn,
 			    (unsigned long)vblank);
 		seqlock_init(&vblank->seqlock);
+		INIT_WORK(&vblank->unprepare.work,
+			  drm_vblank_unprepare_work_fn);
 	}
 
 	DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");
@@ -1170,6 +1188,9 @@ int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 	if (WARN_ON(pipe >= dev->num_crtcs))
 		return -EINVAL;
 
+	if (dev->driver->prepare_vblank)
+		dev->driver->prepare_vblank(dev, pipe);
+
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	/* Going from 0->1 means we have to enable interrupts again */
 	if (atomic_add_return(1, &vblank->refcount) == 1) {
@@ -1182,6 +1203,9 @@ int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 	}
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 
+	if (ret != 0 && dev->driver->unprepare_vblank)
+		dev->driver->unprepare_vblank(dev, pipe);
+
 	return ret;
 }
 EXPORT_SYMBOL(drm_vblank_get);
@@ -1234,6 +1258,9 @@ void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
 			mod_timer(&vblank->disable_timer,
 				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
 	}
+
+	atomic_inc(&vblank->unprepare.counter);
+	schedule_work(&vblank->unprepare.work);
 }
 EXPORT_SYMBOL(drm_vblank_put);
 
@@ -1668,7 +1695,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 	}
 
 	spin_unlock_irqrestore(&dev->event_lock, flags);
-
 	return 0;
 
 err_unlock:
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index c5d2950..7495f56 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -443,6 +443,30 @@ struct drm_driver {
 	u32 (*get_vblank_counter) (struct drm_device *dev, unsigned int pipe);
 
 	/**
+	 * prepare_vblank - Optional prepare vblank hook.
+	 * @dev: DRM device
+	 * @pipe: counter to fetch
+	 *
+	 * Drivers that need to handle any kind of mutex or any other sleeping
+	 * code in combination with vblanks need to implement this hook
+	 * that will be called before drm_vblank_get spin_lock gets.
+	 */
+	void (*prepare_vblank) (struct drm_device *dev, unsigned int pipe);
+
+	/**
+	 * unprepare_vblank - Optional unprepare vblank hook.
+	 * @dev: DRM device
+	 * @pipe: counter to fetch
+	 *
+	 * Drivers that need to handle any kind of mutex or any other sleeping
+	 * code in combination with vblanks need to implement this hook
+	 * that will be called in a work queue to be executed after spin lock
+	 * areas of drm_vblank_put.
+	 */
+	void (*unprepare_vblank) (struct drm_device *dev, unsigned int pipe);
+
+
+	/**
 	 * enable_vblank - enable vblank interrupt events
 	 * @dev: DRM device
 	 * @pipe: which irq to enable
@@ -716,6 +740,11 @@ struct drm_pending_vblank_event {
 	struct drm_event_vblank event;
 };
 
+struct drm_vblank_unprepare {
+	struct work_struct work;	/* Post disable worker */
+	atomic_t counter;		/* Number of vblanks handled */
+};
+
 struct drm_vblank_crtc {
 	struct drm_device *dev;		/* pointer to the drm_device */
 	wait_queue_head_t queue;	/**< VBLANK wait queue */
@@ -736,6 +765,7 @@ struct drm_vblank_crtc {
 	int linedur_ns;			/* line duration in ns */
 	bool enabled;			/* so we don't call enable more than
 					   once per disable */
+	struct drm_vblank_unprepare unprepare;  /* Unprepare work helper */
 };
 
 /**
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Move drm_crtc_vblank_get out of disabled pre-emption area.

[Dhinakaran Pandiyan]

From: Rodrigo Vivi <rodrigo.vivi@intel.com>

drm_crtc_vblank_get call the drm_vblank_prepare that will be used soon
to control power saving states or anything else that needs a mutex
before the vblank happens.

local_irq_disable disables kernel preemption so we won't be able
to use mutex inside drm_crtc_vblank_get. For this reason we need
to move the drm_crtc_vblank_get a little up before disabling the
interruptions.

Another option would be to use local_irq_enable + local_irq_disable
around the drm_crtc_vblank_get, but let's avoid touching the kernel
states if we can call the vblank_get a bit earlier.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 97b1a54..3351c7e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -94,13 +94,15 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
 	min = vblank_start - usecs_to_scanlines(adjusted_mode, 100);
 	max = vblank_start - 1;
 
-	local_irq_disable();
-
-	if (min <= 0 || max <= 0)
+	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
 		return;
 
-	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
+	local_irq_disable();
+
+	if (min <= 0 || max <= 0) {
+		drm_crtc_vblank_put(&crtc->base);
 		return;
+	}
 
 	crtc->debug.min_vbl = min;
 	crtc->debug.max_vbl = max;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915/psr: Do not activate PSR when vblank interrupts are enabled

[Dhinakaran Pandiyan]

PSR in CHV, unlike HSW, can get activated even if vblanks interrupts are
enabled. But, the pipe is not expected to generate timings signals
when PSR is active. Specifically, we do not get vblank interrupts in CHV
if PSR becomes active. This has led to drm_wait_vblank timing out.

Let's disable PSR using the vblank prepare hook that gets called before
enabling vblank interrupts and keep it disabled until the interrupts are
not needed.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/i915_irq.c  | 12 ++++++++
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_psr.c | 61 +++++++++++++++++++++++++++++++++++++++-
 4 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4c8e34..03f311e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -994,6 +994,7 @@ struct i915_psr {
 	bool psr2_support;
 	bool aux_frame_sync;
 	bool link_standby;
+	bool vlv_src_timing;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index caaf1e2..77f3d76 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	return 0;
 }
 
+static void valleyview_prepare_vblank(struct drm_device *dev, unsigned int pipe)
+{
+	vlv_psr_src_timing_get(dev);
+}
+
+static void valleyview_unprepare_vblank(struct drm_device *dev, unsigned int pipe){
+
+	vlv_psr_src_timing_put(dev);
+}
+
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
@@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 		dev->driver->irq_uninstall = cherryview_irq_uninstall;
 		dev->driver->enable_vblank = valleyview_enable_vblank;
 		dev->driver->disable_vblank = valleyview_disable_vblank;
+		dev->driver->prepare_vblank = valleyview_prepare_vblank;
+		dev->driver->unprepare_vblank = valleyview_unprepare_vblank;
 		dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
 	} else if (IS_VALLEYVIEW(dev_priv)) {
 		dev->driver->irq_handler = valleyview_irq_handler;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9b5f663..e2078fd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device *dev,
 void intel_psr_init(struct drm_device *dev);
 void intel_psr_single_frame_update(struct drm_device *dev,
 				   unsigned frontbuffer_bits);
+void vlv_psr_src_timing_get(struct drm_device *dev);
+void vlv_psr_src_timing_put(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_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 29a09bf..c95e680 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 
 		/* Enable PSR on the panel */
 		vlv_psr_enable_sink(intel_dp);
+		dev_priv->psr.vlv_src_timing = false;
 
 		/* On HSW+ enable_source also means go to PSR entry/active
 		 * state as soon as idle_frame achieved and here would be
@@ -608,8 +609,10 @@ static void intel_psr_work(struct work_struct *work)
 	 * The delayed work can race with an invalidate hence we need to
 	 * recheck. Since psr_flush first clears this and then reschedules we
 	 * won't ever miss a flush when bailing out here.
+	 * Also, do not enable PSR if source is required to generate timing
+	 * signals like vblanks.
 	 */
-	if (dev_priv->psr.busy_frontbuffer_bits)
+	if (dev_priv->psr.busy_frontbuffer_bits || dev_priv->psr.vlv_src_timing)
 		goto unlock;
 
 	intel_psr_activate(intel_dp);
@@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct drm_device *dev,
 }
 
 /**
+ * vlv_psr_src_timing_get - src timing generation requested
+ *
+ * CHV does not have HW tracking to trigger PSR exit when VBI are enabled nor
+ * does enabling vblank interrupts prevent PSR entry. This function is called
+ * before enabling VBI to exit PSR and prevent PSR re-entry until vblanks are
+ * disabled again.
+ */
+void vlv_psr_src_timing_get(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->psr.lock);
+        if (!dev_priv->psr.enabled) {
+                mutex_unlock(&dev_priv->psr.lock);
+                return;
+	}
+
+	//Handle racing with intel_psr_work with this flag
+	dev_priv->psr.vlv_src_timing = true;
+
+	if(dev_priv->psr.active)
+		intel_psr_exit(dev);
+
+	mutex_unlock(&dev_priv->psr.lock);
+
+}
+
+
+/**
+ * vlv_psr_src_timing_put - src timing generation not required
+ *
+ * CHV does not have HW tracking to trigger PSR exit when VBI are enabled nor
+ * does enabling vblank interrupts prevent PSR entry. This function is called
+ * when VBI are not required and PSR can be activated.
+ */
+void vlv_psr_src_timing_put(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->psr.lock);
+	if (!dev_priv->psr.enabled) {
+                mutex_unlock(&dev_priv->psr.lock);
+                return;
+        }
+
+	dev_priv->psr.vlv_src_timing = false;
+
+	if (!dev_priv->psr.active)
+                if (!work_busy(&dev_priv->psr.work.work))
+			schedule_delayed_work(&dev_priv->psr.work,
+                                              msecs_to_jiffies(100));
+
+	mutex_unlock(&dev_priv->psr.lock);
+}
+
+/**
  * intel_psr_invalidate - Invalidade PSR
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/psr: Do not activate PSR when vblank interrupts are enabled

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 19405 bytes --]

Hi,

[auto build test WARNING on v4.7-rc2]
[cannot apply to drm-intel/for-linux-next drm/drm-next next-20160608]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dhinakaran-Pandiyan/CHV-vblank-failures-when-PSR-is-active/20160609-094028
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/drm_fb_cma_helper.c:233: warning: No description found for parameter 'dev'
   drivers/gpu/drm/drm_fb_cma_helper.c:233: warning: No description found for parameter 'file_priv'
   drivers/gpu/drm/drm_fb_cma_helper.c:233: warning: No description found for parameter 'mode_cmd'
   drivers/gpu/drm/drm_fb_cma_helper.c:285: warning: No description found for parameter 'm'
   drivers/gpu/drm/drm_fb_cma_helper.c:285: warning: No description found for parameter 'arg'
   include/drm/drm_dp_helper.h:750: warning: No description found for parameter 'i2c_nack_count'
   include/drm/drm_dp_helper.h:750: warning: No description found for parameter 'i2c_defer_count'
   drivers/gpu/drm/drm_dp_mst_topology.c:2383: warning: No description found for parameter 'connector'
   include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'cached_edid'
   include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'has_audio'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'max_dpcd_transaction_bytes'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'sink_count'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_slots'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'avail_slots'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_pbn'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'qlock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_msg_downq'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_down_in_progress'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_lock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'proposed_vcpis'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payloads'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_mask'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'vcpi_mask'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_waitq'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'work'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_work'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_list'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_lock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_work'
   drivers/gpu/drm/drm_dp_mst_topology.c:2383: warning: No description found for parameter 'connector'
   drivers/gpu/drm/drm_irq.c:176: warning: No description found for parameter 'flags'
   include/drm/drmP.h:168: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:184: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:202: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:247: warning: No description found for parameter 'dev'
   include/drm/drmP.h:247: warning: No description found for parameter 'data'
   include/drm/drmP.h:247: warning: No description found for parameter 'file_priv'
   include/drm/drmP.h:280: warning: No description found for parameter 'ioctl'
   include/drm/drmP.h:280: warning: No description found for parameter '_func'
   include/drm/drmP.h:280: warning: No description found for parameter '_flags'
   include/drm/drmP.h:362: warning: cannot understand function prototype: 'struct drm_lock_data '
   include/drm/drmP.h:415: warning: cannot understand function prototype: 'struct drm_driver '
   include/drm/drmP.h:705: warning: cannot understand function prototype: 'struct drm_info_list '
   include/drm/drmP.h:715: warning: cannot understand function prototype: 'struct drm_info_node '
   include/drm/drmP.h:725: warning: cannot understand function prototype: 'struct drm_minor '
   include/drm/drmP.h:779: warning: cannot understand function prototype: 'struct drm_device '
   drivers/gpu/drm/i915/intel_runtime_pm.c:2411: warning: No description found for parameter 'resume'
   drivers/gpu/drm/i915/intel_runtime_pm.c:2411: warning: No description found for parameter 'resume'
   drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_gem.c:416: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:416: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:416: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:681: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:681: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:681: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'args'
   drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1025: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1025: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1025: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1231: warning: No description found for parameter 'rps'
   drivers/gpu/drm/i915/i915_gem.c:1446: warning: No description found for parameter 'req'
   drivers/gpu/drm/i915/i915_gem.c:1473: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:1473: warning: No description found for parameter 'readonly'
   drivers/gpu/drm/i915/i915_gem.c:1590: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1590: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1590: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1653: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1653: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1653: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1698: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1698: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1698: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:2006: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:2006: warning: No description found for parameter 'size'
   drivers/gpu/drm/i915/i915_gem.c:2006: warning: No description found for parameter 'tiling_mode'
   drivers/gpu/drm/i915/i915_gem.c:2006: warning: No description found for parameter 'fenced'
   drivers/gpu/drm/i915/i915_gem.c:2006: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment'
   drivers/gpu/drm/i915/i915_gem.c:2960: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_gem.c:3086: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3136: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:3136: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:3136: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:3136: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl'
   drivers/gpu/drm/i915/i915_gem.c:3498: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3498: warning: No description found for parameter 'vm'
   drivers/gpu/drm/i915/i915_gem.c:3498: warning: No description found for parameter 'ggtt_view'
   drivers/gpu/drm/i915/i915_gem.c:3498: warning: No description found for parameter 'alignment'
   drivers/gpu/drm/i915/i915_gem.c:3498: warning: No description found for parameter 'flags'
   drivers/gpu/drm/i915/i915_gem.c:3754: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3754: warning: No description found for parameter 'write'
   drivers/gpu/drm/i915/i915_gem.c:3832: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3832: warning: No description found for parameter 'cache_level'
   drivers/gpu/drm/i915/i915_gem.c:4106: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:4106: warning: No description found for parameter 'write'
>> drivers/gpu/drm/i915/intel_psr.c:739: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/intel_psr.c:767: warning: No description found for parameter 'dev'
>> drivers/gpu/drm/i915/intel_psr.c:739: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/intel_psr.c:767: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_init_ring'
   drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_fini_ring'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: Excess function parameter 'ring' description in 'i915_needs_cmd_parser'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: Excess function parameter 'ring' description in 'i915_parse_cmds'
   drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_init_ring'
   drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_fini_ring'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: Excess function parameter 'ring' description in 'i915_needs_cmd_parser'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: Excess function parameter 'ring' description in 'i915_parse_cmds'
   drivers/gpu/drm/i915/intel_lrc.c:316: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:316: warning: Excess function parameter 'ring' description in 'intel_lr_context_descriptor_update'
   drivers/gpu/drm/i915/intel_lrc.c:353: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:353: warning: Excess function parameter 'ring' description in 'intel_execlists_ctx_id'
   drivers/gpu/drm/i915/intel_lrc.c:544: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/intel_lrc.c:544: warning: Excess function parameter 'engine' description in 'intel_lrc_irq_handler'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: No description found for parameter 'params'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'dev' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'file' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:1195: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:1195: warning: Excess function parameter 'ring' description in 'gen8_init_indirectctx_bb'
   drivers/gpu/drm/i915/intel_lrc.c:1258: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:1258: warning: Excess function parameter 'ring' description in 'gen8_init_perctx_bb'
   drivers/gpu/drm/i915/intel_lrc.c:1901: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:1901: warning: Excess function parameter 'ring' description in 'intel_logical_ring_cleanup'
   drivers/gpu/drm/i915/intel_lrc.c:2486: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:2486: warning: Excess function parameter 'ring' description in 'intel_lr_context_size'
   drivers/gpu/drm/i915/intel_lrc.c:2525: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:2525: warning: Excess function parameter 'ring' description in 'intel_lr_context_deferred_alloc'
   drivers/gpu/drm/i915/intel_lrc.c:316: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:316: warning: Excess function parameter 'ring' description in 'intel_lr_context_descriptor_update'
   drivers/gpu/drm/i915/intel_lrc.c:353: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:353: warning: Excess function parameter 'ring' description in 'intel_execlists_ctx_id'
   drivers/gpu/drm/i915/intel_lrc.c:544: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/intel_lrc.c:544: warning: Excess function parameter 'engine' description in 'intel_lrc_irq_handler'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: No description found for parameter 'params'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'dev' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'file' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:810: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:1195: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:1195: warning: Excess function parameter 'ring' description in 'gen8_init_indirectctx_bb'
   drivers/gpu/drm/i915/intel_lrc.c:1258: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:1258: warning: Excess function parameter 'ring' description in 'gen8_init_perctx_bb'
   drivers/gpu/drm/i915/intel_lrc.c:1901: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:1901: warning: Excess function parameter 'ring' description in 'intel_logical_ring_cleanup'
   drivers/gpu/drm/i915/intel_lrc.c:2486: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:2486: warning: Excess function parameter 'ring' description in 'intel_lr_context_size'
   drivers/gpu/drm/i915/intel_lrc.c:2525: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:2525: warning: Excess function parameter 'ring' description in 'intel_lr_context_deferred_alloc'
   Warning: didn't use docs for i915_hotplug_interrupt_update
   Warning: didn't use docs for ilk_update_display_irq
   Warning: didn't use docs for ilk_update_gt_irq
   Warning: didn't use docs for snb_update_pm_irq
   Warning: didn't use docs for bdw_update_port_irq
   Warning: didn't use docs for bdw_update_pipe_irq
   Warning: didn't use docs for ibx_display_interrupt_update
   Warning: didn't use docs for i915_enable_asle_pipestat
   Warning: didn't use docs for ivybridge_parity_work
   Warning: didn't use docs for i915_reset_and_wakeup
   Warning: didn't use docs for i915_handle_error
   Warning: didn't use docs for intel_irq_install
   Warning: didn't use docs for intel_irq_uninstall

vim +/dev +739 drivers/gpu/drm/i915/intel_psr.c

   723			 * This bit will be self-clear when it gets to the PSR active state.
   724			 */
   725			I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
   726		}
   727		mutex_unlock(&dev_priv->psr.lock);
   728	}
   729	
   730	/**
   731	 * vlv_psr_src_timing_get - src timing generation requested
   732	 *
   733	 * CHV does not have HW tracking to trigger PSR exit when VBI are enabled nor
   734	 * does enabling vblank interrupts prevent PSR entry. This function is called
   735	 * before enabling VBI to exit PSR and prevent PSR re-entry until vblanks are
   736	 * disabled again.
   737	 */
   738	void vlv_psr_src_timing_get(struct drm_device *dev)
 > 739	{
   740		struct drm_i915_private *dev_priv = dev->dev_private;
   741	
   742		mutex_lock(&dev_priv->psr.lock);
   743	        if (!dev_priv->psr.enabled) {
   744	                mutex_unlock(&dev_priv->psr.lock);
   745	                return;
   746		}
   747	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6370 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/psr: Do not activate PSR when vblank interrupts are enabled

[Vivi, Rodrigo]

On Wed, 2016-06-08 at 18:46 -0700, Dhinakaran Pandiyan wrote:
> PSR in CHV, unlike HSW, can get activated even if vblanks interrupts
> are
> enabled. But, the pipe is not expected to generate timings signals
> when PSR is active. Specifically, we do not get vblank interrupts in
> CHV
> if PSR becomes active. This has led to drm_wait_vblank timing out.
> 
> Let's disable PSR using the vblank prepare hook that gets called
> before
> enabling vblank interrupts and keep it disabled until the interrupts
> are
> not needed.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/i915_irq.c  | 12 ++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_psr.c | 61
> +++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index e4c8e34..03f311e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -994,6 +994,7 @@ struct i915_psr {
>  	bool psr2_support;
>  	bool aux_frame_sync;
>  	bool link_standby;
> +	bool vlv_src_timing;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index caaf1e2..77f3d76 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct
> drm_device *dev, unsigned int pipe)
>  	return 0;
>  }
>  
> +static void valleyview_prepare_vblank(struct drm_device *dev,
> unsigned int pipe)
> +{

shouldn't we force psr_exit here? What if vblank is enabled after psr
is already in active mode?

> +	vlv_psr_src_timing_get(dev);
> +}
> +
> +static void valleyview_unprepare_vblank(struct drm_device *dev,
> unsigned int pipe){
> +
> +	vlv_psr_src_timing_put(dev);
> +}
> +
>  /* Called from drm generic code, passed 'crtc' which
>   * we use as a pipe index
>   */
> @@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private
> *dev_priv)
>  		dev->driver->irq_uninstall =
> cherryview_irq_uninstall;
>  		dev->driver->enable_vblank =
> valleyview_enable_vblank;
>  		dev->driver->disable_vblank =
> valleyview_disable_vblank;
> +		dev->driver->prepare_vblank =
> valleyview_prepare_vblank;
> +		dev->driver->unprepare_vblank =
> valleyview_unprepare_vblank;
>  		dev_priv->display.hpd_irq_setup =
> i915_hpd_irq_setup;
>  	} else if (IS_VALLEYVIEW(dev_priv)) {
>  		dev->driver->irq_handler = valleyview_irq_handler;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 9b5f663..e2078fd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device *dev,
>  void intel_psr_init(struct drm_device *dev);
>  void intel_psr_single_frame_update(struct drm_device *dev,
>  				   unsigned frontbuffer_bits);
> +void vlv_psr_src_timing_get(struct drm_device *dev);
> +void vlv_psr_src_timing_put(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_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 29a09bf..c95e680 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  
>  		/* Enable PSR on the panel */
>  		vlv_psr_enable_sink(intel_dp);
> +		dev_priv->psr.vlv_src_timing = false;
>  
>  		/* On HSW+ enable_source also means go to PSR
> entry/active
>  		 * state as soon as idle_frame achieved and here
> would be
> @@ -608,8 +609,10 @@ static void intel_psr_work(struct work_struct
> *work)
>  	 * The delayed work can race with an invalidate hence we
> need to
>  	 * recheck. Since psr_flush first clears this and then
> reschedules we
>  	 * won't ever miss a flush when bailing out here.
> +	 * Also, do not enable PSR if source is required to generate
> timing
> +	 * signals like vblanks.
>  	 */
> -	if (dev_priv->psr.busy_frontbuffer_bits)
> +	if (dev_priv->psr.busy_frontbuffer_bits || dev_priv
> ->psr.vlv_src_timing)
>  		goto unlock;

I wonder if in this vlv_src_timing case the work should re-schedule
itself... otherwise we have the risk of psr staying disabled forever
right?


>  
>  	intel_psr_activate(intel_dp);
> @@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct
> drm_device *dev,
>  }
>  
>  /**
> + * vlv_psr_src_timing_get - src timing generation requested
> + *
> + * CHV does not have HW tracking to trigger PSR exit when VBI are
> enabled nor
> + * does enabling vblank interrupts prevent PSR entry. This function
> is called
> + * before enabling VBI to exit PSR and prevent PSR re-entry until
> vblanks are
> + * disabled again.
> + */
> +void vlv_psr_src_timing_get(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +        if (!dev_priv->psr.enabled) {
> +                mutex_unlock(&dev_priv->psr.lock);
> +                return;
> +	}
> +
> +	//Handle racing with intel_psr_work with this flag

Is this comment permanent? if so you should use /**/

> +	dev_priv->psr.vlv_src_timing = true;
> +
> +	if(dev_priv->psr.active)
> +		intel_psr_exit(dev);
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +
> +}
> +
> +
> +/**
> + * vlv_psr_src_timing_put - src timing generation not required
> + *
> + * CHV does not have HW tracking to trigger PSR exit when VBI are
> enabled nor
> + * does enabling vblank interrupts prevent PSR entry. This function
> is called
> + * when VBI are not required and PSR can be activated.
> + */
> +void vlv_psr_src_timing_put(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	if (!dev_priv->psr.enabled) {
> +                mutex_unlock(&dev_priv->psr.lock);
> +                return;
> +        }
> +
> +	dev_priv->psr.vlv_src_timing = false;
> +
> +	if (!dev_priv->psr.active)
> +                if (!work_busy(&dev_priv->psr.work.work))
> +			schedule_delayed_work(&dev_priv->psr.work,
> +                                             
>  msecs_to_jiffies(100));
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +/**
>   * intel_psr_invalidate - Invalidade PSR
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/psr: Do not activate PSR when vblank interrupts are enabled

[Pandiyan, Dhinakaran]

On Tue, 2016-06-14 at 00:09 +0000, Vivi, Rodrigo wrote:
> On Wed, 2016-06-08 at 18:46 -0700, Dhinakaran Pandiyan wrote:
> > PSR in CHV, unlike HSW, can get activated even if vblanks interrupts
> > are
> > enabled. But, the pipe is not expected to generate timings signals
> > when PSR is active. Specifically, we do not get vblank interrupts in
> > CHV
> > if PSR becomes active. This has led to drm_wait_vblank timing out.
> > 
> > Let's disable PSR using the vblank prepare hook that gets called
> > before
> > enabling vblank interrupts and keep it disabled until the interrupts
> > are
> > not needed.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 +
> >  drivers/gpu/drm/i915/i915_irq.c  | 12 ++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> >  drivers/gpu/drm/i915/intel_psr.c | 61
> > +++++++++++++++++++++++++++++++++++++++-
> >  4 files changed, 75 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index e4c8e34..03f311e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -994,6 +994,7 @@ struct i915_psr {
> >  	bool psr2_support;
> >  	bool aux_frame_sync;
> >  	bool link_standby;
> > +	bool vlv_src_timing;
> >  };
> >  
> >  enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index caaf1e2..77f3d76 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct
> > drm_device *dev, unsigned int pipe)
> >  	return 0;
> >  }
> >  
> > +static void valleyview_prepare_vblank(struct drm_device *dev,
> > unsigned int pipe)
> > +{
> 
> shouldn't we force psr_exit here? What if vblank is enabled after psr
> is already in active mode?
> 

vlv_psr_src_timing_get calls intel_psr_exit if PSR is already active.

> > +	vlv_psr_src_timing_get(dev);
> > +}
> > +
> > +static void valleyview_unprepare_vblank(struct drm_device *dev,
> > unsigned int pipe){
> > +
> > +	vlv_psr_src_timing_put(dev);
> > +}
> > +
> >  /* Called from drm generic code, passed 'crtc' which
> >   * we use as a pipe index
> >   */
> > @@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private
> > *dev_priv)
> >  		dev->driver->irq_uninstall =
> > cherryview_irq_uninstall;
> >  		dev->driver->enable_vblank =
> > valleyview_enable_vblank;
> >  		dev->driver->disable_vblank =
> > valleyview_disable_vblank;
> > +		dev->driver->prepare_vblank =
> > valleyview_prepare_vblank;
> > +		dev->driver->unprepare_vblank =
> > valleyview_unprepare_vblank;
> >  		dev_priv->display.hpd_irq_setup =
> > i915_hpd_irq_setup;
> >  	} else if (IS_VALLEYVIEW(dev_priv)) {
> >  		dev->driver->irq_handler = valleyview_irq_handler;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 9b5f663..e2078fd 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device *dev,
> >  void intel_psr_init(struct drm_device *dev);
> >  void intel_psr_single_frame_update(struct drm_device *dev,
> >  				   unsigned frontbuffer_bits);
> > +void vlv_psr_src_timing_get(struct drm_device *dev);
> > +void vlv_psr_src_timing_put(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_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 29a09bf..c95e680 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
> >  
> >  		/* Enable PSR on the panel */
> >  		vlv_psr_enable_sink(intel_dp);
> > +		dev_priv->psr.vlv_src_timing = false;
> >  
> >  		/* On HSW+ enable_source also means go to PSR
> > entry/active
> >  		 * state as soon as idle_frame achieved and here
> > would be
> > @@ -608,8 +609,10 @@ static void intel_psr_work(struct work_struct
> > *work)
> >  	 * The delayed work can race with an invalidate hence we
> > need to
> >  	 * recheck. Since psr_flush first clears this and then
> > reschedules we
> >  	 * won't ever miss a flush when bailing out here.
> > +	 * Also, do not enable PSR if source is required to generate
> > timing
> > +	 * signals like vblanks.
> >  	 */
> > -	if (dev_priv->psr.busy_frontbuffer_bits)
> > +	if (dev_priv->psr.busy_frontbuffer_bits || dev_priv
> > ->psr.vlv_src_timing)
> >  		goto unlock;
> 
> I wonder if in this vlv_src_timing case the work should re-schedule
> itself... otherwise we have the risk of psr staying disabled forever
> right?
> 
> 
intel_psr_work gets rescheduled when vblanks are disabled in
vlv_psr_src_timing_get

> >  
> >  	intel_psr_activate(intel_dp);
> > @@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct
> > drm_device *dev,
> >  }
> >  
> >  /**
> > + * vlv_psr_src_timing_get - src timing generation requested
> > + *
> > + * CHV does not have HW tracking to trigger PSR exit when VBI are
> > enabled nor
> > + * does enabling vblank interrupts prevent PSR entry. This function
> > is called
> > + * before enabling VBI to exit PSR and prevent PSR re-entry until
> > vblanks are
> > + * disabled again.
> > + */
> > +void vlv_psr_src_timing_get(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +        if (!dev_priv->psr.enabled) {
> > +                mutex_unlock(&dev_priv->psr.lock);
> > +                return;
> > +	}
> > +
> > +	//Handle racing with intel_psr_work with this flag
> 
> Is this comment permanent? if so you should use /**/
> 
I will fix this and send a new version.
> > +	dev_priv->psr.vlv_src_timing = true;
> > +
> > +	if(dev_priv->psr.active)
> > +		intel_psr_exit(dev);
> > +
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +
> > +}
> > +
> > +
> > +/**
> > + * vlv_psr_src_timing_put - src timing generation not required
> > + *
> > + * CHV does not have HW tracking to trigger PSR exit when VBI are
> > enabled nor
> > + * does enabling vblank interrupts prevent PSR entry. This function
> > is called
> > + * when VBI are not required and PSR can be activated.
> > + */
> > +void vlv_psr_src_timing_put(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +	if (!dev_priv->psr.enabled) {
> > +                mutex_unlock(&dev_priv->psr.lock);
> > +                return;
> > +        }
> > +
> > +	dev_priv->psr.vlv_src_timing = false;
> > +
> > +	if (!dev_priv->psr.active)
> > +                if (!work_busy(&dev_priv->psr.work.work))
> > +			schedule_delayed_work(&dev_priv->psr.work,
> > +                                             
> >  msecs_to_jiffies(100));
> > +
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +}
> > +
> > +/**
> >   * intel_psr_invalidate - Invalidade PSR
> >   * @dev: DRM device
> >   * @frontbuffer_bits: frontbuffer plane tracking bits

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/psr: Do not activate PSR when vblank interrupts are enabled

[Vivi, Rodrigo]

On Wed, 2016-06-15 at 01:02 +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2016-06-14 at 00:09 +0000, Vivi, Rodrigo wrote:
> > On Wed, 2016-06-08 at 18:46 -0700, Dhinakaran Pandiyan wrote:
> > > PSR in CHV, unlike HSW, can get activated even if vblanks
> > > interrupts
> > > are
> > > enabled. But, the pipe is not expected to generate timings
> > > signals
> > > when PSR is active. Specifically, we do not get vblank interrupts
> > > in
> > > CHV
> > > if PSR becomes active. This has led to drm_wait_vblank timing
> > > out.
> > > 
> > > Let's disable PSR using the vblank prepare hook that gets called
> > > before
> > > enabling vblank interrupts and keep it disabled until the
> > > interrupts
> > > are
> > > not needed.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > >
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  1 +
> > >  drivers/gpu/drm/i915/i915_irq.c  | 12 ++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> > >  drivers/gpu/drm/i915/intel_psr.c | 61
> > > +++++++++++++++++++++++++++++++++++++++-
> > >  4 files changed, 75 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index e4c8e34..03f311e 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -994,6 +994,7 @@ struct i915_psr {
> > >  	bool psr2_support;
> > >  	bool aux_frame_sync;
> > >  	bool link_standby;
> > > +	bool vlv_src_timing;
> > >  };
> > >  
> > >  enum intel_pch {
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > b/drivers/gpu/drm/i915/i915_irq.c
> > > index caaf1e2..77f3d76 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct
> > > drm_device *dev, unsigned int pipe)
> > >  	return 0;
> > >  }
> > >  
> > > +static void valleyview_prepare_vblank(struct drm_device *dev,
> > > unsigned int pipe)
> > > +{
> > 
> > shouldn't we force psr_exit here? What if vblank is enabled after
> > psr
> > is already in active mode?
> > 
> 
> vlv_psr_src_timing_get calls intel_psr_exit if PSR is already active.

oh it is already there indeed...
i'm crazy, sorry...

I liked the solution... really simple and straightforward... so simple
that confused me hehe

> 
> > > +	vlv_psr_src_timing_get(dev);
> > > +}
> > > +
> > > +static void valleyview_unprepare_vblank(struct drm_device *dev,
> > > unsigned int pipe){
> > > +
> > > +	vlv_psr_src_timing_put(dev);
> > > +}
> > > +
> > >  /* Called from drm generic code, passed 'crtc' which
> > >   * we use as a pipe index
> > >   */
> > > @@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private
> > > *dev_priv)
> > >  		dev->driver->irq_uninstall =
> > > cherryview_irq_uninstall;
> > >  		dev->driver->enable_vblank =
> > > valleyview_enable_vblank;
> > >  		dev->driver->disable_vblank =
> > > valleyview_disable_vblank;
> > > +		dev->driver->prepare_vblank =
> > > valleyview_prepare_vblank;
> > > +		dev->driver->unprepare_vblank =
> > > valleyview_unprepare_vblank;
> > >  		dev_priv->display.hpd_irq_setup =
> > > i915_hpd_irq_setup;
> > >  	} else if (IS_VALLEYVIEW(dev_priv)) {
> > >  		dev->driver->irq_handler =
> > > valleyview_irq_handler;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 9b5f663..e2078fd 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device
> > > *dev,
> > >  void intel_psr_init(struct drm_device *dev);
> > >  void intel_psr_single_frame_update(struct drm_device *dev,
> > >  				   unsigned frontbuffer_bits);
> > > +void vlv_psr_src_timing_get(struct drm_device *dev);
> > > +void vlv_psr_src_timing_put(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_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 29a09bf..c95e680 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp
> > > *intel_dp)
> > >  
> > >  		/* Enable PSR on the panel */
> > >  		vlv_psr_enable_sink(intel_dp);
> > > +		dev_priv->psr.vlv_src_timing = false;
> > >  
> > >  		/* On HSW+ enable_source also means go to PSR
> > > entry/active
> > >  		 * state as soon as idle_frame achieved and here
> > > would be
> > > @@ -608,8 +609,10 @@ static void intel_psr_work(struct
> > > work_struct
> > > *work)
> > >  	 * The delayed work can race with an invalidate hence we
> > > need to
> > >  	 * recheck. Since psr_flush first clears this and then
> > > reschedules we
> > >  	 * won't ever miss a flush when bailing out here.
> > > +	 * Also, do not enable PSR if source is required to
> > > generate
> > > timing
> > > +	 * signals like vblanks.
> > >  	 */
> > > -	if (dev_priv->psr.busy_frontbuffer_bits)
> > > +	if (dev_priv->psr.busy_frontbuffer_bits || dev_priv
> > > ->psr.vlv_src_timing)
> > >  		goto unlock;
> > 
> > I wonder if in this vlv_src_timing case the work should re-schedule
> > itself... otherwise we have the risk of psr staying disabled
> > forever
> > right?
> > 
> > 
> intel_psr_work gets rescheduled when vblanks are disabled in
> vlv_psr_src_timing_get
> 
> > >  
> > >  	intel_psr_activate(intel_dp);
> > > @@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct
> > > drm_device *dev,
> > >  }
> > >  
> > >  /**
> > > + * vlv_psr_src_timing_get - src timing generation requested
> > > + *
> > > + * CHV does not have HW tracking to trigger PSR exit when VBI
> > > are
> > > enabled nor
> > > + * does enabling vblank interrupts prevent PSR entry. This
> > > function
> > > is called
> > > + * before enabling VBI to exit PSR and prevent PSR re-entry
> > > until
> > > vblanks are
> > > + * disabled again.
> > > + */
> > > +void vlv_psr_src_timing_get(struct drm_device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > +	mutex_lock(&dev_priv->psr.lock);
> > > +        if (!dev_priv->psr.enabled) {
> > > +                mutex_unlock(&dev_priv->psr.lock);
> > > 
> > > +                return;
> > > +	}
> > > +
> > > +	//Handle racing with intel_psr_work with this flag
> > 
> > Is this comment permanent? if so you should use /**/
> > 
> I will fix this and send a new version.

with this fix feel free to use
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Also I believe with this series merge we would be good to re-enable psr
on VLV/CHV...

more random thoughts below... 

> > > +	dev_priv->psr.vlv_src_timing = true;
> > > +
> > > +	if(dev_priv->psr.active)
> > > +		intel_psr_exit(dev);
> > > +
> > > +	mutex_unlock(&dev_priv->psr.lock);
> > > +
> > > +}
> > > +
> > > +
> > > +/**
> > > + * vlv_psr_src_timing_put - src timing generation not required
> > > + *
> > > + * CHV does not have HW tracking to trigger PSR exit when VBI
> > > are
> > > enabled nor
> > > + * does enabling vblank interrupts prevent PSR entry. This
> > > function
> > > is called
> > > + * when VBI are not required and PSR can be activated.
> > > + */
> > > +void vlv_psr_src_timing_put(struct drm_device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > +	mutex_lock(&dev_priv->psr.lock);
> > > +	if (!dev_priv->psr.enabled) {
> > > +                mutex_unlock(&dev_priv->psr.lock);
> > > +                return;
> > > +        }
> > > +
> > > +	dev_priv->psr.vlv_src_timing = false;
> > > +
> > > +	if (!dev_priv->psr.active)
> > > +                if (!work_busy(&dev_priv->psr.work.work))
> > > +			schedule_delayed_work(&dev_priv
> > > ->psr.work,

here I wondered it would be better to call the vlv_psr_activate
directly, but the work function handle some restrictions so your
solution is better...

> +                                             
> > >  msecs_to_jiffies(100));

just asking myself yet again if we should base this timeout in vblank
time...

one way or another maybe it is good to save it in psr struct to keep it
consistent in case we need to change someday...  but not a requirement
at all, just a random thought...

Thanks,
Rodrigo.

> > > +
> > > +	mutex_unlock(&dev_priv->psr.lock);
> > > +}
> > > +
> > > +/**
> > >   * intel_psr_invalidate - Invalidade PSR
> > >   * @dev: DRM device
> > >   * @frontbuffer_bits: frontbuffer plane tracking bits
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/psr: Do not activate PSR when vblank interrupts are enabled

[Pandiyan, Dhinakaran]

On Wed, 2016-06-15 at 21:44 +0000, Vivi, Rodrigo wrote:
> On Wed, 2016-06-15 at 01:02 +0000, Pandiyan, Dhinakaran wrote:
> > On Tue, 2016-06-14 at 00:09 +0000, Vivi, Rodrigo wrote:
> > > On Wed, 2016-06-08 at 18:46 -0700, Dhinakaran Pandiyan wrote:
> > > > PSR in CHV, unlike HSW, can get activated even if vblanks
> > > > interrupts
> > > > are
> > > > enabled. But, the pipe is not expected to generate timings
> > > > signals
> > > > when PSR is active. Specifically, we do not get vblank interrupts
> > > > in
> > > > CHV
> > > > if PSR becomes active. This has led to drm_wait_vblank timing
> > > > out.
> > > > 
> > > > Let's disable PSR using the vblank prepare hook that gets called
> > > > before
> > > > enabling vblank interrupts and keep it disabled until the
> > > > interrupts
> > > > are
> > > > not needed.
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > >
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h  |  1 +
> > > >  drivers/gpu/drm/i915/i915_irq.c  | 12 ++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> > > >  drivers/gpu/drm/i915/intel_psr.c | 61
> > > > +++++++++++++++++++++++++++++++++++++++-
> > > >  4 files changed, 75 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index e4c8e34..03f311e 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -994,6 +994,7 @@ struct i915_psr {
> > > >  	bool psr2_support;
> > > >  	bool aux_frame_sync;
> > > >  	bool link_standby;
> > > > +	bool vlv_src_timing;
> > > >  };
> > > >  
> > > >  enum intel_pch {
> > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > index caaf1e2..77f3d76 100644
> > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > @@ -2790,6 +2790,16 @@ static int gen8_enable_vblank(struct
> > > > drm_device *dev, unsigned int pipe)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static void valleyview_prepare_vblank(struct drm_device *dev,
> > > > unsigned int pipe)
> > > > +{
> > > 
> > > shouldn't we force psr_exit here? What if vblank is enabled after
> > > psr
> > > is already in active mode?
> > > 
> > 
> > vlv_psr_src_timing_get calls intel_psr_exit if PSR is already active.
> 
> oh it is already there indeed...
> i'm crazy, sorry...
> 
> I liked the solution... really simple and straightforward... so simple
> that confused me hehe
> 
> > 
> > > > +	vlv_psr_src_timing_get(dev);
> > > > +}
> > > > +
> > > > +static void valleyview_unprepare_vblank(struct drm_device *dev,
> > > > unsigned int pipe){
> > > > +
> > > > +	vlv_psr_src_timing_put(dev);
> > > > +}
> > > > +
> > > >  /* Called from drm generic code, passed 'crtc' which
> > > >   * we use as a pipe index
> > > >   */
> > > > @@ -4610,6 +4620,8 @@ void intel_irq_init(struct drm_i915_private
> > > > *dev_priv)
> > > >  		dev->driver->irq_uninstall =
> > > > cherryview_irq_uninstall;
> > > >  		dev->driver->enable_vblank =
> > > > valleyview_enable_vblank;
> > > >  		dev->driver->disable_vblank =
> > > > valleyview_disable_vblank;
> > > > +		dev->driver->prepare_vblank =
> > > > valleyview_prepare_vblank;
> > > > +		dev->driver->unprepare_vblank =
> > > > valleyview_unprepare_vblank;
> > > >  		dev_priv->display.hpd_irq_setup =
> > > > i915_hpd_irq_setup;
> > > >  	} else if (IS_VALLEYVIEW(dev_priv)) {
> > > >  		dev->driver->irq_handler =
> > > > valleyview_irq_handler;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 9b5f663..e2078fd 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1511,6 +1511,8 @@ void intel_psr_flush(struct drm_device
> > > > *dev,
> > > >  void intel_psr_init(struct drm_device *dev);
> > > >  void intel_psr_single_frame_update(struct drm_device *dev,
> > > >  				   unsigned frontbuffer_bits);
> > > > +void vlv_psr_src_timing_get(struct drm_device *dev);
> > > > +void vlv_psr_src_timing_put(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_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 29a09bf..c95e680 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -462,6 +462,7 @@ void intel_psr_enable(struct intel_dp
> > > > *intel_dp)
> > > >  
> > > >  		/* Enable PSR on the panel */
> > > >  		vlv_psr_enable_sink(intel_dp);
> > > > +		dev_priv->psr.vlv_src_timing = false;
> > > >  
> > > >  		/* On HSW+ enable_source also means go to PSR
> > > > entry/active
> > > >  		 * state as soon as idle_frame achieved and here
> > > > would be
> > > > @@ -608,8 +609,10 @@ static void intel_psr_work(struct
> > > > work_struct
> > > > *work)
> > > >  	 * The delayed work can race with an invalidate hence we
> > > > need to
> > > >  	 * recheck. Since psr_flush first clears this and then
> > > > reschedules we
> > > >  	 * won't ever miss a flush when bailing out here.
> > > > +	 * Also, do not enable PSR if source is required to
> > > > generate
> > > > timing
> > > > +	 * signals like vblanks.
> > > >  	 */
> > > > -	if (dev_priv->psr.busy_frontbuffer_bits)
> > > > +	if (dev_priv->psr.busy_frontbuffer_bits || dev_priv
> > > > ->psr.vlv_src_timing)
> > > >  		goto unlock;
> > > 
> > > I wonder if in this vlv_src_timing case the work should re-schedule
> > > itself... otherwise we have the risk of psr staying disabled
> > > forever
> > > right?
> > > 
> > > 
> > intel_psr_work gets rescheduled when vblanks are disabled in
> > vlv_psr_src_timing_get
> > 
> > > >  
> > > >  	intel_psr_activate(intel_dp);
> > > > @@ -708,6 +711,62 @@ void intel_psr_single_frame_update(struct
> > > > drm_device *dev,
> > > >  }
> > > >  
> > > >  /**
> > > > + * vlv_psr_src_timing_get - src timing generation requested
> > > > + *
> > > > + * CHV does not have HW tracking to trigger PSR exit when VBI
> > > > are
> > > > enabled nor
> > > > + * does enabling vblank interrupts prevent PSR entry. This
> > > > function
> > > > is called
> > > > + * before enabling VBI to exit PSR and prevent PSR re-entry
> > > > until
> > > > vblanks are
> > > > + * disabled again.
> > > > + */
> > > > +void vlv_psr_src_timing_get(struct drm_device *dev)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +
> > > > +	mutex_lock(&dev_priv->psr.lock);
> > > > +        if (!dev_priv->psr.enabled) {
> > > > +                mutex_unlock(&dev_priv->psr.lock);
> > > > 
> > > > +                return;
> > > > +	}
> > > > +
> > > > +	//Handle racing with intel_psr_work with this flag
> > > 
> > > Is this comment permanent? if so you should use /**/
> > > 
> > I will fix this and send a new version.
> 
> with this fix feel free to use
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Also I believe with this series merge we would be good to re-enable psr
> on VLV/CHV...
> 
> more random thoughts below... 
> 
> > > > +	dev_priv->psr.vlv_src_timing = true;
> > > > +
> > > > +	if(dev_priv->psr.active)
> > > > +		intel_psr_exit(dev);
> > > > +
> > > > +	mutex_unlock(&dev_priv->psr.lock);
> > > > +
> > > > +}
> > > > +
> > > > +
> > > > +/**
> > > > + * vlv_psr_src_timing_put - src timing generation not required
> > > > + *
> > > > + * CHV does not have HW tracking to trigger PSR exit when VBI
> > > > are
> > > > enabled nor
> > > > + * does enabling vblank interrupts prevent PSR entry. This
> > > > function
> > > > is called
> > > > + * when VBI are not required and PSR can be activated.
> > > > + */
> > > > +void vlv_psr_src_timing_put(struct drm_device *dev)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +
> > > > +	mutex_lock(&dev_priv->psr.lock);
> > > > +	if (!dev_priv->psr.enabled) {
> > > > +                mutex_unlock(&dev_priv->psr.lock);
> > > > +                return;
> > > > +        }
> > > > +
> > > > +	dev_priv->psr.vlv_src_timing = false;
> > > > +
> > > > +	if (!dev_priv->psr.active)
> > > > +                if (!work_busy(&dev_priv->psr.work.work))
> > > > +			schedule_delayed_work(&dev_priv
> > > > ->psr.work,
> 
> here I wondered it would be better to call the vlv_psr_activate
> directly, but the work function handle some restrictions so your
> solution is better...
> 
> > +                                             
> > > >  msecs_to_jiffies(100));
> 
> just asking myself yet again if we should base this timeout in vblank
> time...
> 
> one way or another maybe it is good to save it in psr struct to keep it
> consistent in case we need to change someday...  but not a requirement
> at all, just a random thought...
> 
> Thanks,
> Rodrigo.
> 
> > > > +
> > > > +	mutex_unlock(&dev_priv->psr.lock);
> > > > +}
> > > > +
> > > > +/**
> > > >   * intel_psr_invalidate - Invalidade PSR
> > > >   * @dev: DRM device
> > > >   * @frontbuffer_bits: frontbuffer plane tracking bits
> > 

Thanks for the review Rodrigo. 
Unfortunately, I found some IGT failures because of race conditions, I
will fix that and send another version.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for CHV vblank failures when PSR is active

[Patchwork]

== Series Details ==

Series: CHV vblank failures when PSR is active
URL   : https://patchwork.freedesktop.org/series/8477/
State : failure

== Summary ==

Applying: drm: Add vblank prepare and unprepare hooks.
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/drm_irq.c
M	include/drm/drmP.h
Falling back to patching base and 3-way merge...
Auto-merging include/drm/drmP.h
Auto-merging drivers/gpu/drm/drm_irq.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/drm_irq.c
error: Failed to merge in the changes.
Patch failed at 0001 drm: Add vblank prepare and unprepare hooks.
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] CHV vblank failures when PSR is active

[Jani Nikula]

On Thu, 09 Jun 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> IGT vblank tests fail on CHV by timing out on VBIs if PSR is enabled. We
> do not get VBIs as the source timing generation is disabled when PSR is
> active. The first two patches written by Rodrigo add drm hooks. Patch 3
> deactivates PSR when VBI are needed.

The first thing to do would be to submit a patch that disables PSR by
default on CHV. We can enable it again if and when these patches land.

BR,
Jani.


>
> [PATCH 1/3] drm: Add vblank prepare and unprepare hooks.
> [PATCH 2/3] drm/i915: Move drm_crtc_vblank_get out of disabled
> [PATCH 3/3] drm/i915/psr: Do not activate PSR when vblank interrupts
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] CHV vblank failures when PSR is active

[Ville Syrjälä]

On Thu, Jun 09, 2016 at 11:31:20AM +0300, Jani Nikula wrote:
> On Thu, 09 Jun 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> > IGT vblank tests fail on CHV by timing out on VBIs if PSR is enabled. We
> > do not get VBIs as the source timing generation is disabled when PSR is
> > active. The first two patches written by Rodrigo add drm hooks. Patch 3
> > deactivates PSR when VBI are needed.
> 
> The first thing to do would be to submit a patch that disables PSR by
> default on CHV. We can enable it again if and when these patches land.

I already reverted the "enable by default" patch some time ago.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: warning for CHV vblank failures when PSR is active (rev2)

[Patchwork]

== Series Details ==

Series: CHV vblank failures when PSR is active (rev2)
URL   : https://patchwork.freedesktop.org/series/8477/
State : warning

== Summary ==

Series 8477v2 CHV vblank failures when PSR is active
http://patchwork.freedesktop.org/api/1.0/series/8477/revisions/2/mbox

Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                dmesg-warn -> PASS       (ro-hsw-i3-4010u)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> SKIP       (fi-skl-i5-6260u)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                skip       -> PASS       (fi-skl-i5-6260u)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
                skip       -> PASS       (fi-skl-i5-6260u)

fi-bdw-i7-5557u  total:213  pass:201  dwarn:0   dfail:0   fail:0   skip:12 
fi-skl-i5-6260u  total:213  pass:201  dwarn:0   dfail:0   fail:0   skip:12 
fi-skl-i7-6700k  total:213  pass:188  dwarn:0   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:213  pass:174  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i5-5250u  total:213  pass:197  dwarn:1   dfail:0   fail:0   skip:15 
ro-bdw-i7-5557U  total:213  pass:198  dwarn:0   dfail:0   fail:0   skip:15 
ro-bdw-i7-5600u  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-bsw-n3050     total:213  pass:171  dwarn:1   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:213  pass:173  dwarn:0   dfail:0   fail:3   skip:37 
ro-hsw-i3-4010u  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-ilk-i7-620lm  total:213  pass:150  dwarn:0   dfail:0   fail:1   skip:62 
ro-ilk1-i5-650   total:208  pass:150  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:213  pass:181  dwarn:0   dfail:0   fail:0   skip:32 
ro-ivb2-i7-3770  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-skl3-i5-6260u total:213  pass:201  dwarn:1   dfail:0   fail:0   skip:11 
ro-snb-i7-2620M  total:213  pass:174  dwarn:0   dfail:0   fail:1   skip:38 
fi-hsw-i7-4770k failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1164/

94fd582 drm-intel-nightly: 2016y-06m-10d-16h-42m-36s UTC integration manifest
d2535c2 drm/i915/psr: Do not activate PSR when vblank interrupts are enabled
2a0a033 drm/i915: Move drm_crtc_vblank_get out of disabled pre-emption area.
dbbdf6c REBASED: [PATCH 1/3] drm: Add vblank prepare and unprepare hooks.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx