[PATCH 0/3] Make num_sprites a per pipe value (v2)

[PATCH 0/3] Make num_sprites a per pipe value (v2)

[Damien Lespiau]

In the future we want to be able to specify a per-pipe number of sprites. This
series introduces this with a bit of refactoring first to, hopefully, don't
make the same mistake that v1 did again.

-- 
Damien

Damien Lespiau (3):
  drm/i915: Use a pipe variable to cycle trough the pipes
  drm/i915: Add a for_each_sprite() macro
  drm/i915: Make num_sprites a per-pipe value

 drivers/gpu/drm/i915/i915_dma.c      | 14 +++++++++++---
 drivers/gpu/drm/i915/i915_drv.h      |  5 +++--
 drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++----------
 3 files changed, 25 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] drm/i915: Use a pipe variable to cycle trough 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

Re: [PATCH 1/3] drm/i915: Use a pipe variable to cycle trough the pipes

[Chris Wilson]

On Fri, Feb 28, 2014 at 04:42:13PM +0000, Damien Lespiau wrote:
> 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>

I'd review patches to change all for_each_pipe(i) to
for_each_pipe(pipe).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/3] 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

Re: [PATCH 2/3] drm/i915: Add a for_each_sprite() macro

[Chris Wilson]

On Fri, Feb 28, 2014 at 04:42:14PM +0000, Damien Lespiau wrote:
> This macro is similar to for_each_pipe() we already have. Convert the
> two call sites we have at the same time.

Have you considered adding the enum sprite? One day sparse will give us
warnings when we screw up.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/3] 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 | 14 +++++++++++---
 drivers/gpu/drm/i915/i915_drv.h |  6 +++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7688abc..a128f66 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1483,9 +1483,17 @@ static void intel_device_info_runtime_init(struct drm_device *dev)
 
 	info = (struct intel_device_info *)&dev_priv->info;
 
-	info->num_sprites = 1;
-	if (IS_VALLEYVIEW(dev))
-		info->num_sprites = 2;
+	if (IS_VALLEYVIEW(dev)) {
+		info->num_sprites[PIPE_A] = 2;
+		info->num_sprites[PIPE_B] = 2;
+	} else {
+		if (info->num_pipes >= 1)
+			info->num_sprites[PIPE_A] = 1;
+		if (info->num_pipes >= 2)
+			info->num_sprites[PIPE_B] = 1;
+		if (info->num_pipes >= 3)
+			info->num_sprites[PIPE_C] = 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 3/3] drm/i915: Make num_sprites a per-pipe value

[Chris Wilson]

On Fri, Feb 28, 2014 at 04:42:15PM +0000, Damien Lespiau wrote:
> In the future, we need to be able to specify per-pipe number of
> planes/sprites. Let's start today!

But today, what's wrong with:
for_each_pipe(pipe) info->num_sprites[pipe] = IS_VLV(dev) ? 2 : 1;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Damien Lespiau]

On Fri, Feb 28, 2014 at 05:30:02PM +0000, Chris Wilson wrote:
> On Fri, Feb 28, 2014 at 04:42:15PM +0000, Damien Lespiau wrote:
> > In the future, we need to be able to specify per-pipe number of
> > planes/sprites. Let's start today!
> 
> But today, what's wrong with:
> for_each_pipe(pipe) info->num_sprites[pipe] = IS_VLV(dev) ? 2 : 1;

I'd rather have info->num_sprites[pipe] == 0 when the device doesn't
support that pipe, even if that value shouldn't be used anywhere.

-- 
Damien

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

[Chris Wilson]

On Fri, Feb 28, 2014 at 05:37:39PM +0000, Damien Lespiau wrote:
> On Fri, Feb 28, 2014 at 05:30:02PM +0000, Chris Wilson wrote:
> > On Fri, Feb 28, 2014 at 04:42:15PM +0000, Damien Lespiau wrote:
> > > In the future, we need to be able to specify per-pipe number of
> > > planes/sprites. Let's start today!
> > 
> > But today, what's wrong with:
> > for_each_pipe(pipe) info->num_sprites[pipe] = IS_VLV(dev) ? 2 : 1;
> 
> I'd rather have info->num_sprites[pipe] == 0 when the device doesn't
> support that pipe, even if that value shouldn't be used anywhere.

for (; pipe < I915_NUM_PIPES; pipe++) info->num_sprites[pipe] = 0;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Ville Syrjälä]

On Fri, Feb 28, 2014 at 05:49:20PM +0000, Chris Wilson wrote:
> On Fri, Feb 28, 2014 at 05:37:39PM +0000, Damien Lespiau wrote:
> > On Fri, Feb 28, 2014 at 05:30:02PM +0000, Chris Wilson wrote:
> > > On Fri, Feb 28, 2014 at 04:42:15PM +0000, Damien Lespiau wrote:
> > > > In the future, we need to be able to specify per-pipe number of
> > > > planes/sprites. Let's start today!
> > > 
> > > But today, what's wrong with:
> > > for_each_pipe(pipe) info->num_sprites[pipe] = IS_VLV(dev) ? 2 : 1;
> > 
> > I'd rather have info->num_sprites[pipe] == 0 when the device doesn't
> > support that pipe, even if that value shouldn't be used anywhere.
> 
> for (; pipe < I915_NUM_PIPES; pipe++) info->num_sprites[pipe] = 0;

It should be zero initialized anyway, so we shouldn't need to do that.

-- 
Ville Syrjälä
Intel OTC

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

[Damien Lespiau]

On Fri, Feb 28, 2014 at 05:49:20PM +0000, Chris Wilson wrote:
> On Fri, Feb 28, 2014 at 05:37:39PM +0000, Damien Lespiau wrote:
> > On Fri, Feb 28, 2014 at 05:30:02PM +0000, Chris Wilson wrote:
> > > On Fri, Feb 28, 2014 at 04:42:15PM +0000, Damien Lespiau wrote:
> > > > In the future, we need to be able to specify per-pipe number of
> > > > planes/sprites. Let's start today!
> > > 
> > > But today, what's wrong with:
> > > for_each_pipe(pipe) info->num_sprites[pipe] = IS_VLV(dev) ? 2 : 1;
> > 
> > I'd rather have info->num_sprites[pipe] == 0 when the device doesn't
> > support that pipe, even if that value shouldn't be used anywhere.
> 
> for (; pipe < I915_NUM_PIPES; pipe++) info->num_sprites[pipe] = 0;
> -Chris

Would:

        if (IS_VALLEYVIEW(dev)) 
                for_each_pipe(pipe)
                        info->num_sprites[pipe] = 2;
        else 
                for_each_pipe(pipe)
                        info->num_sprites[pipe] = 1;

be acceptable to your keen eye?

-- 
Damien

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

[Chris Wilson]

On Mon, Mar 03, 2014 at 02:49:36PM +0000, Damien Lespiau wrote:
> On Fri, Feb 28, 2014 at 05:49:20PM +0000, Chris Wilson wrote:
> > On Fri, Feb 28, 2014 at 05:37:39PM +0000, Damien Lespiau wrote:
> > > On Fri, Feb 28, 2014 at 05:30:02PM +0000, Chris Wilson wrote:
> > > > On Fri, Feb 28, 2014 at 04:42:15PM +0000, Damien Lespiau wrote:
> > > > > In the future, we need to be able to specify per-pipe number of
> > > > > planes/sprites. Let's start today!
> > > > 
> > > > But today, what's wrong with:
> > > > for_each_pipe(pipe) info->num_sprites[pipe] = IS_VLV(dev) ? 2 : 1;
> > > 
> > > I'd rather have info->num_sprites[pipe] == 0 when the device doesn't
> > > support that pipe, even if that value shouldn't be used anywhere.
> > 
> > for (; pipe < I915_NUM_PIPES; pipe++) info->num_sprites[pipe] = 0;
> > -Chris
> 
> Would:
> 
>         if (IS_VALLEYVIEW(dev)) 
>                 for_each_pipe(pipe)
>                         info->num_sprites[pipe] = 2;
>         else 
>                 for_each_pipe(pipe)
>                         info->num_sprites[pipe] = 1;
> 
> be acceptable to your keen eye?

Only because I strongly suspect the next branch to not be as clean.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Zhenyu Wang]

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

On 2014.02.28 16:42:15 +0000, Damien Lespiau wrote:
> In the future, we need to be able to specify per-pipe number of
> planes/sprites. Let's start today!
> 

The number of sprite planes on each pipe is always fixed in our HW.
Do we really need this in the future?

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

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

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

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