On Wed, Oct 19, 2016 at 2:29 PM, Ben Widawsky <benjamin.widaw...@intel.com>
wrote:
> On 16-10-11 22:26:33, Topi Pohjolainen wrote:
>
>> And fix a mangled comment while at it.
>>
>> Signed-off-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
>> CC: Ben Widawsky <benjamin.widaw...@intel.com>
>> CC: Jason Ekstrand <jason.ekstr...@intel.com>
>> ---
>> src/mesa/drivers/dri/i965/brw_blorp.c | 7 +++-
>> src/mesa/drivers/dri/i965/brw_meta_util.c | 56
>> +++++++++++++++++--------------
>> src/mesa/drivers/dri/i965/brw_meta_util.h | 10 ++++--
>> 3 files changed, 45 insertions(+), 28 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
>> b/src/mesa/drivers/dri/i965/brw_blorp.c
>> index b6c27982..f301192 100644
>> --- a/src/mesa/drivers/dri/i965/brw_blorp.c
>> +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
>> @@ -809,12 +809,17 @@ do_single_blorp_clear(struct brw_context *brw,
>> struct gl_framebuffer *fb,
>> brw, irb->mt);
>>
>> if (can_fast_clear) {
>> + union gl_color_union override_color;
>> + brw_meta_convert_fast_clear_color(brw, irb->mt,
>> &ctx->Color.ClearColor,
>> + &override_color);
>> +
>> /* Record the clear color in the miptree so that it will be
>> * programmed in SURFACE_STATE by later rendering and resolve
>> * operations.
>> */
>> const bool color_updated = brw_meta_set_fast_clear_color(
>> - brw, irb->mt,
>> &ctx->Color.ClearColor);
>> + brw, &irb->mt->gen9_fast_clear_colo
>> r,
>> + &override_color);
>>
>> /* If the buffer is already in INTEL_FAST_CLEAR_STATE_CLEAR, and the
>> * buffer isn't compressed, the clear is redundant and can be
>> skipped.
>> diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.c
>> b/src/mesa/drivers/dri/i965/brw_meta_util.c
>> index 499b6ea..1badea1 100644
>> --- a/src/mesa/drivers/dri/i965/brw_meta_util.c
>> +++ b/src/mesa/drivers/dri/i965/brw_meta_util.c
>> @@ -372,15 +372,14 @@ brw_is_color_fast_clear_compatible(struct
>> brw_context *brw,
>> /**
>> * Convert the given color to a bitfield suitable for ORing into DWORD 7
>> of
>> * SURFACE_STATE (DWORD 12-15 on SKL+).
>> - *
>> - * Returned boolean tells if the given color differs from the stored.
>> */
>> -bool
>> -brw_meta_set_fast_clear_color(struct brw_context *brw,
>> - struct intel_mipmap_tree *mt,
>> - const union gl_color_union *color)
>> +void
>> +brw_meta_convert_fast_clear_color(const struct brw_context *brw,
>> + const struct intel_mipmap_tree *mt,
>> + const union gl_color_union *color,
>> + union gl_color_union *override_color)
>>
>
> I think it makes a lot more sense to return the color, my suggestion would
> be
> (but whatever you like)...
> union gl_color_union
> brw_meta_convert_fast_clear_color(const struct brw_context *brw,
> uint32_t format
> const union gl_color_union *color)
>
Seems reasonable.
> {
>> - union gl_color_union override_color = *color;
>> + *override_color = *color;
>>
>> /* 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
>> @@ -388,55 +387,62 @@ brw_meta_set_fast_clear_color(struct brw_context
>> *brw,
>> */
>> switch (_mesa_get_format_base_format(mt->format)) {
>> case GL_INTENSITY:
>> - override_color.ui[3] = override_color.ui[0];
>> + override_color->ui[3] = override_color->ui[0];
>> /* flow through */
>> case GL_LUMINANCE:
>> case GL_LUMINANCE_ALPHA:
>> - override_color.ui[1] = override_color.ui[0];
>> - override_color.ui[2] = override_color.ui[0];
>> + override_color->ui[1] = override_color->ui[0];
>> + override_color->ui[2] = override_color->ui[0];
>> break;
>> default:
>> for (int i = 0; i < 3; i++) {
>> if (!_mesa_format_has_color_component(mt->format, i))
>> - override_color.ui[i] = 0;
>> + override_color->ui[i] = 0;
>> }
>> break;
>> }
>>
>> if (!_mesa_format_has_color_component(mt->format, 3)) {
>> if (_mesa_is_format_integer_color(mt->format))
>> - override_color.ui[3] = 1;
>> + override_color->ui[3] = 1;
>> else
>> - override_color.f[3] = 1.0f;
>> + override_color->f[3] = 1.0f;
>> }
>>
>> - /* Handle linear→SRGB conversion */
>> + /* Handle linear to SRGB conversion */
>> if (brw->ctx.Color.sRGBEnabled &&
>> _mesa_get_srgb_format_linear(mt->format) != mt->format) {
>> for (int i = 0; i < 3; i++) {
>> - override_color.f[i] =
>> - util_format_linear_to_srgb_float(override_color.f[i]);
>> + override_color->f[i] =
>> + util_format_linear_to_srgb_float(override_color->f[i]);
>> }
>> }
>> +}
>>
>> +/* Returned boolean tells if the given color differs from the current. */
>> +bool
>> +brw_meta_set_fast_clear_color(struct brw_context *brw,
>> + union gl_color_union *curr_color,
>> + const union gl_color_union *new_color)
>> +{
>>
>
Ugh... I really hate that this function even exists. Why are we storing
clear color as "the bits you shove in the hardware" and not as 4
ints/floats????? In any case, I've got plans to fix it eventually so don't
worry about it. Rant over.
This patch is
Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
> bool updated;
>> +
>> if (brw->gen >= 9) {
>> - updated = memcmp(&mt->gen9_fast_clear_color, &override_color,
>> - sizeof(mt->gen9_fast_clear_color));
>> - mt->gen9_fast_clear_color = override_color;
>> + updated = memcmp(curr_color, new_color, sizeof(*curr_color));
>> + *curr_color = *new_color;
>> } else {
>> - const uint32_t old_color_value = mt->fast_clear_color_value;
>> + const uint32_t old_color_value = *(uint32_t *)curr_color;
>> + uint32_t adjusted = 0;
>>
>> - mt->fast_clear_color_value = 0;
>> for (int i = 0; i < 4; i++) {
>> /* Testing for non-0 works for integer and float colors */
>> - if (override_color.f[i] != 0.0f) {
>> - mt->fast_clear_color_value |=
>> - 1 << (GEN7_SURFACE_CLEAR_COLOR_SHIFT + (3 - i));
>> + if (new_color->f[i] != 0.0f) {
>> + adjusted |= 1 << (GEN7_SURFACE_CLEAR_COLOR_SHIFT + (3 - i));
>> }
>> }
>>
>> - updated = (old_color_value != mt->fast_clear_color_value);
>> + updated = (old_color_value != adjusted);
>> + *(uint32_t *)curr_color = adjusted;
>> }
>>
>
> Pretty sure you can remove the mt->fast_clear_color_value field after this
> change.
>
>
>> return updated;
>> diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.h
>> b/src/mesa/drivers/dri/i965/brw_meta_util.h
>> index b9c4eac..c0e3100 100644
>> --- a/src/mesa/drivers/dri/i965/brw_meta_util.h
>> +++ b/src/mesa/drivers/dri/i965/brw_meta_util.h
>> @@ -42,10 +42,16 @@ brw_meta_mirror_clip_and_scissor(const struct
>> gl_context *ctx,
>> GLfloat *dstX1, GLfloat *dstY1,
>> bool *mirror_x, bool *mirror_y);
>>
>> +void
>> +brw_meta_convert_fast_clear_color(const struct brw_context *brw,
>> + const struct intel_mipmap_tree *mt,
>> + const union gl_color_union *color,
>> + union gl_color_union *override_color);
>> +
>> bool
>> brw_meta_set_fast_clear_color(struct brw_context *brw,
>> - struct intel_mipmap_tree *mt,
>> - const union gl_color_union *color);
>> + union gl_color_union *curr_color,
>> + const union gl_color_union *new_color);
>>
>> bool
>> brw_is_color_fast_clear_compatible(struct brw_context *brw,
>>
>
> I don't see any other problems
>
> Reviewed-by: Ben Widawsky <b...@bwidawsk.net>
>
> --
> Ben Widawsky, Intel Open Source Technology Center
>
> _______________________________________________
> 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
