Re: [Mesa-dev] [PATCH v2] isl: don't crash when creating a huge image

[Samuel Iglesias Gonsálvez]

On 11/01/18 23:27, Jason Ekstrand wrote:
> Sorry.  It's taken a bit of time but the WG has a decision and that is
> that we are supposed to throw VK_ERROR_OUT_OF_DEVICE_MEMORY in this
> case.  I believe you had an alternate version of the patch that did
> something like that.  If so, we should revive it.
>

Thanks for the info!

I am thinking on keep this patch and add the following one on top like this:

diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
index ba932ba47c3..652f28460d9 100644
--- a/src/intel/vulkan/anv_image.c
+++ b/src/intel/vulkan/anv_image.c
@@ -335,6 +335,9 @@ make_surface(const struct anv_device *dev,
 */
    assert(ok);
 
+   if (anv_surf->isl.size == UINT64_MAX)
+  return VK_ERROR_OUT_OF_DEVICE_MEMORY;
+
    image->planes[plane].aux_usage = ISL_AUX_USAGE_NONE;
 
    add_surface(image, anv_surf, plane);

Hence, we fail in vkCreateImage() with VK_ERROR_OUT_OF_DEVICE_MEMORY as
the suggested change to the spec, with zero changes to other users of
isl_surf_init() . If you agree, I will send both patches again for review.

Sam

> On Thu, Jan 11, 2018 at 5:04 AM, Samuel Iglesias Gonsálvez
> > wrote:
>
> This patch is still unreviewed.
>
> Sam
>
>
> On 14/11/17 09:45, Samuel Iglesias Gonsálvez 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. This patch
> > logs a debug error and set the size to the UINT64_MAX in order to
> > avoid allocating actual memory later.
> >
> > 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  >
> > ---
> >  src/intel/isl/isl.c | 13 +
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> > index 59f512fc050..cd7f2fcd4cb 100644
> > --- a/src/intel/isl/isl.c
> > +++ b/src/intel/isl/isl.c
> > @@ -26,6 +26,7 @@
> >  #include 
> >
> >  #include "genxml/genX_bits.h"
> > +#include "common/intel_log.h"
> >
> >  #include "isl.h"
> >  #include "isl_gen4.h"
> > @@ -1481,8 +1482,10 @@ isl_surf_init_s(const struct isl_device *dev,
> >         *
> >         * This comment is applicable to all Pre-gen9 platforms.
> >         */
> > -      if (size > (uint64_t) 1 << 31)
> > -         return false;
> > +      if (size > (uint64_t) 1 << 31) {
> > +         intel_logd("%s: Surface size is bigger than the
> supported by the HW: %ld > (1 << 31)", __func__, size);
> > +         size = UINT64_MAX;
> > +      }
> >     } else {
> >        /* From the Skylake PRM Vol 5, Maximum Surface Size in Bytes:
> >         *    "In addition to restrictions on maximum height,
> width, and depth,
> > @@ -1490,8 +1493,10 @@ isl_surf_init_s(const struct isl_device *dev,
> >         *     All pixels within the surface must be contained
> within 2^38 bytes
> >         *     of the base address."
> >         */
> > -      if (size > (uint64_t) 1 << 38)
> > -         return false;
> > +      if (size > (uint64_t) 1 << 38) {
> > +         intel_logd("%s: Surface size is bigger than the
> supported by the HW: %ld > (1 << 38)", __func__, size);
> > +         size = UINT64_MAX;
> > +      }
> >     }
> >
> >     *surf = (struct isl_surf) {
>
>
>



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] isl: don't crash when creating a huge image

[Jason Ekstrand]

Sorry.  It's taken a bit of time but the WG has a decision and that is that
we are supposed to throw VK_ERROR_OUT_OF_DEVICE_MEMORY in this case.  I
believe you had an alternate version of the patch that did something like
that.  If so, we should revive it.

On Thu, Jan 11, 2018 at 5:04 AM, Samuel Iglesias Gonsálvez <
sigles...@igalia.com> wrote:

> This patch is still unreviewed.
>
> Sam
>
>
> On 14/11/17 09:45, Samuel Iglesias Gonsálvez 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. This patch
> > logs a debug error and set the size to the UINT64_MAX in order to
> > avoid allocating actual memory later.
> >
> > 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 
> > ---
> >  src/intel/isl/isl.c | 13 +
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> > index 59f512fc050..cd7f2fcd4cb 100644
> > --- a/src/intel/isl/isl.c
> > +++ b/src/intel/isl/isl.c
> > @@ -26,6 +26,7 @@
> >  #include 
> >
> >  #include "genxml/genX_bits.h"
> > +#include "common/intel_log.h"
> >
> >  #include "isl.h"
> >  #include "isl_gen4.h"
> > @@ -1481,8 +1482,10 @@ isl_surf_init_s(const struct isl_device *dev,
> > *
> > * This comment is applicable to all Pre-gen9 platforms.
> > */
> > -  if (size > (uint64_t) 1 << 31)
> > - return false;
> > +  if (size > (uint64_t) 1 << 31) {
> > + intel_logd("%s: Surface size is bigger than the supported by
> the HW: %ld > (1 << 31)", __func__, size);
> > + size = UINT64_MAX;
> > +  }
> > } else {
> >/* From the Skylake PRM Vol 5, Maximum Surface Size in Bytes:
> > *"In addition to restrictions on maximum height, width, and
> depth,
> > @@ -1490,8 +1493,10 @@ isl_surf_init_s(const struct isl_device *dev,
> > * All pixels within the surface must be contained within
> 2^38 bytes
> > * of the base address."
> > */
> > -  if (size > (uint64_t) 1 << 38)
> > - return false;
> > +  if (size > (uint64_t) 1 << 38) {
> > + intel_logd("%s: Surface size is bigger than the supported by
> the HW: %ld > (1 << 38)", __func__, size);
> > + size = UINT64_MAX;
> > +  }
> > }
> >
> > *surf = (struct isl_surf) {
>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] isl: don't crash when creating a huge image

[Samuel Iglesias Gonsálvez]

This patch is still unreviewed.

Sam


On 14/11/17 09:45, Samuel Iglesias Gonsálvez 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. This patch
> logs a debug error and set the size to the UINT64_MAX in order to
> avoid allocating actual memory later.
>
> 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 
> ---
>  src/intel/isl/isl.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index 59f512fc050..cd7f2fcd4cb 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -26,6 +26,7 @@
>  #include 
>  
>  #include "genxml/genX_bits.h"
> +#include "common/intel_log.h"
>  
>  #include "isl.h"
>  #include "isl_gen4.h"
> @@ -1481,8 +1482,10 @@ isl_surf_init_s(const struct isl_device *dev,
> *
> * This comment is applicable to all Pre-gen9 platforms.
> */
> -  if (size > (uint64_t) 1 << 31)
> - return false;
> +  if (size > (uint64_t) 1 << 31) {
> + intel_logd("%s: Surface size is bigger than the supported by the 
> HW: %ld > (1 << 31)", __func__, size);
> + size = UINT64_MAX;
> +  }
> } else {
>/* From the Skylake PRM Vol 5, Maximum Surface Size in Bytes:
> *"In addition to restrictions on maximum height, width, and depth,
> @@ -1490,8 +1493,10 @@ isl_surf_init_s(const struct isl_device *dev,
> * All pixels within the surface must be contained within 2^38 
> bytes
> * of the base address."
> */
> -  if (size > (uint64_t) 1 << 38)
> - return false;
> +  if (size > (uint64_t) 1 << 38) {
> + intel_logd("%s: Surface size is bigger than the supported by the 
> HW: %ld > (1 << 38)", __func__, size);
> + size = UINT64_MAX;
> +  }
> }
>  
> *surf = (struct isl_surf) {




signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] isl: don't crash when creating a huge image

[Samuel Iglesias Gonsálvez]

On 28/11/17 11:07, Samuel Iglesias Gonsálvez wrote:
> This patch is still unreviewed.
> 

Gently reminder.

Sam

> Sam
> 
> On Tue, 2017-11-14 at 09:45 +0100, Samuel Iglesias Gonsálvez 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. This patch
>> logs a debug error and set the size to the UINT64_MAX in order to
>> avoid allocating actual memory later.
>>
>> 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 
>> ---
>>  src/intel/isl/isl.c | 13 +
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
>> index 59f512fc050..cd7f2fcd4cb 100644
>> --- a/src/intel/isl/isl.c
>> +++ b/src/intel/isl/isl.c
>> @@ -26,6 +26,7 @@
>>  #include 
>>  
>>  #include "genxml/genX_bits.h"
>> +#include "common/intel_log.h"
>>  
>>  #include "isl.h"
>>  #include "isl_gen4.h"
>> @@ -1481,8 +1482,10 @@ isl_surf_init_s(const struct isl_device *dev,
>> *
>> * This comment is applicable to all Pre-gen9 platforms.
>> */
>> -  if (size > (uint64_t) 1 << 31)
>> - return false;
>> +  if (size > (uint64_t) 1 << 31) {
>> + intel_logd("%s: Surface size is bigger than the supported
>> by the HW: %ld > (1 << 31)", __func__, size);
>> + size = UINT64_MAX;
>> +  }
>> } else {
>>/* From the Skylake PRM Vol 5, Maximum Surface Size in Bytes:
>> *"In addition to restrictions on maximum height, width,
>> and depth,
>> @@ -1490,8 +1493,10 @@ isl_surf_init_s(const struct isl_device *dev,
>> * All pixels within the surface must be contained within
>> 2^38 bytes
>> * of the base address."
>> */
>> -  if (size > (uint64_t) 1 << 38)
>> - return false;
>> +  if (size > (uint64_t) 1 << 38) {
>> + intel_logd("%s: Surface size is bigger than the supported
>> by the HW: %ld > (1 << 38)", __func__, size);
>> + size = UINT64_MAX;
>> +  }
>> }
>>  
>> *surf = (struct isl_surf) {
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] isl: don't crash when creating a huge image

[Samuel Iglesias Gonsálvez]

This patch is still unreviewed.

Sam

On Tue, 2017-11-14 at 09:45 +0100, Samuel Iglesias Gonsálvez 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. This patch
> logs a debug error and set the size to the UINT64_MAX in order to
> avoid allocating actual memory later.
> 
> 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 
> ---
>  src/intel/isl/isl.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index 59f512fc050..cd7f2fcd4cb 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -26,6 +26,7 @@
>  #include 
>  
>  #include "genxml/genX_bits.h"
> +#include "common/intel_log.h"
>  
>  #include "isl.h"
>  #include "isl_gen4.h"
> @@ -1481,8 +1482,10 @@ isl_surf_init_s(const struct isl_device *dev,
> *
> * This comment is applicable to all Pre-gen9 platforms.
> */
> -  if (size > (uint64_t) 1 << 31)
> - return false;
> +  if (size > (uint64_t) 1 << 31) {
> + intel_logd("%s: Surface size is bigger than the supported
> by the HW: %ld > (1 << 31)", __func__, size);
> + size = UINT64_MAX;
> +  }
> } else {
>/* From the Skylake PRM Vol 5, Maximum Surface Size in Bytes:
> *"In addition to restrictions on maximum height, width,
> and depth,
> @@ -1490,8 +1493,10 @@ isl_surf_init_s(const struct isl_device *dev,
> * All pixels within the surface must be contained within
> 2^38 bytes
> * of the base address."
> */
> -  if (size > (uint64_t) 1 << 38)
> - return false;
> +  if (size > (uint64_t) 1 << 38) {
> + intel_logd("%s: Surface size is bigger than the supported
> by the HW: %ld > (1 << 38)", __func__, size);
> + size = UINT64_MAX;
> +  }
> }
>  
> *surf = (struct isl_surf) {
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev