On Wed, Nov 18, 2015 at 5:20 PM, Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
> 2015-11-18 13:59 GMT-02:00 Daniel Vetter <daniel@xxxxxxxx>:
>> On Fri, Nov 13, 2015 at 03:12:41PM -0200, Paulo Zanoni wrote:
>>> Hello
>>>
>>> I've been carrying some local IGT patches that reduced the size of buffers
>>> created by igt_create_fb() so they would fit the stolen memory, but when I
>>> decided to test the tree without them, I concluded the lack of sane sizes was
>>> even causing test failures. So here's my attempt to fix this. This series alone
>>> should help reducing the number of kms_frontbuffer_tracking failures seen by QA.
>>>
>>> The last few patches make the FBC tests a little harder. They are all based on
>>> the feedback I got from the last patches I sent.
>>
>> The point of a helper library is that it helps, not that every caller has
>> to work around it's choice of size and stride.
>
> Judging by the amount of users, it is helping even without my changes :)
>
>>
>> The only thing we need to do here is fix up the selection of stride and
>> size to make it not pick the super-conservative value that work even on
>> gen2&3. Something like:
>>
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index 13a6a34982e0..9eb97952ed95 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -87,21 +87,26 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp,
>> if (tiling != LOCAL_DRM_FORMAT_MOD_NONE) {
>> int v;
>>
>> - /* Round the tiling up to the next power-of-two and the
>> - * region up to the next pot fence size so that this works
>> - * on all generations.
>> - *
>> - * This can still fail if the framebuffer is too large to
>> - * be tiled. But then that failure is expected.
>> - */
>> -
>> - v = width * bpp / 8;
>> - for (stride = 512; stride < v; stride *= 2)
>> - ;
>> -
>> - v = stride * height;
>> - for (size = 1024*1024; size < v; size *= 2)
>> - ;
>> + if (gen < 4) {
>> + /* Round the tiling up to the next power-of-two and the
>> + * region up to the next pot fence size so that this works
>> + * on all generations.
>> + *
>> + * This can still fail if the framebuffer is too large to
>> + * be tiled. But then that failure is expected.
>> + */
>> +
>> + v = width * bpp / 8;
>> + for (stride = 512; stride < v; stride *= 2)
>> + ;
>> +
>> + v = stride * height;
>> + for (size = 1024*1024; size < v; size *= 2)
>> + ;
>> + } else {
>> + stride = ALIGN(stride, 512);
>> + size = ALIGN(size, stride * 32);
>
> Shouldn't it be size = stride * ALIGN(height, 32)?
> (it still wouldn't be the minimal size, but would be close to it)
Yeah that's probably what we want.
>> + }
>> } else {
>> /* Scan-out has a 64 byte alignment restriction */
>> stride = (width * (bpp / 8) + 63) & ~63;
>>
>>
>> Or whatever is the right thing to pick that works on gen4+.
>
> While that sounds like an improvement, it won't solve the
> kms_frontbuffer_tracking problem where we want to specify size+stride
> since we want all buffers using the same size+stride independently of
> tiling/no-tiling.
Matching stride is a good reason for your changes (and then
kms_frontbuffer_tracking should allocate the tiled fb first and then
ask for an untiled fb with matching stride to avoid reimplementing the
stride rounding). But your cover letter talked about allocating less
in general, and that problem really should be fixed in the library
itself.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
