[Mesa-dev] [PATCH 03/25] i965/gen7: Implement surface state set-up for shader images.

[Paul Berry]

On 2 December 2013 11:39, Francisco Jerez <currojerez at riseup.net> wrote:

> +static uint32_t
> +get_image_format(struct brw_context *brw, gl_format format)
> +{
>

It's not clear without additional context that this function is only used
in the case where the surface is both written to and read from.  Can we
rename it to something like "get_image_format_for_rw"?  At the very least
there should be a comment explaining this.


> +   switch (format) {
> +   case MESA_FORMAT_RGBA_UINT32:
> +   case MESA_FORMAT_RGBA_INT32:
> +   case MESA_FORMAT_RGBA_FLOAT32:
> +      /* Fail...  We need to fall back to untyped surface access for
> +       * all 128 bpp formats.
> +       */
> +      return BRW_SURFACEFORMAT_RAW;
> +
> +   case MESA_FORMAT_RGBA_UINT16:
> +   case MESA_FORMAT_RGBA_INT16:
> +   case MESA_FORMAT_RGBA_FLOAT16:
> +   case MESA_FORMAT_RGBA_16:
> +   case MESA_FORMAT_SIGNED_RGBA_16:
> +   case MESA_FORMAT_RG_UINT32:
> +   case MESA_FORMAT_RG_INT32:
> +   case MESA_FORMAT_RG_FLOAT32:
> +      /* HSW supports the R16G16B16A16_UINT format natively and
> +       * handles the pixel packing, unpacking and type conversion in
> +       * the shader for other 64 bpp formats.  IVB falls back to
> +       * untyped.
> +       */
> +      return (brw->is_haswell ? BRW_SURFACEFORMAT_R16G16B16A16_UINT :
> +              BRW_SURFACEFORMAT_RAW);
> +
> +   case MESA_FORMAT_RGBA_UINT8:
> +   case MESA_FORMAT_RGBA_INT8:
> +   case MESA_FORMAT_RGBA8888_REV:
> +   case MESA_FORMAT_SIGNED_RGBA8888_REV:
> +      /* HSW supports the R8G8B8A8_UINT format natively, type
> +       * conversion to other formats is handled in the shader.  IVB
> +       * uses R32_UINT and handles the pixel packing, unpacking and
> +       * type conversion in the shader.
> +       */
> +      return (brw->is_haswell ? BRW_SURFACEFORMAT_R8G8B8A8_UINT :
> +              BRW_SURFACEFORMAT_R32_UINT);
> +
> +   case MESA_FORMAT_RG_UINT16:
> +   case MESA_FORMAT_RG_INT16:
> +   case MESA_FORMAT_RG_FLOAT16:
> +   case MESA_FORMAT_GR1616:
> +   case MESA_FORMAT_SIGNED_GR1616:
> +      /* HSW supports the R16G16_UINT format natively, type conversion
> +       * to other formats is handled in the shader.  IVB uses R32_UINT
> +       * and handles the pixel packing, unpacking and type conversion
> +       * in the shader.
> +       */
> +      return (brw->is_haswell ? BRW_SURFACEFORMAT_R16G16_UINT :
> +              BRW_SURFACEFORMAT_R32_UINT);
> +
> +   case MESA_FORMAT_ABGR2101010_UINT:
> +   case MESA_FORMAT_ABGR2101010:
> +   case MESA_FORMAT_R11_G11_B10_FLOAT:
> +   case MESA_FORMAT_R_UINT32:
> +      /* Neither the 2/10/10/10 nor the 11/11/10 packed formats are
> +       * supported by the hardware.  Use R32_UINT and handle the pixel
> +       * packing, unpacking, and type conversion in the shader.
> +       */
> +      return BRW_SURFACEFORMAT_R32_UINT;
> +
> +   case MESA_FORMAT_R_INT32:
> +      return BRW_SURFACEFORMAT_R32_SINT;
> +
> +   case MESA_FORMAT_R_FLOAT32:
> +      return BRW_SURFACEFORMAT_R32_FLOAT;
> +
> +   case MESA_FORMAT_RG_UINT8:
> +   case MESA_FORMAT_RG_INT8:
> +   case MESA_FORMAT_GR88:
> +   case MESA_FORMAT_SIGNED_RG88_REV:
> +      /* HSW supports the R8G8_UINT format natively, type conversion
> +       * to other formats is handled in the shader.  IVB uses R16_UINT
> +       * and handles the pixel packing, unpacking and type conversion
> +       * in the shader.  Note that this relies on the undocumented
> +       * behavior that typed reads from R16_UINT surfaces actually do
> +       * a 32-bit misaligned read on IVB.  The alternative would be to
> +       * use two surface state entries with different formats for each
> +       * image, one for reading (using R32_UINT) and another one for
> +       * writing (using R8G8_UINT), but that would complicate the
> +       * shaders we generate even more.
> +       */
> +      return (brw->is_haswell ? BRW_SURFACEFORMAT_R8G8_UINT :
> +              BRW_SURFACEFORMAT_R16_UINT);
> +
> +   case MESA_FORMAT_R_UINT16:
> +   case MESA_FORMAT_R_FLOAT16:
> +   case MESA_FORMAT_R_INT16:
> +   case MESA_FORMAT_R16:
> +   case MESA_FORMAT_SIGNED_R16:
> +      /* HSW supports the R16_UINT format natively, type conversion to
> +       * other formats is handled in the shader.  IVB relies on the
> +       * same undocumented behavior described above.
> +       */
> +      return BRW_SURFACEFORMAT_R16_UINT;
> +
> +   case MESA_FORMAT_R_UINT8:
> +   case MESA_FORMAT_R_INT8:
> +   case MESA_FORMAT_R8:
> +   case MESA_FORMAT_SIGNED_R8:
> +      /* HSW supports the R8_UINT format natively, type conversion to
> +       * other formats is handled in the shader.  IVB relies on the
> +       * same undocumented behavior described above.
> +       */
> +      return BRW_SURFACEFORMAT_R8_UINT;
> +
> +   default:
> +      unreachable();
>

Change to an assert.


> +   }
> +}
>

With those changes, this patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131230/d66dfd66/attachment-0001.html>