[v6,2/2] drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.

Commit Message

Maarten Lankhorst Aug. 26, 2024, 5:01 p.m. UTC

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.

The scanout flag and size being a multiple of 64k are used together
to enforce 64k physical placement.

VM_BIND is completely unaffected, mappings to a VM can still be aligned
to 4k, just like for normal buffers.

Signed-off-by: Zbigniew Kempczyński <[e-mail]>
Signed-off-by: Maarten Lankhorst <[e-mail]>
Cc: Matthew Auld <[e-mail]>
Cc: Rodrigo Vivi <[e-mail]>
Cc: Thomas Hellström <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Juha-Pekka Heikkilä <[e-mail]>
---
 drivers/gpu/drm/xe/display/intel_fb_bo.c |  9 +++++++++
 drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
 drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)

Comments

Matthew Brost Aug. 26, 2024, 7:30 p.m. UTC | #1

On Mon, Aug 26, 2024 at 07:01:16PM +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.
> 
> The scanout flag and size being a multiple of 64k are used together
> to enforce 64k physical placement.
> 
> VM_BIND is completely unaffected, mappings to a VM can still be aligned
> to 4k, just like for normal buffers.
> 
> Signed-off-by: Zbigniew Kempczyński <[e-mail]>
> Signed-off-by: Maarten Lankhorst <[e-mail]>
> Cc: Matthew Auld <[e-mail]>
> Cc: Rodrigo Vivi <[e-mail]>
> Cc: Thomas Hellström <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Juha-Pekka Heikkilä <[e-mail]>
> ---
>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  9 +++++++++
>  drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
>  drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
>  3 files changed, 26 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..63ce97cc4cfef 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,14 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
>  	int ret;
>  
> +	/*
> +	 * Some modifiers require physical alignment of 64KiB VRAM pages;
> +	 * require that the BO in those cases is created correctly.
> +	 */
> +	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys(mode_cmd->modifier[0]) &&
> +			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
> +		return -EINVAL;

I don't think this is correct use of this macro as XE_BO_FLAG_NEEDS_64K
is an internal flag we set and typically this macro is used to santize
user input. An assert here or WARN would make more sense.

Matt

> +
>  	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 cbe7bf098970f..9d6632f92fa94 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -2019,6 +2019,13 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
>  
>  	bo_flags |= args->placement << (ffs(XE_BO_FLAG_SYSTEM) - 1);
>  
> +	/* CCS formats need physical placement at a 64K alignment in VRAM. */
> +	if ((bo_flags & XE_BO_FLAG_VRAM_MASK) &&
> +	    (bo_flags & XE_BO_FLAG_SCANOUT) &&
> +	    !(xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K) &&
> +	    IS_ALIGNED(args->size, SZ_64K))
> +		bo_flags |= XE_BO_FLAG_NEEDS_64K;
> +
>  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM) {
>  		if (XE_IOCTL_DBG(xe, !(bo_flags & XE_BO_FLAG_VRAM_MASK)))
>  			return -EINVAL;
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 4cc13eddb6b32..3eb76d874eb28 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2878,7 +2878,16 @@ 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) {
> +	/*
> +	 * Some platforms require 64k VM_BIND alignment,
> +	 * specifically those with XE_VRAM_FLAGS_NEED64K.
> +	 *
> +	 * Other platforms may have BO's set to 64k physical placement,
> +	 * but can be mapped at 4k offsets anyway. This check is only
> +	 * there for the former case.
> +	 */
> +	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) ||
> -- 
> 2.45.2
>

Maarten Lankhorst Aug. 26, 2024, 7:42 p.m. UTC | #2

Hey,

Den 2024-08-26 kl. 21:30, skrev Matthew Brost:
> On Mon, Aug 26, 2024 at 07:01:16PM +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.
>>
>> The scanout flag and size being a multiple of 64k are used together
>> to enforce 64k physical placement.
>>
>> VM_BIND is completely unaffected, mappings to a VM can still be aligned
>> to 4k, just like for normal buffers.
>>
>> Signed-off-by: Zbigniew Kempczyński <[e-mail]>
>> Signed-off-by: Maarten Lankhorst <[e-mail]>
>> Cc: Matthew Auld <[e-mail]>
>> Cc: Rodrigo Vivi <[e-mail]>
>> Cc: Thomas Hellström <[e-mail]>
>> Cc: Maarten Lankhorst <[e-mail]>
>> Cc: Juha-Pekka Heikkilä <[e-mail]>
>> ---
>>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  9 +++++++++
>>  drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
>>  drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
>>  3 files changed, 26 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..63ce97cc4cfef 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,14 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
>>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
>>  	int ret;
>>  
>> +	/*
>> +	 * Some modifiers require physical alignment of 64KiB VRAM pages;
>> +	 * require that the BO in those cases is created correctly.
>> +	 */
>> +	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys(mode_cmd->modifier[0]) &&
>> +			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
>> +		return -EINVAL;
> 
> I don't think this is correct use of this macro as XE_BO_FLAG_NEEDS_64K
> is an internal flag we set and typically this macro is used to santize
> user input. An assert here or WARN would make more sense.
Ideally we'd use 'is bo created as scanout', but that flag can be set by fb_init too, so if the BO was used for normal 4-tiled before, then as CCS it would pass when it wouldn't be valid.

I could change it to bo_created_with_scanout_flag_on_64k_platform inline, but I doubt that's more readable. :)

Cheers,
~Maarten

Matthew Brost Aug. 27, 2024, 3:11 a.m. UTC | #3

On Mon, Aug 26, 2024 at 09:42:54PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Den 2024-08-26 kl. 21:30, skrev Matthew Brost:
> > On Mon, Aug 26, 2024 at 07:01:16PM +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.
> >>
> >> The scanout flag and size being a multiple of 64k are used together
> >> to enforce 64k physical placement.
> >>
> >> VM_BIND is completely unaffected, mappings to a VM can still be aligned
> >> to 4k, just like for normal buffers.
> >>
> >> Signed-off-by: Zbigniew Kempczyński <[e-mail]>
> >> Signed-off-by: Maarten Lankhorst <[e-mail]>
> >> Cc: Matthew Auld <[e-mail]>
> >> Cc: Rodrigo Vivi <[e-mail]>
> >> Cc: Thomas Hellström <[e-mail]>
> >> Cc: Maarten Lankhorst <[e-mail]>
> >> Cc: Juha-Pekka Heikkilä <[e-mail]>
> >> ---
> >>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  9 +++++++++
> >>  drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
> >>  drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
> >>  3 files changed, 26 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..63ce97cc4cfef 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,14 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
> >>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
> >>  	int ret;
> >>  
> >> +	/*
> >> +	 * Some modifiers require physical alignment of 64KiB VRAM pages;
> >> +	 * require that the BO in those cases is created correctly.
> >> +	 */
> >> +	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys(mode_cmd->modifier[0]) &&
> >> +			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
> >> +		return -EINVAL;
> > 
> > I don't think this is correct use of this macro as XE_BO_FLAG_NEEDS_64K
> > is an internal flag we set and typically this macro is used to santize
> > user input. An assert here or WARN would make more sense.
> Ideally we'd use 'is bo created as scanout', but that flag can be set by fb_init too, so if the BO was used for normal 4-tiled before, then as CCS it would pass when it wouldn't be valid.
> 
> I could change it to bo_created_with_scanout_flag_on_64k_platform inline, but I doubt that's more readable. :)
> 

Not trying to block the patch and really don't know anything about
display but still think XE_IOCTL_DBG should replaced by either an
assert or WARN (or Xe flavor of warn). Kinda pedantic but we really are
trying hard to uniformly use these types of macros and this just doesn't
look correct.

Matt

> Cheers,
> ~Maarten

Maarten Lankhorst Aug. 27, 2024, 6:43 a.m. UTC | #4

Hey,

Den 2024-08-27 kl. 05:11, skrev Matthew Brost:
> On Mon, Aug 26, 2024 at 09:42:54PM +0200, Maarten Lankhorst wrote:
>> Hey,
>>
>> Den 2024-08-26 kl. 21:30, skrev Matthew Brost:
>>> On Mon, Aug 26, 2024 at 07:01:16PM +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.
>>>>
>>>> The scanout flag and size being a multiple of 64k are used together
>>>> to enforce 64k physical placement.
>>>>
>>>> VM_BIND is completely unaffected, mappings to a VM can still be aligned
>>>> to 4k, just like for normal buffers.
>>>>
>>>> Signed-off-by: Zbigniew Kempczyński <[e-mail]>
>>>> Signed-off-by: Maarten Lankhorst <[e-mail]>
>>>> Cc: Matthew Auld <[e-mail]>
>>>> Cc: Rodrigo Vivi <[e-mail]>
>>>> Cc: Thomas Hellström <[e-mail]>
>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>> Cc: Juha-Pekka Heikkilä <[e-mail]>
>>>> ---
>>>>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  9 +++++++++
>>>>  drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
>>>>  drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
>>>>  3 files changed, 26 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..63ce97cc4cfef 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,14 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
>>>>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
>>>>  	int ret;
>>>>  
>>>> +	/*
>>>> +	 * Some modifiers require physical alignment of 64KiB VRAM pages;
>>>> +	 * require that the BO in those cases is created correctly.
>>>> +	 */
>>>> +	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys(mode_cmd->modifier[0]) &&
>>>> +			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
>>>> +		return -EINVAL;
>>>
>>> I don't think this is correct use of this macro as XE_BO_FLAG_NEEDS_64K
>>> is an internal flag we set and typically this macro is used to santize
>>> user input. An assert here or WARN would make more sense.
>> Ideally we'd use 'is bo created as scanout', but that flag can be set by fb_init too, so if the BO was used for normal 4-tiled before, then as CCS it would pass when it wouldn't be valid.
>>
>> I could change it to bo_created_with_scanout_flag_on_64k_platform inline, but I doubt that's more readable. :)
>>
> 
> Not trying to block the patch and really don't know anything about
> display but still think XE_IOCTL_DBG should replaced by either an
> assert or WARN (or Xe flavor of warn). Kinda pedantic but we really are
> trying hard to uniformly use these types of macros and this just doesn't
> look correct.

mode_cmd->modifier[0] is passed from userspace without validation, and this function is called very early on in fb creation. Anything more than XE_IOCTL_DBG would be invalid here.

Matthew Brost Aug. 27, 2024, 4 p.m. UTC | #5

On Tue, Aug 27, 2024 at 08:43:36AM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Den 2024-08-27 kl. 05:11, skrev Matthew Brost:
> > On Mon, Aug 26, 2024 at 09:42:54PM +0200, Maarten Lankhorst wrote:
> >> Hey,
> >>
> >> Den 2024-08-26 kl. 21:30, skrev Matthew Brost:
> >>> On Mon, Aug 26, 2024 at 07:01:16PM +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.
> >>>>
> >>>> The scanout flag and size being a multiple of 64k are used together
> >>>> to enforce 64k physical placement.
> >>>>
> >>>> VM_BIND is completely unaffected, mappings to a VM can still be aligned
> >>>> to 4k, just like for normal buffers.
> >>>>
> >>>> Signed-off-by: Zbigniew Kempczyński <[e-mail]>
> >>>> Signed-off-by: Maarten Lankhorst <[e-mail]>
> >>>> Cc: Matthew Auld <[e-mail]>
> >>>> Cc: Rodrigo Vivi <[e-mail]>
> >>>> Cc: Thomas Hellström <[e-mail]>
> >>>> Cc: Maarten Lankhorst <[e-mail]>
> >>>> Cc: Juha-Pekka Heikkilä <[e-mail]>
> >>>> ---
> >>>>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  9 +++++++++
> >>>>  drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
> >>>>  drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
> >>>>  3 files changed, 26 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..63ce97cc4cfef 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,14 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
> >>>>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
> >>>>  	int ret;
> >>>>  
> >>>> +	/*
> >>>> +	 * Some modifiers require physical alignment of 64KiB VRAM pages;
> >>>> +	 * require that the BO in those cases is created correctly.
> >>>> +	 */
> >>>> +	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys(mode_cmd->modifier[0]) &&
> >>>> +			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
> >>>> +		return -EINVAL;
> >>>
> >>> I don't think this is correct use of this macro as XE_BO_FLAG_NEEDS_64K
> >>> is an internal flag we set and typically this macro is used to santize
> >>> user input. An assert here or WARN would make more sense.
> >> Ideally we'd use 'is bo created as scanout', but that flag can be set by fb_init too, so if the BO was used for normal 4-tiled before, then as CCS it would pass when it wouldn't be valid.
> >>
> >> I could change it to bo_created_with_scanout_flag_on_64k_platform inline, but I doubt that's more readable. :)
> >>
> > 
> > Not trying to block the patch and really don't know anything about
> > display but still think XE_IOCTL_DBG should replaced by either an
> > assert or WARN (or Xe flavor of warn). Kinda pedantic but we really are
> > trying hard to uniformly use these types of macros and this just doesn't
> > look correct.
> 
> mode_cmd->modifier[0] is passed from userspace without validation, and this function is called very early on in fb creation. Anything more than XE_IOCTL_DBG would be invalid here.

Ok, that makes this usage more clear then. Fine with it as is then. Sorry for the noise.

Matt

Rodrigo Vivi Aug. 27, 2024, 4:23 p.m. UTC | #6

On Mon, Aug 26, 2024 at 07:01:16PM +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.
> 
> The scanout flag and size being a multiple of 64k are used together
> to enforce 64k physical placement.
> 
> VM_BIND is completely unaffected, mappings to a VM can still be aligned
> to 4k, just like for normal buffers.

Reviewed-by: Rodrigo Vivi <[e-mail]>

> 
> Signed-off-by: Zbigniew Kempczyński <[e-mail]>
> Signed-off-by: Maarten Lankhorst <[e-mail]>
> Cc: Matthew Auld <[e-mail]>
> Cc: Rodrigo Vivi <[e-mail]>
> Cc: Thomas Hellström <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Juha-Pekka Heikkilä <[e-mail]>
> ---
>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  9 +++++++++
>  drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
>  drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
>  3 files changed, 26 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..63ce97cc4cfef 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,14 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
>  	int ret;
>  
> +	/*
> +	 * Some modifiers require physical alignment of 64KiB VRAM pages;
> +	 * require that the BO in those cases is created correctly.
> +	 */
> +	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys(mode_cmd->modifier[0]) &&
> +			     !(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 cbe7bf098970f..9d6632f92fa94 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -2019,6 +2019,13 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
>  
>  	bo_flags |= args->placement << (ffs(XE_BO_FLAG_SYSTEM) - 1);
>  
> +	/* CCS formats need physical placement at a 64K alignment in VRAM. */
> +	if ((bo_flags & XE_BO_FLAG_VRAM_MASK) &&
> +	    (bo_flags & XE_BO_FLAG_SCANOUT) &&
> +	    !(xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K) &&
> +	    IS_ALIGNED(args->size, SZ_64K))
> +		bo_flags |= XE_BO_FLAG_NEEDS_64K;
> +
>  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM) {
>  		if (XE_IOCTL_DBG(xe, !(bo_flags & XE_BO_FLAG_VRAM_MASK)))
>  			return -EINVAL;
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 4cc13eddb6b32..3eb76d874eb28 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2878,7 +2878,16 @@ 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) {
> +	/*
> +	 * Some platforms require 64k VM_BIND alignment,
> +	 * specifically those with XE_VRAM_FLAGS_NEED64K.
> +	 *
> +	 * Other platforms may have BO's set to 64k physical placement,
> +	 * but can be mapped at 4k offsets anyway. This check is only
> +	 * there for the former case.
> +	 */
> +	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) ||
> -- 
> 2.45.2
>

Patch

diff --git a/drivers/gpu/drm/xe/display/intel_fb_bo.c b/drivers/gpu/drm/xe/display/intel_fb_bo.c
index f835492f73fb4..63ce97cc4cfef 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,14 @@  int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
 	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
 	int ret;
 
+	/*
+	 * Some modifiers require physical alignment of 64KiB VRAM pages;
+	 * require that the BO in those cases is created correctly.
+	 */
+	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys(mode_cmd->modifier[0]) &&
+			     !(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 cbe7bf098970f..9d6632f92fa94 100644
--- a/drivers/gpu/drm/xe/xe_bo.c
+++ b/drivers/gpu/drm/xe/xe_bo.c
@@ -2019,6 +2019,13 @@  int xe_gem_create_ioctl(struct drm_device *dev, void *data,
 
 	bo_flags |= args->placement << (ffs(XE_BO_FLAG_SYSTEM) - 1);
 
+	/* CCS formats need physical placement at a 64K alignment in VRAM. */
+	if ((bo_flags & XE_BO_FLAG_VRAM_MASK) &&
+	    (bo_flags & XE_BO_FLAG_SCANOUT) &&
+	    !(xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K) &&
+	    IS_ALIGNED(args->size, SZ_64K))
+		bo_flags |= XE_BO_FLAG_NEEDS_64K;
+
 	if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM) {
 		if (XE_IOCTL_DBG(xe, !(bo_flags & XE_BO_FLAG_VRAM_MASK)))
 			return -EINVAL;
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 4cc13eddb6b32..3eb76d874eb28 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -2878,7 +2878,16 @@  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) {
+	/*
+	 * Some platforms require 64k VM_BIND alignment,
+	 * specifically those with XE_VRAM_FLAGS_NEED64K.
+	 *
+	 * Other platforms may have BO's set to 64k physical placement,
+	 * but can be mapped at 4k offsets anyway. This check is only
+	 * there for the former case.
+	 */
+	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) ||