> From: Pekka Paalanen <ppaalanen@xxxxxxxxx>
> Sent: Thursday, November 18, 2021 2:50 PM
> To: Modem, Bhanuprakash <bhanuprakash.modem@xxxxxxxxx>
> Cc: igt-dev@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Juha-Pekka
> Heikkila <juhapekka.heikkila@xxxxxxxxx>; Shankar, Uma <uma.shankar@xxxxxxxxx>
> Subject: Re: [i-g-t 07/14] tests/kms_color: New negative tests for plane level
> color mgmt
>
> On Mon, 15 Nov 2021 15:17:52 +0530
> Bhanuprakash Modem <bhanuprakash.modem@xxxxxxxxx> wrote:
>
> > Negative check for:
> > * plane gamma lut sizes
> > * plane degamma lut sizes
> > * plane ctm matrix sizes
> >
> > Cc: Harry Wentland <harry.wentland@xxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx>
> > Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@xxxxxxxxx>
> > ---
> > tests/kms_color.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 127 insertions(+)
> >
> > diff --git a/tests/kms_color.c b/tests/kms_color.c
> > index e14b37cb6f..d9fe417ba9 100644
> > --- a/tests/kms_color.c
> > +++ b/tests/kms_color.c
> > @@ -736,6 +736,118 @@ static void test_pipe_limited_range_ctm(data_t *data,
> > }
> > #endif
> >
> > +static bool invalid_plane_gamma_test(data_t *data, igt_plane_t *plane)
> > +{
> > + igt_display_t *display = &data->display;
> > + drmModePropertyPtr gamma_mode = NULL;
> > + uint32_t i;
> > +
> > + igt_info("Plane invalid gamma test is running on pipe-%s plane-
> %s(%s)\n",
> > + kmstest_pipe_name(plane->pipe->pipe),
> > + kmstest_plane_type_name(plane->type),
> > + is_hdr_plane(plane) ? "hdr":"sdr");
> > +
> > + igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_MODE));
> > + igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_LUT));
> > +
> > + gamma_mode = get_plane_gamma_degamma_mode(plane, IGT_PLANE_GAMMA_MODE);
> > +
> > + /* Iterate all supported gamma modes. */
> > + for (i = 0; i < gamma_mode->count_enums; i++) {
> > + segment_data_t *segment_info = NULL;
> > + size_t lut_size = 0;
> > +
> > + /* Ignore 'no gamma' from enum list. */
> > + if (!strcmp(gamma_mode->enums[i].name, "no gamma"))
> > + continue;
> > +
> > + igt_info("Trying to use gamma mode: \'%s\'\n", gamma_mode-
> >enums[i].name);
> > +
> > + segment_info = get_segment_data(data, gamma_mode->enums[i].value,
> > + gamma_mode->enums[i].name);
> > + lut_size = sizeof(struct drm_color_lut_ext) * segment_info-
> >entries_count;
> > +
> > + igt_plane_set_prop_enum(plane, IGT_PLANE_GAMMA_MODE, gamma_mode-
> >enums[i].name);
> > + invalid_plane_lut_sizes(display, plane,
> > + IGT_PLANE_GAMMA_LUT,
> > + lut_size);
> > +
> > + clear_segment_data(segment_info);
> > +
> > + /* One enum is enough. */
> > + break;
> > + }
> > +
> > + drmModeFreeProperty(gamma_mode);
> > +
> > + return true;
> > +}
> > +
> > +static bool invalid_plane_degamma_test(data_t *data, igt_plane_t *plane)
> > +{
> > + igt_display_t *display = &data->display;
> > + drmModePropertyPtr degamma_mode = NULL;
> > + uint32_t i;
> > +
> > + igt_info("Plane invalid degamma test is running on pipe-%s plane-
> %s(%s)\n",
> > + kmstest_pipe_name(plane->pipe->pipe),
> > + kmstest_plane_type_name(plane->type),
> > + is_hdr_plane(plane) ? "hdr":"sdr");
> > +
> > + igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_MODE));
> > + igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_LUT));
> > +
> > + degamma_mode = get_plane_gamma_degamma_mode(plane,
> IGT_PLANE_DEGAMMA_MODE);
> > +
> > + /* Iterate all supported degamma modes. */
> > + for (i = 0; i < degamma_mode->count_enums; i++) {
> > + segment_data_t *segment_info = NULL;
> > + size_t lut_size = 0;
> > +
> > + /* Ignore 'no degamma' from enum list. */
> > + if (!strcmp(degamma_mode->enums[i].name, "no degamma"))
> > + continue;
> > +
> > + igt_info("Trying to use degamma mode: \'%s\'\n", degamma_mode-
> >enums[i].name);
> > +
> > + segment_info = get_segment_data(data,
> > + degamma_mode->enums[i].value,
> > + degamma_mode->enums[i].name);
> > + lut_size = sizeof(struct drm_color_lut_ext) * segment_info-
> >entries_count * 2;
> > +
> > + igt_plane_set_prop_enum(plane,
> > + IGT_PLANE_DEGAMMA_MODE,
> > + degamma_mode->enums[i].name);
> > + invalid_plane_lut_sizes(display, plane,
> > + IGT_PLANE_DEGAMMA_LUT,
> > + lut_size);
> > +
> > + clear_segment_data(segment_info);
> > +
> > + /* One enum is enough. */
> > + break;
>
> Why is one enum enough?
>
> The same question for the other case in this patch.
This is just for CI time optimization, seems we don't save much CI time
I'll remove this & float a new rev.
>
>
> > + }
> > +
> > + drmModeFreeProperty(degamma_mode);
> > +
> > + return true;
> > +}
> > +
> > +static bool invalid_plane_ctm_test(data_t *data, igt_plane_t *plane)
> > +{
> > + igt_info("Plane invalid CTM test is running on pipe-%s plane-%s(%s)\n",
> > + kmstest_pipe_name(plane->pipe->pipe),
> > + kmstest_plane_type_name(plane->type),
> > + is_hdr_plane(plane) ? "hdr":"sdr");
> > +
> > + igt_require(igt_plane_has_prop(plane, IGT_PLANE_CTM));
> > + invalid_plane_lut_sizes(&data->display, plane,
> > + IGT_PLANE_CTM,
> > + sizeof(struct drm_color_ctm));
>
> The code says you're trying shove a LUT into a CTM blob. I understand
> that mechanically this is test you want to do, feed a wrong sized blob,
> and in this case the contents do not matter (unlike with actual LUTs),
> but reading this code is completely misleading and does not make sense.
> It takes a while to think about what you actually want to test here,
> and then reverse-engineer the code to understand that that is what
> actually happens, too. That is too much mental burden for the reader to
> realize that this piece of code actually works.
>
Sorry for the poor documentation, I'll try to add some comments.
- Bhanu
>
> Thanks,
> pq
>
> > +
> > + return true;
> > +}
> > +
> > static bool plane_gamma_test(data_t *data, igt_plane_t *plane)
> > {
> > igt_output_t *output;
> > @@ -1411,6 +1523,21 @@ static void run_tests_for_plane(data_t *data, enum
> pipe pipe)
> > ctm_tests[i].iter);
> > }
> > }
> > +
> > + igt_describe("Negative check for invalid plane gamma lut sizes");
> > + igt_subtest_f("pipe-%s-invalid-plane-gamma-lut-sizes",
> > + kmstest_pipe_name(pipe))
> > + run_plane_color_test(data, pipe, invalid_plane_gamma_test);
> > +
> > + igt_describe("Negative check for invalid plane degamma lut sizes");
> > + igt_subtest_f("pipe-%s-invalid-plane-degamma-lut-sizes",
> > + kmstest_pipe_name(pipe))
> > + run_plane_color_test(data, pipe, invalid_plane_degamma_test);
> > +
> > + igt_describe("Negative check for invalid plane ctm matrix sizes");
> > + igt_subtest_f("pipe-%s-invalid-plane-ctm-matrix-sizes",
> > + kmstest_pipe_name(pipe))
> > + run_plane_color_test(data, pipe, invalid_plane_ctm_test);
> > }
> >
> > igt_main
