On 01/02/2022 10:41, Ramalingam C wrote:

From: Matthew Auld matthew.auld@intel.com

This is all kinds of awkward since we now have to contend with using 64K GTT pages when mapping anything in LMEM(including the page-tables themselves).

v2: Rebased [Ram]

Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Ramalingam C ramalingam.c@intel.com

This version seems to be missing your review feedback, which I incorporated here[1].

[1] https://patchwork.freedesktop.org/series/97544/

---

drivers/gpu/drm/i915/gt/intel_migrate.c | 179 +++++++++++++++++++----- 1 file changed, 147 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c b/drivers/gpu/drm/i915/gt/intel_migrate.c index 18b44af56969..cac791155244 100644 --- a/drivers/gpu/drm/i915/gt/intel_migrate.c +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c @@ -32,6 +32,38 @@ static bool engine_supports_migration(struct intel_engine_cs *engine) return true; }

+static void xehpsdv_toggle_pdes(struct i915_address_space *vm,

• 		struct i915_page_table *pt,
  

• 		void *data)
  

+{

• struct insert_pte_data *d = data;
• /*

• * Insert a dummy PTE into every PT that will map to LMEM to ensure
  

• * we have a correctly setup PDE structure for later use.
  

• */
  

• vm->insert_page(vm, 0, d->offset, I915_CACHE_NONE, PTE_LM);
• GEM_BUG_ON(!pt->is_compact);
• d->offset += SZ_2M;

+}

+static void xehpsdv_insert_pte(struct i915_address_space *vm,

• 	       struct i915_page_table *pt,
  

• 	       void *data)
  

+{

• struct insert_pte_data *d = data;
• /*

• * We are playing tricks here, since the actual pt, from the hw
  

• * pov, is only 256bytes with 32 entries, or 4096bytes with 512
  

• * entries, but we are still guaranteed that the physical
  

• * alignment is 64K underneath for the pt, and we are careful
  

• * not to access the space in the void.
  

• */
  

• vm->insert_page(vm, px_dma(pt), d->offset, I915_CACHE_NONE, PTE_LM);
• d->offset += SZ_64K;

+}

• static void insert_pte(struct i915_address_space *vm, struct i915_page_table *pt, void *data)

@@ -74,7 +106,12 @@ static struct i915_address_space *migrate_vm(struct intel_gt *gt) * i.e. within the same non-preemptible window so that we do not switch * to another migration context that overwrites the PTE. *

• * TODO: Add support for huge LMEM PTEs
  

• * On platforms with HAS_64K_PAGES support we have three windows, and
  

• * dedicate two windows just for mapping lmem pages(smem <-> smem is not
  

• * a thing), since we are forced to use 64K GTT pages underneath which
  

• * requires also modifying the PDE. An alternative might be to instead
  

• * map the PD into the GTT, and then on the fly toggle the 4K/64K mode
  

• * in the PDE from the same batch that also modifies the PTEs.
  

  */

  vm = i915_ppgtt_create(gt, I915_BO_ALLOC_PM_EARLY);

@@ -86,6 +123,9 @@ static struct i915_address_space *migrate_vm(struct intel_gt *gt) goto err_vm; }

• if (HAS_64K_PAGES(gt->i915))

• stash.pt_sz = I915_GTT_PAGE_SIZE_64K;
  

• /*
  • Each engine instance is assigned its own chunk in the VM, so
  • that we can run multiple instances concurrently

@@ -105,14 +145,20 @@ static struct i915_address_space *migrate_vm(struct intel_gt *gt) * We copy in 8MiB chunks. Each PDE covers 2MiB, so we need * 4x2 page directories for source/destination. */

• sz = 2 * CHUNK_SZ;
  

• if (HAS_64K_PAGES(gt->i915))
  

• 	sz = 3 * CHUNK_SZ;
  

• else
  

• 	sz = 2 * CHUNK_SZ;
  

  d.offset = base + sz;

  /*

  • We need another page directory setup so that we can write
  • the 8x512 PTE in each chunk.

  */

• sz += (sz >> 12) * sizeof(u64);
  

• if (HAS_64K_PAGES(gt->i915))
  

• 	sz += (sz / SZ_2M) * SZ_64K;
  

• else
  

• 	sz += (sz >> 12) * sizeof(u64);
  
  

  err = i915_vm_alloc_pt_stash(&vm->vm, &stash, sz); if (err)

@@ -133,7 +179,18 @@ static struct i915_address_space *migrate_vm(struct intel_gt *gt) goto err_vm;

/* Now allow the GPU to rewrite the PTE via its own ppGTT */

• vm->vm.foreach(&vm->vm, base, d.offset - base, insert_pte, &d);
  

• if (HAS_64K_PAGES(gt->i915)) {
  

• 	vm->vm.foreach(&vm->vm, base, d.offset - base,
  

• 		       xehpsdv_insert_pte, &d);
  

• 	d.offset = base + CHUNK_SZ;
  

• 	vm->vm.foreach(&vm->vm,
  

• 		       d.offset,
  

• 		       2 * CHUNK_SZ,
  

• 		       xehpsdv_toggle_pdes, &d);
  

• } else {
  

• 	vm->vm.foreach(&vm->vm, base, d.offset - base,
  

• 		       insert_pte, &d);
  

• }
  

  }

  return &vm->vm;

@@ -269,19 +326,38 @@ static int emit_pte(struct i915_request *rq, u64 offset, int length) {

• bool has_64K_pages = HAS_64K_PAGES(rq->engine->i915); const u64 encode = rq->context->vm->pte_encode(0, cache_level, is_lmem ? PTE_LM : 0); struct intel_ring *ring = rq->ring;

• int total = 0;

• int pkt, dword_length;
• u32 total = 0;
• u32 page_size; u32 *hdr, *cs;

• int pkt;

  GEM_BUG_ON(GRAPHICS_VER(rq->engine->i915) < 8);

• page_size = I915_GTT_PAGE_SIZE;
• dword_length = 0x400;
• /* Compute the page directory offset for the target address range */

• offset >>= 12;
• offset *= sizeof(u64);
• offset += 2 * CHUNK_SZ;

• if (has_64K_pages) {

• GEM_BUG_ON(!IS_ALIGNED(offset, SZ_2M));
  

• offset /= SZ_2M;
  

• offset *= SZ_64K;
  

• offset += 3 * CHUNK_SZ;
  

• if (is_lmem) {
  

• 	page_size = I915_GTT_PAGE_SIZE_64K;
  

• 	dword_length = 0x40;
  

• }
  

• } else {

• offset >>= 12;
  

• offset *= sizeof(u64);
  

• offset += 2 * CHUNK_SZ;
  

• }

• offset += (u64)rq->engine->instance << 32;

  cs = intel_ring_begin(rq, 6);

@@ -289,7 +365,7 @@ static int emit_pte(struct i915_request *rq, return PTR_ERR(cs);

/* Pack as many PTE updates as possible into a single MI command */

• pkt = min_t(int, 0x400, ring->space / sizeof(u32) + 5);

• pkt = min_t(int, dword_length, ring->space / sizeof(u32) + 5); pkt = min_t(int, pkt, (ring->size - ring->emit) / sizeof(u32) + 5);

  hdr = cs;

@@ -299,6 +375,8 @@ static int emit_pte(struct i915_request *rq,

do { if (cs - hdr >= pkt) {

• 	int dword_rem;
  

• *hdr += cs - hdr - 2;
  *cs++ = MI_NOOP;
  
  

@@ -310,7 +388,18 @@ static int emit_pte(struct i915_request *rq, if (IS_ERR(cs)) return PTR_ERR(cs);

• 	pkt = min_t(int, 0x400, ring->space / sizeof(u32) + 5);
  

• 	dword_rem = dword_length;
  

• 	if (has_64K_pages) {
  

• 		if (IS_ALIGNED(total, SZ_2M)) {
  

• 			offset = round_up(offset, SZ_64K);
  

• 		} else {
  

• 			dword_rem = SZ_2M - (total & (SZ_2M - 1));
  

• 			dword_rem /= page_size;
  

• 			dword_rem *= 2;
  

• 		}
  

• 	}
  

• 	pkt = min_t(int, dword_rem, ring->space / sizeof(u32) + 5);
  pkt = min_t(int, pkt, (ring->size - ring->emit) / sizeof(u32) + 5);
  
  hdr = cs;
  

@@ -319,13 +408,15 @@ static int emit_pte(struct i915_request *rq, *cs++ = upper_32_bits(offset); }

• GEM_BUG_ON(!IS_ALIGNED(it->dma, page_size));
  

• *cs++ = lower_32_bits(encode | it->dma); *cs++ = upper_32_bits(encode | it->dma);

  offset += 8;

• total += I915_GTT_PAGE_SIZE;
  

• total += page_size;
  
  

• it->dma += I915_GTT_PAGE_SIZE;
  

• it->dma += page_size;
  

  if (it->dma >= it->max) { it->sg = __sg_next(it->sg); if (!it->sg || sg_dma_len(it->sg) == 0)

@@ -356,7 +447,8 @@ static bool wa_1209644611_applies(int ver, u32 size) return height % 4 == 3 && height <= 8; }

-static int emit_copy(struct i915_request *rq, int size) +static int emit_copy(struct i915_request *rq,

•      u32 dst_offset, u32 src_offset, int size)
  

  { const int ver = GRAPHICS_VER(rq->engine->i915); u32 instance = rq->engine->instance;

@@ -371,31 +463,31 @@ static int emit_copy(struct i915_request *rq, int size) *cs++ = BLT_DEPTH_32 | PAGE_SIZE; *cs++ = 0; *cs++ = size >> PAGE_SHIFT << 16 | PAGE_SIZE / 4;

• *cs++ = CHUNK_SZ; /* dst offset */
  

• *cs++ = dst_offset;
  

  *cs++ = instance; *cs++ = 0; *cs++ = PAGE_SIZE;

• *cs++ = 0; /* src offset */
  

• *cs++ = src_offset;
  

  *cs++ = instance; } else if (ver >= 8) { *cs++ = XY_SRC_COPY_BLT_CMD | BLT_WRITE_RGBA | (10 - 2); *cs++ = BLT_DEPTH_32 | BLT_ROP_SRC_COPY | PAGE_SIZE; *cs++ = 0; *cs++ = size >> PAGE_SHIFT << 16 | PAGE_SIZE / 4;

• *cs++ = CHUNK_SZ; /* dst offset */
  

• *cs++ = dst_offset;
  

  *cs++ = instance; *cs++ = 0; *cs++ = PAGE_SIZE;

• *cs++ = 0; /* src offset */
  

• *cs++ = src_offset;
  

  *cs++ = instance; } else { GEM_BUG_ON(instance); *cs++ = SRC_COPY_BLT_CMD | BLT_WRITE_RGBA | (6 - 2); *cs++ = BLT_DEPTH_32 | BLT_ROP_SRC_COPY | PAGE_SIZE; *cs++ = size >> PAGE_SHIFT << 16 | PAGE_SIZE;

• *cs++ = CHUNK_SZ; /* dst offset */
  

• *cs++ = dst_offset;
  

  *cs++ = PAGE_SIZE;

• *cs++ = 0; /* src offset */
  

• *cs++ = src_offset;
  

  }

  intel_ring_advance(rq, cs);

@@ -423,6 +515,7 @@ intel_context_migrate_copy(struct intel_context *ce, GEM_BUG_ON(ce->ring->size < SZ_64K);

do {

• u32 src_offset, dst_offset;
  

  int len;

  rq = i915_request_create(ce);

@@ -450,15 +543,28 @@ intel_context_migrate_copy(struct intel_context *ce, if (err) goto out_rq;

• len = emit_pte(rq, &it_src, src_cache_level, src_is_lmem, 0,
  

• 	       CHUNK_SZ);
  

• src_offset = 0;
  

• dst_offset = CHUNK_SZ;
  

• if (HAS_64K_PAGES(ce->engine->i915)) {
  

• 	GEM_BUG_ON(!src_is_lmem && !dst_is_lmem);
  

• 	src_offset = 0;
  

• 	dst_offset = 0;
  

• 	if (src_is_lmem)
  

• 		src_offset = CHUNK_SZ;
  

• 	if (dst_is_lmem)
  

• 		dst_offset = 2 * CHUNK_SZ;
  

• }
  

• len = emit_pte(rq, &it_src, src_cache_level, src_is_lmem,
  

• 	       src_offset, CHUNK_SZ);
  

  if (len <= 0) { err = len; goto out_rq; }

  err = emit_pte(rq, &it_dst, dst_cache_level, dst_is_lmem,

• 	       CHUNK_SZ, len);
  

• 	       dst_offset, len);
  

  if (err < 0) goto out_rq; if (err < len) {

@@ -470,7 +576,7 @@ intel_context_migrate_copy(struct intel_context *ce, if (err) goto out_rq;

• err = emit_copy(rq, len);
  

• err = emit_copy(rq, dst_offset, src_offset, len);
  
  

  /* Arbitration is re-enabled between requests. */ out_rq:

@@ -488,14 +594,18 @@ intel_context_migrate_copy(struct intel_context *ce, return err; }

-static int emit_clear(struct i915_request *rq, int size, u32 value) +static int emit_clear(struct i915_request *rq,

•       u64 offset,
  

•       int size,
  

•       u32 value)
  

  { const int ver = GRAPHICS_VER(rq->engine->i915);

• u32 instance = rq->engine->instance; u32 *cs;

  GEM_BUG_ON(size >> PAGE_SHIFT > S16_MAX);

• offset += (u64)rq->engine->instance << 32;
• cs = intel_ring_begin(rq, ver >= 8 ? 8 : 6); if (IS_ERR(cs)) return PTR_ERR(cs);

@@ -505,17 +615,17 @@ static int emit_clear(struct i915_request *rq, int size, u32 value) *cs++ = BLT_DEPTH_32 | BLT_ROP_COLOR_COPY | PAGE_SIZE; *cs++ = 0; *cs++ = size >> PAGE_SHIFT << 16 | PAGE_SIZE / 4;

• *cs++ = 0; /* offset */
  

• *cs++ = instance;
  

• *cs++ = lower_32_bits(offset);
  

• *cs++ = upper_32_bits(offset);
  

  *cs++ = value; *cs++ = MI_NOOP; } else {

• GEM_BUG_ON(instance);
  

• GEM_BUG_ON(upper_32_bits(offset));
  

  *cs++ = XY_COLOR_BLT_CMD | BLT_WRITE_RGBA | (6 - 2); *cs++ = BLT_DEPTH_32 | BLT_ROP_COLOR_COPY | PAGE_SIZE; *cs++ = 0; *cs++ = size >> PAGE_SHIFT << 16 | PAGE_SIZE / 4;

• *cs++ = 0;
  

• *cs++ = lower_32_bits(offset);
  

  *cs++ = value; }

@@ -542,6 +652,7 @@ intel_context_migrate_clear(struct intel_context *ce, GEM_BUG_ON(ce->ring->size < SZ_64K);

do {

• u32 offset;
  

  int len;

  rq = i915_request_create(ce);

@@ -569,7 +680,11 @@ intel_context_migrate_clear(struct intel_context *ce, if (err) goto out_rq;

• len = emit_pte(rq, &it, cache_level, is_lmem, 0, CHUNK_SZ);
  

• offset = 0;
  

• if (HAS_64K_PAGES(ce->engine->i915) && is_lmem)
  

• 	offset = CHUNK_SZ;
  

• len = emit_pte(rq, &it, cache_level, is_lmem, offset, CHUNK_SZ);
  

  if (len <= 0) { err = len; goto out_rq;

@@ -579,7 +694,7 @@ intel_context_migrate_clear(struct intel_context *ce, if (err) goto out_rq;

• err = emit_clear(rq, len, value);
  

• err = emit_clear(rq, offset, len, value);
  
  

  /* Arbitration is re-enabled between requests. */ out_rq:

On 2022-02-17 at 21:39:16 -0800, Lucas De Marchi wrote:

On Tue, Feb 01, 2022 at 04:11:22PM +0530, Ramalingam C wrote:

...

Details of the 64k pagesize support added as part of DG2 enabling and its implicit impact on the uAPI.

v2: improvised the Flat-CCS documentation [Danvet & CQ] v3: made only for 64k pagesize support

Signed-off-by: Ramalingam C ramalingam.c@intel.com cc: Daniel Vetter daniel.vetter@ffwll.ch cc: Matthew Auld matthew.auld@intel.com cc: Simon Ser contact@emersion.fr cc: Pekka Paalanen ppaalanen@gmail.com Cc: Jordan Justen jordan.l.justen@intel.com Cc: Kenneth Graunke kenneth@whitecape.org Cc: mesa-dev@lists.freedesktop.org Cc: Tony Ye tony.ye@intel.com Cc: Slawomir Milczarek slawomir.milczarek@intel.com

---

Documentation/gpu/rfc/i915_dg2.rst | 25 +++++++++++++++++++++++++ Documentation/gpu/rfc/index.rst | 3 +++ 2 files changed, 28 insertions(+) create mode 100644 Documentation/gpu/rfc/i915_dg2.rst

diff --git a/Documentation/gpu/rfc/i915_dg2.rst b/Documentation/gpu/rfc/i915_dg2.rst new file mode 100644 index 000000000000..f4eb5a219897 --- /dev/null +++ b/Documentation/gpu/rfc/i915_dg2.rst @@ -0,0 +1,25 @@ +==================== +I915 DG2 RFC Section +====================

+Upstream plan +============= +Plan to upstream the DG2 enabling is:

+* Merge basic HW enabling for DG2 (Still without pciid) +* Merge the 64k support for lmem +* Merge the flat CCS enabling patches +* Add the pciid for DG2 and enable the DG2 in CI

does this make sense after the fact? Earlier version of this patch Daniel Vetter asked this to be moved to the be the first patch. I see you added it in the cover letter, but keeping this in gpu/rfc/i915_dg2.rst doesn't make much sense IMO. Maybe just drop this patch?

Yes. I couldn't move this to the start of the series as the kdoc referenced here are from later patches of the series.

But now considering we have the Kdoc for uapi at the respective patches itself we could drop this patch.

Daniel, Hope you agree on that?

Ram.

Lucas De Marchi

...

+64K page support for lmem +========================= +On DG2 hw, local-memory supports minimum GTT page size of 64k only. 4k is not +supported anymore.

+DG2 hw doesn't support the 64k (lmem) and 4k (smem) pages in the same ppgtt +Page table. Refer the struct drm_i915_gem_create_ext for the implication of +handling the 64k page size.

+.. kernel-doc:: include/uapi/drm/i915_drm.h

•    :functions: drm_i915_gem_create_ext
  

diff --git a/Documentation/gpu/rfc/index.rst b/Documentation/gpu/rfc/index.rst index 91e93a705230..afb320ed4028 100644 --- a/Documentation/gpu/rfc/index.rst +++ b/Documentation/gpu/rfc/index.rst @@ -20,6 +20,9 @@ host such documentation:

i915_gem_lmem.rst

+.. toctree::

• i915_dg2.rst

.. toctree::

i915_scheduler.rst

-- 2.20.1

On Fri, Feb 18, 2022 at 02:08:18AM -0800, Lucas De Marchi wrote:

On Tue, Feb 01, 2022 at 04:11:24PM +0530, Ramalingam C wrote:

...

From: Abdiel Janulgue abdiel.janulgue@linux.intel.com

A portion of device memory is reserved for Flat CCS so usable device memory will be reduced by size of Flat CCS. Size of Flat CCS is specified in “XEHPSDV_FLAT_CCS_BASE_ADDR”. So to get effective device memory we need to subtract total device memory by Flat CCS memory size.

v2: Addressed the small bar related issue [Matt] Removed a reduntant check [Matt]

Cc: Matthew Auld matthew.auld@intel.com Signed-off-by: Abdiel Janulgue abdiel.janulgue@linux.intel.com Signed-off-by: Ramalingam C ramalingam.c@intel.com

---

drivers/gpu/drm/i915/gt/intel_gt.c | 19 ++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt.h | 1 + drivers/gpu/drm/i915/gt/intel_region_lmem.c | 24 +++++++++++++++++++-- drivers/gpu/drm/i915/i915_reg.h | 3 +++ 4 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index f59933abbb3a..e40d98cb3a2d 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -911,6 +911,25 @@ u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg) return intel_uncore_read_fw(gt->uncore, reg); }

+u32 intel_gt_read_register(struct intel_gt *gt, i915_reg_t reg) +{

• int type;
• u8 sliceid, subsliceid;
• for (type = 0; type < NUM_STEERING_TYPES; type++) {

• if (intel_gt_reg_needs_read_steering(gt, reg, type)) {
  

• 	intel_gt_get_valid_steering(gt, type, &sliceid,
  

• 				    &subsliceid);
  

• 	return intel_uncore_read_with_mcr_steering(gt->uncore,
  

• 						   reg,
  

• 						   sliceid,
  

• 						   subsliceid);
  

• }
  

• }
• return intel_uncore_read(gt->uncore, reg);

+}

void intel_gt_info_print(const struct intel_gt_info *info, struct drm_printer *p) { diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h index 2dad46c3eff2..0f571c8ee22b 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.h +++ b/drivers/gpu/drm/i915/gt/intel_gt.h @@ -85,6 +85,7 @@ static inline bool intel_gt_needs_read_steering(struct intel_gt *gt, }

u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg); +u32 intel_gt_read_register(struct intel_gt *gt, i915_reg_t reg);

void intel_gt_info_print(const struct intel_gt_info *info, struct drm_printer *p); diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c index 21215a080088..f1d37b46b505 100644 --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c @@ -205,8 +205,28 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) if (!IS_DGFX(i915)) return ERR_PTR(-ENODEV);

• /* Stolen starts from GSMBASE on DG1 */
• lmem_size = intel_uncore_read64(uncore, GEN12_GSMBASE);

• if (HAS_FLAT_CCS(i915)) {

• u64 tile_stolen, flat_ccs_base_addr_reg, flat_ccs_base;
  

• lmem_size = pci_resource_len(pdev, 2);
  

• flat_ccs_base_addr_reg = intel_gt_read_register(gt, XEHPSDV_FLAT_CCS_BASE_ADDR);
  

nit since this will need a respin due to conflicts: we usually call _reg an i915_reg_t variable. But here you have the value, not the register. Maybe "flat_ccs_base_addr"?

...

• flat_ccs_base = (flat_ccs_base_addr_reg >> XEHPSDV_CCS_BASE_SHIFT) * SZ_64K;
  

• if (GEM_WARN_ON(lmem_size < flat_ccs_base))
  

• 	return ERR_PTR(-ENODEV);
  

• tile_stolen = lmem_size - flat_ccs_base;
  

• /* If the FLAT_CCS_BASE_ADDR register is not populated, flag an error */
  

• if (tile_stolen == lmem_size)
  

• 	DRM_ERROR("CCS_BASE_ADDR register did not have expected value\n");
  

drm_err()

...

• lmem_size -= tile_stolen;
  

• } else {

• /* Stolen starts from GSMBASE without CCS */
  

• lmem_size = intel_uncore_read64(&i915->uncore, GEN12_GSMBASE);
  

• }

• io_start = pci_resource_start(pdev, 2); if (GEM_WARN_ON(lmem_size > pci_resource_len(pdev, 2)))

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 0f36af8dc3a1..9b5423572fe9 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -11651,6 +11651,9 @@ enum skl_power_gate { #define SGGI_DIS REG_BIT(15) #define SGR_DIS REG_BIT(13)

+#define XEHPSDV_FLAT_CCS_BASE_ADDR _MMIO(0x4910) +#define XEHPSDV_CCS_BASE_SHIFT 8

you will have a conflict here... I fixed it locally by moving to gt/intel_gt_regs.h

With the above,

Reviewed-by: Lucas De Marchi lucas.demarchi@intel.com

Also, I may be wrong and couldn't test it today. But... to finally have BAT on CI working, this and "drm/i915/xehpsdv: Add has_flat_ccs to device info" are the only changes from the FlatCCS part that are strictly needed, isn't it? "drm/i915/migrate: add acceleration support for DG2", all the plane format, support for clearing ccs etc are things that could be on top.

Lucas De Marchi

Lucas De Marchi

...

/* gamt regs */ #define GEN8_L3_LRA_1_GPGPU _MMIO(0x4dd4)

#define GEN8_L3_LRA_1_GPGPU_DEFAULT_VALUE_BDW 0x67F1427F /* max/min for LRA1/2 */

2.20.1

On 12.2.2022 3.17, Nanley Chery wrote:

On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C ramalingam.c@intel.com wrote:

...

From: Matt Roper matthew.d.roper@intel.com

DG2 unifies render compression and media compression into a single format for the first time. The programming and buffer layout is supposed to match compression on older gen12 platforms, but the actual compression algorithm is different from any previous platform; as such, we need a new framebuffer modifier to represent buffers in this format, but otherwise we can re-use the existing gen12 compression driver logic.

v2: Display version fix [Imre]

Signed-off-by: Matt Roper matthew.d.roper@intel.com cc: Radhakrishna Sripada radhakrishna.sripada@intel.com Signed-off-by: Mika Kahola mika.kahola@intel.com (v2) cc: Anshuman Gupta anshuman.gupta@intel.com Signed-off-by: Juha-Pekka Heikkilä juha-pekka.heikkila@intel.com Signed-off-by: Ramalingam C ramalingam.c@intel.com

---

drivers/gpu/drm/i915/display/intel_fb.c | 13 ++++++++++ .../drm/i915/display/skl_universal_plane.c | 26 ++++++++++++++++--- include/uapi/drm/drm_fourcc.h | 22 ++++++++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c index 94c57facbb46..4d4d01963f15 100644 --- a/drivers/gpu/drm/i915/display/intel_fb.c +++ b/drivers/gpu/drm/i915/display/intel_fb.c @@ -141,6 +141,14 @@ struct intel_modifier_desc {

static const struct intel_modifier_desc intel_modifiers[] = { {

•           .modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS,
  

•           .display_ver = { 13, 13 },
  

•           .plane_caps = INTEL_PLANE_CAP_TILING_4 | INTEL_PLANE_CAP_CCS_MC,
  

•   }, {
  

•           .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
  

•           .display_ver = { 13, 13 },
  

•           .plane_caps = INTEL_PLANE_CAP_TILING_4 | INTEL_PLANE_CAP_CCS_RC,
  

•   }, {
             .modifier = I915_FORMAT_MOD_4_TILED,
             .display_ver = { 13, 13 },
             .plane_caps = INTEL_PLANE_CAP_TILING_4,
  

@@ -550,6 +558,8 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) return 128; else return 512;

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
  

•   case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
     case I915_FORMAT_MOD_4_TILED:
             /*
              * Each 4K tile consists of 64B(8*8) subtiles, with
  

@@ -752,6 +762,9 @@ unsigned int intel_surf_alignment(const struct drm_framebuffer *fb, case I915_FORMAT_MOD_4_TILED: case I915_FORMAT_MOD_Yf_TILED: return 1 * 1024 * 1024;

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
  

•   case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
  

•           return 16 * 1024;
     default:
             MISSING_CASE(fb->modifier);
             return 0;
  

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c index 5299dfe68802..c38ae0876c15 100644 --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c @@ -764,6 +764,14 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier) return PLANE_CTL_TILED_Y; case I915_FORMAT_MOD_4_TILED: return PLANE_CTL_TILED_4;

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
  

•           return PLANE_CTL_TILED_4 |
  

•                   PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |
  

•                   PLANE_CTL_CLEAR_COLOR_DISABLE;
  

•   case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
  

•           return PLANE_CTL_TILED_4 |
  

•                   PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE |
  

•                   PLANE_CTL_CLEAR_COLOR_DISABLE;
     case I915_FORMAT_MOD_Y_TILED_CCS:
     case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
             return PLANE_CTL_TILED_Y | PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
  

@@ -2094,6 +2102,10 @@ static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915, if (IS_ADLP_DISPLAY_STEP(i915, STEP_A0, STEP_B0)) return false;

•   /* Wa_14013215631 */
  

•   if (IS_DG2_DISPLAY_STEP(i915, STEP_A0, STEP_C0))
  

•           return false;
  

•    return plane_id < PLANE_SPRITE4;
  

  }

@@ -2335,9 +2347,10 @@ skl_get_initial_plane_config(struct intel_crtc *crtc, case PLANE_CTL_TILED_Y: plane_config->tiling = I915_TILING_Y; if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)

•                   fb->modifier = DISPLAY_VER(dev_priv) >= 12 ?
  

•                           I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS :
  

•                           I915_FORMAT_MOD_Y_TILED_CCS;
  

•                   if (DISPLAY_VER(dev_priv) >= 12)
  

•                           fb->modifier = I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS;
  

•                   else
  

•                           fb->modifier = I915_FORMAT_MOD_Y_TILED_CCS;
             else if (val & PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE)
                     fb->modifier = I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS;
             else
  

@@ -2345,7 +2358,12 @@ skl_get_initial_plane_config(struct intel_crtc *crtc, break; case PLANE_CTL_TILED_YF: /* aka PLANE_CTL_TILED_4 on XE_LPD+ */ if (HAS_4TILE(dev_priv)) {

•                   fb->modifier = I915_FORMAT_MOD_4_TILED;
  

•                   if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
  

•                           fb->modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS;
  

•                   else if (val & PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE)
  

•                           fb->modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS;
  

•                   else
  

•                           fb->modifier = I915_FORMAT_MOD_4_TILED;
             } else {
                     if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
                             fb->modifier = I915_FORMAT_MOD_Yf_TILED_CCS;
  

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index b73fe6797fc3..b8fb7b44c03c 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -583,6 +583,28 @@ extern "C" { */ #define I915_FORMAT_MOD_4_TILED fourcc_mod_code(INTEL, 9)

+/*

• • Intel color control surfaces (CCS) for DG2 render compression.
• • DG2 uses a new compression format for render compression. The general
• • layout is the same as I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
• • but a new hashing/compression algorithm is used, so a fresh modifier must
• • be associated with buffers of this type. Render compression uses 128 byte
• • compression blocks.

I think I've seen a way to configure the compression block size on TGL at least. I can't find the spec text for that at the moment though... Could we omit these mentions?

Not sure why general possibility of changing compression block size is relevant? All hw features can be changed but this defines how this modifier is being implemented.

Say you take I915_FORMAT_MOD_4_TILED_DG2_RC_CCS framebuffer including control surface and copy it out, then come back and restore framebuffer with same information. It is expected to be valid?

/Juha-Pekka

...

• */

+#define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS fourcc_mod_code(INTEL, 10)

How about something like:

The main surface is Tile 4 and at plane index 0. The CCS plane is hidden from userspace. The main surface pitch is required to be a multiple of four Tile 4 widths. The CCS is configured with the render compression format associated with the main surface format.

....I think the CCS is technically accessible via the blitter engine, so the part about the plane being "hidden" may need some tweaking.

-Nanley

...

+/*

• • Intel color control surfaces (CCS) for DG2 media compression.
• • DG2 uses a new compression format for media compression. The general
• • layout is the same as I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
• • but a new hashing/compression algorithm is used, so a fresh modifier must
• • be associated with buffers of this type. Media compression uses 256 byte
• • compression blocks.
• */

+#define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS fourcc_mod_code(INTEL, 11)

• /*
  • Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks

-- 2.20.1

On Thu, Feb 17, 2022 at 05:15:15PM +0000, Chery, Nanley G wrote:

...

...

...

[...] --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -583,6 +583,28 @@ extern "C" { */ #define I915_FORMAT_MOD_4_TILED fourcc_mod_code(INTEL, 9)

+/*

• • Intel color control surfaces (CCS) for DG2 render compression.
• • DG2 uses a new compression format for render compression. The general
• • layout is the same as I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
• • but a new hashing/compression algorithm is used, so a fresh modifier must
• • be associated with buffers of this type. Render compression uses 128 byte
• • compression blocks.

I think I've seen a way to configure the compression block size on TGL at least. I can't find the spec text for that at the moment though... Could we omit these mentions?

Not sure why general possibility of changing compression block size is relevant? All hw features can be changed but this defines how this modifier is being implemented.

I was concerned about compatibility between the different modes, but I've looked into the restrictions here and don't see any problems with this.

...

Say you take I915_FORMAT_MOD_4_TILED_DG2_RC_CCS framebuffer including control surface and copy it out, then come back and restore framebuffer with same information. It is expected to be valid?

...

/Juha-Pekka

...

...

• */

+#define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS fourcc_mod_code(INTEL, 10)

How about something like:

The main surface is Tile 4 and at plane index 0. The CCS plane is hidden from userspace. The main surface pitch is required to be a multiple of four Tile 4 widths. The CCS is configured with the render compression format associated with the main surface format.

Actually, let's omit the last sentence. CCS has always been affected by the main surface format, so I don't think there's a need to mention it specifically for the DG2 modifier.

We do need to mention the 4-tile-wide pitch requirement though.

Agreed, the DG2 layout of planes and the tile format used - both different wrt. the GEN12_RC_CCS format - should be described here.

-Nanley

...

...

....I think the CCS is technically accessible via the blitter engine, so the part about the plane being "hidden" may need some tweaking.

Maybe outside of the GEM object? Capturing all the above would you be ok with the following?:

Intel color control surfaces (CCS) for DG2 render compression.

The main surface is Tile 4 and at plane index 0. The CCS data is stored outside of the GEM object in a reserved memory area dedicated for the storage of the CCS data from all GEM objects. The main surface pitch is required to be a multiple of four Tile 4 widths.

Intel color control surfaces (CCS) for DG2 media compression.

The main surface is Tile 4 and at plane index 0. For semi-planar formats like NV12, the UV plane is Tile 4 at plane index 1. The CCS data both for the main and semi-planar UV planes are stored outside of the GEM object in a reserved memory area dedicated for the storage of the CCS data from all GEM objects. The main surface pitch is required to be a multiple of four Tile 4 widths.

...

...

-Nanley

...

+/*

• • Intel color control surfaces (CCS) for DG2 media compression.
• • DG2 uses a new compression format for media compression. The

+general

• • layout is the same as I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
• • but a new hashing/compression algorithm is used, so a fresh

+modifier must

• • be associated with buffers of this type. Media compression uses

+256 byte

• • compression blocks.
• */

+#define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS

fourcc_mod_code(INTEL,

...

...

+11)

• /*
  • Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks

-- 2.20.1

On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C ramalingam.c@intel.com wrote:

From: Mika Kahola mika.kahola@intel.com

DG2 clear color render compression uses Tile4 layout. Therefore, we need to define a new format modifier for uAPI to support clear color rendering.

v2: Display version is fixed. [Imre] KDoc is enhanced for cc modifier. [Nanley & Lionel]

Signed-off-by: Mika Kahola mika.kahola@intel.com cc: Anshuman Gupta anshuman.gupta@intel.com Signed-off-by: Juha-Pekka Heikkilä juha-pekka.heikkila@intel.com Signed-off-by: Ramalingam C ramalingam.c@intel.com

---

drivers/gpu/drm/i915/display/intel_fb.c | 8 ++++++++ drivers/gpu/drm/i915/display/skl_universal_plane.c | 9 ++++++++- include/uapi/drm/drm_fourcc.h | 10 ++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c index 4d4d01963f15..3df6ef5ffec5 100644 --- a/drivers/gpu/drm/i915/display/intel_fb.c +++ b/drivers/gpu/drm/i915/display/intel_fb.c @@ -144,6 +144,12 @@ static const struct intel_modifier_desc intel_modifiers[] = { .modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS, .display_ver = { 13, 13 }, .plane_caps = INTEL_PLANE_CAP_TILING_4 | INTEL_PLANE_CAP_CCS_MC,

•   }, {
  

•           .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC,
  

•           .display_ver = { 13, 13 },
  

•           .plane_caps = INTEL_PLANE_CAP_TILING_4 | INTEL_PLANE_CAP_CCS_RC_CC,
  

•           .ccs.cc_planes = BIT(1),
    }, {
            .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
            .display_ver = { 13, 13 },
  

@@ -559,6 +565,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) else return 512; case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
    case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
    case I915_FORMAT_MOD_4_TILED:
            /*
  

@@ -763,6 +770,7 @@ unsigned int intel_surf_alignment(const struct drm_framebuffer *fb, case I915_FORMAT_MOD_Yf_TILED: return 1 * 1024 * 1024; case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
    case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
            return 16 * 1024;
    default:
  

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c index c38ae0876c15..b4dced1907c5 100644 --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c @@ -772,6 +772,8 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier) return PLANE_CTL_TILED_4 | PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE | PLANE_CTL_CLEAR_COLOR_DISABLE;

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
  

•           return PLANE_CTL_TILED_4 | PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
    case I915_FORMAT_MOD_Y_TILED_CCS:
    case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
            return PLANE_CTL_TILED_Y | PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
  

@@ -2358,10 +2360,15 @@ skl_get_initial_plane_config(struct intel_crtc *crtc, break; case PLANE_CTL_TILED_YF: /* aka PLANE_CTL_TILED_4 on XE_LPD+ */ if (HAS_4TILE(dev_priv)) {

•                   if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
  

•                   u32 rc_mask = PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |
  

•                                 PLANE_CTL_CLEAR_COLOR_DISABLE;
  

•                   if ((val & rc_mask) == rc_mask)
                            fb->modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS;
                    else if (val & PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE)
                            fb->modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS;
  

•                   else if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
  

•                           fb->modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC;
                    else
                            fb->modifier = I915_FORMAT_MOD_4_TILED;
            } else {
  

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index b8fb7b44c03c..697614ea4b84 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -605,6 +605,16 @@ extern "C" { */ #define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS fourcc_mod_code(INTEL, 11)

+/*

• • Intel color control surfaces (CCS) for DG2 clear color render compression.
• • DG2 uses a unified compression format for clear color render compression.

What's unified about DG2's compression format? If this doesn't affect the layout, maybe we should drop this sentence.

• • The general layout is a tiled layout using 4Kb tiles i.e. Tile4 layout.

This also needs a pitch aligned to four tiles, right? I think we can save some effort by referencing the DG2_RC_CCS modifier here.

• • Fast clear color value expected by HW is located in fb at offset 0 of plane#1

Why is the expected offset hardcoded to 0 instead of relying on the offset provided by the modifier API? This looks like a bug.

We should probably give some info about the relevant fields in the fast clear plane (like what's done in the GEN12_RC_CCS_CC modifier).

-Nanley

• */

+#define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC fourcc_mod_code(INTEL, 12)

/*

• Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks

-- 2.20.1

-----Original Message----- From: Juha-Pekka Heikkila juhapekka.heikkila@gmail.com Sent: Tuesday, February 15, 2022 6:56 AM To: Nanley Chery nanleychery@gmail.com; C, Ramalingam ramalingam.c@intel.com Cc: intel-gfx intel-gfx@lists.freedesktop.org; Chery, Nanley G nanley.g.chery@intel.com; Auld, Matthew matthew.auld@intel.com; dri- devel dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v5 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color

On 12.2.2022 3.19, Nanley Chery wrote:

...

On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C ramalingam.c@intel.com

wrote:

...

...

From: Mika Kahola mika.kahola@intel.com

DG2 clear color render compression uses Tile4 layout. Therefore, we need to define a new format modifier for uAPI to support clear color

rendering.

...

...

v2: Display version is fixed. [Imre] KDoc is enhanced for cc modifier. [Nanley & Lionel]

Signed-off-by: Mika Kahola mika.kahola@intel.com cc: Anshuman Gupta anshuman.gupta@intel.com Signed-off-by: Juha-Pekka Heikkilä juha-pekka.heikkila@intel.com Signed-off-by: Ramalingam C ramalingam.c@intel.com

---

drivers/gpu/drm/i915/display/intel_fb.c | 8 ++++++++ drivers/gpu/drm/i915/display/skl_universal_plane.c | 9 ++++++++- include/uapi/drm/drm_fourcc.h | 10 ++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c index 4d4d01963f15..3df6ef5ffec5 100644 --- a/drivers/gpu/drm/i915/display/intel_fb.c +++ b/drivers/gpu/drm/i915/display/intel_fb.c @@ -144,6 +144,12 @@ static const struct intel_modifier_desc

intel_modifiers[] = {

...

...

             .modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS,
             .display_ver = { 13, 13 },
             .plane_caps = INTEL_PLANE_CAP_TILING_4 |

INTEL_PLANE_CAP_CCS_MC,

•   }, {
  

•           .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC,
  

•           .display_ver = { 13, 13 },
  

•           .plane_caps = INTEL_PLANE_CAP_TILING_4 |
  

• INTEL_PLANE_CAP_CCS_RC_CC,

•           .ccs.cc_planes = BIT(1),
     }, {
             .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
             .display_ver = { 13, 13 }, @@ -559,6 +565,7 @@
  

intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) else return 512; case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
     case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
     case I915_FORMAT_MOD_4_TILED:
             /*
  

@@ -763,6 +770,7 @@ unsigned int intel_surf_alignment(const struct

drm_framebuffer *fb,

...

...

     case I915_FORMAT_MOD_Yf_TILED:
             return 1 * 1024 * 1024;
     case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
     case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
             return 16 * 1024;
     default:
  

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c index c38ae0876c15..b4dced1907c5 100644 --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c @@ -772,6 +772,8 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier) return PLANE_CTL_TILED_4 | PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE | PLANE_CTL_CLEAR_COLOR_DISABLE;

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
  

•           return PLANE_CTL_TILED_4 |
  

• PLANE_CTL_RENDER_DECOMPRESSION_ENABLE; case I915_FORMAT_MOD_Y_TILED_CCS: case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC: return PLANE_CTL_TILED_Y |

PLANE_CTL_RENDER_DECOMPRESSION_ENABLE; @@ -2358,10 +2360,15 @@ skl_get_initial_plane_config(struct intel_crtc

*crtc,

...

...

             break;
     case PLANE_CTL_TILED_YF: /* aka PLANE_CTL_TILED_4 on XE_LPD+ */
             if (HAS_4TILE(dev_priv)) {

•                   if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
  

•                   u32 rc_mask = PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |
  

•                                 PLANE_CTL_CLEAR_COLOR_DISABLE;
  

•                   if ((val & rc_mask) == rc_mask)
                             fb->modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS;
                     else if (val & PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE)
                             fb->modifier =
  

I915_FORMAT_MOD_4_TILED_DG2_MC_CCS;

•                   else if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
  

•                           fb->modifier =
  

• I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC; else fb->modifier = I915_FORMAT_MOD_4_TILED; } else {

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index b8fb7b44c03c..697614ea4b84 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -605,6 +605,16 @@ extern "C" { */ #define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS

fourcc_mod_code(INTEL,

...

...

+/*

• • Intel color control surfaces (CCS) for DG2 clear color render compression.
• • DG2 uses a unified compression format for clear color render

compression.

...

What's unified about DG2's compression format? If this doesn't affect the layout, maybe we should drop this sentence.

...

• • The general layout is a tiled layout using 4Kb tiles i.e. Tile4 layout.

This also needs a pitch aligned to four tiles, right? I think we can save some effort by referencing the DG2_RC_CCS modifier here.

...

• • Fast clear color value expected by HW is located in fb at offset
• 0 of plane#1

Why is the expected offset hardcoded to 0 instead of relying on the offset provided by the modifier API? This looks like a bug.

Hi Nanley,

can you elaborate a bit, which offset from modifier API that applies to cc surface?

Hi Juha-Pekka,

On the kernel-side of things, I'm thinking of drm_mode_fb_cmd2::offsets[1].

-Nanley

...

We should probably give some info about the relevant fields in the fast clear plane (like what's done in the GEN12_RC_CCS_CC modifier).

agree, that's totally missing here.

/Juha-Pekka

...

...

• */

+#define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC

fourcc_mod_code(INTEL,

...

...

+12)

• /*
  • Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks

-- 2.20.1

-----Original Message----- From: Juha-Pekka Heikkila juhapekka.heikkila@gmail.com Sent: Tuesday, February 15, 2022 8:15 AM To: Chery, Nanley G nanley.g.chery@intel.com; Nanley Chery nanleychery@gmail.com; C, Ramalingam ramalingam.c@intel.com Cc: intel-gfx intel-gfx@lists.freedesktop.org; Auld, Matthew matthew.auld@intel.com; dri-devel dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v5 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color

On 15.2.2022 17.02, Chery, Nanley G wrote:

...

...

-----Original Message----- From: Juha-Pekka Heikkila juhapekka.heikkila@gmail.com Sent: Tuesday, February 15, 2022 6:56 AM To: Nanley Chery nanleychery@gmail.com; C, Ramalingam ramalingam.c@intel.com Cc: intel-gfx intel-gfx@lists.freedesktop.org; Chery, Nanley G nanley.g.chery@intel.com; Auld, Matthew matthew.auld@intel.com; dri- devel dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v5 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color

On 12.2.2022 3.19, Nanley Chery wrote:

...

On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C ramalingam.c@intel.com

wrote:

...

...

From: Mika Kahola mika.kahola@intel.com

DG2 clear color render compression uses Tile4 layout. Therefore, we need to define a new format modifier for uAPI to support clear color

rendering.

...

...

v2: Display version is fixed. [Imre] KDoc is enhanced for cc modifier. [Nanley & Lionel]

Signed-off-by: Mika Kahola mika.kahola@intel.com cc: Anshuman Gupta anshuman.gupta@intel.com Signed-off-by: Juha-Pekka Heikkilä juha-pekka.heikkila@intel.com Signed-off-by: Ramalingam C ramalingam.c@intel.com

---

drivers/gpu/drm/i915/display/intel_fb.c | 8 ++++++++ drivers/gpu/drm/i915/display/skl_universal_plane.c | 9 ++++++++- include/uapi/drm/drm_fourcc.h | 10 ++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c index 4d4d01963f15..3df6ef5ffec5 100644 --- a/drivers/gpu/drm/i915/display/intel_fb.c +++ b/drivers/gpu/drm/i915/display/intel_fb.c @@ -144,6 +144,12 @@ static const struct intel_modifier_desc

intel_modifiers[] = {

...

...

              .modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS,
              .display_ver = { 13, 13 },
              .plane_caps = INTEL_PLANE_CAP_TILING_4 |

INTEL_PLANE_CAP_CCS_MC,

•   }, {
  

•           .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC,
  

•           .display_ver = { 13, 13 },
  

•           .plane_caps = INTEL_PLANE_CAP_TILING_4 |
  

• INTEL_PLANE_CAP_CCS_RC_CC,

•           .ccs.cc_planes = BIT(1),
      }, {
              .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
              .display_ver = { 13, 13 }, @@ -559,6 +565,7 @@
  

intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) else return 512; case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
      case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
      case I915_FORMAT_MOD_4_TILED:
              /*
  

@@ -763,6 +770,7 @@ unsigned int intel_surf_alignment(const struct

drm_framebuffer *fb,

...

...

      case I915_FORMAT_MOD_Yf_TILED:
              return 1 * 1024 * 1024;
      case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
      case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
              return 16 * 1024;
      default:
  

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c index c38ae0876c15..b4dced1907c5 100644 --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c @@ -772,6 +772,8 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier) return PLANE_CTL_TILED_4 | PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE | PLANE_CTL_CLEAR_COLOR_DISABLE;

•   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
  

•           return PLANE_CTL_TILED_4 |
  

• PLANE_CTL_RENDER_DECOMPRESSION_ENABLE; case I915_FORMAT_MOD_Y_TILED_CCS: case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC: return PLANE_CTL_TILED_Y |

PLANE_CTL_RENDER_DECOMPRESSION_ENABLE; @@ -2358,10 +2360,15 @@ skl_get_initial_plane_config(struct intel_crtc

*crtc,

...

...

              break;
      case PLANE_CTL_TILED_YF: /* aka PLANE_CTL_TILED_4 on XE_LPD+ */
              if (HAS_4TILE(dev_priv)) {

•                   if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
  

•                   u32 rc_mask =
  

PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |

...

...

...

...

• PLANE_CTL_CLEAR_COLOR_DISABLE;

•                   if ((val & rc_mask) == rc_mask)
                              fb->modifier =
  

I915_FORMAT_MOD_4_TILED_DG2_RC_CCS;

...

...

...

...

                      else if (val & PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE)
                              fb->modifier =

I915_FORMAT_MOD_4_TILED_DG2_MC_CCS;

•                   else if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
  

•                           fb->modifier =
  

• I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC; else fb->modifier = I915_FORMAT_MOD_4_TILED; } else {

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index b8fb7b44c03c..697614ea4b84 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -605,6 +605,16 @@ extern "C" { */ #define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS

fourcc_mod_code(INTEL,

...

...

+/*

• • Intel color control surfaces (CCS) for DG2 clear color render

compression.

...

...

...

...

• • DG2 uses a unified compression format for clear color render

compression.

...

What's unified about DG2's compression format? If this doesn't affect the layout, maybe we should drop this sentence.

...

• • The general layout is a tiled layout using 4Kb tiles i.e. Tile4 layout.

This also needs a pitch aligned to four tiles, right? I think we can save some effort by referencing the DG2_RC_CCS modifier here.

...

• • Fast clear color value expected by HW is located in fb at
• offset
• 0 of plane#1

Why is the expected offset hardcoded to 0 instead of relying on the offset provided by the modifier API? This looks like a bug.

Hi Nanley,

can you elaborate a bit, which offset from modifier API that applies to cc

surface?

...

...

Hi Juha-Pekka,

On the kernel-side of things, I'm thinking of drm_mode_fb_cmd2::offsets[1].

Hi Nanley,

this offset is coming from userspace on creation of framebuffer, at that moment from userspace caller can point to offset of desire. Normally offset[0] is set at 0 and then offset[n] at plane n start which is not stated to have to be exactly after plane n-1 end. Or did I misunderstand what you meant?

Perhaps, at least, I'm not sure what you're meaning to say. This modifier description seems to say that the drm_mode_fb_cmd2::offsets value for the clear color plane must be zero. Are you saying that it's correct? This doesn't match the GEN12_RC_CCS_CC behavior and doesn't match mesa's expectations.

-Nanley

For cc plane this offset likely will not be zero anyway and caller can move it as see fit to have cc plane (plane[1]) location[0] at place where wanted to have it.

/Juha-Pekka

...

...

...

We should probably give some info about the relevant fields in the fast clear plane (like what's done in the GEN12_RC_CCS_CC modifier).

agree, that's totally missing here.

/Juha-Pekka

...

...

• */

+#define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC

fourcc_mod_code(INTEL,

...

...

+12)

• /*
  • Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks

-- 2.20.1

-----Original Message----- From: Juha-Pekka Heikkila juhapekka.heikkila@gmail.com Sent: Tuesday, February 15, 2022 9:32 AM To: Chery, Nanley G nanley.g.chery@intel.com; Nanley Chery nanleychery@gmail.com; C, Ramalingam ramalingam.c@intel.com Cc: intel-gfx intel-gfx@lists.freedesktop.org; Auld, Matthew matthew.auld@intel.com; dri-devel dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v5 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color

On 15.2.2022 18.44, Chery, Nanley G wrote:

...

...

-----Original Message----- From: Juha-Pekka Heikkila juhapekka.heikkila@gmail.com Sent: Tuesday, February 15, 2022 8:15 AM To: Chery, Nanley G nanley.g.chery@intel.com; Nanley Chery nanleychery@gmail.com; C, Ramalingam ramalingam.c@intel.com Cc: intel-gfx intel-gfx@lists.freedesktop.org; Auld, Matthew matthew.auld@intel.com; dri-devel dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v5 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color

On 15.2.2022 17.02, Chery, Nanley G wrote:

...

...

-----Original Message----- From: Juha-Pekka Heikkila juhapekka.heikkila@gmail.com Sent: Tuesday, February 15, 2022 6:56 AM To: Nanley Chery nanleychery@gmail.com; C, Ramalingam ramalingam.c@intel.com Cc: intel-gfx intel-gfx@lists.freedesktop.org; Chery, Nanley G nanley.g.chery@intel.com; Auld, Matthew matthew.auld@intel.com; dri- devel dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v5 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color

On 12.2.2022 3.19, Nanley Chery wrote:

...

On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C ramalingam.c@intel.com

wrote:

...

> > From: Mika Kahola mika.kahola@intel.com > > DG2 clear color render compression uses Tile4 layout. Therefore, > we need to define a new format modifier for uAPI to support clear > color

rendering.

...

> > v2: > Display version is fixed. [Imre] > KDoc is enhanced for cc modifier. [Nanley & Lionel] > > Signed-off-by: Mika Kahola mika.kahola@intel.com > cc: Anshuman Gupta anshuman.gupta@intel.com > Signed-off-by: Juha-Pekka Heikkilä > juha-pekka.heikkila@intel.com > Signed-off-by: Ramalingam C ramalingam.c@intel.com > --- > drivers/gpu/drm/i915/display/intel_fb.c | 8 ++++++++ > drivers/gpu/drm/i915/display/skl_universal_plane.c | 9 ++++++++- > include/uapi/drm/drm_fourcc.h | 10 ++++++++++ > 3 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c > b/drivers/gpu/drm/i915/display/intel_fb.c > index 4d4d01963f15..3df6ef5ffec5 100644 > --- a/drivers/gpu/drm/i915/display/intel_fb.c > +++ b/drivers/gpu/drm/i915/display/intel_fb.c > @@ -144,6 +144,12 @@ static const struct intel_modifier_desc

intel_modifiers[] = {

...

> .modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS, > .display_ver = { 13, 13 }, > .plane_caps = INTEL_PLANE_CAP_TILING_4 | > INTEL_PLANE_CAP_CCS_MC, > + }, { > + .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC, > + .display_ver = { 13, 13 }, > + .plane_caps = INTEL_PLANE_CAP_TILING_4 | > + INTEL_PLANE_CAP_CCS_RC_CC, > + > + .ccs.cc_planes = BIT(1), > }, { > .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS, > .display_ver = { 13, 13 }, @@ -559,6 +565,7 @@ > intel_tile_width_bytes(const struct drm_framebuffer *fb, int

color_plane)

...

...

...

...

...

> else > return 512; > case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS: > + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC: > case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS: > case I915_FORMAT_MOD_4_TILED: > /* > @@ -763,6 +770,7 @@ unsigned int intel_surf_alignment(const > struct

drm_framebuffer *fb,

...

> case I915_FORMAT_MOD_Yf_TILED: > return 1 * 1024 * 1024; > case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS: > + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC: > case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS: > return 16 * 1024; > default: > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > index c38ae0876c15..b4dced1907c5 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > @@ -772,6 +772,8 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier) > return PLANE_CTL_TILED_4 | > PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE | > PLANE_CTL_CLEAR_COLOR_DISABLE; > + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC: > + return PLANE_CTL_TILED_4 | > + PLANE_CTL_RENDER_DECOMPRESSION_ENABLE; > case I915_FORMAT_MOD_Y_TILED_CCS: > case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC: > return PLANE_CTL_TILED_Y | > PLANE_CTL_RENDER_DECOMPRESSION_ENABLE; > @@ -2358,10 +2360,15 @@ skl_get_initial_plane_config(struct > intel_crtc

*crtc,

...

> break; > case PLANE_CTL_TILED_YF: /* aka PLANE_CTL_TILED_4 on XE_LPD+

*/

...

...

...

...

...

> if (HAS_4TILE(dev_priv)) { > - if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE) > + u32 rc_mask =

PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |

...

...

...

> + > + PLANE_CTL_CLEAR_COLOR_DISABLE; > + > + if ((val & rc_mask) == rc_mask) > fb->modifier =

I915_FORMAT_MOD_4_TILED_DG2_RC_CCS;

...

...

...

> else if (val &

PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE)

...

...

...

...

...

> fb->modifier = > I915_FORMAT_MOD_4_TILED_DG2_MC_CCS; > + else if (val &

PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)

...

...

...

...

...

> + fb->modifier = > + I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC; > else > fb->modifier = I915_FORMAT_MOD_4_TILED; > } else { > diff --git a/include/uapi/drm/drm_fourcc.h > b/include/uapi/drm/drm_fourcc.h index b8fb7b44c03c..697614ea4b84 > 100644 > --- a/include/uapi/drm/drm_fourcc.h > +++ b/include/uapi/drm/drm_fourcc.h > @@ -605,6 +605,16 @@ extern "C" { > */ > #define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS

fourcc_mod_code(INTEL,

...

> 11) > > +/* > + * Intel color control surfaces (CCS) for DG2 clear color render

compression.

...

...

...

> + * > + * DG2 uses a unified compression format for clear color render

compression.

...

What's unified about DG2's compression format? If this doesn't affect the layout, maybe we should drop this sentence.

> + * The general layout is a tiled layout using 4Kb tiles i.e. Tile4 layout. > + *

This also needs a pitch aligned to four tiles, right? I think we can save some effort by referencing the DG2_RC_CCS modifier here.

> + * Fast clear color value expected by HW is located in fb at > + offset > + 0 of plane#1

Why is the expected offset hardcoded to 0 instead of relying on the offset provided by the modifier API? This looks like a bug.

Hi Nanley,

can you elaborate a bit, which offset from modifier API that applies to cc

surface?

...

...

Hi Juha-Pekka,

On the kernel-side of things, I'm thinking of

drm_mode_fb_cmd2::offsets[1].

...

...

...

Hi Nanley,

this offset is coming from userspace on creation of framebuffer, at that moment from userspace caller can point to offset of desire. Normally offset[0] is set at 0 and then offset[n] at plane n start which is not stated to have to be exactly after plane n-1 end. Or did I

misunderstand what you meant?

...

...

Perhaps, at least, I'm not sure what you're meaning to say. This modifier description seems to say that the drm_mode_fb_cmd2::offsets value for the clear color plane must be zero. Are you saying that it's correct? This doesn't match the GEN12_RC_CCS_CC behavior and doesn't

match mesa's expectations.

...

It doesn't say "drm_mode_fb_cmd2::offsets value for the clear color plane must be zero", it says "Fast clear color value expected by HW is located in fb at offset 0 of plane#1".

Yes, it doesn't say that exactly, but that's what it seems to say. With every other modifier, it's implied that the data for the plane begins at the offset specified through the modifier API. So, explicitly mentioning it here (and with that wording) conveys a new requirement.

Plane#1 location is pointed by drm_mode_fb_cmd2::offsets[1] and there's nothing stated about that offset.

Technically, plane #1's location is specified to be the combination of ::handles[1] and ::offsets[1]. In practice though, I can imagine that there are areas of the stack that are implicitly requiring that all ::handles[] entries match.

These offsets are just offsets to bo which contain the framebuffer information hence drm_mode_fb_cmd2::offsets[1] can be changed as one wish and cc information is found starting at drm_mode_fb_cmd2::offsets[1][0]

If the clear color handling is the same as GEN12_RC_CCS_CC (apart for the plane index), I propose that we drop this sentence due to avoid any confusion.

This offset discussion raises another question. The description says that the value expected by HW is at offset 0. I'm assuming "HW" is referring to the render engine? The kernel is still giving the display engine the packed values at ::offsets[1] + 16B right?

-Nanley

/Juha-Pekka

...

...

For cc plane this offset likely will not be zero anyway and caller can move it as see fit to have cc plane (plane[1]) location[0] at place where

wanted to have it.

...

...

/Juha-Pekka

...

...

...

We should probably give some info about the relevant fields in the fast clear plane (like what's done in the GEN12_RC_CCS_CC modifier).

agree, that's totally missing here.

/Juha-Pekka

...

> + */ > +#define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC

fourcc_mod_code(INTEL,

...

> +12) > + > /* > * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks > * > -- > 2.20.1 >

Hi Nanley, JP,

On Tue, Feb 15, 2022 at 09:34:22PM +0200, Juha-Pekka Heikkila wrote:

[...]

...

...

...

...

...

> > > diff --git a/include/uapi/drm/drm_fourcc.h > > > b/include/uapi/drm/drm_fourcc.h index b8fb7b44c03c..697614ea4b84 100644 > > > --- a/include/uapi/drm/drm_fourcc.h > > > +++ b/include/uapi/drm/drm_fourcc.h > > > @@ -605,6 +605,16 @@ extern "C" { > > > */ > > > #define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS fourcc_mod_code(INTEL, 11) > > > > > > +/* > > > + * Intel color control surfaces (CCS) for DG2 clear color render compression. > > > + * > > > + * DG2 uses a unified compression format for clear color render compression. > > > > What's unified about DG2's compression format? If this doesn't > > affect the layout, maybe we should drop this sentence.

Unified here probably refers to the fact the DG2 render engine is capable of generating both a render and a media compressed surface as opposed to earlier platforms. The display engine still needs to know which compression format the FB uses, hence we need both an RC and MC modifier. Based on this I also think we can drop the mention of unified compression.

...

...

...

...

...

> > > + * The general layout is a tiled layout using 4Kb tiles i.e. Tile4 layout. > > > + * > > > > This also needs a pitch aligned to four tiles, right? I think we > > can save some effort by referencing the DG2_RC_CCS modifier here. > > > > > + * Fast clear color value expected by HW is located in fb at offset 0 of plane#1 > > > > Why is the expected offset hardcoded to 0 instead of relying on > > the offset provided by the modifier API? This looks like a bug. > > Hi Nanley, > > can you elaborate a bit, which offset from modifier API that > applies to cc surface?

Hi Juha-Pekka,

On the kernel-side of things, I'm thinking of drm_mode_fb_cmd2::offsets[1].

Hi Nanley,

this offset is coming from userspace on creation of framebuffer, at that moment from userspace caller can point to offset of desire. Normally offset[0] is set at 0 and then offset[n] at plane n start which is not stated to have to be exactly after plane n-1 end. Or did I misunderstand what you meant?

Perhaps, at least, I'm not sure what you're meaning to say. This modifier description seems to say that the drm_mode_fb_cmd2::offsets value for the clear color plane must be zero. Are you saying that it's correct? This doesn't match the GEN12_RC_CCS_CC behavior and doesn't match mesa's expectations.

It doesn't say "drm_mode_fb_cmd2::offsets value for the clear color plane must be zero", it says "Fast clear color value expected by HW is located in fb at offset 0 of plane#1".

Yes, it doesn't say that exactly, but that's what it seems to say. With every other modifier, it's implied that the data for the plane begins at the offset specified through the modifier API. So, explicitly mentioning it here (and with that wording) conveys a new requirement.

I don't have objections on changing this description but for reference gen12 version of the same says "The main surface is Y-tiled and is at plane index 0 whereas CCS is linear and at index 1. The clear color is stored at index 2, and the pitch should be ignored.", only plane indexes are mentioned. I anyway wrote neither of these descriptions.

...

...

Plane#1 location is pointed by drm_mode_fb_cmd2::offsets[1] and there's nothing stated about that offset.

Technically, plane #1's location is specified to be the combination of ::handles[1] and ::offsets[1]. In practice though, I can imagine that there are areas of the stack that are implicitly requiring that all ::handles[] entries match.

The FB modifier API requires all ::handles[] to match, that is all planes must be contained in one GEM object.

I didn't think we needed to go deeper as you started to just talk about how drm_mode_fb_cmd2::offsets[1] not being used. Let's not waste time.

...

...

These offsets are just offsets to bo which contain the framebuffer information hence drm_mode_fb_cmd2::offsets[1] can be changed as one wish and cc information is found starting at drm_mode_fb_cmd2::offsets[1][0]

If the clear color handling is the same as GEN12_RC_CCS_CC (apart for the plane index), I propose that we drop this sentence due to avoid any confusion.

But it need to defined as part of the modifier. It's the modifier features which are being described here.

...

This offset discussion raises another question. The description says that the value expected by HW is at offset 0. I'm assuming "HW" is referring to the render engine? The kernel is still giving the display engine the packed values at ::offsets[1] + 16B right?

Generally answer is yes but these parts you can see in patch "[PATCH v5 17/19] drm/i915/dg2: Flat CCS Support" and should be discussed there. Here "HW" should probably be changed something meaningful though.

The 256 bit clear color format starting at plane index 1 matches the one described at I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC . So yes, "HW" refers to the render engine and display consumes the 64 bit data at ::offset[1] + 16 bytes (and DE ignores the 64 bit data starting at ::offset[1] + 24 bytes.

The following captures all the above, would it be ok?:

Intel Color Control Surface with Clear Color (CCS) for DG2 render compression.

The main surface is Tile 4 and at plane index 0. The CCS data is stored outside of the GEM object in a reserved memory area dedicated for the storage of the CCS data from all GEM objects. The main surface pitch is required to be a multiple of four Tile 4 widths. The clear color is stored at plane index 1 and the pitch should be ignored. The format of the 256 bits clear color data matches the one used for the I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its description for details.

--Imre

On Thu, Mar 24, 2022 at 01:42:33AM +0200, Chery, Nanley G wrote:

...

-----Original Message----- From: Deak, Imre imre.deak@intel.com Sent: Monday, March 21, 2022 6:20 AM To: Chery, Nanley G nanley.g.chery@intel.com; Juha-Pekka Heikkila juhapekka.heikkila@gmail.com Cc: Nanley Chery nanleychery@gmail.com; C, Ramalingam ramalingam.c@intel.com; intel-gfx intel-gfx@lists.freedesktop.org; Auld, Matthew matthew.auld@intel.com; dri-devel dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v5 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color

Hi Nanley, JP,

On Tue, Feb 15, 2022 at 09:34:22PM +0200, Juha-Pekka Heikkila wrote:

...

[...]

...

...

...

> > > > > diff --git a/include/uapi/drm/drm_fourcc.h > > > > > b/include/uapi/drm/drm_fourcc.h index b8fb7b44c03c..697614ea4b84 100644 > > > > > --- a/include/uapi/drm/drm_fourcc.h > > > > > +++ b/include/uapi/drm/drm_fourcc.h > > > > > @@ -605,6 +605,16 @@ extern "C" { > > > > > */ > > > > > #define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS fourcc_mod_code(INTEL, 11) > > > > > > > > > > +/* > > > > > + * Intel color control surfaces (CCS) for DG2 clear color render compression. > > > > > + * > > > > > + * DG2 uses a unified compression format for clear color render compression. > > > > > > > > What's unified about DG2's compression format? If this doesn't > > > > affect the layout, maybe we should drop this sentence.

Unified here probably refers to the fact the DG2 render engine is capable of generating both a render and a media compressed surface as opposed to earlier platforms. The display engine still needs to know which compression format the FB uses, hence we need both an RC and MC modifier. Based on this I also think we can drop the mention of unified compression.

...

...

...

...

> > > > > + * The general layout is a tiled layout using 4Kb tiles i.e. Tile4 layout. > > > > > + * > > > > > > > > This also needs a pitch aligned to four tiles, right? I think we > > > > can save some effort by referencing the DG2_RC_CCS modifier here. > > > > > > > > > + * Fast clear color value expected by HW is located in fb at offset 0 of plane#1 > > > > > > > > Why is the expected offset hardcoded to 0 instead of relying on > > > > the offset provided by the modifier API? This looks like a bug. > > > > > > Hi Nanley, > > > > > > can you elaborate a bit, which offset from modifier API that > > > applies to cc surface? > > > > Hi Juha-Pekka, > > > > On the kernel-side of things, I'm thinking of drm_mode_fb_cmd2::offsets[1]. > > Hi Nanley, > > this offset is coming from userspace on creation of framebuffer, at > that moment from userspace caller can point to offset of desire. > Normally offset[0] is set at 0 and then offset[n] at plane n start > which is not stated to have to be exactly after plane n-1 end. Or did I > misunderstand what you meant?

Perhaps, at least, I'm not sure what you're meaning to say. This modifier description seems to say that the drm_mode_fb_cmd2::offsets value for the clear color plane must be zero. Are you saying that it's correct? This doesn't match the GEN12_RC_CCS_CC behavior and doesn't match mesa's expectations.

It doesn't say "drm_mode_fb_cmd2::offsets value for the clear color plane must be zero", it says "Fast clear color value expected by HW is located in fb at offset 0 of plane#1".

Yes, it doesn't say that exactly, but that's what it seems to say. With every other modifier, it's implied that the data for the plane begins at the offset specified through the modifier API. So, explicitly mentioning it here (and with that wording) conveys a new requirement.

I don't have objections on changing this description but for reference gen12 version of the same says "The main surface is Y-tiled and is at plane index 0 whereas CCS is linear and at index 1. The clear color is stored at index 2, and the pitch should be ignored.", only plane indexes are mentioned. I anyway wrote neither of these descriptions.

...

...

Plane#1 location is pointed by drm_mode_fb_cmd2::offsets[1] and there's nothing stated about that offset.

Technically, plane #1's location is specified to be the combination of ::handles[1] and ::offsets[1]. In practice though, I can imagine that there are areas of the stack that are implicitly requiring that all ::handles[] entries match.

The FB modifier API requires all ::handles[] to match, that is all planes must be contained in one GEM object.

This is a requirement for i915, or for all drm drivers? I couldn't find anything in the generic DRM headers or docs requiring this. Feel free to ping me about this offline.

It's only an i915 requirement actually.

IIUC, it was added for the SKL CCS modifiers (2e2adb05736c3), where SKL has a restriction on the location of the CCS (and UV) planes. The feasible way to conform to these limits was to require all planes to reside in the same GEM object.

For DG2 there was no plan to expose the CCS plane, so wrt. that this restriction didn't make a difference.

...

...

I didn't think we needed to go deeper as you started to just talk about how drm_mode_fb_cmd2::offsets[1] not being used. Let's not waste time.

...

...

These offsets are just offsets to bo which contain the framebuffer information hence drm_mode_fb_cmd2::offsets[1] can be changed as one wish and cc information is found starting at drm_mode_fb_cmd2::offsets[1][0]

If the clear color handling is the same as GEN12_RC_CCS_CC (apart for the plane index), I propose that we drop this sentence due to avoid any confusion.

But it need to defined as part of the modifier. It's the modifier features which are being described here.

...

This offset discussion raises another question. The description says that the value expected by HW is at offset 0. I'm assuming "HW" is referring to the render engine? The kernel is still giving the display engine the packed values at ::offsets[1] + 16B right?

Generally answer is yes but these parts you can see in patch "[PATCH v5 17/19] drm/i915/dg2: Flat CCS Support" and should be discussed there. Here "HW" should probably be changed something meaningful though.

The 256 bit clear color format starting at plane index 1 matches the one described at I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC . So yes, "HW" refers to the render engine and display consumes the 64 bit data at ::offset[1] + 16 bytes (and DE ignores the 64 bit data starting at ::offset[1] + 24 bytes.

The following captures all the above, would it be ok?:

Intel Color Control Surface with Clear Color (CCS) for DG2 render compression.

The main surface is Tile 4 and at plane index 0. The CCS data is stored outside of the GEM object in a reserved memory area dedicated for the storage of the CCS data from all GEM objects. The main surface pitch is

                                                  ^
                "for all compressible" ? (since SMEM objects don't have this) 

Makes sense, optionally also mentioning that "for all RC/RC_CC/MC CCS compressible GEM objects".

...

required to be a multiple of four Tile 4 widths. The clear color is stored at plane index 1 and the pitch should be ignored. The format of the 256 bits clear color data matches the one used for the

^ "256 bits of"?

Ok.

...

I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its description for details.

Looks good to me. With the above minor changes, Acked-by: Nanley Chery nanley.g.chery@intel.com

Thanks.

--Imre

On Tue, Feb 01, 2022 at 04:11:30PM +0530, Ramalingam C wrote:

From: Anshuman Gupta anshuman.gupta@intel.com

DG2 onwards discrete gfx has support for new flat CCS mapping, which brings in display feature in to avoid Aux walk for compressed surface. This support build on top of Flat CCS support added in XEHPSDV. FLAT CCS surface base address should be 64k aligned, Compressed displayable surfaces must use tile4 format.

HAS: 1407880786 B.Spec : 7655 B.Spec : 53902

Cc: Mika Kahola mika.kahola@intel.com Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com Signed-off-by: Juha-Pekka Heikkilä juha-pekka.heikkila@intel.com Signed-off-by: Ramalingam C ramalingam.c@intel.com

Reviewed-by: Imre Deak imre.deak@intel.com

---

drivers/gpu/drm/i915/display/intel_display.c | 4 ++- drivers/gpu/drm/i915/display/intel_fb.c | 32 +++++++++++++------ .../drm/i915/display/skl_universal_plane.c | 16 ++++++---- 3 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 189767cef356..2828ae612179 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -8588,7 +8588,9 @@ static void intel_atomic_prepare_plane_clear_colors(struct intel_atomic_state *s

/*
 * The layout of the fast clear color value expected by HW

•  * (the DRM ABI requiring this value to be located in fb at offset 0 of plane#2):
  

•  * (the DRM ABI requiring this value to be located in fb at
  

•  * offset 0 of cc plane, plane #2 previous generations or
  

•  * plane #1 for flat ccs):
  

  • • 4 x 4 bytes per-channel value
  • (in surface type specific float/int format provided by the fb user)
  • • 8 bytes native color value used by the display

diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c index 3df6ef5ffec5..e94923e9dbb1 100644 --- a/drivers/gpu/drm/i915/display/intel_fb.c +++ b/drivers/gpu/drm/i915/display/intel_fb.c @@ -107,6 +107,21 @@ static const struct drm_format_info gen12_ccs_cc_formats[] = { .hsub = 1, .vsub = 1, .has_alpha = true }, };

+static const struct drm_format_info gen12_flat_ccs_cc_formats[] = {

• { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2,

•  .char_per_block = { 4, 0 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
  

•  .hsub = 1, .vsub = 1, },
  

• { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2,

•  .char_per_block = { 4, 0 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
  

•  .hsub = 1, .vsub = 1, },
  

• { .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 2,

•  .char_per_block = { 4, 0 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
  

•  .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,

•  .char_per_block = { 4, 0 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
  

•  .hsub = 1, .vsub = 1, .has_alpha = true },
  

+};

struct intel_modifier_desc { u64 modifier; struct { @@ -150,6 +165,8 @@ static const struct intel_modifier_desc intel_modifiers[] = { .plane_caps = INTEL_PLANE_CAP_TILING_4 | INTEL_PLANE_CAP_CCS_RC_CC,

.ccs.cc_planes = BIT(1),

• FORMAT_OVERRIDE(gen12_flat_ccs_cc_formats),
  

  }, { .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS, .display_ver = { 13, 13 },

@@ -399,17 +416,13 @@ bool intel_fb_plane_supports_modifier(struct intel_plane *plane, u64 modifier) static bool format_is_yuv_semiplanar(const struct intel_modifier_desc *md, const struct drm_format_info *info) {

• int yuv_planes;

• if (!info->is_yuv) return false;

• if (plane_caps_contain_any(md->plane_caps, INTEL_PLANE_CAP_CCS_MASK))

• yuv_planes = 4;
  

• if (hweight8(md->ccs.planar_aux_planes) == 2)

• return info->num_planes == 4;
  

  else

• yuv_planes = 2;
  

• return info->num_planes == yuv_planes;

• return info->num_planes == 2;
  

}

/** @@ -534,12 +547,13 @@ static unsigned int gen12_ccs_aux_stride(struct intel_framebuffer *fb, int ccs_p

int skl_main_to_aux_plane(const struct drm_framebuffer *fb, int main_plane) {

• const struct intel_modifier_desc *md = lookup_modifier(fb->modifier); struct drm_i915_private *i915 = to_i915(fb->dev);

• if (intel_fb_is_ccs_modifier(fb->modifier))

• if (md->ccs.packed_aux_planes | md->ccs.planar_aux_planes) return main_to_ccs_plane(fb, main_plane); else if (DISPLAY_VER(i915) < 11 &&

•  intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier))
  

•  format_is_yuv_semiplanar(md, fb->format))
  

  return 1; else return 0;

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c index b4dced1907c5..18e50583abaa 100644 --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c @@ -1176,8 +1176,10 @@ skl_plane_update_arm(struct intel_plane *plane, intel_de_write_fw(dev_priv, PLANE_OFFSET(pipe, plane_id), PLANE_OFFSET_Y(y) | PLANE_OFFSET_X(x));

• intel_de_write_fw(dev_priv, PLANE_AUX_DIST(pipe, plane_id),

• 	  skl_plane_aux_dist(plane_state, color_plane));
  

• /* FLAT CCS doesn't need to program AUX_DIST */

• if (!HAS_FLAT_CCS(dev_priv))

• intel_de_write_fw(dev_priv, PLANE_AUX_DIST(pipe, plane_id),
  

• 		  skl_plane_aux_dist(plane_state, color_plane));
  
  

  if (DISPLAY_VER(dev_priv) < 11) intel_de_write_fw(dev_priv, PLANE_AUX_OFFSET(pipe, plane_id),

@@ -1557,9 +1559,10 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)

/* * CCS AUX surface doesn't have its own x/y offsets, we must make sure

• * they match with the main surface x/y offsets.
  

• * they match with the main surface x/y offsets. On DG2
  

• * there's no aux plane on fb so skip this checking.
  

  */

• if (intel_fb_is_ccs_modifier(fb->modifier)) {

• if (intel_fb_is_ccs_modifier(fb->modifier) && aux_plane) { while (!skl_check_main_ccs_coordinates(plane_state, x, y, offset, aux_plane)) { if (offset == 0)

@@ -1603,6 +1606,8 @@ static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state) const struct drm_framebuffer *fb = plane_state->hw.fb; unsigned int rotation = plane_state->hw.rotation; int uv_plane = 1;

• int ccs_plane = intel_fb_is_ccs_modifier(fb->modifier) ?

• 	skl_main_to_aux_plane(fb, uv_plane) : 0;
  

  int max_width = intel_plane_max_width(plane, fb, uv_plane, rotation); int max_height = intel_plane_max_height(plane, fb, uv_plane, rotation); int x = plane_state->uapi.src.x1 >> 17;

@@ -1623,8 +1628,7 @@ static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state) offset = intel_plane_compute_aligned_offset(&x, &y, plane_state, uv_plane);

• if (intel_fb_is_ccs_modifier(fb->modifier)) {

• int ccs_plane = main_to_ccs_plane(fb, uv_plane);
  

• if (ccs_plane) { u32 aux_offset = plane_state->view.color_plane[ccs_plane].offset; u32 alignment = intel_surf_alignment(fb, uv_plane);

-- 2.20.1