> -----Original Message----- > From: Tvrtko Ursulin [mailto:tvrtko.ursulin@xxxxxxxxxxxxxxx] > Sent: Friday, May 08, 2015 1:36 AM > To: Konduru, Chandra; Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx] > >> Sent: Thursday, May 07, 2015 2:15 AM > >> To: Konduru, Chandra; Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >> 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@xxxxxxxxx> > > Regards, > > Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
