Re: [igt-dev] [PATCH i-g-t 4/6] lib: Don't call igt_require_fb_modifiers() when no modifier

Deepak Singh Rawat
2018-10-11T15:38:11Z

> On Wed, Oct 10, 2018 at 05:21:02PM -0700, Deepak Rawat wrote:
> > vmwgfx doesn't support fb modifier so skip igt_require_fb_modifiers()
> > when modifier are not passed.
> >
> > Signed-off-by: Deepak Rawat <drawat@vmware.com>
> > ---
> > lib/ioctl_wrappers.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > index 0929c43f..3a11cb6e 100644
> > --- a/lib/ioctl_wrappers.c
> > +++ b/lib/ioctl_wrappers.c
> > @@ -1678,7 +1678,10 @@ int __kms_addfb(int fd, uint32_t handle,
> > struct drm_mode_fb_cmd2 f;
> > int ret, i;
> >
> > - igt_require_fb_modifiers(fd);
> > + if (modifier)
> > + igt_require_fb_modifiers(fd);
> > + else
> > + flags &= ~DRM_MODE_FB_MODIFIERS;
>
> This would theoretically change the behviour for i915 at least. Without
> the modifiers flag the kernel will pick the modifier for us based on
> the bo tiling, which in theory might not be what we wanted. But at least
> igt_fb should be fine with that.
>
> Maybe it would be better to just not pass the flags from the caller at
> all, and instead have __kms_addfb() check if the driver has modifiers
> or not and set the flag based on that?
>

Thanks Ville for the review. I thought of checking for modifiers support
in __kms_addfb() but I discarded the idea as it looked an overkill to me,
checking each time __kms_addfb() is called. May be static variable will
be a good idea? And yes resetting the flag from caller looks a better way.

Thanks,
Deepak