| Message ID | 20190403222439.jidjcpw7ywd6eomz@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] lib: Rework __kms_addfb() function | expand |
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.
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
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.
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
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
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); }
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(-)