| Message ID | 20181011002104.1845-4-drawat@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t,1/6] lib/igt_vmwgfx: Add vmwgfx device | expand |
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? > > memset(&f, 0, sizeof(f)); > > -- > 2.17.1 > > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 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
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
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; memset(&f, 0, sizeof(f));
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(-)