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

[Daniel Vetter]

On Thu, Oct 11, 2018 at 03:38:11PM +0000, Deepak Singh Rawat wrote:
> 
> > 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 at 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.

I think simply removing the modifier flag iff the driver doesn't support
modifiers _and_ the modifer is 0 would be ok. And yes probably better to
do that in the higher-level calls that care. Just converting igt_fb should
catch a lot of tests.

There's 2 more crc tests you might care about, but vmwgfx doesn't have crc
support so not really relevant. And maybe by then you do have modifier
support :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch