On Fri, Apr 20, 2018 at 03:18:18PM -0700, Rafael Antognolli wrote:
> On Fri, Apr 20, 2018 at 03:01:44PM -0700, Nanley Chery wrote:
> > On Fri, Apr 20, 2018 at 01:35:39PM -0700, Rafael Antognolli wrote:
> > > On Wed, Apr 11, 2018 at 01:42:24PM -0700, Nanley Chery wrote:
> > > > This getter allows CNL to sample from fast-cleared sRGB textures
> > > > correctly.
> > >
> > > I think it might be worth mentioning in the commit message that the
> > > helper both returns the clear color for inline use, and updates the
> > > pointer to the indirect clear color buffer.
> > >
> >
> > Sure, here's what I'm planning to update it to:
> >
> > It returns the both the inline clear color and a clear address
> > which points to the indirect clear color buffer (or NULL if
> > unused/non-existent). This getter allows CNL to sample from
> > fast-cleared sRGB textures correctly by doing the needed
> > sRGB-decode on the clear color (inline) and making the indirect
> > clear color buffer unused.
>
> That description is great, thanks!
>
> > > > ---
> > > > src/mesa/drivers/dri/i965/brw_blorp.c | 13 ++++-------
> > > > src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 7 +++---
> > > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 29
> > > > ++++++++++++++++++++++++
> > > > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 8 +++++++
> > > > 4 files changed, 46 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > index a1882abb7cb..48022bb1c4f 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > @@ -166,7 +166,11 @@ blorp_surf_for_miptree(struct brw_context *brw,
> > > > /* We only really need a clear color if we also have an auxiliary
> > > > * surface. Without one, it does nothing.
> > > > */
> > > > - surf->clear_color = mt->fast_clear_color;
> > > > + surf->clear_color =
> > > > + intel_miptree_get_clear_color(devinfo, mt, mt->surf.format,
> > > > + !is_render_target, (struct
> > > > brw_bo **)
> > > > + &surf->clear_color_addr.buffer,
> > > > + &surf->clear_color_addr.offset);
> > > >
> > > > struct intel_miptree_aux_buffer *aux_buf =
> > > > intel_miptree_get_aux_buffer(mt);
> > > > @@ -178,13 +182,6 @@ blorp_surf_for_miptree(struct brw_context *brw,
> > > >
> > > > surf->aux_addr.buffer = aux_buf->bo;
> > > > surf->aux_addr.offset = aux_buf->offset;
> > > > -
> > > > - if (devinfo->gen >= 10) {
> > > > - surf->clear_color_addr = (struct blorp_address) {
> > > > - .buffer = aux_buf->clear_color_bo,
> > > > - .offset = aux_buf->clear_color_offset,
> > > > - };
> > > > - }
> > > > } else {
> > > > surf->aux_addr = (struct blorp_address) {
> > > > .buffer = NULL,
> > > > 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 3c70dbcc110..fb8e5942a11 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > > @@ -167,9 +167,10 @@ 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_bo = aux_buf->clear_color_bo;
> > > > - clear_offset = aux_buf->clear_color_offset;
> > > > - clear_color = mt->fast_clear_color;
> > > > + clear_color =
> > > > + intel_miptree_get_clear_color(devinfo, mt, view.format,
> > > > + view.usage &
> > > > ISL_SURF_USAGE_TEXTURE_BIT,
> > > > + &clear_bo, &clear_offset);
> > > > }
> > > >
> > > > void *state = brw_state_batch(brw,
> > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > index c5791835409..88468399e1b 100644
> > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > @@ -46,6 +46,9 @@
> > > > #include "main/texcompress_etc.h"
> > > > #include "main/teximage.h"
> > > > #include "main/streaming-load-memcpy.h"
> > > > +
> > > > +#include "util/format_srgb.h"
> > > > +
> > > > #include "x86/common_x86_asm.h"
> > > >
> > > > #define FILE_DEBUG_FLAG DEBUG_MIPTREE
> > > > @@ -3802,3 +3805,29 @@ intel_miptree_set_depth_clear_value(struct
> > > > brw_context *brw,
> > > > }
> > > > return false;
> > > > }
> > > > +
> > > > +union isl_color_value
> > > > +intel_miptree_get_clear_color(const struct gen_device_info *devinfo,
> > > > + const struct intel_mipmap_tree *mt,
> > > > + enum isl_format view_format, bool
> > > > sampling,
> > > > + struct brw_bo **clear_color_bo,
> > > > + uint32_t *clear_color_offset)
> > > > +{
> > > > + assert(mt->aux_buf);
> > > > +
> > > > + /* The gen10 sampler doesn't gamma-correct the clear color. */
> > > > + if (devinfo->gen == 10 && isl_format_is_srgb(view_format) &&
> > > > sampling) {
> > > > + union isl_color_value srgb_decoded_value = mt->fast_clear_color;
> > > > + for (unsigned i = 0; i < 3; i++) {
> > >
> > > Maybe this is a dumb question, but don't we have a alpha component on
> > > srgb? Or it doesn't require conversion?
> > >
> >
> > Not a dumb question at all. I had to look into this when learning about
> > sRGB myself. As far as I understand it, the alpha channel is generally
> > kept linear to have more accurate blending. So here we just keep the
> > original value when initializing srgb_decoded_value.
>
> Ah, cool, so I think a simple note mentioning that would be good too.
>
The diff due to my note looks like this:
- /* The gen10 sampler doesn't gamma-correct the clear color. */
if (devinfo->gen == 10 && isl_format_is_srgb(view_format) && sampling) {
+ /* The gen10 sampler doesn't gamma-correct the clear color. In this case,
+ * we switch to using the inline clear color and do the sRGB color
+ * conversion process defined in the OpenGL spec. The red, green, and
+ * blue channels take part in gamma correction, while the alpha channel
+ * is unchanged.
+ */
union isl_color_value srgb_decoded_value = mt->fast_clear_color;
-Nanley
> With this and the description you mentioned above, this patch is
>
> Reviewed-by: Rafael Antognolli <rafael.antogno...@intel.com>
>
> >
> > > > + srgb_decoded_value.f32[i] =
> > > > +
> > > > util_format_srgb_to_linear_float(mt->fast_clear_color.f32[i]);
> > > > + }
> > > > + *clear_color_bo = 0;
> > > > + *clear_color_offset = 0;
> > > > + return srgb_decoded_value;
> > > > + } else {
> > > > + *clear_color_bo = mt->aux_buf->clear_color_bo;
> > > > + *clear_color_offset = mt->aux_buf->clear_color_offset;
> > > > + return mt->fast_clear_color;
> > > > + }
> > > > +}
> > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > index 643de962d31..bb059cf4e8f 100644
> > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > @@ -735,6 +735,14 @@ intel_miptree_set_clear_color(struct brw_context
> > > > *brw,
> > > > struct intel_mipmap_tree *mt,
> > > > const union gl_color_union *color);
> > > >
> > > > +/* Get a clear color suitable for filling out an ISL surface state. */
> > > > +union isl_color_value
> > > > +intel_miptree_get_clear_color(const struct gen_device_info *devinfo,
> > > > + const struct intel_mipmap_tree *mt,
> > > > + enum isl_format view_format, bool
> > > > sampling,
> > > > + struct brw_bo **clear_color_bo,
> > > > + uint32_t *clear_color_offset);
> > > > +
> > > > bool
> > > > intel_miptree_set_depth_clear_value(struct brw_context *brw,
> > > > struct intel_mipmap_tree *mt,
> > > > --
> > > > 2.16.2
> > > >
> > > > _______________________________________________
> > > > 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
