Re: [Intel-xe] [PATCH v2 5/9] drivers/xe: Move Xe stolen memory handling away form fbc code

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

00000000000000

On 2023-05-30 12:47, Jouni Högander wrote:
> More correct place for Xe stolen memory handling details is in
> xe_ttm_stolen_mgr. Move it there and add necessary defines into
> i915_gem_stolen.h to satisfy fbc code.
>
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c      | 62 -----------------
>  .../xe/compat-i915-headers/i915_gem_stolen.h  | 66 +++++++++++++++++++
>  drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c        |  8 +++
>  drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h        |  1 +
>  4 files changed, 75 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 90c48ccce1a7..19698ab117a1 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -61,68 +61,6 @@
>  #define i915_gem_stolen_node_offset(node) ((node)->start)
>  #define i915_gem_stolen_node_size(node) ((node)->size)
>  
> -#else
> -
> -#include "xe_ttm_stolen_mgr.h"
> -#include "xe_res_cursor.h"
> -
> -static inline int i915_gem_stolen_insert_node_in_range(struct xe_device *xe,
> -						       struct i915_stolen_fb *fb,
> -						       u32 size, u32 align,
> -						       u32 start, u32 end)
> -{
> -	struct xe_bo *bo;
> -	int err;
> -	u32 flags = XE_BO_CREATE_PINNED_BIT | XE_BO_CREATE_STOLEN_BIT;
> -
> -	bo = xe_bo_create_locked_range(xe, to_gt(xe), NULL, size, start, end,
> -					ttm_bo_type_kernel, flags);
> -	if (IS_ERR(bo)) {
> -		err = PTR_ERR(bo);
> -		bo = NULL;
> -		return err;
> -	}
> -	err = xe_bo_pin(bo);
> -	xe_bo_unlock_vm_held(bo);
> -
> -	if (err) {
> -		xe_bo_put(bo);
> -		bo = NULL;
> -	}
> -
> -	fb->bo = bo;
> -
> -	return err;
> -}
> -
> -static int i915_gem_stolen_insert_node(struct xe_device *xe,
> -				       struct i915_stolen_fb *fb, u32 size,
> -				       u32 align)
> -{
> -	/* Not used on xe */
> -	BUG_ON(1);
> -	return -ENODEV;
> -}
> -
> -static void i915_gem_stolen_remove_node(struct xe_device *xe,
> -					struct i915_stolen_fb *fb)
> -{
> -	xe_bo_unpin_map_no_vm(fb->bo);
> -	fb->bo = NULL;
> -}
> -
> -#define i915_gem_stolen_initialized(xe) (!!ttm_manager_type(&(xe)->ttm, XE_PL_STOLEN))
> -#define i915_gem_stolen_node_allocated(fb) (!!(fb.bo))
> -
> -static inline u32 i915_gem_stolen_node_offset(struct i915_stolen_fb *fb)
> -{
> -	struct xe_res_cursor res;
> -
> -	xe_res_first(fb->bo->ttm.resource, 0, 4096, &res);
> -	return res.start;
> -}
> -
> -#define i915_gem_stolen_node_size(fb) ((u64)((fb)->bo->ttm.base.size))
>  #endif
>  
>  #define for_each_fbc_id(__dev_priv, __fbc_id) \
> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_gem_stolen.h b/drivers/gpu/drm/xe/compat-i915-headers/i915_gem_stolen.h
> index b6eeeba1087b..a9020d408407 100644
> --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_gem_stolen.h
> +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_gem_stolen.h
> @@ -1,10 +1,76 @@
>  #ifndef _I915_GEM_OBJECT_H_
>  #define _I915_GEM_OBJECT_H_
>  
> +#include "xe_ttm_stolen_mgr.h"
> +#include "xe_res_cursor.h"
> +
>  struct xe_bo;
>  
>  struct i915_stolen_fb {
>  	struct xe_bo *bo;
>  };
>  
> +static inline int i915_gem_stolen_insert_node_in_range(struct xe_device *xe,
> +						       struct i915_stolen_fb *fb,
> +						       u32 size, u32 align,
> +						       u32 start, u32 end)
> +{
> +	struct xe_bo *bo;
> +	int err;
> +	u32 flags = XE_BO_CREATE_PINNED_BIT | XE_BO_CREATE_STOLEN_BIT;
> +
> +	bo = xe_bo_create_locked_range(xe, to_gt(xe), NULL, size, start, end,
> +					ttm_bo_type_kernel, flags);
> +	if (IS_ERR(bo)) {
> +		err = PTR_ERR(bo);
> +		bo = NULL;
> +		return err;
> +	}
> +	err = xe_bo_pin(bo);
> +	xe_bo_unlock_vm_held(bo);
> +
> +	if (err) {
> +		xe_bo_put(fb->bo);
> +		bo = NULL;
> +	}
> +
> +	fb->bo = bo;
> +
> +	return err;
> +}
> +
> +static inline int i915_gem_stolen_insert_node(struct xe_device *xe,
> +					      struct i915_stolen_fb *fb,
> +					      u32 size, u32 align)
> +{
> +	/* Not used on xe */
> +	BUG_ON(1);
> +	return -ENODEV;
> +}
> +
> +static inline void i915_gem_stolen_remove_node(struct xe_device *xe,
> +					       struct i915_stolen_fb *fb)
> +{
> +	xe_bo_unpin_map_no_vm(fb->bo);
> +	fb->bo = NULL;
> +}
> +
> +#define i915_gem_stolen_initialized(xe) (!!ttm_manager_type(&(xe)->ttm, XE_PL_STOLEN))
> +#define i915_gem_stolen_node_allocated(fb) (!!(fb.bo))
> +
> +static inline u32 i915_gem_stolen_node_offset(struct i915_stolen_fb *fb)
> +{
> +	struct xe_res_cursor res;
> +
> +	xe_res_first(fb->bo->ttm.resource, 0, 4096, &res);
> +	return res.start;
> +}
> +
> +#define i915_gem_stolen_area_address(xe) (xe_ttm_stolen_gpu_offset(xe))
> +#define i915_gem_stolen_area_size(xe) (xe_ttm_stolen_size(xe))

area_size is used for a workaround that's gen9 specific, no point in adding it for xe. Could be (!WARN_ON(1)) instead.

Similarly, the check for area_address is pointless in Xe, as it's used for programming gen4-.

That's why it was ifdeffed out. If it's not expected to be used, it's better not to implement.

~Maarten