[PATCH 00/12] irq vblank handling rework

[PATCH 00/12] irq vblank handling rework

[Daniel Vetter]

Hi all,

This is Ville's vblank rework series, slightly pimped. I've added kerneldoc,
some polish and also some additional nasty igt testcases for these crtc on/off
vs vblank issues. Seems all to hold together nicely.

Now one thing I wanted to do is roll this out across all drm drivers, but that
looks ugly: Many drivers don't support vblanks really, and I couldn't fully
audit whether they'd e.g. blow up when userspace tries to use the vblank wait
ioctl. But I think we want to have more sanity since otherwise userspace is
doomed to carry countless ugly hacks around forever.

The last two patches are my attempt at that. By doing the drm_vblank_on/off
dance in the crtc helpers after we know that the crtc is on/off we should have
at least a somewhat sane baseline behaviour. Note that since drm_vblank_on/off
are idempotent drivers are free to call these functions in their callback at an
earlier time, where it makes more sense. But at least it's guaranteed to happen.

Otoh drivers still need to manually complete pageflips, so this doesn't solve
all the issues. But until we have a solid cross-driver testsuite for these
corner-cases (all the interesting stuff happens when you race vblank waits
against concurrent modesets, dpms, or system/runtime suspend/resume) we're
pretty much guaranteed to have some that works at best occasionally and is
guaranteed to have different behaviour in corner cases. Note that the patches
won't degrade behaviour for existing drivers, the drm core changes simply allow
us to finally fix things up correctly.

I guess in a way this is a more generic discussion for the drm subsystem at
large.

Coments and review highly welcome.

Cheers, Daniel

Daniel Vetter (8):
  drm/i915: Remove drm_vblank_pre/post_modeset calls
  drm/doc: Discourage usage of MODESET_CTL ioctl
  drm/irq: kerneldoc polish
  drm/irq: Add kms-native crtc interface functions
  drm/i915: rip our vblank reset hacks for runtime PM
  drm/i915: Accurately initialize fifo underrun state on gmch platforms
  [RFC] drm/irq: More robustness in drm_vblank_on|off
  [RFC] drm/crtc-helper: Enforce sane vblank counter semantics

Peter Hurley (1):
  drm: Use correct spinlock flavor in drm_vblank_get()

Ville Syrjälä (3):
  drm: Make the vblank disable timer per-crtc
  drm: Make blocking vblank wait return when the vblank interrupts get
    disabled
  drm: Add drm_vblank_on()

 Documentation/DocBook/drm.tmpl       |  16 +-
 drivers/gpu/drm/drm_crtc_helper.c    |  12 ++
 drivers/gpu/drm/drm_irq.c            | 377 ++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_irq.c      |   4 +-
 drivers/gpu/drm/i915/intel_display.c |  36 ++--
 drivers/gpu/drm/i915/intel_drv.h     |   2 -
 drivers/gpu/drm/i915/intel_pm.c      |  40 ----
 include/drm/drmP.h                   |  10 +-
 8 files changed, 341 insertions(+), 156 deletions(-)

-- 
1.8.3.1

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

[PATCH 01/12] drm: Use correct spinlock flavor in drm_vblank_get()

[Daniel Vetter]

From: Peter Hurley <peter@hurleysoftware.com>

The irq flags state is already established by the outer
spin_lock_irqsave(); re-disabling irqs is redundant.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 7583767ec619..ea20c4aa1b6b 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -848,13 +848,13 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
  */
 int drm_vblank_get(struct drm_device *dev, int crtc)
 {
-	unsigned long irqflags, irqflags2;
+	unsigned long irqflags;
 	int ret = 0;
 
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	/* Going from 0->1 means we have to enable interrupts again */
 	if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
-		spin_lock_irqsave(&dev->vblank_time_lock, irqflags2);
+		spin_lock(&dev->vblank_time_lock);
 		if (!dev->vblank[crtc].enabled) {
 			/* Enable vblank irqs under vblank_time_lock protection.
 			 * All vblank count & timestamp updates are held off
@@ -872,7 +872,7 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
 				drm_update_vblank_count(dev, crtc);
 			}
 		}
-		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags2);
+		spin_unlock(&dev->vblank_time_lock);
 	} else {
 		if (!dev->vblank[crtc].enabled) {
 			atomic_dec(&dev->vblank[crtc].refcount);
-- 
1.8.3.1

Re: [PATCH 01/12] drm: Use correct spinlock flavor in drm_vblank_get()

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 503 bytes --]

On Wed, May 14, 2014 at 08:51:03PM +0200, Daniel Vetter wrote:
> From: Peter Hurley <peter@hurleysoftware.com>
> 
> The irq flags state is already established by the outer
> spin_lock_irqsave(); re-disabling irqs is redundant.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 02/12] drm: Make the vblank disable timer per-crtc

[Daniel Vetter]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently there's one per-device vblank disable timer, and it gets
reset wheneven the vblank refcount for any crtc drops to zero. That
means that one crtc could accidentally be keeping the vblank interrupts
for other crtcs enabled even if there are no users for them. Make the
disable timer per-crtc to avoid this issue.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 40 +++++++++++++++++++++-------------------
 include/drm/drmP.h        |  4 +++-
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index ea20c4aa1b6b..90c59a8c820f 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -140,33 +140,34 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 
 static void vblank_disable_fn(unsigned long arg)
 {
-	struct drm_device *dev = (struct drm_device *)arg;
+	struct drm_vblank_crtc *vblank = (void *)arg;
+	struct drm_device *dev = vblank->dev;
 	unsigned long irqflags;
-	int i;
+	int crtc = vblank->crtc;
 
 	if (!dev->vblank_disable_allowed)
 		return;
 
-	for (i = 0; i < dev->num_crtcs; i++) {
-		spin_lock_irqsave(&dev->vbl_lock, irqflags);
-		if (atomic_read(&dev->vblank[i].refcount) == 0 &&
-		    dev->vblank[i].enabled) {
-			DRM_DEBUG("disabling vblank on crtc %d\n", i);
-			vblank_disable_and_save(dev, i);
-		}
-		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+	spin_lock_irqsave(&dev->vbl_lock, irqflags);
+	if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) {
+		DRM_DEBUG("disabling vblank on crtc %d\n", crtc);
+		vblank_disable_and_save(dev, crtc);
 	}
+	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 }
 
 void drm_vblank_cleanup(struct drm_device *dev)
 {
+	int crtc;
+
 	/* Bail if the driver didn't call drm_vblank_init() */
 	if (dev->num_crtcs == 0)
 		return;
 
-	del_timer_sync(&dev->vblank_disable_timer);
-
-	vblank_disable_fn((unsigned long)dev);
+	for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
+		del_timer_sync(&dev->vblank[crtc].disable_timer);
+		vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
+	}
 
 	kfree(dev->vblank);
 
@@ -178,8 +179,6 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
 {
 	int i, ret = -ENOMEM;
 
-	setup_timer(&dev->vblank_disable_timer, vblank_disable_fn,
-		    (unsigned long)dev);
 	spin_lock_init(&dev->vbl_lock);
 	spin_lock_init(&dev->vblank_time_lock);
 
@@ -189,8 +188,13 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
 	if (!dev->vblank)
 		goto err;
 
-	for (i = 0; i < num_crtcs; i++)
+	for (i = 0; i < num_crtcs; i++) {
+		dev->vblank[i].dev = dev;
+		dev->vblank[i].crtc = i;
 		init_waitqueue_head(&dev->vblank[i].queue);
+		setup_timer(&dev->vblank[i].disable_timer, vblank_disable_fn,
+			    (unsigned long)&dev->vblank[i]);
+	}
 
 	DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");
 
@@ -900,7 +904,7 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
 	/* Last user schedules interrupt disable */
 	if (atomic_dec_and_test(&dev->vblank[crtc].refcount) &&
 	    (drm_vblank_offdelay > 0))
-		mod_timer(&dev->vblank_disable_timer,
+		mod_timer(&dev->vblank[crtc].disable_timer,
 			  jiffies + ((drm_vblank_offdelay * HZ)/1000));
 }
 EXPORT_SYMBOL(drm_vblank_put);
@@ -909,8 +913,6 @@ EXPORT_SYMBOL(drm_vblank_put);
  * drm_vblank_off - disable vblank events on a CRTC
  * @dev: DRM device
  * @crtc: CRTC in question
- *
- * Caller must hold event lock.
  */
 void drm_vblank_off(struct drm_device *dev, int crtc)
 {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 12f10bc2395f..9d982d483f12 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1024,14 +1024,17 @@ struct drm_pending_vblank_event {
 };
 
 struct drm_vblank_crtc {
+	struct drm_device *dev;		/* pointer to the drm_device */
 	wait_queue_head_t queue;	/**< VBLANK wait queue */
 	struct timeval time[DRM_VBLANKTIME_RBSIZE];	/**< timestamp of current count */
+	struct timer_list disable_timer;		/* delayed disable timer */
 	atomic_t count;			/**< number of VBLANK interrupts */
 	atomic_t refcount;		/* number of users of vblank interruptsper crtc */
 	u32 last;			/* protected by dev->vbl_lock, used */
 					/* for wraparound handling */
 	u32 last_wait;			/* Last vblank seqno waited per CRTC */
 	unsigned int inmodeset;		/* Display driver is setting mode */
+	int crtc;			/* crtc index */
 	bool enabled;			/* so we don't call enable more than
 					   once per disable */
 };
@@ -1119,7 +1122,6 @@ struct drm_device {
 
 	spinlock_t vblank_time_lock;    /**< Protects vblank count and time updates during vblank enable/disable */
 	spinlock_t vbl_lock;
-	struct timer_list vblank_disable_timer;
 
 	u32 max_vblank_count;           /**< size of vblank counter register */
 
-- 
1.8.3.1

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

Re: [PATCH 02/12] drm: Make the vblank disable timer per-crtc

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 805 bytes --]

On Wed, May 14, 2014 at 08:51:04PM +0200, Daniel Vetter wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently there's one per-device vblank disable timer, and it gets
> reset wheneven the vblank refcount for any crtc drops to zero. That

"whenever"

> means that one crtc could accidentally be keeping the vblank interrupts
> for other crtcs enabled even if there are no users for them. Make the
> disable timer per-crtc to avoid this issue.

Very pedantically: s/crtc/CRTC/ and maybe even s/vblank/VBLANK/. Feel
free to ignore those, though. =)

Also, and I may have asked before, why do we even need this timer? Why
not simply disable interrupts when the last vblank reference goes away?

Generally, though:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 02/12] drm: Make the vblank disable timer per-crtc

[Daniel Vetter]

On Wed, May 21, 2014 at 01:17:49PM +0200, Thierry Reding wrote:
> On Wed, May 14, 2014 at 08:51:04PM +0200, Daniel Vetter wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently there's one per-device vblank disable timer, and it gets
> > reset wheneven the vblank refcount for any crtc drops to zero. That
> 
> "whenever"
> 
> > means that one crtc could accidentally be keeping the vblank interrupts
> > for other crtcs enabled even if there are no users for them. Make the
> > disable timer per-crtc to avoid this issue.
> 
> Very pedantically: s/crtc/CRTC/ and maybe even s/vblank/VBLANK/. Feel
> free to ignore those, though. =)
> 
> Also, and I may have asked before, why do we even need this timer? Why
> not simply disable interrupts when the last vblank reference goes away?

Without intricate knowledge of where exactly the vblank interrupt fires
wrt the hw frame counter the enabling/disabling of the vblank machinery as
implemented in drm_irq.c is racy. Which means we shouldn't do it all the
time.

In i915 we are now solid enough with vblank handling in general and also
well-covered in tests that we'll attempt to kill the disabling timer as
the next step. Since keeping vblanks going when we don't need them if you
have a hw vblank counter seriously hampers deep sleep states residency.

But given how bug-riddled our vblank code was I want to move slowly. And
we need to keep the hack for all those drivers which haven't properly been
audited and tested (i.e. everyone else).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 03/12] drm: Make blocking vblank wait return when the vblank interrupts get disabled

[Daniel Vetter]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If there's a blocking vblank wait in progress while the vblank interrupt
gets disabled, the current code will just let the vblank wait time out.
Instead make it return immediately when vblank interrupts get disabled.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 90c59a8c820f..13d671ed3421 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1189,6 +1189,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 	DRM_WAIT_ON(ret, dev->vblank[crtc].queue, 3 * HZ,
 		    (((drm_vblank_count(dev, crtc) -
 		       vblwait->request.sequence) <= (1 << 23)) ||
+		     !dev->vblank[crtc].enabled ||
 		     !dev->irq_enabled));
 
 	if (ret != -EINTR) {
-- 
1.8.3.1

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

Re: [PATCH 03/12] drm: Make blocking vblank wait return when the vblank interrupts get disabled

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 628 bytes --]

On Wed, May 14, 2014 at 08:51:05PM +0200, Daniel Vetter wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If there's a blocking vblank wait in progress while the vblank interrupt
> gets disabled, the current code will just let the vblank wait time out.
> Instead make it return immediately when vblank interrupts get disabled.

Can this even happen? drm_wait_vblank() takes a vblank reference earlier
and drops it right before returning. But perhaps this will become
obvious since from a quick peek some of the subsequent patches seem like
they will make it possible to force VBLANK off?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 03/12] drm: Make blocking vblank wait return when the vblank interrupts get disabled

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 856 bytes --]

On Wed, May 21, 2014 at 01:20:55PM +0200, Thierry Reding wrote:
> On Wed, May 14, 2014 at 08:51:05PM +0200, Daniel Vetter wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If there's a blocking vblank wait in progress while the vblank interrupt
> > gets disabled, the current code will just let the vblank wait time out.
> > Instead make it return immediately when vblank interrupts get disabled.
> 
> Can this even happen? drm_wait_vblank() takes a vblank reference earlier
> and drops it right before returning. But perhaps this will become
> obvious since from a quick peek some of the subsequent patches seem like
> they will make it possible to force VBLANK off?

Ah, it seems like drm_vblank_off() can already do exactly that, in which
case this makes sense:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 04/12] drm: Add drm_vblank_on()

[Daniel Vetter]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm_vblank_off() will turn off vblank interrupts, but as long as the
refcount is elevated drm_vblank_get() will not re-enable them. This
is a problem is someone is holding a vblank reference while a modeset is
happening, and the driver requires vblank interrupt to work during that
time.

Add drm_vblank_on() as a counterpart to drm_vblank_off() which will
re-enabled vblank interrupts if the refcount is already elevated. This
will allow drivers to choose the specific places in the modeset sequence
at which vblank interrupts get disabled and enabled.

Testcase: igt/kms_flip/*-vs-suspend
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
[danvet: Add Testcase tag for the igt I've written.]
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c            | 72 ++++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c |  8 ++++
 include/drm/drmP.h                   |  1 +
 3 files changed, 62 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 13d671ed3421..dd786d84daab 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -840,6 +840,41 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 }
 
 /**
+ * drm_vblank_enable - enable the vblank interrupt on a CRTC
+ * @dev: DRM device
+ * @crtc: CRTC in question
+ */
+static int drm_vblank_enable(struct drm_device *dev, int crtc)
+{
+	int ret = 0;
+
+	assert_spin_locked(&dev->vbl_lock);
+
+	spin_lock(&dev->vblank_time_lock);
+
+	if (!dev->vblank[crtc].enabled) {
+		/* Enable vblank irqs under vblank_time_lock protection.
+		 * All vblank count & timestamp updates are held off
+		 * until we are done reinitializing master counter and
+		 * timestamps. Filtercode in drm_handle_vblank() will
+		 * prevent double-accounting of same vblank interval.
+		 */
+		ret = dev->driver->enable_vblank(dev, crtc);
+		DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret);
+		if (ret)
+			atomic_dec(&dev->vblank[crtc].refcount);
+		else {
+			dev->vblank[crtc].enabled = true;
+			drm_update_vblank_count(dev, crtc);
+		}
+	}
+
+	spin_unlock(&dev->vblank_time_lock);
+
+	return ret;
+}
+
+/**
  * drm_vblank_get - get a reference count on vblank events
  * @dev: DRM device
  * @crtc: which CRTC to own
@@ -858,25 +893,7 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	/* Going from 0->1 means we have to enable interrupts again */
 	if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
-		spin_lock(&dev->vblank_time_lock);
-		if (!dev->vblank[crtc].enabled) {
-			/* Enable vblank irqs under vblank_time_lock protection.
-			 * All vblank count & timestamp updates are held off
-			 * until we are done reinitializing master counter and
-			 * timestamps. Filtercode in drm_handle_vblank() will
-			 * prevent double-accounting of same vblank interval.
-			 */
-			ret = dev->driver->enable_vblank(dev, crtc);
-			DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n",
-				  crtc, ret);
-			if (ret)
-				atomic_dec(&dev->vblank[crtc].refcount);
-			else {
-				dev->vblank[crtc].enabled = true;
-				drm_update_vblank_count(dev, crtc);
-			}
-		}
-		spin_unlock(&dev->vblank_time_lock);
+		ret = drm_vblank_enable(dev, crtc);
 	} else {
 		if (!dev->vblank[crtc].enabled) {
 			atomic_dec(&dev->vblank[crtc].refcount);
@@ -946,6 +963,23 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 EXPORT_SYMBOL(drm_vblank_off);
 
 /**
+ * drm_vblank_on - enable vblank events on a CRTC
+ * @dev: DRM device
+ * @crtc: CRTC in question
+ */
+void drm_vblank_on(struct drm_device *dev, int crtc)
+{
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&dev->vbl_lock, irqflags);
+	/* re-enable interrupts if there's are users left */
+	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
+		WARN_ON(drm_vblank_enable(dev, crtc));
+	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+}
+EXPORT_SYMBOL(drm_vblank_on);
+
+/**
  * drm_vblank_pre_modeset - account for vblanks across mode sets
  * @dev: DRM device
  * @crtc: CRTC in question
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b39d0367dd68..858c393b051f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3739,6 +3739,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	 * happening.
 	 */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	drm_vblank_on(dev, pipe);
 }
 
 /* IPS only exists on ULT machines and is tied to pipe A. */
@@ -3830,6 +3832,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	 * to change the workaround. */
 	haswell_mode_set_planes_workaround(intel_crtc);
 	ilk_crtc_enable_planes(crtc);
+
+	drm_vblank_on(dev, pipe);
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc)
@@ -4353,6 +4357,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
+
+	drm_vblank_on(dev, pipe);
 }
 
 static void i9xx_crtc_enable(struct drm_crtc *crtc)
@@ -4400,6 +4406,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
+
+	drm_vblank_on(dev, pipe);
 }
 
 static void i9xx_pfit_disable(struct intel_crtc *crtc)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 9d982d483f12..7339b2b00724 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1360,6 +1360,7 @@ extern bool drm_handle_vblank(struct drm_device *dev, int crtc);
 extern int drm_vblank_get(struct drm_device *dev, int crtc);
 extern void drm_vblank_put(struct drm_device *dev, int crtc);
 extern void drm_vblank_off(struct drm_device *dev, int crtc);
+extern void drm_vblank_on(struct drm_device *dev, int crtc);
 extern void drm_vblank_cleanup(struct drm_device *dev);
 extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
 				     struct timeval *tvblank, unsigned flags);
-- 
1.8.3.1

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

Re: [PATCH 04/12] drm: Add drm_vblank_on()

[Daniel Vetter]

On Wed, May 14, 2014 at 08:51:06PM +0200, Daniel Vetter wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> drm_vblank_off() will turn off vblank interrupts, but as long as the
> refcount is elevated drm_vblank_get() will not re-enable them. This
> is a problem is someone is holding a vblank reference while a modeset is
> happening, and the driver requires vblank interrupt to work during that
> time.
> 
> Add drm_vblank_on() as a counterpart to drm_vblank_off() which will
> re-enabled vblank interrupts if the refcount is already elevated. This
> will allow drivers to choose the specific places in the modeset sequence
> at which vblank interrupts get disabled and enabled.
> 
> Testcase: igt/kms_flip/*-vs-suspend

Testcase: igt/kms_flip/*-vs-vblank-race

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> [danvet: Add Testcase tag for the igt I've written.]
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c            | 72 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++
>  include/drm/drmP.h                   |  1 +
>  3 files changed, 62 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 13d671ed3421..dd786d84daab 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -840,6 +840,41 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  }
>  
>  /**
> + * drm_vblank_enable - enable the vblank interrupt on a CRTC
> + * @dev: DRM device
> + * @crtc: CRTC in question
> + */
> +static int drm_vblank_enable(struct drm_device *dev, int crtc)
> +{
> +	int ret = 0;
> +
> +	assert_spin_locked(&dev->vbl_lock);
> +
> +	spin_lock(&dev->vblank_time_lock);
> +
> +	if (!dev->vblank[crtc].enabled) {
> +		/* Enable vblank irqs under vblank_time_lock protection.
> +		 * All vblank count & timestamp updates are held off
> +		 * until we are done reinitializing master counter and
> +		 * timestamps. Filtercode in drm_handle_vblank() will
> +		 * prevent double-accounting of same vblank interval.
> +		 */
> +		ret = dev->driver->enable_vblank(dev, crtc);
> +		DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret);
> +		if (ret)
> +			atomic_dec(&dev->vblank[crtc].refcount);
> +		else {
> +			dev->vblank[crtc].enabled = true;
> +			drm_update_vblank_count(dev, crtc);
> +		}
> +	}
> +
> +	spin_unlock(&dev->vblank_time_lock);
> +
> +	return ret;
> +}
> +
> +/**
>   * drm_vblank_get - get a reference count on vblank events
>   * @dev: DRM device
>   * @crtc: which CRTC to own
> @@ -858,25 +893,7 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  	/* Going from 0->1 means we have to enable interrupts again */
>  	if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
> -		spin_lock(&dev->vblank_time_lock);
> -		if (!dev->vblank[crtc].enabled) {
> -			/* Enable vblank irqs under vblank_time_lock protection.
> -			 * All vblank count & timestamp updates are held off
> -			 * until we are done reinitializing master counter and
> -			 * timestamps. Filtercode in drm_handle_vblank() will
> -			 * prevent double-accounting of same vblank interval.
> -			 */
> -			ret = dev->driver->enable_vblank(dev, crtc);
> -			DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n",
> -				  crtc, ret);
> -			if (ret)
> -				atomic_dec(&dev->vblank[crtc].refcount);
> -			else {
> -				dev->vblank[crtc].enabled = true;
> -				drm_update_vblank_count(dev, crtc);
> -			}
> -		}
> -		spin_unlock(&dev->vblank_time_lock);
> +		ret = drm_vblank_enable(dev, crtc);
>  	} else {
>  		if (!dev->vblank[crtc].enabled) {
>  			atomic_dec(&dev->vblank[crtc].refcount);
> @@ -946,6 +963,23 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>  EXPORT_SYMBOL(drm_vblank_off);
>  
>  /**
> + * drm_vblank_on - enable vblank events on a CRTC
> + * @dev: DRM device
> + * @crtc: CRTC in question
> + */
> +void drm_vblank_on(struct drm_device *dev, int crtc)
> +{
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	/* re-enable interrupts if there's are users left */
> +	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
> +		WARN_ON(drm_vblank_enable(dev, crtc));
> +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +}
> +EXPORT_SYMBOL(drm_vblank_on);
> +
> +/**
>   * drm_vblank_pre_modeset - account for vblanks across mode sets
>   * @dev: DRM device
>   * @crtc: CRTC in question
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b39d0367dd68..858c393b051f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3739,6 +3739,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	 * happening.
>  	 */
>  	intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +	drm_vblank_on(dev, pipe);
>  }
>  
>  /* IPS only exists on ULT machines and is tied to pipe A. */
> @@ -3830,6 +3832,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	 * to change the workaround. */
>  	haswell_mode_set_planes_workaround(intel_crtc);
>  	ilk_crtc_enable_planes(crtc);
> +
> +	drm_vblank_on(dev, pipe);
>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> @@ -4353,6 +4357,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> +
> +	drm_vblank_on(dev, pipe);
>  }
>  
>  static void i9xx_crtc_enable(struct drm_crtc *crtc)
> @@ -4400,6 +4406,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> +
> +	drm_vblank_on(dev, pipe);
>  }
>  
>  static void i9xx_pfit_disable(struct intel_crtc *crtc)
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 9d982d483f12..7339b2b00724 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1360,6 +1360,7 @@ extern bool drm_handle_vblank(struct drm_device *dev, int crtc);
>  extern int drm_vblank_get(struct drm_device *dev, int crtc);
>  extern void drm_vblank_put(struct drm_device *dev, int crtc);
>  extern void drm_vblank_off(struct drm_device *dev, int crtc);
> +extern void drm_vblank_on(struct drm_device *dev, int crtc);
>  extern void drm_vblank_cleanup(struct drm_device *dev);
>  extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
>  				     struct timeval *tvblank, unsigned flags);
> -- 
> 1.8.3.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 04/12] drm: Add drm_vblank_on()

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2684 bytes --]

On Wed, May 14, 2014 at 08:51:06PM +0200, Daniel Vetter wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> drm_vblank_off() will turn off vblank interrupts, but as long as the
> refcount is elevated drm_vblank_get() will not re-enable them. This
> is a problem is someone is holding a vblank reference while a modeset is
> happening, and the driver requires vblank interrupt to work during that
> time.
> 
> Add drm_vblank_on() as a counterpart to drm_vblank_off() which will
> re-enabled vblank interrupts if the refcount is already elevated. This
> will allow drivers to choose the specific places in the modeset sequence
> at which vblank interrupts get disabled and enabled.
> 
> Testcase: igt/kms_flip/*-vs-suspend
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> [danvet: Add Testcase tag for the igt I've written.]
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c            | 72 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++
>  include/drm/drmP.h                   |  1 +
>  3 files changed, 62 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 13d671ed3421..dd786d84daab 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -840,6 +840,41 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  }
>  
>  /**
> + * drm_vblank_enable - enable the vblank interrupt on a CRTC
> + * @dev: DRM device
> + * @crtc: CRTC in question
> + */

Perhaps the kernel-doc here should contain some of what's described in
the commit message? Also a "Return:" section would be useful here to
specify what's an error and what isn't.

> +static int drm_vblank_enable(struct drm_device *dev, int crtc)

On second thought, since this is a local function, my comments above
apply to drm_vblank_on() below rather than drm_vblank_enable().

> +{
> +	int ret = 0;
> +
> +	assert_spin_locked(&dev->vbl_lock);
> +
> +	spin_lock(&dev->vblank_time_lock);
> +
> +	if (!dev->vblank[crtc].enabled) {
> +		/* Enable vblank irqs under vblank_time_lock protection.
> +		 * All vblank count & timestamp updates are held off
> +		 * until we are done reinitializing master counter and
> +		 * timestamps. Filtercode in drm_handle_vblank() will
> +		 * prevent double-accounting of same vblank interval.
> +		 */

Coding style:

	/*
	 * Enable...
	 */

?

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

Perhaps split off the i915 changes into a separate patch?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 04/12] drm: Add drm_vblank_on()

[Daniel Vetter]

On Wed, May 21, 2014 at 01:32:35PM +0200, Thierry Reding wrote:
> On Wed, May 14, 2014 at 08:51:06PM +0200, Daniel Vetter wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > drm_vblank_off() will turn off vblank interrupts, but as long as the
> > refcount is elevated drm_vblank_get() will not re-enable them. This
> > is a problem is someone is holding a vblank reference while a modeset is
> > happening, and the driver requires vblank interrupt to work during that
> > time.
> > 
> > Add drm_vblank_on() as a counterpart to drm_vblank_off() which will
> > re-enabled vblank interrupts if the refcount is already elevated. This
> > will allow drivers to choose the specific places in the modeset sequence
> > at which vblank interrupts get disabled and enabled.
> > 
> > Testcase: igt/kms_flip/*-vs-suspend
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > [danvet: Add Testcase tag for the igt I've written.]
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_irq.c            | 72 ++++++++++++++++++++++++++----------
> >  drivers/gpu/drm/i915/intel_display.c |  8 ++++
> >  include/drm/drmP.h                   |  1 +
> >  3 files changed, 62 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 13d671ed3421..dd786d84daab 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -840,6 +840,41 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
> >  }
> >  
> >  /**
> > + * drm_vblank_enable - enable the vblank interrupt on a CRTC
> > + * @dev: DRM device
> > + * @crtc: CRTC in question
> > + */
> 
> Perhaps the kernel-doc here should contain some of what's described in
> the commit message? Also a "Return:" section would be useful here to
> specify what's an error and what isn't.
> 
> > +static int drm_vblank_enable(struct drm_device *dev, int crtc)
> 
> On second thought, since this is a local function, my comments above
> apply to drm_vblank_on() below rather than drm_vblank_enable().

Yeah, we don't pull this into the kerneldoc, only functions exported to
drivers. Follow-up patch should (hopefully) sufficiently pimp the
kerneldoc for drm_vblank_on. If not please complain.

> 
> > +{
> > +	int ret = 0;
> > +
> > +	assert_spin_locked(&dev->vbl_lock);
> > +
> > +	spin_lock(&dev->vblank_time_lock);
> > +
> > +	if (!dev->vblank[crtc].enabled) {
> > +		/* Enable vblank irqs under vblank_time_lock protection.
> > +		 * All vblank count & timestamp updates are held off
> > +		 * until we are done reinitializing master counter and
> > +		 * timestamps. Filtercode in drm_handle_vblank() will
> > +		 * prevent double-accounting of same vblank interval.
> > +		 */
> 
> Coding style:
> 
> 	/*
> 	 * Enable...
> 	 */
> 
> ?

Yeah, will polish.
> 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> 
> Perhaps split off the i915 changes into a separate patch?

Yeah, should have. But I've got fed up with the conflicts this topic
branch caused so pulled it in. I'll apply your review feedback as
follow-up patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 05/12] drm/i915: Remove drm_vblank_pre/post_modeset calls

[Daniel Vetter]

Originally these functions have been for user modesetting drivers to
ensure vblank processing doesn't fall over completely around modeset
changes. This has been carried over ever since then.

Now that Ville cleaned our vblank handling with an explicit
drm_vblank_off/on braket when disabling/enabling crtcs. So this seems
to be unnecessary now. The most important side effect was that due to
the delayed vblank disabling we have been pretty much guaranteed to
receive a vblank interrupt soonish after a crtc was enabled.

Note that our vblank handling across modeset is still fairly decent
fubar - we don't actually handle vblank counter all to well.
drm_update_vblank_count will make sure that the frame counter always
rolls forward, but userspace isn't really all to ready to cope with
the big jumps this causes.

This isn't a big mostly because the hardware retains the frame
counter. But with runtime pm and also across suspend/resume we fall
over.

Fixing this is a lot more involved and also needs som i-g-ts. So
material for another patch series.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 858c393b051f..d0eff53a8ad1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7207,15 +7207,10 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 	struct intel_encoder *encoder;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
-	int pipe = intel_crtc->pipe;
 	int ret;
 
-	drm_vblank_pre_modeset(dev, pipe);
-
 	ret = dev_priv->display.crtc_mode_set(crtc, x, y, fb);
 
-	drm_vblank_post_modeset(dev, pipe);
-
 	if (ret != 0)
 		return ret;
 
-- 
1.8.3.1

Re: [PATCH 05/12] drm/i915: Remove drm_vblank_pre/post_modeset calls

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2418 bytes --]

On Wed, May 14, 2014 at 08:51:07PM +0200, Daniel Vetter wrote:
> Originally these functions have been for user modesetting drivers to
> ensure vblank processing doesn't fall over completely around modeset
> changes. This has been carried over ever since then.
> 
> Now that Ville cleaned our vblank handling with an explicit
> drm_vblank_off/on braket when disabling/enabling crtcs. So this seems

s/braket/bracket/

> to be unnecessary now.

Should we document that drivers should start converting to this new set
of functions? Maybe deprecate the drm_vblank_pre/post_modeset()?

> The most important side effect was that due to
> the delayed vblank disabling we have been pretty much guaranteed to
> receive a vblank interrupt soonish after a crtc was enabled.

I don't understand what this sentence means and whether it relates to
code prior to or after this patch.

> Note that our vblank handling across modeset is still fairly decent
> fubar - we don't actually handle vblank counter all to well.
> drm_update_vblank_count will make sure that the frame counter always
> rolls forward, but userspace isn't really all to ready to cope with
> the big jumps this causes.
> 
> This isn't a big mostly because the hardware retains the frame

Not a big what?

> counter. But with runtime pm and also across suspend/resume we fall
> over.
> 
> Fixing this is a lot more involved and also needs som i-g-ts. So
> material for another patch series.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 858c393b051f..d0eff53a8ad1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7207,15 +7207,10 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
>  	struct intel_encoder *encoder;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> -	int pipe = intel_crtc->pipe;
>  	int ret;
>  
> -	drm_vblank_pre_modeset(dev, pipe);
> -
>  	ret = dev_priv->display.crtc_mode_set(crtc, x, y, fb);
>  
> -	drm_vblank_post_modeset(dev, pipe);
> -
>  	if (ret != 0)

Nit: There's now a blank line between ret = ... and if (...).

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 06/12] drm/doc: Discourage usage of MODESET_CTL ioctl

[Daniel Vetter]

Leftover from the old days of ums and should be used any longer. Since

commit 29935554b384b1b3a7377d6f0b03b21d18a61683
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date:   Wed May 30 00:58:09 2012 +0200

    drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers

it is a complete no-Op for kms drivers.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 83dd0b043c28..e37a77a72b8b 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2861,12 +2861,11 @@ int num_ioctls;</synopsis>
             <term>DRM_IOCTL_MODESET_CTL</term>
             <listitem>
               <para>
-                This should be called by application level drivers before and
-                after mode setting, since on many devices the vertical blank
-                counter is reset at that time.  Internally, the DRM snapshots
-                the last vblank count when the ioctl is called with the
-                _DRM_PRE_MODESET command, so that the counter won't go backwards
-                (which is dealt with when _DRM_POST_MODESET is used).
+		This was only used for user-mode-settind drivers around
+		modesetting changes to allow the kernel to update the vblank
+		interrupt after mode setting, since on many devices the vertical
+		blank. Modern drivers should not call this any more since with
+		kernel mode setting it is a no-Op.
               </para>
             </listitem>
           </varlistentry>
-- 
1.8.3.1

Re: [PATCH 06/12] drm/doc: Discourage usage of MODESET_CTL ioctl

[Michel Dänzer]

On 15.05.2014 03:51, Daniel Vetter wrote:
> Leftover from the old days of ums and should be used any longer. Since
> 
> commit 29935554b384b1b3a7377d6f0b03b21d18a61683
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date:   Wed May 30 00:58:09 2012 +0200
> 
>     drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers
> 
> it is a complete no-Op for kms drivers.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 83dd0b043c28..e37a77a72b8b 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2861,12 +2861,11 @@ int num_ioctls;</synopsis>
>              <term>DRM_IOCTL_MODESET_CTL</term>
>              <listitem>
>                <para>
> -                This should be called by application level drivers before and
> -                after mode setting, since on many devices the vertical blank
> -                counter is reset at that time.  Internally, the DRM snapshots
> -                the last vblank count when the ioctl is called with the
> -                _DRM_PRE_MODESET command, so that the counter won't go backwards
> -                (which is dealt with when _DRM_POST_MODESET is used).
> +		This was only used for user-mode-settind drivers around
> +		modesetting changes to allow the kernel to update the vblank
> +		interrupt after mode setting, since on many devices the vertical
> +		blank.

This sentence looks mangled pretty badly. Did you have something like
this in mind?

"This was only used for user-mode-setting drivers around modesetting
changes to allow the kernel to keep the vertical blank counters
consistent, since on many devices the hardware vertical blank counter is
reset to 0 at some point during modeset changes."


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer

[PATCH] drm/doc: Discourage usage of MODESET_CTL ioctl

[Daniel Vetter]

Leftover from the old days of ums and should be used any longer. Since

commit 29935554b384b1b3a7377d6f0b03b21d18a61683
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date:   Wed May 30 00:58:09 2012 +0200

    drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers

it is a complete no-Op for kms drivers.

v2: Fix up mangled sentence spotted by Michel.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Michel Dänzer <michel@daenzer.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 83dd0b043c28..96cee6438472 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2861,12 +2861,12 @@ int num_ioctls;</synopsis>
             <term>DRM_IOCTL_MODESET_CTL</term>
             <listitem>
               <para>
-                This should be called by application level drivers before and
-                after mode setting, since on many devices the vertical blank
-                counter is reset at that time.  Internally, the DRM snapshots
-                the last vblank count when the ioctl is called with the
-                _DRM_PRE_MODESET command, so that the counter won't go backwards
-                (which is dealt with when _DRM_POST_MODESET is used).
+		This was only used for user-mode-settind drivers around
+		modesetting changes to allow the kernel to update the vblank
+		interrupt after mode setting, since on many devices the vertical
+		blank counter is reset to 0 at some point during modeset. Modern
+		drivers should not call this any more since with kernel mode
+		setting it is a no-op.
               </para>
             </listitem>
           </varlistentry>
-- 
1.8.3.1

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

Re: [PATCH] drm/doc: Discourage usage of MODESET_CTL ioctl

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Thursday 15 May 2014 15:00:08 Daniel Vetter wrote:
> Leftover from the old days of ums and should be used any longer. Since
> 
> commit 29935554b384b1b3a7377d6f0b03b21d18a61683
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date:   Wed May 30 00:58:09 2012 +0200
> 
>     drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers
> 
> it is a complete no-Op for kms drivers.
> 
> v2: Fix up mangled sentence spotted by Michel.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/DocBook/drm.tmpl | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 83dd0b043c28..96cee6438472 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2861,12 +2861,12 @@ int num_ioctls;</synopsis>
>              <term>DRM_IOCTL_MODESET_CTL</term>
>              <listitem>
>                <para>
> -                This should be called by application level drivers before
> and -                after mode setting, since on many devices the vertical
> blank -                counter is reset at that time.  Internally, the DRM
> snapshots -                the last vblank count when the ioctl is called
> with the -                _DRM_PRE_MODESET command, so that the counter
> won't go backwards -                (which is dealt with when
> _DRM_POST_MODESET is used). +		This was only used for user-mode-settind
> drivers around
> +		modesetting changes to allow the kernel to update the vblank
> +		interrupt after mode setting, since on many devices the vertical
> +		blank counter is reset to 0 at some point during modeset. Modern
> +		drivers should not call this any more since with kernel mode
> +		setting it is a no-op.
>                </para>
>              </listitem>
>            </varlistentry>

-- 
Regards,

Laurent Pinchart

[PATCH 07/12] drm/irq: kerneldoc polish

[Daniel Vetter]

- Integrate into the drm DocBook
- Disable kerneldoc for functions not exported to drivers.
- Properly document the new drm_vblank_on|off and add cautious
  comments explaining when drm_vblank_pre|post_modesets shouldn't be
  used.
- General polish and OCD.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl |   5 +-
 drivers/gpu/drm/drm_irq.c      | 181 ++++++++++++++++++++++++++++-------------
 2 files changed, 129 insertions(+), 57 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index e37a77a72b8b..70ef87bcea04 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2519,6 +2519,10 @@ void (*disable_vblank) (struct drm_device *dev, int crtc);</synopsis>
       with a call to <function>drm_vblank_cleanup</function> in the driver
       <methodname>unload</methodname> operation handler.
     </para>
+    <sect2>
+      <title>Vertical Blanking and Interrupt Handling Functions Reference</title>
+!Edrivers/gpu/drm/drm_irq.c
+    </sect2>
   </sect1>
 
   <!-- Internals: open/close, file operations and ioctls -->
@@ -2870,7 +2874,6 @@ int num_ioctls;</synopsis>
             </listitem>
           </varlistentry>
         </variablelist>
-<!--!Edrivers/char/drm/drm_irq.c-->
       </para>
     </sect1>
 
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index dd786d84daab..5ff986bd4de4 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1,6 +1,5 @@
-/**
- * \file drm_irq.c
- * IRQ support
+/*
+ * drm_irq.c IRQ and vblank support
  *
  * \author Rickard E. (Rik) Faith <faith@valinux.com>
  * \author Gareth Hughes <gareth@valinux.com>
@@ -156,6 +155,12 @@ static void vblank_disable_fn(unsigned long arg)
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 }
 
+/**
+ * drm_vblank_cleanup - cleanup vblank support
+ * @dev: drm device
+ *
+ * This function cleans up any resources allocated in drm_vblank_init.
+ */
 void drm_vblank_cleanup(struct drm_device *dev)
 {
 	int crtc;
@@ -175,6 +180,16 @@ void drm_vblank_cleanup(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_vblank_cleanup);
 
+/**
+ * drm_vblank_init - initialize vblank support
+ * @dev: drm_device
+ * @num_crtcs: number of crtcs supported by @dev
+ *
+ * This function initializes vblank support for @num_crtcs display pipelines.
+ *
+ * Returns:
+ * Zero on success or a negative error code on failure.
+ */
 int drm_vblank_init(struct drm_device *dev, int num_crtcs)
 {
 	int i, ret = -ENOMEM;
@@ -238,13 +253,21 @@ static void drm_irq_vgaarb_nokms(void *cookie, bool state)
 }
 
 /**
- * Install IRQ handler.
- *
- * \param dev DRM device.
+ * drm_irq_install - install IRQ handler
+ * @dev: drm device
+ * @irq: irq number to install the handler for
  *
  * Initializes the IRQ related data. Installs the handler, calling the driver
- * \c irq_preinstall() and \c irq_postinstall() functions
- * before and after the installation.
+ * irq_preinstall() and irq_postinstall() functions before and after the
+ * installation.
+ *
+ * This is the simplified helper interface provided for drivers with no special
+ * needs. Drivers which need to install interrupt handlers for multiple
+ * interrupts must instead set drm_device->irq_enabled to signal the drm core
+ * that vblank interrupts are available.
+ *
+ * Returns:
+ * Zero on success or a negative error code on failure.
  */
 int drm_irq_install(struct drm_device *dev, int irq)
 {
@@ -304,11 +327,20 @@ int drm_irq_install(struct drm_device *dev, int irq)
 EXPORT_SYMBOL(drm_irq_install);
 
 /**
- * Uninstall the IRQ handler.
+ * drm_irq_uninstall - uninstall the IRQ handler
+ * @dev: drm device
+ *
+ * Calls the driver's irq_uninstall() function and unregisters the irq handler.
+ * This should only be called by drivers which used drm_irq_install() to set up
+ * their interrupt handler. Other drivers must only reset
+ * drm_device->irq_enabled to false.
  *
- * \param dev DRM device.
+ * Note that for kernel modesetting drivers it is a bug if this function fails.
+ * The sanity checks are only to catch buggy user modesetting drivers which call
+ * the same function through an ioctl.
  *
- * Calls the driver's \c irq_uninstall() function, and stops the irq.
+ * Returns:
+ * Zero on success or a negative error code on failure.
  */
 int drm_irq_uninstall(struct drm_device *dev)
 {
@@ -353,7 +385,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_irq_uninstall);
 
-/**
+/*
  * IRQ control ioctl.
  *
  * \param inode device inode.
@@ -406,10 +438,9 @@ int drm_control(struct drm_device *dev, void *data,
 }
 
 /**
- * drm_calc_timestamping_constants - Calculate vblank timestamp constants
- *
- * @crtc drm_crtc whose timestamp constants should be updated.
- * @mode display mode containing the scanout timings
+ * drm_calc_timestamping_constants - calculate vblank timestamp constants
+ * @crtc: drm_crtc whose timestamp constants should be updated.
+ * @mode: display mode containing the scanout timings
  *
  * Calculate and store various constants which are later
  * needed by vblank and swap-completion timestamping, e.g,
@@ -459,11 +490,22 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 EXPORT_SYMBOL(drm_calc_timestamping_constants);
 
 /**
- * drm_calc_vbltimestamp_from_scanoutpos - helper routine for kms
- * drivers. Implements calculation of exact vblank timestamps from
- * given drm_display_mode timings and current video scanout position
- * of a crtc. This can be called from within get_vblank_timestamp()
- * implementation of a kms driver to implement the actual timestamping.
+ * drm_calc_vbltimestamp_from_scanoutpos - precise vblank timestamp helper
+ * @dev: drm device
+ * @crtc: Which crtc's vblank timestamp to retrieve
+ * @max_error: Desired maximum allowable error in timestamps (nanosecs)
+ *             On return contains true maximum error of timestamp
+ * @vblank_time: Pointer to struct timeval which should receive the timestamp
+ * @flags: Flags to pass to driver:
+ *         0 = Default,
+ *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler
+ * @refcrtc: drm_crtc* of crtc which defines scanout timing
+ * @mode: mode which defines the scanout timings
+ *
+ * Implements calculation of exact vblank timestamps from given drm_display_mode
+ * timings and current video scanout position of a crtc. This can be called from
+ * within get_vblank_timestamp() implementation of a kms driver to implement the
+ * actual timestamping.
  *
  * Should return timestamps conforming to the OML_sync_control OpenML
  * extension specification. The timestamp corresponds to the end of
@@ -478,18 +520,8 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
  * returns as no operation if a doublescan or interlaced video mode is
  * active. Higher level code is expected to handle this.
  *
- * @dev: DRM device.
- * @crtc: Which crtc's vblank timestamp to retrieve.
- * @max_error: Desired maximum allowable error in timestamps (nanosecs).
- *             On return contains true maximum error of timestamp.
- * @vblank_time: Pointer to struct timeval which should receive the timestamp.
- * @flags: Flags to pass to driver:
- *         0 = Default.
- *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler.
- * @refcrtc: drm_crtc* of crtc which defines scanout timing.
- * @mode: mode which defines the scanout timings
- *
- * Returns negative value on error, failure or if not supported in current
+ * Returns:
+ * Negative value on error, failure or if not supported in current
  * video mode:
  *
  * -EINVAL   - Invalid crtc.
@@ -641,14 +673,13 @@ static struct timeval get_drm_timestamp(void)
 
 /**
  * drm_get_last_vbltimestamp - retrieve raw timestamp for the most recent
- * vblank interval.
- *
- * @dev: DRM device
+ * 			       vblank interval
+ * @dev: drm device
  * @crtc: which crtc's vblank timestamp to retrieve
  * @tvblank: Pointer to target struct timeval which should receive the timestamp
  * @flags: Flags to pass to driver:
- *         0 = Default.
- *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler.
+ *         0 = Default,
+ *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler
  *
  * Fetches the system timestamp corresponding to the time of the most recent
  * vblank interval on specified crtc. May call into kms-driver to
@@ -657,7 +688,8 @@ static struct timeval get_drm_timestamp(void)
  * Returns zero if timestamp originates from uncorrected do_gettimeofday()
  * call, i.e., it isn't very precisely locked to the true vblank.
  *
- * Returns non-zero if timestamp is considered to be very precise.
+ * Returns:
+ * Non-zero if timestamp is considered to be very precise, zero otherwise.
  */
 u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
 			      struct timeval *tvblank, unsigned flags)
@@ -686,12 +718,15 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
 
 /**
  * drm_vblank_count - retrieve "cooked" vblank counter value
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: which counter to retrieve
  *
  * Fetches the "cooked" vblank count value that represents the number of
  * vblank events since the system was booted, including lost events due to
  * modesetting activity.
+ *
+ * Returns:
+ * The software vblank counter.
  */
 u32 drm_vblank_count(struct drm_device *dev, int crtc)
 {
@@ -703,15 +738,14 @@ EXPORT_SYMBOL(drm_vblank_count);
  * drm_vblank_count_and_time - retrieve "cooked" vblank counter value
  * and the system timestamp corresponding to that vblank counter value.
  *
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: which counter to retrieve
  * @vblanktime: Pointer to struct timeval to receive the vblank timestamp.
  *
  * Fetches the "cooked" vblank count value that represents the number of
  * vblank events since the system was booted, including lost events due to
  * modesetting activity. Returns corresponding system timestamp of the time
- * of the vblank interval that corresponds to the current value vblank counter
- * value.
+ * of the vblank interval that corresponds to the current vblank counter value.
  */
 u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
 			      struct timeval *vblanktime)
@@ -751,7 +785,7 @@ static void send_vblank_event(struct drm_device *dev,
 
 /**
  * drm_send_vblank_event - helper to send vblank event after pageflip
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: CRTC in question
  * @e: the event to send
  *
@@ -777,7 +811,7 @@ EXPORT_SYMBOL(drm_send_vblank_event);
 
 /**
  * drm_update_vblank_count - update the master vblank counter
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: counter to update
  *
  * Call back into the driver to update the appropriate vblank counter
@@ -841,7 +875,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 
 /**
  * drm_vblank_enable - enable the vblank interrupt on a CRTC
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: CRTC in question
  */
 static int drm_vblank_enable(struct drm_device *dev, int crtc)
@@ -876,13 +910,13 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc)
 
 /**
  * drm_vblank_get - get a reference count on vblank events
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: which CRTC to own
  *
  * Acquire a reference count on vblank events to avoid having them disabled
  * while in use.
  *
- * RETURNS
+ * Returns:
  * Zero on success, nonzero on failure.
  */
 int drm_vblank_get(struct drm_device *dev, int crtc)
@@ -908,7 +942,7 @@ EXPORT_SYMBOL(drm_vblank_get);
 
 /**
  * drm_vblank_put - give up ownership of vblank events
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: which counter to give up
  *
  * Release ownership of a given vblank counter, turning off interrupts
@@ -928,8 +962,15 @@ EXPORT_SYMBOL(drm_vblank_put);
 
 /**
  * drm_vblank_off - disable vblank events on a CRTC
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: CRTC in question
+ *
+ * Drivers can use this function to shut down the vblank interrupt handling when
+ * disabling a crtc. This function ensures that the latest vblank frame count is
+ * stored so that drm_vblank_on can restore it again.
+ *
+ * Drivers must use this function when the hardware vblank counter can get
+ * reset, e.g. when suspending.
  */
 void drm_vblank_off(struct drm_device *dev, int crtc)
 {
@@ -964,8 +1005,13 @@ EXPORT_SYMBOL(drm_vblank_off);
 
 /**
  * drm_vblank_on - enable vblank events on a CRTC
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: CRTC in question
+ *
+ * This functions restores the vblank interrupt state captured with
+ * drm_vblank_off() again. Note that calls to drm_vblank_on() and
+ * drm_vblank_off() can be unbalanced and so can also be unconditionaly called
+ * in driver load code to reflect the current hardware state of the crtc.
  */
 void drm_vblank_on(struct drm_device *dev, int crtc)
 {
@@ -981,11 +1027,26 @@ EXPORT_SYMBOL(drm_vblank_on);
 
 /**
  * drm_vblank_pre_modeset - account for vblanks across mode sets
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: CRTC in question
  *
  * Account for vblank events across mode setting events, which will likely
  * reset the hardware frame counter.
+ *
+ * This is done by grabbing a temporary vblank reference to ensure that the
+ * vblank interrupt keeps running across the modeset sequence. With this the
+ * software-side vblank frame counting will ensure that there are no jumps or
+ * discontinuities.
+ *
+ * Unfortunately this approach is racy and also doesn't work when the vblank
+ * interrupt stops running, e.g. across system suspend resume. It is therefore
+ * highly recommended that drivers use the newer drm_vblank_off() and
+ * drm_vblank_on() instead. drm_vblank_pre_modeset() only works correctly when
+ * using "cooked" software vblank frame counters and not relying on any hardware
+ * counters.
+ *
+ * Drivers must call drm_vblank_post_modeset() when re-enabling the same crtc
+ * again.
  */
 void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
 {
@@ -1007,6 +1068,14 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
 }
 EXPORT_SYMBOL(drm_vblank_pre_modeset);
 
+/**
+ * drm_vblank_post_modeset - undo drm_vblank_pre_modeset changes
+ * @dev: drm device
+ * @crtc: CRTC in question
+ *
+ * This function again drops the temporary vblank reference acquired in
+ * drm_vblank_pre_modeset.
+ */
 void drm_vblank_post_modeset(struct drm_device *dev, int crtc)
 {
 	unsigned long irqflags;
@@ -1028,7 +1097,7 @@ void drm_vblank_post_modeset(struct drm_device *dev, int crtc)
 }
 EXPORT_SYMBOL(drm_vblank_post_modeset);
 
-/**
+/*
  * drm_modeset_ctl - handle vblank event counter changes across mode switch
  * @DRM_IOCTL_ARGS: standard ioctl arguments
  *
@@ -1141,11 +1210,11 @@ err_put:
 	return ret;
 }
 
-/**
+/*
  * Wait for VBLANK.
  *
  * \param inode device inode.
- * \param file_priv DRM file private.
+ * \param file_priv drm file private.
  * \param cmd command.
  * \param data user argument, pointing to a drm_wait_vblank structure.
  * \return zero on success or a negative number on failure.
@@ -1276,7 +1345,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
 
 /**
  * drm_handle_vblank - handle a vblank event
- * @dev: DRM device
+ * @dev: drm device
  * @crtc: where this event occurred
  *
  * Drivers should call this routine in their vblank interrupt handlers to
-- 
1.8.3.1

Re: [PATCH 07/12] drm/irq: kerneldoc polish

[Michel Dänzer]

On 15.05.2014 03:51, Daniel Vetter wrote:
> @@ -964,8 +1005,13 @@ EXPORT_SYMBOL(drm_vblank_off);
>  
>  /**
>   * drm_vblank_on - enable vblank events on a CRTC
> - * @dev: DRM device
> + * @dev: drm device
>   * @crtc: CRTC in question
> + *
> + * This functions restores the vblank interrupt state captured with
> + * drm_vblank_off() again. Note that calls to drm_vblank_on() and
> + * drm_vblank_off() can be unbalanced and so can also be unconditionaly called
> + * in driver load code to reflect the current hardware state of the crtc.

Given this description, maybe something like drm_vblank_save/restore
would describe better what these functions do than drm_vblank_off/on?


> @@ -981,11 +1027,26 @@ EXPORT_SYMBOL(drm_vblank_on);
>  
>  /**
>   * drm_vblank_pre_modeset - account for vblanks across mode sets
> - * @dev: DRM device
> + * @dev: drm device
>   * @crtc: CRTC in question
>   *
>   * Account for vblank events across mode setting events, which will likely
>   * reset the hardware frame counter.
> + *
> + * This is done by grabbing a temporary vblank reference to ensure that the
> + * vblank interrupt keeps running across the modeset sequence. With this the
> + * software-side vblank frame counting will ensure that there are no jumps or
> + * discontinuities.
> + *
> + * Unfortunately this approach is racy and also doesn't work when the vblank
> + * interrupt stops running, e.g. across system suspend resume. It is therefore
> + * highly recommended that drivers use the newer drm_vblank_off() and
> + * drm_vblank_on() instead. drm_vblank_pre_modeset() only works correctly when
> + * using "cooked" software vblank frame counters and not relying on any hardware
> + * counters.

That last statement is not true IME with radeon[0].

Basically, it sounds to me like drm_vblank_off/on do more or less what
drm_vblank_pre/post_modeset are intended to do (e.g. the latter can also
be nested arbitrarily). Still not really sure why we need two sets of
these instead of fixing any problems in one set.


[0] Though we certainly don't have as rigorous testing for that as you
guys do in intel-gpu-tools. Any chance some of that could be moved to
somewhere more generic?

-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer

Re: [PATCH 07/12] drm/irq: kerneldoc polish

[Daniel Vetter]

On Thu, May 15, 2014 at 01:44:04PM +0900, Michel Dänzer wrote:
> On 15.05.2014 03:51, Daniel Vetter wrote:
> > @@ -964,8 +1005,13 @@ EXPORT_SYMBOL(drm_vblank_off);
> >  
> >  /**
> >   * drm_vblank_on - enable vblank events on a CRTC
> > - * @dev: DRM device
> > + * @dev: drm device
> >   * @crtc: CRTC in question
> > + *
> > + * This functions restores the vblank interrupt state captured with
> > + * drm_vblank_off() again. Note that calls to drm_vblank_on() and
> > + * drm_vblank_off() can be unbalanced and so can also be unconditionaly called
> > + * in driver load code to reflect the current hardware state of the crtc.
> 
> Given this description, maybe something like drm_vblank_save/restore
> would describe better what these functions do than drm_vblank_off/on?

It also enables/disables the vblank machinery itself, which at least in
i915 will be important shortly - we slowly switch over our code to be more
interrupt driven, including the initial plane enabling/disabling in
modeset changes.

> > @@ -981,11 +1027,26 @@ EXPORT_SYMBOL(drm_vblank_on);
> >  
> >  /**
> >   * drm_vblank_pre_modeset - account for vblanks across mode sets
> > - * @dev: DRM device
> > + * @dev: drm device
> >   * @crtc: CRTC in question
> >   *
> >   * Account for vblank events across mode setting events, which will likely
> >   * reset the hardware frame counter.
> > + *
> > + * This is done by grabbing a temporary vblank reference to ensure that the
> > + * vblank interrupt keeps running across the modeset sequence. With this the
> > + * software-side vblank frame counting will ensure that there are no jumps or
> > + * discontinuities.
> > + *
> > + * Unfortunately this approach is racy and also doesn't work when the vblank
> > + * interrupt stops running, e.g. across system suspend resume. It is therefore
> > + * highly recommended that drivers use the newer drm_vblank_off() and
> > + * drm_vblank_on() instead. drm_vblank_pre_modeset() only works correctly when
> > + * using "cooked" software vblank frame counters and not relying on any hardware
> > + * counters.
> 
> That last statement is not true IME with radeon[0].
> 
> Basically, it sounds to me like drm_vblank_off/on do more or less what
> drm_vblank_pre/post_modeset are intended to do (e.g. the latter can also
> be nested arbitrarily). Still not really sure why we need two sets of
> these instead of fixing any problems in one set.

The problem is that the driver situation is a mess. So the right plan imo
is:
1) Create new functions that work, beat on them.
2) Convert all drivers.
3) Rip out the old stuff.

I've tried to look into this a bit and decided that this isn't something I
can do without the hardware at hand and a few solid tests. So this series
is 1) done for i915, with an rfc for 2).

Also there's the issue of old ums using the same stuff and I think we
shouldn't touch that can of worms at all.

> [0] Though we certainly don't have as rigorous testing for that as you
> guys do in intel-gpu-tools. Any chance some of that could be moved to
> somewhere more generic?

igt is mostly ready - what we need is some rather thin abstraction for the
kms tests to run also on other drivers, with dumb objects. Otherwise all
the test framework can cope with random bits not being there and skipping
those subtests, e.g. the CRC based stuff.

I don't want to extract the generic parts of the tests into a different
repo (at least not if there's not _lots_ of people working on them) since
the code sharing we currently do between tests is fairly massive.

But I'm willing to deal with the hassle of supporting other drivers for
e.g. the kms tests. And I think it would make a nice gsoc project. But
it's definitely not something I can throw intel resource (or too much of
my own time) at ;-)

Having a shared set of tests which clearly spells out all the corner cases
and tests for races would imo be awesome and greatly improve the overall
health of the drm/kms drivers and wider ecosystem.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 07/12] drm/irq: kerneldoc polish

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2991 bytes --]

On Wed, May 14, 2014 at 08:51:09PM +0200, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
[...]
> index dd786d84daab..5ff986bd4de4 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1,6 +1,5 @@
> -/**
> - * \file drm_irq.c
> - * IRQ support
> +/*
> + * drm_irq.c IRQ and vblank support

Perhaps drop the filename here? It's completely redundant.

> +/**
> + * drm_vblank_cleanup - cleanup vblank support
> + * @dev: drm device

Technically DRM is an abbreviation and therefore should be all
uppercase.

> +/**
> + * drm_vblank_init - initialize vblank support
> + * @dev: drm_device

s/drm_device/DRM device/?

> + * @num_crtcs: number of crtcs supported by @dev

CRTC is also an abbreviation, so I think this should be s/crtcs/CRTCs/.

> + * Returns:

According to Documentation/kernel-doc-nano-HOWTO.txt this section should
be called "Return:". I'm not sure it matters all that much, since it
seems to be only used as the title of a new section, but I think it
makes sense to follow the recommendation of the documentation.

>  /**
> - * Install IRQ handler.
> - *
> - * \param dev DRM device.
> + * drm_irq_install - install IRQ handler
> + * @dev: drm device
> + * @irq: irq number to install the handler for

I know you're probably going to hate me for this, but: IRQ is an
abbreviation. =)

> -/**
> +/*
>   * IRQ control ioctl.
>   *
>   * \param inode device inode.

Perhaps it would still make sense to change the comment style to
kernel-doc, even if it isn't exported.

>  /**
> - * drm_calc_vbltimestamp_from_scanoutpos - helper routine for kms
> - * drivers. Implements calculation of exact vblank timestamps from
> - * given drm_display_mode timings and current video scanout position
> - * of a crtc. This can be called from within get_vblank_timestamp()
> - * implementation of a kms driver to implement the actual timestamping.
> + * drm_calc_vbltimestamp_from_scanoutpos - precise vblank timestamp helper
> + * @dev: drm device
> + * @crtc: Which crtc's vblank timestamp to retrieve
> + * @max_error: Desired maximum allowable error in timestamps (nanosecs)
> + *             On return contains true maximum error of timestamp
> + * @vblank_time: Pointer to struct timeval which should receive the timestamp
> + * @flags: Flags to pass to driver:
> + *         0 = Default,
> + *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler
> + * @refcrtc: drm_crtc* of crtc which defines scanout timing

I think s/drm_crtc * of// would be okay here.

>  /**
>   * drm_vblank_off - disable vblank events on a CRTC
> - * @dev: DRM device
> + * @dev: drm device
>   * @crtc: CRTC in question
> + *
> + * Drivers can use this function to shut down the vblank interrupt handling when
> + * disabling a crtc. This function ensures that the latest vblank frame count is
> + * stored so that drm_vblank_on can restore it again.

drm_vblank_on() to have it correctly highlighted?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH] drm/irq: kerneldoc polish

[Daniel Vetter]

- Integrate into the drm DocBook
- Disable kerneldoc for functions not exported to drivers.
- Properly document the new drm_vblank_on|off and add cautious
  comments explaining when drm_vblank_pre|post_modesets shouldn't be
  used.
- General polish and OCD.

v2: Polish as suggested by Thierry.

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl |   5 +-
 drivers/gpu/drm/drm_irq.c      | 165 +++++++++++++++++++++++++++++------------
 2 files changed, 121 insertions(+), 49 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 96cee6438472..9c47e3147ad1 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2519,6 +2519,10 @@ void (*disable_vblank) (struct drm_device *dev, int crtc);</synopsis>
       with a call to <function>drm_vblank_cleanup</function> in the driver
       <methodname>unload</methodname> operation handler.
     </para>
+    <sect2>
+      <title>Vertical Blanking and Interrupt Handling Functions Reference</title>
+!Edrivers/gpu/drm/drm_irq.c
+    </sect2>
   </sect1>
 
   <!-- Internals: open/close, file operations and ioctls -->
@@ -2871,7 +2875,6 @@ int num_ioctls;</synopsis>
             </listitem>
           </varlistentry>
         </variablelist>
-<!--!Edrivers/char/drm/drm_irq.c-->
       </para>
     </sect1>
 
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index dd786d84daab..f3dafccca456 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1,6 +1,5 @@
-/**
- * \file drm_irq.c
- * IRQ support
+/*
+ * drm_irq.c IRQ and vblank support
  *
  * \author Rickard E. (Rik) Faith <faith@valinux.com>
  * \author Gareth Hughes <gareth@valinux.com>
@@ -156,6 +155,12 @@ static void vblank_disable_fn(unsigned long arg)
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 }
 
+/**
+ * drm_vblank_cleanup - cleanup vblank support
+ * @dev: DRM device
+ *
+ * This function cleans up any resources allocated in drm_vblank_init.
+ */
 void drm_vblank_cleanup(struct drm_device *dev)
 {
 	int crtc;
@@ -175,6 +180,16 @@ void drm_vblank_cleanup(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_vblank_cleanup);
 
+/**
+ * drm_vblank_init - initialize vblank support
+ * @dev: drm_device
+ * @num_crtcs: number of crtcs supported by @dev
+ *
+ * This function initializes vblank support for @num_crtcs display pipelines.
+ *
+ * Returns:
+ * Zero on success or a negative error code on failure.
+ */
 int drm_vblank_init(struct drm_device *dev, int num_crtcs)
 {
 	int i, ret = -ENOMEM;
@@ -238,13 +253,21 @@ static void drm_irq_vgaarb_nokms(void *cookie, bool state)
 }
 
 /**
- * Install IRQ handler.
- *
- * \param dev DRM device.
+ * drm_irq_install - install IRQ handler
+ * @dev: DRM device
+ * @irq: IRQ number to install the handler for
  *
  * Initializes the IRQ related data. Installs the handler, calling the driver
- * \c irq_preinstall() and \c irq_postinstall() functions
- * before and after the installation.
+ * irq_preinstall() and irq_postinstall() functions before and after the
+ * installation.
+ *
+ * This is the simplified helper interface provided for drivers with no special
+ * needs. Drivers which need to install interrupt handlers for multiple
+ * interrupts must instead set drm_device->irq_enabled to signal the DRM core
+ * that vblank interrupts are available.
+ *
+ * Returns:
+ * Zero on success or a negative error code on failure.
  */
 int drm_irq_install(struct drm_device *dev, int irq)
 {
@@ -304,11 +327,20 @@ int drm_irq_install(struct drm_device *dev, int irq)
 EXPORT_SYMBOL(drm_irq_install);
 
 /**
- * Uninstall the IRQ handler.
+ * drm_irq_uninstall - uninstall the IRQ handler
+ * @dev: DRM device
+ *
+ * Calls the driver's irq_uninstall() function and unregisters the IRQ handler.
+ * This should only be called by drivers which used drm_irq_install() to set up
+ * their interrupt handler. Other drivers must only reset
+ * drm_device->irq_enabled to false.
  *
- * \param dev DRM device.
+ * Note that for kernel modesetting drivers it is a bug if this function fails.
+ * The sanity checks are only to catch buggy user modesetting drivers which call
+ * the same function through an ioctl.
  *
- * Calls the driver's \c irq_uninstall() function, and stops the irq.
+ * Returns:
+ * Zero on success or a negative error code on failure.
  */
 int drm_irq_uninstall(struct drm_device *dev)
 {
@@ -353,7 +385,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_irq_uninstall);
 
-/**
+/*
  * IRQ control ioctl.
  *
  * \param inode device inode.
@@ -406,15 +438,14 @@ int drm_control(struct drm_device *dev, void *data,
 }
 
 /**
- * drm_calc_timestamping_constants - Calculate vblank timestamp constants
- *
- * @crtc drm_crtc whose timestamp constants should be updated.
- * @mode display mode containing the scanout timings
+ * drm_calc_timestamping_constants - calculate vblank timestamp constants
+ * @crtc: drm_crtc whose timestamp constants should be updated.
+ * @mode: display mode containing the scanout timings
  *
  * Calculate and store various constants which are later
  * needed by vblank and swap-completion timestamping, e.g,
  * by drm_calc_vbltimestamp_from_scanoutpos(). They are
- * derived from crtc's true scanout timing, so they take
+ * derived from CRTC's true scanout timing, so they take
  * things like panel scaling or other adjustments into account.
  */
 void drm_calc_timestamping_constants(struct drm_crtc *crtc,
@@ -459,11 +490,22 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 EXPORT_SYMBOL(drm_calc_timestamping_constants);
 
 /**
- * drm_calc_vbltimestamp_from_scanoutpos - helper routine for kms
- * drivers. Implements calculation of exact vblank timestamps from
- * given drm_display_mode timings and current video scanout position
- * of a crtc. This can be called from within get_vblank_timestamp()
- * implementation of a kms driver to implement the actual timestamping.
+ * drm_calc_vbltimestamp_from_scanoutpos - precise vblank timestamp helper
+ * @dev: DRM device
+ * @crtc: Which CRTC's vblank timestamp to retrieve
+ * @max_error: Desired maximum allowable error in timestamps (nanosecs)
+ *             On return contains true maximum error of timestamp
+ * @vblank_time: Pointer to struct timeval which should receive the timestamp
+ * @flags: Flags to pass to driver:
+ *         0 = Default,
+ *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl IRQ handler
+ * @refcrtc: CRTC which defines scanout timing
+ * @mode: mode which defines the scanout timings
+ *
+ * Implements calculation of exact vblank timestamps from given drm_display_mode
+ * timings and current video scanout position of a CRTC. This can be called from
+ * within get_vblank_timestamp() implementation of a kms driver to implement the
+ * actual timestamping.
  *
  * Should return timestamps conforming to the OML_sync_control OpenML
  * extension specification. The timestamp corresponds to the end of
@@ -478,21 +520,11 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
  * returns as no operation if a doublescan or interlaced video mode is
  * active. Higher level code is expected to handle this.
  *
- * @dev: DRM device.
- * @crtc: Which crtc's vblank timestamp to retrieve.
- * @max_error: Desired maximum allowable error in timestamps (nanosecs).
- *             On return contains true maximum error of timestamp.
- * @vblank_time: Pointer to struct timeval which should receive the timestamp.
- * @flags: Flags to pass to driver:
- *         0 = Default.
- *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler.
- * @refcrtc: drm_crtc* of crtc which defines scanout timing.
- * @mode: mode which defines the scanout timings
- *
- * Returns negative value on error, failure or if not supported in current
+ * Returns:
+ * Negative value on error, failure or if not supported in current
  * video mode:
  *
- * -EINVAL   - Invalid crtc.
+ * -EINVAL   - Invalid CRTC.
  * -EAGAIN   - Temporary unavailable, e.g., called before initial modeset.
  * -ENOTSUPP - Function not supported in current display mode.
  * -EIO      - Failed, e.g., due to failed scanout position query.
@@ -641,23 +673,23 @@ static struct timeval get_drm_timestamp(void)
 
 /**
  * drm_get_last_vbltimestamp - retrieve raw timestamp for the most recent
- * vblank interval.
- *
+ * 			       vblank interval
  * @dev: DRM device
- * @crtc: which crtc's vblank timestamp to retrieve
+ * @crtc: which CRTC's vblank timestamp to retrieve
  * @tvblank: Pointer to target struct timeval which should receive the timestamp
  * @flags: Flags to pass to driver:
- *         0 = Default.
- *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler.
+ *         0 = Default,
+ *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl IRQ handler
  *
  * Fetches the system timestamp corresponding to the time of the most recent
- * vblank interval on specified crtc. May call into kms-driver to
+ * vblank interval on specified CRTC. May call into kms-driver to
  * compute the timestamp with a high-precision GPU specific method.
  *
  * Returns zero if timestamp originates from uncorrected do_gettimeofday()
  * call, i.e., it isn't very precisely locked to the true vblank.
  *
- * Returns non-zero if timestamp is considered to be very precise.
+ * Returns:
+ * Non-zero if timestamp is considered to be very precise, zero otherwise.
  */
 u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
 			      struct timeval *tvblank, unsigned flags)
@@ -692,6 +724,9 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
  * Fetches the "cooked" vblank count value that represents the number of
  * vblank events since the system was booted, including lost events due to
  * modesetting activity.
+ *
+ * Returns:
+ * The software vblank counter.
  */
 u32 drm_vblank_count(struct drm_device *dev, int crtc)
 {
@@ -710,8 +745,7 @@ EXPORT_SYMBOL(drm_vblank_count);
  * Fetches the "cooked" vblank count value that represents the number of
  * vblank events since the system was booted, including lost events due to
  * modesetting activity. Returns corresponding system timestamp of the time
- * of the vblank interval that corresponds to the current value vblank counter
- * value.
+ * of the vblank interval that corresponds to the current vblank counter value.
  */
 u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
 			      struct timeval *vblanktime)
@@ -882,7 +916,7 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc)
  * Acquire a reference count on vblank events to avoid having them disabled
  * while in use.
  *
- * RETURNS
+ * Returns:
  * Zero on success, nonzero on failure.
  */
 int drm_vblank_get(struct drm_device *dev, int crtc)
@@ -930,6 +964,13 @@ EXPORT_SYMBOL(drm_vblank_put);
  * drm_vblank_off - disable vblank events on a CRTC
  * @dev: DRM device
  * @crtc: CRTC in question
+ *
+ * Drivers can use this function to shut down the vblank interrupt handling when
+ * disabling a crtc. This function ensures that the latest vblank frame count is
+ * stored so that drm_vblank_on() can restore it again.
+ *
+ * Drivers must use this function when the hardware vblank counter can get
+ * reset, e.g. when suspending.
  */
 void drm_vblank_off(struct drm_device *dev, int crtc)
 {
@@ -966,6 +1007,11 @@ EXPORT_SYMBOL(drm_vblank_off);
  * drm_vblank_on - enable vblank events on a CRTC
  * @dev: DRM device
  * @crtc: CRTC in question
+ *
+ * This functions restores the vblank interrupt state captured with
+ * drm_vblank_off() again. Note that calls to drm_vblank_on() and
+ * drm_vblank_off() can be unbalanced and so can also be unconditionaly called
+ * in driver load code to reflect the current hardware state of the crtc.
  */
 void drm_vblank_on(struct drm_device *dev, int crtc)
 {
@@ -986,6 +1032,21 @@ EXPORT_SYMBOL(drm_vblank_on);
  *
  * Account for vblank events across mode setting events, which will likely
  * reset the hardware frame counter.
+ *
+ * This is done by grabbing a temporary vblank reference to ensure that the
+ * vblank interrupt keeps running across the modeset sequence. With this the
+ * software-side vblank frame counting will ensure that there are no jumps or
+ * discontinuities.
+ *
+ * Unfortunately this approach is racy and also doesn't work when the vblank
+ * interrupt stops running, e.g. across system suspend resume. It is therefore
+ * highly recommended that drivers use the newer drm_vblank_off() and
+ * drm_vblank_on() instead. drm_vblank_pre_modeset() only works correctly when
+ * using "cooked" software vblank frame counters and not relying on any hardware
+ * counters.
+ *
+ * Drivers must call drm_vblank_post_modeset() when re-enabling the same crtc
+ * again.
  */
 void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
 {
@@ -1007,6 +1068,14 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
 }
 EXPORT_SYMBOL(drm_vblank_pre_modeset);
 
+/**
+ * drm_vblank_post_modeset - undo drm_vblank_pre_modeset changes
+ * @dev: DRM device
+ * @crtc: CRTC in question
+ *
+ * This function again drops the temporary vblank reference acquired in
+ * drm_vblank_pre_modeset.
+ */
 void drm_vblank_post_modeset(struct drm_device *dev, int crtc)
 {
 	unsigned long irqflags;
@@ -1028,7 +1097,7 @@ void drm_vblank_post_modeset(struct drm_device *dev, int crtc)
 }
 EXPORT_SYMBOL(drm_vblank_post_modeset);
 
-/**
+/*
  * drm_modeset_ctl - handle vblank event counter changes across mode switch
  * @DRM_IOCTL_ARGS: standard ioctl arguments
  *
@@ -1141,7 +1210,7 @@ err_put:
 	return ret;
 }
 
-/**
+/*
  * Wait for VBLANK.
  *
  * \param inode device inode.
@@ -1152,7 +1221,7 @@ err_put:
  *
  * This function enables the vblank interrupt on the pipe requested, then
  * sleeps waiting for the requested sequence number to occur, and drops
- * the vblank interrupt refcount afterwards. (vblank irq disable follows that
+ * the vblank interrupt refcount afterwards. (vblank IRQ disable follows that
  * after a timeout with no further vblank waits scheduled).
  */
 int drm_wait_vblank(struct drm_device *dev, void *data,
-- 
1.8.3.1

[PATCH 08/12] drm/irq: Add kms-native crtc interface functions

[Daniel Vetter]

We need to start somewhere ... With this the only places left in i915
where we use pipe integers is in the interrupt handling code. And
there it actually makes some amount of sense.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c            | 81 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 22 +++++-----
 include/drm/drmP.h                   |  5 +++
 3 files changed, 98 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 5ff986bd4de4..51ebe9086be9 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -916,6 +916,8 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc)
  * Acquire a reference count on vblank events to avoid having them disabled
  * while in use.
  *
+ * This is the legacy version of drm_crtc_vblank_get().
+ *
  * Returns:
  * Zero on success, nonzero on failure.
  */
@@ -941,12 +943,33 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
 EXPORT_SYMBOL(drm_vblank_get);
 
 /**
+ * drm_crtc_vblank_get - get a reference count on vblank events
+ * @dev: drm device
+ * @crtc: which CRTC to own
+ *
+ * Acquire a reference count on vblank events to avoid having them disabled
+ * while in use.
+ *
+ * This is the native kms version of drm_vblank_off().
+ *
+ * Returns:
+ * Zero on success, nonzero on failure.
+ */
+int drm_crtc_vblank_get(struct drm_device *dev, struct drm_crtc *crtc)
+{
+	return drm_vblank_get(dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_get);
+
+/**
  * drm_vblank_put - give up ownership of vblank events
  * @dev: drm device
  * @crtc: which counter to give up
  *
  * Release ownership of a given vblank counter, turning off interrupts
  * if possible. Disable interrupts after drm_vblank_offdelay milliseconds.
+ *
+ * This is the legacy version of drm_crtc_vblank_put().
  */
 void drm_vblank_put(struct drm_device *dev, int crtc)
 {
@@ -961,6 +984,22 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
 EXPORT_SYMBOL(drm_vblank_put);
 
 /**
+ * drm_crtc_vblank_put - give up ownership of vblank events
+ * @dev: drm device
+ * @crtc: which counter to give up
+ *
+ * Release ownership of a given vblank counter, turning off interrupts
+ * if possible. Disable interrupts after drm_vblank_offdelay milliseconds.
+ *
+ * This is the native kms version of drm_vblank_put().
+ */
+void drm_crtc_vblank_put(struct drm_device *dev, struct drm_crtc *crtc)
+{
+	drm_vblank_put(dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_put);
+
+/**
  * drm_vblank_off - disable vblank events on a CRTC
  * @dev: drm device
  * @crtc: CRTC in question
@@ -971,6 +1010,8 @@ EXPORT_SYMBOL(drm_vblank_put);
  *
  * Drivers must use this function when the hardware vblank counter can get
  * reset, e.g. when suspending.
+ *
+ * This is the legacy version of drm_crtc_vblank_off().
  */
 void drm_vblank_off(struct drm_device *dev, int crtc)
 {
@@ -1004,6 +1045,26 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 EXPORT_SYMBOL(drm_vblank_off);
 
 /**
+ * drm_crtc_vblank_off - disable vblank events on a CRTC
+ * @dev: drm device
+ * @crtc: CRTC in question
+ *
+ * Drivers can use this function to shut down the vblank interrupt handling when
+ * disabling a crtc. This function ensures that the latest vblank frame count is
+ * stored so that drm_vblank_on can restore it again.
+ *
+ * Drivers must use this function when the hardware vblank counter can get
+ * reset, e.g. when suspending.
+ *
+ * This is the native kms version of drm_vblank_off().
+ */
+void drm_crtc_vblank_off(struct drm_device *dev, struct drm_crtc *crtc)
+{
+	drm_vblank_off(dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_off);
+
+/**
  * drm_vblank_on - enable vblank events on a CRTC
  * @dev: drm device
  * @crtc: CRTC in question
@@ -1012,6 +1073,8 @@ EXPORT_SYMBOL(drm_vblank_off);
  * drm_vblank_off() again. Note that calls to drm_vblank_on() and
  * drm_vblank_off() can be unbalanced and so can also be unconditionaly called
  * in driver load code to reflect the current hardware state of the crtc.
+ *
+ * This is the legacy version of drm_crtc_vblank_on().
  */
 void drm_vblank_on(struct drm_device *dev, int crtc)
 {
@@ -1026,6 +1089,24 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
 EXPORT_SYMBOL(drm_vblank_on);
 
 /**
+ * drm_crtc_vblank_on - enable vblank events on a CRTC
+ * @dev: drm device
+ * @crtc: CRTC in question
+ *
+ * This functions restores the vblank interrupt state captured with
+ * drm_vblank_off() again. Note that calls to drm_vblank_on() and
+ * drm_vblank_off() can be unbalanced and so can also be unconditionaly called
+ * in driver load code to reflect the current hardware state of the crtc.
+ *
+ * This is the native kms version of drm_vblank_on().
+ */
+void drm_crtc_vblank_on(struct drm_device *dev, struct drm_crtc *crtc)
+{
+	drm_vblank_on(dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_on);
+
+/**
  * drm_vblank_pre_modeset - account for vblanks across mode sets
  * @dev: drm device
  * @crtc: CRTC in question
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d0eff53a8ad1..d4abaa4bf2f4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3664,7 +3664,7 @@ static void ilk_crtc_disable_planes(struct drm_crtc *crtc)
 	int plane = intel_crtc->plane;
 
 	intel_crtc_wait_for_pending_flips(crtc);
-	drm_vblank_off(dev, pipe);
+	drm_crtc_vblank_off(dev, crtc);
 
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
@@ -3740,7 +3740,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	 */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-	drm_vblank_on(dev, pipe);
+	drm_crtc_vblank_on(dev, crtc);
 }
 
 /* IPS only exists on ULT machines and is tied to pipe A. */
@@ -3833,7 +3833,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	haswell_mode_set_planes_workaround(intel_crtc);
 	ilk_crtc_enable_planes(crtc);
 
-	drm_vblank_on(dev, pipe);
+	drm_crtc_vblank_on(dev, crtc);
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc)
@@ -4358,7 +4358,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 
-	drm_vblank_on(dev, pipe);
+	drm_crtc_vblank_on(dev, crtc);
 }
 
 static void i9xx_crtc_enable(struct drm_crtc *crtc)
@@ -4407,7 +4407,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 
-	drm_vblank_on(dev, pipe);
+	drm_crtc_vblank_on(dev, crtc);
 }
 
 static void i9xx_pfit_disable(struct intel_crtc *crtc)
@@ -4442,7 +4442,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	/* Give the overlay scaler a chance to disable if it's on this pipe */
 	intel_crtc_wait_for_pending_flips(crtc);
-	drm_vblank_off(dev, pipe);
+	drm_crtc_vblank_off(dev, crtc);
 
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
@@ -8526,7 +8526,7 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
 	if (work->event)
 		drm_send_vblank_event(dev, intel_crtc->pipe, work->event);
 
-	drm_vblank_put(dev, intel_crtc->pipe);
+	drm_crtc_vblank_put(dev, crtc);
 
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
@@ -8918,7 +8918,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
 	INIT_WORK(&work->work, intel_unpin_work_fn);
 
-	ret = drm_vblank_get(dev, intel_crtc->pipe);
+	ret = drm_crtc_vblank_get(dev, crtc);
 	if (ret)
 		goto free_work;
 
@@ -8927,7 +8927,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (intel_crtc->unpin_work) {
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 		kfree(work);
-		drm_vblank_put(dev, intel_crtc->pipe);
+		drm_crtc_vblank_put(dev, crtc);
 
 		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
 		return -EBUSY;
@@ -8979,7 +8979,7 @@ cleanup:
 	intel_crtc->unpin_work = NULL;
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
-	drm_vblank_put(dev, intel_crtc->pipe);
+	drm_crtc_vblank_put(dev, crtc);
 free_work:
 	kfree(work);
 
@@ -10579,6 +10579,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
+
+	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
 }
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 7339b2b00724..455c782422dd 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1359,9 +1359,14 @@ extern void drm_send_vblank_event(struct drm_device *dev, int crtc,
 extern bool drm_handle_vblank(struct drm_device *dev, int crtc);
 extern int drm_vblank_get(struct drm_device *dev, int crtc);
 extern void drm_vblank_put(struct drm_device *dev, int crtc);
+extern int drm_crtc_vblank_get(struct drm_device *dev, struct drm_crtc *crtc);
+extern void drm_crtc_vblank_put(struct drm_device *dev, struct drm_crtc *crtc);
 extern void drm_vblank_off(struct drm_device *dev, int crtc);
 extern void drm_vblank_on(struct drm_device *dev, int crtc);
+extern void drm_crtc_vblank_off(struct drm_device *dev, struct drm_crtc *crtc);
+extern void drm_crtc_vblank_on(struct drm_device *dev, struct drm_crtc *crtc);
 extern void drm_vblank_cleanup(struct drm_device *dev);
+
 extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
 				     struct timeval *tvblank, unsigned flags);
 extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
-- 
1.8.3.1

Re: [PATCH 08/12] drm/irq: Add kms-native crtc interface functions

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1850 bytes --]

On Wed, May 14, 2014 at 08:51:10PM +0200, Daniel Vetter wrote:
> We need to start somewhere ... With this the only places left in i915
> where we use pipe integers is in the interrupt handling code. And
> there it actually makes some amount of sense.

Very much welcome addition. Some minor comments below.

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c            | 81 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c | 22 +++++-----

Perhaps move the i915 changes into a separate commit?

> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
[...]
>  /**
> + * drm_crtc_vblank_get - get a reference count on vblank events
> + * @dev: drm device
> + * @crtc: which CRTC to own
> + *
> + * Acquire a reference count on vblank events to avoid having them disabled
> + * while in use.
> + *
> + * This is the native kms version of drm_vblank_off().
> + *
> + * Returns:
> + * Zero on success, nonzero on failure.
> + */
> +int drm_crtc_vblank_get(struct drm_device *dev, struct drm_crtc *crtc)
> +{
> +	return drm_vblank_get(dev, drm_crtc_index(crtc));
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_get);

This seems slightly backwards. Since drm_vblank_get() is what's being
deprecated here, wouldn't it make more sense to write
drm_crtc_vblank_get() in terms of struct drm_crtc and make
drm_vblank_get() call that instead? I can't seem to find a helper to get
the CRTC from an index, but it seems like that wouldn't be hard to do.

I guess it doesn't matter all that much either way, though, since we
could equally well make that change when drm_vblank_get() is dropped, in
which case at least there's no longer a need for the reverse lookup.

I'd still prefer to have i915 changes in a separate commit, but
otherwise:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 08/12] drm/irq: Add kms-native crtc interface functions

[Daniel Vetter]

On Thu, May 15, 2014 at 9:34 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> This seems slightly backwards. Since drm_vblank_get() is what's being
> deprecated here, wouldn't it make more sense to write
> drm_crtc_vblank_get() in terms of struct drm_crtc and make
> drm_vblank_get() call that instead? I can't seem to find a helper to get
> the CRTC from an index, but it seems like that wouldn't be hard to do.

Two reasons against this:
- More ugly churn since it's a flag day, and when handling vblank
refactorings what I _definitely_ want to avoid is whole-subsystem wide
flag days.
- drm_crtc_ is the common prefix used by many of the crtc functions.

What I actually forgotten to do is drop the dev parameter, we can fish
that out of the crtc. Then it should be even more obvious that this is
a crtc function and rightfully deserve the drm_crtc_ prefix ;-)

> I guess it doesn't matter all that much either way, though, since we
> could equally well make that change when drm_vblank_get() is dropped, in
> which case at least there's no longer a need for the reverse lookup.

Yeah, the reverse lookup is something I want to add later on
eventually. But that requires more thought since it only makes sense
if we also switch the driver callbacks for vblank_enable/disable over.

> I'd still prefer to have i915 changes in a separate commit, but
> otherwise:

Will do, makes indeed more sense.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 08/12] drm/irq: Add kms-native crtc interface functions

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2202 bytes --]

On Thu, May 15, 2014 at 12:10:16PM +0200, Daniel Vetter wrote:
> On Thu, May 15, 2014 at 9:34 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > This seems slightly backwards. Since drm_vblank_get() is what's being
> > deprecated here, wouldn't it make more sense to write
> > drm_crtc_vblank_get() in terms of struct drm_crtc and make
> > drm_vblank_get() call that instead? I can't seem to find a helper to get
> > the CRTC from an index, but it seems like that wouldn't be hard to do.
> 
> Two reasons against this:
> - More ugly churn since it's a flag day, and when handling vblank
> refactorings what I _definitely_ want to avoid is whole-subsystem wide
> flag days.
> - drm_crtc_ is the common prefix used by many of the crtc functions.
> 
> What I actually forgotten to do is drop the dev parameter, we can fish
> that out of the crtc. Then it should be even more obvious that this is
> a crtc function and rightfully deserve the drm_crtc_ prefix ;-)

I think you misunderstood what I was saying. What I proposed wasn't that
drm_vblank_get() was replaced by a new implementation with different
signature. Rather my suggestion was to implement the old
drm_vblank_get() by calling drm_crtc_vblank_get() rather than the other
way around.

Something like this:

	int drm_crtc_vblank_get(struct drm_crtc *crtc)
	{
		new code using CRTC
	}

	int drm_vblank_get(struct drm_device *drm, int crtc)
	{
		struct drm_crtc *c = drm_crtc_from_index(crtc);

		return drm_crtc_vblank_get(c);
	}

> > I guess it doesn't matter all that much either way, though, since we
> > could equally well make that change when drm_vblank_get() is dropped, in
> > which case at least there's no longer a need for the reverse lookup.
> 
> Yeah, the reverse lookup is something I want to add later on
> eventually. But that requires more thought since it only makes sense
> if we also switch the driver callbacks for vblank_enable/disable over.

On that note, is there a plan to move the vblank fields out of the DRM
device and into drm_crtc as well? That seems like a logical next step
since presumably every CRTC can handle it's own vblank events itself.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 08/12] drm/irq: Add kms-native crtc interface functions

[Daniel Vetter]

On Thu, May 15, 2014 at 12:42 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Thu, May 15, 2014 at 12:10:16PM +0200, Daniel Vetter wrote:
>> On Thu, May 15, 2014 at 9:34 AM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > This seems slightly backwards. Since drm_vblank_get() is what's being
>> > deprecated here, wouldn't it make more sense to write
>> > drm_crtc_vblank_get() in terms of struct drm_crtc and make
>> > drm_vblank_get() call that instead? I can't seem to find a helper to get
>> > the CRTC from an index, but it seems like that wouldn't be hard to do.
>>
>> Two reasons against this:
>> - More ugly churn since it's a flag day, and when handling vblank
>> refactorings what I _definitely_ want to avoid is whole-subsystem wide
>> flag days.
>> - drm_crtc_ is the common prefix used by many of the crtc functions.
>>
>> What I actually forgotten to do is drop the dev parameter, we can fish
>> that out of the crtc. Then it should be even more obvious that this is
>> a crtc function and rightfully deserve the drm_crtc_ prefix ;-)
>
> I think you misunderstood what I was saying. What I proposed wasn't that
> drm_vblank_get() was replaced by a new implementation with different
> signature. Rather my suggestion was to implement the old
> drm_vblank_get() by calling drm_crtc_vblank_get() rather than the other
> way around.
>
> Something like this:
>
>         int drm_crtc_vblank_get(struct drm_crtc *crtc)
>         {
>                 new code using CRTC
>         }
>
>         int drm_vblank_get(struct drm_device *drm, int crtc)
>         {
>                 struct drm_crtc *c = drm_crtc_from_index(crtc);
>
>                 return drm_crtc_vblank_get(c);
>         }

As long as the actual code doesn't deal in real drm_crtcs that imo
makes little sense. It's really just the interface shim to start the
long journey into a saner world ;-)

>> > I guess it doesn't matter all that much either way, though, since we
>> > could equally well make that change when drm_vblank_get() is dropped, in
>> > which case at least there's no longer a need for the reverse lookup.
>>
>> Yeah, the reverse lookup is something I want to add later on
>> eventually. But that requires more thought since it only makes sense
>> if we also switch the driver callbacks for vblank_enable/disable over.
>
> On that note, is there a plan to move the vblank fields out of the DRM
> device and into drm_crtc as well? That seems like a logical next step
> since presumably every CRTC can handle it's own vblank events itself.

Yeah, I think that's where we eventually want to go to. The problem is
that the vblank code is deeply intertwined with legacy
user-mode-setting drivers. We might need to do a copy-paste of
drm_irq.c for kms drivers into a new drm_crtc_vblank.c file which
exclusively deals with drm_crtcs. But I don't have any clear idea yet
how to make that transition happen, hence this patch to start with
something small and something we clearly want.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/2] drm/irq: Add kms-native crtc interface functions

[Daniel Vetter]

We need to start somewhere ... With this the only places left in i915
where we use pipe integers is in the interrupt handling code. And
there it actually makes some amount of sense.

v2:
- Polish kerneldoc a bit (Thierry).
- Drop "dev" parameter since it's unecessary.
- Split out i915 changes (Thierry).

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h        |  5 +++
 2 files changed, 82 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index f3dafccca456..5ba9a614e7ad 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -916,6 +916,8 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc)
  * Acquire a reference count on vblank events to avoid having them disabled
  * while in use.
  *
+ * This is the legacy version of drm_crtc_vblank_get().
+ *
  * Returns:
  * Zero on success, nonzero on failure.
  */
@@ -941,12 +943,32 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
 EXPORT_SYMBOL(drm_vblank_get);
 
 /**
+ * drm_crtc_vblank_get - get a reference count on vblank events
+ * @crtc: which CRTC to own
+ *
+ * Acquire a reference count on vblank events to avoid having them disabled
+ * while in use.
+ *
+ * This is the native kms version of drm_vblank_off().
+ *
+ * Returns:
+ * Zero on success, nonzero on failure.
+ */
+int drm_crtc_vblank_get(struct drm_crtc *crtc)
+{
+	return drm_vblank_get(crtc->dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_get);
+
+/**
  * drm_vblank_put - give up ownership of vblank events
  * @dev: DRM device
  * @crtc: which counter to give up
  *
  * Release ownership of a given vblank counter, turning off interrupts
  * if possible. Disable interrupts after drm_vblank_offdelay milliseconds.
+ *
+ * This is the legacy version of drm_crtc_vblank_put().
  */
 void drm_vblank_put(struct drm_device *dev, int crtc)
 {
@@ -961,6 +983,21 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
 EXPORT_SYMBOL(drm_vblank_put);
 
 /**
+ * drm_crtc_vblank_put - give up ownership of vblank events
+ * @crtc: which counter to give up
+ *
+ * Release ownership of a given vblank counter, turning off interrupts
+ * if possible. Disable interrupts after drm_vblank_offdelay milliseconds.
+ *
+ * This is the native kms version of drm_vblank_put().
+ */
+void drm_crtc_vblank_put(struct drm_crtc *crtc)
+{
+	drm_vblank_put(crtc->dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_put);
+
+/**
  * drm_vblank_off - disable vblank events on a CRTC
  * @dev: DRM device
  * @crtc: CRTC in question
@@ -971,6 +1008,8 @@ EXPORT_SYMBOL(drm_vblank_put);
  *
  * Drivers must use this function when the hardware vblank counter can get
  * reset, e.g. when suspending.
+ *
+ * This is the legacy version of drm_crtc_vblank_off().
  */
 void drm_vblank_off(struct drm_device *dev, int crtc)
 {
@@ -1004,6 +1043,25 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 EXPORT_SYMBOL(drm_vblank_off);
 
 /**
+ * drm_crtc_vblank_off - disable vblank events on a CRTC
+ * @crtc: CRTC in question
+ *
+ * Drivers can use this function to shut down the vblank interrupt handling when
+ * disabling a crtc. This function ensures that the latest vblank frame count is
+ * stored so that drm_vblank_on can restore it again.
+ *
+ * Drivers must use this function when the hardware vblank counter can get
+ * reset, e.g. when suspending.
+ *
+ * This is the native kms version of drm_vblank_off().
+ */
+void drm_crtc_vblank_off(struct drm_crtc *crtc)
+{
+	drm_vblank_off(crtc->dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_off);
+
+/**
  * drm_vblank_on - enable vblank events on a CRTC
  * @dev: DRM device
  * @crtc: CRTC in question
@@ -1012,6 +1070,8 @@ EXPORT_SYMBOL(drm_vblank_off);
  * drm_vblank_off() again. Note that calls to drm_vblank_on() and
  * drm_vblank_off() can be unbalanced and so can also be unconditionaly called
  * in driver load code to reflect the current hardware state of the crtc.
+ *
+ * This is the legacy version of drm_crtc_vblank_on().
  */
 void drm_vblank_on(struct drm_device *dev, int crtc)
 {
@@ -1026,6 +1086,23 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
 EXPORT_SYMBOL(drm_vblank_on);
 
 /**
+ * drm_crtc_vblank_on - enable vblank events on a CRTC
+ * @crtc: CRTC in question
+ *
+ * This functions restores the vblank interrupt state captured with
+ * drm_vblank_off() again. Note that calls to drm_vblank_on() and
+ * drm_vblank_off() can be unbalanced and so can also be unconditionaly called
+ * in driver load code to reflect the current hardware state of the crtc.
+ *
+ * This is the native kms version of drm_vblank_on().
+ */
+void drm_crtc_vblank_on(struct drm_crtc *crtc)
+{
+	drm_vblank_on(crtc->dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_on);
+
+/**
  * drm_vblank_pre_modeset - account for vblanks across mode sets
  * @dev: DRM device
  * @crtc: CRTC in question
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 7339b2b00724..76ccaabd0418 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1359,9 +1359,14 @@ extern void drm_send_vblank_event(struct drm_device *dev, int crtc,
 extern bool drm_handle_vblank(struct drm_device *dev, int crtc);
 extern int drm_vblank_get(struct drm_device *dev, int crtc);
 extern void drm_vblank_put(struct drm_device *dev, int crtc);
+extern int drm_crtc_vblank_get(struct drm_crtc *crtc);
+extern void drm_crtc_vblank_put(struct drm_crtc *crtc);
 extern void drm_vblank_off(struct drm_device *dev, int crtc);
 extern void drm_vblank_on(struct drm_device *dev, int crtc);
+extern void drm_crtc_vblank_off(struct drm_crtc *crtc);
+extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
 extern void drm_vblank_cleanup(struct drm_device *dev);
+
 extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
 				     struct timeval *tvblank, unsigned flags);
 extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
-- 
1.8.3.1

[PATCH 2/2] drm/i915: Use new kms-native vblank functions

[Daniel Vetter]

Only the low-level irq handling functions still use integer crtc
indices with this. But fixing that will require a lot more sugery
and some good ideas for backwards compat with old ums userspace.
Both in drivers and in the drm core.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d0eff53a8ad1..48c8c170f08f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3664,7 +3664,7 @@ static void ilk_crtc_disable_planes(struct drm_crtc *crtc)
 	int plane = intel_crtc->plane;
 
 	intel_crtc_wait_for_pending_flips(crtc);
-	drm_vblank_off(dev, pipe);
+	drm_crtc_vblank_off(crtc);
 
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
@@ -3740,7 +3740,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	 */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-	drm_vblank_on(dev, pipe);
+	drm_crtc_vblank_on(crtc);
 }
 
 /* IPS only exists on ULT machines and is tied to pipe A. */
@@ -3833,7 +3833,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	haswell_mode_set_planes_workaround(intel_crtc);
 	ilk_crtc_enable_planes(crtc);
 
-	drm_vblank_on(dev, pipe);
+	drm_crtc_vblank_on(crtc);
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc)
@@ -4358,7 +4358,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 
-	drm_vblank_on(dev, pipe);
+	drm_crtc_vblank_on(crtc);
 }
 
 static void i9xx_crtc_enable(struct drm_crtc *crtc)
@@ -4407,7 +4407,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 
-	drm_vblank_on(dev, pipe);
+	drm_crtc_vblank_on(crtc);
 }
 
 static void i9xx_pfit_disable(struct intel_crtc *crtc)
@@ -4442,7 +4442,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	/* Give the overlay scaler a chance to disable if it's on this pipe */
 	intel_crtc_wait_for_pending_flips(crtc);
-	drm_vblank_off(dev, pipe);
+	drm_crtc_vblank_off(crtc);
 
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
@@ -8526,7 +8526,7 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
 	if (work->event)
 		drm_send_vblank_event(dev, intel_crtc->pipe, work->event);
 
-	drm_vblank_put(dev, intel_crtc->pipe);
+	drm_crtc_vblank_put(crtc);
 
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
@@ -8918,7 +8918,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
 	INIT_WORK(&work->work, intel_unpin_work_fn);
 
-	ret = drm_vblank_get(dev, intel_crtc->pipe);
+	ret = drm_crtc_vblank_get(crtc);
 	if (ret)
 		goto free_work;
 
@@ -8927,7 +8927,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (intel_crtc->unpin_work) {
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 		kfree(work);
-		drm_vblank_put(dev, intel_crtc->pipe);
+		drm_crtc_vblank_put(crtc);
 
 		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
 		return -EBUSY;
@@ -8979,7 +8979,7 @@ cleanup:
 	intel_crtc->unpin_work = NULL;
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
-	drm_vblank_put(dev, intel_crtc->pipe);
+	drm_crtc_vblank_put(crtc);
 free_work:
 	kfree(work);
 
@@ -10579,6 +10579,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
+
+	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
 }
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
-- 
1.8.3.1

[PATCH 09/12] drm/i915: rip our vblank reset hacks for runtime PM

[Daniel Vetter]

Now that we unconditionally dtrt when disabling/enabling crtcs we
don't need any hacks any longer to keep the vblank logic sane when
all the registers go poof. So let's rip it all out.

This essentially undoes

commit 9dbd8febb4dbc9199fcf340b882eb930e36b65b6
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Tue Jul 23 10:48:11 2013 -0300

    drm/i915: update last_vblank when disabling the power well

Apparently igt/kms_flip is already powerful enough to exercise this
properly, yay! See the reference regression report for details.

References: https://bugs.freedesktop.org/show_bug.cgi?id=66808
Testcase: igt/kms_flip/*-vs-rpm
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c | 34 ----------------------------------
 1 file changed, 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 75c1c766b507..45fa43f16bb3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5423,33 +5423,6 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
 	}
 }
 
-static void reset_vblank_counter(struct drm_device *dev, enum pipe pipe)
-{
-	assert_spin_locked(&dev->vbl_lock);
-
-	dev->vblank[pipe].last = 0;
-}
-
-static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	enum pipe pipe;
-	unsigned long irqflags;
-
-	/*
-	 * After this, the registers on the pipes that are part of the power
-	 * well will become zero, so we have to adjust our counters according to
-	 * that.
-	 *
-	 * FIXME: Should we do this in general in drm_vblank_post_modeset?
-	 */
-	spin_lock_irqsave(&dev->vbl_lock, irqflags);
-	for_each_pipe(pipe)
-		if (pipe != PIPE_A)
-			reset_vblank_counter(dev, pipe);
-	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
-}
-
 static void hsw_set_power_well(struct drm_i915_private *dev_priv,
 			       struct i915_power_well *power_well, bool enable)
 {
@@ -5478,8 +5451,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
 			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
 			POSTING_READ(HSW_PWR_WELL_DRIVER);
 			DRM_DEBUG_KMS("Requesting to disable the power well\n");
-
-			hsw_power_well_post_disable(dev_priv);
 		}
 	}
 }
@@ -5646,11 +5617,6 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
 	valleyview_disable_display_irqs(dev_priv);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	spin_lock_irq(&dev->vbl_lock);
-	for_each_pipe(pipe)
-		reset_vblank_counter(dev, pipe);
-	spin_unlock_irq(&dev->vbl_lock);
-
 	vlv_set_power_well(dev_priv, power_well, false);
 }
 
-- 
1.8.3.1

Re: [Intel-gfx] [PATCH 09/12] drm/i915: rip our vblank reset hacks for runtime PM

[Ville Syrjälä]

On Wed, May 14, 2014 at 08:51:11PM +0200, Daniel Vetter wrote:
> Now that we unconditionally dtrt when disabling/enabling crtcs we
> don't need any hacks any longer to keep the vblank logic sane when
> all the registers go poof. So let's rip it all out.

Hmm. drm_update_vblank_count() will now see some kind of diff between
the last and current value when the registers got cloberred. So the
vblank counter reported to userspace will jump. But I guess that's fine
as long as userspace realizes that the counter is not at all reliable
across modesets.

> 
> This essentially undoes
> 
> commit 9dbd8febb4dbc9199fcf340b882eb930e36b65b6
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Tue Jul 23 10:48:11 2013 -0300
> 
>     drm/i915: update last_vblank when disabling the power well
> 
> Apparently igt/kms_flip is already powerful enough to exercise this
> properly, yay! See the reference regression report for details.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=66808
> Testcase: igt/kms_flip/*-vs-rpm
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 34 ----------------------------------
>  1 file changed, 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 75c1c766b507..45fa43f16bb3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5423,33 +5423,6 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
>  	}
>  }
>  
> -static void reset_vblank_counter(struct drm_device *dev, enum pipe pipe)
> -{
> -	assert_spin_locked(&dev->vbl_lock);
> -
> -	dev->vblank[pipe].last = 0;
> -}
> -
> -static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv)
> -{
> -	struct drm_device *dev = dev_priv->dev;
> -	enum pipe pipe;
> -	unsigned long irqflags;
> -
> -	/*
> -	 * After this, the registers on the pipes that are part of the power
> -	 * well will become zero, so we have to adjust our counters according to
> -	 * that.
> -	 *
> -	 * FIXME: Should we do this in general in drm_vblank_post_modeset?
> -	 */
> -	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> -	for_each_pipe(pipe)
> -		if (pipe != PIPE_A)
> -			reset_vblank_counter(dev, pipe);
> -	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> -}
> -
>  static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>  			       struct i915_power_well *power_well, bool enable)
>  {
> @@ -5478,8 +5451,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>  			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
>  			POSTING_READ(HSW_PWR_WELL_DRIVER);
>  			DRM_DEBUG_KMS("Requesting to disable the power well\n");
> -
> -			hsw_power_well_post_disable(dev_priv);
>  		}
>  	}
>  }
> @@ -5646,11 +5617,6 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
>  	valleyview_disable_display_irqs(dev_priv);
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  
> -	spin_lock_irq(&dev->vbl_lock);
> -	for_each_pipe(pipe)
> -		reset_vblank_counter(dev, pipe);
> -	spin_unlock_irq(&dev->vbl_lock);
> -
>  	vlv_set_power_well(dev_priv, power_well, false);
>  }
>  
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 09/12] drm/i915: rip our vblank reset hacks for runtime PM

[Daniel Vetter]

On Tue, May 20, 2014 at 03:03:41PM +0300, Ville Syrjälä wrote:
> On Wed, May 14, 2014 at 08:51:11PM +0200, Daniel Vetter wrote:
> > Now that we unconditionally dtrt when disabling/enabling crtcs we
> > don't need any hacks any longer to keep the vblank logic sane when
> > all the registers go poof. So let's rip it all out.
> 
> Hmm. drm_update_vblank_count() will now see some kind of diff between
> the last and current value when the registers got cloberred. So the
> vblank counter reported to userspace will jump. But I guess that's fine
> as long as userspace realizes that the counter is not at all reliable
> across modesets.

I've added checks for this (the rpm varianst) and for the similiar
suspend/resume issues (the suspend variants) to kms_flip. It seems to work
and we don't actually jump to far. But maybe the tests are horribly
broken.

Can you please take a closer look? I've thought that the entire point of
this series (well, one of them) was to finally fix this gag and avoid
handing totally bogus frame counter values to userspace. Especially for
system suspend/resume where userspace might get susprised ...
-Daniel

> 
> > 
> > This essentially undoes
> > 
> > commit 9dbd8febb4dbc9199fcf340b882eb930e36b65b6
> > Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Date:   Tue Jul 23 10:48:11 2013 -0300
> > 
> >     drm/i915: update last_vblank when disabling the power well
> > 
> > Apparently igt/kms_flip is already powerful enough to exercise this
> > properly, yay! See the reference regression report for details.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=66808
> > Testcase: igt/kms_flip/*-vs-rpm
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 34 ----------------------------------
> >  1 file changed, 34 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 75c1c766b507..45fa43f16bb3 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5423,33 +5423,6 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
> >  	}
> >  }
> >  
> > -static void reset_vblank_counter(struct drm_device *dev, enum pipe pipe)
> > -{
> > -	assert_spin_locked(&dev->vbl_lock);
> > -
> > -	dev->vblank[pipe].last = 0;
> > -}
> > -
> > -static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv)
> > -{
> > -	struct drm_device *dev = dev_priv->dev;
> > -	enum pipe pipe;
> > -	unsigned long irqflags;
> > -
> > -	/*
> > -	 * After this, the registers on the pipes that are part of the power
> > -	 * well will become zero, so we have to adjust our counters according to
> > -	 * that.
> > -	 *
> > -	 * FIXME: Should we do this in general in drm_vblank_post_modeset?
> > -	 */
> > -	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > -	for_each_pipe(pipe)
> > -		if (pipe != PIPE_A)
> > -			reset_vblank_counter(dev, pipe);
> > -	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > -}
> > -
> >  static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> >  			       struct i915_power_well *power_well, bool enable)
> >  {
> > @@ -5478,8 +5451,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> >  			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> >  			DRM_DEBUG_KMS("Requesting to disable the power well\n");
> > -
> > -			hsw_power_well_post_disable(dev_priv);
> >  		}
> >  	}
> >  }
> > @@ -5646,11 +5617,6 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
> >  	valleyview_disable_display_irqs(dev_priv);
> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >  
> > -	spin_lock_irq(&dev->vbl_lock);
> > -	for_each_pipe(pipe)
> > -		reset_vblank_counter(dev, pipe);
> > -	spin_unlock_irq(&dev->vbl_lock);
> > -
> >  	vlv_set_power_well(dev_priv, power_well, false);
> >  }
> >  
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 09/12] drm/i915: rip our vblank reset hacks for runtime PM

[Ville Syrjälä]

On Tue, May 20, 2014 at 03:38:14PM +0200, Daniel Vetter wrote:
> On Tue, May 20, 2014 at 03:03:41PM +0300, Ville Syrjälä wrote:
> > On Wed, May 14, 2014 at 08:51:11PM +0200, Daniel Vetter wrote:
> > > Now that we unconditionally dtrt when disabling/enabling crtcs we
> > > don't need any hacks any longer to keep the vblank logic sane when
> > > all the registers go poof. So let's rip it all out.
> > 
> > Hmm. drm_update_vblank_count() will now see some kind of diff between
> > the last and current value when the registers got cloberred. So the
> > vblank counter reported to userspace will jump. But I guess that's fine
> > as long as userspace realizes that the counter is not at all reliable
> > across modesets.
> 
> I've added checks for this (the rpm varianst) and for the similiar
> suspend/resume issues (the suspend variants) to kms_flip. It seems to work
> and we don't actually jump to far. But maybe the tests are horribly
> broken.

Hmm. If we can force the power well off at the start of the test and then
set a mode, I'd expect the vblank counter to jump by almost max_vblank_count
since the hw counter would appear to wrap.

> 
> Can you please take a closer look? I've thought that the entire point of
> this series (well, one of them) was to finally fix this gag and avoid
> handing totally bogus frame counter values to userspace. Especially for
> system suspend/resume where userspace might get susprised ...

I was mostly interested in making vblank interrupts work during plane
enable/disable. Anything else is a bonus.

> -Daniel
> 
> > 
> > > 
> > > This essentially undoes
> > > 
> > > commit 9dbd8febb4dbc9199fcf340b882eb930e36b65b6
> > > Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Date:   Tue Jul 23 10:48:11 2013 -0300
> > > 
> > >     drm/i915: update last_vblank when disabling the power well
> > > 
> > > Apparently igt/kms_flip is already powerful enough to exercise this
> > > properly, yay! See the reference regression report for details.
> > > 
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=66808
> > > Testcase: igt/kms_flip/*-vs-rpm
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 34 ----------------------------------
> > >  1 file changed, 34 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 75c1c766b507..45fa43f16bb3 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5423,33 +5423,6 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
> > >  	}
> > >  }
> > >  
> > > -static void reset_vblank_counter(struct drm_device *dev, enum pipe pipe)
> > > -{
> > > -	assert_spin_locked(&dev->vbl_lock);
> > > -
> > > -	dev->vblank[pipe].last = 0;
> > > -}
> > > -
> > > -static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv)
> > > -{
> > > -	struct drm_device *dev = dev_priv->dev;
> > > -	enum pipe pipe;
> > > -	unsigned long irqflags;
> > > -
> > > -	/*
> > > -	 * After this, the registers on the pipes that are part of the power
> > > -	 * well will become zero, so we have to adjust our counters according to
> > > -	 * that.
> > > -	 *
> > > -	 * FIXME: Should we do this in general in drm_vblank_post_modeset?
> > > -	 */
> > > -	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > > -	for_each_pipe(pipe)
> > > -		if (pipe != PIPE_A)
> > > -			reset_vblank_counter(dev, pipe);
> > > -	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > > -}
> > > -
> > >  static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > >  			       struct i915_power_well *power_well, bool enable)
> > >  {
> > > @@ -5478,8 +5451,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > >  			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> > >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> > >  			DRM_DEBUG_KMS("Requesting to disable the power well\n");
> > > -
> > > -			hsw_power_well_post_disable(dev_priv);
> > >  		}
> > >  	}
> > >  }
> > > @@ -5646,11 +5617,6 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
> > >  	valleyview_disable_display_irqs(dev_priv);
> > >  	spin_unlock_irq(&dev_priv->irq_lock);
> > >  
> > > -	spin_lock_irq(&dev->vbl_lock);
> > > -	for_each_pipe(pipe)
> > > -		reset_vblank_counter(dev, pipe);
> > > -	spin_unlock_irq(&dev->vbl_lock);
> > > -
> > >  	vlv_set_power_well(dev_priv, power_well, false);
> > >  }
> > >  
> > > -- 
> > > 1.8.3.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 09/12] drm/i915: rip our vblank reset hacks for runtime PM

[Daniel Vetter]

On Tue, May 20, 2014 at 05:03:33PM +0300, Ville Syrjälä wrote:
> On Tue, May 20, 2014 at 03:38:14PM +0200, Daniel Vetter wrote:
> > On Tue, May 20, 2014 at 03:03:41PM +0300, Ville Syrjälä wrote:
> > > On Wed, May 14, 2014 at 08:51:11PM +0200, Daniel Vetter wrote:
> > > > Now that we unconditionally dtrt when disabling/enabling crtcs we
> > > > don't need any hacks any longer to keep the vblank logic sane when
> > > > all the registers go poof. So let's rip it all out.
> > > 
> > > Hmm. drm_update_vblank_count() will now see some kind of diff between
> > > the last and current value when the registers got cloberred. So the
> > > vblank counter reported to userspace will jump. But I guess that's fine
> > > as long as userspace realizes that the counter is not at all reliable
> > > across modesets.
> > 
> > I've added checks for this (the rpm varianst) and for the similiar
> > suspend/resume issues (the suspend variants) to kms_flip. It seems to work
> > and we don't actually jump to far. But maybe the tests are horribly
> > broken.
> 
> Hmm. If we can force the power well off at the start of the test and then
> set a mode, I'd expect the vblank counter to jump by almost max_vblank_count
> since the hw counter would appear to wrap.

Oh, I think the tests are busted ... Need to look at this again.
-Daniel

> 
> > 
> > Can you please take a closer look? I've thought that the entire point of
> > this series (well, one of them) was to finally fix this gag and avoid
> > handing totally bogus frame counter values to userspace. Especially for
> > system suspend/resume where userspace might get susprised ...
> 
> I was mostly interested in making vblank interrupts work during plane
> enable/disable. Anything else is a bonus.
> 
> > -Daniel
> > 
> > > 
> > > > 
> > > > This essentially undoes
> > > > 
> > > > commit 9dbd8febb4dbc9199fcf340b882eb930e36b65b6
> > > > Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Date:   Tue Jul 23 10:48:11 2013 -0300
> > > > 
> > > >     drm/i915: update last_vblank when disabling the power well
> > > > 
> > > > Apparently igt/kms_flip is already powerful enough to exercise this
> > > > properly, yay! See the reference regression report for details.
> > > > 
> > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=66808
> > > > Testcase: igt/kms_flip/*-vs-rpm
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c | 34 ----------------------------------
> > > >  1 file changed, 34 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 75c1c766b507..45fa43f16bb3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -5423,33 +5423,6 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
> > > >  	}
> > > >  }
> > > >  
> > > > -static void reset_vblank_counter(struct drm_device *dev, enum pipe pipe)
> > > > -{
> > > > -	assert_spin_locked(&dev->vbl_lock);
> > > > -
> > > > -	dev->vblank[pipe].last = 0;
> > > > -}
> > > > -
> > > > -static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv)
> > > > -{
> > > > -	struct drm_device *dev = dev_priv->dev;
> > > > -	enum pipe pipe;
> > > > -	unsigned long irqflags;
> > > > -
> > > > -	/*
> > > > -	 * After this, the registers on the pipes that are part of the power
> > > > -	 * well will become zero, so we have to adjust our counters according to
> > > > -	 * that.
> > > > -	 *
> > > > -	 * FIXME: Should we do this in general in drm_vblank_post_modeset?
> > > > -	 */
> > > > -	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > > > -	for_each_pipe(pipe)
> > > > -		if (pipe != PIPE_A)
> > > > -			reset_vblank_counter(dev, pipe);
> > > > -	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > > > -}
> > > > -
> > > >  static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > > >  			       struct i915_power_well *power_well, bool enable)
> > > >  {
> > > > @@ -5478,8 +5451,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > > >  			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> > > >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> > > >  			DRM_DEBUG_KMS("Requesting to disable the power well\n");
> > > > -
> > > > -			hsw_power_well_post_disable(dev_priv);
> > > >  		}
> > > >  	}
> > > >  }
> > > > @@ -5646,11 +5617,6 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
> > > >  	valleyview_disable_display_irqs(dev_priv);
> > > >  	spin_unlock_irq(&dev_priv->irq_lock);
> > > >  
> > > > -	spin_lock_irq(&dev->vbl_lock);
> > > > -	for_each_pipe(pipe)
> > > > -		reset_vblank_counter(dev, pipe);
> > > > -	spin_unlock_irq(&dev->vbl_lock);
> > > > -
> > > >  	vlv_set_power_well(dev_priv, power_well, false);
> > > >  }
> > > >  
> > > > -- 
> > > > 1.8.3.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 09/12] drm/i915: rip our vblank reset hacks for runtime PM

[Daniel Vetter]

On Tue, May 20, 2014 at 4:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, May 20, 2014 at 05:03:33PM +0300, Ville Syrjälä wrote:
>> On Tue, May 20, 2014 at 03:38:14PM +0200, Daniel Vetter wrote:
>> > On Tue, May 20, 2014 at 03:03:41PM +0300, Ville Syrjälä wrote:
>> > > On Wed, May 14, 2014 at 08:51:11PM +0200, Daniel Vetter wrote:
>> > > > Now that we unconditionally dtrt when disabling/enabling crtcs we
>> > > > don't need any hacks any longer to keep the vblank logic sane when
>> > > > all the registers go poof. So let's rip it all out.
>> > >
>> > > Hmm. drm_update_vblank_count() will now see some kind of diff between
>> > > the last and current value when the registers got cloberred. So the
>> > > vblank counter reported to userspace will jump. But I guess that's fine
>> > > as long as userspace realizes that the counter is not at all reliable
>> > > across modesets.
>> >
>> > I've added checks for this (the rpm varianst) and for the similiar
>> > suspend/resume issues (the suspend variants) to kms_flip. It seems to work
>> > and we don't actually jump to far. But maybe the tests are horribly
>> > broken.
>>
>> Hmm. If we can force the power well off at the start of the test and then
>> set a mode, I'd expect the vblank counter to jump by almost max_vblank_count
>> since the hw counter would appear to wrap.
>
> Oh, I think the tests are busted ... Need to look at this again.

I've added some debug output and fixed the suspend variants of the
tests and it actually seems to work. I.e. the frame counter before and
after a runtime pm or suspend/resume cycle is monotonic and only
increases by a few frames. The limit the test uses is 100 frames,
which should be tight enough to not confuse userspace.

Of course userspace still needs to be able to cope with cancelled
vblank events, but that's just how it is. At least the frame counters
look sane now. So I think we're good.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] drm/i915: rip our vblank reset hacks for runtime PM

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 914 bytes --]

On Tue, May 20, 2014 at 03:03:41PM +0300, Ville Syrjälä wrote:
> On Wed, May 14, 2014 at 08:51:11PM +0200, Daniel Vetter wrote:
> > Now that we unconditionally dtrt when disabling/enabling crtcs we
> > don't need any hacks any longer to keep the vblank logic sane when
> > all the registers go poof. So let's rip it all out.
> 
> Hmm. drm_update_vblank_count() will now see some kind of diff between
> the last and current value when the registers got cloberred. So the
> vblank counter reported to userspace will jump. But I guess that's fine
> as long as userspace realizes that the counter is not at all reliable
> across modesets.

I think that's a fair assumption. Modeset is kind of a heavy operation
and I wouldn't expect applications to perform one all that often. That
should be even more true of applications that rely on the counters. At
least that would be my expectation.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 09/12] drm/irq: Lack of interrupt support in drm_vblank_on|off

[Daniel Vetter]

If we want to use this functionality in generic helpers to make
sure that all drivers have somewhat sane vblank handling across
modesets/dpms, we need to make it work for all drivers. But some
don't support interrupts and hence also not vblank waits.

Just return early on such drivers.

Note that with pageflips drivers are free to implement them however
they wish to.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 51ebe9086be9..03fba43ab6be 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1020,6 +1020,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 	unsigned long irqflags;
 	unsigned int seq;
 
+	if (!dev->irq_enabled)
+		return;
+
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	vblank_disable_and_save(dev, crtc);
 	wake_up(&dev->vblank[crtc].queue);
@@ -1080,6 +1083,9 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
 {
 	unsigned long irqflags;
 
+	if (!dev->irq_enabled)
+		return;
+
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	/* re-enable interrupts if there's are users left */
 	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
-- 
1.8.3.1

Re: [PATCH 09/12] drm/irq: Lack of interrupt support in drm_vblank_on|off

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 658 bytes --]

On Wed, May 14, 2014 at 08:51:12PM +0200, Daniel Vetter wrote:
> If we want to use this functionality in generic helpers to make
> sure that all drivers have somewhat sane vblank handling across
> modesets/dpms, we need to make it work for all drivers. But some
> don't support interrupts and hence also not vblank waits.
> 
> Just return early on such drivers.
> 
> Note that with pageflips drivers are free to implement them however
> they wish to.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 10/12] drm/i915: Accurately initialize fifo underrun state on gmch platforms

[Daniel Vetter]

We don't have hardware based disable bits on gmch platforms, so need
to block spurious underrun reports in software. Which means that we
_must_ start out with fifo underrun reporting disabled everywhere.

This is in big contrast to ilk/hsw/cpt where there's only _one_
disable bit for all platforms and hence we must allow underrun
reporting on disabled pipes. Otherwise nothing really works,
especially the CRC support since that's key'ed off the same irq
disable bit.

This allows us to ditch the fifo underrun reporting hack from the vlv
runtime pm code and unexport the internal function from i915_irq.c
again. Yay!

v2: Keep the display irq disabling, spotted by Imre.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c      | 4 ++--
 drivers/gpu/drm/i915/intel_display.c | 9 ++++++++-
 drivers/gpu/drm/i915/intel_drv.h     | 2 --
 drivers/gpu/drm/i915/intel_pm.c      | 6 ------
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index afa55199b829..a502faae0d0b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -415,8 +415,8 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
  *
  * Returns the previous state of underrun reporting.
  */
-bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
-					     enum pipe pipe, bool enable)
+static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
+						    enum pipe pipe, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d4abaa4bf2f4..e78003ac71a0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11510,11 +11510,18 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 			encoder->base.crtc = NULL;
 		}
 	}
-	if (crtc->active) {
+
+	if (crtc->active || IS_VALLEYVIEW(dev) || INTEL_INFO(dev)->gen < 5) {
 		/*
 		 * We start out with underrun reporting disabled to avoid races.
 		 * For correct bookkeeping mark this on active crtcs.
 		 *
+		 * Also on gmch platforms we dont have any hardware bits to
+		 * disable the underrun reporting. Which means we need to start
+		 * out with underrun reporting disabled also on inactive pipes,
+		 * since otherwise we'll complain about the garbage we read when
+		 * e.g. coming up after runtime pm.
+		 *
 		 * No protection against concurrent access is required - at
 		 * worst a fifo underrun happens which also sets this to false.
 		 */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b885df150910..d3fa5e0a13bd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -642,8 +642,6 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
 /* i915_irq.c */
 bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 					   enum pipe pipe, bool enable);
-bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
-					     enum pipe pipe, bool enable);
 bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 					   enum transcoder pch_transcoder,
 					   bool enable);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 45fa43f16bb3..08d5d4c16fdf 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5605,15 +5605,9 @@ static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,
 static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
 					   struct i915_power_well *power_well)
 {
-	struct drm_device *dev = dev_priv->dev;
-	enum pipe pipe;
-
 	WARN_ON_ONCE(power_well->data != PUNIT_POWER_WELL_DISP2D);
 
 	spin_lock_irq(&dev_priv->irq_lock);
-	for_each_pipe(pipe)
-		__intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
-
 	valleyview_disable_display_irqs(dev_priv);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-- 
1.8.3.1

[PATCH 10/12] drm/i915: rip our vblank reset hacks for runtime PM

[Daniel Vetter]

Now that we unconditionally dtrt when disabling/enabling crtcs we
don't need any hacks any longer to keep the vblank logic sane when
all the registers go poof. So let's rip it all out.

This essentially undoes

commit 9dbd8febb4dbc9199fcf340b882eb930e36b65b6
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Tue Jul 23 10:48:11 2013 -0300

    drm/i915: update last_vblank when disabling the power well

Apparently igt/kms_flip is already powerful enough to exercise this
properly, yay! See the reference regression report for details.

References: https://bugs.freedesktop.org/show_bug.cgi?id=66808
Testcase: igt/kms_flip/*-vs-rpm
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c | 34 ----------------------------------
 1 file changed, 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 75c1c766b507..45fa43f16bb3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5423,33 +5423,6 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
 	}
 }
 
-static void reset_vblank_counter(struct drm_device *dev, enum pipe pipe)
-{
-	assert_spin_locked(&dev->vbl_lock);
-
-	dev->vblank[pipe].last = 0;
-}
-
-static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	enum pipe pipe;
-	unsigned long irqflags;
-
-	/*
-	 * After this, the registers on the pipes that are part of the power
-	 * well will become zero, so we have to adjust our counters according to
-	 * that.
-	 *
-	 * FIXME: Should we do this in general in drm_vblank_post_modeset?
-	 */
-	spin_lock_irqsave(&dev->vbl_lock, irqflags);
-	for_each_pipe(pipe)
-		if (pipe != PIPE_A)
-			reset_vblank_counter(dev, pipe);
-	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
-}
-
 static void hsw_set_power_well(struct drm_i915_private *dev_priv,
 			       struct i915_power_well *power_well, bool enable)
 {
@@ -5478,8 +5451,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
 			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
 			POSTING_READ(HSW_PWR_WELL_DRIVER);
 			DRM_DEBUG_KMS("Requesting to disable the power well\n");
-
-			hsw_power_well_post_disable(dev_priv);
 		}
 	}
 }
@@ -5646,11 +5617,6 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
 	valleyview_disable_display_irqs(dev_priv);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	spin_lock_irq(&dev->vbl_lock);
-	for_each_pipe(pipe)
-		reset_vblank_counter(dev, pipe);
-	spin_unlock_irq(&dev->vbl_lock);
-
 	vlv_set_power_well(dev_priv, power_well, false);
 }
 
-- 
1.8.3.1

[PATCH 11/12] drm/i915: Accurately initialize fifo underrun state on gmch platforms

[Daniel Vetter]

We don't have hardware based disable bits on gmch platforms, so need
to block spurious underrun reports in software. Which means that we
_must_ start out with fifo underrun reporting disabled everywhere.

This is in big contrast to ilk/hsw/cpt where there's only _one_
disable bit for all platforms and hence we must allow underrun
reporting on disabled pipes. Otherwise nothing really works,
especially the CRC support since that's key'ed off the same irq
disable bit.

This allows us to ditch the fifo underrun reporting hack from the vlv
runtime pm code and unexport the internal function from i915_irq.c
again. Yay!

v2: Keep the display irq disabling, spotted by Imre.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c      | 4 ++--
 drivers/gpu/drm/i915/intel_display.c | 9 ++++++++-
 drivers/gpu/drm/i915/intel_drv.h     | 2 --
 drivers/gpu/drm/i915/intel_pm.c      | 6 ------
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index afa55199b829..a502faae0d0b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -415,8 +415,8 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
  *
  * Returns the previous state of underrun reporting.
  */
-bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
-					     enum pipe pipe, bool enable)
+static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
+						    enum pipe pipe, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d4abaa4bf2f4..e78003ac71a0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11510,11 +11510,18 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 			encoder->base.crtc = NULL;
 		}
 	}
-	if (crtc->active) {
+
+	if (crtc->active || IS_VALLEYVIEW(dev) || INTEL_INFO(dev)->gen < 5) {
 		/*
 		 * We start out with underrun reporting disabled to avoid races.
 		 * For correct bookkeeping mark this on active crtcs.
 		 *
+		 * Also on gmch platforms we dont have any hardware bits to
+		 * disable the underrun reporting. Which means we need to start
+		 * out with underrun reporting disabled also on inactive pipes,
+		 * since otherwise we'll complain about the garbage we read when
+		 * e.g. coming up after runtime pm.
+		 *
 		 * No protection against concurrent access is required - at
 		 * worst a fifo underrun happens which also sets this to false.
 		 */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b885df150910..d3fa5e0a13bd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -642,8 +642,6 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
 /* i915_irq.c */
 bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 					   enum pipe pipe, bool enable);
-bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
-					     enum pipe pipe, bool enable);
 bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 					   enum transcoder pch_transcoder,
 					   bool enable);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 45fa43f16bb3..08d5d4c16fdf 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5605,15 +5605,9 @@ static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,
 static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
 					   struct i915_power_well *power_well)
 {
-	struct drm_device *dev = dev_priv->dev;
-	enum pipe pipe;
-
 	WARN_ON_ONCE(power_well->data != PUNIT_POWER_WELL_DISP2D);
 
 	spin_lock_irq(&dev_priv->irq_lock);
-	for_each_pipe(pipe)
-		__intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
-
 	valleyview_disable_display_irqs(dev_priv);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-- 
1.8.3.1

Re: [PATCH 11/12] drm/i915: Accurately initialize fifo underrun state on gmch platforms

[Imre Deak]

On Wed, 2014-05-14 at 20:51 +0200, Daniel Vetter wrote:
> We don't have hardware based disable bits on gmch platforms, so need
> to block spurious underrun reports in software. Which means that we
> _must_ start out with fifo underrun reporting disabled everywhere.
> 
> This is in big contrast to ilk/hsw/cpt where there's only _one_
> disable bit for all platforms and hence we must allow underrun
> reporting on disabled pipes. Otherwise nothing really works,
> especially the CRC support since that's key'ed off the same irq
> disable bit.
> 
> This allows us to ditch the fifo underrun reporting hack from the vlv
> runtime pm code and unexport the internal function from i915_irq.c
> again. Yay!
> 
> v2: Keep the display irq disabling, spotted by Imre.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 4 ++--
>  drivers/gpu/drm/i915/intel_display.c | 9 ++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     | 2 --
>  drivers/gpu/drm/i915/intel_pm.c      | 6 ------
>  4 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index afa55199b829..a502faae0d0b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -415,8 +415,8 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
>   *
>   * Returns the previous state of underrun reporting.
>   */
> -bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
> -					     enum pipe pipe, bool enable)
> +static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
> +						    enum pipe pipe, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d4abaa4bf2f4..e78003ac71a0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11510,11 +11510,18 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  			encoder->base.crtc = NULL;
>  		}
>  	}
> -	if (crtc->active) {
> +
> +	if (crtc->active || IS_VALLEYVIEW(dev) || INTEL_INFO(dev)->gen < 5) {
>  		/*
>  		 * We start out with underrun reporting disabled to avoid races.
>  		 * For correct bookkeeping mark this on active crtcs.
>  		 *
> +		 * Also on gmch platforms we dont have any hardware bits to
> +		 * disable the underrun reporting. Which means we need to start
> +		 * out with underrun reporting disabled also on inactive pipes,
> +		 * since otherwise we'll complain about the garbage we read when
> +		 * e.g. coming up after runtime pm.
> +		 *
>  		 * No protection against concurrent access is required - at
>  		 * worst a fifo underrun happens which also sets this to false.
>  		 */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b885df150910..d3fa5e0a13bd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -642,8 +642,6 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
>  /* i915_irq.c */
>  bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
>  					   enum pipe pipe, bool enable);
> -bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
> -					     enum pipe pipe, bool enable);
>  bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
>  					   enum transcoder pch_transcoder,
>  					   bool enable);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 45fa43f16bb3..08d5d4c16fdf 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5605,15 +5605,9 @@ static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,
>  static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
>  					   struct i915_power_well *power_well)
>  {
> -	struct drm_device *dev = dev_priv->dev;
> -	enum pipe pipe;
> -
>  	WARN_ON_ONCE(power_well->data != PUNIT_POWER_WELL_DISP2D);
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
> -	for_each_pipe(pipe)
> -		__intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
> -

As we discussed on IRC, something like the following could be added to
the commit log:

Originally the purpose of disabling the reporting on vlv was to prevent
spurious underflow reports when the power well is turned off and the
pipestat register containing the underflow flag will read 0xffffffff.
After the change in intel_sanitize_crtc() to disable the reporting for
all pipes initially, it's guaranteed that reporting is disabled for all
pipes by the time we call vlv_display_power_well_disable(), so there is
no need to disable it there explicitly.

Reviewed-by: Imre Deak <imre.deak@intel.com> 


>  	valleyview_disable_display_irqs(dev_priv);
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  

[PATCH 11/12] [RFC] drm/irq: More robustness in drm_vblank_on|off

[Daniel Vetter]

If we want to use this functionality in generic helpers to make
sure that all drivers have somewhat sane vblank handling across
modesets/dpms, we need to make it work for all drivers. But some
don't support interrupts and hence also not vblank waits.

Just return early on such drivers.

Note that with pageflips drivers are free to implement them however
they wish to.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 51ebe9086be9..03fba43ab6be 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1020,6 +1020,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 	unsigned long irqflags;
 	unsigned int seq;
 
+	if (!dev->irq_enabled)
+		return;
+
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	vblank_disable_and_save(dev, crtc);
 	wake_up(&dev->vblank[crtc].queue);
@@ -1080,6 +1083,9 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
 {
 	unsigned long irqflags;
 
+	if (!dev->irq_enabled)
+		return;
+
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	/* re-enable interrupts if there's are users left */
 	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
-- 
1.8.3.1

Re: [PATCH 11/12] [RFC] drm/irq: More robustness in drm_vblank_on|off

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 768 bytes --]

On Wed, May 14, 2014 at 08:51:16PM +0200, Daniel Vetter wrote:
> If we want to use this functionality in generic helpers to make
> sure that all drivers have somewhat sane vblank handling across
> modesets/dpms, we need to make it work for all drivers. But some
> don't support interrupts and hence also not vblank waits.
> 
> Just return early on such drivers.
> 
> Note that with pageflips drivers are free to implement them however
> they wish to.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 6 ++++++
>  1 file changed, 6 insertions(+)

I'm confused. This seems to be the very same patch as 09/12. But since
you've already merged this I guess it must have resolved itself
somehow...

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 11/12] [RFC] drm/irq: More robustness in drm_vblank_on|off

[Daniel Vetter]

On Wed, May 21, 2014 at 02:53:00PM +0200, Thierry Reding wrote:
> On Wed, May 14, 2014 at 08:51:16PM +0200, Daniel Vetter wrote:
> > If we want to use this functionality in generic helpers to make
> > sure that all drivers have somewhat sane vblank handling across
> > modesets/dpms, we need to make it work for all drivers. But some
> > don't support interrupts and hence also not vblank waits.
> > 
> > Just return early on such drivers.
> > 
> > Note that with pageflips drivers are free to implement them however
> > they wish to.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_irq.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> 
> I'm confused. This seems to be the very same patch as 09/12. But since
> you've already merged this I guess it must have resolved itself
> somehow...

Screwed up the patch sending and submitted two patch 09/12. This one
really is just rfc and I didn't pull it in yet.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 12/12] [RFC] drm/crtc-helper: Enforce sane vblank counter semantics

[Daniel Vetter]

It literally took us years in i915 to track down all the vblank
bogosity, which means that userspace has now code to deal with random
crap like stuck vblank waits, needless counter wraps and other
hilarity.

To make sure that all drivers are at least somewhat sane enforce
minimal standards in the crtc helpers. Note that this is not perfect
since for intermediate levels it's unclear whether the vblank will
keep on running. Modern userspace is advised to just never use those.

Totatlly untested.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_helper.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index df281b54db01..64d8797cd172 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -167,6 +167,8 @@ static void __drm_helper_disable_unused_functions(struct drm_device *dev)
 			else
 				(*crtc_funcs->dpms)(crtc, DRM_MODE_DPMS_OFF);
 			crtc->primary->fb = NULL;
+
+			drm_crtc_vblank_off(crtc->dev, crtc);
 		}
 	}
 }
@@ -323,6 +325,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 
 	crtc_funcs->prepare(crtc);
 
+	drm_crtc_vblank_off(dev, crtc);
+
 	/* Set up the DPLL and any encoders state that needs to adjust or depend
 	 * on the DPLL.
 	 */
@@ -349,6 +353,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	crtc_funcs->commit(crtc);
 
+	drm_crtc_vblank_on(dev, crtc);
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
 
 		if (encoder->crtc != crtc)
@@ -767,6 +773,9 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode)
 			if (crtc_funcs->dpms)
 				(*crtc_funcs->dpms) (crtc,
 						     drm_helper_choose_crtc_dpms(crtc));
+
+			if (mode == DRM_MODE_DPMS_ON)
+				drm_crtc_vblank_on(crtc->dev, crtc);
 		}
 		if (encoder)
 			drm_helper_encoder_dpms(encoder, encoder_dpms);
@@ -781,6 +790,9 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode)
 			if (crtc_funcs->dpms)
 				(*crtc_funcs->dpms) (crtc,
 						     drm_helper_choose_crtc_dpms(crtc));
+
+			if (mode == DRM_MODE_DPMS_OFF)
+				drm_crtc_vblank_off(crtc->dev, crtc);
 		}
 	}
 
-- 
1.8.3.1

Re: [PATCH 00/12] irq vblank handling rework

[Ville Syrjälä]

For everything but the reset_vblank_counter() thing:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

And another caveat: I only glanced at the crtc_helper stuff. It looks
sane but I didn't go reading through the drivers to figure out if they
would fall over or work.

About the reset_vblank_counter(), I think we might still need it to keep
the counter sane when the power well goes off. But I think we have
other problems on that front esp. with suspend to disk. There the counter
should definitely get reset on all platforms, so we migth apply any kind
of diff to the user visible value. The fix would likely be to skip the
diff adjustment when resuming.

I tried to take a quick look at that yesterday but there was something
really fishy happening as the code didn't seem to observe the wraparound
at all, even though I confirmed w/ intel_reg_read that it definitely
did appear to wrap. I'll take another look at it today.

Another idea might be to rip out the diff adjustment altogether. That
should just mean that the user visible counter wouldn't advance at all
between drm_vblank_off() and drm_vblank_on(). But that might actually
be the sane thing to do. Maybe we should just do a +1 there to make
sure we don't report the same value before and after modeset. It should
fix both the suspend problems and the power well problem.

On Wed, May 14, 2014 at 08:51:02PM +0200, Daniel Vetter wrote:
> Hi all,
> 
> This is Ville's vblank rework series, slightly pimped. I've added kerneldoc,
> some polish and also some additional nasty igt testcases for these crtc on/off
> vs vblank issues. Seems all to hold together nicely.
> 
> Now one thing I wanted to do is roll this out across all drm drivers, but that
> looks ugly: Many drivers don't support vblanks really, and I couldn't fully
> audit whether they'd e.g. blow up when userspace tries to use the vblank wait
> ioctl. But I think we want to have more sanity since otherwise userspace is
> doomed to carry countless ugly hacks around forever.
> 
> The last two patches are my attempt at that. By doing the drm_vblank_on/off
> dance in the crtc helpers after we know that the crtc is on/off we should have
> at least a somewhat sane baseline behaviour. Note that since drm_vblank_on/off
> are idempotent drivers are free to call these functions in their callback at an
> earlier time, where it makes more sense. But at least it's guaranteed to happen.
> 
> Otoh drivers still need to manually complete pageflips, so this doesn't solve
> all the issues. But until we have a solid cross-driver testsuite for these
> corner-cases (all the interesting stuff happens when you race vblank waits
> against concurrent modesets, dpms, or system/runtime suspend/resume) we're
> pretty much guaranteed to have some that works at best occasionally and is
> guaranteed to have different behaviour in corner cases. Note that the patches
> won't degrade behaviour for existing drivers, the drm core changes simply allow
> us to finally fix things up correctly.
> 
> I guess in a way this is a more generic discussion for the drm subsystem at
> large.
> 
> Coments and review highly welcome.
> 
> Cheers, Daniel
> 
> Daniel Vetter (8):
>   drm/i915: Remove drm_vblank_pre/post_modeset calls
>   drm/doc: Discourage usage of MODESET_CTL ioctl
>   drm/irq: kerneldoc polish
>   drm/irq: Add kms-native crtc interface functions
>   drm/i915: rip our vblank reset hacks for runtime PM
>   drm/i915: Accurately initialize fifo underrun state on gmch platforms
>   [RFC] drm/irq: More robustness in drm_vblank_on|off
>   [RFC] drm/crtc-helper: Enforce sane vblank counter semantics
> 
> Peter Hurley (1):
>   drm: Use correct spinlock flavor in drm_vblank_get()
> 
> Ville Syrjälä (3):
>   drm: Make the vblank disable timer per-crtc
>   drm: Make blocking vblank wait return when the vblank interrupts get
>     disabled
>   drm: Add drm_vblank_on()
> 
>  Documentation/DocBook/drm.tmpl       |  16 +-
>  drivers/gpu/drm/drm_crtc_helper.c    |  12 ++
>  drivers/gpu/drm/drm_irq.c            | 377 ++++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/i915_irq.c      |   4 +-
>  drivers/gpu/drm/i915/intel_display.c |  36 ++--
>  drivers/gpu/drm/i915/intel_drv.h     |   2 -
>  drivers/gpu/drm/i915/intel_pm.c      |  40 ----
>  include/drm/drmP.h                   |  10 +-
>  8 files changed, 341 insertions(+), 156 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 00/12] irq vblank handling rework

[Daniel Vetter]

On Wed, May 21, 2014 at 11:26:55AM +0300, Ville Syrjälä wrote:
> For everything but the reset_vblank_counter() thing:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> And another caveat: I only glanced at the crtc_helper stuff. It looks
> sane but I didn't go reading through the drivers to figure out if they
> would fall over or work.

Oh, the rfc was really just that. I don't have any plans to burn my hands
trying to merge those patches ;-) Especially since interest from non-i915
hackers seems to be low.

> About the reset_vblank_counter(), I think we might still need it to keep
> the counter sane when the power well goes off. But I think we have
> other problems on that front esp. with suspend to disk. There the counter
> should definitely get reset on all platforms, so we migth apply any kind
> of diff to the user visible value. The fix would likely be to skip the
> diff adjustment when resuming.
> 
> I tried to take a quick look at that yesterday but there was something
> really fishy happening as the code didn't seem to observe the wraparound
> at all, even though I confirmed w/ intel_reg_read that it definitely
> did appear to wrap. I'll take another look at it today.
> 
> Another idea might be to rip out the diff adjustment altogether. That
> should just mean that the user visible counter wouldn't advance at all
> between drm_vblank_off() and drm_vblank_on(). But that might actually
> be the sane thing to do. Maybe we should just do a +1 there to make
> sure we don't report the same value before and after modeset. It should
> fix both the suspend problems and the power well problem.

Hm, like I've mentioned yesterday on irc the tests I have actually pass,
at least if I throw your sanitize_crtc patch on top. vblank frame counter
values monotonically increase across suspend/resume, runtime pm and
anything else I manged to throw at it. And the limit in the test is 100
frames later, but I've only observed a few tens at most.

So I think the code as-is actually works. Whether intentional or not is
still under dispute though ;-)

The real problem I have with the hsw counter reset is that the same issue
should affect _any_ platform where we support runtime pm. Like snb or byt.
But the code isn't there. Also if we have such a bug then it will also
affect hibernate and suspend to disk. Which means that this should be done
in drm_crtc_vblank_off/on, not in the guts of some random platforms
runtime pm code.

So in my opinion the hsw vblank_count reset code needs to go anyway
because:
- Either it isn't need any more (and we have the tests for this now) and
  it's just cargo-culted duct-tape.
- Or we need, but then it's in the wrong spot.

Given that can you reconsider that patch please?

Thanks, Daniel

> 
> On Wed, May 14, 2014 at 08:51:02PM +0200, Daniel Vetter wrote:
> > Hi all,
> > 
> > This is Ville's vblank rework series, slightly pimped. I've added kerneldoc,
> > some polish and also some additional nasty igt testcases for these crtc on/off
> > vs vblank issues. Seems all to hold together nicely.
> > 
> > Now one thing I wanted to do is roll this out across all drm drivers, but that
> > looks ugly: Many drivers don't support vblanks really, and I couldn't fully
> > audit whether they'd e.g. blow up when userspace tries to use the vblank wait
> > ioctl. But I think we want to have more sanity since otherwise userspace is
> > doomed to carry countless ugly hacks around forever.
> > 
> > The last two patches are my attempt at that. By doing the drm_vblank_on/off
> > dance in the crtc helpers after we know that the crtc is on/off we should have
> > at least a somewhat sane baseline behaviour. Note that since drm_vblank_on/off
> > are idempotent drivers are free to call these functions in their callback at an
> > earlier time, where it makes more sense. But at least it's guaranteed to happen.
> > 
> > Otoh drivers still need to manually complete pageflips, so this doesn't solve
> > all the issues. But until we have a solid cross-driver testsuite for these
> > corner-cases (all the interesting stuff happens when you race vblank waits
> > against concurrent modesets, dpms, or system/runtime suspend/resume) we're
> > pretty much guaranteed to have some that works at best occasionally and is
> > guaranteed to have different behaviour in corner cases. Note that the patches
> > won't degrade behaviour for existing drivers, the drm core changes simply allow
> > us to finally fix things up correctly.
> > 
> > I guess in a way this is a more generic discussion for the drm subsystem at
> > large.
> > 
> > Coments and review highly welcome.
> > 
> > Cheers, Daniel
> > 
> > Daniel Vetter (8):
> >   drm/i915: Remove drm_vblank_pre/post_modeset calls
> >   drm/doc: Discourage usage of MODESET_CTL ioctl
> >   drm/irq: kerneldoc polish
> >   drm/irq: Add kms-native crtc interface functions
> >   drm/i915: rip our vblank reset hacks for runtime PM
> >   drm/i915: Accurately initialize fifo underrun state on gmch platforms
> >   [RFC] drm/irq: More robustness in drm_vblank_on|off
> >   [RFC] drm/crtc-helper: Enforce sane vblank counter semantics
> > 
> > Peter Hurley (1):
> >   drm: Use correct spinlock flavor in drm_vblank_get()
> > 
> > Ville Syrjälä (3):
> >   drm: Make the vblank disable timer per-crtc
> >   drm: Make blocking vblank wait return when the vblank interrupts get
> >     disabled
> >   drm: Add drm_vblank_on()
> > 
> >  Documentation/DocBook/drm.tmpl       |  16 +-
> >  drivers/gpu/drm/drm_crtc_helper.c    |  12 ++
> >  drivers/gpu/drm/drm_irq.c            | 377 ++++++++++++++++++++++++++---------
> >  drivers/gpu/drm/i915/i915_irq.c      |   4 +-
> >  drivers/gpu/drm/i915/intel_display.c |  36 ++--
> >  drivers/gpu/drm/i915/intel_drv.h     |   2 -
> >  drivers/gpu/drm/i915/intel_pm.c      |  40 ----
> >  include/drm/drmP.h                   |  10 +-
> >  8 files changed, 341 insertions(+), 156 deletions(-)
> > 
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 00/12] irq vblank handling rework

[Ville Syrjälä]

On Wed, May 21, 2014 at 10:35:59AM +0200, Daniel Vetter wrote:
> On Wed, May 21, 2014 at 11:26:55AM +0300, Ville Syrjälä wrote:
> > For everything but the reset_vblank_counter() thing:
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > And another caveat: I only glanced at the crtc_helper stuff. It looks
> > sane but I didn't go reading through the drivers to figure out if they
> > would fall over or work.
> 
> Oh, the rfc was really just that. I don't have any plans to burn my hands
> trying to merge those patches ;-) Especially since interest from non-i915
> hackers seems to be low.
> 
> > About the reset_vblank_counter(), I think we might still need it to keep
> > the counter sane when the power well goes off. But I think we have
> > other problems on that front esp. with suspend to disk. There the counter
> > should definitely get reset on all platforms, so we migth apply any kind
> > of diff to the user visible value. The fix would likely be to skip the
> > diff adjustment when resuming.
> > 
> > I tried to take a quick look at that yesterday but there was something
> > really fishy happening as the code didn't seem to observe the wraparound
> > at all, even though I confirmed w/ intel_reg_read that it definitely
> > did appear to wrap. I'll take another look at it today.
> > 
> > Another idea might be to rip out the diff adjustment altogether. That
> > should just mean that the user visible counter wouldn't advance at all
> > between drm_vblank_off() and drm_vblank_on(). But that might actually
> > be the sane thing to do. Maybe we should just do a +1 there to make
> > sure we don't report the same value before and after modeset. It should
> > fix both the suspend problems and the power well problem.
> 
> Hm, like I've mentioned yesterday on irc the tests I have actually pass,
> at least if I throw your sanitize_crtc patch on top. vblank frame counter
> values monotonically increase across suspend/resume, runtime pm and
> anything else I manged to throw at it. And the limit in the test is 100
> frames later, but I've only observed a few tens at most.
> 
> So I think the code as-is actually works. Whether intentional or not is
> still under dispute though ;-)
> 
> The real problem I have with the hsw counter reset is that the same issue
> should affect _any_ platform where we support runtime pm. Like snb or byt.
> But the code isn't there. Also if we have such a bug then it will also
> affect hibernate and suspend to disk. Which means that this should be done
> in drm_crtc_vblank_off/on, not in the guts of some random platforms
> runtime pm code.
> 
> So in my opinion the hsw vblank_count reset code needs to go anyway
> because:
> - Either it isn't need any more (and we have the tests for this now) and
>   it's just cargo-culted duct-tape.
> - Or we need, but then it's in the wrong spot.
> 
> Given that can you reconsider that patch please?

Yeah. So as discussed on irc I think the right fix would be to sample
the current counter in drm_vblank_on(), stick it into
dev->vblank[crtc].last, but skip the diff adjustment to the user visible
counter (maybe just +1 to make sure we never report the same value on
both sides of a modeset). That should cover both the suspend case and the
power well case. I can go and experiment with this a bit...

So I agree that the current code isn't the way things should be done.
And since I now have an idea how it should be done, I'm fine with
ripping the current thing out. So you can add:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 00/12] irq vblank handling rework

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 777 bytes --]

On Wed, May 21, 2014 at 10:35:59AM +0200, Daniel Vetter wrote:
> On Wed, May 21, 2014 at 11:26:55AM +0300, Ville Syrjälä wrote:
> > For everything but the reset_vblank_counter() thing:
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > And another caveat: I only glanced at the crtc_helper stuff. It looks
> > sane but I didn't go reading through the drivers to figure out if they
> > would fall over or work.
> 
> Oh, the rfc was really just that. I don't have any plans to burn my hands
> trying to merge those patches ;-) Especially since interest from non-i915
> hackers seems to be low.

I think that's mostly because nobody except i915 has a testsuite that
would even remotely be able to uncover any of these issues. =)

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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