Re: [igt-dev] [PATCH i-g-t 1/2] lib/intel_allocator: Add safe alignment as a default

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 23 Feb 2022 23:54:14 -0800, Zbigniew Kempczyński wrote:
>
> diff --git a/lib/intel_allocator.c b/lib/intel_allocator.c
> index eabff1f9a..340b88828 100644
> --- a/lib/intel_allocator.c
> +++ b/lib/intel_allocator.c
> @@ -283,7 +283,8 @@ static bool __allocator_put(struct allocator *al)
>  static struct intel_allocator *intel_allocator_create(int fd,
>						      uint64_t start, uint64_t end,
>						      uint8_t allocator_type,
> -						      uint8_t allocator_strategy)
> +						      uint8_t allocator_strategy,
> +						      uint64_t default_alignment)
>  {
>	struct intel_allocator *ial = NULL;
>
> @@ -321,6 +322,7 @@ static struct intel_allocator *intel_allocator_create(int fd,
>
>	ial->type = allocator_type;
>	ial->strategy = allocator_strategy;
> +	ial->default_alignment = default_alignment;

Isn't this a better place to set default_alignment to
gem_detect_safe_alignment() rather than in __intel_allocator_open_full()
below (though I understand they are roughly equivalent)?

> @@ -903,6 +920,9 @@ static uint64_t __intel_allocator_open_full(int fd, uint32_t ctx,
>		req.open.end = gtt_size;
>	}
>
> +	if (!default_alignment)
> +		req.open.default_alignment = gem_detect_safe_alignment(fd);
> +

/snip/

> @@ -948,20 +970,24 @@ static uint64_t __intel_allocator_open_full(int fd, uint32_t ctx,
>  uint64_t intel_allocator_open_full(int fd, uint32_t ctx,
>				   uint64_t start, uint64_t end,
>				   uint8_t allocator_type,
> -				   enum allocator_strategy strategy)
> +				   enum allocator_strategy strategy,
> +				   uint64_t default_alignment)

So we have added default_alignment to all these API's now and that does
provide additional flexibility but I am wondering if the additional
flexibility is really needed and will any IGT ever set default_alignment
other than 0 (i.e. let allocator choose safe_alignment)? And also does
having a different default_alignment improve test coverage in any way?

In any case, this is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>