[PATCH 1/2] drm/i915: enable FBC on gen9+ too

[PATCH 1/2] drm/i915: enable FBC on gen9+ too

[Paulo Zanoni]

Gen9+ platforms have been seeing a lot of screen flickerings and
underruns, so I never felt comfortable in enabling FBC on these
platforms since I didn't want to throw yet another feature on top of
the already complex problem. We now have code that automatically
disables FBC if we ever get an underrun, and the screen flickerings
seem to be mostly gone, so it may be a good time to try to finally
enable FBC by default on the newer platforms.

Besides, BDW FBC has been working fine over the year, which gives me a
little more confidence now.

For a little more information, please refer to commit a98ee79317b4
("drm/i915/fbc: enable FBC by default on HSW and BDW").

v2: Enable not only on SKL, but for everything new (Daniel).
v3: Rebase after the intel_sanitize_fbc_option() change.
v4: New rebase after 8 months, drop expired R-B tags.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Daniel gave me a R-B for this patch maaaaany months ago, but I think
we can consider it as expired now.

QA just went through a round of testing and confirmed 100% pass rate
for SKL/KBL/BXT (VIZ-7181).

Besides, we do have at least one user with FBC enabled on SKL and
reporting possible issues, as we saw on #98213, which is already
fixed.

The problem that I've been discussing with Chris and Daniel is not a
user visible bug since it just keeps FBC deactivated if the client
doesn't issue the dirtyfb calls, and from what I can see, current user
space is fine.

I have a pending patch for an improvement on the CRTC-choosing code,
but that's not a bug fix and doesn't change how HSW+ behaves. It's
patch 2 of this series.

I think we can also consider reenabling FBC on HSW due to the
confirmation I got some time ago that the HSW regression is gone (both
bugs from c7f7e2feffb0 have confirmed that some patches that are
alredy merged fixed the problem without reverting HSW support). But
I won't propose this until we get to understand what's going on with
bug #99169.

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 9aec63b..26a81a9 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1317,7 +1317,7 @@ static int intel_sanitize_fbc_option(struct drm_i915_private *dev_priv)
 	if (!HAS_FBC(dev_priv))
 		return 0;
 
-	if (IS_BROADWELL(dev_priv))
+	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9)
 		return 1;
 
 	return 0;
-- 
2.7.4

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

[PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

[Paulo Zanoni]

Ville pointed out that intel_fbc_choose_crtc() is iterating over all
planes instead of just the primary planes. There are no real
consequences of this problem for HSW+, and for the other platforms it
just means that in some obscure multi-screen cases we'll keep FBC
disabled when we could have enabled it. Still, iterating over all
planes doesn't seem to be the best thing to do.

My initial idea was to just add a check for plane->type and be done,
but then I realized that in commits not involving the primary plane we
would reset crtc_state->enable_fbc back to false even when FBC is
enabled. That also wouldn't result in a bug due to the way the
enable_fbc variable is checked, but, still, our code can be better
than this.

So I went for the solution that involves tracking enable_fbc in the
primary plane state instead of the CRTC state. This way, if a commit
doesn't involve the primary plane for the CRTC we won't be resetting
enable_fbc back to false, so the variable will always reflect the
reality. And this change also makes more sense since FBC is actually
tied to the single plane and not the full pipe. As a bonus, we only
iterate over the CRTCs instead of iterating over all planes.

v2:

But Ville pointed that this only works properly if we have a single
commit with multiple CRTCs. If we have multiple parallel commits, each
with its own CRTC, we'll end with enable_fbc set to true in more than
one plane.

So the solution was to rename enable_fbc to can_enable_fbc. If we just
did the rename as the patch was, we'd end up with a single plane with
can_enable_fbc on commits involving multiple CRTCs: we'd choose the
best one, but we'd still end up with a variable that doesn't 100%
reflect reality.

So in the end I adopted Ville's suggestion of the fbc_score variable.
And then, instead of checking the score at intel_fbc_choose_crtc()
it should be possible to check for the score at intel_fbc_enable().
This allows us to simplify intel_fbc_choose_crtc() to the point where
it only needs to look at the given plane in order to assign its score
instead of looking at every primary plane on the same commit.

We still only set scores of 0 and 1 and we don't really do the
score-checking loop.

v3: Rebase.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h     |  8 ++--
 drivers/gpu/drm/i915/intel_fbc.c     | 85 ++++++++++++------------------------
 3 files changed, 35 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d8effd4..92527d6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14191,7 +14191,6 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	intel_fbc_choose_crtc(dev_priv, state);
 	return calc_watermark_data(state);
 }
 
@@ -14966,6 +14965,8 @@ intel_check_primary_plane(struct drm_plane *plane,
 			return ret;
 	}
 
+	intel_fbc_check_plane(state);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 025e4c8..7430082 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -406,6 +406,9 @@ struct intel_plane_state {
 	 */
 	int scaler_id;
 
+	/* 0: not suitable for FBC, 1+: suitable for FBC, more is better. */
+	unsigned int fbc_score;
+
 	struct drm_intel_sprite_colorkey ckey;
 };
 
@@ -652,8 +655,6 @@ struct intel_crtc_state {
 
 	bool ips_enabled;
 
-	bool enable_fbc;
-
 	bool double_wide;
 
 	int pbn;
@@ -1535,8 +1536,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
 #endif
 
 /* intel_fbc.c */
-void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
-			   struct drm_atomic_state *state);
+void intel_fbc_check_plane(struct intel_plane_state *plane_state);
 bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
 void intel_fbc_pre_update(struct intel_crtc *crtc,
 			  struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 26a81a9..ceda6f4 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1040,68 +1040,32 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_fbc_choose_crtc - select a CRTC to enable FBC on
- * @dev_priv: i915 device instance
- * @state: the atomic state structure
+ * intel_fbc_check_plane - check plane for FBC suitability
+ * @plane_state: the plane state
  *
- * This function looks at the proposed state for CRTCs and planes, then chooses
- * which pipe is going to have FBC by setting intel_crtc_state->enable_fbc to
- * true.
+ * This function should set fbc_score based on how suitable the plane is for
+ * FBC. For now the only scores used are 0 and 1.
  *
- * Later, intel_fbc_enable is going to look for state->enable_fbc and then maybe
- * enable FBC for the chosen CRTC. If it does, it will set dev_priv->fbc.crtc.
+ * We're not changing dev_priv->fbc, so there's no need for the FBC lock.
  */
-void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
-			   struct drm_atomic_state *state)
+void intel_fbc_check_plane(struct intel_plane_state *plane_state)
 {
-	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
-	bool crtc_chosen = false;
-	int i;
+	struct drm_plane *plane = plane_state->base.plane;
+	struct drm_i915_private *dev_priv = to_i915(plane->dev);
+	struct intel_crtc *crtc = to_intel_crtc(plane_state->base.crtc);
 
-	mutex_lock(&fbc->lock);
-
-	/* Does this atomic commit involve the CRTC currently tied to FBC? */
-	if (fbc->crtc &&
-	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc->base))
-		goto out;
+	plane_state->fbc_score = 0;
 
 	if (!intel_fbc_can_enable(dev_priv))
-		goto out;
-
-	/* Simply choose the first CRTC that is compatible and has a visible
-	 * plane. We could go for fancier schemes such as checking the plane
-	 * size, but this would just affect the few platforms that don't tie FBC
-	 * to pipe or plane A. */
-	for_each_plane_in_state(state, plane, plane_state, i) {
-		struct intel_plane_state *intel_plane_state =
-			to_intel_plane_state(plane_state);
-		struct intel_crtc_state *intel_crtc_state;
-		struct intel_crtc *crtc = to_intel_crtc(plane_state->crtc);
-
-		if (!intel_plane_state->base.visible)
-			continue;
-
-		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
-			continue;
-
-		if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
-			continue;
-
-		intel_crtc_state = to_intel_crtc_state(
-			drm_atomic_get_existing_crtc_state(state, &crtc->base));
-
-		intel_crtc_state->enable_fbc = true;
-		crtc_chosen = true;
-		break;
-	}
-
-	if (!crtc_chosen)
-		fbc->no_fbc_reason = "no suitable CRTC for FBC";
+		return;
+	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
+		return;
+	if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
+		return;
+	if (!plane_state->base.visible)
+		return;
 
-out:
-	mutex_unlock(&fbc->lock);
+	plane_state->fbc_score = 1;
 }
 
 /**
@@ -1114,6 +1078,13 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
  * possible. Notice that it doesn't activate FBC. It is valid to call
  * intel_fbc_enable multiple times for the same pipe without an
  * intel_fbc_disable in the middle, as long as it is deactivated.
+ *
+ * In the future this function could make better use of the fbc_score variable.
+ * We could, for example, loop through all the primary planes involved in the
+ * atomic commit and only enable FBC for the plane with the best fbc_score. We
+ * could also try to do some scheme where a plane with better score takes over
+ * FBC from another plane, but our driver currently can't handle the complexity
+ * of switching planes on the fly. This would only affect from Gen 4 up to IVB.
  */
 void intel_fbc_enable(struct intel_crtc *crtc,
 		      struct intel_crtc_state *crtc_state,
@@ -1130,14 +1101,16 @@ void intel_fbc_enable(struct intel_crtc *crtc,
 	if (fbc->enabled) {
 		WARN_ON(fbc->crtc == NULL);
 		if (fbc->crtc == crtc) {
-			WARN_ON(!crtc_state->enable_fbc);
+			WARN_ON(plane_state->fbc_score == 0);
 			WARN_ON(fbc->active);
 		}
 		goto out;
 	}
 
-	if (!crtc_state->enable_fbc)
+	if (plane_state->fbc_score == 0) {
+		fbc->no_fbc_reason = "no suitable CRTC for FBC";
 		goto out;
+	}
 
 	WARN_ON(fbc->active);
 	WARN_ON(fbc->crtc != NULL);
-- 
2.7.4

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

Re: [PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

[Ville Syrjälä]

On Fri, Dec 23, 2016 at 10:23:59AM -0200, Paulo Zanoni wrote:
> Ville pointed out that intel_fbc_choose_crtc() is iterating over all
> planes instead of just the primary planes. There are no real
> consequences of this problem for HSW+, and for the other platforms it
> just means that in some obscure multi-screen cases we'll keep FBC
> disabled when we could have enabled it. Still, iterating over all
> planes doesn't seem to be the best thing to do.
> 
> My initial idea was to just add a check for plane->type and be done,
> but then I realized that in commits not involving the primary plane we
> would reset crtc_state->enable_fbc back to false even when FBC is
> enabled. That also wouldn't result in a bug due to the way the
> enable_fbc variable is checked, but, still, our code can be better
> than this.
> 
> So I went for the solution that involves tracking enable_fbc in the
> primary plane state instead of the CRTC state. This way, if a commit
> doesn't involve the primary plane for the CRTC we won't be resetting
> enable_fbc back to false, so the variable will always reflect the
> reality. And this change also makes more sense since FBC is actually
> tied to the single plane and not the full pipe. As a bonus, we only
> iterate over the CRTCs instead of iterating over all planes.
> 
> v2:
> 
> But Ville pointed that this only works properly if we have a single
> commit with multiple CRTCs. If we have multiple parallel commits, each
> with its own CRTC, we'll end with enable_fbc set to true in more than
> one plane.
> 
> So the solution was to rename enable_fbc to can_enable_fbc. If we just
> did the rename as the patch was, we'd end up with a single plane with
> can_enable_fbc on commits involving multiple CRTCs: we'd choose the
> best one, but we'd still end up with a variable that doesn't 100%
> reflect reality.
> 
> So in the end I adopted Ville's suggestion of the fbc_score variable.
> And then, instead of checking the score at intel_fbc_choose_crtc()
> it should be possible to check for the score at intel_fbc_enable().
> This allows us to simplify intel_fbc_choose_crtc() to the point where
> it only needs to look at the given plane in order to assign its score
> instead of looking at every primary plane on the same commit.
> 
> We still only set scores of 0 and 1 and we don't really do the
> score-checking loop.
> 
> v3: Rebase.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  3 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  8 ++--
>  drivers/gpu/drm/i915/intel_fbc.c     | 85 ++++++++++++------------------------
>  3 files changed, 35 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d8effd4..92527d6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14191,7 +14191,6 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> -	intel_fbc_choose_crtc(dev_priv, state);
>  	return calc_watermark_data(state);
>  }
>  
> @@ -14966,6 +14965,8 @@ intel_check_primary_plane(struct drm_plane *plane,
>  			return ret;
>  	}
>  
> +	intel_fbc_check_plane(state);
> +

We have another 'return 0' a little earlier in the function for the
"plane disable by the user" case. Presumably we'll want to update
the score in that case as well?

>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 025e4c8..7430082 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -406,6 +406,9 @@ struct intel_plane_state {
>  	 */
>  	int scaler_id;
>  
> +	/* 0: not suitable for FBC, 1+: suitable for FBC, more is better. */
> +	unsigned int fbc_score;
> +
>  	struct drm_intel_sprite_colorkey ckey;
>  };
>  
> @@ -652,8 +655,6 @@ struct intel_crtc_state {
>  
>  	bool ips_enabled;
>  
> -	bool enable_fbc;
> -
>  	bool double_wide;
>  
>  	int pbn;
> @@ -1535,8 +1536,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
>  #endif
>  
>  /* intel_fbc.c */
> -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> -			   struct drm_atomic_state *state);
> +void intel_fbc_check_plane(struct intel_plane_state *plane_state);
>  bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
>  void intel_fbc_pre_update(struct intel_crtc *crtc,
>  			  struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 26a81a9..ceda6f4 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1040,68 +1040,32 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  }
>  
>  /**
> - * intel_fbc_choose_crtc - select a CRTC to enable FBC on
> - * @dev_priv: i915 device instance
> - * @state: the atomic state structure
> + * intel_fbc_check_plane - check plane for FBC suitability
> + * @plane_state: the plane state
>   *
> - * This function looks at the proposed state for CRTCs and planes, then chooses
> - * which pipe is going to have FBC by setting intel_crtc_state->enable_fbc to
> - * true.
> + * This function should set fbc_score based on how suitable the plane is for
> + * FBC. For now the only scores used are 0 and 1.
>   *
> - * Later, intel_fbc_enable is going to look for state->enable_fbc and then maybe
> - * enable FBC for the chosen CRTC. If it does, it will set dev_priv->fbc.crtc.
> + * We're not changing dev_priv->fbc, so there's no need for the FBC lock.
>   */
> -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> -			   struct drm_atomic_state *state)
> +void intel_fbc_check_plane(struct intel_plane_state *plane_state)
>  {
> -	struct intel_fbc *fbc = &dev_priv->fbc;
> -	struct drm_plane *plane;
> -	struct drm_plane_state *plane_state;
> -	bool crtc_chosen = false;
> -	int i;
> +	struct drm_plane *plane = plane_state->base.plane;
> +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> +	struct intel_crtc *crtc = to_intel_crtc(plane_state->base.crtc);

What happens if we don't have a crtc?

>  
> -	mutex_lock(&fbc->lock);
> -
> -	/* Does this atomic commit involve the CRTC currently tied to FBC? */
> -	if (fbc->crtc &&
> -	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc->base))
> -		goto out;
> +	plane_state->fbc_score = 0;
>  
>  	if (!intel_fbc_can_enable(dev_priv))
> -		goto out;
> -
> -	/* Simply choose the first CRTC that is compatible and has a visible
> -	 * plane. We could go for fancier schemes such as checking the plane
> -	 * size, but this would just affect the few platforms that don't tie FBC
> -	 * to pipe or plane A. */
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> -		struct intel_plane_state *intel_plane_state =
> -			to_intel_plane_state(plane_state);
> -		struct intel_crtc_state *intel_crtc_state;
> -		struct intel_crtc *crtc = to_intel_crtc(plane_state->crtc);
> -
> -		if (!intel_plane_state->base.visible)
> -			continue;
> -
> -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> -			continue;
> -
> -		if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
> -			continue;
> -
> -		intel_crtc_state = to_intel_crtc_state(
> -			drm_atomic_get_existing_crtc_state(state, &crtc->base));
> -
> -		intel_crtc_state->enable_fbc = true;
> -		crtc_chosen = true;
> -		break;
> -	}
> -
> -	if (!crtc_chosen)
> -		fbc->no_fbc_reason = "no suitable CRTC for FBC";
> +		return;
> +	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> +		return;
> +	if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
> +		return;
> +	if (!plane_state->base.visible)
> +		return;
>  
> -out:
> -	mutex_unlock(&fbc->lock);
> +	plane_state->fbc_score = 1;
>  }
>  
>  /**
> @@ -1114,6 +1078,13 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
>   * possible. Notice that it doesn't activate FBC. It is valid to call
>   * intel_fbc_enable multiple times for the same pipe without an
>   * intel_fbc_disable in the middle, as long as it is deactivated.
> + *
> + * In the future this function could make better use of the fbc_score variable.
> + * We could, for example, loop through all the primary planes involved in the
> + * atomic commit and only enable FBC for the plane with the best fbc_score. We
> + * could also try to do some scheme where a plane with better score takes over
> + * FBC from another plane, but our driver currently can't handle the complexity
> + * of switching planes on the fly. This would only affect from Gen 4 up to IVB.
>   */
>  void intel_fbc_enable(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *crtc_state,
> @@ -1130,14 +1101,16 @@ void intel_fbc_enable(struct intel_crtc *crtc,
>  	if (fbc->enabled) {
>  		WARN_ON(fbc->crtc == NULL);
>  		if (fbc->crtc == crtc) {
> -			WARN_ON(!crtc_state->enable_fbc);
> +			WARN_ON(plane_state->fbc_score == 0);
>  			WARN_ON(fbc->active);
>  		}
>  		goto out;
>  	}
>  
> -	if (!crtc_state->enable_fbc)
> +	if (plane_state->fbc_score == 0) {
> +		fbc->no_fbc_reason = "no suitable CRTC for FBC";
>  		goto out;
> +	}
>  
>  	WARN_ON(fbc->active);
>  	WARN_ON(fbc->crtc != NULL);
> -- 
> 2.7.4

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

✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: enable FBC on gen9+ too

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: enable FBC on gen9+ too
URL   : https://patchwork.freedesktop.org/series/17176/
State : warning

== Summary ==

Series 17176v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/17176/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> DMESG-WARN (fi-snb-2520m)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-snb-2520m)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:82   pass:69   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:213  dwarn:2   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

7165fdc7f0b0b536557fcd0a222d083a901be57c drm-tip: 2016y-12m-22d-19h-42m-25s UTC integration manifest
5fdaf94 drm/i915: rewrite FBC's atomic CRTC-choosing code
be5982e drm/i915: enable FBC on gen9+ too

== Logs ==

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

Re: [PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

[Paulo Zanoni]

Em Sex, 2016-12-23 às 14:43 +0200, Ville Syrjälä escreveu:
> On Fri, Dec 23, 2016 at 10:23:59AM -0200, Paulo Zanoni wrote:
> > 
> > Ville pointed out that intel_fbc_choose_crtc() is iterating over
> > all
> > planes instead of just the primary planes. There are no real
> > consequences of this problem for HSW+, and for the other platforms
> > it
> > just means that in some obscure multi-screen cases we'll keep FBC
> > disabled when we could have enabled it. Still, iterating over all
> > planes doesn't seem to be the best thing to do.
> > 
> > My initial idea was to just add a check for plane->type and be
> > done,
> > but then I realized that in commits not involving the primary plane
> > we
> > would reset crtc_state->enable_fbc back to false even when FBC is
> > enabled. That also wouldn't result in a bug due to the way the
> > enable_fbc variable is checked, but, still, our code can be better
> > than this.
> > 
> > So I went for the solution that involves tracking enable_fbc in the
> > primary plane state instead of the CRTC state. This way, if a
> > commit
> > doesn't involve the primary plane for the CRTC we won't be
> > resetting
> > enable_fbc back to false, so the variable will always reflect the
> > reality. And this change also makes more sense since FBC is
> > actually
> > tied to the single plane and not the full pipe. As a bonus, we only
> > iterate over the CRTCs instead of iterating over all planes.
> > 
> > v2:
> > 
> > But Ville pointed that this only works properly if we have a single
> > commit with multiple CRTCs. If we have multiple parallel commits,
> > each
> > with its own CRTC, we'll end with enable_fbc set to true in more
> > than
> > one plane.
> > 
> > So the solution was to rename enable_fbc to can_enable_fbc. If we
> > just
> > did the rename as the patch was, we'd end up with a single plane
> > with
> > can_enable_fbc on commits involving multiple CRTCs: we'd choose the
> > best one, but we'd still end up with a variable that doesn't 100%
> > reflect reality.
> > 
> > So in the end I adopted Ville's suggestion of the fbc_score
> > variable.
> > And then, instead of checking the score at intel_fbc_choose_crtc()
> > it should be possible to check for the score at intel_fbc_enable().
> > This allows us to simplify intel_fbc_choose_crtc() to the point
> > where
> > it only needs to look at the given plane in order to assign its
> > score
> > instead of looking at every primary plane on the same commit.
> > 
> > We still only set scores of 0 and 1 and we don't really do the
> > score-checking loop.
> > 
> > v3: Rebase.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  3 +-
> >  drivers/gpu/drm/i915/intel_drv.h     |  8 ++--
> >  drivers/gpu/drm/i915/intel_fbc.c     | 85 ++++++++++++----------
> > --------------
> >  3 files changed, 35 insertions(+), 61 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index d8effd4..92527d6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14191,7 +14191,6 @@ static int intel_atomic_check(struct
> > drm_device *dev,
> >  	if (ret)
> >  		return ret;
> >  
> > -	intel_fbc_choose_crtc(dev_priv, state);
> >  	return calc_watermark_data(state);
> >  }
> >  
> > @@ -14966,6 +14965,8 @@ intel_check_primary_plane(struct drm_plane
> > *plane,
> >  			return ret;
> >  	}
> >  
> > +	intel_fbc_check_plane(state);
> > +
> 
> We have another 'return 0' a little earlier in the function for the
> "plane disable by the user" case. Presumably we'll want to update
> the score in that case as well?

I always assumed the state was zero-allocated, not copied, and int his
case this wouldn't be a problem... Thanks for pointing this to me.

I'll move it up. Can't move up further since we need plane_state-
>visible to be set.


> 
> > 
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 025e4c8..7430082 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -406,6 +406,9 @@ struct intel_plane_state {
> >  	 */
> >  	int scaler_id;
> >  
> > +	/* 0: not suitable for FBC, 1+: suitable for FBC, more is
> > better. */
> > +	unsigned int fbc_score;
> > +
> >  	struct drm_intel_sprite_colorkey ckey;
> >  };
> >  
> > @@ -652,8 +655,6 @@ struct intel_crtc_state {
> >  
> >  	bool ips_enabled;
> >  
> > -	bool enable_fbc;
> > -
> >  	bool double_wide;
> >  
> >  	int pbn;
> > @@ -1535,8 +1536,7 @@ static inline void
> > intel_fbdev_restore_mode(struct drm_device *dev)
> >  #endif
> >  
> >  /* intel_fbc.c */
> > -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> > -			   struct drm_atomic_state *state);
> > +void intel_fbc_check_plane(struct intel_plane_state *plane_state);
> >  bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
> >  void intel_fbc_pre_update(struct intel_crtc *crtc,
> >  			  struct intel_crtc_state *crtc_state,
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index 26a81a9..ceda6f4 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -1040,68 +1040,32 @@ void intel_fbc_flush(struct
> > drm_i915_private *dev_priv,
> >  }
> >  
> >  /**
> > - * intel_fbc_choose_crtc - select a CRTC to enable FBC on
> > - * @dev_priv: i915 device instance
> > - * @state: the atomic state structure
> > + * intel_fbc_check_plane - check plane for FBC suitability
> > + * @plane_state: the plane state
> >   *
> > - * This function looks at the proposed state for CRTCs and planes,
> > then chooses
> > - * which pipe is going to have FBC by setting intel_crtc_state-
> > >enable_fbc to
> > - * true.
> > + * This function should set fbc_score based on how suitable the
> > plane is for
> > + * FBC. For now the only scores used are 0 and 1.
> >   *
> > - * Later, intel_fbc_enable is going to look for state->enable_fbc
> > and then maybe
> > - * enable FBC for the chosen CRTC. If it does, it will set
> > dev_priv->fbc.crtc.
> > + * We're not changing dev_priv->fbc, so there's no need for the
> > FBC lock.
> >   */
> > -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> > -			   struct drm_atomic_state *state)
> > +void intel_fbc_check_plane(struct intel_plane_state *plane_state)
> >  {
> > -	struct intel_fbc *fbc = &dev_priv->fbc;
> > -	struct drm_plane *plane;
> > -	struct drm_plane_state *plane_state;
> > -	bool crtc_chosen = false;
> > -	int i;
> > +	struct drm_plane *plane = plane_state->base.plane;
> > +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> > +	struct intel_crtc *crtc = to_intel_crtc(plane_state-
> > >base.crtc);
> 
> What happens if we don't have a crtc?

It's been confirmed to me many times that if a plane is being
committed, its CRTC is included in the atomic commit. Was this what you
were talking aboue? Can you give me an example where we wouldn't have a
CRTC?


> > 
> >  
> > -	mutex_lock(&fbc->lock);
> > -
> > -	/* Does this atomic commit involve the CRTC currently tied
> > to FBC? */
> > -	if (fbc->crtc &&
> > -	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc-
> > >base))
> > -		goto out;
> > +	plane_state->fbc_score = 0;
> >  
> >  	if (!intel_fbc_can_enable(dev_priv))
> > -		goto out;
> > -
> > -	/* Simply choose the first CRTC that is compatible and has
> > a visible
> > -	 * plane. We could go for fancier schemes such as checking
> > the plane
> > -	 * size, but this would just affect the few platforms that
> > don't tie FBC
> > -	 * to pipe or plane A. */
> > -	for_each_plane_in_state(state, plane, plane_state, i) {
> > -		struct intel_plane_state *intel_plane_state =
> > -			to_intel_plane_state(plane_state);
> > -		struct intel_crtc_state *intel_crtc_state;
> > -		struct intel_crtc *crtc =
> > to_intel_crtc(plane_state->crtc);
> > -
> > -		if (!intel_plane_state->base.visible)
> > -			continue;
> > -
> > -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe !=
> > PIPE_A)
> > -			continue;
> > -
> > -		if (fbc_on_plane_a_only(dev_priv) && crtc->plane
> > != PLANE_A)
> > -			continue;
> > -
> > -		intel_crtc_state = to_intel_crtc_state(
> > -			drm_atomic_get_existing_crtc_state(state,
> > &crtc->base));
> > -
> > -		intel_crtc_state->enable_fbc = true;
> > -		crtc_chosen = true;
> > -		break;
> > -	}
> > -
> > -	if (!crtc_chosen)
> > -		fbc->no_fbc_reason = "no suitable CRTC for FBC";
> > +		return;
> > +	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> > +		return;
> > +	if (fbc_on_plane_a_only(dev_priv) && crtc->plane !=
> > PLANE_A)
> > +		return;
> > +	if (!plane_state->base.visible)
> > +		return;
> >  
> > -out:
> > -	mutex_unlock(&fbc->lock);
> > +	plane_state->fbc_score = 1;
> >  }
> >  
> >  /**
> > @@ -1114,6 +1078,13 @@ void intel_fbc_choose_crtc(struct
> > drm_i915_private *dev_priv,
> >   * possible. Notice that it doesn't activate FBC. It is valid to
> > call
> >   * intel_fbc_enable multiple times for the same pipe without an
> >   * intel_fbc_disable in the middle, as long as it is deactivated.
> > + *
> > + * In the future this function could make better use of the
> > fbc_score variable.
> > + * We could, for example, loop through all the primary planes
> > involved in the
> > + * atomic commit and only enable FBC for the plane with the best
> > fbc_score. We
> > + * could also try to do some scheme where a plane with better
> > score takes over
> > + * FBC from another plane, but our driver currently can't handle
> > the complexity
> > + * of switching planes on the fly. This would only affect from Gen
> > 4 up to IVB.
> >   */
> >  void intel_fbc_enable(struct intel_crtc *crtc,
> >  		      struct intel_crtc_state *crtc_state,
> > @@ -1130,14 +1101,16 @@ void intel_fbc_enable(struct intel_crtc
> > *crtc,
> >  	if (fbc->enabled) {
> >  		WARN_ON(fbc->crtc == NULL);
> >  		if (fbc->crtc == crtc) {
> > -			WARN_ON(!crtc_state->enable_fbc);
> > +			WARN_ON(plane_state->fbc_score == 0);
> >  			WARN_ON(fbc->active);
> >  		}
> >  		goto out;
> >  	}
> >  
> > -	if (!crtc_state->enable_fbc)
> > +	if (plane_state->fbc_score == 0) {
> > +		fbc->no_fbc_reason = "no suitable CRTC for FBC";
> >  		goto out;
> > +	}
> >  
> >  	WARN_ON(fbc->active);
> >  	WARN_ON(fbc->crtc != NULL);
> > -- 
> > 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

[Ville Syrjälä]

On Fri, Dec 23, 2016 at 11:23:56AM -0200, Paulo Zanoni wrote:
> Em Sex, 2016-12-23 às 14:43 +0200, Ville Syrjälä escreveu:
> > On Fri, Dec 23, 2016 at 10:23:59AM -0200, Paulo Zanoni wrote:
> > > 
> > > Ville pointed out that intel_fbc_choose_crtc() is iterating over
> > > all
> > > planes instead of just the primary planes. There are no real
> > > consequences of this problem for HSW+, and for the other platforms
> > > it
> > > just means that in some obscure multi-screen cases we'll keep FBC
> > > disabled when we could have enabled it. Still, iterating over all
> > > planes doesn't seem to be the best thing to do.
> > > 
> > > My initial idea was to just add a check for plane->type and be
> > > done,
> > > but then I realized that in commits not involving the primary plane
> > > we
> > > would reset crtc_state->enable_fbc back to false even when FBC is
> > > enabled. That also wouldn't result in a bug due to the way the
> > > enable_fbc variable is checked, but, still, our code can be better
> > > than this.
> > > 
> > > So I went for the solution that involves tracking enable_fbc in the
> > > primary plane state instead of the CRTC state. This way, if a
> > > commit
> > > doesn't involve the primary plane for the CRTC we won't be
> > > resetting
> > > enable_fbc back to false, so the variable will always reflect the
> > > reality. And this change also makes more sense since FBC is
> > > actually
> > > tied to the single plane and not the full pipe. As a bonus, we only
> > > iterate over the CRTCs instead of iterating over all planes.
> > > 
> > > v2:
> > > 
> > > But Ville pointed that this only works properly if we have a single
> > > commit with multiple CRTCs. If we have multiple parallel commits,
> > > each
> > > with its own CRTC, we'll end with enable_fbc set to true in more
> > > than
> > > one plane.
> > > 
> > > So the solution was to rename enable_fbc to can_enable_fbc. If we
> > > just
> > > did the rename as the patch was, we'd end up with a single plane
> > > with
> > > can_enable_fbc on commits involving multiple CRTCs: we'd choose the
> > > best one, but we'd still end up with a variable that doesn't 100%
> > > reflect reality.
> > > 
> > > So in the end I adopted Ville's suggestion of the fbc_score
> > > variable.
> > > And then, instead of checking the score at intel_fbc_choose_crtc()
> > > it should be possible to check for the score at intel_fbc_enable().
> > > This allows us to simplify intel_fbc_choose_crtc() to the point
> > > where
> > > it only needs to look at the given plane in order to assign its
> > > score
> > > instead of looking at every primary plane on the same commit.
> > > 
> > > We still only set scores of 0 and 1 and we don't really do the
> > > score-checking loop.
> > > 
> > > v3: Rebase.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |  3 +-
> > >  drivers/gpu/drm/i915/intel_drv.h     |  8 ++--
> > >  drivers/gpu/drm/i915/intel_fbc.c     | 85 ++++++++++++----------
> > > --------------
> > >  3 files changed, 35 insertions(+), 61 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index d8effd4..92527d6 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -14191,7 +14191,6 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	intel_fbc_choose_crtc(dev_priv, state);
> > >  	return calc_watermark_data(state);
> > >  }
> > >  
> > > @@ -14966,6 +14965,8 @@ intel_check_primary_plane(struct drm_plane
> > > *plane,
> > >  			return ret;
> > >  	}
> > >  
> > > +	intel_fbc_check_plane(state);
> > > +
> > 
> > We have another 'return 0' a little earlier in the function for the
> > "plane disable by the user" case. Presumably we'll want to update
> > the score in that case as well?
> 
> I always assumed the state was zero-allocated, not copied, and int his
> case this wouldn't be a problem... Thanks for pointing this to me.
> 
> I'll move it up. Can't move up further since we need plane_state-
> >visible to be set.

Maybe we just want to clear the score to 0 when visible==false?

> 
> 
> > 
> > > 
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 025e4c8..7430082 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -406,6 +406,9 @@ struct intel_plane_state {
> > >  	 */
> > >  	int scaler_id;
> > >  
> > > +	/* 0: not suitable for FBC, 1+: suitable for FBC, more is
> > > better. */
> > > +	unsigned int fbc_score;
> > > +
> > >  	struct drm_intel_sprite_colorkey ckey;
> > >  };
> > >  
> > > @@ -652,8 +655,6 @@ struct intel_crtc_state {
> > >  
> > >  	bool ips_enabled;
> > >  
> > > -	bool enable_fbc;
> > > -
> > >  	bool double_wide;
> > >  
> > >  	int pbn;
> > > @@ -1535,8 +1536,7 @@ static inline void
> > > intel_fbdev_restore_mode(struct drm_device *dev)
> > >  #endif
> > >  
> > >  /* intel_fbc.c */
> > > -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> > > -			   struct drm_atomic_state *state);
> > > +void intel_fbc_check_plane(struct intel_plane_state *plane_state);
> > >  bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
> > >  void intel_fbc_pre_update(struct intel_crtc *crtc,
> > >  			  struct intel_crtc_state *crtc_state,
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index 26a81a9..ceda6f4 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -1040,68 +1040,32 @@ void intel_fbc_flush(struct
> > > drm_i915_private *dev_priv,
> > >  }
> > >  
> > >  /**
> > > - * intel_fbc_choose_crtc - select a CRTC to enable FBC on
> > > - * @dev_priv: i915 device instance
> > > - * @state: the atomic state structure
> > > + * intel_fbc_check_plane - check plane for FBC suitability
> > > + * @plane_state: the plane state
> > >   *
> > > - * This function looks at the proposed state for CRTCs and planes,
> > > then chooses
> > > - * which pipe is going to have FBC by setting intel_crtc_state-
> > > >enable_fbc to
> > > - * true.
> > > + * This function should set fbc_score based on how suitable the
> > > plane is for
> > > + * FBC. For now the only scores used are 0 and 1.
> > >   *
> > > - * Later, intel_fbc_enable is going to look for state->enable_fbc
> > > and then maybe
> > > - * enable FBC for the chosen CRTC. If it does, it will set
> > > dev_priv->fbc.crtc.
> > > + * We're not changing dev_priv->fbc, so there's no need for the
> > > FBC lock.
> > >   */
> > > -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> > > -			   struct drm_atomic_state *state)
> > > +void intel_fbc_check_plane(struct intel_plane_state *plane_state)
> > >  {
> > > -	struct intel_fbc *fbc = &dev_priv->fbc;
> > > -	struct drm_plane *plane;
> > > -	struct drm_plane_state *plane_state;
> > > -	bool crtc_chosen = false;
> > > -	int i;
> > > +	struct drm_plane *plane = plane_state->base.plane;
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> > > +	struct intel_crtc *crtc = to_intel_crtc(plane_state-
> > > >base.crtc);
> > 
> > What happens if we don't have a crtc?
> 
> It's been confirmed to me many times that if a plane is being
> committed, its CRTC is included in the atomic commit. Was this what you
> were talking aboue? Can you give me an example where we wouldn't have a
> CRTC?
> 
> 
> > > 
> > >  
> > > -	mutex_lock(&fbc->lock);
> > > -
> > > -	/* Does this atomic commit involve the CRTC currently tied
> > > to FBC? */
> > > -	if (fbc->crtc &&
> > > -	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc-
> > > >base))
> > > -		goto out;
> > > +	plane_state->fbc_score = 0;
> > >  
> > >  	if (!intel_fbc_can_enable(dev_priv))
> > > -		goto out;
> > > -
> > > -	/* Simply choose the first CRTC that is compatible and has
> > > a visible
> > > -	 * plane. We could go for fancier schemes such as checking
> > > the plane
> > > -	 * size, but this would just affect the few platforms that
> > > don't tie FBC
> > > -	 * to pipe or plane A. */
> > > -	for_each_plane_in_state(state, plane, plane_state, i) {
> > > -		struct intel_plane_state *intel_plane_state =
> > > -			to_intel_plane_state(plane_state);
> > > -		struct intel_crtc_state *intel_crtc_state;
> > > -		struct intel_crtc *crtc =
> > > to_intel_crtc(plane_state->crtc);
> > > -
> > > -		if (!intel_plane_state->base.visible)
> > > -			continue;
> > > -
> > > -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe !=
> > > PIPE_A)
> > > -			continue;
> > > -
> > > -		if (fbc_on_plane_a_only(dev_priv) && crtc->plane
> > > != PLANE_A)
> > > -			continue;
> > > -
> > > -		intel_crtc_state = to_intel_crtc_state(
> > > -			drm_atomic_get_existing_crtc_state(state,
> > > &crtc->base));
> > > -
> > > -		intel_crtc_state->enable_fbc = true;
> > > -		crtc_chosen = true;
> > > -		break;
> > > -	}
> > > -
> > > -	if (!crtc_chosen)
> > > -		fbc->no_fbc_reason = "no suitable CRTC for FBC";
> > > +		return;
> > > +	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> > > +		return;
> > > +	if (fbc_on_plane_a_only(dev_priv) && crtc->plane !=
> > > PLANE_A)
> > > +		return;
> > > +	if (!plane_state->base.visible)
> > > +		return;
> > >  
> > > -out:
> > > -	mutex_unlock(&fbc->lock);
> > > +	plane_state->fbc_score = 1;
> > >  }
> > >  
> > >  /**
> > > @@ -1114,6 +1078,13 @@ void intel_fbc_choose_crtc(struct
> > > drm_i915_private *dev_priv,
> > >   * possible. Notice that it doesn't activate FBC. It is valid to
> > > call
> > >   * intel_fbc_enable multiple times for the same pipe without an
> > >   * intel_fbc_disable in the middle, as long as it is deactivated.
> > > + *
> > > + * In the future this function could make better use of the
> > > fbc_score variable.
> > > + * We could, for example, loop through all the primary planes
> > > involved in the
> > > + * atomic commit and only enable FBC for the plane with the best
> > > fbc_score. We
> > > + * could also try to do some scheme where a plane with better
> > > score takes over
> > > + * FBC from another plane, but our driver currently can't handle
> > > the complexity
> > > + * of switching planes on the fly. This would only affect from Gen
> > > 4 up to IVB.
> > >   */
> > >  void intel_fbc_enable(struct intel_crtc *crtc,
> > >  		      struct intel_crtc_state *crtc_state,
> > > @@ -1130,14 +1101,16 @@ void intel_fbc_enable(struct intel_crtc
> > > *crtc,
> > >  	if (fbc->enabled) {
> > >  		WARN_ON(fbc->crtc == NULL);
> > >  		if (fbc->crtc == crtc) {
> > > -			WARN_ON(!crtc_state->enable_fbc);
> > > +			WARN_ON(plane_state->fbc_score == 0);
> > >  			WARN_ON(fbc->active);
> > >  		}
> > >  		goto out;
> > >  	}
> > >  
> > > -	if (!crtc_state->enable_fbc)
> > > +	if (plane_state->fbc_score == 0) {
> > > +		fbc->no_fbc_reason = "no suitable CRTC for FBC";
> > >  		goto out;
> > > +	}
> > >  
> > >  	WARN_ON(fbc->active);
> > >  	WARN_ON(fbc->crtc != NULL);
> > > -- 
> > > 2.7.4
> > 

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

[PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

[Paulo Zanoni]

Ville pointed out that intel_fbc_choose_crtc() is iterating over all
planes instead of just the primary planes. There are no real
consequences of this problem for HSW+, and for the other platforms it
just means that in some obscure multi-screen cases we'll keep FBC
disabled when we could have enabled it. Still, iterating over all
planes doesn't seem to be the best thing to do.

My initial idea was to just add a check for plane->type and be done,
but then I realized that in commits not involving the primary plane we
would reset crtc_state->enable_fbc back to false even when FBC is
enabled. That also wouldn't result in a bug due to the way the
enable_fbc variable is checked, but, still, our code can be better
than this.

So I went for the solution that involves tracking enable_fbc in the
primary plane state instead of the CRTC state. This way, if a commit
doesn't involve the primary plane for the CRTC we won't be resetting
enable_fbc back to false, so the variable will always reflect the
reality. And this change also makes more sense since FBC is actually
tied to the single plane and not the full pipe. As a bonus, we only
iterate over the CRTCs instead of iterating over all planes.

v2:

But Ville pointed that this only works properly if we have a single
commit with multiple CRTCs. If we have multiple parallel commits, each
with its own CRTC, we'll end with enable_fbc set to true in more than
one plane.

So the solution was to rename enable_fbc to can_enable_fbc. If we just
did the rename as the patch was, we'd end up with a single plane with
can_enable_fbc on commits involving multiple CRTCs: we'd choose the
best one, but we'd still end up with a variable that doesn't 100%
reflect reality.

So in the end I adopted Ville's suggestion of the fbc_score variable.
And then, instead of checking the score at intel_fbc_choose_crtc()
it should be possible to check for the score at intel_fbc_enable().
This allows us to simplify intel_fbc_choose_crtc() to the point where
it only needs to look at the given plane in order to assign its score
instead of looking at every primary plane on the same commit.

We still only set scores of 0 and 1 and we don't really do the
score-checking loop.

v3: Rebase.

v4:

Move intel_fbc_check_plane() call up so it will still zero the score
if the plane has no FB. (Ville).

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h     |  8 ++--
 drivers/gpu/drm/i915/intel_fbc.c     | 85 ++++++++++++------------------------
 3 files changed, 35 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d8effd4..56ae32c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14191,7 +14191,6 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	intel_fbc_choose_crtc(dev_priv, state);
 	return calc_watermark_data(state);
 }
 
@@ -14957,6 +14956,8 @@ intel_check_primary_plane(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
+	intel_fbc_check_plane(state);
+
 	if (!state->base.fb)
 		return 0;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 025e4c8..7430082 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -406,6 +406,9 @@ struct intel_plane_state {
 	 */
 	int scaler_id;
 
+	/* 0: not suitable for FBC, 1+: suitable for FBC, more is better. */
+	unsigned int fbc_score;
+
 	struct drm_intel_sprite_colorkey ckey;
 };
 
@@ -652,8 +655,6 @@ struct intel_crtc_state {
 
 	bool ips_enabled;
 
-	bool enable_fbc;
-
 	bool double_wide;
 
 	int pbn;
@@ -1535,8 +1536,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
 #endif
 
 /* intel_fbc.c */
-void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
-			   struct drm_atomic_state *state);
+void intel_fbc_check_plane(struct intel_plane_state *plane_state);
 bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
 void intel_fbc_pre_update(struct intel_crtc *crtc,
 			  struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 26a81a9..ceda6f4 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1040,68 +1040,32 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_fbc_choose_crtc - select a CRTC to enable FBC on
- * @dev_priv: i915 device instance
- * @state: the atomic state structure
+ * intel_fbc_check_plane - check plane for FBC suitability
+ * @plane_state: the plane state
  *
- * This function looks at the proposed state for CRTCs and planes, then chooses
- * which pipe is going to have FBC by setting intel_crtc_state->enable_fbc to
- * true.
+ * This function should set fbc_score based on how suitable the plane is for
+ * FBC. For now the only scores used are 0 and 1.
  *
- * Later, intel_fbc_enable is going to look for state->enable_fbc and then maybe
- * enable FBC for the chosen CRTC. If it does, it will set dev_priv->fbc.crtc.
+ * We're not changing dev_priv->fbc, so there's no need for the FBC lock.
  */
-void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
-			   struct drm_atomic_state *state)
+void intel_fbc_check_plane(struct intel_plane_state *plane_state)
 {
-	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
-	bool crtc_chosen = false;
-	int i;
+	struct drm_plane *plane = plane_state->base.plane;
+	struct drm_i915_private *dev_priv = to_i915(plane->dev);
+	struct intel_crtc *crtc = to_intel_crtc(plane_state->base.crtc);
 
-	mutex_lock(&fbc->lock);
-
-	/* Does this atomic commit involve the CRTC currently tied to FBC? */
-	if (fbc->crtc &&
-	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc->base))
-		goto out;
+	plane_state->fbc_score = 0;
 
 	if (!intel_fbc_can_enable(dev_priv))
-		goto out;
-
-	/* Simply choose the first CRTC that is compatible and has a visible
-	 * plane. We could go for fancier schemes such as checking the plane
-	 * size, but this would just affect the few platforms that don't tie FBC
-	 * to pipe or plane A. */
-	for_each_plane_in_state(state, plane, plane_state, i) {
-		struct intel_plane_state *intel_plane_state =
-			to_intel_plane_state(plane_state);
-		struct intel_crtc_state *intel_crtc_state;
-		struct intel_crtc *crtc = to_intel_crtc(plane_state->crtc);
-
-		if (!intel_plane_state->base.visible)
-			continue;
-
-		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
-			continue;
-
-		if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
-			continue;
-
-		intel_crtc_state = to_intel_crtc_state(
-			drm_atomic_get_existing_crtc_state(state, &crtc->base));
-
-		intel_crtc_state->enable_fbc = true;
-		crtc_chosen = true;
-		break;
-	}
-
-	if (!crtc_chosen)
-		fbc->no_fbc_reason = "no suitable CRTC for FBC";
+		return;
+	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
+		return;
+	if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
+		return;
+	if (!plane_state->base.visible)
+		return;
 
-out:
-	mutex_unlock(&fbc->lock);
+	plane_state->fbc_score = 1;
 }
 
 /**
@@ -1114,6 +1078,13 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
  * possible. Notice that it doesn't activate FBC. It is valid to call
  * intel_fbc_enable multiple times for the same pipe without an
  * intel_fbc_disable in the middle, as long as it is deactivated.
+ *
+ * In the future this function could make better use of the fbc_score variable.
+ * We could, for example, loop through all the primary planes involved in the
+ * atomic commit and only enable FBC for the plane with the best fbc_score. We
+ * could also try to do some scheme where a plane with better score takes over
+ * FBC from another plane, but our driver currently can't handle the complexity
+ * of switching planes on the fly. This would only affect from Gen 4 up to IVB.
  */
 void intel_fbc_enable(struct intel_crtc *crtc,
 		      struct intel_crtc_state *crtc_state,
@@ -1130,14 +1101,16 @@ void intel_fbc_enable(struct intel_crtc *crtc,
 	if (fbc->enabled) {
 		WARN_ON(fbc->crtc == NULL);
 		if (fbc->crtc == crtc) {
-			WARN_ON(!crtc_state->enable_fbc);
+			WARN_ON(plane_state->fbc_score == 0);
 			WARN_ON(fbc->active);
 		}
 		goto out;
 	}
 
-	if (!crtc_state->enable_fbc)
+	if (plane_state->fbc_score == 0) {
+		fbc->no_fbc_reason = "no suitable CRTC for FBC";
 		goto out;
+	}
 
 	WARN_ON(fbc->active);
 	WARN_ON(fbc->crtc != NULL);
-- 
2.7.4

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

Re: [PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

[Paulo Zanoni]

Em Sex, 2016-12-23 às 13:29 -0200, Paulo Zanoni escreveu:
> Ville pointed out that intel_fbc_choose_crtc() is iterating over all
> planes instead of just the primary planes. There are no real
> consequences of this problem for HSW+, and for the other platforms it
> just means that in some obscure multi-screen cases we'll keep FBC
> disabled when we could have enabled it. Still, iterating over all
> planes doesn't seem to be the best thing to do.
> 
> My initial idea was to just add a check for plane->type and be done,
> but then I realized that in commits not involving the primary plane
> we
> would reset crtc_state->enable_fbc back to false even when FBC is
> enabled. That also wouldn't result in a bug due to the way the
> enable_fbc variable is checked, but, still, our code can be better
> than this.
> 
> So I went for the solution that involves tracking enable_fbc in the
> primary plane state instead of the CRTC state. This way, if a commit
> doesn't involve the primary plane for the CRTC we won't be resetting
> enable_fbc back to false, so the variable will always reflect the
> reality. And this change also makes more sense since FBC is actually
> tied to the single plane and not the full pipe. As a bonus, we only
> iterate over the CRTCs instead of iterating over all planes.
> 
> v2:
> 
> But Ville pointed that this only works properly if we have a single
> commit with multiple CRTCs. If we have multiple parallel commits,
> each
> with its own CRTC, we'll end with enable_fbc set to true in more than
> one plane.
> 
> So the solution was to rename enable_fbc to can_enable_fbc. If we
> just
> did the rename as the patch was, we'd end up with a single plane with
> can_enable_fbc on commits involving multiple CRTCs: we'd choose the
> best one, but we'd still end up with a variable that doesn't 100%
> reflect reality.
> 
> So in the end I adopted Ville's suggestion of the fbc_score variable.
> And then, instead of checking the score at intel_fbc_choose_crtc()
> it should be possible to check for the score at intel_fbc_enable().
> This allows us to simplify intel_fbc_choose_crtc() to the point where
> it only needs to look at the given plane in order to assign its score
> instead of looking at every primary plane on the same commit.
> 
> We still only set scores of 0 and 1 and we don't really do the
> score-checking loop.
> 
> v3: Rebase.
> 
> v4:
> 
> Move intel_fbc_check_plane() call up so it will still zero the score
> if the plane has no FB. (Ville).

Oh, ok, so after moving the call up the case where the CRTC is NULL can
actually happen. Previously it was prevented because we always ran the
function when we had an FB.

I recall this patch. Shouldn't have sent it so soon.

> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  3 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  8 ++--
>  drivers/gpu/drm/i915/intel_fbc.c     | 85 ++++++++++++------------
> ------------
>  3 files changed, 35 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index d8effd4..56ae32c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14191,7 +14191,6 @@ static int intel_atomic_check(struct
> drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> -	intel_fbc_choose_crtc(dev_priv, state);
>  	return calc_watermark_data(state);
>  }
>  
> @@ -14957,6 +14956,8 @@ intel_check_primary_plane(struct drm_plane
> *plane,
>  	if (ret)
>  		return ret;
>  
> +	intel_fbc_check_plane(state);
> +
>  	if (!state->base.fb)
>  		return 0;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 025e4c8..7430082 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -406,6 +406,9 @@ struct intel_plane_state {
>  	 */
>  	int scaler_id;
>  
> +	/* 0: not suitable for FBC, 1+: suitable for FBC, more is
> better. */
> +	unsigned int fbc_score;
> +
>  	struct drm_intel_sprite_colorkey ckey;
>  };
>  
> @@ -652,8 +655,6 @@ struct intel_crtc_state {
>  
>  	bool ips_enabled;
>  
> -	bool enable_fbc;
> -
>  	bool double_wide;
>  
>  	int pbn;
> @@ -1535,8 +1536,7 @@ static inline void
> intel_fbdev_restore_mode(struct drm_device *dev)
>  #endif
>  
>  /* intel_fbc.c */
> -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> -			   struct drm_atomic_state *state);
> +void intel_fbc_check_plane(struct intel_plane_state *plane_state);
>  bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
>  void intel_fbc_pre_update(struct intel_crtc *crtc,
>  			  struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index 26a81a9..ceda6f4 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1040,68 +1040,32 @@ void intel_fbc_flush(struct drm_i915_private
> *dev_priv,
>  }
>  
>  /**
> - * intel_fbc_choose_crtc - select a CRTC to enable FBC on
> - * @dev_priv: i915 device instance
> - * @state: the atomic state structure
> + * intel_fbc_check_plane - check plane for FBC suitability
> + * @plane_state: the plane state
>   *
> - * This function looks at the proposed state for CRTCs and planes,
> then chooses
> - * which pipe is going to have FBC by setting intel_crtc_state-
> >enable_fbc to
> - * true.
> + * This function should set fbc_score based on how suitable the
> plane is for
> + * FBC. For now the only scores used are 0 and 1.
>   *
> - * Later, intel_fbc_enable is going to look for state->enable_fbc
> and then maybe
> - * enable FBC for the chosen CRTC. If it does, it will set dev_priv-
> >fbc.crtc.
> + * We're not changing dev_priv->fbc, so there's no need for the FBC
> lock.
>   */
> -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> -			   struct drm_atomic_state *state)
> +void intel_fbc_check_plane(struct intel_plane_state *plane_state)
>  {
> -	struct intel_fbc *fbc = &dev_priv->fbc;
> -	struct drm_plane *plane;
> -	struct drm_plane_state *plane_state;
> -	bool crtc_chosen = false;
> -	int i;
> +	struct drm_plane *plane = plane_state->base.plane;
> +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> +	struct intel_crtc *crtc = to_intel_crtc(plane_state-
> >base.crtc);
>  
> -	mutex_lock(&fbc->lock);
> -
> -	/* Does this atomic commit involve the CRTC currently tied
> to FBC? */
> -	if (fbc->crtc &&
> -	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc-
> >base))
> -		goto out;
> +	plane_state->fbc_score = 0;
>  
>  	if (!intel_fbc_can_enable(dev_priv))
> -		goto out;
> -
> -	/* Simply choose the first CRTC that is compatible and has a
> visible
> -	 * plane. We could go for fancier schemes such as checking
> the plane
> -	 * size, but this would just affect the few platforms that
> don't tie FBC
> -	 * to pipe or plane A. */
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> -		struct intel_plane_state *intel_plane_state =
> -			to_intel_plane_state(plane_state);
> -		struct intel_crtc_state *intel_crtc_state;
> -		struct intel_crtc *crtc = to_intel_crtc(plane_state-
> >crtc);
> -
> -		if (!intel_plane_state->base.visible)
> -			continue;
> -
> -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe !=
> PIPE_A)
> -			continue;
> -
> -		if (fbc_on_plane_a_only(dev_priv) && crtc->plane !=
> PLANE_A)
> -			continue;
> -
> -		intel_crtc_state = to_intel_crtc_state(
> -			drm_atomic_get_existing_crtc_state(state,
> &crtc->base));
> -
> -		intel_crtc_state->enable_fbc = true;
> -		crtc_chosen = true;
> -		break;
> -	}
> -
> -	if (!crtc_chosen)
> -		fbc->no_fbc_reason = "no suitable CRTC for FBC";
> +		return;
> +	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> +		return;
> +	if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
> +		return;
> +	if (!plane_state->base.visible)
> +		return;
>  
> -out:
> -	mutex_unlock(&fbc->lock);
> +	plane_state->fbc_score = 1;
>  }
>  
>  /**
> @@ -1114,6 +1078,13 @@ void intel_fbc_choose_crtc(struct
> drm_i915_private *dev_priv,
>   * possible. Notice that it doesn't activate FBC. It is valid to
> call
>   * intel_fbc_enable multiple times for the same pipe without an
>   * intel_fbc_disable in the middle, as long as it is deactivated.
> + *
> + * In the future this function could make better use of the
> fbc_score variable.
> + * We could, for example, loop through all the primary planes
> involved in the
> + * atomic commit and only enable FBC for the plane with the best
> fbc_score. We
> + * could also try to do some scheme where a plane with better score
> takes over
> + * FBC from another plane, but our driver currently can't handle the
> complexity
> + * of switching planes on the fly. This would only affect from Gen 4
> up to IVB.
>   */
>  void intel_fbc_enable(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *crtc_state,
> @@ -1130,14 +1101,16 @@ void intel_fbc_enable(struct intel_crtc
> *crtc,
>  	if (fbc->enabled) {
>  		WARN_ON(fbc->crtc == NULL);
>  		if (fbc->crtc == crtc) {
> -			WARN_ON(!crtc_state->enable_fbc);
> +			WARN_ON(plane_state->fbc_score == 0);
>  			WARN_ON(fbc->active);
>  		}
>  		goto out;
>  	}
>  
> -	if (!crtc_state->enable_fbc)
> +	if (plane_state->fbc_score == 0) {
> +		fbc->no_fbc_reason = "no suitable CRTC for FBC";
>  		goto out;
> +	}
>  
>  	WARN_ON(fbc->active);
>  	WARN_ON(fbc->crtc != NULL);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: enable FBC on gen9+ too (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: enable FBC on gen9+ too (rev2)
URL   : https://patchwork.freedesktop.org/series/17176/
State : failure

== Summary ==

Series 17176v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/17176/revisions/2/mbox/

Test drv_module_reload:
        Subgroup basic-reload:
                dmesg-warn -> DMESG-FAIL (fi-skl-6700k)
                pass       -> DMESG-FAIL (fi-bxt-j4205)
                pass       -> DMESG-FAIL (fi-kbl-7500u)
                pass       -> DMESG-FAIL (fi-skl-6770hq)
                pass       -> DMESG-FAIL (fi-skl-6260u)
                pass       -> DMESG-FAIL (fi-skl-6700hq)
                pass       -> DMESG-FAIL (fi-bxt-t5700)
                pass       -> DMESG-FAIL (fi-bdw-5557u)
        Subgroup basic-reload-final:
                dmesg-warn -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-bxt-j4205)
                pass       -> FAIL       (fi-kbl-7500u)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-skl-6700hq)
                pass       -> FAIL       (fi-bxt-t5700)
                pass       -> FAIL       (fi-bdw-5557u)
        Subgroup basic-reload-inject:
                dmesg-warn -> PASS       (fi-skl-6700k)
Test gem_basic:
        Subgroup bad-close:
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-bdw-5557u)
        Subgroup create-close:
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-bdw-5557u)
        Subgroup create-fd-close:
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-bdw-5557u)
Test gem_busy:
        Subgroup basic-busy-default:
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-bdw-5557u)
        Subgroup basic-hang-default:
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-bdw-5557u)
Test gem_close_race:
        Subgroup basic-process:
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-bdw-5557u)
        Subgroup basic-threads:
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-bdw-5557u)
Test gem_cpu_reloc:
        Subgroup basic:
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-bdw-5557u)
WARNING: Long output truncated

2435e74248d49d8189c20f8d191e897460da5eb0 drm-tip: 2016y-12m-23d-14h-44m-32s UTC integration manifest
132c519 drm/i915: rewrite FBC's atomic CRTC-choosing code
3694668 drm/i915: enable FBC on gen9+ too

== Logs ==

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

[PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

[Paulo Zanoni]

Ville pointed out that intel_fbc_choose_crtc() is iterating over all
planes instead of just the primary planes. There are no real
consequences of this problem for HSW+, and for the other platforms it
just means that in some obscure multi-screen cases we'll keep FBC
disabled when we could have enabled it. Still, iterating over all
planes doesn't seem to be the best thing to do.

My initial idea was to just add a check for plane->type and be done,
but then I realized that in commits not involving the primary plane we
would reset crtc_state->enable_fbc back to false even when FBC is
enabled. That also wouldn't result in a bug due to the way the
enable_fbc variable is checked, but, still, our code can be better
than this.

So I went for the solution that involves tracking enable_fbc in the
primary plane state instead of the CRTC state. This way, if a commit
doesn't involve the primary plane for the CRTC we won't be resetting
enable_fbc back to false, so the variable will always reflect the
reality. And this change also makes more sense since FBC is actually
tied to the single plane and not the full pipe. As a bonus, we only
iterate over the CRTCs instead of iterating over all planes.

v2:

But Ville pointed that this only works properly if we have a single
commit with multiple CRTCs. If we have multiple parallel commits, each
with its own CRTC, we'll end with enable_fbc set to true in more than
one plane.

So the solution was to rename enable_fbc to can_enable_fbc. If we just
did the rename as the patch was, we'd end up with a single plane with
can_enable_fbc on commits involving multiple CRTCs: we'd choose the
best one, but we'd still end up with a variable that doesn't 100%
reflect reality.

So in the end I adopted Ville's suggestion of the fbc_score variable.
And then, instead of checking the score at intel_fbc_choose_crtc()
it should be possible to check for the score at intel_fbc_enable().
This allows us to simplify intel_fbc_choose_crtc() to the point where
it only needs to look at the given plane in order to assign its score
instead of looking at every primary plane on the same commit.

We still only set scores of 0 and 1 and we don't really do the
score-checking loop.

v3: Rebase.

v4: Move intel_fbc_check_plane() call up so it will still zero the
    score if the plane has no FB. (Ville).

v5: Fix the segfault introduced by the previous version and add a
    WARN to validate our current assumptions.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h     |  8 ++--
 drivers/gpu/drm/i915/intel_fbc.c     | 87 +++++++++++++-----------------------
 3 files changed, 37 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d8effd4..56ae32c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14191,7 +14191,6 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	intel_fbc_choose_crtc(dev_priv, state);
 	return calc_watermark_data(state);
 }
 
@@ -14957,6 +14956,8 @@ intel_check_primary_plane(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
+	intel_fbc_check_plane(state);
+
 	if (!state->base.fb)
 		return 0;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 025e4c8..7430082 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -406,6 +406,9 @@ struct intel_plane_state {
 	 */
 	int scaler_id;
 
+	/* 0: not suitable for FBC, 1+: suitable for FBC, more is better. */
+	unsigned int fbc_score;
+
 	struct drm_intel_sprite_colorkey ckey;
 };
 
@@ -652,8 +655,6 @@ struct intel_crtc_state {
 
 	bool ips_enabled;
 
-	bool enable_fbc;
-
 	bool double_wide;
 
 	int pbn;
@@ -1535,8 +1536,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
 #endif
 
 /* intel_fbc.c */
-void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
-			   struct drm_atomic_state *state);
+void intel_fbc_check_plane(struct intel_plane_state *plane_state);
 bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
 void intel_fbc_pre_update(struct intel_crtc *crtc,
 			  struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 26a81a9..19eaa60 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1040,68 +1040,34 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_fbc_choose_crtc - select a CRTC to enable FBC on
- * @dev_priv: i915 device instance
- * @state: the atomic state structure
+ * intel_fbc_check_plane - check plane for FBC suitability
+ * @plane_state: the plane state
  *
- * This function looks at the proposed state for CRTCs and planes, then chooses
- * which pipe is going to have FBC by setting intel_crtc_state->enable_fbc to
- * true.
+ * This function should set fbc_score based on how suitable the plane is for
+ * FBC. For now the only scores used are 0 and 1.
  *
- * Later, intel_fbc_enable is going to look for state->enable_fbc and then maybe
- * enable FBC for the chosen CRTC. If it does, it will set dev_priv->fbc.crtc.
+ * We're not changing dev_priv->fbc, so there's no need for the FBC lock.
  */
-void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
-			   struct drm_atomic_state *state)
+void intel_fbc_check_plane(struct intel_plane_state *plane_state)
 {
-	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
-	bool crtc_chosen = false;
-	int i;
+	struct drm_plane *plane = plane_state->base.plane;
+	struct drm_i915_private *dev_priv = to_i915(plane->dev);
+	struct intel_crtc *crtc = to_intel_crtc(plane_state->base.crtc);
 
-	mutex_lock(&fbc->lock);
-
-	/* Does this atomic commit involve the CRTC currently tied to FBC? */
-	if (fbc->crtc &&
-	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc->base))
-		goto out;
+	plane_state->fbc_score = 0;
 
+	if (!plane_state->base.visible)
+		return;
 	if (!intel_fbc_can_enable(dev_priv))
-		goto out;
-
-	/* Simply choose the first CRTC that is compatible and has a visible
-	 * plane. We could go for fancier schemes such as checking the plane
-	 * size, but this would just affect the few platforms that don't tie FBC
-	 * to pipe or plane A. */
-	for_each_plane_in_state(state, plane, plane_state, i) {
-		struct intel_plane_state *intel_plane_state =
-			to_intel_plane_state(plane_state);
-		struct intel_crtc_state *intel_crtc_state;
-		struct intel_crtc *crtc = to_intel_crtc(plane_state->crtc);
-
-		if (!intel_plane_state->base.visible)
-			continue;
-
-		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
-			continue;
-
-		if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
-			continue;
-
-		intel_crtc_state = to_intel_crtc_state(
-			drm_atomic_get_existing_crtc_state(state, &crtc->base));
-
-		intel_crtc_state->enable_fbc = true;
-		crtc_chosen = true;
-		break;
-	}
-
-	if (!crtc_chosen)
-		fbc->no_fbc_reason = "no suitable CRTC for FBC";
+		return;
+	if (WARN_ON(!crtc))
+		return;
+	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
+		return;
+	if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
+		return;
 
-out:
-	mutex_unlock(&fbc->lock);
+	plane_state->fbc_score = 1;
 }
 
 /**
@@ -1114,6 +1080,13 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
  * possible. Notice that it doesn't activate FBC. It is valid to call
  * intel_fbc_enable multiple times for the same pipe without an
  * intel_fbc_disable in the middle, as long as it is deactivated.
+ *
+ * In the future this function could make better use of the fbc_score variable.
+ * We could, for example, loop through all the primary planes involved in the
+ * atomic commit and only enable FBC for the plane with the best fbc_score. We
+ * could also try to do some scheme where a plane with better score takes over
+ * FBC from another plane, but our driver currently can't handle the complexity
+ * of switching planes on the fly. This would only affect from Gen 4 up to IVB.
  */
 void intel_fbc_enable(struct intel_crtc *crtc,
 		      struct intel_crtc_state *crtc_state,
@@ -1130,14 +1103,16 @@ void intel_fbc_enable(struct intel_crtc *crtc,
 	if (fbc->enabled) {
 		WARN_ON(fbc->crtc == NULL);
 		if (fbc->crtc == crtc) {
-			WARN_ON(!crtc_state->enable_fbc);
+			WARN_ON(plane_state->fbc_score == 0);
 			WARN_ON(fbc->active);
 		}
 		goto out;
 	}
 
-	if (!crtc_state->enable_fbc)
+	if (plane_state->fbc_score == 0) {
+		fbc->no_fbc_reason = "no suitable CRTC for FBC";
 		goto out;
+	}
 
 	WARN_ON(fbc->active);
 	WARN_ON(fbc->crtc != NULL);
-- 
2.7.4

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: enable FBC on gen9+ too (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: enable FBC on gen9+ too (rev3)
URL   : https://patchwork.freedesktop.org/series/17176/
State : success

== Summary ==

Series 17176v3 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/17176/revisions/3/mbox/


fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:82   pass:69   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

6280036ab2d5484b44ce0f1600e56847ab1fb764 drm-tip: 2016y-12m-26d-16h-22m-13s UTC integration manifest
838b131 drm/i915: rewrite FBC's atomic CRTC-choosing code
59b7374 drm/i915: enable FBC on gen9+ too

== Logs ==

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

Re: [PATCH 1/2] drm/i915: enable FBC on gen9+ too

[Daniel Vetter]

On Fri, Dec 23, 2016 at 10:23:58AM -0200, Paulo Zanoni wrote:
> Gen9+ platforms have been seeing a lot of screen flickerings and
> underruns, so I never felt comfortable in enabling FBC on these
> platforms since I didn't want to throw yet another feature on top of
> the already complex problem. We now have code that automatically
> disables FBC if we ever get an underrun, and the screen flickerings
> seem to be mostly gone, so it may be a good time to try to finally
> enable FBC by default on the newer platforms.
> 
> Besides, BDW FBC has been working fine over the year, which gives me a
> little more confidence now.
> 
> For a little more information, please refer to commit a98ee79317b4
> ("drm/i915/fbc: enable FBC by default on HSW and BDW").
> 
> v2: Enable not only on SKL, but for everything new (Daniel).
> v3: Rebase after the intel_sanitize_fbc_option() change.
> v4: New rebase after 8 months, drop expired R-B tags.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Daniel gave me a R-B for this patch maaaaany months ago, but I think
> we can consider it as expired now.
> 
> QA just went through a round of testing and confirmed 100% pass rate
> for SKL/KBL/BXT (VIZ-7181).

For gen9+ still think we can move ahead:

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

> Besides, we do have at least one user with FBC enabled on SKL and
> reporting possible issues, as we saw on #98213, which is already
> fixed.
> 
> The problem that I've been discussing with Chris and Daniel is not a
> user visible bug since it just keeps FBC deactivated if the client
> doesn't issue the dirtyfb calls, and from what I can see, current user
> space is fine.
> 
> I have a pending patch for an improvement on the CRTC-choosing code,
> but that's not a bug fix and doesn't change how HSW+ behaves. It's
> patch 2 of this series.
> 
> I think we can also consider reenabling FBC on HSW due to the
> confirmation I got some time ago that the HSW regression is gone (both
> bugs from c7f7e2feffb0 have confirmed that some patches that are
> alredy merged fixed the problem without reverting HSW support). But
> I won't propose this until we get to understand what's going on with
> bug #99169.

Hm, this is indeed wtf, but I guess first QA needs to deliver that bisect.
-Daniel

> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 9aec63b..26a81a9 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1317,7 +1317,7 @@ static int intel_sanitize_fbc_option(struct drm_i915_private *dev_priv)
>  	if (!HAS_FBC(dev_priv))
>  		return 0;
>  
> -	if (IS_BROADWELL(dev_priv))
> +	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9)
>  		return 1;
>  
>  	return 0;
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

[Maarten Lankhorst]

Hey,

Op 26-12-16 om 19:45 schreef Paulo Zanoni:
> Ville pointed out that intel_fbc_choose_crtc() is iterating over all
> planes instead of just the primary planes. There are no real
> consequences of this problem for HSW+, and for the other platforms it
> just means that in some obscure multi-screen cases we'll keep FBC
> disabled when we could have enabled it. Still, iterating over all
> planes doesn't seem to be the best thing to do.
>
> My initial idea was to just add a check for plane->type and be done,
> but then I realized that in commits not involving the primary plane we
> would reset crtc_state->enable_fbc back to false even when FBC is
> enabled. That also wouldn't result in a bug due to the way the
> enable_fbc variable is checked, but, still, our code can be better
> than this.
>
> So I went for the solution that involves tracking enable_fbc in the
> primary plane state instead of the CRTC state. This way, if a commit
> doesn't involve the primary plane for the CRTC we won't be resetting
> enable_fbc back to false, so the variable will always reflect the
> reality. And this change also makes more sense since FBC is actually
> tied to the single plane and not the full pipe. As a bonus, we only
> iterate over the CRTCs instead of iterating over all planes.
>
> v2:
>
> But Ville pointed that this only works properly if we have a single
> commit with multiple CRTCs. If we have multiple parallel commits, each
> with its own CRTC, we'll end with enable_fbc set to true in more than
> one plane.
>
> So the solution was to rename enable_fbc to can_enable_fbc. If we just
> did the rename as the patch was, we'd end up with a single plane with
> can_enable_fbc on commits involving multiple CRTCs: we'd choose the
> best one, but we'd still end up with a variable that doesn't 100%
> reflect reality.
>
> So in the end I adopted Ville's suggestion of the fbc_score variable.
> And then, instead of checking the score at intel_fbc_choose_crtc()
> it should be possible to check for the score at intel_fbc_enable().
> This allows us to simplify intel_fbc_choose_crtc() to the point where
> it only needs to look at the given plane in order to assign its score
> instead of looking at every primary plane on the same commit.
>
> We still only set scores of 0 and 1 and we don't really do the
> score-checking loop.
>
> v3: Rebase.
>
> v4: Move intel_fbc_check_plane() call up so it will still zero the
>     score if the plane has no FB. (Ville).
>
> v5: Fix the segfault introduced by the previous version and add a
>     WARN to validate our current assumptions.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
I'm hitting the WARN_ON(plane_state->fbc_score == 0) on my gen9 when running kms_atomic_transition.plane-* tests.
This needs some more fixing to work with cases where the primary plane is disabled. :-)

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