Re: [PATCH v5 6/6] drm/xe: Move struct xe_ggtt to xe_ggtt.c

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 10/10/2025 2:07 PM, Maarten Lankhorst wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> 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>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/xe/xe_ggtt.c       | 52 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_ggtt_types.h | 51 -----------------------------
>  2 files changed, 52 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 25f931e4b73a4..6118278c0b59b 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -67,6 +67,58 @@
>   * give us the correct placement for free.
>   */
>  
> +/**
> + * struct xe_ggtt_pt_ops - GGTT Page table operations
> + * Which can vary from platform to platform.
> + */
> +struct xe_ggtt_pt_ops {
> +	/** @pte_encode_flags: Encode PTE flags for a given BO */
> +	u64 (*pte_encode_flags)(struct xe_bo *bo, u16 pat_index);
> +
> +	/** @ggtt_set_pte: Directly write into GGTT's PTE */
> +	xe_ggtt_set_pte_fn ggtt_set_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;
> +        /** @start: Start offset of GGTT */
> +	u64 start;
> +	/** @size: Total usable 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_flags(struct xe_bo *bo, u16 pat_index)
>  {
>  	u64 pte = XE_PAGE_PRESENT;
> diff --git a/drivers/gpu/drm/xe/xe_ggtt_types.h b/drivers/gpu/drm/xe/xe_ggtt_types.h
> index ef26e7ce5064e..715b7cc4cfcdb 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt_types.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt_types.h
> @@ -13,46 +13,6 @@
>  struct xe_bo;
>  struct xe_gt;
>  
> -/**
> - * 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;
> -	/** @start: Start offset of GGTT */
> -	u64 start;
> -	/** @size: Total usable 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.

hmm, leaving xe_ggtt_node here is surprising as initial work was about hiding its implementation details

shouldn't we start with finishing the cleanup of any users who are still accessing xe_ggtt_node directly ?

and then we can just make sure that ggtt_types.h in *only* included from ggtt.c 

>   *
> @@ -76,16 +36,5 @@ typedef void (*xe_ggtt_transform_cb)(struct xe_ggtt *ggtt,
>  				     struct xe_ggtt_node *node,
>  				     u64 pte_flags,
>  				     xe_ggtt_set_pte_fn set_pte, void *arg);
> -/**
> - * struct xe_ggtt_pt_ops - GGTT Page table operations
> - * Which can vary from platform to platform.
> - */
> -struct xe_ggtt_pt_ops {
> -	/** @pte_encode_flags: Encode PTE flags for a given BO */
> -	u64 (*pte_encode_flags)(struct xe_bo *bo, u16 pat_index);
> -
> -	/** @ggtt_set_pte: Directly write into GGTT's PTE */
> -	xe_ggtt_set_pte_fn ggtt_set_pte;
> -};
>  
>  #endif