[PATCH 0/6] drm/atomic: Remove drm_atomic_helper_framebuffer_changed

[PATCH 0/6] drm/atomic: Remove drm_atomic_helper_framebuffer_changed

[Maarten Lankhorst]

drm_atomic_helpers use framebuffer_changed to skip some vblank waits
in case properties don't change. This may skip vblank waits also when
properties like rotation are changed. Fix this by always waiting for
vblank if planes are added to the state, and always calling prepare_fb
even when fb stays the same. Any smarts for this should be in the driver,
they know when waiting can be skipped.

While at it, cleanup drm_atomic_helper_wait_for_vblanks. It's
reusing members of the crtc_state for different things,
Maarten Lankhorst (6):
  drm/atomic: Use active instead of enable in wait_for_vblanks.
  drm/atomic: Unconditionally call prepare_fb.
  drm/atomic: Clean up wait_for_vblanks
  drm/atomic: Wait for vblank whenever a plane is added to state.
  drm/atomic: Remove drm_atomic_helper_framebuffer_changed.
  drm/i915: Add a cursor hack to allow converting legacy page flip to
    atomic.

 drivers/gpu/drm/drm_atomic_helper.c       |  77 ++++---------------
 drivers/gpu/drm/i915/intel_atomic_plane.c |  47 +++++++-----
 drivers/gpu/drm/i915/intel_display.c      | 123 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h          |   2 +
 include/drm/drm_atomic_helper.h           |   3 -
 include/drm/drm_crtc.h                    |   5 --
 6 files changed, 168 insertions(+), 89 deletions(-)

-- 
2.7.4

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

[PATCH 1/6] drm/atomic: Use active instead of enable in wait_for_vblanks.

[Maarten Lankhorst]

When DPMS was introduced to atomic, vblanks only worked when the crtc
was enabled and active. wait_for_vblanks were not converted to check for
crtc_state->active, which may cause an attempt for vblank_get to fail.

This is probably harmless, but convert from enable to active anyway.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 583f47f27b36..23767df72615 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1117,7 +1117,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 		 * vblank wait) in the ->enable boolean. */
 		old_crtc_state->enable = false;
 
-		if (!crtc->state->enable)
+		if (!crtc->state->active)
 			continue;
 
 		/* Legacy cursor ioctls are completely unsynced, and userspace
-- 
2.7.4

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

[PATCH 2/6] drm/atomic: Unconditionally call prepare_fb.

[Maarten Lankhorst]

Atomic drivers may set properties like rotation on the same fb, which
may require a call to prepare_fb even when framebuffer stays identical.

Instead of handling all the special cases in the core, let the driver
decide when prepare_fb and cleanup_fb are noops.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 23767df72615..d19563651e07 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
-			continue;
-
 		if (funcs->prepare_fb) {
 			ret = funcs->prepare_fb(plane, plane_state);
 			if (ret)
@@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 		if (j >= i)
 			continue;
 
-		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
-			continue;
-
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
@@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 	for_each_plane_in_state(old_state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 
-		if (!drm_atomic_helper_framebuffer_changed(dev, old_state, plane_state->crtc))
-			continue;
-
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
-- 
2.7.4

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

[PATCH 3/6] drm/atomic: Clean up wait_for_vblanks

[Maarten Lankhorst]

Stop relying on a per crtc_state last_vblank_count, we know in advance
how many there can be. Also stop re-using new_crtc_state->enable,
we can now simply set a bitmask with crtc_crtc_mask.
---
 drivers/gpu/drm/drm_atomic_helper.c | 29 +++++++++++++++--------------
 include/drm/drm_crtc.h              |  5 -----
 2 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d19563651e07..ccf0bed9bf4a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1110,19 +1110,20 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	int i, ret;
+	unsigned crtc_mask = 0;
+	unsigned last_vblank_count[dev->mode_config.num_crtc];
+
+	 /*
+	  * Legacy cursor ioctls are completely unsynced, and userspace
+	  * relies on that (by doing tons of cursor updates).
+	  */
+	if (old_state->legacy_cursor_update)
+		return;
 
 	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-		/* No one cares about the old state, so abuse it for tracking
-		 * and store whether we hold a vblank reference (and should do a
-		 * vblank wait) in the ->enable boolean. */
-		old_crtc_state->enable = false;
-
-		if (!crtc->state->active)
-			continue;
+		struct drm_crtc_state *new_crtc_state = crtc->state;
 
-		/* Legacy cursor ioctls are completely unsynced, and userspace
-		 * relies on that (by doing tons of cursor updates). */
-		if (old_state->legacy_cursor_update)
+		if (!new_crtc_state->active)
 			continue;
 
 		if (!drm_atomic_helper_framebuffer_changed(dev,
@@ -1133,16 +1134,16 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 		if (ret != 0)
 			continue;
 
-		old_crtc_state->enable = true;
-		old_crtc_state->last_vblank_count = drm_crtc_vblank_count(crtc);
+		crtc_mask |= drm_crtc_mask(crtc);
+		last_vblank_count[i] = drm_crtc_vblank_count(crtc);
 	}
 
 	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-		if (!old_crtc_state->enable)
+		if (!(crtc_mask & drm_crtc_mask(crtc)))
 			continue;
 
 		ret = wait_event_timeout(dev->vblank[i].queue,
-				old_crtc_state->last_vblank_count !=
+				last_vblank_count[i] !=
 					drm_crtc_vblank_count(crtc),
 				msecs_to_jiffies(50));
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 946672f97e1e..e03d194be086 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -93,8 +93,6 @@ struct drm_plane_helper_funcs;
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
  * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
  * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
- * @last_vblank_count: for helpers and drivers to capture the vblank of the
- * 	update to ensure framebuffer cleanup isn't done too early
  * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
  * @mode: current mode timings
  * @mode_blob: &drm_property_blob for @mode
@@ -140,9 +138,6 @@ struct drm_crtc_state {
 	u32 connector_mask;
 	u32 encoder_mask;
 
-	/* last_vblank_count: for vblank waits before cleanup */
-	u32 last_vblank_count;
-
 	/* adjusted_mode: for use by helpers and drivers */
 	struct drm_display_mode adjusted_mode;
 
-- 
2.7.4

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

[PATCH 4/6] drm/atomic: Wait for vblank whenever a plane is added to state.

[Maarten Lankhorst]

The current api doesn't take into account that whenever properties like
rotation or z-pos change we have to wait for vblank. To make sure
that we wait correctly, err on the side of caution and wait whenever
a plane is added to state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ccf0bed9bf4a..fdeaea84a3b7 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1123,11 +1123,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
 		struct drm_crtc_state *new_crtc_state = crtc->state;
 
-		if (!new_crtc_state->active)
-			continue;
-
-		if (!drm_atomic_helper_framebuffer_changed(dev,
-				old_state, crtc))
+		if (!new_crtc_state->active || !new_crtc_state->planes_changed)
 			continue;
 
 		ret = drm_crtc_vblank_get(crtc);
-- 
2.7.4

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

[PATCH 5/6] drm/atomic: Remove drm_atomic_helper_framebuffer_changed.

[Maarten Lankhorst]

This function is now completely unused, zap it.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 35 -----------------------------------
 include/drm/drm_atomic_helper.h     |  3 ---
 2 files changed, 38 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fdeaea84a3b7..69ee1a94e851 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1058,41 +1058,6 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_fences);
 
 /**
- * drm_atomic_helper_framebuffer_changed - check if framebuffer has changed
- * @dev: DRM device
- * @old_state: atomic state object with old state structures
- * @crtc: DRM crtc
- *
- * Checks whether the framebuffer used for this CRTC changes as a result of
- * the atomic update.  This is useful for drivers which cannot use
- * drm_atomic_helper_wait_for_vblanks() and need to reimplement its
- * functionality.
- *
- * Returns:
- * true if the framebuffer changed.
- */
-bool drm_atomic_helper_framebuffer_changed(struct drm_device *dev,
-					   struct drm_atomic_state *old_state,
-					   struct drm_crtc *crtc)
-{
-	struct drm_plane *plane;
-	struct drm_plane_state *old_plane_state;
-	int i;
-
-	for_each_plane_in_state(old_state, plane, old_plane_state, i) {
-		if (plane->state->crtc != crtc &&
-		    old_plane_state->crtc != crtc)
-			continue;
-
-		if (plane->state->fb != old_plane_state->fb)
-			return true;
-	}
-
-	return false;
-}
-EXPORT_SYMBOL(drm_atomic_helper_framebuffer_changed);
-
-/**
  * drm_atomic_helper_wait_for_vblanks - wait for vblank on crtcs
  * @dev: DRM device
  * @old_state: atomic state object with old state structures
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 7ff92b09fd9c..4b2353dc34ba 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -48,9 +48,6 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 					struct drm_atomic_state *state,
 					bool pre_swap);
-bool drm_atomic_helper_framebuffer_changed(struct drm_device *dev,
-					   struct drm_atomic_state *old_state,
-					   struct drm_crtc *crtc);
 
 void drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 					struct drm_atomic_state *old_state);
-- 
2.7.4

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

[PATCH 6/6] drm/i915: Add a cursor hack to allow converting legacy page flip to atomic.

[Maarten Lankhorst]

Do something similar to vc4, only allow updating the cursor state
in-place through a fastpath when the watermarks are unaffected. This
will allow cursor movement to be smooth, but changing cursor size or
showing/hiding cursor will still fall back so watermarks can be updated.

Only moving and changing fb is allowed.

Changes since v1:
- Set page flip to always_unused for trybot.
- Copy fence correctly, ignore plane_state->state, should be NULL.
- Check crtc_state for !active and modeset, go to slowpath if the case.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  47 +++++++-----
 drivers/gpu/drm/i915/intel_display.c      | 123 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h          |   2 +
 3 files changed, 153 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index dbe9fb41ae53..60d75ce8a989 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane,
 	drm_atomic_helper_plane_destroy_state(plane, state);
 }
 
-static int intel_plane_atomic_check(struct drm_plane *plane,
-				    struct drm_plane_state *state)
+int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
+					struct intel_plane_state *intel_state)
 {
+	struct drm_plane *plane = intel_state->base.plane;
 	struct drm_i915_private *dev_priv = to_i915(plane->dev);
-	struct drm_crtc *crtc = state->crtc;
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *crtc_state;
+	struct drm_plane_state *state = &intel_state->base;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct intel_plane_state *intel_state = to_intel_plane_state(state);
-	struct drm_crtc_state *drm_crtc_state;
 	int ret;
 
-	crtc = crtc ? crtc : plane->state->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-
 	/*
 	 * Both crtc and plane->crtc could be NULL if we're updating a
 	 * property while the plane is disabled.  We don't actually have
 	 * anything driver-specific we need to test in that case, so
 	 * just return success.
 	 */
-	if (!crtc)
+	if (!intel_state->base.crtc && !plane->state->crtc)
 		return 0;
 
-	drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
-	if (WARN_ON(!drm_crtc_state))
-		return -EINVAL;
-
-	crtc_state = to_intel_crtc_state(drm_crtc_state);
-
 	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
 	intel_state->clip.x1 = 0;
 	intel_state->clip.y1 = 0;
@@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	return intel_plane_atomic_calc_changes(&crtc_state->base, state);
 }
 
+static int intel_plane_atomic_check(struct drm_plane *plane,
+				    struct drm_plane_state *state)
+{
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_crtc_state *drm_crtc_state;
+
+	crtc = crtc ? crtc : plane->state->crtc;
+
+	/*
+	 * Both crtc and plane->crtc could be NULL if we're updating a
+	 * property while the plane is disabled.  We don't actually have
+	 * anything driver-specific we need to test in that case, so
+	 * just return success.
+	 */
+	if (!crtc)
+		return 0;
+
+	drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
+	if (WARN_ON(!drm_crtc_state))
+		return -EINVAL;
+
+	return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),
+						   to_intel_plane_state(state));
+}
+
 static void intel_plane_atomic_update(struct drm_plane *plane,
 				      struct drm_plane_state *old_state)
 {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9eaf1e5bdae9..a8b8663ecda7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15028,6 +15029,124 @@ const struct drm_plane_funcs intel_plane_funcs = {
 	.atomic_destroy_state = intel_plane_destroy_state,
 };
 
+static int
+intel_legacy_cursor_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 drm_i915_private *dev_priv = to_i915(crtc->dev);
+	int ret;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_framebuffer *old_fb;
+	struct drm_crtc_state *crtc_state = crtc->state;
+
+	if (!crtc_state->active || needs_modeset(crtc_state) ||
+	    to_intel_crtc_state(crtc_state)->update_pipe)
+		goto slow;
+
+	old_plane_state = plane->state;
+
+	if (old_plane_state->crtc != crtc ||
+	    old_plane_state->src_w != src_w ||
+	    old_plane_state->src_h != src_h ||
+	    old_plane_state->crtc_w != crtc_w ||
+	    old_plane_state->crtc_h != crtc_h ||
+	    !old_plane_state->visible ||
+	    old_plane_state->fb->modifier != fb->modifier)
+		goto slow;
+
+	new_plane_state = intel_plane_duplicate_state(plane);
+	if (!new_plane_state)
+		return -ENOMEM;
+
+	drm_atomic_set_fb_for_plane(new_plane_state, fb);
+
+	new_plane_state->src_x = src_x;
+	new_plane_state->src_y = src_y;
+	new_plane_state->src_w = src_w;
+	new_plane_state->src_h = src_h;
+	new_plane_state->crtc_x = crtc_x;
+	new_plane_state->crtc_y = crtc_y;
+	new_plane_state->crtc_w = crtc_w;
+	new_plane_state->crtc_h = crtc_h;
+
+	ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
+						  to_intel_plane_state(new_plane_state));
+	if (ret)
+		return ret;
+
+	if (!new_plane_state->visible) {
+		intel_plane_destroy_state(plane, new_plane_state);
+		goto slow;
+	}
+
+	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
+	if (ret)
+		return ret;
+
+	if (INTEL_INFO(dev_priv)->cursor_needs_physical) {
+		int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
+
+		ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+			return ret;
+		}
+	} else {
+		struct i915_vma *vma;
+
+		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
+		if (IS_ERR(vma)) {
+			DRM_DEBUG_KMS("failed to pin object\n");
+
+			return PTR_ERR(vma);
+		}
+	}
+
+	old_fb = old_plane_state->fb;
+
+	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
+			  intel_plane->frontbuffer_bit);
+
+	/* Swap plane state */
+	new_plane_state->fence = old_plane_state->fence;
+	*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
+	new_plane_state->fence = NULL;
+	new_plane_state->fb = old_fb;
+
+	intel_plane->update_plane(plane,
+				  to_intel_crtc_state(crtc->state),
+				  to_intel_plane_state(plane->state));
+
+	intel_cleanup_plane_fb(plane, new_plane_state);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+
+	intel_plane_destroy_state(plane, new_plane_state);
+
+	return 0;
+
+slow:
+	return drm_atomic_helper_update_plane(plane, crtc, fb,
+					      crtc_x, crtc_y, crtc_w, crtc_h,
+					      src_x, src_y, src_w, src_h);
+}
+
+static const struct drm_plane_funcs intel_cursor_plane_funcs = {
+	.update_plane = intel_legacy_cursor_update,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = intel_plane_destroy,
+	.set_property = drm_atomic_helper_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,
+};
+
 static struct intel_plane *
 intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
@@ -15274,7 +15393,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 	cursor->disable_plane = intel_disable_cursor_plane;
 
 	ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base,
-				       0, &intel_plane_funcs,
+				       0, &intel_cursor_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 8f4ddca0f521..8b6f39506d45 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1844,6 +1844,8 @@ 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);
 extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
+					struct intel_plane_state *intel_state);
 
 /* intel_color.c */
 void intel_color_init(struct drm_crtc *crtc);
-- 
2.7.4

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

Re: [PATCH 6/6] drm/i915: Add a cursor hack to allow converting legacy page flip to atomic.

[Matthew Auld]

On 8 December 2016 at 13:45, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Do something similar to vc4, only allow updating the cursor state
> in-place through a fastpath when the watermarks are unaffected. This
> will allow cursor movement to be smooth, but changing cursor size or
> showing/hiding cursor will still fall back so watermarks can be updated.
>
> Only moving and changing fb is allowed.
>
> Changes since v1:
> - Set page flip to always_unused for trybot.
> - Copy fence correctly, ignore plane_state->state, should be NULL.
> - Check crtc_state for !active and modeset, go to slowpath if the case.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  47 +++++++-----
>  drivers/gpu/drm/i915/intel_display.c      | 123 +++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h          |   2 +
>  3 files changed, 153 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index dbe9fb41ae53..60d75ce8a989 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane,
>         drm_atomic_helper_plane_destroy_state(plane, state);
>  }
>
> -static int intel_plane_atomic_check(struct drm_plane *plane,
> -                                   struct drm_plane_state *state)
> +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
> +                                       struct intel_plane_state *intel_state)
>  {
> +       struct drm_plane *plane = intel_state->base.plane;
>         struct drm_i915_private *dev_priv = to_i915(plane->dev);
> -       struct drm_crtc *crtc = state->crtc;
> -       struct intel_crtc *intel_crtc;
> -       struct intel_crtc_state *crtc_state;
> +       struct drm_plane_state *state = &intel_state->base;
>         struct intel_plane *intel_plane = to_intel_plane(plane);
> -       struct intel_plane_state *intel_state = to_intel_plane_state(state);
> -       struct drm_crtc_state *drm_crtc_state;
>         int ret;
>
> -       crtc = crtc ? crtc : plane->state->crtc;
> -       intel_crtc = to_intel_crtc(crtc);
> -
>         /*
>          * Both crtc and plane->crtc could be NULL if we're updating a
>          * property while the plane is disabled.  We don't actually have
>          * anything driver-specific we need to test in that case, so
>          * just return success.
>          */
> -       if (!crtc)
> +       if (!intel_state->base.crtc && !plane->state->crtc)
>                 return 0;
>
> -       drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
> -       if (WARN_ON(!drm_crtc_state))
> -               return -EINVAL;
> -
> -       crtc_state = to_intel_crtc_state(drm_crtc_state);
> -
>         /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>         intel_state->clip.x1 = 0;
>         intel_state->clip.y1 = 0;
> @@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>         return intel_plane_atomic_calc_changes(&crtc_state->base, state);
>  }
>
> +static int intel_plane_atomic_check(struct drm_plane *plane,
> +                                   struct drm_plane_state *state)
> +{
> +       struct drm_crtc *crtc = state->crtc;
> +       struct drm_crtc_state *drm_crtc_state;
> +
> +       crtc = crtc ? crtc : plane->state->crtc;
> +
> +       /*
> +        * Both crtc and plane->crtc could be NULL if we're updating a
> +        * property while the plane is disabled.  We don't actually have
> +        * anything driver-specific we need to test in that case, so
> +        * just return success.
> +        */
> +       if (!crtc)
> +               return 0;
> +
> +       drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
> +       if (WARN_ON(!drm_crtc_state))
> +               return -EINVAL;
> +
> +       return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),
> +                                                  to_intel_plane_state(state));
> +}
> +
>  static void intel_plane_atomic_update(struct drm_plane *plane,
>                                       struct drm_plane_state *old_state)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9eaf1e5bdae9..a8b8663ecda7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15028,6 +15029,124 @@ const struct drm_plane_funcs intel_plane_funcs = {
>         .atomic_destroy_state = intel_plane_destroy_state,
>  };
>
> +static int
> +intel_legacy_cursor_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 drm_i915_private *dev_priv = to_i915(crtc->dev);
> +       int ret;
> +       struct drm_plane_state *old_plane_state, *new_plane_state;
> +       struct intel_plane *intel_plane = to_intel_plane(plane);
> +       struct drm_framebuffer *old_fb;
> +       struct drm_crtc_state *crtc_state = crtc->state;
> +
> +       if (!crtc_state->active || needs_modeset(crtc_state) ||
> +           to_intel_crtc_state(crtc_state)->update_pipe)
> +               goto slow;
> +
> +       old_plane_state = plane->state;
> +
> +       if (old_plane_state->crtc != crtc ||
> +           old_plane_state->src_w != src_w ||
> +           old_plane_state->src_h != src_h ||
> +           old_plane_state->crtc_w != crtc_w ||
> +           old_plane_state->crtc_h != crtc_h ||
> +           !old_plane_state->visible ||
> +           old_plane_state->fb->modifier != fb->modifier)
> +               goto slow;
> +
> +       new_plane_state = intel_plane_duplicate_state(plane);
> +       if (!new_plane_state)
> +               return -ENOMEM;
> +
> +       drm_atomic_set_fb_for_plane(new_plane_state, fb);
> +
> +       new_plane_state->src_x = src_x;
> +       new_plane_state->src_y = src_y;
> +       new_plane_state->src_w = src_w;
> +       new_plane_state->src_h = src_h;
> +       new_plane_state->crtc_x = crtc_x;
> +       new_plane_state->crtc_y = crtc_y;
> +       new_plane_state->crtc_w = crtc_w;
> +       new_plane_state->crtc_h = crtc_h;
> +
> +       ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
> +                                                 to_intel_plane_state(new_plane_state));
> +       if (ret)
> +               return ret;
> +
> +       if (!new_plane_state->visible) {
> +               intel_plane_destroy_state(plane, new_plane_state);
> +               goto slow;
> +       }
> +
> +       ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> +       if (ret)
> +               return ret;
> +
> +       if (INTEL_INFO(dev_priv)->cursor_needs_physical) {
> +               int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
> +
> +               ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align);
> +               if (ret) {
> +                       DRM_DEBUG_KMS("failed to attach phys object\n");
> +                       return ret;
Drive-by-comment. Shouldn't there be some clean up in both error
paths, like mutex_unlock ?

> +               }
> +       } else {
> +               struct i915_vma *vma;
> +
> +               vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
> +               if (IS_ERR(vma)) {
> +                       DRM_DEBUG_KMS("failed to pin object\n");
> +
> +                       return PTR_ERR(vma);
> +               }
> +       }
> +
> +       old_fb = old_plane_state->fb;
> +
> +       i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
> +                         intel_plane->frontbuffer_bit);
> +
> +       /* Swap plane state */
> +       new_plane_state->fence = old_plane_state->fence;
> +       *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
> +       new_plane_state->fence = NULL;
> +       new_plane_state->fb = old_fb;
> +
> +       intel_plane->update_plane(plane,
> +                                 to_intel_crtc_state(crtc->state),
> +                                 to_intel_plane_state(plane->state));
> +
> +       intel_cleanup_plane_fb(plane, new_plane_state);
> +       mutex_unlock(&dev_priv->drm.struct_mutex);
> +
> +       intel_plane_destroy_state(plane, new_plane_state);
> +
> +       return 0;
> +
> +slow:
> +       return drm_atomic_helper_update_plane(plane, crtc, fb,
> +                                             crtc_x, crtc_y, crtc_w, crtc_h,
> +                                             src_x, src_y, src_w, src_h);
> +}
> +
> +static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> +       .update_plane = intel_legacy_cursor_update,
> +       .disable_plane = drm_atomic_helper_disable_plane,
> +       .destroy = intel_plane_destroy,
> +       .set_property = drm_atomic_helper_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,
> +};
> +
>  static struct intel_plane *
>  intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  {
> @@ -15274,7 +15393,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>         cursor->disable_plane = intel_disable_cursor_plane;
>
>         ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base,
> -                                      0, &intel_plane_funcs,
> +                                      0, &intel_cursor_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 8f4ddca0f521..8b6f39506d45 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1844,6 +1844,8 @@ 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);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
> +                                       struct intel_plane_state *intel_state);
>
>  /* intel_color.c */
>  void intel_color_init(struct drm_crtc *crtc);
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/atomic: Use active instead of enable in wait_for_vblanks.

[Daniel Vetter]

On Thu, Dec 08, 2016 at 02:45:24PM +0100, Maarten Lankhorst wrote:
> When DPMS was introduced to atomic, vblanks only worked when the crtc
> was enabled and active. wait_for_vblanks were not converted to check for
> crtc_state->active, which may cause an attempt for vblank_get to fail.
> 
> This is probably harmless, but convert from enable to active anyway.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 583f47f27b36..23767df72615 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1117,7 +1117,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
>  		 * vblank wait) in the ->enable boolean. */
>  		old_crtc_state->enable = false;
>  
> -		if (!crtc->state->enable)
> +		if (!crtc->state->active)

Hm, with this change we /should/ be able to make the drm_crtc_vblank_get
call a few lines below WARN when it fails. Not doing that was only done at
first because there was no accurate active state tracking for the CRTC,
and since the vblank state should match active I've used that. Care for
that follow-up patch?

Applied this one here meanwhile.
-Daniel
>  			continue;
>  
>  		/* Legacy cursor ioctls are completely unsynced, and userspace
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/6] drm/atomic: Clean up wait_for_vblanks

[Daniel Vetter]

On Thu, Dec 08, 2016 at 02:45:26PM +0100, Maarten Lankhorst wrote:
> Stop relying on a per crtc_state last_vblank_count, we know in advance
> how many there can be. Also stop re-using new_crtc_state->enable,
> we can now simply set a bitmask with crtc_crtc_mask.
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 29 +++++++++++++++--------------
>  include/drm/drm_crtc.h              |  5 -----
>  2 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d19563651e07..ccf0bed9bf4a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1110,19 +1110,20 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state;
>  	int i, ret;
> +	unsigned crtc_mask = 0;
> +	unsigned last_vblank_count[dev->mode_config.num_crtc];

I think putting last_vblank_count into
drm_atomic_state->crtcs.last_vblank_count would be nice(r). At least I
always freak out when we have dynamically sized arrays on the stack ;-)

> +
> +	 /*
> +	  * Legacy cursor ioctls are completely unsynced, and userspace
> +	  * relies on that (by doing tons of cursor updates).
> +	  */
> +	if (old_state->legacy_cursor_update)
> +		return;
>  
>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> -		/* No one cares about the old state, so abuse it for tracking
> -		 * and store whether we hold a vblank reference (and should do a
> -		 * vblank wait) in the ->enable boolean. */
> -		old_crtc_state->enable = false;
> -
> -		if (!crtc->state->active)
> -			continue;
> +		struct drm_crtc_state *new_crtc_state = crtc->state;
>  
> -		/* Legacy cursor ioctls are completely unsynced, and userspace
> -		 * relies on that (by doing tons of cursor updates). */
> -		if (old_state->legacy_cursor_update)
> +		if (!new_crtc_state->active)
>  			continue;
>  
>  		if (!drm_atomic_helper_framebuffer_changed(dev,
> @@ -1133,16 +1134,16 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
>  		if (ret != 0)
>  			continue;

		if (WARN_ON(ret != 0))
			continue;

... is the WARN_ON I meant in patch 1. Would be good to do that too while
we're at it.
-Daniel

>  
> -		old_crtc_state->enable = true;
> -		old_crtc_state->last_vblank_count = drm_crtc_vblank_count(crtc);
> +		crtc_mask |= drm_crtc_mask(crtc);
> +		last_vblank_count[i] = drm_crtc_vblank_count(crtc);
>  	}
>  
>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> -		if (!old_crtc_state->enable)
> +		if (!(crtc_mask & drm_crtc_mask(crtc)))
>  			continue;
>  
>  		ret = wait_event_timeout(dev->vblank[i].queue,
> -				old_crtc_state->last_vblank_count !=
> +				last_vblank_count[i] !=
>  					drm_crtc_vblank_count(crtc),
>  				msecs_to_jiffies(50));
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 946672f97e1e..e03d194be086 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -93,8 +93,6 @@ struct drm_plane_helper_funcs;
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
>   * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
> - * @last_vblank_count: for helpers and drivers to capture the vblank of the
> - * 	update to ensure framebuffer cleanup isn't done too early
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
>   * @mode: current mode timings
>   * @mode_blob: &drm_property_blob for @mode
> @@ -140,9 +138,6 @@ struct drm_crtc_state {
>  	u32 connector_mask;
>  	u32 encoder_mask;
>  
> -	/* last_vblank_count: for vblank waits before cleanup */
> -	u32 last_vblank_count;
> -
>  	/* adjusted_mode: for use by helpers and drivers */
>  	struct drm_display_mode adjusted_mode;
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 2/6] drm/atomic: Unconditionally call prepare_fb.

[Daniel Vetter]

On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote:
> Atomic drivers may set properties like rotation on the same fb, which
> may require a call to prepare_fb even when framebuffer stays identical.
> 
> Instead of handling all the special cases in the core, let the driver
> decide when prepare_fb and cleanup_fb are noops.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I think this makes sense, but would be really good to get a pile of acks
from driver maintainers on this one. Rob, Eric, Laurent, others?
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 23767df72615..d19563651e07 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>  
>  		funcs = plane->helper_private;
>  
> -		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
> -			continue;
> -
>  		if (funcs->prepare_fb) {
>  			ret = funcs->prepare_fb(plane, plane_state);
>  			if (ret)
> @@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>  		if (j >= i)
>  			continue;
>  
> -		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
> -			continue;
> -
>  		funcs = plane->helper_private;
>  
>  		if (funcs->cleanup_fb)
> @@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
>  	for_each_plane_in_state(old_state, plane, plane_state, i) {
>  		const struct drm_plane_helper_funcs *funcs;
>  
> -		if (!drm_atomic_helper_framebuffer_changed(dev, old_state, plane_state->crtc))
> -			continue;
> -
>  		funcs = plane->helper_private;
>  
>  		if (funcs->cleanup_fb)
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/6] drm/atomic: Wait for vblank whenever a plane is added to state.

[Daniel Vetter]

On Thu, Dec 08, 2016 at 02:45:27PM +0100, Maarten Lankhorst wrote:
> The current api doesn't take into account that whenever properties like
> rotation or z-pos change we have to wait for vblank. To make sure
> that we wait correctly, err on the side of caution and wait whenever
> a plane is added to state.

Why do we need to wait when rotation or zpos has changed? I'm a bit lost
... So rotation makes sense since often that means a special, rotated
mapping (or a different offset on omap's TILER as a different example).
But z-pos I have no idad why that matters.
-Daniel

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ccf0bed9bf4a..fdeaea84a3b7 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1123,11 +1123,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
>  		struct drm_crtc_state *new_crtc_state = crtc->state;
>  
> -		if (!new_crtc_state->active)
> -			continue;
> -
> -		if (!drm_atomic_helper_framebuffer_changed(dev,
> -				old_state, crtc))
> +		if (!new_crtc_state->active || !new_crtc_state->planes_changed)
>  			continue;
>  
>  		ret = drm_crtc_vblank_get(crtc);
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

✓ Fi.CI.BAT: success for drm/atomic: Remove drm_atomic_helper_framebuffer_changed

[Patchwork]

== Series Details ==

Series: drm/atomic: Remove drm_atomic_helper_framebuffer_changed
URL   : https://patchwork.freedesktop.org/series/16561/
State : success

== Summary ==

Series 16561v1 drm/atomic: Remove drm_atomic_helper_framebuffer_changed
https://patchwork.freedesktop.org/api/1.0/series/16561/revisions/1/mbox/


fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-t5700     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

24cc1f39920c0caf747c6bda267ca19b99f21786 drm-tip: 2016y-12m-08d-12h-31m-59s UTC integration manifest
d8037d4 drm/i915: Add a cursor hack to allow converting legacy page flip to atomic.
b237f24 drm/atomic: Remove drm_atomic_helper_framebuffer_changed.
11c7d13 drm/atomic: Wait for vblank whenever a plane is added to state.
c82f2eb drm/atomic: Clean up wait_for_vblanks
6507f92 drm/atomic: Unconditionally call prepare_fb.
212a206 drm/atomic: Use active instead of enable in wait_for_vblanks.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3239/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/atomic: Unconditionally call prepare_fb.

[Laurent Pinchart]

Hi Daniel,

On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote:
> On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote:
> > Atomic drivers may set properties like rotation on the same fb, which
> > may require a call to prepare_fb even when framebuffer stays identical.
> > 
> > Instead of handling all the special cases in the core, let the driver
> > decide when prepare_fb and cleanup_fb are noops.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> I think this makes sense, but would be really good to get a pile of acks
> from driver maintainers on this one. Rob, Eric, Laurent, others?

This is all very nice, but it will introduce at least a performance 
regression, and possibly worse, until drivers get updated. There are 7 drivers 
implementing the .prepare_fb() callback (plus a bunch of drivers that probably 
should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this 
patch before they get fixed.

> > ---
> > 
> >  drivers/gpu/drm/drm_atomic_helper.c | 9 ---------
> >  1 file changed, 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c index 23767df72615..d19563651e07
> > 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct
> > drm_device *dev,> 
> >  		funcs = plane->helper_private;
> > 
> > -		if (!drm_atomic_helper_framebuffer_changed(dev, state,
> > plane_state->crtc)) -			continue;
> > -
> > 
> >  		if (funcs->prepare_fb) {
> >  		
> >  			ret = funcs->prepare_fb(plane, plane_state);
> >  			if (ret)
> > 
> > @@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct
> > drm_device *dev,> 
> >  		if (j >= i)
> >  		
> >  			continue;
> > 
> > -		if (!drm_atomic_helper_framebuffer_changed(dev, state,
> > plane_state->crtc)) -			continue;
> > -
> > 
> >  		funcs = plane->helper_private;
> >  		
> >  		if (funcs->cleanup_fb)
> > 
> > @@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct
> > drm_device *dev,> 
> >  	for_each_plane_in_state(old_state, plane, plane_state, i) {
> >  	
> >  		const struct drm_plane_helper_funcs *funcs;
> > 
> > -		if (!drm_atomic_helper_framebuffer_changed(dev, old_state,
> > plane_state->crtc)) -			continue;
> > -
> > 
> >  		funcs = plane->helper_private;
> >  		
> >  		if (funcs->cleanup_fb)

-- 
Regards,

Laurent Pinchart

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

Re: [Intel-gfx] [PATCH 2/6] drm/atomic: Unconditionally call prepare_fb.

[Daniel Vetter]

On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote:
> > On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote:
> > > Atomic drivers may set properties like rotation on the same fb, which
> > > may require a call to prepare_fb even when framebuffer stays identical.
> > > 
> > > Instead of handling all the special cases in the core, let the driver
> > > decide when prepare_fb and cleanup_fb are noops.
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> > I think this makes sense, but would be really good to get a pile of acks
> > from driver maintainers on this one. Rob, Eric, Laurent, others?
> 
> This is all very nice, but it will introduce at least a performance 
> regression, and possibly worse, until drivers get updated. There are 7 drivers 
> implementing the .prepare_fb() callback (plus a bunch of drivers that probably 
> should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this 
> patch before they get fixed.

Maarten's commit message is insufficient, since this is defacto a revert
of

commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
Author: Keith Packard <keithp@keithp.com>
Date:   Sat Jun 4 01:16:22 2016 -0700

    drm: Don't prepare or cleanup unchanging frame buffers [v3]

because that breaks stuff. We're simply going back to where we've been a
few months ago. Since this is a regression fix, back to original
behaviour, can you ack (assuming Maarten updates the commit message to
reflect the nature of the commit here)?
-Daniel

> 
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_atomic_helper.c | 9 ---------
> > >  1 file changed, 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c index 23767df72615..d19563651e07
> > > 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct
> > > drm_device *dev,> 
> > >  		funcs = plane->helper_private;
> > > 
> > > -		if (!drm_atomic_helper_framebuffer_changed(dev, state,
> > > plane_state->crtc)) -			continue;
> > > -
> > > 
> > >  		if (funcs->prepare_fb) {
> > >  		
> > >  			ret = funcs->prepare_fb(plane, plane_state);
> > >  			if (ret)
> > > 
> > > @@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct
> > > drm_device *dev,> 
> > >  		if (j >= i)
> > >  		
> > >  			continue;
> > > 
> > > -		if (!drm_atomic_helper_framebuffer_changed(dev, state,
> > > plane_state->crtc)) -			continue;
> > > -
> > > 
> > >  		funcs = plane->helper_private;
> > >  		
> > >  		if (funcs->cleanup_fb)
> > > 
> > > @@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct
> > > drm_device *dev,> 
> > >  	for_each_plane_in_state(old_state, plane, plane_state, i) {
> > >  	
> > >  		const struct drm_plane_helper_funcs *funcs;
> > > 
> > > -		if (!drm_atomic_helper_framebuffer_changed(dev, old_state,
> > > plane_state->crtc)) -			continue;
> > > -
> > > 
> > >  		funcs = plane->helper_private;
> > >  		
> > >  		if (funcs->cleanup_fb)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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

Re: [PATCH 4/6] drm/atomic: Wait for vblank whenever a plane is added to state.

[Maarten Lankhorst]

Op 08-12-16 om 16:43 schreef Daniel Vetter:
> On Thu, Dec 08, 2016 at 02:45:27PM +0100, Maarten Lankhorst wrote:
>> The current api doesn't take into account that whenever properties like
>> rotation or z-pos change we have to wait for vblank. To make sure
>> that we wait correctly, err on the side of caution and wait whenever
>> a plane is added to state.
> Why do we need to wait when rotation or zpos has changed? I'm a bit lost
> ... So rotation makes sense since often that means a special, rotated
> mapping (or a different offset on omap's TILER as a different example).
> But z-pos I have no idad why that matters.
It was meant as example, blocking commit always waits for the changes to be visible before returning, from what I understand.
So if any plane property changes we have to wait for the changes to be visible first.

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/6] drm/i915: Add a cursor hack to allow converting legacy page flip to atomic, v3.

[Maarten Lankhorst]

Do something similar to vc4, only allow updating the cursor state
in-place through a fastpath when the watermarks are unaffected. This
will allow cursor movement to be smooth, but changing cursor size or
showing/hiding cursor will still fall back so watermarks can be updated.

Only moving and changing fb is allowed.

Changes since v1:
- Set page flip to always_unused for trybot.
- Copy fence correctly, ignore plane_state->state, should be NULL.
- Check crtc_state for !active and modeset, go to slowpath if the case.
Changes since v2:
- Make error handling work correctly. (Matthew Auld)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  47 +++++++----
 drivers/gpu/drm/i915/intel_display.c      | 132 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h          |   2 +
 3 files changed, 163 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index dbe9fb41ae53..60d75ce8a989 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane,
 	drm_atomic_helper_plane_destroy_state(plane, state);
 }
 
-static int intel_plane_atomic_check(struct drm_plane *plane,
-				    struct drm_plane_state *state)
+int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
+					struct intel_plane_state *intel_state)
 {
+	struct drm_plane *plane = intel_state->base.plane;
 	struct drm_i915_private *dev_priv = to_i915(plane->dev);
-	struct drm_crtc *crtc = state->crtc;
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *crtc_state;
+	struct drm_plane_state *state = &intel_state->base;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct intel_plane_state *intel_state = to_intel_plane_state(state);
-	struct drm_crtc_state *drm_crtc_state;
 	int ret;
 
-	crtc = crtc ? crtc : plane->state->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-
 	/*
 	 * Both crtc and plane->crtc could be NULL if we're updating a
 	 * property while the plane is disabled.  We don't actually have
 	 * anything driver-specific we need to test in that case, so
 	 * just return success.
 	 */
-	if (!crtc)
+	if (!intel_state->base.crtc && !plane->state->crtc)
 		return 0;
 
-	drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
-	if (WARN_ON(!drm_crtc_state))
-		return -EINVAL;
-
-	crtc_state = to_intel_crtc_state(drm_crtc_state);
-
 	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
 	intel_state->clip.x1 = 0;
 	intel_state->clip.y1 = 0;
@@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	return intel_plane_atomic_calc_changes(&crtc_state->base, state);
 }
 
+static int intel_plane_atomic_check(struct drm_plane *plane,
+				    struct drm_plane_state *state)
+{
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_crtc_state *drm_crtc_state;
+
+	crtc = crtc ? crtc : plane->state->crtc;
+
+	/*
+	 * Both crtc and plane->crtc could be NULL if we're updating a
+	 * property while the plane is disabled.  We don't actually have
+	 * anything driver-specific we need to test in that case, so
+	 * just return success.
+	 */
+	if (!crtc)
+		return 0;
+
+	drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
+	if (WARN_ON(!drm_crtc_state))
+		return -EINVAL;
+
+	return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),
+						   to_intel_plane_state(state));
+}
+
 static void intel_plane_atomic_update(struct drm_plane *plane,
 				      struct drm_plane_state *old_state)
 {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9eaf1e5bdae9..5568ecac2edc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = {
 	.atomic_destroy_state = intel_plane_destroy_state,
 };
 
+static int
+intel_legacy_cursor_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 drm_i915_private *dev_priv = to_i915(crtc->dev);
+	int ret;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_framebuffer *old_fb;
+	struct drm_crtc_state *crtc_state = crtc->state;
+
+	/*
+	 * When crtc is inactive or there is a modeset pending,
+	 * wait for it to complete in the slowpath
+	 */
+	if (!crtc_state->active || needs_modeset(crtc_state) ||
+	    to_intel_crtc_state(crtc_state)->update_pipe)
+		goto slow;
+
+	old_plane_state = plane->state;
+
+	/*
+	 * If any parameters change that may affect watermarks,
+	 * take the slowpath. Only changing fb or position should be
+	 * in the fastpath.
+	 */
+	if (old_plane_state->crtc != crtc ||
+	    old_plane_state->src_w != src_w ||
+	    old_plane_state->src_h != src_h ||
+	    old_plane_state->crtc_w != crtc_w ||
+	    old_plane_state->crtc_h != crtc_h ||
+	    !old_plane_state->visible ||
+	    old_plane_state->fb->modifier != fb->modifier)
+		goto slow;
+
+	new_plane_state = intel_plane_duplicate_state(plane);
+	if (!new_plane_state)
+		return -ENOMEM;
+
+	drm_atomic_set_fb_for_plane(new_plane_state, fb);
+
+	new_plane_state->src_x = src_x;
+	new_plane_state->src_y = src_y;
+	new_plane_state->src_w = src_w;
+	new_plane_state->src_h = src_h;
+	new_plane_state->crtc_x = crtc_x;
+	new_plane_state->crtc_y = crtc_y;
+	new_plane_state->crtc_w = crtc_w;
+	new_plane_state->crtc_h = crtc_h;
+
+	ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
+						  to_intel_plane_state(new_plane_state));
+	if (ret)
+		goto out_free;
+
+	/* Visibility changed, must take slowpath. */
+	if (!new_plane_state->visible)
+		goto slow_free;
+
+	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
+	if (ret)
+		goto out_free;
+
+	if (INTEL_INFO(dev_priv)->cursor_needs_physical) {
+		int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
+
+		ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+			goto out_unlock;
+		}
+	} else {
+		struct i915_vma *vma;
+
+		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
+		if (IS_ERR(vma)) {
+			DRM_DEBUG_KMS("failed to pin object\n");
+
+			ret = PTR_ERR(vma);
+			goto out_unlock;
+		}
+	}
+
+	old_fb = old_plane_state->fb;
+
+	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
+			  intel_plane->frontbuffer_bit);
+
+	/* Swap plane state */
+	new_plane_state->fence = old_plane_state->fence;
+	*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
+	new_plane_state->fence = NULL;
+	new_plane_state->fb = old_fb;
+
+	intel_plane->update_plane(plane,
+				  to_intel_crtc_state(crtc->state),
+				  to_intel_plane_state(plane->state));
+
+	intel_cleanup_plane_fb(plane, new_plane_state);
+
+out_unlock:
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+out_free:
+	intel_plane_destroy_state(plane, new_plane_state);
+	return ret;
+
+slow_free:
+	intel_plane_destroy_state(plane, new_plane_state);
+slow:
+	return drm_atomic_helper_update_plane(plane, crtc, fb,
+					      crtc_x, crtc_y, crtc_w, crtc_h,
+					      src_x, src_y, src_w, src_h);
+}
+
+static const struct drm_plane_funcs intel_cursor_plane_funcs = {
+	.update_plane = intel_legacy_cursor_update,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = intel_plane_destroy,
+	.set_property = drm_atomic_helper_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,
+};
+
 static struct intel_plane *
 intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
@@ -15274,7 +15404,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 	cursor->disable_plane = intel_disable_cursor_plane;
 
 	ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base,
-				       0, &intel_plane_funcs,
+				       0, &intel_cursor_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 8f4ddca0f521..8b6f39506d45 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1844,6 +1844,8 @@ 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);
 extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
+					struct intel_plane_state *intel_state);
 
 /* intel_color.c */
 void intel_color_init(struct drm_crtc *crtc);
-- 
2.7.4

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

Re: [PATCH v2 6/6] drm/i915: Add a cursor hack to allow converting legacy page flip to atomic, v3.

[Archit Taneja]

Hi,

On 12/12/2016 4:04 PM, Maarten Lankhorst wrote:
> Do something similar to vc4, only allow updating the cursor state
> in-place through a fastpath when the watermarks are unaffected. This
> will allow cursor movement to be smooth, but changing cursor size or
> showing/hiding cursor will still fall back so watermarks can be updated.
>
> Only moving and changing fb is allowed.

I was implementing something similar for msm kms. I have a comment/query
below.

>
> Changes since v1:
> - Set page flip to always_unused for trybot.
> - Copy fence correctly, ignore plane_state->state, should be NULL.
> - Check crtc_state for !active and modeset, go to slowpath if the case.
> Changes since v2:
> - Make error handling work correctly. (Matthew Auld)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  47 +++++++----
>  drivers/gpu/drm/i915/intel_display.c      | 132 +++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h          |   2 +
>  3 files changed, 163 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index dbe9fb41ae53..60d75ce8a989 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane,
>  	drm_atomic_helper_plane_destroy_state(plane, state);
>  }
>
> -static int intel_plane_atomic_check(struct drm_plane *plane,
> -				    struct drm_plane_state *state)
> +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
> +					struct intel_plane_state *intel_state)
>  {
> +	struct drm_plane *plane = intel_state->base.plane;
>  	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> -	struct drm_crtc *crtc = state->crtc;
> -	struct intel_crtc *intel_crtc;
> -	struct intel_crtc_state *crtc_state;
> +	struct drm_plane_state *state = &intel_state->base;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> -	struct drm_crtc_state *drm_crtc_state;
>  	int ret;
>
> -	crtc = crtc ? crtc : plane->state->crtc;
> -	intel_crtc = to_intel_crtc(crtc);
> -
>  	/*
>  	 * Both crtc and plane->crtc could be NULL if we're updating a
>  	 * property while the plane is disabled.  We don't actually have
>  	 * anything driver-specific we need to test in that case, so
>  	 * just return success.
>  	 */
> -	if (!crtc)
> +	if (!intel_state->base.crtc && !plane->state->crtc)
>  		return 0;
>
> -	drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
> -	if (WARN_ON(!drm_crtc_state))
> -		return -EINVAL;
> -
> -	crtc_state = to_intel_crtc_state(drm_crtc_state);
> -
>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
> @@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	return intel_plane_atomic_calc_changes(&crtc_state->base, state);
>  }
>
> +static int intel_plane_atomic_check(struct drm_plane *plane,
> +				    struct drm_plane_state *state)
> +{
> +	struct drm_crtc *crtc = state->crtc;
> +	struct drm_crtc_state *drm_crtc_state;
> +
> +	crtc = crtc ? crtc : plane->state->crtc;
> +
> +	/*
> +	 * Both crtc and plane->crtc could be NULL if we're updating a
> +	 * property while the plane is disabled.  We don't actually have
> +	 * anything driver-specific we need to test in that case, so
> +	 * just return success.
> +	 */
> +	if (!crtc)
> +		return 0;
> +
> +	drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
> +	if (WARN_ON(!drm_crtc_state))
> +		return -EINVAL;
> +
> +	return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),
> +						   to_intel_plane_state(state));
> +}
> +
>  static void intel_plane_atomic_update(struct drm_plane *plane,
>  				      struct drm_plane_state *old_state)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9eaf1e5bdae9..5568ecac2edc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = {
>  	.atomic_destroy_state = intel_plane_destroy_state,
>  };
>
> +static int
> +intel_legacy_cursor_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 drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	int ret;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_framebuffer *old_fb;
> +	struct drm_crtc_state *crtc_state = crtc->state;
> +
> +	/*
> +	 * When crtc is inactive or there is a modeset pending,
> +	 * wait for it to complete in the slowpath
> +	 */
> +	if (!crtc_state->active || needs_modeset(crtc_state) ||
> +	    to_intel_crtc_state(crtc_state)->update_pipe)
> +		goto slow;
> +
> +	old_plane_state = plane->state;
> +
> +	/*
> +	 * If any parameters change that may affect watermarks,
> +	 * take the slowpath. Only changing fb or position should be
> +	 * in the fastpath.
> +	 */
> +	if (old_plane_state->crtc != crtc ||
> +	    old_plane_state->src_w != src_w ||
> +	    old_plane_state->src_h != src_h ||
> +	    old_plane_state->crtc_w != crtc_w ||
> +	    old_plane_state->crtc_h != crtc_h ||
> +	    !old_plane_state->visible ||
> +	    old_plane_state->fb->modifier != fb->modifier)
> +		goto slow;
> +
> +	new_plane_state = intel_plane_duplicate_state(plane);
> +	if (!new_plane_state)
> +		return -ENOMEM;

About creating a new plane state here and swapping it later, I guess
it's more for the sake of intel_plane_atomic_check_with_state() to go
through cleanly, right? Rather than directly modifying the old plane
state?

I didn't see anything equivalent in vc4's implementation of
update_plane, hence was wondering why exactly it is needed.

Also, about updating fb in the fast path, my assumption was
that it was only crtc_x/y was the thing most likely to be
changed really fast. Have you seen use cases where the fb
changes at a faster than vsync rate?

Thanks,
Archit

> +
> +	drm_atomic_set_fb_for_plane(new_plane_state, fb);
> +
> +	new_plane_state->src_x = src_x;
> +	new_plane_state->src_y = src_y;
> +	new_plane_state->src_w = src_w;
> +	new_plane_state->src_h = src_h;
> +	new_plane_state->crtc_x = crtc_x;
> +	new_plane_state->crtc_y = crtc_y;
> +	new_plane_state->crtc_w = crtc_w;
> +	new_plane_state->crtc_h = crtc_h;
> +
> +	ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
> +						  to_intel_plane_state(new_plane_state));
> +	if (ret)
> +		goto out_free;
> +
> +	/* Visibility changed, must take slowpath. */
> +	if (!new_plane_state->visible)
> +		goto slow_free;
> +
> +	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> +	if (ret)
> +		goto out_free;
> +
> +	if (INTEL_INFO(dev_priv)->cursor_needs_physical) {
> +		int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
> +
> +		ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align);
> +		if (ret) {
> +			DRM_DEBUG_KMS("failed to attach phys object\n");
> +			goto out_unlock;
> +		}
> +	} else {
> +		struct i915_vma *vma;
> +
> +		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
> +		if (IS_ERR(vma)) {
> +			DRM_DEBUG_KMS("failed to pin object\n");
> +
> +			ret = PTR_ERR(vma);
> +			goto out_unlock;
> +		}
> +	}
> +
> +	old_fb = old_plane_state->fb;
> +
> +	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
> +			  intel_plane->frontbuffer_bit);
> +
> +	/* Swap plane state */
> +	new_plane_state->fence = old_plane_state->fence;
> +	*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
> +	new_plane_state->fence = NULL;
> +	new_plane_state->fb = old_fb;
> +
> +	intel_plane->update_plane(plane,
> +				  to_intel_crtc_state(crtc->state),
> +				  to_intel_plane_state(plane->state));
> +
> +	intel_cleanup_plane_fb(plane, new_plane_state);
> +
> +out_unlock:
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> +out_free:
> +	intel_plane_destroy_state(plane, new_plane_state);
> +	return ret;
> +
> +slow_free:
> +	intel_plane_destroy_state(plane, new_plane_state);
> +slow:
> +	return drm_atomic_helper_update_plane(plane, crtc, fb,
> +					      crtc_x, crtc_y, crtc_w, crtc_h,
> +					      src_x, src_y, src_w, src_h);
> +}
> +
> +static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> +	.update_plane = intel_legacy_cursor_update,
> +	.disable_plane = drm_atomic_helper_disable_plane,
> +	.destroy = intel_plane_destroy,
> +	.set_property = drm_atomic_helper_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,
> +};
> +
>  static struct intel_plane *
>  intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  {
> @@ -15274,7 +15404,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	cursor->disable_plane = intel_disable_cursor_plane;
>
>  	ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base,
> -				       0, &intel_plane_funcs,
> +				       0, &intel_cursor_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 8f4ddca0f521..8b6f39506d45 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1844,6 +1844,8 @@ 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);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
> +					struct intel_plane_state *intel_state);
>
>  /* intel_color.c */
>  void intel_color_init(struct drm_crtc *crtc);
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/i915: Add a cursor hack to allow converting legacy page flip to atomic, v3.

[Maarten Lankhorst]

Op 13-12-16 om 14:01 schreef Archit Taneja:
> Hi,
>
> On 12/12/2016 4:04 PM, Maarten Lankhorst wrote:
>> Do something similar to vc4, only allow updating the cursor state
>> in-place through a fastpath when the watermarks are unaffected. This
>> will allow cursor movement to be smooth, but changing cursor size or
>> showing/hiding cursor will still fall back so watermarks can be updated.
>>
>> Only moving and changing fb is allowed.
>
> I was implementing something similar for msm kms. I have a comment/query
> below.
>
>>
>> Changes since v1:
>> - Set page flip to always_unused for trybot.
>> - Copy fence correctly, ignore plane_state->state, should be NULL.
>> - Check crtc_state for !active and modeset, go to slowpath if the case.
>> Changes since v2:
>> - Make error handling work correctly. (Matthew Auld)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic_plane.c |  47 +++++++----
>>  drivers/gpu/drm/i915/intel_display.c      | 132 +++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/intel_drv.h          |   2 +
>>  3 files changed, 163 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> index dbe9fb41ae53..60d75ce8a989 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane,
>>      drm_atomic_helper_plane_destroy_state(plane, state);
>>  }
>>
>> -static int intel_plane_atomic_check(struct drm_plane *plane,
>> -                    struct drm_plane_state *state)
>> +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
>> +                    struct intel_plane_state *intel_state)
>>  {
>> +    struct drm_plane *plane = intel_state->base.plane;
>>      struct drm_i915_private *dev_priv = to_i915(plane->dev);
>> -    struct drm_crtc *crtc = state->crtc;
>> -    struct intel_crtc *intel_crtc;
>> -    struct intel_crtc_state *crtc_state;
>> +    struct drm_plane_state *state = &intel_state->base;
>>      struct intel_plane *intel_plane = to_intel_plane(plane);
>> -    struct intel_plane_state *intel_state = to_intel_plane_state(state);
>> -    struct drm_crtc_state *drm_crtc_state;
>>      int ret;
>>
>> -    crtc = crtc ? crtc : plane->state->crtc;
>> -    intel_crtc = to_intel_crtc(crtc);
>> -
>>      /*
>>       * Both crtc and plane->crtc could be NULL if we're updating a
>>       * property while the plane is disabled.  We don't actually have
>>       * anything driver-specific we need to test in that case, so
>>       * just return success.
>>       */
>> -    if (!crtc)
>> +    if (!intel_state->base.crtc && !plane->state->crtc)
>>          return 0;
>>
>> -    drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
>> -    if (WARN_ON(!drm_crtc_state))
>> -        return -EINVAL;
>> -
>> -    crtc_state = to_intel_crtc_state(drm_crtc_state);
>> -
>>      /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>>      intel_state->clip.x1 = 0;
>>      intel_state->clip.y1 = 0;
>> @@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>      return intel_plane_atomic_calc_changes(&crtc_state->base, state);
>>  }
>>
>> +static int intel_plane_atomic_check(struct drm_plane *plane,
>> +                    struct drm_plane_state *state)
>> +{
>> +    struct drm_crtc *crtc = state->crtc;
>> +    struct drm_crtc_state *drm_crtc_state;
>> +
>> +    crtc = crtc ? crtc : plane->state->crtc;
>> +
>> +    /*
>> +     * Both crtc and plane->crtc could be NULL if we're updating a
>> +     * property while the plane is disabled.  We don't actually have
>> +     * anything driver-specific we need to test in that case, so
>> +     * just return success.
>> +     */
>> +    if (!crtc)
>> +        return 0;
>> +
>> +    drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
>> +    if (WARN_ON(!drm_crtc_state))
>> +        return -EINVAL;
>> +
>> +    return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),
>> +                           to_intel_plane_state(state));
>> +}
>> +
>>  static void intel_plane_atomic_update(struct drm_plane *plane,
>>                        struct drm_plane_state *old_state)
>>  {
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 9eaf1e5bdae9..5568ecac2edc 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = {
>>      .atomic_destroy_state = intel_plane_destroy_state,
>>  };
>>
>> +static int
>> +intel_legacy_cursor_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 drm_i915_private *dev_priv = to_i915(crtc->dev);
>> +    int ret;
>> +    struct drm_plane_state *old_plane_state, *new_plane_state;
>> +    struct intel_plane *intel_plane = to_intel_plane(plane);
>> +    struct drm_framebuffer *old_fb;
>> +    struct drm_crtc_state *crtc_state = crtc->state;
>> +
>> +    /*
>> +     * When crtc is inactive or there is a modeset pending,
>> +     * wait for it to complete in the slowpath
>> +     */
>> +    if (!crtc_state->active || needs_modeset(crtc_state) ||
>> +        to_intel_crtc_state(crtc_state)->update_pipe)
>> +        goto slow;
>> +
>> +    old_plane_state = plane->state;
>> +
>> +    /*
>> +     * If any parameters change that may affect watermarks,
>> +     * take the slowpath. Only changing fb or position should be
>> +     * in the fastpath.
>> +     */
>> +    if (old_plane_state->crtc != crtc ||
>> +        old_plane_state->src_w != src_w ||
>> +        old_plane_state->src_h != src_h ||
>> +        old_plane_state->crtc_w != crtc_w ||
>> +        old_plane_state->crtc_h != crtc_h ||
>> +        !old_plane_state->visible ||
>> +        old_plane_state->fb->modifier != fb->modifier)
>> +        goto slow;
>> +
>> +    new_plane_state = intel_plane_duplicate_state(plane);
>> +    if (!new_plane_state)
>> +        return -ENOMEM;
>
> About creating a new plane state here and swapping it later, I guess
> it's more for the sake of intel_plane_atomic_check_with_state() to go
> through cleanly, right? Rather than directly modifying the old plane
> state?
i915 keeps some derived state in intel_plane_state, we copy the full state to make sure the full derived state is updated as well.

> I didn't see anything equivalent in vc4's implementation of
> update_plane, hence was wondering why exactly it is needed.
>
> Also, about updating fb in the fast path, my assumption was
> that it was only crtc_x/y was the thing most likely to be
> changed really fast. Have you seen use cases where the fb
> changes at a faster than vsync rate? 
It didn't cost much extra effort to implement fb changing too since that is handled in
intel's update_plane callback too. This is why I did that as well. I'm not sure it's needed in general.
Any changed fb still has to have the same size or we go to slowpath.

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

Re: [Intel-gfx] [PATCH 2/6] drm/atomic: Unconditionally call prepare_fb.

[Maarten Lankhorst]

Op 09-12-16 om 09:25 schreef Daniel Vetter:
> On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote:
>>> On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote:
>>>> Atomic drivers may set properties like rotation on the same fb, which
>>>> may require a call to prepare_fb even when framebuffer stays identical.
>>>>
>>>> Instead of handling all the special cases in the core, let the driver
>>>> decide when prepare_fb and cleanup_fb are noops.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> I think this makes sense, but would be really good to get a pile of acks
>>> from driver maintainers on this one. Rob, Eric, Laurent, others?
>> This is all very nice, but it will introduce at least a performance 
>> regression, and possibly worse, until drivers get updated. There are 7 drivers 
>> implementing the .prepare_fb() callback (plus a bunch of drivers that probably 
>> should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this 
>> patch before they get fixed.
> Maarten's commit message is insufficient, since this is defacto a revert
> of
>
> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> Author: Keith Packard <keithp@keithp.com>
> Date:   Sat Jun 4 01:16:22 2016 -0700
>
>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
>
> because that breaks stuff. We're simply going back to where we've been a
> few months ago. Since this is a regression fix, back to original
> behaviour, can you ack (assuming Maarten updates the commit message to
> reflect the nature of the commit here)?

Waiting on a reply, but what about this commit message for this patch?
---
Atomic drivers may set properties like rotation on the same fb, which
may require a call to prepare_fb even when framebuffer stays identical.

Instead of handling all the special cases in the core, let the driver
decide when prepare_fb and cleanup_fb are noops.

This is a revert of:

commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
Author: Keith Packard <keithp@keithp.com>
Date:   Sat Jun 4 01:16:22 2016 -0700

    drm: Don't prepare or cleanup unchanging frame buffers [v3]

The original commit mentions that this prevents waiting in i915 on all
previous rendering during cursor updates, but there are better ways to
fix this.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

Re: [PATCH 2/6] drm/atomic: Unconditionally call prepare_fb.

[Daniel Vetter]

On Tue, Dec 13, 2016 at 03:13:54PM +0100, Maarten Lankhorst wrote:
> Op 09-12-16 om 09:25 schreef Daniel Vetter:
> > On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote:
> >> Hi Daniel,
> >>
> >> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote:
> >>> On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote:
> >>>> Atomic drivers may set properties like rotation on the same fb, which
> >>>> may require a call to prepare_fb even when framebuffer stays identical.
> >>>>
> >>>> Instead of handling all the special cases in the core, let the driver
> >>>> decide when prepare_fb and cleanup_fb are noops.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> I think this makes sense, but would be really good to get a pile of acks
> >>> from driver maintainers on this one. Rob, Eric, Laurent, others?
> >> This is all very nice, but it will introduce at least a performance 
> >> regression, and possibly worse, until drivers get updated. There are 7 drivers 
> >> implementing the .prepare_fb() callback (plus a bunch of drivers that probably 
> >> should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this 
> >> patch before they get fixed.
> > Maarten's commit message is insufficient, since this is defacto a revert
> > of
> >
> > commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> > Author: Keith Packard <keithp@keithp.com>
> > Date:   Sat Jun 4 01:16:22 2016 -0700
> >
> >     drm: Don't prepare or cleanup unchanging frame buffers [v3]
> >
> > because that breaks stuff. We're simply going back to where we've been a
> > few months ago. Since this is a regression fix, back to original
> > behaviour, can you ack (assuming Maarten updates the commit message to
> > reflect the nature of the commit here)?
> 
> Waiting on a reply, but what about this commit message for this patch?
> ---
> Atomic drivers may set properties like rotation on the same fb, which
> may require a call to prepare_fb even when framebuffer stays identical.
> 
> Instead of handling all the special cases in the core, let the driver
> decide when prepare_fb and cleanup_fb are noops.
> 
> This is a revert of:
> 
> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> Author: Keith Packard <keithp@keithp.com>
> Date:   Sat Jun 4 01:16:22 2016 -0700
> 
>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
> 
> The original commit mentions that this prevents waiting in i915 on all
> previous rendering during cursor updates, but there are better ways to
> fix this.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Yeah sounds good to me. Since we don't want to backport all the i915
cursor patches no cc: stable on this. Also, this is only an issue for
drivers which both have a cursor plane, and implement that cursor using
universal planes (i.e. settting drm_crtc->cursor). Afaik the only two are
vc4 and i915, and after this series both will have appropriate hacks (for
now) to keep existing userspace happy.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/atomic: Unconditionally call prepare_fb.

[Laurent Pinchart]

Hi Maarten,

On Tuesday 13 Dec 2016 15:13:54 Maarten Lankhorst wrote:
> Op 09-12-16 om 09:25 schreef Daniel Vetter:
> > On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote:
> >> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote:
> >>> On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote:
> >>>> Atomic drivers may set properties like rotation on the same fb, which
> >>>> may require a call to prepare_fb even when framebuffer stays identical.
> >>>> 
> >>>> Instead of handling all the special cases in the core, let the driver
> >>>> decide when prepare_fb and cleanup_fb are noops.
> >>>> 
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> 
> >>> I think this makes sense, but would be really good to get a pile of acks
> >>> from driver maintainers on this one. Rob, Eric, Laurent, others?
> >> 
> >> This is all very nice, but it will introduce at least a performance
> >> regression, and possibly worse, until drivers get updated. There are 7
> >> drivers implementing the .prepare_fb() callback (plus a bunch of drivers
> >> that probably should use drm_fb_cma_prepare_fb() but don't at the
> >> moment). I can't ack this patch before they get fixed.
> > 
> > Maarten's commit message is insufficient, since this is defacto a revert
> > of
> > 
> > commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> > Author: Keith Packard <keithp@keithp.com>
> > Date:   Sat Jun 4 01:16:22 2016 -0700
> > 
> >     drm: Don't prepare or cleanup unchanging frame buffers [v3]
> > 
> > because that breaks stuff. We're simply going back to where we've been a
> > few months ago. Since this is a regression fix, back to original
> > behaviour, can you ack (assuming Maarten updates the commit message to
> > reflect the nature of the commit here)?
> 
> Waiting on a reply, but what about this commit message for this patch?
> ---
> Atomic drivers may set properties like rotation on the same fb, which
> may require a call to prepare_fb even when framebuffer stays identical.
> 
> Instead of handling all the special cases in the core, let the driver
> decide when prepare_fb and cleanup_fb are noops.
> 
> This is a revert of:
> 
> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> Author: Keith Packard <keithp@keithp.com>
> Date:   Sat Jun 4 01:16:22 2016 -0700
> 
>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
> 
> The original commit mentions that this prevents waiting in i915 on all
> previous rendering during cursor updates, but there are better ways to
> fix this.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 6/6] drm/i915: Add a cursor hack to allow converting legacy page flip to atomic, v3.

[Archit Taneja]

On 12/13/2016 07:22 PM, Maarten Lankhorst wrote:
> Op 13-12-16 om 14:01 schreef Archit Taneja:
>> Hi,
>>
>> On 12/12/2016 4:04 PM, Maarten Lankhorst wrote:
>>> Do something similar to vc4, only allow updating the cursor state
>>> in-place through a fastpath when the watermarks are unaffected. This
>>> will allow cursor movement to be smooth, but changing cursor size or
>>> showing/hiding cursor will still fall back so watermarks can be updated.
>>>
>>> Only moving and changing fb is allowed.
>>
>> I was implementing something similar for msm kms. I have a comment/query
>> below.
>>
>>>
>>> Changes since v1:
>>> - Set page flip to always_unused for trybot.
>>> - Copy fence correctly, ignore plane_state->state, should be NULL.
>>> - Check crtc_state for !active and modeset, go to slowpath if the case.
>>> Changes since v2:
>>> - Make error handling work correctly. (Matthew Auld)
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_atomic_plane.c |  47 +++++++----
>>>  drivers/gpu/drm/i915/intel_display.c      | 132 +++++++++++++++++++++++++++++-
>>>  drivers/gpu/drm/i915/intel_drv.h          |   2 +
>>>  3 files changed, 163 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> index dbe9fb41ae53..60d75ce8a989 100644
>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane,
>>>      drm_atomic_helper_plane_destroy_state(plane, state);
>>>  }
>>>
>>> -static int intel_plane_atomic_check(struct drm_plane *plane,
>>> -                    struct drm_plane_state *state)
>>> +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
>>> +                    struct intel_plane_state *intel_state)
>>>  {
>>> +    struct drm_plane *plane = intel_state->base.plane;
>>>      struct drm_i915_private *dev_priv = to_i915(plane->dev);
>>> -    struct drm_crtc *crtc = state->crtc;
>>> -    struct intel_crtc *intel_crtc;
>>> -    struct intel_crtc_state *crtc_state;
>>> +    struct drm_plane_state *state = &intel_state->base;
>>>      struct intel_plane *intel_plane = to_intel_plane(plane);
>>> -    struct intel_plane_state *intel_state = to_intel_plane_state(state);
>>> -    struct drm_crtc_state *drm_crtc_state;
>>>      int ret;
>>>
>>> -    crtc = crtc ? crtc : plane->state->crtc;
>>> -    intel_crtc = to_intel_crtc(crtc);
>>> -
>>>      /*
>>>       * Both crtc and plane->crtc could be NULL if we're updating a
>>>       * property while the plane is disabled.  We don't actually have
>>>       * anything driver-specific we need to test in that case, so
>>>       * just return success.
>>>       */
>>> -    if (!crtc)
>>> +    if (!intel_state->base.crtc && !plane->state->crtc)
>>>          return 0;
>>>
>>> -    drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
>>> -    if (WARN_ON(!drm_crtc_state))
>>> -        return -EINVAL;
>>> -
>>> -    crtc_state = to_intel_crtc_state(drm_crtc_state);
>>> -
>>>      /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>>>      intel_state->clip.x1 = 0;
>>>      intel_state->clip.y1 = 0;
>>> @@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>>      return intel_plane_atomic_calc_changes(&crtc_state->base, state);
>>>  }
>>>
>>> +static int intel_plane_atomic_check(struct drm_plane *plane,
>>> +                    struct drm_plane_state *state)
>>> +{
>>> +    struct drm_crtc *crtc = state->crtc;
>>> +    struct drm_crtc_state *drm_crtc_state;
>>> +
>>> +    crtc = crtc ? crtc : plane->state->crtc;
>>> +
>>> +    /*
>>> +     * Both crtc and plane->crtc could be NULL if we're updating a
>>> +     * property while the plane is disabled.  We don't actually have
>>> +     * anything driver-specific we need to test in that case, so
>>> +     * just return success.
>>> +     */
>>> +    if (!crtc)
>>> +        return 0;
>>> +
>>> +    drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
>>> +    if (WARN_ON(!drm_crtc_state))
>>> +        return -EINVAL;
>>> +
>>> +    return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),
>>> +                           to_intel_plane_state(state));
>>> +}
>>> +
>>>  static void intel_plane_atomic_update(struct drm_plane *plane,
>>>                        struct drm_plane_state *old_state)
>>>  {
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 9eaf1e5bdae9..5568ecac2edc 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = {
>>>      .atomic_destroy_state = intel_plane_destroy_state,
>>>  };
>>>
>>> +static int
>>> +intel_legacy_cursor_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 drm_i915_private *dev_priv = to_i915(crtc->dev);
>>> +    int ret;
>>> +    struct drm_plane_state *old_plane_state, *new_plane_state;
>>> +    struct intel_plane *intel_plane = to_intel_plane(plane);
>>> +    struct drm_framebuffer *old_fb;
>>> +    struct drm_crtc_state *crtc_state = crtc->state;
>>> +
>>> +    /*
>>> +     * When crtc is inactive or there is a modeset pending,
>>> +     * wait for it to complete in the slowpath
>>> +     */
>>> +    if (!crtc_state->active || needs_modeset(crtc_state) ||
>>> +        to_intel_crtc_state(crtc_state)->update_pipe)
>>> +        goto slow;
>>> +
>>> +    old_plane_state = plane->state;
>>> +
>>> +    /*
>>> +     * If any parameters change that may affect watermarks,
>>> +     * take the slowpath. Only changing fb or position should be
>>> +     * in the fastpath.
>>> +     */
>>> +    if (old_plane_state->crtc != crtc ||
>>> +        old_plane_state->src_w != src_w ||
>>> +        old_plane_state->src_h != src_h ||
>>> +        old_plane_state->crtc_w != crtc_w ||
>>> +        old_plane_state->crtc_h != crtc_h ||
>>> +        !old_plane_state->visible ||
>>> +        old_plane_state->fb->modifier != fb->modifier)
>>> +        goto slow;
>>> +
>>> +    new_plane_state = intel_plane_duplicate_state(plane);
>>> +    if (!new_plane_state)
>>> +        return -ENOMEM;
>>
>> About creating a new plane state here and swapping it later, I guess
>> it's more for the sake of intel_plane_atomic_check_with_state() to go
>> through cleanly, right? Rather than directly modifying the old plane
>> state?
> i915 keeps some derived state in intel_plane_state, we copy the full state to make sure the full derived state is updated as well.
>
>> I didn't see anything equivalent in vc4's implementation of
>> update_plane, hence was wondering why exactly it is needed.
>>
>> Also, about updating fb in the fast path, my assumption was
>> that it was only crtc_x/y was the thing most likely to be
>> changed really fast. Have you seen use cases where the fb
>> changes at a faster than vsync rate?
> It didn't cost much extra effort to implement fb changing too since that is handled in
> intel's update_plane callback too. This is why I did that as well. I'm not sure it's needed in general.
> Any changed fb still has to have the same size or we go to slowpath.

Okay, that make sense.

Thanks,
Archit

>
> ~Maarten
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/6] drm/atomic: Clean up wait_for_vblanks, v2.

[Maarten Lankhorst]

Stop relying on a per crtc_state last_vblank_count, we shouldn't touch
crtc_state after commit. Move it to atomic_state->crtcs.

Also stop re-using new_crtc_state->enable, we can now simply set a
bitmask with crtc_crtc_mask.

Changes since v1:
- Keep last_vblank_count in __drm_crtc_state.
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d19563651e07..88c0986d226a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1110,19 +1110,19 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	int i, ret;
+	unsigned crtc_mask = 0;
 
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-		/* No one cares about the old state, so abuse it for tracking
-		 * and store whether we hold a vblank reference (and should do a
-		 * vblank wait) in the ->enable boolean. */
-		old_crtc_state->enable = false;
+	 /*
+	  * Legacy cursor ioctls are completely unsynced, and userspace
+	  * relies on that (by doing tons of cursor updates).
+	  */
+	if (old_state->legacy_cursor_update)
+		return;
 
-		if (!crtc->state->active)
-			continue;
+	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+		struct drm_crtc_state *new_crtc_state = crtc->state;
 
-		/* Legacy cursor ioctls are completely unsynced, and userspace
-		 * relies on that (by doing tons of cursor updates). */
-		if (old_state->legacy_cursor_update)
+		if (!new_crtc_state->active)
 			continue;
 
 		if (!drm_atomic_helper_framebuffer_changed(dev,
@@ -1133,16 +1133,16 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 		if (ret != 0)
 			continue;
 
-		old_crtc_state->enable = true;
-		old_crtc_state->last_vblank_count = drm_crtc_vblank_count(crtc);
+		crtc_mask |= drm_crtc_mask(crtc);
+		old_state->crtcs[i].last_vblank_count = drm_crtc_vblank_count(crtc);
 	}
 
 	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-		if (!old_crtc_state->enable)
+		if (!(crtc_mask & drm_crtc_mask(crtc)))
 			continue;
 
 		ret = wait_event_timeout(dev->vblank[i].queue,
-				old_crtc_state->last_vblank_count !=
+				old_state->crtcs[i].last_vblank_count !=
 					drm_crtc_vblank_count(crtc),
 				msecs_to_jiffies(50));
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d6d241f63b9f..617f095e31ba 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -145,6 +145,7 @@ struct __drm_crtcs_state {
 	struct drm_crtc_state *state;
 	struct drm_crtc_commit *commit;
 	s64 __user *out_fence_ptr;
+	unsigned last_vblank_count;
 };
 
 struct __drm_connnectors_state {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 946672f97e1e..e03d194be086 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -93,8 +93,6 @@ struct drm_plane_helper_funcs;
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
  * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
  * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
- * @last_vblank_count: for helpers and drivers to capture the vblank of the
- * 	update to ensure framebuffer cleanup isn't done too early
  * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
  * @mode: current mode timings
  * @mode_blob: &drm_property_blob for @mode
@@ -140,9 +138,6 @@ struct drm_crtc_state {
 	u32 connector_mask;
 	u32 encoder_mask;
 
-	/* last_vblank_count: for vblank waits before cleanup */
-	u32 last_vblank_count;
-
 	/* adjusted_mode: for use by helpers and drivers */
 	struct drm_display_mode adjusted_mode;
 

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

Re: [PATCH v2 3/6] drm/atomic: Clean up wait_for_vblanks, v2.

[Daniel Vetter]

On Thu, Dec 15, 2016 at 12:51:42PM +0100, Maarten Lankhorst wrote:
> Stop relying on a per crtc_state last_vblank_count, we shouldn't touch
> crtc_state after commit. Move it to atomic_state->crtcs.
> 
> Also stop re-using new_crtc_state->enable, we can now simply set a
> bitmask with crtc_crtc_mask.
> 
> Changes since v1:
> - Keep last_vblank_count in __drm_crtc_state.

Just noticed: sob is missing.

With that fixed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> ---
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d19563651e07..88c0986d226a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1110,19 +1110,19 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state;
>  	int i, ret;
> +	unsigned crtc_mask = 0;
>  
> -	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> -		/* No one cares about the old state, so abuse it for tracking
> -		 * and store whether we hold a vblank reference (and should do a
> -		 * vblank wait) in the ->enable boolean. */
> -		old_crtc_state->enable = false;
> +	 /*
> +	  * Legacy cursor ioctls are completely unsynced, and userspace
> +	  * relies on that (by doing tons of cursor updates).
> +	  */
> +	if (old_state->legacy_cursor_update)
> +		return;
>  
> -		if (!crtc->state->active)
> -			continue;
> +	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> +		struct drm_crtc_state *new_crtc_state = crtc->state;
>  
> -		/* Legacy cursor ioctls are completely unsynced, and userspace
> -		 * relies on that (by doing tons of cursor updates). */
> -		if (old_state->legacy_cursor_update)
> +		if (!new_crtc_state->active)
>  			continue;
>  
>  		if (!drm_atomic_helper_framebuffer_changed(dev,
> @@ -1133,16 +1133,16 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
>  		if (ret != 0)
>  			continue;
>  
> -		old_crtc_state->enable = true;
> -		old_crtc_state->last_vblank_count = drm_crtc_vblank_count(crtc);
> +		crtc_mask |= drm_crtc_mask(crtc);
> +		old_state->crtcs[i].last_vblank_count = drm_crtc_vblank_count(crtc);
>  	}
>  
>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> -		if (!old_crtc_state->enable)
> +		if (!(crtc_mask & drm_crtc_mask(crtc)))
>  			continue;
>  
>  		ret = wait_event_timeout(dev->vblank[i].queue,
> -				old_crtc_state->last_vblank_count !=
> +				old_state->crtcs[i].last_vblank_count !=
>  					drm_crtc_vblank_count(crtc),
>  				msecs_to_jiffies(50));
>  
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d6d241f63b9f..617f095e31ba 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -145,6 +145,7 @@ struct __drm_crtcs_state {
>  	struct drm_crtc_state *state;
>  	struct drm_crtc_commit *commit;
>  	s64 __user *out_fence_ptr;
> +	unsigned last_vblank_count;
>  };
>  
>  struct __drm_connnectors_state {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 946672f97e1e..e03d194be086 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -93,8 +93,6 @@ struct drm_plane_helper_funcs;
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
>   * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
> - * @last_vblank_count: for helpers and drivers to capture the vblank of the
> - * 	update to ensure framebuffer cleanup isn't done too early
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
>   * @mode: current mode timings
>   * @mode_blob: &drm_property_blob for @mode
> @@ -140,9 +138,6 @@ struct drm_crtc_state {
>  	u32 connector_mask;
>  	u32 encoder_mask;
>  
> -	/* last_vblank_count: for vblank waits before cleanup */
> -	u32 last_vblank_count;
> -
>  	/* adjusted_mode: for use by helpers and drivers */
>  	struct drm_display_mode adjusted_mode;
>  
> 

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

Re: [PATCH v2 3/6] drm/atomic: Clean up wait_for_vblanks, v2.

[Maarten Lankhorst]

Op 15-12-16 om 16:55 schreef Daniel Vetter:
> On Thu, Dec 15, 2016 at 12:51:42PM +0100, Maarten Lankhorst wrote:
>> Stop relying on a per crtc_state last_vblank_count, we shouldn't touch
>> crtc_state after commit. Move it to atomic_state->crtcs.
>>
>> Also stop re-using new_crtc_state->enable, we can now simply set a
>> bitmask with crtc_crtc_mask.
>>
>> Changes since v1:
>> - Keep last_vblank_count in __drm_crtc_state.
> Just noticed: sob is missing.
>
> With that fixed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Oh indeed, noticed it too when resending but forgot to re-add.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>


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

[PATCH v2 2/6] drm/atomic: Unconditionally call prepare_fb.

[Maarten Lankhorst]

Op 13-12-16 om 18:10 schreef Daniel Vetter:
> On Tue, Dec 13, 2016 at 03:13:54PM +0100, Maarten Lankhorst wrote:
>> Op 09-12-16 om 09:25 schreef Daniel Vetter:
>>> On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote:
>>>> Hi Daniel,
>>>>
>>>> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote:
>>>>> On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote:
>>>>>> Atomic drivers may set properties like rotation on the same fb, which
>>>>>> may require a call to prepare_fb even when framebuffer stays identical.
>>>>>>
>>>>>> Instead of handling all the special cases in the core, let the driver
>>>>>> decide when prepare_fb and cleanup_fb are noops.
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> I think this makes sense, but would be really good to get a pile of acks
>>>>> from driver maintainers on this one. Rob, Eric, Laurent, others?
>>>> This is all very nice, but it will introduce at least a performance 
>>>> regression, and possibly worse, until drivers get updated. There are 7 drivers 
>>>> implementing the .prepare_fb() callback (plus a bunch of drivers that probably 
>>>> should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this 
>>>> patch before they get fixed.
>>> Maarten's commit message is insufficient, since this is defacto a revert
>>> of
>>>
>>> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
>>> Author: Keith Packard <keithp@keithp.com>
>>> Date:   Sat Jun 4 01:16:22 2016 -0700
>>>
>>>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
>>>
>>> because that breaks stuff. We're simply going back to where we've been a
>>> few months ago. Since this is a regression fix, back to original
>>> behaviour, can you ack (assuming Maarten updates the commit message to
>>> reflect the nature of the commit here)?
>> Waiting on a reply, but what about this commit message for this patch?
>> ---
>> Atomic drivers may set properties like rotation on the same fb, which
>> may require a call to prepare_fb even when framebuffer stays identical.
>>
>> Instead of handling all the special cases in the core, let the driver
>> decide when prepare_fb and cleanup_fb are noops.
>>
>> This is a revert of:
>>
>> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
>> Author: Keith Packard <keithp@keithp.com>
>> Date:   Sat Jun 4 01:16:22 2016 -0700
>>
>>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
>>
>> The original commit mentions that this prevents waiting in i915 on all
>> previous rendering during cursor updates, but there are better ways to
>> fix this.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Yeah sounds good to me. Since we don't want to backport all the i915
> cursor patches no cc: stable on this. Also, this is only an issue for
> drivers which both have a cursor plane, and implement that cursor using
> universal planes (i.e. settting drm_crtc->cursor). Afaik the only two are
> vc4 and i915, and after this series both will have appropriate hacks (for
> now) to keep existing userspace happy.
> -Daniel

Same patch, reworded!
---8<---
Atomic drivers may set properties like rotation on the same fb, which
may require a call to prepare_fb even when framebuffer stays identical.

Instead of handling all the special cases in the core, let the driver
decide when prepare_fb and cleanup_fb are noops.

This is a revert of:

commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
Author: Keith Packard <keithp@keithp.com>
Date:   Sat Jun 4 01:16:22 2016 -0700

    drm: Don't prepare or cleanup unchanging frame buffers [v3]

The original commit mentions that this prevents waiting in i915 on all
previous rendering during cursor updates, but there are better ways to
fix this.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 23767df72615..d19563651e07 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
-			continue;
-
 		if (funcs->prepare_fb) {
 			ret = funcs->prepare_fb(plane, plane_state);
 			if (ret)
@@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 		if (j >= i)
 			continue;
 
-		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
-			continue;
-
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
@@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 	for_each_plane_in_state(old_state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 
-		if (!drm_atomic_helper_framebuffer_changed(dev, old_state, plane_state->crtc))
-			continue;
-
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
-- 
2.7.4


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

Re: [PATCH v2 2/6] drm/atomic: Unconditionally call prepare_fb.

[Laurent Pinchart]

Hi Maarten,

Thank you for the patch.

On Monday 19 Dec 2016 12:08:16 Maarten Lankhorst wrote:
> Op 13-12-16 om 18:10 schreef Daniel Vetter:
> > On Tue, Dec 13, 2016 at 03:13:54PM +0100, Maarten Lankhorst wrote:
> >> Op 09-12-16 om 09:25 schreef Daniel Vetter:
> >>> On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote:
> >>>> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote:
> >>>>> On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote:
> >>>>>> Atomic drivers may set properties like rotation on the same fb, which
> >>>>>> may require a call to prepare_fb even when framebuffer stays
> >>>>>> identical.
> >>>>>> 
> >>>>>> Instead of handling all the special cases in the core, let the driver
> >>>>>> decide when prepare_fb and cleanup_fb are noops.
> >>>>>> 
> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> 
> >>>>> I think this makes sense, but would be really good to get a pile of
> >>>>> acks from driver maintainers on this one. Rob, Eric, Laurent, others?
> >>>> 
> >>>> This is all very nice, but it will introduce at least a performance
> >>>> regression, and possibly worse, until drivers get updated. There are 7
> >>>> drivers implementing the .prepare_fb() callback (plus a bunch of
> >>>> drivers that probably should use drm_fb_cma_prepare_fb() but don't at
> >>>> the moment). I can't ack this patch before they get fixed.
> >>> 
> >>> Maarten's commit message is insufficient, since this is defacto a revert
> >>> of
> >>> 
> >>> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> >>> Author: Keith Packard <keithp@keithp.com>
> >>> Date:   Sat Jun 4 01:16:22 2016 -0700
> >>> 
> >>>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
> >>> 
> >>> because that breaks stuff. We're simply going back to where we've been a
> >>> few months ago. Since this is a regression fix, back to original
> >>> behaviour, can you ack (assuming Maarten updates the commit message to
> >>> reflect the nature of the commit here)?
> >> 
> >> Waiting on a reply, but what about this commit message for this patch?
> >> ---
> >> Atomic drivers may set properties like rotation on the same fb, which
> >> may require a call to prepare_fb even when framebuffer stays identical.
> >> 
> >> Instead of handling all the special cases in the core, let the driver
> >> decide when prepare_fb and cleanup_fb are noops.
> >> 
> >> This is a revert of:
> >> 
> >> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> >> Author: Keith Packard <keithp@keithp.com>
> >> Date:   Sat Jun 4 01:16:22 2016 -0700
> >> 
> >>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
> >> 
> >> The original commit mentions that this prevents waiting in i915 on all
> >> previous rendering during cursor updates, but there are better ways to
> >> fix this.
> >> 
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> > Yeah sounds good to me. Since we don't want to backport all the i915
> > cursor patches no cc: stable on this. Also, this is only an issue for
> > drivers which both have a cursor plane, and implement that cursor using
> > universal planes (i.e. settting drm_crtc->cursor). Afaik the only two are
> > vc4 and i915, and after this series both will have appropriate hacks (for
> > now) to keep existing userspace happy.
> > -Daniel
> 
> Same patch, reworded!
> ---8<---
> Atomic drivers may set properties like rotation on the same fb, which
> may require a call to prepare_fb even when framebuffer stays identical.
> 
> Instead of handling all the special cases in the core, let the driver
> decide when prepare_fb and cleanup_fb are noops.
> 
> This is a revert of:
> 
> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
> Author: Keith Packard <keithp@keithp.com>
> Date:   Sat Jun 4 01:16:22 2016 -0700
> 
>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
> 
> The original commit mentions that this prevents waiting in i915 on all
> previous rendering during cursor updates, but there are better ways to
> fix this.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index 23767df72615..d19563651e07
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device
> *dev,
> 
>  		funcs = plane->helper_private;
> 
> -		if (!drm_atomic_helper_framebuffer_changed(dev, state,
> plane_state->crtc)) -			continue;
> -
>  		if (funcs->prepare_fb) {
>  			ret = funcs->prepare_fb(plane, plane_state);
>  			if (ret)
> @@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device
> *dev, if (j >= i)
>  			continue;
> 
> -		if (!drm_atomic_helper_framebuffer_changed(dev, state,
> plane_state->crtc)) -			continue;
> -
>  		funcs = plane->helper_private;
> 
>  		if (funcs->cleanup_fb)
> @@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct
> drm_device *dev, for_each_plane_in_state(old_state, plane, plane_state, i)
> {
>  		const struct drm_plane_helper_funcs *funcs;
> 
> -		if (!drm_atomic_helper_framebuffer_changed(dev, old_state,
> plane_state->crtc)) -			continue;
> -
>  		funcs = plane->helper_private;
> 
>  		if (funcs->cleanup_fb)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 6/6] drm/i915: Add a cursor hack to allow converting legacy page flip to atomic, v3.

[Daniel Vetter]

On Mon, Dec 12, 2016 at 11:34:55AM +0100, Maarten Lankhorst wrote:
> Do something similar to vc4, only allow updating the cursor state
> in-place through a fastpath when the watermarks are unaffected. This
> will allow cursor movement to be smooth, but changing cursor size or
> showing/hiding cursor will still fall back so watermarks can be updated.
> 
> Only moving and changing fb is allowed.
> 
> Changes since v1:
> - Set page flip to always_unused for trybot.
> - Copy fence correctly, ignore plane_state->state, should be NULL.
> - Check crtc_state for !active and modeset, go to slowpath if the case.
> Changes since v2:
> - Make error handling work correctly. (Matthew Auld)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Pulled in the entire pile through drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  47 +++++++----
>  drivers/gpu/drm/i915/intel_display.c      | 132 +++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h          |   2 +
>  3 files changed, 163 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index dbe9fb41ae53..60d75ce8a989 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane,
>  	drm_atomic_helper_plane_destroy_state(plane, state);
>  }
>  
> -static int intel_plane_atomic_check(struct drm_plane *plane,
> -				    struct drm_plane_state *state)
> +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
> +					struct intel_plane_state *intel_state)
>  {
> +	struct drm_plane *plane = intel_state->base.plane;
>  	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> -	struct drm_crtc *crtc = state->crtc;
> -	struct intel_crtc *intel_crtc;
> -	struct intel_crtc_state *crtc_state;
> +	struct drm_plane_state *state = &intel_state->base;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> -	struct drm_crtc_state *drm_crtc_state;
>  	int ret;
>  
> -	crtc = crtc ? crtc : plane->state->crtc;
> -	intel_crtc = to_intel_crtc(crtc);
> -
>  	/*
>  	 * Both crtc and plane->crtc could be NULL if we're updating a
>  	 * property while the plane is disabled.  We don't actually have
>  	 * anything driver-specific we need to test in that case, so
>  	 * just return success.
>  	 */
> -	if (!crtc)
> +	if (!intel_state->base.crtc && !plane->state->crtc)
>  		return 0;
>  
> -	drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
> -	if (WARN_ON(!drm_crtc_state))
> -		return -EINVAL;
> -
> -	crtc_state = to_intel_crtc_state(drm_crtc_state);
> -
>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
> @@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	return intel_plane_atomic_calc_changes(&crtc_state->base, state);
>  }
>  
> +static int intel_plane_atomic_check(struct drm_plane *plane,
> +				    struct drm_plane_state *state)
> +{
> +	struct drm_crtc *crtc = state->crtc;
> +	struct drm_crtc_state *drm_crtc_state;
> +
> +	crtc = crtc ? crtc : plane->state->crtc;
> +
> +	/*
> +	 * Both crtc and plane->crtc could be NULL if we're updating a
> +	 * property while the plane is disabled.  We don't actually have
> +	 * anything driver-specific we need to test in that case, so
> +	 * just return success.
> +	 */
> +	if (!crtc)
> +		return 0;
> +
> +	drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
> +	if (WARN_ON(!drm_crtc_state))
> +		return -EINVAL;
> +
> +	return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),
> +						   to_intel_plane_state(state));
> +}
> +
>  static void intel_plane_atomic_update(struct drm_plane *plane,
>  				      struct drm_plane_state *old_state)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9eaf1e5bdae9..5568ecac2edc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = {
>  	.atomic_destroy_state = intel_plane_destroy_state,
>  };
>  
> +static int
> +intel_legacy_cursor_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 drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	int ret;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_framebuffer *old_fb;
> +	struct drm_crtc_state *crtc_state = crtc->state;
> +
> +	/*
> +	 * When crtc is inactive or there is a modeset pending,
> +	 * wait for it to complete in the slowpath
> +	 */
> +	if (!crtc_state->active || needs_modeset(crtc_state) ||
> +	    to_intel_crtc_state(crtc_state)->update_pipe)
> +		goto slow;
> +
> +	old_plane_state = plane->state;
> +
> +	/*
> +	 * If any parameters change that may affect watermarks,
> +	 * take the slowpath. Only changing fb or position should be
> +	 * in the fastpath.
> +	 */
> +	if (old_plane_state->crtc != crtc ||
> +	    old_plane_state->src_w != src_w ||
> +	    old_plane_state->src_h != src_h ||
> +	    old_plane_state->crtc_w != crtc_w ||
> +	    old_plane_state->crtc_h != crtc_h ||
> +	    !old_plane_state->visible ||
> +	    old_plane_state->fb->modifier != fb->modifier)
> +		goto slow;
> +
> +	new_plane_state = intel_plane_duplicate_state(plane);
> +	if (!new_plane_state)
> +		return -ENOMEM;
> +
> +	drm_atomic_set_fb_for_plane(new_plane_state, fb);
> +
> +	new_plane_state->src_x = src_x;
> +	new_plane_state->src_y = src_y;
> +	new_plane_state->src_w = src_w;
> +	new_plane_state->src_h = src_h;
> +	new_plane_state->crtc_x = crtc_x;
> +	new_plane_state->crtc_y = crtc_y;
> +	new_plane_state->crtc_w = crtc_w;
> +	new_plane_state->crtc_h = crtc_h;
> +
> +	ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
> +						  to_intel_plane_state(new_plane_state));
> +	if (ret)
> +		goto out_free;
> +
> +	/* Visibility changed, must take slowpath. */
> +	if (!new_plane_state->visible)
> +		goto slow_free;
> +
> +	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> +	if (ret)
> +		goto out_free;
> +
> +	if (INTEL_INFO(dev_priv)->cursor_needs_physical) {
> +		int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
> +
> +		ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align);
> +		if (ret) {
> +			DRM_DEBUG_KMS("failed to attach phys object\n");
> +			goto out_unlock;
> +		}
> +	} else {
> +		struct i915_vma *vma;
> +
> +		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
> +		if (IS_ERR(vma)) {
> +			DRM_DEBUG_KMS("failed to pin object\n");
> +
> +			ret = PTR_ERR(vma);
> +			goto out_unlock;
> +		}
> +	}
> +
> +	old_fb = old_plane_state->fb;
> +
> +	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
> +			  intel_plane->frontbuffer_bit);
> +
> +	/* Swap plane state */
> +	new_plane_state->fence = old_plane_state->fence;
> +	*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
> +	new_plane_state->fence = NULL;
> +	new_plane_state->fb = old_fb;
> +
> +	intel_plane->update_plane(plane,
> +				  to_intel_crtc_state(crtc->state),
> +				  to_intel_plane_state(plane->state));
> +
> +	intel_cleanup_plane_fb(plane, new_plane_state);
> +
> +out_unlock:
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> +out_free:
> +	intel_plane_destroy_state(plane, new_plane_state);
> +	return ret;
> +
> +slow_free:
> +	intel_plane_destroy_state(plane, new_plane_state);
> +slow:
> +	return drm_atomic_helper_update_plane(plane, crtc, fb,
> +					      crtc_x, crtc_y, crtc_w, crtc_h,
> +					      src_x, src_y, src_w, src_h);
> +}
> +
> +static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> +	.update_plane = intel_legacy_cursor_update,
> +	.disable_plane = drm_atomic_helper_disable_plane,
> +	.destroy = intel_plane_destroy,
> +	.set_property = drm_atomic_helper_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,
> +};
> +
>  static struct intel_plane *
>  intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  {
> @@ -15274,7 +15404,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	cursor->disable_plane = intel_disable_cursor_plane;
>  
>  	ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base,
> -				       0, &intel_plane_funcs,
> +				       0, &intel_cursor_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 8f4ddca0f521..8b6f39506d45 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1844,6 +1844,8 @@ 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);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
> +					struct intel_plane_state *intel_state);
>  
>  /* intel_color.c */
>  void intel_color_init(struct drm_crtc *crtc);
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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