On Tue, May 8, 2018 at 3:41 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Thu, May 3, 2018 at 12:03 PM, Nanley Chery <nanleych...@gmail.com>
> 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) {
>>
>
> I don't think the temporary bool is doing you any good. just doing
>
> if (wants_memset || has_indirect_clear) {
> /* map */
> if (wants_memset) ...
> if (has_indirect_clear) ...
> }
>
> is simpler. This needs_memset thing makes it look like we're going "Ha!
> You have an indirect clear so you're getting a memset even though you
> didn't ask for one!"
>
Never mind me. The next patch makes this make sense.
--Jason
> 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;
>> 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
