[PATCH 0/7] FBC atomic cleanups

[PATCH 0/7] FBC atomic cleanups

[Paulo Zanoni]

Ville pointed out two ugly defects in the FBC code, and while trying
to fix them I spotted a few extra things. No real-world bugs fixed
here, but IMHO the code is much easier to read now.

Paulo Zanoni (7):
  drm/i915/fbc: move the intel_fbc_can_choose() call out of the loop
  drm/i915/fbc: replace a loop with drm_atomic_get_existing_crtc_state()
  drm/i915/fbc: extract intel_fbc_can_enable()
  drm/i915/fbc: inline intel_fbc_can_choose()
  drm/i915/fbc: use drm_atomic_get_existing_crtc_state when appropriate
  drm/i915/fbc: move from crtc_state->enable_fbc to
    plane_state->enable_fbc
  drm/i915/fbc: convert intel_fbc.c to use INTEL_GEN()

 drivers/gpu/drm/i915/intel_drv.h |  4 +-
 drivers/gpu/drm/i915/intel_fbc.c | 97 ++++++++++++++++++----------------------
 2 files changed, 46 insertions(+), 55 deletions(-)

-- 
2.7.4

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

[PATCH 1/7] drm/i915/fbc: move the intel_fbc_can_choose() call out of the loop

[Paulo Zanoni]

We can just call it earlier, so do it. The goal of the loop is to get
the plane's CRTC state, and we don't need it in order to call
intel_fbc_can_choose().

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index e230d48..ded77bd 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1096,6 +1096,9 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 		if (!intel_plane_state->base.visible)
 			continue;
 
+		if (!intel_fbc_can_choose(to_intel_crtc(plane_state->crtc)))
+			continue;
+
 		for_each_crtc_in_state(state, crtc, crtc_state, j) {
 			struct intel_crtc_state *intel_crtc_state =
 				to_intel_crtc_state(crtc_state);
@@ -1103,9 +1106,6 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 			if (plane_state->crtc != crtc)
 				continue;
 
-			if (!intel_fbc_can_choose(to_intel_crtc(crtc)))
-				break;
-
 			intel_crtc_state->enable_fbc = true;
 			goto out;
 		}
-- 
2.7.4

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

Re: [PATCH 1/7] drm/i915/fbc: move the intel_fbc_can_choose() call out of the loop

[Chris Wilson]

On Fri, Nov 11, 2016 at 02:57:35PM -0200, Paulo Zanoni wrote:
> We can just call it earlier, so do it. The goal of the loop is to get
> the plane's CRTC state, and we don't need it in order to call
> intel_fbc_can_choose().
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/7] drm/i915/fbc: replace a loop with drm_atomic_get_existing_crtc_state()

[Paulo Zanoni]

Much simpler. Thanks to Ville for pointing this.

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_fbc.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index ded77bd..b53b884 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1071,7 +1071,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
 	bool fbc_crtc_present = false;
-	int i, j;
+	int i;
 
 	mutex_lock(&fbc->lock);
 
@@ -1092,6 +1092,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 	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;
 
 		if (!intel_plane_state->base.visible)
 			continue;
@@ -1099,16 +1100,12 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 		if (!intel_fbc_can_choose(to_intel_crtc(plane_state->crtc)))
 			continue;
 
-		for_each_crtc_in_state(state, crtc, crtc_state, j) {
-			struct intel_crtc_state *intel_crtc_state =
-				to_intel_crtc_state(crtc_state);
-
-			if (plane_state->crtc != crtc)
-				continue;
+		intel_crtc_state = to_intel_crtc_state(
+			drm_atomic_get_existing_crtc_state(state,
+							   plane_state->crtc));
 
-			intel_crtc_state->enable_fbc = true;
-			goto out;
-		}
+		intel_crtc_state->enable_fbc = true;
+		break;
 	}
 
 out:
-- 
2.7.4

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

Re: [PATCH 2/7] drm/i915/fbc: replace a loop with drm_atomic_get_existing_crtc_state()

[Chris Wilson]

On Fri, Nov 11, 2016 at 02:57:36PM -0200, Paulo Zanoni wrote:
> Much simpler. Thanks to Ville for pointing this.
> 
> 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_fbc.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index ded77bd..b53b884 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1071,7 +1071,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
>  	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
>  	bool fbc_crtc_present = false;
> -	int i, j;
> +	int i;
>  
>  	mutex_lock(&fbc->lock);
>  
> @@ -1092,6 +1092,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
>  	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;
>  
>  		if (!intel_plane_state->base.visible)
>  			continue;
> @@ -1099,16 +1100,12 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
>  		if (!intel_fbc_can_choose(to_intel_crtc(plane_state->crtc)))
>  			continue;
>  
> -		for_each_crtc_in_state(state, crtc, crtc_state, j) {
> -			struct intel_crtc_state *intel_crtc_state =
> -				to_intel_crtc_state(crtc_state);
> -
> -			if (plane_state->crtc != crtc)
> -				continue;
> +		intel_crtc_state = to_intel_crtc_state(
> +			drm_atomic_get_existing_crtc_state(state,
> +							   plane_state->crtc));

My knowledge of atomic api is not good enough to say whether this can
return NULL at this point.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915/fbc: replace a loop with drm_atomic_get_existing_crtc_state()

[Ville Syrjälä]

On Fri, Nov 11, 2016 at 05:46:16PM +0000, Chris Wilson wrote:
> On Fri, Nov 11, 2016 at 02:57:36PM -0200, Paulo Zanoni wrote:
> > Much simpler. Thanks to Ville for pointing this.
> > 
> > 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_fbc.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index ded77bd..b53b884 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -1071,7 +1071,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> >  	struct drm_plane *plane;
> >  	struct drm_plane_state *plane_state;
> >  	bool fbc_crtc_present = false;
> > -	int i, j;
> > +	int i;
> >  
> >  	mutex_lock(&fbc->lock);
> >  
> > @@ -1092,6 +1092,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> >  	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;
> >  
> >  		if (!intel_plane_state->base.visible)
> >  			continue;
> > @@ -1099,16 +1100,12 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> >  		if (!intel_fbc_can_choose(to_intel_crtc(plane_state->crtc)))
> >  			continue;
> >  
> > -		for_each_crtc_in_state(state, crtc, crtc_state, j) {
> > -			struct intel_crtc_state *intel_crtc_state =
> > -				to_intel_crtc_state(crtc_state);
> > -
> > -			if (plane_state->crtc != crtc)
> > -				continue;
> > +		intel_crtc_state = to_intel_crtc_state(
> > +			drm_atomic_get_existing_crtc_state(state,
> > +							   plane_state->crtc));
> 
> My knowledge of atomic api is not good enough to say whether this can
> return NULL at this point.

If the plane is part of the state, then plane_state->crtc will be part
of the state as well.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Just to reaffirm I wanted to point out what
drm_atomic_set_crtc_for_plane() does, but looks like that one was
written with the assumption that it could fail, which is not correct
since drm_atomic_get_plane_state() will always add the current crtc
to the state, and drm_atomic_set_crtc_for_plane() will always add the
new crtc to the state.

drm_atomic_set_crtc_for_connector() is written in the way I was
expecting, so I think I'll fire off a patch for
drm_atomic_set_crtc_for_plane() to follow the same pattern.

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

[PATCH 3/7] drm/i915/fbc: extract intel_fbc_can_enable()

[Paulo Zanoni]

Extract that part of the code to a new function and call this function
only once during intel_fbc_choose_crtc() instead of once per plane.
Those checks are independent from planes/CRTCs.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index b53b884..7381011 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -854,9 +854,8 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	return true;
 }
 
-static bool intel_fbc_can_choose(struct intel_crtc *crtc)
+static bool intel_fbc_can_enable(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	struct intel_fbc *fbc = &dev_priv->fbc;
 
 	if (intel_vgpu_active(dev_priv)) {
@@ -874,6 +873,14 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
 		return false;
 	}
 
+	return true;
+}
+
+static bool intel_fbc_can_choose(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	struct intel_fbc *fbc = &dev_priv->fbc;
+
 	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A) {
 		fbc->no_fbc_reason = "no enabled pipes can have FBC";
 		return false;
@@ -1085,6 +1092,9 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 	if (!fbc_crtc_present && fbc->crtc != NULL)
 		goto out;
 
+	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
-- 
2.7.4

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

Re: [PATCH 3/7] drm/i915/fbc: extract intel_fbc_can_enable()

[Chris Wilson]

On Fri, Nov 11, 2016 at 02:57:37PM -0200, Paulo Zanoni wrote:
> Extract that part of the code to a new function and call this function
> only once during intel_fbc_choose_crtc() instead of once per plane.
> Those checks are independent from planes/CRTCs.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm/i915/fbc: inline intel_fbc_can_choose()

[Paulo Zanoni]

It only has two checks now, so it makes sense to just move the code to
the caller.

Also take this opportunity to make no_fbc_reason make more sense: now
we'll only list "no suitable CRTC for FBC" instead of maybe giving a
reason why the last CRTC we checked was not selected, and we'll more
consistently set the reason (e.g., if no primary planes are visible).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 7381011..89d5612 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -876,24 +876,6 @@ static bool intel_fbc_can_enable(struct drm_i915_private *dev_priv)
 	return true;
 }
 
-static bool intel_fbc_can_choose(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	struct intel_fbc *fbc = &dev_priv->fbc;
-
-	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A) {
-		fbc->no_fbc_reason = "no enabled pipes can have FBC";
-		return false;
-	}
-
-	if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A) {
-		fbc->no_fbc_reason = "no enabled planes can have FBC";
-		return false;
-	}
-
-	return true;
-}
-
 static void intel_fbc_get_reg_params(struct intel_crtc *crtc,
 				     struct intel_fbc_reg_params *params)
 {
@@ -1077,7 +1059,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 	struct drm_crtc_state *crtc_state;
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
-	bool fbc_crtc_present = false;
+	bool fbc_crtc_present = false, crtc_chosen = false;
 	int i;
 
 	mutex_lock(&fbc->lock);
@@ -1103,21 +1085,28 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 		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 (!intel_fbc_can_choose(to_intel_crtc(plane_state->crtc)))
+		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,
-							   plane_state->crtc));
+			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";
+
 out:
 	mutex_unlock(&fbc->lock);
 }
-- 
2.7.4

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

Re: [PATCH 4/7] drm/i915/fbc: inline intel_fbc_can_choose()

[Chris Wilson]

On Fri, Nov 11, 2016 at 02:57:38PM -0200, Paulo Zanoni wrote:
> It only has two checks now, so it makes sense to just move the code to
> the caller.
> 
> Also take this opportunity to make no_fbc_reason make more sense: now
> we'll only list "no suitable CRTC for FBC" instead of maybe giving a
> reason why the last CRTC we checked was not selected, and we'll more
> consistently set the reason (e.g., if no primary planes are visible).

That seems a reasonable justification for the change.
 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/7] drm/i915/fbc: use drm_atomic_get_existing_crtc_state when appropriate

[Paulo Zanoni]

Use drm_atomic_get_existing_crtc_state() instead of looping through
the CRTC states and checking if the FBC CRTC is there.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 89d5612..b095175 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1055,23 +1055,16 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 			   struct drm_atomic_state *state)
 {
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
-	bool fbc_crtc_present = false, crtc_chosen = false;
+	bool crtc_chosen = false;
 	int i;
 
 	mutex_lock(&fbc->lock);
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (fbc->crtc == to_intel_crtc(crtc)) {
-			fbc_crtc_present = true;
-			break;
-		}
-	}
-	/* This atomic commit doesn't involve the CRTC currently tied to FBC. */
-	if (!fbc_crtc_present && fbc->crtc != NULL)
+	/* 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;
 
 	if (!intel_fbc_can_enable(dev_priv))
-- 
2.7.4

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

Re: [PATCH 5/7] drm/i915/fbc: use drm_atomic_get_existing_crtc_state when appropriate

[Chris Wilson]

On Fri, Nov 11, 2016 at 02:57:39PM -0200, Paulo Zanoni wrote:
> Use drm_atomic_get_existing_crtc_state() instead of looping through
> the CRTC states and checking if the FBC CRTC is there.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

[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.

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_drv.h |  4 ++--
 drivers/gpu/drm/i915/intel_fbc.c | 36 +++++++++++++++++++-----------------
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 003afb8..025cb74 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -403,6 +403,8 @@ struct intel_plane_state {
 	int scaler_id;
 
 	struct drm_intel_sprite_colorkey ckey;
+
+	bool enable_fbc;
 };
 
 struct intel_initial_plane_config {
@@ -648,8 +650,6 @@ struct intel_crtc_state {
 
 	bool ips_enabled;
 
-	bool enable_fbc;
-
 	bool double_wide;
 
 	bool dp_encoder_is_mst;
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index b095175..fc4ac57 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1055,16 +1055,17 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 			   struct drm_atomic_state *state)
 {
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
 	bool crtc_chosen = false;
 	int i;
 
 	mutex_lock(&fbc->lock);
 
-	/* Does this atomic commit involve the CRTC currently tied to FBC? */
+	/* Does this atomic commit involve the plane currently tied to FBC? */
 	if (fbc->crtc &&
-	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc->base))
+	    !drm_atomic_get_existing_plane_state(state,
+						 fbc->crtc->base.primary))
 		goto out;
 
 	if (!intel_fbc_can_enable(dev_priv))
@@ -1074,25 +1075,26 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 	 * 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);
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_plane_state *plane_state = to_intel_plane_state(
+			drm_atomic_get_existing_plane_state(state,
+							    crtc->primary));
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-		if (!intel_plane_state->base.visible)
+		if (!plane_state)
 			continue;
 
-		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
+		if (!plane_state->base.visible)
 			continue;
 
-		if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
+		if (fbc_on_pipe_a_only(dev_priv) && intel_crtc->pipe != PIPE_A)
 			continue;
 
-		intel_crtc_state = to_intel_crtc_state(
-			drm_atomic_get_existing_crtc_state(state, &crtc->base));
+		if (fbc_on_plane_a_only(dev_priv) &&
+		    intel_crtc->plane != PLANE_A)
+			continue;
 
-		intel_crtc_state->enable_fbc = true;
+		plane_state->enable_fbc = true;
 		crtc_chosen = true;
 		break;
 	}
@@ -1130,13 +1132,13 @@ 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->enable_fbc);
 			WARN_ON(fbc->active);
 		}
 		goto out;
 	}
 
-	if (!crtc_state->enable_fbc)
+	if (!plane_state->enable_fbc)
 		goto out;
 
 	WARN_ON(fbc->active);
-- 
2.7.4

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

Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

[Ville Syrjälä]

On Fri, Nov 11, 2016 at 02:57:40PM -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.
> 
> 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_drv.h |  4 ++--
>  drivers/gpu/drm/i915/intel_fbc.c | 36 +++++++++++++++++++-----------------
>  2 files changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 003afb8..025cb74 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -403,6 +403,8 @@ struct intel_plane_state {
>  	int scaler_id;
>  
>  	struct drm_intel_sprite_colorkey ckey;
> +
> +	bool enable_fbc;
>  };
>  
>  struct intel_initial_plane_config {
> @@ -648,8 +650,6 @@ struct intel_crtc_state {
>  
>  	bool ips_enabled;
>  
> -	bool enable_fbc;
> -
>  	bool double_wide;
>  
>  	bool dp_encoder_is_mst;
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index b095175..fc4ac57 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1055,16 +1055,17 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
>  			   struct drm_atomic_state *state)
>  {
>  	struct intel_fbc *fbc = &dev_priv->fbc;
> -	struct drm_plane *plane;
> -	struct drm_plane_state *plane_state;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
>  	bool crtc_chosen = false;
>  	int i;
>  
>  	mutex_lock(&fbc->lock);
>  
> -	/* Does this atomic commit involve the CRTC currently tied to FBC? */
> +	/* Does this atomic commit involve the plane currently tied to FBC? */
>  	if (fbc->crtc &&
> -	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc->base))
> +	    !drm_atomic_get_existing_plane_state(state,
> +						 fbc->crtc->base.primary))
>  		goto out;
>  
>  	if (!intel_fbc_can_enable(dev_priv))
> @@ -1074,25 +1075,26 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
>  	 * 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);
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_plane_state *plane_state = to_intel_plane_state(
> +			drm_atomic_get_existing_plane_state(state,
> +							    crtc->primary));
> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
> -		if (!intel_plane_state->base.visible)
> +		if (!plane_state)
>  			continue;
>  
> -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> +		if (!plane_state->base.visible)
>  			continue;
>  
> -		if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
> +		if (fbc_on_pipe_a_only(dev_priv) && intel_crtc->pipe != PIPE_A)
>  			continue;
>  
> -		intel_crtc_state = to_intel_crtc_state(
> -			drm_atomic_get_existing_crtc_state(state, &crtc->base));
> +		if (fbc_on_plane_a_only(dev_priv) &&
> +		    intel_crtc->plane != PLANE_A)
> +			continue;
>  
> -		intel_crtc_state->enable_fbc = true;
> +		plane_state->enable_fbc = true;

So looking at this whole thing, I can't see anything that would prevent
enable_fbc being true for multiple primary planes at the same time
Well, apart from the whole "we enable it only for platforms that can only do
pipe A" thing.

So what happens in that case? FBC just ends up getting enabling on
one of the pipes based on the order intel_fbc_enable() gets called,
or something?

>  		crtc_chosen = true;
>  		break;
>  	}
> @@ -1130,13 +1132,13 @@ 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->enable_fbc);
>  			WARN_ON(fbc->active);
>  		}
>  		goto out;
>  	}
>  
> -	if (!crtc_state->enable_fbc)
> +	if (!plane_state->enable_fbc)
>  		goto out;
>  
>  	WARN_ON(fbc->active);
> -- 
> 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

Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

[Paulo Zanoni]

Em Sex, 2016-11-11 às 20:51 +0200, Ville Syrjälä escreveu:
> On Fri, Nov 11, 2016 at 02:57:40PM -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.
> > 
> > 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_drv.h |  4 ++--
> >  drivers/gpu/drm/i915/intel_fbc.c | 36 +++++++++++++++++++---------
> > --------
> >  2 files changed, 21 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 003afb8..025cb74 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -403,6 +403,8 @@ struct intel_plane_state {
> >  	int scaler_id;
> >  
> >  	struct drm_intel_sprite_colorkey ckey;
> > +
> > +	bool enable_fbc;
> >  };
> >  
> >  struct intel_initial_plane_config {
> > @@ -648,8 +650,6 @@ struct intel_crtc_state {
> >  
> >  	bool ips_enabled;
> >  
> > -	bool enable_fbc;
> > -
> >  	bool double_wide;
> >  
> >  	bool dp_encoder_is_mst;
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index b095175..fc4ac57 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -1055,16 +1055,17 @@ void intel_fbc_choose_crtc(struct
> > drm_i915_private *dev_priv,
> >  			   struct drm_atomic_state *state)
> >  {
> >  	struct intel_fbc *fbc = &dev_priv->fbc;
> > -	struct drm_plane *plane;
> > -	struct drm_plane_state *plane_state;
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *crtc_state;
> >  	bool crtc_chosen = false;
> >  	int i;
> >  
> >  	mutex_lock(&fbc->lock);
> >  
> > -	/* Does this atomic commit involve the CRTC currently tied
> > to FBC? */
> > +	/* Does this atomic commit involve the plane currently
> > tied to FBC? */
> >  	if (fbc->crtc &&
> > -	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc-
> > >base))
> > +	    !drm_atomic_get_existing_plane_state(state,
> > +						 fbc->crtc-
> > >base.primary))
> >  		goto out;
> >  
> >  	if (!intel_fbc_can_enable(dev_priv))
> > @@ -1074,25 +1075,26 @@ void intel_fbc_choose_crtc(struct
> > drm_i915_private *dev_priv,
> >  	 * 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);
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		struct intel_plane_state *plane_state =
> > to_intel_plane_state(
> > +			drm_atomic_get_existing_plane_state(state,
> > +							    crtc-
> > >primary));
> > +		struct intel_crtc *intel_crtc =
> > to_intel_crtc(crtc);
> >  
> > -		if (!intel_plane_state->base.visible)
> > +		if (!plane_state)
> >  			continue;
> >  
> > -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe !=
> > PIPE_A)
> > +		if (!plane_state->base.visible)
> >  			continue;
> >  
> > -		if (fbc_on_plane_a_only(dev_priv) && crtc->plane
> > != PLANE_A)
> > +		if (fbc_on_pipe_a_only(dev_priv) && intel_crtc-
> > >pipe != PIPE_A)
> >  			continue;
> >  
> > -		intel_crtc_state = to_intel_crtc_state(
> > -			drm_atomic_get_existing_crtc_state(state,
> > &crtc->base));
> > +		if (fbc_on_plane_a_only(dev_priv) &&
> > +		    intel_crtc->plane != PLANE_A)
> > +			continue;
> >  
> > -		intel_crtc_state->enable_fbc = true;
> > +		plane_state->enable_fbc = true;
> 
> So looking at this whole thing, I can't see anything that would
> prevent
> enable_fbc being true for multiple primary planes at the same time
> Well, apart from the whole "we enable it only for platforms that can
> only do
> pipe A" thing.
> 
> So what happens in that case? FBC just ends up getting enabling on
> one of the pipes based on the order intel_fbc_enable() gets called,
> or something?

The first check of intel_fbc_choose_crtc() is supposed to prevent this
case: if fbc->crtc->primary is not included in the commit we just
return without selecting any plane. Otherwise, we only pick one CRTC
due to the "break;" statement after setting plane_state->enable_fbc to
true.

> 
> > 
> >  		crtc_chosen = true;
> >  		break;
> >  	}
> > @@ -1130,13 +1132,13 @@ 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->enable_fbc);
> >  			WARN_ON(fbc->active);
> >  		}
> >  		goto out;
> >  	}
> >  
> > -	if (!crtc_state->enable_fbc)
> > +	if (!plane_state->enable_fbc)
> >  		goto out;
> >  
> >  	WARN_ON(fbc->active);
> > -- 
> > 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

[Ville Syrjälä]

On Fri, Nov 11, 2016 at 05:01:54PM -0200, Paulo Zanoni wrote:
> Em Sex, 2016-11-11 às 20:51 +0200, Ville Syrjälä escreveu:
> > On Fri, Nov 11, 2016 at 02:57:40PM -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.
> > > 
> > > 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_drv.h |  4 ++--
> > >  drivers/gpu/drm/i915/intel_fbc.c | 36 +++++++++++++++++++---------
> > > --------
> > >  2 files changed, 21 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 003afb8..025cb74 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -403,6 +403,8 @@ struct intel_plane_state {
> > >  	int scaler_id;
> > >  
> > >  	struct drm_intel_sprite_colorkey ckey;
> > > +
> > > +	bool enable_fbc;
> > >  };
> > >  
> > >  struct intel_initial_plane_config {
> > > @@ -648,8 +650,6 @@ struct intel_crtc_state {
> > >  
> > >  	bool ips_enabled;
> > >  
> > > -	bool enable_fbc;
> > > -
> > >  	bool double_wide;
> > >  
> > >  	bool dp_encoder_is_mst;
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index b095175..fc4ac57 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -1055,16 +1055,17 @@ void intel_fbc_choose_crtc(struct
> > > drm_i915_private *dev_priv,
> > >  			   struct drm_atomic_state *state)
> > >  {
> > >  	struct intel_fbc *fbc = &dev_priv->fbc;
> > > -	struct drm_plane *plane;
> > > -	struct drm_plane_state *plane_state;
> > > +	struct drm_crtc *crtc;
> > > +	struct drm_crtc_state *crtc_state;
> > >  	bool crtc_chosen = false;
> > >  	int i;
> > >  
> > >  	mutex_lock(&fbc->lock);
> > >  
> > > -	/* Does this atomic commit involve the CRTC currently tied
> > > to FBC? */
> > > +	/* Does this atomic commit involve the plane currently
> > > tied to FBC? */
> > >  	if (fbc->crtc &&
> > > -	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc-
> > > >base))
> > > +	    !drm_atomic_get_existing_plane_state(state,
> > > +						 fbc->crtc-
> > > >base.primary))
> > >  		goto out;
> > >  
> > >  	if (!intel_fbc_can_enable(dev_priv))
> > > @@ -1074,25 +1075,26 @@ void intel_fbc_choose_crtc(struct
> > > drm_i915_private *dev_priv,
> > >  	 * 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);
> > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +		struct intel_plane_state *plane_state =
> > > to_intel_plane_state(
> > > +			drm_atomic_get_existing_plane_state(state,
> > > +							    crtc-
> > > >primary));
> > > +		struct intel_crtc *intel_crtc =
> > > to_intel_crtc(crtc);
> > >  
> > > -		if (!intel_plane_state->base.visible)
> > > +		if (!plane_state)
> > >  			continue;
> > >  
> > > -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe !=
> > > PIPE_A)
> > > +		if (!plane_state->base.visible)
> > >  			continue;
> > >  
> > > -		if (fbc_on_plane_a_only(dev_priv) && crtc->plane
> > > != PLANE_A)
> > > +		if (fbc_on_pipe_a_only(dev_priv) && intel_crtc-
> > > >pipe != PIPE_A)
> > >  			continue;
> > >  
> > > -		intel_crtc_state = to_intel_crtc_state(
> > > -			drm_atomic_get_existing_crtc_state(state,
> > > &crtc->base));
> > > +		if (fbc_on_plane_a_only(dev_priv) &&
> > > +		    intel_crtc->plane != PLANE_A)
> > > +			continue;
> > >  
> > > -		intel_crtc_state->enable_fbc = true;
> > > +		plane_state->enable_fbc = true;
> > 
> > So looking at this whole thing, I can't see anything that would
> > prevent
> > enable_fbc being true for multiple primary planes at the same time
> > Well, apart from the whole "we enable it only for platforms that can
> > only do
> > pipe A" thing.
> > 
> > So what happens in that case? FBC just ends up getting enabling on
> > one of the pipes based on the order intel_fbc_enable() gets called,
> > or something?
> 
> The first check of intel_fbc_choose_crtc() is supposed to prevent this
> case: if fbc->crtc->primary is not included in the commit we just
> return without selecting any plane.

The fbc->crtc thing only works if intel_fbc_enable() was already called
for some crtc. But what it it wasn't?

> Otherwise, we only pick one CRTC
> due to the "break;" statement after setting plane_state->enable_fbc to
> true.

Only one per atomic operation. But what if there are several happening
in parallel on different crtcs?

> 
> > 
> > > 
> > >  		crtc_chosen = true;
> > >  		break;
> > >  	}
> > > @@ -1130,13 +1132,13 @@ 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->enable_fbc);
> > >  			WARN_ON(fbc->active);
> > >  		}
> > >  		goto out;
> > >  	}
> > >  
> > > -	if (!crtc_state->enable_fbc)
> > > +	if (!plane_state->enable_fbc)
> > >  		goto out;
> > >  
> > >  	WARN_ON(fbc->active);
> > > -- 
> > > 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

Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

[Paulo Zanoni]

Em Sex, 2016-11-11 às 21:13 +0200, Ville Syrjälä escreveu:
> On Fri, Nov 11, 2016 at 05:01:54PM -0200, Paulo Zanoni wrote:
> > 
> > Em Sex, 2016-11-11 às 20:51 +0200, Ville Syrjälä escreveu:
> > > 
> > > On Fri, Nov 11, 2016 at 02:57:40PM -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.
> > > > 
> > > > 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_drv.h |  4 ++--
> > > >  drivers/gpu/drm/i915/intel_fbc.c | 36 +++++++++++++++++++-----
> > > > ----
> > > > --------
> > > >  2 files changed, 21 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 003afb8..025cb74 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -403,6 +403,8 @@ struct intel_plane_state {
> > > >  	int scaler_id;
> > > >  
> > > >  	struct drm_intel_sprite_colorkey ckey;
> > > > +
> > > > +	bool enable_fbc;
> > > >  };
> > > >  
> > > >  struct intel_initial_plane_config {
> > > > @@ -648,8 +650,6 @@ struct intel_crtc_state {
> > > >  
> > > >  	bool ips_enabled;
> > > >  
> > > > -	bool enable_fbc;
> > > > -
> > > >  	bool double_wide;
> > > >  
> > > >  	bool dp_encoder_is_mst;
> > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > index b095175..fc4ac57 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > @@ -1055,16 +1055,17 @@ void intel_fbc_choose_crtc(struct
> > > > drm_i915_private *dev_priv,
> > > >  			   struct drm_atomic_state *state)
> > > >  {
> > > >  	struct intel_fbc *fbc = &dev_priv->fbc;
> > > > -	struct drm_plane *plane;
> > > > -	struct drm_plane_state *plane_state;
> > > > +	struct drm_crtc *crtc;
> > > > +	struct drm_crtc_state *crtc_state;
> > > >  	bool crtc_chosen = false;
> > > >  	int i;
> > > >  
> > > >  	mutex_lock(&fbc->lock);
> > > >  
> > > > -	/* Does this atomic commit involve the CRTC currently
> > > > tied
> > > > to FBC? */
> > > > +	/* Does this atomic commit involve the plane currently
> > > > tied to FBC? */
> > > >  	if (fbc->crtc &&
> > > > -	    !drm_atomic_get_existing_crtc_state(state, &fbc-
> > > > >crtc-
> > > > > 
> > > > > base))
> > > > +	    !drm_atomic_get_existing_plane_state(state,
> > > > +						 fbc->crtc-
> > > > > 
> > > > > base.primary))
> > > >  		goto out;
> > > >  
> > > >  	if (!intel_fbc_can_enable(dev_priv))
> > > > @@ -1074,25 +1075,26 @@ void intel_fbc_choose_crtc(struct
> > > > drm_i915_private *dev_priv,
> > > >  	 * 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);
> > > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > > +		struct intel_plane_state *plane_state =
> > > > to_intel_plane_state(
> > > > +			drm_atomic_get_existing_plane_state(st
> > > > ate,
> > > > +							    cr
> > > > tc-
> > > > > 
> > > > > primary));
> > > > +		struct intel_crtc *intel_crtc =
> > > > to_intel_crtc(crtc);
> > > >  
> > > > -		if (!intel_plane_state->base.visible)
> > > > +		if (!plane_state)
> > > >  			continue;
> > > >  
> > > > -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe 
> > > > !=
> > > > PIPE_A)
> > > > +		if (!plane_state->base.visible)
> > > >  			continue;
> > > >  
> > > > -		if (fbc_on_plane_a_only(dev_priv) && crtc-
> > > > >plane
> > > > != PLANE_A)
> > > > +		if (fbc_on_pipe_a_only(dev_priv) &&
> > > > intel_crtc-
> > > > > 
> > > > > pipe != PIPE_A)
> > > >  			continue;
> > > >  
> > > > -		intel_crtc_state = to_intel_crtc_state(
> > > > -			drm_atomic_get_existing_crtc_state(sta
> > > > te,
> > > > &crtc->base));
> > > > +		if (fbc_on_plane_a_only(dev_priv) &&
> > > > +		    intel_crtc->plane != PLANE_A)
> > > > +			continue;
> > > >  
> > > > -		intel_crtc_state->enable_fbc = true;
> > > > +		plane_state->enable_fbc = true;
> > > 
> > > So looking at this whole thing, I can't see anything that would
> > > prevent
> > > enable_fbc being true for multiple primary planes at the same
> > > time
> > > Well, apart from the whole "we enable it only for platforms that
> > > can
> > > only do
> > > pipe A" thing.
> > > 
> > > So what happens in that case? FBC just ends up getting enabling
> > > on
> > > one of the pipes based on the order intel_fbc_enable() gets
> > > called,
> > > or something?
> > 
> > The first check of intel_fbc_choose_crtc() is supposed to prevent
> > this
> > case: if fbc->crtc->primary is not included in the commit we just
> > return without selecting any plane.
> 
> The fbc->crtc thing only works if intel_fbc_enable() was already
> called
> for some crtc. But what it it wasn't?
> 
> > 
> > Otherwise, we only pick one CRTC
> > due to the "break;" statement after setting plane_state->enable_fbc 
> > to
> > true.
> 
> Only one per atomic operation. But what if there are several
> happening
> in parallel on different crtcs?

I see your point now. Yeah, we'll end up with
plane_state.enable_fbc=true for two different planes. Later, the first
one to call intel_fbc_enable() will win, and the others will be
ignored, so we'll indeed end up with plane states having
enable_fbc=true but FBC not enabled by them. Not a real bug, I would
still like to avoid this confusion.

The simplest solution I can think would be to just
s/plane_state.enable_fbc/plane_state.can_enable_fbc/ and just let the
first one to call intel_fbc_enable() win... And then, if we ever decide
to enable FBC on the older platforms, we can choose to maybe implement
a better method (and fix all the possible FBC problems for those
platforms). Is this solution worth a R-B from you?

If you have any better idea here, feel free to suggest it. I'm not sure
it's possible to come up with something much better due to all the
parallelism involved. Making one CRTC "take over" FBC from the other is
something I'd really love to avoid.

Thanks,
Paulo

> 
> > 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > >  		crtc_chosen = true;
> > > >  		break;
> > > >  	}
> > > > @@ -1130,13 +1132,13 @@ 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->enable_fbc);
> > > >  			WARN_ON(fbc->active);
> > > >  		}
> > > >  		goto out;
> > > >  	}
> > > >  
> > > > -	if (!crtc_state->enable_fbc)
> > > > +	if (!plane_state->enable_fbc)
> > > >  		goto out;
> > > >  
> > > >  	WARN_ON(fbc->active);
> > > > -- 
> > > > 2.7.4
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

[Ville Syrjälä]

On Fri, Nov 11, 2016 at 05:57:28PM -0200, Paulo Zanoni wrote:
> Em Sex, 2016-11-11 às 21:13 +0200, Ville Syrjälä escreveu:
> > On Fri, Nov 11, 2016 at 05:01:54PM -0200, Paulo Zanoni wrote:
> > > 
> > > Em Sex, 2016-11-11 às 20:51 +0200, Ville Syrjälä escreveu:
> > > > 
> > > > On Fri, Nov 11, 2016 at 02:57:40PM -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.
> > > > > 
> > > > > 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_drv.h |  4 ++--
> > > > >  drivers/gpu/drm/i915/intel_fbc.c | 36 +++++++++++++++++++-----
> > > > > ----
> > > > > --------
> > > > >  2 files changed, 21 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 003afb8..025cb74 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -403,6 +403,8 @@ struct intel_plane_state {
> > > > >  	int scaler_id;
> > > > >  
> > > > >  	struct drm_intel_sprite_colorkey ckey;
> > > > > +
> > > > > +	bool enable_fbc;
> > > > >  };
> > > > >  
> > > > >  struct intel_initial_plane_config {
> > > > > @@ -648,8 +650,6 @@ struct intel_crtc_state {
> > > > >  
> > > > >  	bool ips_enabled;
> > > > >  
> > > > > -	bool enable_fbc;
> > > > > -
> > > > >  	bool double_wide;
> > > > >  
> > > > >  	bool dp_encoder_is_mst;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > index b095175..fc4ac57 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > @@ -1055,16 +1055,17 @@ void intel_fbc_choose_crtc(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  			   struct drm_atomic_state *state)
> > > > >  {
> > > > >  	struct intel_fbc *fbc = &dev_priv->fbc;
> > > > > -	struct drm_plane *plane;
> > > > > -	struct drm_plane_state *plane_state;
> > > > > +	struct drm_crtc *crtc;
> > > > > +	struct drm_crtc_state *crtc_state;
> > > > >  	bool crtc_chosen = false;
> > > > >  	int i;
> > > > >  
> > > > >  	mutex_lock(&fbc->lock);
> > > > >  
> > > > > -	/* Does this atomic commit involve the CRTC currently
> > > > > tied
> > > > > to FBC? */
> > > > > +	/* Does this atomic commit involve the plane currently
> > > > > tied to FBC? */
> > > > >  	if (fbc->crtc &&
> > > > > -	    !drm_atomic_get_existing_crtc_state(state, &fbc-
> > > > > >crtc-
> > > > > > 
> > > > > > base))
> > > > > +	    !drm_atomic_get_existing_plane_state(state,
> > > > > +						 fbc->crtc-
> > > > > > 
> > > > > > base.primary))
> > > > >  		goto out;
> > > > >  
> > > > >  	if (!intel_fbc_can_enable(dev_priv))
> > > > > @@ -1074,25 +1075,26 @@ void intel_fbc_choose_crtc(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  	 * 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);
> > > > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > > > +		struct intel_plane_state *plane_state =
> > > > > to_intel_plane_state(
> > > > > +			drm_atomic_get_existing_plane_state(st
> > > > > ate,
> > > > > +							    cr
> > > > > tc-
> > > > > > 
> > > > > > primary));
> > > > > +		struct intel_crtc *intel_crtc =
> > > > > to_intel_crtc(crtc);
> > > > >  
> > > > > -		if (!intel_plane_state->base.visible)
> > > > > +		if (!plane_state)
> > > > >  			continue;
> > > > >  
> > > > > -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe 
> > > > > !=
> > > > > PIPE_A)
> > > > > +		if (!plane_state->base.visible)
> > > > >  			continue;
> > > > >  
> > > > > -		if (fbc_on_plane_a_only(dev_priv) && crtc-
> > > > > >plane
> > > > > != PLANE_A)
> > > > > +		if (fbc_on_pipe_a_only(dev_priv) &&
> > > > > intel_crtc-
> > > > > > 
> > > > > > pipe != PIPE_A)
> > > > >  			continue;
> > > > >  
> > > > > -		intel_crtc_state = to_intel_crtc_state(
> > > > > -			drm_atomic_get_existing_crtc_state(sta
> > > > > te,
> > > > > &crtc->base));
> > > > > +		if (fbc_on_plane_a_only(dev_priv) &&
> > > > > +		    intel_crtc->plane != PLANE_A)
> > > > > +			continue;
> > > > >  
> > > > > -		intel_crtc_state->enable_fbc = true;
> > > > > +		plane_state->enable_fbc = true;
> > > > 
> > > > So looking at this whole thing, I can't see anything that would
> > > > prevent
> > > > enable_fbc being true for multiple primary planes at the same
> > > > time
> > > > Well, apart from the whole "we enable it only for platforms that
> > > > can
> > > > only do
> > > > pipe A" thing.
> > > > 
> > > > So what happens in that case? FBC just ends up getting enabling
> > > > on
> > > > one of the pipes based on the order intel_fbc_enable() gets
> > > > called,
> > > > or something?
> > > 
> > > The first check of intel_fbc_choose_crtc() is supposed to prevent
> > > this
> > > case: if fbc->crtc->primary is not included in the commit we just
> > > return without selecting any plane.
> > 
> > The fbc->crtc thing only works if intel_fbc_enable() was already
> > called
> > for some crtc. But what it it wasn't?
> > 
> > > 
> > > Otherwise, we only pick one CRTC
> > > due to the "break;" statement after setting plane_state->enable_fbc 
> > > to
> > > true.
> > 
> > Only one per atomic operation. But what if there are several
> > happening
> > in parallel on different crtcs?
> 
> I see your point now. Yeah, we'll end up with
> plane_state.enable_fbc=true for two different planes. Later, the first
> one to call intel_fbc_enable() will win, and the others will be
> ignored, so we'll indeed end up with plane states having
> enable_fbc=true but FBC not enabled by them. Not a real bug, I would
> still like to avoid this confusion.
> 
> The simplest solution I can think would be to just
> s/plane_state.enable_fbc/plane_state.can_enable_fbc/ and just let the
> first one to call intel_fbc_enable() win... And then, if we ever decide
> to enable FBC on the older platforms, we can choose to maybe implement
> a better method

Maybe something like "fbc_score"? ;)

> (and fix all the possible FBC problems for those
> platforms).

I think I fixed all the platforms specific issues back in the day. All
the actual problems were in the common code. Although I guess some of
my patches might not have made it in, or I just missed some things.

> Is this solution worth a R-B from you?
> 
> If you have any better idea here, feel free to suggest it. I'm not sure
> it's possible to come up with something much better due to all the
> parallelism involved. Making one CRTC "take over" FBC from the other is
> something I'd really love to avoid.

Yeah, it looked safe enough to me to have the thing set to true on
multiple planes, and if you came to the same conclusion I think we
should be good.

With the rename of the flag to avoid confusion:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Thanks,
> Paulo
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > >  		crtc_chosen = true;
> > > > >  		break;
> > > > >  	}
> > > > > @@ -1130,13 +1132,13 @@ 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->enable_fbc);
> > > > >  			WARN_ON(fbc->active);
> > > > >  		}
> > > > >  		goto out;
> > > > >  	}
> > > > >  
> > > > > -	if (!crtc_state->enable_fbc)
> > > > > +	if (!plane_state->enable_fbc)
> > > > >  		goto out;
> > > > >  
> > > > >  	WARN_ON(fbc->active);
> > > > > -- 
> > > > > 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

Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

[Paulo Zanoni]

Em Sex, 2016-11-11 às 22:24 +0200, Ville Syrjälä escreveu:
> On Fri, Nov 11, 2016 at 05:57:28PM -0200, Paulo Zanoni wrote:
> > 
> > Em Sex, 2016-11-11 às 21:13 +0200, Ville Syrjälä escreveu:
> > > 
> > > On Fri, Nov 11, 2016 at 05:01:54PM -0200, Paulo Zanoni wrote:
> > > > 
> > > > 
> > > > Em Sex, 2016-11-11 às 20:51 +0200, Ville Syrjälä escreveu:
> > > > > 
> > > > > 
> > > > > On Fri, Nov 11, 2016 at 02:57:40PM -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.
> > > > > > 
> > > > > > 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_drv.h |  4 ++--
> > > > > >  drivers/gpu/drm/i915/intel_fbc.c | 36 +++++++++++++++++++-
> > > > > > ----
> > > > > > ----
> > > > > > --------
> > > > > >  2 files changed, 21 insertions(+), 19 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index 003afb8..025cb74 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -403,6 +403,8 @@ struct intel_plane_state {
> > > > > >  	int scaler_id;
> > > > > >  
> > > > > >  	struct drm_intel_sprite_colorkey ckey;
> > > > > > +
> > > > > > +	bool enable_fbc;
> > > > > >  };
> > > > > >  
> > > > > >  struct intel_initial_plane_config {
> > > > > > @@ -648,8 +650,6 @@ struct intel_crtc_state {
> > > > > >  
> > > > > >  	bool ips_enabled;
> > > > > >  
> > > > > > -	bool enable_fbc;
> > > > > > -
> > > > > >  	bool double_wide;
> > > > > >  
> > > > > >  	bool dp_encoder_is_mst;
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > index b095175..fc4ac57 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > @@ -1055,16 +1055,17 @@ void intel_fbc_choose_crtc(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > >  			   struct drm_atomic_state *state)
> > > > > >  {
> > > > > >  	struct intel_fbc *fbc = &dev_priv->fbc;
> > > > > > -	struct drm_plane *plane;
> > > > > > -	struct drm_plane_state *plane_state;
> > > > > > +	struct drm_crtc *crtc;
> > > > > > +	struct drm_crtc_state *crtc_state;
> > > > > >  	bool crtc_chosen = false;
> > > > > >  	int i;
> > > > > >  
> > > > > >  	mutex_lock(&fbc->lock);
> > > > > >  
> > > > > > -	/* Does this atomic commit involve the CRTC
> > > > > > currently
> > > > > > tied
> > > > > > to FBC? */
> > > > > > +	/* Does this atomic commit involve the plane
> > > > > > currently
> > > > > > tied to FBC? */
> > > > > >  	if (fbc->crtc &&
> > > > > > -	    !drm_atomic_get_existing_crtc_state(state,
> > > > > > &fbc-
> > > > > > > 
> > > > > > > crtc-
> > > > > > > 
> > > > > > > base))
> > > > > > +	    !drm_atomic_get_existing_plane_state(state,
> > > > > > +						 fbc-
> > > > > > >crtc-
> > > > > > > 
> > > > > > > 
> > > > > > > base.primary))
> > > > > >  		goto out;
> > > > > >  
> > > > > >  	if (!intel_fbc_can_enable(dev_priv))
> > > > > > @@ -1074,25 +1075,26 @@ void intel_fbc_choose_crtc(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > >  	 * 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);
> > > > > > +	for_each_crtc_in_state(state, crtc, crtc_state, i)
> > > > > > {
> > > > > > +		struct intel_plane_state *plane_state =
> > > > > > to_intel_plane_state(
> > > > > > +			drm_atomic_get_existing_plane_stat
> > > > > > e(st
> > > > > > ate,
> > > > > > +							  
> > > > > >   cr
> > > > > > tc-
> > > > > > > 
> > > > > > > 
> > > > > > > primary));
> > > > > > +		struct intel_crtc *intel_crtc =
> > > > > > to_intel_crtc(crtc);
> > > > > >  
> > > > > > -		if (!intel_plane_state->base.visible)
> > > > > > +		if (!plane_state)
> > > > > >  			continue;
> > > > > >  
> > > > > > -		if (fbc_on_pipe_a_only(dev_priv) && crtc-
> > > > > > >pipe 
> > > > > > !=
> > > > > > PIPE_A)
> > > > > > +		if (!plane_state->base.visible)
> > > > > >  			continue;
> > > > > >  
> > > > > > -		if (fbc_on_plane_a_only(dev_priv) && crtc-
> > > > > > > 
> > > > > > > plane
> > > > > > != PLANE_A)
> > > > > > +		if (fbc_on_pipe_a_only(dev_priv) &&
> > > > > > intel_crtc-
> > > > > > > 
> > > > > > > 
> > > > > > > pipe != PIPE_A)
> > > > > >  			continue;
> > > > > >  
> > > > > > -		intel_crtc_state = to_intel_crtc_state(
> > > > > > -			drm_atomic_get_existing_crtc_state
> > > > > > (sta
> > > > > > te,
> > > > > > &crtc->base));
> > > > > > +		if (fbc_on_plane_a_only(dev_priv) &&
> > > > > > +		    intel_crtc->plane != PLANE_A)
> > > > > > +			continue;
> > > > > >  
> > > > > > -		intel_crtc_state->enable_fbc = true;
> > > > > > +		plane_state->enable_fbc = true;
> > > > > 
> > > > > So looking at this whole thing, I can't see anything that
> > > > > would
> > > > > prevent
> > > > > enable_fbc being true for multiple primary planes at the same
> > > > > time
> > > > > Well, apart from the whole "we enable it only for platforms
> > > > > that
> > > > > can
> > > > > only do
> > > > > pipe A" thing.
> > > > > 
> > > > > So what happens in that case? FBC just ends up getting
> > > > > enabling
> > > > > on
> > > > > one of the pipes based on the order intel_fbc_enable() gets
> > > > > called,
> > > > > or something?
> > > > 
> > > > The first check of intel_fbc_choose_crtc() is supposed to
> > > > prevent
> > > > this
> > > > case: if fbc->crtc->primary is not included in the commit we
> > > > just
> > > > return without selecting any plane.
> > > 
> > > The fbc->crtc thing only works if intel_fbc_enable() was already
> > > called
> > > for some crtc. But what it it wasn't?
> > > 
> > > > 
> > > > 
> > > > Otherwise, we only pick one CRTC
> > > > due to the "break;" statement after setting plane_state-
> > > > >enable_fbc 
> > > > to
> > > > true.
> > > 
> > > Only one per atomic operation. But what if there are several
> > > happening
> > > in parallel on different crtcs?
> > 
> > I see your point now. Yeah, we'll end up with
> > plane_state.enable_fbc=true for two different planes. Later, the
> > first
> > one to call intel_fbc_enable() will win, and the others will be
> > ignored, so we'll indeed end up with plane states having
> > enable_fbc=true but FBC not enabled by them. Not a real bug, I
> > would
> > still like to avoid this confusion.
> > 
> > The simplest solution I can think would be to just
> > s/plane_state.enable_fbc/plane_state.can_enable_fbc/ and just let
> > the
> > first one to call intel_fbc_enable() win... And then, if we ever
> > decide
> > to enable FBC on the older platforms, we can choose to maybe
> > implement
> > a better method
> 
> Maybe something like "fbc_score"? ;)

The design of the current function was supposed to allow Ville to
implement his fbc_score in case he wanted. But this certainly didn't
take into account multiple parallel commits: it would only work if
multiple CRTCs were included in the same commit (as you just pointed
today).

But then: if we're having separate parallel commits, when would we be
able to loop through the scores to only actually enable FBC on the best
score? 

For example, if we do two parallel atomic_check()s and end with
plane_a_score=1 and plane_b_score=2, then later we do A's commit() and
call intel_fbc_enable() for it, how do we conclude that we shouldn't
enable FBC for plane A? We're not even sure if plane B is going to
actually commit the plane state it calculated (maybe it was
check_only).

And then, if we decide to only compute everything during commit()
instead of check(), we'll just also end up enabling FBC for plane A
since A's commit() will run first and we'll have no idea that B's
commit is incoming.

The only option would be to disable FBC for plane A and enable for
plane B during B's commit. But I'm not looking forward to implement
this right now.

So we kinda have 3 options:

1 - Keep the current approach (with the renamed variable) that at least
allows us to pick the best CRTC in case a single commit includes
multiple CRTCs.

2 - Go the simplest route and not even bother picking the best CRTC
among a single commit due to the fact that parallel commits won't
guarantee we always have the best FBC plane enabled, so why bother.

3 - Wait for someone to implement the takeover approach.

Thanks for the review,
Paulo


> > 
> > (and fix all the possible FBC problems for those
> > platforms).
> 
> I think I fixed all the platforms specific issues back in the day.
> All
> the actual problems were in the common code. Although I guess some of
> my patches might not have made it in, or I just missed some things.
> 
> > 
> > Is this solution worth a R-B from you?
> > 
> > If you have any better idea here, feel free to suggest it. I'm not
> > sure
> > it's possible to come up with something much better due to all the
> > parallelism involved. Making one CRTC "take over" FBC from the
> > other is
> > something I'd really love to avoid.
> 
> Yeah, it looked safe enough to me to have the thing set to true on
> multiple planes, and if you came to the same conclusion I think we
> should be good.
> 
> With the rename of the flag to avoid confusion:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> > 
> > 
> > Thanks,
> > Paulo
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > >  		crtc_chosen = true;
> > > > > >  		break;
> > > > > >  	}
> > > > > > @@ -1130,13 +1132,13 @@ 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->enable_fbc);
> > > > > >  			WARN_ON(fbc->active);
> > > > > >  		}
> > > > > >  		goto out;
> > > > > >  	}
> > > > > >  
> > > > > > -	if (!crtc_state->enable_fbc)
> > > > > > +	if (!plane_state->enable_fbc)
> > > > > >  		goto out;
> > > > > >  
> > > > > >  	WARN_ON(fbc->active);
> > > > > > -- 
> > > > > > 2.7.4
> > > > > 
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

[Ville Syrjälä]

On Fri, Nov 11, 2016 at 06:49:59PM -0200, Paulo Zanoni wrote:
> Em Sex, 2016-11-11 às 22:24 +0200, Ville Syrjälä escreveu:
> > On Fri, Nov 11, 2016 at 05:57:28PM -0200, Paulo Zanoni wrote:
> > > 
> > > Em Sex, 2016-11-11 às 21:13 +0200, Ville Syrjälä escreveu:
> > > > 
> > > > On Fri, Nov 11, 2016 at 05:01:54PM -0200, Paulo Zanoni wrote:
> > > > > 
> > > > > 
> > > > > Em Sex, 2016-11-11 às 20:51 +0200, Ville Syrjälä escreveu:
> > > > > > 
> > > > > > 
> > > > > > On Fri, Nov 11, 2016 at 02:57:40PM -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.
> > > > > > > 
> > > > > > > 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_drv.h |  4 ++--
> > > > > > >  drivers/gpu/drm/i915/intel_fbc.c | 36 +++++++++++++++++++-
> > > > > > > ----
> > > > > > > ----
> > > > > > > --------
> > > > > > >  2 files changed, 21 insertions(+), 19 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > index 003afb8..025cb74 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > @@ -403,6 +403,8 @@ struct intel_plane_state {
> > > > > > >  	int scaler_id;
> > > > > > >  
> > > > > > >  	struct drm_intel_sprite_colorkey ckey;
> > > > > > > +
> > > > > > > +	bool enable_fbc;
> > > > > > >  };
> > > > > > >  
> > > > > > >  struct intel_initial_plane_config {
> > > > > > > @@ -648,8 +650,6 @@ struct intel_crtc_state {
> > > > > > >  
> > > > > > >  	bool ips_enabled;
> > > > > > >  
> > > > > > > -	bool enable_fbc;
> > > > > > > -
> > > > > > >  	bool double_wide;
> > > > > > >  
> > > > > > >  	bool dp_encoder_is_mst;
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > index b095175..fc4ac57 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > @@ -1055,16 +1055,17 @@ void intel_fbc_choose_crtc(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >  			   struct drm_atomic_state *state)
> > > > > > >  {
> > > > > > >  	struct intel_fbc *fbc = &dev_priv->fbc;
> > > > > > > -	struct drm_plane *plane;
> > > > > > > -	struct drm_plane_state *plane_state;
> > > > > > > +	struct drm_crtc *crtc;
> > > > > > > +	struct drm_crtc_state *crtc_state;
> > > > > > >  	bool crtc_chosen = false;
> > > > > > >  	int i;
> > > > > > >  
> > > > > > >  	mutex_lock(&fbc->lock);
> > > > > > >  
> > > > > > > -	/* Does this atomic commit involve the CRTC
> > > > > > > currently
> > > > > > > tied
> > > > > > > to FBC? */
> > > > > > > +	/* Does this atomic commit involve the plane
> > > > > > > currently
> > > > > > > tied to FBC? */
> > > > > > >  	if (fbc->crtc &&
> > > > > > > -	    !drm_atomic_get_existing_crtc_state(state,
> > > > > > > &fbc-
> > > > > > > > 
> > > > > > > > crtc-
> > > > > > > > 
> > > > > > > > base))
> > > > > > > +	    !drm_atomic_get_existing_plane_state(state,
> > > > > > > +						 fbc-
> > > > > > > >crtc-
> > > > > > > > 
> > > > > > > > 
> > > > > > > > base.primary))
> > > > > > >  		goto out;
> > > > > > >  
> > > > > > >  	if (!intel_fbc_can_enable(dev_priv))
> > > > > > > @@ -1074,25 +1075,26 @@ void intel_fbc_choose_crtc(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >  	 * 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);
> > > > > > > +	for_each_crtc_in_state(state, crtc, crtc_state, i)
> > > > > > > {
> > > > > > > +		struct intel_plane_state *plane_state =
> > > > > > > to_intel_plane_state(
> > > > > > > +			drm_atomic_get_existing_plane_stat
> > > > > > > e(st
> > > > > > > ate,
> > > > > > > +							  
> > > > > > >   cr
> > > > > > > tc-
> > > > > > > > 
> > > > > > > > 
> > > > > > > > primary));
> > > > > > > +		struct intel_crtc *intel_crtc =
> > > > > > > to_intel_crtc(crtc);
> > > > > > >  
> > > > > > > -		if (!intel_plane_state->base.visible)
> > > > > > > +		if (!plane_state)
> > > > > > >  			continue;
> > > > > > >  
> > > > > > > -		if (fbc_on_pipe_a_only(dev_priv) && crtc-
> > > > > > > >pipe 
> > > > > > > !=
> > > > > > > PIPE_A)
> > > > > > > +		if (!plane_state->base.visible)
> > > > > > >  			continue;
> > > > > > >  
> > > > > > > -		if (fbc_on_plane_a_only(dev_priv) && crtc-
> > > > > > > > 
> > > > > > > > plane
> > > > > > > != PLANE_A)
> > > > > > > +		if (fbc_on_pipe_a_only(dev_priv) &&
> > > > > > > intel_crtc-
> > > > > > > > 
> > > > > > > > 
> > > > > > > > pipe != PIPE_A)
> > > > > > >  			continue;
> > > > > > >  
> > > > > > > -		intel_crtc_state = to_intel_crtc_state(
> > > > > > > -			drm_atomic_get_existing_crtc_state
> > > > > > > (sta
> > > > > > > te,
> > > > > > > &crtc->base));
> > > > > > > +		if (fbc_on_plane_a_only(dev_priv) &&
> > > > > > > +		    intel_crtc->plane != PLANE_A)
> > > > > > > +			continue;
> > > > > > >  
> > > > > > > -		intel_crtc_state->enable_fbc = true;
> > > > > > > +		plane_state->enable_fbc = true;
> > > > > > 
> > > > > > So looking at this whole thing, I can't see anything that
> > > > > > would
> > > > > > prevent
> > > > > > enable_fbc being true for multiple primary planes at the same
> > > > > > time
> > > > > > Well, apart from the whole "we enable it only for platforms
> > > > > > that
> > > > > > can
> > > > > > only do
> > > > > > pipe A" thing.
> > > > > > 
> > > > > > So what happens in that case? FBC just ends up getting
> > > > > > enabling
> > > > > > on
> > > > > > one of the pipes based on the order intel_fbc_enable() gets
> > > > > > called,
> > > > > > or something?
> > > > > 
> > > > > The first check of intel_fbc_choose_crtc() is supposed to
> > > > > prevent
> > > > > this
> > > > > case: if fbc->crtc->primary is not included in the commit we
> > > > > just
> > > > > return without selecting any plane.
> > > > 
> > > > The fbc->crtc thing only works if intel_fbc_enable() was already
> > > > called
> > > > for some crtc. But what it it wasn't?
> > > > 
> > > > > 
> > > > > 
> > > > > Otherwise, we only pick one CRTC
> > > > > due to the "break;" statement after setting plane_state-
> > > > > >enable_fbc 
> > > > > to
> > > > > true.
> > > > 
> > > > Only one per atomic operation. But what if there are several
> > > > happening
> > > > in parallel on different crtcs?
> > > 
> > > I see your point now. Yeah, we'll end up with
> > > plane_state.enable_fbc=true for two different planes. Later, the
> > > first
> > > one to call intel_fbc_enable() will win, and the others will be
> > > ignored, so we'll indeed end up with plane states having
> > > enable_fbc=true but FBC not enabled by them. Not a real bug, I
> > > would
> > > still like to avoid this confusion.
> > > 
> > > The simplest solution I can think would be to just
> > > s/plane_state.enable_fbc/plane_state.can_enable_fbc/ and just let
> > > the
> > > first one to call intel_fbc_enable() win... And then, if we ever
> > > decide
> > > to enable FBC on the older platforms, we can choose to maybe
> > > implement
> > > a better method
> > 
> > Maybe something like "fbc_score"? ;)
> 
> The design of the current function was supposed to allow Ville to
> implement his fbc_score in case he wanted. But this certainly didn't
> take into account multiple parallel commits: it would only work if
> multiple CRTCs were included in the same commit (as you just pointed
> today).
> 
> But then: if we're having separate parallel commits, when would we be
> able to loop through the scores to only actually enable FBC on the best
> score? 
> 
> For example, if we do two parallel atomic_check()s and end with
> plane_a_score=1 and plane_b_score=2, then later we do A's commit() and
> call intel_fbc_enable() for it, how do we conclude that we shouldn't
> enable FBC for plane A? We're not even sure if plane B is going to
> actually commit the plane state it calculated (maybe it was
> check_only).
> 
> And then, if we decide to only compute everything during commit()
> instead of check(), we'll just also end up enabling FBC for plane A
> since A's commit() will run first and we'll have no idea that B's
> commit is incoming.
> 
> The only option would be to disable FBC for plane A and enable for
> plane B during B's commit. But I'm not looking forward to implement
> this right now.

All the enable/disable should be totally async and so you should be
able to kick them off from anywhere. All the state, including the scores,
would be protected by the fbc mutex. I had a vblank worker type of
thing for this purpose in my patches.

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

Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

[Paulo Zanoni]

Em Seg, 2016-11-14 às 22:26 +0200, Ville Syrjälä escreveu:
> On Fri, Nov 11, 2016 at 06:49:59PM -0200, Paulo Zanoni wrote:
> > 
> > Em Sex, 2016-11-11 às 22:24 +0200, Ville Syrjälä escreveu:
> > > 
> > > On Fri, Nov 11, 2016 at 05:57:28PM -0200, Paulo Zanoni wrote:
> > > > 
> > > > 
> > > > Em Sex, 2016-11-11 às 21:13 +0200, Ville Syrjälä escreveu:
> > > > > 
> > > > > 
> > > > > On Fri, Nov 11, 2016 at 05:01:54PM -0200, Paulo Zanoni wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Em Sex, 2016-11-11 às 20:51 +0200, Ville Syrjälä escreveu:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Fri, Nov 11, 2016 at 02:57:40PM -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.
> > > > > > > > 
> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.c
> > > > > > > > om>
> > > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_drv.h |  4 ++--
> > > > > > > >  drivers/gpu/drm/i915/intel_fbc.c | 36
> > > > > > > > +++++++++++++++++++-
> > > > > > > > ----
> > > > > > > > ----
> > > > > > > > --------
> > > > > > > >  2 files changed, 21 insertions(+), 19 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > index 003afb8..025cb74 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > @@ -403,6 +403,8 @@ struct intel_plane_state {
> > > > > > > >  	int scaler_id;
> > > > > > > >  
> > > > > > > >  	struct drm_intel_sprite_colorkey ckey;
> > > > > > > > +
> > > > > > > > +	bool enable_fbc;
> > > > > > > >  };
> > > > > > > >  
> > > > > > > >  struct intel_initial_plane_config {
> > > > > > > > @@ -648,8 +650,6 @@ struct intel_crtc_state {
> > > > > > > >  
> > > > > > > >  	bool ips_enabled;
> > > > > > > >  
> > > > > > > > -	bool enable_fbc;
> > > > > > > > -
> > > > > > > >  	bool double_wide;
> > > > > > > >  
> > > > > > > >  	bool dp_encoder_is_mst;
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > > index b095175..fc4ac57 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > > @@ -1055,16 +1055,17 @@ void
> > > > > > > > intel_fbc_choose_crtc(struct
> > > > > > > > drm_i915_private *dev_priv,
> > > > > > > >  			   struct drm_atomic_state
> > > > > > > > *state)
> > > > > > > >  {
> > > > > > > >  	struct intel_fbc *fbc = &dev_priv->fbc;
> > > > > > > > -	struct drm_plane *plane;
> > > > > > > > -	struct drm_plane_state *plane_state;
> > > > > > > > +	struct drm_crtc *crtc;
> > > > > > > > +	struct drm_crtc_state *crtc_state;
> > > > > > > >  	bool crtc_chosen = false;
> > > > > > > >  	int i;
> > > > > > > >  
> > > > > > > >  	mutex_lock(&fbc->lock);
> > > > > > > >  
> > > > > > > > -	/* Does this atomic commit involve the CRTC
> > > > > > > > currently
> > > > > > > > tied
> > > > > > > > to FBC? */
> > > > > > > > +	/* Does this atomic commit involve the plane
> > > > > > > > currently
> > > > > > > > tied to FBC? */
> > > > > > > >  	if (fbc->crtc &&
> > > > > > > > -	    !drm_atomic_get_existing_crtc_state(state,
> > > > > > > > &fbc-
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > crtc-
> > > > > > > > > 
> > > > > > > > > base))
> > > > > > > > +	    !drm_atomic_get_existing_plane_state(state
> > > > > > > > ,
> > > > > > > > +						 fbc-
> > > > > > > > > 
> > > > > > > > > crtc-
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > base.primary))
> > > > > > > >  		goto out;
> > > > > > > >  
> > > > > > > >  	if (!intel_fbc_can_enable(dev_priv))
> > > > > > > > @@ -1074,25 +1075,26 @@ void
> > > > > > > > intel_fbc_choose_crtc(struct
> > > > > > > > drm_i915_private *dev_priv,
> > > > > > > >  	 * 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_sta
> > > > > > > > te);
> > > > > > > > -		struct intel_crtc_state
> > > > > > > > *intel_crtc_state;
> > > > > > > > -		struct intel_crtc *crtc =
> > > > > > > > to_intel_crtc(plane_state->crtc);
> > > > > > > > +	for_each_crtc_in_state(state, crtc,
> > > > > > > > crtc_state, i)
> > > > > > > > {
> > > > > > > > +		struct intel_plane_state *plane_state
> > > > > > > > =
> > > > > > > > to_intel_plane_state(
> > > > > > > > +			drm_atomic_get_existing_plane_
> > > > > > > > stat
> > > > > > > > e(st
> > > > > > > > ate,
> > > > > > > > +							
> > > > > > > >   
> > > > > > > >   cr
> > > > > > > > tc-
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > primary));
> > > > > > > > +		struct intel_crtc *intel_crtc =
> > > > > > > > to_intel_crtc(crtc);
> > > > > > > >  
> > > > > > > > -		if (!intel_plane_state->base.visible)
> > > > > > > > +		if (!plane_state)
> > > > > > > >  			continue;
> > > > > > > >  
> > > > > > > > -		if (fbc_on_pipe_a_only(dev_priv) &&
> > > > > > > > crtc-
> > > > > > > > > 
> > > > > > > > > pipe 
> > > > > > > > !=
> > > > > > > > PIPE_A)
> > > > > > > > +		if (!plane_state->base.visible)
> > > > > > > >  			continue;
> > > > > > > >  
> > > > > > > > -		if (fbc_on_plane_a_only(dev_priv) &&
> > > > > > > > crtc-
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > plane
> > > > > > > > != PLANE_A)
> > > > > > > > +		if (fbc_on_pipe_a_only(dev_priv) &&
> > > > > > > > intel_crtc-
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > pipe != PIPE_A)
> > > > > > > >  			continue;
> > > > > > > >  
> > > > > > > > -		intel_crtc_state =
> > > > > > > > to_intel_crtc_state(
> > > > > > > > -			drm_atomic_get_existing_crtc_s
> > > > > > > > tate
> > > > > > > > (sta
> > > > > > > > te,
> > > > > > > > &crtc->base));
> > > > > > > > +		if (fbc_on_plane_a_only(dev_priv) &&
> > > > > > > > +		    intel_crtc->plane != PLANE_A)
> > > > > > > > +			continue;
> > > > > > > >  
> > > > > > > > -		intel_crtc_state->enable_fbc = true;
> > > > > > > > +		plane_state->enable_fbc = true;
> > > > > > > 
> > > > > > > So looking at this whole thing, I can't see anything that
> > > > > > > would
> > > > > > > prevent
> > > > > > > enable_fbc being true for multiple primary planes at the
> > > > > > > same
> > > > > > > time
> > > > > > > Well, apart from the whole "we enable it only for
> > > > > > > platforms
> > > > > > > that
> > > > > > > can
> > > > > > > only do
> > > > > > > pipe A" thing.
> > > > > > > 
> > > > > > > So what happens in that case? FBC just ends up getting
> > > > > > > enabling
> > > > > > > on
> > > > > > > one of the pipes based on the order intel_fbc_enable()
> > > > > > > gets
> > > > > > > called,
> > > > > > > or something?
> > > > > > 
> > > > > > The first check of intel_fbc_choose_crtc() is supposed to
> > > > > > prevent
> > > > > > this
> > > > > > case: if fbc->crtc->primary is not included in the commit
> > > > > > we
> > > > > > just
> > > > > > return without selecting any plane.
> > > > > 
> > > > > The fbc->crtc thing only works if intel_fbc_enable() was
> > > > > already
> > > > > called
> > > > > for some crtc. But what it it wasn't?
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Otherwise, we only pick one CRTC
> > > > > > due to the "break;" statement after setting plane_state-
> > > > > > > 
> > > > > > > enable_fbc 
> > > > > > to
> > > > > > true.
> > > > > 
> > > > > Only one per atomic operation. But what if there are several
> > > > > happening
> > > > > in parallel on different crtcs?
> > > > 
> > > > I see your point now. Yeah, we'll end up with
> > > > plane_state.enable_fbc=true for two different planes. Later,
> > > > the
> > > > first
> > > > one to call intel_fbc_enable() will win, and the others will be
> > > > ignored, so we'll indeed end up with plane states having
> > > > enable_fbc=true but FBC not enabled by them. Not a real bug, I
> > > > would
> > > > still like to avoid this confusion.
> > > > 
> > > > The simplest solution I can think would be to just
> > > > s/plane_state.enable_fbc/plane_state.can_enable_fbc/ and just
> > > > let
> > > > the
> > > > first one to call intel_fbc_enable() win... And then, if we
> > > > ever
> > > > decide
> > > > to enable FBC on the older platforms, we can choose to maybe
> > > > implement
> > > > a better method
> > > 
> > > Maybe something like "fbc_score"? ;)
> > 
> > The design of the current function was supposed to allow Ville to
> > implement his fbc_score in case he wanted. But this certainly
> > didn't
> > take into account multiple parallel commits: it would only work if
> > multiple CRTCs were included in the same commit (as you just
> > pointed
> > today).
> > 
> > But then: if we're having separate parallel commits, when would we
> > be
> > able to loop through the scores to only actually enable FBC on the
> > best
> > score? 
> > 
> > For example, if we do two parallel atomic_check()s and end with
> > plane_a_score=1 and plane_b_score=2, then later we do A's commit()
> > and
> > call intel_fbc_enable() for it, how do we conclude that we
> > shouldn't
> > enable FBC for plane A? We're not even sure if plane B is going to
> > actually commit the plane state it calculated (maybe it was
> > check_only).
> > 
> > And then, if we decide to only compute everything during commit()
> > instead of check(), we'll just also end up enabling FBC for plane A
> > since A's commit() will run first and we'll have no idea that B's
> > commit is incoming.
> > 
> > The only option would be to disable FBC for plane A and enable for
> > plane B during B's commit. But I'm not looking forward to implement
> > this right now.
> 
> All the enable/disable should be totally async and so you should be
> able to kick them off from anywhere. All the state, including the
> scores,
> would be protected by the fbc mutex. I had a vblank worker type of
> thing for this purpose in my patches.

As far as I remember, the complicated part here was related to the CFB
allocation/deallocation. If we disable FBC while the pipe is running we
need to wait for a vblank before we deallocate the CFB, otherwise the
HW is going to keep writing to the stolen memory. While we certainly
can adjust to this, in my FBC reworks I simplified this a lot, and now
we only enable/disable the CFB during crtc_{en,dis}able, so we only
ever free the CFB while the pipe is off, and we don't
deallocate+reallocate it at every single page flip.

The other complicated part is that if we try to allocate the new CFB
for the new plane without first freeing the old one (waiting for the
vblank on the other CRTC) we'll probably get an -ENOMEM from the stolen
mm. On the other hand, if we actually do all the vblank the waits,
well, we'll have to implement the code to properly wait for everything.
And while we're doing the waits, the world may change, new modesets may
happen, we may have to abort or change our minds, and handling all
these cases is there things get complicating.

And we have intel_fbc_work_fn() to help.

Anyway, it is possible to do what you're suggesting. It's just that a
lot of simple things will become complicated.

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

Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

[Ville Syrjälä]

On Mon, Nov 14, 2016 at 06:49:22PM -0200, Paulo Zanoni wrote:
> Em Seg, 2016-11-14 às 22:26 +0200, Ville Syrjälä escreveu:
> > On Fri, Nov 11, 2016 at 06:49:59PM -0200, Paulo Zanoni wrote:
> > > 
> > > Em Sex, 2016-11-11 às 22:24 +0200, Ville Syrjälä escreveu:
> > > > 
> > > > On Fri, Nov 11, 2016 at 05:57:28PM -0200, Paulo Zanoni wrote:
> > > > > 
> > > > > 
> > > > > Em Sex, 2016-11-11 às 21:13 +0200, Ville Syrjälä escreveu:
> > > > > > 
> > > > > > 
> > > > > > On Fri, Nov 11, 2016 at 05:01:54PM -0200, Paulo Zanoni wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Em Sex, 2016-11-11 às 20:51 +0200, Ville Syrjälä escreveu:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Fri, Nov 11, 2016 at 02:57:40PM -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.
> > > > > > > > > 
> > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.c
> > > > > > > > > om>
> > > > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/i915/intel_drv.h |  4 ++--
> > > > > > > > >  drivers/gpu/drm/i915/intel_fbc.c | 36
> > > > > > > > > +++++++++++++++++++-
> > > > > > > > > ----
> > > > > > > > > ----
> > > > > > > > > --------
> > > > > > > > >  2 files changed, 21 insertions(+), 19 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > > index 003afb8..025cb74 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > > @@ -403,6 +403,8 @@ struct intel_plane_state {
> > > > > > > > >  	int scaler_id;
> > > > > > > > >  
> > > > > > > > >  	struct drm_intel_sprite_colorkey ckey;
> > > > > > > > > +
> > > > > > > > > +	bool enable_fbc;
> > > > > > > > >  };
> > > > > > > > >  
> > > > > > > > >  struct intel_initial_plane_config {
> > > > > > > > > @@ -648,8 +650,6 @@ struct intel_crtc_state {
> > > > > > > > >  
> > > > > > > > >  	bool ips_enabled;
> > > > > > > > >  
> > > > > > > > > -	bool enable_fbc;
> > > > > > > > > -
> > > > > > > > >  	bool double_wide;
> > > > > > > > >  
> > > > > > > > >  	bool dp_encoder_is_mst;
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > > > index b095175..fc4ac57 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > > > > > @@ -1055,16 +1055,17 @@ void
> > > > > > > > > intel_fbc_choose_crtc(struct
> > > > > > > > > drm_i915_private *dev_priv,
> > > > > > > > >  			   struct drm_atomic_state
> > > > > > > > > *state)
> > > > > > > > >  {
> > > > > > > > >  	struct intel_fbc *fbc = &dev_priv->fbc;
> > > > > > > > > -	struct drm_plane *plane;
> > > > > > > > > -	struct drm_plane_state *plane_state;
> > > > > > > > > +	struct drm_crtc *crtc;
> > > > > > > > > +	struct drm_crtc_state *crtc_state;
> > > > > > > > >  	bool crtc_chosen = false;
> > > > > > > > >  	int i;
> > > > > > > > >  
> > > > > > > > >  	mutex_lock(&fbc->lock);
> > > > > > > > >  
> > > > > > > > > -	/* Does this atomic commit involve the CRTC
> > > > > > > > > currently
> > > > > > > > > tied
> > > > > > > > > to FBC? */
> > > > > > > > > +	/* Does this atomic commit involve the plane
> > > > > > > > > currently
> > > > > > > > > tied to FBC? */
> > > > > > > > >  	if (fbc->crtc &&
> > > > > > > > > -	    !drm_atomic_get_existing_crtc_state(state,
> > > > > > > > > &fbc-
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > crtc-
> > > > > > > > > > 
> > > > > > > > > > base))
> > > > > > > > > +	    !drm_atomic_get_existing_plane_state(state
> > > > > > > > > ,
> > > > > > > > > +						 fbc-
> > > > > > > > > > 
> > > > > > > > > > crtc-
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > base.primary))
> > > > > > > > >  		goto out;
> > > > > > > > >  
> > > > > > > > >  	if (!intel_fbc_can_enable(dev_priv))
> > > > > > > > > @@ -1074,25 +1075,26 @@ void
> > > > > > > > > intel_fbc_choose_crtc(struct
> > > > > > > > > drm_i915_private *dev_priv,
> > > > > > > > >  	 * 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_sta
> > > > > > > > > te);
> > > > > > > > > -		struct intel_crtc_state
> > > > > > > > > *intel_crtc_state;
> > > > > > > > > -		struct intel_crtc *crtc =
> > > > > > > > > to_intel_crtc(plane_state->crtc);
> > > > > > > > > +	for_each_crtc_in_state(state, crtc,
> > > > > > > > > crtc_state, i)
> > > > > > > > > {
> > > > > > > > > +		struct intel_plane_state *plane_state
> > > > > > > > > =
> > > > > > > > > to_intel_plane_state(
> > > > > > > > > +			drm_atomic_get_existing_plane_
> > > > > > > > > stat
> > > > > > > > > e(st
> > > > > > > > > ate,
> > > > > > > > > +							
> > > > > > > > >   
> > > > > > > > >   cr
> > > > > > > > > tc-
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > primary));
> > > > > > > > > +		struct intel_crtc *intel_crtc =
> > > > > > > > > to_intel_crtc(crtc);
> > > > > > > > >  
> > > > > > > > > -		if (!intel_plane_state->base.visible)
> > > > > > > > > +		if (!plane_state)
> > > > > > > > >  			continue;
> > > > > > > > >  
> > > > > > > > > -		if (fbc_on_pipe_a_only(dev_priv) &&
> > > > > > > > > crtc-
> > > > > > > > > > 
> > > > > > > > > > pipe 
> > > > > > > > > !=
> > > > > > > > > PIPE_A)
> > > > > > > > > +		if (!plane_state->base.visible)
> > > > > > > > >  			continue;
> > > > > > > > >  
> > > > > > > > > -		if (fbc_on_plane_a_only(dev_priv) &&
> > > > > > > > > crtc-
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > plane
> > > > > > > > > != PLANE_A)
> > > > > > > > > +		if (fbc_on_pipe_a_only(dev_priv) &&
> > > > > > > > > intel_crtc-
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > pipe != PIPE_A)
> > > > > > > > >  			continue;
> > > > > > > > >  
> > > > > > > > > -		intel_crtc_state =
> > > > > > > > > to_intel_crtc_state(
> > > > > > > > > -			drm_atomic_get_existing_crtc_s
> > > > > > > > > tate
> > > > > > > > > (sta
> > > > > > > > > te,
> > > > > > > > > &crtc->base));
> > > > > > > > > +		if (fbc_on_plane_a_only(dev_priv) &&
> > > > > > > > > +		    intel_crtc->plane != PLANE_A)
> > > > > > > > > +			continue;
> > > > > > > > >  
> > > > > > > > > -		intel_crtc_state->enable_fbc = true;
> > > > > > > > > +		plane_state->enable_fbc = true;
> > > > > > > > 
> > > > > > > > So looking at this whole thing, I can't see anything that
> > > > > > > > would
> > > > > > > > prevent
> > > > > > > > enable_fbc being true for multiple primary planes at the
> > > > > > > > same
> > > > > > > > time
> > > > > > > > Well, apart from the whole "we enable it only for
> > > > > > > > platforms
> > > > > > > > that
> > > > > > > > can
> > > > > > > > only do
> > > > > > > > pipe A" thing.
> > > > > > > > 
> > > > > > > > So what happens in that case? FBC just ends up getting
> > > > > > > > enabling
> > > > > > > > on
> > > > > > > > one of the pipes based on the order intel_fbc_enable()
> > > > > > > > gets
> > > > > > > > called,
> > > > > > > > or something?
> > > > > > > 
> > > > > > > The first check of intel_fbc_choose_crtc() is supposed to
> > > > > > > prevent
> > > > > > > this
> > > > > > > case: if fbc->crtc->primary is not included in the commit
> > > > > > > we
> > > > > > > just
> > > > > > > return without selecting any plane.
> > > > > > 
> > > > > > The fbc->crtc thing only works if intel_fbc_enable() was
> > > > > > already
> > > > > > called
> > > > > > for some crtc. But what it it wasn't?
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Otherwise, we only pick one CRTC
> > > > > > > due to the "break;" statement after setting plane_state-
> > > > > > > > 
> > > > > > > > enable_fbc 
> > > > > > > to
> > > > > > > true.
> > > > > > 
> > > > > > Only one per atomic operation. But what if there are several
> > > > > > happening
> > > > > > in parallel on different crtcs?
> > > > > 
> > > > > I see your point now. Yeah, we'll end up with
> > > > > plane_state.enable_fbc=true for two different planes. Later,
> > > > > the
> > > > > first
> > > > > one to call intel_fbc_enable() will win, and the others will be
> > > > > ignored, so we'll indeed end up with plane states having
> > > > > enable_fbc=true but FBC not enabled by them. Not a real bug, I
> > > > > would
> > > > > still like to avoid this confusion.
> > > > > 
> > > > > The simplest solution I can think would be to just
> > > > > s/plane_state.enable_fbc/plane_state.can_enable_fbc/ and just
> > > > > let
> > > > > the
> > > > > first one to call intel_fbc_enable() win... And then, if we
> > > > > ever
> > > > > decide
> > > > > to enable FBC on the older platforms, we can choose to maybe
> > > > > implement
> > > > > a better method
> > > > 
> > > > Maybe something like "fbc_score"? ;)
> > > 
> > > The design of the current function was supposed to allow Ville to
> > > implement his fbc_score in case he wanted. But this certainly
> > > didn't
> > > take into account multiple parallel commits: it would only work if
> > > multiple CRTCs were included in the same commit (as you just
> > > pointed
> > > today).
> > > 
> > > But then: if we're having separate parallel commits, when would we
> > > be
> > > able to loop through the scores to only actually enable FBC on the
> > > best
> > > score? 
> > > 
> > > For example, if we do two parallel atomic_check()s and end with
> > > plane_a_score=1 and plane_b_score=2, then later we do A's commit()
> > > and
> > > call intel_fbc_enable() for it, how do we conclude that we
> > > shouldn't
> > > enable FBC for plane A? We're not even sure if plane B is going to
> > > actually commit the plane state it calculated (maybe it was
> > > check_only).
> > > 
> > > And then, if we decide to only compute everything during commit()
> > > instead of check(), we'll just also end up enabling FBC for plane A
> > > since A's commit() will run first and we'll have no idea that B's
> > > commit is incoming.
> > > 
> > > The only option would be to disable FBC for plane A and enable for
> > > plane B during B's commit. But I'm not looking forward to implement
> > > this right now.
> > 
> > All the enable/disable should be totally async and so you should be
> > able to kick them off from anywhere. All the state, including the
> > scores,
> > would be protected by the fbc mutex. I had a vblank worker type of
> > thing for this purpose in my patches.
> 
> As far as I remember, the complicated part here was related to the CFB
> allocation/deallocation. If we disable FBC while the pipe is running we
> need to wait for a vblank before we deallocate the CFB, otherwise the
> HW is going to keep writing to the stolen memory. While we certainly
> can adjust to this, in my FBC reworks I simplified this a lot, and now
> we only enable/disable the CFB during crtc_{en,dis}able, so we only
> ever free the CFB while the pipe is off, and we don't
> deallocate+reallocate it at every single page flip.

Well you already have to disable FBC in the hardware when you flip to
a buffer that can't be used with FBC. So that part is there. I admit
there is a little bit of extra complication from making sure you don't
deallocate the cfb before FBC has finished, but that complication is
neatly encapsulated in the FBC code. From the flip/modeset path there
should be just two function calls around the update to signal the FBC
machinery.

> The other complicated part is that if we try to allocate the new CFB
> for the new plane without first freeing the old one (waiting for the
> vblank on the other CRTC) we'll probably get an -ENOMEM from the stolen
> mm. On the other hand, if we actually do all the vblank the waits,
> well, we'll have to implement the code to properly wait for everything.
> And while we're doing the waits, the world may change, new modesets may
> happen, we may have to abort or change our minds, and handling all
> these cases is there things get complicating.
> 
> And we have intel_fbc_work_fn() to help.
> 
> Anyway, it is possible to do what you're suggesting. It's just that a
> lot of simple things will become complicated.

As I see it the FBC machinery effectively just needs three states:
- cfb deallocated + fbc disabled
- cfb allocated + fbc disabled
- cfb allocated + fbc enabled

and you just move between these states, with some synchronization in
between. I guess you may want to consider enabling/disabling as two
more states which makes the synchronization more clear.

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

[PATCH 7/7] drm/i915/fbc: convert intel_fbc.c to use INTEL_GEN()

[Paulo Zanoni]

Because it's shorter, easier to read, newer and cooler. And I don't
think anybody else has pending FBC patches right now, so the conflicts
should be minimal.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index fc4ac57..b15c2c3 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -48,17 +48,17 @@ static inline bool fbc_supported(struct drm_i915_private *dev_priv)
 
 static inline bool fbc_on_pipe_a_only(struct drm_i915_private *dev_priv)
 {
-	return IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8;
+	return IS_HASWELL(dev_priv) || INTEL_GEN(dev_priv) >= 8;
 }
 
 static inline bool fbc_on_plane_a_only(struct drm_i915_private *dev_priv)
 {
-	return INTEL_INFO(dev_priv)->gen < 4;
+	return INTEL_GEN(dev_priv) < 4;
 }
 
 static inline bool no_fbc_on_multiple_pipes(struct drm_i915_private *dev_priv)
 {
-	return INTEL_INFO(dev_priv)->gen <= 3;
+	return INTEL_GEN(dev_priv) <= 3;
 }
 
 /*
@@ -351,7 +351,7 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
 
 static bool intel_fbc_hw_is_active(struct drm_i915_private *dev_priv)
 {
-	if (INTEL_INFO(dev_priv)->gen >= 5)
+	if (INTEL_GEN(dev_priv) >= 5)
 		return ilk_fbc_is_active(dev_priv);
 	else if (IS_GM45(dev_priv))
 		return g4x_fbc_is_active(dev_priv);
@@ -365,9 +365,9 @@ static void intel_fbc_hw_activate(struct drm_i915_private *dev_priv)
 
 	fbc->active = true;
 
-	if (INTEL_INFO(dev_priv)->gen >= 7)
+	if (INTEL_GEN(dev_priv) >= 7)
 		gen7_fbc_activate(dev_priv);
-	else if (INTEL_INFO(dev_priv)->gen >= 5)
+	else if (INTEL_GEN(dev_priv) >= 5)
 		ilk_fbc_activate(dev_priv);
 	else if (IS_GM45(dev_priv))
 		g4x_fbc_activate(dev_priv);
@@ -381,7 +381,7 @@ static void intel_fbc_hw_deactivate(struct drm_i915_private *dev_priv)
 
 	fbc->active = false;
 
-	if (INTEL_INFO(dev_priv)->gen >= 5)
+	if (INTEL_GEN(dev_priv) >= 5)
 		ilk_fbc_deactivate(dev_priv);
 	else if (IS_GM45(dev_priv))
 		g4x_fbc_deactivate(dev_priv);
@@ -561,7 +561,7 @@ static int find_compression_threshold(struct drm_i915_private *dev_priv,
 
 	ret = i915_gem_stolen_insert_node_in_range(dev_priv, node, size >>= 1,
 						   4096, 0, end);
-	if (ret && INTEL_INFO(dev_priv)->gen <= 4) {
+	if (ret && INTEL_GEN(dev_priv) <= 4) {
 		return 0;
 	} else if (ret) {
 		compression_threshold <<= 1;
@@ -594,7 +594,7 @@ static int intel_fbc_alloc_cfb(struct intel_crtc *crtc)
 
 	fbc->threshold = ret;
 
-	if (INTEL_INFO(dev_priv)->gen >= 5)
+	if (INTEL_GEN(dev_priv) >= 5)
 		I915_WRITE(ILK_DPFC_CB_BASE, fbc->compressed_fb.start);
 	else if (IS_GM45(dev_priv)) {
 		I915_WRITE(DPFC_CB_BASE, fbc->compressed_fb.start);
@@ -708,10 +708,10 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 	struct intel_fbc *fbc = &dev_priv->fbc;
 	unsigned int effective_w, effective_h, max_w, max_h;
 
-	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
+	if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) {
 		max_w = 4096;
 		max_h = 4096;
-	} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) {
+	} else if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) {
 		max_w = 4096;
 		max_h = 2048;
 	} else {
@@ -812,7 +812,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
 		return false;
 	}
-	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
+	if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
 	    cache->plane.rotation != DRM_ROTATE_0) {
 		fbc->no_fbc_reason = "rotation unsupported";
 		return false;
@@ -1377,7 +1377,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	}
 
 	/* This value was pulled out of someone's hat */
-	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_GM45(dev_priv))
+	if (INTEL_GEN(dev_priv) <= 4 && !IS_GM45(dev_priv))
 		I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
 
 	/* We still don't have any sort of hardware state readout for FBC, so
-- 
2.7.4

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

Re: [PATCH 7/7] drm/i915/fbc: convert intel_fbc.c to use INTEL_GEN()

[Chris Wilson]

On Fri, Nov 11, 2016 at 02:57:41PM -0200, Paulo Zanoni wrote:
> Because it's shorter, easier to read, newer and cooler. And I don't
> think anybody else has pending FBC patches right now, so the conflicts
> should be minimal.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for FBC atomic cleanups

[Patchwork]

== Series Details ==

Series: FBC atomic cleanups
URL   : https://patchwork.freedesktop.org/series/15185/
State : warning

== Summary ==

Series 15185v1 FBC atomic cleanups
https://patchwork.freedesktop.org/api/1.0/series/15185/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                pass       -> DMESG-WARN (fi-snb-2520m)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:211  dwarn:1   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

75c991da15a4f67572515019e4fb8ed292457563 drm-intel-nightly: 2016y-11m-11d-16h-22m-04s UTC integration manifest
e56bcbe drm/i915/fbc: convert intel_fbc.c to use INTEL_GEN()
a505f19 drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc
71616cd drm/i915/fbc: use drm_atomic_get_existing_crtc_state when appropriate
eefbce6 drm/i915/fbc: inline intel_fbc_can_choose()
df9b237 drm/i915/fbc: extract intel_fbc_can_enable()
a1fff0e drm/i915/fbc: replace a loop with drm_atomic_get_existing_crtc_state()
c4c0a75 drm/i915/fbc: move the intel_fbc_can_choose() call out of the loop

== Logs ==

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

Re: [PATCH 0/7] FBC atomic cleanups

[Paulo Zanoni]

Em Sex, 2016-11-11 às 14:57 -0200, Paulo Zanoni escreveu:
> Ville pointed out two ugly defects in the FBC code, and while trying
> to fix them I spotted a few extra things. No real-world bugs fixed
> here, but IMHO the code is much easier to read now.

I merged patches 1-5 and 7, and will resend patch 6 as a separate
series. Thanks for the quick reviews!

> 
> Paulo Zanoni (7):
>   drm/i915/fbc: move the intel_fbc_can_choose() call out of the loop
>   drm/i915/fbc: replace a loop with
> drm_atomic_get_existing_crtc_state()
>   drm/i915/fbc: extract intel_fbc_can_enable()
>   drm/i915/fbc: inline intel_fbc_can_choose()
>   drm/i915/fbc: use drm_atomic_get_existing_crtc_state when
> appropriate
>   drm/i915/fbc: move from crtc_state->enable_fbc to
>     plane_state->enable_fbc
>   drm/i915/fbc: convert intel_fbc.c to use INTEL_GEN()
> 
>  drivers/gpu/drm/i915/intel_drv.h |  4 +-
>  drivers/gpu/drm/i915/intel_fbc.c | 97 ++++++++++++++++++----------
> ------------
>  2 files changed, 46 insertions(+), 55 deletions(-)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx