Re: [igt-dev] [PATCH i-g-t 2/2] lib/i915/intel_memory_region: Add fallback for creating gem bo

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

On Thu, 09 Dec 2021 12:04:53 -0800, Zbigniew Kempczyński wrote:
>
> diff --git a/lib/i915/intel_memory_region.c b/lib/i915/intel_memory_region.c
> index fd29eec90..2263f1984 100644
> --- a/lib/i915/intel_memory_region.c
> +++ b/lib/i915/intel_memory_region.c
> @@ -197,8 +197,23 @@ int __gem_create_in_memory_region_list(int fd, uint32_t *handle, uint64_t *size,
>		.num_regions = num_regions,
>		.regions = to_user_pointer(mem_regions),
>	};
> +	int ret;
>
> -	return __gem_create_ext(fd, size, handle, &ext_regions.base);
> +	ret = __gem_create_ext(fd, size, handle, &ext_regions.base);
> +
> +	/*
> +	 * Provide fallback for stable kernels if region passed in the array
> +	 * can be system memory. In this case we get -ENODEV but still
> +	 * we're able to allocate gem bo in system memory using legacy call.
> +	 */
> +	if (ret == -ENODEV)
> +		for (int i = 0; i < num_regions; i++)
> +			if (mem_regions[i].memory_class == I915_MEMORY_CLASS_SYSTEM) {
> +				ret = __gem_create(fd, size, handle);
> +				break;
> +			}

Shouldn't this function return success if mem_regions[] array contains only
a single system memory region entry (that is the sime of mem_regions[]
array is 1 and the only entry is system memory)? But here we are returning
success if /any/ of the regions passed in are system memory? Won't this
cause an error in the caller? I am not sure if checking for -ENODEV return
saves us in any way but I think the code should be changed to return
success if mem_regions[] array contains only a single system memory region.

I think the previous patch "lib/i915/intel_memory_region: Provide system
memory region stub" is fine and serves this purpose of returning a single
system memory region. Do we even need this patch?


> +
> +	return ret;
>  }
>
>  /* gem_create_in_memory_region_list:
> --
> 2.26.0
>