Re: [PATCH i-g-t 05/23] lib/rendercopy: Always setup clear color for TGL

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 2.9.2024 17.37, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> TGL apparently automagically converts regular output to fast
> clears when the output matches the configured clear color.
> And if we don't enable the clear color packet at all then we
> just get some rainbow gibberish on all black parts of the
> output.
> 
> To avoid always set up the clear color packet when using
> using a non-clear color modifier. We'll just stick a bunch
> of NaNs into the clear value so it'll never match any
> legitimate output, and thus automagic fast clear should not
> happen.
> 
> TODO: Hide this better inside rendercopy_gen9.c without
>        requring extra allocation in the FB BO
> TODO: Figure out if other platforms need this sort stuff
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   lib/igt_fb.c          | 24 ++++++++++++++++++++++++
>   lib/intel_bufops.h    |  1 +
>   lib/rendercopy_gen9.c | 14 +++++++++++---
>   3 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 21c56a454c5a..0eb3897d0f8b 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -965,6 +965,16 @@ void igt_calc_fb_size(struct igt_fb *fb)
>   		size += calc_plane_size(fb, plane);
>   	}
>   
> +	/*
> +	 * We always need a clear color on TGL, make some extra
> +	 * room for one it if it's not explicit in the modifier.
> +	 *
> +	 * TODO: probably better to allocate this as part of the
> +	 * batch instead so the fb size doesn't need to change...
> +	 */
> +	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS)
> +		size = ALIGN(size + 64, 64);
> +
>   	if (is_xe_device(fb->fd)) {
>   		size = ALIGN(size, xe_get_default_alignment(fb->fd));
>   		if (fb->modifier == I915_FORMAT_MOD_4_TILED_BMG_CCS)
> @@ -2670,6 +2680,20 @@ igt_fb_create_intel_buf(int fd, struct buf_ops *bops,
>   	if (fb->modifier == I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC)
>   		buf->cc.offset = fb->offsets[1];
>   
> +	/*
> +	 * TGL appears to do automagic fast clear when rendering
> +	 * black and the clear color isn't specified, or when the
> +	 * output matches the specified clear color. Force a
> +	 * non-sensical clear color to prevent it from doing this
> +	 * when using a non-clear color modifier.
> +	 *
> +	 * TODO: figure out if other platforms are affected...
> +	 */
> +	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS) {

What about adlp on this? I believe it will now get same treatment as tgl 
and ci doesn't seem to have adlp on shards so there's no good results(?)

/Juha-Pekka

> +		buf->cc.disable = true;
> +		buf->cc.offset = fb->size - 64;
> +	}
> +
>   	return buf;
>   }
>   
> diff --git a/lib/intel_bufops.h b/lib/intel_bufops.h
> index 84e71d41a2c2..06e72ba4ba93 100644
> --- a/lib/intel_bufops.h
> +++ b/lib/intel_bufops.h
> @@ -38,6 +38,7 @@ struct intel_buf {
>   	} ccs[2];
>   	struct {
>   		uint32_t offset;
> +		bool disable;
>   	} cc;
>   	struct {
>   		uint64_t offset;
> diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
> index 5ee4c89f5cdb..f0757a8e6b78 100644
> --- a/lib/rendercopy_gen9.c
> +++ b/lib/rendercopy_gen9.c
> @@ -1133,15 +1133,23 @@ void _gen9_render_op(struct intel_bb *ibb,
>   
>   	gen12_emit_aux_pgtable_state(ibb, aux_pgtable_state, true);
>   
> -	if (fast_clear) {
> +	if (fast_clear || dst->cc.disable) {
>   		for (int i = 0; i < 4; i++) {
>   			intel_bb_out(ibb, MI_STORE_DWORD_IMM_GEN4);
>   			intel_bb_emit_reloc(ibb, dst->handle,
>   					    I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
>                                               dst->cc.offset + i*sizeof(float),
>   					    dst->addr.offset);
> -			intel_bb_out(ibb, *(uint32_t*)&clear_color[i]);
> -               }
> +			if (fast_clear) {
> +				intel_bb_out(ibb, *(uint32_t*)&clear_color[i]);
> +			} else {
> +				/*
> +				 * Emit NaN so it'll match nothing and thus prevent
> +				 * TGL from doing its automagic fast clear tricks.
> +				 */
> +				intel_bb_out(ibb, 0xffffffff);
> +			}
> +		}
>          }
>   
>