[PATCH v2 i-g-t 1/4] igt_kms: Avoid NULL ptr deref when commiting disabled planes

[PATCH v2 i-g-t 1/4] igt_kms: Avoid NULL ptr deref when commiting disabled planes

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I think;

   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

introduced a condition where it attempts to update a disabled plane because
of the newly introduced size_changed flag which is set for disabled frame
buffers. Result is a NULL ptr deref in igt_drm_plane_commit (plane->fb->src_x).

Start recognising this case as disabled plane and act accordingly.

v2: Split out igt_plane_set_fb cleanup. (Thomas Wood)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: chandra konduru <chandra.konduru@intel.com>
Cc: Thomas Wood <thomas.wood@intel.com>
---
There might be a better fix, but this works for me.
---
 lib/igt_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index b7d1e90..33d437d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1331,7 +1331,7 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
 	fb_id = igt_plane_get_fb_id(plane);
 	crtc_id = output->config.crtc->crtc_id;
 
-	if (plane->fb_changed && fb_id == 0) {
+	if ((plane->fb_changed || plane->size_changed) && fb_id == 0) {
 		LOG(display,
 		    "%s: SetPlane pipe %s, plane %d, disabling\n",
 		    igt_output_name(output),
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 2/4] igt_kms: Merge condition in igt_plane_set_fb

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

There were two paths for fb and !fb.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: chandra konduru <chandra.konduru@intel.com>
Cc: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_kms.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 33d437d..b5ba273 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1765,14 +1765,6 @@ 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) {
-		plane->crtc_w = fb->width;
-		plane->crtc_h = fb->height;
-	} else {
-		plane->crtc_w = 0;
-		plane->crtc_h = 0;
-	}
-
-	if (fb) {
 		/* set default plane pos/size as fb size */
 		plane->crtc_x = 0;
 		plane->crtc_y = 0;
@@ -1784,6 +1776,9 @@ void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb)
 		fb->src_y = 0;
 		fb->src_w = fb->width;
 		fb->src_h = fb->height;
+	} else {
+		plane->crtc_w = 0;
+		plane->crtc_h = 0;
 	}
 
 	plane->fb_changed = true;
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/4] igt_kms: Merge condition in igt_plane_set_fb

[Konduru, Chandra]

> @@ -1765,14 +1765,6 @@ 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) {
> -		plane->crtc_w = fb->width;
> -		plane->crtc_h = fb->height;
> -	} else {
> -		plane->crtc_w = 0;
> -		plane->crtc_h = 0;
> -	}
> -
> -	if (fb) {
>  		/* set default plane pos/size as fb size */
>  		plane->crtc_x = 0;
>  		plane->crtc_y = 0;
> @@ -1784,6 +1776,9 @@ void igt_plane_set_fb(igt_plane_t *plane, struct
> igt_fb *fb)
>  		fb->src_y = 0;
>  		fb->src_w = fb->width;
>  		fb->src_h = fb->height;
> +	} else {
> +		plane->crtc_w = 0;
> +		plane->crtc_h = 0;
>  	}
Existing code is simply setting fb src position and plane crtc position to 0s (top left)
and src size as fb size and crtc size as plane size to start a fb with a plane. Then individual
test can change them to whatever fb position/size and plane position/size as it wants.
As I commented to 3/4 patch, if these initializations are removed, then all tests to be
updated to explicitly set them.

As a side note, is there any reason for having two patches 2/4 and 3/4 modifying
same lines of code instead of a single patch?

> 
>  	plane->fb_changed = true;
> --
> 2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/4] igt_kms: Merge condition in igt_plane_set_fb

[Tvrtko Ursulin]

On 05/06/2015 09:56 PM, Konduru, Chandra wrote:
>> @@ -1765,14 +1765,6 @@ 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) {
>> -		plane->crtc_w = fb->width;
>> -		plane->crtc_h = fb->height;
>> -	} else {
>> -		plane->crtc_w = 0;
>> -		plane->crtc_h = 0;
>> -	}
>> -
>> -	if (fb) {
>>   		/* set default plane pos/size as fb size */
>>   		plane->crtc_x = 0;
>>   		plane->crtc_y = 0;
>> @@ -1784,6 +1776,9 @@ void igt_plane_set_fb(igt_plane_t *plane, struct
>> igt_fb *fb)
>>   		fb->src_y = 0;
>>   		fb->src_w = fb->width;
>>   		fb->src_h = fb->height;
>> +	} else {
>> +		plane->crtc_w = 0;
>> +		plane->crtc_h = 0;
>>   	}
> Existing code is simply setting fb src position and plane crtc position to 0s (top left)
> and src size as fb size and crtc size as plane size to start a fb with a plane. Then individual
> test can change them to whatever fb position/size and plane position/size as it wants.
> As I commented to 3/4 patch, if these initializations are removed, then all tests to be
> updated to explicitly set them.

Not sure what you mean. I simply cleaned two "if (fb)" conditions one 
after another, into one. No functional changes.

> As a side note, is there any reason for having two patches 2/4 and 3/4 modifying
> same lines of code instead of a single patch?

Because this is just a code cleanup and the other was a functional 
change. And because it doesn't matter - lets not spend hours going back 
and forth on trivial IGT fixes.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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(-)

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;
 
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Konduru, Chandra]

> 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.


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 4/4] kms_plane_scaling: Find the image regardless how the test is run

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

As it stands running the test like "sudo tests/kms_plane_scaling"
does not work.

Fix it by using the same method igt_paint_image uses.

v2: Export Cairo read callback instead of duplicating it. (Thomas Wood)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: chandra konduru <chandra.konduru@intel.com>
Cc: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_fb.c              | 17 ++++++++++++++---
 lib/igt_fb.h              |  2 ++
 tests/kms_plane_scaling.c |  7 ++++++-
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index cc4b8ee..2416e76 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -338,8 +338,18 @@ void igt_paint_test_pattern(cairo_t *cr, int width, int height)
 	igt_assert(!cairo_status(cr));
 }
 
-static cairo_status_t
-stdio_read_func(void *closure, unsigned char* data, unsigned int size)
+/**
+ * igt_cairo_read_func:
+ * @closure: callback closure parameter
+ * @data: callback data parameter
+ * @size: callback size parameter
+ *
+ * Read callback for cairo_image_surface_create_from_png_stream to be used
+ * in conjuction with igt_fopen_data in order to open data files from IGT
+ * standard locations.
+ */
+cairo_status_t
+igt_cairo_read_func(void *closure, unsigned char* data, unsigned int size)
 {
 	if (fread(data, 1, size, (FILE*)closure) != size)
 		return CAIRO_STATUS_READ_ERROR;
@@ -369,7 +379,8 @@ void igt_paint_image(cairo_t *cr, const char *filename,
 
 	f = igt_fopen_data(filename);
 
-	image = cairo_image_surface_create_from_png_stream(&stdio_read_func, f);
+	image = cairo_image_surface_create_from_png_stream(&igt_cairo_read_func,
+							   f);
 	igt_assert(cairo_surface_status(image) == CAIRO_STATUS_SUCCESS);
 
 	img_width = cairo_image_surface_get_width(image);
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index a07acd2..2da5f0c 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -98,6 +98,8 @@ void igt_write_fb_to_png(int fd, struct igt_fb *fb, const char *filename);
 int igt_cairo_printf_line(cairo_t *cr, enum igt_text_align align,
 			       double yspacing, const char *fmt, ...)
 			       __attribute__((format (printf, 4, 5)));
+cairo_status_t
+igt_cairo_read_func(void *closure, unsigned char* data, unsigned int size);
 
 /* helpers to handle drm fourcc codes */
 uint32_t igt_bpp_depth_to_drm_format(int bpp, int depth);
diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
index 00db5cb..272f759 100644
--- a/tests/kms_plane_scaling.c
+++ b/tests/kms_plane_scaling.c
@@ -210,6 +210,7 @@ static void test_plane_scaling(data_t *d)
 	enum pipe pipe;
 	int valid_tests = 0;
 	int primary_plane_scaling = 0; /* For now */
+	FILE* f;
 
 	igt_require(d->display.has_universal_planes);
 	igt_require(d->num_scalers);
@@ -223,11 +224,15 @@ static void test_plane_scaling(data_t *d)
 		mode = igt_output_get_mode(output);
 
 		/* allocate fb2 with image size */
-		image = cairo_image_surface_create_from_png(FILE_NAME);
+
+		f = igt_fopen_data(FILE_NAME);
+		image = cairo_image_surface_create_from_png_stream(
+						&igt_cairo_read_func, f);
 		igt_assert(cairo_surface_status(image) == CAIRO_STATUS_SUCCESS);
 		d->image_w = cairo_image_surface_get_width(image);
 		d->image_h = cairo_image_surface_get_height(image);
 		cairo_surface_destroy(image);
+		fclose(f);
 
 		d->fb_id2 = igt_create_fb(d->drm_fd,
 				d->image_w, d->image_h,
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 4/4] kms_plane_scaling: Find the image regardless how the test is run

[Konduru, Chandra]

> -----Original Message-----
> From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
> Sent: Tuesday, May 05, 2015 2:53 AM
> To: Intel-gfx@lists.freedesktop.org
> Cc: Ursulin, Tvrtko; Konduru, Chandra; Wood, Thomas
> Subject: [PATCH i-g-t 4/4] kms_plane_scaling: Find the image regardless how the
> test is run
> 
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> As it stands running the test like "sudo tests/kms_plane_scaling"
> does not work.
> 
> Fix it by using the same method igt_paint_image uses.
As image size is required in other tests too, in recent patch
I added igt_get_image_size() which can be used here
instead duplication.

> 
> v2: Export Cairo read callback instead of duplicating it. (Thomas Wood)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: chandra konduru <chandra.konduru@intel.com>
> Cc: Thomas Wood <thomas.wood@intel.com>
> ---
>  lib/igt_fb.c              | 17 ++++++++++++++---
>  lib/igt_fb.h              |  2 ++
>  tests/kms_plane_scaling.c |  7 ++++++-
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c index cc4b8ee..2416e76 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -338,8 +338,18 @@ void igt_paint_test_pattern(cairo_t *cr, int width, int
> height)
>  	igt_assert(!cairo_status(cr));
>  }
> 
> -static cairo_status_t
> -stdio_read_func(void *closure, unsigned char* data, unsigned int size)
> +/**
> + * igt_cairo_read_func:
> + * @closure: callback closure parameter
> + * @data: callback data parameter
> + * @size: callback size parameter
> + *
> + * Read callback for cairo_image_surface_create_from_png_stream to be
> +used
> + * in conjuction with igt_fopen_data in order to open data files from
> +IGT
> + * standard locations.
> + */
> +cairo_status_t
> +igt_cairo_read_func(void *closure, unsigned char* data, unsigned int
> +size)
>  {
>  	if (fread(data, 1, size, (FILE*)closure) != size)
>  		return CAIRO_STATUS_READ_ERROR;
> @@ -369,7 +379,8 @@ void igt_paint_image(cairo_t *cr, const char *filename,
> 
>  	f = igt_fopen_data(filename);
> 
> -	image =
> cairo_image_surface_create_from_png_stream(&stdio_read_func, f);
> +	image =
> cairo_image_surface_create_from_png_stream(&igt_cairo_read_func,
> +							   f);
>  	igt_assert(cairo_surface_status(image) == CAIRO_STATUS_SUCCESS);
> 
>  	img_width = cairo_image_surface_get_width(image);
> diff --git a/lib/igt_fb.h b/lib/igt_fb.h index a07acd2..2da5f0c 100644
> --- a/lib/igt_fb.h
> +++ b/lib/igt_fb.h
> @@ -98,6 +98,8 @@ void igt_write_fb_to_png(int fd, struct igt_fb *fb, const
> char *filename);  int igt_cairo_printf_line(cairo_t *cr, enum igt_text_align align,
>  			       double yspacing, const char *fmt, ...)
>  			       __attribute__((format (printf, 4, 5)));
> +cairo_status_t
> +igt_cairo_read_func(void *closure, unsigned char* data, unsigned int
> +size);
> 
>  /* helpers to handle drm fourcc codes */  uint32_t
> igt_bpp_depth_to_drm_format(int bpp, int depth); diff --git
> a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c index
> 00db5cb..272f759 100644
> --- a/tests/kms_plane_scaling.c
> +++ b/tests/kms_plane_scaling.c
> @@ -210,6 +210,7 @@ static void test_plane_scaling(data_t *d)
>  	enum pipe pipe;
>  	int valid_tests = 0;
>  	int primary_plane_scaling = 0; /* For now */
> +	FILE* f;
> 
>  	igt_require(d->display.has_universal_planes);
>  	igt_require(d->num_scalers);
> @@ -223,11 +224,15 @@ static void test_plane_scaling(data_t *d)
>  		mode = igt_output_get_mode(output);
> 
>  		/* allocate fb2 with image size */
> -		image = cairo_image_surface_create_from_png(FILE_NAME);
> +
> +		f = igt_fopen_data(FILE_NAME);
> +		image = cairo_image_surface_create_from_png_stream(
> +						&igt_cairo_read_func, f);
>  		igt_assert(cairo_surface_status(image) ==
> CAIRO_STATUS_SUCCESS);
>  		d->image_w = cairo_image_surface_get_width(image);
>  		d->image_h = cairo_image_surface_get_height(image);
>  		cairo_surface_destroy(image);
> +		fclose(f);

Above can be replaced with
	igt_get_image_size(FILE_NAME, &d->image_w, &d->image_h);

> 
>  		d->fb_id2 = igt_create_fb(d->drm_fd,
>  				d->image_w, d->image_h,
> --
> 2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 4/4] kms_plane_scaling: Find the image regardless how the test is run

[Tvrtko Ursulin]

On 05/05/2015 06:22 PM, Konduru, Chandra wrote:
>> -----Original Message-----
>> From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
>> Sent: Tuesday, May 05, 2015 2:53 AM
>> To: Intel-gfx@lists.freedesktop.org
>> Cc: Ursulin, Tvrtko; Konduru, Chandra; Wood, Thomas
>> Subject: [PATCH i-g-t 4/4] kms_plane_scaling: Find the image regardless how the
>> test is run
>>
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> As it stands running the test like "sudo tests/kms_plane_scaling"
>> does not work.
>>
>> Fix it by using the same method igt_paint_image uses.
> As image size is required in other tests too, in recent patch
> I added igt_get_image_size() which can be used here
> instead duplication.

Ok, please do, I only attempted to fix it along the way since it did not 
run for me. And at the time there was no igt_get_image_size.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 4/4] kms_plane_scaling: Find the image regardless how the test is run

[Konduru, Chandra]

> -----Original Message-----
> From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
> Sent: Wednesday, May 06, 2015 2:29 AM
> To: Konduru, Chandra; Intel-gfx@lists.freedesktop.org
> Cc: Ursulin, Tvrtko; Wood, Thomas
> Subject: Re: [PATCH i-g-t 4/4] kms_plane_scaling: Find the image regardless how
> the test is run
> 
> 
> On 05/05/2015 06:22 PM, Konduru, Chandra wrote:
> >> -----Original Message-----
> >> From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
> >> Sent: Tuesday, May 05, 2015 2:53 AM
> >> To: Intel-gfx@lists.freedesktop.org
> >> Cc: Ursulin, Tvrtko; Konduru, Chandra; Wood, Thomas
> >> Subject: [PATCH i-g-t 4/4] kms_plane_scaling: Find the image
> >> regardless how the test is run
> >>
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> As it stands running the test like "sudo tests/kms_plane_scaling"
> >> does not work.
> >>
> >> Fix it by using the same method igt_paint_image uses.
> > As image size is required in other tests too, in recent patch I added
> > igt_get_image_size() which can be used here instead duplication.
> 
> Ok, please do, I only attempted to fix it along the way since it did not run for me.
> And at the time there was no igt_get_image_size.
Yeah, you are right. Sure I have few things to changes to do anyway for adding rotation to kms_plane_scaling.
Along with that, I will make above change. In that case, I presume you drop this patch.
> 
> Regards,
> 
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 4/4] kms_plane_scaling: Find the image regardless how the test is run

[Tvrtko Ursulin]

On 05/06/2015 07:17 PM, Konduru, Chandra wrote:
>> -----Original Message-----
>> From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
>> Sent: Wednesday, May 06, 2015 2:29 AM
>> To: Konduru, Chandra; Intel-gfx@lists.freedesktop.org
>> Cc: Ursulin, Tvrtko; Wood, Thomas
>> Subject: Re: [PATCH i-g-t 4/4] kms_plane_scaling: Find the image regardless how
>> the test is run
>>
>>
>> On 05/05/2015 06:22 PM, Konduru, Chandra wrote:
>>>> -----Original Message-----
>>>> From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
>>>> Sent: Tuesday, May 05, 2015 2:53 AM
>>>> To: Intel-gfx@lists.freedesktop.org
>>>> Cc: Ursulin, Tvrtko; Konduru, Chandra; Wood, Thomas
>>>> Subject: [PATCH i-g-t 4/4] kms_plane_scaling: Find the image
>>>> regardless how the test is run
>>>>
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> As it stands running the test like "sudo tests/kms_plane_scaling"
>>>> does not work.
>>>>
>>>> Fix it by using the same method igt_paint_image uses.
>>> As image size is required in other tests too, in recent patch I added
>>> igt_get_image_size() which can be used here instead duplication.
>>
>> Ok, please do, I only attempted to fix it along the way since it did not run for me.
>> And at the time there was no igt_get_image_size.
> Yeah, you are right. Sure I have few things to changes to do anyway for adding rotation to kms_plane_scaling.
> Along with that, I will make above change. In that case, I presume you drop this patch.

Dropping happily.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 i-g-t 1/4] igt_kms: Avoid NULL ptr deref when commiting disabled planes

[Konduru, Chandra]

> -----Original Message-----
> From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
> Sent: Tuesday, May 05, 2015 2:53 AM
> To: Intel-gfx@lists.freedesktop.org
> Cc: Ursulin, Tvrtko; Konduru, Chandra; Wood, Thomas
> Subject: [PATCH v2 i-g-t 1/4] igt_kms: Avoid NULL ptr deref when commiting
> disabled planes
> 
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> I think;
> 
>    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
> 
> introduced a condition where it attempts to update a disabled plane because of
> the newly introduced size_changed flag which is set for disabled frame buffers.
> Result is a NULL ptr deref in igt_drm_plane_commit (plane->fb->src_x).
> 
> Start recognising this case as disabled plane and act accordingly.
> 
> v2: Split out igt_plane_set_fb cleanup. (Thomas Wood)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: chandra konduru <chandra.konduru@intel.com>
> Cc: Thomas Wood <thomas.wood@intel.com>
> ---
> There might be a better fix, but this works for me.
> ---
>  lib/igt_kms.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index b7d1e90..33d437d 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1331,7 +1331,7 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
>  	fb_id = igt_plane_get_fb_id(plane);
>  	crtc_id = output->config.crtc->crtc_id;
> 
> -	if (plane->fb_changed && fb_id == 0) {
> +	if ((plane->fb_changed || plane->size_changed) && fb_id == 0) {

Shouldn't this include plane->position_changed too? Like:
	if ((plane->fb_changed || plane->size_changed || plane->position_changed) && fb_id == 0) {

>  		LOG(display,
>  		    "%s: SetPlane pipe %s, plane %d, disabling\n",
>  		    igt_output_name(output),
> --
> 2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 i-g-t 1/4] igt_kms: Avoid NULL ptr deref when commiting disabled planes

[Tvrtko Ursulin]

On 05/06/2015 09:47 PM, Konduru, Chandra wrote:
>> -----Original Message-----
>> From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
>> Sent: Tuesday, May 05, 2015 2:53 AM
>> To: Intel-gfx@lists.freedesktop.org
>> Cc: Ursulin, Tvrtko; Konduru, Chandra; Wood, Thomas
>> Subject: [PATCH v2 i-g-t 1/4] igt_kms: Avoid NULL ptr deref when commiting
>> disabled planes
>>
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> I think;
>>
>>     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
>>
>> introduced a condition where it attempts to update a disabled plane because of
>> the newly introduced size_changed flag which is set for disabled frame buffers.
>> Result is a NULL ptr deref in igt_drm_plane_commit (plane->fb->src_x).
>>
>> Start recognising this case as disabled plane and act accordingly.
>>
>> v2: Split out igt_plane_set_fb cleanup. (Thomas Wood)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: chandra konduru <chandra.konduru@intel.com>
>> Cc: Thomas Wood <thomas.wood@intel.com>
>> ---
>> There might be a better fix, but this works for me.
>> ---
>>   lib/igt_kms.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index b7d1e90..33d437d 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -1331,7 +1331,7 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
>>   	fb_id = igt_plane_get_fb_id(plane);
>>   	crtc_id = output->config.crtc->crtc_id;
>>
>> -	if (plane->fb_changed && fb_id == 0) {
>> +	if ((plane->fb_changed || plane->size_changed) && fb_id == 0) {
>
> Shouldn't this include plane->position_changed too? Like:
> 	if ((plane->fb_changed || plane->size_changed || plane->position_changed) && fb_id == 0) {

When you added size_changed, state for position_changed and fb == NULL 
remained the same, while size_changed added new state for size_changed 
== true and fb == NULL. So I added handling for that and did not think 
much beyond it. It fixes a segfault so I moved on. Or in other words, I 
don't see how it would harm to merge this, it doesn't make anything worse.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 i-g-t 1/4] igt_kms: Avoid NULL ptr deref when commiting disabled planes

[Konduru, Chandra]

> >> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index b7d1e90..33d437d
> >> 100644
> >> --- a/lib/igt_kms.c
> >> +++ b/lib/igt_kms.c
> >> @@ -1331,7 +1331,7 @@ static int igt_drm_plane_commit(igt_plane_t
> *plane,
> >>   	fb_id = igt_plane_get_fb_id(plane);
> >>   	crtc_id = output->config.crtc->crtc_id;
> >>
> >> -	if (plane->fb_changed && fb_id == 0) {
> >> +	if ((plane->fb_changed || plane->size_changed) && fb_id == 0) {
> >
> > Shouldn't this include plane->position_changed too? Like:
> > 	if ((plane->fb_changed || plane->size_changed ||
> > plane->position_changed) && fb_id == 0) {
> 
> When you added size_changed, state for position_changed and fb == NULL
> remained the same, while size_changed added new state for size_changed ==
> true and fb == NULL. So I added handling for that and did not think much beyond
> it. It fixes a segfault so I moved on. Or in other words, I don't see how it would
> harm to merge this, it doesn't make anything worse.
Agree it doesn't harm to merge. 
> 
> Regards,
> 
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx