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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/11/2016 13:17, Tomeu Vizoso wrote:
> On 1 November 2016 at 16:44, Tvrtko Ursulin <tursulin@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@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?

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