[Intel-gfx] [i-g-t PATCH v1 08/14] lib: Add igt_create_bo_with_dimensions

[Tomeu Vizoso]

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 <tursulin at ursulin.net> 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 <tomeu.vizoso at collabora.com>
>>>> ---
>>>>
>>>>  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
>