| Message ID | 1430819603-492-3-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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? By the way, instead of this, did you look at the option of changing kms_rotation_crc to set plane position to whatever required when fb is changed.
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? Regards, Tvrtko
> -----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
On 05/08/2015 01:03 AM, Konduru, Chandra wrote: >> -----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. So r-b on patches 1-3 ? Regards, Tvrtko
> -----Original Message----- > From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] > Sent: Friday, May 08, 2015 1:36 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/08/2015 01:03 AM, Konduru, Chandra wrote: > > >> -----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. > > So r-b on patches 1-3 ? For patches 1-3: Reviewed-by: Chandra Konduru <chandra.konduru@intel.com> > > Regards, > > Tvrtko
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; + /* set default plane size as fb size */ plane->crtc_w = fb->width; plane->crtc_h = fb->height;