From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 18 Nov 2021 11:19:41 +0200 From: Pekka Paalanen Message-ID: <20211118111941.764737fb@eldfell> In-Reply-To: <20211115094759.520955-8-bhanuprakash.modem@intel.com> References: <20211115094759.520955-1-bhanuprakash.modem@intel.com> <20211115094759.520955-8-bhanuprakash.modem@intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="Sig_/6wSi2wncVoIw06chR.cibzR"; protocol="application/pgp-signature"; micalg=pgp-sha256 Subject: Re: [igt-dev] [i-g-t 07/14] tests/kms_color: New negative tests for plane level color mgmt List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Bhanuprakash Modem Cc: igt-dev@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-ID: --Sig_/6wSi2wncVoIw06chR.cibzR Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Mon, 15 Nov 2021 15:17:52 +0530 Bhanuprakash Modem wrote: > Negative check for: > * plane gamma lut sizes > * plane degamma lut sizes > * plane ctm matrix sizes >=20 > Cc: Harry Wentland > Cc: Ville Syrj=C3=A4l=C3=A4 > Cc: Juha-Pekka Heikkila > Cc: Uma Shankar > Signed-off-by: Bhanuprakash Modem > --- > tests/kms_color.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 127 insertions(+) >=20 > 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 *dat= a, > } > #endif > =20 > +static bool invalid_plane_gamma_test(data_t *data, igt_plane_t *plane) > +{ > + igt_display_t *display =3D &data->display; > + drmModePropertyPtr gamma_mode =3D 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 =3D get_plane_gamma_degamma_mode(plane, IGT_PLANE_GAMMA_MODE= ); > + > + /* Iterate all supported gamma modes. */ > + for (i =3D 0; i < gamma_mode->count_enums; i++) { > + segment_data_t *segment_info =3D NULL; > + size_t lut_size =3D 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].na= me); > + > + segment_info =3D get_segment_data(data, gamma_mode->enums[i].value, > + gamma_mode->enums[i].name); > + lut_size =3D 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 =3D &data->display; > + drmModePropertyPtr degamma_mode =3D 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 =3D get_plane_gamma_degamma_mode(plane, IGT_PLANE_DEGAMMA_= MODE); > + > + /* Iterate all supported degamma modes. */ > + for (i =3D 0; i < degamma_mode->count_enums; i++) { > + segment_data_t *segment_info =3D NULL; > + size_t lut_size =3D 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 =3D get_segment_data(data, > + degamma_mode->enums[i].value, > + degamma_mode->enums[i].name); > + lut_size =3D 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. > + } > + > + 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. 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); > } > =20 > igt_main --Sig_/6wSi2wncVoIw06chR.cibzR Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEJQjwWQChkWOYOIONI1/ltBGqqqcFAmGWGq0ACgkQI1/ltBGq qqcV1A//ZW1h9slYJ0YGb0gkYB5EoCVK8jOtZ/hNt/TV4aot2qNRxGCgQl7JkVe9 ok1qYrxpC31tOWrO6Bi6j5YSnRf6jQwqQHLlpED5IQJQRTvrECWRfH27yQsX37aw lD6zgJXcJCA3kJiYLxT3Rsmgq/PL7iQ3u903EwO364QnDsLEaSSZ2hdTj1sVueg5 jLb8ec/vZQGILL//zZPYQX8o3IIkUO1hbyRbkLCalVgRWfTL+YwSlrmeH8PRBSHt BgvZ64M6y1Q70yB5s+uav/CxQQyHUqnB+UHUyfhM0L7Kh/6VkTND2I34wLKqO7yI iIRehxLYuw8hwALfGnVakml28NoDGaqRORkxM6rw1oroErPM5Skk6sHRDOff4CE8 86ODb+Vg84guXjq3d5PIlE3EoCitx5S8I8/x6rjx6vMwdJKElvT2PqznLmFT+R6P T8R/JPh80YpzHsU1XI/f5HRLOTe5EoYmmmpe6mQLnMo2+YR1TrYeCisYdca7OOyT y2E8x/QBWbQLrnaqxvfuB+BsX5/zkWy8cmUkhoZhQanb/v5AA/3lVFYU3aKh0Dly 6xSWrM276mawC6kvKvsilZAzgnbJagyAo079jMwO/JaF1xAfpej+DIx0Xx+G8YoA sQ1mIM+ojXwJq3pfPYPPoRHfTZC7I6GTZ+7DbW1EKSOidXCgWew= =lh3s -----END PGP SIGNATURE----- --Sig_/6wSi2wncVoIw06chR.cibzR--