<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Oct 19, 2016 at 2:29 PM, Ben Widawsky <<a href="mailto:benjamin.widawsky@intel.com" target="_blank">benjamin.widawsky@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 16-10-11 22:26:33, Topi Pohjolainen wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> And fix a mangled comment while at it. Signed-off-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com" target="_blank">topi.pohjolainen@intel.com</a>> CC: Ben Widawsky <<a href="mailto:benjamin.widawsky@intel.com" target="_blank">benjamin.widawsky@intel.com</a>> CC: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com" target="_blank">jason.ekstrand@intel.com</a>> --- 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_color, + &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) </blockquote> </div></div> 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) </blockquote><div> </div><div>Seems reasonable. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> { - 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) +{ </blockquote></div></div></blockquote><div> </div><div>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. </div><div>This patch is </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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; } </blockquote> </div></div> Pretty sure you can remove the mt->fast_clear_color_value field after this change. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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, </blockquote> I don't see any other problems Reviewed-by: Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> -- Ben Widawsky, Intel Open Source Technology Center<div class="HOEnZb"><div class="h5"> _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>