Re: [PATCH i-g-t v2 12/14] tests/kms_color: Rework tests slightly to work better with new atomic api

Mika Kahola
2017-10-20T07:21:04Z

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

On Thu, 2017-10-12 at 13:54 +0200, Maarten Lankhorst wrote:
> igt_pipe_get_property is about to be removed, so use
> igt_pipe_obj_get_prop instead. This requires adding 2 more properties
> to the crtc property list. Also get rid of the Broadcast RGB call,
> this is already handled in igt_kms.
>
> Change order for DEGAMMA_LUT and GAMMA_LUT around, else this test
> will
> fail if legacy gamma is set. In the legacy path, this will update
> GAMMA_LUT to the new size before DEGAMMA_LUT is set.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  lib/igt_kms.c     |   4 +-
>  lib/igt_kms.h     |   4 +-
>  tests/kms_color.c | 217 +++++++++++++++++++-------------------------
> ----------
>  3 files changed, 83 insertions(+), 142 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 9cf4b68f4191..cb2bc2b8df98 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -177,8 +177,10 @@ const char
> *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>  const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
>   "background_color",
>   "CTM",
> - "DEGAMMA_LUT",
>   "GAMMA_LUT",
> + "GAMMA_LUT_SIZE",
> + "DEGAMMA_LUT",
> + "DEGAMMA_LUT_SIZE",
>   "MODE_ID",
>   "ACTIVE",
>   "OUT_FENCE_PTR"
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index e722f0be3757..200f35e63308 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -95,8 +95,10 @@ void kmstest_restore_vt_mode(void);
>  enum igt_atomic_crtc_properties {
>         IGT_CRTC_BACKGROUND = 0,
>         IGT_CRTC_CTM,
> -       IGT_CRTC_DEGAMMA_LUT,
>         IGT_CRTC_GAMMA_LUT,
> +       IGT_CRTC_GAMMA_LUT_SIZE,
> +       IGT_CRTC_DEGAMMA_LUT,
> +       IGT_CRTC_DEGAMMA_LUT_SIZE,
>         IGT_CRTC_MODE_ID,
>         IGT_CRTC_ACTIVE,
>         IGT_CRTC_OUT_FENCE_PTR,
> diff --git a/tests/kms_color.c b/tests/kms_color.c
> index e30e6bf4e409..dcda12de3a39 100644
> --- a/tests/kms_color.c
> +++ b/tests/kms_color.c
> @@ -231,41 +231,6 @@ static void set_ctm(igt_pipe_t *pipe, const
> double *coefficients)
>  #define disable_gamma(pipe) igt_pipe_obj_replace_prop_blob(pipe,
> IGT_CRTC_GAMMA_LUT, NULL, 0)
>  #define disable_ctm(pipe) igt_pipe_obj_replace_prop_blob(pipe,
> IGT_CRTC_CTM, NULL, 0)
>
> -static void output_set_property_enum(igt_output_t *output,
> -      const char *property,
> -      const char *enum_value)
> -{
> - int i;
> - int32_t value = -1;
> - uint32_t prop_id;
> - drmModePropertyPtr prop;
> -
> - if (!kmstest_get_property(output->display->drm_fd,
> -   output->id,
> -   DRM_MODE_OBJECT_CONNECTOR,
> -   property,
> -   &prop_id, NULL, &prop))
> - return;
> -
> - igt_assert(prop->flags & DRM_MODE_PROP_ENUM);
> -
> - for (i = 0; i < prop->count_enums; i++) {
> - if (!strcmp(prop->enums[i].name, enum_value)) {
> - value = prop->enums[i].value;
> - break;
> - }
> - }
> - igt_assert_neq(value, -1);
> -
> - igt_assert_eq(drmModeObjectSetProperty(output->display-
> >drm_fd,
> -        output->id,
> -        DRM_MODE_OBJECT_CONNE
> CTOR,
> -        prop_id, value), 0);
> -
> -
> - drmModeFreeProperty(prop);
> -}
> -
>  /*
>   * Draw 3 gradient rectangles in red, green and blue, with a maxed
> out
>   * degamma LUT and verify we have the same CRC as drawing solid
> color
> @@ -531,23 +496,16 @@ static void test_pipe_legacy_gamma(data_t
> *data,
>  }
>
>  static drmModePropertyBlobPtr
> -get_blob(data_t *data, igt_pipe_t *pipe, const char *property_name)
> +get_blob(data_t *data, igt_pipe_t *pipe, enum
> igt_atomic_crtc_properties prop)
>  {
>   uint64_t prop_value;
> - drmModePropertyPtr prop;
> - drmModePropertyBlobPtr blob;
>
> - igt_assert(igt_pipe_get_property(pipe, property_name,
> -  NULL, &prop_value, &prop));
> + prop_value = igt_pipe_obj_get_prop(pipe, prop);
>
>   if (prop_value == 0)
>   return NULL;
>
> - igt_assert(prop->flags & DRM_MODE_PROP_BLOB);
> - blob = drmModeGetPropertyBlob(data->drm_fd, prop_value);
> - drmModeFreeProperty(prop);
> -
> - return blob;
> + return drmModeGetPropertyBlob(data->drm_fd, prop_value);
>  }
>
>  /*
> @@ -590,18 +548,18 @@ static void test_pipe_legacy_gamma_reset(data_t
> *data,
>   set_gamma(data, primary->pipe, gamma_zero);
>   igt_display_commit(&data->display);
>
> - blob = get_blob(data, primary->pipe, "DEGAMMA_LUT");
> + blob = get_blob(data, primary->pipe,
> IGT_CRTC_DEGAMMA_LUT);
>   igt_assert(blob &&
>      blob->length == (sizeof(struct
> _drm_color_lut) *
>       data-
> >degamma_lut_size));
>   drmModeFreePropertyBlob(blob);
>
> - blob = get_blob(data, primary->pipe, "CTM");
> + blob = get_blob(data, primary->pipe, IGT_CRTC_CTM);
>   igt_assert(blob &&
>      blob->length == sizeof(struct
> _drm_color_ctm));
>   drmModeFreePropertyBlob(blob);
>
> - blob = get_blob(data, primary->pipe, "GAMMA_LUT");
> + blob = get_blob(data, primary->pipe,
> IGT_CRTC_GAMMA_LUT);
>   igt_assert(blob &&
>      blob->length == (sizeof(struct
> _drm_color_lut) *
>       data->gamma_lut_size));
> @@ -634,10 +592,10 @@ static void test_pipe_legacy_gamma_reset(data_t
> *data,
>   igt_display_commit(&data->display);
>
>   igt_assert(get_blob(data, primary->pipe,
> -     "DEGAMMA_LUT") == NULL);
> - igt_assert(get_blob(data, primary->pipe, "CTM") ==
> NULL);
> +     IGT_CRTC_DEGAMMA_LUT) == NULL);
> + igt_assert(get_blob(data, primary->pipe,
> IGT_CRTC_CTM) == NULL);
>
> - blob = get_blob(data, primary->pipe, "GAMMA_LUT");
> + blob = get_blob(data, primary->pipe,
> IGT_CRTC_GAMMA_LUT);
>   igt_assert(blob &&
>      blob->length == (sizeof(struct
> _drm_color_lut) *
>       legacy_lut_size));
> @@ -777,6 +735,7 @@ static void test_pipe_limited_range_ctm(data_t
> *data,
>   0.0, 0.0, 1.0 };
>   double *degamma_linear, *gamma_linear;
>   igt_output_t *output;
> + bool has_broadcast_rgb_output = false;
>
>   degamma_linear = generate_table(data->degamma_lut_size,
> 1.0);
>   gamma_linear = generate_table(data->gamma_lut_size, 1.0);
> @@ -787,6 +746,11 @@ static void test_pipe_limited_range_ctm(data_t
> *data,
>   igt_crc_t crc_full, crc_limited;
>   int fb_id, fb_modeset_id;
>
> + if (!igt_output_has_prop(output,
> IGT_CONNECTOR_BROADCAST_RGB))
> + continue;
> +
> + has_broadcast_rgb_output = true;
> +
>   igt_output_set_pipe(output, primary->pipe->pipe);
>   mode = igt_output_get_mode(output);
>
> @@ -812,7 +776,7 @@ static void test_pipe_limited_range_ctm(data_t
> *data,
>   set_gamma(data, primary->pipe, gamma_linear);
>   set_ctm(primary->pipe, ctm);
>
> - output_set_property_enum(output, "Broadcast RGB",
> "Full");
> + igt_output_set_prop_value(output,
> IGT_CONNECTOR_BROADCAST_RGB, BROADCAST_RGB_FULL);
>   paint_rectangles(data, mode, red_green_blue_limited,
> &fb);
>   igt_plane_set_fb(primary, &fb);
>   igt_display_commit(&data->display);
> @@ -820,31 +784,34 @@ static void test_pipe_limited_range_ctm(data_t
> *data,
>   igt_pipe_crc_collect_crc(data->pipe_crc, &crc_full);
>
>   /* Set the output into limited range. */
> - output_set_property_enum(output, "Broadcast RGB",
> "Limited 16:235");
> + igt_output_set_prop_value(output,
> IGT_CONNECTOR_BROADCAST_RGB, BROADCAST_RGB_16_235);
>   paint_rectangles(data, mode, red_green_blue_full,
> &fb);
>   igt_plane_set_fb(primary, &fb);
>   igt_display_commit(&data->display);
>   igt_wait_for_vblank(data->drm_fd, primary->pipe-
> >pipe);
>   igt_pipe_crc_collect_crc(data->pipe_crc,
> &crc_limited);
>
> + /* And reset.. */
> + igt_output_set_prop_value(output,
> IGT_CONNECTOR_BROADCAST_RGB, BROADCAST_RGB_FULL);
> + igt_plane_set_fb(primary, NULL);
> + igt_output_set_pipe(output, PIPE_NONE);
> +
>   /* Verify that the CRC of the software computed
> output is
>    * equal to the CRC of the CTM matrix transformation
> output.
>    */
>   igt_assert_crc_equal(&crc_full, &crc_limited);
> -
> - igt_plane_set_fb(primary, NULL);
> - igt_output_set_pipe(output, PIPE_NONE);
>   }
>
>   free(gamma_linear);
>   free(degamma_linear);
> +
> + igt_require(has_broadcast_rgb_output);
>  }
>  #endif
>
>  static void
>  run_tests_for_pipe(data_t *data, enum pipe p)
>  {
> - igt_output_t *output;
>   igt_pipe_t *pipe;
>   igt_plane_t *primary;
>   double delta;
> @@ -856,8 +823,6 @@ run_tests_for_pipe(data_t *data, enum pipe p)
>   };
>
>   igt_fixture {
> - int valid_tests = 0;
> -
>   igt_require_pipe_crc(data->drm_fd);
>
>   igt_require(p < data->display.n_pipes);
> @@ -871,23 +836,18 @@ run_tests_for_pipe(data_t *data, enum pipe p)
>     primary->pipe-
> >pipe,
>     INTEL_PIPE_CRC_SOU
> RCE_AUTO);
>
> - igt_require(igt_pipe_get_property(&data-
> >display.pipes[p],
> -   "DEGAMMA_LUT_SIZE"
> ,
> -   NULL,
> -   &data-
> >degamma_lut_size,
> -   NULL));
> - igt_require(igt_pipe_get_property(&data-
> >display.pipes[p],
> -   "GAMMA_LUT_SIZE",
> -   NULL,
> -   &data-
> >gamma_lut_size,
> -   NULL));
> -
> - for_each_valid_output_on_pipe(&data->display, p,
> output) {
> - output_set_property_enum(output, "Broadcast
> RGB", "Full");
> -
> - valid_tests++;
> - }
> - igt_require_f(valid_tests, "no valid crtc/connector
> combinations found\n");
> + igt_require(igt_pipe_obj_has_prop(&data-
> >display.pipes[p], IGT_CRTC_DEGAMMA_LUT_SIZE));
> + igt_require(igt_pipe_obj_has_prop(&data-
> >display.pipes[p], IGT_CRTC_GAMMA_LUT_SIZE));
> +
> + data->degamma_lut_size =
> + igt_pipe_obj_get_prop(&data-
> >display.pipes[p],
> +       IGT_CRTC_DEGAMMA_LUT_S
> IZE);
> +
> + data->gamma_lut_size =
> + igt_pipe_obj_get_prop(&data-
> >display.pipes[p],
> +       IGT_CRTC_GAMMA_LUT_SIZ
> E);
> +
> + igt_display_require_output_on_pipe(&data->display,
> p);
>   }
>
>   /* We assume an 8bits depth per color for degamma/gamma LUTs
> @@ -1050,9 +1010,6 @@ run_tests_for_pipe(data_t *data, enum pipe p)
>   test_pipe_legacy_gamma_reset(data, primary);
>
>   igt_fixture {
> - for_each_connected_output(&data->display, output)
> - output_set_property_enum(output, "Broadcast
> RGB", "Full");
> -
>   disable_degamma(primary->pipe);
>   disable_gamma(primary->pipe);
>   disable_ctm(primary->pipe);
> @@ -1064,93 +1021,76 @@ run_tests_for_pipe(data_t *data, enum pipe p)
>  }
>
>  static int
> -pipe_set_property_blob_id(igt_pipe_t *pipe, const char *property,
> uint32_t blob_id)
> +pipe_set_property_blob_id(igt_pipe_t *pipe, enum
> igt_atomic_crtc_properties prop, uint32_t blob_id)
>  {
> - uint32_t prop_id;
> -
> - igt_assert(kmstest_get_property(pipe->display->drm_fd,
> - pipe->crtc_id,
> - DRM_MODE_OBJECT_CRTC,
> - property,
> - &prop_id, NULL, NULL));
> -
> - return drmModeObjectSetProperty(pipe->display->drm_fd,
> - pipe->crtc_id,
> - DRM_MODE_OBJECT_CRTC,
> - prop_id, blob_id);
> -}
> + int ret;
>
> -static int
> -pipe_set_property_blob(igt_pipe_t *pipe, const char *property, void
> *ptr, size_t length)
> -{
> - int ret = 0;
> - uint32_t blob_id = 0;
> + igt_pipe_obj_replace_prop_blob(pipe, prop, NULL, 0);
>
> - if (length > 0)
> - igt_assert_eq(drmModeCreatePropertyBlob(pipe-
> >display->drm_fd,
> - ptr, length,
> - &blob_id),
> 0);
> + igt_pipe_obj_set_prop_value(pipe, prop, blob_id);
>
> - ret = pipe_set_property_blob_id(pipe, property, blob_id);
> + ret = igt_display_try_commit2(pipe->display, pipe->display-
> >is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
>
> - if (blob_id != 0)
> - igt_assert_eq(drmModeDestroyPropertyBlob(pipe-
> >display->drm_fd, blob_id), 0);
> + igt_pipe_obj_set_prop_value(pipe, prop, 0);
>
>   return ret;
>  }
>
> +static int
> +pipe_set_property_blob(igt_pipe_t *pipe, enum
> igt_atomic_crtc_properties prop, void *ptr, size_t length)
> +{
> + igt_pipe_obj_replace_prop_blob(pipe, prop, ptr, length);
> +
> + return igt_display_try_commit2(pipe->display, pipe->display-
> >is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> +}
> +
>  static void
>  invalid_lut_sizes(data_t *data)
>  {
> - igt_pipe_t *pipe = &data->display.pipes[0];
> + igt_display_t *display = &data->display;
> + igt_pipe_t *pipe = &display->pipes[0];
>   size_t degamma_lut_size = data->degamma_lut_size *
> sizeof(struct _drm_color_lut);
>   size_t gamma_lut_size = data->gamma_lut_size * sizeof(struct
> _drm_color_lut);
>
>   struct _drm_color_lut *degamma_lut = malloc(data-
> >degamma_lut_size * sizeof(struct _drm_color_lut) * 2);
>   struct _drm_color_lut *gamma_lut = malloc(data-
> >gamma_lut_size * sizeof(struct _drm_color_lut) * 2);
>
> - if (kmstest_get_property(pipe->display->drm_fd,
> -  pipe->crtc_id,
> -  DRM_MODE_OBJECT_CRTC,
> -  "DEGAMMA_LUT",
> -  NULL, NULL, NULL)) {
> - igt_assert_eq(pipe_set_property_blob(pipe,
> "DEGAMMA_LUT",
> + igt_display_commit2(display, display->is_atomic ?
> COMMIT_ATOMIC : COMMIT_LEGACY);
> +
> + if (igt_pipe_obj_has_prop(pipe, IGT_CRTC_DEGAMMA_LUT)) {
> + igt_assert_eq(pipe_set_property_blob(pipe,
> IGT_CRTC_DEGAMMA_LUT,
>        degamma_lut,
> 1), -EINVAL);
> - igt_assert_eq(pipe_set_property_blob(pipe,
> "DEGAMMA_LUT",
> + igt_assert_eq(pipe_set_property_blob(pipe,
> IGT_CRTC_DEGAMMA_LUT,
>        degamma_lut,
> degamma_lut_size + 1),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob(pipe,
> "DEGAMMA_LUT",
> + igt_assert_eq(pipe_set_property_blob(pipe,
> IGT_CRTC_DEGAMMA_LUT,
>        degamma_lut,
> degamma_lut_size - 1),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob(pipe,
> "DEGAMMA_LUT",
> + igt_assert_eq(pipe_set_property_blob(pipe,
> IGT_CRTC_DEGAMMA_LUT,
>        degamma_lut,
> degamma_lut_size + sizeof(struct _drm_color_lut)),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob_id(pipe,
> "DEGAMMA_LUT", pipe->crtc_id),
> + igt_assert_eq(pipe_set_property_blob_id(pipe,
> IGT_CRTC_DEGAMMA_LUT, pipe->crtc_id),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob_id(pipe,
> "DEGAMMA_LUT", 4096 * 4096),
> + igt_assert_eq(pipe_set_property_blob_id(pipe,
> IGT_CRTC_DEGAMMA_LUT, 4096 * 4096),
>         -EINVAL);
>   }
>
> - if (kmstest_get_property(pipe->display->drm_fd,
> -  pipe->crtc_id,
> -  DRM_MODE_OBJECT_CRTC,
> -  "GAMMA_LUT",
> -  NULL, NULL, NULL)) {
> - igt_assert_eq(pipe_set_property_blob(pipe,
> "GAMMA_LUT",
> + if (igt_pipe_obj_has_prop(pipe, IGT_CRTC_GAMMA_LUT)) {
> + igt_assert_eq(pipe_set_property_blob(pipe,
> IGT_CRTC_GAMMA_LUT,
>        gamma_lut, 1),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob(pipe,
> "GAMMA_LUT",
> + igt_assert_eq(pipe_set_property_blob(pipe,
> IGT_CRTC_GAMMA_LUT,
>        gamma_lut,
> gamma_lut_size + 1),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob(pipe,
> "GAMMA_LUT",
> + igt_assert_eq(pipe_set_property_blob(pipe,
> IGT_CRTC_GAMMA_LUT,
>        gamma_lut,
> gamma_lut_size - 1),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob(pipe,
> "GAMMA_LUT",
> + igt_assert_eq(pipe_set_property_blob(pipe,
> IGT_CRTC_GAMMA_LUT,
>        gamma_lut,
> gamma_lut_size + sizeof(struct _drm_color_lut)),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob_id(pipe,
> "GAMMA_LUT", pipe->crtc_id),
> + igt_assert_eq(pipe_set_property_blob_id(pipe,
> IGT_CRTC_GAMMA_LUT, pipe->crtc_id),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob_id(pipe,
> "GAMMA_LUT", 4096 * 4096),
> + igt_assert_eq(pipe_set_property_blob_id(pipe,
> IGT_CRTC_GAMMA_LUT, 4096 * 4096),
>         -EINVAL);
>   }
>
> @@ -1161,32 +1101,29 @@ invalid_lut_sizes(data_t *data)
>  static void
>  invalid_ctm_matrix_sizes(data_t *data)
>  {
> - igt_pipe_t *pipe = &data->display.pipes[0];
> + igt_display_t *display = &data->display;
> + igt_pipe_t *pipe = &display->pipes[0];
>   void *ptr;
>
> - if (!kmstest_get_property(pipe->display->drm_fd,
> -   pipe->crtc_id,
> -   DRM_MODE_OBJECT_CRTC,
> -   "CTM",
> -   NULL, NULL, NULL))
> + if (!igt_pipe_obj_has_prop(pipe, IGT_CRTC_CTM))
>   return;
>
>   ptr = malloc(sizeof(struct _drm_color_ctm) * 4);
>
> - igt_assert_eq(pipe_set_property_blob(pipe, "CTM", ptr, 1),
> + igt_assert_eq(pipe_set_property_blob(pipe, IGT_CRTC_CTM,
> ptr, 1),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob(pipe, "CTM", ptr,
> + igt_assert_eq(pipe_set_property_blob(pipe, IGT_CRTC_CTM,
> ptr,
>        sizeof(struct
> _drm_color_ctm) + 1),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob(pipe, "CTM", ptr,
> + igt_assert_eq(pipe_set_property_blob(pipe, IGT_CRTC_CTM,
> ptr,
>        sizeof(struct
> _drm_color_ctm) - 1),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob(pipe, "CTM", ptr,
> + igt_assert_eq(pipe_set_property_blob(pipe, IGT_CRTC_CTM,
> ptr,
>        sizeof(struct
> _drm_color_ctm) * 2),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob_id(pipe, "CTM", pipe-
> >crtc_id),
> + igt_assert_eq(pipe_set_property_blob_id(pipe, IGT_CRTC_CTM,
> pipe->crtc_id),
>         -EINVAL);
> - igt_assert_eq(pipe_set_property_blob_id(pipe, "CTM", 4096 *
> 4096),
> + igt_assert_eq(pipe_set_property_blob_id(pipe, IGT_CRTC_CTM,
> 4096 * 4096),
>         -EINVAL);
>
>   free(ptr);
--
Mika Kahola - Intel OTC