Re: [PATCH 2/2] selftests: ion: Add simple test with the vgem driver

[Shuah Khan]

On 02/26/2018 06:48 PM, Laura Abbott wrote:
> On 02/26/2018 09:07 AM, Shuah Khan wrote:
>> On 02/19/2018 11:33 AM, Daniel Vetter wrote:
>>> On Mon, Feb 19, 2018 at 10:18:21AM -0800, Laura Abbott wrote:
>>>> On 02/19/2018 07:31 AM, Daniel Vetter wrote:
>>>>> On Thu, Feb 15, 2018 at 05:24:45PM -0800, Laura Abbott wrote:
>>>>>> Ion is designed to be a framework used by other clients who perform
>>>>>> operations on the buffer. Use the DRM vgem client as a simple consumer.
>>>>>> In conjunction with the dma-buf sync ioctls, this tests the full attach/map
>>>>>> path for the system heap.
>>>>>>
>>>>>> Signed-off-by: Laura Abbott <labbott@xxxxxxxxxx>
>>>>>> ---
>>>>>> ÂÂ tools/testing/selftests/android/ion/MakefileÂÂÂÂÂ |ÂÂ 3 +-
>>>>>> ÂÂ tools/testing/selftests/android/ion/configÂÂÂÂÂÂÂ |ÂÂ 1 +
>>>>>> ÂÂ tools/testing/selftests/android/ion/ionmap_test.c | 136 ++++++++++++++++++++++
>>>>>> ÂÂ 3 files changed, 139 insertions(+), 1 deletion(-)
>>>>>> ÂÂ create mode 100644 tools/testing/selftests/android/ion/ionmap_test.c
>>>>>>
>>>>>> diff --git a/tools/testing/selftests/android/ion/Makefile b/tools/testing/selftests/android/ion/Makefile
>>>>>> index 96e0c448b39d..d23b6d537d8b 100644
>>>>>> --- a/tools/testing/selftests/android/ion/Makefile
>>>>>> +++ b/tools/testing/selftests/android/ion/Makefile
>>>>>> @@ -2,7 +2,7 @@
>>>>>> ÂÂ INCLUDEDIR := -I. -I../../../../../drivers/staging/android/uapi/
>>>>>> ÂÂ CFLAGS := $(CFLAGS) $(INCLUDEDIR) -Wall -O2 -g
>>>>>> -TEST_GEN_FILES := ionapp_export ionapp_import
>>>>>> +TEST_GEN_FILES := ionapp_export ionapp_import ionmap_test
>>>>>> ÂÂ all: $(TEST_GEN_FILES)
>>>>>> @@ -14,3 +14,4 @@ include ../../lib.mk
>>>>>> ÂÂ $(OUTPUT)/ionapp_export: ionapp_export.c ipcsocket.c ionutils.c
>>>>>> ÂÂ $(OUTPUT)/ionapp_import: ionapp_import.c ipcsocket.c ionutils.c
>>>>>> +$(OUTPUT)/ionmap_test: ionmap_test.c ionutils.c
>>>>>> diff --git a/tools/testing/selftests/android/ion/config b/tools/testing/selftests/android/ion/config
>>>>>> index 19db6ca9aa2b..b4ad748a9dd9 100644
>>>>>> --- a/tools/testing/selftests/android/ion/config
>>>>>> +++ b/tools/testing/selftests/android/ion/config
>>>>>> @@ -2,3 +2,4 @@ CONFIG_ANDROID=y
>>>>>> ÂÂ CONFIG_STAGING=y
>>>>>> ÂÂ CONFIG_ION=y
>>>>>> ÂÂ CONFIG_ION_SYSTEM_HEAP=y
>>>>>> +CONFIG_DRM_VGEM=y
>>>>>> diff --git a/tools/testing/selftests/android/ion/ionmap_test.c b/tools/testing/selftests/android/ion/ionmap_test.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..dab36b06b37d
>>>>>> --- /dev/null
>>>>>> +++ b/tools/testing/selftests/android/ion/ionmap_test.c
>>>>>> @@ -0,0 +1,136 @@
>>>>>> +#include <errno.h>
>>>>>> +#include <fcntl.h>
>>>>>> +#include <stdio.h>
>>>>>> +#include <stdint.h>
>>>>>> +#include <string.h>
>>>>>> +#include <unistd.h>
>>>>>> +
>>>>>> +#include <sys/ioctl.h>
>>>>>> +#include <sys/types.h>
>>>>>> +#include <sys/stat.h>
>>>>>> +
>>>>>> +#include <linux/dma-buf.h>
>>>>>> +
>>>>>> +#include <drm/drm.h>
>>>>>> +
>>>>>> +#include "ion.h"
>>>>>> +#include "ionutils.h"
>>>>>> +
>>>>>> +int check_vgem(int fd)
>>>>>> +{
>>>>>> +ÂÂÂ drm_version_t version = { 0 };
>>>>>> +ÂÂÂ char name[5];
>>>>>> +ÂÂÂ int ret;
>>>>>> +
>>>>>> +ÂÂÂ version.name_len = 4;
>>>>>> +ÂÂÂ version.name = name;
>>>>>> +
>>>>>> +ÂÂÂ ret = ioctl(fd, DRM_IOCTL_VERSION, &version);
>>>>>> +ÂÂÂ if (ret)
>>>>>> +ÂÂÂÂÂÂÂ return 1;
>>>>>> +
>>>>>> +ÂÂÂ return strcmp(name, "vgem");
>>>>>> +}
>>>>>> +
>>>>>> +int open_vgem(void)
>>>>>> +{
>>>>>> +ÂÂÂ int i, fd;
>>>>>> +ÂÂÂ const char *drmstr = "/dev/dri/card";
>>>>>> +
>>>>>> +ÂÂÂ fd = -1;
>>>>>> +ÂÂÂ for (i = 0; i < 16; i++) {
>>>>>> +ÂÂÂÂÂÂÂ char name[80];
>>>>>> +
>>>>>> +ÂÂÂÂÂÂÂ sprintf(name, "%s%u", drmstr, i);
>>>>>> +
>>>>>> +ÂÂÂÂÂÂÂ fd = open(name, O_RDWR);
>>>>>> +ÂÂÂÂÂÂÂ if (fd < 0)
>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂ continue;
>>>>>> +
>>>>>> +ÂÂÂÂÂÂÂ if (check_vgem(fd)) {
>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂ close(fd);
>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂ continue;
>>>>>> +ÂÂÂÂÂÂÂ } else {
>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>>>>>> +ÂÂÂÂÂÂÂ }
>>>>>> +
>>>>>> +ÂÂÂ }
>>>>>> +ÂÂÂ return fd;
>>>>>> +}
>>>>>> +
>>>>>> +int import_vgem_fd(int vgem_fd, int dma_buf_fd, uint32_t *handle)
>>>>>> +{
>>>>>> +ÂÂÂ struct drm_prime_handle import_handle = { 0 };
>>>>>> +ÂÂÂ int ret;
>>>>>> +
>>>>>> +ÂÂÂ import_handle.fd = dma_buf_fd;
>>>>>> +ÂÂÂ import_handle.flags = 0;
>>>>>> +ÂÂÂ import_handle.handle = 0;
>>>>>> +
>>>>>> +ÂÂÂ ret = ioctl(vgem_fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, &import_handle);
>>>>>> +ÂÂÂ if (ret == 0)
>>>>>> +ÂÂÂÂÂÂÂ *handle = import_handle.handle;
>>>>>> +ÂÂÂ return ret;
>>>>>> +}
>>>>>> +
>>>>>> +void close_handle(int vgem_fd, uint32_t handle)
>>>>>> +{
>>>>>> +ÂÂÂ struct drm_gem_close close = { 0 };
>>>>>> +
>>>>>> +ÂÂÂ close.handle = handle;
>>>>>> +ÂÂÂ ioctl(vgem_fd, DRM_IOCTL_GEM_CLOSE, &close);
>>>>>> +}
>>>>>> +
>>>>>> +int main()
>>>>>> +{
>>>>>> +ÂÂÂ int ret, vgem_fd;
>>>>>> +ÂÂÂ struct ion_buffer_info info;
>>>>>> +ÂÂÂ uint32_t handle = 0;
>>>>>> +ÂÂÂ struct dma_buf_sync sync = { 0 };
>>>>>> +
>>>>>> +ÂÂÂ info.heap_type = ION_HEAP_TYPE_SYSTEM;
>>>>>> +ÂÂÂ info.heap_size = 4096;
>>>>>> +ÂÂÂ info.flag_type = ION_FLAG_CACHED;
>>>>>> +
>>>>>> +ÂÂÂ ret = ion_export_buffer_fd(&info);
>>>>>> +ÂÂÂ if (ret < 0) {
>>>>>> +ÂÂÂÂÂÂÂ printf("ion buffer alloc failed\n");
>>>>>> +ÂÂÂÂÂÂÂ return -1;
>>>>>> +ÂÂÂ }
>>>>>> +
>>>>>> +ÂÂÂ vgem_fd = open_vgem();
>>>>>> +ÂÂÂ if (vgem_fd < 0) {
>>>>>> +ÂÂÂÂÂÂÂ ret = vgem_fd;
>>>>>> +ÂÂÂÂÂÂÂ printf("Failed to open vgem\n");
>>>>>> +ÂÂÂÂÂÂÂ goto out_ion;
>>>>>> +ÂÂÂ }
>>>>>> +
>>>>>> +ÂÂÂ ret = import_vgem_fd(vgem_fd, info.buffd, &handle);
>>>>>> +
>>>>>> +ÂÂÂ if (ret < 0) {
>>>>>> +ÂÂÂÂÂÂÂ printf("Failed to import buffer\n");
>>>>>> +ÂÂÂÂÂÂÂ goto out_vgem;
>>>>>> +ÂÂÂ }
>>>>>> +
>>>>>> +ÂÂÂ sync.flags = DMA_BUF_SYNC_START | DMA_BUF_SYNC_RW;
>>>>>> +ÂÂÂ ret = ioctl(info.buffd, DMA_BUF_IOCTL_SYNC, &sync);
>>>>>> +ÂÂÂ if (ret)
>>>>>> +ÂÂÂÂÂÂÂ printf("sync start failed %d\n", errno);
>>>>>> +
>>>>>> +ÂÂÂ memset(info.buffer, 0xff, 4096);
>>>>>> +
>>>>>> +ÂÂÂ sync.flags = DMA_BUF_SYNC_END | DMA_BUF_SYNC_RW;
>>>>>> +ÂÂÂ ret = ioctl(info.buffd, DMA_BUF_IOCTL_SYNC, &sync);
>>>>>> +ÂÂÂ if (ret)
>>>>>> +ÂÂÂÂÂÂÂ printf("sync end failed %d\n", errno);
>>>>>
>>>>> At least in drm we require that userspace auto-restarts all ioctls when
>>>>> they get interrupt. See
>>>>>
>>>>> https://cgit.freedesktop.org/drm/libdrm/tree/xf86drm.c#n186
>>>>>
>>>>> not really an issue with vgem (which can't wait for hw or anything else).
>>>>> But good to make sure we don't spread bad copypastas.
>>>>>
>>>>> Actual use of the ioctls looks all good. With the drmIoctl wrapper added
>>>>> and used this is:
>>>>>
>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>>>>
>>>> Thanks for the review. The reason I didn't use the standard drmIoctl was
>>>> because I didn't want to introduce a dependency on libdrm. I don't see
>>>> an example of another selftest having a dependency on an external
>>>> library.
>>>>
>>>> Is adding a dependencies on fairly standard but still external userspace
>>>> libraries okay for kernel self tests?
>>
>> We have cases where external libraries are used. fuse test probably is one
>> example.
>>
>>>
>>> Yeah adding a dependency isn't good, I'd just copypaste a local static
>>> version into the test file. That's good enough, the point isn't to use the
>>> libdrm one, but a wrapper that automatically restarts (every other
>>> userspace for gfx has their own copy of it since it's so trivial).
>>
>> Let's go ahead and use libdrm - It is important to keep them in sync. Maybe
>> we can check dependency in the ion Makefile and not compile the test. As long
>> as the rest of the tests run even when libdrm dependency isn't met, we should
>> be good.
>>
>> thanks,
>> -- Shuah
>>
>>
> 
> The example you gave with fuse doesn't compile if you don't have the
> fuse library installed. I don't think adding a dependency on libdrm
> is worth it for a simple wrapper ioctl, especially since Daniel mention
> it gets copied around anyway.
> 
> Thanks,
> Laura

Right. It won't compile without the library. The upside is there is no
duplicate code. That said, I have no problems taking the patch for now.
If I start seeing more and more drm code, I might reconsider and opt to
add the dependency.


I will queue this up for 4.17-rc1

thanks,
-- Shuah