Re: [Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
On 11 November 2016 at 12:33, Tvrtko Ursulin
wrote:
>
> On 11/11/2016 11:23, Tomeu Vizoso wrote:
>>
>> On 11/10/2016 05:23 PM, Tvrtko Ursulin wrote:
>>>
>>>
>>> On 10/11/2016 13:17, Tomeu Vizoso wrote:
On 1 November 2016 at 16:44, Tvrtko Ursulin
wrote:
>
>
> Hi,
>
>
>
> On 02/03/16 14:00, Tomeu Vizoso wrote:
>>
>>
>> igt_create_bo_with_dimensions() is intended to abstract differences
>> between drivers in buffer object creation.
>>
>> The driver-specific ioctls will be called if the driver that is being
>> tested can satisfy the needs of the calling subtest, or it will be
>> skipped otherwise.
>>
>> Signed-off-by: Tomeu Vizoso
>> ---
>>
>> lib/igt_fb.c | 83
>> ++--
>> lib/igt_fb.h | 6 +
>> 2 files changed, 65 insertions(+), 24 deletions(-)
>>
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index cd1605308308..0a3526f4e4ea 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int
>> height,
>> int bpp, uint64_t tiling,
>> *size_ret = size;
>> }
>>
>> +int igt_create_bo_with_dimensions(int fd, int width, int height,
>> + uint32_t format, uint64_t modifier,
>> + unsigned stride, unsigned *size_ret,
>> + unsigned *stride_ret, bool *is_dumb)
>> +{
>> + int bpp = igt_drm_format_to_bpp(format);
>> + int bo;
>> +
>> + igt_assert((modifier && stride) || (!modifier && !stride));
>> +
>> + if (modifier) {
>> + unsigned size, calculated_stride;
>> +
>> + igt_calc_fb_size(fd, width, height, bpp, modifier,
>> &size,
>> +&calculated_stride);
>> + if (stride == 0)
>> + stride = calculated_stride;
>> +
>> + if (is_dumb)
>> + *is_dumb = false;
>> +
>> + if (is_i915_device(fd)) {
>> +
>> + bo = gem_create(fd, size);
>> + gem_set_tiling(fd, bo, modifier, stride);
>
>
>
> This is broken, gem_set_tiling does not take a fb modifier but an
> object
> tiling mode.
>
> You can demonstrate the failure if you got a Skylake system with:
>
> tests/kms_flip_tiling --r flip-changes-tiling-Yf
Hi,
that subtest fails occasionally here due to CRC issues, but the one
that fails due to the tiling constant passed to gem_set_tiling is
flip-Yf-tiled.
Have fixed it by converting the modifier to a tiling mode, but I also
needed to make sure that we don't pass to the kernel the Yf or Ys
constants as the kernel doesn't know about those.
Both fixes have been pushed already.
>>>
>>>
>>> With the two patches you pushed flip-changes-tiling-Yf is still broken
>>> due the tiling mode mismatch, not the CRC.
>>>
>>> Perhaps you tested with all three patches you posted today?
>>
>>
>> Just before pushing I tested with this:
>>
>> IGT-Version: 1.16-g050c00d (x86_64) (Linux: 4.5.5-300.fc24.x86_64 x86_64)
>>
>> So I can only think of a difference in the hw causing a different
>> codepath to execute (it's a i3-6100U), or a difference in the kernel.
>>
>> This is a remote box and I don't have yet all the infrastructure in
>> place so I could monitor the boot, nor power cycle it, so I cannot test
>> with a newer kernel yet.
>>
>> If you can confirm that we should be passing I915_TILING_NONE to
>> set_tiling when the FB has I915_FORMAT_MOD_Yf_TILED, I can fix that, but
>> I would also like to understand why the test is failing for you.
>
>
> It should definitely be I915_TILING_NONE. If you look at the i915 code,
> intel_display.c/intel_framebuffer_init will reject attempts to add a
> framebuffer where object tiling does not match the framebuffer modifier. And
> the intel_fb_modifier_to_tiling helper translates the Yf/Ys modifiers to
> NONE tiling.
Cool, will do that change now.
> So I have no idea how it could have possibly worked for you. :)
Yeah, I'm curious as well, will check in a bit.
Thanks,
Tomeu
>
> Regards,
>
> Tvrtko
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
On 11/11/2016 11:23, Tomeu Vizoso wrote:
On 11/10/2016 05:23 PM, Tvrtko Ursulin wrote:
On 10/11/2016 13:17, Tomeu Vizoso wrote:
On 1 November 2016 at 16:44, Tvrtko Ursulin wrote:
Hi,
On 02/03/16 14:00, Tomeu Vizoso wrote:
igt_create_bo_with_dimensions() is intended to abstract differences
between drivers in buffer object creation.
The driver-specific ioctls will be called if the driver that is being
tested can satisfy the needs of the calling subtest, or it will be
skipped otherwise.
Signed-off-by: Tomeu Vizoso
---
lib/igt_fb.c | 83
++--
lib/igt_fb.h | 6 +
2 files changed, 65 insertions(+), 24 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index cd1605308308..0a3526f4e4ea 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
int bpp, uint64_t tiling,
*size_ret = size;
}
+int igt_create_bo_with_dimensions(int fd, int width, int height,
+ uint32_t format, uint64_t modifier,
+ unsigned stride, unsigned *size_ret,
+ unsigned *stride_ret, bool *is_dumb)
+{
+ int bpp = igt_drm_format_to_bpp(format);
+ int bo;
+
+ igt_assert((modifier && stride) || (!modifier && !stride));
+
+ if (modifier) {
+ unsigned size, calculated_stride;
+
+ igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
+&calculated_stride);
+ if (stride == 0)
+ stride = calculated_stride;
+
+ if (is_dumb)
+ *is_dumb = false;
+
+ if (is_i915_device(fd)) {
+
+ bo = gem_create(fd, size);
+ gem_set_tiling(fd, bo, modifier, stride);
This is broken, gem_set_tiling does not take a fb modifier but an object
tiling mode.
You can demonstrate the failure if you got a Skylake system with:
tests/kms_flip_tiling --r flip-changes-tiling-Yf
Hi,
that subtest fails occasionally here due to CRC issues, but the one
that fails due to the tiling constant passed to gem_set_tiling is
flip-Yf-tiled.
Have fixed it by converting the modifier to a tiling mode, but I also
needed to make sure that we don't pass to the kernel the Yf or Ys
constants as the kernel doesn't know about those.
Both fixes have been pushed already.
With the two patches you pushed flip-changes-tiling-Yf is still broken
due the tiling mode mismatch, not the CRC.
Perhaps you tested with all three patches you posted today?
Just before pushing I tested with this:
IGT-Version: 1.16-g050c00d (x86_64) (Linux: 4.5.5-300.fc24.x86_64 x86_64)
So I can only think of a difference in the hw causing a different
codepath to execute (it's a i3-6100U), or a difference in the kernel.
This is a remote box and I don't have yet all the infrastructure in
place so I could monitor the boot, nor power cycle it, so I cannot test
with a newer kernel yet.
If you can confirm that we should be passing I915_TILING_NONE to
set_tiling when the FB has I915_FORMAT_MOD_Yf_TILED, I can fix that, but
I would also like to understand why the test is failing for you.
It should definitely be I915_TILING_NONE. If you look at the i915 code,
intel_display.c/intel_framebuffer_init will reject attempts to add a
framebuffer where object tiling does not match the framebuffer modifier.
And the intel_fb_modifier_to_tiling helper translates the Yf/Ys
modifiers to NONE tiling.
So I have no idea how it could have possibly worked for you. :)
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
On 11/10/2016 05:23 PM, Tvrtko Ursulin wrote:
>
> On 10/11/2016 13:17, Tomeu Vizoso wrote:
>> On 1 November 2016 at 16:44, Tvrtko Ursulin wrote:
>>>
>>> Hi,
>>>
>>>
>>>
>>> On 02/03/16 14:00, Tomeu Vizoso wrote:
igt_create_bo_with_dimensions() is intended to abstract differences
between drivers in buffer object creation.
The driver-specific ioctls will be called if the driver that is being
tested can satisfy the needs of the calling subtest, or it will be
skipped otherwise.
Signed-off-by: Tomeu Vizoso
---
lib/igt_fb.c | 83
++--
lib/igt_fb.h | 6 +
2 files changed, 65 insertions(+), 24 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index cd1605308308..0a3526f4e4ea 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
int bpp, uint64_t tiling,
*size_ret = size;
}
+int igt_create_bo_with_dimensions(int fd, int width, int height,
+ uint32_t format, uint64_t modifier,
+ unsigned stride, unsigned *size_ret,
+ unsigned *stride_ret, bool *is_dumb)
+{
+ int bpp = igt_drm_format_to_bpp(format);
+ int bo;
+
+ igt_assert((modifier && stride) || (!modifier && !stride));
+
+ if (modifier) {
+ unsigned size, calculated_stride;
+
+ igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
+&calculated_stride);
+ if (stride == 0)
+ stride = calculated_stride;
+
+ if (is_dumb)
+ *is_dumb = false;
+
+ if (is_i915_device(fd)) {
+
+ bo = gem_create(fd, size);
+ gem_set_tiling(fd, bo, modifier, stride);
>>>
>>>
>>> This is broken, gem_set_tiling does not take a fb modifier but an object
>>> tiling mode.
>>>
>>> You can demonstrate the failure if you got a Skylake system with:
>>>
>>> tests/kms_flip_tiling --r flip-changes-tiling-Yf
>>
>> Hi,
>>
>> that subtest fails occasionally here due to CRC issues, but the one
>> that fails due to the tiling constant passed to gem_set_tiling is
>> flip-Yf-tiled.
>>
>> Have fixed it by converting the modifier to a tiling mode, but I also
>> needed to make sure that we don't pass to the kernel the Yf or Ys
>> constants as the kernel doesn't know about those.
>>
>> Both fixes have been pushed already.
>
> With the two patches you pushed flip-changes-tiling-Yf is still broken
> due the tiling mode mismatch, not the CRC.
>
> Perhaps you tested with all three patches you posted today?
Just before pushing I tested with this:
IGT-Version: 1.16-g050c00d (x86_64) (Linux: 4.5.5-300.fc24.x86_64 x86_64)
So I can only think of a difference in the hw causing a different
codepath to execute (it's a i3-6100U), or a difference in the kernel.
This is a remote box and I don't have yet all the infrastructure in
place so I could monitor the boot, nor power cycle it, so I cannot test
with a newer kernel yet.
If you can confirm that we should be passing I915_TILING_NONE to
set_tiling when the FB has I915_FORMAT_MOD_Yf_TILED, I can fix that, but
I would also like to understand why the test is failing for you.
Regards,
Tomeu
>>> I would like to be able to tell you that all subtests there should pass,
>>> since they have been at some point, but I have a feeling that there are
>>> other breakages affecting it these days. :(
>>
>> Yeah, at least I can say that they are passing now in this particular
>> machine (i3-6100U).
>>
>> To make such issues less likely in the future, I have sent a patch
>> that makes clear when a variable contains a fb modifier constant, and
>> when a tiling mode. Haven't pushed it because I'm not 100% sure it's
>> worth it, so I would appreciate any opinions.
>
> I will look at that one later.
>
> Regards,
>
> Tvrtko
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
On 10/11/2016 13:17, Tomeu Vizoso wrote:
On 1 November 2016 at 16:44, Tvrtko Ursulin wrote:
Hi,
On 02/03/16 14:00, Tomeu Vizoso wrote:
igt_create_bo_with_dimensions() is intended to abstract differences
between drivers in buffer object creation.
The driver-specific ioctls will be called if the driver that is being
tested can satisfy the needs of the calling subtest, or it will be
skipped otherwise.
Signed-off-by: Tomeu Vizoso
---
lib/igt_fb.c | 83
++--
lib/igt_fb.h | 6 +
2 files changed, 65 insertions(+), 24 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index cd1605308308..0a3526f4e4ea 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
int bpp, uint64_t tiling,
*size_ret = size;
}
+int igt_create_bo_with_dimensions(int fd, int width, int height,
+ uint32_t format, uint64_t modifier,
+ unsigned stride, unsigned *size_ret,
+ unsigned *stride_ret, bool *is_dumb)
+{
+ int bpp = igt_drm_format_to_bpp(format);
+ int bo;
+
+ igt_assert((modifier && stride) || (!modifier && !stride));
+
+ if (modifier) {
+ unsigned size, calculated_stride;
+
+ igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
+&calculated_stride);
+ if (stride == 0)
+ stride = calculated_stride;
+
+ if (is_dumb)
+ *is_dumb = false;
+
+ if (is_i915_device(fd)) {
+
+ bo = gem_create(fd, size);
+ gem_set_tiling(fd, bo, modifier, stride);
This is broken, gem_set_tiling does not take a fb modifier but an object
tiling mode.
You can demonstrate the failure if you got a Skylake system with:
tests/kms_flip_tiling --r flip-changes-tiling-Yf
Hi,
that subtest fails occasionally here due to CRC issues, but the one
that fails due to the tiling constant passed to gem_set_tiling is
flip-Yf-tiled.
Have fixed it by converting the modifier to a tiling mode, but I also
needed to make sure that we don't pass to the kernel the Yf or Ys
constants as the kernel doesn't know about those.
Both fixes have been pushed already.
With the two patches you pushed flip-changes-tiling-Yf is still broken
due the tiling mode mismatch, not the CRC.
Perhaps you tested with all three patches you posted today?
I would like to be able to tell you that all subtests there should pass,
since they have been at some point, but I have a feeling that there are
other breakages affecting it these days. :(
Yeah, at least I can say that they are passing now in this particular
machine (i3-6100U).
To make such issues less likely in the future, I have sent a patch
that makes clear when a variable contains a fb modifier constant, and
when a tiling mode. Haven't pushed it because I'm not 100% sure it's
worth it, so I would appreciate any opinions.
I will look at that one later.
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
On 1 November 2016 at 16:44, Tvrtko Ursulin wrote:
>
> Hi,
>
>
>
> On 02/03/16 14:00, Tomeu Vizoso wrote:
>>
>> igt_create_bo_with_dimensions() is intended to abstract differences
>> between drivers in buffer object creation.
>>
>> The driver-specific ioctls will be called if the driver that is being
>> tested can satisfy the needs of the calling subtest, or it will be
>> skipped otherwise.
>>
>> Signed-off-by: Tomeu Vizoso
>> ---
>>
>> lib/igt_fb.c | 83
>> ++--
>> lib/igt_fb.h | 6 +
>> 2 files changed, 65 insertions(+), 24 deletions(-)
>>
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index cd1605308308..0a3526f4e4ea 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
>> int bpp, uint64_t tiling,
>> *size_ret = size;
>> }
>>
>> +int igt_create_bo_with_dimensions(int fd, int width, int height,
>> + uint32_t format, uint64_t modifier,
>> + unsigned stride, unsigned *size_ret,
>> + unsigned *stride_ret, bool *is_dumb)
>> +{
>> + int bpp = igt_drm_format_to_bpp(format);
>> + int bo;
>> +
>> + igt_assert((modifier && stride) || (!modifier && !stride));
>> +
>> + if (modifier) {
>> + unsigned size, calculated_stride;
>> +
>> + igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
>> +&calculated_stride);
>> + if (stride == 0)
>> + stride = calculated_stride;
>> +
>> + if (is_dumb)
>> + *is_dumb = false;
>> +
>> + if (is_i915_device(fd)) {
>> +
>> + bo = gem_create(fd, size);
>> + gem_set_tiling(fd, bo, modifier, stride);
>
>
> This is broken, gem_set_tiling does not take a fb modifier but an object
> tiling mode.
>
> You can demonstrate the failure if you got a Skylake system with:
>
> tests/kms_flip_tiling --r flip-changes-tiling-Yf
Hi,
that subtest fails occasionally here due to CRC issues, but the one
that fails due to the tiling constant passed to gem_set_tiling is
flip-Yf-tiled.
Have fixed it by converting the modifier to a tiling mode, but I also
needed to make sure that we don't pass to the kernel the Yf or Ys
constants as the kernel doesn't know about those.
Both fixes have been pushed already.
> I would like to be able to tell you that all subtests there should pass,
> since they have been at some point, but I have a feeling that there are
> other breakages affecting it these days. :(
Yeah, at least I can say that they are passing now in this particular
machine (i3-6100U).
To make such issues less likely in the future, I have sent a patch
that makes clear when a variable contains a fb modifier constant, and
when a tiling mode. Haven't pushed it because I'm not 100% sure it's
worth it, so I would appreciate any opinions.
Thanks,
Tomeu
> Regards,
>
> Tvrtko
>
>
>
>> +
>> + if (size_ret)
>> + *size_ret = size;
>> +
>> + if (stride_ret)
>> + *stride_ret = stride;
>> +
>> + return bo;
>> + } else {
>> + bool driver_has_tiling_support = false;
>> +
>> + igt_require(driver_has_tiling_support);
>> + return -EINVAL;
>> + }
>> + } else {
>> + if (is_dumb)
>> + *is_dumb = true;
>> + return dumb_create(fd, width, height, bpp, stride_ret,
>> size_ret);
>> + }
>> +}
>> +
>> /* helpers to create nice-looking framebuffers */
>> -static int create_bo_for_fb(int fd, int width, int height, int bpp,
>> +static int create_bo_for_fb(int fd, int width, int height, uint32_t
>> format,
>> uint64_t tiling, unsigned bo_size,
>> unsigned bo_stride, uint32_t *gem_handle_ret,
>> - unsigned *size_ret, unsigned *stride_ret)
>> + unsigned *size_ret, unsigned *stride_ret,
>> + bool *is_dumb)
>> {
>> uint32_t gem_handle;
>> int ret = 0;
>> - unsigned size, stride;
>> -
>> - igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
>> - if (bo_size == 0)
>> - bo_size = size;
>> - if (bo_stride == 0)
>> - bo_stride = stride;
>> -
>> - gem_handle = gem_create(fd, bo_size);
>>
>> - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
>> - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
>> - bo_stride);
>> + gem_handle = igt_create_bo_with_dimensions(fd, width, height,
>> format,
>> +
Re: [Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
Hi,
On 02/03/16 14:00, Tomeu Vizoso wrote:
igt_create_bo_with_dimensions() is intended to abstract differences
between drivers in buffer object creation.
The driver-specific ioctls will be called if the driver that is being
tested can satisfy the needs of the calling subtest, or it will be
skipped otherwise.
Signed-off-by: Tomeu Vizoso
---
lib/igt_fb.c | 83 ++--
lib/igt_fb.h | 6 +
2 files changed, 65 insertions(+), 24 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index cd1605308308..0a3526f4e4ea 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height, int
bpp, uint64_t tiling,
*size_ret = size;
}
+int igt_create_bo_with_dimensions(int fd, int width, int height,
+ uint32_t format, uint64_t modifier,
+ unsigned stride, unsigned *size_ret,
+ unsigned *stride_ret, bool *is_dumb)
+{
+ int bpp = igt_drm_format_to_bpp(format);
+ int bo;
+
+ igt_assert((modifier && stride) || (!modifier && !stride));
+
+ if (modifier) {
+ unsigned size, calculated_stride;
+
+ igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
+&calculated_stride);
+ if (stride == 0)
+ stride = calculated_stride;
+
+ if (is_dumb)
+ *is_dumb = false;
+
+ if (is_i915_device(fd)) {
+
+ bo = gem_create(fd, size);
+ gem_set_tiling(fd, bo, modifier, stride);
This is broken, gem_set_tiling does not take a fb modifier but an object
tiling mode.
You can demonstrate the failure if you got a Skylake system with:
tests/kms_flip_tiling --r flip-changes-tiling-Yf
I would like to be able to tell you that all subtests there should pass,
since they have been at some point, but I have a feeling that there are
other breakages affecting it these days. :(
Regards,
Tvrtko
+
+ if (size_ret)
+ *size_ret = size;
+
+ if (stride_ret)
+ *stride_ret = stride;
+
+ return bo;
+ } else {
+ bool driver_has_tiling_support = false;
+
+ igt_require(driver_has_tiling_support);
+ return -EINVAL;
+ }
+ } else {
+ if (is_dumb)
+ *is_dumb = true;
+ return dumb_create(fd, width, height, bpp, stride_ret,
size_ret);
+ }
+}
+
/* helpers to create nice-looking framebuffers */
-static int create_bo_for_fb(int fd, int width, int height, int bpp,
+static int create_bo_for_fb(int fd, int width, int height, uint32_t format,
uint64_t tiling, unsigned bo_size,
unsigned bo_stride, uint32_t *gem_handle_ret,
- unsigned *size_ret, unsigned *stride_ret)
+ unsigned *size_ret, unsigned *stride_ret,
+ bool *is_dumb)
{
uint32_t gem_handle;
int ret = 0;
- unsigned size, stride;
-
- igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
- if (bo_size == 0)
- bo_size = size;
- if (bo_stride == 0)
- bo_stride = stride;
-
- gem_handle = gem_create(fd, bo_size);
- if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
- ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
- bo_stride);
+ gem_handle = igt_create_bo_with_dimensions(fd, width, height, format,
+ tiling, bo_stride, size_ret,
+ stride_ret, is_dumb);
- *stride_ret = bo_stride;
- *size_ret = bo_size;
*gem_handle_ret = gem_handle;
return ret;
@@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
unsigned bo_stride)
{
uint32_t fb_id;
- int bpp;
memset(fb, 0, sizeof(*fb));
- bpp = igt_drm_format_to_bpp(format);
-
- igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64",
size=%d)\n",
- __func__, width, height, format, bpp, tiling, bo_size);
- do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size,
+ igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64",
size=%d)\n",
+ __func__, width, height, format, tiling, bo_size);
+ do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size,
bo_stride, &fb->gem_handle, &fb->size,
- &fb->stride));
+
Re: [Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
Hey, On 5 March 2016 at 12:30, Daniel Vetter wrote: > On Wed, Mar 02, 2016 at 03:00:15PM +0100, Tomeu Vizoso wrote: >> +int igt_create_bo_with_dimensions(int fd, int width, int height, > > Needs gtkdoc. Also this seems to overlap in functionality with the very > recently added igt_calc_fb_size. Could we perhaps implement your new > function here using igt_calc_fb_size plus igt_create_fb_with_size? Then > this would just be a convenience wrapper. It, er, already is implemented in terms of igt_calc_fb_size? When it can be, at least. Dumb buffers calculate stride and size for you, so they _cannot_ be implemented in those terms. So the idea behind this is that you have two API entrypoints: one where you only care about having a buffer with particular dimensions and format (most tests, can use dumb), and one where you want to very specifically control allocation parameters (e.g. invalid-stride/size-too-small tests, cannot use dumb). > I just want to make sure that we have a consistent interface to igt_fb.c > functionality and avoid the need that driver-specific tiling formats need > to overwrite bazillion of different places. That makes sense, and is indeed the intention of this series. It's not complete or the entire way there yet, but Tomeu wanted to get this out there as a pretty good starting point to build on. >> + igt_assert((modifier && stride) || (!modifier && !stride)); As an aside, I think this is wrong. !modifier && stride can be valid, though it shouldn't be necessary most of the time. :) Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
On Sat, Mar 05, 2016 at 01:30:34PM +0100, Daniel Vetter wrote:
> On Wed, Mar 02, 2016 at 03:00:15PM +0100, Tomeu Vizoso wrote:
> > igt_create_bo_with_dimensions() is intended to abstract differences
> > between drivers in buffer object creation.
> >
> > The driver-specific ioctls will be called if the driver that is being
> > tested can satisfy the needs of the calling subtest, or it will be
> > skipped otherwise.
> >
> > Signed-off-by: Tomeu Vizoso
> > ---
> >
> > lib/igt_fb.c | 83
> > ++--
> > lib/igt_fb.h | 6 +
> > 2 files changed, 65 insertions(+), 24 deletions(-)
> >
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index cd1605308308..0a3526f4e4ea 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
> > int bpp, uint64_t tiling,
> > *size_ret = size;
> > }
> >
> > +int igt_create_bo_with_dimensions(int fd, int width, int height,
>
> Needs gtkdoc. Also this seems to overlap in functionality with the very
> recently added igt_calc_fb_size. Could we perhaps implement your new
> function here using igt_calc_fb_size plus igt_create_fb_with_size? Then
> this would just be a convenience wrapper.
BTW I also have some improvements to igt_calc_fb_size() lined up to deal
with offsets[] and whatnot. It also fixes it to account for the passed
in stride when computing the required fb size.
git://github.com/vsyrjala/linux.git fb_offsets
>
> I just want to make sure that we have a consistent interface to igt_fb.c
> functionality and avoid the need that driver-specific tiling formats need
> to overwrite bazillion of different places.
> -Daniel
>
> > + uint32_t format, uint64_t modifier,
> > + unsigned stride, unsigned *size_ret,
> > + unsigned *stride_ret, bool *is_dumb)
> > +{
> > + int bpp = igt_drm_format_to_bpp(format);
> > + int bo;
> > +
> > + igt_assert((modifier && stride) || (!modifier && !stride));
> > +
> > + if (modifier) {
> > + unsigned size, calculated_stride;
> > +
> > + igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
> > +&calculated_stride);
> > + if (stride == 0)
> > + stride = calculated_stride;
> > +
> > + if (is_dumb)
> > + *is_dumb = false;
> > +
> > + if (is_i915_device(fd)) {
> > +
> > + bo = gem_create(fd, size);
> > + gem_set_tiling(fd, bo, modifier, stride);
> > +
> > + if (size_ret)
> > + *size_ret = size;
> > +
> > + if (stride_ret)
> > + *stride_ret = stride;
> > +
> > + return bo;
> > + } else {
> > + bool driver_has_tiling_support = false;
> > +
> > + igt_require(driver_has_tiling_support);
> > + return -EINVAL;
> > + }
> > + } else {
> > + if (is_dumb)
> > + *is_dumb = true;
> > + return dumb_create(fd, width, height, bpp, stride_ret,
> > size_ret);
> > + }
> > +}
> > +
> > /* helpers to create nice-looking framebuffers */
> > -static int create_bo_for_fb(int fd, int width, int height, int bpp,
> > +static int create_bo_for_fb(int fd, int width, int height, uint32_t format,
> > uint64_t tiling, unsigned bo_size,
> > unsigned bo_stride, uint32_t *gem_handle_ret,
> > - unsigned *size_ret, unsigned *stride_ret)
> > + unsigned *size_ret, unsigned *stride_ret,
> > + bool *is_dumb)
> > {
> > uint32_t gem_handle;
> > int ret = 0;
> > - unsigned size, stride;
> > -
> > - igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
> > - if (bo_size == 0)
> > - bo_size = size;
> > - if (bo_stride == 0)
> > - bo_stride = stride;
> > -
> > - gem_handle = gem_create(fd, bo_size);
> >
> > - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
> > - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
> > - bo_stride);
> > + gem_handle = igt_create_bo_with_dimensions(fd, width, height, format,
> > + tiling, bo_stride, size_ret,
> > + stride_ret, is_dumb);
> >
> > - *stride_ret = bo_stride;
> > - *size_ret = bo_size;
> > *gem_handle_ret = gem_handle;
> >
> > return ret;
> > @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int
> > height,
> >unsigned bo_stride)
> > {
> > uint32_t fb_id;
> > - int bpp;
> >
> > memset(fb, 0, sizeof(*fb));
> >
> > - bpp = igt_drm_format_to_bpp(format);
> > -
> > - igt_debug("%s(wid
Re: [Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
On 03/05/2016 01:30 PM, Daniel Vetter wrote:
> On Wed, Mar 02, 2016 at 03:00:15PM +0100, Tomeu Vizoso wrote:
>> igt_create_bo_with_dimensions() is intended to abstract differences
>> between drivers in buffer object creation.
>>
>> The driver-specific ioctls will be called if the driver that is being
>> tested can satisfy the needs of the calling subtest, or it will be
>> skipped otherwise.
>>
>> Signed-off-by: Tomeu Vizoso
>> ---
>>
>> lib/igt_fb.c | 83
>> ++--
>> lib/igt_fb.h | 6 +
>> 2 files changed, 65 insertions(+), 24 deletions(-)
>>
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index cd1605308308..0a3526f4e4ea 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
>> int bpp, uint64_t tiling,
>> *size_ret = size;
>> }
>>
>> +int igt_create_bo_with_dimensions(int fd, int width, int height,
>
> Needs gtkdoc. Also this seems to overlap in functionality with the very
> recently added igt_calc_fb_size. Could we perhaps implement your new
> function here using igt_calc_fb_size plus igt_create_fb_with_size? Then
> this would just be a convenience wrapper.
Feel a bit lost here, sorry. igt_create_bo_with_dimensions is
implemented in terms of igt_calc_fb_size already, and
igt_create_fb_with_size doesn't exist.
I see that there's igt_create_fb_with_bo_size, but why a function
creating BOs would call another that creates FBs?
Thanks,
Tomeu
> I just want to make sure that we have a consistent interface to igt_fb.c
> functionality and avoid the need that driver-specific tiling formats need
> to overwrite bazillion of different places.
> -Daniel
>
>> + uint32_t format, uint64_t modifier,
>> + unsigned stride, unsigned *size_ret,
>> + unsigned *stride_ret, bool *is_dumb)
>> +{
>> +int bpp = igt_drm_format_to_bpp(format);
>> +int bo;
>> +
>> +igt_assert((modifier && stride) || (!modifier && !stride));
>> +
>> +if (modifier) {
>> +unsigned size, calculated_stride;
>> +
>> +igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
>> + &calculated_stride);
>> +if (stride == 0)
>> +stride = calculated_stride;
>> +
>> +if (is_dumb)
>> +*is_dumb = false;
>> +
>> +if (is_i915_device(fd)) {
>> +
>> +bo = gem_create(fd, size);
>> +gem_set_tiling(fd, bo, modifier, stride);
>> +
>> +if (size_ret)
>> +*size_ret = size;
>> +
>> +if (stride_ret)
>> +*stride_ret = stride;
>> +
>> +return bo;
>> +} else {
>> +bool driver_has_tiling_support = false;
>> +
>> +igt_require(driver_has_tiling_support);
>> +return -EINVAL;
>> +}
>> +} else {
>> +if (is_dumb)
>> +*is_dumb = true;
>> +return dumb_create(fd, width, height, bpp, stride_ret,
>> size_ret);
>> +}
>> +}
>> +
>> /* helpers to create nice-looking framebuffers */
>> -static int create_bo_for_fb(int fd, int width, int height, int bpp,
>> +static int create_bo_for_fb(int fd, int width, int height, uint32_t format,
>> uint64_t tiling, unsigned bo_size,
>> unsigned bo_stride, uint32_t *gem_handle_ret,
>> -unsigned *size_ret, unsigned *stride_ret)
>> +unsigned *size_ret, unsigned *stride_ret,
>> +bool *is_dumb)
>> {
>> uint32_t gem_handle;
>> int ret = 0;
>> -unsigned size, stride;
>> -
>> -igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
>> -if (bo_size == 0)
>> -bo_size = size;
>> -if (bo_stride == 0)
>> -bo_stride = stride;
>> -
>> -gem_handle = gem_create(fd, bo_size);
>>
>> -if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
>> -ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
>> - bo_stride);
>> +gem_handle = igt_create_bo_with_dimensions(fd, width, height, format,
>> + tiling, bo_stride, size_ret,
>> + stride_ret, is_dumb);
>>
>> -*stride_ret = bo_stride;
>> -*size_ret = bo_size;
>> *gem_handle_ret = gem_handle;
>>
>> return ret;
>> @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int
>> height,
>> unsigned bo_stride)
>> {
>> uint32_t fb_id;
>> -int bpp;
>>
>> memset(fb, 0, sizeof(*fb));
>>
>> -bpp = igt_drm_format_to_bpp(format);
>> -
>> -igt_debug("%s(width=%d, height=%d, format=0x%x
Re: [Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
On Wed, Mar 02, 2016 at 03:00:15PM +0100, Tomeu Vizoso wrote:
> igt_create_bo_with_dimensions() is intended to abstract differences
> between drivers in buffer object creation.
>
> The driver-specific ioctls will be called if the driver that is being
> tested can satisfy the needs of the calling subtest, or it will be
> skipped otherwise.
>
> Signed-off-by: Tomeu Vizoso
> ---
>
> lib/igt_fb.c | 83
> ++--
> lib/igt_fb.h | 6 +
> 2 files changed, 65 insertions(+), 24 deletions(-)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index cd1605308308..0a3526f4e4ea 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height,
> int bpp, uint64_t tiling,
> *size_ret = size;
> }
>
> +int igt_create_bo_with_dimensions(int fd, int width, int height,
Needs gtkdoc. Also this seems to overlap in functionality with the very
recently added igt_calc_fb_size. Could we perhaps implement your new
function here using igt_calc_fb_size plus igt_create_fb_with_size? Then
this would just be a convenience wrapper.
I just want to make sure that we have a consistent interface to igt_fb.c
functionality and avoid the need that driver-specific tiling formats need
to overwrite bazillion of different places.
-Daniel
> + uint32_t format, uint64_t modifier,
> + unsigned stride, unsigned *size_ret,
> + unsigned *stride_ret, bool *is_dumb)
> +{
> + int bpp = igt_drm_format_to_bpp(format);
> + int bo;
> +
> + igt_assert((modifier && stride) || (!modifier && !stride));
> +
> + if (modifier) {
> + unsigned size, calculated_stride;
> +
> + igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
> + &calculated_stride);
> + if (stride == 0)
> + stride = calculated_stride;
> +
> + if (is_dumb)
> + *is_dumb = false;
> +
> + if (is_i915_device(fd)) {
> +
> + bo = gem_create(fd, size);
> + gem_set_tiling(fd, bo, modifier, stride);
> +
> + if (size_ret)
> + *size_ret = size;
> +
> + if (stride_ret)
> + *stride_ret = stride;
> +
> + return bo;
> + } else {
> + bool driver_has_tiling_support = false;
> +
> + igt_require(driver_has_tiling_support);
> + return -EINVAL;
> + }
> + } else {
> + if (is_dumb)
> + *is_dumb = true;
> + return dumb_create(fd, width, height, bpp, stride_ret,
> size_ret);
> + }
> +}
> +
> /* helpers to create nice-looking framebuffers */
> -static int create_bo_for_fb(int fd, int width, int height, int bpp,
> +static int create_bo_for_fb(int fd, int width, int height, uint32_t format,
> uint64_t tiling, unsigned bo_size,
> unsigned bo_stride, uint32_t *gem_handle_ret,
> - unsigned *size_ret, unsigned *stride_ret)
> + unsigned *size_ret, unsigned *stride_ret,
> + bool *is_dumb)
> {
> uint32_t gem_handle;
> int ret = 0;
> - unsigned size, stride;
> -
> - igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
> - if (bo_size == 0)
> - bo_size = size;
> - if (bo_stride == 0)
> - bo_stride = stride;
> -
> - gem_handle = gem_create(fd, bo_size);
>
> - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
> - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
> -bo_stride);
> + gem_handle = igt_create_bo_with_dimensions(fd, width, height, format,
> +tiling, bo_stride, size_ret,
> +stride_ret, is_dumb);
>
> - *stride_ret = bo_stride;
> - *size_ret = bo_size;
> *gem_handle_ret = gem_handle;
>
> return ret;
> @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int
> height,
> unsigned bo_stride)
> {
> uint32_t fb_id;
> - int bpp;
>
> memset(fb, 0, sizeof(*fb));
>
> - bpp = igt_drm_format_to_bpp(format);
> -
> - igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d],
> tiling=0x%"PRIx64", size=%d)\n",
> - __func__, width, height, format, bpp, tiling, bo_size);
> - do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size,
> + igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64",
> size=%d)\n",
> + __func__, width, height, format, tiling, bo_size);
> + do_or_die(create_bo_for_fb(fd, width, h
[Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions
igt_create_bo_with_dimensions() is intended to abstract differences
between drivers in buffer object creation.
The driver-specific ioctls will be called if the driver that is being
tested can satisfy the needs of the calling subtest, or it will be
skipped otherwise.
Signed-off-by: Tomeu Vizoso
---
lib/igt_fb.c | 83 ++--
lib/igt_fb.h | 6 +
2 files changed, 65 insertions(+), 24 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index cd1605308308..0a3526f4e4ea 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -174,30 +174,66 @@ void igt_calc_fb_size(int fd, int width, int height, int
bpp, uint64_t tiling,
*size_ret = size;
}
+int igt_create_bo_with_dimensions(int fd, int width, int height,
+ uint32_t format, uint64_t modifier,
+ unsigned stride, unsigned *size_ret,
+ unsigned *stride_ret, bool *is_dumb)
+{
+ int bpp = igt_drm_format_to_bpp(format);
+ int bo;
+
+ igt_assert((modifier && stride) || (!modifier && !stride));
+
+ if (modifier) {
+ unsigned size, calculated_stride;
+
+ igt_calc_fb_size(fd, width, height, bpp, modifier, &size,
+&calculated_stride);
+ if (stride == 0)
+ stride = calculated_stride;
+
+ if (is_dumb)
+ *is_dumb = false;
+
+ if (is_i915_device(fd)) {
+
+ bo = gem_create(fd, size);
+ gem_set_tiling(fd, bo, modifier, stride);
+
+ if (size_ret)
+ *size_ret = size;
+
+ if (stride_ret)
+ *stride_ret = stride;
+
+ return bo;
+ } else {
+ bool driver_has_tiling_support = false;
+
+ igt_require(driver_has_tiling_support);
+ return -EINVAL;
+ }
+ } else {
+ if (is_dumb)
+ *is_dumb = true;
+ return dumb_create(fd, width, height, bpp, stride_ret,
size_ret);
+ }
+}
+
/* helpers to create nice-looking framebuffers */
-static int create_bo_for_fb(int fd, int width, int height, int bpp,
+static int create_bo_for_fb(int fd, int width, int height, uint32_t format,
uint64_t tiling, unsigned bo_size,
unsigned bo_stride, uint32_t *gem_handle_ret,
- unsigned *size_ret, unsigned *stride_ret)
+ unsigned *size_ret, unsigned *stride_ret,
+ bool *is_dumb)
{
uint32_t gem_handle;
int ret = 0;
- unsigned size, stride;
-
- igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride);
- if (bo_size == 0)
- bo_size = size;
- if (bo_stride == 0)
- bo_stride = stride;
-
- gem_handle = gem_create(fd, bo_size);
- if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED)
- ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X,
- bo_stride);
+ gem_handle = igt_create_bo_with_dimensions(fd, width, height, format,
+ tiling, bo_stride, size_ret,
+ stride_ret, is_dumb);
- *stride_ret = bo_stride;
- *size_ret = bo_size;
*gem_handle_ret = gem_handle;
return ret;
@@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
unsigned bo_stride)
{
uint32_t fb_id;
- int bpp;
memset(fb, 0, sizeof(*fb));
- bpp = igt_drm_format_to_bpp(format);
-
- igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d],
tiling=0x%"PRIx64", size=%d)\n",
- __func__, width, height, format, bpp, tiling, bo_size);
- do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size,
+ igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64",
size=%d)\n",
+ __func__, width, height, format, tiling, bo_size);
+ do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size,
bo_stride, &fb->gem_handle, &fb->size,
- &fb->stride));
+ &fb->stride, &fb->is_dumb));
igt_debug("%s(handle=%d, pitch=%d)\n",
__func__, fb->gem_handle, fb->stride);
@@ -860,6 +893,7 @@ struct fb_blit_upload {
uint32_t handle;
unsigned size, stride;
uint8_t *map;
+ bool is_dumb;
} linear;
};
@@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct
igt_fb *fb)
