[i-g-t] lib: Rework __kms_addfb() function
diff mbox series

Message ID 20190403222439.jidjcpw7ywd6eomz@smtp.gmail.com
State New
Headers show
Series
  • [i-g-t] lib: Rework __kms_addfb() function
Related show

Commit Message

Rodrigo Siqueira April 3, 2019, 10:24 p.m. UTC
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().

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 lib/ioctl_wrappers.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

Comments

Arkadiusz Hiler April 10, 2019, 11:28 a.m. UTC | #1
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@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@chromium.org>
    Reviewed-by: Rob Clark <robdclark@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@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.
Chris Wilson April 10, 2019, 11:34 a.m. UTC | #2
Quoting Arkadiusz Hiler (2019-04-10 12:28:08)
> 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'

Why introduce a dependency? The primary purpose of igt is breaking
ioctls, and that typically requires avoiding any protective libraries.
In particular, we want to be very, very clear about what igt is doing
(in terms of kernel api/abi) and not obfuscate.
-Chris
Arkadiusz Hiler April 10, 2019, 11:57 a.m. UTC | #3
On Wed, Apr 10, 2019 at 12:34:07PM +0100, Chris Wilson wrote:
> Quoting Arkadiusz Hiler (2019-04-10 12:28:08)
> > 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'
> 
> Why introduce a dependency? The primary purpose of igt is breaking
> ioctls, and that typically requires avoiding any protective libraries.
> In particular, we want to be very, very clear about what igt is doing
> (in terms of kernel api/abi) and not obfuscate.
> -Chris

We depend on one additional call from a library that we already use
widely across the whole codebase and it's straight up a simple wrapper.
Which seems to be a pattern - we us those simple wrapper where
available, and write our own if there is nothing suitable.

It's ok to have some tests that poke the IOCTL more directly, like we
kms_addfb_basic for ADDFB2 (which still uses drmIoctl, so where do we
draw a line?).

If any of thoe wrappers goes awry we can just drop-in replace it with
the original, simple version anyway.
Chris Wilson April 10, 2019, 11:59 a.m. UTC | #4
Quoting Arkadiusz Hiler (2019-04-10 12:57:03)
> On Wed, Apr 10, 2019 at 12:34:07PM +0100, Chris Wilson wrote:
> > Quoting Arkadiusz Hiler (2019-04-10 12:28:08)
> > > 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'
> > 
> > Why introduce a dependency? The primary purpose of igt is breaking
> > ioctls, and that typically requires avoiding any protective libraries.
> > In particular, we want to be very, very clear about what igt is doing
> > (in terms of kernel api/abi) and not obfuscate.
> > -Chris
> 
> We depend on one additional call from a library that we already use
> widely across the whole codebase and it's straight up a simple wrapper.
> Which seems to be a pattern - we us those simple wrapper where
> available, and write our own if there is nothing suitable.

I completely disagree with that assessment, and do not think we should
undermine igt so lightly.
-Chris
Rodrigo Siqueira April 18, 2019, noon UTC | #5
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@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@chromium.org>
>     Reviewed-by: Rob Clark <robdclark@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@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

Patch
diff mbox series

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};
 
 	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;
-		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;
+	return drmModeAddFB2WithModifiers(fd, width, height, pixel_format,
+					  handles, strides, offsets, modifiers,
+					  buf_id, flags);
 }