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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Sat, Sep 07, 2024 at 11:26:52AM +0300, Juha-Pekka Heikkila wrote:
> On 6.9.2024 16.45, Ville Syrjälä wrote:
> > On Fri, Sep 06, 2024 at 04:09:19PM +0300, Juha-Pekka Heikkila wrote:
> >> 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(?)
> > 
> > I *think* adl wasn't affected, but I'll need to double check that...
> > 
> > Although apart from allocating a bit of extra memory this should
> > be completely harmeless on unaffected platforms.
> 
> ah yes, true. At the worst this will reveal something that was not 
> handled for adlp.

Mesa has a wa 1607794140 for this stuff which they limit to
TGL:A0-C0 and DG1.

Digging a bit more based on that there is apparently a new
bit in 3DSTATE_3DMODE ("Fast clear optimization (FCV)" (what the
"V" stands for I have no idea). However that new bit only seems to
exist on DG2+ only, but not for TGL/DG1/ADL.

I re-ran the tests:
- My TGL is definitely affected (can't remember what stepping it is).
  This fake clear color thing seems to cure it 100%.
- Didn't test DG1 since I don't have a suitable machine to plug it into
- ADL never seems to do this optimization, so the fake clear color
  would be redundant.
- for DG2 I can enable this via 3DSTATE_3DMODE, after which it somewhat
  matches the TGL behaviour. Though using the fake clear color trick
  doesn't 100% cure it on DG2 for some unknown reason. Looks mostly
  good but there are a few blocks of incorrect pixels sprinkled here
  and there.

I'll update the comment a bit when pushing this, but based on that
I think we should be OK to do this unconditonally for
I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS.

> 
> Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> 
> > 
> >>
> >> /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);
> >>> +			}
> >>> +		}
> >>>           }
> >>>    
> >>>    
> > 

-- 
Ville Syrjälä
Intel