Re: [Intel-gfx] [PATCH 2/2] drm/i915/tgl: Add Clear Color support for TGL Render Decompression

From: Imre Deak <imre.deak@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjala <ville.syrjala@intel.com>,
	Rafael Antognolli <rafael.antognolli@intel.com>,
	intel-gfx@lists.freedesktop.org,
	Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>,
	Nanley G Chery <nanley.g.chery@intel.com>,
	Shashank Sharma <shashank.sharma@intel.com>
Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915/tgl: Add Clear Color support for TGL Render Decompression
Date: Tue, 1 Dec 2020 22:50:21 +0200	[thread overview]
Message-ID: <20201201205021.GD2849269@ideak-desk.fi.intel.com> (raw)
In-Reply-To: <160682607520.4024.5213539229190522123@build.alporthouse.com>

On Tue, Dec 01, 2020 at 12:34:35PM +0000, Chris Wilson wrote:
> [...]
> > @@ -16647,6 +16697,20 @@ static int intel_plane_pin_fb(struct intel_plane_state *plane_state)
> >  
> >         plane_state->vma = vma;
> >  
> > +       if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
> > +               void *map = kmap_atomic(i915_gem_object_get_page(intel_fb_obj(fb),
> > +                                                                fb->offsets[2] >> PAGE_SHIFT));
> 
> So at this point in time, we have only queued the wait for render
> completion (asynchronous waits) and not actually waited on either the
> explicit or implicit fences.
> 
> Only at intel_atomic_commit_tail do we know that the GPU [+ccs]
> writes will have been flushed.

Ok, so after intel_atomic_commit_fence_wait(). One problem is that
atomic state should not really get modified any more in commit_tail().
But I introduced that already earlier with the TC/TBT PLL selection, so
now I'd add one more exception.

> There's also the matter of coherency. Is the object coherent for reads
> from the CPU? -- in most cases it will not be, but you should check
> obj->cache_coherency to see if the read requires a preceding
> cache_clflush_range() / drm_clflush_virt_range().

Ok, at this point for the TGL-only modifier, we could then just
warn_on(!bo_cache_coherent_for_read) due to HAS_LLC.

> Also the page may not exist, not all scanout objects are backed by struct
> page. In which case, pulling it from a vmap (i915_gem_object_pin_map, or
> iomap) may be required. (A i915_gem_object_read may be very useful for
> such small accesses.)

Ok. Afaiu on TGL this would need the io/vmap special casing for stolen
memory only. That's only used for BIOS FBs, which is unlikely to be
fast-cleared and we haven't even added support to initial_fb for that.
Could we get away with that assumption and keep using kmap_atomic at
least for now?

Thanks for the explanation!

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx