[PATCH 0/4] Misc atomic-related updates

[PATCH 0/4] Misc atomic-related updates

[Matt Roper]

Here's a few atomic-related patches that I've developed in the process of
preparing the atomic watermark code.  I think some of this code is also
useful/important for other features that are being developed (e.g., SKL
scalers), so I'm extracting these and posting them early to get feedback and
possibly smooth the way for other developers.

The first patch here makes our atomic plane updates stop keying off
intel_crtc->active when generating their derived state.  This will be important
when we start handling CRTC state updates as part of an atomic transaction, or
even when we call our plane update functions from within a legacy modeset path.

The second patch updates some fragile plane => crtc mapping that works okay
today, but will definitely cause problems once we merge support for
non-blocking atomic flips.

The third patch switches our update_plane/disable_plane handlers back to the
full atomic helpers.  This caused a lot of breakage last time we tried, but we
try to sidestep those problems this time by replacing the plane vfunc calls we
still have in our legacy modeset path to call the transitional plane helpers
directly.  The important thing here is that we want to ensure legacy SetPlane()
calls from userspace have a top-level atomic transaction since a few pieces of
upcoming code really need this (SKL scalers, atomic watermarks, etc.).

The fourth patch fixes what I believe is a bug in our existing code (but not
one that anyone has stumbled over yet afaik).  Since crtc states aren't fully
wired up for i915 yet, we've been using intel_crtc->atomic as a temporary
dumping ground for stuff that will eventually transition to the CRTC state.  We
properly clear that structure out at the end of a successful atomic transaction
so that we'll start fresh on the next transaction, but we neglect to do so if
the transaction doesn't make it to the commit stage (e.g., because 'check'
failed).  I'm surprised having stale flags from a previous transaction's
'check' hasn't already caused problems for us, but I guess we've just been
lucky.



Matt Roper (4):
  drm/i915: Make atomic use in-flight state for CRTC active value
  drm/i915: Lookup CRTC for plane directly
  drm/i915: Switch to full atomic helpers for plane updates/disable,
    take two
  drm/i915: Clear crtc atomic flags at beginning of transaction

 drivers/gpu/drm/i915/intel_atomic.c       |  18 +++++
 drivers/gpu/drm/i915/intel_atomic_plane.c |  23 ++++---
 drivers/gpu/drm/i915/intel_display.c      | 107 ++++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_drv.h          |  11 +++
 drivers/gpu/drm/i915/intel_sprite.c       |  30 ++++++---
 5 files changed, 149 insertions(+), 40 deletions(-)

-- 
1.8.5.1

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

[PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Matt Roper]

Our atomic plane code currently uses intel_crtc->active to determine
how/when to update some derived state values.  This works fine for pure
plane updates at the moment since the CRTC state itself isn't changed as
part of the operation.  However as we convert more of our driver
internals over to atomic modesetting, we need to look at whether the
CRTC will be active at the *end* of the atomic transaction (which may
not match the currently committed state).

The in-flight value we want to use is generally in a crtc_state object
associated with our top-level atomic transaction.  However there are a
few cases where this isn't the case:

 * While using transitional atomic helpers (as we are at the moment),
   SetPlane() calls will operate on orphaned plane states that aren't
   part of a top-level atomic transaction.  In this case, we're not
   touching the CRTC state, so it's fine to use the already-committed
   value from crtc->state.

 * While updating properties of a disabled plane, we'll have a top-level
   atomic state, but it may not contain the CRTC state we're looking
   for.  Once again, this means we're not actually touching any CRTC
   state so it's safe to use the value from crtc->state directly.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 976b891..90c4a82 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_crtc *crtc = state->crtc;
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *intel_crtc_state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_plane_state *intel_state = to_intel_plane_state(state);
+	bool active;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
+	active = intel_crtc_state->base.enable;
+
 	/*
 	 * Both crtc and plane->crtc could be NULL if we're updating a
 	 * property while the plane is disabled.  We don't actually have
@@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
 	intel_state->clip.x1 = 0;
 	intel_state->clip.y1 = 0;
-	intel_state->clip.x2 =
-		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
-	intel_state->clip.y2 =
-		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
+	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
+	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;
 
 	/*
 	 * Disabling a plane is always okay; we just need to update
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7bfe2af..88b0f69 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12562,6 +12562,53 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	}
 }
 
+/**
+ * intel_crtc_state_for_plane - Obtain CRTC state for a plane
+ * @pstate: plane state to lookup corresponding crtc state for
+ *
+ * When working with a top-level atomic transaction (drm_atomic_state),
+ * a CRTC state should be present that corresponds to the provided
+ * plane state; this function provides a quick way to fetch that
+ * CRTC state.  In cases where we have a plane state unassociated with any
+ * top-level atomic transaction (e.g., while using the transitional atomic
+ * helpers), the current CRTC state from crtc->state will be returned
+ * instead.
+ */
+struct intel_crtc_state *
+intel_crtc_state_for_plane(struct intel_plane_state *pstate)
+{
+	struct drm_atomic_state *state = pstate->base.state;
+	struct intel_plane *plane = to_intel_plane(pstate->base.plane);
+	struct drm_crtc *crtc = intel_get_crtc_for_pipe(pstate->base.plane->dev,
+							plane->pipe);
+	int i;
+
+	/*
+	 * While using transitional plane helpers, we may not have a top-level
+	 * atomic transaction.  Of course that also means that we're not
+	 * actually touching CRTC state, so just return the currently
+	 * committed state.
+	 */
+	if (!state)
+		return to_intel_crtc_state(crtc->state);
+
+	for (i = 0; i < state->dev->mode_config.num_crtc; i++) {
+		if (!state->crtcs[i])
+			continue;
+
+		if (to_intel_crtc(state->crtcs[i])->pipe == plane->pipe)
+			return to_intel_crtc_state(state->crtc_states[i]);
+	}
+
+	/*
+	 * We may have a plane state without a corresponding CRTC state if
+	 * we're updating a property of a disabled plane.  Again, just using
+	 * the already-committed state for this plane's CRTC should be fine
+	 * since we're not actually touching the CRTC here.
+	 */
+	return to_intel_crtc_state(crtc->state);
+}
+
 static int
 intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
@@ -12570,15 +12617,20 @@ intel_check_primary_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *intel_crtc_state;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
+	bool active;
 	int ret;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	intel_crtc_state = intel_crtc_state_for_plane(state);
+	active = intel_crtc_state->base.enable;
+
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
@@ -12587,7 +12639,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
-	if (intel_crtc->active) {
+	if (active) {
 		intel_crtc->atomic.wait_for_flips = true;
 
 		/*
@@ -12638,11 +12690,16 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *intel_crtc_state;
 	struct drm_rect *src = &state->src;
+	bool active;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	intel_crtc_state = intel_crtc_state_for_plane(state);
+	active = intel_crtc_state->base.enable;
+
 	plane->fb = fb;
 	crtc->x = src->x1 >> 16;
 	crtc->y = src->y1 >> 16;
@@ -12854,12 +12911,17 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *intel_crtc_state;
 	unsigned stride;
+	bool active;
 	int ret;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	intel_crtc_state = intel_crtc_state_for_plane(state);
+	active = intel_crtc_state->base.enable;
+
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
@@ -12892,7 +12954,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	}
 
 finish:
-	if (intel_crtc->active) {
+	if (active) {
 		if (plane->state->crtc_w != state->base.crtc_w)
 			intel_crtc->atomic.update_wm = true;
 
@@ -12910,12 +12972,17 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	struct drm_crtc *crtc = state->base.crtc;
 	struct drm_device *dev = plane->dev;
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *intel_crtc_state;
 	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
 	uint32_t addr;
+	bool active;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	intel_crtc_state = intel_crtc_state_for_plane(state);
+	active = intel_crtc_state->base.enable;
+
 	plane->fb = state->base.fb;
 	crtc->cursor_x = state->base.crtc_x;
 	crtc->cursor_y = state->base.crtc_y;
@@ -12934,7 +13001,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	intel_crtc->cursor_bo = obj;
 update:
 
-	if (intel_crtc->active)
+	if (active)
 		intel_crtc_update_cursor(crtc, state->visible);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index efa53d5..71e0152 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -998,6 +998,9 @@ intel_rotation_90_or_270(unsigned int rotation)
 
 bool intel_wm_need_update(struct drm_plane *plane,
 			  struct drm_plane_state *state);
+struct intel_crtc_state *
+intel_crtc_state_for_plane(struct intel_plane_state *pstate);
+
 
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index a4c0a04..2016e78 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -864,6 +864,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 			 struct intel_plane_state *state)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
+	struct intel_crtc_state *intel_crtc_state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	int crtc_x, crtc_y;
@@ -875,9 +876,13 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	int hscale, vscale;
 	int max_scale, min_scale;
 	int pixel_size;
+	bool active;
 
 	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
 
+	intel_crtc_state = intel_crtc_state_for_plane(state);
+	active = intel_crtc_state->base.enable;
+
 	if (!fb) {
 		state->visible = false;
 		goto finish;
@@ -1024,9 +1029,9 @@ finish:
 	 */
 	state->hides_primary = fb != NULL && drm_rect_equals(dst, clip) &&
 		!colorkey_enabled(intel_plane);
-	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
+	WARN_ON(state->hides_primary && !state->visible && active);
 
-	if (intel_crtc->active) {
+	if (active) {
 		if (intel_crtc->primary_enabled == state->hides_primary)
 			intel_crtc->atomic.wait_for_flips = true;
 
@@ -1062,18 +1067,23 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 {
 	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *intel_crtc_state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
+	bool active;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	intel_crtc_state = intel_crtc_state_for_plane(state);
+	active = intel_crtc_state->base.enable;
+
 	plane->fb = fb;
 
-	if (intel_crtc->active) {
+	if (active) {
 		intel_crtc->primary_enabled = !state->hides_primary;
 
 		if (state->visible) {
-- 
1.8.5.1

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

Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Daniel Vetter]

On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> Our atomic plane code currently uses intel_crtc->active to determine
> how/when to update some derived state values.  This works fine for pure
> plane updates at the moment since the CRTC state itself isn't changed as
> part of the operation.  However as we convert more of our driver
> internals over to atomic modesetting, we need to look at whether the
> CRTC will be active at the *end* of the atomic transaction (which may
> not match the currently committed state).
> 
> The in-flight value we want to use is generally in a crtc_state object
> associated with our top-level atomic transaction.  However there are a
> few cases where this isn't the case:
> 
>  * While using transitional atomic helpers (as we are at the moment),
>    SetPlane() calls will operate on orphaned plane states that aren't
>    part of a top-level atomic transaction.  In this case, we're not
>    touching the CRTC state, so it's fine to use the already-committed
>    value from crtc->state.
> 
>  * While updating properties of a disabled plane, we'll have a top-level
>    atomic state, but it may not contain the CRTC state we're looking
>    for.  Once again, this means we're not actually touching any CRTC
>    state so it's safe to use the value from crtc->state directly.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
>  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
>  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
>  4 files changed, 93 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 976b891..90c4a82 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  {
>  	struct drm_crtc *crtc = state->crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> +	active = intel_crtc_state->base.enable;
> +
>  	/*
>  	 * Both crtc and plane->crtc could be NULL if we're updating a
>  	 * property while the plane is disabled.  We don't actually have
> @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
> -	intel_state->clip.x2 =
> -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> -	intel_state->clip.y2 =
> -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;
>  
>  	/*
>  	 * Disabling a plane is always okay; we just need to update
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7bfe2af..88b0f69 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12562,6 +12562,53 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	}
>  }
>  
> +/**
> + * intel_crtc_state_for_plane - Obtain CRTC state for a plane
> + * @pstate: plane state to lookup corresponding crtc state for
> + *
> + * When working with a top-level atomic transaction (drm_atomic_state),
> + * a CRTC state should be present that corresponds to the provided
> + * plane state; this function provides a quick way to fetch that
> + * CRTC state.  In cases where we have a plane state unassociated with any
> + * top-level atomic transaction (e.g., while using the transitional atomic
> + * helpers), the current CRTC state from crtc->state will be returned
> + * instead.
> + */
> +struct intel_crtc_state *
> +intel_crtc_state_for_plane(struct intel_plane_state *pstate)
> +{
> +	struct drm_atomic_state *state = pstate->base.state;
> +	struct intel_plane *plane = to_intel_plane(pstate->base.plane);
> +	struct drm_crtc *crtc = intel_get_crtc_for_pipe(pstate->base.plane->dev,
> +							plane->pipe);
> +	int i;
> +
> +	/*
> +	 * While using transitional plane helpers, we may not have a top-level
> +	 * atomic transaction.  Of course that also means that we're not
> +	 * actually touching CRTC state, so just return the currently
> +	 * committed state.
> +	 */

Imo this needs a big FIXME at the top of the comment ;-)

> +	if (!state)
> +		return to_intel_crtc_state(crtc->state);
> +
> +	for (i = 0; i < state->dev->mode_config.num_crtc; i++) {
> +		if (!state->crtcs[i])
> +			continue;
> +
> +		if (to_intel_crtc(state->crtcs[i])->pipe == plane->pipe)
> +			return to_intel_crtc_state(state->crtc_states[i]);
> +	}
> +
> +	/*
> +	 * We may have a plane state without a corresponding CRTC state if
> +	 * we're updating a property of a disabled plane.  Again, just using
> +	 * the already-committed state for this plane's CRTC should be fine
> +	 * since we're not actually touching the CRTC here.
> +	 */

Is this really still true with Ander's patches? If the udpate is part of a
drm_atomic_state structure, then we should always have the corresponding
crtc state handy I think. Which cases still fail this assumption?

> +	return to_intel_crtc_state(crtc->state);
> +}
> +
>  static int
>  intel_check_primary_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
> @@ -12570,15 +12617,20 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> +	bool active;
>  	int ret;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;

This is the wrong one I think. drm_crtc_state->enable is the logical
enabling state, i.e. whether there's connectors connected to the crtc and
a mode set. state->active is the desired hw state, i.e. taking dpms and
all that into account, and hence reflecting our intel_crtc->active hw
tracking boolean (they match names intentionally).

Do we still miss cases where we update crtc_state->active or what's the
reason for picking ->enable here?
-Daniel

> +
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -12587,7 +12639,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> -	if (intel_crtc->active) {
> +	if (active) {
>  		intel_crtc->atomic.wait_for_flips = true;
>  
>  		/*
> @@ -12638,11 +12690,16 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_rect *src = &state->src;
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	plane->fb = fb;
>  	crtc->x = src->x1 >> 16;
>  	crtc->y = src->y1 >> 16;
> @@ -12854,12 +12911,17 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	const struct drm_rect *clip = &state->clip;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	unsigned stride;
> +	bool active;
>  	int ret;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -12892,7 +12954,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	}
>  
>  finish:
> -	if (intel_crtc->active) {
> +	if (active) {
>  		if (plane->state->crtc_w != state->base.crtc_w)
>  			intel_crtc->atomic.update_wm = true;
>  
> @@ -12910,12 +12972,17 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct drm_device *dev = plane->dev;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
>  	uint32_t addr;
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	plane->fb = state->base.fb;
>  	crtc->cursor_x = state->base.crtc_x;
>  	crtc->cursor_y = state->base.crtc_y;
> @@ -12934,7 +13001,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	intel_crtc->cursor_bo = obj;
>  update:
>  
> -	if (intel_crtc->active)
> +	if (active)
>  		intel_crtc_update_cursor(crtc, state->visible);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index efa53d5..71e0152 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -998,6 +998,9 @@ intel_rotation_90_or_270(unsigned int rotation)
>  
>  bool intel_wm_need_update(struct drm_plane *plane,
>  			  struct drm_plane_state *state);
> +struct intel_crtc_state *
> +intel_crtc_state_for_plane(struct intel_plane_state *pstate);
> +
>  
>  /* shared dpll functions */
>  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a4c0a04..2016e78 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -864,6 +864,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  			 struct intel_plane_state *state)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *fb = state->base.fb;
>  	int crtc_x, crtc_y;
> @@ -875,9 +876,13 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	int hscale, vscale;
>  	int max_scale, min_scale;
>  	int pixel_size;
> +	bool active;
>  
>  	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	if (!fb) {
>  		state->visible = false;
>  		goto finish;
> @@ -1024,9 +1029,9 @@ finish:
>  	 */
>  	state->hides_primary = fb != NULL && drm_rect_equals(dst, clip) &&
>  		!colorkey_enabled(intel_plane);
> -	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
> +	WARN_ON(state->hides_primary && !state->visible && active);
>  
> -	if (intel_crtc->active) {
> +	if (active) {
>  		if (intel_crtc->primary_enabled == state->hides_primary)
>  			intel_crtc->atomic.wait_for_flips = true;
>  
> @@ -1062,18 +1067,23 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  {
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *fb = state->base.fb;
>  	int crtc_x, crtc_y;
>  	unsigned int crtc_w, crtc_h;
>  	uint32_t src_x, src_y, src_w, src_h;
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	plane->fb = fb;
>  
> -	if (intel_crtc->active) {
> +	if (active) {
>  		intel_crtc->primary_enabled = !state->hides_primary;
>  
>  		if (state->visible) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Matt Roper]

On Thu, Apr 09, 2015 at 02:42:36PM +0200, Daniel Vetter wrote:
> On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> > Our atomic plane code currently uses intel_crtc->active to determine
> > how/when to update some derived state values.  This works fine for pure
> > plane updates at the moment since the CRTC state itself isn't changed as
> > part of the operation.  However as we convert more of our driver
> > internals over to atomic modesetting, we need to look at whether the
> > CRTC will be active at the *end* of the atomic transaction (which may
> > not match the currently committed state).
> > 
> > The in-flight value we want to use is generally in a crtc_state object
> > associated with our top-level atomic transaction.  However there are a
> > few cases where this isn't the case:
> > 
> >  * While using transitional atomic helpers (as we are at the moment),
> >    SetPlane() calls will operate on orphaned plane states that aren't
> >    part of a top-level atomic transaction.  In this case, we're not
> >    touching the CRTC state, so it's fine to use the already-committed
> >    value from crtc->state.
> > 
> >  * While updating properties of a disabled plane, we'll have a top-level
> >    atomic state, but it may not contain the CRTC state we're looking
> >    for.  Once again, this means we're not actually touching any CRTC
> >    state so it's safe to use the value from crtc->state directly.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
> >  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
> >  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
> >  4 files changed, 93 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 976b891..90c4a82 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >  {
> >  	struct drm_crtc *crtc = state->crtc;
> >  	struct intel_crtc *intel_crtc;
> > +	struct intel_crtc_state *intel_crtc_state;
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> > +	bool active;
> >  
> >  	crtc = crtc ? crtc : plane->crtc;
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> > +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> > +	active = intel_crtc_state->base.enable;
> > +
> >  	/*
> >  	 * Both crtc and plane->crtc could be NULL if we're updating a
> >  	 * property while the plane is disabled.  We don't actually have
> > @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> >  	intel_state->clip.x1 = 0;
> >  	intel_state->clip.y1 = 0;
> > -	intel_state->clip.x2 =
> > -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> > -	intel_state->clip.y2 =
> > -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> > +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> > +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;
> >  
> >  	/*
> >  	 * Disabling a plane is always okay; we just need to update
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7bfe2af..88b0f69 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12562,6 +12562,53 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> >  	}
> >  }
> >  
> > +/**
> > + * intel_crtc_state_for_plane - Obtain CRTC state for a plane
> > + * @pstate: plane state to lookup corresponding crtc state for
> > + *
> > + * When working with a top-level atomic transaction (drm_atomic_state),
> > + * a CRTC state should be present that corresponds to the provided
> > + * plane state; this function provides a quick way to fetch that
> > + * CRTC state.  In cases where we have a plane state unassociated with any
> > + * top-level atomic transaction (e.g., while using the transitional atomic
> > + * helpers), the current CRTC state from crtc->state will be returned
> > + * instead.
> > + */
> > +struct intel_crtc_state *
> > +intel_crtc_state_for_plane(struct intel_plane_state *pstate)
> > +{
> > +	struct drm_atomic_state *state = pstate->base.state;
> > +	struct intel_plane *plane = to_intel_plane(pstate->base.plane);
> > +	struct drm_crtc *crtc = intel_get_crtc_for_pipe(pstate->base.plane->dev,
> > +							plane->pipe);
> > +	int i;
> > +
> > +	/*
> > +	 * While using transitional plane helpers, we may not have a top-level
> > +	 * atomic transaction.  Of course that also means that we're not
> > +	 * actually touching CRTC state, so just return the currently
> > +	 * committed state.
> > +	 */
> 
> Imo this needs a big FIXME at the top of the comment ;-)
> 
> > +	if (!state)
> > +		return to_intel_crtc_state(crtc->state);
> > +
> > +	for (i = 0; i < state->dev->mode_config.num_crtc; i++) {
> > +		if (!state->crtcs[i])
> > +			continue;
> > +
> > +		if (to_intel_crtc(state->crtcs[i])->pipe == plane->pipe)
> > +			return to_intel_crtc_state(state->crtc_states[i]);
> > +	}
> > +
> > +	/*
> > +	 * We may have a plane state without a corresponding CRTC state if
> > +	 * we're updating a property of a disabled plane.  Again, just using
> > +	 * the already-committed state for this plane's CRTC should be fine
> > +	 * since we're not actually touching the CRTC here.
> > +	 */
> 
> Is this really still true with Ander's patches? If the udpate is part of a
> drm_atomic_state structure, then we should always have the corresponding
> crtc state handy I think. Which cases still fail this assumption?

Any time a transaction updates a plane, the corresponding CRTC state (as
defined by plane_state->crtc) should get added to the transaction by the
atomic core code (specifically in drm_atomic_get_plane_state).  But when
a plane is disabled, state->crtc is NULL so there simply is no
associated CRTC to add (at least as far as the core is concerned).  So
you wind up with just a plane state being added to the top-level atomic
state in that case.


Matt

> 
> > +	return to_intel_crtc_state(crtc->state);
> > +}
> > +
> >  static int
> >  intel_check_primary_plane(struct drm_plane *plane,
> >  			  struct intel_plane_state *state)
> > @@ -12570,15 +12617,20 @@ intel_check_primary_plane(struct drm_plane *plane,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc = state->base.crtc;
> >  	struct intel_crtc *intel_crtc;
> > +	struct intel_crtc_state *intel_crtc_state;
> >  	struct drm_framebuffer *fb = state->base.fb;
> >  	struct drm_rect *dest = &state->dst;
> >  	struct drm_rect *src = &state->src;
> >  	const struct drm_rect *clip = &state->clip;
> > +	bool active;
> >  	int ret;
> >  
> >  	crtc = crtc ? crtc : plane->crtc;
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> > +	intel_crtc_state = intel_crtc_state_for_plane(state);
> > +	active = intel_crtc_state->base.enable;
> 
> This is the wrong one I think. drm_crtc_state->enable is the logical
> enabling state, i.e. whether there's connectors connected to the crtc and
> a mode set. state->active is the desired hw state, i.e. taking dpms and
> all that into account, and hence reflecting our intel_crtc->active hw
> tracking boolean (they match names intentionally).
> 
> Do we still miss cases where we update crtc_state->active or what's the
> reason for picking ->enable here?

I'll have to double check; I thought this was a little fishy, but it
looked like our i915 code wasn't actually setting ->active anywhere yet
in our legacy modeset codepaths (although maybe I just missed it).  We
do set ->enable directly to intel_crtc->active in a few places though,
which is why I went with that one for now.


Matt

> -Daniel
> 

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

Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Daniel Vetter]

On Thu, Apr 09, 2015 at 07:00:31AM -0700, Matt Roper wrote:
> On Thu, Apr 09, 2015 at 02:42:36PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> > > Our atomic plane code currently uses intel_crtc->active to determine
> > > how/when to update some derived state values.  This works fine for pure
> > > plane updates at the moment since the CRTC state itself isn't changed as
> > > part of the operation.  However as we convert more of our driver
> > > internals over to atomic modesetting, we need to look at whether the
> > > CRTC will be active at the *end* of the atomic transaction (which may
> > > not match the currently committed state).
> > > 
> > > The in-flight value we want to use is generally in a crtc_state object
> > > associated with our top-level atomic transaction.  However there are a
> > > few cases where this isn't the case:
> > > 
> > >  * While using transitional atomic helpers (as we are at the moment),
> > >    SetPlane() calls will operate on orphaned plane states that aren't
> > >    part of a top-level atomic transaction.  In this case, we're not
> > >    touching the CRTC state, so it's fine to use the already-committed
> > >    value from crtc->state.
> > > 
> > >  * While updating properties of a disabled plane, we'll have a top-level
> > >    atomic state, but it may not contain the CRTC state we're looking
> > >    for.  Once again, this means we're not actually touching any CRTC
> > >    state so it's safe to use the value from crtc->state directly.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
> > >  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
> > >  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
> > >  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
> > >  4 files changed, 93 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > index 976b891..90c4a82 100644
> > > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > >  {
> > >  	struct drm_crtc *crtc = state->crtc;
> > >  	struct intel_crtc *intel_crtc;
> > > +	struct intel_crtc_state *intel_crtc_state;
> > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> > > +	bool active;
> > >  
> > >  	crtc = crtc ? crtc : plane->crtc;
> > >  	intel_crtc = to_intel_crtc(crtc);
> > >  
> > > +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> > > +	active = intel_crtc_state->base.enable;
> > > +
> > >  	/*
> > >  	 * Both crtc and plane->crtc could be NULL if we're updating a
> > >  	 * property while the plane is disabled.  We don't actually have
> > > @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > >  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> > >  	intel_state->clip.x1 = 0;
> > >  	intel_state->clip.y1 = 0;
> > > -	intel_state->clip.x2 =
> > > -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> > > -	intel_state->clip.y2 =
> > > -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> > > +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> > > +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;
> > >  
> > >  	/*
> > >  	 * Disabling a plane is always okay; we just need to update
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 7bfe2af..88b0f69 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12562,6 +12562,53 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> > >  	}
> > >  }
> > >  
> > > +/**
> > > + * intel_crtc_state_for_plane - Obtain CRTC state for a plane
> > > + * @pstate: plane state to lookup corresponding crtc state for
> > > + *
> > > + * When working with a top-level atomic transaction (drm_atomic_state),
> > > + * a CRTC state should be present that corresponds to the provided
> > > + * plane state; this function provides a quick way to fetch that
> > > + * CRTC state.  In cases where we have a plane state unassociated with any
> > > + * top-level atomic transaction (e.g., while using the transitional atomic
> > > + * helpers), the current CRTC state from crtc->state will be returned
> > > + * instead.
> > > + */
> > > +struct intel_crtc_state *
> > > +intel_crtc_state_for_plane(struct intel_plane_state *pstate)
> > > +{
> > > +	struct drm_atomic_state *state = pstate->base.state;
> > > +	struct intel_plane *plane = to_intel_plane(pstate->base.plane);
> > > +	struct drm_crtc *crtc = intel_get_crtc_for_pipe(pstate->base.plane->dev,
> > > +							plane->pipe);
> > > +	int i;
> > > +
> > > +	/*
> > > +	 * While using transitional plane helpers, we may not have a top-level
> > > +	 * atomic transaction.  Of course that also means that we're not
> > > +	 * actually touching CRTC state, so just return the currently
> > > +	 * committed state.
> > > +	 */
> > 
> > Imo this needs a big FIXME at the top of the comment ;-)
> > 
> > > +	if (!state)
> > > +		return to_intel_crtc_state(crtc->state);
> > > +
> > > +	for (i = 0; i < state->dev->mode_config.num_crtc; i++) {
> > > +		if (!state->crtcs[i])
> > > +			continue;
> > > +
> > > +		if (to_intel_crtc(state->crtcs[i])->pipe == plane->pipe)
> > > +			return to_intel_crtc_state(state->crtc_states[i]);
> > > +	}
> > > +
> > > +	/*
> > > +	 * We may have a plane state without a corresponding CRTC state if
> > > +	 * we're updating a property of a disabled plane.  Again, just using
> > > +	 * the already-committed state for this plane's CRTC should be fine
> > > +	 * since we're not actually touching the CRTC here.
> > > +	 */
> > 
> > Is this really still true with Ander's patches? If the udpate is part of a
> > drm_atomic_state structure, then we should always have the corresponding
> > crtc state handy I think. Which cases still fail this assumption?
> 
> Any time a transaction updates a plane, the corresponding CRTC state (as
> defined by plane_state->crtc) should get added to the transaction by the
> atomic core code (specifically in drm_atomic_get_plane_state).  But when
> a plane is disabled, state->crtc is NULL so there simply is no
> associated CRTC to add (at least as far as the core is concerned).  So
> you wind up with just a plane state being added to the top-level atomic
> state in that case.

We add both the old and the new crtc state, so when you disable a plane
you should have the crtc state at hand. Except when a plane is already
disable, but I guess in that case we should just not call any of the
callbacks?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Matt Roper]

On Thu, Apr 09, 2015 at 04:45:04PM +0200, Daniel Vetter wrote:
> On Thu, Apr 09, 2015 at 07:00:31AM -0700, Matt Roper wrote:
> > On Thu, Apr 09, 2015 at 02:42:36PM +0200, Daniel Vetter wrote:
> > > On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> > > > Our atomic plane code currently uses intel_crtc->active to determine
> > > > how/when to update some derived state values.  This works fine for pure
> > > > plane updates at the moment since the CRTC state itself isn't changed as
> > > > part of the operation.  However as we convert more of our driver
> > > > internals over to atomic modesetting, we need to look at whether the
> > > > CRTC will be active at the *end* of the atomic transaction (which may
> > > > not match the currently committed state).
> > > > 
> > > > The in-flight value we want to use is generally in a crtc_state object
> > > > associated with our top-level atomic transaction.  However there are a
> > > > few cases where this isn't the case:
> > > > 
> > > >  * While using transitional atomic helpers (as we are at the moment),
> > > >    SetPlane() calls will operate on orphaned plane states that aren't
> > > >    part of a top-level atomic transaction.  In this case, we're not
> > > >    touching the CRTC state, so it's fine to use the already-committed
> > > >    value from crtc->state.
> > > > 
> > > >  * While updating properties of a disabled plane, we'll have a top-level
> > > >    atomic state, but it may not contain the CRTC state we're looking
> > > >    for.  Once again, this means we're not actually touching any CRTC
> > > >    state so it's safe to use the value from crtc->state directly.
> > > > 
> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
> > > >  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
> > > >  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
> > > >  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
> > > >  4 files changed, 93 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > index 976b891..90c4a82 100644
> > > > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > > >  {
> > > >  	struct drm_crtc *crtc = state->crtc;
> > > >  	struct intel_crtc *intel_crtc;
> > > > +	struct intel_crtc_state *intel_crtc_state;
> > > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> > > > +	bool active;
> > > >  
> > > >  	crtc = crtc ? crtc : plane->crtc;
> > > >  	intel_crtc = to_intel_crtc(crtc);
> > > >  
> > > > +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> > > > +	active = intel_crtc_state->base.enable;
> > > > +
> > > >  	/*
> > > >  	 * Both crtc and plane->crtc could be NULL if we're updating a
> > > >  	 * property while the plane is disabled.  We don't actually have
> > > > @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > > >  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> > > >  	intel_state->clip.x1 = 0;
> > > >  	intel_state->clip.y1 = 0;
> > > > -	intel_state->clip.x2 =
> > > > -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> > > > -	intel_state->clip.y2 =
> > > > -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> > > > +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> > > > +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;
> > > >  
> > > >  	/*
> > > >  	 * Disabling a plane is always okay; we just need to update
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 7bfe2af..88b0f69 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -12562,6 +12562,53 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> > > >  	}
> > > >  }
> > > >  
> > > > +/**
> > > > + * intel_crtc_state_for_plane - Obtain CRTC state for a plane
> > > > + * @pstate: plane state to lookup corresponding crtc state for
> > > > + *
> > > > + * When working with a top-level atomic transaction (drm_atomic_state),
> > > > + * a CRTC state should be present that corresponds to the provided
> > > > + * plane state; this function provides a quick way to fetch that
> > > > + * CRTC state.  In cases where we have a plane state unassociated with any
> > > > + * top-level atomic transaction (e.g., while using the transitional atomic
> > > > + * helpers), the current CRTC state from crtc->state will be returned
> > > > + * instead.
> > > > + */
> > > > +struct intel_crtc_state *
> > > > +intel_crtc_state_for_plane(struct intel_plane_state *pstate)
> > > > +{
> > > > +	struct drm_atomic_state *state = pstate->base.state;
> > > > +	struct intel_plane *plane = to_intel_plane(pstate->base.plane);
> > > > +	struct drm_crtc *crtc = intel_get_crtc_for_pipe(pstate->base.plane->dev,
> > > > +							plane->pipe);
> > > > +	int i;
> > > > +
> > > > +	/*
> > > > +	 * While using transitional plane helpers, we may not have a top-level
> > > > +	 * atomic transaction.  Of course that also means that we're not
> > > > +	 * actually touching CRTC state, so just return the currently
> > > > +	 * committed state.
> > > > +	 */
> > > 
> > > Imo this needs a big FIXME at the top of the comment ;-)
> > > 
> > > > +	if (!state)
> > > > +		return to_intel_crtc_state(crtc->state);
> > > > +
> > > > +	for (i = 0; i < state->dev->mode_config.num_crtc; i++) {
> > > > +		if (!state->crtcs[i])
> > > > +			continue;
> > > > +
> > > > +		if (to_intel_crtc(state->crtcs[i])->pipe == plane->pipe)
> > > > +			return to_intel_crtc_state(state->crtc_states[i]);
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * We may have a plane state without a corresponding CRTC state if
> > > > +	 * we're updating a property of a disabled plane.  Again, just using
> > > > +	 * the already-committed state for this plane's CRTC should be fine
> > > > +	 * since we're not actually touching the CRTC here.
> > > > +	 */
> > > 
> > > Is this really still true with Ander's patches? If the udpate is part of a
> > > drm_atomic_state structure, then we should always have the corresponding
> > > crtc state handy I think. Which cases still fail this assumption?
> > 
> > Any time a transaction updates a plane, the corresponding CRTC state (as
> > defined by plane_state->crtc) should get added to the transaction by the
> > atomic core code (specifically in drm_atomic_get_plane_state).  But when
> > a plane is disabled, state->crtc is NULL so there simply is no
> > associated CRTC to add (at least as far as the core is concerned).  So
> > you wind up with just a plane state being added to the top-level atomic
> > state in that case.
> 
> We add both the old and the new crtc state, so when you disable a plane
> you should have the crtc state at hand. Except when a plane is already
> disable, but I guess in that case we should just not call any of the
> callbacks?
> -Daniel

Right, if both old and new state both have NULL CRTC's, then no state
gets added.  That typically happens when you update a property of a
disabled plane that stays disabled (which is easily seen with fbdev
restoration resetting rotation for all planes, even the disabled ones).

The driver's check function still gets called in this case, but i915
just bails out because we don't have any driver-specific checking that
needs to be done.


Matt

> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Ville Syrjälä]

On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> Our atomic plane code currently uses intel_crtc->active to determine
> how/when to update some derived state values.  This works fine for pure
> plane updates at the moment since the CRTC state itself isn't changed as
> part of the operation.  However as we convert more of our driver
> internals over to atomic modesetting, we need to look at whether the
> CRTC will be active at the *end* of the atomic transaction (which may
> not match the currently committed state).
> 
> The in-flight value we want to use is generally in a crtc_state object
> associated with our top-level atomic transaction.  However there are a
> few cases where this isn't the case:
> 
>  * While using transitional atomic helpers (as we are at the moment),
>    SetPlane() calls will operate on orphaned plane states that aren't
>    part of a top-level atomic transaction.  In this case, we're not
>    touching the CRTC state, so it's fine to use the already-committed
>    value from crtc->state.
> 
>  * While updating properties of a disabled plane, we'll have a top-level
>    atomic state, but it may not contain the CRTC state we're looking
>    for.  Once again, this means we're not actually touching any CRTC
>    state so it's safe to use the value from crtc->state directly.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
>  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
>  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
>  4 files changed, 93 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 976b891..90c4a82 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  {
>  	struct drm_crtc *crtc = state->crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> +	active = intel_crtc_state->base.enable;
> +
>  	/*
>  	 * Both crtc and plane->crtc could be NULL if we're updating a
>  	 * property while the plane is disabled.  We don't actually have
> @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
> -	intel_state->clip.x2 =
> -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> -	intel_state->clip.y2 =
> -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;

We depend on the clipping to keep planes from getting enabled on a
disabled pipe. So I think this is going to blow up.

>  
>  	/*
>  	 * Disabling a plane is always okay; we just need to update
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7bfe2af..88b0f69 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12562,6 +12562,53 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	}
>  }
>  
> +/**
> + * intel_crtc_state_for_plane - Obtain CRTC state for a plane
> + * @pstate: plane state to lookup corresponding crtc state for
> + *
> + * When working with a top-level atomic transaction (drm_atomic_state),
> + * a CRTC state should be present that corresponds to the provided
> + * plane state; this function provides a quick way to fetch that
> + * CRTC state.  In cases where we have a plane state unassociated with any
> + * top-level atomic transaction (e.g., while using the transitional atomic
> + * helpers), the current CRTC state from crtc->state will be returned
> + * instead.
> + */
> +struct intel_crtc_state *
> +intel_crtc_state_for_plane(struct intel_plane_state *pstate)
> +{
> +	struct drm_atomic_state *state = pstate->base.state;
> +	struct intel_plane *plane = to_intel_plane(pstate->base.plane);
> +	struct drm_crtc *crtc = intel_get_crtc_for_pipe(pstate->base.plane->dev,
> +							plane->pipe);
> +	int i;
> +
> +	/*
> +	 * While using transitional plane helpers, we may not have a top-level
> +	 * atomic transaction.  Of course that also means that we're not
> +	 * actually touching CRTC state, so just return the currently
> +	 * committed state.
> +	 */
> +	if (!state)
> +		return to_intel_crtc_state(crtc->state);
> +
> +	for (i = 0; i < state->dev->mode_config.num_crtc; i++) {
> +		if (!state->crtcs[i])
> +			continue;
> +
> +		if (to_intel_crtc(state->crtcs[i])->pipe == plane->pipe)
> +			return to_intel_crtc_state(state->crtc_states[i]);
> +	}
> +
> +	/*
> +	 * We may have a plane state without a corresponding CRTC state if
> +	 * we're updating a property of a disabled plane.  Again, just using
> +	 * the already-committed state for this plane's CRTC should be fine
> +	 * since we're not actually touching the CRTC here.
> +	 */
> +	return to_intel_crtc_state(crtc->state);
> +}
> +
>  static int
>  intel_check_primary_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
> @@ -12570,15 +12617,20 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> +	bool active;
>  	int ret;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -12587,7 +12639,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> -	if (intel_crtc->active) {
> +	if (active) {
>  		intel_crtc->atomic.wait_for_flips = true;
>  
>  		/*
> @@ -12638,11 +12690,16 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_rect *src = &state->src;
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	plane->fb = fb;
>  	crtc->x = src->x1 >> 16;
>  	crtc->y = src->y1 >> 16;
> @@ -12854,12 +12911,17 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	const struct drm_rect *clip = &state->clip;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	unsigned stride;
> +	bool active;
>  	int ret;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -12892,7 +12954,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	}
>  
>  finish:
> -	if (intel_crtc->active) {
> +	if (active) {
>  		if (plane->state->crtc_w != state->base.crtc_w)
>  			intel_crtc->atomic.update_wm = true;
>  
> @@ -12910,12 +12972,17 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct drm_device *dev = plane->dev;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
>  	uint32_t addr;
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	plane->fb = state->base.fb;
>  	crtc->cursor_x = state->base.crtc_x;
>  	crtc->cursor_y = state->base.crtc_y;
> @@ -12934,7 +13001,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	intel_crtc->cursor_bo = obj;
>  update:
>  
> -	if (intel_crtc->active)
> +	if (active)
>  		intel_crtc_update_cursor(crtc, state->visible);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index efa53d5..71e0152 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -998,6 +998,9 @@ intel_rotation_90_or_270(unsigned int rotation)
>  
>  bool intel_wm_need_update(struct drm_plane *plane,
>  			  struct drm_plane_state *state);
> +struct intel_crtc_state *
> +intel_crtc_state_for_plane(struct intel_plane_state *pstate);
> +
>  
>  /* shared dpll functions */
>  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a4c0a04..2016e78 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -864,6 +864,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  			 struct intel_plane_state *state)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *fb = state->base.fb;
>  	int crtc_x, crtc_y;
> @@ -875,9 +876,13 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	int hscale, vscale;
>  	int max_scale, min_scale;
>  	int pixel_size;
> +	bool active;
>  
>  	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	if (!fb) {
>  		state->visible = false;
>  		goto finish;
> @@ -1024,9 +1029,9 @@ finish:
>  	 */
>  	state->hides_primary = fb != NULL && drm_rect_equals(dst, clip) &&
>  		!colorkey_enabled(intel_plane);
> -	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
> +	WARN_ON(state->hides_primary && !state->visible && active);
>  
> -	if (intel_crtc->active) {
> +	if (active) {
>  		if (intel_crtc->primary_enabled == state->hides_primary)
>  			intel_crtc->atomic.wait_for_flips = true;
>  
> @@ -1062,18 +1067,23 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  {
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *fb = state->base.fb;
>  	int crtc_x, crtc_y;
>  	unsigned int crtc_w, crtc_h;
>  	uint32_t src_x, src_y, src_w, src_h;
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	plane->fb = fb;
>  
> -	if (intel_crtc->active) {
> +	if (active) {
>  		intel_crtc->primary_enabled = !state->hides_primary;
>  
>  		if (state->visible) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Matt Roper]

On Thu, Apr 09, 2015 at 04:18:56PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> > Our atomic plane code currently uses intel_crtc->active to determine
> > how/when to update some derived state values.  This works fine for pure
> > plane updates at the moment since the CRTC state itself isn't changed as
> > part of the operation.  However as we convert more of our driver
> > internals over to atomic modesetting, we need to look at whether the
> > CRTC will be active at the *end* of the atomic transaction (which may
> > not match the currently committed state).
> > 
> > The in-flight value we want to use is generally in a crtc_state object
> > associated with our top-level atomic transaction.  However there are a
> > few cases where this isn't the case:
> > 
> >  * While using transitional atomic helpers (as we are at the moment),
> >    SetPlane() calls will operate on orphaned plane states that aren't
> >    part of a top-level atomic transaction.  In this case, we're not
> >    touching the CRTC state, so it's fine to use the already-committed
> >    value from crtc->state.
> > 
> >  * While updating properties of a disabled plane, we'll have a top-level
> >    atomic state, but it may not contain the CRTC state we're looking
> >    for.  Once again, this means we're not actually touching any CRTC
> >    state so it's safe to use the value from crtc->state directly.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
> >  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
> >  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
> >  4 files changed, 93 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 976b891..90c4a82 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >  {
> >  	struct drm_crtc *crtc = state->crtc;
> >  	struct intel_crtc *intel_crtc;
> > +	struct intel_crtc_state *intel_crtc_state;
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> > +	bool active;
> >  
> >  	crtc = crtc ? crtc : plane->crtc;
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> > +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> > +	active = intel_crtc_state->base.enable;
> > +
> >  	/*
> >  	 * Both crtc and plane->crtc could be NULL if we're updating a
> >  	 * property while the plane is disabled.  We don't actually have
> > @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> >  	intel_state->clip.x1 = 0;
> >  	intel_state->clip.y1 = 0;
> > -	intel_state->clip.x2 =
> > -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> > -	intel_state->clip.y2 =
> > -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> > +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> > +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;
> 
> We depend on the clipping to keep planes from getting enabled on a
> disabled pipe. So I think this is going to blow up.

That was why I made these changes...the idea here was that we should be
basing that clipping on what the CRTC state is going to be when the
plane state is actually committed, not what it happens to be now.  So if
the CRTC is going to be disabled, this should ensure that the planes are
properly clipped to off, even if the CRTC happens to be running at the
moment.  Conversely, if the CRTC is off at the moment, but will be on at
the end of this transaction, we want to make sure that the planes are
not improperly clipped to invisible, otherwise they won't show up.

Am I overlooking something?  It seems the current 'active' value is
unrelated to what we actually want to be basing our plane programming on
and just happens to work at the moment since we don't yet let  ->active
change over the course of an atomic transaction (meaning the final state
will always be the same as the current state).


Matt

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

Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Ville Syrjälä]

On Thu, Apr 09, 2015 at 07:10:57AM -0700, Matt Roper wrote:
> On Thu, Apr 09, 2015 at 04:18:56PM +0300, Ville Syrjälä wrote:
> > On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> > > Our atomic plane code currently uses intel_crtc->active to determine
> > > how/when to update some derived state values.  This works fine for pure
> > > plane updates at the moment since the CRTC state itself isn't changed as
> > > part of the operation.  However as we convert more of our driver
> > > internals over to atomic modesetting, we need to look at whether the
> > > CRTC will be active at the *end* of the atomic transaction (which may
> > > not match the currently committed state).
> > > 
> > > The in-flight value we want to use is generally in a crtc_state object
> > > associated with our top-level atomic transaction.  However there are a
> > > few cases where this isn't the case:
> > > 
> > >  * While using transitional atomic helpers (as we are at the moment),
> > >    SetPlane() calls will operate on orphaned plane states that aren't
> > >    part of a top-level atomic transaction.  In this case, we're not
> > >    touching the CRTC state, so it's fine to use the already-committed
> > >    value from crtc->state.
> > > 
> > >  * While updating properties of a disabled plane, we'll have a top-level
> > >    atomic state, but it may not contain the CRTC state we're looking
> > >    for.  Once again, this means we're not actually touching any CRTC
> > >    state so it's safe to use the value from crtc->state directly.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
> > >  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
> > >  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
> > >  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
> > >  4 files changed, 93 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > index 976b891..90c4a82 100644
> > > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > >  {
> > >  	struct drm_crtc *crtc = state->crtc;
> > >  	struct intel_crtc *intel_crtc;
> > > +	struct intel_crtc_state *intel_crtc_state;
> > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> > > +	bool active;
> > >  
> > >  	crtc = crtc ? crtc : plane->crtc;
> > >  	intel_crtc = to_intel_crtc(crtc);
> > >  
> > > +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> > > +	active = intel_crtc_state->base.enable;
> > > +
> > >  	/*
> > >  	 * Both crtc and plane->crtc could be NULL if we're updating a
> > >  	 * property while the plane is disabled.  We don't actually have
> > > @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > >  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> > >  	intel_state->clip.x1 = 0;
> > >  	intel_state->clip.y1 = 0;
> > > -	intel_state->clip.x2 =
> > > -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> > > -	intel_state->clip.y2 =
> > > -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> > > +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> > > +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;
> > 
> > We depend on the clipping to keep planes from getting enabled on a
> > disabled pipe. So I think this is going to blow up.
> 
> That was why I made these changes...the idea here was that we should be
> basing that clipping on what the CRTC state is going to be when the
> plane state is actually committed, not what it happens to be now.  So if
> the CRTC is going to be disabled, this should ensure that the planes are
> properly clipped to off, even if the CRTC happens to be running at the
> moment.  Conversely, if the CRTC is off at the moment, but will be on at
> the end of this transaction, we want to make sure that the planes are
> not improperly clipped to invisible, otherwise they won't show up.

Well yeah, we want to change the clipping computation to use the future
state but I don't think were ready for it yet. At least I wouldn't want
to make this change until the watermark code gets fixed and we clean up
the primary/cursor plane state handling.

For instance what happens if you dpms off and then enable a plane? If
the clipping is based on the enabled state of the crtc then it'll try
to enable the plane, no?

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

Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Matt Roper]

On Thu, Apr 09, 2015 at 05:46:30PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 09, 2015 at 07:10:57AM -0700, Matt Roper wrote:
> > On Thu, Apr 09, 2015 at 04:18:56PM +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> > > > Our atomic plane code currently uses intel_crtc->active to determine
> > > > how/when to update some derived state values.  This works fine for pure
> > > > plane updates at the moment since the CRTC state itself isn't changed as
> > > > part of the operation.  However as we convert more of our driver
> > > > internals over to atomic modesetting, we need to look at whether the
> > > > CRTC will be active at the *end* of the atomic transaction (which may
> > > > not match the currently committed state).
> > > > 
> > > > The in-flight value we want to use is generally in a crtc_state object
> > > > associated with our top-level atomic transaction.  However there are a
> > > > few cases where this isn't the case:
> > > > 
> > > >  * While using transitional atomic helpers (as we are at the moment),
> > > >    SetPlane() calls will operate on orphaned plane states that aren't
> > > >    part of a top-level atomic transaction.  In this case, we're not
> > > >    touching the CRTC state, so it's fine to use the already-committed
> > > >    value from crtc->state.
> > > > 
> > > >  * While updating properties of a disabled plane, we'll have a top-level
> > > >    atomic state, but it may not contain the CRTC state we're looking
> > > >    for.  Once again, this means we're not actually touching any CRTC
> > > >    state so it's safe to use the value from crtc->state directly.
> > > > 
> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
> > > >  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
> > > >  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
> > > >  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
> > > >  4 files changed, 93 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > index 976b891..90c4a82 100644
> > > > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > > >  {
> > > >  	struct drm_crtc *crtc = state->crtc;
> > > >  	struct intel_crtc *intel_crtc;
> > > > +	struct intel_crtc_state *intel_crtc_state;
> > > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> > > > +	bool active;
> > > >  
> > > >  	crtc = crtc ? crtc : plane->crtc;
> > > >  	intel_crtc = to_intel_crtc(crtc);
> > > >  
> > > > +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> > > > +	active = intel_crtc_state->base.enable;
> > > > +
> > > >  	/*
> > > >  	 * Both crtc and plane->crtc could be NULL if we're updating a
> > > >  	 * property while the plane is disabled.  We don't actually have
> > > > @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > > >  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> > > >  	intel_state->clip.x1 = 0;
> > > >  	intel_state->clip.y1 = 0;
> > > > -	intel_state->clip.x2 =
> > > > -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> > > > -	intel_state->clip.y2 =
> > > > -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> > > > +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> > > > +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;
> > > 
> > > We depend on the clipping to keep planes from getting enabled on a
> > > disabled pipe. So I think this is going to blow up.
> > 
> > That was why I made these changes...the idea here was that we should be
> > basing that clipping on what the CRTC state is going to be when the
> > plane state is actually committed, not what it happens to be now.  So if
> > the CRTC is going to be disabled, this should ensure that the planes are
> > properly clipped to off, even if the CRTC happens to be running at the
> > moment.  Conversely, if the CRTC is off at the moment, but will be on at
> > the end of this transaction, we want to make sure that the planes are
> > not improperly clipped to invisible, otherwise they won't show up.
> 
> Well yeah, we want to change the clipping computation to use the future
> state but I don't think were ready for it yet. At least I wouldn't want
> to make this change until the watermark code gets fixed and we clean up
> the primary/cursor plane state handling.
> 
> For instance what happens if you dpms off and then enable a plane? If
> the clipping is based on the enabled state of the crtc then it'll try
> to enable the plane, no?

Yeah, I think that was Daniel's concern too... ->enable is the wrong
field to be pulling out of the state object here and we should be using
->active instead since that takes DPMS and such into account.

I'm not sure if ->active is properly updated in our CRTC state today by
the legacy modesetting codepaths, but if we can get that squared away
and switch to using crtc_state->active rather than crtc_state->enable do
you forsee any other issues with pulling the value out of the in-flight
state like this patch tries to do?


Matt

> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Daniel Vetter]

On Thu, Apr 09, 2015 at 07:54:14AM -0700, Matt Roper wrote:
> On Thu, Apr 09, 2015 at 05:46:30PM +0300, Ville Syrjälä wrote:
> > On Thu, Apr 09, 2015 at 07:10:57AM -0700, Matt Roper wrote:
> > > On Thu, Apr 09, 2015 at 04:18:56PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> > > > > Our atomic plane code currently uses intel_crtc->active to determine
> > > > > how/when to update some derived state values.  This works fine for pure
> > > > > plane updates at the moment since the CRTC state itself isn't changed as
> > > > > part of the operation.  However as we convert more of our driver
> > > > > internals over to atomic modesetting, we need to look at whether the
> > > > > CRTC will be active at the *end* of the atomic transaction (which may
> > > > > not match the currently committed state).
> > > > > 
> > > > > The in-flight value we want to use is generally in a crtc_state object
> > > > > associated with our top-level atomic transaction.  However there are a
> > > > > few cases where this isn't the case:
> > > > > 
> > > > >  * While using transitional atomic helpers (as we are at the moment),
> > > > >    SetPlane() calls will operate on orphaned plane states that aren't
> > > > >    part of a top-level atomic transaction.  In this case, we're not
> > > > >    touching the CRTC state, so it's fine to use the already-committed
> > > > >    value from crtc->state.
> > > > > 
> > > > >  * While updating properties of a disabled plane, we'll have a top-level
> > > > >    atomic state, but it may not contain the CRTC state we're looking
> > > > >    for.  Once again, this means we're not actually touching any CRTC
> > > > >    state so it's safe to use the value from crtc->state directly.
> > > > > 
> > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
> > > > >  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
> > > > >  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
> > > > >  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
> > > > >  4 files changed, 93 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > > index 976b891..90c4a82 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > > @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > > > >  {
> > > > >  	struct drm_crtc *crtc = state->crtc;
> > > > >  	struct intel_crtc *intel_crtc;
> > > > > +	struct intel_crtc_state *intel_crtc_state;
> > > > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> > > > > +	bool active;
> > > > >  
> > > > >  	crtc = crtc ? crtc : plane->crtc;
> > > > >  	intel_crtc = to_intel_crtc(crtc);
> > > > >  
> > > > > +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> > > > > +	active = intel_crtc_state->base.enable;
> > > > > +
> > > > >  	/*
> > > > >  	 * Both crtc and plane->crtc could be NULL if we're updating a
> > > > >  	 * property while the plane is disabled.  We don't actually have
> > > > > @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > > > >  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> > > > >  	intel_state->clip.x1 = 0;
> > > > >  	intel_state->clip.y1 = 0;
> > > > > -	intel_state->clip.x2 =
> > > > > -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> > > > > -	intel_state->clip.y2 =
> > > > > -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> > > > > +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> > > > > +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;
> > > > 
> > > > We depend on the clipping to keep planes from getting enabled on a
> > > > disabled pipe. So I think this is going to blow up.
> > > 
> > > That was why I made these changes...the idea here was that we should be
> > > basing that clipping on what the CRTC state is going to be when the
> > > plane state is actually committed, not what it happens to be now.  So if
> > > the CRTC is going to be disabled, this should ensure that the planes are
> > > properly clipped to off, even if the CRTC happens to be running at the
> > > moment.  Conversely, if the CRTC is off at the moment, but will be on at
> > > the end of this transaction, we want to make sure that the planes are
> > > not improperly clipped to invisible, otherwise they won't show up.
> > 
> > Well yeah, we want to change the clipping computation to use the future
> > state but I don't think were ready for it yet. At least I wouldn't want
> > to make this change until the watermark code gets fixed and we clean up
> > the primary/cursor plane state handling.
> > 
> > For instance what happens if you dpms off and then enable a plane? If
> > the clipping is based on the enabled state of the crtc then it'll try
> > to enable the plane, no?
> 
> Yeah, I think that was Daniel's concern too... ->enable is the wrong
> field to be pulling out of the state object here and we should be using
> ->active instead since that takes DPMS and such into account.
> 
> I'm not sure if ->active is properly updated in our CRTC state today by
> the legacy modesetting codepaths, but if we can get that squared away
> and switch to using crtc_state->active rather than crtc_state->enable do
> you forsee any other issues with pulling the value out of the in-flight
> state like this patch tries to do?

On reconsideration ->enable is imo the right field we should use for wm
computation and everything. Otherwise resource constraint checking will be
fail and a dpms on might not work, and that's not good.

Ofc we can't just try to enable planes when the crtc is off, but imo that
needs to be fixed differently. Rough plan (which is what Maarten's working
on).
- Unconditionally shut down planes in crtc_disable, bypassing any state
  changes. Maarten is working on some force plane disable patches to
  overwrite the ->visible checks.
- When a crtc is in the disable_pipes or modeset_pipes sets add all the
  plane states to the atomic update (in the crtc compute_config functions
  with a hack for disabled pipes until we get that one right). Also call
  plane atomic_check functions for all planes in the state update.
- Remove the plane enable code from crtc_enable and instead just do an
  atomic plane update (as we already have from Matt's atomic work) at the
  very end of the modeset sequence, but _only_ when a crtc is active.

This way we shouldn't have any issue with checking state and ->active is
only gating what hw we enable.

What we might want to change is pimp the shared atomic helpers to not call
any atomic_check functions for planes/crtcs if the crtc is not enabled. I
think that should functionally substitute this patch here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Lookup CRTC for plane directly

[Matt Roper]

Various places in the atomic plane code obtain the CRTC via
plane_state->crtc.  But plane_state->crtc is NULL when disabling the
plane, so the code will fall back to looking at the old CRTC value in
plane->crtc in that case.  This isn't going to work when we start
supporting non-blocking flips (since the legacy plane->crtc pointer will
already be updated to its new value before the asynchronous workqueue
task runs the plane commit function).  The code is also fragile in
general (we have to be careful when doing stuff like updating properties
on a disabled plane).  Since Intel hardware has a fixed plane to pipe
mapping, let's just lookup the CRTC via that fixed mapping and avoid
future mistakes.

Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Reported-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_display.c      |  8 ++++----
 drivers/gpu/drm/i915/intel_drv.h          |  7 +++++++
 drivers/gpu/drm/i915/intel_sprite.c       |  4 ++--
 4 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 90c4a82..740d1a6 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -116,19 +116,19 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	struct intel_plane_state *intel_state = to_intel_plane_state(state);
 	bool active;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
 	active = intel_crtc_state->base.enable;
 
 	/*
-	 * 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.
+	 * Both state->crtc and plane->state->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 (!state->crtc && !plane->state->crtc)
 		return 0;
 
 	/*
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 88b0f69..1cf91ad 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12625,7 +12625,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 	bool active;
 	int ret;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	intel_crtc_state = intel_crtc_state_for_plane(state);
@@ -12694,7 +12694,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 	bool active;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	intel_crtc_state = intel_crtc_state_for_plane(state);
@@ -12916,7 +12916,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	bool active;
 	int ret;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	intel_crtc_state = intel_crtc_state_for_plane(state);
@@ -12977,7 +12977,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	uint32_t addr;
 	bool active;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	intel_crtc_state = intel_crtc_state_for_plane(state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 71e0152..d5ea24f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -731,6 +731,13 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane)
 	return dev_priv->plane_to_crtc_mapping[plane];
 }
 
+static inline struct drm_crtc *
+intel_get_crtc_for_drm_plane(struct drm_plane *plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->dev);
+	return dev_priv->pipe_to_crtc_mapping[to_intel_plane(plane)->pipe];
+}
+
 struct intel_unpin_work {
 	struct work_struct work;
 	struct drm_crtc *crtc;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 2016e78..492abcd 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -878,7 +878,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	int pixel_size;
 	bool active;
 
-	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
+	intel_crtc = to_intel_crtc(intel_get_crtc_for_drm_plane(plane));
 
 	intel_crtc_state = intel_crtc_state_for_plane(state);
 	active = intel_crtc_state->base.enable;
@@ -1075,7 +1075,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	uint32_t src_x, src_y, src_w, src_h;
 	bool active;
 
-	crtc = crtc ? crtc : plane->crtc;
+	crtc = intel_get_crtc_for_drm_plane(plane);
 	intel_crtc = to_intel_crtc(crtc);
 
 	intel_crtc_state = intel_crtc_state_for_plane(state);
-- 
1.8.5.1

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

Re: [PATCH 2/4] drm/i915: Lookup CRTC for plane directly

[Daniel Vetter]

On Wed, Apr 08, 2015 at 06:56:52PM -0700, Matt Roper wrote:
> Various places in the atomic plane code obtain the CRTC via
> plane_state->crtc.  But plane_state->crtc is NULL when disabling the
> plane, so the code will fall back to looking at the old CRTC value in
> plane->crtc in that case.  This isn't going to work when we start
> supporting non-blocking flips (since the legacy plane->crtc pointer will
> already be updated to its new value before the asynchronous workqueue
> task runs the plane commit function).  The code is also fragile in
> general (we have to be careful when doing stuff like updating properties
> on a disabled plane).  Since Intel hardware has a fixed plane to pipe
> mapping, let's just lookup the CRTC via that fixed mapping and avoid
> future mistakes.
> 
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Reported-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Because I'm a lazy reviewer ... does cocci agree that you've spotted all
the drm_plane->crtc references?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 12 ++++++------
>  drivers/gpu/drm/i915/intel_display.c      |  8 ++++----
>  drivers/gpu/drm/i915/intel_drv.h          |  7 +++++++
>  drivers/gpu/drm/i915/intel_sprite.c       |  4 ++--
>  4 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 90c4a82..740d1a6 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -116,19 +116,19 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
>  	bool active;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
>  	active = intel_crtc_state->base.enable;
>  
>  	/*
> -	 * 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.
> +	 * Both state->crtc and plane->state->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 (!state->crtc && !plane->state->crtc)
>  		return 0;
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 88b0f69..1cf91ad 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12625,7 +12625,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	bool active;
>  	int ret;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	intel_crtc_state = intel_crtc_state_for_plane(state);
> @@ -12694,7 +12694,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	struct drm_rect *src = &state->src;
>  	bool active;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	intel_crtc_state = intel_crtc_state_for_plane(state);
> @@ -12916,7 +12916,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	bool active;
>  	int ret;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	intel_crtc_state = intel_crtc_state_for_plane(state);
> @@ -12977,7 +12977,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	uint32_t addr;
>  	bool active;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	intel_crtc_state = intel_crtc_state_for_plane(state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 71e0152..d5ea24f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -731,6 +731,13 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane)
>  	return dev_priv->plane_to_crtc_mapping[plane];
>  }
>  
> +static inline struct drm_crtc *
> +intel_get_crtc_for_drm_plane(struct drm_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> +	return dev_priv->pipe_to_crtc_mapping[to_intel_plane(plane)->pipe];
> +}
> +
>  struct intel_unpin_work {
>  	struct work_struct work;
>  	struct drm_crtc *crtc;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 2016e78..492abcd 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -878,7 +878,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	int pixel_size;
>  	bool active;
>  
> -	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> +	intel_crtc = to_intel_crtc(intel_get_crtc_for_drm_plane(plane));
>  
>  	intel_crtc_state = intel_crtc_state_for_plane(state);
>  	active = intel_crtc_state->base.enable;
> @@ -1075,7 +1075,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  	uint32_t src_x, src_y, src_w, src_h;
>  	bool active;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> +	crtc = intel_get_crtc_for_drm_plane(plane);
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	intel_crtc_state = intel_crtc_state_for_plane(state);
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915: Lookup CRTC for plane directly

[Matt Roper]

On Thu, Apr 09, 2015 at 02:44:05PM +0200, Daniel Vetter wrote:
> On Wed, Apr 08, 2015 at 06:56:52PM -0700, Matt Roper wrote:
> > Various places in the atomic plane code obtain the CRTC via
> > plane_state->crtc.  But plane_state->crtc is NULL when disabling the
> > plane, so the code will fall back to looking at the old CRTC value in
> > plane->crtc in that case.  This isn't going to work when we start
> > supporting non-blocking flips (since the legacy plane->crtc pointer will
> > already be updated to its new value before the asynchronous workqueue
> > task runs the plane commit function).  The code is also fragile in
> > general (we have to be careful when doing stuff like updating properties
> > on a disabled plane).  Since Intel hardware has a fixed plane to pipe
> > mapping, let's just lookup the CRTC via that fixed mapping and avoid
> > future mistakes.
> > 
> > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > Reported-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Because I'm a lazy reviewer ... does cocci agree that you've spotted all
> the drm_plane->crtc references?
> -Daniel

According to

        @@ struct drm_plane *P; @@
        * P->crtc

We have four remaining uses of drm_plane->crtc in the driver.  Two of
them are just updating it to properly mirror the value of
plane->state->crtc (e.g., in the load detect code), so those are good.
The other two uses are in intel_plane_restore(), which is intentionally
looking at what's already been committed and isn't in danger of being
used in the non-blocking/async code flow.  So I think we're okay now.


Matt

> 
> > ---
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 12 ++++++------
> >  drivers/gpu/drm/i915/intel_display.c      |  8 ++++----
> >  drivers/gpu/drm/i915/intel_drv.h          |  7 +++++++
> >  drivers/gpu/drm/i915/intel_sprite.c       |  4 ++--
> >  4 files changed, 19 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 90c4a82..740d1a6 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -116,19 +116,19 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> >  	bool active;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> >  	active = intel_crtc_state->base.enable;
> >  
> >  	/*
> > -	 * 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.
> > +	 * Both state->crtc and plane->state->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 (!state->crtc && !plane->state->crtc)
> >  		return 0;
> >  
> >  	/*
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 88b0f69..1cf91ad 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12625,7 +12625,7 @@ intel_check_primary_plane(struct drm_plane *plane,
> >  	bool active;
> >  	int ret;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> > @@ -12694,7 +12694,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
> >  	struct drm_rect *src = &state->src;
> >  	bool active;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> > @@ -12916,7 +12916,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> >  	bool active;
> >  	int ret;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> > @@ -12977,7 +12977,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> >  	uint32_t addr;
> >  	bool active;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 71e0152..d5ea24f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -731,6 +731,13 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane)
> >  	return dev_priv->plane_to_crtc_mapping[plane];
> >  }
> >  
> > +static inline struct drm_crtc *
> > +intel_get_crtc_for_drm_plane(struct drm_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> > +	return dev_priv->pipe_to_crtc_mapping[to_intel_plane(plane)->pipe];
> > +}
> > +
> >  struct intel_unpin_work {
> >  	struct work_struct work;
> >  	struct drm_crtc *crtc;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 2016e78..492abcd 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -878,7 +878,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
> >  	int pixel_size;
> >  	bool active;
> >  
> > -	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> > +	intel_crtc = to_intel_crtc(intel_get_crtc_for_drm_plane(plane));
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> >  	active = intel_crtc_state->base.enable;
> > @@ -1075,7 +1075,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> >  	uint32_t src_x, src_y, src_w, src_h;
> >  	bool active;
> >  
> > -	crtc = crtc ? crtc : plane->crtc;
> > +	crtc = intel_get_crtc_for_drm_plane(plane);
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> >  	intel_crtc_state = intel_crtc_state_for_plane(state);
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

[PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two

[Matt Roper]

Switch from our plane update/disable entrypoints to use the full atomic
helpers (which generate a top-level atomic transaction) rather than the
transitional helpers (which only create/manipulate orphaned plane states
independent of a top-level transaction).  Various upcoming work (SKL
scalers, atomic watermarks, etc.) requires a full atomic transaction to
behave properly/cleanly.

Last time we tried this, we had to back out the change because we still
call the drm_plane vfuncs directly from within our legacy modesetting
code.  This potentially results in nested atomic transactions, locking
collisions, and other failures.  To avoid that problem again, we
sidestep the issue by calling the transitional helpers directly (rather
than through a vfunc) when we're nested inside of other legacy
modesetting code.  However this does allow legacy SetPlane() ioctl's to
process an entire drm_atomic_state transaction, which is important for
upcoming patches.

Cc: Chandra Konduru <chandra.konduru@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++------------
 drivers/gpu/drm/i915/intel_sprite.c  | 10 +++++-----
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1cf91ad..3a74923 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5576,7 +5576,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	dev_priv->display.crtc_disable(crtc);
 	dev_priv->display.off(crtc);
 
-	crtc->primary->funcs->disable_plane(crtc->primary);
+	drm_plane_helper_disable(crtc->primary);
 
 	/* Update computed state. */
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
@@ -11731,11 +11731,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		int vdisplay, hdisplay;
 
 		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
-		ret = primary->funcs->update_plane(primary, &intel_crtc->base,
-						   fb, 0, 0,
-						   hdisplay, vdisplay,
-						   x << 16, y << 16,
-						   hdisplay << 16, vdisplay << 16);
+		ret = drm_plane_helper_update(primary, &intel_crtc->base,
+					      fb, 0, 0,
+					      hdisplay, vdisplay,
+					      x << 16, y << 16,
+					      hdisplay << 16, vdisplay << 16);
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
@@ -12286,10 +12286,10 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		int vdisplay, hdisplay;
 
 		drm_crtc_get_hv_timing(set->mode, &hdisplay, &vdisplay);
-		ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
-						   0, 0, hdisplay, vdisplay,
-						   set->x << 16, set->y << 16,
-						   hdisplay << 16, vdisplay << 16);
+		ret = drm_plane_helper_update(primary, set->crtc, set->fb,
+					      0, 0, hdisplay, vdisplay,
+					      set->x << 16, set->y << 16,
+					      hdisplay << 16, vdisplay << 16);
 
 		/*
 		 * We need to make sure the primary plane is re-enabled if it
@@ -12830,8 +12830,8 @@ void intel_plane_destroy(struct drm_plane *plane)
 }
 
 const struct drm_plane_funcs intel_plane_funcs = {
-	.update_plane = drm_plane_helper_update,
-	.disable_plane = drm_plane_helper_disable,
+	.update_plane = drm_atomic_helper_update_plane,
+	.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,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 492abcd..f4094d2 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1149,11 +1149,11 @@ int intel_plane_restore(struct drm_plane *plane)
 	if (!plane->crtc || !plane->state->fb)
 		return 0;
 
-	return plane->funcs->update_plane(plane, plane->crtc, plane->state->fb,
-				  plane->state->crtc_x, plane->state->crtc_y,
-				  plane->state->crtc_w, plane->state->crtc_h,
-				  plane->state->src_x, plane->state->src_y,
-				  plane->state->src_w, plane->state->src_h);
+	return drm_plane_helper_update(plane, plane->crtc, plane->state->fb,
+				       plane->state->crtc_x, plane->state->crtc_y,
+				       plane->state->crtc_w, plane->state->crtc_h,
+				       plane->state->src_x, plane->state->src_y,
+				       plane->state->src_w, plane->state->src_h);
 }
 
 static uint32_t ilk_plane_formats[] = {
-- 
1.8.5.1

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

Re: [PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two

[Daniel Vetter]

On Wed, Apr 08, 2015 at 06:56:53PM -0700, Matt Roper wrote:
> Switch from our plane update/disable entrypoints to use the full atomic
> helpers (which generate a top-level atomic transaction) rather than the
> transitional helpers (which only create/manipulate orphaned plane states
> independent of a top-level transaction).  Various upcoming work (SKL
> scalers, atomic watermarks, etc.) requires a full atomic transaction to
> behave properly/cleanly.
> 
> Last time we tried this, we had to back out the change because we still
> call the drm_plane vfuncs directly from within our legacy modesetting
> code.  This potentially results in nested atomic transactions, locking
> collisions, and other failures.  To avoid that problem again, we
> sidestep the issue by calling the transitional helpers directly (rather
> than through a vfunc) when we're nested inside of other legacy
> modesetting code.  However this does allow legacy SetPlane() ioctl's to
> process an entire drm_atomic_state transaction, which is important for
> upcoming patches.
> 
> Cc: Chandra Konduru <chandra.konduru@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Maarten is working to fix this properly (by wiring up plane states to the
transitional drm_atomic_state scaffolding from Ander), but seems like a
good interim idea to get back some solid testing for our atomic code.

Can I apply this without patches 1&2 right away or is there a tricky
depency?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++------------
>  drivers/gpu/drm/i915/intel_sprite.c  | 10 +++++-----
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1cf91ad..3a74923 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5576,7 +5576,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
>  	dev_priv->display.crtc_disable(crtc);
>  	dev_priv->display.off(crtc);
>  
> -	crtc->primary->funcs->disable_plane(crtc->primary);
> +	drm_plane_helper_disable(crtc->primary);
>  
>  	/* Update computed state. */
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> @@ -11731,11 +11731,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  		int vdisplay, hdisplay;
>  
>  		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> -		ret = primary->funcs->update_plane(primary, &intel_crtc->base,
> -						   fb, 0, 0,
> -						   hdisplay, vdisplay,
> -						   x << 16, y << 16,
> -						   hdisplay << 16, vdisplay << 16);
> +		ret = drm_plane_helper_update(primary, &intel_crtc->base,
> +					      fb, 0, 0,
> +					      hdisplay, vdisplay,
> +					      x << 16, y << 16,
> +					      hdisplay << 16, vdisplay << 16);
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> @@ -12286,10 +12286,10 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  		int vdisplay, hdisplay;
>  
>  		drm_crtc_get_hv_timing(set->mode, &hdisplay, &vdisplay);
> -		ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
> -						   0, 0, hdisplay, vdisplay,
> -						   set->x << 16, set->y << 16,
> -						   hdisplay << 16, vdisplay << 16);
> +		ret = drm_plane_helper_update(primary, set->crtc, set->fb,
> +					      0, 0, hdisplay, vdisplay,
> +					      set->x << 16, set->y << 16,
> +					      hdisplay << 16, vdisplay << 16);
>  
>  		/*
>  		 * We need to make sure the primary plane is re-enabled if it
> @@ -12830,8 +12830,8 @@ void intel_plane_destroy(struct drm_plane *plane)
>  }
>  
>  const struct drm_plane_funcs intel_plane_funcs = {
> -	.update_plane = drm_plane_helper_update,
> -	.disable_plane = drm_plane_helper_disable,
> +	.update_plane = drm_atomic_helper_update_plane,
> +	.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,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 492abcd..f4094d2 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1149,11 +1149,11 @@ int intel_plane_restore(struct drm_plane *plane)
>  	if (!plane->crtc || !plane->state->fb)
>  		return 0;
>  
> -	return plane->funcs->update_plane(plane, plane->crtc, plane->state->fb,
> -				  plane->state->crtc_x, plane->state->crtc_y,
> -				  plane->state->crtc_w, plane->state->crtc_h,
> -				  plane->state->src_x, plane->state->src_y,
> -				  plane->state->src_w, plane->state->src_h);
> +	return drm_plane_helper_update(plane, plane->crtc, plane->state->fb,
> +				       plane->state->crtc_x, plane->state->crtc_y,
> +				       plane->state->crtc_w, plane->state->crtc_h,
> +				       plane->state->src_x, plane->state->src_y,
> +				       plane->state->src_w, plane->state->src_h);
>  }
>  
>  static uint32_t ilk_plane_formats[] = {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two

[Matt Roper]

On Thu, Apr 09, 2015 at 02:46:19PM +0200, Daniel Vetter wrote:
> On Wed, Apr 08, 2015 at 06:56:53PM -0700, Matt Roper wrote:
> > Switch from our plane update/disable entrypoints to use the full atomic
> > helpers (which generate a top-level atomic transaction) rather than the
> > transitional helpers (which only create/manipulate orphaned plane states
> > independent of a top-level transaction).  Various upcoming work (SKL
> > scalers, atomic watermarks, etc.) requires a full atomic transaction to
> > behave properly/cleanly.
> > 
> > Last time we tried this, we had to back out the change because we still
> > call the drm_plane vfuncs directly from within our legacy modesetting
> > code.  This potentially results in nested atomic transactions, locking
> > collisions, and other failures.  To avoid that problem again, we
> > sidestep the issue by calling the transitional helpers directly (rather
> > than through a vfunc) when we're nested inside of other legacy
> > modesetting code.  However this does allow legacy SetPlane() ioctl's to
> > process an entire drm_atomic_state transaction, which is important for
> > upcoming patches.
> > 
> > Cc: Chandra Konduru <chandra.konduru@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Maarten is working to fix this properly (by wiring up plane states to the
> transitional drm_atomic_state scaffolding from Ander), but seems like a
> good interim idea to get back some solid testing for our atomic code.
> 
> Can I apply this without patches 1&2 right away or is there a tricky
> depency?
> -Daniel

I think this one can be applied independently of 1&2.  I think this one
might be a prereq for #4 to fully work as advertised though...without
using the full atomic helpers, we simply don't have a 'start of
transaction' point at which we can clear the existing atomic flags when
using legacy plane ioctls.


Matt

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++------------
> >  drivers/gpu/drm/i915/intel_sprite.c  | 10 +++++-----
> >  2 files changed, 17 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1cf91ad..3a74923 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5576,7 +5576,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
> >  	dev_priv->display.crtc_disable(crtc);
> >  	dev_priv->display.off(crtc);
> >  
> > -	crtc->primary->funcs->disable_plane(crtc->primary);
> > +	drm_plane_helper_disable(crtc->primary);
> >  
> >  	/* Update computed state. */
> >  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> > @@ -11731,11 +11731,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> >  		int vdisplay, hdisplay;
> >  
> >  		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> > -		ret = primary->funcs->update_plane(primary, &intel_crtc->base,
> > -						   fb, 0, 0,
> > -						   hdisplay, vdisplay,
> > -						   x << 16, y << 16,
> > -						   hdisplay << 16, vdisplay << 16);
> > +		ret = drm_plane_helper_update(primary, &intel_crtc->base,
> > +					      fb, 0, 0,
> > +					      hdisplay, vdisplay,
> > +					      x << 16, y << 16,
> > +					      hdisplay << 16, vdisplay << 16);
> >  	}
> >  
> >  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> > @@ -12286,10 +12286,10 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  		int vdisplay, hdisplay;
> >  
> >  		drm_crtc_get_hv_timing(set->mode, &hdisplay, &vdisplay);
> > -		ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
> > -						   0, 0, hdisplay, vdisplay,
> > -						   set->x << 16, set->y << 16,
> > -						   hdisplay << 16, vdisplay << 16);
> > +		ret = drm_plane_helper_update(primary, set->crtc, set->fb,
> > +					      0, 0, hdisplay, vdisplay,
> > +					      set->x << 16, set->y << 16,
> > +					      hdisplay << 16, vdisplay << 16);
> >  
> >  		/*
> >  		 * We need to make sure the primary plane is re-enabled if it
> > @@ -12830,8 +12830,8 @@ void intel_plane_destroy(struct drm_plane *plane)
> >  }
> >  
> >  const struct drm_plane_funcs intel_plane_funcs = {
> > -	.update_plane = drm_plane_helper_update,
> > -	.disable_plane = drm_plane_helper_disable,
> > +	.update_plane = drm_atomic_helper_update_plane,
> > +	.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,
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 492abcd..f4094d2 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1149,11 +1149,11 @@ int intel_plane_restore(struct drm_plane *plane)
> >  	if (!plane->crtc || !plane->state->fb)
> >  		return 0;
> >  
> > -	return plane->funcs->update_plane(plane, plane->crtc, plane->state->fb,
> > -				  plane->state->crtc_x, plane->state->crtc_y,
> > -				  plane->state->crtc_w, plane->state->crtc_h,
> > -				  plane->state->src_x, plane->state->src_y,
> > -				  plane->state->src_w, plane->state->src_h);
> > +	return drm_plane_helper_update(plane, plane->crtc, plane->state->fb,
> > +				       plane->state->crtc_x, plane->state->crtc_y,
> > +				       plane->state->crtc_w, plane->state->crtc_h,
> > +				       plane->state->src_x, plane->state->src_y,
> > +				       plane->state->src_w, plane->state->src_h);
> >  }
> >  
> >  static uint32_t ilk_plane_formats[] = {
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Matt Roper
> Sent: Thursday, April 09, 2015 11:04 AM
> To: Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915: Switch to full atomic helpers for
> plane updates/disable, take two
> 
> On Thu, Apr 09, 2015 at 02:46:19PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 08, 2015 at 06:56:53PM -0700, Matt Roper wrote:
> > > Switch from our plane update/disable entrypoints to use the full
> > > atomic helpers (which generate a top-level atomic transaction)
> > > rather than the transitional helpers (which only create/manipulate
> > > orphaned plane states independent of a top-level transaction).
> > > Various upcoming work (SKL scalers, atomic watermarks, etc.)
> > > requires a full atomic transaction to behave properly/cleanly.
> > >
> > > Last time we tried this, we had to back out the change because we
> > > still call the drm_plane vfuncs directly from within our legacy
> > > modesetting code.  This potentially results in nested atomic
> > > transactions, locking collisions, and other failures.  To avoid that
> > > problem again, we sidestep the issue by calling the transitional
> > > helpers directly (rather than through a vfunc) when we're nested
> > > inside of other legacy modesetting code.  
Matt,
I rebased scaler patch 13/14 on top of 90/270 but hit into issues. 
After much debugging, found below:
By keeping transitional helpers for some scenarios as you mentioned,
this is leaving intel_plane_state->src/dst rect values with 0s.
I recall having a src/dst rects copy in intel_plane_state is to
hold modified values due to any clipping etc. And modified values
will be used in platform specific update function
(i.e., skylake_update_primary_plane()). From these paths,
src/dst rects in both drm and intel plane states are same and works
by accessing drm_plane_state to make it work, but that isn't true 
in other cases and should use intel_plane_state src/rects only.
Working on a fix but any suggestions/comments welcome...


> > > However this does allow
> > > legacy SetPlane() ioctl's to process an entire drm_atomic_state
> > > transaction, which is important for upcoming patches.
> > >
> > > Cc: Chandra Konduru <chandra.konduru@intel.com>
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >
> > Maarten is working to fix this properly (by wiring up plane states to
> > the transitional drm_atomic_state scaffolding from Ander), but seems
> > like a good interim idea to get back some solid testing for our atomic code.
> >
> > Can I apply this without patches 1&2 right away or is there a tricky
> > depency?
> > -Daniel
> 
> I think this one can be applied independently of 1&2.  I think this one might be a
> prereq for #4 to fully work as advertised though...without using the full atomic
> helpers, we simply don't have a 'start of transaction' point at which we can clear
> the existing atomic flags when using legacy plane ioctls.
> 
> 
> Matt
> 
> >
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++------------
> > > drivers/gpu/drm/i915/intel_sprite.c  | 10 +++++-----
> > >  2 files changed, 17 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 1cf91ad..3a74923 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5576,7 +5576,7 @@ static void intel_crtc_disable(struct drm_crtc
> *crtc)
> > >  	dev_priv->display.crtc_disable(crtc);
> > >  	dev_priv->display.off(crtc);
> > >
> > > -	crtc->primary->funcs->disable_plane(crtc->primary);
> > > +	drm_plane_helper_disable(crtc->primary);
> > >
> > >  	/* Update computed state. */
> > >  	list_for_each_entry(connector, &dev->mode_config.connector_list,
> > > head) { @@ -11731,11 +11731,11 @@ static int __intel_set_mode(struct
> drm_crtc *crtc,
> > >  		int vdisplay, hdisplay;
> > >
> > >  		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> > > -		ret = primary->funcs->update_plane(primary, &intel_crtc->base,
> > > -						   fb, 0, 0,
> > > -						   hdisplay, vdisplay,
> > > -						   x << 16, y << 16,
> > > -						   hdisplay << 16, vdisplay <<
> 16);
> > > +		ret = drm_plane_helper_update(primary, &intel_crtc->base,
> > > +					      fb, 0, 0,
> > > +					      hdisplay, vdisplay,
> > > +					      x << 16, y << 16,
> > > +					      hdisplay << 16, vdisplay << 16);
> > >  	}
> > >
> > >  	/* Now enable the clocks, plane, pipe, and connectors that we set
> > > up. */ @@ -12286,10 +12286,10 @@ static int intel_crtc_set_config(struct
> drm_mode_set *set)
> > >  		int vdisplay, hdisplay;
> > >
> > >  		drm_crtc_get_hv_timing(set->mode, &hdisplay, &vdisplay);
> > > -		ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
> > > -						   0, 0, hdisplay, vdisplay,
> > > -						   set->x << 16, set->y << 16,
> > > -						   hdisplay << 16, vdisplay <<
> 16);
> > > +		ret = drm_plane_helper_update(primary, set->crtc, set->fb,
> > > +					      0, 0, hdisplay, vdisplay,
> > > +					      set->x << 16, set->y << 16,
> > > +					      hdisplay << 16, vdisplay << 16);
> > >
> > >  		/*
> > >  		 * We need to make sure the primary plane is re-enabled if it
> @@
> > > -12830,8 +12830,8 @@ void intel_plane_destroy(struct drm_plane
> > > *plane)  }
> > >
> > >  const struct drm_plane_funcs intel_plane_funcs = {
> > > -	.update_plane = drm_plane_helper_update,
> > > -	.disable_plane = drm_plane_helper_disable,
> > > +	.update_plane = drm_atomic_helper_update_plane,
> > > +	.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, diff --git
> > > a/drivers/gpu/drm/i915/intel_sprite.c
> > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 492abcd..f4094d2 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -1149,11 +1149,11 @@ int intel_plane_restore(struct drm_plane
> *plane)
> > >  	if (!plane->crtc || !plane->state->fb)
> > >  		return 0;
> > >
> > > -	return plane->funcs->update_plane(plane, plane->crtc, plane->state-
> >fb,
> > > -				  plane->state->crtc_x, plane->state->crtc_y,
> > > -				  plane->state->crtc_w, plane->state->crtc_h,
> > > -				  plane->state->src_x, plane->state->src_y,
> > > -				  plane->state->src_w, plane->state->src_h);
> > > +	return drm_plane_helper_update(plane, plane->crtc, plane->state->fb,
> > > +				       plane->state->crtc_x, plane->state->crtc_y,
> > > +				       plane->state->crtc_w, plane->state-
> >crtc_h,
> > > +				       plane->state->src_x, plane->state->src_y,
> > > +				       plane->state->src_w, plane->state->src_h);
> > >  }
> > >
> > >  static uint32_t ilk_plane_formats[] = {
> > > --
> > > 1.8.5.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Clear crtc atomic flags at beginning of transaction

[Matt Roper]

Once we have full atomic modeset, these kind of flags should be in a
real intel_crtc_state that's tracked properly.  In the meantime, make
sure we clear out any old flags at the beginning of a transaction so
that we don't wind up seeing leftover flags from old transactions that
were checked, but never went to the commit step.

A simple memset would have done here, but I expect there to be a few
more things that we *don't* want to clear that get added into this
structure before we're ready to kill off and roll everything into the
CRTC state.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 3903b90..542230d 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -35,6 +35,22 @@
 #include <drm/drm_plane_helper.h>
 #include "intel_drv.h"
 
+/**
+ * intel_clear_atomic_crtc_flags
+ * @crtc: CRTC to clear flags for
+ *
+ * Until we have proper atomic handling of CRTC states, we dump some atomic
+ * task flags in intel_crtc->atomic.  Those flags need to be cleared at
+ * the beginning of each transaction so that we don't carry over stale flags
+ * from previous transactions.
+ *
+ * Note that the whole intel_crtc->atomic structure is a temporary hack and
+ * should transition into the CRTC state eventually.
+ */
+void intel_clear_atomic_crtc_flags(struct intel_crtc *crtc)
+{
+	memset(&crtc->atomic, 0, sizeof(crtc->atomic));
+}
 
 /**
  * intel_atomic_check - validate state object
@@ -76,6 +92,8 @@ int intel_atomic_check(struct drm_device *dev,
 	state->allow_modeset = false;
 	for (i = 0; i < ncrtcs; i++) {
 		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
+		if (crtc)
+			intel_clear_atomic_crtc_flags(crtc);
 		if (crtc && crtc->pipe != nuclear_pipe)
 			not_nuclear = true;
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3a74923..bb8d345 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12812,7 +12812,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
 						       false, false);
 
-	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
+	intel_clear_atomic_crtc_flags(intel_crtc);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d5ea24f..28d838e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1303,6 +1303,7 @@ void intel_pre_disable_primary(struct drm_crtc *crtc);
 void intel_tv_init(struct drm_device *dev);
 
 /* intel_atomic.c */
+void intel_clear_atomic_crtc_flags(struct intel_crtc *crtc);
 int intel_atomic_check(struct drm_device *dev,
 		       struct drm_atomic_state *state);
 int intel_atomic_commit(struct drm_device *dev,
-- 
1.8.5.1

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

Re: [PATCH 4/4] drm/i915: Clear crtc atomic flags at beginning of transaction

[Daniel Vetter]

On Wed, Apr 08, 2015 at 06:56:54PM -0700, Matt Roper wrote:
> Once we have full atomic modeset, these kind of flags should be in a
> real intel_crtc_state that's tracked properly.  In the meantime, make
> sure we clear out any old flags at the beginning of a transaction so
> that we don't wind up seeing leftover flags from old transactions that
> were checked, but never went to the commit step.
> 
> A simple memset would have done here, but I expect there to be a few
> more things that we *don't* want to clear that get added into this
> structure before we're ready to kill off and roll everything into the
> CRTC state.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Not sure whether this helps a lot with moving intel_crtc->atomic into
intel_crtc_state. We probably want to just move things one-by-one to
reduce the overall churn, and then we can add them one-by-one to the intel
crtc_state_duplicate function.

Or do you have some bigger plans here?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_atomic.c  | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 3903b90..542230d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -35,6 +35,22 @@
>  #include <drm/drm_plane_helper.h>
>  #include "intel_drv.h"
>  
> +/**
> + * intel_clear_atomic_crtc_flags
> + * @crtc: CRTC to clear flags for
> + *
> + * Until we have proper atomic handling of CRTC states, we dump some atomic
> + * task flags in intel_crtc->atomic.  Those flags need to be cleared at
> + * the beginning of each transaction so that we don't carry over stale flags
> + * from previous transactions.
> + *
> + * Note that the whole intel_crtc->atomic structure is a temporary hack and
> + * should transition into the CRTC state eventually.
> + */
> +void intel_clear_atomic_crtc_flags(struct intel_crtc *crtc)
> +{
> +	memset(&crtc->atomic, 0, sizeof(crtc->atomic));
> +}
>  
>  /**
>   * intel_atomic_check - validate state object
> @@ -76,6 +92,8 @@ int intel_atomic_check(struct drm_device *dev,
>  	state->allow_modeset = false;
>  	for (i = 0; i < ncrtcs; i++) {
>  		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
> +		if (crtc)
> +			intel_clear_atomic_crtc_flags(crtc);
>  		if (crtc && crtc->pipe != nuclear_pipe)
>  			not_nuclear = true;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3a74923..bb8d345 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12812,7 +12812,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>  			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
>  						       false, false);
>  
> -	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> +	intel_clear_atomic_crtc_flags(intel_crtc);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d5ea24f..28d838e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1303,6 +1303,7 @@ void intel_pre_disable_primary(struct drm_crtc *crtc);
>  void intel_tv_init(struct drm_device *dev);
>  
>  /* intel_atomic.c */
> +void intel_clear_atomic_crtc_flags(struct intel_crtc *crtc);
>  int intel_atomic_check(struct drm_device *dev,
>  		       struct drm_atomic_state *state);
>  int intel_atomic_commit(struct drm_device *dev,
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/4] drm/i915: Clear crtc atomic flags at beginning of transaction

[Matt Roper]

On Thu, Apr 09, 2015 at 02:48:43PM +0200, Daniel Vetter wrote:
> On Wed, Apr 08, 2015 at 06:56:54PM -0700, Matt Roper wrote:
> > Once we have full atomic modeset, these kind of flags should be in a
> > real intel_crtc_state that's tracked properly.  In the meantime, make
> > sure we clear out any old flags at the beginning of a transaction so
> > that we don't wind up seeing leftover flags from old transactions that
> > were checked, but never went to the commit step.
> > 
> > A simple memset would have done here, but I expect there to be a few
> > more things that we *don't* want to clear that get added into this
> > structure before we're ready to kill off and roll everything into the
> > CRTC state.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Not sure whether this helps a lot with moving intel_crtc->atomic into
> intel_crtc_state. We probably want to just move things one-by-one to
> reduce the overall churn, and then we can add them one-by-one to the intel
> crtc_state_duplicate function.
> 
> Or do you have some bigger plans here?
> -Daniel

My in-progress WM work had to stick some other things in
intel_crtc->atomic, but maybe with Ander's latest patch sets we're close
enough that I can rewrite those to use crtc_state instead; I'll have to
go back and check.

Regardless, I think we do still have a bug today where an uncommitted
transaction (e.g., because check or prepare fail) will leave stale flags
in intel_crtc->atomic that the next transaction doesn't realize it needs
to clear.  We either need to clear those out (as I'm doing here,
although a simple memset would work too), or we need to transition all
those flags over to CRTC state and kill off the hacky intel_crtc->atomic
structure.


Matt

> 
> > ---
> >  drivers/gpu/drm/i915/intel_atomic.c  | 18 ++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  3 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> > index 3903b90..542230d 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -35,6 +35,22 @@
> >  #include <drm/drm_plane_helper.h>
> >  #include "intel_drv.h"
> >  
> > +/**
> > + * intel_clear_atomic_crtc_flags
> > + * @crtc: CRTC to clear flags for
> > + *
> > + * Until we have proper atomic handling of CRTC states, we dump some atomic
> > + * task flags in intel_crtc->atomic.  Those flags need to be cleared at
> > + * the beginning of each transaction so that we don't carry over stale flags
> > + * from previous transactions.
> > + *
> > + * Note that the whole intel_crtc->atomic structure is a temporary hack and
> > + * should transition into the CRTC state eventually.
> > + */
> > +void intel_clear_atomic_crtc_flags(struct intel_crtc *crtc)
> > +{
> > +	memset(&crtc->atomic, 0, sizeof(crtc->atomic));
> > +}
> >  
> >  /**
> >   * intel_atomic_check - validate state object
> > @@ -76,6 +92,8 @@ int intel_atomic_check(struct drm_device *dev,
> >  	state->allow_modeset = false;
> >  	for (i = 0; i < ncrtcs; i++) {
> >  		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
> > +		if (crtc)
> > +			intel_clear_atomic_crtc_flags(crtc);
> >  		if (crtc && crtc->pipe != nuclear_pipe)
> >  			not_nuclear = true;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3a74923..bb8d345 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12812,7 +12812,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> >  			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
> >  						       false, false);
> >  
> > -	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> > +	intel_clear_atomic_crtc_flags(intel_crtc);
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index d5ea24f..28d838e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1303,6 +1303,7 @@ void intel_pre_disable_primary(struct drm_crtc *crtc);
> >  void intel_tv_init(struct drm_device *dev);
> >  
> >  /* intel_atomic.c */
> > +void intel_clear_atomic_crtc_flags(struct intel_crtc *crtc);
> >  int intel_atomic_check(struct drm_device *dev,
> >  		       struct drm_atomic_state *state);
> >  int intel_atomic_commit(struct drm_device *dev,
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH 4/4] drm/i915: Clear crtc atomic flags at beginning of transaction

[Daniel Vetter]

On Thu, Apr 09, 2015 at 08:03:35AM -0700, Matt Roper wrote:
> On Thu, Apr 09, 2015 at 02:48:43PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 08, 2015 at 06:56:54PM -0700, Matt Roper wrote:
> > > Once we have full atomic modeset, these kind of flags should be in a
> > > real intel_crtc_state that's tracked properly.  In the meantime, make
> > > sure we clear out any old flags at the beginning of a transaction so
> > > that we don't wind up seeing leftover flags from old transactions that
> > > were checked, but never went to the commit step.
> > > 
> > > A simple memset would have done here, but I expect there to be a few
> > > more things that we *don't* want to clear that get added into this
> > > structure before we're ready to kill off and roll everything into the
> > > CRTC state.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > Not sure whether this helps a lot with moving intel_crtc->atomic into
> > intel_crtc_state. We probably want to just move things one-by-one to
> > reduce the overall churn, and then we can add them one-by-one to the intel
> > crtc_state_duplicate function.
> > 
> > Or do you have some bigger plans here?
> > -Daniel
> 
> My in-progress WM work had to stick some other things in
> intel_crtc->atomic, but maybe with Ander's latest patch sets we're close
> enough that I can rewrite those to use crtc_state instead; I'll have to
> go back and check.
> 
> Regardless, I think we do still have a bug today where an uncommitted
> transaction (e.g., because check or prepare fail) will leave stale flags
> in intel_crtc->atomic that the next transaction doesn't realize it needs
> to clear.  We either need to clear those out (as I'm doing here,
> although a simple memset would work too), or we need to transition all
> those flags over to CRTC state and kill off the hacky intel_crtc->atomic
> structure.

Yeah I agree that we need the memset. Can you resend just that one,
without the wrapping please?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Clear crtc atomic flags at beginning of transaction

[Matt Roper]

Once we have full atomic modeset, these kind of flags should be in a
real intel_crtc_state that's tracked properly.  In the meantime, make
sure we clear out any old flags at the beginning of a transaction so
that we don't wind up seeing leftover flags from old transactions that
were checked, but never went to the commit step.  At the moment, a
failed check or prepare could leave stale flags behind that interfere
with the next atomic transaction.

v2: Just do a memset; the series this patch was originally part of
    placed additional fields into the structure that shouldn't be
    cleared, but that's no longer the case.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 3903b90..b4ea676 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -76,6 +76,8 @@ int intel_atomic_check(struct drm_device *dev,
 	state->allow_modeset = false;
 	for (i = 0; i < ncrtcs; i++) {
 		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
+		if (crtc)
+			memset(&crtc->atomic, 0, sizeof(crtc->atomic));
 		if (crtc && crtc->pipe != nuclear_pipe)
 			not_nuclear = true;
 	}
-- 
1.8.5.1

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

Re: [PATCH] drm/i915: Clear crtc atomic flags at beginning of transaction

[Daniel Vetter]

On Thu, Apr 09, 2015 at 10:48:38AM -0700, Matt Roper wrote:
> Once we have full atomic modeset, these kind of flags should be in a
> real intel_crtc_state that's tracked properly.  In the meantime, make
> sure we clear out any old flags at the beginning of a transaction so
> that we don't wind up seeing leftover flags from old transactions that
> were checked, but never went to the commit step.  At the moment, a
> failed check or prepare could leave stale flags behind that interfere
> with the next atomic transaction.
> 
> v2: Just do a memset; the series this patch was originally part of
>     placed additional fields into the structure that shouldn't be
>     cleared, but that's no longer the case.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Patch 3 and this one here merged, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_atomic.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 3903b90..b4ea676 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -76,6 +76,8 @@ int intel_atomic_check(struct drm_device *dev,
>  	state->allow_modeset = false;
>  	for (i = 0; i < ncrtcs; i++) {
>  		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
> +		if (crtc)
> +			memset(&crtc->atomic, 0, sizeof(crtc->atomic));
>  		if (crtc && crtc->pipe != nuclear_pipe)
>  			not_nuclear = true;
>  	}
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Clear crtc atomic flags at beginning of transaction

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6166
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  313/313              313/313
IVB                                  337/337              337/337
BYT                                  286/286              286/286
HSW                                  395/395              395/395
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Clear crtc atomic flags at beginning of transaction

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6151
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                 -34              313/313              279/313
IVB                                  337/337              337/337
BYT                                  286/286              286/286
HSW                                  395/395              395/395
BDW                 -1              321/321              320/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt@gem_mmap_gtt@write-gtt      PASS(5)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_cursor_crc@cursor-size-change      DMESG_WARN(4)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_flip@bo-too-big      DMESG_WARN(4)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_flip@bo-too-big-interruptible      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_flip_event_leak      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_flip@flip-vs-dpms-off-vs-modeset      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_flip@flip-vs-dpms-off-vs-modeset-interruptible      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_flip@nonexisting-fb      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_flip@nonexisting-fb-interruptible      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_flip@single-buffer-flip-vs-dpms-off-vs-modeset      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_flip@single-buffer-flip-vs-dpms-off-vs-modeset-interruptible      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_flip_tiling@flip-changes-tiling      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_mmio_vs_cs_flip@setcrtc_vs_cs_flip      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_mmio_vs_cs_flip@setplane_vs_cs_flip      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@kms_rotation_crc@primary-rotation      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@cursor      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@cursor-dpms      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@debugfs-forcewake-user      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@dpms-non-lpsp      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@drm-resources-equal      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@fences      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@fences-dpms      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@gem-execbuf      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@gem-idle      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@gem-mmap-cpu      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@gem-mmap-gtt      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@gem-pread      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@i2c      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@modeset-non-lpsp      DMESG_WARN(5)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@modeset-non-lpsp-stress-no-wait      DMESG_WARN(4)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@pci-d3-state      DMESG_WARN(4)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@reg-read-ioctl      DMESG_WARN(4)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
 SNB  igt@pm_rpm@rte      DMESG_WARN(4)PASS(3)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting
*BDW  igt@kms_fence_pin_leak      PASS(4)      DMESG_WARN(2)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/drm_irq.c:#drm_wait_one_vblank[drm]()@WARNING:.* at .* drm_wait_one_vblank+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Rodrigo Vivi]

Hi Daniel,

It seems that one check with one good commit followed by many zeroed
intel_crtc->atomic commits is again in place.

So I'm getting that annoying false positives on latest -nightly.

Shouldn't we just merge this patch while atomic modeset design doesn't
get fixed at all?

Unfortunately nothing comes to my mind than moving all
intel_crtc->atomic set to commit time and let pre_commit just with
pm_get...
Ohter than that just a full redesign of atomic modeset.

Thanks,
Rodrigo.


On Fri, Feb 13, 2015 at 12:48 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Feb 12, 2015 at 05:17:04PM -0800, Rodrigo Vivi wrote:
>> No, we had solved old frontbuffer false positives... some missing
>> flush somewhere at that time...
>>
>> So, I added a bunch of printk and I insist that it is conceptually
>> wrong to set intel_crtc_atomic_commit on check times when you do
>> memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
>> on every finish_commit.
>>
>> With exception of atomic.disabled_planes I believe the rest shouldn't
>> work in the way it is implemented because you can have one check
>> followed by many commits, but after the first commit all atomic
>> variables are zeroed, except the disabled_planes that is set outside
>> check...
>
> Ok here's the trouble: Every commit should have at exactly one check for
> the new state objects. Unfortunately in the transition that seems to have
> been lost for some cases.
>
>> For instance: on every cursor movement atomic.fb_bits was 0x000 when
>> it should be 0x002. This is why this patch solved the false positive,
>> i.e. setting it on commit instead on check time we get it propperly
>> set. One of the problems is the false positive but also it breaks
>> entirely SW tracking on VLV/CHV....
>>
>> I believe wait_for flips, update_fbc, watermarks, etc should keep the
>> value got on check for the commit or the check should be done at
>> commit plane instead of on check.
>>
>> I started doing a patch here to move all atomic sets from check to
>> commit functions but gave up on middle when I noticed the
>> prepare_commit would almost get empty...
>
> All state precomputation must be done in check, at commit time you have a
> lot less information since the old state is somewhat gone. You can still
> get at it, but as soon as we add an async flip queue that will get really
> ugly. The current placement is imo the correct one. Instead we need to
> figure out where we're doing a ->commit without properly calling ->check
> beforehand.
>
>> Another idea was to make a atomic set per plane and just memset(0) on
>> begin of every check... But this would require reliable access to the
>> plane being updated on finish_commit... I believe loop on all planes
>> would be messy and cause other issues...
>>
>> So, I'll be out returning only next wed. Please let me know if you
>> have any suggestion of best changes to do that I can implement the
>> changes.
>
> Since you've done this testing I've landed Matt's patches to switch legacy
> plane entry points over to atomic. Which means cursor updates should now
> be done properly using atomic, always. But even then the old transitional
> plane helpers should have called the check functions ... So not sure where
> exactly we're loosing that check call.
>
> Matt Roper might have more insights.
>
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Clear crtc atomic flags at beginning of transaction

[Matt Roper]

Once we have full atomic modeset, these kind of flags should be in a
real intel_crtc_state that's tracked properly.  In the meantime, make
sure we clear out any old flags at the beginning of a transaction so
that we don't wind up seeing leftover flags from old transactions that
were checked, but never went to the commit step.

Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
Rodrigo, does this solve the frontbuffer problems you're seeing?  I haven't had
time to test this myself, but I think this will fix at least one problem that
may or may not have been the source of your troubles.

 drivers/gpu/drm/i915/intel_atomic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 011b896..fe51e23 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -76,6 +76,8 @@ int intel_atomic_check(struct drm_device *dev,
 	state->allow_modeset = false;
 	for (i = 0; i < ncrtcs; i++) {
 		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
+		if (crtc)
+			memset(&crtc->atomic, 0, sizeof(crtc->atomic));
 		if (crtc && crtc->pipe != nuclear_pipe)
 			not_nuclear = true;
 	}
-- 
1.8.5.1

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

Re: [PATCH] drm/i915: Clear crtc atomic flags at beginning of transaction

[Rodrigo Vivi]

Hi Matt,

It probably doesn't fix because the issue is to have many commits
without a check, having many commits with intel_crtc->atomic zeroed
already.
imho cleaning it more wouldn't solve it... but anyway, let me try.

Thanks,
Rodrigo.

On Mon, Feb 23, 2015 at 6:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> Once we have full atomic modeset, these kind of flags should be in a
> real intel_crtc_state that's tracked properly.  In the meantime, make
> sure we clear out any old flags at the beginning of a transaction so
> that we don't wind up seeing leftover flags from old transactions that
> were checked, but never went to the commit step.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> Rodrigo, does this solve the frontbuffer problems you're seeing?  I haven't had
> time to test this myself, but I think this will fix at least one problem that
> may or may not have been the source of your troubles.
>
>  drivers/gpu/drm/i915/intel_atomic.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 011b896..fe51e23 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -76,6 +76,8 @@ int intel_atomic_check(struct drm_device *dev,
>         state->allow_modeset = false;
>         for (i = 0; i < ncrtcs; i++) {
>                 struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
> +               if (crtc)
> +                       memset(&crtc->atomic, 0, sizeof(crtc->atomic));
>                 if (crtc && crtc->pipe != nuclear_pipe)
>                         not_nuclear = true;
>         }
> --
> 1.8.5.1
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Clear crtc atomic flags at beginning of transaction

[Rodrigo Vivi]

It doesn't fix.

Let's me grab some new logs...

On Tue, Feb 24, 2015 at 9:43 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> Hi Matt,
>
> It probably doesn't fix because the issue is to have many commits
> without a check, having many commits with intel_crtc->atomic zeroed
> already.
> imho cleaning it more wouldn't solve it... but anyway, let me try.
>
> Thanks,
> Rodrigo.
>
> On Mon, Feb 23, 2015 at 6:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
>> Once we have full atomic modeset, these kind of flags should be in a
>> real intel_crtc_state that's tracked properly.  In the meantime, make
>> sure we clear out any old flags at the beginning of a transaction so
>> that we don't wind up seeing leftover flags from old transactions that
>> were checked, but never went to the commit step.
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
>> Rodrigo, does this solve the frontbuffer problems you're seeing?  I haven't had
>> time to test this myself, but I think this will fix at least one problem that
>> may or may not have been the source of your troubles.
>>
>>  drivers/gpu/drm/i915/intel_atomic.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 011b896..fe51e23 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -76,6 +76,8 @@ int intel_atomic_check(struct drm_device *dev,
>>         state->allow_modeset = false;
>>         for (i = 0; i < ncrtcs; i++) {
>>                 struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
>> +               if (crtc)
>> +                       memset(&crtc->atomic, 0, sizeof(crtc->atomic));
>>                 if (crtc && crtc->pipe != nuclear_pipe)
>>                         not_nuclear = true;
>>         }
>> --
>> 1.8.5.1
>>
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx