[PATCH 0/2] Make for_each_{plane, sprite} take dev_priv as first argument

[PATCH 0/2] Make for_each_{plane, sprite} take dev_priv as first argument

[Damien Lespiau]

When upstreaming the SKL WM series I didn't really want to go, again, rework
the patches for a small change like that. I promissed I'd get around to do it
though and sounds about the right time.

-- 
Damien

Damien Lespiau (2):
  drm/i915: Make for_each_plane() take dev_priv as argument
  drm/i915: Make for_each_sprite() take dev_priv as argument

 drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
 drivers/gpu/drm/i915/i915_drv.h      | 11 ++++++++---
 drivers/gpu/drm/i915/intel_display.c |  8 ++++----
 drivers/gpu/drm/i915/intel_pm.c      |  9 ++++-----
 4 files changed, 17 insertions(+), 13 deletions(-)

-- 
1.8.3.1

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

[PATCH 1/2] drm/i915: Make for_each_plane() take dev_priv as argument

[Damien Lespiau]

Implicit usage of local variables in macros isn't exactly the greatest
thing in the world, especially when that variable is the drm device and
we want to move towards a broader use of the i915 device structure.

Let's make for_each_plane() take dev_priv as its first argument then.

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  | 2 +-
 drivers/gpu/drm/i915/i915_drv.h      | 6 ++++--
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 9 ++++-----
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e4d5f41..6717052 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2854,7 +2854,7 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
 	for_each_pipe(dev_priv, pipe) {
 		seq_printf(m, "Pipe %c\n", pipe_name(pipe));
 
-		for_each_plane(pipe, plane) {
+		for_each_plane(dev_priv, pipe, plane) {
 			entry = &ddb->plane[pipe][plane];
 			seq_printf(m, "  Plane%-8d%8u%8u%8u\n", plane + 1,
 				   entry->start, entry->end,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e07a1cb..c204e30 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -223,8 +223,10 @@ enum hpd_pin {
 
 #define for_each_pipe(__dev_priv, __p) \
 	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
-#define for_each_plane(pipe, p) \
-	for ((p) = 0; (p) < INTEL_INFO(dev)->num_sprites[(pipe)] + 1; (p)++)
+#define for_each_plane(__dev_priv, __pipe, __p)				\
+	for ((__p) = 0;							\
+	     (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1;	\
+	     (__p)++)
 #define for_each_sprite(p, s) for ((s) = 0; (s) < INTEL_INFO(dev)->num_sprites[(p)]; (s)++)
 
 #define for_each_crtc(dev, crtc) \
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3156d77..23c3a37 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10806,7 +10806,7 @@ static void check_wm_state(struct drm_device *dev)
 			continue;
 
 		/* planes */
-		for_each_plane(pipe, plane) {
+		for_each_plane(dev_priv, pipe, plane) {
 			hw_entry = &hw_ddb.plane[pipe][plane];
 			sw_entry = &sw_ddb->plane[pipe][plane];
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0bf6767..e710b43 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2463,13 +2463,12 @@ static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry, u32 reg)
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */)
 {
-	struct drm_device *dev = dev_priv->dev;
 	enum pipe pipe;
 	int plane;
 	u32 val;
 
 	for_each_pipe(dev_priv, pipe) {
-		for_each_plane(pipe, plane) {
+		for_each_plane(dev_priv, pipe, plane) {
 			val = I915_READ(PLANE_BUF_CFG(pipe, plane));
 			skl_ddb_entry_init_from_hw(&ddb->plane[pipe][plane],
 						   val);
@@ -2518,6 +2517,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 		      struct skl_ddb_allocation *ddb /* out */)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
@@ -2542,7 +2542,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 	alloc->end -= cursor_blocks;
 
 	/* 1. Allocate the mininum required blocks for each active plane */
-	for_each_plane(pipe, plane) {
+	for_each_plane(dev_priv, pipe, plane) {
 		const struct intel_plane_wm_parameters *p;
 
 		p = &params->plane[plane];
@@ -2996,12 +2996,11 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 static void
 skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, int pass)
 {
-	struct drm_device *dev = dev_priv->dev;
 	int plane;
 
 	DRM_DEBUG_KMS("flush pipe %c (pass %d)\n", pipe_name(pipe), pass);
 
-	for_each_plane(pipe, plane) {
+	for_each_plane(dev_priv, pipe, plane) {
 		I915_WRITE(PLANE_SURF(pipe, plane),
 			   I915_READ(PLANE_SURF(pipe, plane)));
 	}
-- 
1.8.3.1

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

Re: [PATCH 1/2] drm/i915: Make for_each_plane() take dev_priv as argument

[Chris Wilson]

On Sat, Feb 28, 2015 at 02:54:08PM +0000, Damien Lespiau wrote:
> Implicit usage of local variables in macros isn't exactly the greatest
> thing in the world, especially when that variable is the drm device and
> we want to move towards a broader use of the i915 device structure.
> 
> Let's make for_each_plane() take dev_priv as its first argument then.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Both patches are Reviewed-by: Chris Wilson <chris-wilson.co.uk>

Just a quick question...

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e07a1cb..c204e30 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -223,8 +223,10 @@ enum hpd_pin {
>  
>  #define for_each_pipe(__dev_priv, __p) \
>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
> -#define for_each_plane(pipe, p) \
> -	for ((p) = 0; (p) < INTEL_INFO(dev)->num_sprites[(pipe)] + 1; (p)++)
> +#define for_each_plane(__dev_priv, __pipe, __p)				\
> +	for ((__p) = 0;							\
> +	     (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1;	\
> +	     (__p)++)
>  #define for_each_sprite(p, s) for ((s) = 0; (s) < INTEL_INFO(dev)->num_sprites[(p)]; (s)++)

Is num_sprites explained anywhere? In particular the plane = num_sprites+1?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Make for_each_plane() take dev_priv as argument

[Damien Lespiau]

On Sat, Feb 28, 2015 at 09:04:44PM +0000, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index e07a1cb..c204e30 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -223,8 +223,10 @@ enum hpd_pin {
> >  
> >  #define for_each_pipe(__dev_priv, __p) \
> >  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
> > -#define for_each_plane(pipe, p) \
> > -	for ((p) = 0; (p) < INTEL_INFO(dev)->num_sprites[(pipe)] + 1; (p)++)
> > +#define for_each_plane(__dev_priv, __pipe, __p)				\
> > +	for ((__p) = 0;							\
> > +	     (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1;	\
> > +	     (__p)++)
> >  #define for_each_sprite(p, s) for ((s) = 0; (s) < INTEL_INFO(dev)->num_sprites[(p)]; (s)++)
> 
> Is num_sprites explained anywhere? In particular the plane = num_sprites+1?
> -Chris

There's a tentative explanation also talking about the cursor plane:

/*
 * This is the maximum (across all platforms) number of planes (primary +
 * sprites) that can be active at the same time on one pipe.
 *      
 * This value doesn't count the cursor plane.
 */     
#define I915_MAX_PLANES 3

But agreed that this couple be improved.

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

[PATCH 2/2] drm/i915: Make for_each_sprite() take dev_priv as argument

[Damien Lespiau]

Implicit usage of local variables in macros isn't exactly the greatest
thing in the world, especially when that variable is the drm device and
we want to move towards a broader use of the i915 device structure.

Let's make for_each_sprite() take dev_priv as its first argument then.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 5 ++++-
 drivers/gpu/drm/i915/intel_display.c | 6 +++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c204e30..92f8300 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -227,7 +227,10 @@ enum hpd_pin {
 	for ((__p) = 0;							\
 	     (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1;	\
 	     (__p)++)
-#define for_each_sprite(p, s) for ((s) = 0; (s) < INTEL_INFO(dev)->num_sprites[(p)]; (s)++)
+#define for_each_sprite(__dev_priv, __p, __s)				\
+	for ((__s) = 0;							\
+	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)];	\
+	     (__s)++)
 
 #define for_each_crtc(dev, crtc) \
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 23c3a37..589addf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1300,14 +1300,14 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
 	u32 val;
 
 	if (INTEL_INFO(dev)->gen >= 9) {
-		for_each_sprite(pipe, sprite) {
+		for_each_sprite(dev_priv, pipe, sprite) {
 			val = I915_READ(PLANE_CTL(pipe, sprite));
 			I915_STATE_WARN(val & PLANE_CTL_ENABLE,
 			     "plane %d assertion failure, should be off on pipe %c but is still active\n",
 			     sprite, pipe_name(pipe));
 		}
 	} else if (IS_VALLEYVIEW(dev)) {
-		for_each_sprite(pipe, sprite) {
+		for_each_sprite(dev_priv, pipe, sprite) {
 			reg = SPCNTR(pipe, sprite);
 			val = I915_READ(reg);
 			I915_STATE_WARN(val & SP_ENABLE,
@@ -13360,7 +13360,7 @@ void intel_modeset_init(struct drm_device *dev)
 
 	for_each_pipe(dev_priv, pipe) {
 		intel_crtc_init(dev, pipe);
-		for_each_sprite(pipe, sprite) {
+		for_each_sprite(dev_priv, pipe, sprite) {
 			ret = intel_plane_init(dev, pipe, sprite);
 			if (ret)
 				DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
-- 
1.8.3.1

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

Re: [PATCH 2/2] drm/i915: Make for_each_sprite() take dev_priv as argument

[Daniel Vetter]

On Sat, Feb 28, 2015 at 02:54:09PM +0000, Damien Lespiau wrote:
> Implicit usage of local variables in macros isn't exactly the greatest
> thing in the world, especially when that variable is the drm device and
> we want to move towards a broader use of the i915 device structure.
> 
> Let's make for_each_sprite() take dev_priv as its first argument then.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Make for_each_sprite() take dev_priv as argument

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5866
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -9              278/278              269/278
ILK                                  308/308              308/308
SNB                                  284/284              284/284
IVB                                  380/380              380/380
BYT                                  294/294              294/294
HSW                                  387/387              387/387
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_fence_thrash_bo-write-verify-none      PASS(4)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-x      PASS(4)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-y      PASS(4)      FAIL(1)PASS(1)
 PNV  igt_gem_userptr_blits_coherency-sync      CRASH(3)PASS(6)      CRASH(2)
 PNV  igt_gem_userptr_blits_coherency-unsync      CRASH(2)PASS(5)      CRASH(2)
 PNV  igt_gen3_render_linear_blits      FAIL(3)PASS(5)      FAIL(2)
 PNV  igt_gen3_render_mixed_blits      FAIL(2)PASS(8)      FAIL(2)
 PNV  igt_gem_fence_thrash_bo-write-verify-threaded-none      FAIL(2)CRASH(1)PASS(3)      CRASH(1)PASS(1)
*PNV  igt_gem_partial_pwrite_pread_reads      PASS(4)      CRASH(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(12)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx