[igt-dev] [PATCH i-g-t] lib: Rework __kms_addfb() function

[Arkadiusz Hiler]

On Wed, Apr 03, 2019 at 07:24:39PM -0300, Rodrigo Siqueira wrote:
> The function __kms_addfb() and drmModeAddFB2WithModifiers() have a
> similar code. Due to this similarity, this commit replace part of the
> code inside __kms_addfb() by using drmModeAddFB2WithModifiers().

igt master % grep '^libdrm_version' meson.build
libdrm_version = '>=2.4.82'

libdrm master % git log -S drmModeAddFB2WithModifiers
commit abfa680dbdfa4600105d904f4903c047d453cdb5
Author: Kristian H. Kristensen <hoegsberg at chromium.org>
Date:   Thu Sep 8 13:08:59 2016 -0700

    Add drmModeAddFB2WithModifiers() which takes format modifiers

    The only other user of this feature open codes the ioctl. Let's add an
    entry point for this to libdrm.

    Signed-off-by: Kristian H. Kristensen <hoegsberg at chromium.org>
    Reviewed-by: Rob Clark <robdclark at gmail.com>

libdrm master % git describe abfa680
libdrm-2.4.70-15-gabfa680d

We are good on this front.

> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
> ---
>  lib/ioctl_wrappers.c | 27 ++++++---------------------
>  1 file changed, 6 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 39920f87..4240d138 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -46,6 +46,7 @@
>  #include <sys/utsname.h>
>  #include <termios.h>
>  #include <errno.h>
> +#include <xf86drmMode.h>
>  
>  #include "drmtest.h"
>  #include "i915_drm.h"
> @@ -1479,29 +1480,13 @@ int __kms_addfb(int fd, uint32_t handle,
>  		uint32_t strides[4], uint32_t offsets[4],
>  		int num_planes, uint32_t flags, uint32_t *buf_id)
>  {
> -	struct drm_mode_fb_cmd2 f;
> -	int ret, i;
> +	uint32_t handles[4] = {handle};
> +	uint64_t modifiers[4] = {modifier};

This notation will initialize first element to handle and zero out the
remining ones.

It is not equivalent to what is done below, where handle and modifier
are commpied to each of the num_planes first elements of the arrays.

>  	if (flags & DRM_MODE_FB_MODIFIERS)
>  		igt_require_fb_modifiers(fd);
>  
> -	memset(&f, 0, sizeof(f));
> -
> -	f.width  = width;
> -	f.height = height;
> -	f.pixel_format = pixel_format;
> -	f.flags = flags;
> -
> -	for (i = 0; i < num_planes; i++) {
> -		f.handles[i] = handle;
> -		f.modifier[i] = modifier;

here ^^^

Which may break testing if we ever call it with (num_planes > 1).

> -		f.pitches[i] = strides[i];
> -		f.offsets[i] = offsets[i];
> -	}
> -
> -	ret = igt_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &f);
> -
> -	*buf_id = f.fb_id;
> -
> -	return ret < 0 ? -errno : ret;

This also changes behavior, as we log return value of __kms_addfb in few
places, so we lose the information from errno and we would get -1 in
case of any failue now.

> +	return drmModeAddFB2WithModifiers(fd, width, height, pixel_format,
> +					  handles, strides, offsets, modifiers,
> +					  buf_id, flags);
>  }

igt master % ff __kms_addfb
tests/kms_draw_crc.c:166:9:             ret =  __kms_addfb(drm_fd, gem_handle, 64, 64,
tests/kms_available_modes_crc.c:228:8:  ret = __kms_addfb(data->gfx_fd, data->gem_handle, w, h,
tests/prime_vgem.c:765:15:              igt_require(__kms_addfb(i915, handle[i],
lib/igt_fb.c:1243:12:                   do_or_die(__kms_addfb(fb->fd, fb->gem_handle,
lib/ioctl_wrappers.h:217:5:             int __kms_addfb(int fd, uint32_t handle,
lib/ioctl_wrappers.c:1457:5:            int __kms_addfb(int fd, uint32_t handle,

We call __kms_addfb in 4 places only. It may be worth dropping this
ioctl wrapper introduced in 2015 (so prior to drmModeAddFB2WithModifiers
addition) altogether.

-- 
Cheers,
Arek