On Thu, Jan 11, 2018 at 02:19:40PM +0100, Neil Roberts wrote:
> When programming the fast clear color there was previously a chunk of
> code to try to make the color match the constraints of the surface
> format such as by filling in missing components and handling luminance
> formats. These cases are not handled by the hardware. There are some
> additional possible restrictions that the hardware does seem to
> handle, such as clamping to [0,1] for normalised formats. However for
> whatever reason it doesn't clamp to [0,∞] for the special float
> formats that don't have a sign bit.
>
> Rather than having special cases for all of these this patch makes it
> instead convert the color to the actual surface format and back again
> so that we can be sure it will have all of the possible restrictions.
> Additionally this would avoid some other potential surprises such as
> getting more precision for the clear color when fast clears are used.
>
> Originally this patch was created to fix the following bug, but it is
> no longer neccessary since f1fa4be871e13c68b50685aaf64. However I
> think this approach is still worthwhile because it has the advantage
> of reducing code redundancy.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93338
> v2: Rebase and add support for converting int formats
> ---
>
> This is a rebase of this patch from 2 years ago:
>
> https://patchwork.freedesktop.org/patch/67912/
>
This is a really nice clean up. A couple of suggestions below:
> src/mesa/drivers/dri/i965/brw_meta_util.c | 107
> +++++++++++-------------------
> 1 file changed, 37 insertions(+), 70 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.c
> b/src/mesa/drivers/dri/i965/brw_meta_util.c
> index 54dc6a5..ec727eb 100644
> --- a/src/mesa/drivers/dri/i965/brw_meta_util.c
> +++ b/src/mesa/drivers/dri/i965/brw_meta_util.c
> @@ -29,6 +29,8 @@
> #include "main/blend.h"
> #include "main/fbobject.h"
> #include "util/format_srgb.h"
> +#include "main/format_pack.h"
> +#include "main/format_unpack.h"
>
> /**
> * Helper function for handling mirror image blits.
> @@ -345,6 +347,7 @@ brw_meta_convert_fast_clear_color(const struct
> brw_context *brw,
> const struct intel_mipmap_tree *mt,
> const union gl_color_union *color)
> {
> + mesa_format linear_format = _mesa_get_srgb_format_linear(mt->format);
> union isl_color_value override_color = {
> .u32 = {
> color->ui[0],
> @@ -354,82 +357,46 @@ brw_meta_convert_fast_clear_color(const struct
> brw_context *brw,
> },
> };
>
> - /* The sampler doesn't look at the format of the surface when the fast
> - * clear color is used so we need to implement luminance, intensity and
> - * missing components manually.
> - */
> - switch (_mesa_get_format_base_format(mt->format)) {
> - case GL_INTENSITY:
> - override_color.u32[3] = override_color.u32[0];
> - /* flow through */
> - case GL_LUMINANCE:
> - case GL_LUMINANCE_ALPHA:
> - override_color.u32[1] = override_color.u32[0];
> - override_color.u32[2] = override_color.u32[0];
> - break;
> - default:
> - for (int i = 0; i < 3; i++) {
> - if (!_mesa_format_has_color_component(mt->format, i))
> - override_color.u32[i] = 0;
> - }
> - break;
> - }
> -
> - switch (_mesa_get_format_datatype(mt->format)) {
> - case GL_UNSIGNED_NORMALIZED:
> - for (int i = 0; i < 4; i++)
> - override_color.f32[i] = CLAMP(override_color.f32[i], 0.0f, 1.0f);
> - break;
> -
> - case GL_SIGNED_NORMALIZED:
> - for (int i = 0; i < 4; i++)
> - override_color.f32[i] = CLAMP(override_color.f32[i], -1.0f, 1.0f);
> - break;
> -
> - case GL_UNSIGNED_INT:
> - for (int i = 0; i < 4; i++) {
> - unsigned bits = _mesa_get_format_bits(mt->format, GL_RED_BITS + i);
> - if (bits < 32) {
> - uint32_t max = (1u << bits) - 1;
> - override_color.u32[i] = MIN2(override_color.u32[i], max);
> - }
> - }
> - break;
> -
> - case GL_INT:
> - for (int i = 0; i < 4; i++) {
> - unsigned bits = _mesa_get_format_bits(mt->format, GL_RED_BITS + i);
> - if (bits < 32) {
> - int32_t max = (1 << (bits - 1)) - 1;
> - int32_t min = -(1 << (bits - 1));
> - override_color.i32[i] = CLAMP(override_color.i32[i], min, max);
> - }
> - }
> - break;
> -
> - case GL_FLOAT:
> - if (!_mesa_is_format_signed(mt->format)) {
> - for (int i = 0; i < 4; i++)
> - override_color.f32[i] = MAX2(override_color.f32[i], 0.0f);
> - }
> - break;
> - }
> -
> - if (!_mesa_format_has_color_component(mt->format, 3)) {
> - if (_mesa_is_format_integer_color(mt->format))
> - override_color.u32[3] = 1;
> - else
> - override_color.f32[3] = 1.0f;
> - }
> -
> /* Handle linear to SRGB conversion */
> - if (brw->ctx.Color.sRGBEnabled &&
> - _mesa_get_srgb_format_linear(mt->format) != mt->format) {
> + if (brw->ctx.Color.sRGBEnabled && linear_format != mt->format) {
> for (int i = 0; i < 3; i++) {
> override_color.f32[i] =
> util_format_linear_to_srgb_float(override_color.f32[i]);
> }
> }
>
> + union gl_color_union tmp_color;
I think it'd be more descriptive and clearer to use something like:
char tmp_pixel[16];
> +
> + /* Convert the clear color to the surface format and back so that the
> color
> + * returned when sampling is guaranteed to be a value that could be stored
> + * in the surface. For example if the surface is a luminance format and we
> + * clear to 0.5,0.75,0.1,0.2 we want the color to come back as
> + * 0.5,0.5,0.5,1.0. In general the hardware doesn't seem to look at the
> + * surface format when returning the clear color so we need to do this to
> + * implement luminance, intensity and missing components. However it does
> + * seem to look at it in some cases such as to clamp to the range [0,1]
> for
> + * unorm formats. Suprisingly however it doesn't clamp to [0,∞] for the
> + * special float formats that don't have a sign bit.
> + */
> + if (_mesa_is_format_integer_color(linear_format)) {
> + _mesa_pack_uint_rgba_row(linear_format,
> + 1, /* n_pixels */
> + (const GLuint (*)[4]) override_color.u32,
> + &tmp_color);
> + _mesa_unpack_uint_rgba_row(linear_format,
> + 1, /* n_pixels */
> + &tmp_color,
> + (GLuint (*)[4]) override_color.u32);
> + } else {
I think we should let the following packing function handle the sRGB
conversion. I've prototyped the core of this suggestion to something like:
mesa_format mt_format_linear = _mesa_get_srgb_format_linear(mt->format);
bool mt_format_is_srgb = mt->format != mt_format_linear;
mesa_format pack_format = srgb_enabled && mt_format_is_srgb ?
mt->format : linear_format;
-Nanley
> + _mesa_pack_float_rgba_row(linear_format,
> + 1, /* n_pixels */
> + (const GLfloat (*)[4]) override_color.f32,
> + &tmp_color);
> + _mesa_unpack_rgba_row(linear_format,
> + 1, /* n_pixels */
> + &tmp_color,
> + (GLfloat (*)[4]) override_color.f32);
> + }
> +
> return override_color;
> }
> --
> 2.9.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
