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

Message ID 1430819603-492-3-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin May 5, 2015, 9:53 a.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

    commit a26f9f9ad0e679c7ce413a25d34f6914e1174151
    Author: chandra konduru <chandra.konduru@intel.com>
    Date:   Mon Mar 30 13:52:04 2015 -0700

        i-g-t: Adding plane scaling test case

Started doing this and broke kms_rotation_crc.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: chandra konduru <chandra.konduru@intel.com>
---
 lib/igt_kms.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Chandra Konduru May 6, 2015, 7:02 p.m. UTC | #1
> 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.
Tvrtko Ursulin May 7, 2015, 9:15 a.m. UTC | #2
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
Chandra Konduru May 8, 2015, 12:03 a.m. UTC | #3
> -----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
Tvrtko Ursulin May 8, 2015, 8:36 a.m. UTC | #4
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
Chandra Konduru May 8, 2015, 5:57 p.m. UTC | #5
> -----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

Patch
diff mbox

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;