Re: [igt-dev] [PATCH i-g-t] tests/kms_flip_scaled_crc: add flip to downscaled subtests

["Shankar, Uma" <uma.shankar@intel.com>]

> -----Original Message-----
> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Shankar,
> Uma
> Sent: Tuesday, September 15, 2020 5:39 PM
> To: juhapekka.heikkila@gmail.com; igt-dev@lists.freedesktop.org
> Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_flip_scaled_crc: add flip to
> downscaled subtests
> 
> 
> 
> > -----Original Message-----
> > From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > Sent: Monday, September 14, 2020 5:29 PM
> > To: Shankar, Uma <uma.shankar@intel.com>;
> > igt-dev@lists.freedesktop.org
> > Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_flip_scaled_crc: add
> > flip to downscaled subtests
> >
> > On 4.9.2020 17.31, Shankar, Uma wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of
> > >> Juha-Pekka Heikkila
> > >> Sent: Wednesday, September 2, 2020 2:42 PM
> > >> To: igt-dev@lists.freedesktop.org
> > >> Subject: [igt-dev] [PATCH i-g-t] tests/kms_flip_scaled_crc: add
> > >> flip to downscaled subtests
> > >
> > > I think this would sound better "Add Subtests for Flip with fb downscaling"
> > >
> > >>
> > >> attempt to cause cdclk changes and stress scalers with flipping to
> > >> different size fb with different modifiers. Verify results with crc.
> > >
> > > This is failing on some platforms like in GLK:
> > > https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4945/shard-glk9/igt@k
> > > ms _flip_scaled_crc@flip-32bpp-ytile-to-32bpp-ytileccs.html
> > >
> > > Can you check why it could be happening, I can see you have the
> > > modifier
> > check. But still its rejecting the combination in driver.
> > > May be something to fix.
> >
> > This I suspect is something problematic on i915 side, what is returned
> > there is - EINVAL from flip. Those GLK dmesgs generally look somehow
> > really messy, it is for some reason all the time trying to do
> > something with pipe b even when it's supposing to be disabled.
> 
> Ok, we may have to ensure that this doesn't fail else enabling it in CI will crib.
> May be create a separate Bug for this and get that tracked separately.

With this planned to be tracked separately for GLK with a JIRA Bug, this can be
merged. Please raise a JIRA to track that. With that and other minor nit comments
fixed, this is

Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> > >
> > > Overall this is really a good plan to have this combination tested,
> > > something
> > we have lacked.
> >
> > There was regression on drm-tip on these which was just recently fixed
> > hence this test. If for example would have run some videoplayer which
> > uses hw downscaling result would've been just black screen ;)
> > >
> > >> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > >> ---
> > >>   tests/Makefile.sources      |   1 +
> > >>   tests/kms_flip_scaled_crc.c | 255
> ++++++++++++++++++++++++++++++++++++
> > >>   tests/meson.build           |   1 +
> > >>   3 files changed, 257 insertions(+)
> > >>   create mode 100644 tests/kms_flip_scaled_crc.c
> > >>
> > >> diff --git a/tests/Makefile.sources b/tests/Makefile.sources index
> > >> 269b506d..fe9fb7ed 100644
> > >> --- a/tests/Makefile.sources
> > >> +++ b/tests/Makefile.sources
> > >> @@ -53,6 +53,7 @@ TESTS_progs = \
> > >>   	kms_fence_pin_leak \
> > >>   	kms_flip \
> > >>   	kms_flip_event_leak \
> > >> +	kms_flip_scaled_crc \
> > >>   	kms_flip_tiling \
> > >>   	kms_force_connector_basic \
> > >>   	kms_frontbuffer_tracking \
> > >> diff --git a/tests/kms_flip_scaled_crc.c
> > >> b/tests/kms_flip_scaled_crc.c new file mode 100644 index
> > >> 00000000..9daa2f92
> > >> --- /dev/null
> > >> +++ b/tests/kms_flip_scaled_crc.c
> > >> @@ -0,0 +1,255 @@
> > >> +/*
> > >> + * Copyright © 2020 Intel Corporation
> > >> + *
> > >> + * Permission is hereby granted, free of charge, to any person
> > >> +obtaining a
> > >> + * copy of this software and associated documentation files (the
> > >> +"Software"),
> > >> + * to deal in the Software without restriction, including without
> > >> +limitation
> > >> + * the rights to use, copy, modify, merge, publish, distribute,
> > >> +sublicense,
> > >> + * and/or sell copies of the Software, and to permit persons to
> > >> +whom the
> > >> + * Software is furnished to do so, subject to the following conditions:
> > >> + *
> > >> + * The above copyright notice and this permission notice
> > >> +(including the next
> > >> + * paragraph) shall be included in all copies or substantial
> > >> +portions of the
> > >> + * Software.
> > >> + *
> > >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > >> +EXPRESS OR
> > >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > >> +MERCHANTABILITY,
> > >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> > EVENT
> > >> +SHALL
> > >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES
> > >> OR
> > >> +OTHER
> > >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > >> +ARISING
> > >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > >> OTHER
> > >> +DEALINGS
> > >> + * IN THE SOFTWARE.
> > >> + *
> > >> + */
> > >> +
> > >> +#include "igt.h"
> > >> +
> > >> +IGT_TEST_DESCRIPTION("Test flipping between scaled/nonscaled
> > >> +framebuffers");
> > >> +
> > >> +typedef struct {
> > >> +	int drm_fd;
> > >> +	igt_display_t display;
> > >> +	igt_output_t *output;
> > >> +	enum pipe pipe;
> > >> +	uint32_t gen;
> > >> +	struct igt_fb small_fb;
> > >> +	struct igt_fb big_fb;
> > >> +	igt_pipe_crc_t *pipe_crc;
> > >> +} data_t;
> > >> +
> > >> +const struct {
> > >> +	const char * const name;
> > >> +	const char * const describe;
> > >> +	const uint64_t firstmodifier;
> > >> +	const uint32_t firstformat;
> > >> +	const uint64_t secondmodifier;
> > >> +	const uint32_t secondformat;
> > >> +} flip_scenario_test[] = {
> > >> +	{
> > >> +		"flip-32bpp-ytile-to-64bpp-ytile",
> > >> +		"Try to flip from 32bpp non scaled fb to 64bpp downscaled fb in
> > >> attempt to stress cdclk reprogramming.",
> > >
> > > We can trim this description, Like: "Flip from 32bpp non scaled fb
> > > to 64bpp
> > downscaled fb to stress CD clock programming"
> > > Trim this for all combinations.
> > >
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED, DRM_FORMAT_XRGB8888,
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED,
> > >> DRM_FORMAT_XRGB16161616F
> > >> +	},
> > >> +	{
> > >> +		"flip-64bpp-ytile-to-32bpp-ytile",
> > >> +		"Try to flip from 64bpp non scaled fb to 32bpp downscaled fb.",
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED,
> > >> DRM_FORMAT_XRGB16161616F,
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED, DRM_FORMAT_XRGB8888
> > >> +	},
> > >> +	{
> > >> +		"flip-64bpp-ytile-to-16bpp-ytile",
> > >> +		"Try to flip from 64bpp non scaled fb to 16bpp downscaled fb.",
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED,
> > >> DRM_FORMAT_XRGB16161616F,
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED, DRM_FORMAT_RGB565
> > >> +	},
> > >> +	{
> > >> +		"flip-32bpp-ytileccs-to-64bpp-ytile",
> > >> +		"Try to flip from 32bpp ccs non scaled fb to 64bpp downscaled fb
> > >> ytile.",
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED_CCS,
> > >> DRM_FORMAT_XRGB8888,
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED,
> > >> DRM_FORMAT_XRGB16161616F
> > >> +	},
> > >> +	{
> > >> +		"flip-32bpp-ytile-to-32bpp-ytilegen12rcccs",
> > >> +		"Try to flip from 32bpp non scaled fb to 32bpp downscaled gen12
> > >> rc ccs fb in attempt to stress cdclk reprogramming.",
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED, DRM_FORMAT_XRGB8888,
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > >> DRM_FORMAT_XRGB8888
> > >> +	},
> > >> +	{
> > >> +		"flip-32bpp-ytile-to-32bpp-ytileccs",
> > >> +		"Try to flip from 32bpp non scaled ytiled fb to 32bpp downscaled
> > >> ytiled ccs fb.",
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED, DRM_FORMAT_XRGB8888,
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED_CCS,
> > >> DRM_FORMAT_XRGB8888
> > >> +	},
> > >> +	{
> > >> +		"flip-64bpp-ytile-to-32bpp-ytilercccs",
> > >> +		"Try to flip from 64bpp non scaled ytiled fb to 32bpp downscaled
> > >> gen12 rc ccs fb.",
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED,
> > >> DRM_FORMAT_XRGB16161616F,
> > >> +		LOCAL_I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > >> DRM_FORMAT_XRGB8888
> > >> +	},
> > >> +};
> > >> +
> > >> +static void setup_fb(data_t *data, struct igt_fb *newfb, uint32_t width,
> > >> +		     uint32_t height, uint64_t format, uint64_t modifier) {
> > >> +	struct drm_mode_fb_cmd2 f = {0};
> > >> +	cairo_t *cr;
> > >> +
> > >> +	igt_require(igt_display_has_format_mod(&data->display, format,
> > >> +					       modifier));
> > >> +
> > >> +	igt_create_bo_for_fb(data->drm_fd, width, height, format, modifier,
> > >> +			     newfb);
> > >> +	igt_assert(newfb->gem_handle > 0);
> > >> +
> > >> +	f.width = newfb->width;
> > >> +	f.height = newfb->height;
> > >> +	f.pixel_format = newfb->drm_format;
> > >> +	f.flags = LOCAL_DRM_MODE_FB_MODIFIERS;
> > >> +
> > >> +	for (int n = 0; n < newfb->num_planes; n++) {
> > >> +		f.handles[n] = newfb->gem_handle;
> > >> +		f.modifier[n] = newfb->modifier;
> > >> +		f.pitches[n] = newfb->strides[n];
> > >> +		f.offsets[n] = newfb->offsets[n];
> > >> +	}
> > >> +
> > >> +	cr = igt_get_cairo_ctx(data->drm_fd, newfb);
> > >> +	igt_paint_color(cr, 0, 0, newfb->width, newfb->height, 0, 1, 0);
> > >> +	igt_put_cairo_ctx(cr);
> > >> +
> > >> +	igt_assert(drmIoctl(data->drm_fd, LOCAL_DRM_IOCTL_MODE_ADDFB2,
> > >> &f) == 0);
> > >> +	newfb->fb_id = f.fb_id;
> > >> +}
> > >> +
> > >> +static void free_fbs(data_t *data) {
> > >> +	igt_remove_fb(data->drm_fd, &data->small_fb);
> > >> +	igt_remove_fb(data->drm_fd, &data->big_fb); }
> > >> +
> > >> +static void test_flip_to_scaled(data_t *data, uint32_t index, enum pipe
> pipe,
> > >> +				igt_output_t *output)
> > >> +{
> > >> +	igt_plane_t *primary;
> > >> +	igt_crc_t small_crc, big_crc;
> > >> +
> > >> +	drmModeModeInfo *mode;
> > >> +	struct drm_event_vblank ev;
> > >> +	int ret;
> > >> +
> > >> +	igt_display_reset(&data->display);
> > >> +
> > >> +	igt_debug("running on output %s pipe %s\n", output->name,
> > >> +		  kmstest_pipe_name(pipe));
> > >> +	if (data->big_fb.fb_id == 0) {
> > >> +		mode = igt_output_get_mode(output);
> > >> +
> > >> +		// big fb will be 4k unless running on older than ICL
> > >> +		if (data->gen < 11) {
> > >> +			setup_fb(data, &data->small_fb,
> > >> +				 min(mode->hdisplay, 640),
> > >> +				 min(mode->vdisplay, 480),
> > >> +				 flip_scenario_test[index].firstformat,
> > >> +				 flip_scenario_test[index].firstmodifier);
> > >> +
> > >> +			setup_fb(data, &data->big_fb,
> > >> +				 1280, 960,
> > >
> > > Not sure, but just something to check. Whether this downscaling will
> > > trigger a
> > cd clock change for all platforms.
> > > We should somehow check that as well if cd clock change really
> > > happened.  CD
> > clock steps are not uniform across platforms.
> > > Also if driver hardcodes a higher cd clock this test may pass
> > > without its
> > intended objective.
> >
> > I cannot enforce cdclk changes but what happen here with downscaling
> > is it always go 4:1 I figure there's minimum four times more samples
> > needed with filtering. As much as I verified those dmesgs from CI
> > there was always display clock reprogrammed on all platforms but it is
> > just what happen on today's drm-tip. If would run this on i915 from
> > year ago there would be no such change thus on description it just
> > says 'attempt to stress cdclk'.
> >
> > As a flipping test I think final intended objective is flipping
> > between framebuffers succeeded and it is verified with crc below.
> 
> Yeah agree, as long as this combination works we should be fine.
> 
> >
> > >
> > >> +				 flip_scenario_test[index].secondformat,
> > >> +				 flip_scenario_test[index].secondmodifier);
> > >> +			igt_debug("small fb %dx%d\n", data->small_fb.width,
> > >> +			          data->small_fb.height);
> > >> +			igt_debug("big fb %dx%d\n", data->big_fb.width,
> > >> +			          data->big_fb.height);
> > >> +		} else {
> > >> +			setup_fb(data, &data->small_fb,
> > >> +				 min(mode->hdisplay, 1920),
> > >> +				 min(mode->vdisplay, 1080),
> > >> +				 flip_scenario_test[index].firstformat,
> > >> +				 flip_scenario_test[index].firstmodifier);
> > >> +
> > >> +			setup_fb(data, &data->big_fb,
> > >> +				 3840, 2160,
> > >> +				 flip_scenario_test[index].secondformat,
> > >> +				 flip_scenario_test[index].secondmodifier);
> > >> +			igt_debug("small fb %dx%d\n", data->small_fb.width,
> > >> +			          data->small_fb.height);
> > >> +			igt_debug("big fb %dx%d\n", data->big_fb.width,
> > >> +			          data->big_fb.height);
> > >> +		}
> > >> +	}
> > >> +
> > >> +	igt_output_set_pipe(output, pipe);
> > >> +
> > >> +	primary = igt_output_get_plane_type(output,
> > >> DRM_PLANE_TYPE_PRIMARY);
> > >> +	igt_display_commit_atomic(&data->display,
> > >> DRM_MODE_ATOMIC_ALLOW_MODESET,
> > >> +				  NULL);
> > >> +	data->pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe,
> > >> +					  INTEL_PIPE_CRC_SOURCE_AUTO);
> > >> +	igt_pipe_crc_start(data->pipe_crc);
> > >> +
> > >> +	igt_plane_set_position(primary, 0, 0);
> > >> +	igt_plane_set_fb(primary, &data->small_fb);
> > >> +
> > >> +	igt_display_commit_atomic(&data->display,
> > >> +				  DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > >> +	igt_pipe_crc_get_current(data->drm_fd, data->pipe_crc,
> > >> +&small_crc);
> > >> +
> > >> +	igt_plane_set_fb(primary, &data->big_fb);
> > >> +	igt_plane_set_size(primary, data->small_fb.width,
> > >> +			   data->small_fb.height);
> > >> +	ret = igt_display_try_commit_atomic(&data->display,
> > >> +
> > >> DRM_MODE_ATOMIC_ALLOW_MODESET  |
> > >> +					    DRM_MODE_PAGE_FLIP_EVENT, NULL);
> > >> +
> > >> +	igt_require_f(ret != -ERANGE,
> > >> +		      "Platform scaling limits exceeded, skipping.");
> > >> +	igt_assert_eq(ret, 0);
> > >> +
> > >> +	igt_assert(read(data->drm_fd, &ev, sizeof(ev)) == sizeof(ev));
> > >> +
> > >> +	igt_pipe_crc_get_current(data->drm_fd, data->pipe_crc, &big_crc);
> > >> +	igt_assert(igt_check_crc_equal(&small_crc, &big_crc));
> > >
> > > With this downscaling, are we able to match crc. Scalar may do some
> > > rounding,
> > so not sure if this will
> > > match in all cases. Good to check this.
> >
> > We cannot really do crc checking with scalers but I have idea here
> > maximum solid green would stay maximum solid green regardless of the
> > scaler, all components for scaler are maximum solid green. With these
> > starting values if scaler couldn't produce maximum solid green I'd
> > call scaler broken.
> 
> Idea looks good, should be fine.
> 
> > >
> > >> +
> > >> +	igt_pipe_crc_stop(data->pipe_crc);
> > >> +	igt_pipe_crc_free(data->pipe_crc);
> > >> +	data->pipe_crc = NULL;
> > >> +}
> > >> +
> > >> +igt_main
> > >> +{
> > >> +	enum pipe pipe;
> > >> +	data_t data = {};
> > >> +	igt_output_t *output;
> > >> +
> > >> +	igt_fixture {
> > >> +		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> > >> +		data.gen = intel_gen(intel_get_drm_devid(data.drm_fd));
> > >> +		igt_require(data.gen >= 9);
> > >> +		igt_display_require(&data.display, data.drm_fd);
> > >> +		igt_require(data.display.is_atomic);
> > >> +		igt_require_pipe_crc(data.drm_fd);
> > >> +		kmstest_set_vt_graphics_mode();
> > >> +	}
> > >> +
> > >> +	for (int index = 0; index < ARRAY_SIZE(flip_scenario_test); index++) {
> > >> +		igt_describe(flip_scenario_test[index].describe);
> > >> +		igt_subtest(flip_scenario_test[index].name) {
> > >> +			for_each_pipe_with_single_output(&data.display, pipe,
> > >> +							 output)
> > >> +				test_flip_to_scaled(&data, index, pipe, output);
> > >> +
> > >> +			free_fbs(&data);
> > >> +		}
> > >> +	}
> > >> +	igt_fixture {
> > >> +		free_fbs(&data);
> > >> +		if (data.pipe_crc) {
> > >> +			igt_pipe_crc_stop(data.pipe_crc);
> > >> +			igt_pipe_crc_free(data.pipe_crc);
> > >> +			data.pipe_crc = NULL;
> > >> +		}
> > >> +		kmstest_set_vt_text_mode();
> > >> +		igt_display_fini(&data.display);
> > >> +	}
> > >> +}
> > >> diff --git a/tests/meson.build b/tests/meson.build index
> > >> 684de043..8e17a6ae
> > >> 100644
> > >> --- a/tests/meson.build
> > >> +++ b/tests/meson.build
> > >> @@ -37,6 +37,7 @@ test_progs = [
> > >>   	'kms_fence_pin_leak',
> > >>   	'kms_flip',
> > >>   	'kms_flip_event_leak',
> > >> +	'kms_flip_scaled_crc',
> > >>   	'kms_flip_tiling',
> > >>   	'kms_force_connector_basic',
> > >>   	'kms_frontbuffer_tracking',
> > >> --
> > >> 2.26.0
> > >>
> > >> _______________________________________________
> > >> igt-dev mailing list
> > >> igt-dev@lists.freedesktop.org
> > >> https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev