[PATCH 0/6] i915 atomic properties

[PATCH 0/6] i915 atomic properties

[Matt Roper]

I had hoped to make this a full "nuclear pageflip" enablement series, but I
didn't have as much time to work on this today as I hoped, so I still need to
finish up the final integration with the DRM core atomic code.  Several of my
early patches are working and relatively straightforward, so I figured I'd post
these as a preliminary patchset; hopefully I'll have the rest of the patches
necessary to expose "nuclear pageflip" (i.e., single-crtc, plane-only updates)
ready in the next day or so.

This series does add (and then use) a transitional helper for planes'
.set_property() handler, similar to what we have for .update_plane() and
.disable_plane().  The transitional helper can be used by drivers that are
starting to implement the atomic driver entrypoints, but aren't ready to switch
over to full atomic functionality yet.

Note that the i915-specific patches here depend on Ander's recent series which
hasn't been merged yet:
        [PATCH 0/7] Make drm_crtc->state match pipe_config

Cc: dri-devel@lists.freedesktop.org

Matt Roper (6):
  drm/i915: Move rotation from intel_plane to intel_plane_state
  drm/i915: Consolidate plane handler vtables
  drm/plane-helper: Add transitional helper for .set_plane().
  drm/plane-helper: Fix transitional helper kerneldocs
  drm/i915: Add .atomic_{get,set}_property() entrypoints to planes
  drm/i915: Replace intel_set_property() with transitional helper

 drivers/gpu/drm/drm_plane_helper.c        | 109 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_atomic_plane.c |  84 +++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c      |  63 ++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h          |  22 ++++--
 drivers/gpu/drm/i915/intel_fbc.c          |   4 +-
 drivers/gpu/drm/i915/intel_sprite.c       |  65 ++++++------------
 include/drm/drm_plane_helper.h            |   3 +
 7 files changed, 268 insertions(+), 82 deletions(-)

-- 
1.8.5.1

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

[PATCH 1/6] drm/i915: Move rotation from intel_plane to intel_plane_state

[Matt Roper]

Runtime state that can be manipulated via properties should now go in
intel_plane_state instead so that it can be tracked as part of an atomic
transaction.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b6c4667..84b0b4d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2435,6 +2435,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane_state *state =
+		to_intel_plane_state(crtc->primary->state);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_i915_gem_object *obj;
 	int plane = intel_crtc->plane;
@@ -2532,7 +2534,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
 		intel_crtc->dspaddr_offset = linear_offset;
 	}
 
-	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
+	if (state->rotation == BIT(DRM_ROTATE_180)) {
 		dspcntr |= DISPPLANE_ROTATE_180;
 
 		x += (intel_crtc->config->pipe_src_w - 1);
@@ -2568,6 +2570,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane_state *state =
+		to_intel_plane_state(crtc->primary->state);
 	struct drm_i915_gem_object *obj;
 	int plane = intel_crtc->plane;
 	unsigned long linear_offset;
@@ -2634,7 +2638,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
 					       pixel_size,
 					       fb->pitches[0]);
 	linear_offset -= intel_crtc->dspaddr_offset;
-	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
+	if (state->rotation == BIT(DRM_ROTATE_180)) {
 		dspcntr |= DISPPLANE_ROTATE_180;
 
 		if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
@@ -2673,6 +2677,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane_state *state =
+		to_intel_plane_state(crtc->primary->state);
 	struct intel_framebuffer *intel_fb;
 	struct drm_i915_gem_object *obj;
 	int pipe = intel_crtc->pipe;
@@ -2731,7 +2737,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	}
 
 	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
-	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180))
+	if (state->rotation == BIT(DRM_ROTATE_180))
 		plane_ctl |= PLANE_CTL_ROTATE_180;
 
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
@@ -8216,6 +8222,8 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane_state *state =
+		to_intel_plane_state(crtc->cursor->state);
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl;
 
@@ -8242,7 +8250,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 			cntl |= CURSOR_PIPE_CSC_ENABLE;
 	}
 
-	if (to_intel_plane(crtc->cursor)->rotation == BIT(DRM_ROTATE_180))
+	if (state->rotation == BIT(DRM_ROTATE_180))
 		cntl |= CURSOR_ROTATE_180;
 
 	if (intel_crtc->cursor_cntl != cntl) {
@@ -8265,6 +8273,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane_state *state =
+		to_intel_plane_state(crtc->cursor->state);
 	int pipe = intel_crtc->pipe;
 	int x = crtc->cursor_x;
 	int y = crtc->cursor_y;
@@ -8303,8 +8313,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	I915_WRITE(CURPOS(pipe), pos);
 
 	/* ILK+ do this automagically */
-	if (HAS_GMCH_DISPLAY(dev) &&
-		to_intel_plane(crtc->cursor)->rotation == BIT(DRM_ROTATE_180)) {
+	if (HAS_GMCH_DISPLAY(dev) && state->rotation == BIT(DRM_ROTATE_180)) {
 		base += (intel_crtc->cursor_height *
 			intel_crtc->cursor_width - 1) * 4;
 	}
@@ -11756,7 +11765,6 @@ intel_check_primary_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc;
-	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
@@ -11790,7 +11798,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 		if (intel_crtc->primary_enabled &&
 		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
 		    dev_priv->fbc.plane == intel_crtc->plane &&
-		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
+		    state->rotation != BIT(DRM_ROTATE_0)) {
 			intel_crtc->atomic.disable_fbc = true;
 		}
 
@@ -11975,6 +11983,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 						    int pipe)
 {
 	struct intel_plane *primary;
+	struct intel_plane_state *state;
 	const uint32_t *intel_primary_formats;
 	int num_formats;
 
@@ -11987,12 +11996,13 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 		kfree(primary);
 		return NULL;
 	}
+	state = to_intel_plane_state(primary->base.state);
 
 	primary->can_scale = false;
 	primary->max_downscale = 1;
 	primary->pipe = pipe;
 	primary->plane = pipe;
-	primary->rotation = BIT(DRM_ROTATE_0);
+	state->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)
@@ -12020,7 +12030,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 		if (dev->mode_config.rotation_property)
 			drm_object_attach_property(&primary->base.base,
 				dev->mode_config.rotation_property,
-				primary->rotation);
+				state->rotation);
 	}
 
 	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
@@ -12148,6 +12158,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 						   int pipe)
 {
 	struct intel_plane *cursor;
+	struct intel_plane_state *state;
 
 	cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
 	if (cursor == NULL)
@@ -12158,12 +12169,13 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 		kfree(cursor);
 		return NULL;
 	}
+	state = to_intel_plane_state(cursor->base.state);
 
 	cursor->can_scale = false;
 	cursor->max_downscale = 1;
 	cursor->pipe = pipe;
 	cursor->plane = pipe;
-	cursor->rotation = BIT(DRM_ROTATE_0);
+	state->rotation = BIT(DRM_ROTATE_0);
 	cursor->check_plane = intel_check_cursor_plane;
 	cursor->commit_plane = intel_commit_cursor_plane;
 
@@ -12182,7 +12194,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 		if (dev->mode_config.rotation_property)
 			drm_object_attach_property(&cursor->base.base,
 				dev->mode_config.rotation_property,
-				cursor->rotation);
+				state->rotation);
 	}
 
 	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c8c0b7f..918cce2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -250,6 +250,9 @@ struct intel_plane_state {
 	struct drm_rect clip;
 	bool visible;
 
+	/* Intel-specific plane properties */
+	unsigned int rotation;
+
 	/*
 	 * used only for sprite planes to determine when to implicitly
 	 * enable/disable the primary plane
@@ -509,7 +512,6 @@ struct intel_plane {
 	struct drm_i915_gem_object *obj;
 	bool can_scale;
 	int max_downscale;
-	unsigned int rotation;
 
 	/* Since we need to change the watermarks before/after
 	 * enabling/disabling the planes, we need to store the parameters here
@@ -518,6 +520,12 @@ struct intel_plane {
 	 */
 	struct intel_plane_wm_parameters wm;
 
+	/*
+	 * NOTE: Do not place new plane state fields here (e.g., when adding
+	 * new plane properties).  New runtime state should now be placed in
+	 * the intel_plane_state structure and accessed via drm_plane->state.
+	 */
+
 	void (*update_plane)(struct drm_plane *plane,
 			     struct drm_crtc *crtc,
 			     struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 5b1d7c4..3e87454 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -496,6 +496,7 @@ void intel_fbc_update(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = NULL, *tmp_crtc;
 	struct intel_crtc *intel_crtc;
+	struct intel_plane_state *primary_state;
 	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 	const struct drm_display_mode *adjusted_mode;
@@ -540,6 +541,7 @@ void intel_fbc_update(struct drm_device *dev)
 	}
 
 	intel_crtc = to_intel_crtc(crtc);
+	primary_state = to_intel_plane_state(crtc->primary->state);
 	fb = crtc->primary->fb;
 	obj = intel_fb_obj(fb);
 	adjusted_mode = &intel_crtc->config->base.adjusted_mode;
@@ -595,7 +597,7 @@ void intel_fbc_update(struct drm_device *dev)
 		goto out_disable;
 	}
 	if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-	    to_intel_plane(crtc->primary)->rotation != BIT(DRM_ROTATE_0)) {
+	    primary_state->rotation != BIT(DRM_ROTATE_0)) {
 		if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
 			DRM_DEBUG_KMS("Rotation unsupported, disabling\n");
 		goto out_disable;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0a6094e..140c5b7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -187,6 +187,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	struct drm_device *dev = drm_plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
+	struct intel_plane_state *state =
+		to_intel_plane_state(drm_plane->state);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 	u32 plane_ctl, stride;
@@ -256,7 +258,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	default:
 		BUG();
 	}
-	if (intel_plane->rotation == BIT(DRM_ROTATE_180))
+	if (state->rotation == BIT(DRM_ROTATE_180))
 		plane_ctl |= PLANE_CTL_ROTATE_180;
 
 	plane_ctl |= PLANE_CTL_ENABLE;
@@ -407,6 +409,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_plane_state *state = to_intel_plane_state(dplane->state);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
@@ -493,7 +496,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
-	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
+	if (state->rotation == BIT(DRM_ROTATE_180)) {
 		sprctl |= SP_ROTATE_180;
 
 		x += src_w;
@@ -608,6 +611,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane_state *state = to_intel_plane_state(plane->state);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
@@ -684,7 +688,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
-	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
+	if (state->rotation == BIT(DRM_ROTATE_180)) {
 		sprctl |= SPRITE_ROTATE_180;
 
 		/* HSW and BDW does this automagically in hardware */
@@ -813,6 +817,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane_state *state = to_intel_plane_state(plane->state);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
@@ -884,7 +889,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
-	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
+	if (state->rotation == BIT(DRM_ROTATE_180)) {
 		dvscntr |= DVS_ROTATE_180;
 
 		x += src_w;
@@ -1125,7 +1130,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
 
 	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
-			intel_plane->rotation);
+			state->rotation);
 
 	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
 	BUG_ON(hscale < 0);
@@ -1166,7 +1171,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 				     drm_rect_height(dst) * vscale - drm_rect_height(src));
 
 		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
-				    intel_plane->rotation);
+				    state->rotation);
 
 		/* sanity check to make sure the src viewport wasn't enlarged */
 		WARN_ON(src->x1 < (int) state->base.src_x ||
@@ -1367,7 +1372,7 @@ int intel_plane_set_property(struct drm_plane *plane,
 			     uint64_t val)
 {
 	struct drm_device *dev = plane->dev;
-	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane_state *state = to_intel_plane_state(plane->state);
 	uint64_t old_val;
 	int ret = -ENOENT;
 
@@ -1376,14 +1381,14 @@ int intel_plane_set_property(struct drm_plane *plane,
 		if (hweight32(val & 0xf) != 1)
 			return -EINVAL;
 
-		if (intel_plane->rotation == val)
+		if (state->rotation == val)
 			return 0;
 
-		old_val = intel_plane->rotation;
-		intel_plane->rotation = val;
+		old_val = state->rotation;
+		state->rotation = val;
 		ret = intel_plane_restore(plane);
 		if (ret)
-			intel_plane->rotation = old_val;
+			state->rotation = old_val;
 	}
 
 	return ret;
@@ -1457,6 +1462,7 @@ int
 intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 {
 	struct intel_plane *intel_plane;
+	struct intel_plane_state *state;
 	unsigned long possible_crtcs;
 	const uint32_t *plane_formats;
 	int num_plane_formats;
@@ -1475,6 +1481,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 		kfree(intel_plane);
 		return -ENOMEM;
 	}
+	state = to_intel_plane_state(intel_plane->base.state);
 
 	switch (INTEL_INFO(dev)->gen) {
 	case 5:
@@ -1545,7 +1552,7 @@ 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);
+	state->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);
@@ -1567,7 +1574,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	if (dev->mode_config.rotation_property)
 		drm_object_attach_property(&intel_plane->base.base,
 					   dev->mode_config.rotation_property,
-					   intel_plane->rotation);
+					   state->rotation);
 
 	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
 
-- 
1.8.5.1

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

[PATCH 2/6] drm/i915: Consolidate plane handler vtables

[Matt Roper]

All of the previous refactoring/consolidation of plane code has resulted
in intel_primary_plane_funcs, intel_cursor_plane_funcs, and
intel_sprite_plane_funcs being identical.  Replace all of these with a
single 'intel_plane_funcs' vtable for simplicity.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 84b0b4d..24787d9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11969,7 +11969,7 @@ void intel_plane_destroy(struct drm_plane *plane)
 	kfree(intel_plane);
 }
 
-static const struct drm_plane_funcs intel_primary_plane_funcs = {
+const struct drm_plane_funcs intel_plane_funcs = {
 	.update_plane = drm_plane_helper_update,
 	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
@@ -12017,7 +12017,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	}
 
 	drm_universal_plane_init(dev, &primary->base, 0,
-				 &intel_primary_plane_funcs,
+				 &intel_plane_funcs,
 				 intel_primary_formats, num_formats,
 				 DRM_PLANE_TYPE_PRIMARY);
 
@@ -12145,15 +12145,6 @@ update:
 		intel_crtc_update_cursor(crtc, state->visible);
 }
 
-static const struct drm_plane_funcs intel_cursor_plane_funcs = {
-	.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,
-	.atomic_destroy_state = intel_plane_destroy_state,
-};
-
 static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 						   int pipe)
 {
@@ -12180,7 +12171,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	cursor->commit_plane = intel_commit_cursor_plane;
 
 	drm_universal_plane_init(dev, &cursor->base, 0,
-				 &intel_cursor_plane_funcs,
+				 &intel_plane_funcs,
 				 intel_cursor_formats,
 				 ARRAY_SIZE(intel_cursor_formats),
 				 DRM_PLANE_TYPE_CURSOR);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 918cce2..2133fd1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -908,6 +908,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv);
 void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
 
 /* intel_display.c */
+extern const struct drm_plane_funcs intel_plane_funcs;
 bool intel_has_pending_fb_unpin(struct drm_device *dev);
 int intel_pch_rawclk(struct drm_device *dev);
 void intel_mark_busy(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 140c5b7..8af4c69 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1406,15 +1406,6 @@ int intel_plane_restore(struct drm_plane *plane)
 				  plane->state->src_w, plane->state->src_h);
 }
 
-static const struct drm_plane_funcs intel_sprite_plane_funcs = {
-	.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,
-	.atomic_destroy_state = intel_plane_destroy_state,
-};
-
 static uint32_t ilk_plane_formats[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_YUYV,
@@ -1557,7 +1548,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int 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_sprite_plane_funcs,
+				       &intel_plane_funcs,
 				       plane_formats, num_plane_formats,
 				       DRM_PLANE_TYPE_OVERLAY);
 	if (ret) {
-- 
1.8.5.1

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

[PATCH 3/6] drm/plane-helper: Add transitional helper for .set_plane().

[Matt Roper]

Add a transitional helper for planes' .set_property() entrypoint,
similar to what we already have for .update_plane() and
.disable_plane().  This allows drivers that are still in the process of
transitioning to implement and exercise the .atomic_set_property()
driver entrypoint that will eventually be used by atomic.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 105 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_plane_helper.h     |   3 ++
 2 files changed, 108 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index f24c4cf..1b1e927 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -578,3 +578,108 @@ int drm_plane_helper_disable(struct drm_plane *plane)
 	return drm_plane_helper_commit(plane, plane_state, plane->fb);
 }
 EXPORT_SYMBOL(drm_plane_helper_disable);
+
+/**
+ * drm_plane_helper_set_property() - Transitional helper for property updates
+ * @plane: plane object to update
+ * @prop: property to update
+ * @val: value to set property to
+ *
+ * Provides a default plane property handler using the atomic plane update
+ * functions.  Drivers in the process of transitioning to atomic should
+ * replace their plane.set_property() entrypoint with this function and
+ * then implement the plane.atomic_set_property() entrypoint.
+ *
+ * This is useful for piecewise transitioning of a driver to the atomic helpers.
+ *
+ * Note that this helper assumes that no hardware programming should be
+ * performed if the plane is disabled.  When the plane is not attached to a
+ * crtc, we just swap in the validated plane state and return; there's no
+ * call to atomic_begin()/atomic_update()/atomic_flush().
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_plane_helper_set_property(struct drm_plane *plane,
+				  struct drm_property *prop,
+				  uint64_t val)
+{
+	struct drm_plane_state *plane_state;
+	struct drm_plane_helper_funcs *plane_funcs = plane->helper_private;
+	struct drm_crtc_helper_funcs *crtc_funcs;
+	int ret;
+
+	/*
+	 * Drivers may not have an .atomic_set_property() handler if they have
+	 * no driver-specific properties, but they generally wouldn't setup a
+	 * .set_property() handler (pointing to this function) for the plane in
+	 * that case either; throw a warning since this is probably a mistake.
+	 */
+	if (WARN_ON(!plane->funcs->atomic_set_property))
+		return -EINVAL;
+
+	if (plane->funcs->atomic_duplicate_state)
+		plane_state = plane->funcs->atomic_duplicate_state(plane);
+	else if (plane->state)
+		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
+	else
+		plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
+	if (!plane_state)
+		return -ENOMEM;
+	plane_state->plane = plane;
+
+	/*
+	 * Update the state object with the new property value we want to
+	 * set.  If the property is not recognized as a driver-specific property,
+	 * -EINVAL will be returned here.
+	 */
+	ret = plane->funcs->atomic_set_property(plane, plane_state, prop, val);
+	if (ret)
+		goto out;
+
+	/*
+	 * Check that the updated plane state is actually programmable (e.g.,
+	 * doesn't violate hardware constraints).
+	 */
+	if (plane_funcs->atomic_check) {
+		ret = plane_funcs->atomic_check(plane, plane_state);
+		if (ret)
+			goto out;
+	}
+
+	/* Point of no return, commit sw state. */
+	swap(plane->state, plane_state);
+
+	/*
+	 * If the plane is disabled, we're done.  No hardware programming is
+	 * attempted when the plane is disabled.
+	 */
+	if (!plane->crtc)
+		goto out;
+
+	/* Start hardware transaction to commit new state to hardware */
+	crtc_funcs = plane->crtc->helper_private;
+	if (crtc_funcs && crtc_funcs->atomic_begin)
+		crtc_funcs->atomic_begin(plane->crtc);
+	plane_funcs->atomic_update(plane, plane_state);
+	if (crtc_funcs && crtc_funcs->atomic_flush)
+		crtc_funcs->atomic_flush(plane->crtc);
+
+	/*
+	 * Since we're not using full atomic yet, we still need to update the shadow
+	 * property value that's managed by the DRM core since that's where the
+	 * property values will be read back from.
+	 */
+	drm_object_property_set_value(&plane->base, prop, val);
+
+out:
+	if (plane_state) {
+		if (plane->funcs->atomic_destroy_state)
+			plane->funcs->atomic_destroy_state(plane, plane_state);
+		else
+			drm_atomic_helper_plane_destroy_state(plane, plane_state);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_plane_helper_set_property);
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index a185392..4a56961 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -107,6 +107,9 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 			    uint32_t src_x, uint32_t src_y,
 			    uint32_t src_w, uint32_t src_h);
 int drm_plane_helper_disable(struct drm_plane *plane);
+int drm_plane_helper_set_property(struct drm_plane *plane,
+				  struct drm_property *prop,
+				  uint64_t val);
 
 /* For use by drm_crtc_helper.c */
 int drm_plane_helper_commit(struct drm_plane *plane,
-- 
1.8.5.1

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

[PATCH 4/6] drm/plane-helper: Fix transitional helper kerneldocs

[Matt Roper]

drm_plane_helper_{update,disable} are not specific to primary planes;
fix some copy/paste summaries to avoid confusion.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 1b1e927..3e7e26f 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -484,7 +484,7 @@ out:
 }
 
 /**
- * drm_plane_helper_update() - Helper for primary plane update
+ * drm_plane_helper_update() - Transitional helper for plane update
  * @plane: plane object to update
  * @crtc: owning CRTC of owning plane
  * @fb: framebuffer to flip onto plane
@@ -541,7 +541,7 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 EXPORT_SYMBOL(drm_plane_helper_update);
 
 /**
- * drm_plane_helper_disable() - Helper for primary plane disable
+ * drm_plane_helper_disable() - Transitional helper for plane disable
  * @plane: plane to disable
  *
  * Provides a default plane disable handler using the atomic plane update
-- 
1.8.5.1

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

[PATCH 5/6] drm/i915: Add .atomic_{get, set}_property() entrypoints to planes

[Matt Roper]

When we flip on the DRIVER_ATOMIC bit, the DRM core will start calling
this entrypoint to set and lookup driver-specific plane property values,
rather than maintaining a shadow copy in object->properties.

Note that although we add these functions to the plane vtable, they will
not yet be called.  Future patches that switch our .set_property()
handler and/or enable full atomic functionality are required before
these code paths will be executed.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 4027fc0..0b07b90 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -150,3 +150,63 @@ const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
 	.atomic_update = intel_plane_atomic_update,
 };
 
+/**
+ * intel_plane_atomic_get_property - fetch plane property value
+ * @plane: plane to fetch property for
+ * @state: state containing the property value
+ * @property: property to look up
+ * @val: pointer to write property value into
+ *
+ * The DRM core does not store shadow copies of properties for
+ * atomic-capable drivers.  This entrypoint is used to fetch
+ * the current value of a driver-specific plane property.
+ */
+int
+intel_plane_atomic_get_property(struct drm_plane *plane,
+				const struct drm_plane_state *state,
+				struct drm_property *property,
+				uint64_t *val)
+{
+	struct drm_mode_config *config = &plane->dev->mode_config;
+	struct intel_plane_state *intel_state = to_intel_plane_state(state);
+
+	if (property == config->rotation_property) {
+		*val = intel_state->rotation;
+	} else {
+		DRM_DEBUG_KMS("Unknown plane property '%s'\n", property->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * intel_plane_atomic_set_property - set plane property value
+ * @plane: plane to set property for
+ * @state: state to update property value in
+ * @property: property to set
+ * @val: value to set property to
+ *
+ * Writes the specified property value for a plane into the provided atomic
+ * state object.
+ *
+ * Returns 0 on success, -EINVAL on unrecognized properties
+ */
+int
+intel_plane_atomic_set_property(struct drm_plane *plane,
+				struct drm_plane_state *state,
+				struct drm_property *property,
+				uint64_t val)
+{
+	struct drm_mode_config *config = &plane->dev->mode_config;
+	struct intel_plane_state *intel_state = to_intel_plane_state(state);
+
+	if (property == config->rotation_property) {
+		intel_state->rotation = val;
+	} else {
+		DRM_DEBUG_KMS("Unknown plane property '%s'\n", property->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 24787d9..7db18d16 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11974,6 +11974,8 @@ const struct drm_plane_funcs intel_plane_funcs = {
 	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
+	.atomic_get_property = intel_plane_atomic_get_property,
+	.atomic_set_property = intel_plane_atomic_set_property,
 	.atomic_duplicate_state = intel_plane_duplicate_state,
 	.atomic_destroy_state = intel_plane_destroy_state,
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2133fd1..97c3ace 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -963,6 +963,14 @@ int intel_prepare_plane_fb(struct drm_plane *plane,
 			   struct drm_framebuffer *fb);
 void intel_cleanup_plane_fb(struct drm_plane *plane,
 			    struct drm_framebuffer *fb);
+int intel_plane_atomic_get_property(struct drm_plane *plane,
+				    const struct drm_plane_state *state,
+				    struct drm_property *property,
+				    uint64_t *val);
+int intel_plane_atomic_set_property(struct drm_plane *plane,
+				    struct drm_plane_state *state,
+				    struct drm_property *property,
+				    uint64_t val);
 
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
-- 
1.8.5.1

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

[PATCH 6/6] drm/i915: Replace intel_set_property() with transitional helper

[Matt Roper]

This switch allows us to exercise the .atomic_set_property() that will
be used by atomic.  The only real changes we need to make here are:
 * extract the property validation from our old set_property handler and
   stick it in intel_plane_atomic_check().
 * make intel_check_*_plane() functions capable of handling a NULL crtc
   (which will happen if userspace tries to set a property value on a
   disabled plane).

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

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 0b07b90..966c908 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -104,13 +104,20 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	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 */
+	/*
+	 * Clip all planes to CRTC size, or 0x0 if CRTC unset or disabled.
+	 * Note that CRTC may be unset if we're setting a property of a
+	 * disabled plane.
+	 */
 	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;
+	if (crtc && intel_crtc->active) {
+		intel_state->clip.x2 = intel_crtc->config->pipe_src_w;
+		intel_state->clip.y2 = intel_crtc->config->pipe_src_h;
+	} else {
+		intel_state->clip.x2 = 0;
+		intel_state->clip.y2 = 0;
+	}
 
 	/*
 	 * Disabling a plane is always okay; we just need to update
@@ -118,6 +125,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	 * get called by the plane helpers.
 	 */
 	if (state->fb == NULL && plane->state->fb != NULL) {
+		if (WARN_ON(!crtc))
+			return -EINVAL;
+
 		/*
 		 * 'prepare' is never called when plane is being disabled, so
 		 * we need to handle frontbuffer tracking as a special case
@@ -126,6 +136,10 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 			(1 << drm_plane_index(plane));
 	}
 
+	/* Rotation property should contain only a single rotation bit */
+	if (hweight32(intel_state->rotation & 0xf) != 1)
+		return -EINVAL;
+
 	return intel_plane->check_plane(plane, intel_state);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7db18d16..d34a8ca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11774,6 +11774,10 @@ intel_check_primary_plane(struct drm_plane *plane,
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	/* CRTC may be unset if we're updating a property of a disabled plane */
+	if (!crtc)
+		return 0;
+
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
@@ -11973,7 +11977,7 @@ const struct drm_plane_funcs intel_plane_funcs = {
 	.update_plane = drm_plane_helper_update,
 	.disable_plane = drm_plane_helper_disable,
 	.destroy = intel_plane_destroy,
-	.set_property = intel_plane_set_property,
+	.set_property = drm_plane_helper_set_property,
 	.atomic_get_property = intel_plane_atomic_get_property,
 	.atomic_set_property = intel_plane_atomic_set_property,
 	.atomic_duplicate_state = intel_plane_duplicate_state,
@@ -12058,6 +12062,10 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	/* CRTC may be unset if we're updating a property of a disabled plane */
+	if (!crtc)
+		return 0;
+
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 97c3ace..2d9e9a4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1246,9 +1246,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
 int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
 void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
 			       enum plane plane);
-int intel_plane_set_property(struct drm_plane *plane,
-			     struct drm_property *prop,
-			     uint64_t val);
 int intel_plane_restore(struct drm_plane *plane);
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8af4c69..c62f263 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1094,6 +1094,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
 
 	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
 
+	/* CRTC may be unset if we're updating a property of a disabled plane */
+	if (!intel_crtc)
+		return 0;
+
 	if (!fb) {
 		state->visible = false;
 		goto finish;
@@ -1367,33 +1371,6 @@ out_unlock:
 	return ret;
 }
 
-int intel_plane_set_property(struct drm_plane *plane,
-			     struct drm_property *prop,
-			     uint64_t val)
-{
-	struct drm_device *dev = plane->dev;
-	struct intel_plane_state *state = to_intel_plane_state(plane->state);
-	uint64_t old_val;
-	int ret = -ENOENT;
-
-	if (prop == dev->mode_config.rotation_property) {
-		/* exactly one rotation angle please */
-		if (hweight32(val & 0xf) != 1)
-			return -EINVAL;
-
-		if (state->rotation == val)
-			return 0;
-
-		old_val = state->rotation;
-		state->rotation = val;
-		ret = intel_plane_restore(plane);
-		if (ret)
-			state->rotation = old_val;
-	}
-
-	return ret;
-}
-
 int intel_plane_restore(struct drm_plane *plane)
 {
 	if (!plane->crtc || !plane->fb)
-- 
1.8.5.1

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

Re: [PATCH 1/6] drm/i915: Move rotation from intel_plane to intel_plane_state

[Daniel Vetter]

On Thu, Jan 15, 2015 at 06:34:19PM -0800, Matt Roper wrote:
> Runtime state that can be manipulated via properties should now go in
> intel_plane_state instead so that it can be tracked as part of an atomic
> transaction.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Imo we should move this to drm_plane_state so that fb helpers or and the
drm atomic ioctl could do the decoding in shared code instead of
duplicating things all over. But we probably want to do that once the i915
conversion (at least on the plane side) has settled a bit.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h     | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_fbc.c     |  4 +++-
>  drivers/gpu/drm/i915/intel_sprite.c  | 33 ++++++++++++++++++++-------------
>  4 files changed, 56 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b6c4667..84b0b4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2435,6 +2435,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(crtc->primary->state);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_i915_gem_object *obj;
>  	int plane = intel_crtc->plane;
> @@ -2532,7 +2534,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  		intel_crtc->dspaddr_offset = linear_offset;
>  	}
>  
> -	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
> +	if (state->rotation == BIT(DRM_ROTATE_180)) {
>  		dspcntr |= DISPPLANE_ROTATE_180;
>  
>  		x += (intel_crtc->config->pipe_src_w - 1);
> @@ -2568,6 +2570,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(crtc->primary->state);
>  	struct drm_i915_gem_object *obj;
>  	int plane = intel_crtc->plane;
>  	unsigned long linear_offset;
> @@ -2634,7 +2638,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  					       pixel_size,
>  					       fb->pitches[0]);
>  	linear_offset -= intel_crtc->dspaddr_offset;
> -	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
> +	if (state->rotation == BIT(DRM_ROTATE_180)) {
>  		dspcntr |= DISPPLANE_ROTATE_180;
>  
>  		if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> @@ -2673,6 +2677,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(crtc->primary->state);
>  	struct intel_framebuffer *intel_fb;
>  	struct drm_i915_gem_object *obj;
>  	int pipe = intel_crtc->pipe;
> @@ -2731,7 +2737,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	}
>  
>  	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> -	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180))
> +	if (state->rotation == BIT(DRM_ROTATE_180))
>  		plane_ctl |= PLANE_CTL_ROTATE_180;
>  
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> @@ -8216,6 +8222,8 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(crtc->cursor->state);
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl;
>  
> @@ -8242,7 +8250,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  			cntl |= CURSOR_PIPE_CSC_ENABLE;
>  	}
>  
> -	if (to_intel_plane(crtc->cursor)->rotation == BIT(DRM_ROTATE_180))
> +	if (state->rotation == BIT(DRM_ROTATE_180))
>  		cntl |= CURSOR_ROTATE_180;
>  
>  	if (intel_crtc->cursor_cntl != cntl) {
> @@ -8265,6 +8273,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(crtc->cursor->state);
>  	int pipe = intel_crtc->pipe;
>  	int x = crtc->cursor_x;
>  	int y = crtc->cursor_y;
> @@ -8303,8 +8313,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	I915_WRITE(CURPOS(pipe), pos);
>  
>  	/* ILK+ do this automagically */
> -	if (HAS_GMCH_DISPLAY(dev) &&
> -		to_intel_plane(crtc->cursor)->rotation == BIT(DRM_ROTATE_180)) {
> +	if (HAS_GMCH_DISPLAY(dev) && state->rotation == BIT(DRM_ROTATE_180)) {
>  		base += (intel_crtc->cursor_height *
>  			intel_crtc->cursor_width - 1) * 4;
>  	}
> @@ -11756,7 +11765,6 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc;
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
> @@ -11790,7 +11798,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  		if (intel_crtc->primary_enabled &&
>  		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
>  		    dev_priv->fbc.plane == intel_crtc->plane &&
> -		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> +		    state->rotation != BIT(DRM_ROTATE_0)) {
>  			intel_crtc->atomic.disable_fbc = true;
>  		}
>  
> @@ -11975,6 +11983,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  						    int pipe)
>  {
>  	struct intel_plane *primary;
> +	struct intel_plane_state *state;
>  	const uint32_t *intel_primary_formats;
>  	int num_formats;
>  
> @@ -11987,12 +11996,13 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  		kfree(primary);
>  		return NULL;
>  	}
> +	state = to_intel_plane_state(primary->base.state);
>  
>  	primary->can_scale = false;
>  	primary->max_downscale = 1;
>  	primary->pipe = pipe;
>  	primary->plane = pipe;
> -	primary->rotation = BIT(DRM_ROTATE_0);
> +	state->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)
> @@ -12020,7 +12030,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  		if (dev->mode_config.rotation_property)
>  			drm_object_attach_property(&primary->base.base,
>  				dev->mode_config.rotation_property,
> -				primary->rotation);
> +				state->rotation);
>  	}
>  
>  	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> @@ -12148,6 +12158,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  						   int pipe)
>  {
>  	struct intel_plane *cursor;
> +	struct intel_plane_state *state;
>  
>  	cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
>  	if (cursor == NULL)
> @@ -12158,12 +12169,13 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  		kfree(cursor);
>  		return NULL;
>  	}
> +	state = to_intel_plane_state(cursor->base.state);
>  
>  	cursor->can_scale = false;
>  	cursor->max_downscale = 1;
>  	cursor->pipe = pipe;
>  	cursor->plane = pipe;
> -	cursor->rotation = BIT(DRM_ROTATE_0);
> +	state->rotation = BIT(DRM_ROTATE_0);
>  	cursor->check_plane = intel_check_cursor_plane;
>  	cursor->commit_plane = intel_commit_cursor_plane;
>  
> @@ -12182,7 +12194,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  		if (dev->mode_config.rotation_property)
>  			drm_object_attach_property(&cursor->base.base,
>  				dev->mode_config.rotation_property,
> -				cursor->rotation);
> +				state->rotation);
>  	}
>  
>  	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c8c0b7f..918cce2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -250,6 +250,9 @@ struct intel_plane_state {
>  	struct drm_rect clip;
>  	bool visible;
>  
> +	/* Intel-specific plane properties */
> +	unsigned int rotation;
> +
>  	/*
>  	 * used only for sprite planes to determine when to implicitly
>  	 * enable/disable the primary plane
> @@ -509,7 +512,6 @@ struct intel_plane {
>  	struct drm_i915_gem_object *obj;
>  	bool can_scale;
>  	int max_downscale;
> -	unsigned int rotation;
>  
>  	/* Since we need to change the watermarks before/after
>  	 * enabling/disabling the planes, we need to store the parameters here
> @@ -518,6 +520,12 @@ struct intel_plane {
>  	 */
>  	struct intel_plane_wm_parameters wm;
>  
> +	/*
> +	 * NOTE: Do not place new plane state fields here (e.g., when adding
> +	 * new plane properties).  New runtime state should now be placed in
> +	 * the intel_plane_state structure and accessed via drm_plane->state.
> +	 */
> +
>  	void (*update_plane)(struct drm_plane *plane,
>  			     struct drm_crtc *crtc,
>  			     struct drm_framebuffer *fb,
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 5b1d7c4..3e87454 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -496,6 +496,7 @@ void intel_fbc_update(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = NULL, *tmp_crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_plane_state *primary_state;
>  	struct drm_framebuffer *fb;
>  	struct drm_i915_gem_object *obj;
>  	const struct drm_display_mode *adjusted_mode;
> @@ -540,6 +541,7 @@ void intel_fbc_update(struct drm_device *dev)
>  	}
>  
>  	intel_crtc = to_intel_crtc(crtc);
> +	primary_state = to_intel_plane_state(crtc->primary->state);
>  	fb = crtc->primary->fb;
>  	obj = intel_fb_obj(fb);
>  	adjusted_mode = &intel_crtc->config->base.adjusted_mode;
> @@ -595,7 +597,7 @@ void intel_fbc_update(struct drm_device *dev)
>  		goto out_disable;
>  	}
>  	if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> -	    to_intel_plane(crtc->primary)->rotation != BIT(DRM_ROTATE_0)) {
> +	    primary_state->rotation != BIT(DRM_ROTATE_0)) {
>  		if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
>  			DRM_DEBUG_KMS("Rotation unsupported, disabling\n");
>  		goto out_disable;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0a6094e..140c5b7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -187,6 +187,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	struct drm_device *dev = drm_plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
> +	struct intel_plane_state *state =
> +		to_intel_plane_state(drm_plane->state);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  	u32 plane_ctl, stride;
> @@ -256,7 +258,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	default:
>  		BUG();
>  	}
> -	if (intel_plane->rotation == BIT(DRM_ROTATE_180))
> +	if (state->rotation == BIT(DRM_ROTATE_180))
>  		plane_ctl |= PLANE_CTL_ROTATE_180;
>  
>  	plane_ctl |= PLANE_CTL_ENABLE;
> @@ -407,6 +409,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> +	struct intel_plane_state *state = to_intel_plane_state(dplane->state);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	int plane = intel_plane->plane;
> @@ -493,7 +496,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  							fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> -	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
> +	if (state->rotation == BIT(DRM_ROTATE_180)) {
>  		sprctl |= SP_ROTATE_180;
>  
>  		x += src_w;
> @@ -608,6 +611,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *state = to_intel_plane_state(plane->state);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	u32 sprctl, sprscale = 0;
> @@ -684,7 +688,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> -	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
> +	if (state->rotation == BIT(DRM_ROTATE_180)) {
>  		sprctl |= SPRITE_ROTATE_180;
>  
>  		/* HSW and BDW does this automagically in hardware */
> @@ -813,6 +817,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *state = to_intel_plane_state(plane->state);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	unsigned long dvssurf_offset, linear_offset;
> @@ -884,7 +889,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= dvssurf_offset;
>  
> -	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
> +	if (state->rotation == BIT(DRM_ROTATE_180)) {
>  		dvscntr |= DVS_ROTATE_180;
>  
>  		x += src_w;
> @@ -1125,7 +1130,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
>  
>  	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> -			intel_plane->rotation);
> +			state->rotation);
>  
>  	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
>  	BUG_ON(hscale < 0);
> @@ -1166,7 +1171,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  				     drm_rect_height(dst) * vscale - drm_rect_height(src));
>  
>  		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
> -				    intel_plane->rotation);
> +				    state->rotation);
>  
>  		/* sanity check to make sure the src viewport wasn't enlarged */
>  		WARN_ON(src->x1 < (int) state->base.src_x ||
> @@ -1367,7 +1372,7 @@ int intel_plane_set_property(struct drm_plane *plane,
>  			     uint64_t val)
>  {
>  	struct drm_device *dev = plane->dev;
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *state = to_intel_plane_state(plane->state);
>  	uint64_t old_val;
>  	int ret = -ENOENT;
>  
> @@ -1376,14 +1381,14 @@ int intel_plane_set_property(struct drm_plane *plane,
>  		if (hweight32(val & 0xf) != 1)
>  			return -EINVAL;
>  
> -		if (intel_plane->rotation == val)
> +		if (state->rotation == val)
>  			return 0;
>  
> -		old_val = intel_plane->rotation;
> -		intel_plane->rotation = val;
> +		old_val = state->rotation;
> +		state->rotation = val;
>  		ret = intel_plane_restore(plane);
>  		if (ret)
> -			intel_plane->rotation = old_val;
> +			state->rotation = old_val;
>  	}
>  
>  	return ret;
> @@ -1457,6 +1462,7 @@ int
>  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  {
>  	struct intel_plane *intel_plane;
> +	struct intel_plane_state *state;
>  	unsigned long possible_crtcs;
>  	const uint32_t *plane_formats;
>  	int num_plane_formats;
> @@ -1475,6 +1481,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  		kfree(intel_plane);
>  		return -ENOMEM;
>  	}
> +	state = to_intel_plane_state(intel_plane->base.state);
>  
>  	switch (INTEL_INFO(dev)->gen) {
>  	case 5:
> @@ -1545,7 +1552,7 @@ 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);
> +	state->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);
> @@ -1567,7 +1574,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  	if (dev->mode_config.rotation_property)
>  		drm_object_attach_property(&intel_plane->base.base,
>  					   dev->mode_config.rotation_property,
> -					   intel_plane->rotation);
> +					   state->rotation);
>  
>  	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>  
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/6] drm/plane-helper: Add transitional helper for .set_plane().

[Daniel Vetter]

On Thu, Jan 15, 2015 at 06:34:21PM -0800, Matt Roper wrote:
> Add a transitional helper for planes' .set_property() entrypoint,
> similar to what we already have for .update_plane() and
> .disable_plane().  This allows drivers that are still in the process of
> transitioning to implement and exercise the .atomic_set_property()
> driver entrypoint that will eventually be used by atomic.
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Since i915 is the only driver with an interest in this (I presume at
least) maybe we should make that much fuzz about this here and just move
this function into i915 private code?

At least the problem I see with too fancy transitional helpers is that
they'll grow stale fast. Which will happen with this one here as soon as
we move rotation into drm_plane_state, and will happen every time we add
something there. And I expect a lot of common core properties, e.g. for Z
order or blending modes.
-Daniel

> ---
>  drivers/gpu/drm/drm_plane_helper.c | 105 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_plane_helper.h     |   3 ++
>  2 files changed, 108 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index f24c4cf..1b1e927 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -578,3 +578,108 @@ int drm_plane_helper_disable(struct drm_plane *plane)
>  	return drm_plane_helper_commit(plane, plane_state, plane->fb);
>  }
>  EXPORT_SYMBOL(drm_plane_helper_disable);
> +
> +/**
> + * drm_plane_helper_set_property() - Transitional helper for property updates
> + * @plane: plane object to update
> + * @prop: property to update
> + * @val: value to set property to
> + *
> + * Provides a default plane property handler using the atomic plane update
> + * functions.  Drivers in the process of transitioning to atomic should
> + * replace their plane.set_property() entrypoint with this function and
> + * then implement the plane.atomic_set_property() entrypoint.
> + *
> + * This is useful for piecewise transitioning of a driver to the atomic helpers.
> + *
> + * Note that this helper assumes that no hardware programming should be
> + * performed if the plane is disabled.  When the plane is not attached to a
> + * crtc, we just swap in the validated plane state and return; there's no
> + * call to atomic_begin()/atomic_update()/atomic_flush().
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_plane_helper_set_property(struct drm_plane *plane,
> +				  struct drm_property *prop,
> +				  uint64_t val)
> +{
> +	struct drm_plane_state *plane_state;
> +	struct drm_plane_helper_funcs *plane_funcs = plane->helper_private;
> +	struct drm_crtc_helper_funcs *crtc_funcs;
> +	int ret;
> +
> +	/*
> +	 * Drivers may not have an .atomic_set_property() handler if they have
> +	 * no driver-specific properties, but they generally wouldn't setup a
> +	 * .set_property() handler (pointing to this function) for the plane in
> +	 * that case either; throw a warning since this is probably a mistake.
> +	 */
> +	if (WARN_ON(!plane->funcs->atomic_set_property))
> +		return -EINVAL;
> +
> +	if (plane->funcs->atomic_duplicate_state)
> +		plane_state = plane->funcs->atomic_duplicate_state(plane);
> +	else if (plane->state)
> +		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
> +	else
> +		plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
> +	if (!plane_state)
> +		return -ENOMEM;
> +	plane_state->plane = plane;
> +
> +	/*
> +	 * Update the state object with the new property value we want to
> +	 * set.  If the property is not recognized as a driver-specific property,
> +	 * -EINVAL will be returned here.
> +	 */
> +	ret = plane->funcs->atomic_set_property(plane, plane_state, prop, val);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * Check that the updated plane state is actually programmable (e.g.,
> +	 * doesn't violate hardware constraints).
> +	 */
> +	if (plane_funcs->atomic_check) {
> +		ret = plane_funcs->atomic_check(plane, plane_state);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	/* Point of no return, commit sw state. */
> +	swap(plane->state, plane_state);
> +
> +	/*
> +	 * If the plane is disabled, we're done.  No hardware programming is
> +	 * attempted when the plane is disabled.
> +	 */
> +	if (!plane->crtc)
> +		goto out;
> +
> +	/* Start hardware transaction to commit new state to hardware */
> +	crtc_funcs = plane->crtc->helper_private;
> +	if (crtc_funcs && crtc_funcs->atomic_begin)
> +		crtc_funcs->atomic_begin(plane->crtc);
> +	plane_funcs->atomic_update(plane, plane_state);
> +	if (crtc_funcs && crtc_funcs->atomic_flush)
> +		crtc_funcs->atomic_flush(plane->crtc);
> +
> +	/*
> +	 * Since we're not using full atomic yet, we still need to update the shadow
> +	 * property value that's managed by the DRM core since that's where the
> +	 * property values will be read back from.
> +	 */
> +	drm_object_property_set_value(&plane->base, prop, val);
> +
> +out:
> +	if (plane_state) {
> +		if (plane->funcs->atomic_destroy_state)
> +			plane->funcs->atomic_destroy_state(plane, plane_state);
> +		else
> +			drm_atomic_helper_plane_destroy_state(plane, plane_state);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_plane_helper_set_property);
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index a185392..4a56961 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -107,6 +107,9 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  			    uint32_t src_x, uint32_t src_y,
>  			    uint32_t src_w, uint32_t src_h);
>  int drm_plane_helper_disable(struct drm_plane *plane);
> +int drm_plane_helper_set_property(struct drm_plane *plane,
> +				  struct drm_property *prop,
> +				  uint64_t val);
>  
>  /* For use by drm_crtc_helper.c */
>  int drm_plane_helper_commit(struct drm_plane *plane,
> -- 
> 1.8.5.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 4/6] drm/plane-helper: Fix transitional helper kerneldocs

[Daniel Vetter]

On Thu, Jan 15, 2015 at 06:34:22PM -0800, Matt Roper wrote:
> drm_plane_helper_{update,disable} are not specific to primary planes;
> fix some copy/paste summaries to avoid confusion.
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Applied to my topic/atomic-core branch.

Thanks, Daniel
> ---
>  drivers/gpu/drm/drm_plane_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 1b1e927..3e7e26f 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -484,7 +484,7 @@ out:
>  }
>  
>  /**
> - * drm_plane_helper_update() - Helper for primary plane update
> + * drm_plane_helper_update() - Transitional helper for plane update
>   * @plane: plane object to update
>   * @crtc: owning CRTC of owning plane
>   * @fb: framebuffer to flip onto plane
> @@ -541,7 +541,7 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  EXPORT_SYMBOL(drm_plane_helper_update);
>  
>  /**
> - * drm_plane_helper_disable() - Helper for primary plane disable
> + * drm_plane_helper_disable() - Transitional helper for plane disable
>   * @plane: plane to disable
>   *
>   * Provides a default plane disable handler using the atomic plane update
> -- 
> 1.8.5.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 1/6] drm/i915: Move rotation from intel_plane to intel_plane_state

[Matt Roper]

On Tue, Jan 20, 2015 at 10:44:06AM +0100, Daniel Vetter wrote:
> On Thu, Jan 15, 2015 at 06:34:19PM -0800, Matt Roper wrote:
> > Runtime state that can be manipulated via properties should now go in
> > intel_plane_state instead so that it can be tracked as part of an atomic
> > transaction.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Imo we should move this to drm_plane_state so that fb helpers or and the
> drm atomic ioctl could do the decoding in shared code instead of
> duplicating things all over. But we probably want to do that once the i915
> conversion (at least on the plane side) has settled a bit.
> -Daniel

I considered this, but I wasn't sure how it would shake out if some
drivers have converted to atomic (and store their rotation in plane
state) and other drivers haven't converted (and still store it in some
driver-specific area).  If drivers haven't converted to atomic, the
atomic ioctl wouldn't be a concern, but it seems like the fb helpers
might have more difficulty figuring out whether it could trust rotation
values or not.  From a quick cscope, it looks like exynos and omap at
least have some rotation support and I didn't want to deal with them
until I had the Intel work in good shape.


Matt

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++++++++++++++++------------
> >  drivers/gpu/drm/i915/intel_drv.h     | 10 +++++++++-
> >  drivers/gpu/drm/i915/intel_fbc.c     |  4 +++-
> >  drivers/gpu/drm/i915/intel_sprite.c  | 33 ++++++++++++++++++++-------------
> >  4 files changed, 56 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b6c4667..84b0b4d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2435,6 +2435,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_plane_state *state =
> > +		to_intel_plane_state(crtc->primary->state);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct drm_i915_gem_object *obj;
> >  	int plane = intel_crtc->plane;
> > @@ -2532,7 +2534,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> >  		intel_crtc->dspaddr_offset = linear_offset;
> >  	}
> >  
> > -	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
> > +	if (state->rotation == BIT(DRM_ROTATE_180)) {
> >  		dspcntr |= DISPPLANE_ROTATE_180;
> >  
> >  		x += (intel_crtc->config->pipe_src_w - 1);
> > @@ -2568,6 +2570,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_plane_state *state =
> > +		to_intel_plane_state(crtc->primary->state);
> >  	struct drm_i915_gem_object *obj;
> >  	int plane = intel_crtc->plane;
> >  	unsigned long linear_offset;
> > @@ -2634,7 +2638,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> >  					       pixel_size,
> >  					       fb->pitches[0]);
> >  	linear_offset -= intel_crtc->dspaddr_offset;
> > -	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
> > +	if (state->rotation == BIT(DRM_ROTATE_180)) {
> >  		dspcntr |= DISPPLANE_ROTATE_180;
> >  
> >  		if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> > @@ -2673,6 +2677,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_plane_state *state =
> > +		to_intel_plane_state(crtc->primary->state);
> >  	struct intel_framebuffer *intel_fb;
> >  	struct drm_i915_gem_object *obj;
> >  	int pipe = intel_crtc->pipe;
> > @@ -2731,7 +2737,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >  	}
> >  
> >  	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> > -	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180))
> > +	if (state->rotation == BIT(DRM_ROTATE_180))
> >  		plane_ctl |= PLANE_CTL_ROTATE_180;
> >  
> >  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> > @@ -8216,6 +8222,8 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_plane_state *state =
> > +		to_intel_plane_state(crtc->cursor->state);
> >  	int pipe = intel_crtc->pipe;
> >  	uint32_t cntl;
> >  
> > @@ -8242,7 +8250,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >  			cntl |= CURSOR_PIPE_CSC_ENABLE;
> >  	}
> >  
> > -	if (to_intel_plane(crtc->cursor)->rotation == BIT(DRM_ROTATE_180))
> > +	if (state->rotation == BIT(DRM_ROTATE_180))
> >  		cntl |= CURSOR_ROTATE_180;
> >  
> >  	if (intel_crtc->cursor_cntl != cntl) {
> > @@ -8265,6 +8273,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_plane_state *state =
> > +		to_intel_plane_state(crtc->cursor->state);
> >  	int pipe = intel_crtc->pipe;
> >  	int x = crtc->cursor_x;
> >  	int y = crtc->cursor_y;
> > @@ -8303,8 +8313,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >  	I915_WRITE(CURPOS(pipe), pos);
> >  
> >  	/* ILK+ do this automagically */
> > -	if (HAS_GMCH_DISPLAY(dev) &&
> > -		to_intel_plane(crtc->cursor)->rotation == BIT(DRM_ROTATE_180)) {
> > +	if (HAS_GMCH_DISPLAY(dev) && state->rotation == BIT(DRM_ROTATE_180)) {
> >  		base += (intel_crtc->cursor_height *
> >  			intel_crtc->cursor_width - 1) * 4;
> >  	}
> > @@ -11756,7 +11765,6 @@ intel_check_primary_plane(struct drm_plane *plane,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc = state->base.crtc;
> >  	struct intel_crtc *intel_crtc;
> > -	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	struct drm_framebuffer *fb = state->base.fb;
> >  	struct drm_rect *dest = &state->dst;
> >  	struct drm_rect *src = &state->src;
> > @@ -11790,7 +11798,7 @@ intel_check_primary_plane(struct drm_plane *plane,
> >  		if (intel_crtc->primary_enabled &&
> >  		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> >  		    dev_priv->fbc.plane == intel_crtc->plane &&
> > -		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> > +		    state->rotation != BIT(DRM_ROTATE_0)) {
> >  			intel_crtc->atomic.disable_fbc = true;
> >  		}
> >  
> > @@ -11975,6 +11983,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >  						    int pipe)
> >  {
> >  	struct intel_plane *primary;
> > +	struct intel_plane_state *state;
> >  	const uint32_t *intel_primary_formats;
> >  	int num_formats;
> >  
> > @@ -11987,12 +11996,13 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >  		kfree(primary);
> >  		return NULL;
> >  	}
> > +	state = to_intel_plane_state(primary->base.state);
> >  
> >  	primary->can_scale = false;
> >  	primary->max_downscale = 1;
> >  	primary->pipe = pipe;
> >  	primary->plane = pipe;
> > -	primary->rotation = BIT(DRM_ROTATE_0);
> > +	state->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)
> > @@ -12020,7 +12030,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >  		if (dev->mode_config.rotation_property)
> >  			drm_object_attach_property(&primary->base.base,
> >  				dev->mode_config.rotation_property,
> > -				primary->rotation);
> > +				state->rotation);
> >  	}
> >  
> >  	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> > @@ -12148,6 +12158,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> >  						   int pipe)
> >  {
> >  	struct intel_plane *cursor;
> > +	struct intel_plane_state *state;
> >  
> >  	cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
> >  	if (cursor == NULL)
> > @@ -12158,12 +12169,13 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> >  		kfree(cursor);
> >  		return NULL;
> >  	}
> > +	state = to_intel_plane_state(cursor->base.state);
> >  
> >  	cursor->can_scale = false;
> >  	cursor->max_downscale = 1;
> >  	cursor->pipe = pipe;
> >  	cursor->plane = pipe;
> > -	cursor->rotation = BIT(DRM_ROTATE_0);
> > +	state->rotation = BIT(DRM_ROTATE_0);
> >  	cursor->check_plane = intel_check_cursor_plane;
> >  	cursor->commit_plane = intel_commit_cursor_plane;
> >  
> > @@ -12182,7 +12194,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> >  		if (dev->mode_config.rotation_property)
> >  			drm_object_attach_property(&cursor->base.base,
> >  				dev->mode_config.rotation_property,
> > -				cursor->rotation);
> > +				state->rotation);
> >  	}
> >  
> >  	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index c8c0b7f..918cce2 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -250,6 +250,9 @@ struct intel_plane_state {
> >  	struct drm_rect clip;
> >  	bool visible;
> >  
> > +	/* Intel-specific plane properties */
> > +	unsigned int rotation;
> > +
> >  	/*
> >  	 * used only for sprite planes to determine when to implicitly
> >  	 * enable/disable the primary plane
> > @@ -509,7 +512,6 @@ struct intel_plane {
> >  	struct drm_i915_gem_object *obj;
> >  	bool can_scale;
> >  	int max_downscale;
> > -	unsigned int rotation;
> >  
> >  	/* Since we need to change the watermarks before/after
> >  	 * enabling/disabling the planes, we need to store the parameters here
> > @@ -518,6 +520,12 @@ struct intel_plane {
> >  	 */
> >  	struct intel_plane_wm_parameters wm;
> >  
> > +	/*
> > +	 * NOTE: Do not place new plane state fields here (e.g., when adding
> > +	 * new plane properties).  New runtime state should now be placed in
> > +	 * the intel_plane_state structure and accessed via drm_plane->state.
> > +	 */
> > +
> >  	void (*update_plane)(struct drm_plane *plane,
> >  			     struct drm_crtc *crtc,
> >  			     struct drm_framebuffer *fb,
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index 5b1d7c4..3e87454 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -496,6 +496,7 @@ void intel_fbc_update(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc = NULL, *tmp_crtc;
> >  	struct intel_crtc *intel_crtc;
> > +	struct intel_plane_state *primary_state;
> >  	struct drm_framebuffer *fb;
> >  	struct drm_i915_gem_object *obj;
> >  	const struct drm_display_mode *adjusted_mode;
> > @@ -540,6 +541,7 @@ void intel_fbc_update(struct drm_device *dev)
> >  	}
> >  
> >  	intel_crtc = to_intel_crtc(crtc);
> > +	primary_state = to_intel_plane_state(crtc->primary->state);
> >  	fb = crtc->primary->fb;
> >  	obj = intel_fb_obj(fb);
> >  	adjusted_mode = &intel_crtc->config->base.adjusted_mode;
> > @@ -595,7 +597,7 @@ void intel_fbc_update(struct drm_device *dev)
> >  		goto out_disable;
> >  	}
> >  	if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> > -	    to_intel_plane(crtc->primary)->rotation != BIT(DRM_ROTATE_0)) {
> > +	    primary_state->rotation != BIT(DRM_ROTATE_0)) {
> >  		if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
> >  			DRM_DEBUG_KMS("Rotation unsupported, disabling\n");
> >  		goto out_disable;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 0a6094e..140c5b7 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -187,6 +187,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >  	struct drm_device *dev = drm_plane->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
> > +	struct intel_plane_state *state =
> > +		to_intel_plane_state(drm_plane->state);
> >  	const int pipe = intel_plane->pipe;
> >  	const int plane = intel_plane->plane + 1;
> >  	u32 plane_ctl, stride;
> > @@ -256,7 +258,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >  	default:
> >  		BUG();
> >  	}
> > -	if (intel_plane->rotation == BIT(DRM_ROTATE_180))
> > +	if (state->rotation == BIT(DRM_ROTATE_180))
> >  		plane_ctl |= PLANE_CTL_ROTATE_180;
> >  
> >  	plane_ctl |= PLANE_CTL_ENABLE;
> > @@ -407,6 +409,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >  	struct drm_device *dev = dplane->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> > +	struct intel_plane_state *state = to_intel_plane_state(dplane->state);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	int pipe = intel_plane->pipe;
> >  	int plane = intel_plane->plane;
> > @@ -493,7 +496,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >  							fb->pitches[0]);
> >  	linear_offset -= sprsurf_offset;
> >  
> > -	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
> > +	if (state->rotation == BIT(DRM_ROTATE_180)) {
> >  		sprctl |= SP_ROTATE_180;
> >  
> >  		x += src_w;
> > @@ -608,6 +611,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	struct drm_device *dev = plane->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	struct intel_plane_state *state = to_intel_plane_state(plane->state);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	int pipe = intel_plane->pipe;
> >  	u32 sprctl, sprscale = 0;
> > @@ -684,7 +688,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  					       pixel_size, fb->pitches[0]);
> >  	linear_offset -= sprsurf_offset;
> >  
> > -	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
> > +	if (state->rotation == BIT(DRM_ROTATE_180)) {
> >  		sprctl |= SPRITE_ROTATE_180;
> >  
> >  		/* HSW and BDW does this automagically in hardware */
> > @@ -813,6 +817,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	struct drm_device *dev = plane->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	struct intel_plane_state *state = to_intel_plane_state(plane->state);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	int pipe = intel_plane->pipe;
> >  	unsigned long dvssurf_offset, linear_offset;
> > @@ -884,7 +889,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  					       pixel_size, fb->pitches[0]);
> >  	linear_offset -= dvssurf_offset;
> >  
> > -	if (intel_plane->rotation == BIT(DRM_ROTATE_180)) {
> > +	if (state->rotation == BIT(DRM_ROTATE_180)) {
> >  		dvscntr |= DVS_ROTATE_180;
> >  
> >  		x += src_w;
> > @@ -1125,7 +1130,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
> >  	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
> >  
> >  	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> > -			intel_plane->rotation);
> > +			state->rotation);
> >  
> >  	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
> >  	BUG_ON(hscale < 0);
> > @@ -1166,7 +1171,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
> >  				     drm_rect_height(dst) * vscale - drm_rect_height(src));
> >  
> >  		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
> > -				    intel_plane->rotation);
> > +				    state->rotation);
> >  
> >  		/* sanity check to make sure the src viewport wasn't enlarged */
> >  		WARN_ON(src->x1 < (int) state->base.src_x ||
> > @@ -1367,7 +1372,7 @@ int intel_plane_set_property(struct drm_plane *plane,
> >  			     uint64_t val)
> >  {
> >  	struct drm_device *dev = plane->dev;
> > -	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	struct intel_plane_state *state = to_intel_plane_state(plane->state);
> >  	uint64_t old_val;
> >  	int ret = -ENOENT;
> >  
> > @@ -1376,14 +1381,14 @@ int intel_plane_set_property(struct drm_plane *plane,
> >  		if (hweight32(val & 0xf) != 1)
> >  			return -EINVAL;
> >  
> > -		if (intel_plane->rotation == val)
> > +		if (state->rotation == val)
> >  			return 0;
> >  
> > -		old_val = intel_plane->rotation;
> > -		intel_plane->rotation = val;
> > +		old_val = state->rotation;
> > +		state->rotation = val;
> >  		ret = intel_plane_restore(plane);
> >  		if (ret)
> > -			intel_plane->rotation = old_val;
> > +			state->rotation = old_val;
> >  	}
> >  
> >  	return ret;
> > @@ -1457,6 +1462,7 @@ int
> >  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  {
> >  	struct intel_plane *intel_plane;
> > +	struct intel_plane_state *state;
> >  	unsigned long possible_crtcs;
> >  	const uint32_t *plane_formats;
> >  	int num_plane_formats;
> > @@ -1475,6 +1481,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  		kfree(intel_plane);
> >  		return -ENOMEM;
> >  	}
> > +	state = to_intel_plane_state(intel_plane->base.state);
> >  
> >  	switch (INTEL_INFO(dev)->gen) {
> >  	case 5:
> > @@ -1545,7 +1552,7 @@ 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);
> > +	state->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);
> > @@ -1567,7 +1574,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  	if (dev->mode_config.rotation_property)
> >  		drm_object_attach_property(&intel_plane->base.base,
> >  					   dev->mode_config.rotation_property,
> > -					   intel_plane->rotation);
> > +					   state->rotation);
> >  
> >  	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> >  
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
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 1/6] drm/i915: Move rotation from intel_plane to intel_plane_state

[Daniel Vetter]

On Tue, Jan 20, 2015 at 4:26 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Tue, Jan 20, 2015 at 10:44:06AM +0100, Daniel Vetter wrote:
>> On Thu, Jan 15, 2015 at 06:34:19PM -0800, Matt Roper wrote:
>> > Runtime state that can be manipulated via properties should now go in
>> > intel_plane_state instead so that it can be tracked as part of an atomic
>> > transaction.
>> >
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>
>> Imo we should move this to drm_plane_state so that fb helpers or and the
>> drm atomic ioctl could do the decoding in shared code instead of
>> duplicating things all over. But we probably want to do that once the i915
>> conversion (at least on the plane side) has settled a bit.
>> -Daniel
>
> I considered this, but I wasn't sure how it would shake out if some
> drivers have converted to atomic (and store their rotation in plane
> state) and other drivers haven't converted (and still store it in some
> driver-specific area).  If drivers haven't converted to atomic, the
> atomic ioctl wouldn't be a concern, but it seems like the fb helpers
> might have more difficulty figuring out whether it could trust rotation
> values or not.  From a quick cscope, it looks like exynos and omap at
> least have some rotation support and I didn't want to deal with them
> until I had the Intel work in good shape.

Oh, we don't yet have an atomic fb helper. If we ever get around to
that then we'd need 2 completely separate fb helper paths anyway, so
the slight duplication because of rotation won't hurt all that much.
And the benefits for the atomic ioctl are imo worth it on it's own for
converted drivers.
-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