Re: [PATCH v3 12/12] drm/xe: Move struct xe_ggtt to xe_ggtt.c

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Oct 03, 2024 at 05:44:21PM +0200, Maarten Lankhorst wrote:
> No users left outside of xe_ggtt.c, so we can make the struct private.
> 
> This prevents us from accidentally touching it before init.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This is a good cleanup and honestly we likely should do across the Xe
driver for various public structs which no reason to be public (mostly
my fault). This enforces correct layering which is a very good thing.

With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_ggtt.c       | 37 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_ggtt_types.h | 39 +-----------------------------
>  2 files changed, 38 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 9c4baa22ebe49..0ff9d25ec0172 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -74,6 +74,43 @@ struct xe_ggtt_pt_ops {
>  	void (*ggtt_set_pte)(struct xe_ggtt *ggtt, u64 addr, u64 pte);
>  };
>  
> +/**
> + * struct xe_ggtt - Main GGTT struct
> + *
> + * In general, each tile can contains its own Global Graphics Translation Table
> + * (GGTT) instance.
> + */
> +struct xe_ggtt {
> +	/** @tile: Back pointer to tile where this GGTT belongs */
> +	struct xe_tile *tile;
> +	/** @size: Total size of this GGTT */
> +	u64 size;
> +
> +#define XE_GGTT_FLAGS_64K BIT(0)
> +	/**
> +	 * @flags: Flags for this GGTT
> +	 * Acceptable flags:
> +	 * - %XE_GGTT_FLAGS_64K - if PTE size is 64K. Otherwise, regular is 4K.
> +	 */
> +	unsigned int flags;
> +	/** @scratch: Internal object allocation used as a scratch page */
> +	struct xe_bo *scratch;
> +	/** @lock: Mutex lock to protect GGTT data */
> +	struct mutex lock;
> +	/**
> +	 *  @gsm: The iomem pointer to the actual location of the translation
> +	 * table located in the GSM for easy PTE manipulation
> +	 */
> +	u64 __iomem *gsm;
> +	/** @pt_ops: Page Table operations per platform */
> +	const struct xe_ggtt_pt_ops *pt_ops;
> +	/** @mm: The memory manager used to manage individual GGTT allocations */
> +	struct drm_mm mm;
> +	/** @access_count: counts GGTT writes */
> +	unsigned int access_count;
> +	/** @wq: Dedicated unordered work queue to process node removals */
> +	struct workqueue_struct *wq;
> +};
>  
>  static u64 xelp_ggtt_pte_encode_bo(struct xe_bo *bo, u64 bo_offset,
>  				   u16 pat_index)
> diff --git a/drivers/gpu/drm/xe/xe_ggtt_types.h b/drivers/gpu/drm/xe/xe_ggtt_types.h
> index c142ff59c4504..8b0fd528569d3 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt_types.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt_types.h
> @@ -12,47 +12,10 @@
>  
>  struct xe_bo;
>  struct xe_gt;
> +struct xe_ggtt;
>  
>  typedef u64 (*xe_ggtt_pte_encode_bo_fn)(struct xe_bo *bo, u64 bo_offset, u16 pat_index);
>  
> -/**
> - * struct xe_ggtt - Main GGTT struct
> - *
> - * In general, each tile can contains its own Global Graphics Translation Table
> - * (GGTT) instance.
> - */
> -struct xe_ggtt {
> -	/** @tile: Back pointer to tile where this GGTT belongs */
> -	struct xe_tile *tile;
> -	/** @size: Total size of this GGTT */
> -	u64 size;
> -
> -#define XE_GGTT_FLAGS_64K BIT(0)
> -	/**
> -	 * @flags: Flags for this GGTT
> -	 * Acceptable flags:
> -	 * - %XE_GGTT_FLAGS_64K - if PTE size is 64K. Otherwise, regular is 4K.
> -	 */
> -	unsigned int flags;
> -	/** @scratch: Internal object allocation used as a scratch page */
> -	struct xe_bo *scratch;
> -	/** @lock: Mutex lock to protect GGTT data */
> -	struct mutex lock;
> -	/**
> -	 *  @gsm: The iomem pointer to the actual location of the translation
> -	 * table located in the GSM for easy PTE manipulation
> -	 */
> -	u64 __iomem *gsm;
> -	/** @pt_ops: Page Table operations per platform */
> -	const struct xe_ggtt_pt_ops *pt_ops;
> -	/** @mm: The memory manager used to manage individual GGTT allocations */
> -	struct drm_mm mm;
> -	/** @access_count: counts GGTT writes */
> -	unsigned int access_count;
> -	/** @wq: Dedicated unordered work queue to process node removals */
> -	struct workqueue_struct *wq;
> -};
> -
>  /**
>   * struct xe_ggtt_node - A node in GGTT.
>   *
> -- 
> 2.45.2
>