Re: [PATCH v2 1/6] drm/xe/ggtt: Add xe_ggtt_insert_node_at

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jul 15, 2026 at 01:05:52PM +0200, Maarten Lankhorst wrote:
> Create a new function xe_ggtt_insert_node_at() which will be used
> for reserving the part of GGTT where the initial framebuffer was
> allocated.
> 
> This will allow us to either take over the initial mapping, or
> reserve it to have the newly allocated GGTT mapping not overwriting
> the initial mapping, which would cause flickering.
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/xe/xe_ggtt.c | 35 +++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/xe/xe_ggtt.h |  2 ++
>  2 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 8ec23862477fc..c9f84db3bfecd 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -636,14 +636,17 @@ static struct xe_ggtt_node *ggtt_node_init(struct xe_ggtt *ggtt)
>  }
>  
>  /**
> - * xe_ggtt_insert_node - Insert a &xe_ggtt_node into the GGTT
> + * xe_ggtt_insert_node_at - Insert a &xe_ggtt_node into the GGTT
>   * @ggtt: the &xe_ggtt into which the node should be inserted.
>   * @size: size of the node
>   * @align: alignment constrain of the node
> + * @start: Starting offset of range to insert node
> + * @end: Last offset for node insertion
>   *
>   * Return: &xe_ggtt_node on success or a ERR_PTR on failure.
>   */
> -struct xe_ggtt_node *xe_ggtt_insert_node(struct xe_ggtt *ggtt, u32 size, u32 align)
> +struct xe_ggtt_node *xe_ggtt_insert_node_at(struct xe_ggtt *ggtt, u32 size,
> +					    u32 align, u64 start, u64 end)
>  {
>  	struct xe_ggtt_node *node;
>  	int ret;
> @@ -653,8 +656,19 @@ struct xe_ggtt_node *xe_ggtt_insert_node(struct xe_ggtt *ggtt, u32 size, u32 ali
>  		return node;
>  
>  	guard(mutex)(&ggtt->lock);
> -	ret = xe_ggtt_insert_node_locked(node, size, align,
> -					 DRM_MM_INSERT_HIGH);
> +	if (start >= ggtt->start)
> +		start -= ggtt->start;
> +	else
> +		start = 0;
> +
> +	/* Should never happen, but since we handle start, fail graciously for end */

I remember seeing this weird comment in the existing code as well.
I confused me then and still does. Which should never happen, the
'if' or the 'else'? And both cases seem entirely possible to me. 
The default end==~0ull is certainly going to hit the 'if', and the
initial fb can certainly be fully below ggtt->start so 'else' seems
possible as well.

> +	if (end >= ggtt->start)
> +		end -= ggtt->start;
> +	else
> +		end = 0;
> +
> +	ret = drm_mm_insert_node_in_range(&ggtt->mm, &node->base, size, align,
> +					  0, start, end, DRM_MM_INSERT_HIGH);

That is going to fail if the size matches the original range, and
then we reduce the range due to ggtt_start/end.

Can I presume the drm_mm code can deal with the start/end > ggtt_end case?

Hmm, I now see that you handle those cases in the caller in the
later patch. But that just makes just this whole function feel
rather strange; Why do we even allow start/end that aren't within
the valid range for the mm if the caller has to handle that anyway?
OTOH I guess the 0/~0ull stuff does need this here :/

>  	if (ret) {
>  		ggtt_node_fini(node);
>  		return ERR_PTR(ret);
> @@ -663,6 +677,19 @@ struct xe_ggtt_node *xe_ggtt_insert_node(struct xe_ggtt *ggtt, u32 size, u32 ali
>  	return node;
>  }
>  
> +/**
> + * xe_ggtt_insert_node - Insert a &xe_ggtt_node into the GGTT
> + * @ggtt: the &xe_ggtt into which the node should be inserted.
> + * @size: size of the node
> + * @align: alignment constrain of the node
> + *
> + * Return: &xe_ggtt_node on success or a ERR_PTR on failure.
> + */
> +struct xe_ggtt_node *xe_ggtt_insert_node(struct xe_ggtt *ggtt, u32 size, u32 align)
> +{
> +	return xe_ggtt_insert_node_at(ggtt, size, align, 0, ~0ULL);
> +}
> +
>  /**
>   * xe_ggtt_node_pt_size() - Get the size of page table entries needed to map a GGTT node.
>   * @node: the &xe_ggtt_node
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index c864cc975a695..69974da523f74 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -22,6 +22,8 @@ void xe_ggtt_shift_nodes(struct xe_ggtt *ggtt, u64 new_base);
>  u64 xe_ggtt_start(struct xe_ggtt *ggtt);
>  u64 xe_ggtt_size(struct xe_ggtt *ggtt);
>  
> +struct xe_ggtt_node *
> +xe_ggtt_insert_node_at(struct xe_ggtt *ggtt, u32 size, u32 align, u64 start, u64 end);
>  struct xe_ggtt_node *
>  xe_ggtt_insert_node(struct xe_ggtt *ggtt, u32 size, u32 align);
>  struct xe_ggtt_node *
> -- 
> 2.53.0

-- 
Ville Syrjälä
Intel