[Intel-gfx] [PATCH] tests/kms_addfb: Add support for fb modifiers

[Tvrtko Ursulin]

On 02/06/2015 09:33 AM, Daniel Vetter wrote:
> On Thu, Feb 05, 2015 at 02:50:09PM +0000, Tvrtko Ursulin wrote:
>>
>> On 02/05/2015 02:21 PM, Daniel Vetter wrote:
>>> On Wed, Feb 04, 2015 at 04:42:14PM +0000, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> Just a few basic tests to make sure fb modifiers can be used and
>>>> behave sanely when mixed with the old set_tiling API.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> ---
>>>>   lib/ioctl_wrappers.c | 45 ++++++++++++++++++++++++++++++++++++++
>>>>   lib/ioctl_wrappers.h | 36 ++++++++++++++++++++++++++++++
>>>>   tests/kms_addfb.c    | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 143 insertions(+)
>>>>
>>>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
>>>> index 19a457a..bca6d2a 100644
>>>> --- a/lib/ioctl_wrappers.c
>>>> +++ b/lib/ioctl_wrappers.c
>>>> @@ -1091,3 +1091,48 @@ int gem_context_has_param(int fd, uint64_t param)
>>>>
>>>>   	return gem_context_get_param(fd, &p) == 0;
>>>>   }
>>>> +
>>>
>>> gtkdoc is missing here. Easiest way to avoid it is to just move these two
>>> wrappers into the kms_fb testcase. Whomever needs to reuse these gets to
>>> write the docs.
>>
>> I'll need it so will fix myself.
>
> A then we'll need to do some proper abi polish I think.
>>
>>>> +int _drmModeAddFB2(int fd, uint32_t width, uint32_t height,
>>>> +		   uint32_t pixel_format, uint32_t bo_handles[4],
>>>> +		   uint32_t pitches[4], uint32_t offsets[4],
>>>> +		   uint64_t modifier[0], uint32_t *buf_id, uint32_t flags)
>
> No camelcase for library functions. Also I think we want a kms_ prefix
> here for a bit of namespacing. And since your test only creates
> framebuffers with 1 plane I think simplifying the function interface would
> be good to. Finally we use a __ prefix for raw functions (i.e. those
> returning errno) in the ioctl wrapper library. And I'd put the bo handle
> right after the fd to be more consistent with other functions. And usually
> we put out-parameters last. So
>
> int __kms_addfb(int fd, unint32_t handle,
> 		width, height, stride, pixel_format, modifier, flags,
> 		uint32_t *buf_id);

I wanted to stay close to the drmModeAddFB2 as in libdrm, expecting new 
flavour for fb modifiers there will stay close to that API. Benefit 
would be trivial edits to move over to the official API and ability to 
drop the internal copy&paste when that happens.

And looked more convenient for users like igt_create_fb_with_bo_size who 
are using drmModeAddFB2 today, and are expected to grow features in the 
future.

Seems trivial to me, but if you feel strongly about this sure I'll 
change it.

>>>> +{
>>>> +	struct local_drm_mode_fb_cmd2 f;
>>>> +	int ret;
>>>> +
>>>> +	f.width  = width;
>>>> +	f.height = height;
>>>> +	f.pixel_format = pixel_format;
>>>> +	f.flags = flags;
>>>> +
>>>> +	memcpy(f.handles, bo_handles, 4 * sizeof(bo_handles[0]));
>>>> +	memcpy(f.pitches, pitches, 4 * sizeof(pitches[0]));
>>>> +	memcpy(f.offsets, offsets, 4 * sizeof(offsets[0]));
>>>> +	memcpy(f.modifier, modifier, 4 * sizeof(modifier[0]));
>>>> +
>>>> +	if ((ret = drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f)))
>>>> +		return ret < 0 ? -errno : ret;
>>>> +
>>>> +	*buf_id = f.fb_id;
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +unsigned int has_drm_fb_modifiers(int fd)
>
> Common patterin is to pusht he require into this function and use a void
> return value. Also drm prefix is used for core drm and libdrm stuff,
> better to pick kms_. So

Yes if I wanted it to be called igt_require_fb_modifiers, but it wasn't 
exactly the case. Idea was slightly different usage in future work, 
outside the kms_addfb test.

Regards,

Tvrtko