From d37bf148d2a20e51d85ff5db93f3b4cb84235c68 Mon Sep 17 00:00:00 2001 From: Alyssa Rosenzweig Date: Fri, 13 Jun 2025 14:30:27 -0400 Subject: [PATCH] nir/lower_blend: fix snorm factor clamping The spec says (emphasis mine): If the color attachment is fixed-point, the components of the source and destination values **AND BLEND FACTORS** are each clamped to [0,1] or [-1,1] respectively for an unsigned normalized or signed normalized color attachment prior to evaluating the blend operations. If the color attachment is floating-point, no clamping occurs. However, neither the CTS nor any hardware implement this semantic. For unsigned normalized formats, the definitions are roughly equivalent (except perhaps around constant colours). 0 <= x <= 1 implies that 0 <= 1 - x <= 1. Therefore if the source/destination colours are clamped to [0, 1], then their complements are also in [0, 1], so clamping any blend factor (except constant colour) has no effect if the source/dest were already clamped. For signed normalized formats, however, this difference matters. -1 <= x <= 1 implies that 0 <= 1 - x <= 2... so to implement the spec text faithfully, we would need to clamp again the complemented colour blend factors to return back to signed normalized range. Software blending implementations can of course do that... but doing so causes CTS fails, as the CTS reference renderer does not do this. This commit adjusts nir_lower_blend to match what actual hardware does, what CTS requires, and what the spec should have said. See https://gitlab.khronos.org/vulkan/vulkan/-/issues/4293 for the spec resolution. Signed-off-by: Alyssa Rosenzweig Reviewed-by: Adam Jackson Part-of: --- src/compiler/nir/nir_lower_blend.c | 60 ++------------------------ src/panfrost/ci/panfrost-g52-fails.txt | 1 - src/panfrost/ci/panfrost-g57-fails.txt | 2 - 3 files changed, 4 insertions(+), 59 deletions(-) diff --git a/src/compiler/nir/nir_lower_blend.c b/src/compiler/nir/nir_lower_blend.c index 2a647839879..adc5a2d8f79 100644 --- a/src/compiler/nir/nir_lower_blend.c +++ b/src/compiler/nir/nir_lower_blend.c @@ -150,47 +150,6 @@ nir_fsat_to_format(nir_builder *b, nir_def *x, enum pipe_format format) return x; } -/* - * The spec says we need to clamp blend factors. However, we don't want to clamp - * unnecessarily, as the clamp might not be optimized out. Check whether - * clamping a blend factor is needed. - */ -static bool -should_clamp_factor(enum pipe_blendfactor factor, bool snorm) -{ - switch (util_blendfactor_without_invert(factor)) { - case PIPE_BLENDFACTOR_ONE: - /* 0, 1 are in [0, 1] and [-1, 1] */ - return false; - - case PIPE_BLENDFACTOR_SRC_COLOR: - case PIPE_BLENDFACTOR_SRC1_COLOR: - case PIPE_BLENDFACTOR_DST_COLOR: - case PIPE_BLENDFACTOR_SRC_ALPHA: - case PIPE_BLENDFACTOR_SRC1_ALPHA: - case PIPE_BLENDFACTOR_DST_ALPHA: - /* Colours are already clamped. For unorm, the complement of something - * clamped is still clamped. But for snorm, this is not true. Clamp for - * snorm only. - */ - return util_blendfactor_is_inverted(factor) && snorm; - - case PIPE_BLENDFACTOR_CONST_COLOR: - case PIPE_BLENDFACTOR_CONST_ALPHA: - /* Constant colours are not yet clamped */ - return true; - - case PIPE_BLENDFACTOR_SRC_ALPHA_SATURATE: - /* For unorm, this is in bounds (and hence so is its complement). For - * snorm, it may not be. - */ - return snorm; - - default: - unreachable("invalid blend factor"); - } -} - static bool channel_uses_dest(nir_lower_blend_channel chan) { @@ -229,9 +188,6 @@ nir_blend_factor( if (util_blendfactor_is_inverted(factor)) f = nir_fadd_imm(b, nir_fneg(b, f), 1.0); - if (should_clamp_factor(factor, util_format_is_snorm(format))) - f = nir_fsat_to_format(b, f, format); - return nir_fmul(b, raw_scalar, f); } @@ -407,20 +363,12 @@ nir_blend( /* Fixed-point framebuffers require their inputs clamped. */ enum pipe_format format = options->format[rt]; - /* From section 17.3.6 "Blending" of the OpenGL 4.5 spec: - * - * If the color buffer is fixed-point, the components of the source and - * destination values and blend factors are each clamped to [0, 1] or - * [-1, 1] respectively for an unsigned normalized or signed normalized - * color buffer prior to evaluating the blend equation. If the color - * buffer is floating-point, no clamping occurs. - * - * Blend factors are clamped at the time of their use to ensure we properly - * clamp negative constant colours with signed normalized formats and - * ONE_MINUS_CONSTANT_* factors. Notice that -1 is in [-1, 1] but 1 - (-1) = - * 2 is not in [-1, 1] and should be clamped to 1. + /* The input colours need to be clamped to the format. Contrary to the + * OpenGL/Vulkan specs, it really is the inputs that get clamped and not the + * intermediate blend factors. This matches the CTS and hardware behaviour. */ src = nir_fsat_to_format(b, src, format); + bconst = nir_fsat_to_format(b, bconst, format); if (src1) src1 = nir_fsat_to_format(b, src1, format); diff --git a/src/panfrost/ci/panfrost-g52-fails.txt b/src/panfrost/ci/panfrost-g52-fails.txt index c958dc9411e..177163518ab 100644 --- a/src/panfrost/ci/panfrost-g52-fails.txt +++ b/src/panfrost/ci/panfrost-g52-fails.txt @@ -97,7 +97,6 @@ spec@ext_framebuffer_object@fbo-blending-formats@GL_LUMINANCE8_ALPHA8,Fail spec@ext_framebuffer_object@fbo-blending-formats@GL_LUMINANCE8,Fail spec@ext_framebuffer_object@fbo-blending-formats@GL_LUMINANCE_ALPHA,Fail spec@ext_framebuffer_object@fbo-blending-formats@GL_LUMINANCE,Fail -spec@ext_framebuffer_object@fbo-blending-snorm,Fail spec@ext_framebuffer_object@getteximage-formats init-by-clear-and-render,Fail spec@ext_framebuffer_object@getteximage-formats init-by-rendering,Fail spec@ext_transform_feedback2@draw-auto,Fail diff --git a/src/panfrost/ci/panfrost-g57-fails.txt b/src/panfrost/ci/panfrost-g57-fails.txt index e37898fe6b3..d75f24032f6 100644 --- a/src/panfrost/ci/panfrost-g57-fails.txt +++ b/src/panfrost/ci/panfrost-g57-fails.txt @@ -87,7 +87,6 @@ spec@ext_framebuffer_object@fbo-blending-formats@GL_INTENSITY,Fail spec@ext_framebuffer_object@fbo-blending-formats@GL_LUMINANCE4,Fail spec@ext_framebuffer_object@fbo-blending-formats@GL_LUMINANCE8,Fail spec@ext_framebuffer_object@fbo-blending-formats@GL_LUMINANCE,Fail -spec@ext_framebuffer_object@fbo-blending-snorm,Fail spec@ext_framebuffer_object@getteximage-formats init-by-clear-and-render,Fail spec@ext_framebuffer_object@getteximage-formats init-by-rendering,Fail spec@ext_transform_feedback2@draw-auto,Fail @@ -416,7 +415,6 @@ afbcp-spec@ext_framebuffer_object@fbo-blending-formats@GL_INTENSITY,Fail afbcp-spec@ext_framebuffer_object@fbo-blending-formats@GL_LUMINANCE4,Fail afbcp-spec@ext_framebuffer_object@fbo-blending-formats@GL_LUMINANCE8,Fail afbcp-spec@ext_framebuffer_object@fbo-blending-formats@GL_LUMINANCE,Fail -afbcp-spec@ext_framebuffer_object@fbo-blending-snorm,Fail afbcp-spec@ext_framebuffer_object@getteximage-formats init-by-clear-and-render,Fail afbcp-spec@ext_framebuffer_object@getteximage-formats init-by-rendering,Fail afbcp-spec@ext_transform_feedback2@draw-auto,Fail