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

[Konduru, Chandra]

> -----Original Message-----
> From: Tvrtko Ursulin [mailto:tvrtko.ursulin at linux.intel.com]
> Sent: Friday, May 08, 2015 1:36 AM
> To: Konduru, Chandra; Intel-gfx at 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/08/2015 01:03 AM, Konduru, Chandra wrote:
> 
> >> -----Original Message-----
> >> From: Tvrtko Ursulin [mailto:tvrtko.ursulin at linux.intel.com]
> >> Sent: Thursday, May 07, 2015 2:15 AM
> >> To: Konduru, Chandra; Intel-gfx at 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.
> 
> So r-b on patches 1-3 ?

For patches 1-3:
Reviewed-by: Chandra Konduru <chandra.konduru at intel.com>

> 
> Regards,
> 
> Tvrtko