Re: [igt-dev] [PATCH i-g-t 1/3] lib/igt_fb: Allow creating yuv fbs with different encodings/ranges

Shankar, Uma
2019-07-02T14:47:55Z

>-----Original Message-----
>From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
>Sent: Saturday, June 29, 2019 1:15 AM
>To: igt-dev@lists.freedesktop.org
>Cc: Shankar, Uma <uma.shankar@intel.com>
>Subject: [PATCH i-g-t 1/3] lib/igt_fb: Allow creating yuv fbs with different
>encodings/ranges
>
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>Provide alternate versions of the fb creation functions that allow the caller to specify
>the yuv color encoding/range.
>
>Cc: Uma Shankar <uma.shankar@intel.com>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> lib/igt_fb.c | 207 +++++++++++++++++++++++++++----
> lib/igt_fb.h | 29 ++++-
> tests/kms_flip.c | 6 +-
> tests/kms_frontbuffer_tracking.c | 6 +-
> 4 files changed, 217 insertions(+), 31 deletions(-)
>
>diff --git a/lib/igt_fb.c b/lib/igt_fb.c index 415a3d65d24b..86988d6ca805 100644
>--- a/lib/igt_fb.c
>+++ b/lib/igt_fb.c
>@@ -1415,6 +1415,8 @@ void igt_paint_image(cairo_t *cr, const char *filename,
> * @height: height of the framebuffer in pixel
> * @format: drm fourcc pixel format code
> * @modifier: tiling layout of the framebuffer (as framebuffer modifier)
>+ * @color_encoding: color encoding for YCbCr formats (ignored
>+ otherwise)
>+ * @color_range: color range for YCbCr formats (ignored otherwise)
> * @fb: pointer to an #igt_fb structure
> * @bo_size: size of the backing bo (0 for automatic size)
> * @bo_stride: stride of the backing bo (0 for automatic stride) @@ -1432,12
>+1434,11 @@ void igt_paint_image(cairo_t *cr, const char *filename, unsigned int
>igt_create_fb_with_bo_size(int fd, int width, int height,
> uint32_t format, uint64_t modifier,
>+ enum igt_color_encoding color_encoding,
>+ enum igt_color_range color_range,
> struct igt_fb *fb, uint64_t bo_size,
> unsigned bo_stride)
> {
>- /* FIXME allow the caller to pass these in */
>- enum igt_color_encoding color_encoding = IGT_COLOR_YCBCR_BT709;
>- enum igt_color_range color_range = IGT_COLOR_YCBCR_LIMITED_RANGE;
> uint32_t flags = 0;
>
> fb_init(fb, fd, width, height, format, modifier, @@ -1471,6 +1472,38 @@
>igt_create_fb_with_bo_size(int fd, int width, int height,
> return fb->fb_id;
> }
>
>+/**
>+ * igt_create_fb_full:
>+ * @fd: open i915 drm file descriptor
>+ * @width: width of the framebuffer in pixel
>+ * @height: height of the framebuffer in pixel
>+ * @format: drm fourcc pixel format code
>+ * @modifier: tiling layout of the framebuffer
>+ * @color_encoding: color encoding for YCbCr formats (ignored
>+otherwise)
>+ * @color_range: color range for YCbCr formats (ignored otherwise)
>+ * @fb: pointer to an #igt_fb structure
>+ *
>+ * This function allocates a gem buffer object suitable to back a
>+framebuffer
>+ * with the requested properties and then wraps it up in a drm
>+framebuffer
>+ * object. All metadata is stored in @fb.
>+ *
>+ * The backing storage of the framebuffer is filled with all zeros,
>+i.e. black
>+ * for rgb pixel formats.
>+ *
>+ * Returns:
>+ * The kms id of the created framebuffer.
>+ */
>+unsigned int igt_create_fb_full(int fd, int width, int height,
>+ uint32_t format, uint64_t modifier,
>+ enum igt_color_encoding color_encoding,
>+ enum igt_color_range color_range,
>+ struct igt_fb *fb)
>+{
>+ return igt_create_fb_with_bo_size(fd, width, height, format, modifier,
>+ color_encoding, color_range,
>+ fb, 0, 0);
>+}
>+
> /**
> * igt_create_fb:
> * @fd: open i915 drm file descriptor
>@@ -1490,11 +1523,59 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
> * Returns:
> * The kms id of the created framebuffer.
> */
>-unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
>- uint64_t modifier, struct igt_fb *fb)
>+unsigned int igt_create_fb(int fd, int width, int height,
>+ uint32_t format, uint64_t modifier,
>+ struct igt_fb *fb)
> {
>- return igt_create_fb_with_bo_size(fd, width, height, format, modifier, fb,
>- 0, 0);
>+ return igt_create_fb_full(fd, width, height, format, modifier,
>+ IGT_COLOR_YCBCR_BT709,
>+ IGT_COLOR_YCBCR_LIMITED_RANGE, fb); }
>+
>+/**
>+ * igt_create_color_fb_full:
>+ * @fd: open i915 drm file descriptor
>+ * @width: width of the framebuffer in pixel
>+ * @height: height of the framebuffer in pixel
>+ * @format: drm fourcc pixel format code
>+ * @modifier: tiling layout of the framebuffer
>+ * @color_encoding: color encoding for YCbCr formats (ignored
>+otherwise)
>+ * @color_range: color range for YCbCr formats (ignored otherwise)
>+ * @r: red value to use as fill color
>+ * @g: green value to use as fill color
>+ * @b: blue value to use as fill color
>+ * @fb: pointer to an #igt_fb structure
>+ *
>+ * This function allocates a gem buffer object suitable to back a
>+framebuffer
>+ * with the requested properties and then wraps it up in a drm
>+framebuffer
>+ * object. All metadata is stored in @fb.
>+ *
>+ * Compared to igt_create_fb() this function also fills the entire
>+framebuffer
>+ * with the given color, which is useful for some simple pipe crc based tests.
>+ *
>+ * Returns:
>+ * The kms id of the created framebuffer on success or a negative error
>+code on
>+ * failure.
>+ */
>+unsigned int igt_create_color_fb_full(int fd, int width, int height,
>+ uint32_t format, uint64_t modifier,
>+ enum igt_color_encoding color_encoding,
>+ enum igt_color_range color_range,
>+ double r, double g, double b,
>+ struct igt_fb *fb /* out */)
>+{
>+ unsigned int fb_id;
>+ cairo_t *cr;
>+
>+ fb_id = igt_create_fb_full(fd, width, height, format, modifier,
>+ color_encoding, color_range, fb);
>+ igt_assert(fb_id);
>+
>+ cr = igt_get_cairo_ctx(fd, fb);
>+ igt_paint_color(cr, 0, 0, width, height, r, g, b);
>+ igt_put_cairo_ctx(fd, fb, cr);
>+
>+ return fb_id;
> }
>
> /**
>@@ -1524,15 +1605,51 @@ unsigned int igt_create_color_fb(int fd, int width, int
>height,
> uint32_t format, uint64_t modifier,
> double r, double g, double b,
> struct igt_fb *fb /* out */)
>+{
>+ return igt_create_color_fb_full(fd, width, height,
>+ format, modifier,
>+ IGT_COLOR_YCBCR_BT709,
>+ IGT_COLOR_YCBCR_LIMITED_RANGE,
>+ r, g, b, fb);
>+}
>+
>+/**
>+ * igt_create_pattern_fb_full:
>+ * @fd: open i915 drm file descriptor
>+ * @width: width of the framebuffer in pixel
>+ * @height: height of the framebuffer in pixel
>+ * @format: drm fourcc pixel format code
>+ * @modifier: tiling layout of the framebuffer
>+ * @color_encoding: color encoding for YCbCr formats (ignored
>+otherwise)
>+ * @color_range: color range for YCbCr formats (ignored otherwise)
>+ * @fb: pointer to an #igt_fb structure
>+ *
>+ * This function allocates a gem buffer object suitable to back a
>+framebuffer
>+ * with the requested properties and then wraps it up in a drm
>+framebuffer
>+ * object. All metadata is stored in @fb.
>+ *
>+ * Compared to igt_create_fb() this function also draws the standard
>+test pattern
>+ * into the framebuffer.
>+ *
>+ * Returns:
>+ * The kms id of the created framebuffer on success or a negative error
>+code on
>+ * failure.
>+ */
>+unsigned int igt_create_pattern_fb_full(int fd, int width, int height,
>+ uint32_t format, uint64_t modifier,
>+ enum igt_color_encoding color_encoding,
>+ enum igt_color_range color_range,
>+ struct igt_fb *fb /* out */)
> {
> unsigned int fb_id;
> cairo_t *cr;
>
>- fb_id = igt_create_fb(fd, width, height, format, modifier, fb);
>+ fb_id = igt_create_fb_full(fd, width, height, format, modifier,
>+ color_encoding, color_range, fb);
> igt_assert(fb_id);
>
> cr = igt_get_cairo_ctx(fd, fb);
>- igt_paint_color(cr, 0, 0, width, height, r, g, b);
>+ igt_paint_test_pattern(cr, width, height);
> igt_put_cairo_ctx(fd, fb, cr);
>
> return fb_id;
>@@ -1561,14 +1678,56 @@ unsigned int igt_create_color_fb(int fd, int width, int
>height, unsigned int igt_create_pattern_fb(int fd, int width, int height,
> uint32_t format, uint64_t modifier,
> struct igt_fb *fb /* out */)
>+{
>+ return igt_create_pattern_fb_full(fd, width, height,
>+ format, modifier,
>+ IGT_COLOR_YCBCR_BT709,
>+ IGT_COLOR_YCBCR_LIMITED_RANGE,
>+ fb);
>+}
>+
>+/**
>+ * igt_create_color_pattern_fb_full:
>+ * @fd: open i915 drm file descriptor
>+ * @width: width of the framebuffer in pixel
>+ * @height: height of the framebuffer in pixel
>+ * @format: drm fourcc pixel format code
>+ * @modifier: tiling layout of the framebuffer
>+ * @color_encoding: color encoding for YCbCr formats (ignored
>+otherwise)
>+ * @color_range: color range for YCbCr formats (ignored otherwise)
>+ * @r: red value to use as fill color
>+ * @g: green value to use as fill color
>+ * @b: blue value to use as fill color
>+ * @fb: pointer to an #igt_fb structure
>+ *
>+ * This function allocates a gem buffer object suitable to back a
>+framebuffer
>+ * with the requested properties and then wraps it up in a drm
>+framebuffer
>+ * object. All metadata is stored in @fb.
>+ *
>+ * Compared to igt_create_fb() this function also fills the entire
>+framebuffer
>+ * with the given color, and then draws the standard test pattern into
>+the
>+ * framebuffer.
>+ *
>+ * Returns:
>+ * The kms id of the created framebuffer on success or a negative error
>+code on
>+ * failure.
>+ */
>+unsigned int igt_create_color_pattern_fb_full(int fd, int width, int height,
>+ uint32_t format, uint64_t modifier,
>+ enum igt_color_encoding
>color_encoding,
>+ enum igt_color_range color_range,
>+ double r, double g, double b,
>+ struct igt_fb *fb /* out */)
> {
> unsigned int fb_id;
> cairo_t *cr;
>
>- fb_id = igt_create_fb(fd, width, height, format, modifier, fb);
>+ fb_id = igt_create_fb_full(fd, width, height, format, modifier,
>+ color_encoding, color_range, fb);
> igt_assert(fb_id);
>
> cr = igt_get_cairo_ctx(fd, fb);
>+ igt_paint_color(cr, 0, 0, width, height, r, g, b);
> igt_paint_test_pattern(cr, width, height);
> igt_put_cairo_ctx(fd, fb, cr);
>
>@@ -1582,6 +1741,8 @@ unsigned int igt_create_pattern_fb(int fd, int width, int
>height,
> * @height: height of the framebuffer in pixel
> * @format: drm fourcc pixel format code
> * @modifier: tiling layout of the framebuffer
>+ * @color_encoding: color encoding for YCbCr formats (ignored
>+ otherwise)
>+ * @color_range: color range for YCbCr formats (ignored otherwise)
> * @r: red value to use as fill color
> * @g: green value to use as fill color
> * @b: blue value to use as fill color
>@@ -1604,18 +1765,11 @@ unsigned int igt_create_color_pattern_fb(int fd, int
>width, int height,
> double r, double g, double b,
> struct igt_fb *fb /* out */)
> {
>- unsigned int fb_id;
>- cairo_t *cr;
>-
>- fb_id = igt_create_fb(fd, width, height, format, modifier, fb);
>- igt_assert(fb_id);
>-
>- cr = igt_get_cairo_ctx(fd, fb);
>- igt_paint_color(cr, 0, 0, width, height, r, g, b);
>- igt_paint_test_pattern(cr, width, height);
>- igt_put_cairo_ctx(fd, fb, cr);
>-
>- return fb_id;
>+ return igt_create_color_pattern_fb_full(fd, width, height,
>+ format, modifier,
>+ IGT_COLOR_YCBCR_BT709,
>+
> IGT_COLOR_YCBCR_LIMITED_RANGE,
>+ r, g, b, fb);
> }
>
> /**
>@@ -1745,8 +1899,8 @@ unsigned int igt_create_stereo_fb(int drm_fd,
>drmModeModeInfo *mode,
> struct igt_fb fb;
>
> stereo_fb_layout_from_mode(&layout, mode);
>- fb_id = igt_create_fb(drm_fd, layout.fb_width, layout.fb_height, format,
>- modifier, &fb);
>+ fb_id = igt_create_fb(drm_fd, layout.fb_width, layout.fb_height,
>+ format, modifier, &fb);
> cr = igt_get_cairo_ctx(drm_fd, &fb);
>
> igt_paint_image(cr, "1080p-left.png",
>@@ -3329,7 +3483,10 @@ unsigned int igt_fb_convert_with_stride(struct igt_fb
>*dst, struct igt_fb *src,
>
> fb_id = igt_create_fb_with_bo_size(src->fd, src->width,
> src->height, dst_fourcc,
>- dst_modifier, dst, 0,
>+ dst_modifier,
>+ IGT_COLOR_YCBCR_BT709,
>+ IGT_COLOR_YCBCR_LIMITED_RANGE,
>+ dst, 0,
> dst_stride);
> igt_assert(fb_id > 0);
>
>diff --git a/lib/igt_fb.h b/lib/igt_fb.h index be786911d577..db936b1608a6 100644
>--- a/lib/igt_fb.h
>+++ b/lib/igt_fb.h
>@@ -120,17 +120,42 @@ void igt_calc_fb_size(int fd, int width, int height, uint32_t
>format, uint64_t m unsigned int igt_create_fb_with_bo_size(int fd, int width, int
>height,
> uint32_t format, uint64_t modifier,
>+ enum igt_color_encoding color_encoding,
>+ enum igt_color_range color_range,
> struct igt_fb *fb, uint64_t bo_size,
> unsigned bo_stride);
>-unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
>- uint64_t modifier, struct igt_fb *fb);
>+unsigned int igt_create_fb_full(int fd, int width, int height,
>+ uint32_t format, uint64_t modifier,
>+ enum igt_color_encoding color_encoding,
>+ enum igt_color_range color_range,
>+ struct igt_fb *fb /* out */);
>+unsigned int igt_create_fb(int fd, int width, int height,
>+ uint32_t format, uint64_t modifier,
>+ struct igt_fb *fb /* out */);
>+unsigned int igt_create_color_fb_full(int fd, int width, int height,
>+ uint32_t format, uint64_t modifier,
>+ enum igt_color_encoding color_encoding,
>+ enum igt_color_range color_range,
>+ double r, double g, double b,
>+ struct igt_fb *fb /* out */);

I feel the usage of some of these new helpers is limited.
igt_create_color_fb_full is being used at just 2 instances. So I guess we
can either consolidate them or let the caller add those extra steps being done as
part of the helpers.

Will leave it your discretion. Overall the changes looks good.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> unsigned int igt_create_color_fb(int fd, int width, int height,
> uint32_t format, uint64_t modifier,
> double r, double g, double b,
> struct igt_fb *fb /* out */);
>+unsigned int igt_create_pattern_fb_full(int fd, int width, int height,
>+ uint32_t format, uint64_t modifier,
>+ enum igt_color_encoding color_encoding,
>+ enum igt_color_range color_range,
>+ struct igt_fb *fb /* out */);
> unsigned int igt_create_pattern_fb(int fd, int width, int height,
> uint32_t format, uint64_t modifier,
> struct igt_fb *fb /* out */);
>+unsigned int igt_create_color_pattern_fb_full(int fd, int width, int height,
>+ uint32_t format, uint64_t modifier,
>+ enum igt_color_encoding
>color_encoding,
>+ enum igt_color_range color_range,
>+ double r, double g, double b,
>+ struct igt_fb *fb /* out */);
> unsigned int igt_create_color_pattern_fb(int fd, int width, int height,
> uint32_t format, uint64_t modifier,
> double r, double g, double b,
>diff --git a/tests/kms_flip.c b/tests/kms_flip.c index 0a8d1c55a7aa..ef2521f21a42
>100755
>--- a/tests/kms_flip.c
>+++ b/tests/kms_flip.c
>@@ -1247,8 +1247,10 @@ static void run_test_on_crtc_set(struct test_output *o,
>int *crtc_idxs,
> igt_bpp_depth_to_drm_format(o->bpp, o-
>>depth),
> tiling, &o->fb_info[0]);
> o->fb_ids[1] = igt_create_fb_with_bo_size(drm_fd, o->fb_width, o-
>>fb_height,
>- igt_bpp_depth_to_drm_format(o->bpp, o-
>>depth),
>- tiling, &o->fb_info[1], bo_size, 0);
>+ igt_bpp_depth_to_drm_format(o-
>>bpp, o->depth),
>+ tiling, IGT_COLOR_YCBCR_BT709,
>+
>IGT_COLOR_YCBCR_LIMITED_RANGE,
>+ &o->fb_info[1], bo_size, 0);
>
> igt_assert(o->fb_ids[0]);
> igt_assert(o->fb_ids[1]);
>diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
>index 1037faf8e6f7..c788b59eefba 100644
>--- a/tests/kms_frontbuffer_tracking.c
>+++ b/tests/kms_frontbuffer_tracking.c
>@@ -491,8 +491,10 @@ static void create_fb(enum pixel_format pformat, int width,
>int height,
> igt_calc_fb_size(drm.fd, width, height, format, tiling_for_size, &size,
> &stride);
>
>- igt_create_fb_with_bo_size(drm.fd, width, height, format, tiling, fb,
>- size, stride);
>+ igt_create_fb_with_bo_size(drm.fd, width, height, format, tiling,
>+ IGT_COLOR_YCBCR_BT709,
>+ IGT_COLOR_YCBCR_LIMITED_RANGE,
>+ fb, size, stride);
> }
>
> static uint32_t pick_color(struct igt_fb *fb, enum color ecolor)
>--
>2.21.0