[PATCH] drm/i915: Clean up intel_plane->plane usage

[PATCH] drm/i915: Clean up intel_plane->plane usage

[Matt Roper]

intel_plane->plane has inconsistent meanings across the driver:
 * For primary planes, intel_plane->plane is usually the pipe number
   (except for old platforms where it had to be swapped for FBC
   reasons).
 * For sprite planes, intel_plane->plane represents the sprite index for
   the specific CRTC the sprite belongs to (0, 1, 2, ...)
 * For cursor planes, intel_plane->plane is again the pipe number,
   but without the FBC swapping on old platforms.

This overloading of the field can be quite confusing and easily leads to
bugs due to differences in how the hardware actually gets programmed
(e.g., SKL code needs to use p->plane + 1 because the hardware expects
universal plane ID's that include the primary, whereas VLV code needs to
use just p->plane because hardware expects a sprite index that does not
include the primary).

Let's try to clean up this situation by giving intel_plane->plane a
consistent meaning across all plane types, and then update all uses
across driver code to use macros to interpret it properly.  The
following macros should be used in the code where we previously accessed
p->plane and will do additional plane type checking to help prevent
misuse:
 * I915_UNIVERSAL_NUM(p) --- Universal plane ID (primary = 0, sprites
   start from 1); intended for use in gen9+ code.
 * I915_I915_PRIMARY_NUM(p) --- Primary plane ID for pre-gen9 platforms;
   determines whether FBC-related swapping is needed or not.
 * I915_SPRITE_NUM(p) --- Sprite plane ID (first sprite = 0); intended
   for use in pre-gen9 code.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 24 ++++++++++++++++++------
 drivers/gpu/drm/i915/intel_display.c | 12 ++++--------
 drivers/gpu/drm/i915/intel_pm.c      | 10 +++++-----
 drivers/gpu/drm/i915/intel_sprite.c  | 13 +++++++------
 4 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3bf8a9b..132fe53 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -131,22 +131,34 @@ enum transcoder {
 #define transcoder_name(t) ((t) + 'A')
 
 /*
- * 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.
+ * I915_MAX_PLANES in the enum below is the maximum (across all platforms)
+ * number of planes per CRTC.  Not all platforms really have this many planes,
+ * which means some arrays of size I915_MAX_PLANES may have unused entries
+ * between the topmost sprite plane and the cursor plane.
  */
-#define I915_MAX_PLANES	4
-
 enum plane {
 	PLANE_A = 0,
+	PLANE_PRIMARY = PLANE_A,
 	PLANE_B,
 	PLANE_C,
+	PLANE_CURSOR,
+	I915_MAX_PLANES,
 };
 #define plane_name(p) ((p) + 'A')
 
 #define sprite_name(p, s) ((p) * INTEL_INFO(dev)->num_sprites[(p)] + (s) + 'A')
 
+#define I915_UNIVERSAL_NUM(p) ( \
+	WARN_ON(p->base.type == DRM_PLANE_TYPE_CURSOR), \
+	p->plane)
+#define I915_PRIMARY_NUM(p) ( \
+	WARN_ON(p->base.type != DRM_PLANE_TYPE_PRIMARY), \
+	(HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) ? \
+			      !p->pipe : p->pipe)
+#define I915_SPRITE_NUM(p) ( \
+	WARN_ON(p->base.type != DRM_PLANE_TYPE_OVERLAY), \
+	p->plane - 1)
+
 enum port {
 	PORT_A = 0,
 	PORT_B,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fc00867..dab0b71 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11976,16 +11976,14 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 			DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d "
 				"disabled, scaler_id = %d\n",
 				plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
-				plane->base.id, intel_plane->pipe,
-				(crtc->base.primary == plane) ? 0 : intel_plane->plane + 1,
+				plane->base.id, intel_plane->pipe, intel_plane->plane,
 				drm_plane_index(plane), state->scaler_id);
 			continue;
 		}
 
 		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled",
 			plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
-			plane->base.id, intel_plane->pipe,
-			crtc->base.primary == plane ? 0 : intel_plane->plane + 1,
+			plane->base.id, intel_plane->pipe, intel_plane->plane,
 			drm_plane_index(plane));
 		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x",
 			fb->base.id, fb->width, fb->height, fb->pixel_format);
@@ -13547,13 +13545,11 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 		state->scaler_id = -1;
 	}
 	primary->pipe = pipe;
-	primary->plane = pipe;
+	primary->plane = 0;
 	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
 	primary->check_plane = intel_check_primary_plane;
 	primary->commit_plane = intel_commit_primary_plane;
 	primary->disable_plane = intel_disable_primary_plane;
-	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
-		primary->plane = !pipe;
 
 	if (INTEL_INFO(dev)->gen >= 9) {
 		intel_primary_formats = skl_primary_formats;
@@ -13699,7 +13695,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	cursor->can_scale = false;
 	cursor->max_downscale = 1;
 	cursor->pipe = pipe;
-	cursor->plane = pipe;
+	cursor->plane = PLANE_CURSOR;
 	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
 	cursor->check_plane = intel_check_cursor_plane;
 	cursor->commit_plane = intel_commit_cursor_plane;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 62de97e..9db19f2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1131,7 +1131,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
 					wm_state->wm[level].primary;
 				break;
 			case DRM_PLANE_TYPE_OVERLAY:
-				sprite = plane->plane;
+				sprite = I915_SPRITE_NUM(plane);
 				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
 					wm_state->wm[level].sprite[sprite];
 				break;
@@ -1195,7 +1195,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 				wm_state->wm[level].primary = wm;
 				break;
 			case DRM_PLANE_TYPE_OVERLAY:
-				sprite = plane->plane;
+				sprite = I915_SPRITE_NUM(plane);
 				wm_state->wm[level].sprite[sprite] = wm;
 				break;
 			}
@@ -1221,7 +1221,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 					    wm_state->wm[level].primary);
 			break;
 		case DRM_PLANE_TYPE_OVERLAY:
-			sprite = plane->plane;
+			sprite = I915_SPRITE_NUM(plane);
 			for (level = 0; level < wm_state->num_levels; level++)
 				wm_state->sr[level].plane =
 					min(wm_state->sr[level].plane,
@@ -1257,7 +1257,7 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
 
 		if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
 			sprite0_start = plane->wm.fifo_size;
-		else if (plane->plane == 0)
+		else if (I915_SPRITE_NUM(plane) == 0)
 			sprite1_start = sprite0_start + plane->wm.fifo_size;
 		else
 			fifo_size = sprite1_start + plane->wm.fifo_size;
@@ -4076,7 +4076,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, 0);
 			break;
 		case DRM_PLANE_TYPE_OVERLAY:
-			sprite = plane->plane;
+			sprite = I915_SPRITE_NUM(plane);
 			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, sprite + 1);
 			break;
 		}
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 4d27243..11ca40a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -180,7 +180,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	const int pipe = intel_plane->pipe;
-	const int plane = intel_plane->plane + 1;
+	const int plane = I915_UNIVERSAL_NUM(intel_plane);
 	u32 plane_ctl, stride_div, stride;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
 	const struct drm_intel_sprite_colorkey *key =
@@ -280,7 +280,7 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
 	const int pipe = intel_plane->pipe;
-	const int plane = intel_plane->plane + 1;
+	const int plane = I915_UNIVERSAL_NUM(intel_plane);
 
 	I915_WRITE(PLANE_CTL(pipe, plane), 0);
 
@@ -294,7 +294,7 @@ static void
 chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
 {
 	struct drm_i915_private *dev_priv = intel_plane->base.dev->dev_private;
-	int plane = intel_plane->plane;
+	int plane = I915_SPRITE_NUM(intel_plane);
 
 	/* Seems RGB data bypasses the CSC always */
 	if (!format_is_yuv(format))
@@ -342,7 +342,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int pipe = intel_plane->pipe;
-	int plane = intel_plane->plane;
+	int plane = I915_SPRITE_NUM(intel_plane);
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
@@ -461,7 +461,7 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
 	int pipe = intel_plane->pipe;
-	int plane = intel_plane->plane;
+	int plane = I915_SPRITE_NUM(intel_plane);
 
 	I915_WRITE(SPCNTR(pipe, plane), 0);
 
@@ -1121,8 +1121,9 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 		return -ENODEV;
 	}
 
+	/* primary = 0, sprites start from 1 */
+	intel_plane->plane = plane + 1;
 	intel_plane->pipe = pipe;
-	intel_plane->plane = plane;
 	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane);
 	intel_plane->check_plane = intel_check_sprite_plane;
 	intel_plane->commit_plane = intel_commit_sprite_plane;
-- 
2.1.4

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

Re: [PATCH] drm/i915: Clean up intel_plane->plane usage

[Ville Syrjälä]

On Thu, Sep 17, 2015 at 06:55:21PM -0700, Matt Roper wrote:
> intel_plane->plane has inconsistent meanings across the driver:
>  * For primary planes, intel_plane->plane is usually the pipe number
>    (except for old platforms where it had to be swapped for FBC
>    reasons).
>  * For sprite planes, intel_plane->plane represents the sprite index for
>    the specific CRTC the sprite belongs to (0, 1, 2, ...)
>  * For cursor planes, intel_plane->plane is again the pipe number,
>    but without the FBC swapping on old platforms.
> 
> This overloading of the field can be quite confusing and easily leads to
> bugs due to differences in how the hardware actually gets programmed
> (e.g., SKL code needs to use p->plane + 1 because the hardware expects
> universal plane ID's that include the primary, whereas VLV code needs to
> use just p->plane because hardware expects a sprite index that does not
> include the primary).
> 
> Let's try to clean up this situation by giving intel_plane->plane a
> consistent meaning across all plane types, and then update all uses
> across driver code to use macros to interpret it properly.  The
> following macros should be used in the code where we previously accessed
> p->plane and will do additional plane type checking to help prevent
> misuse:
>  * I915_UNIVERSAL_NUM(p) --- Universal plane ID (primary = 0, sprites
>    start from 1); intended for use in gen9+ code.
>  * I915_I915_PRIMARY_NUM(p) --- Primary plane ID for pre-gen9 platforms;
>    determines whether FBC-related swapping is needed or not.
>  * I915_SPRITE_NUM(p) --- Sprite plane ID (first sprite = 0); intended
>    for use in pre-gen9 code.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      | 24 ++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_display.c | 12 ++++--------
>  drivers/gpu/drm/i915/intel_pm.c      | 10 +++++-----
>  drivers/gpu/drm/i915/intel_sprite.c  | 13 +++++++------
>  4 files changed, 34 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3bf8a9b..132fe53 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -131,22 +131,34 @@ enum transcoder {
>  #define transcoder_name(t) ((t) + 'A')
>  
>  /*
> - * 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.
> + * I915_MAX_PLANES in the enum below is the maximum (across all platforms)
> + * number of planes per CRTC.  Not all platforms really have this many planes,
> + * which means some arrays of size I915_MAX_PLANES may have unused entries
> + * between the topmost sprite plane and the cursor plane.
>   */
> -#define I915_MAX_PLANES	4
> -
>  enum plane {
>  	PLANE_A = 0,
> +	PLANE_PRIMARY = PLANE_A,
>  	PLANE_B,
>  	PLANE_C,
> +	PLANE_CURSOR,
> +	I915_MAX_PLANES,
>  };

If we're really going to redefine this as a per-pipe thing, then I
think we'd need to rename it. A,B,C have a definite meaning on most of
our hardware, so using it for something else is confusing.

I know that looking for enum plane in the code probably won't give you
a lot of hits, but that's just because we suck and usually use int or
something. I have a patch series in a branch somewhere to clean that
stuff up, but I don't think I posted it to the list so far.

I think it would be better to have a separate per-pipe plane enum.
enum plane_id or something? And maybe store it as plane->id.

>  #define plane_name(p) ((p) + 'A')
>  
>  #define sprite_name(p, s) ((p) * INTEL_INFO(dev)->num_sprites[(p)] + (s) + 'A')
>  
> +#define I915_UNIVERSAL_NUM(p) ( \
> +	WARN_ON(p->base.type == DRM_PLANE_TYPE_CURSOR), \

I don't think that'll fly with SKL. The cursor is just another universal
plane, or well, will be. I think the patch to do that change got stuck
in review or something.

I guess that should be easily fixable by tossing in a !IS_SKL&& once
that patch lands.

> +	p->plane)
> +#define I915_PRIMARY_NUM(p) ( \
> +	WARN_ON(p->base.type != DRM_PLANE_TYPE_PRIMARY), \
> +	(HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) ? \
> +			      !p->pipe : p->pipe)

I'm not sure we want to do this swapping here.

Maybe we'll just want a some kind of
enum plane plane_id_to_plane(crtc, enum plane_id) function.
Or we could just store the enum plane alongside the plane_id in the
plane, and make it clear that it should only be used by pre-SKL
platforms. In theory we could limit it to pre-gen4, or just gmch, but
I think we probably want to share a bunch of the primary plane code
all the way up to BDW, so allowing it's use there seems sane to me.

> +#define I915_SPRITE_NUM(p) ( \
> +	WARN_ON(p->base.type != DRM_PLANE_TYPE_OVERLAY), \
> +	p->plane - 1)
> +
>  enum port {
>  	PORT_A = 0,
>  	PORT_B,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fc00867..dab0b71 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11976,16 +11976,14 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  			DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d "
>  				"disabled, scaler_id = %d\n",
>  				plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
> -				plane->base.id, intel_plane->pipe,
> -				(crtc->base.primary == plane) ? 0 : intel_plane->plane + 1,
> +				plane->base.id, intel_plane->pipe, intel_plane->plane,
>  				drm_plane_index(plane), state->scaler_id);
>  			continue;
>  		}
>  
>  		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled",
>  			plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
> -			plane->base.id, intel_plane->pipe,
> -			crtc->base.primary == plane ? 0 : intel_plane->plane + 1,
> +			plane->base.id, intel_plane->pipe, intel_plane->plane,
>  			drm_plane_index(plane));
>  		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x",
>  			fb->base.id, fb->width, fb->height, fb->pixel_format);
> @@ -13547,13 +13545,11 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  		state->scaler_id = -1;
>  	}
>  	primary->pipe = pipe;
> -	primary->plane = pipe;
> +	primary->plane = 0;
>  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
>  	primary->check_plane = intel_check_primary_plane;
>  	primary->commit_plane = intel_commit_primary_plane;
>  	primary->disable_plane = intel_disable_primary_plane;
> -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> -		primary->plane = !pipe;
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		intel_primary_formats = skl_primary_formats;
> @@ -13699,7 +13695,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  	cursor->can_scale = false;
>  	cursor->max_downscale = 1;
>  	cursor->pipe = pipe;
> -	cursor->plane = pipe;
> +	cursor->plane = PLANE_CURSOR;
>  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
>  	cursor->check_plane = intel_check_cursor_plane;
>  	cursor->commit_plane = intel_commit_cursor_plane;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 62de97e..9db19f2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1131,7 +1131,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
>  					wm_state->wm[level].primary;
>  				break;
>  			case DRM_PLANE_TYPE_OVERLAY:
> -				sprite = plane->plane;
> +				sprite = I915_SPRITE_NUM(plane);
>  				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
>  					wm_state->wm[level].sprite[sprite];
>  				break;
> @@ -1195,7 +1195,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  				wm_state->wm[level].primary = wm;
>  				break;
>  			case DRM_PLANE_TYPE_OVERLAY:
> -				sprite = plane->plane;
> +				sprite = I915_SPRITE_NUM(plane);
>  				wm_state->wm[level].sprite[sprite] = wm;
>  				break;
>  			}
> @@ -1221,7 +1221,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  					    wm_state->wm[level].primary);
>  			break;
>  		case DRM_PLANE_TYPE_OVERLAY:
> -			sprite = plane->plane;
> +			sprite = I915_SPRITE_NUM(plane);
>  			for (level = 0; level < wm_state->num_levels; level++)
>  				wm_state->sr[level].plane =
>  					min(wm_state->sr[level].plane,
> @@ -1257,7 +1257,7 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
>  
>  		if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
>  			sprite0_start = plane->wm.fifo_size;
> -		else if (plane->plane == 0)
> +		else if (I915_SPRITE_NUM(plane) == 0)
>  			sprite1_start = sprite0_start + plane->wm.fifo_size;
>  		else
>  			fifo_size = sprite1_start + plane->wm.fifo_size;
> @@ -4076,7 +4076,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, 0);
>  			break;
>  		case DRM_PLANE_TYPE_OVERLAY:
> -			sprite = plane->plane;
> +			sprite = I915_SPRITE_NUM(plane);
>  			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, sprite + 1);
>  			break;
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 4d27243..11ca40a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -180,7 +180,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	const int pipe = intel_plane->pipe;
> -	const int plane = intel_plane->plane + 1;
> +	const int plane = I915_UNIVERSAL_NUM(intel_plane);
>  	u32 plane_ctl, stride_div, stride;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  	const struct drm_intel_sprite_colorkey *key =
> @@ -280,7 +280,7 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
>  	const int pipe = intel_plane->pipe;
> -	const int plane = intel_plane->plane + 1;
> +	const int plane = I915_UNIVERSAL_NUM(intel_plane);
>  
>  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
>  
> @@ -294,7 +294,7 @@ static void
>  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
>  {
>  	struct drm_i915_private *dev_priv = intel_plane->base.dev->dev_private;
> -	int plane = intel_plane->plane;
> +	int plane = I915_SPRITE_NUM(intel_plane);
>  
>  	/* Seems RGB data bypasses the CSC always */
>  	if (!format_is_yuv(format))
> @@ -342,7 +342,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	int pipe = intel_plane->pipe;
> -	int plane = intel_plane->plane;
> +	int plane = I915_SPRITE_NUM(intel_plane);
>  	u32 sprctl;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> @@ -461,7 +461,7 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
>  	int pipe = intel_plane->pipe;
> -	int plane = intel_plane->plane;
> +	int plane = I915_SPRITE_NUM(intel_plane);
>  
>  	I915_WRITE(SPCNTR(pipe, plane), 0);
>  
> @@ -1121,8 +1121,9 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  		return -ENODEV;
>  	}
>  
> +	/* primary = 0, sprites start from 1 */
> +	intel_plane->plane = plane + 1;
>  	intel_plane->pipe = pipe;
> -	intel_plane->plane = plane;
>  	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane);
>  	intel_plane->check_plane = intel_check_sprite_plane;
>  	intel_plane->commit_plane = intel_commit_sprite_plane;
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Clean up intel_plane->plane usage

[Maarten Lankhorst]

Op 18-09-15 om 03:55 schreef Matt Roper:
> intel_plane->plane has inconsistent meanings across the driver:
>  * For primary planes, intel_plane->plane is usually the pipe number
>    (except for old platforms where it had to be swapped for FBC
>    reasons).
>  * For sprite planes, intel_plane->plane represents the sprite index for
>    the specific CRTC the sprite belongs to (0, 1, 2, ...)
>  * For cursor planes, intel_plane->plane is again the pipe number,
>    but without the FBC swapping on old platforms.
>
> This overloading of the field can be quite confusing and easily leads to
> bugs due to differences in how the hardware actually gets programmed
> (e.g., SKL code needs to use p->plane + 1 because the hardware expects
> universal plane ID's that include the primary, whereas VLV code needs to
> use just p->plane because hardware expects a sprite index that does not
> include the primary).
>
> Let's try to clean up this situation by giving intel_plane->plane a
> consistent meaning across all plane types, and then update all uses
> across driver code to use macros to interpret it properly.  The
> following macros should be used in the code where we previously accessed
> p->plane and will do additional plane type checking to help prevent
> misuse:
>  * I915_UNIVERSAL_NUM(p) --- Universal plane ID (primary = 0, sprites
>    start from 1); intended for use in gen9+ code.
>  * I915_I915_PRIMARY_NUM(p) --- Primary plane ID for pre-gen9 platforms;
>    determines whether FBC-related swapping is needed or not.
The extra I915_ here should probably be fixed when committing, though it's
not sufficient to require a resend of this patch. :-)
>  * I915_SPRITE_NUM(p) --- Sprite plane ID (first sprite = 0); intended
>    for use in pre-gen9 code.
>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Clean up intel_plane->plane usage

[Daniel Vetter]

On Fri, Sep 18, 2015 at 11:48:30AM +0300, Ville Syrjälä wrote:
> On Thu, Sep 17, 2015 at 06:55:21PM -0700, Matt Roper wrote:
> > intel_plane->plane has inconsistent meanings across the driver:
> >  * For primary planes, intel_plane->plane is usually the pipe number
> >    (except for old platforms where it had to be swapped for FBC
> >    reasons).
> >  * For sprite planes, intel_plane->plane represents the sprite index for
> >    the specific CRTC the sprite belongs to (0, 1, 2, ...)
> >  * For cursor planes, intel_plane->plane is again the pipe number,
> >    but without the FBC swapping on old platforms.
> > 
> > This overloading of the field can be quite confusing and easily leads to
> > bugs due to differences in how the hardware actually gets programmed
> > (e.g., SKL code needs to use p->plane + 1 because the hardware expects
> > universal plane ID's that include the primary, whereas VLV code needs to
> > use just p->plane because hardware expects a sprite index that does not
> > include the primary).
> > 
> > Let's try to clean up this situation by giving intel_plane->plane a
> > consistent meaning across all plane types, and then update all uses
> > across driver code to use macros to interpret it properly.  The
> > following macros should be used in the code where we previously accessed
> > p->plane and will do additional plane type checking to help prevent
> > misuse:
> >  * I915_UNIVERSAL_NUM(p) --- Universal plane ID (primary = 0, sprites
> >    start from 1); intended for use in gen9+ code.
> >  * I915_I915_PRIMARY_NUM(p) --- Primary plane ID for pre-gen9 platforms;
> >    determines whether FBC-related swapping is needed or not.
> >  * I915_SPRITE_NUM(p) --- Sprite plane ID (first sprite = 0); intended
> >    for use in pre-gen9 code.
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      | 24 ++++++++++++++++++------
> >  drivers/gpu/drm/i915/intel_display.c | 12 ++++--------
> >  drivers/gpu/drm/i915/intel_pm.c      | 10 +++++-----
> >  drivers/gpu/drm/i915/intel_sprite.c  | 13 +++++++------
> >  4 files changed, 34 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 3bf8a9b..132fe53 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -131,22 +131,34 @@ enum transcoder {
> >  #define transcoder_name(t) ((t) + 'A')
> >  
> >  /*
> > - * 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.
> > + * I915_MAX_PLANES in the enum below is the maximum (across all platforms)
> > + * number of planes per CRTC.  Not all platforms really have this many planes,
> > + * which means some arrays of size I915_MAX_PLANES may have unused entries
> > + * between the topmost sprite plane and the cursor plane.
> >   */
> > -#define I915_MAX_PLANES	4
> > -
> >  enum plane {
> >  	PLANE_A = 0,
> > +	PLANE_PRIMARY = PLANE_A,
> >  	PLANE_B,
> >  	PLANE_C,
> > +	PLANE_CURSOR,
> > +	I915_MAX_PLANES,
> >  };
> 
> If we're really going to redefine this as a per-pipe thing, then I
> think we'd need to rename it. A,B,C have a definite meaning on most of
> our hardware, so using it for something else is confusing.
> 
> I know that looking for enum plane in the code probably won't give you
> a lot of hits, but that's just because we suck and usually use int or
> something. I have a patch series in a branch somewhere to clean that
> stuff up, but I don't think I posted it to the list so far.
> 
> I think it would be better to have a separate per-pipe plane enum.
> enum plane_id or something? And maybe store it as plane->id.

Yeah, enum plane is for the legacy per-pipe plane, so converting that over
to a per-pipe plane enum for universal planes would result in tons of
confusion.

I think short term Ville's idea of adding a plane->id might be best, or
perhaps adding an enum universal_plane.

Also note that in the future we won't use the backwards compat cursor
plane on gen9+ since it just aliases with the last universal plane.
Insteaad that code should die and we'll just mark the last universal plane
with type = TYPE_CURSOR. Atm we can't expose that last plane due to
this aliasing.

> >  #define plane_name(p) ((p) + 'A')
> >  
> >  #define sprite_name(p, s) ((p) * INTEL_INFO(dev)->num_sprites[(p)] + (s) + 'A')
> >  
> > +#define I915_UNIVERSAL_NUM(p) ( \
> > +	WARN_ON(p->base.type == DRM_PLANE_TYPE_CURSOR), \
> 
> I don't think that'll fly with SKL. The cursor is just another universal
> plane, or well, will be. I think the patch to do that change got stuck
> in review or something.

Yup that patch is still lost somewhere :(

> I guess that should be easily fixable by tossing in a !IS_SKL&& once
> that patch lands.
> 
> > +	p->plane)
> > +#define I915_PRIMARY_NUM(p) ( \
> > +	WARN_ON(p->base.type != DRM_PLANE_TYPE_PRIMARY), \
> > +	(HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) ? \
> > +			      !p->pipe : p->pipe)
> 
> I'm not sure we want to do this swapping here.
> 
> Maybe we'll just want a some kind of
> enum plane plane_id_to_plane(crtc, enum plane_id) function.
> Or we could just store the enum plane alongside the plane_id in the
> plane, and make it clear that it should only be used by pre-SKL
> platforms. In theory we could limit it to pre-gen4, or just gmch, but
> I think we probably want to share a bunch of the primary plane code
> all the way up to BDW, so allowing it's use there seems sane to me.

Yeah, maybe rename enum plane to enum i9xx_plane (and same for all the
variables holding it). Same should be done probably for intel_crtc->plane.

The problem with that one really is that we can't just derive the plane
from the pipe because of fbc and fun stuff like that on gen2/3.

Then I think we should add a new intel_plane->id like Ville suggested with
the semantics you're using in your patch here.
-Daniel
> 
> > +#define I915_SPRITE_NUM(p) ( \
> > +	WARN_ON(p->base.type != DRM_PLANE_TYPE_OVERLAY), \
> > +	p->plane - 1) I think that should untangle this mess sufficiently.
> > +
> >  enum port {
> >  	PORT_A = 0,
> >  	PORT_B,
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index fc00867..dab0b71 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11976,16 +11976,14 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> >  			DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d "
> >  				"disabled, scaler_id = %d\n",
> >  				plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
> > -				plane->base.id, intel_plane->pipe,
> > -				(crtc->base.primary == plane) ? 0 : intel_plane->plane + 1,
> > +				plane->base.id, intel_plane->pipe, intel_plane->plane,
> >  				drm_plane_index(plane), state->scaler_id);
> >  			continue;
> >  		}
> >  
> >  		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled",
> >  			plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
> > -			plane->base.id, intel_plane->pipe,
> > -			crtc->base.primary == plane ? 0 : intel_plane->plane + 1,
> > +			plane->base.id, intel_plane->pipe, intel_plane->plane,
> >  			drm_plane_index(plane));
> >  		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x",
> >  			fb->base.id, fb->width, fb->height, fb->pixel_format);
> > @@ -13547,13 +13545,11 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >  		state->scaler_id = -1;
> >  	}
> >  	primary->pipe = pipe;
> > -	primary->plane = pipe;
> > +	primary->plane = 0;
> >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> >  	primary->check_plane = intel_check_primary_plane;
> >  	primary->commit_plane = intel_commit_primary_plane;
> >  	primary->disable_plane = intel_disable_primary_plane;
> > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > -		primary->plane = !pipe;
> >  
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> >  		intel_primary_formats = skl_primary_formats;
> > @@ -13699,7 +13695,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> >  	cursor->can_scale = false;
> >  	cursor->max_downscale = 1;
> >  	cursor->pipe = pipe;
> > -	cursor->plane = pipe;
> > +	cursor->plane = PLANE_CURSOR;
> >  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> >  	cursor->check_plane = intel_check_cursor_plane;
> >  	cursor->commit_plane = intel_commit_cursor_plane;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 62de97e..9db19f2 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1131,7 +1131,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
> >  					wm_state->wm[level].primary;
> >  				break;
> >  			case DRM_PLANE_TYPE_OVERLAY:
> > -				sprite = plane->plane;
> > +				sprite = I915_SPRITE_NUM(plane);
> >  				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
> >  					wm_state->wm[level].sprite[sprite];
> >  				break;
> > @@ -1195,7 +1195,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> >  				wm_state->wm[level].primary = wm;
> >  				break;
> >  			case DRM_PLANE_TYPE_OVERLAY:
> > -				sprite = plane->plane;
> > +				sprite = I915_SPRITE_NUM(plane);
> >  				wm_state->wm[level].sprite[sprite] = wm;
> >  				break;
> >  			}
> > @@ -1221,7 +1221,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> >  					    wm_state->wm[level].primary);
> >  			break;
> >  		case DRM_PLANE_TYPE_OVERLAY:
> > -			sprite = plane->plane;
> > +			sprite = I915_SPRITE_NUM(plane);
> >  			for (level = 0; level < wm_state->num_levels; level++)
> >  				wm_state->sr[level].plane =
> >  					min(wm_state->sr[level].plane,
> > @@ -1257,7 +1257,7 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> >  
> >  		if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> >  			sprite0_start = plane->wm.fifo_size;
> > -		else if (plane->plane == 0)
> > +		else if (I915_SPRITE_NUM(plane) == 0)
> >  			sprite1_start = sprite0_start + plane->wm.fifo_size;
> >  		else
> >  			fifo_size = sprite1_start + plane->wm.fifo_size;
> > @@ -4076,7 +4076,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >  			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, 0);
> >  			break;
> >  		case DRM_PLANE_TYPE_OVERLAY:
> > -			sprite = plane->plane;
> > +			sprite = I915_SPRITE_NUM(plane);
> >  			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, sprite + 1);
> >  			break;
> >  		}
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 4d27243..11ca40a 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -180,7 +180,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
> >  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >  	const int pipe = intel_plane->pipe;
> > -	const int plane = intel_plane->plane + 1;
> > +	const int plane = I915_UNIVERSAL_NUM(intel_plane);
> >  	u32 plane_ctl, stride_div, stride;
> >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >  	const struct drm_intel_sprite_colorkey *key =
> > @@ -280,7 +280,7 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> >  	const int pipe = intel_plane->pipe;
> > -	const int plane = intel_plane->plane + 1;
> > +	const int plane = I915_UNIVERSAL_NUM(intel_plane);
> >  
> >  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
> >  
> > @@ -294,7 +294,7 @@ static void
> >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
> >  {
> >  	struct drm_i915_private *dev_priv = intel_plane->base.dev->dev_private;
> > -	int plane = intel_plane->plane;
> > +	int plane = I915_SPRITE_NUM(intel_plane);
> >  
> >  	/* Seems RGB data bypasses the CSC always */
> >  	if (!format_is_yuv(format))
> > @@ -342,7 +342,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> >  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >  	int pipe = intel_plane->pipe;
> > -	int plane = intel_plane->plane;
> > +	int plane = I915_SPRITE_NUM(intel_plane);
> >  	u32 sprctl;
> >  	unsigned long sprsurf_offset, linear_offset;
> >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> > @@ -461,7 +461,7 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> >  	int pipe = intel_plane->pipe;
> > -	int plane = intel_plane->plane;
> > +	int plane = I915_SPRITE_NUM(intel_plane);
> >  
> >  	I915_WRITE(SPCNTR(pipe, plane), 0);
> >  
> > @@ -1121,8 +1121,9 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  		return -ENODEV;
> >  	}
> >  
> > +	/* primary = 0, sprites start from 1 */
> > +	intel_plane->plane = plane + 1;
> >  	intel_plane->pipe = pipe;
> > -	intel_plane->plane = plane;
> >  	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane);
> >  	intel_plane->check_plane = intel_check_sprite_plane;
> >  	intel_plane->commit_plane = intel_commit_sprite_plane;
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx