[PATCH 1/4] drm/irq: Add drm_crtc_vblank_reset

[PATCH 1/4] drm/irq: Add drm_crtc_vblank_reset

[Daniel Vetter]

At driver load we need to tell the vblank code about the state of the
pipes, so that the logic around reject vblank_get when the pipe is off
works correctly.

Thus far i915 used drm_vblank_off, but one of the side-effects of it
is that it also saves the vblank counter. And for that it calls down
into the ->get_vblank_counter hook. Which isn't really a good idea
when the pipe is off for a few reasons:
- With runtime pm the register might not respond.
- If the pipe is off some datastructures might not be around or
  unitialized.

The later is what blew up on gen3: We look at intel_crtc->config to
compute the vblank counter, and for a disabled pipe at boot-up that's
just not there. Thus far this was papered over by a check for
intel_crtc->active, but I want to get rid of that (since it's fairly
race, vblank hooks are called from all kinds of places).

So prep for that by adding a _reset functions which only does what we
really need to be done at driver load: Mark the vblank pipe as off,
but don't do any vblank counter saving or event flushing - neither of
that is required.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_irq.c            | 32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  4 ++--
 include/drm/drmP.h                   |  1 +
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 75647e7f012b..1e5fb1b994d7 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1226,6 +1226,38 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 EXPORT_SYMBOL(drm_crtc_vblank_off);
 
 /**
+ * drm_crtc_vblank_reset - reset vblank state to off on a CRTC
+ * @crtc: CRTC in question
+ *
+ * Drivers can use this function to reset the vblank state to off at load time.
+ * Drivers should use this together with the drm_crtc_vblank_off() and
+ * drm_crtc_vblank_on() functions. The diffrence comparet to
+ * drm_crtc_vblank_off() is that this function doesn't save the vblank counter
+ * and hence doesn't need to call any driver hooks.
+ */
+void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc)
+{
+	struct drm_device *dev = drm_crtc->dev;
+	unsigned long irqflags;
+	int crtc = drm_crtc_index(drm_crtc);
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
+
+	spin_lock_irqsave(&dev->vbl_lock, irqflags);
+	/*
+	 * Prevent subsequent drm_vblank_get() from enabling the vblank
+	 * interrupt by bumping the refcount.
+	 */
+	if (!vblank->inmodeset) {
+		atomic_inc(&vblank->refcount);
+		vblank->inmodeset = 1;
+	}
+	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+
+	WARN_ON(!list_empty(&dev->vblank_event_list));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_reset);
+
+/**
  * drm_vblank_on - enable vblank events on a CRTC
  * @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 423ef959264d..f8871a184747 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13296,9 +13296,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	/* restore vblank interrupts to correct state */
 	if (crtc->active) {
 		update_scanline_offset(crtc);
-		drm_vblank_on(dev, crtc->pipe);
+		drm_crtc_vblank_on(&crtc->base);
 	} else
-		drm_vblank_off(dev, crtc->pipe);
+		drm_crtc_vblank_reset(&crtc->base);
 
 	/* We need to sanitize the plane -> pipe mapping first because this will
 	 * disable the crtc (and hence change the state) if it is wrong. Note
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index e928625a9da0..54c6ea1e5866 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -922,6 +922,7 @@ extern void drm_crtc_wait_one_vblank(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_reset(struct drm_crtc *crtc);
 extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
 extern void drm_vblank_cleanup(struct drm_device *dev);
 
-- 
1.9.3

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

[PATCH 2/4] drm/i915: Drop pipe_enable checks in vblank funcs

[Daniel Vetter]

With Ville's rework to use drm_crtc_vblank_on/off the core will take
care of rejecting drm_vblank_get calls when the pipe is off. Also the
core won't call the get_vblank_counter hooks in that case either. And
since we've dropped ums support recently we can now remove these
hacks, yay!

Noticed while trying to answer questions Laurent had about how the new
atomic helpers work.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 52 -----------------------------------------
 1 file changed, 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 23bfe2232b6a..80f35dcffea4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -492,31 +492,6 @@ static void i915_enable_asle_pipestat(struct drm_device *dev)
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
-/**
- * i915_pipe_enabled - check if a pipe is enabled
- * @dev: DRM device
- * @pipe: pipe to check
- *
- * Reading certain registers when the pipe is disabled can hang the chip.
- * Use this routine to make sure the PLL is running and the pipe is active
- * before reading such registers if unsure.
- */
-static int
-i915_pipe_enabled(struct drm_device *dev, int pipe)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-		/* Locking is horribly broken here, but whatever. */
-		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-		return intel_crtc->active;
-	} else {
-		return I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE;
-	}
-}
-
 /*
  * This timing diagram depicts the video signal in and
  * around the vertical blanking period.
@@ -583,12 +558,6 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
 	unsigned long low_frame;
 	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
 
-	if (!i915_pipe_enabled(dev, pipe)) {
-		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
-				"pipe %c\n", pipe_name(pipe));
-		return 0;
-	}
-
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		struct intel_crtc *intel_crtc =
 			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
@@ -648,12 +617,6 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int reg = PIPE_FRMCOUNT_GM45(pipe);
 
-	if (!i915_pipe_enabled(dev, pipe)) {
-		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
-				 "pipe %c\n", pipe_name(pipe));
-		return 0;
-	}
-
 	return I915_READ(reg);
 }
 
@@ -2660,9 +2623,6 @@ static int i915_enable_vblank(struct drm_device *dev, int pipe)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 
-	if (!i915_pipe_enabled(dev, pipe))
-		return -EINVAL;
-
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	if (INTEL_INFO(dev)->gen >= 4)
 		i915_enable_pipestat(dev_priv, pipe,
@@ -2682,9 +2642,6 @@ static int ironlake_enable_vblank(struct drm_device *dev, int pipe)
 	uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) :
 						     DE_PIPE_VBLANK(pipe);
 
-	if (!i915_pipe_enabled(dev, pipe))
-		return -EINVAL;
-
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	ironlake_enable_display_irq(dev_priv, bit);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
@@ -2697,9 +2654,6 @@ static int valleyview_enable_vblank(struct drm_device *dev, int pipe)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 
-	if (!i915_pipe_enabled(dev, pipe))
-		return -EINVAL;
-
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, pipe,
 			     PIPE_START_VBLANK_INTERRUPT_STATUS);
@@ -2713,9 +2667,6 @@ static int gen8_enable_vblank(struct drm_device *dev, int pipe)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 
-	if (!i915_pipe_enabled(dev, pipe))
-		return -EINVAL;
-
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	dev_priv->de_irq_mask[pipe] &= ~GEN8_PIPE_VBLANK;
 	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
@@ -2767,9 +2718,6 @@ static void gen8_disable_vblank(struct drm_device *dev, int pipe)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 
-	if (!i915_pipe_enabled(dev, pipe))
-		return;
-
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	dev_priv->de_irq_mask[pipe] |= GEN8_PIPE_VBLANK;
 	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
-- 
1.9.3

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

Re: [PATCH 2/4] drm/i915: Drop pipe_enable checks in vblank funcs

[Imre Deak]

On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> With Ville's rework to use drm_crtc_vblank_on/off the core will take
> care of rejecting drm_vblank_get calls when the pipe is off. Also the
> core won't call the get_vblank_counter hooks in that case either. And
> since we've dropped ums support recently we can now remove these
> hacks, yay!
> 
> Noticed while trying to answer questions Laurent had about how the new
> atomic helpers work.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 52 -----------------------------------------
>  1 file changed, 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 23bfe2232b6a..80f35dcffea4 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -492,31 +492,6 @@ static void i915_enable_asle_pipestat(struct drm_device *dev)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
>  
> -/**
> - * i915_pipe_enabled - check if a pipe is enabled
> - * @dev: DRM device
> - * @pipe: pipe to check
> - *
> - * Reading certain registers when the pipe is disabled can hang the chip.
> - * Use this routine to make sure the PLL is running and the pipe is active
> - * before reading such registers if unsure.
> - */
> -static int
> -i915_pipe_enabled(struct drm_device *dev, int pipe)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> -		/* Locking is horribly broken here, but whatever. */
> -		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
> -		return intel_crtc->active;
> -	} else {
> -		return I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE;
> -	}
> -}
> -
>  /*
>   * This timing diagram depicts the video signal in and
>   * around the vertical blanking period.
> @@ -583,12 +558,6 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
>  	unsigned long low_frame;
>  	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
>  
> -	if (!i915_pipe_enabled(dev, pipe)) {
> -		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
> -				"pipe %c\n", pipe_name(pipe));
> -		return 0;
> -	}

Wouldn't it make sense to still keep these as an assert (by throwing
away the !MODESET part in i915_pipe_enabled). To me it seems we could
still get here with the pipe disabled at least via drm_irq_uninstall().

> -
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		struct intel_crtc *intel_crtc =
>  			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> @@ -648,12 +617,6 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int reg = PIPE_FRMCOUNT_GM45(pipe);
>  
> -	if (!i915_pipe_enabled(dev, pipe)) {
> -		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
> -				 "pipe %c\n", pipe_name(pipe));
> -		return 0;
> -	}
> -
>  	return I915_READ(reg);
>  }
>  
> @@ -2660,9 +2623,6 @@ static int i915_enable_vblank(struct drm_device *dev, int pipe)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
>  
> -	if (!i915_pipe_enabled(dev, pipe))
> -		return -EINVAL;
> -
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	if (INTEL_INFO(dev)->gen >= 4)
>  		i915_enable_pipestat(dev_priv, pipe,
> @@ -2682,9 +2642,6 @@ static int ironlake_enable_vblank(struct drm_device *dev, int pipe)
>  	uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) :
>  						     DE_PIPE_VBLANK(pipe);
>  
> -	if (!i915_pipe_enabled(dev, pipe))
> -		return -EINVAL;
> -
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	ironlake_enable_display_irq(dev_priv, bit);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> @@ -2697,9 +2654,6 @@ static int valleyview_enable_vblank(struct drm_device *dev, int pipe)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
>  
> -	if (!i915_pipe_enabled(dev, pipe))
> -		return -EINVAL;
> -
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	i915_enable_pipestat(dev_priv, pipe,
>  			     PIPE_START_VBLANK_INTERRUPT_STATUS);
> @@ -2713,9 +2667,6 @@ static int gen8_enable_vblank(struct drm_device *dev, int pipe)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
>  
> -	if (!i915_pipe_enabled(dev, pipe))
> -		return -EINVAL;
> -
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	dev_priv->de_irq_mask[pipe] &= ~GEN8_PIPE_VBLANK;
>  	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
> @@ -2767,9 +2718,6 @@ static void gen8_disable_vblank(struct drm_device *dev, int pipe)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
>  
> -	if (!i915_pipe_enabled(dev, pipe))
> -		return;
> -
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	dev_priv->de_irq_mask[pipe] |= GEN8_PIPE_VBLANK;
>  	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);


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

Re: [PATCH 2/4] drm/i915: Drop pipe_enable checks in vblank funcs

[Daniel Vetter]

On Fri, Feb 13, 2015 at 12:37:27AM +0200, Imre Deak wrote:
> On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> > With Ville's rework to use drm_crtc_vblank_on/off the core will take
> > care of rejecting drm_vblank_get calls when the pipe is off. Also the
> > core won't call the get_vblank_counter hooks in that case either. And
> > since we've dropped ums support recently we can now remove these
> > hacks, yay!
> > 
> > Noticed while trying to answer questions Laurent had about how the new
> > atomic helpers work.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 52 -----------------------------------------
> >  1 file changed, 52 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 23bfe2232b6a..80f35dcffea4 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -492,31 +492,6 @@ static void i915_enable_asle_pipestat(struct drm_device *dev)
> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >  }
> >  
> > -/**
> > - * i915_pipe_enabled - check if a pipe is enabled
> > - * @dev: DRM device
> > - * @pipe: pipe to check
> > - *
> > - * Reading certain registers when the pipe is disabled can hang the chip.
> > - * Use this routine to make sure the PLL is running and the pipe is active
> > - * before reading such registers if unsure.
> > - */
> > -static int
> > -i915_pipe_enabled(struct drm_device *dev, int pipe)
> > -{
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> > -	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > -		/* Locking is horribly broken here, but whatever. */
> > -		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -
> > -		return intel_crtc->active;
> > -	} else {
> > -		return I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE;
> > -	}
> > -}
> > -
> >  /*
> >   * This timing diagram depicts the video signal in and
> >   * around the vertical blanking period.
> > @@ -583,12 +558,6 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
> >  	unsigned long low_frame;
> >  	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> >  
> > -	if (!i915_pipe_enabled(dev, pipe)) {
> > -		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
> > -				"pipe %c\n", pipe_name(pipe));
> > -		return 0;
> > -	}
> 
> Wouldn't it make sense to still keep these as an assert (by throwing
> away the !MODESET part in i915_pipe_enabled). To me it seems we could
> still get here with the pipe disabled at least via drm_irq_uninstall().

The problem I see with the i915_pipe_enable check is that we do noodle
around in modeset state structures from irq context. Which is not awesome
at all, so I'd like to get rid of it completely.

The code in drm_irq_uninstall is indeed buggy though, it should grab the
timestamp from the software side like drm_vblank_off does. Note that on
i915 this code can't run since we disable the pipe first, which will
already flush out any pending events. Of which there shouldn't be any left
when the driver unloads anyway.
-Daniel

> 
> > -
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >  		struct intel_crtc *intel_crtc =
> >  			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > @@ -648,12 +617,6 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int reg = PIPE_FRMCOUNT_GM45(pipe);
> >  
> > -	if (!i915_pipe_enabled(dev, pipe)) {
> > -		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
> > -				 "pipe %c\n", pipe_name(pipe));
> > -		return 0;
> > -	}
> > -
> >  	return I915_READ(reg);
> >  }
> >  
> > @@ -2660,9 +2623,6 @@ static int i915_enable_vblank(struct drm_device *dev, int pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	unsigned long irqflags;
> >  
> > -	if (!i915_pipe_enabled(dev, pipe))
> > -		return -EINVAL;
> > -
> >  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >  	if (INTEL_INFO(dev)->gen >= 4)
> >  		i915_enable_pipestat(dev_priv, pipe,
> > @@ -2682,9 +2642,6 @@ static int ironlake_enable_vblank(struct drm_device *dev, int pipe)
> >  	uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) :
> >  						     DE_PIPE_VBLANK(pipe);
> >  
> > -	if (!i915_pipe_enabled(dev, pipe))
> > -		return -EINVAL;
> > -
> >  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >  	ironlake_enable_display_irq(dev_priv, bit);
> >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > @@ -2697,9 +2654,6 @@ static int valleyview_enable_vblank(struct drm_device *dev, int pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	unsigned long irqflags;
> >  
> > -	if (!i915_pipe_enabled(dev, pipe))
> > -		return -EINVAL;
> > -
> >  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >  	i915_enable_pipestat(dev_priv, pipe,
> >  			     PIPE_START_VBLANK_INTERRUPT_STATUS);
> > @@ -2713,9 +2667,6 @@ static int gen8_enable_vblank(struct drm_device *dev, int pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	unsigned long irqflags;
> >  
> > -	if (!i915_pipe_enabled(dev, pipe))
> > -		return -EINVAL;
> > -
> >  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >  	dev_priv->de_irq_mask[pipe] &= ~GEN8_PIPE_VBLANK;
> >  	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
> > @@ -2767,9 +2718,6 @@ static void gen8_disable_vblank(struct drm_device *dev, int pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	unsigned long irqflags;
> >  
> > -	if (!i915_pipe_enabled(dev, pipe))
> > -		return;
> > -
> >  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >  	dev_priv->de_irq_mask[pipe] |= GEN8_PIPE_VBLANK;
> >  	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/i915: Flatten DRIVER_MODESET checks in i915_irq.c

[Daniel Vetter]

UMS is no more!

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 80f35dcffea4..37189a25ca82 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -557,28 +557,16 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
 	unsigned long high_frame;
 	unsigned long low_frame;
 	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+	const struct drm_display_mode *mode =
+		&intel_crtc->config->base.adjusted_mode;
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-		struct intel_crtc *intel_crtc =
-			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-		const struct drm_display_mode *mode =
-			&intel_crtc->config->base.adjusted_mode;
-
-		htotal = mode->crtc_htotal;
-		hsync_start = mode->crtc_hsync_start;
-		vbl_start = mode->crtc_vblank_start;
-		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
-			vbl_start = DIV_ROUND_UP(vbl_start, 2);
-	} else {
-		enum transcoder cpu_transcoder = (enum transcoder) pipe;
-
-		htotal = ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff) + 1;
-		hsync_start = (I915_READ(HSYNC(cpu_transcoder))  & 0x1fff) + 1;
-		vbl_start = (I915_READ(VBLANK(cpu_transcoder)) & 0x1fff) + 1;
-		if ((I915_READ(PIPECONF(cpu_transcoder)) &
-		     PIPECONF_INTERLACE_MASK) != PIPECONF_PROGRESSIVE)
-			vbl_start = DIV_ROUND_UP(vbl_start, 2);
-	}
+	htotal = mode->crtc_htotal;
+	hsync_start = mode->crtc_hsync_start;
+	vbl_start = mode->crtc_vblank_start;
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		vbl_start = DIV_ROUND_UP(vbl_start, 2);
 
 	/* Convert to pixel count */
 	vbl_start *= htotal;
@@ -4330,10 +4318,8 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	if (!IS_GEN2(dev_priv))
 		dev->vblank_disable_immediate = true;
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-		dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
-		dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
-	}
+	dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
+	dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
 
 	if (IS_CHERRYVIEW(dev_priv)) {
 		dev->driver->irq_handler = cherryview_irq_handler;
-- 
1.9.3

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

Re: [PATCH 3/4] drm/i915: Flatten DRIVER_MODESET checks in i915_irq.c

[Imre Deak]

On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> UMS is no more!
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

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

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 36 +++++++++++-------------------------
>  1 file changed, 11 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 80f35dcffea4..37189a25ca82 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -557,28 +557,16 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
>  	unsigned long high_frame;
>  	unsigned long low_frame;
>  	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> +	struct intel_crtc *intel_crtc =
> +		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> +	const struct drm_display_mode *mode =
> +		&intel_crtc->config->base.adjusted_mode;
>  
> -	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> -		struct intel_crtc *intel_crtc =
> -			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> -		const struct drm_display_mode *mode =
> -			&intel_crtc->config->base.adjusted_mode;
> -
> -		htotal = mode->crtc_htotal;
> -		hsync_start = mode->crtc_hsync_start;
> -		vbl_start = mode->crtc_vblank_start;
> -		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> -			vbl_start = DIV_ROUND_UP(vbl_start, 2);
> -	} else {
> -		enum transcoder cpu_transcoder = (enum transcoder) pipe;
> -
> -		htotal = ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff) + 1;
> -		hsync_start = (I915_READ(HSYNC(cpu_transcoder))  & 0x1fff) + 1;
> -		vbl_start = (I915_READ(VBLANK(cpu_transcoder)) & 0x1fff) + 1;
> -		if ((I915_READ(PIPECONF(cpu_transcoder)) &
> -		     PIPECONF_INTERLACE_MASK) != PIPECONF_PROGRESSIVE)
> -			vbl_start = DIV_ROUND_UP(vbl_start, 2);
> -	}
> +	htotal = mode->crtc_htotal;
> +	hsync_start = mode->crtc_hsync_start;
> +	vbl_start = mode->crtc_vblank_start;
> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		vbl_start = DIV_ROUND_UP(vbl_start, 2);
>  
>  	/* Convert to pixel count */
>  	vbl_start *= htotal;
> @@ -4330,10 +4318,8 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	if (!IS_GEN2(dev_priv))
>  		dev->vblank_disable_immediate = true;
>  
> -	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> -		dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
> -		dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
> -	}
> +	dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
> +	dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
>  
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		dev->driver->irq_handler = cherryview_irq_handler;


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

Re: [PATCH 3/4] drm/i915: Flatten DRIVER_MODESET checks in i915_irq.c

[Dave Gordon]

On 12/02/15 22:38, Imre Deak wrote:
> On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
>> UMS is no more!
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Some machines now won't boot in "recovery mode", which specifies
"nomodeset" and therefore results in various important bits of code not
being executed. Will we eventually ignore "modeset" completely, or just
refuse to load at all if "nomodeset" is explicitly specified?

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

Re: [PATCH 3/4] drm/i915: Flatten DRIVER_MODESET checks in i915_irq.c

[Imre Deak]

On to, 2015-02-19 at 12:25 +0000, Dave Gordon wrote:
> On 12/02/15 22:38, Imre Deak wrote:
> > On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> >> UMS is no more!
> >>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Some machines now won't boot in "recovery mode", which specifies
> "nomodeset" and therefore results in various important bits of code not
> being executed. Will we eventually ignore "modeset" completely, or just
> refuse to load at all if "nomodeset" is explicitly specified?

The driver will already refuse to load with nomodeset for GEN6+ for
quite some time now. On old platforms UMS would still work before this
patch, but afaik there was a decision to stop supporting UMS. Note that
this doesn't mean "recovery mode" or equivalently nomodeset will break
booting, it just means user space will fall back to vesa/vga or text
mode.

--Imre

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

Re: [PATCH 3/4] drm/i915: Flatten DRIVER_MODESET checks in i915_irq.c

[Imre Deak]

On to, 2015-02-19 at 15:39 +0200, Imre Deak wrote:
> On to, 2015-02-19 at 12:25 +0000, Dave Gordon wrote:
> > On 12/02/15 22:38, Imre Deak wrote:
> > > On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> > >> UMS is no more!
> > >>
> > >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > 
> > Some machines now won't boot in "recovery mode", which specifies
> > "nomodeset" and therefore results in various important bits of code not
> > being executed. Will we eventually ignore "modeset" completely, or just
> > refuse to load at all if "nomodeset" is explicitly specified?
> 
> The driver will already refuse to load with nomodeset for GEN6+ for
> quite some time now. On old platforms UMS would still work before this
> patch, but afaik there was a decision to stop supporting UMS. Note that
> this doesn't mean "recovery mode" or equivalently nomodeset will break
> booting, it just means user space will fall back to vesa/vga or text
> mode.

Ah, or did you mean after this patch we should refuse loading the driver
in case of nomodeset even for old platforms? That would make sense
indeed.

--Imre

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

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Flatten DRIVER_MODESET checks in i915_irq.c

[Imre Deak]

Hi Dave,

On to, 2015-02-19 at 15:42 +0200, Imre Deak wrote:
> On to, 2015-02-19 at 15:39 +0200, Imre Deak wrote:
> > On to, 2015-02-19 at 12:25 +0000, Dave Gordon wrote:
> > > On 12/02/15 22:38, Imre Deak wrote:
> > > > On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> > > >> UMS is no more!
> > > >>
> > > >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > 
> > > Some machines now won't boot in "recovery mode", which specifies
> > > "nomodeset" and therefore results in various important bits of code not
> > > being executed. Will we eventually ignore "modeset" completely, or just
> > > refuse to load at all if "nomodeset" is explicitly specified?
> > 
> > The driver will already refuse to load with nomodeset for GEN6+ for
> > quite some time now. On old platforms UMS would still work before this
> > patch, but afaik there was a decision to stop supporting UMS. Note that
> > this doesn't mean "recovery mode" or equivalently nomodeset will break
> > booting, it just means user space will fall back to vesa/vga or text
> > mode.
> 
> Ah, or did you mean after this patch we should refuse loading the driver
> in case of nomodeset even for old platforms? That would make sense
> indeed.

I was wrong here, I was thinking only about the GEN6 MODESET check in
i915_driver_load. As Daniel pointed out on IRC in addition to that we
also silently fail to load driver in i915_init for !MODESET always,
regardless of the platform, so the check in i915_driver_load is
redundant. Based on this it's safe to remove the !MODESET parts.

--Imre

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

[PATCH 4/4] drm/i915: Switch to drm_crtc variants of vblank functions

[Daniel Vetter]

Where possible right now. Just a small step towards nirvana ...

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 2 +-
 drivers/gpu/drm/i915/intel_display.c | 9 +++++----
 drivers/gpu/drm/i915/intel_sprite.c  | 4 ++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3b332a493674..479ea0825166 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -580,7 +580,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
 			seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
 				   work->flip_queued_vblank,
 				   work->flip_ready_vblank,
-				   drm_vblank_count(dev, crtc->pipe));
+				   drm_crtc_vblank_count(dev, &crtc->base));
 			if (work->enable_stall_check)
 				seq_puts(m, "Stall check enabled, ");
 			else
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f8871a184747..2c33ef5c5f68 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9674,10 +9674,10 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
 		    !i915_gem_request_completed(work->flip_queued_req, true))
 			return false;
 
-		work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
+		work->flip_ready_vblank = drm_crtc_vblank_count(dev, crtc);
 	}
 
-	if (drm_vblank_count(dev, intel_crtc->pipe) - work->flip_ready_vblank < 3)
+	if (drm_crtc_vblank_count(dev, crtc) - work->flip_ready_vblank < 3)
 		return false;
 
 	/* Potential stall - if we see that the flip has happened,
@@ -9708,7 +9708,8 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
 	spin_lock(&dev->event_lock);
 	if (intel_crtc->unpin_work && __intel_pageflip_stall_check(dev, crtc)) {
 		WARN_ONCE(1, "Kicking stuck page flip: queued at %d, now %d\n",
-			 intel_crtc->unpin_work->flip_queued_vblank, drm_vblank_count(dev, pipe));
+			 intel_crtc->unpin_work->flip_queued_vblank,
+			 drm_vblank_count(dev, pipe));
 		page_flip_completed(intel_crtc);
 	}
 	spin_unlock(&dev->event_lock);
@@ -9849,7 +9850,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 					intel_ring_get_request(ring));
 	}
 
-	work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
+	work->flip_queued_vblank = drm_crtc_vblank_count(dev, crtc);
 	work->enable_stall_check = true;
 
 	i915_gem_track_fb(work->old_fb_obj, obj,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0a52c44ad03d..0f499adfe2e0 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -98,7 +98,7 @@ bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 	if (min <= 0 || max <= 0)
 		return false;
 
-	if (WARN_ON(drm_vblank_get(dev, pipe)))
+	if (WARN_ON(drm_crtc_vblank_get(dev, &crtc->base)))
 		return false;
 
 	local_irq_disable();
@@ -132,7 +132,7 @@ bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 
 	finish_wait(wq, &wait);
 
-	drm_vblank_put(dev, pipe);
+	drm_crtc_vblank_put(dev, &crtc->base);
 
 	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
 
-- 
1.9.3

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

Re: [PATCH 4/4] drm/i915: Switch to drm_crtc variants of vblank functions

[Imre Deak]

On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> Where possible right now. Just a small step towards nirvana ...
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 2 +-
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
>  drivers/gpu/drm/i915/intel_sprite.c  | 4 ++--
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3b332a493674..479ea0825166 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -580,7 +580,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
>  			seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
>  				   work->flip_queued_vblank,
>  				   work->flip_ready_vblank,
> -				   drm_vblank_count(dev, crtc->pipe));
> +				   drm_crtc_vblank_count(dev, &crtc->base));

This doesn't compile, did these functions change meanwhile?

>  			if (work->enable_stall_check)
>  				seq_puts(m, "Stall check enabled, ");
>  			else
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f8871a184747..2c33ef5c5f68 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9674,10 +9674,10 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
>  		    !i915_gem_request_completed(work->flip_queued_req, true))
>  			return false;
>  
> -		work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> +		work->flip_ready_vblank = drm_crtc_vblank_count(dev, crtc);
>  	}
>  
> -	if (drm_vblank_count(dev, intel_crtc->pipe) - work->flip_ready_vblank < 3)
> +	if (drm_crtc_vblank_count(dev, crtc) - work->flip_ready_vblank < 3)
>  		return false;
>  
>  	/* Potential stall - if we see that the flip has happened,
> @@ -9708,7 +9708,8 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
>  	spin_lock(&dev->event_lock);
>  	if (intel_crtc->unpin_work && __intel_pageflip_stall_check(dev, crtc)) {
>  		WARN_ONCE(1, "Kicking stuck page flip: queued at %d, now %d\n",
> -			 intel_crtc->unpin_work->flip_queued_vblank, drm_vblank_count(dev, pipe));
> +			 intel_crtc->unpin_work->flip_queued_vblank,
> +			 drm_vblank_count(dev, pipe));

Did you want to change this too? Regardless of this by making it
compile:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>  		page_flip_completed(intel_crtc);
>  	}
>  	spin_unlock(&dev->event_lock);
> @@ -9849,7 +9850,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  					intel_ring_get_request(ring));
>  	}
>  
> -	work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> +	work->flip_queued_vblank = drm_crtc_vblank_count(dev, crtc);
>  	work->enable_stall_check = true;
>  
>  	i915_gem_track_fb(work->old_fb_obj, obj,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0a52c44ad03d..0f499adfe2e0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -98,7 +98,7 @@ bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
>  	if (min <= 0 || max <= 0)
>  		return false;
>  
> -	if (WARN_ON(drm_vblank_get(dev, pipe)))
> +	if (WARN_ON(drm_crtc_vblank_get(dev, &crtc->base)))
>  		return false;
>  
>  	local_irq_disable();
> @@ -132,7 +132,7 @@ bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
>  
>  	finish_wait(wq, &wait);
>  
> -	drm_vblank_put(dev, pipe);
> +	drm_crtc_vblank_put(dev, &crtc->base);
>  
>  	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
>  


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

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Switch to drm_crtc variants of vblank functions

[Daniel Vetter]

On Fri, Feb 13, 2015 at 12:57:12AM +0200, Imre Deak wrote:
> On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> > Where possible right now. Just a small step towards nirvana ...
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c  | 2 +-
> >  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
> >  drivers/gpu/drm/i915/intel_sprite.c  | 4 ++--
> >  3 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 3b332a493674..479ea0825166 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -580,7 +580,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
> >  			seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
> >  				   work->flip_queued_vblank,
> >  				   work->flip_ready_vblank,
> > -				   drm_vblank_count(dev, crtc->pipe));
> > +				   drm_crtc_vblank_count(dev, &crtc->base));
> 
> This doesn't compile, did these functions change meanwhile?

Oh dear, git add fail. I'll resend, including the follow-up fix for
drm_irq_uninstall.

Thanks a lot for your review.
-Daniel

> 
> >  			if (work->enable_stall_check)
> >  				seq_puts(m, "Stall check enabled, ");
> >  			else
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f8871a184747..2c33ef5c5f68 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9674,10 +9674,10 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
> >  		    !i915_gem_request_completed(work->flip_queued_req, true))
> >  			return false;
> >  
> > -		work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> > +		work->flip_ready_vblank = drm_crtc_vblank_count(dev, crtc);
> >  	}
> >  
> > -	if (drm_vblank_count(dev, intel_crtc->pipe) - work->flip_ready_vblank < 3)
> > +	if (drm_crtc_vblank_count(dev, crtc) - work->flip_ready_vblank < 3)
> >  		return false;
> >  
> >  	/* Potential stall - if we see that the flip has happened,
> > @@ -9708,7 +9708,8 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
> >  	spin_lock(&dev->event_lock);
> >  	if (intel_crtc->unpin_work && __intel_pageflip_stall_check(dev, crtc)) {
> >  		WARN_ONCE(1, "Kicking stuck page flip: queued at %d, now %d\n",
> > -			 intel_crtc->unpin_work->flip_queued_vblank, drm_vblank_count(dev, pipe));
> > +			 intel_crtc->unpin_work->flip_queued_vblank,
> > +			 drm_vblank_count(dev, pipe));
> 
> Did you want to change this too? Regardless of this by making it
> compile:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> >  		page_flip_completed(intel_crtc);
> >  	}
> >  	spin_unlock(&dev->event_lock);
> > @@ -9849,7 +9850,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  					intel_ring_get_request(ring));
> >  	}
> >  
> > -	work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> > +	work->flip_queued_vblank = drm_crtc_vblank_count(dev, crtc);
> >  	work->enable_stall_check = true;
> >  
> >  	i915_gem_track_fb(work->old_fb_obj, obj,
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 0a52c44ad03d..0f499adfe2e0 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -98,7 +98,7 @@ bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
> >  	if (min <= 0 || max <= 0)
> >  		return false;
> >  
> > -	if (WARN_ON(drm_vblank_get(dev, pipe)))
> > +	if (WARN_ON(drm_crtc_vblank_get(dev, &crtc->base)))
> >  		return false;
> >  
> >  	local_irq_disable();
> > @@ -132,7 +132,7 @@ bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
> >  
> >  	finish_wait(wq, &wait);
> >  
> > -	drm_vblank_put(dev, pipe);
> > +	drm_crtc_vblank_put(dev, &crtc->base);
> >  
> >  	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
> >  
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/irq: Add drm_crtc_vblank_reset

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Tuesday 03 February 2015 11:30:11 Daniel Vetter wrote:
> At driver load we need to tell the vblank code about the state of the
> pipes, so that the logic around reject vblank_get when the pipe is off
> works correctly.
> 
> Thus far i915 used drm_vblank_off, but one of the side-effects of it
> is that it also saves the vblank counter. And for that it calls down
> into the ->get_vblank_counter hook. Which isn't really a good idea
> when the pipe is off for a few reasons:
> - With runtime pm the register might not respond.
> - If the pipe is off some datastructures might not be around or
>   unitialized.
> 
> The later is what blew up on gen3: We look at intel_crtc->config to
> compute the vblank counter, and for a disabled pipe at boot-up that's
> just not there. Thus far this was papered over by a check for
> intel_crtc->active, but I want to get rid of that (since it's fairly
> race, vblank hooks are called from all kinds of places).
> 
> So prep for that by adding a _reset functions which only does what we
> really need to be done at driver load: Mark the vblank pipe as off,
> but don't do any vblank counter saving or event flushing - neither of
> that is required.

(Thinking out loud)

In principle this looks good, but I find the naming pretty confusing. The 
drm_crtc_vblank_* functions now have get, put, on, off and reset variants. The 
fact that on is supposed to be called both at runtime and an init time while 
off is replaced by reset at init time breaks the symmetry between on and off. 
What would you think of a drm_crtc_vblank_init() function instead, used at 
init time only, and that would take an enabled boolean argument ?

On the other hand, calling drm_crtc_vblank_on() at init time also makes sense, 
as even if the CRTC is active the vblank interrupt should be off then, and an 
explicit call to the function means "turn the vblank interrupt on". I'm thus 
not totally opposed to keeping that as-is. Wouldn't it then be better to 
modify the core to default to off, and let drivers call drm_crtc_vblank_on() 
explicitly if the default isn't correct ? I think I like this solution better, 
and it could be conditioned by a driver flag if we don't want to audit all 
drivers for possible breakages.

> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c            | 32 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  4 ++--
>  include/drm/drmP.h                   |  1 +
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 75647e7f012b..1e5fb1b994d7 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1226,6 +1226,38 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
>  EXPORT_SYMBOL(drm_crtc_vblank_off);
> 
>  /**
> + * drm_crtc_vblank_reset - reset vblank state to off on a CRTC
> + * @crtc: CRTC in question
> + *
> + * Drivers can use this function to reset the vblank state to off at load
> time.
> + * Drivers should use this together with the drm_crtc_vblank_off() and
> + * drm_crtc_vblank_on() functions. The diffrence comparet to

Typo: difference compared to

> + * drm_crtc_vblank_off() is that this function doesn't save the vblank
> counter
> + * and hence doesn't need to call any driver hooks.
> + */
> +void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc)
> +{
> +	struct drm_device *dev = drm_crtc->dev;
> +	unsigned long irqflags;
> +	int crtc = drm_crtc_index(drm_crtc);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> +
> +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	/*
> +	 * Prevent subsequent drm_vblank_get() from enabling the vblank
> +	 * interrupt by bumping the refcount.
> +	 */
> +	if (!vblank->inmodeset) {
> +		atomic_inc(&vblank->refcount);
> +		vblank->inmodeset = 1;
> +	}
> +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +
> +	WARN_ON(!list_empty(&dev->vblank_event_list));
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_reset);
> +
> +/**
>   * drm_vblank_on - enable vblank events on a CRTC
>   * @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 423ef959264d..f8871a184747
> 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13296,9 +13296,9 @@ static void intel_sanitize_crtc(struct intel_crtc
> *crtc) /* restore vblank interrupts to correct state */
>  	if (crtc->active) {
>  		update_scanline_offset(crtc);
> -		drm_vblank_on(dev, crtc->pipe);
> +		drm_crtc_vblank_on(&crtc->base);
>  	} else
> -		drm_vblank_off(dev, crtc->pipe);
> +		drm_crtc_vblank_reset(&crtc->base);
> 
>  	/* We need to sanitize the plane -> pipe mapping first because this will
>  	 * disable the crtc (and hence change the state) if it is wrong. Note
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index e928625a9da0..54c6ea1e5866 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -922,6 +922,7 @@ extern void drm_crtc_wait_one_vblank(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_reset(struct drm_crtc *crtc);
>  extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
>  extern void drm_vblank_cleanup(struct drm_device *dev);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/4] drm/irq: Add drm_crtc_vblank_reset

[Daniel Vetter]

On Tue, Feb 03, 2015 at 01:31:34PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Tuesday 03 February 2015 11:30:11 Daniel Vetter wrote:
> > At driver load we need to tell the vblank code about the state of the
> > pipes, so that the logic around reject vblank_get when the pipe is off
> > works correctly.
> > 
> > Thus far i915 used drm_vblank_off, but one of the side-effects of it
> > is that it also saves the vblank counter. And for that it calls down
> > into the ->get_vblank_counter hook. Which isn't really a good idea
> > when the pipe is off for a few reasons:
> > - With runtime pm the register might not respond.
> > - If the pipe is off some datastructures might not be around or
> >   unitialized.
> > 
> > The later is what blew up on gen3: We look at intel_crtc->config to
> > compute the vblank counter, and for a disabled pipe at boot-up that's
> > just not there. Thus far this was papered over by a check for
> > intel_crtc->active, but I want to get rid of that (since it's fairly
> > race, vblank hooks are called from all kinds of places).
> > 
> > So prep for that by adding a _reset functions which only does what we
> > really need to be done at driver load: Mark the vblank pipe as off,
> > but don't do any vblank counter saving or event flushing - neither of
> > that is required.
> 
> (Thinking out loud)
> 
> In principle this looks good, but I find the naming pretty confusing. The 
> drm_crtc_vblank_* functions now have get, put, on, off and reset variants. The 
> fact that on is supposed to be called both at runtime and an init time while 
> off is replaced by reset at init time breaks the symmetry between on and off. 
> What would you think of a drm_crtc_vblank_init() function instead, used at 
> init time only, and that would take an enabled boolean argument ?
> 
> On the other hand, calling drm_crtc_vblank_on() at init time also makes sense, 
> as even if the CRTC is active the vblank interrupt should be off then, and an 
> explicit call to the function means "turn the vblank interrupt on". I'm thus 
> not totally opposed to keeping that as-is. Wouldn't it then be better to 
> modify the core to default to off, and let drivers call drm_crtc_vblank_on() 
> explicitly if the default isn't correct ? I think I like this solution better, 
> and it could be conditioned by a driver flag if we don't want to audit all 
> drivers for possible breakages.

Yeah, that's been my thinking with sticking with on and only replacing the
off: For _on the environment exactly matches what we do when enabling the
crtc in a modeset:
- the pipe is actually running
- we want vblanks to start working

Totally different for _off, which assumes:
- pipe is still on
- vblanks should stop running and state should be saved
- any pending
Wheras _reset just disallows vblanks.

Long-term I wonder whether a drm_crtc_vblank_init would be useful, which:
- uses dev->mode_config.num_crtcs to set up the right amount of vblank
  pipes.
- calls _reset directly
- in the future might even store the vblank state in the drm_crtc.

But before we can do that we need to split the vblank code into ums legacy
paths using int pipe and kms paths dealing in struct drm_crtc *. We're not
there yet.

> 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_irq.c            | 32 ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c |  4 ++--
> >  include/drm/drmP.h                   |  1 +
> >  3 files changed, 35 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 75647e7f012b..1e5fb1b994d7 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -1226,6 +1226,38 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
> >  EXPORT_SYMBOL(drm_crtc_vblank_off);
> > 
> >  /**
> > + * drm_crtc_vblank_reset - reset vblank state to off on a CRTC
> > + * @crtc: CRTC in question
> > + *
> > + * Drivers can use this function to reset the vblank state to off at load
> > time.
> > + * Drivers should use this together with the drm_crtc_vblank_off() and
> > + * drm_crtc_vblank_on() functions. The diffrence comparet to
> 
> Typo: difference compared to
> 
> > + * drm_crtc_vblank_off() is that this function doesn't save the vblank
> > counter
> > + * and hence doesn't need to call any driver hooks.
> > + */
> > +void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc)
> > +{
> > +	struct drm_device *dev = drm_crtc->dev;
> > +	unsigned long irqflags;
> > +	int crtc = drm_crtc_index(drm_crtc);
> > +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> > +
> > +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > +	/*
> > +	 * Prevent subsequent drm_vblank_get() from enabling the vblank
> > +	 * interrupt by bumping the refcount.
> > +	 */
> > +	if (!vblank->inmodeset) {
> > +		atomic_inc(&vblank->refcount);
> > +		vblank->inmodeset = 1;
> > +	}
> > +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > +
> > +	WARN_ON(!list_empty(&dev->vblank_event_list));
> > +}
> > +EXPORT_SYMBOL(drm_crtc_vblank_reset);
> > +
> > +/**
> >   * drm_vblank_on - enable vblank events on a CRTC
> >   * @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 423ef959264d..f8871a184747
> > 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13296,9 +13296,9 @@ static void intel_sanitize_crtc(struct intel_crtc
> > *crtc) /* restore vblank interrupts to correct state */
> >  	if (crtc->active) {
> >  		update_scanline_offset(crtc);
> > -		drm_vblank_on(dev, crtc->pipe);
> > +		drm_crtc_vblank_on(&crtc->base);
> >  	} else
> > -		drm_vblank_off(dev, crtc->pipe);
> > +		drm_crtc_vblank_reset(&crtc->base);
> > 
> >  	/* We need to sanitize the plane -> pipe mapping first because this will
> >  	 * disable the crtc (and hence change the state) if it is wrong. Note
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index e928625a9da0..54c6ea1e5866 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -922,6 +922,7 @@ extern void drm_crtc_wait_one_vblank(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_reset(struct drm_crtc *crtc);
> >  extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
> >  extern void drm_vblank_cleanup(struct drm_device *dev);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
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: [Intel-gfx] [PATCH 1/4] drm/irq: Add drm_crtc_vblank_reset

[Ville Syrjälä]

On Tue, Feb 03, 2015 at 11:30:11AM +0100, Daniel Vetter wrote:
> At driver load we need to tell the vblank code about the state of the
> pipes, so that the logic around reject vblank_get when the pipe is off
> works correctly.
> 
> Thus far i915 used drm_vblank_off, but one of the side-effects of it
> is that it also saves the vblank counter. And for that it calls down
> into the ->get_vblank_counter hook. Which isn't really a good idea
> when the pipe is off for a few reasons:
> - With runtime pm the register might not respond.
> - If the pipe is off some datastructures might not be around or
>   unitialized.
> 
> The later is what blew up on gen3: We look at intel_crtc->config to
> compute the vblank counter, and for a disabled pipe at boot-up that's
> just not there. Thus far this was papered over by a check for
> intel_crtc->active, but I want to get rid of that (since it's fairly
> race, vblank hooks are called from all kinds of places).
> 
> So prep for that by adding a _reset functions which only does what we
> really need to be done at driver load: Mark the vblank pipe as off,
> but don't do any vblank counter saving or event flushing - neither of
> that is required.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c            | 32 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  4 ++--
>  include/drm/drmP.h                   |  1 +
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 75647e7f012b..1e5fb1b994d7 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1226,6 +1226,38 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
>  EXPORT_SYMBOL(drm_crtc_vblank_off);
>  
>  /**
> + * drm_crtc_vblank_reset - reset vblank state to off on a CRTC
> + * @crtc: CRTC in question
> + *
> + * Drivers can use this function to reset the vblank state to off at load time.
> + * Drivers should use this together with the drm_crtc_vblank_off() and
> + * drm_crtc_vblank_on() functions. The diffrence comparet to
> + * drm_crtc_vblank_off() is that this function doesn't save the vblank counter
> + * and hence doesn't need to call any driver hooks.
> + */
> +void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc)
> +{
> +	struct drm_device *dev = drm_crtc->dev;
> +	unsigned long irqflags;
> +	int crtc = drm_crtc_index(drm_crtc);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> +
> +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	/*
> +	 * Prevent subsequent drm_vblank_get() from enabling the vblank
> +	 * interrupt by bumping the refcount.
> +	 */
> +	if (!vblank->inmodeset) {
> +		atomic_inc(&vblank->refcount);
> +		vblank->inmodeset = 1;
> +	}
> +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +
> +	WARN_ON(!list_empty(&dev->vblank_event_list));
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_reset);
> +
> +/**
>   * drm_vblank_on - enable vblank events on a CRTC
>   * @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 423ef959264d..f8871a184747 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13296,9 +13296,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	/* restore vblank interrupts to correct state */
>  	if (crtc->active) {
>  		update_scanline_offset(crtc);
> -		drm_vblank_on(dev, crtc->pipe);
> +		drm_crtc_vblank_on(&crtc->base);
>  	} else
> -		drm_vblank_off(dev, crtc->pipe);
> +		drm_crtc_vblank_reset(&crtc->base);

Maybe

drm_vblank_reset()
if (active)
	drm_vblank_on()

?


>  
>  	/* We need to sanitize the plane -> pipe mapping first because this will
>  	 * disable the crtc (and hence change the state) if it is wrong. Note
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index e928625a9da0..54c6ea1e5866 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -922,6 +922,7 @@ extern void drm_crtc_wait_one_vblank(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_reset(struct drm_crtc *crtc);
>  extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
>  extern void drm_vblank_cleanup(struct drm_device *dev);
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/irq: Add drm_crtc_vblank_reset

[Imre Deak]

On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> At driver load we need to tell the vblank code about the state of the
> pipes, so that the logic around reject vblank_get when the pipe is off
> works correctly.
> 
> Thus far i915 used drm_vblank_off, but one of the side-effects of it
> is that it also saves the vblank counter. And for that it calls down
> into the ->get_vblank_counter hook. Which isn't really a good idea
> when the pipe is off for a few reasons:
> - With runtime pm the register might not respond.
> - If the pipe is off some datastructures might not be around or
>   unitialized.
> 
> The later is what blew up on gen3: We look at intel_crtc->config to
> compute the vblank counter, and for a disabled pipe at boot-up that's
> just not there. Thus far this was papered over by a check for
> intel_crtc->active, but I want to get rid of that (since it's fairly
> race, vblank hooks are called from all kinds of places).
> 
> So prep for that by adding a _reset functions which only does what we
> really need to be done at driver load: Mark the vblank pipe as off,
> but don't do any vblank counter saving or event flushing - neither of
> that is required.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c            | 32 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  4 ++--
>  include/drm/drmP.h                   |  1 +
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 75647e7f012b..1e5fb1b994d7 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1226,6 +1226,38 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
>  EXPORT_SYMBOL(drm_crtc_vblank_off);
>  
>  /**
> + * drm_crtc_vblank_reset - reset vblank state to off on a CRTC
> + * @crtc: CRTC in question
> + *
> + * Drivers can use this function to reset the vblank state to off at load time.
> + * Drivers should use this together with the drm_crtc_vblank_off() and
> + * drm_crtc_vblank_on() functions. The diffrence comparet to
> + * drm_crtc_vblank_off() is that this function doesn't save the vblank counter
> + * and hence doesn't need to call any driver hooks.
> + */
> +void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc)
> +{
> +	struct drm_device *dev = drm_crtc->dev;
> +	unsigned long irqflags;
> +	int crtc = drm_crtc_index(drm_crtc);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> +
> +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	/*
> +	 * Prevent subsequent drm_vblank_get() from enabling the vblank
> +	 * interrupt by bumping the refcount.
> +	 */
> +	if (!vblank->inmodeset) {
> +		atomic_inc(&vblank->refcount);
> +		vblank->inmodeset = 1;
> +	}
> +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +
> +	WARN_ON(!list_empty(&dev->vblank_event_list));
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_reset);
> +
> +/**
>   * drm_vblank_on - enable vblank events on a CRTC
>   * @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 423ef959264d..f8871a184747 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13296,9 +13296,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	/* restore vblank interrupts to correct state */
>  	if (crtc->active) {
>  		update_scanline_offset(crtc);
> -		drm_vblank_on(dev, crtc->pipe);
> +		drm_crtc_vblank_on(&crtc->base);
>  	} else
> -		drm_vblank_off(dev, crtc->pipe);
> +		drm_crtc_vblank_reset(&crtc->base);

Since DRM_IOCTL_WAIT_VBLANK is an unlocked ioctl it could trigger the
WARN in drm_crtc_vblank_reset() if the ioctl is called during driver
loading. I know it's a corner case and that probably other ioctls are
already broken in this regard, but we could try not to make things
worse. One way to that would be to call drm_crtc_vblank_reset()
unconditionally as Ville suggested, but before enabling irqs.

>  
>  	/* We need to sanitize the plane -> pipe mapping first because this will
>  	 * disable the crtc (and hence change the state) if it is wrong. Note
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index e928625a9da0..54c6ea1e5866 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -922,6 +922,7 @@ extern void drm_crtc_wait_one_vblank(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_reset(struct drm_crtc *crtc);
>  extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
>  extern void drm_vblank_cleanup(struct drm_device *dev);
>  


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

Re: [PATCH 1/4] drm/irq: Add drm_crtc_vblank_reset

[Daniel Vetter]

On Thu, Feb 12, 2015 at 11:56:50PM +0200, Imre Deak wrote:
> On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> > At driver load we need to tell the vblank code about the state of the
> > pipes, so that the logic around reject vblank_get when the pipe is off
> > works correctly.
> > 
> > Thus far i915 used drm_vblank_off, but one of the side-effects of it
> > is that it also saves the vblank counter. And for that it calls down
> > into the ->get_vblank_counter hook. Which isn't really a good idea
> > when the pipe is off for a few reasons:
> > - With runtime pm the register might not respond.
> > - If the pipe is off some datastructures might not be around or
> >   unitialized.
> > 
> > The later is what blew up on gen3: We look at intel_crtc->config to
> > compute the vblank counter, and for a disabled pipe at boot-up that's
> > just not there. Thus far this was papered over by a check for
> > intel_crtc->active, but I want to get rid of that (since it's fairly
> > race, vblank hooks are called from all kinds of places).
> > 
> > So prep for that by adding a _reset functions which only does what we
> > really need to be done at driver load: Mark the vblank pipe as off,
> > but don't do any vblank counter saving or event flushing - neither of
> > that is required.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_irq.c            | 32 ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c |  4 ++--
> >  include/drm/drmP.h                   |  1 +
> >  3 files changed, 35 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 75647e7f012b..1e5fb1b994d7 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -1226,6 +1226,38 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
> >  EXPORT_SYMBOL(drm_crtc_vblank_off);
> >  
> >  /**
> > + * drm_crtc_vblank_reset - reset vblank state to off on a CRTC
> > + * @crtc: CRTC in question
> > + *
> > + * Drivers can use this function to reset the vblank state to off at load time.
> > + * Drivers should use this together with the drm_crtc_vblank_off() and
> > + * drm_crtc_vblank_on() functions. The diffrence comparet to
> > + * drm_crtc_vblank_off() is that this function doesn't save the vblank counter
> > + * and hence doesn't need to call any driver hooks.
> > + */
> > +void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc)
> > +{
> > +	struct drm_device *dev = drm_crtc->dev;
> > +	unsigned long irqflags;
> > +	int crtc = drm_crtc_index(drm_crtc);
> > +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> > +
> > +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > +	/*
> > +	 * Prevent subsequent drm_vblank_get() from enabling the vblank
> > +	 * interrupt by bumping the refcount.
> > +	 */
> > +	if (!vblank->inmodeset) {
> > +		atomic_inc(&vblank->refcount);
> > +		vblank->inmodeset = 1;
> > +	}
> > +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > +
> > +	WARN_ON(!list_empty(&dev->vblank_event_list));
> > +}
> > +EXPORT_SYMBOL(drm_crtc_vblank_reset);
> > +
> > +/**
> >   * drm_vblank_on - enable vblank events on a CRTC
> >   * @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 423ef959264d..f8871a184747 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13296,9 +13296,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> >  	/* restore vblank interrupts to correct state */
> >  	if (crtc->active) {
> >  		update_scanline_offset(crtc);
> > -		drm_vblank_on(dev, crtc->pipe);
> > +		drm_crtc_vblank_on(&crtc->base);
> >  	} else
> > -		drm_vblank_off(dev, crtc->pipe);
> > +		drm_crtc_vblank_reset(&crtc->base);
> 
> Since DRM_IOCTL_WAIT_VBLANK is an unlocked ioctl it could trigger the
> WARN in drm_crtc_vblank_reset() if the ioctl is called during driver
> loading. I know it's a corner case and that probably other ioctls are
> already broken in this regard, but we could try not to make things
> worse. One way to that would be to call drm_crtc_vblank_reset()
> unconditionally as Ville suggested, but before enabling irqs.

You can't open the drm file until driver load completes, drm_global_mutex
ensures that. Which is totally not how it's supposed to be done (correct
way is to delay registering the dev node until it's all loaded), but until
we've completely ripped out UMS we can't switch over.

Unconditionally calling vblank_reset would break all the other drivers not
using vblank_on/off yet. And I don't want to audit all of them ...
-Daniel

> 
> >  
> >  	/* We need to sanitize the plane -> pipe mapping first because this will
> >  	 * disable the crtc (and hence change the state) if it is wrong. Note
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index e928625a9da0..54c6ea1e5866 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -922,6 +922,7 @@ extern void drm_crtc_wait_one_vblank(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_reset(struct drm_crtc *crtc);
> >  extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
> >  extern void drm_vblank_cleanup(struct drm_device *dev);
> >  
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/irq: Add drm_crtc_vblank_reset

[Imre Deak]

On pe, 2015-02-13 at 08:44 +0100, Daniel Vetter wrote:
> On Thu, Feb 12, 2015 at 11:56:50PM +0200, Imre Deak wrote:
> > On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> > > At driver load we need to tell the vblank code about the state of the
> > > pipes, so that the logic around reject vblank_get when the pipe is off
> > > works correctly.
> > > 
> > > Thus far i915 used drm_vblank_off, but one of the side-effects of it
> > > is that it also saves the vblank counter. And for that it calls down
> > > into the ->get_vblank_counter hook. Which isn't really a good idea
> > > when the pipe is off for a few reasons:
> > > - With runtime pm the register might not respond.
> > > - If the pipe is off some datastructures might not be around or
> > >   unitialized.
> > > 
> > > The later is what blew up on gen3: We look at intel_crtc->config to
> > > compute the vblank counter, and for a disabled pipe at boot-up that's
> > > just not there. Thus far this was papered over by a check for
> > > intel_crtc->active, but I want to get rid of that (since it's fairly
> > > race, vblank hooks are called from all kinds of places).
> > > 
> > > So prep for that by adding a _reset functions which only does what we
> > > really need to be done at driver load: Mark the vblank pipe as off,
> > > but don't do any vblank counter saving or event flushing - neither of
> > > that is required.
> > > 
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_irq.c            | 32 ++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_display.c |  4 ++--
> > >  include/drm/drmP.h                   |  1 +
> > >  3 files changed, 35 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > > index 75647e7f012b..1e5fb1b994d7 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -1226,6 +1226,38 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
> > >  EXPORT_SYMBOL(drm_crtc_vblank_off);
> > >  
> > >  /**
> > > + * drm_crtc_vblank_reset - reset vblank state to off on a CRTC
> > > + * @crtc: CRTC in question
> > > + *
> > > + * Drivers can use this function to reset the vblank state to off at load time.
> > > + * Drivers should use this together with the drm_crtc_vblank_off() and
> > > + * drm_crtc_vblank_on() functions. The diffrence comparet to
> > > + * drm_crtc_vblank_off() is that this function doesn't save the vblank counter
> > > + * and hence doesn't need to call any driver hooks.
> > > + */
> > > +void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc)
> > > +{
> > > +	struct drm_device *dev = drm_crtc->dev;
> > > +	unsigned long irqflags;
> > > +	int crtc = drm_crtc_index(drm_crtc);
> > > +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> > > +
> > > +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > > +	/*
> > > +	 * Prevent subsequent drm_vblank_get() from enabling the vblank
> > > +	 * interrupt by bumping the refcount.
> > > +	 */
> > > +	if (!vblank->inmodeset) {
> > > +		atomic_inc(&vblank->refcount);
> > > +		vblank->inmodeset = 1;
> > > +	}
> > > +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > > +
> > > +	WARN_ON(!list_empty(&dev->vblank_event_list));
> > > +}
> > > +EXPORT_SYMBOL(drm_crtc_vblank_reset);
> > > +
> > > +/**
> > >   * drm_vblank_on - enable vblank events on a CRTC
> > >   * @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 423ef959264d..f8871a184747 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13296,9 +13296,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> > >  	/* restore vblank interrupts to correct state */
> > >  	if (crtc->active) {
> > >  		update_scanline_offset(crtc);
> > > -		drm_vblank_on(dev, crtc->pipe);
> > > +		drm_crtc_vblank_on(&crtc->base);
> > >  	} else
> > > -		drm_vblank_off(dev, crtc->pipe);
> > > +		drm_crtc_vblank_reset(&crtc->base);
> > 
> > Since DRM_IOCTL_WAIT_VBLANK is an unlocked ioctl it could trigger the
> > WARN in drm_crtc_vblank_reset() if the ioctl is called during driver
> > loading. I know it's a corner case and that probably other ioctls are
> > already broken in this regard, but we could try not to make things
> > worse. One way to that would be to call drm_crtc_vblank_reset()
> > unconditionally as Ville suggested, but before enabling irqs.
> 
> You can't open the drm file until driver load completes, drm_global_mutex
> ensures that.

Oops, I didn't think about checking open too. What I wrote above can be
ignored then.

> Which is totally not how it's supposed to be done (correct
> way is to delay registering the dev node until it's all loaded), but until
> we've completely ripped out UMS we can't switch over.

Agreed. Also driver unloading should be fixed by doing the reverse. 

> Unconditionally calling vblank_reset would break all the other drivers not
> using vblank_on/off yet. And I don't want to audit all of them ...

I meant doing that from intel code; I think it would be more logical as
others pointed out. But the above is functionally correct so this is:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> -Daniel
> 
> > 
> > >  
> > >  	/* We need to sanitize the plane -> pipe mapping first because this will
> > >  	 * disable the crtc (and hence change the state) if it is wrong. Note
> > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > > index e928625a9da0..54c6ea1e5866 100644
> > > --- a/include/drm/drmP.h
> > > +++ b/include/drm/drmP.h
> > > @@ -922,6 +922,7 @@ extern void drm_crtc_wait_one_vblank(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_reset(struct drm_crtc *crtc);
> > >  extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
> > >  extern void drm_vblank_cleanup(struct drm_device *dev);
> > >  
> > 
> > 
> 


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