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

[Lionel Landwerlin]

On 06/06/18 18:17, 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-stable at 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 &isl_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;
Do you want to use it in isl_buffer_fill_image_param() too?

Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>