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

From: Daniel Vetter <daniel@ffwll.ch>
To: Deepak Singh Rawat <drawat@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>,
	"petri.latvala@intel.com" <petri.latvala@intel.com>,
	Sinclair Yeh <syeh@vmware.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"igt-dev@lists.freedesktop.org" <igt-dev@lists.freedesktop.org>,
	linux-graphics-maintainer <linux-graphics-maintainer@vmware.com>
Subject: Re: [igt-dev] [PATCH i-g-t 4/6] lib: Don't call igt_require_fb_modifiers() when no modifier
Date: Thu, 11 Oct 2018 17:48:40 +0200	[thread overview]
Message-ID: <20181011154840.GG31561@phenom.ffwll.local> (raw)
In-Reply-To: <BYAPR05MB418182BBA7EF8892A5CB6D67BAE10@BYAPR05MB4181.namprd05.prod.outlook.com>

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