* [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-22 5:03 ` [PATCH i-g-t v2 " Ville Syrjala
2023-12-22 5:37 ` [PATCH i-g-t " Zbigniew Kempczyński
2023-12-20 17:59 ` [PATCH i-g-t 02/12] tests/kms_big_fb: Use igt_fb_create_intel_buf() Ville Syrjala
` (10 subsequent siblings)
11 siblings, 2 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
In order to copy stuff not at offset 0 in the BO we need
to include the delta in the relocs/etc.
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
lib/rendercopy_gen4.c | 9 +++++----
lib/rendercopy_gen6.c | 9 +++++----
lib/rendercopy_gen7.c | 9 +++++----
lib/rendercopy_gen8.c | 9 +++++----
lib/rendercopy_gen9.c | 13 +++++++------
lib/rendercopy_i830.c | 10 +++++++---
lib/rendercopy_i915.c | 6 ++++--
7 files changed, 38 insertions(+), 27 deletions(-)
diff --git a/lib/rendercopy_gen4.c b/lib/rendercopy_gen4.c
index 8536d6b632c5..d10e5b7780c0 100644
--- a/lib/rendercopy_gen4.c
+++ b/lib/rendercopy_gen4.c
@@ -148,10 +148,11 @@ gen4_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
ss->ss0.color_blend = 1;
- address = intel_bb_offset_reloc(ibb, buf->handle,
- read_domain, write_domain,
- intel_bb_offset(ibb) + 4,
- buf->addr.offset);
+ address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
+ read_domain, write_domain,
+ buf->surface[0].offset,
+ intel_bb_offset(ibb) + 4,
+ buf->addr.offset);
ss->ss1.base_addr = (uint32_t) address;
ss->ss2.height = intel_buf_height(buf) - 1;
diff --git a/lib/rendercopy_gen6.c b/lib/rendercopy_gen6.c
index e941257eb606..ebd4c5cf241c 100644
--- a/lib/rendercopy_gen6.c
+++ b/lib/rendercopy_gen6.c
@@ -91,10 +91,11 @@ gen6_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
ss->ss0.color_blend = 1;
- address = intel_bb_offset_reloc(ibb, buf->handle,
- read_domain, write_domain,
- intel_bb_offset(ibb) + 4,
- buf->addr.offset);
+ address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
+ read_domain, write_domain,
+ buf->surface[0].offset,
+ intel_bb_offset(ibb) + 4,
+ buf->addr.offset);
ss->ss1.base_addr = (uint32_t) address;
ss->ss2.height = intel_buf_height(buf) - 1;
diff --git a/lib/rendercopy_gen7.c b/lib/rendercopy_gen7.c
index 267f6f8038ba..279eb45bdc36 100644
--- a/lib/rendercopy_gen7.c
+++ b/lib/rendercopy_gen7.c
@@ -86,10 +86,11 @@ gen7_bind_buf(struct intel_bb *ibb,
gen7_tiling_bits(buf->tiling) |
format << GEN7_SURFACE_FORMAT_SHIFT);
- address = intel_bb_offset_reloc(ibb, buf->handle,
- read_domain, write_domain,
- intel_bb_offset(ibb) + 4,
- buf->addr.offset);
+ address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
+ read_domain, write_domain,
+ intel_bb_offset(ibb) + 4,
+ buf->surface[0].offset,
+ buf->addr.offset);
ss[1] = address;
ss[2] = ((intel_buf_width(buf) - 1) << GEN7_SURFACE_WIDTH_SHIFT |
(intel_buf_height(buf) - 1) << GEN7_SURFACE_HEIGHT_SHIFT);
diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c
index ba7897fb475f..2039e5fc6b8e 100644
--- a/lib/rendercopy_gen8.c
+++ b/lib/rendercopy_gen8.c
@@ -109,10 +109,11 @@ gen8_bind_buf(struct intel_bb *ibb,
ss->ss1.memory_object_control = BDW_MOCS_PTE |
BDW_MOCS_TC_L3_PTE | BDW_MOCS_AGE(0);
- address = intel_bb_offset_reloc(ibb, buf->handle,
- read_domain, write_domain,
- intel_bb_offset(ibb) + 4 * 8,
- buf->addr.offset);
+ address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
+ read_domain, write_domain,
+ buf->surface[0].offset,
+ intel_bb_offset(ibb) + 4 * 8,
+ buf->addr.offset);
ss->ss8.base_addr = address;
ss->ss9.base_addr_hi = address >> 32;
diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
index 363bc6c1b203..bfac62b0fc23 100644
--- a/lib/rendercopy_gen9.c
+++ b/lib/rendercopy_gen9.c
@@ -194,12 +194,13 @@ gen9_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst,
else if (buf->tiling == I915_TILING_Ys)
ss->ss5.trmode = 2;
- address = intel_bb_offset_reloc(ibb, buf->handle,
- read_domain, write_domain,
- intel_bb_offset(ibb) + 4 * 8,
- buf->addr.offset);
- ss->ss8.base_addr = address;
- ss->ss9.base_addr_hi = address >> 32;
+ address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
+ read_domain, write_domain,
+ buf->surface[0].offset,
+ intel_bb_offset(ibb) + 4 * 8,
+ buf->addr.offset);
+ ss->ss8.base_addr = (address + buf->surface[0].offset);
+ ss->ss9.base_addr_hi = (address + buf->surface[0].offset) >> 32;
ss->ss2.height = intel_buf_height(buf) - 1;
ss->ss2.width = intel_buf_width(buf) - 1;
diff --git a/lib/rendercopy_i830.c b/lib/rendercopy_i830.c
index 4c4271493b4b..4b0ea3b859e2 100644
--- a/lib/rendercopy_i830.c
+++ b/lib/rendercopy_i830.c
@@ -158,8 +158,10 @@ static void gen2_emit_target(struct intel_bb *ibb,
intel_bb_out(ibb, _3DSTATE_BUF_INFO_CMD);
intel_bb_out(ibb, BUF_3D_ID_COLOR_BACK | tiling |
BUF_3D_PITCH(dst->surface[0].stride));
- intel_bb_emit_reloc(ibb, dst->handle, I915_GEM_DOMAIN_RENDER,
- I915_GEM_DOMAIN_RENDER, 0, dst->addr.offset);
+ intel_bb_emit_reloc(ibb, dst->handle,
+ I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
+ dst->surface[0].offset,
+ dst->addr.offset);
intel_bb_out(ibb, _3DSTATE_DST_BUF_VARS_CMD);
intel_bb_out(ibb, format |
@@ -199,7 +201,9 @@ static void gen2_emit_texture(struct intel_bb *ibb,
tiling |= TM0S1_TILE_WALK;
intel_bb_out(ibb, _3DSTATE_LOAD_STATE_IMMEDIATE_2 | LOAD_TEXTURE_MAP(unit) | 4);
- intel_bb_emit_reloc(ibb, src->handle, I915_GEM_DOMAIN_SAMPLER, 0, 0,
+ intel_bb_emit_reloc(ibb, src->handle,
+ I915_GEM_DOMAIN_SAMPLER, 0,
+ src->surface[0].offset,
src->addr.offset);
intel_bb_out(ibb, (intel_buf_height(src) - 1) << TM0S1_HEIGHT_SHIFT |
(intel_buf_width(src) - 1) << TM0S1_WIDTH_SHIFT |
diff --git a/lib/rendercopy_i915.c b/lib/rendercopy_i915.c
index 3e421301e6a6..94cdfb99af9a 100644
--- a/lib/rendercopy_i915.c
+++ b/lib/rendercopy_i915.c
@@ -112,7 +112,8 @@ void gen3_render_copyfunc(struct intel_bb *ibb,
intel_bb_out(ibb, (1 << TEX_COUNT) - 1);
intel_bb_emit_reloc(ibb, src->handle,
I915_GEM_DOMAIN_SAMPLER, 0,
- 0, src->addr.offset);
+ src->surface[0].offset,
+ src->addr.offset);
intel_bb_out(ibb, format_bits | tiling_bits |
(intel_buf_height(src) - 1) << MS3_HEIGHT_SHIFT |
(intel_buf_width(src) - 1) << MS3_WIDTH_SHIFT);
@@ -155,7 +156,8 @@ void gen3_render_copyfunc(struct intel_bb *ibb,
BUF_3D_PITCH(dst->surface[0].stride));
intel_bb_emit_reloc(ibb, dst->handle,
I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
- 0, dst->addr.offset);
+ dst->surface[0].offset,
+ dst->addr.offset);
intel_bb_out(ibb, _3DSTATE_DST_BUF_VARS_CMD);
intel_bb_out(ibb, format_bits |
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t v2 01/12] lib/rendercopy: Add deltas to all surface relocs
2023-12-20 17:59 ` [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs Ville Syrjala
@ 2023-12-22 5:03 ` Ville Syrjala
2023-12-22 5:37 ` [PATCH i-g-t " Zbigniew Kempczyński
1 sibling, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-22 5:03 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
In order to copy stuff not at offset 0 in the BO we need
to include the delta in the relocs/etc.
v2: also add the offset to the surface state on gen4/6/7/8
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
lib/rendercopy_gen4.c | 11 ++++++-----
lib/rendercopy_gen6.c | 11 ++++++-----
lib/rendercopy_gen7.c | 11 ++++++-----
lib/rendercopy_gen8.c | 13 +++++++------
lib/rendercopy_gen9.c | 13 +++++++------
lib/rendercopy_i830.c | 10 +++++++---
lib/rendercopy_i915.c | 6 ++++--
7 files changed, 43 insertions(+), 32 deletions(-)
diff --git a/lib/rendercopy_gen4.c b/lib/rendercopy_gen4.c
index 8536d6b632c5..8582e0efb886 100644
--- a/lib/rendercopy_gen4.c
+++ b/lib/rendercopy_gen4.c
@@ -148,11 +148,12 @@ gen4_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
ss->ss0.color_blend = 1;
- address = intel_bb_offset_reloc(ibb, buf->handle,
- read_domain, write_domain,
- intel_bb_offset(ibb) + 4,
- buf->addr.offset);
- ss->ss1.base_addr = (uint32_t) address;
+ address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
+ read_domain, write_domain,
+ buf->surface[0].offset,
+ intel_bb_offset(ibb) + 4,
+ buf->addr.offset);
+ ss->ss1.base_addr = address + buf->surface[0].offset;
ss->ss2.height = intel_buf_height(buf) - 1;
ss->ss2.width = intel_buf_width(buf) - 1;
diff --git a/lib/rendercopy_gen6.c b/lib/rendercopy_gen6.c
index e941257eb606..ec197661702f 100644
--- a/lib/rendercopy_gen6.c
+++ b/lib/rendercopy_gen6.c
@@ -91,11 +91,12 @@ gen6_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
ss->ss0.color_blend = 1;
- address = intel_bb_offset_reloc(ibb, buf->handle,
- read_domain, write_domain,
- intel_bb_offset(ibb) + 4,
- buf->addr.offset);
- ss->ss1.base_addr = (uint32_t) address;
+ address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
+ read_domain, write_domain,
+ buf->surface[0].offset,
+ intel_bb_offset(ibb) + 4,
+ buf->addr.offset);
+ ss->ss1.base_addr = address + buf->surface[0].offset;
ss->ss2.height = intel_buf_height(buf) - 1;
ss->ss2.width = intel_buf_width(buf) - 1;
diff --git a/lib/rendercopy_gen7.c b/lib/rendercopy_gen7.c
index 267f6f8038ba..095ba7bde654 100644
--- a/lib/rendercopy_gen7.c
+++ b/lib/rendercopy_gen7.c
@@ -86,11 +86,12 @@ gen7_bind_buf(struct intel_bb *ibb,
gen7_tiling_bits(buf->tiling) |
format << GEN7_SURFACE_FORMAT_SHIFT);
- address = intel_bb_offset_reloc(ibb, buf->handle,
- read_domain, write_domain,
- intel_bb_offset(ibb) + 4,
- buf->addr.offset);
- ss[1] = address;
+ address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
+ read_domain, write_domain,
+ intel_bb_offset(ibb) + 4,
+ buf->surface[0].offset,
+ buf->addr.offset);
+ ss[1] = address + buf->surface[0].offset;
ss[2] = ((intel_buf_width(buf) - 1) << GEN7_SURFACE_WIDTH_SHIFT |
(intel_buf_height(buf) - 1) << GEN7_SURFACE_HEIGHT_SHIFT);
ss[3] = (buf->surface[0].stride - 1) << GEN7_SURFACE_PITCH_SHIFT;
diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c
index ba7897fb475f..8c08bab6d52e 100644
--- a/lib/rendercopy_gen8.c
+++ b/lib/rendercopy_gen8.c
@@ -109,12 +109,13 @@ gen8_bind_buf(struct intel_bb *ibb,
ss->ss1.memory_object_control = BDW_MOCS_PTE |
BDW_MOCS_TC_L3_PTE | BDW_MOCS_AGE(0);
- address = intel_bb_offset_reloc(ibb, buf->handle,
- read_domain, write_domain,
- intel_bb_offset(ibb) + 4 * 8,
- buf->addr.offset);
- ss->ss8.base_addr = address;
- ss->ss9.base_addr_hi = address >> 32;
+ address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
+ read_domain, write_domain,
+ buf->surface[0].offset,
+ intel_bb_offset(ibb) + 4 * 8,
+ buf->addr.offset);
+ ss->ss8.base_addr = (address + buf->surface[0].offset);
+ ss->ss9.base_addr_hi = (address + buf->surface[0].offset) >> 32;
ss->ss2.height = intel_buf_height(buf) - 1;
ss->ss2.width = intel_buf_width(buf) - 1;
diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
index 363bc6c1b203..bfac62b0fc23 100644
--- a/lib/rendercopy_gen9.c
+++ b/lib/rendercopy_gen9.c
@@ -194,12 +194,13 @@ gen9_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst,
else if (buf->tiling == I915_TILING_Ys)
ss->ss5.trmode = 2;
- address = intel_bb_offset_reloc(ibb, buf->handle,
- read_domain, write_domain,
- intel_bb_offset(ibb) + 4 * 8,
- buf->addr.offset);
- ss->ss8.base_addr = address;
- ss->ss9.base_addr_hi = address >> 32;
+ address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
+ read_domain, write_domain,
+ buf->surface[0].offset,
+ intel_bb_offset(ibb) + 4 * 8,
+ buf->addr.offset);
+ ss->ss8.base_addr = (address + buf->surface[0].offset);
+ ss->ss9.base_addr_hi = (address + buf->surface[0].offset) >> 32;
ss->ss2.height = intel_buf_height(buf) - 1;
ss->ss2.width = intel_buf_width(buf) - 1;
diff --git a/lib/rendercopy_i830.c b/lib/rendercopy_i830.c
index 4c4271493b4b..4b0ea3b859e2 100644
--- a/lib/rendercopy_i830.c
+++ b/lib/rendercopy_i830.c
@@ -158,8 +158,10 @@ static void gen2_emit_target(struct intel_bb *ibb,
intel_bb_out(ibb, _3DSTATE_BUF_INFO_CMD);
intel_bb_out(ibb, BUF_3D_ID_COLOR_BACK | tiling |
BUF_3D_PITCH(dst->surface[0].stride));
- intel_bb_emit_reloc(ibb, dst->handle, I915_GEM_DOMAIN_RENDER,
- I915_GEM_DOMAIN_RENDER, 0, dst->addr.offset);
+ intel_bb_emit_reloc(ibb, dst->handle,
+ I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
+ dst->surface[0].offset,
+ dst->addr.offset);
intel_bb_out(ibb, _3DSTATE_DST_BUF_VARS_CMD);
intel_bb_out(ibb, format |
@@ -199,7 +201,9 @@ static void gen2_emit_texture(struct intel_bb *ibb,
tiling |= TM0S1_TILE_WALK;
intel_bb_out(ibb, _3DSTATE_LOAD_STATE_IMMEDIATE_2 | LOAD_TEXTURE_MAP(unit) | 4);
- intel_bb_emit_reloc(ibb, src->handle, I915_GEM_DOMAIN_SAMPLER, 0, 0,
+ intel_bb_emit_reloc(ibb, src->handle,
+ I915_GEM_DOMAIN_SAMPLER, 0,
+ src->surface[0].offset,
src->addr.offset);
intel_bb_out(ibb, (intel_buf_height(src) - 1) << TM0S1_HEIGHT_SHIFT |
(intel_buf_width(src) - 1) << TM0S1_WIDTH_SHIFT |
diff --git a/lib/rendercopy_i915.c b/lib/rendercopy_i915.c
index 3e421301e6a6..94cdfb99af9a 100644
--- a/lib/rendercopy_i915.c
+++ b/lib/rendercopy_i915.c
@@ -112,7 +112,8 @@ void gen3_render_copyfunc(struct intel_bb *ibb,
intel_bb_out(ibb, (1 << TEX_COUNT) - 1);
intel_bb_emit_reloc(ibb, src->handle,
I915_GEM_DOMAIN_SAMPLER, 0,
- 0, src->addr.offset);
+ src->surface[0].offset,
+ src->addr.offset);
intel_bb_out(ibb, format_bits | tiling_bits |
(intel_buf_height(src) - 1) << MS3_HEIGHT_SHIFT |
(intel_buf_width(src) - 1) << MS3_WIDTH_SHIFT);
@@ -155,7 +156,8 @@ void gen3_render_copyfunc(struct intel_bb *ibb,
BUF_3D_PITCH(dst->surface[0].stride));
intel_bb_emit_reloc(ibb, dst->handle,
I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
- 0, dst->addr.offset);
+ dst->surface[0].offset,
+ dst->addr.offset);
intel_bb_out(ibb, _3DSTATE_DST_BUF_VARS_CMD);
intel_bb_out(ibb, format_bits |
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs
2023-12-20 17:59 ` [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs Ville Syrjala
2023-12-22 5:03 ` [PATCH i-g-t v2 " Ville Syrjala
@ 2023-12-22 5:37 ` Zbigniew Kempczyński
2023-12-22 6:01 ` Ville Syrjälä
1 sibling, 1 reply; 20+ messages in thread
From: Zbigniew Kempczyński @ 2023-12-22 5:37 UTC (permalink / raw)
To: Ville Syrjala; +Cc: igt-dev
On Wed, Dec 20, 2023 at 07:59:23PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> In order to copy stuff not at offset 0 in the BO we need
> to include the delta in the relocs/etc.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> lib/rendercopy_gen4.c | 9 +++++----
> lib/rendercopy_gen6.c | 9 +++++----
> lib/rendercopy_gen7.c | 9 +++++----
> lib/rendercopy_gen8.c | 9 +++++----
> lib/rendercopy_gen9.c | 13 +++++++------
> lib/rendercopy_i830.c | 10 +++++++---
> lib/rendercopy_i915.c | 6 ++++--
> 7 files changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/lib/rendercopy_gen4.c b/lib/rendercopy_gen4.c
> index 8536d6b632c5..d10e5b7780c0 100644
> --- a/lib/rendercopy_gen4.c
> +++ b/lib/rendercopy_gen4.c
> @@ -148,10 +148,11 @@ gen4_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
> ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
> ss->ss0.color_blend = 1;
>
> - address = intel_bb_offset_reloc(ibb, buf->handle,
> - read_domain, write_domain,
> - intel_bb_offset(ibb) + 4,
> - buf->addr.offset);
> + address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> + read_domain, write_domain,
> + buf->surface[0].offset,
> + intel_bb_offset(ibb) + 4,
> + buf->addr.offset);
> ss->ss1.base_addr = (uint32_t) address;
I've just took a look to intel_bb_add_reloc() because I didn't
remember all details and I see address returned there is offset
at which object will be binded, not offset + delta.
Until this patch only users of this function reside in
rendercopy_gen9.c so for older platforms it doesn't matter
(xe driver doesn't support them).
Anyway, delta addition sets up 10-bit if buf->cc.offset is not zero.
I mean:
address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
read_domain, write_domain,
(buf->cc.offset ? (1 << 10) : 0)
| buf->ccs[0].offset,
intel_bb_offset(ibb) + 4 * 10,
buf->addr.offset);
ss->ss10.aux_base_addr = (address + buf->ccs[0].offset) >> 12;
ss->ss11.aux_base_addr_hi = (address + buf->ccs[0].offset) >> 32;
I wonder shouldn't intel_bb_add_reloc() return offset + delta.
I also have some doubts regarding:
(buf->cc.offset ? (1 << 10) : 0) | buf->ccs[0].offset
This will set up bit-10 (if I'm not wrong clearvalue enable) only
when relocation will happen. Without relocation or on softpinning
this bit is never set.
Above also would require remove delta in __intel_bb_emit_reloc()
but if I'm not wrong this would be enough to start directly using
offset returned in intel_bb_add_reloc() derivatives.
Have I missed something?
--
Zbigniew
>
> ss->ss2.height = intel_buf_height(buf) - 1;
> diff --git a/lib/rendercopy_gen6.c b/lib/rendercopy_gen6.c
> index e941257eb606..ebd4c5cf241c 100644
> --- a/lib/rendercopy_gen6.c
> +++ b/lib/rendercopy_gen6.c
> @@ -91,10 +91,11 @@ gen6_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
> ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
> ss->ss0.color_blend = 1;
>
> - address = intel_bb_offset_reloc(ibb, buf->handle,
> - read_domain, write_domain,
> - intel_bb_offset(ibb) + 4,
> - buf->addr.offset);
> + address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> + read_domain, write_domain,
> + buf->surface[0].offset,
> + intel_bb_offset(ibb) + 4,
> + buf->addr.offset);
> ss->ss1.base_addr = (uint32_t) address;
>
> ss->ss2.height = intel_buf_height(buf) - 1;
> diff --git a/lib/rendercopy_gen7.c b/lib/rendercopy_gen7.c
> index 267f6f8038ba..279eb45bdc36 100644
> --- a/lib/rendercopy_gen7.c
> +++ b/lib/rendercopy_gen7.c
> @@ -86,10 +86,11 @@ gen7_bind_buf(struct intel_bb *ibb,
> gen7_tiling_bits(buf->tiling) |
> format << GEN7_SURFACE_FORMAT_SHIFT);
>
> - address = intel_bb_offset_reloc(ibb, buf->handle,
> - read_domain, write_domain,
> - intel_bb_offset(ibb) + 4,
> - buf->addr.offset);
> + address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> + read_domain, write_domain,
> + intel_bb_offset(ibb) + 4,
> + buf->surface[0].offset,
> + buf->addr.offset);
> ss[1] = address;
> ss[2] = ((intel_buf_width(buf) - 1) << GEN7_SURFACE_WIDTH_SHIFT |
> (intel_buf_height(buf) - 1) << GEN7_SURFACE_HEIGHT_SHIFT);
> diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c
> index ba7897fb475f..2039e5fc6b8e 100644
> --- a/lib/rendercopy_gen8.c
> +++ b/lib/rendercopy_gen8.c
> @@ -109,10 +109,11 @@ gen8_bind_buf(struct intel_bb *ibb,
> ss->ss1.memory_object_control = BDW_MOCS_PTE |
> BDW_MOCS_TC_L3_PTE | BDW_MOCS_AGE(0);
>
> - address = intel_bb_offset_reloc(ibb, buf->handle,
> - read_domain, write_domain,
> - intel_bb_offset(ibb) + 4 * 8,
> - buf->addr.offset);
> + address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> + read_domain, write_domain,
> + buf->surface[0].offset,
> + intel_bb_offset(ibb) + 4 * 8,
> + buf->addr.offset);
> ss->ss8.base_addr = address;
> ss->ss9.base_addr_hi = address >> 32;
>
> diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
> index 363bc6c1b203..bfac62b0fc23 100644
> --- a/lib/rendercopy_gen9.c
> +++ b/lib/rendercopy_gen9.c
> @@ -194,12 +194,13 @@ gen9_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst,
> else if (buf->tiling == I915_TILING_Ys)
> ss->ss5.trmode = 2;
>
> - address = intel_bb_offset_reloc(ibb, buf->handle,
> - read_domain, write_domain,
> - intel_bb_offset(ibb) + 4 * 8,
> - buf->addr.offset);
> - ss->ss8.base_addr = address;
> - ss->ss9.base_addr_hi = address >> 32;
> + address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> + read_domain, write_domain,
> + buf->surface[0].offset,
> + intel_bb_offset(ibb) + 4 * 8,
> + buf->addr.offset);
> + ss->ss8.base_addr = (address + buf->surface[0].offset);
> + ss->ss9.base_addr_hi = (address + buf->surface[0].offset) >> 32;
>
> ss->ss2.height = intel_buf_height(buf) - 1;
> ss->ss2.width = intel_buf_width(buf) - 1;
> diff --git a/lib/rendercopy_i830.c b/lib/rendercopy_i830.c
> index 4c4271493b4b..4b0ea3b859e2 100644
> --- a/lib/rendercopy_i830.c
> +++ b/lib/rendercopy_i830.c
> @@ -158,8 +158,10 @@ static void gen2_emit_target(struct intel_bb *ibb,
> intel_bb_out(ibb, _3DSTATE_BUF_INFO_CMD);
> intel_bb_out(ibb, BUF_3D_ID_COLOR_BACK | tiling |
> BUF_3D_PITCH(dst->surface[0].stride));
> - intel_bb_emit_reloc(ibb, dst->handle, I915_GEM_DOMAIN_RENDER,
> - I915_GEM_DOMAIN_RENDER, 0, dst->addr.offset);
> + intel_bb_emit_reloc(ibb, dst->handle,
> + I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
> + dst->surface[0].offset,
> + dst->addr.offset);
>
> intel_bb_out(ibb, _3DSTATE_DST_BUF_VARS_CMD);
> intel_bb_out(ibb, format |
> @@ -199,7 +201,9 @@ static void gen2_emit_texture(struct intel_bb *ibb,
> tiling |= TM0S1_TILE_WALK;
>
> intel_bb_out(ibb, _3DSTATE_LOAD_STATE_IMMEDIATE_2 | LOAD_TEXTURE_MAP(unit) | 4);
> - intel_bb_emit_reloc(ibb, src->handle, I915_GEM_DOMAIN_SAMPLER, 0, 0,
> + intel_bb_emit_reloc(ibb, src->handle,
> + I915_GEM_DOMAIN_SAMPLER, 0,
> + src->surface[0].offset,
> src->addr.offset);
> intel_bb_out(ibb, (intel_buf_height(src) - 1) << TM0S1_HEIGHT_SHIFT |
> (intel_buf_width(src) - 1) << TM0S1_WIDTH_SHIFT |
> diff --git a/lib/rendercopy_i915.c b/lib/rendercopy_i915.c
> index 3e421301e6a6..94cdfb99af9a 100644
> --- a/lib/rendercopy_i915.c
> +++ b/lib/rendercopy_i915.c
> @@ -112,7 +112,8 @@ void gen3_render_copyfunc(struct intel_bb *ibb,
> intel_bb_out(ibb, (1 << TEX_COUNT) - 1);
> intel_bb_emit_reloc(ibb, src->handle,
> I915_GEM_DOMAIN_SAMPLER, 0,
> - 0, src->addr.offset);
> + src->surface[0].offset,
> + src->addr.offset);
> intel_bb_out(ibb, format_bits | tiling_bits |
> (intel_buf_height(src) - 1) << MS3_HEIGHT_SHIFT |
> (intel_buf_width(src) - 1) << MS3_WIDTH_SHIFT);
> @@ -155,7 +156,8 @@ void gen3_render_copyfunc(struct intel_bb *ibb,
> BUF_3D_PITCH(dst->surface[0].stride));
> intel_bb_emit_reloc(ibb, dst->handle,
> I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
> - 0, dst->addr.offset);
> + dst->surface[0].offset,
> + dst->addr.offset);
>
> intel_bb_out(ibb, _3DSTATE_DST_BUF_VARS_CMD);
> intel_bb_out(ibb, format_bits |
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs
2023-12-22 5:37 ` [PATCH i-g-t " Zbigniew Kempczyński
@ 2023-12-22 6:01 ` Ville Syrjälä
2023-12-22 6:06 ` Ville Syrjälä
0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2023-12-22 6:01 UTC (permalink / raw)
To: Zbigniew Kempczyński; +Cc: igt-dev
On Fri, Dec 22, 2023 at 06:37:31AM +0100, Zbigniew Kempczyński wrote:
> On Wed, Dec 20, 2023 at 07:59:23PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > In order to copy stuff not at offset 0 in the BO we need
> > to include the delta in the relocs/etc.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > lib/rendercopy_gen4.c | 9 +++++----
> > lib/rendercopy_gen6.c | 9 +++++----
> > lib/rendercopy_gen7.c | 9 +++++----
> > lib/rendercopy_gen8.c | 9 +++++----
> > lib/rendercopy_gen9.c | 13 +++++++------
> > lib/rendercopy_i830.c | 10 +++++++---
> > lib/rendercopy_i915.c | 6 ++++--
> > 7 files changed, 38 insertions(+), 27 deletions(-)
> >
> > diff --git a/lib/rendercopy_gen4.c b/lib/rendercopy_gen4.c
> > index 8536d6b632c5..d10e5b7780c0 100644
> > --- a/lib/rendercopy_gen4.c
> > +++ b/lib/rendercopy_gen4.c
> > @@ -148,10 +148,11 @@ gen4_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
> > ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
> > ss->ss0.color_blend = 1;
> >
> > - address = intel_bb_offset_reloc(ibb, buf->handle,
> > - read_domain, write_domain,
> > - intel_bb_offset(ibb) + 4,
> > - buf->addr.offset);
> > + address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> > + read_domain, write_domain,
> > + buf->surface[0].offset,
> > + intel_bb_offset(ibb) + 4,
> > + buf->addr.offset);
> > ss->ss1.base_addr = (uint32_t) address;
>
> I've just took a look to intel_bb_add_reloc() because I didn't
> remember all details and I see address returned there is offset
> at which object will be binded, not offset + delta.
> Until this patch only users of this function reside in
> rendercopy_gen9.c so for older platforms it doesn't matter
> (xe driver doesn't support them).
>
> Anyway, delta addition sets up 10-bit if buf->cc.offset is not zero.
> I mean:
>
> address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> read_domain, write_domain,
> (buf->cc.offset ? (1 << 10) : 0)
> | buf->ccs[0].offset,
> intel_bb_offset(ibb) + 4 * 10,
> buf->addr.offset);
> ss->ss10.aux_base_addr = (address + buf->ccs[0].offset) >> 12;
> ss->ss11.aux_base_addr_hi = (address + buf->ccs[0].offset) >> 32;
>
> I wonder shouldn't intel_bb_add_reloc() return offset + delta.
>
> I also have some doubts regarding:
>
> (buf->cc.offset ? (1 << 10) : 0) | buf->ccs[0].offset
>
> This will set up bit-10 (if I'm not wrong clearvalue enable) only
> when relocation will happen. Without relocation or on softpinning
> this bit is never set.
>
> Above also would require remove delta in __intel_bb_emit_reloc()
> but if I'm not wrong this would be enough to start directly using
> offset returned in intel_bb_add_reloc() derivatives.
>
> Have I missed something?
No, I think you're right. The lack of the cc enable bit diretly
written to ss10 would seem to be wrong for the no-reloc case.
Also the >>12 there looks wrong. And the >>6 in the ss12 clear
color address looks wrong (and the reloc case should again
get fixed up by the kernel).
Sadly after fixing up those I still can't see the correct
output on tgl in kms_big_fb. So there is still something wrong :(
As for including the delta in the retuened value, we'd need to
review every usage to make sure no one relies on the current
behaviour.
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs
2023-12-22 6:01 ` Ville Syrjälä
@ 2023-12-22 6:06 ` Ville Syrjälä
2023-12-22 6:52 ` Ville Syrjälä
0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2023-12-22 6:06 UTC (permalink / raw)
To: Zbigniew Kempczyński; +Cc: igt-dev
On Fri, Dec 22, 2023 at 08:01:03AM +0200, Ville Syrjälä wrote:
> On Fri, Dec 22, 2023 at 06:37:31AM +0100, Zbigniew Kempczyński wrote:
> > On Wed, Dec 20, 2023 at 07:59:23PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > In order to copy stuff not at offset 0 in the BO we need
> > > to include the delta in the relocs/etc.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > > lib/rendercopy_gen4.c | 9 +++++----
> > > lib/rendercopy_gen6.c | 9 +++++----
> > > lib/rendercopy_gen7.c | 9 +++++----
> > > lib/rendercopy_gen8.c | 9 +++++----
> > > lib/rendercopy_gen9.c | 13 +++++++------
> > > lib/rendercopy_i830.c | 10 +++++++---
> > > lib/rendercopy_i915.c | 6 ++++--
> > > 7 files changed, 38 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/lib/rendercopy_gen4.c b/lib/rendercopy_gen4.c
> > > index 8536d6b632c5..d10e5b7780c0 100644
> > > --- a/lib/rendercopy_gen4.c
> > > +++ b/lib/rendercopy_gen4.c
> > > @@ -148,10 +148,11 @@ gen4_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
> > > ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
> > > ss->ss0.color_blend = 1;
> > >
> > > - address = intel_bb_offset_reloc(ibb, buf->handle,
> > > - read_domain, write_domain,
> > > - intel_bb_offset(ibb) + 4,
> > > - buf->addr.offset);
> > > + address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> > > + read_domain, write_domain,
> > > + buf->surface[0].offset,
> > > + intel_bb_offset(ibb) + 4,
> > > + buf->addr.offset);
> > > ss->ss1.base_addr = (uint32_t) address;
> >
> > I've just took a look to intel_bb_add_reloc() because I didn't
> > remember all details and I see address returned there is offset
> > at which object will be binded, not offset + delta.
> > Until this patch only users of this function reside in
> > rendercopy_gen9.c so for older platforms it doesn't matter
> > (xe driver doesn't support them).
> >
> > Anyway, delta addition sets up 10-bit if buf->cc.offset is not zero.
> > I mean:
> >
> > address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> > read_domain, write_domain,
> > (buf->cc.offset ? (1 << 10) : 0)
> > | buf->ccs[0].offset,
> > intel_bb_offset(ibb) + 4 * 10,
> > buf->addr.offset);
> > ss->ss10.aux_base_addr = (address + buf->ccs[0].offset) >> 12;
> > ss->ss11.aux_base_addr_hi = (address + buf->ccs[0].offset) >> 32;
> >
> > I wonder shouldn't intel_bb_add_reloc() return offset + delta.
> >
> > I also have some doubts regarding:
> >
> > (buf->cc.offset ? (1 << 10) : 0) | buf->ccs[0].offset
> >
> > This will set up bit-10 (if I'm not wrong clearvalue enable) only
> > when relocation will happen. Without relocation or on softpinning
> > this bit is never set.
> >
> > Above also would require remove delta in __intel_bb_emit_reloc()
> > but if I'm not wrong this would be enough to start directly using
> > offset returned in intel_bb_add_reloc() derivatives.
> >
> > Have I missed something?
>
> No, I think you're right. The lack of the cc enable bit diretly
> written to ss10 would seem to be wrong for the no-reloc case.
> Also the >>12 there looks wrong. And the >>6 in the ss12 clear
> color address looks wrong (and the reloc case should again
> get fixed up by the kernel).
Actually no, the extra shift are correct due to the bitfields.
But yeah clear color enable was missing for the no-reloc case.
But that doesn't help either. And I just realized that I'm actually
testing a modifier w/o CC anyway so the problem must be elsewhere.
>
> Sadly after fixing up those I still can't see the correct
> output on tgl in kms_big_fb. So there is still something wrong :(
>
> As for including the delta in the retuened value, we'd need to
> review every usage to make sure no one relies on the current
> behaviour.
>
> --
> Ville Syrjälä
> Intel
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs
2023-12-22 6:06 ` Ville Syrjälä
@ 2023-12-22 6:52 ` Ville Syrjälä
2023-12-22 7:26 ` Zbigniew Kempczyński
0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2023-12-22 6:52 UTC (permalink / raw)
To: Zbigniew Kempczyński; +Cc: igt-dev
On Fri, Dec 22, 2023 at 08:06:32AM +0200, Ville Syrjälä wrote:
> On Fri, Dec 22, 2023 at 08:01:03AM +0200, Ville Syrjälä wrote:
> > On Fri, Dec 22, 2023 at 06:37:31AM +0100, Zbigniew Kempczyński wrote:
> > > On Wed, Dec 20, 2023 at 07:59:23PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > In order to copy stuff not at offset 0 in the BO we need
> > > > to include the delta in the relocs/etc.
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > > lib/rendercopy_gen4.c | 9 +++++----
> > > > lib/rendercopy_gen6.c | 9 +++++----
> > > > lib/rendercopy_gen7.c | 9 +++++----
> > > > lib/rendercopy_gen8.c | 9 +++++----
> > > > lib/rendercopy_gen9.c | 13 +++++++------
> > > > lib/rendercopy_i830.c | 10 +++++++---
> > > > lib/rendercopy_i915.c | 6 ++++--
> > > > 7 files changed, 38 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/lib/rendercopy_gen4.c b/lib/rendercopy_gen4.c
> > > > index 8536d6b632c5..d10e5b7780c0 100644
> > > > --- a/lib/rendercopy_gen4.c
> > > > +++ b/lib/rendercopy_gen4.c
> > > > @@ -148,10 +148,11 @@ gen4_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
> > > > ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
> > > > ss->ss0.color_blend = 1;
> > > >
> > > > - address = intel_bb_offset_reloc(ibb, buf->handle,
> > > > - read_domain, write_domain,
> > > > - intel_bb_offset(ibb) + 4,
> > > > - buf->addr.offset);
> > > > + address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> > > > + read_domain, write_domain,
> > > > + buf->surface[0].offset,
> > > > + intel_bb_offset(ibb) + 4,
> > > > + buf->addr.offset);
> > > > ss->ss1.base_addr = (uint32_t) address;
> > >
> > > I've just took a look to intel_bb_add_reloc() because I didn't
> > > remember all details and I see address returned there is offset
> > > at which object will be binded, not offset + delta.
> > > Until this patch only users of this function reside in
> > > rendercopy_gen9.c so for older platforms it doesn't matter
> > > (xe driver doesn't support them).
> > >
> > > Anyway, delta addition sets up 10-bit if buf->cc.offset is not zero.
> > > I mean:
> > >
> > > address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> > > read_domain, write_domain,
> > > (buf->cc.offset ? (1 << 10) : 0)
> > > | buf->ccs[0].offset,
> > > intel_bb_offset(ibb) + 4 * 10,
> > > buf->addr.offset);
> > > ss->ss10.aux_base_addr = (address + buf->ccs[0].offset) >> 12;
> > > ss->ss11.aux_base_addr_hi = (address + buf->ccs[0].offset) >> 32;
> > >
> > > I wonder shouldn't intel_bb_add_reloc() return offset + delta.
> > >
> > > I also have some doubts regarding:
> > >
> > > (buf->cc.offset ? (1 << 10) : 0) | buf->ccs[0].offset
> > >
> > > This will set up bit-10 (if I'm not wrong clearvalue enable) only
> > > when relocation will happen. Without relocation or on softpinning
> > > this bit is never set.
> > >
> > > Above also would require remove delta in __intel_bb_emit_reloc()
> > > but if I'm not wrong this would be enough to start directly using
> > > offset returned in intel_bb_add_reloc() derivatives.
> > >
> > > Have I missed something?
> >
> > No, I think you're right. The lack of the cc enable bit diretly
> > written to ss10 would seem to be wrong for the no-reloc case.
> > Also the >>12 there looks wrong. And the >>6 in the ss12 clear
> > color address looks wrong (and the reloc case should again
> > get fixed up by the kernel).
>
> Actually no, the extra shift are correct due to the bitfields.
> But yeah clear color enable was missing for the no-reloc case.
> But that doesn't help either. And I just realized that I'm actually
> testing a modifier w/o CC anyway so the problem must be elsewhere.
OK, the problem seems to be that somehow the hardware thinks the
initial state of the AUX surface is compressed. So any part of the
FB I don't explicitly touch is left with interesting rainbow colors
when the zeroed main surface data is treated as compressed.
Did the AUX format change such that all zeroes in AUX is no
longer uncompressed?
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs
2023-12-22 6:52 ` Ville Syrjälä
@ 2023-12-22 7:26 ` Zbigniew Kempczyński
2023-12-22 8:33 ` Ville Syrjälä
0 siblings, 1 reply; 20+ messages in thread
From: Zbigniew Kempczyński @ 2023-12-22 7:26 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: igt-dev
On Fri, Dec 22, 2023 at 08:52:53AM +0200, Ville Syrjälä wrote:
> On Fri, Dec 22, 2023 at 08:06:32AM +0200, Ville Syrjälä wrote:
> > On Fri, Dec 22, 2023 at 08:01:03AM +0200, Ville Syrjälä wrote:
> > > On Fri, Dec 22, 2023 at 06:37:31AM +0100, Zbigniew Kempczyński wrote:
> > > > On Wed, Dec 20, 2023 at 07:59:23PM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > In order to copy stuff not at offset 0 in the BO we need
> > > > > to include the delta in the relocs/etc.
> > > > >
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > > lib/rendercopy_gen4.c | 9 +++++----
> > > > > lib/rendercopy_gen6.c | 9 +++++----
> > > > > lib/rendercopy_gen7.c | 9 +++++----
> > > > > lib/rendercopy_gen8.c | 9 +++++----
> > > > > lib/rendercopy_gen9.c | 13 +++++++------
> > > > > lib/rendercopy_i830.c | 10 +++++++---
> > > > > lib/rendercopy_i915.c | 6 ++++--
> > > > > 7 files changed, 38 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git a/lib/rendercopy_gen4.c b/lib/rendercopy_gen4.c
> > > > > index 8536d6b632c5..d10e5b7780c0 100644
> > > > > --- a/lib/rendercopy_gen4.c
> > > > > +++ b/lib/rendercopy_gen4.c
> > > > > @@ -148,10 +148,11 @@ gen4_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
> > > > > ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
> > > > > ss->ss0.color_blend = 1;
> > > > >
> > > > > - address = intel_bb_offset_reloc(ibb, buf->handle,
> > > > > - read_domain, write_domain,
> > > > > - intel_bb_offset(ibb) + 4,
> > > > > - buf->addr.offset);
> > > > > + address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> > > > > + read_domain, write_domain,
> > > > > + buf->surface[0].offset,
> > > > > + intel_bb_offset(ibb) + 4,
> > > > > + buf->addr.offset);
> > > > > ss->ss1.base_addr = (uint32_t) address;
> > > >
> > > > I've just took a look to intel_bb_add_reloc() because I didn't
> > > > remember all details and I see address returned there is offset
> > > > at which object will be binded, not offset + delta.
> > > > Until this patch only users of this function reside in
> > > > rendercopy_gen9.c so for older platforms it doesn't matter
> > > > (xe driver doesn't support them).
> > > >
> > > > Anyway, delta addition sets up 10-bit if buf->cc.offset is not zero.
> > > > I mean:
> > > >
> > > > address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> > > > read_domain, write_domain,
> > > > (buf->cc.offset ? (1 << 10) : 0)
> > > > | buf->ccs[0].offset,
> > > > intel_bb_offset(ibb) + 4 * 10,
> > > > buf->addr.offset);
> > > > ss->ss10.aux_base_addr = (address + buf->ccs[0].offset) >> 12;
> > > > ss->ss11.aux_base_addr_hi = (address + buf->ccs[0].offset) >> 32;
> > > >
> > > > I wonder shouldn't intel_bb_add_reloc() return offset + delta.
> > > >
> > > > I also have some doubts regarding:
> > > >
> > > > (buf->cc.offset ? (1 << 10) : 0) | buf->ccs[0].offset
> > > >
> > > > This will set up bit-10 (if I'm not wrong clearvalue enable) only
> > > > when relocation will happen. Without relocation or on softpinning
> > > > this bit is never set.
> > > >
> > > > Above also would require remove delta in __intel_bb_emit_reloc()
> > > > but if I'm not wrong this would be enough to start directly using
> > > > offset returned in intel_bb_add_reloc() derivatives.
> > > >
> > > > Have I missed something?
> > >
> > > No, I think you're right. The lack of the cc enable bit diretly
> > > written to ss10 would seem to be wrong for the no-reloc case.
> > > Also the >>12 there looks wrong. And the >>6 in the ss12 clear
> > > color address looks wrong (and the reloc case should again
> > > get fixed up by the kernel).
> >
> > Actually no, the extra shift are correct due to the bitfields.
> > But yeah clear color enable was missing for the no-reloc case.
> > But that doesn't help either. And I just realized that I'm actually
> > testing a modifier w/o CC anyway so the problem must be elsewhere.
>
> OK, the problem seems to be that somehow the hardware thinks the
> initial state of the AUX surface is compressed. So any part of the
> FB I don't explicitly touch is left with interesting rainbow colors
> when the zeroed main surface data is treated as compressed.
> Did the AUX format change such that all zeroes in AUX is no
> longer uncompressed?
I don't know, but I would expect zero means uncompressed.
Maybe J-P knows more about it?
--
Zbigniew
>
> --
> Ville Syrjälä
> Intel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs
2023-12-22 7:26 ` Zbigniew Kempczyński
@ 2023-12-22 8:33 ` Ville Syrjälä
0 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjälä @ 2023-12-22 8:33 UTC (permalink / raw)
To: Zbigniew Kempczyński; +Cc: igt-dev
On Fri, Dec 22, 2023 at 08:26:08AM +0100, Zbigniew Kempczyński wrote:
> On Fri, Dec 22, 2023 at 08:52:53AM +0200, Ville Syrjälä wrote:
> > On Fri, Dec 22, 2023 at 08:06:32AM +0200, Ville Syrjälä wrote:
> > > On Fri, Dec 22, 2023 at 08:01:03AM +0200, Ville Syrjälä wrote:
> > > > On Fri, Dec 22, 2023 at 06:37:31AM +0100, Zbigniew Kempczyński wrote:
> > > > > On Wed, Dec 20, 2023 at 07:59:23PM +0200, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > >
> > > > > > In order to copy stuff not at offset 0 in the BO we need
> > > > > > to include the delta in the relocs/etc.
> > > > > >
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > ---
> > > > > > lib/rendercopy_gen4.c | 9 +++++----
> > > > > > lib/rendercopy_gen6.c | 9 +++++----
> > > > > > lib/rendercopy_gen7.c | 9 +++++----
> > > > > > lib/rendercopy_gen8.c | 9 +++++----
> > > > > > lib/rendercopy_gen9.c | 13 +++++++------
> > > > > > lib/rendercopy_i830.c | 10 +++++++---
> > > > > > lib/rendercopy_i915.c | 6 ++++--
> > > > > > 7 files changed, 38 insertions(+), 27 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/rendercopy_gen4.c b/lib/rendercopy_gen4.c
> > > > > > index 8536d6b632c5..d10e5b7780c0 100644
> > > > > > --- a/lib/rendercopy_gen4.c
> > > > > > +++ b/lib/rendercopy_gen4.c
> > > > > > @@ -148,10 +148,11 @@ gen4_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst)
> > > > > > ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
> > > > > > ss->ss0.color_blend = 1;
> > > > > >
> > > > > > - address = intel_bb_offset_reloc(ibb, buf->handle,
> > > > > > - read_domain, write_domain,
> > > > > > - intel_bb_offset(ibb) + 4,
> > > > > > - buf->addr.offset);
> > > > > > + address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> > > > > > + read_domain, write_domain,
> > > > > > + buf->surface[0].offset,
> > > > > > + intel_bb_offset(ibb) + 4,
> > > > > > + buf->addr.offset);
> > > > > > ss->ss1.base_addr = (uint32_t) address;
> > > > >
> > > > > I've just took a look to intel_bb_add_reloc() because I didn't
> > > > > remember all details and I see address returned there is offset
> > > > > at which object will be binded, not offset + delta.
> > > > > Until this patch only users of this function reside in
> > > > > rendercopy_gen9.c so for older platforms it doesn't matter
> > > > > (xe driver doesn't support them).
> > > > >
> > > > > Anyway, delta addition sets up 10-bit if buf->cc.offset is not zero.
> > > > > I mean:
> > > > >
> > > > > address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> > > > > read_domain, write_domain,
> > > > > (buf->cc.offset ? (1 << 10) : 0)
> > > > > | buf->ccs[0].offset,
> > > > > intel_bb_offset(ibb) + 4 * 10,
> > > > > buf->addr.offset);
> > > > > ss->ss10.aux_base_addr = (address + buf->ccs[0].offset) >> 12;
> > > > > ss->ss11.aux_base_addr_hi = (address + buf->ccs[0].offset) >> 32;
> > > > >
> > > > > I wonder shouldn't intel_bb_add_reloc() return offset + delta.
> > > > >
> > > > > I also have some doubts regarding:
> > > > >
> > > > > (buf->cc.offset ? (1 << 10) : 0) | buf->ccs[0].offset
> > > > >
> > > > > This will set up bit-10 (if I'm not wrong clearvalue enable) only
> > > > > when relocation will happen. Without relocation or on softpinning
> > > > > this bit is never set.
> > > > >
> > > > > Above also would require remove delta in __intel_bb_emit_reloc()
> > > > > but if I'm not wrong this would be enough to start directly using
> > > > > offset returned in intel_bb_add_reloc() derivatives.
> > > > >
> > > > > Have I missed something?
> > > >
> > > > No, I think you're right. The lack of the cc enable bit diretly
> > > > written to ss10 would seem to be wrong for the no-reloc case.
> > > > Also the >>12 there looks wrong. And the >>6 in the ss12 clear
> > > > color address looks wrong (and the reloc case should again
> > > > get fixed up by the kernel).
> > >
> > > Actually no, the extra shift are correct due to the bitfields.
> > > But yeah clear color enable was missing for the no-reloc case.
> > > But that doesn't help either. And I just realized that I'm actually
> > > testing a modifier w/o CC anyway so the problem must be elsewhere.
> >
> > OK, the problem seems to be that somehow the hardware thinks the
> > initial state of the AUX surface is compressed. So any part of the
> > FB I don't explicitly touch is left with interesting rainbow colors
> > when the zeroed main surface data is treated as compressed.
> > Did the AUX format change such that all zeroes in AUX is no
> > longer uncompressed?
>
> I don't know, but I would expect zero means uncompressed.
Actually that part seems OK. What is not OK is that when we write
all black compressed pixels we do something wrong. Feels like we're
writing some kind of fast clear data in that case that the display
doesn't understand (since I'm not using a CC modifier). So I
suppose the question is whether we can somehow disable that,
or if we need to do an explicit resolve afterwards.
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH i-g-t 02/12] tests/kms_big_fb: Use igt_fb_create_intel_buf()
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 03/12] lib/igt_fb: Expose igt_fb_is_ccs_modifier() Ville Syrjala
` (9 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Use igt_fb_create_intel_buf() instead of hand rolling something
similar but less capable (igt_fb_create_intel_buf() handles
planar and compressed formats, the hand rolled version does not).
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
tests/intel/kms_big_fb.c | 27 +--------------------------
1 file changed, 1 insertion(+), 26 deletions(-)
diff --git a/tests/intel/kms_big_fb.c b/tests/intel/kms_big_fb.c
index 3eb43515a1fd..eef8148d2ba3 100644
--- a/tests/intel/kms_big_fb.c
+++ b/tests/intel/kms_big_fb.c
@@ -187,32 +187,7 @@ static struct intel_buf *init_buf(data_t *data,
const struct igt_fb *fb,
const char *buf_name)
{
- struct intel_buf *buf;
- enum intel_driver driver = buf_ops_get_driver(data->bops);
- uint32_t name, handle, tiling, stride, width, height, bpp, size;
- uint64_t region = driver == INTEL_DRIVER_XE ?
- vram_if_possible(data->drm_fd, 0) : -1;
-
- igt_assert_eq(fb->offsets[0], 0);
-
- tiling = igt_fb_mod_to_tiling(fb->modifier);
- stride = fb->strides[0];
- bpp = fb->plane_bpp[0];
- size = fb->size;
- width = stride / (bpp / 8);
- height = size / stride;
-
- name = gem_flink(data->drm_fd, fb->gem_handle);
- handle = gem_open(data->drm_fd, name);
- buf = intel_buf_create_full(data->bops, handle, width, height,
- bpp, 0, tiling, 0, size, 0,
- region,
- intel_get_pat_idx_uc(data->drm_fd));
-
- intel_buf_set_name(buf, buf_name);
- intel_buf_set_ownership(buf, true);
-
- return buf;
+ return igt_fb_create_intel_buf(data->drm_fd, data->bops, fb, buf_name);
}
static void fini_buf(struct intel_buf *buf)
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t 03/12] lib/igt_fb: Expose igt_fb_is_ccs_modifier()
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 01/12] lib/rendercopy: Add deltas to all surface relocs Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 02/12] tests/kms_big_fb: Use igt_fb_create_intel_buf() Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 04/12] tests/kms_big_fb: Fix async Ville Syrjala
` (8 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
I'm going to need is_ccs_modifier() outside of igt_fb.c.
Rename it to igt_fb_is_ccs_modifier() and expose it to everyone.
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
lib/igt_fb.c | 21 ++++++++++++---------
lib/igt_fb.h | 1 +
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 683eb176b7a4..d7d8840fa432 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -605,7 +605,7 @@ static bool is_gen12_ccs_modifier(uint64_t modifier)
modifier == I915_FORMAT_MOD_4_TILED_MTL_RC_CCS_CC;
}
-static bool is_ccs_modifier(uint64_t modifier)
+bool igt_fb_is_ccs_modifier(uint64_t modifier)
{
return is_gen12_ccs_modifier(modifier) ||
modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
@@ -614,7 +614,8 @@ static bool is_ccs_modifier(uint64_t modifier)
static bool is_ccs_plane(const struct igt_fb *fb, int plane)
{
- if (!is_ccs_modifier(fb->modifier) || HAS_FLATCCS(intel_get_drm_devid(fb->fd)))
+ if (!igt_fb_is_ccs_modifier(fb->modifier) ||
+ HAS_FLATCCS(intel_get_drm_devid(fb->fd)))
return false;
return plane >= fb->num_planes / 2;
@@ -724,7 +725,8 @@ static int fb_num_planes(const struct igt_fb *fb)
{
int num_planes = lookup_drm_format(fb->drm_format)->num_planes;
- if (is_ccs_modifier(fb->modifier) && !HAS_FLATCCS(intel_get_drm_devid(fb->fd)))
+ if (igt_fb_is_ccs_modifier(fb->modifier) &&
+ !HAS_FLATCCS(intel_get_drm_devid(fb->fd)))
num_planes *= 2;
if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC ||
@@ -2491,7 +2493,7 @@ static enum blt_tiling_type fb_tile_to_blt_tile(uint64_t tile)
static bool fast_blit_ok(const struct igt_fb *fb)
{
return blt_has_fast_copy(fb->fd) &&
- !is_ccs_modifier(fb->modifier) &&
+ !igt_fb_is_ccs_modifier(fb->modifier) &&
blt_fast_copy_supports_tiling(fb->fd,
fb_tile_to_blt_tile(fb->modifier));
}
@@ -2508,7 +2510,7 @@ static bool blitter_ok(const struct igt_fb *fb)
if (!is_intel_device(fb->fd))
return false;
- if ((is_ccs_modifier(fb->modifier) &&
+ if ((igt_fb_is_ccs_modifier(fb->modifier) &&
!HAS_FLATCCS(intel_get_drm_devid(fb->fd))) ||
is_gen12_mc_ccs_modifier(fb->modifier))
return false;
@@ -2548,7 +2550,8 @@ static bool use_enginecopy(const struct igt_fb *fb)
return false;
return fb->modifier == I915_FORMAT_MOD_Yf_TILED ||
- (!HAS_FLATCCS(intel_get_drm_devid(fb->fd)) && is_ccs_modifier(fb->modifier)) ||
+ (!HAS_FLATCCS(intel_get_drm_devid(fb->fd)) &&
+ igt_fb_is_ccs_modifier(fb->modifier)) ||
is_gen12_mc_ccs_modifier(fb->modifier);
}
@@ -2608,7 +2611,7 @@ igt_fb_create_intel_buf(int fd, struct buf_ops *bops,
igt_assert_eq(fb->offsets[0], 0);
- if (is_ccs_modifier(fb->modifier)) {
+ if (igt_fb_is_ccs_modifier(fb->modifier)) {
igt_assert_eq(fb->strides[0] & 127, 0);
if (is_gen12_ccs_modifier(fb->modifier)) {
@@ -2651,7 +2654,7 @@ igt_fb_create_intel_buf(int fd, struct buf_ops *bops,
if (buf->format_is_yuv_semiplanar)
buf->yuv_semiplanar_bpp = yuv_semiplanar_bpp(fb->drm_format);
- if (is_ccs_modifier(fb->modifier)) {
+ if (igt_fb_is_ccs_modifier(fb->modifier)) {
num_surfaces = fb->num_planes / (HAS_FLATCCS(intel_get_drm_devid(fb->fd)) ? 1 : 2);
for (i = 0; i < num_surfaces; i++)
init_buf_ccs(buf, i,
@@ -2773,7 +2776,7 @@ static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb,
intel_get_uc_mocs_index(fb->fd),
intel_get_pat_idx_uc(fb->fd),
blt_tile,
- is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED : COMPRESSION_DISABLED,
+ igt_fb_is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED : COMPRESSION_DISABLED,
is_gen12_mc_ccs_modifier(fb->modifier) ? COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D);
blt_set_geom(blt, stride, 0, 0, fb->width, fb->plane_height[plane], 0, 0);
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index 834aaef54dea..0b85819b0f9e 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -182,6 +182,7 @@ void igt_fb_calc_crc(struct igt_fb *fb, igt_crc_t *crc);
uint64_t igt_fb_mod_to_tiling(uint64_t modifier);
uint64_t igt_fb_tiling_to_mod(uint64_t tiling);
+bool igt_fb_is_ccs_modifier(uint64_t modifier);
bool igt_fb_is_ccs_plane(const struct igt_fb *fb, int plane);
bool igt_fb_is_gen12_ccs_cc_plane(const struct igt_fb *fb, int plane);
int igt_fb_ccs_to_main_plane(const struct igt_fb *fb, int ccs_plane);
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t 04/12] tests/kms_big_fb: Fix async
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
` (2 preceding siblings ...)
2023-12-20 17:59 ` [PATCH i-g-t 03/12] lib/igt_fb: Expose igt_fb_is_ccs_modifier() Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 05/12] tests/kms_big_fb: Test async flips + linear on tgl+ Ville Syrjala
` (7 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
The 'continue' statement causes us to skip test_cleanup()
which means the next iteration of the loop will not re-create
the fb, instead attempting to use the previous fb which has
the wrong format. The result is that things blow up. Skip
the async flip test using igt_require() instead which will let
the cleanup fixture do its job.
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
tests/intel/kms_big_fb.c | 33 ++++++++++++++++++++++++---------
1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/tests/intel/kms_big_fb.c b/tests/intel/kms_big_fb.c
index eef8148d2ba3..6393504be7d7 100644
--- a/tests/intel/kms_big_fb.c
+++ b/tests/intel/kms_big_fb.c
@@ -75,6 +75,12 @@
* and %arg[2]-rotation
* Functionality: big_fbs, kms_gem_interop, rotation
*
+ * SUBTEST: linear-max-hw-stride-%dbpp-rotate-%d-async-flip
+ * Description: Test maximum hardware supported stride length for given combination
+ * of linear modifier with max hardware stride length, %arg[1]-bpp,
+ * and %arg[2]-rotation
+ * Functionality: async_flips, big_fbs, kms_gem_interop, rotation
+ *
* arg[1].values: 32, 64
* arg[2].values: 0, 180
*/
@@ -918,6 +924,18 @@ static void test_cleanup(data_t *data)
data->output = NULL;
}
+static bool has_async_flip(data_t *data)
+{
+ /*
+ * TODO: preferably probe all this stuff with
+ * TEST_ONLY rather than hardcoding it...
+ */
+ if (data->modifier == DRM_FORMAT_MOD_LINEAR)
+ return false;
+
+ return igt_has_drm_cap(data->drm_fd, DRM_CAP_ASYNC_PAGE_FLIP);
+}
+
static data_t data = {};
static const struct {
@@ -1108,17 +1126,14 @@ igt_main
test_scanout(&data);
}
- // async flip doesn't support linear fbs.
- if (modifiers[i].modifier == DRM_FORMAT_MOD_LINEAR)
- continue;
-
data.async_flip_test = true;
igt_describe("test async flip on maximum hardware supported stride length for given bpp and modifiers.");
- igt_subtest_f("%s-max-hw-stride-%dbpp-rotate-%d%s-async-flip", modifiers[i].name,
- formats[j].bpp, rotations[k].angle, fliptab[l].flipname) {
- igt_require(igt_has_drm_cap(data.drm_fd, DRM_CAP_ASYNC_PAGE_FLIP));
- data.max_hw_fb_width = min(data.hw_stride / (formats[j].bpp >> 3), data.max_fb_width);
- test_scanout(&data);
+ igt_subtest_f("%s-max-hw-stride-%dbpp-rotate-%d%s-async-flip",
+ modifiers[i].name, formats[j].bpp,
+ rotations[k].angle, fliptab[l].flipname) {
+ igt_require(has_async_flip(&data));
+ data.max_hw_fb_width = min(data.hw_stride / (formats[j].bpp >> 3), data.max_fb_width);
+ test_scanout(&data);
}
data.async_flip_test = false;
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t 05/12] tests/kms_big_fb: Test async flips + linear on tgl+
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
` (3 preceding siblings ...)
2023-12-20 17:59 ` [PATCH i-g-t 04/12] tests/kms_big_fb: Fix async Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 06/12] tests/kms_big_fb: Determine the max fb size the same way always Ville Syrjala
` (6 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
i915 claims to support linear+async flips on TGL+,
so let's test that here as well.
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
tests/intel/kms_big_fb.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/intel/kms_big_fb.c b/tests/intel/kms_big_fb.c
index 6393504be7d7..75b0c4758743 100644
--- a/tests/intel/kms_big_fb.c
+++ b/tests/intel/kms_big_fb.c
@@ -930,7 +930,8 @@ static bool has_async_flip(data_t *data)
* TODO: preferably probe all this stuff with
* TEST_ONLY rather than hardcoding it...
*/
- if (data->modifier == DRM_FORMAT_MOD_LINEAR)
+ if (intel_display_ver(data->devid) < 12 &&
+ data->modifier == DRM_FORMAT_MOD_LINEAR)
return false;
return igt_has_drm_cap(data->drm_fd, DRM_CAP_ASYNC_PAGE_FLIP);
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t 06/12] tests/kms_big_fb: Determine the max fb size the same way always
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
` (4 preceding siblings ...)
2023-12-20 17:59 ` [PATCH i-g-t 05/12] tests/kms_big_fb: Test async flips + linear on tgl+ Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 07/12] tests/kms_frontbuffer_tracking: Use igt_create_fb() Ville Syrjala
` (5 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Currently the scanout tests and the addfb test may determine
the max framebuffer size differently. That may be OK for now,
but I have plans to extend the test to cover compressed formats
as well, and those can't generally use the reported max fb size.
So reuse the same piece of code to always determine the max fb
size (to which I can add compressed formats checks later).
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
tests/intel/kms_big_fb.c | 41 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/tests/intel/kms_big_fb.c b/tests/intel/kms_big_fb.c
index 75b0c4758743..f758155d9bdd 100644
--- a/tests/intel/kms_big_fb.c
+++ b/tests/intel/kms_big_fb.c
@@ -337,7 +337,6 @@ static bool size_ok(data_t *data, uint64_t size)
return true;
}
-
static void max_fb_size(data_t *data, int *width, int *height,
uint32_t format, uint64_t modifier)
{
@@ -345,6 +344,21 @@ static void max_fb_size(data_t *data, int *width, int *height,
uint64_t size;
int i = 0;
+ if (data->max_hw_stride_test) {
+ igt_output_t *output;
+
+ *width = data->max_hw_fb_width;
+ *height = 0;
+
+ for_each_connected_output(&data->display, output) {
+ if (*height < output->config.default_mode.vdisplay * 2)
+ *height = output->config.default_mode.vdisplay * 2;
+ }
+ } else {
+ *width = data->max_fb_width;
+ *height = data->max_fb_height;
+ }
+
/* max fence stride is only 8k bytes on gen3 */
if (intel_display_ver(data->devid) < 4 &&
format == DRM_FORMAT_XRGB8888)
@@ -690,26 +704,11 @@ max_hw_stride_async_flip_test(data_t *data)
static void test_scanout(data_t *data)
{
- igt_output_t *output;
-
igt_require(data->format == DRM_FORMAT_C8 ||
igt_fb_supported_format(data->format));
igt_require(igt_display_has_format_mod(&data->display, data->format, data->modifier));
- if (data->max_hw_stride_test) {
- data->big_fb_width = data->max_hw_fb_width;
- data->big_fb_height = 0;
-
- for_each_connected_output(&data->display, output) {
- if (data->big_fb_height < output->config.default_mode.vdisplay * 2)
- data->big_fb_height = output->config.default_mode.vdisplay * 2;
- }
- } else {
- data->big_fb_width = data->max_fb_width;
- data->big_fb_height = data->max_fb_height;
- }
-
max_fb_size(data, &data->big_fb_width, &data->big_fb_height,
data->format, data->modifier);
@@ -844,6 +843,7 @@ test_addfb(data_t *data)
uint32_t offsets[4] = {};
uint32_t strides[4] = {};
uint32_t format;
+ int width, height;
int ret;
/*
@@ -859,9 +859,9 @@ test_addfb(data_t *data)
igt_require(igt_display_has_format_mod(&data->display,
format, data->modifier));
- igt_calc_fb_size(data->drm_fd,
- data->max_fb_width,
- data->max_fb_height,
+ max_fb_size(data, &width, &height, format, data->modifier);
+
+ igt_calc_fb_size(data->drm_fd, width, height,
format, data->modifier,
&size, &strides[0]);
@@ -878,8 +878,7 @@ test_addfb(data_t *data)
igt_fb_mod_to_tiling(data->modifier), strides[0]);
ret = __kms_addfb(data->drm_fd, bo,
- data->max_fb_width,
- data->max_fb_height,
+ width, height,
format, data->modifier,
strides, offsets, 1,
DRM_MODE_FB_MODIFIERS, &fb_id);
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t 07/12] tests/kms_frontbuffer_tracking: Use igt_create_fb()
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
` (5 preceding siblings ...)
2023-12-20 17:59 ` [PATCH i-g-t 06/12] tests/kms_big_fb: Determine the max fb size the same way always Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 08/12] lib/igt_fb: Make igt_calc_fb_size() somewhat usable Ville Syrjala
` (4 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Calling igt_calc_fb_size() and igt_create_fb_with_bo_size() like
this back to back is the same as just calling igt_create_fb().
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
tests/intel/kms_frontbuffer_tracking.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/tests/intel/kms_frontbuffer_tracking.c b/tests/intel/kms_frontbuffer_tracking.c
index 1daf27d2e006..f3dc30024691 100644
--- a/tests/intel/kms_frontbuffer_tracking.c
+++ b/tests/intel/kms_frontbuffer_tracking.c
@@ -1615,8 +1615,7 @@ static void create_fb(enum pixel_format pformat, int width, int height,
enum tiling_type tiling, int plane, struct igt_fb *fb)
{
uint32_t format;
- uint64_t size, modifier;
- unsigned int stride;
+ uint64_t modifier;
switch (pformat) {
case FORMAT_RGB888:
@@ -1650,13 +1649,7 @@ static void create_fb(enum pixel_format pformat, int width, int height,
igt_warn_on(plane == PLANE_CUR && tiling != TILING_LINEAR);
- igt_calc_fb_size(drm.fd, width, height, format, modifier, &size,
- &stride);
-
- igt_create_fb_with_bo_size(drm.fd, width, height, format, modifier,
- IGT_COLOR_YCBCR_BT709,
- IGT_COLOR_YCBCR_LIMITED_RANGE,
- fb, size, stride);
+ igt_create_fb(drm.fd, width, height, format, modifier, fb);
}
static uint32_t pick_color(struct igt_fb *fb, enum color ecolor)
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t 08/12] lib/igt_fb: Make igt_calc_fb_size() somewhat usable
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
` (6 preceding siblings ...)
2023-12-20 17:59 ` [PATCH i-g-t 07/12] tests/kms_frontbuffer_tracking: Use igt_create_fb() Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 09/12] tests/kms_big_fb: Nuke fliptab[] Ville Syrjala
` (3 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
igt_calc_fb_size() is very awkward to use in combinations
with planar/compressed formats. To fix it we either need
to add tons of output parameters (strides/offsets/etc.),
or we just get rid of it and promote calc_fb_size() to
take its place. I chose the latter approach. This does
mean that the callers will need to have a struct igt_fb
around, but I think that's nicer than adding tons of
output parameters.
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
lib/igt_fb.c | 52 +++++++++++-----------------------------
lib/igt_fb.h | 3 +--
tests/intel/gem_pxp.c | 4 +---
tests/intel/kms_big_fb.c | 33 +++++++++++++------------
tests/kms_addfb_basic.c | 14 ++++++-----
tests/kms_prime.c | 12 ++++++++--
tests/kms_rotation_crc.c | 10 ++++----
7 files changed, 56 insertions(+), 72 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index d7d8840fa432..a94815a67108 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -936,7 +936,15 @@ static unsigned int get_plane_alignment(struct igt_fb *fb, int color_plane)
return alignment;
}
-static uint64_t calc_fb_size(struct igt_fb *fb)
+/**
+ * igt_calc_fb_size:
+ * @fb: the framebuffer
+ *
+ * This function calculates the framebuffer size/strides/offsets/etc.
+ * appropriately. The framebuffer needs to be sufficiently initialized
+ * beforehand eg. with igt_init_fb().
+ */
+void igt_calc_fb_size(struct igt_fb *fb)
{
uint64_t size = 0;
int plane;
@@ -958,38 +966,11 @@ static uint64_t calc_fb_size(struct igt_fb *fb)
}
if (is_xe_device(fb->fd))
- size = ALIGN(size, xe_get_default_alignment(fb->fd));
-
- return size;
-}
-
-/**
- * igt_calc_fb_size:
- * @fd: the DRM file descriptor
- * @width: width of the framebuffer in pixels
- * @height: height of the framebuffer in pixels
- * @format: drm fourcc pixel format code
- * @modifier: tiling layout of the framebuffer (as framebuffer modifier)
- * @size_ret: returned size for the framebuffer
- * @stride_ret: returned stride for the framebuffer
- *
- * This function returns valid stride and size values for a framebuffer with the
- * specified parameters.
- */
-void igt_calc_fb_size(int fd, int width, int height, uint32_t drm_format, uint64_t modifier,
- uint64_t *size_ret, unsigned *stride_ret)
-{
- struct igt_fb fb;
-
- igt_init_fb(&fb, fd, width, height, drm_format, modifier,
- IGT_COLOR_YCBCR_BT709, IGT_COLOR_YCBCR_LIMITED_RANGE);
-
- fb.size = calc_fb_size(&fb);
+ size = ALIGN(fb->size, xe_get_default_alignment(fb->fd));
- if (size_ret)
- *size_ret = fb.size;
- if (stride_ret)
- *stride_ret = fb.strides[0];
+ /* Respect the size requested by the caller. */
+ if (fb->size == 0)
+ fb->size = size;
}
/**
@@ -1174,7 +1155,6 @@ static int create_bo_for_fb(struct igt_fb *fb, bool prefer_sysmem)
unsigned *strides = &fb->strides[0];
bool device_bo = false;
int fd = fb->fd;
- uint64_t size;
/*
* The current dumb buffer allocation API doesn't really allow to
@@ -1189,11 +1169,7 @@ static int create_bo_for_fb(struct igt_fb *fb, bool prefer_sysmem)
device_bo = true;
/* Sets offets and stride if necessary. */
- size = calc_fb_size(fb);
-
- /* Respect the size requested by the caller. */
- if (fb->size == 0)
- fb->size = size;
+ igt_calc_fb_size(fb);
if (device_bo) {
fb->is_dumb = false;
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index 0b85819b0f9e..b1b40b858610 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -125,8 +125,7 @@ enum igt_text_align {
void igt_get_fb_tile_size(int fd, uint64_t modifier, int fb_bpp,
unsigned *width_ret, unsigned *height_ret);
-void igt_calc_fb_size(int fd, int width, int height, uint32_t format, uint64_t modifier,
- uint64_t *size_ret, unsigned *stride_ret);
+void igt_calc_fb_size(struct igt_fb *fb);
void igt_init_fb(struct igt_fb *fb, int fd, int width, int height,
uint32_t drm_format, uint64_t modifier,
enum igt_color_encoding color_encoding,
diff --git a/tests/intel/gem_pxp.c b/tests/intel/gem_pxp.c
index dff4a5d25222..8af4f1fd60c4 100644
--- a/tests/intel/gem_pxp.c
+++ b/tests/intel/gem_pxp.c
@@ -1128,9 +1128,7 @@ static void setup_protected_fb(int i915, int width, int height, igt_fb_t *fb, ui
igt_init_fb(fb, i915, width, height, DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_NONE,
IGT_COLOR_YCBCR_BT709, IGT_COLOR_YCBCR_LIMITED_RANGE);
-
- igt_calc_fb_size(i915, width, height, DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_NONE,
- &fb->size, &fb->strides[0]);
+ igt_calc_fb_size(fb);
err = create_bo_ext(i915, fb->size, true, &(fb->gem_handle));
igt_assert_eq(err, 0);
diff --git a/tests/intel/kms_big_fb.c b/tests/intel/kms_big_fb.c
index f758155d9bdd..f0b3ac7d66c3 100644
--- a/tests/intel/kms_big_fb.c
+++ b/tests/intel/kms_big_fb.c
@@ -340,8 +340,7 @@ static bool size_ok(data_t *data, uint64_t size)
static void max_fb_size(data_t *data, int *width, int *height,
uint32_t format, uint64_t modifier)
{
- unsigned int stride;
- uint64_t size;
+ struct igt_fb fb;
int i = 0;
if (data->max_hw_stride_test) {
@@ -364,17 +363,19 @@ static void max_fb_size(data_t *data, int *width, int *height,
format == DRM_FORMAT_XRGB8888)
*width = min(*width, 8192 / 4);
- igt_calc_fb_size(data->drm_fd, *width, *height,
- format, modifier, &size, &stride);
+ igt_init_fb(&fb, data->drm_fd, *width, *height, format, modifier,
+ IGT_COLOR_YCBCR_BT709, IGT_COLOR_YCBCR_LIMITED_RANGE);
+ igt_calc_fb_size(&fb);
- while (!size_ok(data, size)) {
+ while (!size_ok(data, fb.size)) {
if (i++ & 1)
*width >>= 1;
else
*height >>= 1;
- igt_calc_fb_size(data->drm_fd, *width, *height,
- format, modifier, &size, &stride);
+ igt_init_fb(&fb, data->drm_fd, *width, *height, format, modifier,
+ IGT_COLOR_YCBCR_BT709, IGT_COLOR_YCBCR_LIMITED_RANGE);
+ igt_calc_fb_size(&fb);
}
igt_info("Max usable framebuffer size for format "IGT_FORMAT_FMT" / modifier 0x%"PRIx64": %dx%d\n",
@@ -837,11 +838,9 @@ static int rmfb(int fd, uint32_t id)
static void
test_addfb(data_t *data)
{
- uint64_t size;
+ struct igt_fb fb;
uint32_t fb_id;
uint32_t bo;
- uint32_t offsets[4] = {};
- uint32_t strides[4] = {};
uint32_t format;
int width, height;
int ret;
@@ -861,26 +860,26 @@ test_addfb(data_t *data)
max_fb_size(data, &width, &height, format, data->modifier);
- igt_calc_fb_size(data->drm_fd, width, height,
- format, data->modifier,
- &size, &strides[0]);
+ igt_init_fb(&fb, data->drm_fd, width, height, format, data->modifier,
+ IGT_COLOR_YCBCR_BT709, IGT_COLOR_YCBCR_LIMITED_RANGE);
+ igt_calc_fb_size(&fb);
if (is_i915_device(data->drm_fd))
- bo = gem_buffer_create_fb_obj(data->drm_fd, size);
+ bo = gem_buffer_create_fb_obj(data->drm_fd, fb.size);
else
bo = xe_bo_create(data->drm_fd, 0,
- ALIGN(size, xe_get_default_alignment(data->drm_fd)),
+ ALIGN(fb.size, xe_get_default_alignment(data->drm_fd)),
vram_if_possible(data->drm_fd, 0), 0);
igt_require(bo);
if (is_i915_device(data->drm_fd) && intel_display_ver(data->devid) < 4)
gem_set_tiling(data->drm_fd, bo,
- igt_fb_mod_to_tiling(data->modifier), strides[0]);
+ igt_fb_mod_to_tiling(data->modifier), fb.strides[0]);
ret = __kms_addfb(data->drm_fd, bo,
width, height,
format, data->modifier,
- strides, offsets, 1,
+ fb.strides, fb.offsets, fb.num_planes,
DRM_MODE_FB_MODIFIERS, &fb_id);
igt_assert_eq(ret, 0);
diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
index 5c812a49fce8..8fe22ec05166 100644
--- a/tests/kms_addfb_basic.c
+++ b/tests/kms_addfb_basic.c
@@ -294,19 +294,21 @@ static void invalid_tests(int fd)
igt_describe("Check if addfb2 with a system memory gem object "
"fails correctly if device requires local memory framebuffers");
igt_subtest("invalid-smem-bo-on-discrete") {
- uint32_t handle, stride;
- uint64_t size;
+ struct igt_fb fb;
+ uint32_t handle;
igt_require_intel(fd);
- igt_calc_fb_size(fd, f.width, f.height,
- DRM_FORMAT_XRGB8888, 0, &size, &stride);
+ igt_init_fb(&fb, fd, f.width, f.height,
+ DRM_FORMAT_XRGB8888, 0,
+ IGT_COLOR_YCBCR_BT709, IGT_COLOR_YCBCR_LIMITED_RANGE);
+ igt_calc_fb_size(&fb);
if (is_i915_device(fd)) {
igt_require(gem_has_lmem(fd));
- handle = gem_create_in_memory_regions(fd, size, REGION_SMEM);
+ handle = gem_create_in_memory_regions(fd, fb.size, REGION_SMEM);
} else {
igt_require(xe_has_vram(fd));
- handle = xe_bo_create(fd, 0, size, system_memory(fd), 0);
+ handle = xe_bo_create(fd, 0, fb.size, system_memory(fd), 0);
}
f.handles[0] = handle;
diff --git a/tests/kms_prime.c b/tests/kms_prime.c
index 135c7516819d..3784cec59d9d 100644
--- a/tests/kms_prime.c
+++ b/tests/kms_prime.c
@@ -149,8 +149,16 @@ static void prepare_scratch(int exporter_fd, struct dumb_bo *scratch,
ptr = kmstest_dumb_map_buffer(exporter_fd, scratch->handle,
scratch->size, PROT_WRITE);
} else {
- igt_calc_fb_size(exporter_fd, mode->hdisplay, mode->vdisplay, DRM_FORMAT_XRGB8888,
- DRM_FORMAT_MOD_LINEAR, &scratch->size, &scratch->pitch);
+ struct igt_fb fb;
+
+ igt_init_fb(&fb, exporter_fd, mode->hdisplay, mode->vdisplay,
+ DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
+ IGT_COLOR_YCBCR_BT709, IGT_COLOR_YCBCR_LIMITED_RANGE);
+ igt_calc_fb_size(&fb);
+
+ scratch->size = fb.size;
+ scratch->pitch = fb.strides[0];
+
if (gem_has_lmem(exporter_fd))
scratch->handle = gem_create_in_memory_regions(exporter_fd, scratch->size,
REGION_LMEM(0), REGION_SMEM);
diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
index 8d8c53b5ff84..9888ac6ac3c4 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -1091,8 +1091,8 @@ static void test_plane_rotation_exhaust_fences(data_t *data,
int fd = data->gfx_fd;
drmModeModeInfo *mode;
struct igt_fb fb[MAX_FENCES+1] = {};
- uint64_t size;
- unsigned int stride, w, h;
+ struct igt_fb tmp_fb;
+ unsigned int w, h;
uint64_t total_aperture_size, total_fbs_size;
int i;
@@ -1106,13 +1106,15 @@ static void test_plane_rotation_exhaust_fences(data_t *data,
w = mode->hdisplay;
h = mode->vdisplay;
- igt_calc_fb_size(fd, w, h, format, modifier, &size, &stride);
+ igt_init_fb(&tmp_fb, fd, w, h, format, modifier,
+ IGT_COLOR_YCBCR_BT709, IGT_COLOR_YCBCR_LIMITED_RANGE);
+ igt_calc_fb_size(&tmp_fb);
/*
* Make sure there is atleast 90% of the available GTT space left
* for creating (MAX_FENCES+1) framebuffers.
*/
- total_fbs_size = size * (MAX_FENCES + 1);
+ total_fbs_size = tmp_fb.size * (MAX_FENCES + 1);
total_aperture_size = gem_available_aperture_size(fd);
igt_require(total_fbs_size < total_aperture_size * 0.9);
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t 09/12] tests/kms_big_fb: Nuke fliptab[]
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
` (7 preceding siblings ...)
2023-12-20 17:59 ` [PATCH i-g-t 08/12] lib/igt_fb: Make igt_calc_fb_size() somewhat usable Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 10/12] tests/kms_big_fb: Replace 'bpp' with 'name' Ville Syrjala
` (2 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Let's just stuff the full transformation into rotations[]
and get rid of fliptab[] and yet another nested loop.
This also extends the hflip tests to be done universally
(as opposed to just in combination with the max-hw-stride
tests).
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
tests/intel/kms_big_fb.c | 139 ++++++++++++++++++++++-----------------
1 file changed, 77 insertions(+), 62 deletions(-)
diff --git a/tests/intel/kms_big_fb.c b/tests/intel/kms_big_fb.c
index f0b3ac7d66c3..675afb54905d 100644
--- a/tests/intel/kms_big_fb.c
+++ b/tests/intel/kms_big_fb.c
@@ -47,6 +47,11 @@
* of Linear modifier with %arg[1]-bpp & %arg[2]-rotation
* Functionality: big_fbs, kms_gem_interop, rotation
*
+ * SUBTEST: linear-%dbpp-rotate-%d-hflip
+ * Description: Sanity check if addfb ioctl works correctly for given combination
+ * of Linear modifier with %arg[1]-bpp & %arg[2]-rotation
+ * Functionality: big_fbs, kms_gem_interop, rotation
+ *
* arg[1].values: 8, 16, 32, 64
* arg[2].values: 0, 90, 180, 270
*/
@@ -57,6 +62,11 @@
* of %arg[1] with %arg[2]-bpp & %arg[3]-rotation
* Functionality: big_fbs, kms_gem_interop, rotation, tiling
*
+ * SUBTEST: %s-%dbpp-rotate-%d-hflip
+ * Description: Sanity check if addfb ioctl works correctly for given combination
+ * of %arg[1] with %arg[2]-bpp & %arg[3]-rotation
+ * Functionality: big_fbs, kms_gem_interop, rotation, tiling
+*
* arg[1]:
*
* @4-tiled: TILE-4 modifier
@@ -66,6 +76,8 @@
*
* arg[2].values: 8, 16, 32, 64
* arg[3].values: 0, 90, 180, 270
+ *
+ * arg[4]:
*/
/**
@@ -958,23 +970,22 @@ static const struct {
{ DRM_FORMAT_XBGR16161616F, 64, },
};
-static const struct {
- igt_rotation_t rotation;
- uint16_t angle;
-} rotations[] = {
- { IGT_ROTATION_0, 0, },
- { IGT_ROTATION_90, 90, },
- { IGT_ROTATION_180, 180, },
- { IGT_ROTATION_270, 270, },
+static const igt_rotation_t rotations[] = {
+ IGT_ROTATION_0,
+ IGT_ROTATION_90,
+ IGT_ROTATION_180,
+ IGT_ROTATION_270,
+ IGT_ROTATION_0 | IGT_REFLECT_X,
+ IGT_ROTATION_90 | IGT_REFLECT_X,
+ IGT_ROTATION_180 | IGT_REFLECT_X,
+ IGT_ROTATION_270 | IGT_REFLECT_X,
};
-static const struct {
- igt_rotation_t flip;
- const char *flipname;
-} fliptab[] = {
- { 0, "" },
- { IGT_REFLECT_X, "-hflip" },
-};
+static const char *rotation_flip_str(igt_rotation_t rotation)
+{
+ return rotation & IGT_REFLECT_X ? "-hflip" : "";
+}
+
igt_main
{
igt_fixture {
@@ -1074,12 +1085,15 @@ igt_main
data.format = formats[j].format;
for (int k = 0; k < ARRAY_SIZE(rotations); k++) {
- data.rotation = rotations[k].rotation;
-
- igt_describe("Sanity check if addfb ioctl works correctly for given "
- "combination of modifier formats and rotation");
- igt_subtest_f("%s-%dbpp-rotate-%d", modifiers[i].name,
- formats[j].bpp, rotations[k].angle)
+ data.rotation = rotations[k];
+
+ igt_describe("Sanity check if scanout of big framebuffers works "
+ "correctly for given combination of modifier formats "
+ "and rotation");
+ igt_subtest_f("%s-%dbpp-rotate-%s%s", modifiers[i].name,
+ formats[j].bpp,
+ igt_plane_rotation_name(data.rotation),
+ rotation_flip_str(data.rotation))
test_scanout(&data);
}
@@ -1095,50 +1109,51 @@ igt_main
set_max_hw_stride(&data);
- for (int l = 0; l < ARRAY_SIZE(fliptab); l++) {
- for (int j = 0; j < ARRAY_SIZE(formats); j++) {
- /*
- * try only those formats which can show full length.
- * Here 32K is used to have CI test results consistent
- * for all platforms, 32K is smallest number possbily
- * coming to data.hw_stride from above set_max_hw_stride()
- */
- if (32768 / (formats[j].bpp >> 3) > 8192)
+ for (int j = 0; j < ARRAY_SIZE(formats); j++) {
+ /*
+ * try only those formats which can show full length.
+ * Here 32K is used to have CI test results consistent
+ * for all platforms, 32K is smallest number possbily
+ * coming to data.hw_stride from above set_max_hw_stride()
+ */
+ if (32768 / (formats[j].bpp >> 3) > 8192)
+ continue;
+
+ data.format = formats[j].format;
+
+ for (int k = 0; k < ARRAY_SIZE(rotations); k++) {
+ data.rotation = rotations[k];
+
+ // this combination will never happen.
+ if (igt_rotation_90_or_270(data.rotation) ||
+ (data.rotation & IGT_REFLECT_X &&
+ modifiers[i].modifier == DRM_FORMAT_MOD_LINEAR))
continue;
- data.format = formats[j].format;
-
- for (int k = 0; k < ARRAY_SIZE(rotations); k++) {
- data.rotation = rotations[k].rotation | fliptab[l].flip;
-
- // this combination will never happen.
- if (igt_rotation_90_or_270(data.rotation) ||
- (fliptab[l].flip == IGT_REFLECT_X && modifiers[i].modifier == DRM_FORMAT_MOD_LINEAR))
- continue;
-
- igt_describe("test maximum hardware supported stride length for given bpp and modifiers.");
- igt_subtest_f("%s-max-hw-stride-%dbpp-rotate-%d%s", modifiers[i].name,
- formats[j].bpp, rotations[k].angle, fliptab[l].flipname) {
- igt_require(intel_display_ver(intel_get_drm_devid(data.drm_fd)) >= 5);
- data.max_hw_fb_width = min(data.hw_stride / (formats[j].bpp >> 3), data.max_fb_width);
-
- test_scanout(&data);
- }
-
- data.async_flip_test = true;
- igt_describe("test async flip on maximum hardware supported stride length for given bpp and modifiers.");
- igt_subtest_f("%s-max-hw-stride-%dbpp-rotate-%d%s-async-flip",
- modifiers[i].name, formats[j].bpp,
- rotations[k].angle, fliptab[l].flipname) {
- igt_require(has_async_flip(&data));
- data.max_hw_fb_width = min(data.hw_stride / (formats[j].bpp >> 3), data.max_fb_width);
- test_scanout(&data);
- }
- data.async_flip_test = false;
-
- igt_fixture
- test_cleanup(&data);
+ igt_describe("test maximum hardware supported stride length for given bpp and modifiers.");
+ igt_subtest_f("%s-max-hw-stride-%dbpp-rotate-%s%s",
+ modifiers[i].name, formats[j].bpp,
+ igt_plane_rotation_name(data.rotation),
+ rotation_flip_str(data.rotation)) {
+ igt_require(intel_display_ver(intel_get_drm_devid(data.drm_fd)) >= 5);
+ data.max_hw_fb_width = min(data.hw_stride / (formats[j].bpp >> 3), data.max_fb_width);
+ test_scanout(&data);
}
+
+ data.async_flip_test = true;
+ igt_describe("test async flip on maximum hardware supported stride length for given bpp and modifiers.");
+ igt_subtest_f("%s-max-hw-stride-%dbpp-rotate-%s%s-async-flip",
+ modifiers[i].name, formats[j].bpp,
+ igt_plane_rotation_name(data.rotation),
+ rotation_flip_str(data.rotation)) {
+ igt_require(has_async_flip(&data));
+ data.max_hw_fb_width = min(data.hw_stride / (formats[j].bpp >> 3), data.max_fb_width);
+ test_scanout(&data);
+ }
+ data.async_flip_test = false;
+
+ igt_fixture
+ test_cleanup(&data);
}
}
}
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t 10/12] tests/kms_big_fb: Replace 'bpp' with 'name'
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
` (8 preceding siblings ...)
2023-12-20 17:59 ` [PATCH i-g-t 09/12] tests/kms_big_fb: Nuke fliptab[] Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 11/12] tests/kms_big_fb: Test planar YCbCr formats Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 12/12] tests/kms_big_fb: Also test some CCS modifiers Ville Syrjala
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Instead of storing the bpp in the formats[] array let's instead
store a string represtantion. I want to add planar YCbCr tests
to the mix, and so a simple bpp value will no logner cut it.
A bunch of stuff is using the numerical bpp value for some things,
but we can replace those with igt_drm_format_to_bpp().
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
tests/intel/kms_big_fb.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/tests/intel/kms_big_fb.c b/tests/intel/kms_big_fb.c
index 675afb54905d..80644ce94ab6 100644
--- a/tests/intel/kms_big_fb.c
+++ b/tests/intel/kms_big_fb.c
@@ -962,12 +962,12 @@ static const struct {
static const struct {
uint32_t format;
- uint8_t bpp;
+ const char *name;
} formats[] = {
- { DRM_FORMAT_C8, 8, },
- { DRM_FORMAT_RGB565, 16, },
- { DRM_FORMAT_XRGB8888, 32, },
- { DRM_FORMAT_XBGR16161616F, 64, },
+ { DRM_FORMAT_C8, "8bpp", },
+ { DRM_FORMAT_RGB565, "16bpp", },
+ { DRM_FORMAT_XRGB8888, "32bpp", },
+ { DRM_FORMAT_XBGR16161616F, "64bpp", },
};
static const igt_rotation_t rotations[] = {
@@ -1090,8 +1090,8 @@ igt_main
igt_describe("Sanity check if scanout of big framebuffers works "
"correctly for given combination of modifier formats "
"and rotation");
- igt_subtest_f("%s-%dbpp-rotate-%s%s", modifiers[i].name,
- formats[j].bpp,
+ igt_subtest_f("%s-%s-rotate-%s%s", modifiers[i].name,
+ formats[j].name,
igt_plane_rotation_name(data.rotation),
rotation_flip_str(data.rotation))
test_scanout(&data);
@@ -1110,13 +1110,15 @@ igt_main
set_max_hw_stride(&data);
for (int j = 0; j < ARRAY_SIZE(formats); j++) {
+ int bpp = igt_drm_format_to_bpp(formats[j].format);
+
/*
* try only those formats which can show full length.
* Here 32K is used to have CI test results consistent
* for all platforms, 32K is smallest number possbily
* coming to data.hw_stride from above set_max_hw_stride()
*/
- if (32768 / (formats[j].bpp >> 3) > 8192)
+ if (32768 / (bpp >> 3) > 8192)
continue;
data.format = formats[j].format;
@@ -1131,23 +1133,23 @@ igt_main
continue;
igt_describe("test maximum hardware supported stride length for given bpp and modifiers.");
- igt_subtest_f("%s-max-hw-stride-%dbpp-rotate-%s%s",
- modifiers[i].name, formats[j].bpp,
+ igt_subtest_f("%s-max-hw-stride-%s-rotate-%s%s",
+ modifiers[i].name, formats[j].name,
igt_plane_rotation_name(data.rotation),
rotation_flip_str(data.rotation)) {
igt_require(intel_display_ver(intel_get_drm_devid(data.drm_fd)) >= 5);
- data.max_hw_fb_width = min(data.hw_stride / (formats[j].bpp >> 3), data.max_fb_width);
+ data.max_hw_fb_width = min(data.hw_stride / (bpp >> 3), data.max_fb_width);
test_scanout(&data);
}
data.async_flip_test = true;
igt_describe("test async flip on maximum hardware supported stride length for given bpp and modifiers.");
- igt_subtest_f("%s-max-hw-stride-%dbpp-rotate-%s%s-async-flip",
- modifiers[i].name, formats[j].bpp,
+ igt_subtest_f("%s-max-hw-stride-%s-rotate-%s%s-async-flip",
+ modifiers[i].name, formats[j].name,
igt_plane_rotation_name(data.rotation),
rotation_flip_str(data.rotation)) {
igt_require(has_async_flip(&data));
- data.max_hw_fb_width = min(data.hw_stride / (formats[j].bpp >> 3), data.max_fb_width);
+ data.max_hw_fb_width = min(data.hw_stride / (bpp >> 3), data.max_fb_width);
test_scanout(&data);
}
data.async_flip_test = false;
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t 11/12] tests/kms_big_fb: Test planar YCbCr formats
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
` (9 preceding siblings ...)
2023-12-20 17:59 ` [PATCH i-g-t 10/12] tests/kms_big_fb: Replace 'bpp' with 'name' Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
2023-12-20 17:59 ` [PATCH i-g-t 12/12] tests/kms_big_fb: Also test some CCS modifiers Ville Syrjala
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Panning around inside planar YCbCr framebuffers is a bit
more involved that the RGB counterpart. It seems prudent
to test that we are correctly handling these case.
We'll manually trick rendercopy to also copy the chroma
plane for us. It might be nice to make rendercopy eventually
handle this entirely on its own, but for now we'll take this
slight shortcut.
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
tests/intel/kms_big_fb.c | 38 ++++++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/tests/intel/kms_big_fb.c b/tests/intel/kms_big_fb.c
index 80644ce94ab6..f8dd5a015ee1 100644
--- a/tests/intel/kms_big_fb.c
+++ b/tests/intel/kms_big_fb.c
@@ -42,27 +42,27 @@
#include "xe/xe_query.h"
/**
- * SUBTEST: linear-%dbpp-rotate-%d
+ * SUBTEST: linear-%s-rotate-%d
* Description: Sanity check if addfb ioctl works correctly for given combination
* of Linear modifier with %arg[1]-bpp & %arg[2]-rotation
* Functionality: big_fbs, kms_gem_interop, rotation
*
- * SUBTEST: linear-%dbpp-rotate-%d-hflip
+ * SUBTEST: linear-%s-rotate-%d-hflip
* Description: Sanity check if addfb ioctl works correctly for given combination
* of Linear modifier with %arg[1]-bpp & %arg[2]-rotation
* Functionality: big_fbs, kms_gem_interop, rotation
*
- * arg[1].values: 8, 16, 32, 64
+ * arg[1].values: 8bpp, 16bpp, 32bpp, 64bpp, nv12, p016
* arg[2].values: 0, 90, 180, 270
*/
/**
- * SUBTEST: %s-%dbpp-rotate-%d
+ * SUBTEST: %s-%s-rotate-%d
* Description: Sanity check if addfb ioctl works correctly for given combination
* of %arg[1] with %arg[2]-bpp & %arg[3]-rotation
* Functionality: big_fbs, kms_gem_interop, rotation, tiling
*
- * SUBTEST: %s-%dbpp-rotate-%d-hflip
+ * SUBTEST: %s-%s-rotate-%d-hflip
* Description: Sanity check if addfb ioctl works correctly for given combination
* of %arg[1] with %arg[2]-bpp & %arg[3]-rotation
* Functionality: big_fbs, kms_gem_interop, rotation, tiling
@@ -74,7 +74,7 @@
* @y-tiled: TILE-Y modifier
* @yf-tiled: TILE-YF modifier
*
- * arg[2].values: 8, 16, 32, 64
+ * arg[2].values: 8bpp, 16bpp, 32bpp, 64bpp, nv12, p016
* arg[3].values: 0, 90, 180, 270
*
* arg[4]:
@@ -237,6 +237,18 @@ static void copy_pattern(data_t *data,
*/
if (data->render_copy) {
data->render_copy(data->ibb, src, sx, sy, w, h, dst, dx, dy);
+
+ /* FIXME rendercopy should do this for us perhaps? */
+ if (igt_format_is_yuv_semiplanar(data->format)) {
+ igt_assert(!igt_fb_is_ccs_modifier(data->modifier));
+
+ src->bpp *= 2;
+ src->surface[0] = src->surface[1];
+ dst->bpp *= 2;
+ dst->surface[0] = dst->surface[1];
+
+ data->render_copy(data->ibb, src, sx/2, sy/2, w/2, h/2, dst, dx/2, dy/2);
+ }
} else {
w = min(w, src_fb->width - sx);
w = min(w, dst_fb->width - dx);
@@ -320,6 +332,10 @@ static void generate_pattern(data_t *data,
pat_fb.width, pat_fb.height);
w++;
h++;
+ if (igt_format_is_yuv_semiplanar(data->format)) {
+ w++;
+ h++;
+ }
}
}
@@ -478,8 +494,9 @@ static bool test_plane(data_t *data)
int y = coords[i].y;
/* Hardware limitation */
- if (data->format == DRM_FORMAT_RGB565 &&
- igt_rotation_90_or_270(data->rotation)) {
+ if ((data->format == DRM_FORMAT_RGB565 &&
+ igt_rotation_90_or_270(data->rotation)) ||
+ igt_format_is_yuv_semiplanar(data->format)) {
x &= ~1;
y &= ~1;
}
@@ -940,6 +957,9 @@ static bool has_async_flip(data_t *data)
* TODO: preferably probe all this stuff with
* TEST_ONLY rather than hardcoding it...
*/
+ if (igt_format_is_yuv_semiplanar(data->format))
+ return false;
+
if (intel_display_ver(data->devid) < 12 &&
data->modifier == DRM_FORMAT_MOD_LINEAR)
return false;
@@ -968,6 +988,8 @@ static const struct {
{ DRM_FORMAT_RGB565, "16bpp", },
{ DRM_FORMAT_XRGB8888, "32bpp", },
{ DRM_FORMAT_XBGR16161616F, "64bpp", },
+ { DRM_FORMAT_NV12, "nv12", },
+ { DRM_FORMAT_P016, "p016", },
};
static const igt_rotation_t rotations[] = {
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH i-g-t 12/12] tests/kms_big_fb: Also test some CCS modifiers
2023-12-20 17:59 [PATCH i-g-t 00/12] tests/kms_big_fb: Test planar formats, and CCS Ville Syrjala
` (10 preceding siblings ...)
2023-12-20 17:59 ` [PATCH i-g-t 11/12] tests/kms_big_fb: Test planar YCbCr formats Ville Syrjala
@ 2023-12-20 17:59 ` Ville Syrjala
11 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjala @ 2023-12-20 17:59 UTC (permalink / raw)
To: igt-dev
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Panning inside compressed framebuffers has similar challenges
as planar YCbCr framebuffers. So let's test this as well.
The difference is that i915 can't do GGTT remapping for CCS,
so we must limit the fb size to fit insie the hw limits.
Only enable this for the pre-TGL modifiers for now as testing
on TGL didn't go well. I suspect the problem lies with rendercopy
but until that is sorted let's not add tons of failures to CI.
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
tests/intel/kms_big_fb.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/tests/intel/kms_big_fb.c b/tests/intel/kms_big_fb.c
index f8dd5a015ee1..f7551fe0e873 100644
--- a/tests/intel/kms_big_fb.c
+++ b/tests/intel/kms_big_fb.c
@@ -73,6 +73,8 @@
* @x-tiled: TILE-X modifier
* @y-tiled: TILE-Y modifier
* @yf-tiled: TILE-YF modifier
+ * @y-tiled-ccs: TILE-Y+CCS modifier
+ * @yf-tiled-ccs: TILE-YF+CCS modifier
*
* arg[2].values: 8bpp, 16bpp, 32bpp, 64bpp, nv12, p016
* arg[3].values: 0, 90, 180, 270
@@ -110,6 +112,8 @@
* @x-tiled: TILE-X modifier
* @y-tiled: TILE-Y modifier
* @yf-tiled: TILE-YF modifier
+ * @y-tiled-ccs: TILE-Y+CCS modifier
+ * @yf-tiled-ccs: TILE-YF+CCS modifier
*
* arg[2].values: 32, 64
* arg[3].values: 0, 180
@@ -134,6 +138,8 @@
* @x-tiled: TILE-X modifier
* @y-tiled: TILE-Y modifier
* @yf-tiled: TILE-YF modifier
+ * @y-tiled-ccs: TILE-Y+CCS modifier
+ * @yf-tiled-ccs: TILE-YF+CCS modifier
*
* arg[2].values: 32, 64
* arg[3].values: 0, 180
@@ -171,6 +177,8 @@
* @x-tiled: TILE-X
* @y-tiled: TILE-Y
* @yf-tiled: TILE-YF
+ * @y-tiled-ccs: TILE-Y+CCS modifier
+ * @yf-tiled-ccs: TILE-YF+CCS modifier
*/
IGT_TEST_DESCRIPTION("Test big framebuffers");
@@ -371,7 +379,11 @@ static void max_fb_size(data_t *data, int *width, int *height,
struct igt_fb fb;
int i = 0;
- if (data->max_hw_stride_test) {
+ if (igt_fb_is_ccs_modifier(data->modifier)) {
+ /* FIXME figure out what's correct */
+ *width = 8192;
+ *height = 8192;
+ } else if (data->max_hw_stride_test) {
igt_output_t *output;
*width = data->max_hw_fb_width;
@@ -957,6 +969,9 @@ static bool has_async_flip(data_t *data)
* TODO: preferably probe all this stuff with
* TEST_ONLY rather than hardcoding it...
*/
+ if (igt_fb_is_ccs_modifier(data->modifier))
+ return false;
+
if (igt_format_is_yuv_semiplanar(data->format))
return false;
@@ -978,6 +993,8 @@ static const struct {
{ I915_FORMAT_MOD_Y_TILED, "y-tiled", },
{ I915_FORMAT_MOD_Yf_TILED, "yf-tiled", },
{ I915_FORMAT_MOD_4_TILED, "4-tiled", },
+ { I915_FORMAT_MOD_Y_TILED_CCS, "y-tiled-ccs", },
+ { I915_FORMAT_MOD_Yf_TILED_CCS, "yf-tiled-ccs", },
};
static const struct {
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread