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

[Tomeu Vizoso]

On 11 November 2016 at 12:33, Tvrtko Ursulin
<tvrtko.ursulin at linux.intel.com> 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 <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.

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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx