Re: [PATCH i-g-t 3/4] igt_kms: Do not reset plane position on assigning a fb

Konduru, Chandra
2015-05-08T00:03:52Z

> -----Original Message-----
> From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
> Sent: Thursday, May 07, 2015 2:15 AM
> To: Konduru, Chandra; Intel-gfx@lists.freedesktop.org
> Cc: Ursulin, Tvrtko
> Subject: Re: [PATCH i-g-t 3/4] igt_kms: Do not reset plane position on assigning
> a fb
>
>
> On 05/06/2015 08:02 PM, Konduru, Chandra wrote:
> >
> >> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index b5ba273..0665d70
> >> 100644
> >> --- a/lib/igt_kms.c
> >> +++ b/lib/igt_kms.c
> >> @@ -1765,9 +1765,7 @@ void igt_plane_set_fb(igt_plane_t *plane,
> >> struct igt_fb *fb)
> >> plane->fb = fb;
> >> /* hack to keep tests working that don't call igt_plane_set_size() */
> >> if (fb) {
> >> - /* set default plane pos/size as fb size */
> >> - plane->crtc_x = 0;
> >> - plane->crtc_y = 0;
> > Reason for doing this is to make existing kms tests to run without
> modification.
> > Otherwise every existing test has to explicitly call
> > igt_plane_set_position to make sure it works. Can you make sure removing
> above 2 lines doesn't break existing tests?
>
> Hm, before your patch tests could set a plane position, change the fb, and plane
> position would remain the same. After your patch changing the fb resets the
> plane position. My patch reverts this behaviour to old one.
>
> So I am not sure in what cases resetting plane position on fb changes helps and
> which tests?
>
> Which tests do you think should be run?
>
Ignore my previous comment, I was thinking that this change will
make plane->crtc_x/y uninitialized before calling igt_drm_plane_commit.
But igt_display_init() does memsets display which will initialize these to 0s.
So no initialization required as part here. Your changes are fine.

> Regards,
>
> Tvrtko