Re: [igt-dev] [PATCH i-g-t v5 03/22] lib/igt_fb: Refactor create_bo_for_fb to prepare for VC4 support

Lyude Paul
2019-02-20T19:28:49Z

On Wed, 2019-02-20 at 16:38 +0100, Paul Kocialkowski wrote:
> The current create_bo_for_fb uses a device-specific BO instead of dumb
> allocation when dumb allocation is not appropriate and the driver is
> Intel. Then, it will assert that the parameters are appropriate for
> dumb allocation.
>
> The conditions related to tiling, size and stride are sufficient for
> needing a device-specific BO and they are not specific to Intel.
> However, a device-specific BO for YUV is only needed for Intel.
>
> Change the conditions accordingly and set a device_bo variable. This
> variable allows making fb->size calculation common between the
> device-specific and dumb paths. Use the variable after that and
> distinguish between the device types for allocating and error out if
> it's not supported.
>
> This makes the extra checks that dumb allocation is valid redundant,
> since these cases will always fall under device-specific allocation
> and potentially error out then.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
> lib/igt_fb.c | 43 ++++++++++++++++++++++---------------------
> 1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 058cfab604b8..60e6bb00b55c 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -610,36 +610,37 @@ static int create_bo_for_fb(struct igt_fb *fb)
> unsigned int bpp = 0;
> unsigned int plane;
> unsigned *strides = &fb->strides[0];
> + bool device_bo = false;
> int fd = fb->fd;
>
> - if (is_i915_device(fd) &&
> - (fb->tiling || fb->size || fb->strides[0] || igt_format_is_yuv(fb-
> >drm_format))) {
> - uint64_t size;
> -
> - size = calc_fb_size(fb);
> + /*
> + * The current dumb buffer allocation API doesn't really allow to
> + * specify a custom size or stride. Yet the caller is free to specify
> + * them, so we need to make sure to use a device BO then.
> + */
> + if (fb->tiling || fb->size || fb->strides[0] ||
> + (is_i915_device(fd) && igt_format_is_yuv(fb->drm_format)))
> + device_bo = true;
>
> - /* respect the size requested by the caller */
> - if (fb->size == 0)
> - fb->size = size;
> + /* Respect the size requested by the caller. */
> + if (fb->size == 0)
> + fb->size = calc_fb_size(fb);
>
> + if (device_bo) {
> fb->is_dumb = false;
> - 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]);
> +
> + if (is_i915_device(fd)) {
> + 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]);
> + } else {
> + igt_assert(false);
> + }
>
> goto out;
> }
>
> - /*
> - * The current dumb buffer allocation API doesn't really allow to
> - * specify a custom size or stride. Yet the caller is free to specify
> - * them, so we need to make sure to error out in this case.
> - */
> - igt_assert(fb->size == 0);
> - igt_assert(fb->strides[0] == 0);
> -
> - fb->size = calc_fb_size(fb);
> for (plane = 0; plane < fb->num_planes; plane++)
> bpp += DIV_ROUND_UP(fb->plane_bpp[plane],
> plane ? fmt->hsub * fmt->vsub : 1);
--
Cheers,
Lyude Paul