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

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

[Rodrigo Siqueira]

[-- Attachment #1.1: Type: text/plain, Size: 1636 bytes --]

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(-)

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);
 }
-- 
2.21.0

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Siqueira]

[-- Attachment #1.1: Type: text/plain, Size: 1636 bytes --]

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(-)

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);
 }
-- 
2.21.0

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for lib: Rework __kms_addfb() function

[Patchwork]

== Series Details ==

Series: lib: Rework __kms_addfb() function
URL   : https://patchwork.freedesktop.org/series/58969/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5867 -> IGTPW_2781
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_2781 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2781, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58969/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_2781:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_ctx_exec@basic:
    - fi-icl-u2:          PASS -> INCOMPLETE

  
Known issues
------------

  Here are the changes found in IGTPW_2781 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-byt-clapper:     PASS -> INCOMPLETE [fdo#102657]

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      PASS -> FAIL [fdo#108511]

  * igt@i915_selftest@live_contexts:
    - fi-bdw-gvtdvm:      PASS -> DMESG-FAIL [fdo#110235 ]

  * igt@i915_selftest@live_hangcheck:
    - fi-skl-iommu:       PASS -> INCOMPLETE [fdo#108602] / [fdo#108744]

  * igt@i915_selftest@live_uncore:
    - fi-ivb-3770:        PASS -> DMESG-FAIL [fdo#110210]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - fi-ivb-3770:        PASS -> SKIP [fdo#109271] +1

  * igt@runner@aborted:
    - fi-skl-iommu:       NOTRUN -> FAIL [fdo#104108] / [fdo#108602]

  
#### Possible fixes ####

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-u3}:        FAIL [fdo#103167] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102657]: https://bugs.freedesktop.org/show_bug.cgi?id=102657
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210
  [fdo#110235 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110235 


Participating hosts (50 -> 44)
------------------------------

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus 


Build changes
-------------

    * IGT: IGT_4926 -> IGTPW_2781

  CI_DRM_5867: 4a41303673f5b18b2b176182dd220d455f33d204 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2781: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2781/
  IGT_4926: c9a9cf357b6b2a304623790bf8dae797e12888a8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2781/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [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@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.

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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.

-- 
Cheers,
Arek
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Arkadiusz Hiler]

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.

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Arkadiusz Hiler]

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.

-- 
Cheers,
Arek
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Siqueira]

[-- Attachment #1.1: Type: text/plain, Size: 5198 bytes --]

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

-- 
Rodrigo Siqueira
https://siqueira.tech

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] lib: Rework __kms_addfb() function

[Rodrigo Siqueira]

[-- Attachment #1.1: Type: text/plain, Size: 5198 bytes --]

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

-- 
Rodrigo Siqueira
https://siqueira.tech

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx