[igt-dev] [PATCH i-g-t] tests/amdgpu/amd_color: check supported color prop before running tests

[Melissa Wen]

On 03/06, Alex Hung wrote:
> 
> 
> On 2023-02-14 19:42, Melissa Wen wrote:
> > If a DRM CRTC color prop is not advertised, the test should skip instead
> > of fail. It currently fits DCE case. The driver doesn't support user
> > degamma, therefore, DRM CRTC degamma properties are disabled in the
> > kernel driver [1]. Previously the test just fails on DCE because the
> > driver advertises degamma props although user degamma isn't supported by
> > HW. Now we can just skip the test in case of a missing color prop
> > support.
> > 
> > [1] https://lore.kernel.org/dri-devel/20221103184500.14450-1-mwen@igalia.com/
> > 
> > Signed-off-by: Melissa Wen <mwen at igalia.com>
> > ---
> >   tests/amdgpu/amd_color.c | 36 ++++++++++++++++++++++++++----------
> >   1 file changed, 26 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tests/amdgpu/amd_color.c b/tests/amdgpu/amd_color.c
> > index defe57bd..2dbc3dfb 100644
> > --- a/tests/amdgpu/amd_color.c
> > +++ b/tests/amdgpu/amd_color.c
> > @@ -196,14 +196,6 @@ static void test_init(data_t *data)
> >   	igt_output_set_pipe(data->output, data->pipe_id);
> > -	data->degamma_lut_size =
> > -		igt_pipe_obj_get_prop(data->pipe, IGT_CRTC_DEGAMMA_LUT_SIZE);
> > -	igt_assert_lt(0, data->degamma_lut_size);
> > -
> > -	data->regamma_lut_size =
> > -		igt_pipe_obj_get_prop(data->pipe, IGT_CRTC_GAMMA_LUT_SIZE);
> > -	igt_assert_lt(0, data->regamma_lut_size);
> > -
> >   	data->w = data->mode->hdisplay;
> >   	data->h = data->mode->vdisplay;
> >   }
> > @@ -221,6 +213,8 @@ static void test_fini(data_t *data)
> >    * matrix if not passed any. The whole pipe should be in linear bypass mode
> >    * when all the matrices are NULL - CRCs for a linear degamma matrix and
> >    * a NULL one should match.
> > + *
> > + * This test skips on DCE because it doesn't support user degamma.
> >    */
> >   static void test_crtc_linear_degamma(data_t *data)
> >   {
> > @@ -231,6 +225,12 @@ static void test_crtc_linear_degamma(data_t *data)
> >   	test_init(data);
> > +	igt_require(igt_pipe_obj_has_prop(data->pipe, IGT_CRTC_DEGAMMA_LUT));
> > +
> > +	data->degamma_lut_size =
> > +		igt_pipe_obj_get_prop(data->pipe, IGT_CRTC_DEGAMMA_LUT_SIZE);
> > +	igt_assert_lt(0, data->degamma_lut_size);
> > +
> >   	lut_init(&lut_linear, data->degamma_lut_size);
> >   	lut_gen_linear(&lut_linear, 0xffff);
> > @@ -252,6 +252,8 @@ static void test_crtc_linear_degamma(data_t *data)
> >   	igt_pipe_crc_collect_crc(data->pipe_crc, &new_crc);
> >   	igt_assert_crc_equal(&ref_crc, &new_crc);
> > +	igt_pipe_obj_replace_prop_blob(data->pipe, IGT_CRTC_DEGAMMA_LUT, NULL, 0);
> > +
> 
> These are not wrong, but isn't using "set_degamma_lut(data, NULL)" more
> symmetric?
> 
> This also applies to other igt_pipe_obj_replace_prop_blob(data->pipe,
> IGT_CRTC_DEGAMMA_LUT/IGT_CRTC_GAMMA_LUT, NULL, 0) below:
> 
> >   	test_fini(data);
> >   	igt_remove_fb(data->fd, &afb);
> >   	lut_free(&lut_linear);
> > @@ -273,6 +275,13 @@ static void test_crtc_linear_regamma(data_t *data)
> >   	test_init(data);
> > +	igt_require(igt_pipe_obj_has_prop(data->pipe, IGT_CRTC_GAMMA_LUT));
> > +
> > +
> > +	data->regamma_lut_size =
> > +		igt_pipe_obj_get_prop(data->pipe, IGT_CRTC_GAMMA_LUT_SIZE);
> > +	igt_assert_lt(0, data->regamma_lut_size);
> > +
> >   	lut_init(&lut_linear, data->regamma_lut_size);
> >   	lut_gen_linear(&lut_linear, 0xffff);
> > @@ -282,7 +291,6 @@ static void test_crtc_linear_regamma(data_t *data)
> >   	/* Draw the reference image. */
> >   	igt_plane_set_fb(data->primary, &afb);
> >   	set_regamma_lut(data, NULL);
> > -	set_degamma_lut(data, NULL);
> 
> The set_degamma_lut is removed in test_crtc_linear_regamma(). Will it be a
> good idea to remove set_regamma_lut in earlier function
> test_crtc_linear_degamma() as well?
> 
> >   	igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> >   	igt_pipe_crc_collect_crc(data->pipe_crc, &ref_crc);
> > @@ -294,6 +302,8 @@ static void test_crtc_linear_regamma(data_t *data)
> >   	igt_pipe_crc_collect_crc(data->pipe_crc, &new_crc);
> >   	igt_assert_crc_equal(&ref_crc, &new_crc);
> > +	igt_pipe_obj_replace_prop_blob(data->pipe, IGT_CRTC_GAMMA_LUT, NULL, 0);
> > +
> 
> set_regamma_lut(data, NULL);
> 
> >   	test_fini(data);
> >   	igt_remove_fb(data->fd, &afb);
> >   	lut_free(&lut_linear);
> > @@ -305,7 +315,7 @@ static void test_crtc_linear_regamma(data_t *data)
> >    * being CRC level accurate across a full test gradient but most values should
> >    * still match.
> >    *
> > - * This test can't pass on DCE because it doesn't support non-linear degamma.
> > + * This test skips on DCE because it doesn't support user degamma.
> >    */
> >   static void test_crtc_lut_accuracy(data_t *data)
> >   {
> > @@ -331,6 +341,9 @@ static void test_crtc_lut_accuracy(data_t *data)
> >   	test_init(data);
> > +	igt_require(igt_pipe_obj_has_prop(data->pipe, IGT_CRTC_DEGAMMA_LUT));
> > +	igt_require(igt_pipe_obj_has_prop(data->pipe, IGT_CRTC_GAMMA_LUT));
> > +
> >   	lut_init(&lut_degamma, data->degamma_lut_size);
> >   	lut_gen_degamma_srgb(&lut_degamma, 0xffff);
> > @@ -369,6 +382,9 @@ static void test_crtc_lut_accuracy(data_t *data)
> >   		igt_assert_crc_equal(&ref_crc, &new_crc);
> >   	}
> > +	igt_pipe_obj_replace_prop_blob(data->pipe, IGT_CRTC_GAMMA_LUT, NULL, 0);
> > +	igt_pipe_obj_replace_prop_blob(data->pipe, IGT_CRTC_DEGAMMA_LUT, NULL, 0);
> > +
> 
> set_regamma_lut(data, NULL);
> set_degamma_lut(data, NULL);

Hi Alex,

You're right. I just sent a v2 addressing your feedback.

Thanks for reviewing it,

Melissa

> 
> >   	test_fini(data);
> >   	igt_remove_fb(data->fd, &afb);
> >   	lut_free(&lut_regamma);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20230312/a1576756/attachment.sig>