[PATCH 0/5] Make num_sprites a per pipe value (v3)

[PATCH 0/5] Make num_sprites a per pipe value (v3)

[Damien Lespiau]

Follow up of:
  http://lists.freedesktop.org/archives/intel-gfx/2014-February/040789.html

With a couple of changes:

  - Fix up the poor style when initializing the per-pipe num_sprites
  - Added more for_each_pipe(pipe) conversion
  - Added a small cleanup patch spotted when doing the previous patch

-- 
Damien

Damien Lespiau (5):
  drm/i915: Use a pipe variable to cycle through the pipes
  drm/i915: Don't declare unnecessary shadowing variable
  drm/i915: Replace a few for_each_pipe(i) by for_each_pipe(pipe)
  drm/i915: Add a for_each_sprite() macro
  drm/i915: Make num_sprites a per-pipe value

 drivers/gpu/drm/i915/i915_debugfs.c  | 15 +++++++--------
 drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++--
 drivers/gpu/drm/i915/i915_drv.h      |  5 +++--
 drivers/gpu/drm/i915/i915_irq.c      | 14 +++++++-------
 drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++----------
 drivers/gpu/drm/i915/intel_pm.c      | 16 ++++++++--------
 6 files changed, 42 insertions(+), 37 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] drm/i915: Use a pipe variable to cycle through the pipes

[Damien Lespiau]

I recently fumbled a patch because I wrote twice num_sprites[i], and it
was the right thing to do in only 50% of the cases.

This patch ensures I need to write num_sprites[pipe], ie it should be
self-documented that it's per-pipe number of sprites without having to
look at what is 'i' this time around.

It's all a lame excuse, but it does make it harder to redo the same
mistake.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2e05d1e..71638fd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10997,7 +10997,8 @@ void intel_modeset_suspend_hw(struct drm_device *dev)
 void intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int i, j, ret;
+	int j, ret;
+	enum pipe pipe;
 
 	drm_mode_config_init(dev);
 
@@ -11034,13 +11035,13 @@ void intel_modeset_init(struct drm_device *dev)
 		      INTEL_INFO(dev)->num_pipes,
 		      INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
 
-	for_each_pipe(i) {
-		intel_crtc_init(dev, i);
+	for_each_pipe(pipe) {
+		intel_crtc_init(dev, pipe);
 		for (j = 0; j < INTEL_INFO(dev)->num_sprites; j++) {
-			ret = intel_plane_init(dev, i, j);
+			ret = intel_plane_init(dev, pipe, j);
 			if (ret)
 				DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
-					      pipe_name(i), sprite_name(i, j), ret);
+					      pipe_name(pipe), sprite_name(pipe, j), ret);
 		}
 	}
 
-- 
1.8.3.1

[PATCH 2/5] drm/i915: Don't declare unnecessary shadowing variable

[Damien Lespiau]

'i' is already defined in the function scope and used elsewhere. Let's
use it instead.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d90a707..d247926 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -602,7 +602,6 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
 	intel_runtime_pm_get(dev_priv);
 
 	if (INTEL_INFO(dev)->gen >= 8) {
-		int i;
 		seq_printf(m, "Master Interrupt Control:\t%08x\n",
 			   I915_READ(GEN8_MASTER_IRQ));
 
-- 
1.8.3.1

[PATCH 3/5] drm/i915: Replace a few for_each_pipe(i) by for_each_pipe(pipe)

[Damien Lespiau]

Consistency throughout the code base is good and remove some room for
mistakes (as explained in the "drm/i915: Use a pipe variable to cycle
through the pipes" commit)

So, let's replace the for_each_pipe(i) occurences by for_each_pipe(pipe)
when it's reasonable and practical to do so (eg. when there isn't another
pipe variable already).

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++-------
 drivers/gpu/drm/i915/i915_irq.c     | 14 +++++++-------
 drivers/gpu/drm/i915/intel_pm.c     | 16 ++++++++--------
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d247926..99d48ec 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -614,16 +614,16 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
 				   i, I915_READ(GEN8_GT_IER(i)));
 		}
 
-		for_each_pipe(i) {
+		for_each_pipe(pipe) {
 			seq_printf(m, "Pipe %c IMR:\t%08x\n",
-				   pipe_name(i),
-				   I915_READ(GEN8_DE_PIPE_IMR(i)));
+				   pipe_name(pipe),
+				   I915_READ(GEN8_DE_PIPE_IMR(pipe)));
 			seq_printf(m, "Pipe %c IIR:\t%08x\n",
-				   pipe_name(i),
-				   I915_READ(GEN8_DE_PIPE_IIR(i)));
+				   pipe_name(pipe),
+				   I915_READ(GEN8_DE_PIPE_IIR(pipe)));
 			seq_printf(m, "Pipe %c IER:\t%08x\n",
-				   pipe_name(i),
-				   I915_READ(GEN8_DE_PIPE_IER(i)));
+				   pipe_name(pipe),
+				   I915_READ(GEN8_DE_PIPE_IER(pipe)));
 		}
 
 		seq_printf(m, "Display Engine port interrupt mask:\t%08x\n",
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f68aee3..44fca75 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1865,7 +1865,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe i;
+	enum pipe pipe;
 
 	if (de_iir & DE_ERR_INT_IVB)
 		ivb_err_int_handler(dev);
@@ -1876,14 +1876,14 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 	if (de_iir & DE_GSE_IVB)
 		intel_opregion_asle_intr(dev);
 
-	for_each_pipe(i) {
-		if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
-			drm_handle_vblank(dev, i);
+	for_each_pipe(pipe) {
+		if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
+			drm_handle_vblank(dev, pipe);
 
 		/* plane/pipes map 1:1 on ilk+ */
-		if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
-			intel_prepare_page_flip(dev, i);
-			intel_finish_page_flip_plane(dev, i);
+		if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
+			intel_prepare_page_flip(dev, pipe);
+			intel_finish_page_flip_plane(dev, pipe);
 		}
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c8347ae..007bf76 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4788,7 +4788,7 @@ static void lpt_suspend_hw(struct drm_device *dev)
 static void gen8_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe i;
+	enum pipe pipe;
 
 	I915_WRITE(WM3_LP_ILK, 0);
 	I915_WRITE(WM2_LP_ILK, 0);
@@ -4824,9 +4824,9 @@ static void gen8_init_clock_gating(struct drm_device *dev)
 		   I915_READ(CHICKEN_PAR1_1) | DPA_MASK_VBLANK_SRD);
 
 	/* WaPsrDPRSUnmaskVBlankInSRD:bdw */
-	for_each_pipe(i) {
-		I915_WRITE(CHICKEN_PIPESL_1(i),
-			   I915_READ(CHICKEN_PIPESL_1(i) |
+	for_each_pipe(pipe) {
+		I915_WRITE(CHICKEN_PIPESL_1(pipe),
+			   I915_READ(CHICKEN_PIPESL_1(pipe) |
 				     DPRS_MASK_VBLANK_SRD));
 	}
 
@@ -5256,7 +5256,7 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
 static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
-	enum pipe p;
+	enum pipe pipe;
 	unsigned long irqflags;
 
 	/*
@@ -5267,9 +5267,9 @@ static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv)
 	 * FIXME: Should we do this in general in drm_vblank_post_modeset?
 	 */
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
-	for_each_pipe(p)
-		if (p != PIPE_A)
-			dev->vblank[p].last = 0;
+	for_each_pipe(pipe)
+		if (pipe != PIPE_A)
+			dev->vblank[pipe].last = 0;
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 }
 
-- 
1.8.3.1

[PATCH 4/5] drm/i915: Add a for_each_sprite() macro

[Damien Lespiau]

This macro is similar to for_each_pipe() we already have. Convert the
two call sites we have at the same time.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 05cfcc1..ad51a0e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -159,6 +159,7 @@ enum hpd_pin {
 	 I915_GEM_DOMAIN_VERTEX)
 
 #define for_each_pipe(p) for ((p) = 0; (p) < INTEL_INFO(dev)->num_pipes; (p)++)
+#define for_each_sprite(p, s) for ((s) = 0; (s) < INTEL_INFO(dev)->num_sprites; (s)++)
 
 #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
 	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 71638fd..5d8763f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1189,16 +1189,16 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
 				    enum pipe pipe)
 {
 	struct drm_device *dev = dev_priv->dev;
-	int reg, i;
+	int reg, sprite;
 	u32 val;
 
 	if (IS_VALLEYVIEW(dev)) {
-		for (i = 0; i < INTEL_INFO(dev)->num_sprites; i++) {
-			reg = SPCNTR(pipe, i);
+		for_each_sprite(pipe, sprite) {
+			reg = SPCNTR(pipe, sprite);
 			val = I915_READ(reg);
 			WARN((val & SP_ENABLE),
 			     "sprite %c assertion failure, should be off on pipe %c but is still active\n",
-			     sprite_name(pipe, i), pipe_name(pipe));
+			     sprite_name(pipe, sprite), pipe_name(pipe));
 		}
 	} else if (INTEL_INFO(dev)->gen >= 7) {
 		reg = SPRCTL(pipe);
@@ -10997,7 +10997,7 @@ void intel_modeset_suspend_hw(struct drm_device *dev)
 void intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int j, ret;
+	int sprite, ret;
 	enum pipe pipe;
 
 	drm_mode_config_init(dev);
@@ -11037,11 +11037,11 @@ void intel_modeset_init(struct drm_device *dev)
 
 	for_each_pipe(pipe) {
 		intel_crtc_init(dev, pipe);
-		for (j = 0; j < INTEL_INFO(dev)->num_sprites; j++) {
-			ret = intel_plane_init(dev, pipe, j);
+		for_each_sprite(pipe, sprite) {
+			ret = intel_plane_init(dev, pipe, sprite);
 			if (ret)
 				DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
-					      pipe_name(pipe), sprite_name(pipe, j), ret);
+					      pipe_name(pipe), sprite_name(pipe, sprite), ret);
 		}
 	}
 
-- 
1.8.3.1

[PATCH 5/5] drm/i915: Make num_sprites a per-pipe value

[Damien Lespiau]

In the future, we need to be able to specify per-pipe number of
planes/sprites. Let's start today!

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 8 ++++++--
 drivers/gpu/drm/i915/i915_drv.h | 6 +++---
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7688abc..899f2ae 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1480,12 +1480,16 @@ static void intel_device_info_runtime_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_device_info *info;
+	enum pipe pipe;
 
 	info = (struct intel_device_info *)&dev_priv->info;
 
-	info->num_sprites = 1;
 	if (IS_VALLEYVIEW(dev))
-		info->num_sprites = 2;
+		for_each_pipe(pipe)
+			info->num_sprites[pipe] = 2;
+	else
+		for_each_pipe(pipe)
+			info->num_sprites[pipe] = 1;
 
 	if (i915.disable_display) {
 		DRM_INFO("Display disabled (module parameter)\n");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ad51a0e..7cd0941 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -79,7 +79,7 @@ enum plane {
 };
 #define plane_name(p) ((p) + 'A')
 
-#define sprite_name(p, s) ((p) * INTEL_INFO(dev)->num_sprites + (s) + 'A')
+#define sprite_name(p, s) ((p) * INTEL_INFO(dev)->num_sprites[(p)] + (s) + 'A')
 
 enum port {
 	PORT_A = 0,
@@ -159,7 +159,7 @@ enum hpd_pin {
 	 I915_GEM_DOMAIN_VERTEX)
 
 #define for_each_pipe(p) for ((p) = 0; (p) < INTEL_INFO(dev)->num_pipes; (p)++)
-#define for_each_sprite(p, s) for ((s) = 0; (s) < INTEL_INFO(dev)->num_sprites; (s)++)
+#define for_each_sprite(p, s) for ((s) = 0; (s) < INTEL_INFO(dev)->num_sprites[(p)]; (s)++)
 
 #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
 	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
@@ -535,7 +535,7 @@ struct intel_uncore {
 struct intel_device_info {
 	u32 display_mmio_offset;
 	u8 num_pipes:3;
-	u8 num_sprites:2;
+	u8 num_sprites[I915_MAX_PIPES];
 	u8 gen;
 	u8 ring_mask; /* Rings supported by the HW */
 	DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON);
-- 
1.8.3.1

Re: [PATCH 0/5] Make num_sprites a per pipe value (v3)

[Chris Wilson]

On Mon, Mar 03, 2014 at 05:31:43PM +0000, Damien Lespiau wrote:
> Follow up of:
>   http://lists.freedesktop.org/archives/intel-gfx/2014-February/040789.html
> 
> With a couple of changes:
> 
>   - Fix up the poor style when initializing the per-pipe num_sprites
>   - Added more for_each_pipe(pipe) conversion
>   - Added a small cleanup patch spotted when doing the previous patch

Nothing upset me (apart from the concept of variable number of planes
per pipe and the inflexible assumption that it is constant in certain
client facing APIs), not even the expansion of intel_info, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/5] Make num_sprites a per pipe value (v3)

[Daniel Vetter]

On Mon, Mar 03, 2014 at 09:16:38PM +0000, Chris Wilson wrote:
> On Mon, Mar 03, 2014 at 05:31:43PM +0000, Damien Lespiau wrote:
> > Follow up of:
> >   http://lists.freedesktop.org/archives/intel-gfx/2014-February/040789.html
> > 
> > With a couple of changes:
> > 
> >   - Fix up the poor style when initializing the per-pipe num_sprites
> >   - Added more for_each_pipe(pipe) conversion
> >   - Added a small cleanup patch spotted when doing the previous patch
> 
> Nothing upset me (apart from the concept of variable number of planes
> per pipe and the inflexible assumption that it is constant in certain
> client facing APIs), not even the expansion of intel_info, so
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

All pulled in, thanks for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch