Re: [Mesa-dev] [PATCH 1/7] intel/isl: Add bounds-checking assertions in isl_format_get_layout

2018-06-07 Thread Lionel Landwerlin 

You left out isl_buffer_fill_image_param() in isl_storage_image.c

Regardless, this is an improvement and with this I actually hit the 
assert :)


Reviewed-by: Lionel Landwerlin 

On 06/06/18 18:47, Jason Ekstrand wrote:

We add two assertions instead of one because the first assertion that
format != ISL_FORMAT_UNSUPPORTED is more descriptive and checks for a
real but unsupported enumerant while the second ensures that they don't
pass in garbage values.  We also update some other helpers to use
isl_format_get_layout instead of using the table directly so that they
get bounds checking too.

Cc: mesa-sta...@lists.freedesktop.org
---
  src/intel/isl/isl.h | 32 +---
  1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
index 00cfe31fc04..6800b1d76a6 100644
--- a/src/intel/isl/isl.h
+++ b/src/intel/isl/isl.h
@@ -389,6 +389,9 @@ enum isl_format {
 ISL_FORMAT_GEN9_CCS_64BPP,
 ISL_FORMAT_GEN9_CCS_128BPP,
  
+   /* An upper bound on the supported format enumerations */

+   ISL_NUM_FORMATS,
+
 /* Hardware doesn't understand this out-of-band value */
 ISL_FORMAT_UNSUPPORTED = UINT16_MAX,
  };
@@ -1423,6 +1426,8 @@ isl_device_get_sample_counts(struct isl_device *dev);
  static inline const struct isl_format_layout * ATTRIBUTE_CONST
  isl_format_get_layout(enum isl_format fmt)
  {
+   assert(fmt != ISL_FORMAT_UNSUPPORTED);
+   assert(fmt < ISL_NUM_FORMATS);
 return _format_layouts[fmt];
  }
  
@@ -1431,7 +1436,7 @@ bool isl_format_is_valid(enum isl_format);

  static inline const char * ATTRIBUTE_CONST
  isl_format_get_name(enum isl_format fmt)
  {
-   return isl_format_layouts[fmt].name;
+   return isl_format_get_layout(fmt)->name;
  }
  
  bool isl_format_supports_rendering(const struct gen_device_info *devinfo,

@@ -1546,7 +1551,7 @@ isl_format_block_is_1x1x1(enum isl_format fmt)
  static inline bool
  isl_format_is_srgb(enum isl_format fmt)
  {
-   return isl_format_layouts[fmt].colorspace == ISL_COLORSPACE_SRGB;
+   return isl_format_get_layout(fmt)->colorspace == ISL_COLORSPACE_SRGB;
  }
  
  enum isl_format isl_format_srgb_to_linear(enum isl_format fmt);

@@ -1556,20 +1561,25 @@ isl_format_is_rgb(enum isl_format fmt)
  {
 if (isl_format_is_yuv(fmt))
return false;
-   return isl_format_layouts[fmt].channels.r.bits > 0 &&
-  isl_format_layouts[fmt].channels.g.bits > 0 &&
-  isl_format_layouts[fmt].channels.b.bits > 0 &&
-  isl_format_layouts[fmt].channels.a.bits == 0;
+
+   const struct isl_format_layout *fmtl = isl_format_get_layout(fmt);
+
+   return fmtl->channels.r.bits > 0 &&
+  fmtl->channels.g.bits > 0 &&
+  fmtl->channels.b.bits > 0 &&
+  fmtl->channels.a.bits == 0;
  }
  
  static inline bool

  isl_format_is_rgbx(enum isl_format fmt)
  {
-   return isl_format_layouts[fmt].channels.r.bits > 0 &&
-  isl_format_layouts[fmt].channels.g.bits > 0 &&
-  isl_format_layouts[fmt].channels.b.bits > 0 &&
-  isl_format_layouts[fmt].channels.a.bits > 0 &&
-  isl_format_layouts[fmt].channels.a.type == ISL_VOID;
+   const struct isl_format_layout *fmtl = isl_format_get_layout(fmt);
+
+   return fmtl->channels.r.bits > 0 &&
+  fmtl->channels.g.bits > 0 &&
+  fmtl->channels.b.bits > 0 &&
+  fmtl->channels.a.bits > 0 &&
+  fmtl->channels.a.type == ISL_VOID;
  }
  
  enum isl_format isl_format_rgb_to_rgba(enum isl_format rgb) ATTRIBUTE_CONST;



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/7] intel/isl: Add bounds-checking assertions in isl_format_get_layout

2018-06-06 Thread Jason Ekstrand 
We add two assertions instead of one because the first assertion that
format != ISL_FORMAT_UNSUPPORTED is more descriptive and checks for a
real but unsupported enumerant while the second ensures that they don't
pass in garbage values.  We also update some other helpers to use
isl_format_get_layout instead of using the table directly so that they
get bounds checking too.

Cc: mesa-sta...@lists.freedesktop.org
---
 src/intel/isl/isl.h | 32 +---
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
index 00cfe31fc04..6800b1d76a6 100644
--- a/src/intel/isl/isl.h
+++ b/src/intel/isl/isl.h
@@ -389,6 +389,9 @@ enum isl_format {
ISL_FORMAT_GEN9_CCS_64BPP,
ISL_FORMAT_GEN9_CCS_128BPP,
 
+   /* An upper bound on the supported format enumerations */
+   ISL_NUM_FORMATS,
+
/* Hardware doesn't understand this out-of-band value */
ISL_FORMAT_UNSUPPORTED = UINT16_MAX,
 };
@@ -1423,6 +1426,8 @@ isl_device_get_sample_counts(struct isl_device *dev);
 static inline const struct isl_format_layout * ATTRIBUTE_CONST
 isl_format_get_layout(enum isl_format fmt)
 {
+   assert(fmt != ISL_FORMAT_UNSUPPORTED);
+   assert(fmt < ISL_NUM_FORMATS);
return _format_layouts[fmt];
 }
 
@@ -1431,7 +1436,7 @@ bool isl_format_is_valid(enum isl_format);
 static inline const char * ATTRIBUTE_CONST
 isl_format_get_name(enum isl_format fmt)
 {
-   return isl_format_layouts[fmt].name;
+   return isl_format_get_layout(fmt)->name;
 }
 
 bool isl_format_supports_rendering(const struct gen_device_info *devinfo,
@@ -1546,7 +1551,7 @@ isl_format_block_is_1x1x1(enum isl_format fmt)
 static inline bool
 isl_format_is_srgb(enum isl_format fmt)
 {
-   return isl_format_layouts[fmt].colorspace == ISL_COLORSPACE_SRGB;
+   return isl_format_get_layout(fmt)->colorspace == ISL_COLORSPACE_SRGB;
 }
 
 enum isl_format isl_format_srgb_to_linear(enum isl_format fmt);
@@ -1556,20 +1561,25 @@ isl_format_is_rgb(enum isl_format fmt)
 {
if (isl_format_is_yuv(fmt))
   return false;
-   return isl_format_layouts[fmt].channels.r.bits > 0 &&
-  isl_format_layouts[fmt].channels.g.bits > 0 &&
-  isl_format_layouts[fmt].channels.b.bits > 0 &&
-  isl_format_layouts[fmt].channels.a.bits == 0;
+
+   const struct isl_format_layout *fmtl = isl_format_get_layout(fmt);
+
+   return fmtl->channels.r.bits > 0 &&
+  fmtl->channels.g.bits > 0 &&
+  fmtl->channels.b.bits > 0 &&
+  fmtl->channels.a.bits == 0;
 }
 
 static inline bool
 isl_format_is_rgbx(enum isl_format fmt)
 {
-   return isl_format_layouts[fmt].channels.r.bits > 0 &&
-  isl_format_layouts[fmt].channels.g.bits > 0 &&
-  isl_format_layouts[fmt].channels.b.bits > 0 &&
-  isl_format_layouts[fmt].channels.a.bits > 0 &&
-  isl_format_layouts[fmt].channels.a.type == ISL_VOID;
+   const struct isl_format_layout *fmtl = isl_format_get_layout(fmt);
+
+   return fmtl->channels.r.bits > 0 &&
+  fmtl->channels.g.bits > 0 &&
+  fmtl->channels.b.bits > 0 &&
+  fmtl->channels.a.bits > 0 &&
+  fmtl->channels.a.type == ISL_VOID;
 }
 
 enum isl_format isl_format_rgb_to_rgba(enum isl_format rgb) ATTRIBUTE_CONST;
-- 
2.17.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev