[igt-dev] [PATCH i-g-t 2/2] tests/kms_ccs: CCS Clear Color test
Kahola, Mika
mika.kahola at intel.com
Mon Jun 1 10:25:30 UTC 2020
Thanks for the great review. I'll try to add my comments below.
> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Thursday, May 28, 2020 1:35 AM
> To: Kahola, Mika <mika.kahola at intel.com>
> Cc: igt-dev at lists.freedesktop.org
> Subject: Re: [igt-dev] [PATCH i-g-t 2/2] tests/kms_ccs: CCS Clear Color test
>
> On Tue, May 19, 2020 at 12:48:03PM +0300, Mika Kahola wrote:
> > This is a RFC patch that proposes to test CCS 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.
> >
> > Clear Color testing is performed with Clear Color DRM format modifier
> > and with YUV format only.
> >
> > Any comments/proposals to improve the test are warmly welcomed.
> >
> > Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> > ---
> > lib/intel_batchbuffer.c | 10 +++
> > lib/intel_batchbuffer.h | 5 ++
> > lib/rendercopy.h | 3 +
> > lib/rendercopy_gen9.c | 131
> ++++++++++++++++++++++++++++++++++++++++
> > tests/kms_ccs.c | 56 +++++++++++++++--
> > 5 files changed, 199 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c index
> > f1a45b47..fb5b49a4 100644
> > --- a/lib/intel_batchbuffer.c
> > +++ b/lib/intel_batchbuffer.c
> > @@ -1090,6 +1090,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;
> > +
> > + if (IS_GEN12(devid))
> > + clear = gen12_render_clearfunc;
> > +
> > + return clear;
> > +}
>
> Any specific reason this is in intel_batchbuffer.c? I know similar functions like
> get_{render,vebox}_copyfunc and such are also in this file, but that seems a bit
> strange to me as well when the non-blt implementations are in separate files.
I had no specific reason. I just added these as render and vebox functions were defined here.
>
> > +
> > /**
> > * igt_get_media_fillfunc:
> > * @devid: pci device id
> > diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h index
> > 442f3a18..b378b97c 100644
> > --- a/lib/intel_batchbuffer.h
> > +++ b/lib/intel_batchbuffer.h
> > @@ -367,6 +367,11 @@ typedef void (*igt_vebox_copyfunc_t)(struct
> > intel_batchbuffer *batch,
> >
> > igt_vebox_copyfunc_t igt_get_vebox_copyfunc(int devid);
> >
> > +typedef void (*igt_render_clearfunc_t)(struct intel_batchbuffer *batch,
> > + const struct igt_buf *dst, unsigned int
> dst_x, unsigned int dst_y,
> > + unsigned int width, unsigned int height);
>
> Should there be a kerneldoc block above this like there is for the other platform
> operation function pointers?
Yep, some documentation is needed. I'll address this on next spin.
>
> I was expecting there to be a color value provided as a parameter here too; are
> we just always clearing to black rather than giving the test the option of what
> color to use? It seems like if we don't explicitly set the color then we won't have
> good confidence that the Clear Color functionality is actually working as
> expected.
This could be parameter. It is a better test that way. We don't need to stick with just one color, but feature could be tested with multiple colors.
>
> It's too bad we can't just re-use igt_fillfunc_t for this; it seems to be pretty much
> what we want except that it only supports 8-bit "color"
> values to fill/clear with.
>
> > +igt_render_clearfunc_t igt_get_render_clearfunc(int devid);
> > +
> > /**
> > * igt_fillfunc_t:
> > * @batch: batchbuffer object
> > diff --git a/lib/rendercopy.h b/lib/rendercopy.h index
> > e0577cac..8c14830c 100644
> > --- a/lib/rendercopy.h
> > +++ b/lib/rendercopy.h
> > @@ -23,6 +23,9 @@ static inline void emit_vertex_normalized(struct
> intel_batchbuffer *batch,
> > OUT_BATCH(u.ui);
> > }
> >
> > +void gen12_render_clearfunc(struct intel_batchbuffer *batch,
> > + const struct igt_buf *dst, unsigned int dst_x, unsigned
> int dst_y,
> > + unsigned int width, unsigned int height);
> > void gen12_render_copyfunc(struct intel_batchbuffer *batch,
> > drm_intel_context *context,
> > const struct igt_buf *src, unsigned src_x, unsigned
> src_y, diff
> > --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c index
> > 85ae4cab..fa985fec 100644
> > --- a/lib/rendercopy_gen9.c
> > +++ b/lib/rendercopy_gen9.c
> > @@ -1110,6 +1110,120 @@ void _gen9_render_copyfunc(struct
> intel_batchbuffer *batch,
> > intel_batchbuffer_reset(batch);
> > }
> >
> > +static
> > +void _gen12_render_clearfunc(struct intel_batchbuffer *batch,
> > + drm_intel_context *context,
> > + const struct igt_buf *dst, unsigned int dst_x,
> > + unsigned int dst_y, unsigned int width, unsigned int
> height,
> > + drm_intel_bo *aux_pgtable_bo,
> > + const uint32_t ps_kernel[][4],
> > + uint32_t ps_kernel_size)
> > +{
>
> The implementation of this function is nearly identical to
> _gen9_render_copyfunc; can we find a way to just re-use that function and
> make it smart enough to deal with a NULL src so that we don't have so much
> code duplication here? That will likely make it easier when we add new
> generations in the future and need to tweak the general state emission.
My starting point was a copy of render_copyfunc. As I'm not really sure what the sequence would be I started testing with this. I could modify _gen9_render_copyfunc so that I supports color clear as well and handle the NULL src's what comes with color clear.
>
> I'm not an expert on the 3d pipeline so I don't have a good feel for the state that
> we need to emit, but it doesn't look like we're binding the destination surface
> anywhere here (i.e., as we do in
> gen8_bind_surfaces()) --- is that expected/okay?
Well, this is something that I'm not sure of. I was hoping that someone could give me tips/pointers how to deal with this 3d pipeline.
>
> > + uint32_t ps_sampler_state, ps_kernel_off;
> > + uint32_t scissor_state;
> > + uint32_t vertex_buffer;
> > + uint32_t batch_end;
> > + uint32_t aux_pgtable_state;
> > +
> > + intel_batchbuffer_flush_with_context(batch, context);
> > +
> > + intel_batchbuffer_align(batch, 8);
> > +
> > + batch->ptr = &batch->buffer[BATCH_STATE_SPLIT];
> > +
> > + annotation_init(&aub_annotations);
> > +
> > + ps_sampler_state = gen8_create_sampler(batch);
> > + ps_kernel_off = gen8_fill_ps(batch, ps_kernel, ps_kernel_size);
> > + vertex_buffer = gen7_fill_vertex_buffer_data(batch, NULL,
> > + 0, 0,
> > + dst_x, dst_y,
> > + width, height);
> > + cc.cc_state = gen6_create_cc_state(batch);
> > + cc.blend_state = gen8_create_blend_state(batch);
> > + viewport.cc_state = gen6_create_cc_viewport(batch);
> > + viewport.sf_clip_state = gen7_create_sf_clip_viewport(batch);
> > + scissor_state = gen6_create_scissor_rect(batch);
> > +
> > + aux_pgtable_state = gen12_create_aux_pgtable_state(batch,
> > + aux_pgtable_bo);
> > +
> > + /* TODO: there is other state which isn't setup */
> > +
> > + assert(batch->ptr < &batch->buffer[4095]);
> > +
> > + batch->ptr = batch->buffer;
> > +
> > + /* Start emitting the commands. The order roughly follows the mesa
> blorp
> > + * order */
> > + OUT_BATCH(G4X_PIPELINE_SELECT | PIPELINE_SELECT_3D |
> > + GEN9_PIPELINE_SELECTION_MASK);
> > +
> > + gen12_emit_aux_pgtable_state(batch, aux_pgtable_state, true);
> > +
> > + gen8_emit_sip(batch);
> > +
> > + gen7_emit_push_constants(batch);
> > +
> > + gen9_emit_state_base_address(batch);
> > +
> > + OUT_BATCH(GEN7_3DSTATE_VIEWPORT_STATE_POINTERS_CC);
> > + OUT_BATCH(viewport.cc_state);
> > + OUT_BATCH(GEN8_3DSTATE_VIEWPORT_STATE_POINTERS_SF_CLIP);
> > + OUT_BATCH(viewport.sf_clip_state);
> > +
> > + gen7_emit_urb(batch);
> > +
> > + gen8_emit_cc(batch);
> > +
> > + gen8_emit_multisample(batch);
> > +
> > + gen8_emit_null_state(batch);
> > +
> > + OUT_BATCH(GEN7_3DSTATE_STREAMOUT | (5 - 2));
> > + OUT_BATCH(0);
> > + OUT_BATCH(0);
> > + OUT_BATCH(0);
> > + OUT_BATCH(0);
> > +
> > + gen7_emit_clip(batch);
> > +
> > + gen8_emit_sf(batch);
> > +
> > + gen8_emit_ps(batch, ps_kernel_off);
>
> Again, my 3d pipeline experience is pretty minimal so I may be way off base, but
> don't we want to trigger a fast clear here? If I understand bspec 46969 / 47704
> correctly, I think we need to set bit 8 in dword 5 of the 3DSTATE_PS instruction
> for that, which our usual pixel shader emission helper doesn't do.
Yes, we do want to trigger a fast clear here. I'll update this on next spin.
>
> > +
> > + OUT_BATCH(GEN7_3DSTATE_SAMPLER_STATE_POINTERS_PS);
> > + OUT_BATCH(ps_sampler_state);
> > +
> > + OUT_BATCH(GEN8_3DSTATE_SCISSOR_STATE_POINTERS);
> > + OUT_BATCH(scissor_state);
> > +
> > + gen9_emit_depth(batch);
> > +
> > + gen7_emit_clear(batch);
> > +
> > + gen6_emit_drawing_rectangle(batch, dst);
> > +
> > + gen7_emit_vertex_buffer(batch, vertex_buffer);
> > + gen6_emit_vertex_elements(batch);
> > +
> > + gen8_emit_vf_topology(batch);
> > + gen8_emit_primitive(batch, vertex_buffer);
> > +
> > + OUT_BATCH(MI_BATCH_BUFFER_END);
> > +
> > + batch_end = intel_batchbuffer_align(batch, 8);
> > + assert(batch_end < BATCH_STATE_SPLIT);
> > + annotation_add_batch(&aub_annotations, batch_end);
> > +
> > + dump_batch(batch);
> > +
> > + annotation_flush(&aub_annotations, batch);
> > +
> > + gen6_render_flush(batch, context, batch_end);
> > + intel_batchbuffer_reset(batch);
> > +}
> > +
> > void gen9_render_copyfunc(struct intel_batchbuffer *batch,
> > drm_intel_context *context,
> > const struct igt_buf *src, unsigned src_x, unsigned
> src_y, @@
> > -1153,3 +1267,20 @@ void gen12_render_copyfunc(struct
> > intel_batchbuffer *batch,
> >
> > gen12_aux_pgtable_cleanup(&pgtable_info);
> > }
> > +
> > +void gen12_render_clearfunc(struct intel_batchbuffer *batch,
> > + const struct igt_buf *dst, unsigned int dst_x, unsigned
> int dst_y,
> > + unsigned int width, unsigned int height) {
> > + struct aux_pgtable_info pgtable_info = { };
> > +
> > + gen12_aux_pgtable_init(&pgtable_info, batch->bufmgr, NULL, dst);
> > +
> > + _gen12_render_clearfunc(batch, NULL,
> > + dst, dst_x, dst_y, 0, 0,
> > + pgtable_info.pgtable_bo,
> > + gen12_render_copy,
> > + sizeof(gen12_render_copy));
>
> The rendercopy shader samples values from the source buffer and writes them
> to the destination; is that appropriate to use in this case?
> Should we be using a different/simpler shader when we're just trying to do a
> clear?
This has been discussed in the past that should we use different shader here. Should this shader update come from MESA side? I know, that I'm an expert on writing shader code.
>
> > +
> > + gen12_aux_pgtable_cleanup(&pgtable_info);
> > +}
> > diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c index
> > c23b4e44..c9a781bf 100644
> > --- a/tests/kms_ccs.c
> > +++ b/tests/kms_ccs.c
> > @@ -50,10 +50,12 @@ enum test_fb_flags {
> > FB_MISALIGN_AUX_STRIDE = 1 << 2,
> > FB_SMALL_AUX_STRIDE = 1 << 3,
> > FB_ZERO_AUX_STRIDE = 1 << 4,
> > + FB_CLEAR_COLOR = 1 << 5,
> > };
> >
> > typedef struct {
> > int drm_fd;
> > + int devid;
> > igt_display_t display;
> > igt_output_t *output;
> > enum pipe pipe;
> > @@ -62,6 +64,9 @@ typedef struct {
> > igt_pipe_crc_t *pipe_crc;
> > uint32_t format;
> > uint64_t ccs_modifier;
> > + struct igt_fb primary_fb;
> > + igt_plane_t *primary;
> > + igt_render_clearfunc_t fast_clear;
> > } data_t;
> >
> > static const struct {
> > @@ -290,6 +295,17 @@ static igt_plane_t *compatible_main_plane(data_t
> *data)
> > return igt_output_get_plane_type(data->output,
> > DRM_PLANE_TYPE_PRIMARY); }
> >
> > +static void scratch_buf_init(data_t *data, drm_intel_bo *drmibo,
> > + struct igt_buf *igtbo)
> > +{
> > + igtbo->bo = drmibo;
> > + igtbo->surface[0].stride = data->primary_fb.strides[0];
> > + igtbo->tiling = data->primary_fb.modifier;
> > + igtbo->surface[0].size = data->primary_fb.size;
> > + igtbo->bpp = data->primary_fb.plane_bpp[0]; }
> > +
> > +
> > static bool try_config(data_t *data, enum test_fb_flags fb_flags,
> > igt_crc_t *crc)
> > {
> > @@ -299,6 +315,10 @@ static bool try_config(data_t *data, enum
> test_fb_flags fb_flags,
> > int fb_width = drm_mode->hdisplay;
> > enum igt_commit_style commit;
> > struct igt_fb fb, fb_sprite;
> > + drm_intel_bufmgr *bufmgr = NULL;
> > + drm_intel_bo *drmibo;
> > + struct intel_batchbuffer *batch;
> > + struct igt_buf igtbo;
> > int ret;
> >
> > if (data->display.is_atomic)
> > @@ -333,7 +353,6 @@ static bool try_config(data_t *data, enum
> > test_fb_flags fb_flags,
> >
> > if (data->flags & TEST_FAIL_ON_ADDFB2)
> > return true;
> > -
> > igt_plane_set_position(primary, 0, 0);
> > igt_plane_set_size(primary, drm_mode->hdisplay, drm_mode-
> >vdisplay);
> > igt_plane_set_fb(primary, &fb);
> > @@ -349,6 +368,22 @@ 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 (fb_flags & FB_CLEAR_COLOR) {
> > + drmibo = gem_handle_to_libdrm_bo(bufmgr, data->drm_fd, "",
> > + data-
> >primary_fb.gem_handle);
> > + igt_assert(drmibo);
> > +
> > + scratch_buf_init(data, drmibo, &igtbo);
> > +
> > + batch = intel_batchbuffer_alloc(bufmgr, data->devid);
> > + igt_assert(batch);
> > +
> > + /* use fast clear */
> > + data->fast_clear(batch, &igtbo, 0, 0,
> > + data->primary_fb.width,
> > + data->primary_fb.height);
> > + }
> > +
> > ret = igt_display_try_commit2(display, commit);
> > if (data->flags & TEST_BAD_ROTATION_90) {
> > igt_assert_eq(ret, -EINVAL);
> > @@ -370,6 +405,7 @@ static bool try_config(data_t *data, enum
> > test_fb_flags fb_flags,
> >
> > igt_plane_set_fb(primary, NULL);
> > igt_plane_set_rotation(primary, IGT_ROTATION_0);
> > +
> > igt_display_commit2(display, commit);
> >
> > if (data->flags & TEST_CRC)
> > @@ -386,12 +422,18 @@ static int test_ccs(data_t *data)
> > if (data->flags & TEST_CRC) {
> > data->pipe_crc = igt_pipe_crc_new(data->drm_fd, data->pipe,
> > INTEL_PIPE_CRC_SOURCE_AUTO);
> >
> > - if (try_config(data, fb_flags | FB_COMPRESSED, &ref_crc) &&
> > - try_config(data, fb_flags, &crc)) {
> > + if (data->ccs_modifier ==
> > +LOCAL_I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC &&
>
> This should probably be separated out into a helper function since we'll probably
> have other "CCS_CC" modifiers on future platforms.
Yes. Just to be futureproof.
>
> > + data->format == DRM_FORMAT_YUYV) {
>
> It's not obvious to me why we test for YUYV here?
I just selected one format to test this feature. Maybe we should loop through all formats?
Thanks again for a review! I'll try to address these issues.
Cheers,
Mika
>
>
> Matt
>
> > + valid_tests += try_config(data, fb_flags |
> FB_COMPRESSED, &ref_crc);
> > + valid_tests += try_config(data, fb_flags |
> FB_COMPRESSED |
> > +FB_CLEAR_COLOR, &crc);
> > igt_assert_crc_equal(&crc, &ref_crc);
> > - valid_tests++;
> > + } else {
> > + if (try_config(data, fb_flags | FB_COMPRESSED,
> &ref_crc) &&
> > + try_config(data, fb_flags, &crc)) {
> > + igt_assert_crc_equal(&crc, &ref_crc);
> > + valid_tests++;
> > + }
> > }
> > -
> > igt_pipe_crc_free(data->pipe_crc);
> > data->pipe_crc = NULL;
> > }
> > @@ -471,11 +513,13 @@ igt_main_args("c", NULL, help_str, opt_handler,
> NULL)
> > igt_fixture {
> > data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> >
> > - igt_require(intel_gen(intel_get_drm_devid(data.drm_fd)) >= 9);
> > + data.devid = intel_gen(intel_get_drm_devid(data.drm_fd));
> > + igt_require(data.devid >= 9);
> > kmstest_set_vt_graphics_mode();
> > igt_require_pipe_crc(data.drm_fd);
> >
> > igt_display_require(&data.display, data.drm_fd);
> > + data.fast_clear = igt_get_render_clearfunc(data.devid);
> > }
> >
> > for_each_pipe_static(pipe) {
> > --
> > 2.20.1
> >
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
More information about the igt-dev
mailing list