[Mesa-dev] [PATCH 2/2] i965/cnl: Correctly sample from fast-cleared sRGB textures
Nanley Chery
nanleychery at gmail.com
Tue Nov 21 01:18:12 UTC 2017
On Mon, Nov 20, 2017 at 04:03:20PM -0800, Jason Ekstrand wrote:
> On Mon, Nov 20, 2017 at 3:48 PM, Nanley Chery <nanleychery at gmail.com> wrote:
>
> > On Mon, Nov 20, 2017 at 02:55:28PM -0800, Jason Ekstrand wrote:
> > > On Mon, Nov 20, 2017 at 2:10 PM, Nanley Chery <nanleychery at gmail.com>
> > wrote:
> > >
> > > > On Mon, Nov 20, 2017 at 01:54:39PM -0800, Nanley Chery wrote:
> > > > > When sampling from fast-cleared sRGB textures on gen10, the hardware
> > > > > will not decode the clear color to linear. We must therefore do it in
> > > > > software.
> > > > >
> > > >
> > > > I should've included the tests that are now passing (added in locally):
> > > >
> > > > * spec at arb_framebuffer_srgb@arb_framebuffer_srgb-fast-clear-blend
> > > > * spec at arb_framebuffer_srgb@fbo-fast-clear
> > > > * spec at arb_framebuffer_srgb@msaa-fast-clear
> > > > * spec at ext_texture_srgb@fbo-fast-clear
> > > > * spec at ext_texture_srgb@multisample-fast-clear gl_ext_texture_srgb
> > > >
> > > > -Nanley
> > > >
> > > > > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > > > > ---
> > > > > src/mesa/drivers/dri/i965/brw_blorp.c | 7 +++++
> > > > > src/mesa/drivers/dri/i965/brw_meta_util.c | 38
> > > > ++++++++++++++++++++++++
> > > > > src/mesa/drivers/dri/i965/brw_meta_util.h | 5 ++++
> > > > > src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 9 +++++-
> > > > > 4 files changed, 58 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > > index 38284d3b85..b10e9b95b7 100644
> > > > > --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > > @@ -328,6 +328,13 @@ brw_blorp_blit_miptrees(struct brw_context *brw,
> > > > > struct blorp_surf src_surf, dst_surf;
> > > > > blorp_surf_for_miptree(brw, &src_surf, src_mt, src_aux_usage,
> > false,
> > > > > &src_level, src_layer, 1, &tmp_surfs[0]);
> > > > > + if (devinfo->gen >= 10 && isl_format_is_srgb(src_isl_format) &&
> > > > > + src_clear_supported) {
> > > > > + src_surf.clear_color =
> > > > > + gen10_convert_srgb_fast_clear_color(devinfo,
> > src_isl_format,
> > > > > +
> > src_mt->fast_clear_color);
> > > > > + }
> > > > > +
> > > > > blorp_surf_for_miptree(brw, &dst_surf, dst_mt, dst_aux_usage,
> > true,
> > > > > &dst_level, dst_layer, 1, &tmp_surfs[1]);
> > > > >
> > > > > diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.c
> > > > b/src/mesa/drivers/dri/i965/brw_meta_util.c
> > > > > index d292f5a8e2..97c6114f8a 100644
> > > > > --- a/src/mesa/drivers/dri/i965/brw_meta_util.c
> > > > > +++ b/src/mesa/drivers/dri/i965/brw_meta_util.c
> > > > > @@ -28,6 +28,7 @@
> > > > > #include "brw_state.h"
> > > > > #include "main/blend.h"
> > > > > #include "main/fbobject.h"
> > > > > +#include "main/format_utils.h"
> > > > > #include "util/format_srgb.h"
> > > > >
> > > > > /**
> > > > > @@ -315,6 +316,43 @@ brw_is_color_fast_clear_compatible(struct
> > > > brw_context *brw,
> > > > > return true;
> > > > > }
> > > > >
> > > > > +/* When sampling from fast-cleared sRGB textures on gen10, the
> > hardware
> > > > > + * will not decode the clear color to linear. We must therefore do
> > it in
> > > > > + * software.
> > > > > + */
> > > > > +union isl_color_value
> > > > > +gen10_convert_srgb_fast_clear_color(const struct gen_device_info
> > > > *devinfo,
> > > > > + enum isl_format format,
> > > > > + const union isl_color_value
> > color)
> > > > > +{
> > > > > + assert(devinfo->gen >= 10);
> > > > > + assert(isl_format_is_srgb(format));
> > > > > + assert(isl_format_supports_ccs_d(devinfo, format));
> > > > > +
> > > > > + /* All the CCS-supported sRGB textures in GL are 4-channel,
> > > > 8-bit/channel,
> > > > > + * UNORM formats.
> > > > > + */
> > > > > + assert(isl_format_get_num_channels(format) == 4);
> > > > > + assert(isl_format_channels_have_size(format, 8));
> > > > > + assert(isl_format_has_unorm_channel(format));
> > > >
> > >
> > > There are exactly 4 ISL formats which match this. You could just assert
> > > that it's one of the four and drop the other 5 asserts. It doesn't
> > really
> > > matter, but this is rather complicated.
> > >
> > >
> >
> > I agree. I'll fix this locally and send out a v2 after getting more of
> > your feedback.
> >
> > > > > +
> > > > > + /* According to do_single_blorp_clear(), fast-clears for
> > > > texture-views are
> > > > > + * disabled. This means that we only have to do
> > channel-to-channel
> > > > format
> > > > > + * conversions.
> > > > > + */
> > > > > + union isl_color_value override_color = color;
> > > > > + for (unsigned i = 0; i < 3; i++) {
> > > > > + /* According to brw_is_color_fast_clear_compatible(), only
> > > > floating-point
> > > > > + * fast-clears have been enabled, so it is safe to rely on
> > > > > + * isl_color_value::f32.
> > > > > + */
> > > >
> > >
> > > There is no such thing as integer sRGB, it wouldn't even make sense.
> > >
> > >
> >
> > Wouldn't we run into an issue if integer fast-clears are enabled and
> > texture views of different formats are allowed to be fast-cleared? The
> > specific example I have in mind is:
> > * Fast clear an R8G8B8A8_UINT to (255,255,255,255)
> > * Sample from the texture as SRGB8_ALPHA8
> >
> > We'd need to use the u32 and directly instead of doing the conversion
> > with the f32. I hope I've explained that clearly.
> >
>
> Yes, but in that case, this function will have to change radically (if we
> keep it at all).
>
>
True. How's this:
According to brw_is_color_fast_clear_compatible(), only floating-point
fast-clears have been enabled. Therefore, only one format conversion
path is necessesary.
> > > > > + const uint8_t srgb_unorm8 = _mesa_float_to_unorm(color.
> > f32[i],
> > > > 8);
> > > > > + override_color.f32[i] =
> > > > > + util_format_srgb_8unorm_to_linear_float(srgb_unorm8);
> > > >
> > >
> > > Any particular reason why you're going through 8-bit? I can think of a
> > few
> > > reasons why you might but I don't know if any of them actually matter in
> > > practice.
> > >
> > >
> >
> > As opposed to writing util_format_srgb_to_linear_float()? We should get
> > the same result going through 8-bit and I did a quick skim of the repo
> > and couldn't find any other potential users of
> > util_format_srgb_to_linear_float. I could write that if you'd like
> > though.
> >
>
> I've got it in a branch somewhere...
>
> https://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=wip/i965-packed-clear&id=e6208a8c195441b65e6da648203b332af4d0c0fb
>
> There it is. I don't really care that much; laziness is as good a reason
> as any. :P
>
>
Cool. I'm not really sure how to handle authorship except to ask. Is
this patch in a reviewable state? If so, I guess I'll send it out as
part of my series. I'd either add-on an rb or reply to it with review
comments.
-Nanley
> > -Nanley
> >
> > > > > + }
> > > > > + return override_color;
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * Convert the given color to a bitfield suitable for ORing into
> > DWORD
> > > > 7 of
> > > > > * SURFACE_STATE (DWORD 12-15 on SKL+).
> > > > > diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.h
> > > > b/src/mesa/drivers/dri/i965/brw_meta_util.h
> > > > > index 4b3408df15..ee0b3bd3e1 100644
> > > > > --- a/src/mesa/drivers/dri/i965/brw_meta_util.h
> > > > > +++ b/src/mesa/drivers/dri/i965/brw_meta_util.h
> > > > > @@ -42,6 +42,11 @@ brw_meta_mirror_clip_and_scissor(const struct
> > > > gl_context *ctx,
> > > > > GLfloat *dstX1, GLfloat *dstY1,
> > > > > bool *mirror_x, bool *mirror_y);
> > > > >
> > > > > +union isl_color_value
> > > > > +gen10_convert_srgb_fast_clear_color(const struct gen_device_info
> > > > *devinfo,
> > > > > + enum isl_format format,
> > > > > + const union isl_color_value
> > color);
> > > > > +
> > > > > union isl_color_value
> > > > > brw_meta_convert_fast_clear_color(const struct brw_context *brw,
> > > > > const struct intel_mipmap_tree
> > *mt,
> > > > > 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 adf60a840b..315b96e9c4 100644
> > > > > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > > > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > > > @@ -51,6 +51,7 @@
> > > > > #include "intel_buffer_objects.h"
> > > > >
> > > > > #include "brw_context.h"
> > > > > +#include "brw_meta_util.h"
> > > > > #include "brw_state.h"
> > > > > #include "brw_defines.h"
> > > > > #include "brw_wm.h"
> > > > > @@ -174,7 +175,13 @@ brw_emit_surface_state(struct brw_context *brw,
> > > > > /* We only really need a clear color if we also have an
> > auxiliary
> > > > > * surface. Without one, it does nothing.
> > > > > */
> > > > > - clear_color = mt->fast_clear_color;
> > > > > + if (devinfo->gen >= 10 && isl_format_is_srgb(view.format)) {
> > > > > + clear_color =
> > > > > + gen10_convert_srgb_fast_clear_color(devinfo,
> > view.format,
> > > > > +
> > mt->fast_clear_color);
> > > > > + } else {
> > > > > + clear_color = mt->fast_clear_color;
> > > > > + }
> > > > > }
> > > > >
> > > > > void *state = brw_state_batch(brw,
> > > > > --
> > > > > 2.14.3
> > > > >
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >
> >
More information about the mesa-dev
mailing list