[PATCH v3 00/12] drm/xe: Reduce flickering when inheriting BIOS fb.

[PATCH v3 00/12] drm/xe: Reduce flickering when inheriting BIOS fb.

[Maarten Lankhorst]

It turns out that xe is performing all kinds of allocations before
inheriting the BIOS fb. This is problematic as it completely overwrites
the initial FB on discrete, and on integrated causes flickering until
the original FB is restored (preserved in stolen memory).

The complete fix requires reshuffling the entire init sequence. I want to
do that, but first add some preparations. This way I don't need to resubmit
all these patches that are ready to commit each time.

Maarten Lankhorst (12):
  drm/xe/display: Handle stolen bar readout in the same way as lmem
  drm/xe: Remove double pageflip
  drm/i915/display: Use async flip when available for initial plane
    config
  drm/xe/display: Remove single wait for vblank
  drm/xe: Move suballocator init to after display init
  drm/xe: Use xe_ggtt_map_bo_unlocked for resume
  drm/xe: Add xe_ggtt_might_lock
  drm/xe: Add xe_ggtt_alloc
  drm/xe: Abstract read/write functions for GGTT PTEs
  drm/xe: Make xe_ggtt_pt_ops private
  drm/xe/display: Stop dereferencing ggtt in xe_fb_pin
  drm/xe: Move struct xe_ggtt to xe_ggtt.c

 .../drm/i915/display/skl_universal_plane.c    |  13 +-
 drivers/gpu/drm/xe/display/xe_fb_pin.c        |  36 ++---
 drivers/gpu/drm/xe/display/xe_plane_initial.c |  27 +---
 drivers/gpu/drm/xe/xe_bo.c                    |   2 +-
 drivers/gpu/drm/xe/xe_bo_evict.c              |   9 +-
 drivers/gpu/drm/xe/xe_device.c                |   6 +
 drivers/gpu/drm/xe/xe_ggtt.c                  | 141 +++++++++++++++++-
 drivers/gpu/drm/xe/xe_ggtt.h                  |  22 ++-
 drivers/gpu/drm/xe/xe_ggtt_types.h            |  50 +------
 drivers/gpu/drm/xe/xe_tile.c                  |  16 +-
 drivers/gpu/drm/xe/xe_tile.h                  |   1 +
 11 files changed, 218 insertions(+), 105 deletions(-)

-- 
2.45.2

[PATCH v3 01/12] drm/xe/display: Handle stolen bar readout in the same way as lmem

[Maarten Lankhorst]

i915 already does this, we should do the same for Xe.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/xe/display/xe_plane_initial.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
index 1b10ea499d8c8..cf139921e7817 100644
--- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
+++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
@@ -69,7 +69,7 @@ initial_plane_bo(struct xe_device *xe,
 	flags = XE_BO_FLAG_PINNED | XE_BO_FLAG_SCANOUT | XE_BO_FLAG_GGTT;
 
 	base = round_down(plane_config->base, page_size);
-	if (IS_DGFX(xe)) {
+	if (IS_DGFX(xe) || GRAPHICS_VERx100(xe) >= 1270) {
 		u64 __iomem *gte = tile0->mem.ggtt->gsm;
 		u64 pte;
 
@@ -83,7 +83,6 @@ initial_plane_bo(struct xe_device *xe,
 		}
 
 		phys_base = pte & ~(page_size - 1);
-		flags |= XE_BO_FLAG_VRAM0;
 
 		/*
 		 * We don't currently expect this to ever be placed in the
@@ -105,7 +104,6 @@ initial_plane_bo(struct xe_device *xe,
 		if (!stolen)
 			return NULL;
 		phys_base = base;
-		flags |= XE_BO_FLAG_STOLEN;
 
 		if (XE_WA(xe_root_mmio_gt(xe), 22019338487_display))
 			return NULL;
@@ -120,6 +118,11 @@ initial_plane_bo(struct xe_device *xe,
 			return NULL;
 	}
 
+	if (IS_DGFX(xe))
+		flags |= XE_BO_FLAG_VRAM0;
+	else
+		flags |= XE_BO_FLAG_STOLEN;
+
 	size = round_up(plane_config->base + plane_config->size,
 			page_size);
 	size -= base;
-- 
2.45.2

Re: [PATCH v3 01/12] drm/xe/display: Handle stolen bar readout in the same way as lmem

[Lucas De Marchi]

On Thu, Oct 03, 2024 at 05:44:10PM +0200, Maarten Lankhorst wrote:
>i915 already does this, we should do the same for Xe.
>
>Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>---
> drivers/gpu/drm/xe/display/xe_plane_initial.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>index 1b10ea499d8c8..cf139921e7817 100644
>--- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
>+++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>@@ -69,7 +69,7 @@ initial_plane_bo(struct xe_device *xe,
> 	flags = XE_BO_FLAG_PINNED | XE_BO_FLAG_SCANOUT | XE_BO_FLAG_GGTT;
>
> 	base = round_down(plane_config->base, page_size);
>-	if (IS_DGFX(xe)) {
>+	if (IS_DGFX(xe) || GRAPHICS_VERx100(xe) >= 1270) {

and with this you just dropped Wa 22019338487 from e.g. LNL.

Also, making igfx going through this path is very odd when it has checks
like phys_base >= mem.vram.usable_size

Lucas De Marchi

> 		u64 __iomem *gte = tile0->mem.ggtt->gsm;
> 		u64 pte;
>
>@@ -83,7 +83,6 @@ initial_plane_bo(struct xe_device *xe,
> 		}
>
> 		phys_base = pte & ~(page_size - 1);
>-		flags |= XE_BO_FLAG_VRAM0;
>
> 		/*
> 		 * We don't currently expect this to ever be placed in the
>@@ -105,7 +104,6 @@ initial_plane_bo(struct xe_device *xe,
> 		if (!stolen)
> 			return NULL;
> 		phys_base = base;
>-		flags |= XE_BO_FLAG_STOLEN;
>
> 		if (XE_WA(xe_root_mmio_gt(xe), 22019338487_display))
> 			return NULL;
>@@ -120,6 +118,11 @@ initial_plane_bo(struct xe_device *xe,
> 			return NULL;
> 	}
>
>+	if (IS_DGFX(xe))
>+		flags |= XE_BO_FLAG_VRAM0;
>+	else
>+		flags |= XE_BO_FLAG_STOLEN;
>+
> 	size = round_up(plane_config->base + plane_config->size,
> 			page_size);
> 	size -= base;
>-- 
>2.45.2
>

[PATCH v3 02/12] drm/xe: Remove double pageflip

[Maarten Lankhorst]

This is already handled below by fixup_initial_plane_config.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: a8153627520a ("drm/i915: Try to relocate the BIOS fb to the start of ggtt")
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/xe/display/xe_plane_initial.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
index cf139921e7817..1f5128927c07c 100644
--- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
+++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
@@ -197,8 +197,6 @@ intel_find_initial_plane_obj(struct intel_crtc *crtc,
 		to_intel_plane(crtc->base.primary);
 	struct intel_plane_state *plane_state =
 		to_intel_plane_state(plane->base.state);
-	struct intel_crtc_state *crtc_state =
-		to_intel_crtc_state(crtc->base.state);
 	struct drm_framebuffer *fb;
 	struct i915_vma *vma;
 
@@ -245,13 +243,6 @@ intel_find_initial_plane_obj(struct intel_crtc *crtc,
 
 	plane_config->vma = vma;
 
-	/*
-	 * Flip to the newly created mapping ASAP, so we can re-use the
-	 * first part of GGTT for WOPCM, prevent flickering, and prevent
-	 * the lookup of sysmem scratch pages.
-	 */
-	plane->check_plane(crtc_state, plane_state);
-	plane->async_flip(plane, crtc_state, plane_state, true);
 	return;
 
 nofb:
-- 
2.45.2

Re: [PATCH v3 02/12] drm/xe: Remove double pageflip

[Govindapillai, Vinod]

On Thu, 2024-10-03 at 17:44 +0200, Maarten Lankhorst wrote:
> This is already handled below by fixup_initial_plane_config.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: a8153627520a ("drm/i915: Try to relocate the BIOS fb to the start of ggtt")
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/display/xe_plane_initial.c | 9 ---------
>  1 file changed, 9 deletions(-)

I had the same change as part of some other BMG bug analysis. But was never merged upstream!
(https://patchwork.freedesktop.org/patch/590387/?series=132649&rev=2)

Anyway, I think this change as part of this series could go instead.

Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>

> 
> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> index cf139921e7817..1f5128927c07c 100644
> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> @@ -197,8 +197,6 @@ intel_find_initial_plane_obj(struct intel_crtc *crtc,
>                 to_intel_plane(crtc->base.primary);
>         struct intel_plane_state *plane_state =
>                 to_intel_plane_state(plane->base.state);
> -       struct intel_crtc_state *crtc_state =
> -               to_intel_crtc_state(crtc->base.state);
>         struct drm_framebuffer *fb;
>         struct i915_vma *vma;
>  
> @@ -245,13 +243,6 @@ intel_find_initial_plane_obj(struct intel_crtc *crtc,
>  
>         plane_config->vma = vma;
>  
> -       /*
> -        * Flip to the newly created mapping ASAP, so we can re-use the
> -        * first part of GGTT for WOPCM, prevent flickering, and prevent
> -        * the lookup of sysmem scratch pages.
> -        */
> -       plane->check_plane(crtc_state, plane_state);
> -       plane->async_flip(plane, crtc_state, plane_state, true);
>         return;
>  
>  nofb:

[PATCH v3 03/12] drm/i915/display: Use async flip when available for initial plane config

[Maarten Lankhorst]

I'm planning to reorder readout in the Xe sequence in such a way that
interrupts will not be available, so just use an async flip.

Since the new FB points to the same pages, it will not tear. It also
has the benefit of perhaps being slightly faster.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index fdb141cfa4274..73a3624e34098 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
 		to_intel_plane_state(plane->base.state);
 	enum plane_id plane_id = plane->id;
 	enum pipe pipe = crtc->pipe;
-	u32 base;
+	u32 base, plane_ctl;
 
 	if (!plane_state->uapi.visible)
 		return false;
@@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
 	if (plane_config->base == base)
 		return false;
 
+	/* Perform an async flip to the new surface. */
+	plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id));
+	plane_ctl |= PLANE_CTL_ASYNC_FLIP;
+
+	intel_de_write(i915, PLANE_CTL(pipe, plane_id), plane_ctl);
 	intel_de_write(i915, PLANE_SURF(pipe, plane_id), base);
 
-	return true;
+	if (intel_de_wait_custom(i915, PLANE_SURFLIVE(pipe, plane_id), ~0U, base, 0, 40, NULL) < 0)
+		drm_warn(&i915->drm, "async flip timed out\n");
+
+	/* No need to vblank wait either */
+	return false;
 }
-- 
2.45.2

Re: [PATCH v3 03/12] drm/i915/display: Use async flip when available for initial plane config

[Jani Nikula]

On Thu, 03 Oct 2024, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> I'm planning to reorder readout in the Xe sequence in such a way that
> interrupts will not be available, so just use an async flip.
>
> Since the new FB points to the same pages, it will not tear. It also
> has the benefit of perhaps being slightly faster.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index fdb141cfa4274..73a3624e34098 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
>  		to_intel_plane_state(plane->base.state);
>  	enum plane_id plane_id = plane->id;
>  	enum pipe pipe = crtc->pipe;
> -	u32 base;
> +	u32 base, plane_ctl;
>  
>  	if (!plane_state->uapi.visible)
>  		return false;
> @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
>  	if (plane_config->base == base)
>  		return false;
>  
> +	/* Perform an async flip to the new surface. */
> +	plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id));
> +	plane_ctl |= PLANE_CTL_ASYNC_FLIP;
> +
> +	intel_de_write(i915, PLANE_CTL(pipe, plane_id), plane_ctl);
>  	intel_de_write(i915, PLANE_SURF(pipe, plane_id), base);
>  
> -	return true;
> +	if (intel_de_wait_custom(i915, PLANE_SURFLIVE(pipe, plane_id), ~0U, base, 0, 40, NULL) < 0)

Why not just intel_de_wait()?

BR,
Jani.

> +		drm_warn(&i915->drm, "async flip timed out\n");
> +
> +	/* No need to vblank wait either */
> +	return false;
>  }

-- 
Jani Nikula, Intel

Re: [PATCH v3 03/12] drm/i915/display: Use async flip when available for initial plane config

[Maarten Lankhorst]

Den 2024-10-03 kl. 17:58, skrev Jani Nikula:
> On Thu, 03 Oct 2024, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
>> I'm planning to reorder readout in the Xe sequence in such a way that
>> interrupts will not be available, so just use an async flip.
>>
>> Since the new FB points to the same pages, it will not tear. It also
>> has the benefit of perhaps being slightly faster.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> index fdb141cfa4274..73a3624e34098 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
>>   		to_intel_plane_state(plane->base.state);
>>   	enum plane_id plane_id = plane->id;
>>   	enum pipe pipe = crtc->pipe;
>> -	u32 base;
>> +	u32 base, plane_ctl;
>>   
>>   	if (!plane_state->uapi.visible)
>>   		return false;
>> @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
>>   	if (plane_config->base == base)
>>   		return false;
>>   
>> +	/* Perform an async flip to the new surface. */
>> +	plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id));
>> +	plane_ctl |= PLANE_CTL_ASYNC_FLIP;
>> +
>> +	intel_de_write(i915, PLANE_CTL(pipe, plane_id), plane_ctl);
>>   	intel_de_write(i915, PLANE_SURF(pipe, plane_id), base);
>>   
>> -	return true;
>> +	if (intel_de_wait_custom(i915, PLANE_SURFLIVE(pipe, plane_id), ~0U, base, 0, 40, NULL) < 0)
> 
> Why not just intel_de_wait()?
> 
> BR,
> Jani.
Yes, I overlooked that when I was browsing through intel_de.h :-)

Can respin if we agree on the direction.

Cheers,
~Maarten

Re: [PATCH v3 03/12] drm/i915/display: Use async flip when available for initial plane config

[Ville Syrjälä]

On Thu, Oct 03, 2024 at 05:44:12PM +0200, Maarten Lankhorst wrote:
> I'm planning to reorder readout in the Xe sequence in such a way that
> interrupts will not be available, so just use an async flip.
> 
> Since the new FB points to the same pages, it will not tear. It also
> has the benefit of perhaps being slightly faster.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index fdb141cfa4274..73a3624e34098 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
>  		to_intel_plane_state(plane->base.state);
>  	enum plane_id plane_id = plane->id;
>  	enum pipe pipe = crtc->pipe;
> -	u32 base;
> +	u32 base, plane_ctl;
>  
>  	if (!plane_state->uapi.visible)
>  		return false;
> @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
>  	if (plane_config->base == base)
>  		return false;
>  
> +	/* Perform an async flip to the new surface. */
> +	plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id));
> +	plane_ctl |= PLANE_CTL_ASYNC_FLIP;
> +

No async flips!

> +	intel_de_write(i915, PLANE_CTL(pipe, plane_id), plane_ctl);
>  	intel_de_write(i915, PLANE_SURF(pipe, plane_id), base);
>  
> -	return true;
> +	if (intel_de_wait_custom(i915, PLANE_SURFLIVE(pipe, plane_id), ~0U, base, 0, 40, NULL) < 0)
> +		drm_warn(&i915->drm, "async flip timed out\n");
> +
> +	/* No need to vblank wait either */
> +	return false;
>  }
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 03/12] drm/i915/display: Use async flip when available for initial plane config

[Maarten Lankhorst]

Hello,

Den 2024-10-03 kl. 18:14, skrev Ville Syrjälä:
> On Thu, Oct 03, 2024 at 05:44:12PM +0200, Maarten Lankhorst wrote:
>> I'm planning to reorder readout in the Xe sequence in such a way that
>> interrupts will not be available, so just use an async flip.
>>
>> Since the new FB points to the same pages, it will not tear. It also
>> has the benefit of perhaps being slightly faster.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> index fdb141cfa4274..73a3624e34098 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
>>   		to_intel_plane_state(plane->base.state);
>>   	enum plane_id plane_id = plane->id;
>>   	enum pipe pipe = crtc->pipe;
>> -	u32 base;
>> +	u32 base, plane_ctl;
>>   
>>   	if (!plane_state->uapi.visible)
>>   		return false;
>> @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
>>   	if (plane_config->base == base)
>>   		return false;
>>   
>> +	/* Perform an async flip to the new surface. */
>> +	plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id));
>> +	plane_ctl |= PLANE_CTL_ASYNC_FLIP;
>> +
> 
> No async flips!

Can you please explain your reasoning? I think async flip would fit 
perfectly here. We cannot perform a wait_for_vblank as we will not have 
interrupts enabled yet. Additionally an async flip would cause a faster 
driver loading. Since the FB is exactly the same except set to a 
different address, no tearing will occur.

Cheers,
~Maarten

Re: [PATCH v3 03/12] drm/i915/display: Use async flip when available for initial plane config

[Ville Syrjälä]

On Thu, Oct 03, 2024 at 10:50:24PM +0200, Maarten Lankhorst wrote:
> Hello,
> 
> Den 2024-10-03 kl. 18:14, skrev Ville Syrjälä:
> > On Thu, Oct 03, 2024 at 05:44:12PM +0200, Maarten Lankhorst wrote:
> >> I'm planning to reorder readout in the Xe sequence in such a way that
> >> interrupts will not be available, so just use an async flip.
> >>
> >> Since the new FB points to the same pages, it will not tear. It also
> >> has the benefit of perhaps being slightly faster.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++--
> >>   1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> index fdb141cfa4274..73a3624e34098 100644
> >> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
> >>   		to_intel_plane_state(plane->base.state);
> >>   	enum plane_id plane_id = plane->id;
> >>   	enum pipe pipe = crtc->pipe;
> >> -	u32 base;
> >> +	u32 base, plane_ctl;
> >>   
> >>   	if (!plane_state->uapi.visible)
> >>   		return false;
> >> @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
> >>   	if (plane_config->base == base)
> >>   		return false;
> >>   
> >> +	/* Perform an async flip to the new surface. */
> >> +	plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id));
> >> +	plane_ctl |= PLANE_CTL_ASYNC_FLIP;
> >> +
> > 
> > No async flips!
> 
> Can you please explain your reasoning?

Async flips are special. They have all kinds of random hardware
limitations.

> I think async flip would fit 
> perfectly here. We cannot perform a wait_for_vblank as we will not have 
> interrupts enabled yet.

The type of flip is irrelevant when you just poll until it's done.

> Additionally an async flip would cause a faster 
> driver loading. Since the FB is exactly the same except set to a 
> different address, no tearing will occur.

Until we violate some hardware requirement and you get a fault.

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 03/12] drm/i915/display: Use async flip when available for initial plane config

[kernel test robot]

Hi Maarten,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-xe/drm-xe-next]
[also build test ERROR on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.12-rc1]
[cannot apply to next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/drm-xe-display-Handle-stolen-bar-readout-in-the-same-way-as-lmem/20241004-000534
base:   https://gitlab.freedesktop.org/drm/xe/kernel.git drm-xe-next
patch link:    https://lore.kernel.org/r/20241003154421.33805-4-maarten.lankhorst%40linux.intel.com
patch subject: [PATCH v3 03/12] drm/i915/display: Use async flip when available for initial plane config
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20241004/202410042053.DCNgBMOr-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241004/202410042053.DCNgBMOr-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410042053.DCNgBMOr-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/skl_universal_plane.c:2814:14: error: call to undeclared function 'intel_read'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    2814 |         plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id));
         |                     ^
   1 error generated.


vim +/intel_read +2814 drivers/gpu/drm/i915/display/skl_universal_plane.c

  2789	
  2790	bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
  2791					    const struct intel_initial_plane_config *plane_config)
  2792	{
  2793		struct drm_i915_private *i915 = to_i915(crtc->base.dev);
  2794		struct intel_plane *plane = to_intel_plane(crtc->base.primary);
  2795		const struct intel_plane_state *plane_state =
  2796			to_intel_plane_state(plane->base.state);
  2797		enum plane_id plane_id = plane->id;
  2798		enum pipe pipe = crtc->pipe;
  2799		u32 base, plane_ctl;
  2800	
  2801		if (!plane_state->uapi.visible)
  2802			return false;
  2803	
  2804		base = intel_plane_ggtt_offset(plane_state);
  2805	
  2806		/*
  2807		 * We may have moved the surface to a different
  2808		 * part of ggtt, make the plane aware of that.
  2809		 */
  2810		if (plane_config->base == base)
  2811			return false;
  2812	
  2813		/* Perform an async flip to the new surface. */
> 2814		plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id));

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2.1 03/12] drm/i915/display: Use async flip when available for initial plane config

[Maarten Lankhorst]

I'm planning to reorder readout in the Xe sequence in such a way that
interrupts will not be available, so just use an async flip.

Since the new FB points to the same pages, it will not tear. It also
has the benefit of perhaps being slightly faster.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
Fix compiler fails..
Change intel_de_wait_custom to normal variant.

I still believe we should be fine with async flips. The buffer will not
be a standard RGBX8888 on the first plane. If we violate a constraint, it
will be from alignment, and for that it would be interesting to find any
border cases we missed.

 drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index fdb141cfa4274..c7ba8fcff20c9 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
 		to_intel_plane_state(plane->base.state);
 	enum plane_id plane_id = plane->id;
 	enum pipe pipe = crtc->pipe;
-	u32 base;
+	u32 base, plane_ctl;
 
 	if (!plane_state->uapi.visible)
 		return false;
@@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
 	if (plane_config->base == base)
 		return false;
 
+	/* Perform an async flip to the new surface. */
+	plane_ctl = intel_de_read(i915, PLANE_CTL(pipe, plane_id));
+	plane_ctl |= PLANE_CTL_ASYNC_FLIP;
+
+	intel_de_write(i915, PLANE_CTL(pipe, plane_id), plane_ctl);
 	intel_de_write(i915, PLANE_SURF(pipe, plane_id), base);
 
-	return true;
+	if (intel_de_wait(i915, PLANE_SURFLIVE(pipe, plane_id), ~0U, base, 40) < 0)
+		drm_warn(&i915->drm, "async flip timed out\n");
+
+	/* No need to vblank wait either */
+	return false;
 }
-- 
2.45.2

[PATCH v3 04/12] drm/xe/display: Remove single wait for vblank

[Maarten Lankhorst]

Now that i915/display is using async flip waits, we can remove the
vblank handling from xe. This makes the initial allocation code no
longer require interrupts.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/xe/display/xe_plane_initial.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
index 1f5128927c07c..ddbd964dc20f5 100644
--- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
+++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
@@ -296,8 +296,7 @@ void intel_initial_plane_config(struct drm_i915_private *i915)
 		 */
 		intel_find_initial_plane_obj(crtc, plane_configs);
 
-		if (i915->display.funcs.display->fixup_initial_plane_config(crtc, plane_config))
-			intel_crtc_wait_for_next_vblank(crtc);
+		i915->display.funcs.display->fixup_initial_plane_config(crtc, plane_config);
 
 		plane_config_fini(plane_config);
 	}
-- 
2.45.2

[PATCH v3 05/12] drm/xe: Move suballocator init to after display init

[Maarten Lankhorst]

No allocations should be done before we have had a chance to preserve
the display fb.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_device.c |  6 ++++++
 drivers/gpu/drm/xe/xe_tile.c   | 12 ++++++++----
 drivers/gpu/drm/xe/xe_tile.h   |  1 +
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 09a7ad830e695..e9e2946b0b53c 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -732,6 +732,12 @@ int xe_device_probe(struct xe_device *xe)
 	if (err)
 		goto err;
 
+	for_each_tile(tile, xe, id) {
+		err = xe_tile_init(tile);
+		if (err)
+			goto err;
+	}
+
 	for_each_gt(gt, xe, id) {
 		last_gt = id;
 
diff --git a/drivers/gpu/drm/xe/xe_tile.c b/drivers/gpu/drm/xe/xe_tile.c
index dda5268507d8e..164751bd9af22 100644
--- a/drivers/gpu/drm/xe/xe_tile.c
+++ b/drivers/gpu/drm/xe/xe_tile.c
@@ -167,10 +167,6 @@ int xe_tile_init_noalloc(struct xe_tile *tile)
 	if (err)
 		return err;
 
-	tile->mem.kernel_bb_pool = xe_sa_bo_manager_init(tile, SZ_1M, 16);
-	if (IS_ERR(tile->mem.kernel_bb_pool))
-		return PTR_ERR(tile->mem.kernel_bb_pool);
-
 	xe_wa_apply_tile_workarounds(tile);
 
 	err = xe_tile_sysfs_init(tile);
@@ -178,6 +174,14 @@ int xe_tile_init_noalloc(struct xe_tile *tile)
 	return 0;
 }
 
+int xe_tile_init(struct xe_tile *tile)
+{
+	tile->mem.kernel_bb_pool = xe_sa_bo_manager_init(tile, SZ_1M, 16);
+	if (IS_ERR(tile->mem.kernel_bb_pool))
+		return PTR_ERR(tile->mem.kernel_bb_pool);
+
+	return 0;
+}
 void xe_tile_migrate_wait(struct xe_tile *tile)
 {
 	xe_migrate_wait(tile->migrate);
diff --git a/drivers/gpu/drm/xe/xe_tile.h b/drivers/gpu/drm/xe/xe_tile.h
index 1c9e42ade6b05..eb939316d55b0 100644
--- a/drivers/gpu/drm/xe/xe_tile.h
+++ b/drivers/gpu/drm/xe/xe_tile.h
@@ -12,6 +12,7 @@ struct xe_tile;
 
 int xe_tile_init_early(struct xe_tile *tile, struct xe_device *xe, u8 id);
 int xe_tile_init_noalloc(struct xe_tile *tile);
+int xe_tile_init(struct xe_tile *tile);
 
 void xe_tile_migrate_wait(struct xe_tile *tile);
 
-- 
2.45.2

[PATCH v3 06/12] drm/xe: Use xe_ggtt_map_bo_unlocked for resume

[Maarten Lankhorst]

This is the first step to hide the details of struct xe_ggtt.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_bo_evict.c |  9 ++-------
 drivers/gpu/drm/xe/xe_ggtt.c     | 16 +++++++++++++++-
 drivers/gpu/drm/xe/xe_ggtt.h     |  2 +-
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_bo_evict.c b/drivers/gpu/drm/xe/xe_bo_evict.c
index 541b49007d738..788c071af92e0 100644
--- a/drivers/gpu/drm/xe/xe_bo_evict.c
+++ b/drivers/gpu/drm/xe/xe_bo_evict.c
@@ -146,13 +146,8 @@ int xe_bo_restore_kernel(struct xe_device *xe)
 			return ret;
 		}
 
-		if (bo->flags & XE_BO_FLAG_GGTT) {
-			struct xe_tile *tile = bo->tile;
-
-			mutex_lock(&tile->mem.ggtt->lock);
-			xe_ggtt_map_bo(tile->mem.ggtt, bo);
-			mutex_unlock(&tile->mem.ggtt->lock);
-		}
+		if (bo->flags & XE_BO_FLAG_GGTT)
+			xe_ggtt_map_bo_unlocked(bo->tile->mem.ggtt, bo);
 
 		/*
 		 * We expect validate to trigger a move VRAM and our move code
diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
index f68af56c3f865..1ffc0917e28fe 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.c
+++ b/drivers/gpu/drm/xe/xe_ggtt.c
@@ -579,7 +579,7 @@ bool xe_ggtt_node_allocated(const struct xe_ggtt_node *node)
  * @ggtt: the &xe_ggtt where node will be mapped
  * @bo: the &xe_bo to be mapped
  */
-void xe_ggtt_map_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
+static void xe_ggtt_map_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
 {
 	u16 cache_mode = bo->flags & XE_BO_FLAG_NEEDS_UC ? XE_CACHE_NONE : XE_CACHE_WB;
 	u16 pat_index = tile_to_xe(ggtt->tile)->pat.idx[cache_mode];
@@ -597,6 +597,20 @@ void xe_ggtt_map_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
 	}
 }
 
+/**
+ * xe_ggtt_map_bo_unlocked - Restore a mapping of a BO into GGTT
+ * @ggtt: the &xe_ggtt where node will be mapped
+ * @bo: the &xe_bo to be mapped
+ *
+ * This is used to restore a GGTT mapping after suspend.
+ */
+void xe_ggtt_map_bo_unlocked(struct xe_ggtt *ggtt, struct xe_bo *bo)
+{
+	mutex_lock(&ggtt->lock);
+	xe_ggtt_map_bo(ggtt, bo);
+	mutex_unlock(&ggtt->lock);
+}
+
 static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
 				  u64 start, u64 end)
 {
diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
index 27e7d67de0047..bdf6d0733e2ca 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.h
+++ b/drivers/gpu/drm/xe/xe_ggtt.h
@@ -24,7 +24,7 @@ int xe_ggtt_node_insert_locked(struct xe_ggtt_node *node,
 			       u32 size, u32 align, u32 mm_flags);
 void xe_ggtt_node_remove(struct xe_ggtt_node *node, bool invalidate);
 bool xe_ggtt_node_allocated(const struct xe_ggtt_node *node);
-void xe_ggtt_map_bo(struct xe_ggtt *ggtt, struct xe_bo *bo);
+void xe_ggtt_map_bo_unlocked(struct xe_ggtt *ggtt, struct xe_bo *bo);
 int xe_ggtt_insert_bo(struct xe_ggtt *ggtt, struct xe_bo *bo);
 int xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
 			 u64 start, u64 end);
-- 
2.45.2

Re: [PATCH v3 06/12] drm/xe: Use xe_ggtt_map_bo_unlocked for resume

[Matthew Brost]

On Thu, Oct 03, 2024 at 05:44:15PM +0200, Maarten Lankhorst wrote:
> This is the first step to hide the details of struct xe_ggtt.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

> ---
>  drivers/gpu/drm/xe/xe_bo_evict.c |  9 ++-------
>  drivers/gpu/drm/xe/xe_ggtt.c     | 16 +++++++++++++++-
>  drivers/gpu/drm/xe/xe_ggtt.h     |  2 +-
>  3 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo_evict.c b/drivers/gpu/drm/xe/xe_bo_evict.c
> index 541b49007d738..788c071af92e0 100644
> --- a/drivers/gpu/drm/xe/xe_bo_evict.c
> +++ b/drivers/gpu/drm/xe/xe_bo_evict.c
> @@ -146,13 +146,8 @@ int xe_bo_restore_kernel(struct xe_device *xe)
>  			return ret;
>  		}
>  
> -		if (bo->flags & XE_BO_FLAG_GGTT) {
> -			struct xe_tile *tile = bo->tile;
> -
> -			mutex_lock(&tile->mem.ggtt->lock);
> -			xe_ggtt_map_bo(tile->mem.ggtt, bo);
> -			mutex_unlock(&tile->mem.ggtt->lock);
> -		}
> +		if (bo->flags & XE_BO_FLAG_GGTT)
> +			xe_ggtt_map_bo_unlocked(bo->tile->mem.ggtt, bo);
>  
>  		/*
>  		 * We expect validate to trigger a move VRAM and our move code
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index f68af56c3f865..1ffc0917e28fe 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -579,7 +579,7 @@ bool xe_ggtt_node_allocated(const struct xe_ggtt_node *node)
>   * @ggtt: the &xe_ggtt where node will be mapped
>   * @bo: the &xe_bo to be mapped
>   */
> -void xe_ggtt_map_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
> +static void xe_ggtt_map_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
>  {
>  	u16 cache_mode = bo->flags & XE_BO_FLAG_NEEDS_UC ? XE_CACHE_NONE : XE_CACHE_WB;
>  	u16 pat_index = tile_to_xe(ggtt->tile)->pat.idx[cache_mode];
> @@ -597,6 +597,20 @@ void xe_ggtt_map_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
>  	}
>  }
>  
> +/**
> + * xe_ggtt_map_bo_unlocked - Restore a mapping of a BO into GGTT
> + * @ggtt: the &xe_ggtt where node will be mapped
> + * @bo: the &xe_bo to be mapped
> + *
> + * This is used to restore a GGTT mapping after suspend.
> + */
> +void xe_ggtt_map_bo_unlocked(struct xe_ggtt *ggtt, struct xe_bo *bo)
> +{
> +	mutex_lock(&ggtt->lock);
> +	xe_ggtt_map_bo(ggtt, bo);
> +	mutex_unlock(&ggtt->lock);
> +}
> +
>  static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
>  				  u64 start, u64 end)
>  {
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index 27e7d67de0047..bdf6d0733e2ca 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -24,7 +24,7 @@ int xe_ggtt_node_insert_locked(struct xe_ggtt_node *node,
>  			       u32 size, u32 align, u32 mm_flags);
>  void xe_ggtt_node_remove(struct xe_ggtt_node *node, bool invalidate);
>  bool xe_ggtt_node_allocated(const struct xe_ggtt_node *node);
> -void xe_ggtt_map_bo(struct xe_ggtt *ggtt, struct xe_bo *bo);
> +void xe_ggtt_map_bo_unlocked(struct xe_ggtt *ggtt, struct xe_bo *bo);
>  int xe_ggtt_insert_bo(struct xe_ggtt *ggtt, struct xe_bo *bo);
>  int xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
>  			 u64 start, u64 end);
> -- 
> 2.45.2
> 

[PATCH v3 07/12] drm/xe: Add xe_ggtt_might_lock

[Maarten Lankhorst]

Another requirement of hiding more of struct xe_ggtt.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_bo.c   | 2 +-
 drivers/gpu/drm/xe/xe_ggtt.c | 7 +++++++
 drivers/gpu/drm/xe/xe_ggtt.h | 7 +++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
index 5f2f1ec46b57a..f85c389e9a1f8 100644
--- a/drivers/gpu/drm/xe/xe_bo.c
+++ b/drivers/gpu/drm/xe/xe_bo.c
@@ -2359,7 +2359,7 @@ void xe_bo_put(struct xe_bo *bo)
 			might_lock(&bo->client->bos_lock);
 #endif
 		if (bo->ggtt_node && bo->ggtt_node->ggtt)
-			might_lock(&bo->ggtt_node->ggtt->lock);
+			xe_ggtt_might_lock(bo->ggtt_node->ggtt);
 		drm_gem_object_put(&bo->ttm.base);
 	}
 }
diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
index 1ffc0917e28fe..7e5a793651583 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.c
+++ b/drivers/gpu/drm/xe/xe_ggtt.c
@@ -175,6 +175,13 @@ static void ggtt_fini(void *arg)
 	ggtt->scratch = NULL;
 }
 
+#ifdef CONFIG_LOCKDEP
+void xe_ggtt_might_lock(struct xe_ggtt *ggtt)
+{
+	might_lock(&ggtt->lock);
+}
+#endif
+
 static void primelockdep(struct xe_ggtt *ggtt)
 {
 	if (!IS_ENABLED(CONFIG_LOCKDEP))
diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
index bdf6d0733e2ca..62c8ce636939a 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.h
+++ b/drivers/gpu/drm/xe/xe_ggtt.h
@@ -38,4 +38,11 @@ u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer
 void xe_ggtt_assign(const struct xe_ggtt_node *node, u16 vfid);
 #endif
 
+#ifndef CONFIG_LOCKDEP
+static inline void xe_ggtt_might_lock(struct xe_ggtt *ggtt)
+{ }
+#else
+void xe_ggtt_might_lock(struct xe_ggtt *ggtt);
+#endif
+
 #endif
-- 
2.45.2

Re: [PATCH v3 07/12] drm/xe: Add xe_ggtt_might_lock

[Matthew Brost]

On Thu, Oct 03, 2024 at 05:44:16PM +0200, Maarten Lankhorst wrote:
> Another requirement of hiding more of struct xe_ggtt.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

> ---
>  drivers/gpu/drm/xe/xe_bo.c   | 2 +-
>  drivers/gpu/drm/xe/xe_ggtt.c | 7 +++++++
>  drivers/gpu/drm/xe/xe_ggtt.h | 7 +++++++
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 5f2f1ec46b57a..f85c389e9a1f8 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -2359,7 +2359,7 @@ void xe_bo_put(struct xe_bo *bo)
>  			might_lock(&bo->client->bos_lock);
>  #endif
>  		if (bo->ggtt_node && bo->ggtt_node->ggtt)
> -			might_lock(&bo->ggtt_node->ggtt->lock);
> +			xe_ggtt_might_lock(bo->ggtt_node->ggtt);
>  		drm_gem_object_put(&bo->ttm.base);
>  	}
>  }
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 1ffc0917e28fe..7e5a793651583 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -175,6 +175,13 @@ static void ggtt_fini(void *arg)
>  	ggtt->scratch = NULL;
>  }
>  
> +#ifdef CONFIG_LOCKDEP
> +void xe_ggtt_might_lock(struct xe_ggtt *ggtt)
> +{
> +	might_lock(&ggtt->lock);
> +}
> +#endif
> +
>  static void primelockdep(struct xe_ggtt *ggtt)
>  {
>  	if (!IS_ENABLED(CONFIG_LOCKDEP))
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index bdf6d0733e2ca..62c8ce636939a 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -38,4 +38,11 @@ u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer
>  void xe_ggtt_assign(const struct xe_ggtt_node *node, u16 vfid);
>  #endif
>  
> +#ifndef CONFIG_LOCKDEP
> +static inline void xe_ggtt_might_lock(struct xe_ggtt *ggtt)
> +{ }
> +#else
> +void xe_ggtt_might_lock(struct xe_ggtt *ggtt);
> +#endif
> +
>  #endif
> -- 
> 2.45.2
> 

[PATCH v3 08/12] drm/xe: Add xe_ggtt_alloc

[Maarten Lankhorst]

Instead of allocating inside xe_tile, create a new function that returns
an allocated struct xe_ggtt from xe_ggtt.c

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_ggtt.c | 8 ++++++++
 drivers/gpu/drm/xe/xe_ggtt.h | 2 ++
 drivers/gpu/drm/xe/xe_tile.c | 4 +---
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
index 7e5a793651583..4866e9b252ad9 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.c
+++ b/drivers/gpu/drm/xe/xe_ggtt.c
@@ -159,6 +159,14 @@ static void xe_ggtt_clear(struct xe_ggtt *ggtt, u64 start, u64 size)
 	}
 }
 
+struct xe_ggtt *xe_ggtt_alloc(struct xe_tile *tile)
+{
+	struct xe_ggtt *ggtt = drmm_kzalloc(&tile_to_xe(tile)->drm, sizeof(*ggtt), GFP_KERNEL);
+	if (ggtt)
+		ggtt->tile = tile;
+	return ggtt;
+}
+
 static void ggtt_fini_early(struct drm_device *drm, void *arg)
 {
 	struct xe_ggtt *ggtt = arg;
diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
index 62c8ce636939a..0bab1fd7cc817 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.h
+++ b/drivers/gpu/drm/xe/xe_ggtt.h
@@ -9,7 +9,9 @@
 #include "xe_ggtt_types.h"
 
 struct drm_printer;
+struct xe_tile;
 
+struct xe_ggtt *xe_ggtt_alloc(struct xe_tile *tile);
 int xe_ggtt_init_early(struct xe_ggtt *ggtt);
 int xe_ggtt_init(struct xe_ggtt *ggtt);
 
diff --git a/drivers/gpu/drm/xe/xe_tile.c b/drivers/gpu/drm/xe/xe_tile.c
index 164751bd9af22..1cd4450305a6a 100644
--- a/drivers/gpu/drm/xe/xe_tile.c
+++ b/drivers/gpu/drm/xe/xe_tile.c
@@ -86,11 +86,9 @@ static int xe_tile_alloc(struct xe_tile *tile)
 {
 	struct drm_device *drm = &tile_to_xe(tile)->drm;
 
-	tile->mem.ggtt = drmm_kzalloc(drm, sizeof(*tile->mem.ggtt),
-				      GFP_KERNEL);
+	tile->mem.ggtt = xe_ggtt_alloc(tile);
 	if (!tile->mem.ggtt)
 		return -ENOMEM;
-	tile->mem.ggtt->tile = tile;
 
 	tile->mem.vram_mgr = drmm_kzalloc(drm, sizeof(*tile->mem.vram_mgr), GFP_KERNEL);
 	if (!tile->mem.vram_mgr)
-- 
2.45.2

Re: [PATCH v3 08/12] drm/xe: Add xe_ggtt_alloc

[Matthew Brost]

On Thu, Oct 03, 2024 at 05:44:17PM +0200, Maarten Lankhorst wrote:
> Instead of allocating inside xe_tile, create a new function that returns
> an allocated struct xe_ggtt from xe_ggtt.c
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/xe_ggtt.c | 8 ++++++++
>  drivers/gpu/drm/xe/xe_ggtt.h | 2 ++
>  drivers/gpu/drm/xe/xe_tile.c | 4 +---
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 7e5a793651583..4866e9b252ad9 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -159,6 +159,14 @@ static void xe_ggtt_clear(struct xe_ggtt *ggtt, u64 start, u64 size)
>  	}
>  }
>  

Probably kernel doc as we shouldn't add more undocumented public
functions.

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

> +struct xe_ggtt *xe_ggtt_alloc(struct xe_tile *tile)
> +{
> +	struct xe_ggtt *ggtt = drmm_kzalloc(&tile_to_xe(tile)->drm, sizeof(*ggtt), GFP_KERNEL);
> +	if (ggtt)
> +		ggtt->tile = tile;
> +	return ggtt;
> +}
> +
>  static void ggtt_fini_early(struct drm_device *drm, void *arg)
>  {
>  	struct xe_ggtt *ggtt = arg;
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index 62c8ce636939a..0bab1fd7cc817 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -9,7 +9,9 @@
>  #include "xe_ggtt_types.h"
>  
>  struct drm_printer;
> +struct xe_tile;
>  
> +struct xe_ggtt *xe_ggtt_alloc(struct xe_tile *tile);
>  int xe_ggtt_init_early(struct xe_ggtt *ggtt);
>  int xe_ggtt_init(struct xe_ggtt *ggtt);
>  
> diff --git a/drivers/gpu/drm/xe/xe_tile.c b/drivers/gpu/drm/xe/xe_tile.c
> index 164751bd9af22..1cd4450305a6a 100644
> --- a/drivers/gpu/drm/xe/xe_tile.c
> +++ b/drivers/gpu/drm/xe/xe_tile.c
> @@ -86,11 +86,9 @@ static int xe_tile_alloc(struct xe_tile *tile)
>  {
>  	struct drm_device *drm = &tile_to_xe(tile)->drm;
>  
> -	tile->mem.ggtt = drmm_kzalloc(drm, sizeof(*tile->mem.ggtt),
> -				      GFP_KERNEL);
> +	tile->mem.ggtt = xe_ggtt_alloc(tile);
>  	if (!tile->mem.ggtt)
>  		return -ENOMEM;
> -	tile->mem.ggtt->tile = tile;
>  
>  	tile->mem.vram_mgr = drmm_kzalloc(drm, sizeof(*tile->mem.vram_mgr), GFP_KERNEL);
>  	if (!tile->mem.vram_mgr)
> -- 
> 2.45.2
> 

[PATCH v3 09/12] drm/xe: Abstract read/write functions for GGTT PTEs

[Maarten Lankhorst]

Instead of writing directly, use GGTT functions. This allows us to
hide away more GGTT details.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/xe/display/xe_fb_pin.c        |  4 +--
 drivers/gpu/drm/xe/display/xe_plane_initial.c |  6 +----
 drivers/gpu/drm/xe/xe_ggtt.c                  | 25 +++++++++++++++++++
 drivers/gpu/drm/xe/xe_ggtt.h                  |  3 +++
 4 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
index 79dbbbe03c7f6..bddd526b33297 100644
--- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
+++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
@@ -172,7 +172,7 @@ write_ggtt_rotated(struct xe_bo *bo, struct xe_ggtt *ggtt, u32 *ggtt_ofs, u32 bo
 			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
 							      xe->pat.idx[XE_CACHE_NONE]);
 
-			ggtt->pt_ops->ggtt_set_pte(ggtt, *ggtt_ofs, pte);
+			xe_ggtt_write_pte(ggtt, *ggtt_ofs, pte);
 			*ggtt_ofs += XE_PAGE_SIZE;
 			src_idx -= src_stride;
 		}
@@ -226,7 +226,7 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
 			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x,
 							      xe->pat.idx[XE_CACHE_NONE]);
 
-			ggtt->pt_ops->ggtt_set_pte(ggtt, vma->node->base.start + x, pte);
+			xe_ggtt_write_pte(ggtt, vma->node->base.start + x, pte);
 		}
 	} else {
 		u32 i, ggtt_ofs;
diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
index ddbd964dc20f5..7cbe7fcf8b600 100644
--- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
+++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
@@ -70,12 +70,8 @@ initial_plane_bo(struct xe_device *xe,
 
 	base = round_down(plane_config->base, page_size);
 	if (IS_DGFX(xe) || GRAPHICS_VERx100(xe) >= 1270) {
-		u64 __iomem *gte = tile0->mem.ggtt->gsm;
-		u64 pte;
+		u64 pte = xe_ggtt_read_pte(tile0->mem.ggtt, base);
 
-		gte += base / XE_PAGE_SIZE;
-
-		pte = ioread64(gte);
 		if (!(pte & XE_GGTT_PTE_DM)) {
 			drm_err(&xe->drm,
 				"Initial plane programming missing DM bit\n");
diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
index 4866e9b252ad9..6945fbfc555ce 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.c
+++ b/drivers/gpu/drm/xe/xe_ggtt.c
@@ -855,3 +855,28 @@ u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer
 
 	return total;
 }
+
+/**
+ * xe_ggtt_read_pte - Read a PTE from the GGTT
+ * @ggtt: &xe_ggtt
+ * @offset: the offset for which the mapping should be read.
+ *
+ * Used by display for inheriting a bios set FB.
+ */
+u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset)
+{
+	return ioread64(ggtt->gsm + (offset / XE_PAGE_SIZE));
+}
+
+/**
+ * xe_ggtt_write_pte - Write a PTE to the GGTT
+ * @ggtt: &xe_ggtt
+ * @offset: the offset for which the mapping should be written.
+ * @pte: The page table entry to write
+ *
+ * Used by display for writing normal and rotated GGTT entries for temporary pinned FB's.
+ */
+void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte)
+{
+	return ggtt->pt_ops->ggtt_set_pte(ggtt, offset, pte);
+}
diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
index 0bab1fd7cc817..f83e5af0400e9 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.h
+++ b/drivers/gpu/drm/xe/xe_ggtt.h
@@ -47,4 +47,7 @@ static inline void xe_ggtt_might_lock(struct xe_ggtt *ggtt)
 void xe_ggtt_might_lock(struct xe_ggtt *ggtt);
 #endif
 
+u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset);
+void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte);
+
 #endif
-- 
2.45.2

Re: [PATCH v3 09/12] drm/xe: Abstract read/write functions for GGTT PTEs

[Matthew Brost]

On Thu, Oct 03, 2024 at 05:44:18PM +0200, Maarten Lankhorst wrote:
> Instead of writing directly, use GGTT functions. This allows us to
> hide away more GGTT details.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

> ---
>  drivers/gpu/drm/xe/display/xe_fb_pin.c        |  4 +--
>  drivers/gpu/drm/xe/display/xe_plane_initial.c |  6 +----
>  drivers/gpu/drm/xe/xe_ggtt.c                  | 25 +++++++++++++++++++
>  drivers/gpu/drm/xe/xe_ggtt.h                  |  3 +++
>  4 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> index 79dbbbe03c7f6..bddd526b33297 100644
> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> @@ -172,7 +172,7 @@ write_ggtt_rotated(struct xe_bo *bo, struct xe_ggtt *ggtt, u32 *ggtt_ofs, u32 bo
>  			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
>  							      xe->pat.idx[XE_CACHE_NONE]);
>  
> -			ggtt->pt_ops->ggtt_set_pte(ggtt, *ggtt_ofs, pte);
> +			xe_ggtt_write_pte(ggtt, *ggtt_ofs, pte);
>  			*ggtt_ofs += XE_PAGE_SIZE;
>  			src_idx -= src_stride;
>  		}
> @@ -226,7 +226,7 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
>  			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x,
>  							      xe->pat.idx[XE_CACHE_NONE]);
>  
> -			ggtt->pt_ops->ggtt_set_pte(ggtt, vma->node->base.start + x, pte);
> +			xe_ggtt_write_pte(ggtt, vma->node->base.start + x, pte);
>  		}
>  	} else {
>  		u32 i, ggtt_ofs;
> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> index ddbd964dc20f5..7cbe7fcf8b600 100644
> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> @@ -70,12 +70,8 @@ initial_plane_bo(struct xe_device *xe,
>  
>  	base = round_down(plane_config->base, page_size);
>  	if (IS_DGFX(xe) || GRAPHICS_VERx100(xe) >= 1270) {
> -		u64 __iomem *gte = tile0->mem.ggtt->gsm;
> -		u64 pte;
> +		u64 pte = xe_ggtt_read_pte(tile0->mem.ggtt, base);
>  
> -		gte += base / XE_PAGE_SIZE;
> -
> -		pte = ioread64(gte);
>  		if (!(pte & XE_GGTT_PTE_DM)) {
>  			drm_err(&xe->drm,
>  				"Initial plane programming missing DM bit\n");
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 4866e9b252ad9..6945fbfc555ce 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -855,3 +855,28 @@ u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer
>  
>  	return total;
>  }
> +
> +/**
> + * xe_ggtt_read_pte - Read a PTE from the GGTT
> + * @ggtt: &xe_ggtt
> + * @offset: the offset for which the mapping should be read.
> + *
> + * Used by display for inheriting a bios set FB.
> + */
> +u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset)
> +{
> +	return ioread64(ggtt->gsm + (offset / XE_PAGE_SIZE));
> +}
> +
> +/**
> + * xe_ggtt_write_pte - Write a PTE to the GGTT
> + * @ggtt: &xe_ggtt
> + * @offset: the offset for which the mapping should be written.
> + * @pte: The page table entry to write
> + *
> + * Used by display for writing normal and rotated GGTT entries for temporary pinned FB's.
> + */
> +void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte)
> +{
> +	return ggtt->pt_ops->ggtt_set_pte(ggtt, offset, pte);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index 0bab1fd7cc817..f83e5af0400e9 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -47,4 +47,7 @@ static inline void xe_ggtt_might_lock(struct xe_ggtt *ggtt)
>  void xe_ggtt_might_lock(struct xe_ggtt *ggtt);
>  #endif
>  
> +u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset);
> +void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte);
> +
>  #endif
> -- 
> 2.45.2
> 

[PATCH v3 10/12] drm/xe: Make xe_ggtt_pt_ops private

[Maarten Lankhorst]

The only user outside xe_ggtt.c is fb pinning, which makes sense as
all the operations it performs can be considered part of GGTT.

We could move this to xe_ggtt.c, but lets keep it inside display for
now.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/xe/display/xe_fb_pin.c | 22 ++++++++++++----------
 drivers/gpu/drm/xe/xe_ggtt.c           | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_ggtt.h           |  2 ++
 drivers/gpu/drm/xe/xe_ggtt_types.h     | 13 ++-----------
 4 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
index bddd526b33297..0ae5d917f20fe 100644
--- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
+++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
@@ -20,6 +20,7 @@ write_dpt_rotated(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs, u32 bo_
 {
 	struct xe_device *xe = xe_bo_device(bo);
 	struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
+	xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
 	u32 column, row;
 
 	/* TODO: Maybe rewrite so we can traverse the bo addresses sequentially,
@@ -30,8 +31,8 @@ write_dpt_rotated(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs, u32 bo_
 		u32 src_idx = src_stride * (height - 1) + column + bo_ofs;
 
 		for (row = 0; row < height; row++) {
-			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
-							      xe->pat.idx[XE_CACHE_NONE]);
+			u64 pte = pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
+						xe->pat.idx[XE_CACHE_NONE]);
 
 			iosys_map_wr(map, *dpt_ofs, u64, pte);
 			*dpt_ofs += 8;
@@ -53,8 +54,7 @@ write_dpt_remapped(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs,
 {
 	struct xe_device *xe = xe_bo_device(bo);
 	struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
-	u64 (*pte_encode_bo)(struct xe_bo *bo, u64 bo_offset, u16 pat_index)
-		= ggtt->pt_ops->pte_encode_bo;
+	xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
 	u32 column, row;
 
 	for (row = 0; row < height; row++) {
@@ -120,11 +120,12 @@ static int __xe_pin_fb_vma_dpt(const struct intel_framebuffer *fb,
 		return PTR_ERR(dpt);
 
 	if (view->type == I915_GTT_VIEW_NORMAL) {
+		xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
 		u32 x;
 
 		for (x = 0; x < size / XE_PAGE_SIZE; x++) {
-			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x * XE_PAGE_SIZE,
-							      xe->pat.idx[XE_CACHE_NONE]);
+			u64 pte = pte_encode_bo(bo, x * XE_PAGE_SIZE,
+						xe->pat.idx[XE_CACHE_NONE]);
 
 			iosys_map_wr(&dpt->vmap, x * 8, u64, pte);
 		}
@@ -163,14 +164,15 @@ write_ggtt_rotated(struct xe_bo *bo, struct xe_ggtt *ggtt, u32 *ggtt_ofs, u32 bo
 		   u32 width, u32 height, u32 src_stride, u32 dst_stride)
 {
 	struct xe_device *xe = xe_bo_device(bo);
+	xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
 	u32 column, row;
 
 	for (column = 0; column < width; column++) {
 		u32 src_idx = src_stride * (height - 1) + column + bo_ofs;
 
 		for (row = 0; row < height; row++) {
-			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
-							      xe->pat.idx[XE_CACHE_NONE]);
+			u64 pte = pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
+						xe->pat.idx[XE_CACHE_NONE]);
 
 			xe_ggtt_write_pte(ggtt, *ggtt_ofs, pte);
 			*ggtt_ofs += XE_PAGE_SIZE;
@@ -209,6 +211,7 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
 		vma->node = bo->ggtt_node;
 	} else if (view->type == I915_GTT_VIEW_NORMAL) {
 		u32 x, size = bo->ttm.base.size;
+		xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
 
 		vma->node = xe_ggtt_node_init(ggtt);
 		if (IS_ERR(vma->node)) {
@@ -223,8 +226,7 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
 		}
 
 		for (x = 0; x < size; x += XE_PAGE_SIZE) {
-			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x,
-							      xe->pat.idx[XE_CACHE_NONE]);
+			u64 pte = pte_encode_bo(bo, x, xe->pat.idx[XE_CACHE_NONE]);
 
 			xe_ggtt_write_pte(ggtt, vma->node->base.start + x, pte);
 		}
diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
index 6945fbfc555ce..db6a761398064 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.c
+++ b/drivers/gpu/drm/xe/xe_ggtt.c
@@ -63,6 +63,18 @@
  * 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_bo: Encode PTE address for a given BO */
+	xe_ggtt_pte_encode_bo_fn pte_encode_bo;
+	/** @ggtt_set_pte: Directly write into GGTT's PTE */
+	void (*ggtt_set_pte)(struct xe_ggtt *ggtt, u64 addr, u64 pte);
+};
+
+
 static u64 xelp_ggtt_pte_encode_bo(struct xe_bo *bo, u64 bo_offset,
 				   u16 pat_index)
 {
@@ -880,3 +892,15 @@ void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte)
 {
 	return ggtt->pt_ops->ggtt_set_pte(ggtt, offset, pte);
 }
+
+/**
+ * xe_ggtt_write_pte - Write a PTE to the GGTT
+ * @ggtt: &xe_ggtt
+ * @offset: the offset for which the mapping should be written.
+ *
+ * Used by display for DPT and GGTT paths to enccode BO's.
+ */
+xe_ggtt_pte_encode_bo_fn xe_ggtt_get_encode_pte_bo_fn(struct xe_ggtt *ggtt)
+{
+	return ggtt->pt_ops->pte_encode_bo;
+}
diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
index f83e5af0400e9..0c63cfa083c03 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.h
+++ b/drivers/gpu/drm/xe/xe_ggtt.h
@@ -48,6 +48,8 @@ void xe_ggtt_might_lock(struct xe_ggtt *ggtt);
 #endif
 
 u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset);
+
+xe_ggtt_pte_encode_bo_fn xe_ggtt_get_encode_pte_bo_fn(struct xe_ggtt *ggtt);
 void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte);
 
 #endif
diff --git a/drivers/gpu/drm/xe/xe_ggtt_types.h b/drivers/gpu/drm/xe/xe_ggtt_types.h
index cb02b7994a9ac..c142ff59c4504 100644
--- a/drivers/gpu/drm/xe/xe_ggtt_types.h
+++ b/drivers/gpu/drm/xe/xe_ggtt_types.h
@@ -13,6 +13,8 @@
 struct xe_bo;
 struct xe_gt;
 
+typedef u64 (*xe_ggtt_pte_encode_bo_fn)(struct xe_bo *bo, u64 bo_offset, u16 pat_index);
+
 /**
  * struct xe_ggtt - Main GGTT struct
  *
@@ -69,15 +71,4 @@ struct xe_ggtt_node {
 	bool invalidate_on_remove;
 };
 
-/**
- * struct xe_ggtt_pt_ops - GGTT Page table operations
- * Which can vary from platform to platform.
- */
-struct xe_ggtt_pt_ops {
-	/** @pte_encode_bo: Encode PTE address for a given BO */
-	u64 (*pte_encode_bo)(struct xe_bo *bo, u64 bo_offset, u16 pat_index);
-	/** @ggtt_set_pte: Directly write into GGTT's PTE */
-	void (*ggtt_set_pte)(struct xe_ggtt *ggtt, u64 addr, u64 pte);
-};
-
 #endif
-- 
2.45.2

Re: [PATCH v3 10/12] drm/xe: Make xe_ggtt_pt_ops private

[Matthew Brost]

On Thu, Oct 03, 2024 at 05:44:19PM +0200, Maarten Lankhorst wrote:
> The only user outside xe_ggtt.c is fb pinning, which makes sense as
> all the operations it performs can be considered part of GGTT.
> 
> We could move this to xe_ggtt.c, but lets keep it inside display for
> now.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

> ---
>  drivers/gpu/drm/xe/display/xe_fb_pin.c | 22 ++++++++++++----------
>  drivers/gpu/drm/xe/xe_ggtt.c           | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_ggtt.h           |  2 ++
>  drivers/gpu/drm/xe/xe_ggtt_types.h     | 13 ++-----------
>  4 files changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> index bddd526b33297..0ae5d917f20fe 100644
> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> @@ -20,6 +20,7 @@ write_dpt_rotated(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs, u32 bo_
>  {
>  	struct xe_device *xe = xe_bo_device(bo);
>  	struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
> +	xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
>  	u32 column, row;
>  
>  	/* TODO: Maybe rewrite so we can traverse the bo addresses sequentially,
> @@ -30,8 +31,8 @@ write_dpt_rotated(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs, u32 bo_
>  		u32 src_idx = src_stride * (height - 1) + column + bo_ofs;
>  
>  		for (row = 0; row < height; row++) {
> -			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
> -							      xe->pat.idx[XE_CACHE_NONE]);
> +			u64 pte = pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
> +						xe->pat.idx[XE_CACHE_NONE]);
>  
>  			iosys_map_wr(map, *dpt_ofs, u64, pte);
>  			*dpt_ofs += 8;
> @@ -53,8 +54,7 @@ write_dpt_remapped(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs,
>  {
>  	struct xe_device *xe = xe_bo_device(bo);
>  	struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
> -	u64 (*pte_encode_bo)(struct xe_bo *bo, u64 bo_offset, u16 pat_index)
> -		= ggtt->pt_ops->pte_encode_bo;
> +	xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
>  	u32 column, row;
>  
>  	for (row = 0; row < height; row++) {
> @@ -120,11 +120,12 @@ static int __xe_pin_fb_vma_dpt(const struct intel_framebuffer *fb,
>  		return PTR_ERR(dpt);
>  
>  	if (view->type == I915_GTT_VIEW_NORMAL) {
> +		xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
>  		u32 x;
>  
>  		for (x = 0; x < size / XE_PAGE_SIZE; x++) {
> -			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x * XE_PAGE_SIZE,
> -							      xe->pat.idx[XE_CACHE_NONE]);
> +			u64 pte = pte_encode_bo(bo, x * XE_PAGE_SIZE,
> +						xe->pat.idx[XE_CACHE_NONE]);
>  
>  			iosys_map_wr(&dpt->vmap, x * 8, u64, pte);
>  		}
> @@ -163,14 +164,15 @@ write_ggtt_rotated(struct xe_bo *bo, struct xe_ggtt *ggtt, u32 *ggtt_ofs, u32 bo
>  		   u32 width, u32 height, u32 src_stride, u32 dst_stride)
>  {
>  	struct xe_device *xe = xe_bo_device(bo);
> +	xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
>  	u32 column, row;
>  
>  	for (column = 0; column < width; column++) {
>  		u32 src_idx = src_stride * (height - 1) + column + bo_ofs;
>  
>  		for (row = 0; row < height; row++) {
> -			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
> -							      xe->pat.idx[XE_CACHE_NONE]);
> +			u64 pte = pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
> +						xe->pat.idx[XE_CACHE_NONE]);
>  
>  			xe_ggtt_write_pte(ggtt, *ggtt_ofs, pte);
>  			*ggtt_ofs += XE_PAGE_SIZE;
> @@ -209,6 +211,7 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
>  		vma->node = bo->ggtt_node;
>  	} else if (view->type == I915_GTT_VIEW_NORMAL) {
>  		u32 x, size = bo->ttm.base.size;
> +		xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
>  
>  		vma->node = xe_ggtt_node_init(ggtt);
>  		if (IS_ERR(vma->node)) {
> @@ -223,8 +226,7 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
>  		}
>  
>  		for (x = 0; x < size; x += XE_PAGE_SIZE) {
> -			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x,
> -							      xe->pat.idx[XE_CACHE_NONE]);
> +			u64 pte = pte_encode_bo(bo, x, xe->pat.idx[XE_CACHE_NONE]);
>  
>  			xe_ggtt_write_pte(ggtt, vma->node->base.start + x, pte);
>  		}
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 6945fbfc555ce..db6a761398064 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -63,6 +63,18 @@
>   * 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_bo: Encode PTE address for a given BO */
> +	xe_ggtt_pte_encode_bo_fn pte_encode_bo;
> +	/** @ggtt_set_pte: Directly write into GGTT's PTE */
> +	void (*ggtt_set_pte)(struct xe_ggtt *ggtt, u64 addr, u64 pte);
> +};
> +
> +
>  static u64 xelp_ggtt_pte_encode_bo(struct xe_bo *bo, u64 bo_offset,
>  				   u16 pat_index)
>  {
> @@ -880,3 +892,15 @@ void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte)
>  {
>  	return ggtt->pt_ops->ggtt_set_pte(ggtt, offset, pte);
>  }
> +
> +/**
> + * xe_ggtt_write_pte - Write a PTE to the GGTT
> + * @ggtt: &xe_ggtt
> + * @offset: the offset for which the mapping should be written.
> + *
> + * Used by display for DPT and GGTT paths to enccode BO's.
> + */
> +xe_ggtt_pte_encode_bo_fn xe_ggtt_get_encode_pte_bo_fn(struct xe_ggtt *ggtt)
> +{
> +	return ggtt->pt_ops->pte_encode_bo;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index f83e5af0400e9..0c63cfa083c03 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -48,6 +48,8 @@ void xe_ggtt_might_lock(struct xe_ggtt *ggtt);
>  #endif
>  
>  u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset);
> +
> +xe_ggtt_pte_encode_bo_fn xe_ggtt_get_encode_pte_bo_fn(struct xe_ggtt *ggtt);
>  void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte);
>  
>  #endif
> diff --git a/drivers/gpu/drm/xe/xe_ggtt_types.h b/drivers/gpu/drm/xe/xe_ggtt_types.h
> index cb02b7994a9ac..c142ff59c4504 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt_types.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt_types.h
> @@ -13,6 +13,8 @@
>  struct xe_bo;
>  struct xe_gt;
>  
> +typedef u64 (*xe_ggtt_pte_encode_bo_fn)(struct xe_bo *bo, u64 bo_offset, u16 pat_index);
> +
>  /**
>   * struct xe_ggtt - Main GGTT struct
>   *
> @@ -69,15 +71,4 @@ struct xe_ggtt_node {
>  	bool invalidate_on_remove;
>  };
>  
> -/**
> - * struct xe_ggtt_pt_ops - GGTT Page table operations
> - * Which can vary from platform to platform.
> - */
> -struct xe_ggtt_pt_ops {
> -	/** @pte_encode_bo: Encode PTE address for a given BO */
> -	u64 (*pte_encode_bo)(struct xe_bo *bo, u64 bo_offset, u16 pat_index);
> -	/** @ggtt_set_pte: Directly write into GGTT's PTE */
> -	void (*ggtt_set_pte)(struct xe_ggtt *ggtt, u64 addr, u64 pte);
> -};
> -
>  #endif
> -- 
> 2.45.2
> 

Re: [PATCH v3 10/12] drm/xe: Make xe_ggtt_pt_ops private

[kernel test robot]

Hi Maarten,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-xe/drm-xe-next]
[also build test WARNING on linus/master v6.12-rc1]
[cannot apply to drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/drm-xe-display-Handle-stolen-bar-readout-in-the-same-way-as-lmem/20241004-000534
base:   https://gitlab.freedesktop.org/drm/xe/kernel.git drm-xe-next
patch link:    https://lore.kernel.org/r/20241003154421.33805-11-maarten.lankhorst%40linux.intel.com
patch subject: [PATCH v3 10/12] drm/xe: Make xe_ggtt_pt_ops private
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20241004/202410042102.EdHRO7JI-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241004/202410042102.EdHRO7JI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410042102.EdHRO7JI-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/xe/xe_ggtt.c:906: warning: expecting prototype for xe_ggtt_write_pte(). Prototype was for xe_ggtt_get_encode_pte_bo_fn() instead


vim +906 drivers/gpu/drm/xe/xe_ggtt.c

   897	
   898	/**
   899	 * xe_ggtt_write_pte - Write a PTE to the GGTT
   900	 * @ggtt: &xe_ggtt
   901	 * @offset: the offset for which the mapping should be written.
   902	 *
   903	 * Used by display for DPT and GGTT paths to enccode BO's.
   904	 */
   905	xe_ggtt_pte_encode_bo_fn xe_ggtt_get_encode_pte_bo_fn(struct xe_ggtt *ggtt)
 > 906	{

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 10/12] drm/xe: Make xe_ggtt_pt_ops private

[Lucas De Marchi]

On Thu, Oct 03, 2024 at 05:44:19PM +0200, Maarten Lankhorst wrote:
>The only user outside xe_ggtt.c is fb pinning, which makes sense as
>all the operations it performs can be considered part of GGTT.
>
>We could move this to xe_ggtt.c, but lets keep it inside display for
>now.
>
>Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>---
> drivers/gpu/drm/xe/display/xe_fb_pin.c | 22 ++++++++++++----------
> drivers/gpu/drm/xe/xe_ggtt.c           | 24 ++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_ggtt.h           |  2 ++
> drivers/gpu/drm/xe/xe_ggtt_types.h     | 13 ++-----------
> 4 files changed, 40 insertions(+), 21 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>index bddd526b33297..0ae5d917f20fe 100644
>--- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
>+++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>@@ -20,6 +20,7 @@ write_dpt_rotated(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs, u32 bo_
> {
> 	struct xe_device *xe = xe_bo_device(bo);
> 	struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
>+	xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);


ugh... that's awful. It's not private at all. You are just trading the
direct access ggtt->pt_ops->pte_encode_bo with a function that returns a
function pointer and you all it later.


Lucas De Marchi

> 	u32 column, row;
>
> 	/* TODO: Maybe rewrite so we can traverse the bo addresses sequentially,
>@@ -30,8 +31,8 @@ write_dpt_rotated(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs, u32 bo_
> 		u32 src_idx = src_stride * (height - 1) + column + bo_ofs;
>
> 		for (row = 0; row < height; row++) {
>-			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
>-							      xe->pat.idx[XE_CACHE_NONE]);
>+			u64 pte = pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
>+						xe->pat.idx[XE_CACHE_NONE]);
>
> 			iosys_map_wr(map, *dpt_ofs, u64, pte);
> 			*dpt_ofs += 8;
>@@ -53,8 +54,7 @@ write_dpt_remapped(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs,
> {
> 	struct xe_device *xe = xe_bo_device(bo);
> 	struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
>-	u64 (*pte_encode_bo)(struct xe_bo *bo, u64 bo_offset, u16 pat_index)
>-		= ggtt->pt_ops->pte_encode_bo;
>+	xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
> 	u32 column, row;
>
> 	for (row = 0; row < height; row++) {
>@@ -120,11 +120,12 @@ static int __xe_pin_fb_vma_dpt(const struct intel_framebuffer *fb,
> 		return PTR_ERR(dpt);
>
> 	if (view->type == I915_GTT_VIEW_NORMAL) {
>+		xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
> 		u32 x;
>
> 		for (x = 0; x < size / XE_PAGE_SIZE; x++) {
>-			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x * XE_PAGE_SIZE,
>-							      xe->pat.idx[XE_CACHE_NONE]);
>+			u64 pte = pte_encode_bo(bo, x * XE_PAGE_SIZE,
>+						xe->pat.idx[XE_CACHE_NONE]);
>
> 			iosys_map_wr(&dpt->vmap, x * 8, u64, pte);
> 		}
>@@ -163,14 +164,15 @@ write_ggtt_rotated(struct xe_bo *bo, struct xe_ggtt *ggtt, u32 *ggtt_ofs, u32 bo
> 		   u32 width, u32 height, u32 src_stride, u32 dst_stride)
> {
> 	struct xe_device *xe = xe_bo_device(bo);
>+	xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
> 	u32 column, row;
>
> 	for (column = 0; column < width; column++) {
> 		u32 src_idx = src_stride * (height - 1) + column + bo_ofs;
>
> 		for (row = 0; row < height; row++) {
>-			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
>-							      xe->pat.idx[XE_CACHE_NONE]);
>+			u64 pte = pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
>+						xe->pat.idx[XE_CACHE_NONE]);
>
> 			xe_ggtt_write_pte(ggtt, *ggtt_ofs, pte);
> 			*ggtt_ofs += XE_PAGE_SIZE;
>@@ -209,6 +211,7 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
> 		vma->node = bo->ggtt_node;
> 	} else if (view->type == I915_GTT_VIEW_NORMAL) {
> 		u32 x, size = bo->ttm.base.size;
>+		xe_ggtt_pte_encode_bo_fn pte_encode_bo = xe_ggtt_get_encode_pte_bo_fn(ggtt);
>
> 		vma->node = xe_ggtt_node_init(ggtt);
> 		if (IS_ERR(vma->node)) {
>@@ -223,8 +226,7 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
> 		}
>
> 		for (x = 0; x < size; x += XE_PAGE_SIZE) {
>-			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x,
>-							      xe->pat.idx[XE_CACHE_NONE]);
>+			u64 pte = pte_encode_bo(bo, x, xe->pat.idx[XE_CACHE_NONE]);
>
> 			xe_ggtt_write_pte(ggtt, vma->node->base.start + x, pte);
> 		}
>diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
>index 6945fbfc555ce..db6a761398064 100644
>--- a/drivers/gpu/drm/xe/xe_ggtt.c
>+++ b/drivers/gpu/drm/xe/xe_ggtt.c
>@@ -63,6 +63,18 @@
>  * 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_bo: Encode PTE address for a given BO */
>+	xe_ggtt_pte_encode_bo_fn pte_encode_bo;
>+	/** @ggtt_set_pte: Directly write into GGTT's PTE */
>+	void (*ggtt_set_pte)(struct xe_ggtt *ggtt, u64 addr, u64 pte);
>+};
>+
>+
> static u64 xelp_ggtt_pte_encode_bo(struct xe_bo *bo, u64 bo_offset,
> 				   u16 pat_index)
> {
>@@ -880,3 +892,15 @@ void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte)
> {
> 	return ggtt->pt_ops->ggtt_set_pte(ggtt, offset, pte);
> }
>+
>+/**
>+ * xe_ggtt_write_pte - Write a PTE to the GGTT


copy & pasta

I don't think we should have this partial transition. If we want to hide
the pointer indirection we should provide functions like
xe_ggtt_pte_encode_bo() and xe_ggtt_set_pte() instead of adding a getter
to return the function pointer.

Lucas De Marchi

>+ * @ggtt: &xe_ggtt
>+ * @offset: the offset for which the mapping should be written.
>+ *
>+ * Used by display for DPT and GGTT paths to enccode BO's.
>+ */
>+xe_ggtt_pte_encode_bo_fn xe_ggtt_get_encode_pte_bo_fn(struct xe_ggtt *ggtt)
>+{
>+	return ggtt->pt_ops->pte_encode_bo;
>+}
>diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
>index f83e5af0400e9..0c63cfa083c03 100644
>--- a/drivers/gpu/drm/xe/xe_ggtt.h
>+++ b/drivers/gpu/drm/xe/xe_ggtt.h
>@@ -48,6 +48,8 @@ void xe_ggtt_might_lock(struct xe_ggtt *ggtt);
> #endif
>
> u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset);
>+
>+xe_ggtt_pte_encode_bo_fn xe_ggtt_get_encode_pte_bo_fn(struct xe_ggtt *ggtt);
> void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte);
>
> #endif
>diff --git a/drivers/gpu/drm/xe/xe_ggtt_types.h b/drivers/gpu/drm/xe/xe_ggtt_types.h
>index cb02b7994a9ac..c142ff59c4504 100644
>--- a/drivers/gpu/drm/xe/xe_ggtt_types.h
>+++ b/drivers/gpu/drm/xe/xe_ggtt_types.h
>@@ -13,6 +13,8 @@
> struct xe_bo;
> struct xe_gt;
>
>+typedef u64 (*xe_ggtt_pte_encode_bo_fn)(struct xe_bo *bo, u64 bo_offset, u16 pat_index);
>+
> /**
>  * struct xe_ggtt - Main GGTT struct
>  *
>@@ -69,15 +71,4 @@ struct xe_ggtt_node {
> 	bool invalidate_on_remove;
> };
>
>-/**
>- * struct xe_ggtt_pt_ops - GGTT Page table operations
>- * Which can vary from platform to platform.
>- */
>-struct xe_ggtt_pt_ops {
>-	/** @pte_encode_bo: Encode PTE address for a given BO */
>-	u64 (*pte_encode_bo)(struct xe_bo *bo, u64 bo_offset, u16 pat_index);
>-	/** @ggtt_set_pte: Directly write into GGTT's PTE */
>-	void (*ggtt_set_pte)(struct xe_ggtt *ggtt, u64 addr, u64 pte);
>-};
>-
> #endif
>-- 
>2.45.2
>

[PATCH v3 11/12] drm/xe/display: Stop dereferencing ggtt in xe_fb_pin

[Maarten Lankhorst]

This is the only user of the ggtt struct still there, add
some calls to lock/unlock ggtt and remove other dereferencing.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/xe/display/xe_fb_pin.c | 10 +++++-----
 drivers/gpu/drm/xe/xe_ggtt.c           | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_ggtt.h           | 10 ++++++----
 3 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
index 0ae5d917f20fe..fcd9a519183b5 100644
--- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
+++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
@@ -198,13 +198,13 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
 	/* TODO: Consider sharing framebuffer mapping?
 	 * embed i915_vma inside intel_framebuffer
 	 */
-	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
-	ret = mutex_lock_interruptible(&ggtt->lock);
+	xe_pm_runtime_get_noresume(xe);
+	ret = xe_ggtt_lock_interruptible(ggtt);
 	if (ret)
 		goto out;
 
 	align = XE_PAGE_SIZE;
-	if (xe_bo_is_vram(bo) && ggtt->flags & XE_GGTT_FLAGS_64K)
+	if (xe_bo_is_vram(bo) && xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)
 		align = max_t(u32, align, SZ_64K);
 
 	if (bo->ggtt_node && view->type == I915_GTT_VIEW_NORMAL) {
@@ -261,9 +261,9 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
 	}
 
 out_unlock:
-	mutex_unlock(&ggtt->lock);
+	xe_ggtt_unlock(ggtt);
 out:
-	xe_pm_runtime_put(tile_to_xe(ggtt->tile));
+	xe_pm_runtime_put(xe);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
index db6a761398064..9c4baa22ebe49 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.c
+++ b/drivers/gpu/drm/xe/xe_ggtt.c
@@ -868,6 +868,30 @@ u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer
 	return total;
 }
 
+/**
+ * xe_ggtt_lock_interruptible - Lock GGTT for display updates
+ * @ggtt: &xe_ggtt
+ *
+ * There's no reason to want this outside of display, and that is only
+ * there because moving to here would be a layering violation.
+ */
+int xe_ggtt_lock_interruptible(struct xe_ggtt *ggtt)
+{
+	return mutex_lock_interruptible(&ggtt->lock);
+}
+
+/**
+ * xe_ggtt_unlock - Unlock GGTT after display updates
+ * @ggtt: &xe_ggtt
+ *
+ * There's no reason to want this outside of display, and that is only
+ * there because moving to here would be a layering violation.
+ */
+void xe_ggtt_unlock(struct xe_ggtt *ggtt)
+{
+	mutex_unlock(&ggtt->lock);
+}
+
 /**
  * xe_ggtt_read_pte - Read a PTE from the GGTT
  * @ggtt: &xe_ggtt
diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
index 0c63cfa083c03..09bb1c9c0a743 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.h
+++ b/drivers/gpu/drm/xe/xe_ggtt.h
@@ -22,8 +22,6 @@ int xe_ggtt_node_insert_balloon(struct xe_ggtt_node *node,
 void xe_ggtt_node_remove_balloon(struct xe_ggtt_node *node);
 
 int xe_ggtt_node_insert(struct xe_ggtt_node *node, u32 size, u32 align);
-int xe_ggtt_node_insert_locked(struct xe_ggtt_node *node,
-			       u32 size, u32 align, u32 mm_flags);
 void xe_ggtt_node_remove(struct xe_ggtt_node *node, bool invalidate);
 bool xe_ggtt_node_allocated(const struct xe_ggtt_node *node);
 void xe_ggtt_map_bo_unlocked(struct xe_ggtt *ggtt, struct xe_bo *bo);
@@ -47,9 +45,13 @@ static inline void xe_ggtt_might_lock(struct xe_ggtt *ggtt)
 void xe_ggtt_might_lock(struct xe_ggtt *ggtt);
 #endif
 
-u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset);
-
+/* Display specific function calls, don't use outside of xe/display */
+int xe_ggtt_lock_interruptible(struct xe_ggtt *ggtt);
+void xe_ggtt_unlock(struct xe_ggtt *ggtt);
+int xe_ggtt_node_insert_locked(struct xe_ggtt_node *node,
+			       u32 size, u32 align, u32 mm_flags);
 xe_ggtt_pte_encode_bo_fn xe_ggtt_get_encode_pte_bo_fn(struct xe_ggtt *ggtt);
+u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset);
 void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte);
 
 #endif
-- 
2.45.2

Re: [PATCH v3 11/12] drm/xe/display: Stop dereferencing ggtt in xe_fb_pin

[Matthew Brost]

On Thu, Oct 03, 2024 at 05:44:20PM +0200, Maarten Lankhorst wrote:
> This is the only user of the ggtt struct still there, add
> some calls to lock/unlock ggtt and remove other dereferencing.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

> ---
>  drivers/gpu/drm/xe/display/xe_fb_pin.c | 10 +++++-----
>  drivers/gpu/drm/xe/xe_ggtt.c           | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_ggtt.h           | 10 ++++++----
>  3 files changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> index 0ae5d917f20fe..fcd9a519183b5 100644
> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> @@ -198,13 +198,13 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
>  	/* TODO: Consider sharing framebuffer mapping?
>  	 * embed i915_vma inside intel_framebuffer
>  	 */
> -	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
> -	ret = mutex_lock_interruptible(&ggtt->lock);
> +	xe_pm_runtime_get_noresume(xe);
> +	ret = xe_ggtt_lock_interruptible(ggtt);
>  	if (ret)
>  		goto out;
>  
>  	align = XE_PAGE_SIZE;
> -	if (xe_bo_is_vram(bo) && ggtt->flags & XE_GGTT_FLAGS_64K)
> +	if (xe_bo_is_vram(bo) && xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)
>  		align = max_t(u32, align, SZ_64K);
>  
>  	if (bo->ggtt_node && view->type == I915_GTT_VIEW_NORMAL) {
> @@ -261,9 +261,9 @@ static int __xe_pin_fb_vma_ggtt(const struct intel_framebuffer *fb,
>  	}
>  
>  out_unlock:
> -	mutex_unlock(&ggtt->lock);
> +	xe_ggtt_unlock(ggtt);
>  out:
> -	xe_pm_runtime_put(tile_to_xe(ggtt->tile));
> +	xe_pm_runtime_put(xe);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index db6a761398064..9c4baa22ebe49 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -868,6 +868,30 @@ u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer
>  	return total;
>  }
>  
> +/**
> + * xe_ggtt_lock_interruptible - Lock GGTT for display updates
> + * @ggtt: &xe_ggtt
> + *
> + * There's no reason to want this outside of display, and that is only
> + * there because moving to here would be a layering violation.
> + */
> +int xe_ggtt_lock_interruptible(struct xe_ggtt *ggtt)
> +{
> +	return mutex_lock_interruptible(&ggtt->lock);
> +}
> +
> +/**
> + * xe_ggtt_unlock - Unlock GGTT after display updates
> + * @ggtt: &xe_ggtt
> + *
> + * There's no reason to want this outside of display, and that is only
> + * there because moving to here would be a layering violation.
> + */
> +void xe_ggtt_unlock(struct xe_ggtt *ggtt)
> +{
> +	mutex_unlock(&ggtt->lock);
> +}
> +
>  /**
>   * xe_ggtt_read_pte - Read a PTE from the GGTT
>   * @ggtt: &xe_ggtt
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index 0c63cfa083c03..09bb1c9c0a743 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -22,8 +22,6 @@ int xe_ggtt_node_insert_balloon(struct xe_ggtt_node *node,
>  void xe_ggtt_node_remove_balloon(struct xe_ggtt_node *node);
>  
>  int xe_ggtt_node_insert(struct xe_ggtt_node *node, u32 size, u32 align);
> -int xe_ggtt_node_insert_locked(struct xe_ggtt_node *node,
> -			       u32 size, u32 align, u32 mm_flags);
>  void xe_ggtt_node_remove(struct xe_ggtt_node *node, bool invalidate);
>  bool xe_ggtt_node_allocated(const struct xe_ggtt_node *node);
>  void xe_ggtt_map_bo_unlocked(struct xe_ggtt *ggtt, struct xe_bo *bo);
> @@ -47,9 +45,13 @@ static inline void xe_ggtt_might_lock(struct xe_ggtt *ggtt)
>  void xe_ggtt_might_lock(struct xe_ggtt *ggtt);
>  #endif
>  
> -u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset);
> -
> +/* Display specific function calls, don't use outside of xe/display */
> +int xe_ggtt_lock_interruptible(struct xe_ggtt *ggtt);
> +void xe_ggtt_unlock(struct xe_ggtt *ggtt);
> +int xe_ggtt_node_insert_locked(struct xe_ggtt_node *node,
> +			       u32 size, u32 align, u32 mm_flags);
>  xe_ggtt_pte_encode_bo_fn xe_ggtt_get_encode_pte_bo_fn(struct xe_ggtt *ggtt);
> +u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset);
>  void xe_ggtt_write_pte(struct xe_ggtt *ggtt, u64 offset, u64 pte);
>  
>  #endif
> -- 
> 2.45.2
> 

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

[Maarten Lankhorst]

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>
---
 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

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

[Matthew Brost]

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
> 

✗ Fi.CI.BUILD: failure for drm/xe: Reduce flickering when inheriting BIOS fb.

[Patchwork]

== Series Details ==

Series: drm/xe: Reduce flickering when inheriting BIOS fb.
URL   : https://patchwork.freedesktop.org/series/139517/
State : failure

== Summary ==

Error: make failed
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/gpu/drm/i915/display/skl_universal_plane.o
drivers/gpu/drm/i915/display/skl_universal_plane.c: In function ‘skl_fixup_initial_plane_config’:
drivers/gpu/drm/i915/display/skl_universal_plane.c:2818:21: error: implicit declaration of function ‘intel_read’; did you mean ‘intel_de_read’? [-Werror=implicit-function-declaration]
 2818 |         plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id));
      |                     ^~~~~~~~~~
      |                     intel_de_read
cc1: all warnings being treated as errors
make[6]: *** [scripts/Makefile.build:229: drivers/gpu/drm/i915/display/skl_universal_plane.o] Error 1
make[5]: *** [scripts/Makefile.build:478: drivers/gpu/drm/i915] Error 2
make[4]: *** [scripts/Makefile.build:478: drivers/gpu/drm] Error 2
make[3]: *** [scripts/Makefile.build:478: drivers/gpu] Error 2
make[2]: *** [scripts/Makefile.build:478: drivers] Error 2
make[1]: *** [/home/kbuild2/kernel/Makefile:1936: .] Error 2
make: *** [Makefile:224: __sub-make] Error 2
Build failed, no error log produced

✗ Fi.CI.BUILD: failure for drm/xe: Reduce flickering when inheriting BIOS fb. (rev2)

[Patchwork]

== Series Details ==

Series: drm/xe: Reduce flickering when inheriting BIOS fb. (rev2)
URL   : https://patchwork.freedesktop.org/series/139517/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/139517/revisions/2/mbox/ not applied
Applying: drm/xe/display: Handle stolen bar readout in the same way as lmem
Applying: drm/xe: Remove double pageflip
error: sha1 information is lacking or useless (drivers/gpu/drm/xe/display/xe_plane_initial.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 drm/xe: Remove double pageflip
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced