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

[Imre Deak]

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