[PATCH v1 1/1] drm/xe/bo: Fix fixed placement ggtt pinning code

[PATCH v1 1/1] drm/xe/bo: Fix fixed placement ggtt pinning code

[Alan Previn]

When calling xe_bo_create_pin_map_at, use the correct
starting offset provided by caller at xe_ggtt_insert_bo_at.

Fixes: 44e694958b95 ("drm/xe/display: Implement display support")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/xe/xe_bo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
index 74294f1b05bc..cc6101b49c08 100644
--- a/drivers/gpu/drm/xe/xe_bo.c
+++ b/drivers/gpu/drm/xe/xe_bo.c
@@ -1436,7 +1436,7 @@ __xe_bo_create_locked(struct xe_device *xe,
 
 		if (flags & XE_BO_FLAG_FIXED_PLACEMENT) {
 			err = xe_ggtt_insert_bo_at(tile->mem.ggtt, bo,
-						   start + bo->size, U64_MAX);
+						   start, end);
 		} else {
 			err = xe_ggtt_insert_bo(tile->mem.ggtt, bo);
 		}

base-commit: cffd77865f476994680892601e09bc2164179907
-- 
2.34.1

[PATCH v1 1/1] drm/xe/bo: Fix fixed placement ggtt pinning code

[Alan Previn]

When calling xe_bo_create_pin_map_at, use the correct
starting offset provided by caller at xe_ggtt_insert_bo_at.

Fixes: 44e694958b95 ("drm/xe/display: Implement display support")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/xe/xe_bo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
index 74294f1b05bc..cc6101b49c08 100644
--- a/drivers/gpu/drm/xe/xe_bo.c
+++ b/drivers/gpu/drm/xe/xe_bo.c
@@ -1436,7 +1436,7 @@ __xe_bo_create_locked(struct xe_device *xe,
 
 		if (flags & XE_BO_FLAG_FIXED_PLACEMENT) {
 			err = xe_ggtt_insert_bo_at(tile->mem.ggtt, bo,
-						   start + bo->size, U64_MAX);
+						   start, end);
 		} else {
 			err = xe_ggtt_insert_bo(tile->mem.ggtt, bo);
 		}

base-commit: cffd77865f476994680892601e09bc2164179907
-- 
2.34.1

Re: [PATCH v1 1/1] drm/xe/bo: Fix fixed placement ggtt pinning code

[Matthew Auld]

On 21/06/2024 08:15, Alan Previn wrote:
> When calling xe_bo_create_pin_map_at, use the correct
> starting offset provided by caller at xe_ggtt_insert_bo_at.
> 
> Fixes: 44e694958b95 ("drm/xe/display: Implement display support")
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_bo.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 74294f1b05bc..cc6101b49c08 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1436,7 +1436,7 @@ __xe_bo_create_locked(struct xe_device *xe,
>   
>   		if (flags & XE_BO_FLAG_FIXED_PLACEMENT) {
>   			err = xe_ggtt_insert_bo_at(tile->mem.ggtt, bo,
> -						   start + bo->size, U64_MAX);
> +						   start, end);


I think this is just about controlling the ggtt bias range. i.e please 
allocate bo->size somewhere within [start + bo->size, U64_MAX]. There 
was a reason for the + bo->size, I think something odd like initial fb 
using low ggtt address (pre-programmed by bios), and wanting to use that 
for guc or something instead so we effectively move it with the above.

I think we also only need such an interface for funky stuff like initial 
fb, which is either in stolen or vram. The actual allocate backing pages 
part of the api should respect start, like for vram and stolen, which is 
the main point of FIXED_PLACEMENT. Trying to do that for system memory 
is not really possible AFAIK.

If we just want something with specific ggtt alignment we could plumb 
that through to the mm interface. For example, put this anywhere within 
ggtt but ensure address is aligned to 256M. We then only pass FLAG_GGTT 
and not FIXED_PLACEMENT.

>   		} else {
>   			err = xe_ggtt_insert_bo(tile->mem.ggtt, bo);
>   		}
> 
> base-commit: cffd77865f476994680892601e09bc2164179907

Re: [PATCH v1 1/1] drm/xe/bo: Fix fixed placement ggtt pinning code

[Rodrigo Vivi]

On Fri, Jun 21, 2024 at 12:15:06AM -0700, Alan Previn wrote:
> When calling xe_bo_create_pin_map_at, use the correct
> starting offset provided by caller at xe_ggtt_insert_bo_at.
> 
> Fixes: 44e694958b95 ("drm/xe/display: Implement display support")
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_bo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 74294f1b05bc..cc6101b49c08 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1436,7 +1436,7 @@ __xe_bo_create_locked(struct xe_device *xe,
>  
>  		if (flags & XE_BO_FLAG_FIXED_PLACEMENT) {
>  			err = xe_ggtt_insert_bo_at(tile->mem.ggtt, bo,
> -						   start + bo->size, U64_MAX);
> +						   start, end);

interesting! o.O

looking up to the initial_plane_bo and down to the drm_mm_insert node
I don't believe this ever worked, right?! We were never reusing the
plane and just getting the blank screen on the start until it got
recreated?

is there any link to any open issue or history?

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>  		} else {
>  			err = xe_ggtt_insert_bo(tile->mem.ggtt, bo);
>  		}
> 
> base-commit: cffd77865f476994680892601e09bc2164179907
> -- 
> 2.34.1
>