| Message ID | 20151118155918.GG20799@phenom.ffwll.local (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-11-18 13:59 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > 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) > + } > } 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. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 18, 2015 at 5:20 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > 2015-11-18 13:59 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: >> 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
On Wed, Nov 18, 2015 at 05:38:47PM +0100, Daniel Vetter wrote: > On Wed, Nov 18, 2015 at 5:20 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > > 2015-11-18 13:59 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > >> 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. Or just have a helper to get us the actual tile size and use that. > > >> + } > >> } 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Qua, 2015-11-18 às 17:38 +0100, Daniel Vetter escreveu: > On Wed, Nov 18, 2015 at 5:20 PM, Paulo Zanoni <przanoni@gmail.com> > wrote: > > 2015-11-18 13:59 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > > > 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). That sounds a good idea, but it will require yet another rework to the code. > But your cover letter talked about allocating less > in general, and that problem really should be fixed in the library > itself. It is both problems actually. That's why I think we need both this series so it's still possible to specify a custom stride+size, and also your patch so the sizes are saner. > -Daniel
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); + } } else { /* Scan-out has a 64 byte alignment restriction */ stride = (width * (bpp / 8) + 63) & ~63;