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

[Rodrigo Siqueira]

Hi,

First of all, thank you for your review.

I just have a few questions below.

On 04/10, Arkadiusz Hiler wrote:
> 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.

Nice catch, I didn’t know about that. I’ll fix it in the V2.

> 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.

I’m little bit confused here, because drmModeAddFB2WithModifiers() has
the following code:

 if ((ret = DRM_IOCTL(fd, DRM_IOCTL_MODE_ADDFB2, &f)))
    return ret;

In its turn, DRM_IOCTL(), has the following code:

static inline int DRM_IOCTL(int fd, unsigned long cmd, void *arg)
{
    int ret = drmIoctl(fd, cmd, arg);
    return ret < 0 ? -errno : ret;
}

Because of this, I thought that “return drmModeAddFB2WithModifiers()”
has the same behaviour of the previous code. Could you give me some
extra explanation on this issue?
 
> > +	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.

I agree about removing __kms_addfb(); however, I notice the following
side effect:

1. Probably, we’ll duplicate some pieces of code in those 4 functions.
2. The function igt_create_fb_with_bo_size() is used around all of the
   tests, and remove __kms_addfb() may impact in this function.

Anyway, what do you think about that? Should I prepare a V2 that removes
this function? I prefer to keep it based on the above points.

Best Regards,
Rodrigo Siqueira

> -- 
> Cheers,
> Arek

-- 
Rodrigo Siqueira
https://siqueira.tech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20190418/62d3c53b/attachment.sig>