[PATCH 0/7] i915 atomic plane helper conversion

[PATCH 0/7] i915 atomic plane helper conversion

[Matt Roper]

This series takes another step along the path to atomic modeset / nuclear
pageflip by transitioning the i915 driver to the new atomic plane helpers
provided by Daniel Vetter in:

  http://lists.freedesktop.org/archives/dri-devel/2014-November/071623.html

Previous work by Gustavo Padovan had already split most of the i915 plane
internals into separate prepare/check/commit steps, so a lot of the heavy
lifting was already done.  This patch series sets up the hooks expected by the
atomic plane helpers, replaces the existing .update_plane() and
.disable_plane() entrypoints with the helpers, and does some general cleanup.
Patches #5 and #6 are the meat here; the other five patches are minor
refactoring and cleanup work.

I've tested this series on IVB with the plane-related i-g-t tests and some very
casual X usage.

Matt Roper (7):
  drm/i915: Make intel_plane_state subclass drm_plane_state
  drm/i915: Allow intel_plane_disable() to operate on all plane types
  drm/i915: Clarify sprite plane function names
  drm/i915: Make intel_crtc_has_pending_flip() non-static
  drm/i915: Prepare for atomic plane helpers
  drm/i915: Switch plane handling to atomic helpers
  drm/i915: Drop unused position fields

 drivers/gpu/drm/i915/Makefile        |   1 +
 drivers/gpu/drm/i915/intel_atomic.c  | 217 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 344 +++++++++--------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  28 ++-
 drivers/gpu/drm/i915/intel_sprite.c  | 170 +++--------------
 5 files changed, 351 insertions(+), 409 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_atomic.c

-- 
1.8.5.1

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

[PATCH 1/7] drm/i915: Make intel_plane_state subclass drm_plane_state

[Matt Roper]

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h     |  3 +--
 drivers/gpu/drm/i915/intel_sprite.c  | 16 ++++++++--------
 3 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 73e6656..a9f90b8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11553,8 +11553,8 @@ static int
 intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->crtc;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_crtc *crtc = state->base.crtc;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
@@ -11570,8 +11570,8 @@ static int
 intel_prepare_primary_plane(struct drm_plane *plane,
 			    struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->crtc;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_crtc *crtc = state->base.crtc;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
@@ -11606,8 +11606,8 @@ static void
 intel_commit_primary_plane(struct drm_plane *plane,
 			   struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->crtc;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_crtc *crtc = state->base.crtc;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -11707,8 +11707,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret;
 
-	state.crtc = crtc;
-	state.fb = fb;
+	state.base.crtc = crtc;
+	state.base.fb = fb;
 
 	/* sample coordinates in 16.16 fixed point */
 	state.src.x1 = src_x;
@@ -11820,9 +11820,9 @@ static int
 intel_check_cursor_plane(struct drm_plane *plane,
 			 struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->crtc;
+	struct drm_crtc *crtc = state->base.crtc;
 	struct drm_device *dev = crtc->dev;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
@@ -11876,8 +11876,8 @@ static int
 intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->crtc;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_crtc *crtc = state->base.crtc;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
@@ -11922,8 +11922,8 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct intel_plane_state state;
 	int ret;
 
-	state.crtc = crtc;
-	state.fb = fb;
+	state.base.crtc = crtc;
+	state.base.fb = fb;
 
 	/* sample coordinates in 16.16 fixed point */
 	state.src.x1 = src_x;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8432ae2..bd5ef4e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -244,8 +244,7 @@ typedef struct dpll {
 } intel_clock_t;
 
 struct intel_plane_state {
-	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb;
+	struct drm_plane_state base;
 	struct drm_rect src;
 	struct drm_rect dst;
 	struct drm_rect clip;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 7d9c340..fc96d13 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1096,9 +1096,9 @@ static int
 intel_check_sprite_plane(struct drm_plane *plane,
 			 struct intel_plane_state *state)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
+	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
@@ -1262,11 +1262,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
 			   struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_crtc *crtc = state->crtc;
+	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int ret;
@@ -1297,11 +1297,11 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_crtc *crtc = state->crtc;
+	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int crtc_x, crtc_y;
@@ -1391,8 +1391,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret;
 
-	state.crtc = crtc;
-	state.fb = fb;
+	state.base.crtc = crtc;
+	state.base.fb = fb;
 
 	/* sample coordinates in 16.16 fixed point */
 	state.src.x1 = src_x;
-- 
1.8.5.1

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

Re: [PATCH 1/7] drm/i915: Make intel_plane_state subclass drm_plane_state

[Bob Paauwe]

On Thu, 13 Nov 2014 10:43:20 -0800
Matt Roper <matthew.d.roper@intel.com> wrote:

> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +--
>  drivers/gpu/drm/i915/intel_sprite.c  | 16 ++++++++--------
>  3 files changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 73e6656..a9f90b8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11553,8 +11553,8 @@ static int
>  intel_check_primary_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
>  {
> -	struct drm_crtc *crtc = state->crtc;
> -	struct drm_framebuffer *fb = state->fb;
> +	struct drm_crtc *crtc = state->base.crtc;
> +	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> @@ -11570,8 +11570,8 @@ static int
>  intel_prepare_primary_plane(struct drm_plane *plane,
>  			    struct intel_plane_state *state)
>  {
> -	struct drm_crtc *crtc = state->crtc;
> -	struct drm_framebuffer *fb = state->fb;
> +	struct drm_crtc *crtc = state->base.crtc;
> +	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
> @@ -11606,8 +11606,8 @@ static void
>  intel_commit_primary_plane(struct drm_plane *plane,
>  			   struct intel_plane_state *state)
>  {
> -	struct drm_crtc *crtc = state->crtc;
> -	struct drm_framebuffer *fb = state->fb;
> +	struct drm_crtc *crtc = state->base.crtc;
> +	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -11707,8 +11707,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int ret;
>  
> -	state.crtc = crtc;
> -	state.fb = fb;
> +	state.base.crtc = crtc;
> +	state.base.fb = fb;
>  
>  	/* sample coordinates in 16.16 fixed point */
>  	state.src.x1 = src_x;
> @@ -11820,9 +11820,9 @@ static int
>  intel_check_cursor_plane(struct drm_plane *plane,
>  			 struct intel_plane_state *state)
>  {
> -	struct drm_crtc *crtc = state->crtc;
> +	struct drm_crtc *crtc = state->base.crtc;
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_framebuffer *fb = state->fb;
> +	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> @@ -11876,8 +11876,8 @@ static int
>  intel_commit_cursor_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
>  {
> -	struct drm_crtc *crtc = state->crtc;
> -	struct drm_framebuffer *fb = state->fb;
> +	struct drm_crtc *crtc = state->base.crtc;
> +	struct drm_framebuffer *fb = state->base.fb;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> @@ -11922,8 +11922,8 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct intel_plane_state state;
>  	int ret;
>  
> -	state.crtc = crtc;
> -	state.fb = fb;
> +	state.base.crtc = crtc;
> +	state.base.fb = fb;
>  
>  	/* sample coordinates in 16.16 fixed point */
>  	state.src.x1 = src_x;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8432ae2..bd5ef4e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -244,8 +244,7 @@ typedef struct dpll {
>  } intel_clock_t;
>  
>  struct intel_plane_state {
> -	struct drm_crtc *crtc;
> -	struct drm_framebuffer *fb;
> +	struct drm_plane_state base;
>  	struct drm_rect src;
>  	struct drm_rect dst;
>  	struct drm_rect clip;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 7d9c340..fc96d13 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1096,9 +1096,9 @@ static int
>  intel_check_sprite_plane(struct drm_plane *plane,
>  			 struct intel_plane_state *state)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	struct drm_framebuffer *fb = state->fb;
> +	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	int crtc_x, crtc_y;
>  	unsigned int crtc_w, crtc_h;
> @@ -1262,11 +1262,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
>  			   struct intel_plane_state *state)
>  {
>  	struct drm_device *dev = plane->dev;
> -	struct drm_crtc *crtc = state->crtc;
> +	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_framebuffer *fb = state->fb;
> +	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	struct drm_i915_gem_object *old_obj = intel_plane->obj;
>  	int ret;
> @@ -1297,11 +1297,11 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
>  {
>  	struct drm_device *dev = plane->dev;
> -	struct drm_crtc *crtc = state->crtc;
> +	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_framebuffer *fb = state->fb;
> +	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	struct drm_i915_gem_object *old_obj = intel_plane->obj;
>  	int crtc_x, crtc_y;
> @@ -1391,8 +1391,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int ret;
>  
> -	state.crtc = crtc;
> -	state.fb = fb;
> +	state.base.crtc = crtc;
> +	state.base.fb = fb;
>  
>  	/* sample coordinates in 16.16 fixed point */
>  	state.src.x1 = src_x;

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

[PATCH 2/7] drm/i915: Allow intel_plane_disable() to operate on all plane types

[Matt Roper]

We'll want to call this from the type-agnostic atomic plane helper
hooks.  Since it's not sprite-specific anymore, more it to
intel_display.c as well.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_sprite.c  | 10 +---------
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a9f90b8..c6598e9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13679,3 +13679,24 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
 		spin_unlock_irq(&dev->event_lock);
 	}
 }
+
+void intel_plane_disable(struct drm_plane *plane)
+{
+	if (!plane->crtc || !plane->fb)
+		return;
+
+	switch (plane->type) {
+	case DRM_PLANE_TYPE_PRIMARY:
+		intel_primary_plane_disable(plane);
+		break;
+	case DRM_PLANE_TYPE_CURSOR:
+		intel_cursor_plane_disable(plane);
+		break;
+	case DRM_PLANE_TYPE_OVERLAY:
+		intel_disable_plane(plane);
+		break;
+	default:
+		WARN(1, "Unknown plane type");
+	}
+}
+
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bd5ef4e..df1420b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -920,6 +920,7 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
 void intel_finish_page_flip(struct drm_device *dev, int pipe);
 void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
 void intel_check_page_flip(struct drm_device *dev, int pipe);
+void intel_plane_disable(struct drm_plane *plane);
 
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
@@ -1180,7 +1181,6 @@ int intel_plane_set_property(struct drm_plane *plane,
 			     struct drm_property *prop,
 			     uint64_t val);
 int intel_plane_restore(struct drm_plane *plane);
-void intel_plane_disable(struct drm_plane *plane);
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
@@ -1188,6 +1188,7 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
 bool intel_pipe_update_start(struct intel_crtc *crtc,
 			     uint32_t *start_vbl_count);
 void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
+int intel_disable_plane(struct drm_plane *plane);
 
 /* intel_tv.c */
 void intel_tv_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index fc96d13..115acd3 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1425,7 +1425,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	return 0;
 }
 
-static int
+int
 intel_disable_plane(struct drm_plane *plane)
 {
 	struct drm_device *dev = plane->dev;
@@ -1576,14 +1576,6 @@ int intel_plane_restore(struct drm_plane *plane)
 				  intel_plane->src_w, intel_plane->src_h);
 }
 
-void intel_plane_disable(struct drm_plane *plane)
-{
-	if (!plane->crtc || !plane->fb)
-		return;
-
-	intel_disable_plane(plane);
-}
-
 static const struct drm_plane_funcs intel_plane_funcs = {
 	.update_plane = intel_update_plane,
 	.disable_plane = intel_disable_plane,
-- 
1.8.5.1

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

Re: [PATCH 2/7] drm/i915: Allow intel_plane_disable() to operate on all plane types

[Bob Paauwe]

On Thu, 13 Nov 2014 10:43:21 -0800
Matt Roper <matthew.d.roper@intel.com> wrote:

> We'll want to call this from the type-agnostic atomic plane helper
> hooks.  Since it's not sprite-specific anymore, more it to
> intel_display.c as well.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
>  drivers/gpu/drm/i915/intel_sprite.c  | 10 +---------
>  3 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a9f90b8..c6598e9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13679,3 +13679,24 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
>  		spin_unlock_irq(&dev->event_lock);
>  	}
>  }
> +
> +void intel_plane_disable(struct drm_plane *plane)
> +{
> +	if (!plane->crtc || !plane->fb)
> +		return;
> +
> +	switch (plane->type) {
> +	case DRM_PLANE_TYPE_PRIMARY:
> +		intel_primary_plane_disable(plane);
> +		break;
> +	case DRM_PLANE_TYPE_CURSOR:
> +		intel_cursor_plane_disable(plane);
> +		break;
> +	case DRM_PLANE_TYPE_OVERLAY:
> +		intel_disable_plane(plane);
> +		break;
> +	default:
> +		WARN(1, "Unknown plane type");
> +	}
> +}
> +
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bd5ef4e..df1420b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -920,6 +920,7 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
>  void intel_finish_page_flip(struct drm_device *dev, int pipe);
>  void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
>  void intel_check_page_flip(struct drm_device *dev, int pipe);
> +void intel_plane_disable(struct drm_plane *plane);
>  
>  /* shared dpll functions */
>  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> @@ -1180,7 +1181,6 @@ int intel_plane_set_property(struct drm_plane *plane,
>  			     struct drm_property *prop,
>  			     uint64_t val);
>  int intel_plane_restore(struct drm_plane *plane);
> -void intel_plane_disable(struct drm_plane *plane);
>  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
>  int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> @@ -1188,6 +1188,7 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
>  bool intel_pipe_update_start(struct intel_crtc *crtc,
>  			     uint32_t *start_vbl_count);
>  void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> +int intel_disable_plane(struct drm_plane *plane);

Would it make sense to rename this to intel_sprite_plane_disable() as
part of this?  It would be more consistent with the cursor and primary
plane naming conventions and likely avoid some confusion with the
intel_plane_disable() function.
 
>  
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index fc96d13..115acd3 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1425,7 +1425,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> -static int
> +int
>  intel_disable_plane(struct drm_plane *plane)
>  {
>  	struct drm_device *dev = plane->dev;
> @@ -1576,14 +1576,6 @@ int intel_plane_restore(struct drm_plane *plane)
>  				  intel_plane->src_w, intel_plane->src_h);
>  }
>  
> -void intel_plane_disable(struct drm_plane *plane)
> -{
> -	if (!plane->crtc || !plane->fb)
> -		return;
> -
> -	intel_disable_plane(plane);
> -}
> -
>  static const struct drm_plane_funcs intel_plane_funcs = {
>  	.update_plane = intel_update_plane,
>  	.disable_plane = intel_disable_plane,

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/i915: Allow intel_plane_disable() to operate on all plane types

[Matt Roper]

On Thu, Nov 13, 2014 at 11:11:38AM -0800, Bob Paauwe wrote:
> On Thu, 13 Nov 2014 10:43:21 -0800
> Matt Roper <matthew.d.roper@intel.com> wrote:
> 
> > We'll want to call this from the type-agnostic atomic plane helper
> > hooks.  Since it's not sprite-specific anymore, more it to
> > intel_display.c as well.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
> >  drivers/gpu/drm/i915/intel_sprite.c  | 10 +---------
> >  3 files changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a9f90b8..c6598e9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13679,3 +13679,24 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
> >  		spin_unlock_irq(&dev->event_lock);
> >  	}
> >  }
> > +
> > +void intel_plane_disable(struct drm_plane *plane)
> > +{
> > +	if (!plane->crtc || !plane->fb)
> > +		return;
> > +
> > +	switch (plane->type) {
> > +	case DRM_PLANE_TYPE_PRIMARY:
> > +		intel_primary_plane_disable(plane);
> > +		break;
> > +	case DRM_PLANE_TYPE_CURSOR:
> > +		intel_cursor_plane_disable(plane);
> > +		break;
> > +	case DRM_PLANE_TYPE_OVERLAY:
> > +		intel_disable_plane(plane);
> > +		break;
> > +	default:
> > +		WARN(1, "Unknown plane type");
> > +	}
> > +}
> > +
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index bd5ef4e..df1420b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -920,6 +920,7 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
> >  void intel_finish_page_flip(struct drm_device *dev, int pipe);
> >  void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> >  void intel_check_page_flip(struct drm_device *dev, int pipe);
> > +void intel_plane_disable(struct drm_plane *plane);
> >  
> >  /* shared dpll functions */
> >  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> > @@ -1180,7 +1181,6 @@ int intel_plane_set_property(struct drm_plane *plane,
> >  			     struct drm_property *prop,
> >  			     uint64_t val);
> >  int intel_plane_restore(struct drm_plane *plane);
> > -void intel_plane_disable(struct drm_plane *plane);
> >  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> >  			      struct drm_file *file_priv);
> >  int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> > @@ -1188,6 +1188,7 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> >  bool intel_pipe_update_start(struct intel_crtc *crtc,
> >  			     uint32_t *start_vbl_count);
> >  void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> > +int intel_disable_plane(struct drm_plane *plane);
> 
> Would it make sense to rename this to intel_sprite_plane_disable() as
> part of this?  It would be more consistent with the cursor and primary
> plane naming conventions and likely avoid some confusion with the
> intel_plane_disable() function.
>  

Yeah, that happens in the next patch of the series (along with some
other naming cleanup).  I tripped over the confusing names too many
times while working on this...


Matt


> >  
> >  /* intel_tv.c */
> >  void intel_tv_init(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index fc96d13..115acd3 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1425,7 +1425,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	return 0;
> >  }
> >  
> > -static int
> > +int
> >  intel_disable_plane(struct drm_plane *plane)
> >  {
> >  	struct drm_device *dev = plane->dev;
> > @@ -1576,14 +1576,6 @@ int intel_plane_restore(struct drm_plane *plane)
> >  				  intel_plane->src_w, intel_plane->src_h);
> >  }
> >  
> > -void intel_plane_disable(struct drm_plane *plane)
> > -{
> > -	if (!plane->crtc || !plane->fb)
> > -		return;
> > -
> > -	intel_disable_plane(plane);
> > -}
> > -
> >  static const struct drm_plane_funcs intel_plane_funcs = {
> >  	.update_plane = intel_update_plane,
> >  	.disable_plane = intel_disable_plane,
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/i915: Allow intel_plane_disable() to operate on all plane types

[Bob Paauwe]

On Thu, 13 Nov 2014 11:12:01 -0800
Matt Roper <matthew.d.roper@intel.com> wrote:

> On Thu, Nov 13, 2014 at 11:11:38AM -0800, Bob Paauwe wrote:
> > On Thu, 13 Nov 2014 10:43:21 -0800
> > Matt Roper <matthew.d.roper@intel.com> wrote:
> > 
> > > We'll want to call this from the type-agnostic atomic plane helper
> > > hooks.  Since it's not sprite-specific anymore, more it to
> > > intel_display.c as well.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
> > >  drivers/gpu/drm/i915/intel_sprite.c  | 10 +---------
> > >  3 files changed, 24 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index a9f90b8..c6598e9 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13679,3 +13679,24 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
> > >  		spin_unlock_irq(&dev->event_lock);
> > >  	}
> > >  }
> > > +
> > > +void intel_plane_disable(struct drm_plane *plane)
> > > +{
> > > +	if (!plane->crtc || !plane->fb)
> > > +		return;
> > > +
> > > +	switch (plane->type) {
> > > +	case DRM_PLANE_TYPE_PRIMARY:
> > > +		intel_primary_plane_disable(plane);
> > > +		break;
> > > +	case DRM_PLANE_TYPE_CURSOR:
> > > +		intel_cursor_plane_disable(plane);
> > > +		break;
> > > +	case DRM_PLANE_TYPE_OVERLAY:
> > > +		intel_disable_plane(plane);
> > > +		break;
> > > +	default:
> > > +		WARN(1, "Unknown plane type");
> > > +	}
> > > +}
> > > +
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index bd5ef4e..df1420b 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -920,6 +920,7 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
> > >  void intel_finish_page_flip(struct drm_device *dev, int pipe);
> > >  void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> > >  void intel_check_page_flip(struct drm_device *dev, int pipe);
> > > +void intel_plane_disable(struct drm_plane *plane);
> > >  
> > >  /* shared dpll functions */
> > >  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> > > @@ -1180,7 +1181,6 @@ int intel_plane_set_property(struct drm_plane *plane,
> > >  			     struct drm_property *prop,
> > >  			     uint64_t val);
> > >  int intel_plane_restore(struct drm_plane *plane);
> > > -void intel_plane_disable(struct drm_plane *plane);
> > >  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> > >  			      struct drm_file *file_priv);
> > >  int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> > > @@ -1188,6 +1188,7 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> > >  bool intel_pipe_update_start(struct intel_crtc *crtc,
> > >  			     uint32_t *start_vbl_count);
> > >  void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> > > +int intel_disable_plane(struct drm_plane *plane);
> > 
> > Would it make sense to rename this to intel_sprite_plane_disable() as
> > part of this?  It would be more consistent with the cursor and primary
> > plane naming conventions and likely avoid some confusion with the
> > intel_plane_disable() function.
> >  
> 
> Yeah, that happens in the next patch of the series (along with some
> other naming cleanup).  I tripped over the confusing names too many
> times while working on this...
> 
> 
> Matt

I tripped over it on my first pass reading through this which is why I
think it might be better as part of this patch.  But being part of the
next patch is fine too.

Bob

> 
> 
> > >  
> > >  /* intel_tv.c */
> > >  void intel_tv_init(struct drm_device *dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index fc96d13..115acd3 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -1425,7 +1425,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  	return 0;
> > >  }
> > >  
> > > -static int
> > > +int
> > >  intel_disable_plane(struct drm_plane *plane)
> > >  {
> > >  	struct drm_device *dev = plane->dev;
> > > @@ -1576,14 +1576,6 @@ int intel_plane_restore(struct drm_plane *plane)
> > >  				  intel_plane->src_w, intel_plane->src_h);
> > >  }
> > >  
> > > -void intel_plane_disable(struct drm_plane *plane)
> > > -{
> > > -	if (!plane->crtc || !plane->fb)
> > > -		return;
> > > -
> > > -	intel_disable_plane(plane);
> > > -}
> > > -
> > >  static const struct drm_plane_funcs intel_plane_funcs = {
> > >  	.update_plane = intel_update_plane,
> > >  	.disable_plane = intel_disable_plane,
> > 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/i915: Allow intel_plane_disable() to operate on all plane types

[Ville Syrjälä]

On Thu, Nov 13, 2014 at 11:11:38AM -0800, Bob Paauwe wrote:
> On Thu, 13 Nov 2014 10:43:21 -0800
> Matt Roper <matthew.d.roper@intel.com> wrote:
> 
> > We'll want to call this from the type-agnostic atomic plane helper
> > hooks.  Since it's not sprite-specific anymore, more it to
> > intel_display.c as well.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
> >  drivers/gpu/drm/i915/intel_sprite.c  | 10 +---------
> >  3 files changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a9f90b8..c6598e9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13679,3 +13679,24 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
> >  		spin_unlock_irq(&dev->event_lock);
> >  	}
> >  }
> > +
> > +void intel_plane_disable(struct drm_plane *plane)
> > +{
> > +	if (!plane->crtc || !plane->fb)
> > +		return;
> > +
> > +	switch (plane->type) {
> > +	case DRM_PLANE_TYPE_PRIMARY:
> > +		intel_primary_plane_disable(plane);
> > +		break;
> > +	case DRM_PLANE_TYPE_CURSOR:
> > +		intel_cursor_plane_disable(plane);
> > +		break;
> > +	case DRM_PLANE_TYPE_OVERLAY:
> > +		intel_disable_plane(plane);
> > +		break;
> > +	default:
> > +		WARN(1, "Unknown plane type");
> > +	}
> > +}
> > +
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index bd5ef4e..df1420b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -920,6 +920,7 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
> >  void intel_finish_page_flip(struct drm_device *dev, int pipe);
> >  void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> >  void intel_check_page_flip(struct drm_device *dev, int pipe);
> > +void intel_plane_disable(struct drm_plane *plane);
> >  
> >  /* shared dpll functions */
> >  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> > @@ -1180,7 +1181,6 @@ int intel_plane_set_property(struct drm_plane *plane,
> >  			     struct drm_property *prop,
> >  			     uint64_t val);
> >  int intel_plane_restore(struct drm_plane *plane);
> > -void intel_plane_disable(struct drm_plane *plane);
> >  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> >  			      struct drm_file *file_priv);
> >  int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> > @@ -1188,6 +1188,7 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> >  bool intel_pipe_update_start(struct intel_crtc *crtc,
> >  			     uint32_t *start_vbl_count);
> >  void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> > +int intel_disable_plane(struct drm_plane *plane);
> 
> Would it make sense to rename this to intel_sprite_plane_disable() as
> part of this?  It would be more consistent with the cursor and primary
> plane naming conventions and likely avoid some confusion with the
> intel_plane_disable() function.

Hmm. Why do we even still have some kind of disable hook in the atomic
age? I would expect to just have .commit() or somesuch thing.

But is there's still some need for a disable hook, shouldn't we just call
the .disable_plane() hook of drm_plane? I guess I should really go and
read some this new helper stuff...

> >  
> >  /* intel_tv.c */
> >  void intel_tv_init(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index fc96d13..115acd3 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1425,7 +1425,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	return 0;
> >  }
> >  
> > -static int
> > +int
> >  intel_disable_plane(struct drm_plane *plane)
> >  {
> >  	struct drm_device *dev = plane->dev;
> > @@ -1576,14 +1576,6 @@ int intel_plane_restore(struct drm_plane *plane)
> >  				  intel_plane->src_w, intel_plane->src_h);
> >  }
> >  
> > -void intel_plane_disable(struct drm_plane *plane)
> > -{
> > -	if (!plane->crtc || !plane->fb)
> > -		return;
> > -
> > -	intel_disable_plane(plane);
> > -}
> > -
> >  static const struct drm_plane_funcs intel_plane_funcs = {
> >  	.update_plane = intel_update_plane,
> >  	.disable_plane = intel_disable_plane,
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 2/7] drm/i915: Allow intel_plane_disable() to operate on all plane types

[Matt Roper]

On Thu, Nov 13, 2014 at 09:23:02PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 13, 2014 at 11:11:38AM -0800, Bob Paauwe wrote:
> > On Thu, 13 Nov 2014 10:43:21 -0800
> > Matt Roper <matthew.d.roper@intel.com> wrote:
> > 
> > > We'll want to call this from the type-agnostic atomic plane helper
> > > hooks.  Since it's not sprite-specific anymore, more it to
> > > intel_display.c as well.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
> > >  drivers/gpu/drm/i915/intel_sprite.c  | 10 +---------
> > >  3 files changed, 24 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index a9f90b8..c6598e9 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13679,3 +13679,24 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
> > >  		spin_unlock_irq(&dev->event_lock);
> > >  	}
> > >  }
> > > +
> > > +void intel_plane_disable(struct drm_plane *plane)
> > > +{
> > > +	if (!plane->crtc || !plane->fb)
> > > +		return;
> > > +
> > > +	switch (plane->type) {
> > > +	case DRM_PLANE_TYPE_PRIMARY:
> > > +		intel_primary_plane_disable(plane);
> > > +		break;
> > > +	case DRM_PLANE_TYPE_CURSOR:
> > > +		intel_cursor_plane_disable(plane);
> > > +		break;
> > > +	case DRM_PLANE_TYPE_OVERLAY:
> > > +		intel_disable_plane(plane);
> > > +		break;
> > > +	default:
> > > +		WARN(1, "Unknown plane type");
> > > +	}
> > > +}
> > > +
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index bd5ef4e..df1420b 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -920,6 +920,7 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
> > >  void intel_finish_page_flip(struct drm_device *dev, int pipe);
> > >  void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> > >  void intel_check_page_flip(struct drm_device *dev, int pipe);
> > > +void intel_plane_disable(struct drm_plane *plane);
> > >  
> > >  /* shared dpll functions */
> > >  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> > > @@ -1180,7 +1181,6 @@ int intel_plane_set_property(struct drm_plane *plane,
> > >  			     struct drm_property *prop,
> > >  			     uint64_t val);
> > >  int intel_plane_restore(struct drm_plane *plane);
> > > -void intel_plane_disable(struct drm_plane *plane);
> > >  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> > >  			      struct drm_file *file_priv);
> > >  int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> > > @@ -1188,6 +1188,7 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> > >  bool intel_pipe_update_start(struct intel_crtc *crtc,
> > >  			     uint32_t *start_vbl_count);
> > >  void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> > > +int intel_disable_plane(struct drm_plane *plane);
> > 
> > Would it make sense to rename this to intel_sprite_plane_disable() as
> > part of this?  It would be more consistent with the cursor and primary
> > plane naming conventions and likely avoid some confusion with the
> > intel_plane_disable() function.
> 
> Hmm. Why do we even still have some kind of disable hook in the atomic
> age? I would expect to just have .commit() or somesuch thing.
> 
> But is there's still some need for a disable hook, shouldn't we just call
> the .disable_plane() hook of drm_plane? I guess I should really go and
> read some this new helper stuff...

You're right that it does eventually come down to
drm_plane_helper_commit() --> intel_plane_atomic_update() which should
be able to program the plane to either a new state or off.  To reuse as
much of our existing code as possible and keep my changes minimal, I'm
just implementing intel_plane_atomic_update() as:

        if (!plane->state->fb)
                intel_plane_disable(plane);
        else                                                                                                                                     
                intel_plane->commit_plane(plane, intel_state);                                                                                    

since we already had both of those functions implemented and working.
intel_plane->commit_plane() doesn't expect to be called in the disabled
case today (assumes crtc and/or fb are non-NULL), but it definitely
seems reasonable/easy to tweak that and roll the disable logic directly
into the lowest-level commit_plane in the future.  I just wanted to keep
things as simple as possible with minimal code changes for the initial
patchset.


Matt

> 
> > >  
> > >  /* intel_tv.c */
> > >  void intel_tv_init(struct drm_device *dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index fc96d13..115acd3 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -1425,7 +1425,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  	return 0;
> > >  }
> > >  
> > > -static int
> > > +int
> > >  intel_disable_plane(struct drm_plane *plane)
> > >  {
> > >  	struct drm_device *dev = plane->dev;
> > > @@ -1576,14 +1576,6 @@ int intel_plane_restore(struct drm_plane *plane)
> > >  				  intel_plane->src_w, intel_plane->src_h);
> > >  }
> > >  
> > > -void intel_plane_disable(struct drm_plane *plane)
> > > -{
> > > -	if (!plane->crtc || !plane->fb)
> > > -		return;
> > > -
> > > -	intel_disable_plane(plane);
> > > -}
> > > -
> > >  static const struct drm_plane_funcs intel_plane_funcs = {
> > >  	.update_plane = intel_update_plane,
> > >  	.disable_plane = intel_disable_plane,
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/7] drm/i915: Clarify sprite plane function names

[Matt Roper]

A few of the sprite-related function names in i915 are very similar
(intel_plane_disable() vs intel_disable_plane() or
intel_enable_planes() vs intel_crtc_enable_planes()) and don't make it
clear whether they only operate on sprite planes, or whether they also
apply to all universal plane types.  Rename a few functions to be more
consistent with our function naming for primary/cursor planes or to
clarify that they apply specifically to sprite planes:

 - s/intel_disable_plane/intel_sprite_plane_disable/
 - s/intel_disable_planes/intel_disable_sprite_planes/
 - s/intel_enable_planes/intel_enable_sprite_planes/

Also, drop the sprite-specific intel_destroy_plane() and just use
the type-agnostic intel_plane_destroy() function.  The extra 'disable'
call that intel_destroy_plane() did is unnecessary since the plane will
already be disabled due to framebuffer destruction by the point it gets
called.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_sprite.c  | 14 +++-----------
 3 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c6598e9..52ca8c4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4024,7 +4024,7 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
 	}
 }
 
-static void intel_enable_planes(struct drm_crtc *crtc)
+static void intel_enable_sprite_planes(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	enum pipe pipe = to_intel_crtc(crtc)->pipe;
@@ -4038,7 +4038,7 @@ static void intel_enable_planes(struct drm_crtc *crtc)
 	}
 }
 
-static void intel_disable_planes(struct drm_crtc *crtc)
+static void intel_disable_sprite_planes(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	enum pipe pipe = to_intel_crtc(crtc)->pipe;
@@ -4182,7 +4182,7 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 
 	intel_enable_primary_hw_plane(crtc->primary, crtc);
-	intel_enable_planes(crtc);
+	intel_enable_sprite_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 	intel_crtc_dpms_overlay(intel_crtc, true);
 
@@ -4217,7 +4217,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 
 	intel_crtc_dpms_overlay(intel_crtc, false);
 	intel_crtc_update_cursor(crtc, false);
-	intel_disable_planes(crtc);
+	intel_disable_sprite_planes(crtc);
 	intel_disable_primary_hw_plane(crtc->primary, crtc);
 
 	/*
@@ -11744,7 +11744,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 }
 
 /* Common destruction function for both primary and cursor planes */
-static void intel_plane_destroy(struct drm_plane *plane)
+void intel_plane_destroy(struct drm_plane *plane)
 {
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	drm_plane_cleanup(plane);
@@ -13693,7 +13693,7 @@ void intel_plane_disable(struct drm_plane *plane)
 		intel_cursor_plane_disable(plane);
 		break;
 	case DRM_PLANE_TYPE_OVERLAY:
-		intel_disable_plane(plane);
+		intel_sprite_plane_disable(plane);
 		break;
 	default:
 		WARN(1, "Unknown plane type");
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index df1420b..4913b3f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1181,6 +1181,7 @@ int intel_plane_set_property(struct drm_plane *plane,
 			     struct drm_property *prop,
 			     uint64_t val);
 int intel_plane_restore(struct drm_plane *plane);
+void intel_plane_destroy(struct drm_plane *plane);
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
@@ -1188,7 +1189,7 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
 bool intel_pipe_update_start(struct intel_crtc *crtc,
 			     uint32_t *start_vbl_count);
 void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
-int intel_disable_plane(struct drm_plane *plane);
+int intel_sprite_plane_disable(struct drm_plane *plane);
 
 /* intel_tv.c */
 void intel_tv_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 115acd3..9e6a72a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1426,7 +1426,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 }
 
 int
-intel_disable_plane(struct drm_plane *plane)
+intel_sprite_plane_disable(struct drm_plane *plane)
 {
 	struct drm_device *dev = plane->dev;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
@@ -1469,14 +1469,6 @@ intel_disable_plane(struct drm_plane *plane)
 	return 0;
 }
 
-static void intel_destroy_plane(struct drm_plane *plane)
-{
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	intel_disable_plane(plane);
-	drm_plane_cleanup(plane);
-	kfree(intel_plane);
-}
-
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv)
 {
@@ -1578,8 +1570,8 @@ int intel_plane_restore(struct drm_plane *plane)
 
 static const struct drm_plane_funcs intel_plane_funcs = {
 	.update_plane = intel_update_plane,
-	.disable_plane = intel_disable_plane,
-	.destroy = intel_destroy_plane,
+	.disable_plane = intel_sprite_plane_disable,
+	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
 };
 
-- 
1.8.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Clarify sprite plane function names

[Bob Paauwe]

On Thu, 13 Nov 2014 10:43:22 -0800
Matt Roper <matthew.d.roper@intel.com> wrote:

> A few of the sprite-related function names in i915 are very similar
> (intel_plane_disable() vs intel_disable_plane() or
> intel_enable_planes() vs intel_crtc_enable_planes()) and don't make it
> clear whether they only operate on sprite planes, or whether they also
> apply to all universal plane types.  Rename a few functions to be more
> consistent with our function naming for primary/cursor planes or to
> clarify that they apply specifically to sprite planes:
> 
>  - s/intel_disable_plane/intel_sprite_plane_disable/
>  - s/intel_disable_planes/intel_disable_sprite_planes/
>  - s/intel_enable_planes/intel_enable_sprite_planes/
> 
> Also, drop the sprite-specific intel_destroy_plane() and just use
> the type-agnostic intel_plane_destroy() function.  The extra 'disable'
> call that intel_destroy_plane() did is unnecessary since the plane will
> already be disabled due to framebuffer destruction by the point it gets
> called.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 12 ++++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
>  drivers/gpu/drm/i915/intel_sprite.c  | 14 +++-----------
>  3 files changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c6598e9..52ca8c4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4024,7 +4024,7 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
>  	}
>  }
>  
> -static void intel_enable_planes(struct drm_crtc *crtc)
> +static void intel_enable_sprite_planes(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> @@ -4038,7 +4038,7 @@ static void intel_enable_planes(struct drm_crtc *crtc)
>  	}
>  }
>  
> -static void intel_disable_planes(struct drm_crtc *crtc)
> +static void intel_disable_sprite_planes(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> @@ -4182,7 +4182,7 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  
>  	intel_enable_primary_hw_plane(crtc->primary, crtc);
> -	intel_enable_planes(crtc);
> +	intel_enable_sprite_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
>  	intel_crtc_dpms_overlay(intel_crtc, true);
>  
> @@ -4217,7 +4217,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>  
>  	intel_crtc_dpms_overlay(intel_crtc, false);
>  	intel_crtc_update_cursor(crtc, false);
> -	intel_disable_planes(crtc);
> +	intel_disable_sprite_planes(crtc);
>  	intel_disable_primary_hw_plane(crtc->primary, crtc);
>  
>  	/*
> @@ -11744,7 +11744,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  }
>  
>  /* Common destruction function for both primary and cursor planes */
> -static void intel_plane_destroy(struct drm_plane *plane)
> +void intel_plane_destroy(struct drm_plane *plane)
>  {
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	drm_plane_cleanup(plane);
> @@ -13693,7 +13693,7 @@ void intel_plane_disable(struct drm_plane *plane)
>  		intel_cursor_plane_disable(plane);
>  		break;
>  	case DRM_PLANE_TYPE_OVERLAY:
> -		intel_disable_plane(plane);
> +		intel_sprite_plane_disable(plane);
>  		break;
>  	default:
>  		WARN(1, "Unknown plane type");
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index df1420b..4913b3f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1181,6 +1181,7 @@ int intel_plane_set_property(struct drm_plane *plane,
>  			     struct drm_property *prop,
>  			     uint64_t val);
>  int intel_plane_restore(struct drm_plane *plane);
> +void intel_plane_destroy(struct drm_plane *plane);
>  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
>  int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> @@ -1188,7 +1189,7 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
>  bool intel_pipe_update_start(struct intel_crtc *crtc,
>  			     uint32_t *start_vbl_count);
>  void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> -int intel_disable_plane(struct drm_plane *plane);
> +int intel_sprite_plane_disable(struct drm_plane *plane);
>  
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 115acd3..9e6a72a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1426,7 +1426,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  }
>  
>  int
> -intel_disable_plane(struct drm_plane *plane)
> +intel_sprite_plane_disable(struct drm_plane *plane)
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -1469,14 +1469,6 @@ intel_disable_plane(struct drm_plane *plane)
>  	return 0;
>  }
>  
> -static void intel_destroy_plane(struct drm_plane *plane)
> -{
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	intel_disable_plane(plane);
> -	drm_plane_cleanup(plane);
> -	kfree(intel_plane);
> -}
> -
>  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  			      struct drm_file *file_priv)
>  {
> @@ -1578,8 +1570,8 @@ int intel_plane_restore(struct drm_plane *plane)
>  
>  static const struct drm_plane_funcs intel_plane_funcs = {
>  	.update_plane = intel_update_plane,
> -	.disable_plane = intel_disable_plane,
> -	.destroy = intel_destroy_plane,
> +	.disable_plane = intel_sprite_plane_disable,
> +	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
>  };
>  

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/7] drm/i915: Make intel_crtc_has_pending_flip() non-static

[Matt Roper]

We'll want to use this from the atomic plane helpers, so ensure it can
be called outside intel_display.c.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_drv.h     | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 52ca8c4..2d4ef04 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2833,7 +2833,7 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
 	return ret;
 }
 
-static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
+bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4913b3f..49358c8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -921,6 +921,7 @@ void intel_finish_page_flip(struct drm_device *dev, int pipe);
 void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
 void intel_check_page_flip(struct drm_device *dev, int pipe);
 void intel_plane_disable(struct drm_plane *plane);
+bool intel_crtc_has_pending_flip(struct drm_crtc *crtc);
 
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
-- 
1.8.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/i915: Make intel_crtc_has_pending_flip() non-static

[Bob Paauwe]

On Thu, 13 Nov 2014 10:43:23 -0800
Matt Roper <matthew.d.roper@intel.com> wrote:

> We'll want to use this from the atomic plane helpers, so ensure it can
> be called outside intel_display.c.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  drivers/gpu/drm/i915/intel_drv.h     | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 52ca8c4..2d4ef04 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2833,7 +2833,7 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
>  	return ret;
>  }
>  
> -static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> +bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4913b3f..49358c8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -921,6 +921,7 @@ void intel_finish_page_flip(struct drm_device *dev, int pipe);
>  void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
>  void intel_check_page_flip(struct drm_device *dev, int pipe);
>  void intel_plane_disable(struct drm_plane *plane);
> +bool intel_crtc_has_pending_flip(struct drm_crtc *crtc);
>  
>  /* shared dpll functions */
>  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);

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

[PATCH 5/7] drm/i915: Prepare for atomic plane helpers

[Matt Roper]

Add the new driver entrypoints that will be called by the atomic plane
helpers.

This patch does not actually switch over to the new plane helpers yet,
so there should be no functional change here.  Also note that although
plane programming was already split into check/prepare/commit steps,
some of the semantics of those individual functions will need to change
slightly when we do make the jump so that they match the behavior the
plane helpers expect.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/Makefile        |   1 +
 drivers/gpu/drm/i915/intel_atomic.c  | 220 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  20 +++-
 drivers/gpu/drm/i915/intel_drv.h     |  15 +++
 drivers/gpu/drm/i915/intel_sprite.c  |  10 ++
 5 files changed, 265 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/intel_atomic.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 891e584..8b67617 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -63,6 +63,7 @@ i915-y += dvo_ch7017.o \
 	  dvo_ns2501.o \
 	  dvo_sil164.o \
 	  dvo_tfp410.o \
+	  intel_atomic.o \
 	  intel_crt.o \
 	  intel_ddi.o \
 	  intel_dp.o \
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
new file mode 100644
index 0000000..2e9b56e
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -0,0 +1,220 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_plane_helper.h>
+#include "intel_drv.h"
+
+/**
+ * intel_plane_duplicate_state - duplicate plane state
+ * @plane: drm plane
+ *
+ * Allocates and returns a copy of the plane state (both common and
+ * Intel-specific) for the specified plane.
+ */
+struct drm_plane_state *
+intel_plane_duplicate_state(struct drm_plane *plane)
+{
+	struct intel_plane_state *state;
+
+	if (plane->state)
+		state = kmemdup(plane->state, sizeof(*state), GFP_KERNEL);
+	else
+		state = kzalloc(sizeof(*state), GFP_KERNEL);
+
+	if (state && state->base.fb)
+		drm_framebuffer_reference(state->base.fb);
+
+	return &state->base;
+}
+
+/**
+ * intel_plane_destroy_state - destroy plane state
+ * @plane: drm plane
+ *
+ * Allocates and returns a copy of the plane state (both common and
+ * Intel-specific) for the specified plane.
+ */
+void
+intel_plane_destroy_state(struct drm_plane *plane,
+			  struct drm_plane_state *state)
+{
+	drm_atomic_helper_plane_destroy_state(plane, state);
+}
+
+
+/**
+ * intel_crtc_atomic_begin - Begins an atomic commit on a CRTC
+ * @crtc: drm crtc
+ *
+ * Prepares to write registers associated with the atomic commit of a CRTC
+ * by using vblank evasion to ensure that all register writes happen within
+ * the same vblank period.
+ */
+void intel_crtc_atomic_begin(struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	intel_pipe_update_start(intel_crtc, &intel_crtc->atomic_vbl_count);
+}
+
+/**
+ * intel_crtc_atomic_flush - Finishes an atomic commit on a CRTC
+ * @crtc: drm crtc
+ *
+ * Concludes the writing of registers for an atomic commit of a CRTC.
+ */
+void intel_crtc_atomic_flush(struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	intel_pipe_update_end(intel_crtc, intel_crtc->atomic_vbl_count);
+}
+
+static int intel_prepare_fb(struct drm_plane *plane,
+			    struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = plane->dev;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_plane->obj;
+	enum pipe pipe = intel_plane->pipe;
+	unsigned front_bits = 0;
+	int ret = 0;
+
+	switch (plane->type) {
+	case DRM_PLANE_TYPE_PRIMARY:
+		front_bits = INTEL_FRONTBUFFER_PRIMARY(pipe);
+
+		if (plane->crtc) {
+			intel_crtc_wait_for_pending_flips(plane->crtc);
+			if (intel_crtc_has_pending_flip(plane->crtc)) {
+				DRM_ERROR("pipe is still busy with an old pageflip\n");
+				return -EBUSY;
+			}
+		}
+
+		break;
+	case DRM_PLANE_TYPE_OVERLAY:
+		front_bits = INTEL_FRONTBUFFER_SPRITE(pipe);
+		break;
+	case DRM_PLANE_TYPE_CURSOR:
+		front_bits = INTEL_FRONTBUFFER_CURSOR(pipe);
+		break;
+	}
+
+	mutex_lock(&dev->struct_mutex);
+
+	/* Note that this will apply the VT-d workaround for scanouts,
+	 * which is more restrictive than required for sprites. (The
+	 * primary plane requires 256KiB alignment with 64 PTE padding,
+	 * the sprite planes only require 128KiB alignment and 32 PTE
+	 * padding.
+	 */
+	ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
+	if (ret == 0)
+		i915_gem_track_fb(old_obj, obj, front_bits);
+
+	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
+	    INTEL_INFO(dev)->cursor_needs_physical) {
+		int align = IS_I830(dev) ? 16 * 1024 : 256;
+
+		ret = i915_gem_object_attach_phys(obj, align);
+		if (ret)
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+	}
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return ret;
+}
+
+static void intel_cleanup_fb(struct drm_plane *plane,
+			     struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+
+	mutex_lock(&dev->struct_mutex);
+	intel_unpin_fb_obj(obj);
+	mutex_unlock(&dev->struct_mutex);
+}
+
+static int intel_plane_atomic_check(struct drm_plane *plane,
+				    struct drm_plane_state *state)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
+	struct intel_plane_state *intel_state = to_intel_plane_state(state);
+
+	/* Disabling a plane is always okay */
+	if (state->fb == NULL)
+		return 0;
+
+	/*
+	 * The original src/dest coordinates are stored in state->base, but
+	 * we want to keep another copy internal to our driver that we can
+	 * clip/modify ourselves.
+	 */
+	intel_state->src.x1 = state->src_x;
+	intel_state->src.y1 = state->src_y;
+	intel_state->src.x2 = state->src_x + state->src_w;
+	intel_state->src.y2 = state->src_y + state->src_h;
+	intel_state->dst.x1 = state->crtc_x;
+	intel_state->dst.y1 = state->crtc_y;
+	intel_state->dst.x2 = state->crtc_x + state->crtc_w;
+	intel_state->dst.y2 = state->crtc_y + state->crtc_h;
+
+	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
+	if (intel_crtc) {
+		intel_state->clip.x1 = 0;
+		intel_state->clip.y1 = 0;
+		intel_state->clip.x2 =
+			intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
+		intel_state->clip.y2 =
+			intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
+	}
+
+	return intel_plane->check_plane(plane, intel_state);
+}
+
+static void intel_plane_atomic_update(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane_state *intel_state =
+		to_intel_plane_state(plane->state);
+
+	if (!plane->state->fb)
+		intel_plane_disable(plane);
+	else
+		intel_plane->commit_plane(plane, intel_state);
+}
+
+const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
+	.prepare_fb = intel_prepare_fb,
+	.cleanup_fb = intel_cleanup_fb,
+	.atomic_check = intel_plane_atomic_check,
+	.atomic_update = intel_plane_atomic_update,
+};
+
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2d4ef04..32a2b12 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9705,6 +9705,8 @@ out_hang:
 static struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.mode_set_base_atomic = intel_pipe_set_base_atomic,
 	.load_lut = intel_crtc_load_lut,
+	.atomic_begin = intel_crtc_atomic_begin,
+	.atomic_flush = intel_crtc_atomic_flush,
 };
 
 /**
@@ -11747,6 +11749,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 void intel_plane_destroy(struct drm_plane *plane)
 {
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	intel_plane_destroy_state(plane, plane->state);
 	drm_plane_cleanup(plane);
 	kfree(intel_plane);
 }
@@ -11755,7 +11758,10 @@ static const struct drm_plane_funcs intel_primary_plane_funcs = {
 	.update_plane = intel_primary_plane_setplane,
 	.disable_plane = intel_primary_plane_disable,
 	.destroy = intel_plane_destroy,
-	.set_property = intel_plane_set_property
+	.set_property = intel_plane_set_property,
+	.atomic_duplicate_state = intel_plane_duplicate_state,
+	.atomic_destroy_state = intel_plane_destroy_state,
+
 };
 
 static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
@@ -11769,11 +11775,14 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	if (primary == NULL)
 		return NULL;
 
+	primary->base.state = intel_plane_duplicate_state(&primary->base);
 	primary->can_scale = false;
 	primary->max_downscale = 1;
 	primary->pipe = pipe;
 	primary->plane = pipe;
 	primary->rotation = BIT(DRM_ROTATE_0);
+	primary->check_plane = intel_check_primary_plane;
+	primary->commit_plane = intel_commit_primary_plane;
 	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
 		primary->plane = !pipe;
 
@@ -11802,6 +11811,8 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 				primary->rotation);
 	}
 
+	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
+
 	return &primary->base;
 }
 
@@ -11957,6 +11968,8 @@ static const struct drm_plane_funcs intel_cursor_plane_funcs = {
 	.disable_plane = intel_cursor_plane_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
+	.atomic_duplicate_state = intel_plane_duplicate_state,
+	.atomic_destroy_state = intel_plane_destroy_state,
 };
 
 static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
@@ -11968,11 +11981,14 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	if (cursor == NULL)
 		return NULL;
 
+	cursor->base.state = intel_plane_duplicate_state(&cursor->base);
 	cursor->can_scale = false;
 	cursor->max_downscale = 1;
 	cursor->pipe = pipe;
 	cursor->plane = pipe;
 	cursor->rotation = BIT(DRM_ROTATE_0);
+	cursor->check_plane = intel_check_cursor_plane;
+	cursor->commit_plane = intel_commit_cursor_plane;
 
 	drm_universal_plane_init(dev, &cursor->base, 0,
 				 &intel_cursor_plane_funcs,
@@ -11992,6 +12008,8 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 				cursor->rotation);
 	}
 
+	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
+
 	return &cursor->base;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 49358c8..1882de1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -466,6 +466,8 @@ struct intel_crtc {
 
 	int scanline_offset;
 	struct intel_mmio_flip mmio_flip;
+
+	uint32_t atomic_vbl_count;
 };
 
 struct intel_plane_wm_parameters {
@@ -506,6 +508,10 @@ struct intel_plane {
 			     uint32_t src_w, uint32_t src_h);
 	void (*disable_plane)(struct drm_plane *plane,
 			      struct drm_crtc *crtc);
+	int (*check_plane)(struct drm_plane *plane,
+			   struct intel_plane_state *state);
+	void (*commit_plane)(struct drm_plane *plane,
+			     struct intel_plane_state *state);
 	int (*update_colorkey)(struct drm_plane *plane,
 			       struct drm_intel_sprite_colorkey *key);
 	void (*get_colorkey)(struct drm_plane *plane,
@@ -536,6 +542,7 @@ struct cxsr_latency {
 #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
 #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
 #define to_intel_plane(x) container_of(x, struct intel_plane, base)
+#define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
 #define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
 
 struct intel_hdmi {
@@ -1195,4 +1202,12 @@ int intel_sprite_plane_disable(struct drm_plane *plane);
 /* intel_tv.c */
 void intel_tv_init(struct drm_device *dev);
 
+/* intel_atomic.c */
+struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
+void intel_plane_destroy_state(struct drm_plane *plane,
+			       struct drm_plane_state *state);
+void intel_crtc_atomic_begin(struct drm_crtc *crtc);
+void intel_crtc_atomic_flush(struct drm_crtc *crtc);
+extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9e6a72a..d3677c3 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -33,6 +33,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_rect.h>
+#include <drm/drm_plane_helper.h>
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
@@ -1573,6 +1574,8 @@ static const struct drm_plane_funcs intel_plane_funcs = {
 	.disable_plane = intel_sprite_plane_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
+	.atomic_duplicate_state = intel_plane_duplicate_state,
+	.atomic_destroy_state = intel_plane_destroy_state,
 };
 
 static uint32_t ilk_plane_formats[] = {
@@ -1634,6 +1637,9 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	if (!intel_plane)
 		return -ENOMEM;
 
+	intel_plane->base.state =
+		intel_plane_duplicate_state(&intel_plane->base);
+
 	switch (INTEL_INFO(dev)->gen) {
 	case 5:
 	case 6:
@@ -1704,6 +1710,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	intel_plane->pipe = pipe;
 	intel_plane->plane = plane;
 	intel_plane->rotation = BIT(DRM_ROTATE_0);
+	intel_plane->check_plane = intel_check_sprite_plane;
+	intel_plane->commit_plane = intel_commit_sprite_plane;
 	possible_crtcs = (1 << pipe);
 	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
 				       &intel_plane_funcs,
@@ -1725,6 +1733,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 					   dev->mode_config.rotation_property,
 					   intel_plane->rotation);
 
+	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
+
  out:
 	return ret;
 }
-- 
1.8.5.1

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

Re: [PATCH 5/7] drm/i915: Prepare for atomic plane helpers

[Bob Paauwe]

On Thu, 13 Nov 2014 10:43:24 -0800
Matt Roper <matthew.d.roper@intel.com> wrote:

> Add the new driver entrypoints that will be called by the atomic plane
> helpers.
> 
> This patch does not actually switch over to the new plane helpers yet,
> so there should be no functional change here.  Also note that although
> plane programming was already split into check/prepare/commit steps,
> some of the semantics of those individual functions will need to change
> slightly when we do make the jump so that they match the behavior the
> plane helpers expect.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile        |   1 +
>  drivers/gpu/drm/i915/intel_atomic.c  | 220 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  20 +++-
>  drivers/gpu/drm/i915/intel_drv.h     |  15 +++
>  drivers/gpu/drm/i915/intel_sprite.c  |  10 ++
>  5 files changed, 265 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_atomic.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 891e584..8b67617 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -63,6 +63,7 @@ i915-y += dvo_ch7017.o \
>  	  dvo_ns2501.o \
>  	  dvo_sil164.o \
>  	  dvo_tfp410.o \
> +	  intel_atomic.o \
>  	  intel_crt.o \
>  	  intel_ddi.o \
>  	  intel_dp.o \
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> new file mode 100644
> index 0000000..2e9b56e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -0,0 +1,220 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include "intel_drv.h"
> +
> +/**
> + * intel_plane_duplicate_state - duplicate plane state
> + * @plane: drm plane
> + *
> + * Allocates and returns a copy of the plane state (both common and
> + * Intel-specific) for the specified plane.
> + */
> +struct drm_plane_state *
> +intel_plane_duplicate_state(struct drm_plane *plane)
> +{
> +	struct intel_plane_state *state;
> +
> +	if (plane->state)
> +		state = kmemdup(plane->state, sizeof(*state), GFP_KERNEL);
> +	else
> +		state = kzalloc(sizeof(*state), GFP_KERNEL);
> +
> +	if (state && state->base.fb)
> +		drm_framebuffer_reference(state->base.fb);
> +
> +	return &state->base;
> +}
> +
> +/**
> + * intel_plane_destroy_state - destroy plane state
> + * @plane: drm plane
> + *
> + * Allocates and returns a copy of the plane state (both common and
> + * Intel-specific) for the specified plane.

Comment should be updated for destroy.

> + */
> +void
> +intel_plane_destroy_state(struct drm_plane *plane,
> +			  struct drm_plane_state *state)
> +{
> +	drm_atomic_helper_plane_destroy_state(plane, state);
> +}
> +
> +
> +/**
> + * intel_crtc_atomic_begin - Begins an atomic commit on a CRTC
> + * @crtc: drm crtc
> + *
> + * Prepares to write registers associated with the atomic commit of a CRTC
> + * by using vblank evasion to ensure that all register writes happen within
> + * the same vblank period.
> + */
> +void intel_crtc_atomic_begin(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	intel_pipe_update_start(intel_crtc, &intel_crtc->atomic_vbl_count);
> +}
> +
> +/**
> + * intel_crtc_atomic_flush - Finishes an atomic commit on a CRTC
> + * @crtc: drm crtc
> + *
> + * Concludes the writing of registers for an atomic commit of a CRTC.
> + */
> +void intel_crtc_atomic_flush(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	intel_pipe_update_end(intel_crtc, intel_crtc->atomic_vbl_count);
> +}
> +
> +static int intel_prepare_fb(struct drm_plane *plane,
> +			    struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> +	enum pipe pipe = intel_plane->pipe;
> +	unsigned front_bits = 0;
> +	int ret = 0;
> +
> +	switch (plane->type) {
> +	case DRM_PLANE_TYPE_PRIMARY:
> +		front_bits = INTEL_FRONTBUFFER_PRIMARY(pipe);
> +
> +		if (plane->crtc) {
> +			intel_crtc_wait_for_pending_flips(plane->crtc);
> +			if (intel_crtc_has_pending_flip(plane->crtc)) {
> +				DRM_ERROR("pipe is still busy with an old pageflip\n");
> +				return -EBUSY;
> +			}
> +		}
> +
> +		break;
> +	case DRM_PLANE_TYPE_OVERLAY:
> +		front_bits = INTEL_FRONTBUFFER_SPRITE(pipe);
> +		break;
> +	case DRM_PLANE_TYPE_CURSOR:
> +		front_bits = INTEL_FRONTBUFFER_CURSOR(pipe);
> +		break;
> +	}
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	/* Note that this will apply the VT-d workaround for scanouts,
> +	 * which is more restrictive than required for sprites. (The
> +	 * primary plane requires 256KiB alignment with 64 PTE padding,
> +	 * the sprite planes only require 128KiB alignment and 32 PTE
> +	 * padding.
> +	 */
> +	ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> +	if (ret == 0)
> +		i915_gem_track_fb(old_obj, obj, front_bits);
> +
> +	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> +	    INTEL_INFO(dev)->cursor_needs_physical) {
> +		int align = IS_I830(dev) ? 16 * 1024 : 256;
> +
> +		ret = i915_gem_object_attach_phys(obj, align);
> +		if (ret)
> +			DRM_DEBUG_KMS("failed to attach phys object\n");
> +	}
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return ret;
> +}
> +
> +static void intel_cleanup_fb(struct drm_plane *plane,
> +			     struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +
> +	mutex_lock(&dev->struct_mutex);
> +	intel_unpin_fb_obj(obj);
> +	mutex_unlock(&dev->struct_mutex);
> +}
> +
> +static int intel_plane_atomic_check(struct drm_plane *plane,
> +				    struct drm_plane_state *state)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
> +	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> +
> +	/* Disabling a plane is always okay */
> +	if (state->fb == NULL)
> +		return 0;
> +
> +	/*
> +	 * The original src/dest coordinates are stored in state->base, but
> +	 * we want to keep another copy internal to our driver that we can
> +	 * clip/modify ourselves.
> +	 */
> +	intel_state->src.x1 = state->src_x;
> +	intel_state->src.y1 = state->src_y;
> +	intel_state->src.x2 = state->src_x + state->src_w;
> +	intel_state->src.y2 = state->src_y + state->src_h;
> +	intel_state->dst.x1 = state->crtc_x;
> +	intel_state->dst.y1 = state->crtc_y;
> +	intel_state->dst.x2 = state->crtc_x + state->crtc_w;
> +	intel_state->dst.y2 = state->crtc_y + state->crtc_h;
> +
> +	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> +	if (intel_crtc) {
> +		intel_state->clip.x1 = 0;
> +		intel_state->clip.y1 = 0;
> +		intel_state->clip.x2 =
> +			intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> +		intel_state->clip.y2 =
> +			intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;

Should this be using intel_crtc->new_config here?  I'm not entierly
clear on how the crtc config/new_config would tie into the atomic state.

> +	}
> +
> +	return intel_plane->check_plane(plane, intel_state);
> +}
> +
> +static void intel_plane_atomic_update(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *intel_state =
> +		to_intel_plane_state(plane->state);
> +
> +	if (!plane->state->fb)
> +		intel_plane_disable(plane);
> +	else
> +		intel_plane->commit_plane(plane, intel_state);
> +}
> +
> +const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
> +	.prepare_fb = intel_prepare_fb,
> +	.cleanup_fb = intel_cleanup_fb,
> +	.atomic_check = intel_plane_atomic_check,
> +	.atomic_update = intel_plane_atomic_update,
> +};
> +
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2d4ef04..32a2b12 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9705,6 +9705,8 @@ out_hang:
>  static struct drm_crtc_helper_funcs intel_helper_funcs = {
>  	.mode_set_base_atomic = intel_pipe_set_base_atomic,
>  	.load_lut = intel_crtc_load_lut,
> +	.atomic_begin = intel_crtc_atomic_begin,
> +	.atomic_flush = intel_crtc_atomic_flush,
>  };
>  
>  /**
> @@ -11747,6 +11749,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  void intel_plane_destroy(struct drm_plane *plane)
>  {
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	intel_plane_destroy_state(plane, plane->state);
>  	drm_plane_cleanup(plane);
>  	kfree(intel_plane);
>  }
> @@ -11755,7 +11758,10 @@ static const struct drm_plane_funcs intel_primary_plane_funcs = {
>  	.update_plane = intel_primary_plane_setplane,
>  	.disable_plane = intel_primary_plane_disable,
>  	.destroy = intel_plane_destroy,
> -	.set_property = intel_plane_set_property
> +	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
> +
>  };
>  
>  static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> @@ -11769,11 +11775,14 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	if (primary == NULL)
>  		return NULL;
>  
> +	primary->base.state = intel_plane_duplicate_state(&primary->base);
>  	primary->can_scale = false;
>  	primary->max_downscale = 1;
>  	primary->pipe = pipe;
>  	primary->plane = pipe;
>  	primary->rotation = BIT(DRM_ROTATE_0);
> +	primary->check_plane = intel_check_primary_plane;
> +	primary->commit_plane = intel_commit_primary_plane;
>  	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
>  		primary->plane = !pipe;
>  
> @@ -11802,6 +11811,8 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  				primary->rotation);
>  	}
>  
> +	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> +
>  	return &primary->base;
>  }
>  
> @@ -11957,6 +11968,8 @@ static const struct drm_plane_funcs intel_cursor_plane_funcs = {
>  	.disable_plane = intel_cursor_plane_disable,
>  	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
>  };
>  
>  static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> @@ -11968,11 +11981,14 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  	if (cursor == NULL)
>  		return NULL;
>  
> +	cursor->base.state = intel_plane_duplicate_state(&cursor->base);
>  	cursor->can_scale = false;
>  	cursor->max_downscale = 1;
>  	cursor->pipe = pipe;
>  	cursor->plane = pipe;
>  	cursor->rotation = BIT(DRM_ROTATE_0);
> +	cursor->check_plane = intel_check_cursor_plane;
> +	cursor->commit_plane = intel_commit_cursor_plane;
>  
>  	drm_universal_plane_init(dev, &cursor->base, 0,
>  				 &intel_cursor_plane_funcs,
> @@ -11992,6 +12008,8 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  				cursor->rotation);
>  	}
>  
> +	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
> +
>  	return &cursor->base;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 49358c8..1882de1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -466,6 +466,8 @@ struct intel_crtc {
>  
>  	int scanline_offset;
>  	struct intel_mmio_flip mmio_flip;
> +
> +	uint32_t atomic_vbl_count;
>  };
>  
>  struct intel_plane_wm_parameters {
> @@ -506,6 +508,10 @@ struct intel_plane {
>  			     uint32_t src_w, uint32_t src_h);
>  	void (*disable_plane)(struct drm_plane *plane,
>  			      struct drm_crtc *crtc);
> +	int (*check_plane)(struct drm_plane *plane,
> +			   struct intel_plane_state *state);
> +	void (*commit_plane)(struct drm_plane *plane,
> +			     struct intel_plane_state *state);
>  	int (*update_colorkey)(struct drm_plane *plane,
>  			       struct drm_intel_sprite_colorkey *key);
>  	void (*get_colorkey)(struct drm_plane *plane,
> @@ -536,6 +542,7 @@ struct cxsr_latency {
>  #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
>  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
>  #define to_intel_plane(x) container_of(x, struct intel_plane, base)
> +#define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
>  #define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
>  
>  struct intel_hdmi {
> @@ -1195,4 +1202,12 @@ int intel_sprite_plane_disable(struct drm_plane *plane);
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_device *dev);
>  
> +/* intel_atomic.c */
> +struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
> +void intel_plane_destroy_state(struct drm_plane *plane,
> +			       struct drm_plane_state *state);
> +void intel_crtc_atomic_begin(struct drm_crtc *crtc);
> +void intel_crtc_atomic_flush(struct drm_crtc *crtc);
> +extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> +
>  #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 9e6a72a..d3677c3 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -33,6 +33,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_rect.h>
> +#include <drm/drm_plane_helper.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
> @@ -1573,6 +1574,8 @@ static const struct drm_plane_funcs intel_plane_funcs = {
>  	.disable_plane = intel_sprite_plane_disable,
>  	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
>  };
>  
>  static uint32_t ilk_plane_formats[] = {
> @@ -1634,6 +1637,9 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  	if (!intel_plane)
>  		return -ENOMEM;
>  
> +	intel_plane->base.state =
> +		intel_plane_duplicate_state(&intel_plane->base);
> +
>  	switch (INTEL_INFO(dev)->gen) {
>  	case 5:
>  	case 6:
> @@ -1704,6 +1710,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  	intel_plane->pipe = pipe;
>  	intel_plane->plane = plane;
>  	intel_plane->rotation = BIT(DRM_ROTATE_0);
> +	intel_plane->check_plane = intel_check_sprite_plane;
> +	intel_plane->commit_plane = intel_commit_sprite_plane;
>  	possible_crtcs = (1 << pipe);
>  	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
>  				       &intel_plane_funcs,
> @@ -1725,6 +1733,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  					   dev->mode_config.rotation_property,
>  					   intel_plane->rotation);
>  
> +	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> +
>   out:
>  	return ret;
>  }

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

Re: [PATCH 5/7] drm/i915: Prepare for atomic plane helpers

[Matt Roper]

On Thu, Nov 13, 2014 at 11:46:25AM -0800, Bob Paauwe wrote:
> On Thu, 13 Nov 2014 10:43:24 -0800
> Matt Roper <matthew.d.roper@intel.com> wrote:
...
> > +
> > +/**
> > + * intel_plane_destroy_state - destroy plane state
> > + * @plane: drm plane
> > + *
> > + * Allocates and returns a copy of the plane state (both common and
> > + * Intel-specific) for the specified plane.
> 
> Comment should be updated for destroy.

Good catch; will update.

...
> > +static int intel_plane_atomic_check(struct drm_plane *plane,
> > +				    struct drm_plane_state *state)
> > +{
> > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
> > +	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> > +
> > +	/* Disabling a plane is always okay */
> > +	if (state->fb == NULL)
> > +		return 0;
> > +
> > +	/*
> > +	 * The original src/dest coordinates are stored in state->base, but
> > +	 * we want to keep another copy internal to our driver that we can
> > +	 * clip/modify ourselves.
> > +	 */
> > +	intel_state->src.x1 = state->src_x;
> > +	intel_state->src.y1 = state->src_y;
> > +	intel_state->src.x2 = state->src_x + state->src_w;
> > +	intel_state->src.y2 = state->src_y + state->src_h;
> > +	intel_state->dst.x1 = state->crtc_x;
> > +	intel_state->dst.y1 = state->crtc_y;
> > +	intel_state->dst.x2 = state->crtc_x + state->crtc_w;
> > +	intel_state->dst.y2 = state->crtc_y + state->crtc_h;
> > +
> > +	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> > +	if (intel_crtc) {
> > +		intel_state->clip.x1 = 0;
> > +		intel_state->clip.y1 = 0;
> > +		intel_state->clip.x2 =
> > +			intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> > +		intel_state->clip.y2 =
> > +			intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> 
> Should this be using intel_crtc->new_config here?  I'm not entierly
> clear on how the crtc config/new_config would tie into the atomic state.

I don't think so.  config vs new_config tracks pipe configuration during
the modeset sequence, but we're only doing a plane update here, so the
current crtc configuration is what we'd clip against.  I haven't
converted over any of the CRTC modeset handling, just the plane updates.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/8] drm/i915: Prepare for atomic plane helpers (v2)

[Matt Roper]

Add the new driver entrypoints that will be called by the atomic plane
helpers.

This patch does not actually switch over to the new plane helpers yet,
so there should be no functional change here.  Also note that although
plane programming was already split into check/prepare/commit steps,
some of the semantics of those individual functions will need to change
slightly when we do make the jump so that they match the behavior the
plane helpers expect.

v2:
 - Renamed file from intel_atomic.c to intel_atomic_plane.c (Daniel)
 - Fix a copy/paste comment mistake (Bob)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/Makefile             |   1 +
 drivers/gpu/drm/i915/intel_atomic_plane.c | 220 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c      |  20 ++-
 drivers/gpu/drm/i915/intel_drv.h          |  15 ++
 drivers/gpu/drm/i915/intel_sprite.c       |  10 ++
 5 files changed, 265 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/intel_atomic_plane.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 891e584..9f5c57d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -63,6 +63,7 @@ i915-y += dvo_ch7017.o \
 	  dvo_ns2501.o \
 	  dvo_sil164.o \
 	  dvo_tfp410.o \
+	  intel_atomic_plane.o \
 	  intel_crt.o \
 	  intel_ddi.o \
 	  intel_dp.o \
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
new file mode 100644
index 0000000..7c039bf
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -0,0 +1,220 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_plane_helper.h>
+#include "intel_drv.h"
+
+/**
+ * intel_plane_duplicate_state - duplicate plane state
+ * @plane: drm plane
+ *
+ * Allocates and returns a copy of the plane state (both common and
+ * Intel-specific) for the specified plane.
+ */
+struct drm_plane_state *
+intel_plane_duplicate_state(struct drm_plane *plane)
+{
+	struct intel_plane_state *state;
+
+	if (plane->state)
+		state = kmemdup(plane->state, sizeof(*state), GFP_KERNEL);
+	else
+		state = kzalloc(sizeof(*state), GFP_KERNEL);
+
+	if (state && state->base.fb)
+		drm_framebuffer_reference(state->base.fb);
+
+	return &state->base;
+}
+
+/**
+ * intel_plane_destroy_state - destroy plane state
+ * @plane: drm plane
+ *
+ * Destroys the plane state (both common and Intel-specific) for the
+ * specified plane.
+ */
+void
+intel_plane_destroy_state(struct drm_plane *plane,
+			  struct drm_plane_state *state)
+{
+	drm_atomic_helper_plane_destroy_state(plane, state);
+}
+
+
+/**
+ * intel_crtc_atomic_begin - Begins an atomic commit on a CRTC
+ * @crtc: drm crtc
+ *
+ * Prepares to write registers associated with the atomic commit of a CRTC
+ * by using vblank evasion to ensure that all register writes happen within
+ * the same vblank period.
+ */
+void intel_crtc_atomic_begin(struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	intel_pipe_update_start(intel_crtc, &intel_crtc->atomic_vbl_count);
+}
+
+/**
+ * intel_crtc_atomic_flush - Finishes an atomic commit on a CRTC
+ * @crtc: drm crtc
+ *
+ * Concludes the writing of registers for an atomic commit of a CRTC.
+ */
+void intel_crtc_atomic_flush(struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	intel_pipe_update_end(intel_crtc, intel_crtc->atomic_vbl_count);
+}
+
+static int intel_prepare_fb(struct drm_plane *plane,
+			    struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = plane->dev;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_plane->obj;
+	enum pipe pipe = intel_plane->pipe;
+	unsigned front_bits = 0;
+	int ret = 0;
+
+	switch (plane->type) {
+	case DRM_PLANE_TYPE_PRIMARY:
+		front_bits = INTEL_FRONTBUFFER_PRIMARY(pipe);
+
+		if (plane->crtc) {
+			intel_crtc_wait_for_pending_flips(plane->crtc);
+			if (intel_crtc_has_pending_flip(plane->crtc)) {
+				DRM_ERROR("pipe is still busy with an old pageflip\n");
+				return -EBUSY;
+			}
+		}
+
+		break;
+	case DRM_PLANE_TYPE_OVERLAY:
+		front_bits = INTEL_FRONTBUFFER_SPRITE(pipe);
+		break;
+	case DRM_PLANE_TYPE_CURSOR:
+		front_bits = INTEL_FRONTBUFFER_CURSOR(pipe);
+		break;
+	}
+
+	mutex_lock(&dev->struct_mutex);
+
+	/* Note that this will apply the VT-d workaround for scanouts,
+	 * which is more restrictive than required for sprites. (The
+	 * primary plane requires 256KiB alignment with 64 PTE padding,
+	 * the sprite planes only require 128KiB alignment and 32 PTE
+	 * padding.
+	 */
+	ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
+	if (ret == 0)
+		i915_gem_track_fb(old_obj, obj, front_bits);
+
+	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
+	    INTEL_INFO(dev)->cursor_needs_physical) {
+		int align = IS_I830(dev) ? 16 * 1024 : 256;
+
+		ret = i915_gem_object_attach_phys(obj, align);
+		if (ret)
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+	}
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return ret;
+}
+
+static void intel_cleanup_fb(struct drm_plane *plane,
+			     struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+
+	mutex_lock(&dev->struct_mutex);
+	intel_unpin_fb_obj(obj);
+	mutex_unlock(&dev->struct_mutex);
+}
+
+static int intel_plane_atomic_check(struct drm_plane *plane,
+				    struct drm_plane_state *state)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
+	struct intel_plane_state *intel_state = to_intel_plane_state(state);
+
+	/* Disabling a plane is always okay */
+	if (state->fb == NULL)
+		return 0;
+
+	/*
+	 * The original src/dest coordinates are stored in state->base, but
+	 * we want to keep another copy internal to our driver that we can
+	 * clip/modify ourselves.
+	 */
+	intel_state->src.x1 = state->src_x;
+	intel_state->src.y1 = state->src_y;
+	intel_state->src.x2 = state->src_x + state->src_w;
+	intel_state->src.y2 = state->src_y + state->src_h;
+	intel_state->dst.x1 = state->crtc_x;
+	intel_state->dst.y1 = state->crtc_y;
+	intel_state->dst.x2 = state->crtc_x + state->crtc_w;
+	intel_state->dst.y2 = state->crtc_y + state->crtc_h;
+
+	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
+	if (intel_crtc) {
+		intel_state->clip.x1 = 0;
+		intel_state->clip.y1 = 0;
+		intel_state->clip.x2 =
+			intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
+		intel_state->clip.y2 =
+			intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
+	}
+
+	return intel_plane->check_plane(plane, intel_state);
+}
+
+static void intel_plane_atomic_update(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane_state *intel_state =
+		to_intel_plane_state(plane->state);
+
+	if (!plane->state->fb)
+		intel_plane_disable(plane);
+	else
+		intel_plane->commit_plane(plane, intel_state);
+}
+
+const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
+	.prepare_fb = intel_prepare_fb,
+	.cleanup_fb = intel_cleanup_fb,
+	.atomic_check = intel_plane_atomic_check,
+	.atomic_update = intel_plane_atomic_update,
+};
+
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2d4ef04..32a2b12 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9705,6 +9705,8 @@ out_hang:
 static struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.mode_set_base_atomic = intel_pipe_set_base_atomic,
 	.load_lut = intel_crtc_load_lut,
+	.atomic_begin = intel_crtc_atomic_begin,
+	.atomic_flush = intel_crtc_atomic_flush,
 };
 
 /**
@@ -11747,6 +11749,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 void intel_plane_destroy(struct drm_plane *plane)
 {
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	intel_plane_destroy_state(plane, plane->state);
 	drm_plane_cleanup(plane);
 	kfree(intel_plane);
 }
@@ -11755,7 +11758,10 @@ static const struct drm_plane_funcs intel_primary_plane_funcs = {
 	.update_plane = intel_primary_plane_setplane,
 	.disable_plane = intel_primary_plane_disable,
 	.destroy = intel_plane_destroy,
-	.set_property = intel_plane_set_property
+	.set_property = intel_plane_set_property,
+	.atomic_duplicate_state = intel_plane_duplicate_state,
+	.atomic_destroy_state = intel_plane_destroy_state,
+
 };
 
 static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
@@ -11769,11 +11775,14 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	if (primary == NULL)
 		return NULL;
 
+	primary->base.state = intel_plane_duplicate_state(&primary->base);
 	primary->can_scale = false;
 	primary->max_downscale = 1;
 	primary->pipe = pipe;
 	primary->plane = pipe;
 	primary->rotation = BIT(DRM_ROTATE_0);
+	primary->check_plane = intel_check_primary_plane;
+	primary->commit_plane = intel_commit_primary_plane;
 	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
 		primary->plane = !pipe;
 
@@ -11802,6 +11811,8 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 				primary->rotation);
 	}
 
+	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
+
 	return &primary->base;
 }
 
@@ -11957,6 +11968,8 @@ static const struct drm_plane_funcs intel_cursor_plane_funcs = {
 	.disable_plane = intel_cursor_plane_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
+	.atomic_duplicate_state = intel_plane_duplicate_state,
+	.atomic_destroy_state = intel_plane_destroy_state,
 };
 
 static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
@@ -11968,11 +11981,14 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	if (cursor == NULL)
 		return NULL;
 
+	cursor->base.state = intel_plane_duplicate_state(&cursor->base);
 	cursor->can_scale = false;
 	cursor->max_downscale = 1;
 	cursor->pipe = pipe;
 	cursor->plane = pipe;
 	cursor->rotation = BIT(DRM_ROTATE_0);
+	cursor->check_plane = intel_check_cursor_plane;
+	cursor->commit_plane = intel_commit_cursor_plane;
 
 	drm_universal_plane_init(dev, &cursor->base, 0,
 				 &intel_cursor_plane_funcs,
@@ -11992,6 +12008,8 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 				cursor->rotation);
 	}
 
+	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
+
 	return &cursor->base;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 49358c8..1882de1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -466,6 +466,8 @@ struct intel_crtc {
 
 	int scanline_offset;
 	struct intel_mmio_flip mmio_flip;
+
+	uint32_t atomic_vbl_count;
 };
 
 struct intel_plane_wm_parameters {
@@ -506,6 +508,10 @@ struct intel_plane {
 			     uint32_t src_w, uint32_t src_h);
 	void (*disable_plane)(struct drm_plane *plane,
 			      struct drm_crtc *crtc);
+	int (*check_plane)(struct drm_plane *plane,
+			   struct intel_plane_state *state);
+	void (*commit_plane)(struct drm_plane *plane,
+			     struct intel_plane_state *state);
 	int (*update_colorkey)(struct drm_plane *plane,
 			       struct drm_intel_sprite_colorkey *key);
 	void (*get_colorkey)(struct drm_plane *plane,
@@ -536,6 +542,7 @@ struct cxsr_latency {
 #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
 #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
 #define to_intel_plane(x) container_of(x, struct intel_plane, base)
+#define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
 #define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
 
 struct intel_hdmi {
@@ -1195,4 +1202,12 @@ int intel_sprite_plane_disable(struct drm_plane *plane);
 /* intel_tv.c */
 void intel_tv_init(struct drm_device *dev);
 
+/* intel_atomic.c */
+struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
+void intel_plane_destroy_state(struct drm_plane *plane,
+			       struct drm_plane_state *state);
+void intel_crtc_atomic_begin(struct drm_crtc *crtc);
+void intel_crtc_atomic_flush(struct drm_crtc *crtc);
+extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9e6a72a..d3677c3 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -33,6 +33,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_rect.h>
+#include <drm/drm_plane_helper.h>
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
@@ -1573,6 +1574,8 @@ static const struct drm_plane_funcs intel_plane_funcs = {
 	.disable_plane = intel_sprite_plane_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
+	.atomic_duplicate_state = intel_plane_duplicate_state,
+	.atomic_destroy_state = intel_plane_destroy_state,
 };
 
 static uint32_t ilk_plane_formats[] = {
@@ -1634,6 +1637,9 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	if (!intel_plane)
 		return -ENOMEM;
 
+	intel_plane->base.state =
+		intel_plane_duplicate_state(&intel_plane->base);
+
 	switch (INTEL_INFO(dev)->gen) {
 	case 5:
 	case 6:
@@ -1704,6 +1710,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	intel_plane->pipe = pipe;
 	intel_plane->plane = plane;
 	intel_plane->rotation = BIT(DRM_ROTATE_0);
+	intel_plane->check_plane = intel_check_sprite_plane;
+	intel_plane->commit_plane = intel_commit_sprite_plane;
 	possible_crtcs = (1 << pipe);
 	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
 				       &intel_plane_funcs,
@@ -1725,6 +1733,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 					   dev->mode_config.rotation_property,
 					   intel_plane->rotation);
 
+	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
+
  out:
 	return ret;
 }
-- 
1.8.5.1

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

Re: [PATCH 5/8] drm/i915: Prepare for atomic plane helpers (v2)

[Matt Roper]

On Thu, Nov 13, 2014 at 02:51:32PM -0800, Matt Roper wrote:
> Add the new driver entrypoints that will be called by the atomic plane
> helpers.
> 
> This patch does not actually switch over to the new plane helpers yet,
> so there should be no functional change here.  Also note that although
> plane programming was already split into check/prepare/commit steps,
> some of the semantics of those individual functions will need to change
> slightly when we do make the jump so that they match the behavior the
> plane helpers expect.
> 
> v2:
>  - Renamed file from intel_atomic.c to intel_atomic_plane.c (Daniel)
>  - Fix a copy/paste comment mistake (Bob)
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Actually, I don't think this is quite ready to go yet.  I must have had
too much debug spam turned on before and missed the important messages
in the kernel log, but it looks like we're still calling something that
can sleep (in the frontbuffer tracking code?) while irqs are disabled
for vblank evasion.

I'll have to look into this some more tomorrow and decide what an
appropriate fix is.


Matt

> ---
>  drivers/gpu/drm/i915/Makefile             |   1 +
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 220 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c      |  20 ++-
>  drivers/gpu/drm/i915/intel_drv.h          |  15 ++
>  drivers/gpu/drm/i915/intel_sprite.c       |  10 ++
>  5 files changed, 265 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_atomic_plane.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 891e584..9f5c57d 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -63,6 +63,7 @@ i915-y += dvo_ch7017.o \
>  	  dvo_ns2501.o \
>  	  dvo_sil164.o \
>  	  dvo_tfp410.o \
> +	  intel_atomic_plane.o \
>  	  intel_crt.o \
>  	  intel_ddi.o \
>  	  intel_dp.o \
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> new file mode 100644
> index 0000000..7c039bf
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -0,0 +1,220 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include "intel_drv.h"
> +
> +/**
> + * intel_plane_duplicate_state - duplicate plane state
> + * @plane: drm plane
> + *
> + * Allocates and returns a copy of the plane state (both common and
> + * Intel-specific) for the specified plane.
> + */
> +struct drm_plane_state *
> +intel_plane_duplicate_state(struct drm_plane *plane)
> +{
> +	struct intel_plane_state *state;
> +
> +	if (plane->state)
> +		state = kmemdup(plane->state, sizeof(*state), GFP_KERNEL);
> +	else
> +		state = kzalloc(sizeof(*state), GFP_KERNEL);
> +
> +	if (state && state->base.fb)
> +		drm_framebuffer_reference(state->base.fb);
> +
> +	return &state->base;
> +}
> +
> +/**
> + * intel_plane_destroy_state - destroy plane state
> + * @plane: drm plane
> + *
> + * Destroys the plane state (both common and Intel-specific) for the
> + * specified plane.
> + */
> +void
> +intel_plane_destroy_state(struct drm_plane *plane,
> +			  struct drm_plane_state *state)
> +{
> +	drm_atomic_helper_plane_destroy_state(plane, state);
> +}
> +
> +
> +/**
> + * intel_crtc_atomic_begin - Begins an atomic commit on a CRTC
> + * @crtc: drm crtc
> + *
> + * Prepares to write registers associated with the atomic commit of a CRTC
> + * by using vblank evasion to ensure that all register writes happen within
> + * the same vblank period.
> + */
> +void intel_crtc_atomic_begin(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	intel_pipe_update_start(intel_crtc, &intel_crtc->atomic_vbl_count);
> +}
> +
> +/**
> + * intel_crtc_atomic_flush - Finishes an atomic commit on a CRTC
> + * @crtc: drm crtc
> + *
> + * Concludes the writing of registers for an atomic commit of a CRTC.
> + */
> +void intel_crtc_atomic_flush(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	intel_pipe_update_end(intel_crtc, intel_crtc->atomic_vbl_count);
> +}
> +
> +static int intel_prepare_fb(struct drm_plane *plane,
> +			    struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> +	enum pipe pipe = intel_plane->pipe;
> +	unsigned front_bits = 0;
> +	int ret = 0;
> +
> +	switch (plane->type) {
> +	case DRM_PLANE_TYPE_PRIMARY:
> +		front_bits = INTEL_FRONTBUFFER_PRIMARY(pipe);
> +
> +		if (plane->crtc) {
> +			intel_crtc_wait_for_pending_flips(plane->crtc);
> +			if (intel_crtc_has_pending_flip(plane->crtc)) {
> +				DRM_ERROR("pipe is still busy with an old pageflip\n");
> +				return -EBUSY;
> +			}
> +		}
> +
> +		break;
> +	case DRM_PLANE_TYPE_OVERLAY:
> +		front_bits = INTEL_FRONTBUFFER_SPRITE(pipe);
> +		break;
> +	case DRM_PLANE_TYPE_CURSOR:
> +		front_bits = INTEL_FRONTBUFFER_CURSOR(pipe);
> +		break;
> +	}
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	/* Note that this will apply the VT-d workaround for scanouts,
> +	 * which is more restrictive than required for sprites. (The
> +	 * primary plane requires 256KiB alignment with 64 PTE padding,
> +	 * the sprite planes only require 128KiB alignment and 32 PTE
> +	 * padding.
> +	 */
> +	ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> +	if (ret == 0)
> +		i915_gem_track_fb(old_obj, obj, front_bits);
> +
> +	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> +	    INTEL_INFO(dev)->cursor_needs_physical) {
> +		int align = IS_I830(dev) ? 16 * 1024 : 256;
> +
> +		ret = i915_gem_object_attach_phys(obj, align);
> +		if (ret)
> +			DRM_DEBUG_KMS("failed to attach phys object\n");
> +	}
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return ret;
> +}
> +
> +static void intel_cleanup_fb(struct drm_plane *plane,
> +			     struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +
> +	mutex_lock(&dev->struct_mutex);
> +	intel_unpin_fb_obj(obj);
> +	mutex_unlock(&dev->struct_mutex);
> +}
> +
> +static int intel_plane_atomic_check(struct drm_plane *plane,
> +				    struct drm_plane_state *state)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
> +	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> +
> +	/* Disabling a plane is always okay */
> +	if (state->fb == NULL)
> +		return 0;
> +
> +	/*
> +	 * The original src/dest coordinates are stored in state->base, but
> +	 * we want to keep another copy internal to our driver that we can
> +	 * clip/modify ourselves.
> +	 */
> +	intel_state->src.x1 = state->src_x;
> +	intel_state->src.y1 = state->src_y;
> +	intel_state->src.x2 = state->src_x + state->src_w;
> +	intel_state->src.y2 = state->src_y + state->src_h;
> +	intel_state->dst.x1 = state->crtc_x;
> +	intel_state->dst.y1 = state->crtc_y;
> +	intel_state->dst.x2 = state->crtc_x + state->crtc_w;
> +	intel_state->dst.y2 = state->crtc_y + state->crtc_h;
> +
> +	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> +	if (intel_crtc) {
> +		intel_state->clip.x1 = 0;
> +		intel_state->clip.y1 = 0;
> +		intel_state->clip.x2 =
> +			intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> +		intel_state->clip.y2 =
> +			intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> +	}
> +
> +	return intel_plane->check_plane(plane, intel_state);
> +}
> +
> +static void intel_plane_atomic_update(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *intel_state =
> +		to_intel_plane_state(plane->state);
> +
> +	if (!plane->state->fb)
> +		intel_plane_disable(plane);
> +	else
> +		intel_plane->commit_plane(plane, intel_state);
> +}
> +
> +const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
> +	.prepare_fb = intel_prepare_fb,
> +	.cleanup_fb = intel_cleanup_fb,
> +	.atomic_check = intel_plane_atomic_check,
> +	.atomic_update = intel_plane_atomic_update,
> +};
> +
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2d4ef04..32a2b12 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9705,6 +9705,8 @@ out_hang:
>  static struct drm_crtc_helper_funcs intel_helper_funcs = {
>  	.mode_set_base_atomic = intel_pipe_set_base_atomic,
>  	.load_lut = intel_crtc_load_lut,
> +	.atomic_begin = intel_crtc_atomic_begin,
> +	.atomic_flush = intel_crtc_atomic_flush,
>  };
>  
>  /**
> @@ -11747,6 +11749,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  void intel_plane_destroy(struct drm_plane *plane)
>  {
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	intel_plane_destroy_state(plane, plane->state);
>  	drm_plane_cleanup(plane);
>  	kfree(intel_plane);
>  }
> @@ -11755,7 +11758,10 @@ static const struct drm_plane_funcs intel_primary_plane_funcs = {
>  	.update_plane = intel_primary_plane_setplane,
>  	.disable_plane = intel_primary_plane_disable,
>  	.destroy = intel_plane_destroy,
> -	.set_property = intel_plane_set_property
> +	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
> +
>  };
>  
>  static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> @@ -11769,11 +11775,14 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	if (primary == NULL)
>  		return NULL;
>  
> +	primary->base.state = intel_plane_duplicate_state(&primary->base);
>  	primary->can_scale = false;
>  	primary->max_downscale = 1;
>  	primary->pipe = pipe;
>  	primary->plane = pipe;
>  	primary->rotation = BIT(DRM_ROTATE_0);
> +	primary->check_plane = intel_check_primary_plane;
> +	primary->commit_plane = intel_commit_primary_plane;
>  	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
>  		primary->plane = !pipe;
>  
> @@ -11802,6 +11811,8 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  				primary->rotation);
>  	}
>  
> +	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> +
>  	return &primary->base;
>  }
>  
> @@ -11957,6 +11968,8 @@ static const struct drm_plane_funcs intel_cursor_plane_funcs = {
>  	.disable_plane = intel_cursor_plane_disable,
>  	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
>  };
>  
>  static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> @@ -11968,11 +11981,14 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  	if (cursor == NULL)
>  		return NULL;
>  
> +	cursor->base.state = intel_plane_duplicate_state(&cursor->base);
>  	cursor->can_scale = false;
>  	cursor->max_downscale = 1;
>  	cursor->pipe = pipe;
>  	cursor->plane = pipe;
>  	cursor->rotation = BIT(DRM_ROTATE_0);
> +	cursor->check_plane = intel_check_cursor_plane;
> +	cursor->commit_plane = intel_commit_cursor_plane;
>  
>  	drm_universal_plane_init(dev, &cursor->base, 0,
>  				 &intel_cursor_plane_funcs,
> @@ -11992,6 +12008,8 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  				cursor->rotation);
>  	}
>  
> +	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
> +
>  	return &cursor->base;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 49358c8..1882de1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -466,6 +466,8 @@ struct intel_crtc {
>  
>  	int scanline_offset;
>  	struct intel_mmio_flip mmio_flip;
> +
> +	uint32_t atomic_vbl_count;
>  };
>  
>  struct intel_plane_wm_parameters {
> @@ -506,6 +508,10 @@ struct intel_plane {
>  			     uint32_t src_w, uint32_t src_h);
>  	void (*disable_plane)(struct drm_plane *plane,
>  			      struct drm_crtc *crtc);
> +	int (*check_plane)(struct drm_plane *plane,
> +			   struct intel_plane_state *state);
> +	void (*commit_plane)(struct drm_plane *plane,
> +			     struct intel_plane_state *state);
>  	int (*update_colorkey)(struct drm_plane *plane,
>  			       struct drm_intel_sprite_colorkey *key);
>  	void (*get_colorkey)(struct drm_plane *plane,
> @@ -536,6 +542,7 @@ struct cxsr_latency {
>  #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
>  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
>  #define to_intel_plane(x) container_of(x, struct intel_plane, base)
> +#define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
>  #define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
>  
>  struct intel_hdmi {
> @@ -1195,4 +1202,12 @@ int intel_sprite_plane_disable(struct drm_plane *plane);
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_device *dev);
>  
> +/* intel_atomic.c */
> +struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
> +void intel_plane_destroy_state(struct drm_plane *plane,
> +			       struct drm_plane_state *state);
> +void intel_crtc_atomic_begin(struct drm_crtc *crtc);
> +void intel_crtc_atomic_flush(struct drm_crtc *crtc);
> +extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> +
>  #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 9e6a72a..d3677c3 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -33,6 +33,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_rect.h>
> +#include <drm/drm_plane_helper.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
> @@ -1573,6 +1574,8 @@ static const struct drm_plane_funcs intel_plane_funcs = {
>  	.disable_plane = intel_sprite_plane_disable,
>  	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
>  };
>  
>  static uint32_t ilk_plane_formats[] = {
> @@ -1634,6 +1637,9 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  	if (!intel_plane)
>  		return -ENOMEM;
>  
> +	intel_plane->base.state =
> +		intel_plane_duplicate_state(&intel_plane->base);
> +
>  	switch (INTEL_INFO(dev)->gen) {
>  	case 5:
>  	case 6:
> @@ -1704,6 +1710,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  	intel_plane->pipe = pipe;
>  	intel_plane->plane = plane;
>  	intel_plane->rotation = BIT(DRM_ROTATE_0);
> +	intel_plane->check_plane = intel_check_sprite_plane;
> +	intel_plane->commit_plane = intel_commit_sprite_plane;
>  	possible_crtcs = (1 << pipe);
>  	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
>  				       &intel_plane_funcs,
> @@ -1725,6 +1733,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  					   dev->mode_config.rotation_property,
>  					   intel_plane->rotation);
>  
> +	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> +
>   out:
>  	return ret;
>  }
> -- 
> 1.8.5.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/8] drm/i915: Prepare for atomic plane helpers (v2)

[Daniel Vetter]

On Thu, Nov 13, 2014 at 05:23:05PM -0800, Matt Roper wrote:
> On Thu, Nov 13, 2014 at 02:51:32PM -0800, Matt Roper wrote:
> > Add the new driver entrypoints that will be called by the atomic plane
> > helpers.
> > 
> > This patch does not actually switch over to the new plane helpers yet,
> > so there should be no functional change here.  Also note that although
> > plane programming was already split into check/prepare/commit steps,
> > some of the semantics of those individual functions will need to change
> > slightly when we do make the jump so that they match the behavior the
> > plane helpers expect.
> > 
> > v2:
> >  - Renamed file from intel_atomic.c to intel_atomic_plane.c (Daniel)
> >  - Fix a copy/paste comment mistake (Bob)
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Actually, I don't think this is quite ready to go yet.  I must have had
> too much debug spam turned on before and missed the important messages
> in the kernel log, but it looks like we're still calling something that
> can sleep (in the frontbuffer tracking code?) while irqs are disabled
> for vblank evasion.

Yeah the frontbuffer tracking code will sleep since it can grab mutexes.

We also want to eventually grow async support out of this, so I think the
best approach might be to simply use frontbuffer_flip_prepare/complete
around the entire sequence. Well the important part is that we call
prepare before we start waiting for outstanding rendering and complete
after pipe_update_end.

Since this loops over abstract plane objects I think we need to store the
correct frontbuffer tracking bit somewhere in intel_plane so that we can
get at them.
-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

[PATCH 6/7] drm/i915: Switch plane handling to atomic helpers

[Matt Roper]

Now that we have hooks to enable the atomic plane helpers, we can use
the plane helpers for our .update_plane() and .disable_plane()
entrypoints.

Even though we'd already refactored our plane handling code into
check/commit, we still have to rework some of that logic here as we make
the transition.  The atomic plane helpers now call prepare_fb/cleanup_fb
for us, so note the following:
 - 'commit' should not longer unpin the old object; the cleanup_fb will
   take care of this for us
 - prepare_fb only gets called when we actually have an fb, so the
   internal 'disable' code needs to update frontbuffer tracking
 - the cursor plane code never had 'prepare' code separated from its
   'commit' function, so a lot of the cursor's 'commit' needs to be
   dropped since prepare_fb already does it for us
 - with all the pinning work removed from cursor commit, it can no
   longer fail and should return void rather than int

Testcase: igt/kms_plane
Testcase: igt/kms_universal_plane
Testcase: igt/kms_cursor_crc
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 255 +++++------------------------------
 drivers/gpu/drm/i915/intel_sprite.c  | 122 ++---------------
 2 files changed, 41 insertions(+), 336 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 32a2b12..8d5a3b4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8285,17 +8285,15 @@ static bool cursor_size_ok(struct drm_device *dev,
 	return true;
 }
 
-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
-				     struct drm_i915_gem_object *obj,
-				     uint32_t width, uint32_t height)
+static void intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
+				      struct drm_i915_gem_object *obj,
+				      uint32_t width, uint32_t height)
 {
 	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;
 	unsigned old_width;
 	uint32_t addr;
-	int ret;
 
 	/* if we want to turn off the cursor ignore width and height */
 	if (!obj) {
@@ -8305,64 +8303,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		goto finish;
 	}
 
-	/* we only need to pin inside GTT if cursor is non-phy */
+	/* plane helper's prepare_fb() already handled pinning */
 	mutex_lock(&dev->struct_mutex);
-	if (!INTEL_INFO(dev)->cursor_needs_physical) {
-		unsigned alignment;
-
-		/*
-		 * Global gtt pte registers are special registers which actually
-		 * forward writes to a chunk of system memory. Which means that
-		 * there is no risk that the register values disappear as soon
-		 * as we call intel_runtime_pm_put(), so it is correct to wrap
-		 * only the pin/unpin/fence and not more.
-		 */
-		intel_runtime_pm_get(dev_priv);
-
-		/* Note that the w/a also requires 2 PTE of padding following
-		 * the bo. We currently fill all unused PTE with the shadow
-		 * page and so we should always have valid PTE following the
-		 * cursor preventing the VT-d warning.
-		 */
-		alignment = 0;
-		if (need_vtd_wa(dev))
-			alignment = 64*1024;
-
-		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_put_fence(obj);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to release fence for cursor");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_unpin;
-		}
-
+	if (!INTEL_INFO(dev)->cursor_needs_physical)
 		addr = i915_gem_obj_ggtt_offset(obj);
-
-		intel_runtime_pm_put(dev_priv);
-	} else {
-		int align = IS_I830(dev) ? 16 * 1024 : 256;
-		ret = i915_gem_object_attach_phys(obj, align);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to attach phys object\n");
-			goto fail_locked;
-		}
+	else
 		addr = obj->phys_handle->busaddr;
-	}
 
  finish:
-	if (intel_crtc->cursor_bo) {
-		if (!INTEL_INFO(dev)->cursor_needs_physical)
-			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
-	}
-
-	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
-			  INTEL_FRONTBUFFER_CURSOR(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;
@@ -8379,13 +8327,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
 	}
-
-	return 0;
-fail_unpin:
-	i915_gem_object_unpin_from_display_plane(obj);
-fail_locked:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
 }
 
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
@@ -11544,7 +11485,6 @@ disable_unpin:
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
 			  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
-	intel_unpin_fb_obj(intel_fb_obj(plane->fb));
 	mutex_unlock(&dev->struct_mutex);
 	plane->fb = NULL;
 
@@ -11568,42 +11508,6 @@ intel_check_primary_plane(struct drm_plane *plane,
 					     false, true, &state->visible);
 }
 
-static int
-intel_prepare_primary_plane(struct drm_plane *plane,
-			    struct intel_plane_state *state)
-{
-	struct drm_crtc *crtc = state->base.crtc;
-	struct drm_framebuffer *fb = state->base.fb;
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
-	int ret;
-
-	intel_crtc_wait_for_pending_flips(crtc);
-
-	if (intel_crtc_has_pending_flip(crtc)) {
-		DRM_ERROR("pipe is still busy with an old pageflip\n");
-		return -EBUSY;
-	}
-
-	if (old_obj != obj) {
-		mutex_lock(&dev->struct_mutex);
-		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
-		if (ret == 0)
-			i915_gem_track_fb(old_obj, obj,
-					  INTEL_FRONTBUFFER_PRIMARY(pipe));
-		mutex_unlock(&dev->struct_mutex);
-		if (ret != 0) {
-			DRM_DEBUG_KMS("pin & fence failed\n");
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 static void
 intel_commit_primary_plane(struct drm_plane *plane,
 			   struct intel_plane_state *state)
@@ -11614,9 +11518,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	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 drm_framebuffer *old_fb = plane->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_rect *src = &state->src;
 
@@ -11687,62 +11589,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 		intel_update_fbc(dev);
 		mutex_unlock(&dev->struct_mutex);
 	}
-
-	if (old_fb && old_fb != fb) {
-		if (intel_crtc->active)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(old_obj);
-		mutex_unlock(&dev->struct_mutex);
-	}
-}
-
-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 intel_plane_state state;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int ret;
-
-	state.base.crtc = crtc;
-	state.base.fb = fb;
-
-	/* sample coordinates in 16.16 fixed point */
-	state.src.x1 = src_x;
-	state.src.x2 = src_x + src_w;
-	state.src.y1 = src_y;
-	state.src.y2 = src_y + src_h;
-
-	/* integer pixels */
-	state.dst.x1 = crtc_x;
-	state.dst.x2 = crtc_x + crtc_w;
-	state.dst.y1 = crtc_y;
-	state.dst.y2 = crtc_y + crtc_h;
-
-	state.clip.x1 = 0;
-	state.clip.y1 = 0;
-	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
-	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
-
-	state.orig_src = state.src;
-	state.orig_dst = state.dst;
-
-	ret = intel_check_primary_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	ret = intel_prepare_primary_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	intel_commit_primary_plane(plane, &state);
-
-	return 0;
 }
 
 /* Common destruction function for both primary and cursor planes */
@@ -11755,8 +11601,8 @@ void intel_plane_destroy(struct drm_plane *plane)
 }
 
 static const struct drm_plane_funcs intel_primary_plane_funcs = {
-	.update_plane = intel_primary_plane_setplane,
-	.disable_plane = intel_primary_plane_disable,
+	.update_plane = drm_plane_helper_update,
+	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
 	.atomic_duplicate_state = intel_plane_duplicate_state,
@@ -11816,15 +11662,22 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	return &primary->base;
 }
 
-static int
+static void
 intel_cursor_plane_disable(struct drm_plane *plane)
 {
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+
 	if (!plane->fb)
-		return 0;
+		return;
 
 	BUG_ON(!plane->crtc);
 
-	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+	intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+
+	mutex_lock(&plane->dev->struct_mutex);
+	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
+			  INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe));
+	mutex_unlock(&plane->dev->struct_mutex);
 }
 
 static int
@@ -11838,7 +11691,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	int crtc_w, crtc_h;
 	unsigned stride;
 	int ret;
 
@@ -11856,15 +11708,14 @@ intel_check_cursor_plane(struct drm_plane *plane,
 		return 0;
 
 	/* Check for which cursor types we support */
-	crtc_w = drm_rect_width(&state->orig_dst);
-	crtc_h = drm_rect_height(&state->orig_dst);
-	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
-		DRM_DEBUG("Cursor dimension not supported\n");
+	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
+		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
+			  state->base.crtc_w, state->base.crtc_h);
 		return -EINVAL;
 	}
 
-	stride = roundup_pow_of_two(crtc_w) * 4;
-	if (obj->base.size < stride * crtc_h) {
+	stride = roundup_pow_of_two(state->base.crtc_w) * 4;
+	if (obj->base.size < stride * state->base.crtc_h) {
 		DRM_DEBUG_KMS("buffer is too small\n");
 		return -ENOMEM;
 	}
@@ -11883,7 +11734,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	return ret;
 }
 
-static int
+static void
 intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
@@ -11893,10 +11744,9 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
-	int crtc_w, crtc_h;
 
-	crtc->cursor_x = state->orig_dst.x1;
-	crtc->cursor_y = state->orig_dst.y1;
+	crtc->cursor_x = state->base.crtc_x;
+	crtc->cursor_y = state->base.crtc_y;
 
 	intel_plane->crtc_x = state->orig_dst.x1;
 	intel_plane->crtc_y = state->orig_dst.y1;
@@ -11909,63 +11759,20 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	intel_plane->obj = obj;
 
 	if (fb != crtc->cursor->fb) {
-		crtc_w = drm_rect_width(&state->orig_dst);
-		crtc_h = drm_rect_height(&state->orig_dst);
-		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+		intel_crtc_cursor_set_obj(crtc, obj,
+					  state->base.crtc_w,
+					  state->base.crtc_h);
 	} else {
 		intel_crtc_update_cursor(crtc, state->visible);
 
 		intel_frontbuffer_flip(crtc->dev,
 				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
-
-		return 0;
 	}
 }
 
-static int
-intel_cursor_plane_update(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 intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane_state state;
-	int ret;
-
-	state.base.crtc = crtc;
-	state.base.fb = fb;
-
-	/* sample coordinates in 16.16 fixed point */
-	state.src.x1 = src_x;
-	state.src.x2 = src_x + src_w;
-	state.src.y1 = src_y;
-	state.src.y2 = src_y + src_h;
-
-	/* integer pixels */
-	state.dst.x1 = crtc_x;
-	state.dst.x2 = crtc_x + crtc_w;
-	state.dst.y1 = crtc_y;
-	state.dst.y2 = crtc_y + crtc_h;
-
-	state.clip.x1 = 0;
-	state.clip.y1 = 0;
-	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
-	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
-
-	state.orig_src = state.src;
-	state.orig_dst = state.dst;
-
-	ret = intel_check_cursor_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	return intel_commit_cursor_plane(plane, &state);
-}
-
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {
-	.update_plane = intel_cursor_plane_update,
-	.disable_plane = intel_cursor_plane_disable,
+	.update_plane = drm_plane_helper_update,
+	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
 	.atomic_duplicate_state = intel_plane_duplicate_state,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d3677c3..28ccaf7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1106,7 +1106,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	uint32_t src_x, src_y, src_w, src_h;
 	struct drm_rect *src = &state->src;
 	struct drm_rect *dst = &state->dst;
-	struct drm_rect *orig_src = &state->orig_src;
 	const struct drm_rect *clip = &state->clip;
 	int hscale, vscale;
 	int max_scale, min_scale;
@@ -1187,10 +1186,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
 				    intel_plane->rotation);
 
 		/* sanity check to make sure the src viewport wasn't enlarged */
-		WARN_ON(src->x1 < (int) orig_src->x1 ||
-			src->y1 < (int) orig_src->y1 ||
-			src->x2 > (int) orig_src->x2 ||
-			src->y2 > (int) orig_src->y2);
+		WARN_ON(src->x1 < (int) state->base.src_x ||
+			src->y1 < (int) state->base.src_y ||
+			src->x2 > (int) state->base.src_x + state->base.src_w ||
+			src->y2 > (int) state->base.src_y + state->base.src_h);
 
 		/*
 		 * Hardware doesn't handle subpixel coordinates.
@@ -1258,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	return 0;
 }
 
-static int
-intel_prepare_sprite_plane(struct drm_plane *plane,
-			   struct intel_plane_state *state)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_crtc *crtc = state->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *fb = state->base.fb;
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
-	int ret;
-
-	if (old_obj != obj) {
-		mutex_lock(&dev->struct_mutex);
-
-		/* Note that this will apply the VT-d workaround for scanouts,
-		 * which is more restrictive than required for sprites. (The
-		 * primary plane requires 256KiB alignment with 64 PTE padding,
-		 * the sprite planes only require 128KiB alignment and 32 PTE
-		 * padding.
-		 */
-		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
-		if (ret == 0)
-			i915_gem_track_fb(old_obj, obj,
-					  INTEL_FRONTBUFFER_SPRITE(pipe));
-		mutex_unlock(&dev->struct_mutex);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
 static void
 intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
@@ -1304,7 +1268,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
@@ -1362,68 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 		if (!primary_was_enabled && primary_enabled)
 			intel_post_enable_primary(crtc);
 	}
-
-	/* Unpin old obj after new one is active to avoid ugliness */
-	if (old_obj && old_obj != obj) {
-
-		/*
-		 * It's fairly common to simply update the position of
-		 * an existing object.  In that case, we don't need to
-		 * wait for vblank to avoid ugliness, we only need to
-		 * do the pin & ref bookkeeping.
-		 */
-		if (intel_crtc->active)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(old_obj);
-		mutex_unlock(&dev->struct_mutex);
-	}
-}
-
-static int
-intel_update_plane(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 intel_plane_state state;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int ret;
-
-	state.base.crtc = crtc;
-	state.base.fb = fb;
-
-	/* sample coordinates in 16.16 fixed point */
-	state.src.x1 = src_x;
-	state.src.x2 = src_x + src_w;
-	state.src.y1 = src_y;
-	state.src.y2 = src_y + src_h;
-
-	/* integer pixels */
-	state.dst.x1 = crtc_x;
-	state.dst.x2 = crtc_x + crtc_w;
-	state.dst.y1 = crtc_y;
-	state.dst.y2 = crtc_y + crtc_h;
-
-	state.clip.x1 = 0;
-	state.clip.y1 = 0;
-	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
-	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
-	state.orig_src = state.src;
-	state.orig_dst = state.dst;
-
-	ret = intel_check_sprite_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	ret = intel_prepare_sprite_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	intel_commit_sprite_plane(plane, &state);
-	return 0;
 }
 
 int
@@ -1459,7 +1360,6 @@ intel_sprite_plane_disable(struct drm_plane *plane)
 			intel_wait_for_vblank(dev, intel_plane->pipe);
 
 		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(intel_plane->obj);
 		i915_gem_track_fb(intel_plane->obj, NULL,
 				  INTEL_FRONTBUFFER_SPRITE(pipe));
 		mutex_unlock(&dev->struct_mutex);
@@ -1557,21 +1457,19 @@ int intel_plane_set_property(struct drm_plane *plane,
 
 int intel_plane_restore(struct drm_plane *plane)
 {
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-
 	if (!plane->crtc || !plane->fb)
 		return 0;
 
 	return plane->funcs->update_plane(plane, plane->crtc, plane->fb,
-				  intel_plane->crtc_x, intel_plane->crtc_y,
-				  intel_plane->crtc_w, intel_plane->crtc_h,
-				  intel_plane->src_x, intel_plane->src_y,
-				  intel_plane->src_w, intel_plane->src_h);
+				  plane->state->crtc_x, plane->state->crtc_y,
+				  plane->state->crtc_w, plane->state->crtc_h,
+				  plane->state->src_x, plane->state->src_y,
+				  plane->state->src_w, plane->state->src_h);
 }
 
 static const struct drm_plane_funcs intel_plane_funcs = {
-	.update_plane = intel_update_plane,
-	.disable_plane = intel_sprite_plane_disable,
+	.update_plane = drm_plane_helper_update,
+	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
 	.atomic_duplicate_state = intel_plane_duplicate_state,
-- 
1.8.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] drm/i915: Switch plane handling to atomic helpers

[Bob Paauwe]

On Thu, 13 Nov 2014 10:43:25 -0800
Matt Roper <matthew.d.roper@intel.com> wrote:

> Now that we have hooks to enable the atomic plane helpers, we can use
> the plane helpers for our .update_plane() and .disable_plane()
> entrypoints.
> 
> Even though we'd already refactored our plane handling code into
> check/commit, we still have to rework some of that logic here as we make
> the transition.  The atomic plane helpers now call prepare_fb/cleanup_fb
> for us, so note the following:
>  - 'commit' should not longer unpin the old object; the cleanup_fb will

s/not/no/

>    take care of this for us
>  - prepare_fb only gets called when we actually have an fb, so the
>    internal 'disable' code needs to update frontbuffer tracking
>  - the cursor plane code never had 'prepare' code separated from its
>    'commit' function, so a lot of the cursor's 'commit' needs to be
>    dropped since prepare_fb already does it for us
>  - with all the pinning work removed from cursor commit, it can no
>    longer fail and should return void rather than int
> 
> Testcase: igt/kms_plane
> Testcase: igt/kms_universal_plane
> Testcase: igt/kms_cursor_crc
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

With the above typo fixed this is
Acknowledged-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 255 +++++------------------------------
>  drivers/gpu/drm/i915/intel_sprite.c  | 122 ++---------------
>  2 files changed, 41 insertions(+), 336 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 32a2b12..8d5a3b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8285,17 +8285,15 @@ static bool cursor_size_ok(struct drm_device *dev,
>  	return true;
>  }
>  
> -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> -				     struct drm_i915_gem_object *obj,
> -				     uint32_t width, uint32_t height)
> +static void intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> +				      struct drm_i915_gem_object *obj,
> +				      uint32_t width, uint32_t height)
>  {
>  	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;
>  	unsigned old_width;
>  	uint32_t addr;
> -	int ret;
>  
>  	/* if we want to turn off the cursor ignore width and height */
>  	if (!obj) {
> @@ -8305,64 +8303,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  		goto finish;
>  	}
>  
> -	/* we only need to pin inside GTT if cursor is non-phy */
> +	/* plane helper's prepare_fb() already handled pinning */
>  	mutex_lock(&dev->struct_mutex);
> -	if (!INTEL_INFO(dev)->cursor_needs_physical) {
> -		unsigned alignment;
> -
> -		/*
> -		 * Global gtt pte registers are special registers which actually
> -		 * forward writes to a chunk of system memory. Which means that
> -		 * there is no risk that the register values disappear as soon
> -		 * as we call intel_runtime_pm_put(), so it is correct to wrap
> -		 * only the pin/unpin/fence and not more.
> -		 */
> -		intel_runtime_pm_get(dev_priv);
> -
> -		/* Note that the w/a also requires 2 PTE of padding following
> -		 * the bo. We currently fill all unused PTE with the shadow
> -		 * page and so we should always have valid PTE following the
> -		 * cursor preventing the VT-d warning.
> -		 */
> -		alignment = 0;
> -		if (need_vtd_wa(dev))
> -			alignment = 64*1024;
> -
> -		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> -		if (ret) {
> -			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> -			intel_runtime_pm_put(dev_priv);
> -			goto fail_locked;
> -		}
> -
> -		ret = i915_gem_object_put_fence(obj);
> -		if (ret) {
> -			DRM_DEBUG_KMS("failed to release fence for cursor");
> -			intel_runtime_pm_put(dev_priv);
> -			goto fail_unpin;
> -		}
> -
> +	if (!INTEL_INFO(dev)->cursor_needs_physical)
>  		addr = i915_gem_obj_ggtt_offset(obj);
> -
> -		intel_runtime_pm_put(dev_priv);
> -	} else {
> -		int align = IS_I830(dev) ? 16 * 1024 : 256;
> -		ret = i915_gem_object_attach_phys(obj, align);
> -		if (ret) {
> -			DRM_DEBUG_KMS("failed to attach phys object\n");
> -			goto fail_locked;
> -		}
> +	else
>  		addr = obj->phys_handle->busaddr;
> -	}
>  
>   finish:
> -	if (intel_crtc->cursor_bo) {
> -		if (!INTEL_INFO(dev)->cursor_needs_physical)
> -			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> -	}
> -
> -	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> -			  INTEL_FRONTBUFFER_CURSOR(pipe));
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	old_width = intel_crtc->cursor_width;
> @@ -8379,13 +8327,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  
>  		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
>  	}
> -
> -	return 0;
> -fail_unpin:
> -	i915_gem_object_unpin_from_display_plane(obj);
> -fail_locked:
> -	mutex_unlock(&dev->struct_mutex);
> -	return ret;
>  }
>  
>  static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
> @@ -11544,7 +11485,6 @@ disable_unpin:
>  	mutex_lock(&dev->struct_mutex);
>  	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
>  			  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
> -	intel_unpin_fb_obj(intel_fb_obj(plane->fb));
>  	mutex_unlock(&dev->struct_mutex);
>  	plane->fb = NULL;
>  
> @@ -11568,42 +11508,6 @@ intel_check_primary_plane(struct drm_plane *plane,
>  					     false, true, &state->visible);
>  }
>  
> -static int
> -intel_prepare_primary_plane(struct drm_plane *plane,
> -			    struct intel_plane_state *state)
> -{
> -	struct drm_crtc *crtc = state->base.crtc;
> -	struct drm_framebuffer *fb = state->base.fb;
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> -	int ret;
> -
> -	intel_crtc_wait_for_pending_flips(crtc);
> -
> -	if (intel_crtc_has_pending_flip(crtc)) {
> -		DRM_ERROR("pipe is still busy with an old pageflip\n");
> -		return -EBUSY;
> -	}
> -
> -	if (old_obj != obj) {
> -		mutex_lock(&dev->struct_mutex);
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> -		if (ret == 0)
> -			i915_gem_track_fb(old_obj, obj,
> -					  INTEL_FRONTBUFFER_PRIMARY(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -		if (ret != 0) {
> -			DRM_DEBUG_KMS("pin & fence failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  static void
>  intel_commit_primary_plane(struct drm_plane *plane,
>  			   struct intel_plane_state *state)
> @@ -11614,9 +11518,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	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 drm_framebuffer *old_fb = plane->fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_rect *src = &state->src;
>  
> @@ -11687,62 +11589,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  		intel_update_fbc(dev);
>  		mutex_unlock(&dev->struct_mutex);
>  	}
> -
> -	if (old_fb && old_fb != fb) {
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_obj);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -}
> -
> -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 intel_plane_state state;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int ret;
> -
> -	state.base.crtc = crtc;
> -	state.base.fb = fb;
> -
> -	/* sample coordinates in 16.16 fixed point */
> -	state.src.x1 = src_x;
> -	state.src.x2 = src_x + src_w;
> -	state.src.y1 = src_y;
> -	state.src.y2 = src_y + src_h;
> -
> -	/* integer pixels */
> -	state.dst.x1 = crtc_x;
> -	state.dst.x2 = crtc_x + crtc_w;
> -	state.dst.y1 = crtc_y;
> -	state.dst.y2 = crtc_y + crtc_h;
> -
> -	state.clip.x1 = 0;
> -	state.clip.y1 = 0;
> -	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> -	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> -
> -	state.orig_src = state.src;
> -	state.orig_dst = state.dst;
> -
> -	ret = intel_check_primary_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	ret = intel_prepare_primary_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	intel_commit_primary_plane(plane, &state);
> -
> -	return 0;
>  }
>  
>  /* Common destruction function for both primary and cursor planes */
> @@ -11755,8 +11601,8 @@ void intel_plane_destroy(struct drm_plane *plane)
>  }
>  
>  static const struct drm_plane_funcs intel_primary_plane_funcs = {
> -	.update_plane = intel_primary_plane_setplane,
> -	.disable_plane = intel_primary_plane_disable,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>  	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
>  	.atomic_duplicate_state = intel_plane_duplicate_state,
> @@ -11816,15 +11662,22 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	return &primary->base;
>  }
>  
> -static int
> +static void
>  intel_cursor_plane_disable(struct drm_plane *plane)
>  {
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +
>  	if (!plane->fb)
> -		return 0;
> +		return;
>  
>  	BUG_ON(!plane->crtc);
>  
> -	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> +	intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> +
> +	mutex_lock(&plane->dev->struct_mutex);
> +	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> +			  INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe));
> +	mutex_unlock(&plane->dev->struct_mutex);
>  }
>  
>  static int
> @@ -11838,7 +11691,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	int crtc_w, crtc_h;
>  	unsigned stride;
>  	int ret;
>  
> @@ -11856,15 +11708,14 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  		return 0;
>  
>  	/* Check for which cursor types we support */
> -	crtc_w = drm_rect_width(&state->orig_dst);
> -	crtc_h = drm_rect_height(&state->orig_dst);
> -	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
> -		DRM_DEBUG("Cursor dimension not supported\n");
> +	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
> +		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
> +			  state->base.crtc_w, state->base.crtc_h);
>  		return -EINVAL;
>  	}
>  
> -	stride = roundup_pow_of_two(crtc_w) * 4;
> -	if (obj->base.size < stride * crtc_h) {
> +	stride = roundup_pow_of_two(state->base.crtc_w) * 4;
> +	if (obj->base.size < stride * state->base.crtc_h) {
>  		DRM_DEBUG_KMS("buffer is too small\n");
>  		return -ENOMEM;
>  	}
> @@ -11883,7 +11734,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	return ret;
>  }
>  
> -static int
> +static void
>  intel_commit_cursor_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
>  {
> @@ -11893,10 +11744,9 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
>  	struct drm_i915_gem_object *obj = intel_fb->obj;
> -	int crtc_w, crtc_h;
>  
> -	crtc->cursor_x = state->orig_dst.x1;
> -	crtc->cursor_y = state->orig_dst.y1;
> +	crtc->cursor_x = state->base.crtc_x;
> +	crtc->cursor_y = state->base.crtc_y;
>  
>  	intel_plane->crtc_x = state->orig_dst.x1;
>  	intel_plane->crtc_y = state->orig_dst.y1;
> @@ -11909,63 +11759,20 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	intel_plane->obj = obj;
>  
>  	if (fb != crtc->cursor->fb) {
> -		crtc_w = drm_rect_width(&state->orig_dst);
> -		crtc_h = drm_rect_height(&state->orig_dst);
> -		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> +		intel_crtc_cursor_set_obj(crtc, obj,
> +					  state->base.crtc_w,
> +					  state->base.crtc_h);
>  	} else {
>  		intel_crtc_update_cursor(crtc, state->visible);
>  
>  		intel_frontbuffer_flip(crtc->dev,
>  				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> -
> -		return 0;
>  	}
>  }
>  
> -static int
> -intel_cursor_plane_update(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 intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane_state state;
> -	int ret;
> -
> -	state.base.crtc = crtc;
> -	state.base.fb = fb;
> -
> -	/* sample coordinates in 16.16 fixed point */
> -	state.src.x1 = src_x;
> -	state.src.x2 = src_x + src_w;
> -	state.src.y1 = src_y;
> -	state.src.y2 = src_y + src_h;
> -
> -	/* integer pixels */
> -	state.dst.x1 = crtc_x;
> -	state.dst.x2 = crtc_x + crtc_w;
> -	state.dst.y1 = crtc_y;
> -	state.dst.y2 = crtc_y + crtc_h;
> -
> -	state.clip.x1 = 0;
> -	state.clip.y1 = 0;
> -	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> -	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> -
> -	state.orig_src = state.src;
> -	state.orig_dst = state.dst;
> -
> -	ret = intel_check_cursor_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	return intel_commit_cursor_plane(plane, &state);
> -}
> -
>  static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> -	.update_plane = intel_cursor_plane_update,
> -	.disable_plane = intel_cursor_plane_disable,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>  	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
>  	.atomic_duplicate_state = intel_plane_duplicate_state,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index d3677c3..28ccaf7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1106,7 +1106,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	uint32_t src_x, src_y, src_w, src_h;
>  	struct drm_rect *src = &state->src;
>  	struct drm_rect *dst = &state->dst;
> -	struct drm_rect *orig_src = &state->orig_src;
>  	const struct drm_rect *clip = &state->clip;
>  	int hscale, vscale;
>  	int max_scale, min_scale;
> @@ -1187,10 +1186,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  				    intel_plane->rotation);
>  
>  		/* sanity check to make sure the src viewport wasn't enlarged */
> -		WARN_ON(src->x1 < (int) orig_src->x1 ||
> -			src->y1 < (int) orig_src->y1 ||
> -			src->x2 > (int) orig_src->x2 ||
> -			src->y2 > (int) orig_src->y2);
> +		WARN_ON(src->x1 < (int) state->base.src_x ||
> +			src->y1 < (int) state->base.src_y ||
> +			src->x2 > (int) state->base.src_x + state->base.src_w ||
> +			src->y2 > (int) state->base.src_y + state->base.src_h);
>  
>  		/*
>  		 * Hardware doesn't handle subpixel coordinates.
> @@ -1258,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	return 0;
>  }
>  
> -static int
> -intel_prepare_sprite_plane(struct drm_plane *plane,
> -			   struct intel_plane_state *state)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_framebuffer *fb = state->base.fb;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> -	int ret;
> -
> -	if (old_obj != obj) {
> -		mutex_lock(&dev->struct_mutex);
> -
> -		/* Note that this will apply the VT-d workaround for scanouts,
> -		 * which is more restrictive than required for sprites. (The
> -		 * primary plane requires 256KiB alignment with 64 PTE padding,
> -		 * the sprite planes only require 128KiB alignment and 32 PTE
> -		 * padding.
> -		 */
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> -		if (ret == 0)
> -			i915_gem_track_fb(old_obj, obj,
> -					  INTEL_FRONTBUFFER_SPRITE(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static void
>  intel_commit_sprite_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
> @@ -1304,7 +1268,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_plane->obj;
>  	int crtc_x, crtc_y;
>  	unsigned int crtc_w, crtc_h;
>  	uint32_t src_x, src_y, src_w, src_h;
> @@ -1362,68 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  		if (!primary_was_enabled && primary_enabled)
>  			intel_post_enable_primary(crtc);
>  	}
> -
> -	/* Unpin old obj after new one is active to avoid ugliness */
> -	if (old_obj && old_obj != obj) {
> -
> -		/*
> -		 * It's fairly common to simply update the position of
> -		 * an existing object.  In that case, we don't need to
> -		 * wait for vblank to avoid ugliness, we only need to
> -		 * do the pin & ref bookkeeping.
> -		 */
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_obj);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -}
> -
> -static int
> -intel_update_plane(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 intel_plane_state state;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int ret;
> -
> -	state.base.crtc = crtc;
> -	state.base.fb = fb;
> -
> -	/* sample coordinates in 16.16 fixed point */
> -	state.src.x1 = src_x;
> -	state.src.x2 = src_x + src_w;
> -	state.src.y1 = src_y;
> -	state.src.y2 = src_y + src_h;
> -
> -	/* integer pixels */
> -	state.dst.x1 = crtc_x;
> -	state.dst.x2 = crtc_x + crtc_w;
> -	state.dst.y1 = crtc_y;
> -	state.dst.y2 = crtc_y + crtc_h;
> -
> -	state.clip.x1 = 0;
> -	state.clip.y1 = 0;
> -	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> -	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> -	state.orig_src = state.src;
> -	state.orig_dst = state.dst;
> -
> -	ret = intel_check_sprite_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	ret = intel_prepare_sprite_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	intel_commit_sprite_plane(plane, &state);
> -	return 0;
>  }
>  
>  int
> @@ -1459,7 +1360,6 @@ intel_sprite_plane_disable(struct drm_plane *plane)
>  			intel_wait_for_vblank(dev, intel_plane->pipe);
>  
>  		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(intel_plane->obj);
>  		i915_gem_track_fb(intel_plane->obj, NULL,
>  				  INTEL_FRONTBUFFER_SPRITE(pipe));
>  		mutex_unlock(&dev->struct_mutex);
> @@ -1557,21 +1457,19 @@ int intel_plane_set_property(struct drm_plane *plane,
>  
>  int intel_plane_restore(struct drm_plane *plane)
>  {
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -
>  	if (!plane->crtc || !plane->fb)
>  		return 0;
>  
>  	return plane->funcs->update_plane(plane, plane->crtc, plane->fb,
> -				  intel_plane->crtc_x, intel_plane->crtc_y,
> -				  intel_plane->crtc_w, intel_plane->crtc_h,
> -				  intel_plane->src_x, intel_plane->src_y,
> -				  intel_plane->src_w, intel_plane->src_h);
> +				  plane->state->crtc_x, plane->state->crtc_y,
> +				  plane->state->crtc_w, plane->state->crtc_h,
> +				  plane->state->src_x, plane->state->src_y,
> +				  plane->state->src_w, plane->state->src_h);
>  }
>  
>  static const struct drm_plane_funcs intel_plane_funcs = {
> -	.update_plane = intel_update_plane,
> -	.disable_plane = intel_sprite_plane_disable,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>  	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
>  	.atomic_duplicate_state = intel_plane_duplicate_state,

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

[PATCH 6/8] drm/i915: Switch plane handling to atomic helpers (v2)

[Matt Roper]

Now that we have hooks to enable the atomic plane helpers, we can use
the plane helpers for our .update_plane() and .disable_plane()
entrypoints.

Even though we'd already refactored our plane handling code into
check/commit, we still have to rework some of that logic here as we make
the transition.  The atomic plane helpers now call prepare_fb/cleanup_fb
for us, so note the following:
 - 'commit' should no longer unpin the old object; the cleanup_fb will
   take care of this for us
 - prepare_fb only gets called when we actually have an fb, so the
   internal 'disable' code needs to update frontbuffer tracking
 - the cursor plane code never had 'prepare' code separated from its
   'commit' function, so a lot of the cursor's 'commit' needs to be
   dropped since prepare_fb already does it for us
 - with all the pinning work removed from cursor commit, it can no
   longer fail and should return void rather than int

v2: Fix commit message typo (Bob)

Testcase: igt/kms_plane
Testcase: igt/kms_universal_plane
Testcase: igt/kms_cursor_crc
Acknowledged-by: Bob Paauwe <bob.j.paauwe@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 255 +++++------------------------------
 drivers/gpu/drm/i915/intel_sprite.c  | 122 ++---------------
 2 files changed, 41 insertions(+), 336 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 32a2b12..8d5a3b4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8285,17 +8285,15 @@ static bool cursor_size_ok(struct drm_device *dev,
 	return true;
 }
 
-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
-				     struct drm_i915_gem_object *obj,
-				     uint32_t width, uint32_t height)
+static void intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
+				      struct drm_i915_gem_object *obj,
+				      uint32_t width, uint32_t height)
 {
 	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;
 	unsigned old_width;
 	uint32_t addr;
-	int ret;
 
 	/* if we want to turn off the cursor ignore width and height */
 	if (!obj) {
@@ -8305,64 +8303,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		goto finish;
 	}
 
-	/* we only need to pin inside GTT if cursor is non-phy */
+	/* plane helper's prepare_fb() already handled pinning */
 	mutex_lock(&dev->struct_mutex);
-	if (!INTEL_INFO(dev)->cursor_needs_physical) {
-		unsigned alignment;
-
-		/*
-		 * Global gtt pte registers are special registers which actually
-		 * forward writes to a chunk of system memory. Which means that
-		 * there is no risk that the register values disappear as soon
-		 * as we call intel_runtime_pm_put(), so it is correct to wrap
-		 * only the pin/unpin/fence and not more.
-		 */
-		intel_runtime_pm_get(dev_priv);
-
-		/* Note that the w/a also requires 2 PTE of padding following
-		 * the bo. We currently fill all unused PTE with the shadow
-		 * page and so we should always have valid PTE following the
-		 * cursor preventing the VT-d warning.
-		 */
-		alignment = 0;
-		if (need_vtd_wa(dev))
-			alignment = 64*1024;
-
-		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_put_fence(obj);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to release fence for cursor");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_unpin;
-		}
-
+	if (!INTEL_INFO(dev)->cursor_needs_physical)
 		addr = i915_gem_obj_ggtt_offset(obj);
-
-		intel_runtime_pm_put(dev_priv);
-	} else {
-		int align = IS_I830(dev) ? 16 * 1024 : 256;
-		ret = i915_gem_object_attach_phys(obj, align);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to attach phys object\n");
-			goto fail_locked;
-		}
+	else
 		addr = obj->phys_handle->busaddr;
-	}
 
  finish:
-	if (intel_crtc->cursor_bo) {
-		if (!INTEL_INFO(dev)->cursor_needs_physical)
-			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
-	}
-
-	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
-			  INTEL_FRONTBUFFER_CURSOR(pipe));
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;
@@ -8379,13 +8327,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
 	}
-
-	return 0;
-fail_unpin:
-	i915_gem_object_unpin_from_display_plane(obj);
-fail_locked:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
 }
 
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
@@ -11544,7 +11485,6 @@ disable_unpin:
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
 			  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
-	intel_unpin_fb_obj(intel_fb_obj(plane->fb));
 	mutex_unlock(&dev->struct_mutex);
 	plane->fb = NULL;
 
@@ -11568,42 +11508,6 @@ intel_check_primary_plane(struct drm_plane *plane,
 					     false, true, &state->visible);
 }
 
-static int
-intel_prepare_primary_plane(struct drm_plane *plane,
-			    struct intel_plane_state *state)
-{
-	struct drm_crtc *crtc = state->base.crtc;
-	struct drm_framebuffer *fb = state->base.fb;
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
-	int ret;
-
-	intel_crtc_wait_for_pending_flips(crtc);
-
-	if (intel_crtc_has_pending_flip(crtc)) {
-		DRM_ERROR("pipe is still busy with an old pageflip\n");
-		return -EBUSY;
-	}
-
-	if (old_obj != obj) {
-		mutex_lock(&dev->struct_mutex);
-		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
-		if (ret == 0)
-			i915_gem_track_fb(old_obj, obj,
-					  INTEL_FRONTBUFFER_PRIMARY(pipe));
-		mutex_unlock(&dev->struct_mutex);
-		if (ret != 0) {
-			DRM_DEBUG_KMS("pin & fence failed\n");
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 static void
 intel_commit_primary_plane(struct drm_plane *plane,
 			   struct intel_plane_state *state)
@@ -11614,9 +11518,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	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 drm_framebuffer *old_fb = plane->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_rect *src = &state->src;
 
@@ -11687,62 +11589,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 		intel_update_fbc(dev);
 		mutex_unlock(&dev->struct_mutex);
 	}
-
-	if (old_fb && old_fb != fb) {
-		if (intel_crtc->active)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(old_obj);
-		mutex_unlock(&dev->struct_mutex);
-	}
-}
-
-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 intel_plane_state state;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int ret;
-
-	state.base.crtc = crtc;
-	state.base.fb = fb;
-
-	/* sample coordinates in 16.16 fixed point */
-	state.src.x1 = src_x;
-	state.src.x2 = src_x + src_w;
-	state.src.y1 = src_y;
-	state.src.y2 = src_y + src_h;
-
-	/* integer pixels */
-	state.dst.x1 = crtc_x;
-	state.dst.x2 = crtc_x + crtc_w;
-	state.dst.y1 = crtc_y;
-	state.dst.y2 = crtc_y + crtc_h;
-
-	state.clip.x1 = 0;
-	state.clip.y1 = 0;
-	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
-	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
-
-	state.orig_src = state.src;
-	state.orig_dst = state.dst;
-
-	ret = intel_check_primary_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	ret = intel_prepare_primary_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	intel_commit_primary_plane(plane, &state);
-
-	return 0;
 }
 
 /* Common destruction function for both primary and cursor planes */
@@ -11755,8 +11601,8 @@ void intel_plane_destroy(struct drm_plane *plane)
 }
 
 static const struct drm_plane_funcs intel_primary_plane_funcs = {
-	.update_plane = intel_primary_plane_setplane,
-	.disable_plane = intel_primary_plane_disable,
+	.update_plane = drm_plane_helper_update,
+	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
 	.atomic_duplicate_state = intel_plane_duplicate_state,
@@ -11816,15 +11662,22 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	return &primary->base;
 }
 
-static int
+static void
 intel_cursor_plane_disable(struct drm_plane *plane)
 {
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+
 	if (!plane->fb)
-		return 0;
+		return;
 
 	BUG_ON(!plane->crtc);
 
-	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+	intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+
+	mutex_lock(&plane->dev->struct_mutex);
+	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
+			  INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe));
+	mutex_unlock(&plane->dev->struct_mutex);
 }
 
 static int
@@ -11838,7 +11691,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	int crtc_w, crtc_h;
 	unsigned stride;
 	int ret;
 
@@ -11856,15 +11708,14 @@ intel_check_cursor_plane(struct drm_plane *plane,
 		return 0;
 
 	/* Check for which cursor types we support */
-	crtc_w = drm_rect_width(&state->orig_dst);
-	crtc_h = drm_rect_height(&state->orig_dst);
-	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
-		DRM_DEBUG("Cursor dimension not supported\n");
+	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
+		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
+			  state->base.crtc_w, state->base.crtc_h);
 		return -EINVAL;
 	}
 
-	stride = roundup_pow_of_two(crtc_w) * 4;
-	if (obj->base.size < stride * crtc_h) {
+	stride = roundup_pow_of_two(state->base.crtc_w) * 4;
+	if (obj->base.size < stride * state->base.crtc_h) {
 		DRM_DEBUG_KMS("buffer is too small\n");
 		return -ENOMEM;
 	}
@@ -11883,7 +11734,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	return ret;
 }
 
-static int
+static void
 intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
@@ -11893,10 +11744,9 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
-	int crtc_w, crtc_h;
 
-	crtc->cursor_x = state->orig_dst.x1;
-	crtc->cursor_y = state->orig_dst.y1;
+	crtc->cursor_x = state->base.crtc_x;
+	crtc->cursor_y = state->base.crtc_y;
 
 	intel_plane->crtc_x = state->orig_dst.x1;
 	intel_plane->crtc_y = state->orig_dst.y1;
@@ -11909,63 +11759,20 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	intel_plane->obj = obj;
 
 	if (fb != crtc->cursor->fb) {
-		crtc_w = drm_rect_width(&state->orig_dst);
-		crtc_h = drm_rect_height(&state->orig_dst);
-		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+		intel_crtc_cursor_set_obj(crtc, obj,
+					  state->base.crtc_w,
+					  state->base.crtc_h);
 	} else {
 		intel_crtc_update_cursor(crtc, state->visible);
 
 		intel_frontbuffer_flip(crtc->dev,
 				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
-
-		return 0;
 	}
 }
 
-static int
-intel_cursor_plane_update(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 intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane_state state;
-	int ret;
-
-	state.base.crtc = crtc;
-	state.base.fb = fb;
-
-	/* sample coordinates in 16.16 fixed point */
-	state.src.x1 = src_x;
-	state.src.x2 = src_x + src_w;
-	state.src.y1 = src_y;
-	state.src.y2 = src_y + src_h;
-
-	/* integer pixels */
-	state.dst.x1 = crtc_x;
-	state.dst.x2 = crtc_x + crtc_w;
-	state.dst.y1 = crtc_y;
-	state.dst.y2 = crtc_y + crtc_h;
-
-	state.clip.x1 = 0;
-	state.clip.y1 = 0;
-	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
-	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
-
-	state.orig_src = state.src;
-	state.orig_dst = state.dst;
-
-	ret = intel_check_cursor_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	return intel_commit_cursor_plane(plane, &state);
-}
-
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {
-	.update_plane = intel_cursor_plane_update,
-	.disable_plane = intel_cursor_plane_disable,
+	.update_plane = drm_plane_helper_update,
+	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
 	.atomic_duplicate_state = intel_plane_duplicate_state,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d3677c3..28ccaf7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1106,7 +1106,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	uint32_t src_x, src_y, src_w, src_h;
 	struct drm_rect *src = &state->src;
 	struct drm_rect *dst = &state->dst;
-	struct drm_rect *orig_src = &state->orig_src;
 	const struct drm_rect *clip = &state->clip;
 	int hscale, vscale;
 	int max_scale, min_scale;
@@ -1187,10 +1186,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
 				    intel_plane->rotation);
 
 		/* sanity check to make sure the src viewport wasn't enlarged */
-		WARN_ON(src->x1 < (int) orig_src->x1 ||
-			src->y1 < (int) orig_src->y1 ||
-			src->x2 > (int) orig_src->x2 ||
-			src->y2 > (int) orig_src->y2);
+		WARN_ON(src->x1 < (int) state->base.src_x ||
+			src->y1 < (int) state->base.src_y ||
+			src->x2 > (int) state->base.src_x + state->base.src_w ||
+			src->y2 > (int) state->base.src_y + state->base.src_h);
 
 		/*
 		 * Hardware doesn't handle subpixel coordinates.
@@ -1258,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	return 0;
 }
 
-static int
-intel_prepare_sprite_plane(struct drm_plane *plane,
-			   struct intel_plane_state *state)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_crtc *crtc = state->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *fb = state->base.fb;
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
-	int ret;
-
-	if (old_obj != obj) {
-		mutex_lock(&dev->struct_mutex);
-
-		/* Note that this will apply the VT-d workaround for scanouts,
-		 * which is more restrictive than required for sprites. (The
-		 * primary plane requires 256KiB alignment with 64 PTE padding,
-		 * the sprite planes only require 128KiB alignment and 32 PTE
-		 * padding.
-		 */
-		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
-		if (ret == 0)
-			i915_gem_track_fb(old_obj, obj,
-					  INTEL_FRONTBUFFER_SPRITE(pipe));
-		mutex_unlock(&dev->struct_mutex);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
 static void
 intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
@@ -1304,7 +1268,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
@@ -1362,68 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 		if (!primary_was_enabled && primary_enabled)
 			intel_post_enable_primary(crtc);
 	}
-
-	/* Unpin old obj after new one is active to avoid ugliness */
-	if (old_obj && old_obj != obj) {
-
-		/*
-		 * It's fairly common to simply update the position of
-		 * an existing object.  In that case, we don't need to
-		 * wait for vblank to avoid ugliness, we only need to
-		 * do the pin & ref bookkeeping.
-		 */
-		if (intel_crtc->active)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(old_obj);
-		mutex_unlock(&dev->struct_mutex);
-	}
-}
-
-static int
-intel_update_plane(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 intel_plane_state state;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int ret;
-
-	state.base.crtc = crtc;
-	state.base.fb = fb;
-
-	/* sample coordinates in 16.16 fixed point */
-	state.src.x1 = src_x;
-	state.src.x2 = src_x + src_w;
-	state.src.y1 = src_y;
-	state.src.y2 = src_y + src_h;
-
-	/* integer pixels */
-	state.dst.x1 = crtc_x;
-	state.dst.x2 = crtc_x + crtc_w;
-	state.dst.y1 = crtc_y;
-	state.dst.y2 = crtc_y + crtc_h;
-
-	state.clip.x1 = 0;
-	state.clip.y1 = 0;
-	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
-	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
-	state.orig_src = state.src;
-	state.orig_dst = state.dst;
-
-	ret = intel_check_sprite_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	ret = intel_prepare_sprite_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	intel_commit_sprite_plane(plane, &state);
-	return 0;
 }
 
 int
@@ -1459,7 +1360,6 @@ intel_sprite_plane_disable(struct drm_plane *plane)
 			intel_wait_for_vblank(dev, intel_plane->pipe);
 
 		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(intel_plane->obj);
 		i915_gem_track_fb(intel_plane->obj, NULL,
 				  INTEL_FRONTBUFFER_SPRITE(pipe));
 		mutex_unlock(&dev->struct_mutex);
@@ -1557,21 +1457,19 @@ int intel_plane_set_property(struct drm_plane *plane,
 
 int intel_plane_restore(struct drm_plane *plane)
 {
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-
 	if (!plane->crtc || !plane->fb)
 		return 0;
 
 	return plane->funcs->update_plane(plane, plane->crtc, plane->fb,
-				  intel_plane->crtc_x, intel_plane->crtc_y,
-				  intel_plane->crtc_w, intel_plane->crtc_h,
-				  intel_plane->src_x, intel_plane->src_y,
-				  intel_plane->src_w, intel_plane->src_h);
+				  plane->state->crtc_x, plane->state->crtc_y,
+				  plane->state->crtc_w, plane->state->crtc_h,
+				  plane->state->src_x, plane->state->src_y,
+				  plane->state->src_w, plane->state->src_h);
 }
 
 static const struct drm_plane_funcs intel_plane_funcs = {
-	.update_plane = intel_update_plane,
-	.disable_plane = intel_sprite_plane_disable,
+	.update_plane = drm_plane_helper_update,
+	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
 	.atomic_duplicate_state = intel_plane_duplicate_state,
-- 
1.8.5.1

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

Re: [Intel-gfx] [PATCH 6/7] drm/i915: Switch plane handling to atomic helpers

[Ville Syrjälä]

On Thu, Nov 13, 2014 at 10:43:25AM -0800, Matt Roper wrote:
> Now that we have hooks to enable the atomic plane helpers, we can use
> the plane helpers for our .update_plane() and .disable_plane()
> entrypoints.
> 
> Even though we'd already refactored our plane handling code into
> check/commit, we still have to rework some of that logic here as we make
> the transition.  The atomic plane helpers now call prepare_fb/cleanup_fb
> for us, so note the following:
>  - 'commit' should not longer unpin the old object; the cleanup_fb will
>    take care of this for us
>  - prepare_fb only gets called when we actually have an fb, so the
>    internal 'disable' code needs to update frontbuffer tracking
>  - the cursor plane code never had 'prepare' code separated from its
>    'commit' function, so a lot of the cursor's 'commit' needs to be
>    dropped since prepare_fb already does it for us
>  - with all the pinning work removed from cursor commit, it can no
>    longer fail and should return void rather than int
> 
> Testcase: igt/kms_plane
> Testcase: igt/kms_universal_plane
> Testcase: igt/kms_cursor_crc
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 255 +++++------------------------------
>  drivers/gpu/drm/i915/intel_sprite.c  | 122 ++---------------
>  2 files changed, 41 insertions(+), 336 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 32a2b12..8d5a3b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8285,17 +8285,15 @@ static bool cursor_size_ok(struct drm_device *dev,
>  	return true;
>  }
>  
> -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> -				     struct drm_i915_gem_object *obj,
> -				     uint32_t width, uint32_t height)
> +static void intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> +				      struct drm_i915_gem_object *obj,
> +				      uint32_t width, uint32_t height)
>  {
>  	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;
>  	unsigned old_width;
>  	uint32_t addr;
> -	int ret;
>  
>  	/* if we want to turn off the cursor ignore width and height */
>  	if (!obj) {
> @@ -8305,64 +8303,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  		goto finish;
>  	}
>  
> -	/* we only need to pin inside GTT if cursor is non-phy */
> +	/* plane helper's prepare_fb() already handled pinning */
>  	mutex_lock(&dev->struct_mutex);
> -	if (!INTEL_INFO(dev)->cursor_needs_physical) {
> -		unsigned alignment;
> -
> -		/*
> -		 * Global gtt pte registers are special registers which actually
> -		 * forward writes to a chunk of system memory. Which means that
> -		 * there is no risk that the register values disappear as soon
> -		 * as we call intel_runtime_pm_put(), so it is correct to wrap
> -		 * only the pin/unpin/fence and not more.
> -		 */
> -		intel_runtime_pm_get(dev_priv);
> -
> -		/* Note that the w/a also requires 2 PTE of padding following
> -		 * the bo. We currently fill all unused PTE with the shadow
> -		 * page and so we should always have valid PTE following the
> -		 * cursor preventing the VT-d warning.
> -		 */
> -		alignment = 0;
> -		if (need_vtd_wa(dev))
> -			alignment = 64*1024;
> -
> -		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> -		if (ret) {
> -			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> -			intel_runtime_pm_put(dev_priv);
> -			goto fail_locked;
> -		}
> -
> -		ret = i915_gem_object_put_fence(obj);
> -		if (ret) {
> -			DRM_DEBUG_KMS("failed to release fence for cursor");
> -			intel_runtime_pm_put(dev_priv);
> -			goto fail_unpin;
> -		}
> -

So I'm now wondering where all this code is disappearing into?

Anyway this conflicts pretty harshly with Gutavo's pending patches. I
think at least the particular patch touching this had the r-b already
(apart from a stale bo unre left behind, which can easily be fixed up
when applying the patch).

So I would like to get Gustavo's patches in first, especially as they
move the code already to the right direction. We also have bugs in the
current code that need to fixed. Gustavo's patches came close, but not
quite there IIRC. So I think it would be nice to get that stuff done
first, and then toss this atomic stuff on top.


> +	if (!INTEL_INFO(dev)->cursor_needs_physical)
>  		addr = i915_gem_obj_ggtt_offset(obj);
> -
> -		intel_runtime_pm_put(dev_priv);
> -	} else {
> -		int align = IS_I830(dev) ? 16 * 1024 : 256;
> -		ret = i915_gem_object_attach_phys(obj, align);
> -		if (ret) {
> -			DRM_DEBUG_KMS("failed to attach phys object\n");
> -			goto fail_locked;
> -		}
> +	else
>  		addr = obj->phys_handle->busaddr;
> -	}
>  
>   finish:
> -	if (intel_crtc->cursor_bo) {
> -		if (!INTEL_INFO(dev)->cursor_needs_physical)
> -			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> -	}
> -
> -	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> -			  INTEL_FRONTBUFFER_CURSOR(pipe));
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	old_width = intel_crtc->cursor_width;
> @@ -8379,13 +8327,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  
>  		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
>  	}
> -
> -	return 0;
> -fail_unpin:
> -	i915_gem_object_unpin_from_display_plane(obj);
> -fail_locked:
> -	mutex_unlock(&dev->struct_mutex);
> -	return ret;
>  }
>  
>  static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
> @@ -11544,7 +11485,6 @@ disable_unpin:
>  	mutex_lock(&dev->struct_mutex);
>  	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
>  			  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
> -	intel_unpin_fb_obj(intel_fb_obj(plane->fb));
>  	mutex_unlock(&dev->struct_mutex);
>  	plane->fb = NULL;
>  
> @@ -11568,42 +11508,6 @@ intel_check_primary_plane(struct drm_plane *plane,
>  					     false, true, &state->visible);
>  }
>  
> -static int
> -intel_prepare_primary_plane(struct drm_plane *plane,
> -			    struct intel_plane_state *state)
> -{
> -	struct drm_crtc *crtc = state->base.crtc;
> -	struct drm_framebuffer *fb = state->base.fb;
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> -	int ret;
> -
> -	intel_crtc_wait_for_pending_flips(crtc);
> -
> -	if (intel_crtc_has_pending_flip(crtc)) {
> -		DRM_ERROR("pipe is still busy with an old pageflip\n");
> -		return -EBUSY;
> -	}
> -
> -	if (old_obj != obj) {
> -		mutex_lock(&dev->struct_mutex);
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> -		if (ret == 0)
> -			i915_gem_track_fb(old_obj, obj,
> -					  INTEL_FRONTBUFFER_PRIMARY(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -		if (ret != 0) {
> -			DRM_DEBUG_KMS("pin & fence failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  static void
>  intel_commit_primary_plane(struct drm_plane *plane,
>  			   struct intel_plane_state *state)
> @@ -11614,9 +11518,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	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 drm_framebuffer *old_fb = plane->fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_rect *src = &state->src;
>  
> @@ -11687,62 +11589,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  		intel_update_fbc(dev);
>  		mutex_unlock(&dev->struct_mutex);
>  	}
> -
> -	if (old_fb && old_fb != fb) {
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_obj);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -}
> -
> -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 intel_plane_state state;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int ret;
> -
> -	state.base.crtc = crtc;
> -	state.base.fb = fb;
> -
> -	/* sample coordinates in 16.16 fixed point */
> -	state.src.x1 = src_x;
> -	state.src.x2 = src_x + src_w;
> -	state.src.y1 = src_y;
> -	state.src.y2 = src_y + src_h;
> -
> -	/* integer pixels */
> -	state.dst.x1 = crtc_x;
> -	state.dst.x2 = crtc_x + crtc_w;
> -	state.dst.y1 = crtc_y;
> -	state.dst.y2 = crtc_y + crtc_h;
> -
> -	state.clip.x1 = 0;
> -	state.clip.y1 = 0;
> -	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> -	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> -
> -	state.orig_src = state.src;
> -	state.orig_dst = state.dst;
> -
> -	ret = intel_check_primary_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	ret = intel_prepare_primary_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	intel_commit_primary_plane(plane, &state);
> -
> -	return 0;
>  }
>  
>  /* Common destruction function for both primary and cursor planes */
> @@ -11755,8 +11601,8 @@ void intel_plane_destroy(struct drm_plane *plane)
>  }
>  
>  static const struct drm_plane_funcs intel_primary_plane_funcs = {
> -	.update_plane = intel_primary_plane_setplane,
> -	.disable_plane = intel_primary_plane_disable,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>  	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
>  	.atomic_duplicate_state = intel_plane_duplicate_state,
> @@ -11816,15 +11662,22 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	return &primary->base;
>  }
>  
> -static int
> +static void
>  intel_cursor_plane_disable(struct drm_plane *plane)
>  {
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +
>  	if (!plane->fb)
> -		return 0;
> +		return;
>  
>  	BUG_ON(!plane->crtc);
>  
> -	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> +	intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> +
> +	mutex_lock(&plane->dev->struct_mutex);
> +	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> +			  INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe));
> +	mutex_unlock(&plane->dev->struct_mutex);
>  }
>  
>  static int
> @@ -11838,7 +11691,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	int crtc_w, crtc_h;
>  	unsigned stride;
>  	int ret;
>  
> @@ -11856,15 +11708,14 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  		return 0;
>  
>  	/* Check for which cursor types we support */
> -	crtc_w = drm_rect_width(&state->orig_dst);
> -	crtc_h = drm_rect_height(&state->orig_dst);
> -	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
> -		DRM_DEBUG("Cursor dimension not supported\n");
> +	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
> +		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
> +			  state->base.crtc_w, state->base.crtc_h);
>  		return -EINVAL;
>  	}
>  
> -	stride = roundup_pow_of_two(crtc_w) * 4;
> -	if (obj->base.size < stride * crtc_h) {
> +	stride = roundup_pow_of_two(state->base.crtc_w) * 4;
> +	if (obj->base.size < stride * state->base.crtc_h) {
>  		DRM_DEBUG_KMS("buffer is too small\n");
>  		return -ENOMEM;
>  	}
> @@ -11883,7 +11734,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	return ret;
>  }
>  
> -static int
> +static void
>  intel_commit_cursor_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
>  {
> @@ -11893,10 +11744,9 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
>  	struct drm_i915_gem_object *obj = intel_fb->obj;
> -	int crtc_w, crtc_h;
>  
> -	crtc->cursor_x = state->orig_dst.x1;
> -	crtc->cursor_y = state->orig_dst.y1;
> +	crtc->cursor_x = state->base.crtc_x;
> +	crtc->cursor_y = state->base.crtc_y;
>  
>  	intel_plane->crtc_x = state->orig_dst.x1;
>  	intel_plane->crtc_y = state->orig_dst.y1;
> @@ -11909,63 +11759,20 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	intel_plane->obj = obj;
>  
>  	if (fb != crtc->cursor->fb) {
> -		crtc_w = drm_rect_width(&state->orig_dst);
> -		crtc_h = drm_rect_height(&state->orig_dst);
> -		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> +		intel_crtc_cursor_set_obj(crtc, obj,
> +					  state->base.crtc_w,
> +					  state->base.crtc_h);
>  	} else {
>  		intel_crtc_update_cursor(crtc, state->visible);
>  
>  		intel_frontbuffer_flip(crtc->dev,
>  				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> -
> -		return 0;
>  	}
>  }
>  
> -static int
> -intel_cursor_plane_update(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 intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane_state state;
> -	int ret;
> -
> -	state.base.crtc = crtc;
> -	state.base.fb = fb;
> -
> -	/* sample coordinates in 16.16 fixed point */
> -	state.src.x1 = src_x;
> -	state.src.x2 = src_x + src_w;
> -	state.src.y1 = src_y;
> -	state.src.y2 = src_y + src_h;
> -
> -	/* integer pixels */
> -	state.dst.x1 = crtc_x;
> -	state.dst.x2 = crtc_x + crtc_w;
> -	state.dst.y1 = crtc_y;
> -	state.dst.y2 = crtc_y + crtc_h;
> -
> -	state.clip.x1 = 0;
> -	state.clip.y1 = 0;
> -	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> -	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> -
> -	state.orig_src = state.src;
> -	state.orig_dst = state.dst;
> -
> -	ret = intel_check_cursor_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	return intel_commit_cursor_plane(plane, &state);
> -}
> -
>  static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> -	.update_plane = intel_cursor_plane_update,
> -	.disable_plane = intel_cursor_plane_disable,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>  	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
>  	.atomic_duplicate_state = intel_plane_duplicate_state,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index d3677c3..28ccaf7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1106,7 +1106,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	uint32_t src_x, src_y, src_w, src_h;
>  	struct drm_rect *src = &state->src;
>  	struct drm_rect *dst = &state->dst;
> -	struct drm_rect *orig_src = &state->orig_src;
>  	const struct drm_rect *clip = &state->clip;
>  	int hscale, vscale;
>  	int max_scale, min_scale;
> @@ -1187,10 +1186,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  				    intel_plane->rotation);
>  
>  		/* sanity check to make sure the src viewport wasn't enlarged */
> -		WARN_ON(src->x1 < (int) orig_src->x1 ||
> -			src->y1 < (int) orig_src->y1 ||
> -			src->x2 > (int) orig_src->x2 ||
> -			src->y2 > (int) orig_src->y2);
> +		WARN_ON(src->x1 < (int) state->base.src_x ||
> +			src->y1 < (int) state->base.src_y ||
> +			src->x2 > (int) state->base.src_x + state->base.src_w ||
> +			src->y2 > (int) state->base.src_y + state->base.src_h);
>  
>  		/*
>  		 * Hardware doesn't handle subpixel coordinates.
> @@ -1258,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	return 0;
>  }
>  
> -static int
> -intel_prepare_sprite_plane(struct drm_plane *plane,
> -			   struct intel_plane_state *state)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_framebuffer *fb = state->base.fb;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> -	int ret;
> -
> -	if (old_obj != obj) {
> -		mutex_lock(&dev->struct_mutex);
> -
> -		/* Note that this will apply the VT-d workaround for scanouts,
> -		 * which is more restrictive than required for sprites. (The
> -		 * primary plane requires 256KiB alignment with 64 PTE padding,
> -		 * the sprite planes only require 128KiB alignment and 32 PTE
> -		 * padding.
> -		 */
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> -		if (ret == 0)
> -			i915_gem_track_fb(old_obj, obj,
> -					  INTEL_FRONTBUFFER_SPRITE(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static void
>  intel_commit_sprite_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
> @@ -1304,7 +1268,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_plane->obj;
>  	int crtc_x, crtc_y;
>  	unsigned int crtc_w, crtc_h;
>  	uint32_t src_x, src_y, src_w, src_h;
> @@ -1362,68 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  		if (!primary_was_enabled && primary_enabled)
>  			intel_post_enable_primary(crtc);
>  	}
> -
> -	/* Unpin old obj after new one is active to avoid ugliness */
> -	if (old_obj && old_obj != obj) {
> -
> -		/*
> -		 * It's fairly common to simply update the position of
> -		 * an existing object.  In that case, we don't need to
> -		 * wait for vblank to avoid ugliness, we only need to
> -		 * do the pin & ref bookkeeping.
> -		 */
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_obj);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -}
> -
> -static int
> -intel_update_plane(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 intel_plane_state state;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int ret;
> -
> -	state.base.crtc = crtc;
> -	state.base.fb = fb;
> -
> -	/* sample coordinates in 16.16 fixed point */
> -	state.src.x1 = src_x;
> -	state.src.x2 = src_x + src_w;
> -	state.src.y1 = src_y;
> -	state.src.y2 = src_y + src_h;
> -
> -	/* integer pixels */
> -	state.dst.x1 = crtc_x;
> -	state.dst.x2 = crtc_x + crtc_w;
> -	state.dst.y1 = crtc_y;
> -	state.dst.y2 = crtc_y + crtc_h;
> -
> -	state.clip.x1 = 0;
> -	state.clip.y1 = 0;
> -	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> -	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> -	state.orig_src = state.src;
> -	state.orig_dst = state.dst;
> -
> -	ret = intel_check_sprite_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	ret = intel_prepare_sprite_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	intel_commit_sprite_plane(plane, &state);
> -	return 0;
>  }
>  
>  int
> @@ -1459,7 +1360,6 @@ intel_sprite_plane_disable(struct drm_plane *plane)
>  			intel_wait_for_vblank(dev, intel_plane->pipe);
>  
>  		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(intel_plane->obj);
>  		i915_gem_track_fb(intel_plane->obj, NULL,
>  				  INTEL_FRONTBUFFER_SPRITE(pipe));
>  		mutex_unlock(&dev->struct_mutex);
> @@ -1557,21 +1457,19 @@ int intel_plane_set_property(struct drm_plane *plane,
>  
>  int intel_plane_restore(struct drm_plane *plane)
>  {
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -
>  	if (!plane->crtc || !plane->fb)
>  		return 0;
>  
>  	return plane->funcs->update_plane(plane, plane->crtc, plane->fb,
> -				  intel_plane->crtc_x, intel_plane->crtc_y,
> -				  intel_plane->crtc_w, intel_plane->crtc_h,
> -				  intel_plane->src_x, intel_plane->src_y,
> -				  intel_plane->src_w, intel_plane->src_h);
> +				  plane->state->crtc_x, plane->state->crtc_y,
> +				  plane->state->crtc_w, plane->state->crtc_h,
> +				  plane->state->src_x, plane->state->src_y,
> +				  plane->state->src_w, plane->state->src_h);
>  }
>  
>  static const struct drm_plane_funcs intel_plane_funcs = {
> -	.update_plane = intel_update_plane,
> -	.disable_plane = intel_sprite_plane_disable,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>  	.destroy = intel_plane_destroy,
>  	.set_property = intel_plane_set_property,
>  	.atomic_duplicate_state = intel_plane_duplicate_state,
> -- 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/7] drm/i915: Drop unused position fields

[Matt Roper]

The userspace-requested plane coordinates are now stored in
plane->state.base (and the i915-adjusted values are stored in
plane->state), so we no longer use the coordinate fields in intel_plane
or the orig_{src,dst} fields in intel_plane_state.  Drop them.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 ----------------
 drivers/gpu/drm/i915/intel_drv.h     |  6 ------
 drivers/gpu/drm/i915/intel_sprite.c  |  8 --------
 3 files changed, 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8d5a3b4..a595a58 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11526,14 +11526,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	crtc->x = src->x1;
 	crtc->y = src->y1;
 
-	intel_plane->crtc_x = state->orig_dst.x1;
-	intel_plane->crtc_y = state->orig_dst.y1;
-	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
-	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
-	intel_plane->src_x = state->orig_src.x1;
-	intel_plane->src_y = state->orig_src.y1;
-	intel_plane->src_w = drm_rect_width(&state->orig_src);
-	intel_plane->src_h = drm_rect_height(&state->orig_src);
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
@@ -11748,14 +11740,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	crtc->cursor_x = state->base.crtc_x;
 	crtc->cursor_y = state->base.crtc_y;
 
-	intel_plane->crtc_x = state->orig_dst.x1;
-	intel_plane->crtc_y = state->orig_dst.y1;
-	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
-	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
-	intel_plane->src_x = state->orig_src.x1;
-	intel_plane->src_y = state->orig_src.y1;
-	intel_plane->src_w = drm_rect_width(&state->orig_src);
-	intel_plane->src_h = drm_rect_height(&state->orig_src);
 	intel_plane->obj = obj;
 
 	if (fb != crtc->cursor->fb) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1882de1..fdebeea 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -248,8 +248,6 @@ struct intel_plane_state {
 	struct drm_rect src;
 	struct drm_rect dst;
 	struct drm_rect clip;
-	struct drm_rect orig_src;
-	struct drm_rect orig_dst;
 	bool visible;
 };
 
@@ -485,10 +483,6 @@ struct intel_plane {
 	struct drm_i915_gem_object *obj;
 	bool can_scale;
 	int max_downscale;
-	int crtc_x, crtc_y;
-	unsigned int crtc_w, crtc_h;
-	uint32_t src_x, src_y;
-	uint32_t src_w, src_h;
 	unsigned int rotation;
 
 	/* Since we need to change the watermarks before/after
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 28ccaf7..9c00b68 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1282,14 +1282,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
 	WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
 
-	intel_plane->crtc_x = state->orig_dst.x1;
-	intel_plane->crtc_y = state->orig_dst.y1;
-	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
-	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
-	intel_plane->src_x = state->orig_src.x1;
-	intel_plane->src_y = state->orig_src.y1;
-	intel_plane->src_w = drm_rect_width(&state->orig_src);
-	intel_plane->src_h = drm_rect_height(&state->orig_src);
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-- 
1.8.5.1

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

Re: [PATCH 7/7] drm/i915: Drop unused position fields

[Bob Paauwe]

On Thu, 13 Nov 2014 10:43:26 -0800
Matt Roper <matthew.d.roper@intel.com> wrote:

> The userspace-requested plane coordinates are now stored in
> plane->state.base (and the i915-adjusted values are stored in
> plane->state), so we no longer use the coordinate fields in intel_plane
> or the orig_{src,dst} fields in intel_plane_state.  Drop them.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 ----------------
>  drivers/gpu/drm/i915/intel_drv.h     |  6 ------
>  drivers/gpu/drm/i915/intel_sprite.c  |  8 --------
>  3 files changed, 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8d5a3b4..a595a58 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11526,14 +11526,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	crtc->x = src->x1;
>  	crtc->y = src->y1;
>  
> -	intel_plane->crtc_x = state->orig_dst.x1;
> -	intel_plane->crtc_y = state->orig_dst.y1;
> -	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> -	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> -	intel_plane->src_x = state->orig_src.x1;
> -	intel_plane->src_y = state->orig_src.y1;
> -	intel_plane->src_w = drm_rect_width(&state->orig_src);
> -	intel_plane->src_h = drm_rect_height(&state->orig_src);
>  	intel_plane->obj = obj;
>  
>  	if (intel_crtc->active) {
> @@ -11748,14 +11740,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	crtc->cursor_x = state->base.crtc_x;
>  	crtc->cursor_y = state->base.crtc_y;
>  
> -	intel_plane->crtc_x = state->orig_dst.x1;
> -	intel_plane->crtc_y = state->orig_dst.y1;
> -	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> -	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> -	intel_plane->src_x = state->orig_src.x1;
> -	intel_plane->src_y = state->orig_src.y1;
> -	intel_plane->src_w = drm_rect_width(&state->orig_src);
> -	intel_plane->src_h = drm_rect_height(&state->orig_src);
>  	intel_plane->obj = obj;
>  
>  	if (fb != crtc->cursor->fb) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1882de1..fdebeea 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -248,8 +248,6 @@ struct intel_plane_state {
>  	struct drm_rect src;
>  	struct drm_rect dst;
>  	struct drm_rect clip;
> -	struct drm_rect orig_src;
> -	struct drm_rect orig_dst;
>  	bool visible;
>  };
>  
> @@ -485,10 +483,6 @@ struct intel_plane {
>  	struct drm_i915_gem_object *obj;
>  	bool can_scale;
>  	int max_downscale;
> -	int crtc_x, crtc_y;
> -	unsigned int crtc_w, crtc_h;
> -	uint32_t src_x, src_y;
> -	uint32_t src_w, src_h;
>  	unsigned int rotation;
>  
>  	/* Since we need to change the watermarks before/after
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 28ccaf7..9c00b68 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1282,14 +1282,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  	primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
>  	WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
>  
> -	intel_plane->crtc_x = state->orig_dst.x1;
> -	intel_plane->crtc_y = state->orig_dst.y1;
> -	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> -	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> -	intel_plane->src_x = state->orig_src.x1;
> -	intel_plane->src_y = state->orig_src.y1;
> -	intel_plane->src_w = drm_rect_width(&state->orig_src);
> -	intel_plane->src_h = drm_rect_height(&state->orig_src);
>  	intel_plane->obj = obj;
>  
>  	if (intel_crtc->active) {

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

Re: [PATCH 7/7] drm/i915: Drop unused position fields

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=291/291->290/291
PNV: pass/total=356/356->356/356
ILK: pass/total=372/372->371/372
IVB: pass/total=544/546->524/546
SNB: pass/total=423/425->418/425
HSW: pass/total=579/579->570/579
BDW: pass/total=433/435->434/435
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
BYT: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(18, M36M31)PASS(1, M38) -> TIMEOUT(4, M36)
ILK: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, FAIL(6, M26)DMESG_FAIL(1, M26)TIMEOUT(33, M37M6M26)PASS(1, M26) -> TIMEOUT(4, M6)
IVB: Intel_gpu_tools, igt_gem_bad_reloc_negative-reloc, NSPT(9, M4M21M34M45)PASS(16, M21M34M4M45) -> NSPT(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-128x128-onscreen, DMESG_WARN(3, M21)PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-128x128-random, DMESG_WARN(1, M21)TIMEOUT(2, M21)PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-128x128-sliding, DMESG_WARN(2, M21)TIMEOUT(1, M21)PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-256x256-offscreen, DMESG_WARN(3, M21)PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-256x256-onscreen, DMESG_WARN(3, M21)PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-256x256-random, DMESG_WARN(3, M21)PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-256x256-sliding, DMESG_WARN(2, M21)TIMEOUT(1, M21)PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-64x64-offscreen, DMESG_WARN(3, M21)PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-64x64-onscreen, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-64x64-random, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-64x64-sliding, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_plane_plane-position-covered-pipe-A-plane-1, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_plane_plane-position-covered-pipe-B-plane-1, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_plane_plane-position-covered-pipe-C-plane-1, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_plane_plane-position-hole-pipe-A-plane-1, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_plane_plane-position-hole-pipe-A-plane-2, PASS(1, M4) -> TIMEOUT(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_plane_plane-position-hole-pipe-B-plane-1, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_plane_plane-position-hole-pipe-C-plane-1, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_rotation_crc_primary-rotation, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_rotation_crc_sprite-rotation, PASS(1, M4) -> DMESG_WARN(1, M21)PASS(3, M21)
SNB: Intel_gpu_tools, igt_gem_concurrent_blit_gpuX-bcs-overwrite-source-forked, FAIL(1, M22)PASS(21, M35M22) -> PASS(4, M22)
SNB: Intel_gpu_tools, igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip, DMESG_WARN(3, M22)PASS(1, M22) -> DMESG_WARN(1, M22)PASS(3, M22)
SNB: Intel_gpu_tools, igt_kms_plane_plane-position-covered-pipe-B-plane-1, DMESG_WARN(3, M22)PASS(1, M22) -> DMESG_WARN(1, M22)PASS(3, M22)
SNB: Intel_gpu_tools, igt_kms_plane_plane-position-hole-pipe-A-plane-1, DMESG_WARN(3, M22)PASS(1, M22) -> DMESG_WARN(1, M22)PASS(3, M22)
SNB: Intel_gpu_tools, igt_kms_plane_plane-position-hole-pipe-B-plane-1, DMESG_WARN(3, M22)PASS(1, M22) -> DMESG_WARN(1, M22)PASS(3, M22)
SNB: Intel_gpu_tools, igt_kms_rotation_crc_sprite-rotation, DMESG_WARN(3, M22)PASS(1, M22) -> DMESG_WARN(1, M22)PASS(3, M22)
SNB: Intel_gpu_tools, igt_pm_rpm_cursor, DMESG_WARN(3, M22)PASS(1, M35) -> DMESG_WARN(1, M22)PASS(3, M22)
HSW: Intel_gpu_tools, igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip, DMESG_WARN(3, M19)PASS(1, M39) -> DMESG_WARN(1, M19)PASS(3, M19)
HSW: Intel_gpu_tools, igt_kms_plane_plane-position-covered-pipe-A-plane-1, DMESG_WARN(3, M19)PASS(1, M39) -> DMESG_WARN(1, M19)PASS(3, M19)
HSW: Intel_gpu_tools, igt_kms_plane_plane-position-covered-pipe-B-plane-1, DMESG_WARN(3, M19)PASS(1, M39) -> DMESG_WARN(1, M19)PASS(3, M19)
HSW: Intel_gpu_tools, igt_kms_plane_plane-position-covered-pipe-C-plane-1, DMESG_WARN(3, M19)PASS(1, M39) -> DMESG_WARN(1, M19)PASS(3, M19)
HSW: Intel_gpu_tools, igt_kms_plane_plane-position-hole-pipe-B-plane-1, DMESG_WARN(3, M19)PASS(1, M39) -> DMESG_WARN(1, M19)PASS(3, M19)
HSW: Intel_gpu_tools, igt_kms_plane_plane-position-hole-pipe-C-plane-2, TIMEOUT(1, M19)PASS(3, M39M19) -> TIMEOUT(1, M19)PASS(3, M19)
HSW: Intel_gpu_tools, igt_kms_rotation_crc_primary-rotation, DMESG_WARN(3, M19)PASS(1, M39) -> DMESG_WARN(1, M19)PASS(3, M19)
HSW: Intel_gpu_tools, igt_pm_rpm_cursor, DMESG_WARN(2, M19)TIMEOUT(1, M19)PASS(1, M39) -> DMESG_WARN(1, M19)PASS(3, M19)
HSW: Intel_gpu_tools, igt_pm_rpm_cursor-dpms, DMESG_WARN(3, M19)PASS(1, M39) -> DMESG_WARN(1, M19)PASS(3, M19)
BDW: Intel_gpu_tools, igt_gem_reset_stats_ban-bsd, DMESG_WARN(1, M30)PASS(24, M30M42M28) -> PASS(4, M28)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/8] drm/i915: Integrate kerneldoc for atomic plane helpers

[Matt Roper]

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 Documentation/DocBook/drm.tmpl            |  5 +++++
 drivers/gpu/drm/i915/intel_atomic_plane.c | 10 ++++++++++
 2 files changed, 15 insertions(+)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 9449cd6..fefee7d 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -3879,6 +3879,11 @@ int num_ioctls;</synopsis>
         </para>
       </sect2>
       <sect2>
+        <title>Atomic Plane Helpers</title>
+!Pdrivers/gpu/drm/i915/intel_atomic_plane.c atomic plane helpers
+!Idrivers/gpu/drm/i915/intel_atomic_plane.c
+      </sect2>
+      <sect2>
         <title>Output Probing</title>
         <para>
 	  This section covers output probing and related infrastructure like the
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 7c039bf..b0c319c 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -21,6 +21,16 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+/**
+ * DOC: atomic plane helper support
+ *
+ * The functions here are used by the atomic plane helper functions to
+ * implement legacy plane updates (i.e., drm_plane->update_plane() and
+ * drm_plane->disable_plane()).  This allows plane updates to use the
+ * atomic state infrastructure and perform plane updates as separate
+ * prepare/check/commit/cleanup steps.
+ */
+
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_plane_helper.h>
-- 
1.8.5.1

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