Re: [PATCH v2 1/3] drm/xe: Align all 64k VRAM buffers physically when multiple of 64k.

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi, Maarten

On Wed, 2024-08-21 at 22:56 +0200, Maarten Lankhorst wrote:
> For CCS formats on affected platforms, CCS can be used freely, but
> display engine requires a multiple of 64k physical pages. No other
> changes are needed.
> 
> At the BO creation time we don't know if the BO will be used for CCS
> or not. If the scanout flag is set, and the BO is a multiple of 64k,
> we take the safe route and force the physical alignment of 64k pages.
> 
> If the BO is not a multiple of 64k, or the scanout flag was not set
> at BO creation, we reject it for usage as CCS in display. The
> physical
> pages are likely not aligned correctly, and this will cause
> corruption
> when used as FB.
> 
> This is a slightly different approach from my previous patch. Instead
> of requiring a scanout flag at FB creation, we now make all buffers
> of
> the right size physically aligned correctly, so no change from
> userspace
> is needed.
> 
> It will be interesting to see if it affects performance in any way,
> could potentially even improve things with 64k PTE's.
> 
> Inspired by Zbigniews patch.

I'm still concerned about this patch, since I think we should restrict
the 64K contigous requirement to scanout bos only, and IMO it's better
to completely understand the case where we need to implicitly promote
to scanout so that UMDs do not need to do that for every bo.

The worry is that *if* fragmentation comes to play, we no longer have
the option to go back to scanout bos only, since that would break UAPI
and the only viable option would then be to require 64K minimum
alignment for all bos.

/Thomas


> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Co-developed-by: Zbigniew Kempczyński
> <zbigniew.kempczynski@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Juha-Pekka Heikkilä <juha-pekka.heikkila@intel.com>
> ---
>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  6 ++++++
>  drivers/gpu/drm/xe/xe_bo.c               | 10 ++++++++++
>  drivers/gpu/drm/xe/xe_device_types.h     |  1 +
>  drivers/gpu/drm/xe/xe_vm.c               |  3 ++-
>  4 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/intel_fb_bo.c
> b/drivers/gpu/drm/xe/display/intel_fb_bo.c
> index f835492f73fb4..407367719abe2 100644
> --- a/drivers/gpu/drm/xe/display/intel_fb_bo.c
> +++ b/drivers/gpu/drm/xe/display/intel_fb_bo.c
> @@ -7,6 +7,7 @@
>  #include <drm/ttm/ttm_bo.h>
>  
>  #include "intel_display_types.h"
> +#include "intel_fb.h"
>  #include "intel_fb_bo.h"
>  #include "xe_bo.h"
>  
> @@ -28,6 +29,11 @@ int intel_fb_bo_framebuffer_init(struct
> intel_framebuffer *intel_fb,
>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
>  	int ret;
>  
> +	if (XE_IOCTL_DBG(xe, intel_fb_is_ccs_modifier(mode_cmd-
> >modifier[0]) &&
> +			     (xe->info.vram_flags &
> XE_VRAM_FLAGS_DISPLAY_NEED64K_CCS) &&
> +			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
> +		return -EINVAL;
> +
>  	xe_bo_get(bo);
>  
>  	ret = ttm_bo_reserve(&bo->ttm, true, false, NULL);
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 6ed0e19552159..3a753f4644cb6 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -2017,6 +2017,16 @@ int xe_gem_create_ioctl(struct drm_device
> *dev, void *data,
>  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_SCANOUT)
>  		bo_flags |= XE_BO_FLAG_SCANOUT;
>  
> +	/*
> +	 * Lets see what happens if we simply align any buffer
> that's
> +	 * a multiple of 64k to 64k in places where it's not
> officially
> +	 * needed.
> +	 */
> +	if ((bo_flags & XE_BO_FLAG_VRAM_MASK) &&
> +	    !(xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K) &&
> +	    !(args->size % SZ_64K))
> +		bo_flags |= XE_BO_FLAG_NEEDS_64K;
> +
>  	bo_flags |= args->placement << (ffs(XE_BO_FLAG_SYSTEM) - 1);
>  
>  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM)
> {
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> b/drivers/gpu/drm/xe/xe_device_types.h
> index 5ed6f5434f42c..12ddab91a01c0 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -47,6 +47,7 @@ struct xe_pat_ops;
>  #define HAS_HECI_CSCFI(xe) ((xe)->info.has_heci_cscfi)
>  
>  #define XE_VRAM_FLAGS_NEED64K		BIT(0)
> +#define XE_VRAM_FLAGS_DISPLAY_NEED64K_CCS	BIT(1)
>  
>  #define XE_GT0		0
>  #define XE_GT1		1
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index d1bfd0b6e9558..af215f6d6588b 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2878,7 +2878,8 @@ static int xe_vm_bind_ioctl_validate_bo(struct
> xe_device *xe, struct xe_bo *bo,
>  		return -EINVAL;
>  	}
>  
> -	if (bo->flags & XE_BO_FLAG_INTERNAL_64K) {
> +	if ((bo->flags & XE_BO_FLAG_INTERNAL_64K) &&
> +	    (xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)) {
>  		if (XE_IOCTL_DBG(xe, obj_offset &
>  				 XE_64K_PAGE_MASK) ||
>  		    XE_IOCTL_DBG(xe, addr & XE_64K_PAGE_MASK) ||