| Message ID | 1449060421-29849-2-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 02, 2015 at 10:47:01AM -0200, Paulo Zanoni wrote: > We want to make sure that both tiled and untiled buffers have the same > size for the same width/height/format. This will allow better control > over the failure paths exercised by our tests: when we try to flip > from tiled to untiled, we'll be sure that we won't execute the error > path that checks for buffer sizes. > > v2: Use the new igt_calc_fb_size() instead of implementing our own > size calculation (Daniel). > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Yeah, this is what I had in mind, looks great. On the series: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > tests/kms_frontbuffer_tracking.c | 51 ++++++++++++++++++++++++++-------------- > 1 file changed, 34 insertions(+), 17 deletions(-) > > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c > index 81703ec..3db95d2 100644 > --- a/tests/kms_frontbuffer_tracking.c > +++ b/tests/kms_frontbuffer_tracking.c > @@ -479,10 +479,28 @@ static bool init_modeset_cached_params(void) > return true; > } > > +static int format_get_bpp(uint32_t format) > +{ > + switch (format) { > + case DRM_FORMAT_RGB565: > + return 16; > + case DRM_FORMAT_XRGB8888: > + case DRM_FORMAT_ARGB8888: > + case DRM_FORMAT_ARGB2101010: > + case DRM_FORMAT_XRGB2101010: > + return 32; > + default: > + igt_assert(false); > + } > +} > + > static void create_fb(enum pixel_format pformat, int width, int height, > uint64_t tiling, int plane, struct igt_fb *fb) > { > uint32_t format; > + unsigned int size, stride; > + int bpp; > + uint64_t tiling_for_size; > > switch (pformat) { > case FORMAT_RGB888: > @@ -512,7 +530,21 @@ static void create_fb(enum pixel_format pformat, int width, int height, > igt_assert(false); > } > > - igt_create_fb(drm.fd, width, height, format, tiling, fb); > + /* We want all frontbuffers with the same width/height/format to have > + * the same size regardless of tiling since we want to properly exercise > + * the Kernel's specific tiling-checking code paths without accidentally > + * hitting size-checking ones first. */ > + bpp = format_get_bpp(format); > + if (plane == PLANE_CUR) > + tiling_for_size = LOCAL_DRM_FORMAT_MOD_NONE; > + else > + tiling_for_size = LOCAL_I915_FORMAT_MOD_X_TILED; > + > + igt_calc_fb_size(drm.fd, width, height, bpp, tiling_for_size, &size, > + &stride); > + > + igt_create_fb_with_bo_size(drm.fd, width, height, format, tiling, fb, > + size, stride); > } > > static uint32_t pick_color(struct igt_fb *fb, enum color ecolor) > @@ -1094,21 +1126,6 @@ static void *busy_thread_func(void *data) > pthread_exit(0); > } > > -static int fb_get_bpp(struct igt_fb *fb) > -{ > - switch (fb->drm_format) { > - case DRM_FORMAT_RGB565: > - return 16; > - case DRM_FORMAT_XRGB8888: > - case DRM_FORMAT_ARGB8888: > - case DRM_FORMAT_ARGB2101010: > - case DRM_FORMAT_XRGB2101010: > - return 32; > - default: > - igt_assert(false); > - } > -} > - > static void start_busy_thread(struct igt_fb *fb) > { > int rc; > @@ -1121,7 +1138,7 @@ static void start_busy_thread(struct igt_fb *fb) > busy_thread.width = fb->width; > busy_thread.height = fb->height; > busy_thread.color = pick_color(fb, COLOR_PRIM_BG); > - busy_thread.bpp = fb_get_bpp(fb); > + busy_thread.bpp = format_get_bpp(fb->drm_format); > > rc = pthread_create(&busy_thread.thread, NULL, busy_thread_func, NULL); > igt_assert_eq(rc, 0); > -- > 2.6.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Dec 02, 2015 at 10:47:01AM -0200, Paulo Zanoni wrote: > We want to make sure that both tiled and untiled buffers have the same > size for the same width/height/format. This will allow better control > over the failure paths exercised by our tests: when we try to flip > from tiled to untiled, we'll be sure that we won't execute the error > path that checks for buffer sizes. > > v2: Use the new igt_calc_fb_size() instead of implementing our own > size calculation (Daniel). > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > tests/kms_frontbuffer_tracking.c | 51 ++++++++++++++++++++++++++-------------- > 1 file changed, 34 insertions(+), 17 deletions(-) > > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c > index 81703ec..3db95d2 100644 > --- a/tests/kms_frontbuffer_tracking.c > +++ b/tests/kms_frontbuffer_tracking.c > @@ -479,10 +479,28 @@ static bool init_modeset_cached_params(void) > return true; > } > > +static int format_get_bpp(uint32_t format) Ah, missed one: igt_drm_format_to_bpp please, and if it doesn't cover them all we need to fix that asap. -Daniel > +{ > + switch (format) { > + case DRM_FORMAT_RGB565: > + return 16; > + case DRM_FORMAT_XRGB8888: > + case DRM_FORMAT_ARGB8888: > + case DRM_FORMAT_ARGB2101010: > + case DRM_FORMAT_XRGB2101010: > + return 32; > + default: > + igt_assert(false); > + } > +} > + > static void create_fb(enum pixel_format pformat, int width, int height, > uint64_t tiling, int plane, struct igt_fb *fb) > { > uint32_t format; > + unsigned int size, stride; > + int bpp; > + uint64_t tiling_for_size; > > switch (pformat) { > case FORMAT_RGB888: > @@ -512,7 +530,21 @@ static void create_fb(enum pixel_format pformat, int width, int height, > igt_assert(false); > } > > - igt_create_fb(drm.fd, width, height, format, tiling, fb); > + /* We want all frontbuffers with the same width/height/format to have > + * the same size regardless of tiling since we want to properly exercise > + * the Kernel's specific tiling-checking code paths without accidentally > + * hitting size-checking ones first. */ > + bpp = format_get_bpp(format); > + if (plane == PLANE_CUR) > + tiling_for_size = LOCAL_DRM_FORMAT_MOD_NONE; > + else > + tiling_for_size = LOCAL_I915_FORMAT_MOD_X_TILED; > + > + igt_calc_fb_size(drm.fd, width, height, bpp, tiling_for_size, &size, > + &stride); > + > + igt_create_fb_with_bo_size(drm.fd, width, height, format, tiling, fb, > + size, stride); > } > > static uint32_t pick_color(struct igt_fb *fb, enum color ecolor) > @@ -1094,21 +1126,6 @@ static void *busy_thread_func(void *data) > pthread_exit(0); > } > > -static int fb_get_bpp(struct igt_fb *fb) > -{ > - switch (fb->drm_format) { > - case DRM_FORMAT_RGB565: > - return 16; > - case DRM_FORMAT_XRGB8888: > - case DRM_FORMAT_ARGB8888: > - case DRM_FORMAT_ARGB2101010: > - case DRM_FORMAT_XRGB2101010: > - return 32; > - default: > - igt_assert(false); > - } > -} > - > static void start_busy_thread(struct igt_fb *fb) > { > int rc; > @@ -1121,7 +1138,7 @@ static void start_busy_thread(struct igt_fb *fb) > busy_thread.width = fb->width; > busy_thread.height = fb->height; > busy_thread.color = pick_color(fb, COLOR_PRIM_BG); > - busy_thread.bpp = fb_get_bpp(fb); > + busy_thread.bpp = format_get_bpp(fb->drm_format); > > rc = pthread_create(&busy_thread.thread, NULL, busy_thread_func, NULL); > igt_assert_eq(rc, 0); > -- > 2.6.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Sex, 2015-12-04 às 16:28 +0100, Daniel Vetter escreveu: > On Wed, Dec 02, 2015 at 10:47:01AM -0200, Paulo Zanoni wrote: > > We want to make sure that both tiled and untiled buffers have the > > same > > size for the same width/height/format. This will allow better > > control > > over the failure paths exercised by our tests: when we try to flip > > from tiled to untiled, we'll be sure that we won't execute the > > error > > path that checks for buffer sizes. > > > > v2: Use the new igt_calc_fb_size() instead of implementing our own > > size calculation (Daniel). > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > tests/kms_frontbuffer_tracking.c | 51 ++++++++++++++++++++++++++ > > -------------- > > 1 file changed, 34 insertions(+), 17 deletions(-) > > > > diff --git a/tests/kms_frontbuffer_tracking.c > > b/tests/kms_frontbuffer_tracking.c > > index 81703ec..3db95d2 100644 > > --- a/tests/kms_frontbuffer_tracking.c > > +++ b/tests/kms_frontbuffer_tracking.c > > @@ -479,10 +479,28 @@ static bool init_modeset_cached_params(void) > > return true; > > } > > > > +static int format_get_bpp(uint32_t format) > > Ah, missed one: igt_drm_format_to_bpp please, and if it doesn't cover > them all we > need to fix that asap. I'm not sure if this is a good idea. Not every DRM format has a matching cairo format, and it seems the whole igt_fb code is built based on this assumption. On a quick look, it really seems that both kms_render.c and kms_atomic.c will break if I add ARGB2101010 with a matching CAIRO_INVALID (since they call igt_get_all_formats() and use cairo). I know, you can question the use of ARGB2101010 by kms_frontbuffer_tracking (we don't use it anymore, the format is there as an artifact of an older attempt when I initially added support for multiple formats), but that doesn't solve the bigger problem that we can't easily expand igt_drm_format_to_bpp(). If you still insist, the big plan should be to make sure that both igt_fb and the libs can properly handle the cases where a DRM format doesn't have a matching cairo format, but I don't want to block my current tasks on this. So I'd vote to merge my patches as-is for now. What do you think? > -Daniel > > > +{ > > + switch (format) { > > + case DRM_FORMAT_RGB565: > > + return 16; > > + case DRM_FORMAT_XRGB8888: > > + case DRM_FORMAT_ARGB8888: > > + case DRM_FORMAT_ARGB2101010: > > + case DRM_FORMAT_XRGB2101010: > > + return 32; > > + default: > > + igt_assert(false); > > + } > > +} > > + > > static void create_fb(enum pixel_format pformat, int width, int > > height, > > uint64_t tiling, int plane, struct igt_fb > > *fb) > > { > > uint32_t format; > > + unsigned int size, stride; > > + int bpp; > > + uint64_t tiling_for_size; > > > > switch (pformat) { > > case FORMAT_RGB888: > > @@ -512,7 +530,21 @@ static void create_fb(enum pixel_format > > pformat, int width, int height, > > igt_assert(false); > > } > > > > - igt_create_fb(drm.fd, width, height, format, tiling, fb); > > + /* We want all frontbuffers with the same > > width/height/format to have > > + * the same size regardless of tiling since we want to > > properly exercise > > + * the Kernel's specific tiling-checking code paths > > without accidentally > > + * hitting size-checking ones first. */ > > + bpp = format_get_bpp(format); > > + if (plane == PLANE_CUR) > > + tiling_for_size = LOCAL_DRM_FORMAT_MOD_NONE; > > + else > > + tiling_for_size = LOCAL_I915_FORMAT_MOD_X_TILED; > > + > > + igt_calc_fb_size(drm.fd, width, height, bpp, > > tiling_for_size, &size, > > + &stride); > > + > > + igt_create_fb_with_bo_size(drm.fd, width, height, format, > > tiling, fb, > > + size, stride); > > } > > > > static uint32_t pick_color(struct igt_fb *fb, enum color ecolor) > > @@ -1094,21 +1126,6 @@ static void *busy_thread_func(void *data) > > pthread_exit(0); > > } > > > > -static int fb_get_bpp(struct igt_fb *fb) > > -{ > > - switch (fb->drm_format) { > > - case DRM_FORMAT_RGB565: > > - return 16; > > - case DRM_FORMAT_XRGB8888: > > - case DRM_FORMAT_ARGB8888: > > - case DRM_FORMAT_ARGB2101010: > > - case DRM_FORMAT_XRGB2101010: > > - return 32; > > - default: > > - igt_assert(false); > > - } > > -} > > - > > static void start_busy_thread(struct igt_fb *fb) > > { > > int rc; > > @@ -1121,7 +1138,7 @@ static void start_busy_thread(struct igt_fb > > *fb) > > busy_thread.width = fb->width; > > busy_thread.height = fb->height; > > busy_thread.color = pick_color(fb, COLOR_PRIM_BG); > > - busy_thread.bpp = fb_get_bpp(fb); > > + busy_thread.bpp = format_get_bpp(fb->drm_format); > > > > rc = pthread_create(&busy_thread.thread, NULL, > > busy_thread_func, NULL); > > igt_assert_eq(rc, 0); > > -- > > 2.6.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Dec 07, 2015 at 02:04:51PM +0000, Zanoni, Paulo R wrote: > Em Sex, 2015-12-04 às 16:28 +0100, Daniel Vetter escreveu: > > On Wed, Dec 02, 2015 at 10:47:01AM -0200, Paulo Zanoni wrote: > > > We want to make sure that both tiled and untiled buffers have the > > > same > > > size for the same width/height/format. This will allow better > > > control > > > over the failure paths exercised by our tests: when we try to flip > > > from tiled to untiled, we'll be sure that we won't execute the > > > error > > > path that checks for buffer sizes. > > > > > > v2: Use the new igt_calc_fb_size() instead of implementing our own > > > size calculation (Daniel). > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > tests/kms_frontbuffer_tracking.c | 51 ++++++++++++++++++++++++++ > > > -------------- > > > 1 file changed, 34 insertions(+), 17 deletions(-) > > > > > > diff --git a/tests/kms_frontbuffer_tracking.c > > > b/tests/kms_frontbuffer_tracking.c > > > index 81703ec..3db95d2 100644 > > > --- a/tests/kms_frontbuffer_tracking.c > > > +++ b/tests/kms_frontbuffer_tracking.c > > > @@ -479,10 +479,28 @@ static bool init_modeset_cached_params(void) > > > return true; > > > } > > > > > > +static int format_get_bpp(uint32_t format) > > > > Ah, missed one: igt_drm_format_to_bpp please, and if it doesn't cover > > them all we > > need to fix that asap. > > I'm not sure if this is a good idea. Not every DRM format has a > matching cairo format, and it seems the whole igt_fb code is built > based on this assumption. On a quick look, it really seems that both > kms_render.c and kms_atomic.c will break if I add ARGB2101010 with a > matching CAIRO_INVALID (since they call igt_get_all_formats() and use > cairo). > > I know, you can question the use of ARGB2101010 by > kms_frontbuffer_tracking (we don't use it anymore, the format is there > as an artifact of an older attempt when I initially added support for > multiple formats), but that doesn't solve the bigger problem that we > can't easily expand igt_drm_format_to_bpp(). Hm, if you don't need it maybe push these patches without the new format and use the existing library function for now? > If you still insist, the big plan should be to make sure that both > igt_fb and the libs can properly handle the cases where a DRM format > doesn't have a matching cairo format, but I don't want to block my > current tasks on this. So I'd vote to merge my patches as-is for now. > > What do you think? igt_get_all_cairo_formats which skips on CAIRO_INVALID, roll it out in existing users, extend what we have here? Shouldn't be too much work, and we need this kind of stuff anyway. -Daniel
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c index 81703ec..3db95d2 100644 --- a/tests/kms_frontbuffer_tracking.c +++ b/tests/kms_frontbuffer_tracking.c @@ -479,10 +479,28 @@ static bool init_modeset_cached_params(void) return true; } +static int format_get_bpp(uint32_t format) +{ + switch (format) { + case DRM_FORMAT_RGB565: + return 16; + case DRM_FORMAT_XRGB8888: + case DRM_FORMAT_ARGB8888: + case DRM_FORMAT_ARGB2101010: + case DRM_FORMAT_XRGB2101010: + return 32; + default: + igt_assert(false); + } +} + static void create_fb(enum pixel_format pformat, int width, int height, uint64_t tiling, int plane, struct igt_fb *fb) { uint32_t format; + unsigned int size, stride; + int bpp; + uint64_t tiling_for_size; switch (pformat) { case FORMAT_RGB888: @@ -512,7 +530,21 @@ static void create_fb(enum pixel_format pformat, int width, int height, igt_assert(false); } - igt_create_fb(drm.fd, width, height, format, tiling, fb); + /* We want all frontbuffers with the same width/height/format to have + * the same size regardless of tiling since we want to properly exercise + * the Kernel's specific tiling-checking code paths without accidentally + * hitting size-checking ones first. */ + bpp = format_get_bpp(format); + if (plane == PLANE_CUR) + tiling_for_size = LOCAL_DRM_FORMAT_MOD_NONE; + else + tiling_for_size = LOCAL_I915_FORMAT_MOD_X_TILED; + + igt_calc_fb_size(drm.fd, width, height, bpp, tiling_for_size, &size, + &stride); + + igt_create_fb_with_bo_size(drm.fd, width, height, format, tiling, fb, + size, stride); } static uint32_t pick_color(struct igt_fb *fb, enum color ecolor) @@ -1094,21 +1126,6 @@ static void *busy_thread_func(void *data) pthread_exit(0); } -static int fb_get_bpp(struct igt_fb *fb) -{ - switch (fb->drm_format) { - case DRM_FORMAT_RGB565: - return 16; - case DRM_FORMAT_XRGB8888: - case DRM_FORMAT_ARGB8888: - case DRM_FORMAT_ARGB2101010: - case DRM_FORMAT_XRGB2101010: - return 32; - default: - igt_assert(false); - } -} - static void start_busy_thread(struct igt_fb *fb) { int rc; @@ -1121,7 +1138,7 @@ static void start_busy_thread(struct igt_fb *fb) busy_thread.width = fb->width; busy_thread.height = fb->height; busy_thread.color = pick_color(fb, COLOR_PRIM_BG); - busy_thread.bpp = fb_get_bpp(fb); + busy_thread.bpp = format_get_bpp(fb->drm_format); rc = pthread_create(&busy_thread.thread, NULL, busy_thread_func, NULL); igt_assert_eq(rc, 0);
We want to make sure that both tiled and untiled buffers have the same size for the same width/height/format. This will allow better control over the failure paths exercised by our tests: when we try to flip from tiled to untiled, we'll be sure that we won't execute the error path that checks for buffer sizes. v2: Use the new igt_calc_fb_size() instead of implementing our own size calculation (Daniel). Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- tests/kms_frontbuffer_tracking.c | 51 ++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 17 deletions(-)