Re: [igt-dev] [PATCH 1/2] lib/ioctl_wrappers: Query if device supports set/get legacy tiling

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Fri, 15 Nov 2019 10:51:54 -0800, Vanshidhar Konda wrote:
>
> On Fri, Nov 15, 2019 at 10:16:19AM -0800, Dixit, Ashutosh wrote:
> > On Thu, 14 Nov 2019 21:50:39 -0800, Vanshidhar Konda wrote:
> >>
> >> On Thu, Nov 14, 2019 at 09:42:24PM -0800, Dixit, Ashutosh wrote:
> >> > On Thu, 14 Nov 2019 21:33:25 -0800, Vanshidhar Konda wrote:
> >> >>
> >> >> Add a method to query if the device supports setting and
> >> >> getting legacy tiling formats for buffer objects.
> >> >>
> >> >> Signed-off-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>
> >> >> ---
> >> >>  lib/ioctl_wrappers.c | 16 ++++++++++++++++
> >> >>  lib/ioctl_wrappers.h |  1 +
> >> >>  2 files changed, 17 insertions(+)
> >> >>
> >> >> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> >> >> index 628f8b83..837ef2cf 100644
> >> >> --- a/lib/ioctl_wrappers.c
> >> >> +++ b/lib/ioctl_wrappers.c
> >> >> @@ -133,6 +133,22 @@ __gem_get_tiling(int fd, struct drm_i915_gem_get_tiling *arg)
> >> >>	return err;
> >> >>  }
> >> >>
> >> >> +/**
> >> >> + * gem_has_legacy_hw_tiling:
> >> >> + * @fd: open i915 drm file descriptor
> >> >> + *
> >> >> + * Feature check to query if the device supports setting/getting
> >> >> + * legacy tiling formats for buffer objects
> >> >> + *
> >> >> + * Returns: True if tiling is supported
> >> >> + */
> >> >> +bool
> >> >> +gem_has_legacy_hw_tiling(int fd)
> >> >> +{
> >> >> +	struct drm_i915_gem_get_tiling arg = {};
> >> >> +	return (__gem_get_tiling(fd, &arg) != -EOPNOTSUPP);
> >> >
> >> > Probably just
> >> >
> >> >	return !__gem_get_tiling(fd, &arg);
> >>
> >> No. Only this specific error means it is not supported by the hardware.
> >> Other errors could mean that I made a mistake with the arguments - I've
> >> not set them to valid values.
> >
> > __gem_get_tiling() returns 0 if tiling is supported, it returns -EOPNOTSUPP
> > if tiling is not supported and if it returns anything else we don't know if
> > tiling is supported or not supported (error in arguments etc.). So if
> > gem_has_legacy_hw_tiling() is to definitively indicate tiling is supported
> > it should just:
> >
> >	return !__gem_get_tiling(fd, &arg);
> >
> > For example, if __gem_get_tiling() returns -EINVAL I don't think we want to
> > return true from gem_has_legacy_hw_tiling().
>
> The basis of my change is the assumption that GET/SET_TILING is
> supported by default. All user mode code today is written with the
> assumption that this is true. Only the return of EOPNOTSUPP indicates that
> it is not supported on the device. Even return of EINVAL would mean that it is
> supported but the argument passed is incorrect.

OK, finally understand that this way of doing it allows
gem_has_legacy_hw_tiling() be called without a valid gem object. A comment
would help but since the function doesn't take a gem object as an input
anyway, I guess it's fine as is too.

Perhaps need an empty line between the declaration and the following
statement to keep static checkers happy?

With that:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev