| Message ID | 1456927221-32421-9-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 02, 2016 at 03:00:15PM +0100, 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, Needs gtkdoc. Also this seems to overlap in functionality with the very recently added igt_calc_fb_size. Could we perhaps implement your new function here using igt_calc_fb_size plus igt_create_fb_with_size? Then this would just be a convenience wrapper. I just want to make sure that we have a consistent interface to igt_fb.c functionality and avoid the need that driver-specific tiling formats need to overwrite bazillion of different places. -Daniel > + 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); > + > + if (size_ret) > + *size_ret = size; > + > + if (stride_ret) > + *stride_ret = stride; > + > + return bo; > + } else { > + bool driver_has_tiling_support = false; > + > + igt_require(driver_has_tiling_support); > + return -EINVAL; > + } > + } else { > + if (is_dumb) > + *is_dumb = true; > + return dumb_create(fd, width, height, bpp, stride_ret, size_ret); > + } > +} > + > /* helpers to create nice-looking framebuffers */ > -static int create_bo_for_fb(int fd, int width, int height, int bpp, > +static int create_bo_for_fb(int fd, int width, int height, uint32_t format, > uint64_t tiling, unsigned bo_size, > unsigned bo_stride, uint32_t *gem_handle_ret, > - unsigned *size_ret, unsigned *stride_ret) > + unsigned *size_ret, unsigned *stride_ret, > + bool *is_dumb) > { > uint32_t gem_handle; > int ret = 0; > - unsigned size, stride; > - > - igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride); > - if (bo_size == 0) > - bo_size = size; > - if (bo_stride == 0) > - bo_stride = stride; > - > - gem_handle = gem_create(fd, bo_size); > > - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED) > - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, > - bo_stride); > + gem_handle = igt_create_bo_with_dimensions(fd, width, height, format, > + tiling, bo_stride, size_ret, > + stride_ret, is_dumb); > > - *stride_ret = bo_stride; > - *size_ret = bo_size; > *gem_handle_ret = gem_handle; > > return ret; > @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height, > unsigned bo_stride) > { > uint32_t fb_id; > - int bpp; > > memset(fb, 0, sizeof(*fb)); > > - bpp = igt_drm_format_to_bpp(format); > - > - igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n", > - __func__, width, height, format, bpp, tiling, bo_size); > - do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size, > + igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64", size=%d)\n", > + __func__, width, height, format, tiling, bo_size); > + do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size, > bo_stride, &fb->gem_handle, &fb->size, > - &fb->stride)); > + &fb->stride, &fb->is_dumb)); > > igt_debug("%s(handle=%d, pitch=%d)\n", > __func__, fb->gem_handle, fb->stride); > @@ -860,6 +893,7 @@ struct fb_blit_upload { > uint32_t handle; > unsigned size, stride; > uint8_t *map; > + bool is_dumb; > } linear; > }; > > @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb) > LOCAL_DRM_FORMAT_MOD_NONE, 0, 0, > &blit->linear.handle, > &blit->linear.size, > - &blit->linear.stride); > + &blit->linear.stride, > + &blit->linear.is_dumb); > > igt_assert(ret == 0); > > diff --git a/lib/igt_fb.h b/lib/igt_fb.h > index 4e6a76947c18..10e59deebe88 100644 > --- a/lib/igt_fb.h > +++ b/lib/igt_fb.h > @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t; > struct igt_fb { > uint32_t fb_id; > uint32_t gem_handle; > + bool is_dumb; > uint32_t drm_format; > int width; > int height; > @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode, > uint32_t format, uint64_t tiling); > void igt_remove_fb(int fd, struct igt_fb *fb); > > +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format, > + uint64_t modifier, unsigned stride, > + unsigned *stride_out, unsigned *size_out, > + bool *is_dumb); > + > /* cairo-based painting */ > cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb); > void igt_paint_color(cairo_t *cr, int x, int y, int w, int h, > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 03/05/2016 01:30 PM, Daniel Vetter wrote: > On Wed, Mar 02, 2016 at 03:00:15PM +0100, 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, > > Needs gtkdoc. Also this seems to overlap in functionality with the very > recently added igt_calc_fb_size. Could we perhaps implement your new > function here using igt_calc_fb_size plus igt_create_fb_with_size? Then > this would just be a convenience wrapper. Feel a bit lost here, sorry. igt_create_bo_with_dimensions is implemented in terms of igt_calc_fb_size already, and igt_create_fb_with_size doesn't exist. I see that there's igt_create_fb_with_bo_size, but why a function creating BOs would call another that creates FBs? Thanks, Tomeu > I just want to make sure that we have a consistent interface to igt_fb.c > functionality and avoid the need that driver-specific tiling formats need > to overwrite bazillion of different places. > -Daniel > >> + 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); >> + >> + if (size_ret) >> + *size_ret = size; >> + >> + if (stride_ret) >> + *stride_ret = stride; >> + >> + return bo; >> + } else { >> + bool driver_has_tiling_support = false; >> + >> + igt_require(driver_has_tiling_support); >> + return -EINVAL; >> + } >> + } else { >> + if (is_dumb) >> + *is_dumb = true; >> + return dumb_create(fd, width, height, bpp, stride_ret, size_ret); >> + } >> +} >> + >> /* helpers to create nice-looking framebuffers */ >> -static int create_bo_for_fb(int fd, int width, int height, int bpp, >> +static int create_bo_for_fb(int fd, int width, int height, uint32_t format, >> uint64_t tiling, unsigned bo_size, >> unsigned bo_stride, uint32_t *gem_handle_ret, >> - unsigned *size_ret, unsigned *stride_ret) >> + unsigned *size_ret, unsigned *stride_ret, >> + bool *is_dumb) >> { >> uint32_t gem_handle; >> int ret = 0; >> - unsigned size, stride; >> - >> - igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride); >> - if (bo_size == 0) >> - bo_size = size; >> - if (bo_stride == 0) >> - bo_stride = stride; >> - >> - gem_handle = gem_create(fd, bo_size); >> >> - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED) >> - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, >> - bo_stride); >> + gem_handle = igt_create_bo_with_dimensions(fd, width, height, format, >> + tiling, bo_stride, size_ret, >> + stride_ret, is_dumb); >> >> - *stride_ret = bo_stride; >> - *size_ret = bo_size; >> *gem_handle_ret = gem_handle; >> >> return ret; >> @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height, >> unsigned bo_stride) >> { >> uint32_t fb_id; >> - int bpp; >> >> memset(fb, 0, sizeof(*fb)); >> >> - bpp = igt_drm_format_to_bpp(format); >> - >> - igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n", >> - __func__, width, height, format, bpp, tiling, bo_size); >> - do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size, >> + igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64", size=%d)\n", >> + __func__, width, height, format, tiling, bo_size); >> + do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size, >> bo_stride, &fb->gem_handle, &fb->size, >> - &fb->stride)); >> + &fb->stride, &fb->is_dumb)); >> >> igt_debug("%s(handle=%d, pitch=%d)\n", >> __func__, fb->gem_handle, fb->stride); >> @@ -860,6 +893,7 @@ struct fb_blit_upload { >> uint32_t handle; >> unsigned size, stride; >> uint8_t *map; >> + bool is_dumb; >> } linear; >> }; >> >> @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb) >> LOCAL_DRM_FORMAT_MOD_NONE, 0, 0, >> &blit->linear.handle, >> &blit->linear.size, >> - &blit->linear.stride); >> + &blit->linear.stride, >> + &blit->linear.is_dumb); >> >> igt_assert(ret == 0); >> >> diff --git a/lib/igt_fb.h b/lib/igt_fb.h >> index 4e6a76947c18..10e59deebe88 100644 >> --- a/lib/igt_fb.h >> +++ b/lib/igt_fb.h >> @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t; >> struct igt_fb { >> uint32_t fb_id; >> uint32_t gem_handle; >> + bool is_dumb; >> uint32_t drm_format; >> int width; >> int height; >> @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode, >> uint32_t format, uint64_t tiling); >> void igt_remove_fb(int fd, struct igt_fb *fb); >> >> +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format, >> + uint64_t modifier, unsigned stride, >> + unsigned *stride_out, unsigned *size_out, >> + bool *is_dumb); >> + >> /* cairo-based painting */ >> cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb); >> void igt_paint_color(cairo_t *cr, int x, int y, int w, int h, >> -- >> 2.5.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Sat, Mar 05, 2016 at 01:30:34PM +0100, Daniel Vetter wrote: > On Wed, Mar 02, 2016 at 03:00:15PM +0100, 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, > > Needs gtkdoc. Also this seems to overlap in functionality with the very > recently added igt_calc_fb_size. Could we perhaps implement your new > function here using igt_calc_fb_size plus igt_create_fb_with_size? Then > this would just be a convenience wrapper. BTW I also have some improvements to igt_calc_fb_size() lined up to deal with offsets[] and whatnot. It also fixes it to account for the passed in stride when computing the required fb size. git://github.com/vsyrjala/linux.git fb_offsets > > I just want to make sure that we have a consistent interface to igt_fb.c > functionality and avoid the need that driver-specific tiling formats need > to overwrite bazillion of different places. > -Daniel > > > + 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); > > + > > + if (size_ret) > > + *size_ret = size; > > + > > + if (stride_ret) > > + *stride_ret = stride; > > + > > + return bo; > > + } else { > > + bool driver_has_tiling_support = false; > > + > > + igt_require(driver_has_tiling_support); > > + return -EINVAL; > > + } > > + } else { > > + if (is_dumb) > > + *is_dumb = true; > > + return dumb_create(fd, width, height, bpp, stride_ret, size_ret); > > + } > > +} > > + > > /* helpers to create nice-looking framebuffers */ > > -static int create_bo_for_fb(int fd, int width, int height, int bpp, > > +static int create_bo_for_fb(int fd, int width, int height, uint32_t format, > > uint64_t tiling, unsigned bo_size, > > unsigned bo_stride, uint32_t *gem_handle_ret, > > - unsigned *size_ret, unsigned *stride_ret) > > + unsigned *size_ret, unsigned *stride_ret, > > + bool *is_dumb) > > { > > uint32_t gem_handle; > > int ret = 0; > > - unsigned size, stride; > > - > > - igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride); > > - if (bo_size == 0) > > - bo_size = size; > > - if (bo_stride == 0) > > - bo_stride = stride; > > - > > - gem_handle = gem_create(fd, bo_size); > > > > - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED) > > - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, > > - bo_stride); > > + gem_handle = igt_create_bo_with_dimensions(fd, width, height, format, > > + tiling, bo_stride, size_ret, > > + stride_ret, is_dumb); > > > > - *stride_ret = bo_stride; > > - *size_ret = bo_size; > > *gem_handle_ret = gem_handle; > > > > return ret; > > @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height, > > unsigned bo_stride) > > { > > uint32_t fb_id; > > - int bpp; > > > > memset(fb, 0, sizeof(*fb)); > > > > - bpp = igt_drm_format_to_bpp(format); > > - > > - igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n", > > - __func__, width, height, format, bpp, tiling, bo_size); > > - do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size, > > + igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64", size=%d)\n", > > + __func__, width, height, format, tiling, bo_size); > > + do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size, > > bo_stride, &fb->gem_handle, &fb->size, > > - &fb->stride)); > > + &fb->stride, &fb->is_dumb)); > > > > igt_debug("%s(handle=%d, pitch=%d)\n", > > __func__, fb->gem_handle, fb->stride); > > @@ -860,6 +893,7 @@ struct fb_blit_upload { > > uint32_t handle; > > unsigned size, stride; > > uint8_t *map; > > + bool is_dumb; > > } linear; > > }; > > > > @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb) > > LOCAL_DRM_FORMAT_MOD_NONE, 0, 0, > > &blit->linear.handle, > > &blit->linear.size, > > - &blit->linear.stride); > > + &blit->linear.stride, > > + &blit->linear.is_dumb); > > > > igt_assert(ret == 0); > > > > diff --git a/lib/igt_fb.h b/lib/igt_fb.h > > index 4e6a76947c18..10e59deebe88 100644 > > --- a/lib/igt_fb.h > > +++ b/lib/igt_fb.h > > @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t; > > struct igt_fb { > > uint32_t fb_id; > > uint32_t gem_handle; > > + bool is_dumb; > > uint32_t drm_format; > > int width; > > int height; > > @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode, > > uint32_t format, uint64_t tiling); > > void igt_remove_fb(int fd, struct igt_fb *fb); > > > > +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format, > > + uint64_t modifier, unsigned stride, > > + unsigned *stride_out, unsigned *size_out, > > + bool *is_dumb); > > + > > /* cairo-based painting */ > > cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb); > > void igt_paint_color(cairo_t *cr, int x, int y, int w, int h, > > -- > > 2.5.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hey, On 5 March 2016 at 12:30, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Mar 02, 2016 at 03:00:15PM +0100, Tomeu Vizoso wrote: >> +int igt_create_bo_with_dimensions(int fd, int width, int height, > > Needs gtkdoc. Also this seems to overlap in functionality with the very > recently added igt_calc_fb_size. Could we perhaps implement your new > function here using igt_calc_fb_size plus igt_create_fb_with_size? Then > this would just be a convenience wrapper. It, er, already is implemented in terms of igt_calc_fb_size? When it can be, at least. Dumb buffers calculate stride and size for you, so they _cannot_ be implemented in those terms. So the idea behind this is that you have two API entrypoints: one where you only care about having a buffer with particular dimensions and format (most tests, can use dumb), and one where you want to very specifically control allocation parameters (e.g. invalid-stride/size-too-small tests, cannot use dumb). > I just want to make sure that we have a consistent interface to igt_fb.c > functionality and avoid the need that driver-specific tiling formats need > to overwrite bazillion of different places. That makes sense, and is indeed the intention of this series. It's not complete or the entire way there yet, but Tomeu wanted to get this out there as a pretty good starting point to build on. >> + igt_assert((modifier && stride) || (!modifier && !stride)); As an aside, I think this is wrong. !modifier && stride can be valid, though it shouldn't be necessary most of the time. :) Cheers, Daniel
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 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. :( Regards, Tvrtko > + > + if (size_ret) > + *size_ret = size; > + > + if (stride_ret) > + *stride_ret = stride; > + > + return bo; > + } else { > + bool driver_has_tiling_support = false; > + > + igt_require(driver_has_tiling_support); > + return -EINVAL; > + } > + } else { > + if (is_dumb) > + *is_dumb = true; > + return dumb_create(fd, width, height, bpp, stride_ret, size_ret); > + } > +} > + > /* helpers to create nice-looking framebuffers */ > -static int create_bo_for_fb(int fd, int width, int height, int bpp, > +static int create_bo_for_fb(int fd, int width, int height, uint32_t format, > uint64_t tiling, unsigned bo_size, > unsigned bo_stride, uint32_t *gem_handle_ret, > - unsigned *size_ret, unsigned *stride_ret) > + unsigned *size_ret, unsigned *stride_ret, > + bool *is_dumb) > { > uint32_t gem_handle; > int ret = 0; > - unsigned size, stride; > - > - igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride); > - if (bo_size == 0) > - bo_size = size; > - if (bo_stride == 0) > - bo_stride = stride; > - > - gem_handle = gem_create(fd, bo_size); > > - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED) > - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, > - bo_stride); > + gem_handle = igt_create_bo_with_dimensions(fd, width, height, format, > + tiling, bo_stride, size_ret, > + stride_ret, is_dumb); > > - *stride_ret = bo_stride; > - *size_ret = bo_size; > *gem_handle_ret = gem_handle; > > return ret; > @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height, > unsigned bo_stride) > { > uint32_t fb_id; > - int bpp; > > memset(fb, 0, sizeof(*fb)); > > - bpp = igt_drm_format_to_bpp(format); > - > - igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n", > - __func__, width, height, format, bpp, tiling, bo_size); > - do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size, > + igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64", size=%d)\n", > + __func__, width, height, format, tiling, bo_size); > + do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size, > bo_stride, &fb->gem_handle, &fb->size, > - &fb->stride)); > + &fb->stride, &fb->is_dumb)); > > igt_debug("%s(handle=%d, pitch=%d)\n", > __func__, fb->gem_handle, fb->stride); > @@ -860,6 +893,7 @@ struct fb_blit_upload { > uint32_t handle; > unsigned size, stride; > uint8_t *map; > + bool is_dumb; > } linear; > }; > > @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb) > LOCAL_DRM_FORMAT_MOD_NONE, 0, 0, > &blit->linear.handle, > &blit->linear.size, > - &blit->linear.stride); > + &blit->linear.stride, > + &blit->linear.is_dumb); > > igt_assert(ret == 0); > > diff --git a/lib/igt_fb.h b/lib/igt_fb.h > index 4e6a76947c18..10e59deebe88 100644 > --- a/lib/igt_fb.h > +++ b/lib/igt_fb.h > @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t; > struct igt_fb { > uint32_t fb_id; > uint32_t gem_handle; > + bool is_dumb; > uint32_t drm_format; > int width; > int height; > @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode, > uint32_t format, uint64_t tiling); > void igt_remove_fb(int fd, struct igt_fb *fb); > > +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format, > + uint64_t modifier, unsigned stride, > + unsigned *stride_out, unsigned *size_out, > + bool *is_dumb); > + > /* cairo-based painting */ > cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb); > void igt_paint_color(cairo_t *cr, int x, int y, int w, int h, >
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. > 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. Thanks, Tomeu > Regards, > > Tvrtko > > > >> + >> + if (size_ret) >> + *size_ret = size; >> + >> + if (stride_ret) >> + *stride_ret = stride; >> + >> + return bo; >> + } else { >> + bool driver_has_tiling_support = false; >> + >> + igt_require(driver_has_tiling_support); >> + return -EINVAL; >> + } >> + } else { >> + if (is_dumb) >> + *is_dumb = true; >> + return dumb_create(fd, width, height, bpp, stride_ret, >> size_ret); >> + } >> +} >> + >> /* helpers to create nice-looking framebuffers */ >> -static int create_bo_for_fb(int fd, int width, int height, int bpp, >> +static int create_bo_for_fb(int fd, int width, int height, uint32_t >> format, >> uint64_t tiling, unsigned bo_size, >> unsigned bo_stride, uint32_t *gem_handle_ret, >> - unsigned *size_ret, unsigned *stride_ret) >> + unsigned *size_ret, unsigned *stride_ret, >> + bool *is_dumb) >> { >> uint32_t gem_handle; >> int ret = 0; >> - unsigned size, stride; >> - >> - igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride); >> - if (bo_size == 0) >> - bo_size = size; >> - if (bo_stride == 0) >> - bo_stride = stride; >> - >> - gem_handle = gem_create(fd, bo_size); >> >> - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED) >> - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, >> - bo_stride); >> + gem_handle = igt_create_bo_with_dimensions(fd, width, height, >> format, >> + tiling, bo_stride, >> size_ret, >> + stride_ret, is_dumb); >> >> - *stride_ret = bo_stride; >> - *size_ret = bo_size; >> *gem_handle_ret = gem_handle; >> >> return ret; >> @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int >> height, >> unsigned bo_stride) >> { >> uint32_t fb_id; >> - int bpp; >> >> memset(fb, 0, sizeof(*fb)); >> >> - bpp = igt_drm_format_to_bpp(format); >> - >> - igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], >> tiling=0x%"PRIx64", size=%d)\n", >> - __func__, width, height, format, bpp, tiling, bo_size); >> - do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, >> bo_size, >> + igt_debug("%s(width=%d, height=%d, format=0x%x, >> tiling=0x%"PRIx64", size=%d)\n", >> + __func__, width, height, format, tiling, bo_size); >> + do_or_die(create_bo_for_fb(fd, width, height, format, tiling, >> bo_size, >> bo_stride, &fb->gem_handle, &fb->size, >> - &fb->stride)); >> + &fb->stride, &fb->is_dumb)); >> >> igt_debug("%s(handle=%d, pitch=%d)\n", >> __func__, fb->gem_handle, fb->stride); >> @@ -860,6 +893,7 @@ struct fb_blit_upload { >> uint32_t handle; >> unsigned size, stride; >> uint8_t *map; >> + bool is_dumb; >> } linear; >> }; >> >> @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct >> igt_fb *fb) >> LOCAL_DRM_FORMAT_MOD_NONE, 0, 0, >> &blit->linear.handle, >> &blit->linear.size, >> - &blit->linear.stride); >> + &blit->linear.stride, >> + &blit->linear.is_dumb); >> >> igt_assert(ret == 0); >> >> diff --git a/lib/igt_fb.h b/lib/igt_fb.h >> index 4e6a76947c18..10e59deebe88 100644 >> --- a/lib/igt_fb.h >> +++ b/lib/igt_fb.h >> @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t; >> struct igt_fb { >> uint32_t fb_id; >> uint32_t gem_handle; >> + bool is_dumb; >> uint32_t drm_format; >> int width; >> int height; >> @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, >> drmModeModeInfo *mode, >> uint32_t format, uint64_t tiling); >> void igt_remove_fb(int fd, struct igt_fb *fb); >> >> +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t >> format, >> + uint64_t modifier, unsigned stride, >> + unsigned *stride_out, unsigned >> *size_out, >> + bool *is_dumb); >> + >> /* cairo-based painting */ >> cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb); >> void igt_paint_color(cairo_t *cr, int x, int y, int w, int h, >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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
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@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? 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. Regards, Tomeu >>> 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 >
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@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? > > 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
On 11 November 2016 at 12:33, Tvrtko Ursulin <tvrtko.ursulin@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@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? >> >> >> 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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); + + if (size_ret) + *size_ret = size; + + if (stride_ret) + *stride_ret = stride; + + return bo; + } else { + bool driver_has_tiling_support = false; + + igt_require(driver_has_tiling_support); + return -EINVAL; + } + } else { + if (is_dumb) + *is_dumb = true; + return dumb_create(fd, width, height, bpp, stride_ret, size_ret); + } +} + /* helpers to create nice-looking framebuffers */ -static int create_bo_for_fb(int fd, int width, int height, int bpp, +static int create_bo_for_fb(int fd, int width, int height, uint32_t format, uint64_t tiling, unsigned bo_size, unsigned bo_stride, uint32_t *gem_handle_ret, - unsigned *size_ret, unsigned *stride_ret) + unsigned *size_ret, unsigned *stride_ret, + bool *is_dumb) { uint32_t gem_handle; int ret = 0; - unsigned size, stride; - - igt_calc_fb_size(fd, width, height, bpp, tiling, &size, &stride); - if (bo_size == 0) - bo_size = size; - if (bo_stride == 0) - bo_stride = stride; - - gem_handle = gem_create(fd, bo_size); - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED) - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, - bo_stride); + gem_handle = igt_create_bo_with_dimensions(fd, width, height, format, + tiling, bo_stride, size_ret, + stride_ret, is_dumb); - *stride_ret = bo_stride; - *size_ret = bo_size; *gem_handle_ret = gem_handle; return ret; @@ -501,17 +537,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height, unsigned bo_stride) { uint32_t fb_id; - int bpp; memset(fb, 0, sizeof(*fb)); - bpp = igt_drm_format_to_bpp(format); - - igt_debug("%s(width=%d, height=%d, format=0x%x [bpp=%d], tiling=0x%"PRIx64", size=%d)\n", - __func__, width, height, format, bpp, tiling, bo_size); - do_or_die(create_bo_for_fb(fd, width, height, bpp, tiling, bo_size, + igt_debug("%s(width=%d, height=%d, format=0x%x, tiling=0x%"PRIx64", size=%d)\n", + __func__, width, height, format, tiling, bo_size); + do_or_die(create_bo_for_fb(fd, width, height, format, tiling, bo_size, bo_stride, &fb->gem_handle, &fb->size, - &fb->stride)); + &fb->stride, &fb->is_dumb)); igt_debug("%s(handle=%d, pitch=%d)\n", __func__, fb->gem_handle, fb->stride); @@ -860,6 +893,7 @@ struct fb_blit_upload { uint32_t handle; unsigned size, stride; uint8_t *map; + bool is_dumb; } linear; }; @@ -928,7 +962,8 @@ static void create_cairo_surface__blit(int fd, struct igt_fb *fb) LOCAL_DRM_FORMAT_MOD_NONE, 0, 0, &blit->linear.handle, &blit->linear.size, - &blit->linear.stride); + &blit->linear.stride, + &blit->linear.is_dumb); igt_assert(ret == 0); diff --git a/lib/igt_fb.h b/lib/igt_fb.h index 4e6a76947c18..10e59deebe88 100644 --- a/lib/igt_fb.h +++ b/lib/igt_fb.h @@ -47,6 +47,7 @@ typedef struct _cairo cairo_t; struct igt_fb { uint32_t fb_id; uint32_t gem_handle; + bool is_dumb; uint32_t drm_format; int width; int height; @@ -97,6 +98,11 @@ unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode, uint32_t format, uint64_t tiling); void igt_remove_fb(int fd, struct igt_fb *fb); +int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format, + uint64_t modifier, unsigned stride, + unsigned *stride_out, unsigned *size_out, + bool *is_dumb); + /* cairo-based painting */ cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb); void igt_paint_color(cairo_t *cr, int x, int y, int w, int h,
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(-)