Re: [PATCH v2 1/4] drm/i915/fbc: Rework cfb stride/size calculations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> Sent: Tuesday, September 21, 2021 8:55 PM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>
> Subject: [PATCH v2 1/4] drm/i915/fbc: Rework cfb stride/size calculations
> 
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> The code to calculate the cfb stride/size is a bit of mess.
> The cfb size is getting calculated based purely on the plane stride and plane height.
> That doesn't account for extra alignment we want for the cfb stride. The gen9
> override stride OTOH is just calculated based on the plane width, and it does try to
> make things more aligned but any extra alignment added there is not considered in
> the cfb size calculations.
> So not at all convinced this is working as intended. Additionally the compression limit
> handling is split between the cfb allocation code and g4x_dpfc_ctl_limit() (for the
> 16bpp case), which is just confusing.
> 
> Let's streamline the whole thing:
> - Start with the plane stride, convert that into cfb stride (cfb is
>   always 4 bytes per pixel). All the calculations will assume 1:1
>   compression limit since that will give us the max values, and we
>   don't yet know how much stolen memory we will be able to allocate
> - Align the cfb stride to 512 bytes on modern platforms. This guarantees
>   the 4 line segment will be 512 byte aligned regardles of the final
>   compression limit we choose later. The 512 byte alignment for the
>   segment is required by at least some of the platforms, and just doing
>   it always seems like the easiest option
> - Figure out if we need to use the override stride or not. For X-tiled
>   it's never needed since the plane stride is already 512 byte aligned,
>   for Y-tiled it will be needed if the plane stride is not a multiple
>   of 512 bytes, and for linear it's apparently always needed because the
>   hardware miscalculates the cfb stride as PLANE_STRIDE*512 instead of
>   the PLANE_STRIDE*64 that it use with linear.
> - The cfb size will be calculated based on the aligned cfb stride to
>   guarantee we actually reserved enough stolen memory and the FBC hw
>   won't end up scribbling over whatever else is allocated in stolen
> - The compression limit handling we just do fully in the cfb allocation
>   code to make things less confusing
> 
> v2: Write the min cfb segment stride calculation in a more
>     explicit way to make it clear what is going on

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx>

> Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c | 163 +++++++++++++++--------
>  drivers/gpu/drm/i915/i915_drv.h          |   4 +-
>  2 files changed, 112 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> b/drivers/gpu/drm/i915/display/intel_fbc.c
> index b1c1a23c36be..f51871f39eb6 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -62,19 +62,76 @@ static void intel_fbc_get_plane_source_size(const struct
> intel_fbc_state_cache *
>  		*height = cache->plane.src_h;
>  }
> 
> -static int intel_fbc_calculate_cfb_size(struct drm_i915_private *dev_priv,
> -					const struct intel_fbc_state_cache *cache)
> +/* plane stride in pixels */
> +static unsigned int intel_fbc_plane_stride(const struct
> +intel_plane_state *plane_state)
>  {
> -	int lines;
> +	const struct drm_framebuffer *fb = plane_state->hw.fb;
> +	unsigned int stride;
> +
> +	stride = plane_state->view.color_plane[0].stride;
> +	if (!drm_rotation_90_or_270(plane_state->hw.rotation))
> +		stride /= fb->format->cpp[0];
> +
> +	return stride;
> +}
> +
> +/* 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) {
> +	unsigned int cpp = 4; /* FBC always 4 bytes per pixel */
> +
> +	return cache->fb.stride * cpp;
> +}
> +
> +/* minimum acceptable cfb stride in bytes, assuming 1:1 compression
> +limit */ static unsigned int skl_fbc_min_cfb_stride(const struct
> +intel_fbc_state_cache *cache) {
> +	unsigned int limit = 4; /* 1:4 compression limit is the worst case */
> +	unsigned int cpp = 4; /* FBC always 4 bytes per pixel */
> +	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;
> +
> +	/*
> +	 * At least some of the platforms require each 4 line segment to
> +	 * be 512 byte aligned. Just do it always for simplicity.
> +	 */
> +	stride = ALIGN(stride, 512);
> +
> +	/* convert back to single line equivalent with 1:1 compression limit */
> +	return stride * limit / height;
> +}
> +
> +/* properly aligned cfb stride in bytes, assuming 1:1 compression limit
> +*/ static unsigned int intel_fbc_cfb_stride(struct drm_i915_private *i915,
> +					 const struct intel_fbc_state_cache *cache)
> {
> +	unsigned int stride = _intel_fbc_cfb_stride(cache);
> +
> +	/*
> +	 * At least some of the platforms require each 4 line segment to
> +	 * be 512 byte aligned. Aligning each line to 512 bytes guarantees
> +	 * that regardless of the compression limit we choose later.
> +	 */
> +	if (DISPLAY_VER(i915) == 9)
> +		return max(ALIGN(stride, 512), skl_fbc_min_cfb_stride(cache));
> +	else
> +		return stride;
> +}
> +
> +static unsigned int intel_fbc_cfb_size(struct drm_i915_private *dev_priv,
> +				       const struct intel_fbc_state_cache *cache) {
> +	int lines = cache->plane.src_h;
> 
> -	intel_fbc_get_plane_source_size(cache, NULL, &lines);
>  	if (DISPLAY_VER(dev_priv) == 7)
>  		lines = min(lines, 2048);
>  	else if (DISPLAY_VER(dev_priv) >= 8)
>  		lines = min(lines, 2560);
> 
> -	/* Hardware needs the full buffer stride, not just the active area. */
> -	return lines * cache->fb.stride;
> +	return lines * intel_fbc_cfb_stride(dev_priv, cache);
>  }
> 
>  static void i8xx_fbc_deactivate(struct drm_i915_private *dev_priv) @@ -150,15
> +207,9 @@ static bool i8xx_fbc_is_active(struct drm_i915_private *dev_priv)
> 
>  static u32 g4x_dpfc_ctl_limit(struct drm_i915_private *i915)  {
> -	const struct intel_fbc_reg_params *params = &i915->fbc.params;
> -	int limit = i915->fbc.limit;
> -
> -	if (params->fb.format->cpp[0] == 2)
> -		limit <<= 1;
> -
> -	switch (limit) {
> +	switch (i915->fbc.limit) {
>  	default:
> -		MISSING_CASE(limit);
> +		MISSING_CASE(i915->fbc.limit);
>  		fallthrough;
>  	case 1:
>  		return DPFC_CTL_LIMIT_1X;
> @@ -301,7 +352,8 @@ static bool ilk_fbc_is_active(struct drm_i915_private
> *dev_priv)
> 
>  static void gen7_fbc_activate(struct drm_i915_private *dev_priv)  {
> -	struct intel_fbc_reg_params *params = &dev_priv->fbc.params;
> +	struct intel_fbc *fbc = &dev_priv->fbc;
> +	const struct intel_fbc_reg_params *params = &fbc->params;
>  	u32 dpfc_ctl;
> 
>  	/* Display WA #0529: skl, kbl, bxt. */ @@ -310,7 +362,7 @@ static void
> gen7_fbc_activate(struct drm_i915_private *dev_priv)
> 
>  		if (params->override_cfb_stride)
>  			val |= CHICKEN_FBC_STRIDE_OVERRIDE |
> -				CHICKEN_FBC_STRIDE(params-
> >override_cfb_stride);
> +				CHICKEN_FBC_STRIDE(params-
> >override_cfb_stride / fbc->limit);
> 
>  		intel_de_rmw(dev_priv, CHICKEN_MISC_4,
>  			     CHICKEN_FBC_STRIDE_OVERRIDE |
> @@ -443,7 +495,12 @@ static u64 intel_fbc_stolen_end(struct drm_i915_private
> *dev_priv)
>  	return min(end, intel_fbc_cfb_base_max(dev_priv));
>  }
> 
> -static int intel_fbc_max_limit(struct drm_i915_private *dev_priv, int fb_cpp)
> +static int intel_fbc_min_limit(int fb_cpp) {
> +	return fb_cpp == 2 ? 2 : 1;
> +}
> +
> +static int intel_fbc_max_limit(struct drm_i915_private *dev_priv)
>  {
>  	/*
>  	 * FIXME: FBC1 can have arbitrary cfb stride, @@ -457,7 +514,7 @@ static
> int intel_fbc_max_limit(struct drm_i915_private *dev_priv, int fb_cpp)
>  		return 1;
> 
>  	/* FBC2 can only do 1:1, 1:2, 1:4 */
> -	return fb_cpp == 2 ? 2 : 4;
> +	return 4;
>  }
> 
>  static int find_compression_limit(struct drm_i915_private *dev_priv, @@ -466,7
> +523,9 @@ static int find_compression_limit(struct drm_i915_private *dev_priv,  {
>  	struct intel_fbc *fbc = &dev_priv->fbc;
>  	u64 end = intel_fbc_stolen_end(dev_priv);
> -	int ret, limit = 1;
> +	int ret, limit = intel_fbc_min_limit(fb_cpp);
> +
> +	size /= limit;
> 
>  	/* Try to over-allocate to reduce reallocations and fragmentation. */
>  	ret = i915_gem_stolen_insert_node_in_range(dev_priv, &fbc-
> >compressed_fb, @@ -474,7 +533,7 @@ static int find_compression_limit(struct
> drm_i915_private *dev_priv,
>  	if (ret == 0)
>  		return limit;
> 
> -	for (; limit <= intel_fbc_max_limit(dev_priv, fb_cpp); limit <<= 1) {
> +	for (; limit <= intel_fbc_max_limit(dev_priv); limit <<= 1) {
>  		ret = i915_gem_stolen_insert_node_in_range(dev_priv, &fbc-
> >compressed_fb,
>  							   size >>= 1, 4096, 0, end);
>  		if (ret == 0)
> @@ -505,10 +564,9 @@ static int intel_fbc_alloc_cfb(struct drm_i915_private
> *dev_priv,
>  	ret = find_compression_limit(dev_priv, size, fb_cpp);
>  	if (!ret)
>  		goto err_llb;
> -	else if (ret > 1) {
> +	else if (ret > intel_fbc_min_limit(fb_cpp))
>  		drm_info_once(&dev_priv->drm,
>  			      "Reducing the compressed framebuffer size. This may
> lead to less power savings than a non-reduced-size. Try to increase stolen memory
> size if available in BIOS.\n");
> -	}
> 
>  	fbc->limit = ret;
> 
> @@ -719,11 +777,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc
> *crtc,
> 
>  	cache->fb.format = fb->format;
>  	cache->fb.modifier = fb->modifier;
> -
> -	/* FIXME is this correct? */
> -	cache->fb.stride = plane_state->view.color_plane[0].stride;
> -	if (drm_rotation_90_or_270(plane_state->hw.rotation))
> -		cache->fb.stride *= fb->format->cpp[0];
> +	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);
> @@ -746,27 +800,29 @@ static bool intel_fbc_cfb_size_changed(struct
> drm_i915_private *dev_priv)  {
>  	struct intel_fbc *fbc = &dev_priv->fbc;
> 
> -	return intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) >
> +	return intel_fbc_cfb_size(dev_priv, &fbc->state_cache) >
>  		fbc->compressed_fb.size * fbc->limit;  }
> 
> -static u16 intel_fbc_override_cfb_stride(struct drm_i915_private *dev_priv)
> +static u16 intel_fbc_override_cfb_stride(struct drm_i915_private *dev_priv,
> +					 const struct intel_fbc_state_cache *cache)
>  {
> -	struct intel_fbc *fbc = &dev_priv->fbc;
> -	struct intel_fbc_state_cache *cache = &fbc->state_cache;
> +	unsigned int stride = _intel_fbc_cfb_stride(cache);
> +	unsigned int stride_aligned = intel_fbc_cfb_stride(dev_priv, cache);
> 
> -	if ((DISPLAY_VER(dev_priv) == 9) &&
> -	    cache->fb.modifier != I915_FORMAT_MOD_X_TILED)
> -		return DIV_ROUND_UP(cache->plane.src_w, 32 * fbc->limit) * 8;
> -	else
> -		return 0;
> -}
> +	/*
> +	 * Override stride in 64 byte units per 4 line segment.
> +	 *
> +	 * Gen9 hw miscalculates cfb stride for linear as
> +	 * PLANE_STRIDE*512 instead of PLANE_STRIDE*64, so
> +	 * we always need to use the override there.
> +	 */
> +	if (stride != stride_aligned ||
> +	    (DISPLAY_VER(dev_priv) == 9 &&
> +	     cache->fb.modifier == DRM_FORMAT_MOD_LINEAR))
> +		return stride_aligned * 4 / 64;
> 
> -static bool intel_fbc_override_cfb_stride_changed(struct drm_i915_private
> *dev_priv) -{
> -	struct intel_fbc *fbc = &dev_priv->fbc;
> -
> -	return fbc->params.override_cfb_stride !=
> intel_fbc_override_cfb_stride(dev_priv);
> +	return 0;
>  }
> 
>  static bool intel_fbc_can_enable(struct drm_i915_private *dev_priv) @@ -861,7
> +917,8 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
> 
> -	if (!stride_is_valid(dev_priv, cache->fb.modifier, cache->fb.stride)) {
> +	if (!stride_is_valid(dev_priv, cache->fb.modifier,
> +			     cache->fb.stride * cache->fb.format->cpp[0])) {
>  		fbc->no_fbc_reason = "framebuffer stride not supported";
>  		return false;
>  	}
> @@ -949,9 +1006,9 @@ static void intel_fbc_get_reg_params(struct intel_crtc
> *crtc,
>  	params->fb.modifier = cache->fb.modifier;
>  	params->fb.stride = cache->fb.stride;
> 
> -	params->cfb_size = intel_fbc_calculate_cfb_size(dev_priv, cache);
> -
> -	params->override_cfb_stride = cache->override_cfb_stride;
> +	params->cfb_stride = intel_fbc_cfb_stride(dev_priv, cache);
> +	params->cfb_size = intel_fbc_cfb_size(dev_priv, cache);
> +	params->override_cfb_stride = intel_fbc_override_cfb_stride(dev_priv,
> +cache);
> 
>  	params->plane_visible = cache->plane.visible;  } @@ -982,10 +1039,13 @@
> static bool intel_fbc_can_flip_nuke(const struct intel_crtc_state *crtc_state)
>  	if (params->fb.stride != cache->fb.stride)
>  		return false;
> 
> -	if (params->cfb_size != intel_fbc_calculate_cfb_size(dev_priv, cache))
> +	if (params->cfb_stride != intel_fbc_cfb_stride(dev_priv, cache))
>  		return false;
> 
> -	if (params->override_cfb_stride != cache->override_cfb_stride)
> +	if (params->cfb_size != intel_fbc_cfb_size(dev_priv, cache))
> +		return false;
> +
> +	if (params->override_cfb_stride !=
> +intel_fbc_override_cfb_stride(dev_priv, cache))
>  		return false;
> 
>  	return true;
> @@ -1258,8 +1318,7 @@ static void intel_fbc_enable(struct intel_atomic_state
> *state,
> 
>  	if (fbc->crtc) {
>  		if (fbc->crtc != crtc ||
> -		    (!intel_fbc_cfb_size_changed(dev_priv) &&
> -		     !intel_fbc_override_cfb_stride_changed(dev_priv)))
> +		    !intel_fbc_cfb_size_changed(dev_priv))
>  			goto out;
> 
>  		__intel_fbc_disable(dev_priv);
> @@ -1274,15 +1333,13 @@ static void intel_fbc_enable(struct intel_atomic_state
> *state,
>  		goto out;
> 
>  	if (intel_fbc_alloc_cfb(dev_priv,
> -				intel_fbc_calculate_cfb_size(dev_priv, cache),
> +				intel_fbc_cfb_size(dev_priv, cache),
>  				plane_state->hw.fb->format->cpp[0])) {
>  		cache->plane.visible = false;
>  		fbc->no_fbc_reason = "not enough stolen memory";
>  		goto out;
>  	}
> 
> -	cache->override_cfb_stride = intel_fbc_override_cfb_stride(dev_priv);
> -
>  	drm_dbg_kms(&dev_priv->drm, "Enabling FBC on pipe %c\n",
>  		    pipe_name(crtc->pipe));
>  	fbc->no_fbc_reason = "FBC enabled but not active yet\n"; diff --git
> a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index
> cc355aa05dbf..804c2a470e94 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -453,7 +453,6 @@ struct intel_fbc {
>  		} fb;
> 
>  		unsigned int fence_y_offset;
> -		u16 override_cfb_stride;
>  		u16 interval;
>  		s8 fence_id;
>  		bool psr2_active;
> @@ -478,7 +477,8 @@ struct intel_fbc {
>  			u64 modifier;
>  		} fb;
> 
> -		int cfb_size;
> +		unsigned int cfb_stride;
> +		unsigned int cfb_size;
>  		unsigned int fence_y_offset;
>  		u16 override_cfb_stride;
>  		u16 interval;
> --
> 2.32.0





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux