[PATCH 0/6] Allow DC state to reset the counter on screen enabled.

[PATCH 0/6] Allow DC state to reset the counter on screen enabled.

[Rodrigo Vivi]

For now DC is only helping on screen off scenarios since PSR is disabled.

But if we want to enable PSR first we need to make DC reliable with screen on.
Biggest challenge is to deal with vblank counters since frame counter register
is read only and can be reset in DC state.

This series is one of possible approaches, but brings the down side of not
being possible to use runtime pm with vblank enabled. Some test cases needs
to be adapted to represent this new vision.

But also this series is not fully tested. Apparently I have an issue yet
with flip-vs-expired-vblank_* tests and pm_rpm basic tests.

So, while I investigate and finish the test execution I'd like to get some
feedback on this approach. This is why I'm sending this series right now.

Please let me know if this is acceptable or if you have any better aproach
ideas or any idea about the test failures above.

Thanks a lot,
Rodrigo.

Rodrigo Vivi (6):
  drm: Add vblank prepare and unprepare hooks.
  drm/i915: Move drm_crtc_vblank_get out of disabled pre-emption area.
  drm/i915: Split gen 9 irq hooks definitions.
  drm/i915: Introduce vblank power domain to avoid DC entry when waiting
    for vblank.
  drm: Introduce drm_crtc_vblank_sanitize_counter.
  drm/i915: Sanitize drm crtc vblank counter after DC reset frame count.

 drivers/gpu/drm/drm_irq.c               | 83 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/i915_irq.c         | 28 +++++++++--
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++
 drivers/gpu/drm/i915/intel_sprite.c     |  8 ++--
 include/drm/drmP.h                      | 43 +++++++++++++++++
 include/drm/drm_irq.h                   | 21 +++++++++
 7 files changed, 179 insertions(+), 8 deletions(-)

-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rodrigo Vivi]

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.

v2: - Rebase after a split on drm_irq.h.
    - Use refcount to minimize the prepare/unprepare calls to
      only when enabling or disabling the vblank irq. (DK's)
    - Improved doc.
    - Fix the negative unprepare.count case.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/drm_irq.c | 37 ++++++++++++++++++++++++++++++++++++-
 include/drm/drmP.h        | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_irq.h     | 20 ++++++++++++++++++++
 3 files changed, 99 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 77f357b..d0d1dde 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -339,6 +339,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);
@@ -347,6 +349,24 @@ 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;
+
+	if (atomic_read(&vblank->unprepare.counter) == 0)
+		return;
+
+	do {
+		if (dev->driver->unprepare_vblank &&
+		    atomic_read(&vblank->refcount) == 0)
+			dev->driver->unprepare_vblank(dev, vblank->pipe);
+	} while (!atomic_dec_and_test(&vblank->unprepare.counter));
+}
+
 /**
  * drm_vblank_init - initialize vblank support
  * @dev: DRM device
@@ -380,6 +400,9 @@ 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");
@@ -1117,6 +1140,10 @@ static 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 &&
+	    atomic_read(&vblank->refcount) == 0)
+		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) {
@@ -1129,6 +1156,10 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 	}
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 
+	if (dev->driver->unprepare_vblank &&
+	    atomic_read(&vblank->refcount) == 0)
+		dev->driver->unprepare_vblank(dev, pipe);
+
 	return ret;
 }
 
@@ -1178,6 +1209,11 @@ static void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
 			mod_timer(&vblank->disable_timer,
 				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
 	}
+
+	if (atomic_read(&vblank->refcount) == 0) {
+		atomic_inc(&vblank->unprepare.counter);
+		schedule_work(&vblank->unprepare.work);
+	}
 }
 
 /**
@@ -1603,7 +1639,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 d377865..c6ca061 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -422,6 +422,49 @@ 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.
+	 *
+	 * Prepare/Unprepare was designed to be able to sleep so they are not
+	 * free from concurrence. There might be few simultaneous calls.
+	 * This concurrence needs to be handled on the driver side that is
+	 * implementing these hooks. Probably with protected get/put counters.
+	 *
+	 * If this is used with sleeping code you need to make sure you are
+	 * not calling drm_crtc_vblank_get inside spinlock areas or with local
+	 * irq disabled.
+	 *
+	 */
+	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.
+	 *
+	 * Prepare/Unprepare was designed to be able to sleep so they are not
+	 * free from concurrence. There might be few simultaneous calls.
+	 * This concurrence needs to be handled on the driver side that is
+	 * implementing these hooks. Probably with protected get/put counters.
+	 *
+	 * Unprepare is called from a workqueue because asynchronous
+	 * drm_crtc_vblank_put will be in spinlock areas. So it is safe to
+	 * keep drm_crtc_vblank_put in areas that cannot spleep.
+	 */
+	void (*unprepare_vblank) (struct drm_device *dev, unsigned int pipe);
+
+
+	/**
 	 * enable_vblank - enable vblank interrupt events
 	 * @dev: DRM device
 	 * @pipe: which irq to enable
diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
index 2401b14..93a9e9d 100644
--- a/include/drm/drm_irq.h
+++ b/include/drm/drm_irq.h
@@ -45,6 +45,20 @@ struct drm_pending_vblank_event {
 };
 
 /**
+ * struct drm_vblank_unprepare - unprepare vblank
+ */
+struct drm_vblank_unprepare {
+	/**
+	 * @work: Delayed work to handle unprepare out of spinlock areas
+	 */
+	struct work_struct work;
+	/**
+	 * @counter: Number of vblanks handled
+	 */
+	atomic_t counter;
+};
+
+/**
  * struct drm_vblank_crtc - vblank tracking for a CRTC
  *
  * This structure tracks the vblank state for one CRTC.
@@ -128,6 +142,12 @@ struct drm_vblank_crtc {
 	 * disabling functions multiple times.
 	 */
 	bool enabled;
+	/**
+	 * @unprepare: Tracks the amount of drm_vblank_put handled that needs
+	 * a delayed unprepare call so the unprepare can run out of protected
+	 * areas where code can sleep.
+	 */
+	struct drm_vblank_unprepare unprepare;
 };
 
 extern int drm_irq_install(struct drm_device *dev, int irq);
-- 
2.4.3

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

Re: [Intel-gfx] [PATCH 1/6] drm: Add vblank prepare and unprepare hooks.

[Daniel Vetter]

On Wed, Aug 03, 2016 at 02:33:34PM -0700, Rodrigo Vivi wrote:
> 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.
> 
> v2: - Rebase after a split on drm_irq.h.
>     - Use refcount to minimize the prepare/unprepare calls to
>       only when enabling or disabling the vblank irq. (DK's)
>     - Improved doc.
>     - Fix the negative unprepare.count case.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 37 ++++++++++++++++++++++++++++++++++++-
>  include/drm/drmP.h        | 43 +++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_irq.h     | 20 ++++++++++++++++++++
>  3 files changed, 99 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 77f357b..d0d1dde 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -339,6 +339,8 @@ void drm_vblank_cleanup(struct drm_device *dev)
>  			drm_core_check_feature(dev, DRIVER_MODESET));
>  
>  		del_timer_sync(&vblank->disable_timer);

Needs a schedule_work here, since if you delete the timer and then unload
you'll leak all the unprepare calls. Which will upset a lot of things at
least on module unload.

> +
> +		flush_work(&vblank->unprepare.work);
>  	}
>  
>  	kfree(dev->vblank);
> @@ -347,6 +349,24 @@ 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;
> +
> +	if (atomic_read(&vblank->unprepare.counter) == 0)
> +		return;

while (atomic_read > 0) {
	/* do stuff */
	atomic_dec();
}

is simpler. Plus this needs a big comment that the worker here is the
_only_ code wich ever decrements the counter. Without that this would be
racy.

> +
> +	do {
> +		if (dev->driver->unprepare_vblank &&
> +		    atomic_read(&vblank->refcount) == 0)
> +			dev->driver->unprepare_vblank(dev, vblank->pipe);
> +	} while (!atomic_dec_and_test(&vblank->unprepare.counter));
> +}
> +
>  /**
>   * drm_vblank_init - initialize vblank support
>   * @dev: DRM device
> @@ -380,6 +400,9 @@ 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");
> @@ -1117,6 +1140,10 @@ static 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 &&
> +	    atomic_read(&vblank->refcount) == 0)

This is racy. If you don't want to prepare multiple times then you need
another atomic counter. Tbh that seems overkill, I'm not sure at all why
we should avoid multiple prepare/unprepare calls.

> +		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) {
> @@ -1129,6 +1156,10 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
>  	}
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  
> +	if (dev->driver->unprepare_vblank &&
> +	    atomic_read(&vblank->refcount) == 0)

This is racy. You need to check whether _this_ vblank_get call failed -
after you dropped the lock someone else might have successfully completed
a vblank_get breaking your code here.

Note that atomic_t only means the counter stays consistent. It makes
_zero_ gurantees about ordering or anything else going on in the system,
and in general you need a comment explaining why you can access it and
expect some consistency. In the worker you've been lucky, here it's
broken. Please audit your usage of atomic_t carefully.

> +		dev->driver->unprepare_vblank(dev, pipe);
> +
>  	return ret;
>  }
>  
> @@ -1178,6 +1209,11 @@ static void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
>  			mod_timer(&vblank->disable_timer,
>  				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
>  	}
> +
> +	if (atomic_read(&vblank->refcount) == 0) {
> +		atomic_inc(&vblank->unprepare.counter);
> +		schedule_work(&vblank->unprepare.work);

So prepare is done once, but unprepare every time we put. I really don't
think the change dk suggested was all that good ;-)

Also, this needs a check for the unprepare hook, we don't want to incure
the overhead for drivers which don't need it.

> +	}
>  }
>  
>  /**
> @@ -1603,7 +1639,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 d377865..c6ca061 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -422,6 +422,49 @@ 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.
> +	 *
> +	 * Prepare/Unprepare was designed to be able to sleep so they are not
> +	 * free from concurrence. There might be few simultaneous calls.
> +	 * This concurrence needs to be handled on the driver side that is
> +	 * implementing these hooks. Probably with protected get/put counters.

"This function can be called concurrently." Says all the same things with
fewer words ;-)

But note that if you decide to implement the "prepare/unprepare only once"
logic (without bugs), then this is not true.

> +	 *
> +	 * If this is used with sleeping code you need to make sure you are
> +	 * not calling drm_crtc_vblank_get inside spinlock areas or with local
> +	 * irq disabled.

Imo this comment should be moved to drm_crtc_vblank_get under something
like this

Note:

Drivers which use the prepare_vblank hook and which can block in their
callback must ensure that they never call drm_crtc_vblank_get() from
atomic contexts. The DRM core and heleprs already make this guarantee.

Also I think this should reference the sanitize_counter function as an
example.


> +	 *
> +	 */
> +	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.
> +	 *
> +	 * Prepare/Unprepare was designed to be able to sleep so they are not
> +	 * free from concurrence. There might be few simultaneous calls.
> +	 * This concurrence needs to be handled on the driver side that is
> +	 * implementing these hooks. Probably with protected get/put counters.

This is not true, you only have one worker, there's no concurrency.

> +	 *
> +	 * Unprepare is called from a workqueue because asynchronous

asynchronous_ly_

> +	 * drm_crtc_vblank_put will be in spinlock areas. So it is safe to

s/will/can/ s/spinlock areas/atomic context/.

Also please add () at the end of every function you reference, to enable
the kerneldoc cross-linking. Please also check what make htmldocs
generates to make sure it's all good.

> +	 * keep drm_crtc_vblank_put in areas that cannot spleep.
> +	 */
> +	void (*unprepare_vblank) (struct drm_device *dev, unsigned int pipe);
> +
> +
> +	/**
>  	 * enable_vblank - enable vblank interrupt events
>  	 * @dev: DRM device
>  	 * @pipe: which irq to enable
> diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
> index 2401b14..93a9e9d 100644
> --- a/include/drm/drm_irq.h
> +++ b/include/drm/drm_irq.h
> @@ -45,6 +45,20 @@ struct drm_pending_vblank_event {
>  };
>  
>  /**
> + * struct drm_vblank_unprepare - unprepare vblank
> + */
> +struct drm_vblank_unprepare {
> +	/**
> +	 * @work: Delayed work to handle unprepare out of spinlock areas
> +	 */
> +	struct work_struct work;
> +	/**
> +	 * @counter: Number of vblanks handled
> +	 */
> +	atomic_t counter;
> +};
> +
> +/**
>   * struct drm_vblank_crtc - vblank tracking for a CRTC
>   *
>   * This structure tracks the vblank state for one CRTC.
> @@ -128,6 +142,12 @@ struct drm_vblank_crtc {
>  	 * disabling functions multiple times.
>  	 */
>  	bool enabled;
> +	/**
> +	 * @unprepare: Tracks the amount of drm_vblank_put handled that needs
> +	 * a delayed unprepare call so the unprepare can run out of protected
> +	 * areas where code can sleep.
> +	 */
> +	struct drm_vblank_unprepare unprepare;

Not sure why the substruct, I'd just call the unprepare_work and
unprepare_counter and less kernel-doc noise. We do substrucst a lot in the
i915 device sturct because it's so huge, and to be able to split up things
a bit. But in general substruct means you need to jump around more, and it
makes it harder to find definitions using tools like cscope or grep or
similar.

Cheers, Daniel
>  };
>  
>  extern int drm_irq_install(struct drm_device *dev, int irq);
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rodrigo Vivi]

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.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0de935a..d8bc27c 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -94,14 +94,14 @@ 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)
 		return;
 
 	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
 		return;
 
+	local_irq_disable();
+
 	crtc->debug.min_vbl = min;
 	crtc->debug.max_vbl = max;
 	trace_i915_pipe_update_start(crtc);
@@ -166,6 +166,8 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
 
 	trace_i915_pipe_update_end(crtc, end_vbl_count, scanline_end);
 
+	local_irq_enable();
+
 	/* We're still in the vblank-evade critical section, this can't race.
 	 * Would be slightly nice to just grab the vblank count and arm the
 	 * event outside of the critical section - the spinlock might spin for a
@@ -180,8 +182,6 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
 		crtc->base.state->event = NULL;
 	}
 
-	local_irq_enable();
-
 	if (crtc->debug.start_vbl_count &&
 	    crtc->debug.start_vbl_count != end_vbl_count) {
 		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n",
-- 
2.4.3

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

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

[Daniel Vetter]

On Wed, Aug 03, 2016 at 02:33:35PM -0700, Rodrigo Vivi wrote:
> 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.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0de935a..d8bc27c 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -94,14 +94,14 @@ 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)
>  		return;
>  
>  	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>  		return;
>  
> +	local_irq_disable();
> +
>  	crtc->debug.min_vbl = min;
>  	crtc->debug.max_vbl = max;
>  	trace_i915_pipe_update_start(crtc);
> @@ -166,6 +166,8 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>  
>  	trace_i915_pipe_update_end(crtc, end_vbl_count, scanline_end);
>  
> +	local_irq_enable();
> +
>  	/* We're still in the vblank-evade critical section, this can't race.
>  	 * Would be slightly nice to just grab the vblank count and arm the
>  	 * event outside of the critical section - the spinlock might spin for a
> @@ -180,8 +182,6 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>  		crtc->base.state->event = NULL;
>  	}
>  
> -	local_irq_enable();

You can't do this. Pls reaad the comment right above why.

What we need here is a drm_crtc_vblank_get_noresume() or similar which
will fail very loudly if the vblank refcount is 0. _noresume since that
mirroros runtime PM, but it's a really bad name.
-Daniel


> -
>  	if (crtc->debug.start_vbl_count &&
>  	    crtc->debug.start_vbl_count != end_vbl_count) {
>  		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n",
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/6] drm/i915: Split gen 9 irq hooks definitions.

[Rodrigo Vivi]

No functional change. This is just a reorg that aims to allow
a cleaner introduction of new vblank hooks for gen9+.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f5bf4f9..76d48da 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4582,7 +4582,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 		dev->driver->enable_vblank = valleyview_enable_vblank;
 		dev->driver->disable_vblank = valleyview_disable_vblank;
 		dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
-	} else if (INTEL_INFO(dev_priv)->gen >= 8) {
+	} else if (INTEL_INFO(dev_priv)->gen >= 9) {
 		dev->driver->irq_handler = gen8_irq_handler;
 		dev->driver->irq_preinstall = gen8_irq_reset;
 		dev->driver->irq_postinstall = gen8_irq_postinstall;
@@ -4593,8 +4593,14 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 			dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
 		else if (HAS_PCH_SPT(dev) || HAS_PCH_KBP(dev))
 			dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
-		else
-			dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
+	} else if (INTEL_INFO(dev_priv)->gen >= 8) {
+		dev->driver->irq_handler = gen8_irq_handler;
+		dev->driver->irq_preinstall = gen8_irq_reset;
+		dev->driver->irq_postinstall = gen8_irq_postinstall;
+		dev->driver->irq_uninstall = gen8_irq_uninstall;
+		dev->driver->enable_vblank = gen8_enable_vblank;
+		dev->driver->disable_vblank = gen8_disable_vblank;
+		dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
 	} else if (HAS_PCH_SPLIT(dev)) {
 		dev->driver->irq_handler = ironlake_irq_handler;
 		dev->driver->irq_preinstall = ironlake_irq_reset;
-- 
2.4.3

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

[PATCH 4/6] drm/i915: Introduce vblank power domain to avoid DC entry when waiting for vblank.

[Rodrigo Vivi]

Vblank counters are not restored by DMC when exiting deep DC states
because frame counter register is read-only. So it is better to avoid
Deep DC states when waiting for Vblanks. At least we don't mess with
the counters when already waiting for vblank.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/i915_irq.c         | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
 3 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fe152df..7aa87c4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -229,6 +229,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_PORT_OTHER,
 	POWER_DOMAIN_VGA,
 	POWER_DOMAIN_AUDIO,
+	POWER_DOMAIN_VBLANK,
 	POWER_DOMAIN_PLLS,
 	POWER_DOMAIN_AUX_A,
 	POWER_DOMAIN_AUX_B,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 76d48da..4efe20c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2756,6 +2756,18 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	return 0;
 }
 
+static void gen9_prepare_vblank(struct drm_device *dev, unsigned int pipe)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	intel_display_power_get(dev_priv, POWER_DOMAIN_VBLANK);
+}
+
+static void gen9_unprepare_vblank(struct drm_device *dev, unsigned int pipe)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	intel_display_power_put(dev_priv, POWER_DOMAIN_VBLANK);
+}
+
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
@@ -4589,6 +4601,8 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 		dev->driver->irq_uninstall = gen8_irq_uninstall;
 		dev->driver->enable_vblank = gen8_enable_vblank;
 		dev->driver->disable_vblank = gen8_disable_vblank;
+		dev->driver->prepare_vblank = gen9_prepare_vblank;
+		dev->driver->unprepare_vblank = gen9_unprepare_vblank;
 		if (IS_BROXTON(dev))
 			dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
 		else if (HAS_PCH_SPT(dev) || HAS_PCH_KBP(dev))
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6e6e079..167fead 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -116,6 +116,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
 		return "VGA";
 	case POWER_DOMAIN_AUDIO:
 		return "AUDIO";
+	case POWER_DOMAIN_VBLANK:
+		return "VBLANK";
 	case POWER_DOMAIN_PLLS:
 		return "PLLS";
 	case POWER_DOMAIN_AUX_A:
@@ -419,6 +421,7 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
 	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
 	BIT(POWER_DOMAIN_MODESET) |			\
 	BIT(POWER_DOMAIN_AUX_A) |			\
+	BIT(POWER_DOMAIN_VBLANK) |			\
 	BIT(POWER_DOMAIN_INIT))
 #define SKL_DISPLAY_PSR_BLOCK_POWER_DOMAINS (		\
 	BIT(POWER_DOMAIN_MODESET) |			\
-- 
2.4.3

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

[PATCH 5/6] drm: Introduce drm_crtc_vblank_sanitize_counter.

[Rodrigo Vivi]

In modern systems there are situations that you can let the screen
enabled and sleep or shut off a most of the display controler.

In situations like this the vblank hw counter can be reset.
When this happens everything in the system gets crazy by
the big count.

So, the right approach is to make sure we are avoiding any
runtime suspend when vblank is enabled but also restore
drm crtc vblank counter to a state we can rely.

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

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index d0d1dde..7320836 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -265,6 +265,52 @@ u32 drm_accurate_vblank_count(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_accurate_vblank_count);
 
+/**
+ * drm_crtc_vblank_sanitize_counter - Sanitize vblank counter
+ * @crtc: which counter to sanitize
+ *
+ * This function returns drm crtc vblank counter to the latest
+ * known counter.
+ *
+ * This function is useful for runtime suspend where the hw
+ * counter or timer might reset.
+ *
+ * Use this only when there is no risk of the runtime suspend
+ * reseting hardware counter and before enabling vblank irq.
+ */
+void drm_crtc_vblank_sanitize_counter(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	unsigned int pipe = drm_crtc_index(crtc);
+	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+	unsigned long irqflags;
+	struct timeval t_vblank;
+	int count = DRM_TIMESTAMP_MAXRETRIES;
+	u32 cur_vblank;
+	bool rc;
+
+	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
+
+	if (vblank->enabled) {
+		DRM_DEBUG_VBL("Skip sanitize - Vblank is enabled on pipe %d\n",
+			      pipe);
+		goto unlock;
+	}
+
+	do {
+		cur_vblank = dev->driver->get_vblank_counter(dev, pipe);
+		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, 0);
+	} while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0);
+
+	if (!rc)
+		t_vblank = (struct timeval) {0, 0};
+
+	store_vblank(dev, pipe, 0, &t_vblank, cur_vblank);
+unlock:
+	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
+}
+EXPORT_SYMBOL(drm_crtc_vblank_sanitize_counter);
+
 /*
  * Disable vblank irq's on crtc, make sure that last vblank count
  * of hardware and corresponding consistent software vblank counter
diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
index 93a9e9d..55c419a 100644
--- a/include/drm/drm_irq.h
+++ b/include/drm/drm_irq.h
@@ -160,6 +160,7 @@ extern u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe);
 extern u32 drm_crtc_vblank_count(struct drm_crtc *crtc);
 extern u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
 					  struct timeval *vblanktime);
+extern void drm_crtc_vblank_sanitize_counter(struct drm_crtc *crtc);
 extern void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 				       struct drm_pending_vblank_event *e);
 extern void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
-- 
2.4.3

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

Re: [PATCH 5/6] drm: Introduce drm_crtc_vblank_sanitize_counter.

[Daniel Vetter]

On Wed, Aug 03, 2016 at 02:33:38PM -0700, Rodrigo Vivi wrote:
> In modern systems there are situations that you can let the screen
> enabled and sleep or shut off a most of the display controler.
> 
> In situations like this the vblank hw counter can be reset.
> When this happens everything in the system gets crazy by
> the big count.
> 
> So, the right approach is to make sure we are avoiding any
> runtime suspend when vblank is enabled but also restore
> drm crtc vblank counter to a state we can rely.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_irq.h     |  1 +
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index d0d1dde..7320836 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -265,6 +265,52 @@ u32 drm_accurate_vblank_count(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_accurate_vblank_count);
>  
> +/**
> + * drm_crtc_vblank_sanitize_counter - Sanitize vblank counter
> + * @crtc: which counter to sanitize
> + *
> + * This function returns drm crtc vblank counter to the latest
> + * known counter.
> + *
> + * This function is useful for runtime suspend where the hw
> + * counter or timer might reset.
> + *
> + * Use this only when there is no risk of the runtime suspend
> + * reseting hardware counter and before enabling vblank irq.

I think this needs to be a bit more precise, and should reference the
prepare/unprepare hooks.

> + */
> +void drm_crtc_vblank_sanitize_counter(struct drm_crtc *crtc)

sanitize_counter is a bit an unclear function name. I think resync_counter
would be better, but still not good really.

> +{
> +	struct drm_device *dev = crtc->dev;
> +	unsigned int pipe = drm_crtc_index(crtc);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +	unsigned long irqflags;
> +	struct timeval t_vblank;
> +	int count = DRM_TIMESTAMP_MAXRETRIES;
> +	u32 cur_vblank;
> +	bool rc;
> +
> +	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
> +
> +	if (vblank->enabled) {

If we put the sanitize call like I suggested, this would be a driver bug.
We don't want to sanitize it when it's already sanitized and nothing
could have corrupted it meanwhile, and especially not when the vblank
counter is in use. Should be a WARN_ON, not debug level.

> +		DRM_DEBUG_VBL("Skip sanitize - Vblank is enabled on pipe %d\n",
> +			      pipe);
> +		goto unlock;
> +	}
> +
> +	do {
> +		cur_vblank = dev->driver->get_vblank_counter(dev, pipe);
> +		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, 0);
> +	} while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0);
> +
> +	if (!rc)
> +		t_vblank = (struct timeval) {0, 0};
> +
> +	store_vblank(dev, pipe, 0, &t_vblank, cur_vblank);

We also need to sufficiently fake the missed vblanks by comparing the last
timestamp with the new one and dividing the elapsed time by the frame
time. Ville has added such logic to already to make sure we correctly
account for the time between drm_vblank_on and drm_vblank_off when the
vblank counter is not running. Please reuse that code to make that
adjustment, instead of open-code it.

> +unlock:
> +	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_sanitize_counter);
> +
>  /*
>   * Disable vblank irq's on crtc, make sure that last vblank count
>   * of hardware and corresponding consistent software vblank counter
> diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
> index 93a9e9d..55c419a 100644
> --- a/include/drm/drm_irq.h
> +++ b/include/drm/drm_irq.h
> @@ -160,6 +160,7 @@ extern u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe);
>  extern u32 drm_crtc_vblank_count(struct drm_crtc *crtc);
>  extern u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
>  					  struct timeval *vblanktime);
> +extern void drm_crtc_vblank_sanitize_counter(struct drm_crtc *crtc);
>  extern void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  				       struct drm_pending_vblank_event *e);
>  extern void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/6] drm/i915: Sanitize drm crtc vblank counter after DC reset frame count.

[Rodrigo Vivi]

DC state reset the frame counter that is a read-only register.

So, besides blocking DC state on vblank let's restore the
drm crtc vblank counter to a place we know it is reliable.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4efe20c..82d6896 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2759,7 +2759,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
 static void gen9_prepare_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 	intel_display_power_get(dev_priv, POWER_DOMAIN_VBLANK);
+	drm_crtc_vblank_sanitize_counter(crtc);
 }
 
 static void gen9_unprepare_vblank(struct drm_device *dev, unsigned int pipe)
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm/i915: Sanitize drm crtc vblank counter after DC reset frame count.

[Daniel Vetter]

On Wed, Aug 03, 2016 at 02:33:39PM -0700, Rodrigo Vivi wrote:
> DC state reset the frame counter that is a read-only register.
> 
> So, besides blocking DC state on vblank let's restore the
> drm crtc vblank counter to a place we know it is reliable.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4efe20c..82d6896 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2759,7 +2759,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  static void gen9_prepare_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>  	intel_display_power_get(dev_priv, POWER_DOMAIN_VBLANK);
> +	drm_crtc_vblank_sanitize_counter(crtc);

I think this should be done within the platform power code. Otherwise
something else might keep the system out of dc states, but then we might
wreak havoc by calling this function here.
-Daniel

>  }
>  
>  static void gen9_unprepare_vblank(struct drm_device *dev, unsigned int pipe)
> -- 
> 2.4.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm/i915: Sanitize drm crtc vblank counter after DC reset frame count.

[Rodrigo Vivi]

On Thu, Aug 4, 2016 at 1:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 03, 2016 at 02:33:39PM -0700, Rodrigo Vivi wrote:
>> DC state reset the frame counter that is a read-only register.
>>
>> So, besides blocking DC state on vblank let's restore the
>> drm crtc vblank counter to a place we know it is reliable.
>>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 4efe20c..82d6896 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -2759,7 +2759,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
>>  static void gen9_prepare_vblank(struct drm_device *dev, unsigned int pipe)
>>  {
>>       struct drm_i915_private *dev_priv = to_i915(dev);
>> +     struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>>       intel_display_power_get(dev_priv, POWER_DOMAIN_VBLANK);
>> +     drm_crtc_vblank_sanitize_counter(crtc);
>
> I think this should be done within the platform power code. Otherwise
> something else might keep the system out of dc states, but then we might
> wreak havoc by calling this function here.

This is safe here because DC is handled as a power_well... so as long
as there is one domain holding its reference DC will be off.

Also this needs to be in a place we are sure that vblanks are yet disabled.

Now it comes back to that point on how to make sure that we only run 1
prepare pre-enable...

multiple prepare/unprepares will keep trying to resync it useless and
if we have that WARN_ON we will get flooded....

> -Daniel
>
>>  }
>>
>>  static void gen9_unprepare_vblank(struct drm_device *dev, unsigned int pipe)
>> --
>> 2.4.3
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Sanitize drm crtc vblank counter after DC reset frame count.

[Daniel Vetter]

On Fri, Aug 05, 2016 at 02:49:44PM -0700, Rodrigo Vivi wrote:
> On Thu, Aug 4, 2016 at 1:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Aug 03, 2016 at 02:33:39PM -0700, Rodrigo Vivi wrote:
> >> DC state reset the frame counter that is a read-only register.
> >>
> >> So, besides blocking DC state on vblank let's restore the
> >> drm crtc vblank counter to a place we know it is reliable.
> >>
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 4efe20c..82d6896 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -2759,7 +2759,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
> >>  static void gen9_prepare_vblank(struct drm_device *dev, unsigned int pipe)
> >>  {
> >>       struct drm_i915_private *dev_priv = to_i915(dev);
> >> +     struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> >>       intel_display_power_get(dev_priv, POWER_DOMAIN_VBLANK);
> >> +     drm_crtc_vblank_sanitize_counter(crtc);
> >
> > I think this should be done within the platform power code. Otherwise
> > something else might keep the system out of dc states, but then we might
> > wreak havoc by calling this function here.
> 
> This is safe here because DC is handled as a power_well... so as long
> as there is one domain holding its reference DC will be off.
> 
> Also this needs to be in a place we are sure that vblanks are yet disabled.
> 
> Now it comes back to that point on how to make sure that we only run 1
> prepare pre-enable...
> 
> multiple prepare/unprepares will keep trying to resync it useless and
> if we have that WARN_ON we will get flooded....

Yeah, my comment was under the assumption that there can be multiple
prepare/unprepare, and then it makes sense to reuse the refcounting we
already have. Still not sure it'll make sense to implement
prepare/unprepare refcounting in the vblank code, it'll be fairly tricky
in there. And for use useless, since our power well code already has
refcounting.
-Daniel
> 
> > -Daniel
> >
> >>  }
> >>
> >>  static void gen9_unprepare_vblank(struct drm_device *dev, unsigned int pipe)
> >> --
> >> 2.4.3
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/6] Allow DC state to reset the counter on screen enabled.

[Michel Dänzer]

On 04.08.2016 06:33, Rodrigo Vivi wrote:
> For now DC is only helping on screen off scenarios since PSR is disabled.
> 
> But if we want to enable PSR first we need to make DC reliable with screen on.
> Biggest challenge is to deal with vblank counters since frame counter register
> is read only and can be reset in DC state.
> 
> This series is one of possible approaches, but brings the down side of not
> being possible to use runtime pm with vblank enabled. Some test cases needs
> to be adapted to represent this new vision.

Note that the frame counter register must not reset before
drm_crtc_vblank_off / after drm_crtc_vblank_on, even while the vblank
interrupt is disabled. Otherwise the DRM vblank counter as seen by
userspace via the DRM_IOCTL_WAIT_VBLANK ioctl will jump around, because
the core DRM code uses the frame counter register to keep track of how
many vertical blank periods occurred since the interrupt was disabled.


> But also this series is not fully tested. Apparently I have an issue yet
> with flip-vs-expired-vblank_* tests and pm_rpm basic tests.

Maybe some of the test failures are due to the above.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Ro.CI.BAT: failure for Allow DC state to reset the counter on screen enabled.

[Patchwork]

== Series Details ==

Series: Allow DC state to reset the counter on screen enabled.
URL   : https://patchwork.freedesktop.org/series/10630/
State : failure

== Summary ==

Applying: drm: Add vblank prepare and unprepare hooks.
Applying: drm/i915: Move drm_crtc_vblank_get out of disabled pre-emption area.
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_sprite.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_sprite.c
Applying: drm/i915: Split gen 9 irq hooks definitions.
Applying: drm/i915: Introduce vblank power domain to avoid DC entry when waiting for vblank.
fatal: sha1 information is lacking or useless (drivers/gpu/drm/i915/i915_drv.h).
error: could not build fake ancestor
Patch failed at 0004 drm/i915: Introduce vblank power domain to avoid DC entry when waiting for vblank.
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