Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_plane: Throw away yet another bi

Kazlauskas, Nicholas
2019-07-02T13:20:50Z

On 6/28/19 3:44 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> CHV pipe A/C sprites are causing a crc mismatch with BT.601
> when we keep the six msbs. If we throw away one more bit we get
> matches for BT.601. BT.709 matches even with 6 bits, as do the
> pipe B planes with their programmable CSC. Also IVB and KBL
> were both happy with 6 bits, so doesn't seem that that these
> CHV mismatches are due to bugs in our code, just the hw is
> a bit imprecise.
>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

With the title of this patch corrected ("Throw away yet another bit"),
this series is:

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

These patches are useful to have.

However, I'm not sure how I feel about the naming on all of these new
*_full functions or if they're really all needed. I think just having
one function that explicitly sets all of this is probably enough rather
than having these 4 new functions.

Nicholas Kazlauskas

> ---
> tests/kms_plane.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> index 13d36254dfe3..e4f3b8d68748 100644
> --- a/tests/kms_plane.c
> +++ b/tests/kms_plane.c
> @@ -30,6 +30,13 @@
> #include <stdio.h>
> #include <string.h>
>
> +/*
> + * Throw away enough lsbs in pixel formats tests
> + * to get a match despite some differences between
> + * the software and hardware YCbCr<->RGB conversion
> + * routines.
> + */
> +#define LUT_MASK 0xf800
>
> typedef struct {
> float red;
> @@ -690,7 +697,7 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
> continue;
>
> if (format == DRM_FORMAT_C8) {
> - if (!set_c8_legacy_lut(data, pipe, 0xfc00))
> + if (!set_c8_legacy_lut(data, pipe, LUT_MASK))
> continue;
> } else {
> if (!igt_fb_supported_format(format))
> @@ -709,7 +716,7 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
> ref_crc, &fb);
>
> if (format == DRM_FORMAT_C8)
> - set_legacy_lut(data, pipe, 0xfc00);
> + set_legacy_lut(data, pipe, LUT_MASK);
> }
>
> igt_pipe_crc_stop(data->pipe_crc);
> @@ -744,7 +751,7 @@ test_pixel_formats(data_t *data, enum pipe pipe)
>
> igt_display_commit2(&data->display, data->display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
>
> - set_legacy_lut(data, pipe, 0xfc00);
> + set_legacy_lut(data, pipe, LUT_MASK);
>
> test_init(data, pipe);
>
>