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@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
> Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
>
> libdrm master % git describe abfa680
> libdrm-2.4.70-15-gabfa680d
>
> We are good on this front.
>
> >
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
