Re: [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Fri, 2018-07-27 at 13:43 -0700, Radhakrishna Sripada wrote:
> On Mon, Jul 23, 2018 at 08:26:06PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Mon, 2018-07-23 at 19:52 -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> > > > 
> > > > 
> > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > 
> > > > Cleanup the testcases by moving the platform checks to a single
> > > > function.
> > > > 
> > > > The earlier version of the path is posted here [1]
> > > > 
> > > > v2: Make use of the property enums to get the supported
> > > > rotations
> > > > v3: Move hardcodings to a single function(Ville)
> > > > v4: Include the cherryview exception for reflect
> > > > subtest(Maarten)
> > > > v5: Rebase and move the check from CNL to ICL for reflect-x
> > > > case
> > > > v6: Fix the CI regression
> > > > v7: rebase
> > > > 
> > > > [1]: https://patchwork.freedesktop.org/patch/209647/
> > > > 
> > > Oh well, I wrote my comments below and then read this link.
> > > Please
> > > add
> > > new test requirements in separate patches. Only have the code
> > > movement
> > > here.
> > > 
> > Quoting Ville from [1] above
> > 
> > "Perhaps the best solution would be to make it as generic as
> > possible
> > by checking the plane supported rotations, while still keeoing the
> > manual checks for the few exceptions I listed above. Might even be
> > nice to put the generic stuff into something like
> > igt_plane_has_rotation(). And maybe the exceptions should be there
> > as well?"
> > 
> > If I am reading this correctly, this patch should have retained the
> > plane property checks in addition to exceptions you have added.
> Based on the feedback received from patch v2 [1], we moved back to
> using
> the platform checks instead of using the connector property.
> 

Okay, I am not reading the comment, specifically "make it as generic as
possible by checking the plane supported rotations", the way you are.
Check with Ville.

Anyway, if you want to add new platform checks, I do not recommend
adding all of them in a single patch.


> 
> [1] https://patchwork.freedesktop.org/patch/209647/
> > 
> > 
> > -DK
> > 
> > > 
> > > > 
> > > > 
> > > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel
> > > > .com
> > > > > 
> > > > > 
> > > > ---
> > > >  tests/kms_rotation_crc.c | 35 ++++++++++++++++++++----------
> > > > -----
> > > >  1 file changed, 20 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/tests/kms_rotation_crc.c
> > > > b/tests/kms_rotation_crc.c
> > > > index 6cb5858adb0f..f20b8a6d4ba1 100644
> > > > --- a/tests/kms_rotation_crc.c
> > > > +++ b/tests/kms_rotation_crc.c
> > > > @@ -43,6 +43,7 @@ typedef struct {
> > > >  	uint32_t override_fmt;
> > > >  	uint64_t override_tiling;
> > > >  	int devid;
> > > > +	int gen;
> > > >  } data_t;
> > > >  
> > > >  typedef struct {
> > > > @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> > > > igt_output_t *output,
> > > >  		igt_plane_set_position(plane, data->pos_x,
> > > > data-
> > > > > 
> > > > > 
> > > > > pos_y);
> > > >  }
> > > >  
> > > > +static void igt_check_rotation(data_t *data)
> > > > +{
> > > > +	if (data->rotation & (IGT_ROTATION_90 |
> > > > IGT_ROTATION_270))
> > > > +		igt_require(data->gen >= 9);
> > > > +	if (data->rotation & IGT_REFLECT_X)
> > > > +		igt_require(data->gen >= 11 ||
> > > This check used to be igt_require(gen >= 10
> Will move it back to gen10.
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +			    (IS_CHERRYVIEW(data->devid) &&
> > > > (data-
> > > > > 
> > > > > 
> > > > > rotation & IGT_ROTATION_0)));
> > > There was also a check for tiling format 
> > > -                                   (IS_CHERRYVIEW(data.devid) &&
> > > reflect_x->rot == IGT_ROTATION_0
> > > -                                    && reflect_x->tiling ==
> > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > > 
> > > 
> > > > 
> > > > 
> > > > +	if (data->rotation & IGT_ROTATION_180)
> > > > +		igt_require(data->gen >= 4);
> > > Doesn't look like this requirement was is in the test earlier.
> > > 
> This is the requirement present in the kernel and got included here
> on suggestion from here[1].
> 
> > 
> > > 
> > > > 
> > > > 
> > > > +}
> > > > +
> > > >  static void test_single_case(data_t *data, enum pipe pipe,
> > > >  			     igt_output_t *output, igt_plane_t
> > > > *plane,
> > > >  			     enum rectangle_type rect,
> > > > @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t
> > > > *data,
> > > > int plane_type, bool test_bad_form
> > > >  
> > > >  	igt_display_require_output(display);
> > > >  
> > > > +	igt_check_rotation(data);
> > > > +
> > > >  	for_each_pipe_with_valid_output(display, pipe, output)
> > > > {
> > > >  		igt_plane_t *plane;
> > > >  		int i, j;
> > > >  
> > > > -		if (IS_CHERRYVIEW(data->devid) && pipe !=
> > > > PIPE_B)
> > > > -			continue;
> > > > -
> > > >  		igt_output_set_pipe(output, pipe);
> > > >  
> > > > +		if (IS_CHERRYVIEW(data->devid) && (data-
> > > > >rotation
> > > > &
> > > > IGT_REFLECT_X) &&
> > > > +		    pipe != kmstest_pipe_to_index('B'))
> > > > +			continue;
> > > > +
> > > Why do this? 
> > > 
> On braswell reflectx is supported only on Pipe B and the test case
> needs to be skipped for pipes A,C.
> 
> 
> Regards,
> Radhhakrishna(RK) Sripada
> > 
> > > 
> > > > 
> > > > 
> > > >  		plane = igt_output_get_plane_type(output,
> > > > plane_type);
> > > >  		igt_require(igt_plane_has_prop(plane,
> > > > IGT_PLANE_ROTATION));
> > > >  
> > > > @@ -521,14 +536,13 @@ igt_main
> > > >  	};
> > > >  
> > > >  	data_t data = {};
> > > > -	int gen = 0;
> > > >  
> > > >  	igt_skip_on_simulation();
> > > >  
> > > >  	igt_fixture {
> > > >  		data.gfx_fd =
> > > > drm_open_driver_master(DRIVER_INTEL);
> > > >  		data.devid = intel_get_drm_devid(data.gfx_fd);
> > > > -		gen = intel_gen(data.devid);
> > > > +		data.gen = intel_gen(data.devid);
> > > >  
> > > >  		kmstest_set_vt_graphics_mode();
> > > >  
> > > > @@ -541,16 +555,12 @@ igt_main
> > > >  		igt_subtest_f("%s-rotation-%s",
> > > >  			      plane_test_str(subtest->plane),
> > > >  			      rot_test_str(subtest->rot)) {
> > > > -			igt_require(!(subtest->rot &
> > > > -				    (IGT_ROTATION_90 |
> > > > IGT_ROTATION_270)) ||
> > > > -				    gen >= 9);
> > > >  			data.rotation = subtest->rot;
> > > >  			test_plane_rotation(&data, subtest-
> > > > >plane,
> > > > false);
> > > >  		}
> > > >  	}
> > > >  
> > > >  	igt_subtest_f("sprite-rotation-90-pos-100-0") {
> > > > -		igt_require(gen >= 9);
> > > >  		data.rotation = IGT_ROTATION_90;
> > > >  		data.pos_x = 100,
> > > >  		data.pos_y = 0;
> > > > @@ -560,7 +570,6 @@ igt_main
> > > >  	data.pos_y = 0;
> > > >  
> > > >  	igt_subtest_f("bad-pixel-format") {
> > > > -		igt_require(gen >= 9);
> > > >  		data.rotation = IGT_ROTATION_90;
> > > >  		data.override_fmt = DRM_FORMAT_RGB565;
> > > >  		test_plane_rotation(&data,
> > > > DRM_PLANE_TYPE_PRIMARY,
> > > > true);
> > > > @@ -568,7 +577,6 @@ igt_main
> > > >  	data.override_fmt = 0;
> > > >  
> > > >  	igt_subtest_f("bad-tiling") {
> > > > -		igt_require(gen >= 9);
> > > >  		data.rotation = IGT_ROTATION_90;
> > > >  		data.override_tiling =
> > > > LOCAL_I915_FORMAT_MOD_X_TILED;
> > > >  		test_plane_rotation(&data,
> > > > DRM_PLANE_TYPE_PRIMARY,
> > > > true);
> > > > @@ -579,9 +587,6 @@ igt_main
> > > >  		igt_subtest_f("primary-%s-reflect-x-%s",
> > > >  			      tiling_test_str(reflect_x-
> > > > >tiling),
> > > >  			      rot_test_str(reflect_x->rot)) {
> > > > -			igt_require(gen >= 10 ||
> > > > -				    (IS_CHERRYVIEW(data.devid)
> > > > &&
> > > > reflect_x->rot == IGT_ROTATION_0
> > > > -				     && reflect_x->tiling ==
> > > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > > >  			data.rotation = (IGT_REFLECT_X |
> > > > reflect_x-
> > > > > 
> > > > > 
> > > > > rot);
> > > >  			data.override_tiling = reflect_x-
> > > > >tiling;
> > > >  			test_plane_rotation(&data,
> > > > DRM_PLANE_TYPE_PRIMARY, false);
> > > > @@ -596,7 +601,7 @@ igt_main
> > > >  		enum pipe pipe;
> > > >  		igt_output_t *output;
> > > >  
> > > > -		igt_require(gen >= 9);
> > > > +		igt_require(data.gen >= 9);
> > > >  		igt_display_require_output(&data.display);
> > > >  
> > > >  		for_each_pipe_with_valid_output(&data.display,
> > > > pipe,
> > > > output) {
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev