Re: [igt-dev] [PATCH i-g-t 4/6] tests/kms_color: Allow most subtests to run with a partial color pipeline

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 03, 2019 at 03:50:54PM +0200, Daniel Vetter wrote:
> On Tue, Apr 02, 2019 at 07:33:46PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Requiring a full color pipeline when we're just testing eg. the gamma
> > LUT is silly. Make the requirements more sensible.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  tests/kms_color.c | 47 +++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 33 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tests/kms_color.c b/tests/kms_color.c
> > index c65bb88cd7dc..554356a04f75 100644
> > --- a/tests/kms_color.c
> > +++ b/tests/kms_color.c
> > @@ -252,9 +252,15 @@ static void set_ctm(igt_pipe_t *pipe, const double *coefficients)
> >  	igt_pipe_obj_replace_prop_blob(pipe, IGT_CRTC_CTM, &ctm, sizeof(ctm));
> >  }
> >  
> > -#define disable_degamma(pipe) igt_pipe_obj_replace_prop_blob(pipe, IGT_CRTC_DEGAMMA_LUT, NULL, 0)
> > -#define disable_gamma(pipe) igt_pipe_obj_replace_prop_blob(pipe, IGT_CRTC_GAMMA_LUT, NULL, 0)
> > -#define disable_ctm(pipe) igt_pipe_obj_replace_prop_blob(pipe, IGT_CRTC_CTM, NULL, 0)
> > +static void disable_prop(igt_pipe_t *pipe, enum igt_atomic_crtc_properties prop)
> 
> Hm, could even be an igt_kms helper function I think. For another patch.
> 
> > +{
> > +	if (igt_pipe_obj_has_prop(pipe, prop))
> > +		igt_pipe_obj_replace_prop_blob(pipe, prop, NULL, 0);
> > +}
> > +
> > +#define disable_degamma(pipe) disable_prop(pipe, IGT_CRTC_DEGAMMA_LUT)
> > +#define disable_gamma(pipe) disable_prop(pipe, IGT_CRTC_GAMMA_LUT)
> > +#define disable_ctm(pipe) disable_prop(pipe, IGT_CRTC_CTM)
> >  
> >  /*
> >   * Draw 3 gradient rectangles in red, green and blue, with a maxed out
> > @@ -273,6 +279,9 @@ static void test_pipe_degamma(data_t *data,
> >  		{ 0.0, 0.0, 1.0 }
> >  	};
> >  
> > +	igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_DEGAMMA_LUT));
> > +	igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_GAMMA_LUT));
> > +
> >  	degamma_linear = generate_table(data->degamma_lut_size, 1.0);
> >  	degamma_full = generate_table_max(data->degamma_lut_size);
> >  
> > @@ -356,6 +365,8 @@ static void test_pipe_gamma(data_t *data,
> >  		{ 0.0, 0.0, 1.0 }
> >  	};
> >  
> > +	igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_GAMMA_LUT));
> > +
> >  	gamma_full = generate_table_max(data->gamma_lut_size);
> >  
> >  	for_each_valid_output_on_pipe(&data->display, primary->pipe->pipe, output) {
> > @@ -553,6 +564,11 @@ static void test_pipe_legacy_gamma_reset(data_t *data,
> >  	drmModePropertyBlobPtr blob;
> >  	igt_output_t *output;
> >  
> > +	/* FIXME accept any one of these */
> > +	igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_DEGAMMA_LUT));
> > +	igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_GAMMA_LUT));
> > +	igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_CTM));
> > +
> >  	degamma_linear = generate_table(data->degamma_lut_size, 1.0);
> >  	gamma_zero = generate_table_zero(data->gamma_lut_size);
> >  
> > @@ -663,6 +679,8 @@ static bool test_pipe_ctm(data_t *data,
> >  	igt_output_t *output;
> >  	bool ret = true;
> >  
> > +	igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_CTM));
> 
> Hm, am I blind or did we not have any igt require for the CTM beforehand?

Looks like we did not.

> Would be good to mention that in the commit message this your patch here
> fixes that.

Sure, I'll amend the message a bit.

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> > +
> >  	degamma_linear = generate_table(data->degamma_lut_size, 1.0);
> >  	gamma_linear = generate_table(data->gamma_lut_size, 1.0);
> >  
> > @@ -861,19 +879,20 @@ run_tests_for_pipe(data_t *data, enum pipe p)
> >  						  primary->pipe->pipe,
> >  						  INTEL_PIPE_CRC_SOURCE_AUTO);
> >  
> > -		igt_require(igt_pipe_obj_has_prop(&data->display.pipes[p], IGT_CRTC_DEGAMMA_LUT_SIZE));
> > -		igt_require(igt_pipe_obj_has_prop(&data->display.pipes[p], IGT_CRTC_GAMMA_LUT_SIZE));
> > -
> > -		data->degamma_lut_size =
> > -			igt_pipe_obj_get_prop(&data->display.pipes[p],
> > -					      IGT_CRTC_DEGAMMA_LUT_SIZE);
> > +		if (igt_pipe_obj_has_prop(&data->display.pipes[p], IGT_CRTC_DEGAMMA_LUT_SIZE)) {
> > +			data->degamma_lut_size =
> > +				igt_pipe_obj_get_prop(&data->display.pipes[p],
> > +						      IGT_CRTC_DEGAMMA_LUT_SIZE);
> > +			igt_assert_lt(0, data->degamma_lut_size);
> > +		}
> >  
> > -		data->gamma_lut_size =
> > -			igt_pipe_obj_get_prop(&data->display.pipes[p],
> > -					      IGT_CRTC_GAMMA_LUT_SIZE);
> > +		if (igt_pipe_obj_has_prop(&data->display.pipes[p], IGT_CRTC_GAMMA_LUT_SIZE)) {
> > +			data->gamma_lut_size =
> > +				igt_pipe_obj_get_prop(&data->display.pipes[p],
> > +						      IGT_CRTC_GAMMA_LUT_SIZE);
> > +			igt_assert_lt(0, data->gamma_lut_size);
> > +		}
> >  
> > -		igt_assert_lt(0, data->degamma_lut_size);
> > -		igt_assert_lt(0, data->gamma_lut_size);
> >  		igt_display_require_output_on_pipe(&data->display, p);
> >  	}
> >  
> > -- 
> > 2.19.2
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev