> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Ville
> Syrjala
> Sent: Wednesday, November 24, 2021 1:37 PM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH 05/20] drm/i915/fbc: Nuke more FBC state
>
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> There isn't a good reason why we'd have to cache all this plane state stuff in the
> FBC state. Instead we can just pre-calculate what FBC will really need.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Reviewed-by: Mika Kahola <mika.kahola@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_fbc.c | 132 +++++++++++------------
> drivers/gpu/drm/i915/i915_drv.h | 20 +---
> 2 files changed, 67 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 6368dddf977c..1e8b75cdfad8 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -73,25 +73,25 @@ static unsigned int intel_fbc_plane_stride(const struct
> intel_plane_state *plane }
>
> /* plane stride based cfb stride in bytes, assuming 1:1 compression limit */ -
> static unsigned int _intel_fbc_cfb_stride(const struct intel_fbc_state_cache
> *cache)
> +static unsigned int _intel_fbc_cfb_stride(const struct
> +intel_plane_state *plane_state)
> {
> unsigned int cpp = 4; /* FBC always 4 bytes per pixel */
>
> - return cache->fb.stride * cpp;
> + return intel_fbc_plane_stride(plane_state) * cpp;
> }
>
> /* minimum acceptable cfb stride in bytes, assuming 1:1 compression limit */ -
> static unsigned int skl_fbc_min_cfb_stride(struct intel_fbc *fbc,
> - const struct intel_fbc_state_cache
> *cache)
> +static unsigned int skl_fbc_min_cfb_stride(const struct
> +intel_plane_state *plane_state)
> {
> - struct drm_i915_private *i915 = fbc->i915;
> + struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> unsigned int limit = 4; /* 1:4 compression limit is the worst case */
> unsigned int cpp = 4; /* FBC always 4 bytes per pixel */
> + unsigned int width = drm_rect_width(&plane_state->uapi.src) >> 16;
> unsigned int height = 4; /* FBC segment is 4 lines */
> unsigned int stride;
>
> /* minimum segment stride we can use */
> - stride = cache->plane.src_w * cpp * height / limit;
> + stride = width * cpp * height / limit;
>
> /*
> * Wa_16011863758: icl+
> @@ -111,11 +111,10 @@ static unsigned int skl_fbc_min_cfb_stride(struct
> intel_fbc *fbc, }
>
> /* properly aligned cfb stride in bytes, assuming 1:1 compression limit */ -static
> unsigned int intel_fbc_cfb_stride(struct intel_fbc *fbc,
> - const struct intel_fbc_state_cache
> *cache)
> +static unsigned int intel_fbc_cfb_stride(const struct intel_plane_state
> +*plane_state)
> {
> - struct drm_i915_private *i915 = fbc->i915;
> - unsigned int stride = _intel_fbc_cfb_stride(cache);
> + struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> + unsigned int stride = _intel_fbc_cfb_stride(plane_state);
>
> /*
> * At least some of the platforms require each 4 line segment to @@ -
> 123,30 +122,30 @@ static unsigned int intel_fbc_cfb_stride(struct intel_fbc
> *fbc,
> * that regardless of the compression limit we choose later.
> */
> if (DISPLAY_VER(i915) >= 9)
> - return max(ALIGN(stride, 512), skl_fbc_min_cfb_stride(fbc,
> cache));
> + return max(ALIGN(stride, 512),
> skl_fbc_min_cfb_stride(plane_state));
> else
> return stride;
> }
>
> -static unsigned int intel_fbc_cfb_size(struct intel_fbc *fbc,
> - const struct intel_fbc_state_cache *cache)
> +static unsigned int intel_fbc_cfb_size(const struct intel_plane_state
> +*plane_state)
> {
> - struct drm_i915_private *i915 = fbc->i915;
> - int lines = cache->plane.src_h;
> + struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> + int lines = drm_rect_height(&plane_state->uapi.src) >> 16;
>
> if (DISPLAY_VER(i915) == 7)
> lines = min(lines, 2048);
> else if (DISPLAY_VER(i915) >= 8)
> lines = min(lines, 2560);
>
> - return lines * intel_fbc_cfb_stride(fbc, cache);
> + return lines * intel_fbc_cfb_stride(plane_state);
> }
>
> -static u16 intel_fbc_override_cfb_stride(struct intel_fbc *fbc,
> - const struct intel_fbc_state_cache
> *cache)
> +static u16 intel_fbc_override_cfb_stride(const struct intel_plane_state
> +*plane_state)
> {
> - unsigned int stride = _intel_fbc_cfb_stride(cache);
> - unsigned int stride_aligned = intel_fbc_cfb_stride(fbc, cache);
> + struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> + unsigned int stride_aligned = intel_fbc_cfb_stride(plane_state);
> + unsigned int stride = _intel_fbc_cfb_stride(plane_state);
> + const struct drm_framebuffer *fb = plane_state->hw.fb;
>
> /*
> * Override stride in 64 byte units per 4 line segment.
> @@ -156,8 +155,7 @@ static u16 intel_fbc_override_cfb_stride(struct intel_fbc
> *fbc,
> * we always need to use the override there.
> */
> if (stride != stride_aligned ||
> - (DISPLAY_VER(fbc->i915) == 9 &&
> - cache->fb.modifier == DRM_FORMAT_MOD_LINEAR))
> + (DISPLAY_VER(i915) == 9 && fb->modifier ==
> DRM_FORMAT_MOD_LINEAR))
> return stride_aligned * 4 / 64;
>
> return 0;
> @@ -925,31 +923,22 @@ static bool tiling_is_valid(const struct
> intel_plane_state *plane_state)
> }
> }
>
> -static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
> - const struct intel_crtc_state
> *crtc_state,
> - const struct intel_plane_state
> *plane_state)
> +static void intel_fbc_update_state_cache(struct intel_atomic_state *state,
> + struct intel_crtc *crtc,
> + struct intel_plane *plane)
> {
> - struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> - struct intel_fbc *fbc = &i915->fbc;
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> + const struct intel_crtc_state *crtc_state =
> + intel_atomic_get_new_crtc_state(state, crtc);
> + const struct intel_plane_state *plane_state =
> + intel_atomic_get_new_plane_state(state, plane);
> + struct intel_fbc *fbc = plane->fbc;
> struct intel_fbc_state_cache *cache = &fbc->state_cache;
> - struct drm_framebuffer *fb = plane_state->hw.fb;
>
> cache->no_fbc_reason = plane_state->no_fbc_reason;
> if (cache->no_fbc_reason)
> return;
>
> - /*
> - * Src coordinates are already rotated by 270 degrees for
> - * the 90/270 degree plane rotation cases (to match the
> - * GTT mapping), hence no need to account for rotation here.
> - */
> - cache->plane.src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
> - cache->plane.src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
> -
> - cache->fb.format = fb->format;
> - cache->fb.modifier = fb->modifier;
> - cache->fb.stride = intel_fbc_plane_stride(plane_state);
> -
> /* FBC1 compression interval: arbitrary choice of 1 second */
> cache->interval = drm_mode_vrefresh(&crtc_state-
> >hw.adjusted_mode);
>
> @@ -963,12 +952,15 @@ static void intel_fbc_update_state_cache(struct
> intel_crtc *crtc,
> cache->fence_id = plane_state->ggtt_vma->fence->id;
> else
> cache->fence_id = -1;
> +
> + cache->cfb_stride = intel_fbc_cfb_stride(plane_state);
> + cache->cfb_size = intel_fbc_cfb_size(plane_state);
> + cache->override_cfb_stride =
> +intel_fbc_override_cfb_stride(plane_state);
> }
>
> static bool intel_fbc_cfb_size_changed(struct intel_fbc *fbc) {
> - return intel_fbc_cfb_size(fbc, &fbc->state_cache) >
> - fbc->compressed_fb.size * fbc->limit;
> + return fbc->state_cache.cfb_size > fbc->compressed_fb.size *
> +fbc->limit;
> }
>
> static bool intel_fbc_can_enable(struct intel_fbc *fbc) @@ -1178,45 +1170,53
> @@ static void intel_fbc_get_reg_params(struct intel_fbc *fbc,
> params->interval = cache->interval;
> params->crtc.i9xx_plane = to_intel_plane(crtc->base.primary)-
> >i9xx_plane;
>
> - params->fb.format = cache->fb.format;
> - params->fb.modifier = cache->fb.modifier;
> - params->fb.stride = cache->fb.stride;
> -
> - params->cfb_stride = intel_fbc_cfb_stride(fbc, cache);
> - params->cfb_size = intel_fbc_cfb_size(fbc, cache);
> - params->override_cfb_stride = intel_fbc_override_cfb_stride(fbc,
> cache);
> + params->cfb_stride = cache->cfb_stride;
> + params->cfb_size = cache->cfb_size;
> + params->override_cfb_stride = cache->override_cfb_stride;
> }
>
> -static bool intel_fbc_can_flip_nuke(const struct intel_crtc_state *crtc_state)
> +static bool intel_fbc_can_flip_nuke(struct intel_atomic_state *state,
> + struct intel_crtc *crtc,
> + struct intel_plane *plane)
> {
> - struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> - struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> - struct intel_fbc *fbc = &i915->fbc;
> + struct intel_fbc *fbc = plane->fbc;
> + const struct intel_crtc_state *new_crtc_state =
> + intel_atomic_get_new_crtc_state(state, crtc);
> + const struct intel_plane_state *old_plane_state =
> + intel_atomic_get_old_plane_state(state, plane);
> + const struct intel_plane_state *new_plane_state =
> + intel_atomic_get_new_plane_state(state, plane);
> + const struct drm_framebuffer *old_fb = old_plane_state->hw.fb;
> + const struct drm_framebuffer *new_fb = new_plane_state->hw.fb;
> const struct intel_fbc_state_cache *cache = &fbc->state_cache;
> const struct intel_fbc_reg_params *params = &fbc->params;
>
> - if (drm_atomic_crtc_needs_modeset(&crtc_state->uapi))
> + if (drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi))
> return false;
>
> if (!intel_fbc_can_activate(crtc))
> return false;
>
> - if (params->fb.format != cache->fb.format)
> + if (!old_fb || !new_fb)
> return false;
>
> - if (params->fb.modifier != cache->fb.modifier)
> + if (old_fb->format->format != new_fb->format->format)
> return false;
>
> - if (params->fb.stride != cache->fb.stride)
> + if (old_fb->modifier != new_fb->modifier)
> return false;
>
> - if (params->cfb_stride != intel_fbc_cfb_stride(fbc, cache))
> + if (intel_fbc_plane_stride(old_plane_state) !=
> + intel_fbc_plane_stride(new_plane_state))
> return false;
>
> - if (params->cfb_size != intel_fbc_cfb_size(fbc, cache))
> + if (params->cfb_stride != cache->cfb_stride)
> return false;
>
> - if (params->override_cfb_stride != intel_fbc_override_cfb_stride(fbc,
> cache))
> + if (params->cfb_size != cache->cfb_size)
> + return false;
> +
> + if (params->override_cfb_stride != cache->override_cfb_stride)
> return false;
>
> return true;
> @@ -1226,8 +1226,6 @@ bool intel_fbc_pre_update(struct intel_atomic_state
> *state,
> struct intel_crtc *crtc)
> {
> struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> - const struct intel_crtc_state *crtc_state =
> - intel_atomic_get_new_crtc_state(state, crtc);
> const struct intel_plane_state *plane_state =
> intel_atomic_get_new_plane_state(state, plane);
> struct drm_i915_private *i915 = to_i915(crtc->base.dev); @@ -1243,10
> +1241,10 @@ bool intel_fbc_pre_update(struct intel_atomic_state *state,
> if (fbc->crtc != crtc)
> goto unlock;
>
> - intel_fbc_update_state_cache(crtc, crtc_state, plane_state);
> + intel_fbc_update_state_cache(state, crtc, plane);
> fbc->flip_pending = true;
>
> - if (!intel_fbc_can_flip_nuke(crtc_state)) {
> + if (!intel_fbc_can_flip_nuke(state, crtc, plane)) {
> intel_fbc_deactivate(fbc, reason);
>
> /*
> @@ -1425,8 +1423,6 @@ static void intel_fbc_enable(struct intel_atomic_state
> *state, {
> struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> - const struct intel_crtc_state *crtc_state =
> - intel_atomic_get_new_crtc_state(state, crtc);
> const struct intel_plane_state *plane_state =
> intel_atomic_get_new_plane_state(state, plane);
> struct intel_fbc *fbc = plane->fbc;
> @@ -1455,12 +1451,12 @@ static void intel_fbc_enable(struct
> intel_atomic_state *state,
>
> drm_WARN_ON(&i915->drm, fbc->active);
>
> - intel_fbc_update_state_cache(crtc, crtc_state, plane_state);
> + intel_fbc_update_state_cache(state, crtc, plane);
>
> if (cache->no_fbc_reason)
> goto out;
>
> - if (intel_fbc_alloc_cfb(fbc, intel_fbc_cfb_size(fbc, cache), min_limit)) {
> + if (intel_fbc_alloc_cfb(fbc, intel_fbc_cfb_size(plane_state),
> +min_limit)) {
> fbc->no_fbc_reason = "not enough stolen memory";
> goto out;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d79aa827d937..66fa46d41fa5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -436,18 +436,10 @@ struct intel_fbc {
> struct intel_fbc_state_cache {
> const char *no_fbc_reason;
>
> - struct {
> - int src_w;
> - int src_h;
> - } plane;
> -
> - struct {
> - const struct drm_format_info *format;
> - unsigned int stride;
> - u64 modifier;
> - } fb;
> -
> + unsigned int cfb_stride;
> + unsigned int cfb_size;
> unsigned int fence_y_offset;
> + u16 override_cfb_stride;
> u16 interval;
> s8 fence_id;
> } state_cache;
> @@ -464,12 +456,6 @@ struct intel_fbc {
> enum i9xx_plane_id i9xx_plane;
> } crtc;
>
> - struct {
> - const struct drm_format_info *format;
> - unsigned int stride;
> - u64 modifier;
> - } fb;
> -
> unsigned int cfb_stride;
> unsigned int cfb_size;
> unsigned int fence_y_offset;
> --
> 2.32.0
