[igt-dev] [PATCH i-g-t v8 2/2] tests/kms_ccs: CCS Clear Color test
Kahola, Mika
mika.kahola at intel.com
Thu Nov 12 08:24:27 UTC 2020
> -----Original Message-----
> From: Imre Deak <imre.deak at intel.com>
> Sent: Wednesday, November 11, 2020 6:38 PM
> To: Kahola, Mika <mika.kahola at intel.com>
> Cc: igt-dev at lists.freedesktop.org
> Subject: Re: [PATCH i-g-t v8 2/2] tests/kms_ccs: CCS Clear Color test
>
> On Wed, Nov 11, 2020 at 04:18:40PM +0200, Mika Kahola wrote:
> > The patch proposes a method to test CCS with clear color capability.
> >
> > The test paints a solid color on primary fb and a small sprite fb.
> > These are cleared with fast clear feature. A crc is captured and
> > compared against the reference.
> >
> > v2: Modify _gen9_render_copyfunc to support fast clear (Matt)
> > Enable fast clear bit on 3D sequence (Matt)
> > Add helper function to figure out clear color modifier (Matt)
> > v3: Remove unrelated line additions/removes
> > v4: Fast clear with color (Imre)
> > v5: Write raw 32-bit color values to register (Imre)
> > Require 32-bit color format
> > v6: Rebase to use batchbuffer without libdrm dependency
> > v7: Enable clear color (Nanley)
> > v8: Various cleanups (Imre)
>
> Too many changes in one patch, please split them to
>
> - lib/intel_aux_pgtable: Add support for creating pagetables for a single
> buffer
> - lib/rendercopy_gen9: Add support for fast clear
> - kms_ccs: Add RC-CC subtest
Ok, I will split the patches into smaller chunks. Thanks!
Cheers,
Mika
>
> >
> > Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> > ---
> > lib/gen8_render.h | 1 +
> > lib/gen9_render.h | 6 +-
> > lib/igt_fb.c | 20 ++++--
> > lib/igt_fb.h | 3 +
> > lib/intel_batchbuffer.c | 10 +++
> > lib/intel_batchbuffer.h | 6 ++
> > lib/rendercopy.h | 4 ++
> > lib/rendercopy_gen9.c | 146 ++++++++++++++++++++++++++++------------
> > tests/kms_ccs.c | 64 ++++++++++++++++--
> > 9 files changed, 203 insertions(+), 57 deletions(-)
> >
> > diff --git a/lib/gen8_render.h b/lib/gen8_render.h index
> > 31dc01bc..1b0f527e 100644
> > --- a/lib/gen8_render.h
> > +++ b/lib/gen8_render.h
> > @@ -26,6 +26,7 @@
> >
> > # define GEN8_VS_FLOATING_POINT_MODE_ALTERNATE (1 << 16)
> >
> > +#define GEN8_3DSTATE_FAST_CLEAR_ENABLE (1 << 8)
> > #define GEN8_3DSTATE_VIEWPORT_STATE_POINTERS_SF_CLIP \
> > GEN4_3D(3, 0, 0x21)
> > #define GEN8_3DSTATE_PS_BLEND GEN4_3D(3, 0, 0x4d)
> > diff --git a/lib/gen9_render.h b/lib/gen9_render.h index
> > 6274e902..cf5bd2d9 100644
> > --- a/lib/gen9_render.h
> > +++ b/lib/gen9_render.h
> > @@ -127,7 +127,11 @@ struct gen9_surface_state {
> > } ss9;
> >
> > struct {
> > - uint32_t aux_base_addr;
> > + uint32_t aux_base_addr:20;
> > + uint32_t procedual_texture:1;
> > + uint32_t clearvalue_addr_enable:1;
> > + uint32_t quilt_height:5;
> > + uint32_t quilt_width:5;
> > } ss10;
> >
> > struct {
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c index 43f8c475..422a9e06
> > 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -2141,9 +2141,10 @@ static int yuv_semiplanar_bpp(uint32_t
> drm_format)
> > }
> > }
> >
> > -static struct intel_buf *create_buf(struct fb_blit_upload *blit,
> > - const struct igt_fb *fb,
> > - const char *name)
> > +struct intel_buf *
> > +igt_fb_create_intel_buf(int fd, struct buf_ops *bops,
> > + const struct igt_fb *fb,
> > + const char *name)
> > {
> > struct intel_buf *buf;
> > uint32_t bo_name, handle, compression; @@ -2169,10 +2170,10
> @@
> > static struct intel_buf *create_buf(struct fb_blit_upload *blit,
> > compression = I915_COMPRESSION_NONE;
> > }
> >
> > - bo_name = gem_flink(blit->fd, fb->gem_handle);
> > - handle = gem_open(blit->fd, bo_name);
> > + bo_name = gem_flink(fd, fb->gem_handle);
> > + handle = gem_open(fd, bo_name);
> >
> > - buf = intel_buf_create_using_handle(blit->bops, handle,
> > + buf = intel_buf_create_using_handle(bops, handle,
> > fb->width, fb->height,
> > fb->plane_bpp[0], 0,
> > igt_fb_mod_to_tiling(fb->modifier),
> > @@ -2213,6 +2214,13 @@ static struct intel_buf *create_buf(struct
> fb_blit_upload *blit,
> > return buf;
> > }
> >
> > +static struct intel_buf *create_buf(struct fb_blit_upload *blit,
> > + const struct igt_fb *fb,
> > + const char *name)
> > +{
> > + return igt_fb_create_intel_buf(blit->fd, blit->bops, fb, name); }
> > +
> > static void fini_buf(struct intel_buf *buf) {
> > intel_buf_destroy(buf);
> > diff --git a/lib/igt_fb.h b/lib/igt_fb.h index b36db965..bc5b8fa0
> > 100644
> > --- a/lib/igt_fb.h
> > +++ b/lib/igt_fb.h
> > @@ -39,6 +39,7 @@
> >
> > #include "igt_color_encoding.h"
> > #include "igt_debugfs.h"
> > +#include "intel_bufops.h"
> >
> > /*
> > * Internal format to denote a buffer compatible with pixman's @@
> > -129,6 +130,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
> > enum igt_color_range color_range,
> > struct igt_fb *fb, uint64_t bo_size,
> > unsigned bo_stride);
> > +struct intel_buf *igt_fb_create_intel_buf(int fd, struct buf_ops *bops,
> > + const struct igt_fb *fb, const char
> *name);
> > unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
> > uint64_t modifier, struct igt_fb *fb); unsigned int
> > igt_create_color_fb(int fd, int width, int height, diff --git
> > a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c index
> > fc73495c..905f69ff 100644
> > --- a/lib/intel_batchbuffer.c
> > +++ b/lib/intel_batchbuffer.c
> > @@ -1096,6 +1096,16 @@ igt_vebox_copyfunc_t
> igt_get_vebox_copyfunc(int devid)
> > return copy;
> > }
> >
> > +igt_render_clearfunc_t igt_get_render_clearfunc(int devid) {
> > + igt_render_clearfunc_t clear = NULL;
>
> No need for a variable.
>
> > +
> > + if (IS_GEN12(devid))
> > + clear = gen12_render_clearfunc;
> > +
> > + return clear;
> > +}
> > +
> > /**
> > * igt_get_media_fillfunc:
> > * @devid: pci device id
> > diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h index
> > ab1b0c28..5d996ddf 100644
> > --- a/lib/intel_batchbuffer.h
> > +++ b/lib/intel_batchbuffer.h
> > @@ -374,6 +374,12 @@ typedef void (*igt_vebox_copyfunc_t)(struct
> > intel_bb *ibb,
> >
> > igt_vebox_copyfunc_t igt_get_vebox_copyfunc(int devid);
> >
> > +typedef void (*igt_render_clearfunc_t)(struct intel_bb *ibb,
> > + struct intel_buf *dst, unsigned int dst_x,
> unsigned int dst_y,
> > + unsigned int width, unsigned int height,
> > + float cc_color[4]);
> > +igt_render_clearfunc_t igt_get_render_clearfunc(int devid);
> > +
> > /**
> > * igt_fillfunc_t:
> > * @i915: drm fd
> > diff --git a/lib/rendercopy.h b/lib/rendercopy.h index
> > 7d5f0802..dd2e1c43 100644
> > --- a/lib/rendercopy.h
> > +++ b/lib/rendercopy.h
> > @@ -23,6 +23,10 @@ static inline void emit_vertex_normalized(struct
> intel_bb *ibb,
> > intel_bb_out(ibb, u.ui);
> > }
> >
> > +void gen12_render_clearfunc(struct intel_bb *ibb,
> > + struct intel_buf *dst, unsigned int dst_x, unsigned
> int dst_y,
> > + unsigned int width, unsigned int height,
> > + float clear_color[4]);
> > void gen12_render_copyfunc(struct intel_bb *ibb,
> > struct intel_buf *src, uint32_t src_x, uint32_t src_y,
> > uint32_t width, uint32_t height, diff --git
> > a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c index
> > ef6855c9..73272085 100644
> > --- a/lib/rendercopy_gen9.c
> > +++ b/lib/rendercopy_gen9.c
> > @@ -188,20 +188,12 @@ gen8_bind_buf(struct intel_bb *ibb, const struct
> intel_buf *buf, int is_dst) {
> > buf->ccs[0].offset,
> > intel_bb_offset(ibb)
> + 4 * 10,
> > buf->addr.offset);
> > - ss->ss10.aux_base_addr = (address + buf->ccs[0].offset);
> > - ss->ss11.aux_base_addr_hi = (address + buf->ccs[0].offset)
> >> 32;
> > - }
> > -
> > - if (buf->cc.offset) {
> > - igt_assert(buf->compression ==
> I915_COMPRESSION_RENDER);
> > + if (buf->cc.offset) {
> > + ss->ss10.aux_base_addr = (address + buf-
> >ccs[0].offset);
>
> aux needs to get setup for the !buf->cc.offset case as well.
>
> > + ss->ss10.clearvalue_addr_enable = 1;
>
> This will also need to get or'd into the aux addr relocation delta.
>
> > + ss->ss11.aux_base_addr_hi = (address + buf-
> >ccs[0].offset) >> 32;
> >
> > - address = intel_bb_offset_reloc_with_delta(ibb, buf->handle,
> > - read_domain,
> write_domain,
> > - buf->cc.offset,
> > - intel_bb_offset(ibb)
> + 4 * 12,
> > - buf->addr.offset);
> > - ss->ss12.clear_address = address + buf->cc.offset;
> > - ss->ss13.clear_address_hi = (address + buf->cc.offset) >> 32;
>
> The above shouldn't be removed.
>
> > + }
> > }
> >
> > return intel_bb_ptr_add_return_prev_offset(ibb, sizeof(*ss)); @@
> > -218,7 +210,9 @@ gen8_bind_surfaces(struct intel_bb *ibb,
> > binding_table_offset = intel_bb_ptr_add_return_prev_offset(ibb,
> 32);
> >
> > binding_table[0] = gen8_bind_buf(ibb, dst, 1);
> > - binding_table[1] = gen8_bind_buf(ibb, src, 0);
> > +
> > + if (src != NULL)
> > + binding_table[1] = gen8_bind_buf(ibb, src, 1);
> >
> > return binding_table_offset;
> > }
> > @@ -274,16 +268,39 @@ gen7_fill_vertex_buffer_data(struct intel_bb
> *ibb,
> > offset = intel_bb_offset(ibb);
> >
> > emit_vertex_2s(ibb, dst_x + width, dst_y + height);
>
> Missing scaling for the fast clear case.
>
> > - emit_vertex_normalized(ibb, src_x + width, intel_buf_width(src));
> > - emit_vertex_normalized(ibb, src_y + height, intel_buf_height(src));
> > +
> > + if (src == NULL) {
> > + dst_x /= 64;
> > + dst_y /= 16;
> > + }
>
> Correcty you need to scale the whole sum of dst_y + height for instance, also
> rounding up when needed. Probably better to emit all the vertices in one if
> branch for the copy case and all the scaled vertices and 0 place holders for
> the fast clear case in the else branch. Please also add a comment that src ==
> NULL is a fast clear.
>
> > +
> > + if (src != NULL) {
> > + emit_vertex_normalized(ibb, src_x + width,
> intel_buf_width(src));
> > + emit_vertex_normalized(ibb, src_y + height,
> intel_buf_height(src));
> > + } else {
> > + emit_vertex_normalized(ibb, 0, 0);
>
> Just emit_vertex(ibb, 0);
>
> > + emit_vertex_normalized(ibb, 0, 0);
> > + }
> >
> > emit_vertex_2s(ibb, dst_x, dst_y + height);
> > - emit_vertex_normalized(ibb, src_x, intel_buf_width(src));
> > - emit_vertex_normalized(ibb, src_y + height, intel_buf_height(src));
> > +
> > + if (src != NULL) {
> > + emit_vertex_normalized(ibb, src_x, intel_buf_width(src));
> > + emit_vertex_normalized(ibb, src_y + height,
> intel_buf_height(src));
> > + } else {
> > + emit_vertex_normalized(ibb, 0, 0);
> > + emit_vertex_normalized(ibb, 0, 0);
> > + }
> >
> > emit_vertex_2s(ibb, dst_x, dst_y);
> > - emit_vertex_normalized(ibb, src_x, intel_buf_width(src));
> > - emit_vertex_normalized(ibb, src_y, intel_buf_height(src));
> > +
> > + if (src != NULL) {
> > + emit_vertex_normalized(ibb, src_x, intel_buf_width(src));
> > + emit_vertex_normalized(ibb, src_y, intel_buf_height(src));
> > + } else {
> > + emit_vertex_normalized(ibb, 0, 0);
> > + emit_vertex_normalized(ibb, 0, 0);
> > + }
> >
> > return offset;
> > }
> > @@ -729,7 +746,7 @@ gen8_emit_sf(struct intel_bb *ibb) }
> >
> > static void
> > -gen8_emit_ps(struct intel_bb *ibb, uint32_t kernel) {
> > +gen8_emit_ps(struct intel_bb *ibb, uint32_t kernel, bool fast_clear)
> > +{
> > const int max_threads = 63;
> >
> > intel_bb_out(ibb, GEN6_3DSTATE_WM | (2 - 2)); @@ -753,10
> +770,16 @@
> > gen8_emit_ps(struct intel_bb *ibb, uint32_t kernel) {
> > intel_bb_out(ibb, GEN7_3DSTATE_PS | (12-2));
> > intel_bb_out(ibb, kernel);
> > intel_bb_out(ibb, 0); /* kernel hi */
> > - intel_bb_out(ibb, 1 <<
> GEN6_3DSTATE_WM_SAMPLER_COUNT_SHIFT |
> > - 2 <<
> GEN6_3DSTATE_WM_BINDING_TABLE_ENTRY_COUNT_SHIFT);
> > +
> > + if (fast_clear)
> > + intel_bb_out(ibb, 1)
>
> 1 <<
> GEN6_3DSTATE_WM_BINDING_TABLE_ENTRY_COUNT_SHIFT
>
> > + else
> > + intel_bb_out(ibb, 1 <<
> GEN6_3DSTATE_WM_SAMPLER_COUNT_SHIFT |
> > + 2 <<
> GEN6_3DSTATE_WM_BINDING_TABLE_ENTRY_COUNT_SHIFT);
> > +
> > intel_bb_out(ibb, 0); /* scratch space stuff */
> > intel_bb_out(ibb, 0); /* scratch hi */
> > +
>
> Extra w/s.
>
> > intel_bb_out(ibb, (max_threads - 1) <<
> GEN8_3DSTATE_PS_MAX_THREADS_SHIFT |
> > GEN6_3DSTATE_WM_16_DISPATCH_ENABLE);
> > intel_bb_out(ibb, 6 <<
> GEN6_3DSTATE_WM_DISPATCH_START_GRF_0_SHIFT);
> > @@ -765,6 +788,9 @@ gen8_emit_ps(struct intel_bb *ibb, uint32_t kernel)
> {
> > intel_bb_out(ibb, 0); // kernel 2
> > intel_bb_out(ibb, 0); /* kernel 2 hi */
> >
> > + if (fast_clear)
> > + intel_bb_out(ibb, GEN8_3DSTATE_FAST_CLEAR_ENABLE);
>
> Still in the wrong dword, should be or'd to the
> max_threads/wm_16_dispatch enabling value.
>
> > +
> > intel_bb_out(ibb, GEN8_3DSTATE_PS_BLEND | (2 - 2));
> > intel_bb_out(ibb, GEN8_PS_BLEND_HAS_WRITEABLE_RT);
> >
> > @@ -876,27 +902,31 @@ static void gen8_emit_primitive(struct intel_bb
> > *ibb, uint32_t offset) #define BATCH_STATE_SPLIT 2048
> >
> > static
> > -void _gen9_render_copyfunc(struct intel_bb *ibb,
> > - struct intel_buf *src,
> > - unsigned int src_x, unsigned int src_y,
> > - unsigned int width, unsigned int height,
> > - struct intel_buf *dst,
> > - unsigned int dst_x, unsigned int dst_y,
> > - struct intel_buf *aux_pgtable_buf,
> > - const uint32_t ps_kernel[][4],
> > - uint32_t ps_kernel_size)
> > +void _gen9_render_op(struct intel_bb *ibb,
> > + struct intel_buf *src,
> > + unsigned int src_x, unsigned int src_y,
> > + unsigned int width, unsigned int height,
> > + struct intel_buf *dst,
> > + unsigned int dst_x, unsigned int dst_y,
> > + struct intel_buf *aux_pgtable_buf,
> > + const uint32_t ps_kernel[][4],
> > + uint32_t ps_kernel_size)
> > {
> > uint32_t ps_sampler_state, ps_kernel_off, ps_binding_table;
> > uint32_t scissor_state;
> > uint32_t vertex_buffer;
> > uint32_t aux_pgtable_state;
> > + bool fast_clear = src != NULL;
>
> bool fast_clear = !src;
>
> >
> > - igt_assert(src->bpp == dst->bpp);
> > + if (src != NULL)
> > + igt_assert(src->bpp == dst->bpp);
> >
> > intel_bb_flush_render(ibb);
> >
> > intel_bb_add_intel_buf(ibb, dst, true);
> > - intel_bb_add_intel_buf(ibb, src, false);
> > +
> > + if (!fast_clear)
> > + intel_bb_add_intel_buf(ibb, src, false);
> >
> > intel_bb_ptr_set(ibb, BATCH_STATE_SPLIT);
> >
> > @@ -949,11 +979,13 @@ void _gen9_render_copyfunc(struct intel_bb
> *ibb,
> > intel_bb_out(ibb, 0);
> > intel_bb_out(ibb, 0);
> >
> > + gen8_emit_ps(ibb, ps_kernel_off, fast_clear);
>
> This isn't needed.
>
> > +
> > gen7_emit_clip(ibb);
> >
> > gen8_emit_sf(ibb);
> >
> > - gen8_emit_ps(ibb, ps_kernel_off);
> > + gen8_emit_ps(ibb, ps_kernel_off, fast_clear);
> >
> > intel_bb_out(ibb, GEN7_3DSTATE_BINDING_TABLE_POINTERS_PS);
> > intel_bb_out(ibb, ps_binding_table); @@ -991,9 +1023,9 @@ void
> > gen9_render_copyfunc(struct intel_bb *ibb,
> > unsigned int dst_x, unsigned int dst_y)
> >
> > {
> > - _gen9_render_copyfunc(ibb, src, src_x, src_y,
> > - width, height, dst, dst_x, dst_y, NULL,
> > - ps_kernel_gen9, sizeof(ps_kernel_gen9));
> > + _gen9_render_op(ibb, src, src_x, src_y,
> > + width, height, dst, dst_x, dst_y, NULL,
> > + ps_kernel_gen9, sizeof(ps_kernel_gen9));
> > }
> >
> > void gen11_render_copyfunc(struct intel_bb *ibb, @@ -1003,9 +1035,9
> > @@ void gen11_render_copyfunc(struct intel_bb *ibb,
> > struct intel_buf *dst,
> > unsigned int dst_x, unsigned int dst_y) {
> > - _gen9_render_copyfunc(ibb, src, src_x, src_y,
> > - width, height, dst, dst_x, dst_y, NULL,
> > - ps_kernel_gen11, sizeof(ps_kernel_gen11));
> > + _gen9_render_op(ibb, src, src_x, src_y,
> > + width, height, dst, dst_x, dst_y, NULL,
> > + ps_kernel_gen11, sizeof(ps_kernel_gen11));
> > }
> >
> > void gen12_render_copyfunc(struct intel_bb *ibb, @@ -1019,11 +1051,37
> > @@ void gen12_render_copyfunc(struct intel_bb *ibb,
> >
> > gen12_aux_pgtable_init(&pgtable_info, ibb, src, dst);
> >
> > - _gen9_render_copyfunc(ibb, src, src_x, src_y,
> > - width, height, dst, dst_x, dst_y,
> > - pgtable_info.pgtable_buf,
> > + _gen9_render_op(ibb, src, src_x, src_y,
> > + width, height, dst, dst_x, dst_y,
> > + pgtable_info.pgtable_buf,
> > gen12_render_copy,
> > sizeof(gen12_render_copy));
> >
> > gen12_aux_pgtable_cleanup(ibb, &pgtable_info); }
> > +
> > +void gen12_render_clearfunc(struct intel_bb *ibb,
> > + struct intel_buf *dst,
> > + unsigned int dst_x, unsigned int dst_y,
> > + unsigned int width, unsigned int height,
> > + float clear_color[4])
> > +{
> > + struct aux_pgtable_info pgtable_info = { };
> > +
> > + gen12_aux_pgtable_init(&pgtable_info, ibb, NULL, dst);
>
> Still missing the required change for this in gen12_aux_pgtable_init().
>
> > +
> > + /* BSpec 21136 */
> > + intel_bb_ptr_set(ibb, dst->cc.offset);
> > + intel_bb_out(ibb, clear_color[0]);
> > + intel_bb_out(ibb, clear_color[2]);
> > + intel_bb_out(ibb, clear_color[1]);
> > + intel_bb_out(ibb, clear_color[4]);
>
> This is still not ok, please check the previous review.
>
> > +
> > + _gen9_render_op(ibb, NULL, 0, 0,
> > + width, height, dst, dst_x, dst_y,
> > + pgtable_info.pgtable_buf,
> > + gen12_render_copy,
> > + sizeof(gen12_render_copy));
> > +
> > + gen12_aux_pgtable_cleanup(ibb, &pgtable_info); }
> > diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c index
> > 53abecce..d34bf428 100644
> > --- a/tests/kms_ccs.c
> > +++ b/tests/kms_ccs.c
> > @@ -120,6 +120,16 @@ static void addfb_init(struct igt_fb *fb, struct
> drm_mode_fb_cmd2 *f)
> > }
> > }
> >
> > +static bool is_ccs_cc_modifier(uint64_t modifier) {
> > + switch (modifier) {
> > + case LOCAL_I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > + return true;
> > + default:
> > + return false;
> > + }
>
> No need for a switch for this.
>
> > +}
> > +
> > /*
> > * The CCS planes of compressed framebuffers contain non-zero bytes if
> the
> > * engine compressed effectively the framebuffer. The actual encoding
> > of these @@ -134,6 +144,8 @@ static void check_ccs_plane(int drm_fd,
> igt_fb_t *fb, int plane)
> > void *ccs_p;
> > size_t ccs_size;
> > int i;
> > + uint32_t cc_map[4];
> > + uint32_t native_color;
>
> We don't want to check these for every plane, so needs a
> check_ccs_cc_plane() func and called from check_all_ccs_planes() only if
> plane == igt_fb_is_gen12_ccs_cc_plane() .
>
>
> >
> > ccs_size = fb->strides[plane] * fb->plane_height[plane];
> > igt_assert(ccs_size);
> > @@ -148,6 +160,17 @@ static void check_ccs_plane(int drm_fd, igt_fb_t
> *fb, int plane)
> > if (*(uint32_t *)(ccs_p + i))
> > break;
> >
> > + memcpy(cc_map, map + fb->offsets[2], 4*sizeof(uint32_t));
> > + igt_assert(colors->r == cc_map[0] &&
> > + colors->g == cc_map[1] &&
> > + colors->b == cc_map[2]);
>
> This will do an incorrect type conversion, need to compare float vs.
> float. You can use for instance a float/uint32_t union map for both the above
> and the native_color check.
>
> Using the global colors ptr here is too arbitrary, it needs to get passed in a
> proper func param.
>
> > +
> > + native_color = (uint8_t)(colors->r * 0xff) << 16 |
> > + (uint8_t)(colors->g * 0xff) << 8 |
> > + (uint8_t)(colors->b * 0xff);
> > +
> > + igt_assert(native_color == cc_map[3]);
>
> It's cc_map[4] that contains the required value.
>
> > +
> > munmap(map, fb->size);
> >
> > igt_assert_f(i < ccs_size,
> > @@ -160,8 +183,7 @@ static void check_all_ccs_planes(int drm_fd,
> igt_fb_t *fb)
> > int i;
> >
> > for (i = 0; i < fb->num_planes; i++) {
> > - if (igt_fb_is_ccs_plane(fb, i) &&
> > - !igt_fb_is_gen12_ccs_cc_plane(fb, i))
> > + if (igt_fb_is_ccs_plane(fb, i))
> > check_ccs_plane(drm_fd, fb, i);
> > }
> > }
> > @@ -176,6 +198,11 @@ static int get_ccs_plane_index(uint32_t format)
> > return index;
> > }
> >
> > +static struct intel_buf *fast_clear_fb(int drm_fd, struct buf_ops
> > +*bops, struct igt_fb *fb, const char *name) {
> > + return igt_fb_create_intel_buf(drm_fd, bops, fb, name); }
> > +
> > static void generate_fb(data_t *data, struct igt_fb *fb,
> > int width, int height,
> > enum test_fb_flags fb_flags)
> > @@ -246,10 +273,31 @@ static void generate_fb(data_t *data, struct
> igt_fb *fb,
> > if (!(data->flags & TEST_BAD_PIXEL_FORMAT)) {
> > int c = !!data->plane;
> >
> > - cr = igt_get_cairo_ctx(data->drm_fd, fb);
> > - igt_paint_color(cr, 0, 0, width, height,
> > - colors[c].r, colors[c].g, colors[c].b);
> > - igt_put_cairo_ctx(cr);
> > + if (is_ccs_cc_modifier(modifier)) {
> > + float cc_color[4] = {colors[0].r, colors[0].g,
> colors[0].b, 1.0};
> > + struct intel_bb *ibb = intel_bb_create(data->drm_fd,
> 4096);
> > + struct buf_ops *bops = buf_ops_create(data-
> >drm_fd);
> > + struct intel_buf *dst = fast_clear_fb(data->drm_fd,
> bops, fb,
> > +"fast clear dst");
> > +
> > + gem_set_domain(data->drm_fd, fb->gem_handle,
> > + I915_GEM_DOMAIN_GTT,
> I915_GEM_DOMAIN_GTT);
> > +
> > + /*
> > + * We expect the kernel to limit the max fb
> > + * size/stride to something that can still
> > + * rendered with the blitter/render engine.
> > + */
>
> The above is a left-over comment.
>
> > + gen12_render_clearfunc(ibb, dst, 0, 0,
> > + fb->width,
> > + fb->height,
> > + cc_color);
>
> igt_render_clearfunc_t fast_clear = igt_get_render_clearfunc();
> fast_clear(...)
>
> > + intel_bb_reset(ibb, true);
>
> Instead of reset, we need an intel_bb_sync()/intel_bb_destroy(). Also
> missing cleanup for bops and dst.
>
> Please make all the above setup/cleanup be part of fast_clear_fb() as well.
>
> > + } else {
> > + cr = igt_get_cairo_ctx(data->drm_fd, fb);
> > + igt_paint_color(cr, 0, 0, width, height,
> > + colors[c].r, colors[c].g, colors[c].b);
> > + igt_put_cairo_ctx(cr);
>
> Wrong indent.
>
> > + }
> > }
> >
> > ret = drmIoctl(data->drm_fd, LOCAL_DRM_IOCTL_MODE_ADDFB2,
> &f); @@
> > -349,6 +397,10 @@ static bool try_config(data_t *data, enum test_fb_flags
> fb_flags,
> > if (data->flags & TEST_BAD_ROTATION_90)
> > igt_plane_set_rotation(primary, IGT_ROTATION_90);
> >
> > + if (!is_ccs_cc_modifier(data->ccs_modifier)
>
> if (is_ccs_cc() && format != XRGB8888)
>
> > + && data->format != DRM_FORMAT_XRGB8888)
> > + return false;
> > +
> > ret = igt_display_try_commit2(display, commit);
> > if (data->flags & TEST_BAD_ROTATION_90) {
> > igt_assert_eq(ret, -EINVAL);
> > --
> > 2.25.1
> >
More information about the igt-dev
mailing list