[PATCH 3/4] drm/i915: Rename similar plane functions to avoid confusion

[PATCH 3/4] drm/i915: Rename similar plane functions to avoid confusion

[Matt Roper]

The name 'update_plane' was used both for the primary plane functions in
intel_display.c and the sprite/overlay functions in intel_sprite.c.
Rename the primary plane functions to 'update_primary_plane' to avoid
confusion.

On a similar note, intel_display.c already had a function called
intel_disable_primary_plane() that programs the hardware to disable a
pipe's primary plane.  When we hook up primary planes through the DRM
plane interface, one of the natural handler names will be
intel_primary_plane_disable(), which is very similar.  To avoid
confusion, rename the existing intel_disable_primary_plane() to
intel_disable_primary_hw_plane() to make the two names a little more
distinct.

Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  5 +++--
 drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++---------------
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8c64831..fc36e6c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -450,8 +450,9 @@ struct drm_i915_display_funcs {
 			  struct drm_framebuffer *fb,
 			  struct drm_i915_gem_object *obj,
 			  uint32_t flags);
-	int (*update_plane)(struct drm_crtc *crtc, struct drm_framebuffer *fb,
-			    int x, int y);
+	int (*update_primary_plane)(struct drm_crtc *crtc,
+				    struct drm_framebuffer *fb,
+				    int x, int y);
 	void (*hpd_irq_setup)(struct drm_device *dev);
 	/* clock updates for mode set */
 	/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f19e6ea..9757010 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1905,15 +1905,15 @@ static void intel_enable_primary_plane(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_disable_primary_plane - disable the primary plane
+ * intel_disable_primary_hw_plane - disable the primary hardware plane
  * @dev_priv: i915 private structure
  * @plane: plane to disable
  * @pipe: pipe consuming the data
  *
  * Disable @plane; should be an independent operation.
  */
-static void intel_disable_primary_plane(struct drm_i915_private *dev_priv,
-					enum plane plane, enum pipe pipe)
+static void intel_disable_primary_hw_plane(struct drm_i915_private *dev_priv,
+					   enum plane plane, enum pipe pipe)
 {
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
@@ -2047,8 +2047,9 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 	}
 }
 
-static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
-			     int x, int y)
+static int i9xx_update_primary_plane(struct drm_crtc *crtc,
+				     struct drm_framebuffer *fb,
+				     int x, int y)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2147,8 +2148,9 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 	return 0;
 }
 
-static int ironlake_update_plane(struct drm_crtc *crtc,
-				 struct drm_framebuffer *fb, int x, int y)
+static int ironlake_update_primary_plane(struct drm_crtc *crtc,
+					 struct drm_framebuffer *fb,
+					 int x, int y)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2252,7 +2254,7 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 		dev_priv->display.disable_fbc(dev);
 	intel_increase_pllclock(crtc);
 
-	return dev_priv->display.update_plane(crtc, fb, x, y);
+	return dev_priv->display.update_primary_plane(crtc, fb, x, y);
 }
 
 void intel_display_handle_reset(struct drm_device *dev)
@@ -2292,7 +2294,7 @@ void intel_display_handle_reset(struct drm_device *dev)
 		 * a NULL crtc->fb.
 		 */
 		if (intel_crtc->active && crtc->fb)
-			dev_priv->display.update_plane(crtc, crtc->fb,
+			dev_priv->display.update_primary_plane(crtc, crtc->fb,
 						       crtc->x, crtc->y);
 		mutex_unlock(&crtc->mutex);
 	}
@@ -2385,7 +2387,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		intel_crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay;
 	}
 
-	ret = dev_priv->display.update_plane(crtc, fb, x, y);
+	ret = dev_priv->display.update_primary_plane(crtc, fb, x, y);
 	if (ret) {
 		intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj);
 		mutex_unlock(&dev->struct_mutex);
@@ -3653,7 +3655,7 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
 
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
-	intel_disable_primary_plane(dev_priv, plane, pipe);
+	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 }
 
 /*
@@ -3781,7 +3783,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
-	intel_disable_primary_plane(dev_priv, plane, pipe);
+	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev, pipe, false);
@@ -4247,7 +4249,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc_dpms_overlay(intel_crtc, false);
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
-	intel_disable_primary_plane(dev_priv, plane, pipe);
+	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
 	intel_disable_pipe(dev_priv, pipe);
@@ -10719,28 +10721,32 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
 		dev_priv->display.off = haswell_crtc_off;
-		dev_priv->display.update_plane = ironlake_update_plane;
+		dev_priv->display.update_primary_plane =
+			ironlake_update_primary_plane;
 	} else if (HAS_PCH_SPLIT(dev)) {
 		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
 		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
 		dev_priv->display.crtc_enable = ironlake_crtc_enable;
 		dev_priv->display.crtc_disable = ironlake_crtc_disable;
 		dev_priv->display.off = ironlake_crtc_off;
-		dev_priv->display.update_plane = ironlake_update_plane;
+		dev_priv->display.update_primary_plane =
+			ironlake_update_primary_plane;
 	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = valleyview_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
-		dev_priv->display.update_plane = i9xx_update_plane;
+		dev_priv->display.update_primary_plane =
+			i9xx_update_primary_plane;
 	} else {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = i9xx_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
-		dev_priv->display.update_plane = i9xx_update_plane;
+		dev_priv->display.update_primary_plane =
+			i9xx_update_primary_plane;
 	}
 
 	/* Returns the core display clock speed */
-- 
1.8.5.1

[PATCH 4/4] drm/i915: Register primary plane for each CRTC

[Matt Roper]

Create a primary plane at CRTC init and hook up handlers for the various
operations that may be performed on it.  The DRM core will only
advertise the primary planes to clients that set the appropriate
capability bit.

Since we're limited to the legacy plane operations at the moment
(SetPlane and such) this isn't terribly interesting yet; the plane
update handler will perform an MMIO flip of the display plane and the
disable handler will disable the CRTC.  Once we migrate more of the
plane and CRTC info over to properties in preparation for atomic/nuclear
operations, primary planes will be more useful.

Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 92 ++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9757010..d9a5cd5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8260,6 +8260,10 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
 
 	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
 
+	drm_plane_cleanup(crtc->primary_plane);
+	kfree(crtc->primary_plane);
+	crtc->primary_plane = NULL;
+
 	drm_crtc_cleanup(crtc);
 
 	kfree(intel_crtc);
@@ -10272,17 +10276,105 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
 }
 
+static int
+intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
+			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			     unsigned int crtc_w, unsigned int crtc_h,
+			     uint32_t src_x, uint32_t src_y,
+			     uint32_t src_w, uint32_t src_h)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* setplane API takes shifted source rectangle values; unshift them */
+	src_x >>= 16;
+	src_y >>= 16;
+	src_w >>= 16;
+	src_h >>= 16;
+
+	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
+		DRM_DEBUG_KMS("Unsuitable framebuffer for primary plane\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Current hardware can't reposition the primary plane or scale it
+	 * (although this could change in the future).  This means that we
+	 * don't actually need any of the destination (crtc) rectangle values,
+	 * or the source rectangle width/height; only the source x/y winds up
+	 * getting used for panning.  Nevertheless, let's sanity check the
+	 * incoming values to make sure userspace didn't think it could scale
+	 * or reposition this plane.
+	 */
+	if (crtc_w != crtc->mode.hdisplay || crtc_h != crtc->mode.vdisplay ||
+	    crtc_x != 0 || crtc_y != 0) {
+		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
+		return -EINVAL;
+	}
+	if (crtc_w != src_w || crtc_h != src_h) {
+		DRM_DEBUG_KMS("Can't scale primary plane\n");
+		return -EINVAL;
+	}
+
+	intel_pipe_set_base(crtc, src_x, src_y, fb);
+	dev_priv->display.crtc_enable(crtc);
+
+	return 0;
+}
+
+static int
+intel_primary_plane_disable(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	if (!plane->fb)
+		return 0;
+
+	if (WARN_ON(!plane->crtc || plane->crtc->primary_plane != plane))
+		return -EINVAL;
+
+	dev_priv->display.crtc_disable(plane->crtc);
+
+	return 0;
+}
+
+static void intel_primary_plane_destroy(struct drm_plane *plane)
+{
+	/*
+	 * Since primary planes are never put on the mode_config plane list,
+	 * this entry point should never be called.  Primary plane cleanup
+	 * happens during CRTC destruction.
+	 */
+	BUG();
+}
+
+static const struct drm_plane_funcs intel_primary_plane_funcs = {
+	.update_plane = intel_primary_plane_setplane,
+	.disable_plane = intel_primary_plane_disable,
+	.destroy = intel_primary_plane_destroy,
+};
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
+	struct drm_plane *primary_plane;
 	int i;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
+	primary_plane = kzalloc(sizeof(*primary_plane), GFP_KERNEL);
+	if (primary_plane == NULL) {
+		kfree(intel_crtc);
+		return;
+	}
+
 	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
+	drm_plane_set_primary(dev, primary_plane, &intel_crtc->base,
+			      &intel_primary_plane_funcs, NULL, 0);
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
-- 
1.8.5.1

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Register primary plane for each CRTC

[Ville Syrjälä]

On Thu, Feb 27, 2014 at 02:14:43PM -0800, Matt Roper wrote:
> Create a primary plane at CRTC init and hook up handlers for the various
> operations that may be performed on it.  The DRM core will only
> advertise the primary planes to clients that set the appropriate
> capability bit.
> 
> Since we're limited to the legacy plane operations at the moment
> (SetPlane and such) this isn't terribly interesting yet; the plane
> update handler will perform an MMIO flip of the display plane and the
> disable handler will disable the CRTC.  Once we migrate more of the
> plane and CRTC info over to properties in preparation for atomic/nuclear
> operations, primary planes will be more useful.
> 
> Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 92 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9757010..d9a5cd5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8260,6 +8260,10 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  
>  	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
>  
> +	drm_plane_cleanup(crtc->primary_plane);
> +	kfree(crtc->primary_plane);
> +	crtc->primary_plane = NULL;
> +
>  	drm_crtc_cleanup(crtc);
>  
>  	kfree(intel_crtc);
> @@ -10272,17 +10276,105 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>  }
>  
> +static int
> +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> +			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> +			     unsigned int crtc_w, unsigned int crtc_h,
> +			     uint32_t src_x, uint32_t src_y,
> +			     uint32_t src_w, uint32_t src_h)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* setplane API takes shifted source rectangle values; unshift them */
> +	src_x >>= 16;
> +	src_y >>= 16;
> +	src_w >>= 16;
> +	src_h >>= 16;
> +
> +	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
> +		DRM_DEBUG_KMS("Unsuitable framebuffer for primary plane\n");
> +		return -EINVAL;
> +	}

These aren't quite right for all gens. Actually I think the primary plane
limits are genereally more relaxed than the sprite plane limits, so 
intel_framebuffer_init() should have already done the necessary checks,
at least as far the stride is concerned. So I'd just drop the stride
check. In the future I suppose we might need to add some extra checks
here too, but for now I think we should fine w/o them.

> +
> +	/*
> +	 * Current hardware can't reposition the primary plane or scale it
> +	 * (although this could change in the future).  This means that we
> +	 * don't actually need any of the destination (crtc) rectangle values,
> +	 * or the source rectangle width/height; only the source x/y winds up
> +	 * getting used for panning.  Nevertheless, let's sanity check the
> +	 * incoming values to make sure userspace didn't think it could scale
> +	 * or reposition this plane.
> +	 */
> +	if (crtc_w != crtc->mode.hdisplay || crtc_h != crtc->mode.vdisplay ||
> +	    crtc_x != 0 || crtc_y != 0) {
> +		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> +		return -EINVAL;
> +	}
> +	if (crtc_w != src_w || crtc_h != src_h) {
> +		DRM_DEBUG_KMS("Can't scale primary plane\n");
> +		return -EINVAL;
> +	}

I'd do the clipping anyway, and then check that the dst size matches the
pipe src size post clipping. Otherwise the behaviour is rather
inconsistent with the sprite planes.

> +
> +	intel_pipe_set_base(crtc, src_x, src_y, fb);

This can return an error.

> +	dev_priv->display.crtc_enable(crtc);

Shouldn't enable the entire pipe, just the plane.

> +
> +	return 0;
> +}
> +
> +static int
> +intel_primary_plane_disable(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	drm_i915_private_t *dev_priv = dev->dev_private;
> +
> +	if (!plane->fb)
> +		return 0;
> +
> +	if (WARN_ON(!plane->crtc || plane->crtc->primary_plane != plane))
> +		return -EINVAL;
> +
> +	dev_priv->display.crtc_disable(plane->crtc);

This should just disable the plane, not the entire pipe.

> +
> +	return 0;
> +}
> +
> +static void intel_primary_plane_destroy(struct drm_plane *plane)
> +{
> +	/*
> +	 * Since primary planes are never put on the mode_config plane list,
> +	 * this entry point should never be called.  Primary plane cleanup
> +	 * happens during CRTC destruction.
> +	 */
> +	BUG();

Just leave the func pointer as NULL, you get a backtrace either way.

> +}
> +
> +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> +	.update_plane = intel_primary_plane_setplane,
> +	.disable_plane = intel_primary_plane_disable,
> +	.destroy = intel_primary_plane_destroy,
> +};
> +
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> +	struct drm_plane *primary_plane;
>  	int i;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
>  	if (intel_crtc == NULL)
>  		return;
>  
> +	primary_plane = kzalloc(sizeof(*primary_plane), GFP_KERNEL);
> +	if (primary_plane == NULL) {
> +		kfree(intel_crtc);
> +		return;
> +	}
> +
>  	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> +	drm_plane_set_primary(dev, primary_plane, &intel_crtc->base,
> +			      &intel_primary_plane_funcs, NULL, 0);
>  
>  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
>  	for (i = 0; i < 256; i++) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC