Re: [igt-dev] [PATCH i-g-t v4] tests/kms_chamelium: Add support to validate RGB/YUV pixel formats

Arkadiusz Hiler
2020-03-06T14:29:24Z

On Fri, Mar 06, 2020 at 05:32:55AM +0530, Kunal Joshi wrote:
> Currently, only hard coded pixel formats are validated which are not
> generic across platforms which leads to SKIPS.
> Added support to validate only those pixel formats which are supported by
> platform. Covering both RGB and YUV pixel formats.
>
> v2: added support for RGB pixel formats (Swati)
> v3: simplified constraint and made it more readable (Petri)
> v4: -used igt_format_str instead of macro to print fourcc code (Ville)
> -dropped patch#1 because of above change
>
> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
> ---
> tests/kms_chamelium.c | 124 +++++++++++++++++++-------------------------------
> 1 file changed, 48 insertions(+), 76 deletions(-)
>
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 5c4a189..a0ec925 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -2612,6 +2612,14 @@ static const struct edid *get_edid(enum test_edid edid)
> if (chamelium_port_get_type(port) == \
> DRM_MODE_CONNECTOR_ ## type__)
>
> +#define connector_subtest_start(name__, type__) \
> + igt_subtest_with_dynamic(name__) \
> + for_each_port(p, port) \
> + if (chamelium_port_get_type(port) == \
> + DRM_MODE_CONNECTOR_ ## type__) {
> +
> +#define connector_subtest_end }

I agree with petri on having explicit {}. All the other macros are
following that, e.g.: igt_subtest.

#define connector_subtest_start(name__, type__) \
igt_subtest_with_dynamic(name__) \
for_each_port(p, port) \
if (chamelium_port_get_type(port) == \
DRM_MODE_CONNECTOR_ ## type__)

connector_subtest_start("hdmi-rgb-pixel-format", HDMIA) {
/* ... */
}

> @@ -2817,54 +2825,31 @@ igt_main
> CHAMELIUM_CHECK_CRC, 3);
>
> igt_describe(test_display_one_mode_desc);
> - connector_subtest("hdmi-crc-argb8888", HDMIA)
> - test_display_one_mode(&data, port, DRM_FORMAT_ARGB8888,
> - CHAMELIUM_CHECK_CRC, 1);
> -
> - igt_describe(test_display_one_mode_desc);
> - connector_subtest("hdmi-crc-abgr8888", HDMIA)
> - test_display_one_mode(&data, port, DRM_FORMAT_ABGR8888,
> - CHAMELIUM_CHECK_CRC, 1);
> -
> - igt_describe(test_display_one_mode_desc);
> - connector_subtest("hdmi-crc-xrgb8888", HDMIA)
> - test_display_one_mode(&data, port, DRM_FORMAT_XRGB8888,
> - CHAMELIUM_CHECK_CRC, 1);
> + connector_subtest_start("hdmi-rgb-pixel-format", HDMIA)

you are dropping few thing from the titles, especially information that
this is chamelium CRC tests

how about naming them:
hdmi-crc-nonplanar-formats instead of rgb
hdmi-crc-planar-formats instead of yuv

and then having the dunamic subsubtests named after the format alone, so
the end result would be like this:

igt@kms_chamelium@hdmi-crc-nonplanar-formats@rgb888

other than that looks good to me, thanks! :-)

--
Cheers,
Arek