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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
