Re: [igt-dev] [PATCH 5/8] igt: fb: Move i915 buffer allocation to a function of its own

Ville Syrjälä
2018-12-04T19:28:34Z

On Tue, Dec 04, 2018 at 11:08:19AM +0100, Maxime Ripard wrote:
> The create_bo_for_fb function has a rather big, non trivial block of
> code to allocate buffers in some non-trivial situation on the i915
> driver.
>
> Since this is both non-trivial and driver specific, move that code
> into a function of its own so that we can rework that function later
> on.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
> lib/igt_fb.c | 89 +++++++++++++++++++++++++++--------------------------
> 1 file changed, 47 insertions(+), 42 deletions(-)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 2462d6ba6fab..d96689aa5fe6 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -453,6 +453,52 @@ uint64_t igt_fb_tiling_to_mod(uint64_t tiling)
> }
> }
>
> +static int i915_create_gem_for_fb(struct igt_fb *fb)
> +{
> + bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> + int fd = fb->fd;
> + void *ptr;
> +
> + fb->gem_handle = gem_create(fd, fb->size);
> + gem_set_tiling(fd, fb->gem_handle,
> + igt_fb_mod_to_tiling(fb->tiling),
> + fb->strides[0]);
> +
> + gem_set_domain(fd, fb->gem_handle,
> + I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +
> + /* Ensure the framebuffer is preallocated */
> + ptr = gem_mmap__gtt(fd, fb->gem_handle,
> + fb->size, PROT_READ | PROT_WRITE);
> + igt_assert(*(uint32_t *)ptr == 0);
> +
> + switch (fb->drm_format) {
> + case DRM_FORMAT_NV12:
> + memset(ptr + fb->offsets[0],
> + full_range ? 0x00 : 0x10,
> + fb->strides[0] * fb->plane_height[0]);
> + memset(ptr + fb->offsets[1],
> + 0x80,
> + fb->strides[1] * fb->plane_height[1]);
> + break;
> + case DRM_FORMAT_YUYV:
> + case DRM_FORMAT_YVYU:
> + wmemset(ptr + fb->offsets[0],
> + full_range ? 0x80008000 : 0x80108010,
> + fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> + break;
> + case DRM_FORMAT_UYVY:
> + case DRM_FORMAT_VYUY:
> + wmemset(ptr + fb->offsets[0],
> + full_range ? 0x00800080 : 0x10801080,
> + fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> + break;
> + }
> + gem_munmap(ptr, fb->size);

We should probably extract this memset() stuff into a separate
function because every driver will require it.

> +
> + return fb->gem_handle;
> +}
> +
> /* helpers to create nice-looking framebuffers */
> static int create_bo_for_fb(struct igt_fb *fb)
> {
> @@ -470,48 +516,7 @@ static int create_bo_for_fb(struct igt_fb *fb)
> fb->is_dumb = false;
>
> if (is_i915_device(fd)) {
> - void *ptr;
> - bool full_range = fb->color_range == IGT_COLOR_YCBCR_FULL_RANGE;
> -
> - fb->gem_handle = gem_create(fd, fb->size);
> -
> - gem_set_tiling(fd, fb->gem_handle,
> - igt_fb_mod_to_tiling(fb->tiling),
> - fb->strides[0]);
> -
> - gem_set_domain(fd, fb->gem_handle,
> - I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> -
> - /* Ensure the framebuffer is preallocated */
> - ptr = gem_mmap__gtt(fd, fb->gem_handle,
> - fb->size, PROT_READ | PROT_WRITE);
> - igt_assert(*(uint32_t *)ptr == 0);
> -
> - switch (fb->drm_format) {
> - case DRM_FORMAT_NV12:
> - memset(ptr + fb->offsets[0],
> - full_range ? 0x00 : 0x10,
> - fb->strides[0] * fb->plane_height[0]);
> - memset(ptr + fb->offsets[1],
> - 0x80,
> - fb->strides[1] * fb->plane_height[1]);
> - break;
> - case DRM_FORMAT_YUYV:
> - case DRM_FORMAT_YVYU:
> - wmemset(ptr + fb->offsets[0],
> - full_range ? 0x80008000 : 0x80108010,
> - fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> - break;
> - case DRM_FORMAT_UYVY:
> - case DRM_FORMAT_VYUY:
> - wmemset(ptr + fb->offsets[0],
> - full_range ? 0x00800080 : 0x10801080,
> - fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
> - break;
> - }
> - gem_munmap(ptr, fb->size);
> -
> - return fb->gem_handle;
> + return i915_create_gem_for_fb(fb);
> } else {
> bool driver_has_gem_api = false;
>
> --
> git-series 0.9.1

--
Ville Syrjälä
Intel