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

[Tvrtko Ursulin]

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

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