[Intel-gfx] [PATCH] i-g-t: Adding rotation to plane scaling test

[Konduru, Chandra]

> -----Original Message-----
> From: Thomas Wood [mailto:thomas.wood at intel.com]
> Sent: Wednesday, April 22, 2015 9:58 AM
> To: Konduru, Chandra
> Cc: Intel Graphics Development
> Subject: Re: [Intel-gfx] [PATCH] i-g-t: Adding rotation to plane scaling test
> 
> "kms_plane_scaling" would be a better tag for this commit. You can still make
> sure that "i-g-t" appears in the subject line by using the --subject-prefix="PATCH
> i-g-t" option when using git send-email.

Submitted updated patch "[PREFIX i-g-t] Improvements to kms_plane_scaling" 
to address your feedback. Also resolved an issue Tvrtko reported.

> 
> On 16 April 2015 at 00:19, Chandra Konduru <chandra.konduru at intel.com>
> wrote:
> > From: chandra konduru <chandra.konduru at intel.com>
> >
> > Adding rotation to kms_plane_scaling test.
> >
> > Signed-off-by: chandra konduru <chandra.konduru at intel.com>
> > ---
> >  tests/kms_plane_scaling.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
> > index 00db5cb..8d22ba4 100644
> > --- a/tests/kms_plane_scaling.c
> > +++ b/tests/kms_plane_scaling.c
> > @@ -101,11 +101,11 @@ static void prepare_crtc(data_t *data, igt_output_t
> *output, enum pipe pipe,
> >         data->fb_id1 = igt_create_fb(data->drm_fd,
> >                         mode->hdisplay, mode->vdisplay,
> >                         DRM_FORMAT_XRGB8888,
> > -                       LOCAL_I915_FORMAT_MOD_X_TILED, /* tiled */
> > +                       LOCAL_I915_FORMAT_MOD_Y_TILED, /* tiled */
> >                         &data->fb1);
> >         igt_assert(data->fb_id1);
> >
> > -       paint_color(data, &data->fb1, mode->hdisplay, mode->vdisplay);
> > +       paint_color(data, &data->fb1, data->fb1.width,
> > + data->fb1.height);
> 
> This looks like an unrelated change.
> 
> 
> >
> >         /*
> >          * We always set the primary plane to actually enable the pipe
> > as @@ -209,7 +209,7 @@ static void test_plane_scaling(data_t *d)
> >         cairo_surface_t *image;
> >         enum pipe pipe;
> >         int valid_tests = 0;
> > -       int primary_plane_scaling = 0; /* For now */
> > +       int primary_plane_scaling = 1;
> 
> Since this is now enabled and not set elsewhere, could it and the if statements
> that use it now be removed? Since it doesn't just affect the new rotation tests, it
> could be removed in a separate patch.
> 
> 
> >
> >         igt_require(d->display.has_universal_planes);
> >         igt_require(d->num_scalers);
> > @@ -250,18 +250,20 @@ static void test_plane_scaling(data_t *d)
> >                 prepare_crtc(d, output, pipe, d->plane1, mode,
> > COMMIT_UNIVERSAL);
> >
> >                 if (primary_plane_scaling) {
> > -                       /* Primary plane upscaling */
> > +                       /* Primary plane upscaling with rotation */
> 
> Isn't this now testing two things at once? Would it be better to test them
> separately?
> 
> 
> >                         igt_fb_set_position(&d->fb1, d->plane1, 100, 100);
> >                         igt_fb_set_size(&d->fb1, d->plane1, 500, 500);
> >                         igt_plane_set_position(d->plane1, 0, 0);
> >                         igt_plane_set_size(d->plane1, mode->hdisplay,
> > mode->vdisplay);
> > +                       igt_plane_set_rotation(d->plane1,
> > + IGT_ROTATION_90);
> >                         igt_display_commit2(display,
> > COMMIT_UNIVERSAL);
> >
> > -                       /* Primary plane 1:1 no scaling */
> > +                       /* Primary plane 1:1 no scaling & no rotation
> > + */
> >                         igt_fb_set_position(&d->fb1, d->plane1, 0, 0);
> >                         igt_fb_set_size(&d->fb1, d->plane1, d->fb1.width, d->fb1.height);
> >                         igt_plane_set_position(d->plane1, 0, 0);
> >                         igt_plane_set_size(d->plane1, mode->hdisplay,
> > mode->vdisplay);
> > +                       igt_plane_set_rotation(d->plane1,
> > + IGT_ROTATION_0);
> >                         igt_display_commit2(display, COMMIT_UNIVERSAL);
> >                 }
> >
> > @@ -318,10 +320,10 @@ static void test_plane_scaling(data_t *d)
> >                 igt_plane_set_position(d->plane2, 100, 100);
> >                 igt_plane_set_size(d->plane2, d->fb2.width-200,
> > d->fb2.height-200);
> >
> > -               igt_fb_set_position(&d->fb3, d->plane3, 100, 100);
> > -               igt_fb_set_size(&d->fb3, d->plane3, d->fb3.width-400, d->fb3.height-
> 400);
> > -               igt_plane_set_position(d->plane3, 10, 10);
> > -               igt_plane_set_size(d->plane3, mode->hdisplay-300, mode->vdisplay-
> 300);
> > +               igt_fb_set_position(&d->fb3, d->plane3, 0, 0);
> > +               igt_fb_set_size(&d->fb3, d->plane3, d->fb3.width, d->fb3.height);
> > +               igt_plane_set_position(d->plane3, 500, 500);
> > +               igt_plane_set_size(d->plane3, d->fb3.width * 2/3,
> > + d->fb3.height * 2/3);
> 
> As above, these changes also look unrelated to rotation and should be in a
> separate patch.
> 
> 
> >                 igt_display_commit2(display, COMMIT_UNIVERSAL);
> >
> >                 if (primary_plane_scaling) {
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx