Re: [PATCH v2 1/3] drm/i915: Move stolen memory handling into i915_gem_stolen

[Nirmoy Das <nirmoy.das@xxxxxxxxxxxxxxx>]

Hi Jouni,

On 6/14/2023 7:17 AM, Jouni Högander wrote:
> We are preparing for Xe. Xe stolen memory handling differs from i915 so we
> want to move stolen memory handling details into i915_gem_stolen.
>
> Also add a common type for fbc compressed fb and use it from fbc code
> instead of underlying type directly. This way we can have common type
> i915_stolen_fb for both i915 and Xe.
>
> v2: Fix couple of checkpatch warnings
>
> Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/display/intel_fbc.c   | 46 +++++++++++-----------
>   drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 36 +++++++++++++++++
>   drivers/gpu/drm/i915/gem/i915_gem_stolen.h | 13 ++++++
>   3 files changed, 73 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 7f8b2d7713c7..a18e84efe911 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -94,8 +94,7 @@ struct intel_fbc {
>   	struct mutex lock;
>   	unsigned int busy_bits;
>   
> -	struct drm_mm_node compressed_fb;
> -	struct drm_mm_node compressed_llb;
> +	struct i915_stolen_fb compressed_fb, compressed_llb;
>   
>   	enum intel_fbc_id id;
>   
> @@ -332,15 +331,16 @@ static void i8xx_fbc_program_cfb(struct intel_fbc *fbc)
>   {
>   	struct drm_i915_private *i915 = fbc->i915;
>   
> -	GEM_BUG_ON(range_overflows_end_t(u64, i915->dsm.stolen.start,
> -					 fbc->compressed_fb.start, U32_MAX));
> -	GEM_BUG_ON(range_overflows_end_t(u64, i915->dsm.stolen.start,
> -					 fbc->compressed_llb.start, U32_MAX));
> -
> +	GEM_BUG_ON(range_overflows_end_t(u64, i915_gem_stolen_area_address(i915),
> +					 i915_gem_stolen_node_offset(&fbc->compressed_fb),
> +					 U32_MAX));
> +	GEM_BUG_ON(range_overflows_end_t(u64, i915_gem_stolen_area_address(i915),
> +					 i915_gem_stolen_node_offset(&fbc->compressed_llb),
> +					 U32_MAX));
>   	intel_de_write(i915, FBC_CFB_BASE,
> -		       i915->dsm.stolen.start + fbc->compressed_fb.start);
> +		       i915_gem_stolen_node_address(i915, &fbc->compressed_fb));
>   	intel_de_write(i915, FBC_LL_BASE,
> -		       i915->dsm.stolen.start + fbc->compressed_llb.start);
> +		       i915_gem_stolen_node_address(i915, &fbc->compressed_llb));
>   }
>   
>   static const struct intel_fbc_funcs i8xx_fbc_funcs = {
> @@ -447,7 +447,8 @@ static void g4x_fbc_program_cfb(struct intel_fbc *fbc)
>   {
>   	struct drm_i915_private *i915 = fbc->i915;
>   
> -	intel_de_write(i915, DPFC_CB_BASE, fbc->compressed_fb.start);
> +	intel_de_write(i915, DPFC_CB_BASE,
> +		       i915_gem_stolen_node_offset(&fbc->compressed_fb));
>   }
>   
>   static const struct intel_fbc_funcs g4x_fbc_funcs = {
> @@ -498,7 +499,8 @@ static void ilk_fbc_program_cfb(struct intel_fbc *fbc)
>   {
>   	struct drm_i915_private *i915 = fbc->i915;
>   
> -	intel_de_write(i915, ILK_DPFC_CB_BASE(fbc->id), fbc->compressed_fb.start);
> +	intel_de_write(i915, ILK_DPFC_CB_BASE(fbc->id),
> +		       i915_gem_stolen_node_offset(&fbc->compressed_fb));
>   }
>   
>   static const struct intel_fbc_funcs ilk_fbc_funcs = {
> @@ -713,7 +715,7 @@ static u64 intel_fbc_stolen_end(struct drm_i915_private *i915)
>   	 * underruns, even if that range is not reserved by the BIOS. */
>   	if (IS_BROADWELL(i915) ||
>   	    (DISPLAY_VER(i915) == 9 && !IS_BROXTON(i915)))
> -		end = resource_size(&i915->dsm.stolen) - 8 * 1024 * 1024;
> +		end = i915_gem_stolen_area_size(i915) - 8 * 1024 * 1024;
>   	else
>   		end = U64_MAX;
>   
> @@ -770,9 +772,9 @@ static int intel_fbc_alloc_cfb(struct intel_fbc *fbc,
>   	int ret;
>   
>   	drm_WARN_ON(&i915->drm,
> -		    drm_mm_node_allocated(&fbc->compressed_fb));
> +		    i915_gem_stolen_node_allocated(&fbc->compressed_fb));
>   	drm_WARN_ON(&i915->drm,
> -		    drm_mm_node_allocated(&fbc->compressed_llb));
> +		    i915_gem_stolen_node_allocated(&fbc->compressed_llb));
>   
>   	if (DISPLAY_VER(i915) < 5 && !IS_G4X(i915)) {
>   		ret = i915_gem_stolen_insert_node(i915, &fbc->compressed_llb,
> @@ -792,15 +794,14 @@ static int intel_fbc_alloc_cfb(struct intel_fbc *fbc,
>   
>   	drm_dbg_kms(&i915->drm,
>   		    "reserved %llu bytes of contiguous stolen space for FBC, limit: %d\n",
> -		    fbc->compressed_fb.size, fbc->limit);
> -
> +		    i915_gem_stolen_node_size(&fbc->compressed_fb), fbc->limit);
>   	return 0;
>   
>   err_llb:
> -	if (drm_mm_node_allocated(&fbc->compressed_llb))
> +	if (i915_gem_stolen_node_allocated(&fbc->compressed_llb))
>   		i915_gem_stolen_remove_node(i915, &fbc->compressed_llb);
>   err:
> -	if (drm_mm_initialized(&i915->mm.stolen))
> +	if (i915_gem_stolen_initialized(i915))
>   		drm_info_once(&i915->drm, "not enough stolen space for compressed buffer (need %d more bytes), disabling. Hint: you may be able to increase stolen memory size in the BIOS to avoid this.\n", size);
>   	return -ENOSPC;
>   }
> @@ -825,9 +826,9 @@ static void __intel_fbc_cleanup_cfb(struct intel_fbc *fbc)
>   	if (WARN_ON(intel_fbc_hw_is_active(fbc)))
>   		return;
>   
> -	if (drm_mm_node_allocated(&fbc->compressed_llb))
> +	if (i915_gem_stolen_node_allocated(&fbc->compressed_llb))
>   		i915_gem_stolen_remove_node(i915, &fbc->compressed_llb);
> -	if (drm_mm_node_allocated(&fbc->compressed_fb))
> +	if (i915_gem_stolen_node_allocated(&fbc->compressed_fb))
>   		i915_gem_stolen_remove_node(i915, &fbc->compressed_fb);
>   }
>   
> @@ -1030,7 +1031,8 @@ static bool intel_fbc_is_cfb_ok(const struct intel_plane_state *plane_state)
>   	struct intel_fbc *fbc = plane->fbc;
>   
>   	return intel_fbc_min_limit(plane_state) <= fbc->limit &&
> -		intel_fbc_cfb_size(plane_state) <= fbc->compressed_fb.size * fbc->limit;
> +		intel_fbc_cfb_size(plane_state) <= fbc->limit *
> +			i915_gem_stolen_node_size(&fbc->compressed_fb);
>   }
>   
>   static bool intel_fbc_is_ok(const struct intel_plane_state *plane_state)
> @@ -1707,7 +1709,7 @@ void intel_fbc_init(struct drm_i915_private *i915)
>   {
>   	enum intel_fbc_id fbc_id;
>   
> -	if (!drm_mm_initialized(&i915->mm.stolen))
> +	if (!i915_gem_stolen_initialized(i915))
>   		DISPLAY_RUNTIME_INFO(i915)->fbc_mask = 0;
>   
>   	if (need_fbc_vtd_wa(i915))
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index 3b094d36a0b0..78bac1e611dd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -974,3 +974,39 @@ bool i915_gem_object_is_stolen(const struct drm_i915_gem_object *obj)
>   {
>   	return obj->ops == &i915_gem_object_stolen_ops;
>   }
> +
> +bool i915_gem_stolen_initialized(const struct drm_i915_private *i915)
> +{
> +	return drm_mm_initialized(&i915->mm.stolen);
> +}
> +
> +u64 i915_gem_stolen_area_address(const struct drm_i915_private *i915)
> +{
> +	return i915->dsm.stolen.start;
> +}
> +
> +u64 i915_gem_stolen_area_size(const struct drm_i915_private *i915)
> +{
> +	return resource_size(&i915->dsm.stolen);
> +}
> +
> +u64 i915_gem_stolen_node_address(const struct drm_i915_private *i915,
> +				 const struct drm_mm_node *node)
> +{
> +	return i915->dsm.stolen.start + i915_gem_stolen_node_offset(node);
> +}
> +
> +bool i915_gem_stolen_node_allocated(const struct drm_mm_node *node)
> +{
> +	return drm_mm_node_allocated(node);
> +}
> +
> +u64 i915_gem_stolen_node_offset(const struct drm_mm_node *node)
> +{
> +	return node->start;
> +}
> +
> +u64 i915_gem_stolen_node_size(const struct drm_mm_node *node)
> +{
> +	return node->size;


Above 3 functions are core drm functions/struct and not related to 
stolen so I don't think

they deserve special functions in stolen code.


Regards,

Nirmoy

> +}
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
> index d5005a39d130..258381d1c054 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
> @@ -12,6 +12,8 @@ struct drm_i915_private;
>   struct drm_mm_node;
>   struct drm_i915_gem_object;
>   
> +#define i915_stolen_fb drm_mm_node
> +
>   int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
>   				struct drm_mm_node *node, u64 size,
>   				unsigned alignment);
> @@ -36,4 +38,15 @@ bool i915_gem_object_is_stolen(const struct drm_i915_gem_object *obj);
>   
>   #define I915_GEM_STOLEN_BIAS SZ_128K
>   
> +bool i915_gem_stolen_initialized(const struct drm_i915_private *i915);
> +u64 i915_gem_stolen_area_address(const struct drm_i915_private *i915);
> +u64 i915_gem_stolen_area_size(const struct drm_i915_private *i915);
> +
> +u64 i915_gem_stolen_node_address(const struct drm_i915_private *i915,
> +				 const struct drm_mm_node *node);
> +
> +bool i915_gem_stolen_node_allocated(const struct drm_mm_node *node);
> +u64 i915_gem_stolen_node_offset(const struct drm_mm_node *node);
> +u64 i915_gem_stolen_node_size(const struct drm_mm_node *node);
> +
>   #endif /* __I915_GEM_STOLEN_H__ */