[1/4] drm/i915/fbc: Use the correct plane stride
diff mbox series

Message ID 20200702153723.24327-2-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • drm/i915: FBC fixes
Related show

Commit Message

Ville Syrjälä July 2, 2020, 3:37 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Consult the actual plane stride instead of the fb stride. The two
will disagree when we remap the gtt. The plane stride is what the
hw will be fed so that's what we should look at for the FBC
retrictions/cfb allocation.

Since we no longer require a fence we are going to attempt using
FBC with remapping, and so we should look at correct stride.

With 90/270 degree rotation the plane stride is stored in units
of pixels, so we need to conver it to bytes for the purposes
of calculating the cfb stride. Not entirely sure if this matches
the hw behaviour though. Need to reverse engineer that at some
point...

We also need to reorder the pixel format check vs. stride check
to avoid triggering a spurious WARN(stride & 63) with cpp==1 and
plane stride==32.

v2: Try to deal with rotated stride and related WARN

Cc: José Roberto de Souza <jose.souza@intel.com>
Fixes: 691f7ba58d52 ("drm/i915/display/fbc: Make fences a nice-to-have for GEN9+")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Souza, Jose July 2, 2020, 11:24 p.m. UTC | #1
On Thu, 2020-07-02 at 18:37 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Consult the actual plane stride instead of the fb stride. The two
> will disagree when we remap the gtt. The plane stride is what the
> hw will be fed so that's what we should look at for the FBC
> retrictions/cfb allocation.
> 
> Since we no longer require a fence we are going to attempt using
> FBC with remapping, and so we should look at correct stride.
> 
> With 90/270 degree rotation the plane stride is stored in units
> of pixels, so we need to conver it to bytes for the purposes
> of calculating the cfb stride. Not entirely sure if this matches
> the hw behaviour though. Need to reverse engineer that at some
> point...
> 
> We also need to reorder the pixel format check vs. stride check
> to avoid triggering a spurious WARN(stride & 63) with cpp==1 and
> plane stride==32.
> 
> v2: Try to deal with rotated stride and related WARN
> 

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> Cc: José Roberto de Souza <jose.souza@intel.com>
> Fixes: 691f7ba58d52 ("drm/i915/display/fbc: Make fences a nice-to-have for GEN9+")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 69a0682ddb6a..d30c2a389294 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -695,9 +695,13 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>  	cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode;
>  
>  	cache->fb.format = fb->format;
> -	cache->fb.stride = fb->pitches[0];
>  	cache->fb.modifier = fb->modifier;
>  
> +	/* FIXME is this correct? */
> +	cache->fb.stride = plane_state->color_plane[0].stride;
> +	if (drm_rotation_90_or_270(plane_state->hw.rotation))

If it was wrong our CI would caught this in BDW or SNB for example.

> +		cache->fb.stride *= fb->format->cpp[0];
> +
>  	/* FBC1 compression interval: arbitrary choice of 1 second */
>  	cache->interval = drm_mode_vrefresh(&crtc_state->hw.adjusted_mode);
>  
> @@ -797,6 +801,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
>  
> +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> +		fbc->no_fbc_reason = "pixel format is invalid";
> +		return false;
> +	}
> +
>  	if (!rotation_is_valid(dev_priv, cache->fb.format->format,
>  			       cache->plane.rotation)) {
>  		fbc->no_fbc_reason = "rotation unsupported";
> @@ -813,11 +822,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
>  
> -	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> -		fbc->no_fbc_reason = "pixel format is invalid";
> -		return false;
> -	}
> -
>  	if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE &&
>  	    cache->fb.format->has_alpha) {
>  		fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC";
Ville Syrjälä July 3, 2020, 11:38 a.m. UTC | #2
On Thu, Jul 02, 2020 at 11:24:04PM +0000, Souza, Jose wrote:
> On Thu, 2020-07-02 at 18:37 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Consult the actual plane stride instead of the fb stride. The two
> > will disagree when we remap the gtt. The plane stride is what the
> > hw will be fed so that's what we should look at for the FBC
> > retrictions/cfb allocation.
> > 
> > Since we no longer require a fence we are going to attempt using
> > FBC with remapping, and so we should look at correct stride.
> > 
> > With 90/270 degree rotation the plane stride is stored in units
> > of pixels, so we need to conver it to bytes for the purposes
> > of calculating the cfb stride. Not entirely sure if this matches
> > the hw behaviour though. Need to reverse engineer that at some
> > point...
> > 
> > We also need to reorder the pixel format check vs. stride check
> > to avoid triggering a spurious WARN(stride & 63) with cpp==1 and
> > plane stride==32.
> > 
> > v2: Try to deal with rotated stride and related WARN
> > 
> 
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Fixes: 691f7ba58d52 ("drm/i915/display/fbc: Make fences a nice-to-have for GEN9+")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fbc.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index 69a0682ddb6a..d30c2a389294 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -695,9 +695,13 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
> >  	cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode;
> >  
> >  	cache->fb.format = fb->format;
> > -	cache->fb.stride = fb->pitches[0];
> >  	cache->fb.modifier = fb->modifier;
> >  
> > +	/* FIXME is this correct? */
> > +	cache->fb.stride = plane_state->color_plane[0].stride;
> > +	if (drm_rotation_90_or_270(plane_state->hw.rotation))
> 
> If it was wrong our CI would caught this in BDW or SNB for example.

Not really. Well, certainly not on pre-skl since they don't do 90/270
rotation. And probably not even on skl+ since any wrong assumption
about how the cfb stride is calculated by the hw would just cause it
to scribble over stolen memory we didn't allocate. So unless we've
started to use stolen more extensively in recent times such problems
would probably go unnoticed.

> 
> > +		cache->fb.stride *= fb->format->cpp[0];
> > +
> >  	/* FBC1 compression interval: arbitrary choice of 1 second */
> >  	cache->interval = drm_mode_vrefresh(&crtc_state->hw.adjusted_mode);
> >  
> > @@ -797,6 +801,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> >  		return false;
> >  	}
> >  
> > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> > +		fbc->no_fbc_reason = "pixel format is invalid";
> > +		return false;
> > +	}
> > +
> >  	if (!rotation_is_valid(dev_priv, cache->fb.format->format,
> >  			       cache->plane.rotation)) {
> >  		fbc->no_fbc_reason = "rotation unsupported";
> > @@ -813,11 +822,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> >  		return false;
> >  	}
> >  
> > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> > -		fbc->no_fbc_reason = "pixel format is invalid";
> > -		return false;
> > -	}
> > -
> >  	if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE &&
> >  	    cache->fb.format->has_alpha) {
> >  		fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC";
Souza, Jose July 6, 2020, 8:53 p.m. UTC | #3
On Fri, 2020-07-03 at 14:38 +0300, Ville Syrjälä wrote:
> On Thu, Jul 02, 2020 at 11:24:04PM +0000, Souza, Jose wrote:
> > On Thu, 2020-07-02 at 18:37 +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Consult the actual plane stride instead of the fb stride. The two
> > > will disagree when we remap the gtt. The plane stride is what the
> > > hw will be fed so that's what we should look at for the FBC
> > > retrictions/cfb allocation.
> > > 
> > > Since we no longer require a fence we are going to attempt using
> > > FBC with remapping, and so we should look at correct stride.
> > > 
> > > With 90/270 degree rotation the plane stride is stored in units
> > > of pixels, so we need to conver it to bytes for the purposes
> > > of calculating the cfb stride. Not entirely sure if this matches
> > > the hw behaviour though. Need to reverse engineer that at some
> > > point...
> > > 
> > > We also need to reorder the pixel format check vs. stride check
> > > to avoid triggering a spurious WARN(stride & 63) with cpp==1 and
> > > plane stride==32.
> > > 
> > > v2: Try to deal with rotated stride and related WARN
> > > 
> > 
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > 
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Fixes: 691f7ba58d52 ("drm/i915/display/fbc: Make fences a nice-to-have for GEN9+")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_fbc.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index 69a0682ddb6a..d30c2a389294 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -695,9 +695,13 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
> > >  	cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode;
> > >  
> > >  	cache->fb.format = fb->format;
> > > -	cache->fb.stride = fb->pitches[0];
> > >  	cache->fb.modifier = fb->modifier;
> > >  
> > > +	/* FIXME is this correct? */
> > > +	cache->fb.stride = plane_state->color_plane[0].stride;
> > > +	if (drm_rotation_90_or_270(plane_state->hw.rotation))
> > 
> > If it was wrong our CI would caught this in BDW or SNB for example.
> 
> Not really. Well, certainly not on pre-skl since they don't do 90/270
> rotation. And probably not even on skl+ since any wrong assumption

Checking rotation_is_valid() GEN5 up to GEN8 allows 90/270 rotation.


> about how the cfb stride is calculated by the hw would just cause it
> to scribble over stolen memory we didn't allocate. So unless we've
> started to use stolen more extensively in recent times such problems
> would probably go unnoticed.
> 
> > > +		cache->fb.stride *= fb->format->cpp[0];
> > > +
> > >  	/* FBC1 compression interval: arbitrary choice of 1 second */
> > >  	cache->interval = drm_mode_vrefresh(&crtc_state->hw.adjusted_mode);
> > >  
> > > @@ -797,6 +801,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> > >  		return false;
> > >  	}
> > >  
> > > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> > > +		fbc->no_fbc_reason = "pixel format is invalid";
> > > +		return false;
> > > +	}
> > > +
> > >  	if (!rotation_is_valid(dev_priv, cache->fb.format->format,
> > >  			       cache->plane.rotation)) {
> > >  		fbc->no_fbc_reason = "rotation unsupported";
> > > @@ -813,11 +822,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> > >  		return false;
> > >  	}
> > >  
> > > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> > > -		fbc->no_fbc_reason = "pixel format is invalid";
> > > -		return false;
> > > -	}
> > > -
> > >  	if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE &&
> > >  	    cache->fb.format->has_alpha) {
> > >  		fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC";
Vivi, Rodrigo July 7, 2020, 12:37 a.m. UTC | #4
On Thu, Jul 02, 2020 at 06:37:20PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Consult the actual plane stride instead of the fb stride. The two
> will disagree when we remap the gtt. The plane stride is what the
> hw will be fed so that's what we should look at for the FBC
> retrictions/cfb allocation.
> 
> Since we no longer require a fence we are going to attempt using
> FBC with remapping, and so we should look at correct stride.
> 
> With 90/270 degree rotation the plane stride is stored in units
> of pixels, so we need to conver it to bytes for the purposes
> of calculating the cfb stride. Not entirely sure if this matches
> the hw behaviour though. Need to reverse engineer that at some
> point...
> 
> We also need to reorder the pixel format check vs. stride check
> to avoid triggering a spurious WARN(stride & 63) with cpp==1 and
> plane stride==32.
> 
> v2: Try to deal with rotated stride and related WARN
> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Fixes: 691f7ba58d52 ("drm/i915/display/fbc: Make fences a nice-to-have for GEN9+")

This patch didn't apply cleanly on drm-intel-fixes. If it is critical
for 5.8 please consider to provide a backported version.

Thanks,
Rodrigo.

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 69a0682ddb6a..d30c2a389294 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -695,9 +695,13 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>  	cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode;
>  
>  	cache->fb.format = fb->format;
> -	cache->fb.stride = fb->pitches[0];
>  	cache->fb.modifier = fb->modifier;
>  
> +	/* FIXME is this correct? */
> +	cache->fb.stride = plane_state->color_plane[0].stride;
> +	if (drm_rotation_90_or_270(plane_state->hw.rotation))
> +		cache->fb.stride *= fb->format->cpp[0];
> +
>  	/* FBC1 compression interval: arbitrary choice of 1 second */
>  	cache->interval = drm_mode_vrefresh(&crtc_state->hw.adjusted_mode);
>  
> @@ -797,6 +801,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
>  
> +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> +		fbc->no_fbc_reason = "pixel format is invalid";
> +		return false;
> +	}
> +
>  	if (!rotation_is_valid(dev_priv, cache->fb.format->format,
>  			       cache->plane.rotation)) {
>  		fbc->no_fbc_reason = "rotation unsupported";
> @@ -813,11 +822,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
>  
> -	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> -		fbc->no_fbc_reason = "pixel format is invalid";
> -		return false;
> -	}
> -
>  	if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE &&
>  	    cache->fb.format->has_alpha) {
>  		fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC";
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä July 7, 2020, 12:24 p.m. UTC | #5
On Mon, Jul 06, 2020 at 08:53:06PM +0000, Souza, Jose wrote:
> On Fri, 2020-07-03 at 14:38 +0300, Ville Syrjälä wrote:
> > On Thu, Jul 02, 2020 at 11:24:04PM +0000, Souza, Jose wrote:
> > > On Thu, 2020-07-02 at 18:37 +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Consult the actual plane stride instead of the fb stride. The two
> > > > will disagree when we remap the gtt. The plane stride is what the
> > > > hw will be fed so that's what we should look at for the FBC
> > > > retrictions/cfb allocation.
> > > > 
> > > > Since we no longer require a fence we are going to attempt using
> > > > FBC with remapping, and so we should look at correct stride.
> > > > 
> > > > With 90/270 degree rotation the plane stride is stored in units
> > > > of pixels, so we need to conver it to bytes for the purposes
> > > > of calculating the cfb stride. Not entirely sure if this matches
> > > > the hw behaviour though. Need to reverse engineer that at some
> > > > point...
> > > > 
> > > > We also need to reorder the pixel format check vs. stride check
> > > > to avoid triggering a spurious WARN(stride & 63) with cpp==1 and
> > > > plane stride==32.
> > > > 
> > > > v2: Try to deal with rotated stride and related WARN
> > > > 
> > > 
> > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > > 
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Fixes: 691f7ba58d52 ("drm/i915/display/fbc: Make fences a nice-to-have for GEN9+")
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_fbc.c | 16 ++++++++++------
> > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > index 69a0682ddb6a..d30c2a389294 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > @@ -695,9 +695,13 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
> > > >  	cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode;
> > > >  
> > > >  	cache->fb.format = fb->format;
> > > > -	cache->fb.stride = fb->pitches[0];
> > > >  	cache->fb.modifier = fb->modifier;
> > > >  
> > > > +	/* FIXME is this correct? */
> > > > +	cache->fb.stride = plane_state->color_plane[0].stride;
> > > > +	if (drm_rotation_90_or_270(plane_state->hw.rotation))
> > > 
> > > If it was wrong our CI would caught this in BDW or SNB for example.
> > 
> > Not really. Well, certainly not on pre-skl since they don't do 90/270
> > rotation. And probably not even on skl+ since any wrong assumption
> 
> Checking rotation_is_valid() GEN5 up to GEN8 allows 90/270 rotation.

The display engine on pre-skl does not support 90/270 rotaiton *at all*.

> 
> 
> > about how the cfb stride is calculated by the hw would just cause it
> > to scribble over stolen memory we didn't allocate. So unless we've
> > started to use stolen more extensively in recent times such problems
> > would probably go unnoticed.
> > 
> > > > +		cache->fb.stride *= fb->format->cpp[0];
> > > > +
> > > >  	/* FBC1 compression interval: arbitrary choice of 1 second */
> > > >  	cache->interval = drm_mode_vrefresh(&crtc_state->hw.adjusted_mode);
> > > >  
> > > > @@ -797,6 +801,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> > > >  		return false;
> > > >  	}
> > > >  
> > > > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> > > > +		fbc->no_fbc_reason = "pixel format is invalid";
> > > > +		return false;
> > > > +	}
> > > > +
> > > >  	if (!rotation_is_valid(dev_priv, cache->fb.format->format,
> > > >  			       cache->plane.rotation)) {
> > > >  		fbc->no_fbc_reason = "rotation unsupported";
> > > > @@ -813,11 +822,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> > > > -		fbc->no_fbc_reason = "pixel format is invalid";
> > > > -		return false;
> > > > -	}
> > > > -
> > > >  	if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE &&
> > > >  	    cache->fb.format->has_alpha) {
> > > >  		fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC";

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index 69a0682ddb6a..d30c2a389294 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -695,9 +695,13 @@  static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
 	cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode;
 
 	cache->fb.format = fb->format;
-	cache->fb.stride = fb->pitches[0];
 	cache->fb.modifier = fb->modifier;
 
+	/* FIXME is this correct? */
+	cache->fb.stride = plane_state->color_plane[0].stride;
+	if (drm_rotation_90_or_270(plane_state->hw.rotation))
+		cache->fb.stride *= fb->format->cpp[0];
+
 	/* FBC1 compression interval: arbitrary choice of 1 second */
 	cache->interval = drm_mode_vrefresh(&crtc_state->hw.adjusted_mode);
 
@@ -797,6 +801,11 @@  static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 		return false;
 	}
 
+	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
+		fbc->no_fbc_reason = "pixel format is invalid";
+		return false;
+	}
+
 	if (!rotation_is_valid(dev_priv, cache->fb.format->format,
 			       cache->plane.rotation)) {
 		fbc->no_fbc_reason = "rotation unsupported";
@@ -813,11 +822,6 @@  static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 		return false;
 	}
 
-	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
-		fbc->no_fbc_reason = "pixel format is invalid";
-		return false;
-	}
-
 	if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE &&
 	    cache->fb.format->has_alpha) {
 		fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC";