On Wed, May 16, 2018 at 09:02:51AM +0300, Pohjolainen, Topi wrote:
> On Tue, May 08, 2018 at 02:05:37PM -0700, Nanley Chery wrote:
> > On Tue, May 08, 2018 at 08:31:39AM +0300, Pohjolainen, Topi wrote:
> > > On Mon, May 07, 2018 at 10:11:39AM -0700, Nanley Chery wrote:
> > > > On Mon, May 07, 2018 at 11:30:15AM +0300, Pohjolainen, Topi wrote:
> > > > > On Thu, May 03, 2018 at 12:03:51PM -0700, Nanley Chery wrote:
> > > > > > The indirect clear color isn't correctly tracked in
> > > > > > intel_miptree::fast_clear_color. The initial value of
> > > > > > ::fast_clear_color
> > > > > > is zero, while that of the indirect clear color is undefined or
> > > > > > non-zero.
> > > > > >
> > > > > > Topi Pohjolainen discovered this issue with MCS buffers. This issue
> > > > > > is
> > > > > > apparent when fast-clearing an MCS buffer for the first time with
> > > > > > glClearColor = {0.0,}. Although the indirect clear color is
> > > > > > non-zero,
> > > > > > the initial aux state of the MCS is CLEAR and the tracked clear
> > > > > > color is
> > > > > > zero, so we avoid updating the indirect clear color with {0.0,}.
> > > > > >
> > > > > > Make the indirect clear color match the initial value of
> > > > > > ::fast_clear_color.
> > > > > >
> > > > > > ---
> > > > > >
> > > > > > Hey Topi,
> > > > > >
> > > > > > Just FYI, this patch should fix the MCS bug you reported earlier.
> > > > > >
> > > > > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 33
> > > > > > ++++++++++++++++++---------
> > > > > > 1 file changed, 22 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > > index 5d3ee569bd8..e70c9ff1ef4 100644
> > > > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > > @@ -978,11 +978,11 @@ create_ccs_buf_for_image(struct brw_context
> > > > > > *brw,
> > > > > > * system with CCS, we don't have the extra space at the end of
> > > > > > the aux
> > > > > > * buffer. So create a new bo here that will store that clear
> > > > > > color.
> > > > > > */
> > > > > > - const struct gen_device_info *devinfo = &brw->screen->devinfo;
> > > > > > - if (devinfo->gen >= 10) {
> > > > > > + if (brw->isl_dev.ss.clear_color_state_size > 0) {
> > > > > > mt->aux_buf->clear_color_bo =
> > > > > > - brw_bo_alloc(brw->bufmgr, "clear_color_bo",
> > > > > > - brw->isl_dev.ss.clear_color_state_size);
> > > > > > + brw_bo_alloc_tiled(brw->bufmgr, "clear_color_bo",
> > > > > > + brw->isl_dev.ss.clear_color_state_size,
> > > > > > + I915_TILING_NONE, 0, BO_ALLOC_ZEROED);
> > > > > > if (!mt->aux_buf->clear_color_bo) {
> > > > > > free(mt->aux_buf);
> > > > > > mt->aux_buf = NULL;
> > > > > > @@ -1673,9 +1673,9 @@ intel_alloc_aux_buffer(struct brw_context
> > > > > > *brw,
> > > > > >
> > > > > > buf->size = aux_surf->size;
> > > > > >
> > > > > > - const struct gen_device_info *devinfo = &brw->screen->devinfo;
> > > > > > - if (devinfo->gen >= 10) {
> > > > > > - /* On CNL, instead of setting the clear color in the
> > > > > > SURFACE_STATE, we
> > > > > > + const bool has_indirect_clear =
> > > > > > brw->isl_dev.ss.clear_color_state_size > 0;
> > > > > > + if (has_indirect_clear) {
> > > > > > + /* On CNL+, instead of setting the clear color in the
> > > > > > SURFACE_STATE, we
> > > > > > * will set a pointer to a dword somewhere that contains the
> > > > > > color. So,
> > > > > > * allocate the space for the clear color value here on the
> > > > > > aux buffer.
> > > > > > */
> > > > > > @@ -1698,7 +1698,8 @@ intel_alloc_aux_buffer(struct brw_context
> > > > > > *brw,
> > > > > > }
> > > > > >
> > > > > > /* Initialize the bo to the desired value */
> > > > > > - if (wants_memset) {
> > > > > > + const bool needs_memset = wants_memset || has_indirect_clear;
> > > > > > + if (needs_memset) {
> > > > > > assert(!(alloc_flags & BO_ALLOC_BUSY));
> > > > > >
> > > > > > void *map = brw_bo_map(brw, buf->bo, MAP_WRITE | MAP_RAW);
> > > > > > @@ -1706,11 +1707,21 @@ intel_alloc_aux_buffer(struct brw_context
> > > > > > *brw,
> > > > > > intel_miptree_aux_buffer_free(buf);
> > > > > > return NULL;
> > > > > > }
> > > > > > - memset(map, memset_value, mt->aux_buf->size);
> > > > > > +
> > > > > > + /* Memset the aux_surf portion of the BO. */
> > > > > > + if (wants_memset)
> > > > > > + memset(map, memset_value, aux_surf->size);
> > > > > > +
> > > > > > + /* Zero the indirect clear color to match
> > > > > > ::fast_clear_color. */
> > > > > > + if (has_indirect_clear) {
> > > > > > + memset((char *)map + buf->clear_color_offset, 0,
> > > > > > + brw->isl_dev.ss.clear_color_state_size);
> > > > > > + }
> > > > > > +
> > > > > > brw_bo_unmap(buf->bo);
> > > > > > }
> > > > > >
> > > > > > - if (devinfo->gen >= 10) {
> > > > > > + if (has_indirect_clear) {
> > > > > > buf->clear_color_bo = buf->bo;
> > > > > > brw_bo_reference(buf->clear_color_bo);
> > > > > > }
> > > > > > @@ -1869,7 +1880,7 @@ intel_miptree_alloc_hiz(struct brw_context
> > > > > > *brw,
> > > > > > isl_surf_get_hiz_surf(&brw->isl_dev, &mt->surf,
> > > > > > &temp_hiz_surf);
> > > > > > assert(ok);
> > > > > >
> > > > > > - const uint32_t alloc_flags = BO_ALLOC_BUSY;
> > > > > > + const uint32_t alloc_flags = 0;
> > > > >
> > > > > This change is unrelated (and really should be _BUSY as we don't
> > > > > memset() in
> > > > > case of hiz), right?
> > > > >
> > > >
> > > > This change is needed because the HiZ buffer's clear value/color becomes
> > > > indirect on gen10+. Since we allocate the indirect clear color as part
> > > > of the auxiliary surface, we will memset this bo.
> > >
> > > But isn't this dropping it for gen < 10 also until patch 6?
> > >
> >
> > Yes, it is. I tried not to optimize the case for gen < 10 for
> > simplicity. Since we add it back in patch 6, it should be okay right?
> > I'm working off the assumption that we won't revert patch 6 in the
> > future...
>
> Do you mind adding a small note to the commit message? Anyway:
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolai...@ointel.com>
>
Thanks! I've added a note.
-Nanley
> >
> > By the way, I had to fix a rebase conflict today. Upstream now avoids
> > memsetting the indirect clear color for MCS to 0xFF. I've updated the
> > commit message so that uses of "non-zero" are either dropped or replaced
> > with "undefined".
> >
> > -Nanley
> >
> > > >
> > > > > Otherwise this looks good and given there isn't any major change in
> > > > > the
> > > > > previous patch, this is:
> > > > >
> > > > > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
> > > > >
> > > >
> > > > Thanks!
> > > >
> > > > -Nanley
> > > >
> > > > > > mt->aux_buf = intel_alloc_aux_buffer(brw, "hiz-miptree",
> > > > > > &temp_hiz_surf,
> > > > > > alloc_flags, false, 0, 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
