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

[Vanshidhar Konda]

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 at 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.

Vanshi

>
>