[PATCH i-g-t] kms_flip_tiling: [linear, X]<->[Y, Yf] tiling changes are not allowed

[PATCH i-g-t] kms_flip_tiling: [linear, X]<->[Y, Yf] tiling changes are not allowed

[Tvrtko Ursulin]

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

This matches the behaviour in kernel patch
"drm/i915/skl: Disallow tiling changes during page flip".

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/kms_flip_tiling.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tests/kms_flip_tiling.c b/tests/kms_flip_tiling.c
index 8345505..3eef4cc 100644
--- a/tests/kms_flip_tiling.c
+++ b/tests/kms_flip_tiling.c
@@ -63,7 +63,7 @@ fill_linear_fb(struct igt_fb *fb, data_t *data, drmModeModeInfo *mode)
 }
 
 static void
-test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling)
+test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling, int expect)
 {
 	struct igt_fb linear, tiled;
 	drmModeModeInfo *mode;
@@ -107,13 +107,15 @@ test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling)
 	/* flip to the linear buffer */
 	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
 			      fb_id, 0, NULL);
-	igt_assert_eq(ret, 0);
+	igt_assert_eq(ret, expect);
 
-	igt_wait_for_vblank(data->drm_fd, pipe);
+	if (expect == 0) {
+		igt_wait_for_vblank(data->drm_fd, pipe);
 
-	/* get a crc and compare with the reference */
-	igt_pipe_crc_collect_crc(pipe_crc, &crc);
-	igt_assert_crc_equal(&reference_crc, &crc);
+		/* get a crc and compare with the reference */
+		igt_pipe_crc_collect_crc(pipe_crc, &crc);
+		igt_assert_crc_equal(&reference_crc, &crc);
+	}
 
 	/* clean up */
 	igt_plane_set_fb(primary, NULL);
@@ -145,7 +147,8 @@ igt_main
 	igt_subtest_f("flip-changes-tiling") {
 		for_each_connected_output(&data.display, output)
 			test_flip_changes_tiling(&data, output,
-						LOCAL_I915_FORMAT_MOD_X_TILED);
+						LOCAL_I915_FORMAT_MOD_X_TILED,
+						0);
 	}
 
 	igt_subtest_f("flip-changes-tiling-Y") {
@@ -154,7 +157,8 @@ igt_main
 
 		for_each_connected_output(&data.display, output)
 			test_flip_changes_tiling(&data, output,
-						LOCAL_I915_FORMAT_MOD_Y_TILED);
+						LOCAL_I915_FORMAT_MOD_Y_TILED,
+						-EINVAL);
 	}
 
 	igt_subtest_f("flip-changes-tiling-Yf") {
@@ -163,7 +167,8 @@ igt_main
 
 		for_each_connected_output(&data.display, output)
 			test_flip_changes_tiling(&data, output,
-						LOCAL_I915_FORMAT_MOD_Yf_TILED);
+						LOCAL_I915_FORMAT_MOD_Yf_TILED,
+						-EINVAL);
 	}
 
 	igt_fixture {
-- 
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] kms_flip_tiling: [linear, X]<->[Y, Yf] tiling changes are not allowed

[Chris Wilson]

On Wed, Apr 22, 2015 at 05:00:27PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> This matches the behaviour in kernel patch
> "drm/i915/skl: Disallow tiling changes during page flip".
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/kms_flip_tiling.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/kms_flip_tiling.c b/tests/kms_flip_tiling.c
> index 8345505..3eef4cc 100644
> --- a/tests/kms_flip_tiling.c
> +++ b/tests/kms_flip_tiling.c
> @@ -63,7 +63,7 @@ fill_linear_fb(struct igt_fb *fb, data_t *data, drmModeModeInfo *mode)
>  }
>  
>  static void
> -test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling)
> +test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling, int expect)
>  {
>  	struct igt_fb linear, tiled;
>  	drmModeModeInfo *mode;
> @@ -107,13 +107,15 @@ test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling)
>  	/* flip to the linear buffer */
>  	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
>  			      fb_id, 0, NULL);
> -	igt_assert_eq(ret, 0);
> +	igt_assert_eq(ret, expect);
>  
> -	igt_wait_for_vblank(data->drm_fd, pipe);
> +	if (expect == 0) {

I'd still accept ret == 0 since maybe one day it will work happily.

So perhaps:
if (ret) igt_assert_rq(ret, expect);
igt_require(ret == 0);
?

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] kms_flip_tiling: [linear, X]<->[Y, Yf] tiling changes are not allowed

[Tvrtko Ursulin]

Hi,

On 04/22/2015 05:07 PM, Chris Wilson wrote:
> On Wed, Apr 22, 2015 at 05:00:27PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> This matches the behaviour in kernel patch
>> "drm/i915/skl: Disallow tiling changes during page flip".
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   tests/kms_flip_tiling.c | 23 ++++++++++++++---------
>>   1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/kms_flip_tiling.c b/tests/kms_flip_tiling.c
>> index 8345505..3eef4cc 100644
>> --- a/tests/kms_flip_tiling.c
>> +++ b/tests/kms_flip_tiling.c
>> @@ -63,7 +63,7 @@ fill_linear_fb(struct igt_fb *fb, data_t *data, drmModeModeInfo *mode)
>>   }
>>
>>   static void
>> -test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling)
>> +test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling, int expect)
>>   {
>>   	struct igt_fb linear, tiled;
>>   	drmModeModeInfo *mode;
>> @@ -107,13 +107,15 @@ test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling)
>>   	/* flip to the linear buffer */
>>   	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
>>   			      fb_id, 0, NULL);
>> -	igt_assert_eq(ret, 0);
>> +	igt_assert_eq(ret, expect);
>>
>> -	igt_wait_for_vblank(data->drm_fd, pipe);
>> +	if (expect == 0) {
>
> I'd still accept ret == 0 since maybe one day it will work happily.
>
> So perhaps:
> if (ret) igt_assert_rq(ret, expect);
> igt_require(ret == 0);
> ?

Well would have to at least run the cleanup part before skipping, but 
even before that I am not sure. That means test would skip today, and 
who knows for how long in the future, and then soon no one kind of knows 
what that means - is it good or bad?

Regards,

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

Re: [PATCH i-g-t] kms_flip_tiling: [linear, X]<->[Y, Yf] tiling changes are not allowed

[Chris Wilson]

On Wed, Apr 22, 2015 at 05:23:11PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 04/22/2015 05:07 PM, Chris Wilson wrote:
> >On Wed, Apr 22, 2015 at 05:00:27PM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>This matches the behaviour in kernel patch
> >>"drm/i915/skl: Disallow tiling changes during page flip".
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>---
> >>  tests/kms_flip_tiling.c | 23 ++++++++++++++---------
> >>  1 file changed, 14 insertions(+), 9 deletions(-)
> >>
> >>diff --git a/tests/kms_flip_tiling.c b/tests/kms_flip_tiling.c
> >>index 8345505..3eef4cc 100644
> >>--- a/tests/kms_flip_tiling.c
> >>+++ b/tests/kms_flip_tiling.c
> >>@@ -63,7 +63,7 @@ fill_linear_fb(struct igt_fb *fb, data_t *data, drmModeModeInfo *mode)
> >>  }
> >>
> >>  static void
> >>-test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling)
> >>+test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling, int expect)
> >>  {
> >>  	struct igt_fb linear, tiled;
> >>  	drmModeModeInfo *mode;
> >>@@ -107,13 +107,15 @@ test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling)
> >>  	/* flip to the linear buffer */
> >>  	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
> >>  			      fb_id, 0, NULL);
> >>-	igt_assert_eq(ret, 0);
> >>+	igt_assert_eq(ret, expect);
> >>
> >>-	igt_wait_for_vblank(data->drm_fd, pipe);
> >>+	if (expect == 0) {
> >
> >I'd still accept ret == 0 since maybe one day it will work happily.
> >
> >So perhaps:
> >if (ret) igt_assert_rq(ret, expect);
> >igt_require(ret == 0);
> >?
> 
> Well would have to at least run the cleanup part before skipping,
> but even before that I am not sure. That means test would skip
> today, and who knows for how long in the future, and then soon no
> one kind of knows what that means - is it good or bad?

The other side of the coin is that the test implies such tiling changes
are verboten, which is just an implementation detail of today's kernel
and also not guaranteed to stay the same.

I'd rather see a test ready for checking Y-tiled flipping so that should
someone try and decide to run this test, they are not immediately
disappointed. Also it should show the bug in current kernels.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] kms_flip_tiling: [linear, X]<->[Y, Yf] tiling changes are not allowed

[Tvrtko Ursulin]

On 04/22/2015 05:45 PM, Chris Wilson wrote:
> On Wed, Apr 22, 2015 at 05:23:11PM +0100, Tvrtko Ursulin wrote:
>>
>> Hi,
>>
>> On 04/22/2015 05:07 PM, Chris Wilson wrote:
>>> On Wed, Apr 22, 2015 at 05:00:27PM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> This matches the behaviour in kernel patch
>>>> "drm/i915/skl: Disallow tiling changes during page flip".
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> ---
>>>>   tests/kms_flip_tiling.c | 23 ++++++++++++++---------
>>>>   1 file changed, 14 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/tests/kms_flip_tiling.c b/tests/kms_flip_tiling.c
>>>> index 8345505..3eef4cc 100644
>>>> --- a/tests/kms_flip_tiling.c
>>>> +++ b/tests/kms_flip_tiling.c
>>>> @@ -63,7 +63,7 @@ fill_linear_fb(struct igt_fb *fb, data_t *data, drmModeModeInfo *mode)
>>>>   }
>>>>
>>>>   static void
>>>> -test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling)
>>>> +test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling, int expect)
>>>>   {
>>>>   	struct igt_fb linear, tiled;
>>>>   	drmModeModeInfo *mode;
>>>> @@ -107,13 +107,15 @@ test_flip_changes_tiling(data_t *data, igt_output_t *output, uint64_t tiling)
>>>>   	/* flip to the linear buffer */
>>>>   	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
>>>>   			      fb_id, 0, NULL);
>>>> -	igt_assert_eq(ret, 0);
>>>> +	igt_assert_eq(ret, expect);
>>>>
>>>> -	igt_wait_for_vblank(data->drm_fd, pipe);
>>>> +	if (expect == 0) {
>>>
>>> I'd still accept ret == 0 since maybe one day it will work happily.
>>>
>>> So perhaps:
>>> if (ret) igt_assert_rq(ret, expect);
>>> igt_require(ret == 0);
>>> ?
>>
>> Well would have to at least run the cleanup part before skipping,
>> but even before that I am not sure. That means test would skip
>> today, and who knows for how long in the future, and then soon no
>> one kind of knows what that means - is it good or bad?
>
> The other side of the coin is that the test implies such tiling changes
> are verboten, which is just an implementation detail of today's kernel
> and also not guaranteed to stay the same.
>
> I'd rather see a test ready for checking Y-tiled flipping so that should
> someone try and decide to run this test, they are not immediately
> disappointed. Also it should show the bug in current kernels.

I hope to post those new test cases today.

I think drop this patch then, which will continue passing and new test 
cases will be failing until it is properly fixed. Combined together that 
shouldn't give any false ideas.

Regards,

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