[Mesa-dev] [PATCH 1/2] mesa: Make a Mesa core function for sRGB render encoding handling.

[Kenneth Graunke]

On 04/18/2013 02:25 PM, Eric Anholt wrote:
> ---
>   src/mesa/drivers/dri/i965/brw_wm_surface_state.c  |   27 +++++--------------
>   src/mesa/drivers/dri/i965/gen7_wm_surface_state.c |   29 ++++++---------------
>   src/mesa/main/blend.c                             |    8 ++++++
>   src/mesa/main/blend.h                             |    4 +++
>   4 files changed, 27 insertions(+), 41 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> index a74b2c7..a331c0c 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -31,6 +31,7 @@
>
>
>   #include "main/context.h"
> +#include "main/blend.h"
>   #include "main/mtypes.h"
>   #include "main/samplerobj.h"
>   #include "program/prog_parameter.h"
> @@ -1216,7 +1217,8 @@ brw_update_renderbuffer_surface(struct brw_context *brw,
>      uint32_t *surf;
>      uint32_t tile_x, tile_y;
>      uint32_t format = 0;
> -   gl_format rb_format = intel_rb_format(irb);
> +   /* _NEW_BUFFERS */
> +   gl_format rb_format = _mesa_get_render_format(ctx, intel_rb_format(irb));
>
>      if (irb->tex_image && !brw->has_surface_tile_offset) {
>         intel_renderbuffer_tile_offsets(irb, &tile_x, &tile_y);
> @@ -1238,25 +1240,10 @@ brw_update_renderbuffer_surface(struct brw_context *brw,
>      surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
>   			  6 * 4, 32, &brw->wm.surf_offset[unit]);
>
> -   switch (rb_format) {
> -   case MESA_FORMAT_SARGB8:
> -      /* _NEW_BUFFERS
> -       *
> -       * Without GL_EXT_framebuffer_sRGB we shouldn't bind sRGB surfaces to the
> -       * blend/update as sRGB.
> -       */
> -      if (ctx->Color.sRGBEnabled)
> -	 format = brw_format_for_mesa_format(rb_format);
> -      else
> -	 format = BRW_SURFACEFORMAT_B8G8R8A8_UNORM;
> -      break;
> -   default:
> -      format = brw->render_target_format[rb_format];
> -      if (unlikely(!brw->format_supported_as_render_target[rb_format])) {
> -	 _mesa_problem(ctx, "%s: renderbuffer format %s unsupported\n",
> -		       __FUNCTION__, _mesa_get_format_name(rb_format));
> -      }
> -      break;
> +   format = brw->render_target_format[rb_format];
> +   if (unlikely(!brw->format_supported_as_render_target[rb_format])) {
> +      _mesa_problem(ctx, "%s: renderbuffer format %s unsupported\n",
> +                    __FUNCTION__, _mesa_get_format_name(rb_format));
>      }
>
>      surf[0] = (BRW_SURFACE_2D << BRW_SURFACE_TYPE_SHIFT |
> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> index 2c12be3..3ce7678 100644
> --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> @@ -21,6 +21,7 @@
>    * IN THE SOFTWARE.
>    */
>   #include "main/mtypes.h"
> +#include "main/blend.h"
>   #include "main/samplerobj.h"
>   #include "program/prog_parameter.h"
>
> @@ -525,7 +526,8 @@ gen7_update_renderbuffer_surface(struct brw_context *brw,
>      struct intel_region *region = irb->mt->region;
>      uint32_t tile_x, tile_y;
>      uint32_t format;
> -   gl_format rb_format = intel_rb_format(irb);
> +   /* _NEW_BUFFERS */
> +   gl_format rb_format = _mesa_get_render_format(ctx, intel_rb_format(irb));
>
>      uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
>                                       8 * 4, 32, &brw->wm.surf_offset[unit]);
> @@ -534,26 +536,11 @@ gen7_update_renderbuffer_surface(struct brw_context *brw,
>      /* Render targets can't use IMS layout */
>      assert(irb->mt->msaa_layout != INTEL_MSAA_LAYOUT_IMS);
>
> -   switch (rb_format) {
> -   case MESA_FORMAT_SARGB8:
> -      /* _NEW_BUFFERS
> -       *
> -       * Without GL_EXT_framebuffer_sRGB we shouldn't bind sRGB surfaces to the
> -       * blend/update as sRGB.
> -       */
> -      if (ctx->Color.sRGBEnabled)
> -	 format = brw_format_for_mesa_format(rb_format);
> -      else
> -	 format = BRW_SURFACEFORMAT_B8G8R8A8_UNORM;
> -      break;
> -   default:
> -      assert(brw_render_target_supported(intel, rb));
> -      format = brw->render_target_format[rb_format];
> -      if (unlikely(!brw->format_supported_as_render_target[rb_format])) {
> -	 _mesa_problem(ctx, "%s: renderbuffer format %s unsupported\n",
> -		       __FUNCTION__, _mesa_get_format_name(rb_format));
> -      }
> -      break;
> +   assert(brw_render_target_supported(intel, rb));
> +   format = brw->render_target_format[rb_format];
> +   if (unlikely(!brw->format_supported_as_render_target[rb_format])) {
> +      _mesa_problem(ctx, "%s: renderbuffer format %s unsupported\n",
> +                    __FUNCTION__, _mesa_get_format_name(rb_format));
>      }
>
>      surf[0] = BRW_SURFACE_2D << BRW_SURFACE_TYPE_SHIFT |
> diff --git a/src/mesa/main/blend.c b/src/mesa/main/blend.c
> index 09a1c9a..882fd61 100644
> --- a/src/mesa/main/blend.c
> +++ b/src/mesa/main/blend.c
> @@ -856,6 +856,14 @@ _mesa_update_clamp_vertex_color(struct gl_context *ctx)
>      ctx->Light._ClampVertexColor = _mesa_get_clamp_vertex_color(ctx);
>   }

It's not immediately obvious from the name that this has anything to do 
with sRGB.  Perhaps adding a Doxygen comment here or in the .h file 
would be helpful?

> +gl_format
> +_mesa_get_render_format(struct gl_context *ctx, gl_format format)
> +{
> +   if (ctx->Color.sRGBEnabled)
> +      return format;
> +   else
> +      return _mesa_get_srgb_format_linear(format);
> +}

This is much more succinct and likely to not break.

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>