On Wed 08 Nov 2017, Jason Ekstrand wrote:
> On Wed, Nov 8, 2017 at 1:34 AM, Samuel Iglesias Gonsálvez <[1]
> sigles...@igalia.com> wrote:
>
> The HW has some limits but, according to the spec, we can create
> the image as it has not yet any memory backing it. When we allocate
> that memory, then we fail following what Vulkan spec, "10.2. Device
> Memory" says when talking about vkAllocateMemory():
>
> "Some platforms may have a limit on the maximum size of a single
> allocation. For example, certain systems may fail to create
> allocations with a size greater than or equal to 4GB. Such a limit is
> implementation-dependent, and if such a failure occurs then the error
> VK_ERROR_OUT_OF_DEVICE_MEMORY must be returned."
>
> Fixes the crashes on BDW for the following tests:
>
> dEQP-VK.pipeline.render_to_image.core.2d_array.huge.*
> dEQP-VK.pipeline.render_to_image.core.cube_array.huge.*
>
> Signed-off-by: Samuel Iglesias Gonsálvez <[2]sigles...@igalia.com>
> ---
>
> Jason, I was tempted to move this piece of code to anv_AllocateMemory()
> but then I found the kernel relocation limitation of 32-bit... Is that
> limitation still applicable? Or was it from the BDW age and we forgot
> to update that limitation for gen9+?
>
>
> We're still using relocations on all hardware so it applies to everything
> today. One of my 2018 projects is to fix that and get rid of relocations on
> gen8+.
>
>
> Sam
>
> src/intel/isl/isl.c | 22 ----------------------
> 1 file changed, 22 deletions(-)
>
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index 59f512fc050..aaadcbaf991 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -1472,28 +1472,6 @@ isl_surf_init_s(const struct isl_device *dev,
> base_alignment = MAX(info->min_alignment, tile_size);
> }
>
> - if (ISL_DEV_GEN(dev) < 9) {
> - /* From the Broadwell PRM Vol 5, Surface Layout:
> - *
> - * "In addition to restrictions on maximum height, width, and
> depth,
> - * surfaces are also restricted to a maximum size in bytes.
> This
> - * maximum is 2 GB for all products and all surface types."
> - *
> - * This comment is applicable to all Pre-gen9 platforms.
> - */
> - if (size > (uint64_t) 1 << 31)
> - return false;
> - } else {
> - /* From the Skylake PRM Vol 5, Maximum Surface Size in Bytes:
> - * "In addition to restrictions on maximum height, width, and
> depth,
> - * surfaces are also restricted to a maximum size of 2^38
> bytes.
> - * All pixels within the surface must be contained within 2^38
> bytes
> - * of the base address."
> - */
> - if (size > (uint64_t) 1 << 38)
> - return false;
> - }
I think it very unwise to delete code that enforces requirements defined
by the hardware spec. Deleting the code doesn't make the hardware
requirements go away :)
> I'm not sure how I feel about removing this from ISL. There are really two
> limitations going on here. One is a limitation imposed by relocations, and
> the
> other is some sort of fundamental hardware surface size limitation. Most
> likely, the surface size limitation has to do with how many bits they use for
> image address computations in the hardware. Most likely, on gen8, they do all
> of the internal calculations in 32 bits and only convert to 48 at the end when
> they need to add it to Surface Base Address.
>
> If my understanding is correct then we will still have this limitation on gen8
> even after we get rid of relocations and remove the BO size limitation. I see
> a couple of options, neither of which I like very much:
>
> 1) Take something like this patch and then keep the BO size limitation on BDW
> to 2GiB when we get rid of relocations even though it's artificial.
> 2) "Gracefully" handle isl_surf_init failure by doing a debug_log and
> succeeding but setting the image size (that will be returned by
> vkGetImageMemoryRequirements) to UINT64_MAX so that the client won't ever be
> able to find memory for it.
>
> My feeling is that 1) above is probably the better of the two as 2) seems to
> be
> a twisting of the spec. That said, I would like to keep the restriction in
> ISL
> somehow and we need to make sure it still gets applied in GL.
I dislike both. I originally designed isl to mimic the VkImage API, so
let's continue that trend.
Option 3) Change isl_surf_init() to return a meaningful result code:
ISL_SUCCESS = 0
ISL_ERROR_SOMETHING_SOMETHING_THE_USUAL_FAILURES = -1
ISL_ERROR_SURFACE_SIZE_TOO_LARGE = -2
I like option 3 because it avoids secret implicit contracts between isl
and anvil, and thus avoids hidden hacks.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
