[igt-dev] [PATCH i-g-t 2/2] Add a new IGT test to validate DC3CO state
Arkadiusz Hiler
arkadiusz.hiler at intel.com
Tue Oct 29 12:37:14 UTC 2019
On Tue, Oct 29, 2019 at 03:42:06PM +0530, Jeevan B wrote:
> Add a subtest for DC3CO video playback case
> to generate selective frame update and validate
> that system stays in DC3CO state during execution.
>
> v2: Changed PSR2 idle check to sleep check and addressed cosmetic changes.
> v3: Renamed a function and restructured code according to Anshuman’s comments.
> v4: Cosmetic changes.
> v5: Removed DC5 check, Platform check and a function parameter.
> Renamed a function name as per Arek and Imre's Comments.
> v6: Added a new function require_dc_counter as per Arek's Comments.
> The test is now running based on time instead of the number of frames
> increased frame delay from 1 to 1.5 as per Imre's Comments.
> v7: Removed psr2_active_sleep_check and corrected the switch indentation.
>
> Signed-off-by: Jeevan B <jeevan.b at intel.com>
> Reviewed-by: Anshuman Gupta <anshuman.gupta at intel.com>
> ---
> tests/i915/i915_pm_dc.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 151 insertions(+), 1 deletion(-)
>
> diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
> index 44c6538..46db135 100644
> --- a/tests/i915/i915_pm_dc.c
> +++ b/tests/i915/i915_pm_dc.c
> @@ -32,6 +32,7 @@
> #include "igt_psr.h"
> #include "igt_sysfs.h"
> #include "limits.h"
> +#include "time.h"
>
> /* DC State Flags */
> #define CHECK_DC5 (1 << 0)
> @@ -39,12 +40,16 @@
> #define CHECK_DC3CO (1 << 2)
>
> typedef struct {
> + double r, g, b;
> +} color_t;
> +
> +typedef struct {
> int drm_fd;
> int msr_fd;
> int debugfs_fd;
> uint32_t devid;
> igt_display_t display;
> - struct igt_fb fb_white;
> + struct igt_fb fb_white, fb_rgb, fb_rgr;
> enum psr_mode op_psr_mode;
> drmModeModeInfo *mode;
> igt_output_t *output;
> @@ -110,6 +115,42 @@ static void cleanup_dc_psr(data_t *data)
> igt_remove_fb(data->drm_fd, &data->fb_white);
> }
>
> +static void cleanup_dc3co_fbs(data_t *data)
> +{
> + igt_plane_t *primary;
> +
> + primary = igt_output_get_plane_type(data->output,
> + DRM_PLANE_TYPE_PRIMARY);
> + igt_plane_set_fb(primary, NULL);
> + /* Clear Frame Buffers */
> + igt_display_commit(&data->display);
> + igt_remove_fb(data->drm_fd, &data->fb_rgb);
> + igt_remove_fb(data->drm_fd, &data->fb_rgr);
> +}
> +
> +static void paint_rectangles(data_t *data,
> + drmModeModeInfo *mode,
> + color_t *colors,
> + igt_fb_t *fb)
> +{
> + cairo_t *cr = igt_get_cairo_ctx(data->drm_fd, fb);
> + int i, l = mode->hdisplay / 3;
> + int rows_remaining = mode->hdisplay % 3;
> +
> + /* Paint 3 solid rectangles. */
> + for (i = 0 ; i < 3; i++) {
> + igt_paint_color(cr, i * l, 0, l, mode->vdisplay,
> + colors[i].r, colors[i].g, colors[i].b);
> + }
> +
> + if (rows_remaining > 0)
> + igt_paint_color(cr, i * l, 0, rows_remaining, mode->vdisplay,
> + colors[i - 1].r, colors[i - 1].g,
> + colors[i - 1].b);
> +
> + igt_put_cairo_ctx(data->drm_fd, fb, cr);
> +}
> +
> static void setup_primary(data_t *data)
> {
> igt_plane_t *primary;
> @@ -127,6 +168,20 @@ static void setup_primary(data_t *data)
> igt_display_commit(&data->display);
> }
>
> +static void create_color_fb(data_t *data, igt_fb_t *fb, color_t *fb_color)
> +{
> + int fb_id;
> +
> + fb_id = igt_create_fb(data->drm_fd,
> + data->mode->hdisplay,
> + data->mode->vdisplay,
> + DRM_FORMAT_XRGB8888,
> + LOCAL_DRM_FORMAT_MOD_NONE,
> + fb);
> + igt_assert(fb_id);
> + paint_rectangles(data, data->mode, fb_color, fb);
> +}
> +
> static uint32_t get_dc_counter(char *dc_data)
> {
> char *e;
> @@ -183,6 +238,94 @@ static void check_dc_counter(int drm_fd, int dc_flag, uint32_t prev_dc_count)
> "%s state is not achieved\n", tmp);
> }
>
> +static void setup_videoplayback(data_t *data)
> +{
> + color_t red_green_blue[] = {
> + { 1.0, 0.0, 0.0 },
> + { 0.0, 1.0, 0.0 },
> + { 0.0, 0.0, 1.0 },
> + };
> + color_t red_green_red[] = {
> + { 1.0, 0.0, 0.0 },
> + { 0.0, 1.0, 0.0 },
> + { 1.0, 0.0, 0.0 },
> + };
> +
> + create_color_fb(data, &data->fb_rgb, red_green_blue);
> + create_color_fb(data, &data->fb_rgr, red_green_red);
> +}
> +
> +static void check_dc3co_with_videoplayback_like_load(data_t *data)
> +{
> + igt_plane_t *primary;
> + uint32_t dc3co_prev_cnt;
> + int delay;
> + time_t secs = 6;
> + time_t startTime = time(NULL);
> +
> + primary = igt_output_get_plane_type(data->output,
> + DRM_PLANE_TYPE_PRIMARY);
> + igt_plane_set_fb(primary, NULL);
> + dc3co_prev_cnt = read_dc_counter(data->drm_fd, CHECK_DC3CO);
> + /* Calculate delay to generate idle frame in usec*/
> + delay = 1.5 * ((1000 * 1000) / data->mode->vrefresh);
> +
> + while (time(NULL) - startTime < secs) {
> + igt_plane_set_fb(primary, &data->fb_rgb);
> + igt_display_commit(&data->display);
> + usleep(delay);
> +
> + igt_plane_set_fb(primary, &data->fb_rgr);
> + igt_display_commit(&data->display);
> + usleep(delay);
> + }
> +
> + check_dc_counter(data->drm_fd, CHECK_DC3CO, dc3co_prev_cnt);
> +}
> +
> +static void require_dc_counter(int drm_fd, int dc_flag)
Nice! Thanks for introducing this. Now we should use that for the tests
that are using other counters so they have the requirements up-front and
don't skip on reading.
As a general rule of thumb:
* skip only from functions that have *skip* or *require* in the name
* let *read* and other functions to fail
Something like:
diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
index 43af86d4..64d81440 100644
--- a/tests/i915/i915_pm_dc.c
+++ b/tests/i915/i915_pm_dc.c
@@ -205,13 +205,13 @@ static uint32_t read_dc_counter(uint32_t drm_fd, int dc_flag)
if (dc_flag & CHECK_DC5) {
str = strstr(buf, "DC3 -> DC5 count");
- igt_skip_on_f(!str, "DC5 counter is not available\n");
+ igt_assert_f(str, "DC5 counter is not available\n");
} else if (dc_flag & CHECK_DC6) {
str = strstr(buf, "DC5 -> DC6 count");
- igt_skip_on_f(!str, "DC6 counter is not available\n");
+ igt_assert_f(str, "DC6 counter is not available\n");
} else if (dc_flag & CHECK_DC3CO) {
str = strstr(buf, "DC3CO count");
- igt_skip_on_f(!str, "DC3CO counter is not available\n");
+ igt_assert_f(str, "DC3CO counter is not available\n");
}
return get_dc_counter(str);
@@ -305,7 +305,6 @@ static void require_dc_counter(int drm_fd, int dc_flag)
static void setup_dc3co(data_t *data)
{
- require_dc_counter(data->drm_fd, CHECK_DC3CO);
data->op_psr_mode = PSR_MODE_2;
psr_enable(data->debugfs_fd, data->op_psr_mode);
igt_require_f(edp_psr2_enabled(data),
@@ -314,6 +313,7 @@ static void setup_dc3co(data_t *data)
static void test_dc3co_vpb_simulation(data_t *data)
{
+ require_dc_counter(data->drm_fd, CHECK_DC3CO);
setup_output(data);
setup_dc3co(data);
setup_videoplayback(data);
@@ -325,6 +325,7 @@ static void test_dc_state_psr(data_t *data, int dc_flag)
{
uint32_t dc_counter_before_psr;
+ require_dc_counter(data->drm_fd, dc_flag);
dc_counter_before_psr = read_dc_counter(data->drm_fd, dc_flag);
setup_output(data);
setup_primary(data);
@@ -386,6 +387,7 @@ static void test_dc_state_dpms(data_t *data, int dc_flag)
{
uint32_t dc_counter;
+ require_dc_counter(data->drm_fd, dc_flag);
setup_dc_dpms(data);
dc_counter = read_dc_counter(data->drm_fd, dc_flag);
dpms_off(data);
> +{
> + char buf[4096];
> +
> + igt_debugfs_simple_read(drm_fd, "i915_dmc_info",
> + buf, sizeof(buf));
> +
> + switch (dc_flag) {
> + case CHECK_DC3CO:
> + igt_skip_on_f(strstr(buf, "DC3CO count"),
> + "DC3CO counter is not available\n");
> + break;
> + case CHECK_DC5:
> + igt_skip_on_f(strstr(buf, "DC3 -> DC5 count"),
> + "DC5 counter is not available\n");
> + break;
> + case CHECK_DC6:
> + igt_skip_on_f(strstr(buf, "DC5 -> DC6 count"),
> + "DC6 counter is not available\n");
> + break;
> + default:
> + igt_assert_f(0, "Unknown DC counter %d\n", dc_flag);
> + }
> +}
> +
> +static void setup_dc3co(data_t *data)
> +{
> + require_dc_counter(data->drm_fd, CHECK_DC3CO);
> + data->op_psr_mode = PSR_MODE_2;
> + psr_enable(data->debugfs_fd, data->op_psr_mode);
> + igt_require_f(edp_psr2_enabled(data),
> + "PSR2 is not enabled\n");
> +}
> +
> +static void test_dc3co_vpb_simulation(data_t *data)
> +{
> + setup_output(data);
> + setup_dc3co(data);
> + setup_videoplayback(data);
> + check_dc3co_with_videoplayback_like_load(data);
> + cleanup_dc3co_fbs(data);
> +}
> +
> static void test_dc_state_psr(data_t *data, int dc_flag)
> {
> uint32_t dc_counter_before_psr;
> @@ -284,6 +427,13 @@ int main(int argc, char *argv[])
> "Can't open /dev/cpu/0/msr.\n");
> }
>
> + igt_describe("This test simulate video playback "
> + "in order to validate DC3CO state "
> + "while PSR2 is active and in SLEEP state");
"This test" is something that the reader already knows. Using this kind of
reference makes sense when you need to address the whole thing few
sentences down the line. Here it just unecessarily inflates the word
count.
I suggest you to rephrase it like that:
"Make sure that we enter DC3CO when PSR2 is active and we are in SLEEP
state."
There are few more tests here that could use similar treatement so feel
free to do that in some follow up series later.
> + igt_subtest("dc3co-vpb-simulation") {
> + test_dc3co_vpb_simulation(&data);
> + }
> +
> igt_describe("This test validates display engine entry to DC5 state "
> "while PSR is active");
> igt_subtest("dc5-psr") {
> --
> 2.7.4
>
More information about the igt-dev
mailing list