Re: [Mesa-dev] [PATCH 01/21] vulkan: Add KHR_display extension using DRM

[Keith Packard]

Daniel Stone  writes:

> Or better, just use drmModeAddFB2 and pass the format directly. That
> landed back in 3.2 or so, and I don't think there's anyone trying to
> use Vulkan on a kernel from 2011.

Yeah, that's probably a better plan. I've pushed a patch that does this
on top of the long list of patches made in response to Jason's email.

Once he's replied to that, I'll go ahead and smash the new patches back
on top of the original series and re-publish that.

-- 
-keith


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/21] vulkan: Add KHR_display extension using DRM

[Keith Packard]

Jason Ekstrand  writes:

Thanks a million for the intense review. I've pushed 15 tiny patches
that address individual questions in this message. If you want to look
at those separately, that would be great. When we're done, I'll merge
them back into the giant patch.

Sorry for the delay in answering; you asked a hard question about GEM
handles and reference counting which I think I've resolved by
eliminating the ability to share the same fd for modesetting and
rendering.

Let me know if I've deleted too much context from your answers.

> With the exception of NIR (which is a bit odd), we usually use ALL_CAPS for
> enum values.

Thanks, fixed.

> Yes, this does seem like the only reasonable mode for now.  I suppose you
> could check to see if the platform supports ASYNC_FLIP and advertise
> IMMEDIATE in that case.  I know intel doesn't advertise it but I thought
> amdgpu does.

Ok, we'll put that on the wish list, along with MAILBOX (which
applications actually want :-)

> vk_outarray, though I suppose you've probably already made that
> change.

Yup, vk_outarray everywhere now.

> I don't see use_prime_blit being set anywhere.  Perhaps that comes in a
> later patch?  The line is obviously correct, so I'm fine with leaving
> it.

You're right -- this was a cult-n-paste error. I don't support prime at
all, so I've removed this bit.

> This will have to be updated for modifiers.  I'm reasonably happy for it to
> be the no-op update for now though KHR_display supporting real modifiers
> would be nice. :-)

Yeah, "patches welcome", of course. I don't have a requirement for them
on Radeon at this point.

>> +   if (result != VK_SUCCESS)
>> +  return result;
>> +
>> +   ret = drmPrimeFDToHandle(wsi->master_fd, image->base.fd,
>> _handle);
>>
>
> This opens up some interesting questions.  GEM handles are not reference
> counted by the kernel.  If the driver that we're running on is using
> master_fd, then we shouldn't close our handle because that would close the
> handle for the driver as well.  If it's not using master_fd, we should
> close the handle to avoid leaking it.  The later is only a worry in the
> prime case but given that I saw a line above about use_prime_blit, you have
> me scared. :-)

Ok, as I mentioned in the header of this message, I've eliminated any
code which talks about potentially sharing master_fd and render_fd. I
removed render_fd from the wsi layer entirely as it is no longer used
anywhere.

In the drivers, when you request KHR_display, I attempt to open the
related primary node and if that succeeds, I pass that to the wsi layer
for use there. That fd is otherwise unused by the driver, and in fact
the driver doesn't need to close it as the wsi layer will.

Obviously having two FDs is "wasteful" at some level as we really don't
need that, but that's what the acquire_xlib extension will have to do
anyways.

> One solution to this connundrum would be to simply never use the master FD
> for the Vulkan driver itself and always open a render node.  If handed a
> master FD, you could check if it matches your render node and set
> use_prime_blit accordingly.  Then the driver and WSI would have separate
> handle domains and this problem would be solved.  You could also just open
> the master FD twice, once for WSI and once for the driver.

Yup, two FDs, one master, one render. That way, the kludge
seen in this patch where it opens the master when you request the
KHR_display extension works the same as the acquire_xlib code in the
later patch.

> You have the format in create_info.  We could add some generic VkFormat
> introspection or we can just handle the 2-4 cases we care about
> directly.

Given that this layer provides the list of valid surface formats which
that image could contain, I've added depth/bpp information to that list
and the image_init code walks over the list of possible formats to find
the associated depth/bpp values.

If the application provides an invalid format, I'm returning
VK_ERROR_DEVICE_LOST as that is a valid error return and I couldn't find
anything else that seemed like a better value. If we get here, the
application made a mistake anyways.

> What happens if we close the handle immediately after calling
> drmModeAddFB?  Does the FB become invalid?  If so, then we probably want to
> stash the handle so we can clean it up when we destroy the swapchain.
> Unless, of course, we're willing to make the assumption (as stated above)
> that the driver and WSI are always using the same FD.

It looks like the FB ends up with a reference to the object, so it would
should be safe to just close the handle at this point.  However, to make
it less dependent on the kernel behavior, I've changed the code to store
the buffer_object handle in the image and then added code to free both
the buffer object and the frame buffer in wsi_display_image_finish.

Thanks for finding the leak.

>> +/* call with wait_mutex held */
>>
>
> It might be worth a line in the 

Re: [Mesa-dev] [PATCH 01/21] vulkan: Add KHR_display extension using DRM

[Daniel Stone]

Hi,

On 24 February 2018 at 00:43, Jason Ekstrand  wrote:
> On Tue, Feb 13, 2018 at 4:31 PM, Keith Packard  wrote:
>> +   image->chain = chain;
>> +   image->state = wsi_image_idle;
>> +   image->fb_id = 0;
>> +
>> +   /* XXX extract depth and bpp from image somehow */
>
> You have the format in create_info.  We could add some generic VkFormat
> introspection or we can just handle the 2-4 cases we care about directly.

Or better, just use drmModeAddFB2 and pass the format directly. That
landed back in 3.2 or so, and I don't think there's anyone trying to
use Vulkan on a kernel from 2011.

Cheers,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/21] vulkan: Add KHR_display extension using DRM

[Jason Ekstrand]

Continuing on the new version of the patch...

On Tue, Feb 13, 2018 at 4:31 PM, Keith Packard  wrote:

> +enum wsi_image_state {
> +   wsi_image_idle,
> +   wsi_image_drawing,
> +   wsi_image_queued,
> +   wsi_image_flipping,
> +   wsi_image_displaying
> +};
>

With the exception of NIR (which is a bit odd), we usually use ALL_CAPS for
enum values.


> +
> +static const VkPresentModeKHR available_present_modes[] = {
> +   VK_PRESENT_MODE_FIFO_KHR,
>

Yes, this does seem like the only reasonable mode for now.  I suppose you
could check to see if the platform supports ASYNC_FLIP and advertise
IMMEDIATE in that case.  I know intel doesn't advertise it but I thought
amdgpu does.


> +};
> +
> +static VkResult
> +wsi_display_surface_get_present_modes(VkIcdSurfaceBase  *surface,
> +  uint32_t
> *present_mode_count,
> +  VkPresentModeKHR  *present_modes)
> +{
> +   if (present_modes == NULL) {
> +  *present_mode_count = ARRAY_SIZE(available_present_modes);
> +  return VK_SUCCESS;
> +   }
> +
> +   *present_mode_count = MIN2(*present_mode_count,
> ARRAY_SIZE(available_present_modes));
> +   typed_memcpy(present_modes, available_present_modes,
> *present_mode_count);
> +
> +   if (*present_mode_count < ARRAY_SIZE(available_present_modes))
> +  return VK_INCOMPLETE;
> +   return VK_SUCCESS;
>

vk_outarray, though I suppose you've probably already made that change.


> +}
> +
> +static VkResult
> +wsi_display_image_init(VkDevice device_h,
> +   struct wsi_swapchain *drv_chain,
> +   const VkSwapchainCreateInfoKHR   *create_info,
> +   const VkAllocationCallbacks  *allocator,
> +   struct wsi_display_image *image)
> +{
> +   struct wsi_display_swapchain *chain = (struct wsi_display_swapchain *)
> drv_chain;
> +   struct wsi_display   *wsi = chain->wsi;
> +   VkResult result;
> +   int  ret;
> +   uint32_t image_handle;
> +
> +   if (chain->base.use_prime_blit)
> +  result = wsi_create_prime_image(>base, create_info,
> >base);
>

I don't see use_prime_blit being set anywhere.  Perhaps that comes in a
later patch?  The line is obviously correct, so I'm fine with leaving it.


> +   else
> +  result = wsi_create_native_image(>base, create_info,
> >base);
>

This will have to be updated for modifiers.  I'm reasonably happy for it to
be the no-op update for now though KHR_display supporting real modifiers
would be nice. :-)


> +   if (result != VK_SUCCESS)
> +  return result;
> +
> +   ret = drmPrimeFDToHandle(wsi->master_fd, image->base.fd,
> _handle);
>

This opens up some interesting questions.  GEM handles are not reference
counted by the kernel.  If the driver that we're running on is using
master_fd, then we shouldn't close our handle because that would close the
handle for the driver as well.  If it's not using master_fd, we should
close the handle to avoid leaking it.  The later is only a worry in the
prime case but given that I saw a line above about use_prime_blit, you have
me scared. :-)

One solution to this connundrum would be to simply never use the master FD
for the Vulkan driver itself and always open a render node.  If handed a
master FD, you could check if it matches your render node and set
use_prime_blit accordingly.  Then the driver and WSI would have separate
handle domains and this problem would be solved.  You could also just open
the master FD twice, once for WSI and once for the driver.


> +
> +   close(image->base.fd);
> +   image->base.fd = -1;
> +
> +   if (ret < 0)
> +  goto fail_handle;
> +
> +   image->chain = chain;
> +   image->state = wsi_image_idle;
> +   image->fb_id = 0;
> +
> +   /* XXX extract depth and bpp from image somehow */
>

You have the format in create_info.  We could add some generic VkFormat
introspection or we can just handle the 2-4 cases we care about directly.


> +   ret = drmModeAddFB(wsi->master_fd, create_info->imageExtent.width,
> create_info->imageExtent.height,
> +  24, 32, image->base.row_pitch, image_handle,
> >fb_id);
>

What happens if we close the handle immediately after calling
drmModeAddFB?  Does the FB become invalid?  If so, then we probably want to
stash the handle so we can clean it up when we destroy the swapchain.
Unless, of course, we're willing to make the assumption (as stated above)
that the driver and WSI are always using the same FD.


> +
> +   if (ret)
> +  goto fail_fb;
> +
> +   return VK_SUCCESS;
> +
> +fail_fb:
> +   /* fall through */
> +
> +fail_handle:
> +   wsi_destroy_image(>base, >base);
> +
> +   return VK_ERROR_OUT_OF_HOST_MEMORY;
> +}
> +
> +static void
> +wsi_display_image_finish(struct wsi_swapchain   *drv_chain,
> + const VkAllocationCallbacks

[PATCH 01/21] vulkan: Add KHR_display extension using DRM

[Keith Packard]

This adds support for the KHR_display extension support to the vulkan
WSI layer. Driver support will be added separately.

Signed-off-by: Keith Packard 
---
 configure.ac|1 +
 meson.build |4 +-
 src/amd/vulkan/radv_wsi.c   |3 +-
 src/intel/vulkan/anv_wsi.c  |3 +-
 src/vulkan/Makefile.am  |7 +
 src/vulkan/Makefile.sources |4 +
 src/vulkan/wsi/meson.build  |   10 +
 src/vulkan/wsi/wsi_common.c |   19 +-
 src/vulkan/wsi/wsi_common.h |5 +-
 src/vulkan/wsi/wsi_common_display.c | 1368 +++
 src/vulkan/wsi/wsi_common_display.h |   72 ++
 src/vulkan/wsi/wsi_common_private.h |   10 +
 12 files changed, 1500 insertions(+), 6 deletions(-)
 create mode 100644 src/vulkan/wsi/wsi_common_display.c
 create mode 100644 src/vulkan/wsi/wsi_common_display.h

diff --git a/configure.ac b/configure.ac
index 8ed606c7694..46318365603 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1849,6 +1849,7 @@ fi
 AM_CONDITIONAL(HAVE_PLATFORM_X11, echo "$platforms" | grep -q 'x11')
 AM_CONDITIONAL(HAVE_PLATFORM_WAYLAND, echo "$platforms" | grep -q 'wayland')
 AM_CONDITIONAL(HAVE_PLATFORM_DRM, echo "$platforms" | grep -q 'drm')
+AM_CONDITIONAL(HAVE_PLATFORM_DISPLAY, echo "$platforms" | grep -q 'drm')
 AM_CONDITIONAL(HAVE_PLATFORM_SURFACELESS, echo "$platforms" | grep -q 
'surfaceless')
 AM_CONDITIONAL(HAVE_PLATFORM_ANDROID, echo "$platforms" | grep -q 'android')
 
diff --git a/meson.build b/meson.build
index b39e2f8ab96..aeb7f5e2917 100644
--- a/meson.build
+++ b/meson.build
@@ -239,11 +239,12 @@ with_platform_wayland = false
 with_platform_x11 = false
 with_platform_drm = false
 with_platform_surfaceless = false
+with_platform_display = false
 egl_native_platform = ''
 _platforms = get_option('platforms')
 if _platforms == 'auto'
   if system_has_kms_drm
-_platforms = 'x11,wayland,drm,surfaceless'
+_platforms = 'x11,wayland,drm,surfaceless,display'
   elif ['darwin', 'windows', 'cygwin'].contains(host_machine.system())
 _platforms = 'x11,surfaceless'
   else
@@ -257,6 +258,7 @@ if _platforms != ''
   with_platform_wayland = _split.contains('wayland')
   with_platform_drm = _split.contains('drm')
   with_platform_surfaceless = _split.contains('surfaceless')
+  with_platform_display = _split.contains('display')
   egl_native_platform = _split[0]
 endif
 
diff --git a/src/amd/vulkan/radv_wsi.c b/src/amd/vulkan/radv_wsi.c
index e016e837102..9bdd55ef11c 100644
--- a/src/amd/vulkan/radv_wsi.c
+++ b/src/amd/vulkan/radv_wsi.c
@@ -41,7 +41,8 @@ radv_init_wsi(struct radv_physical_device *physical_device)
return wsi_device_init(_device->wsi_device,
   radv_physical_device_to_handle(physical_device),
   radv_wsi_proc_addr,
-  _device->instance->alloc);
+  _device->instance->alloc,
+  physical_device->local_fd);
 }
 
 void
diff --git a/src/intel/vulkan/anv_wsi.c b/src/intel/vulkan/anv_wsi.c
index 6082c3dd093..f86d83589ea 100644
--- a/src/intel/vulkan/anv_wsi.c
+++ b/src/intel/vulkan/anv_wsi.c
@@ -39,7 +39,8 @@ anv_init_wsi(struct anv_physical_device *physical_device)
return wsi_device_init(_device->wsi_device,
   anv_physical_device_to_handle(physical_device),
   anv_wsi_proc_addr,
-  _device->instance->alloc);
+  _device->instance->alloc,
+  physical_device->local_fd);
 }
 
 void
diff --git a/src/vulkan/Makefile.am b/src/vulkan/Makefile.am
index 037436c1cd7..c33ac5758f7 100644
--- a/src/vulkan/Makefile.am
+++ b/src/vulkan/Makefile.am
@@ -57,6 +57,13 @@ AM_CPPFLAGS += \
 VULKAN_WSI_SOURCES += $(VULKAN_WSI_X11_FILES)
 endif
 
+if HAVE_PLATFORM_DISPLAY
+AM_CPPFLAGS += \
+   -DVK_USE_PLATFORM_DISPLAY_KHR
+
+VULKAN_WSI_SOURCES += $(VULKAN_WSI_DISPLAY_FILES)
+endif
+
 BUILT_SOURCES += $(VULKAN_WSI_WAYLAND_GENERATED_FILES)
 CLEANFILES = $(BUILT_SOURCES)
 
diff --git a/src/vulkan/Makefile.sources b/src/vulkan/Makefile.sources
index a0a24ce7de8..3642c7662c4 100644
--- a/src/vulkan/Makefile.sources
+++ b/src/vulkan/Makefile.sources
@@ -17,6 +17,10 @@ VULKAN_WSI_X11_FILES := \
wsi/wsi_common_x11.c \
wsi/wsi_common_x11.h
 
+VULKAN_WSI_DISPLAY_FILES := \
+   wsi/wsi_common_display.c \
+   wsi/wsi_common_display.h
+
 VULKAN_UTIL_FILES := \
util/vk_alloc.h \
util/vk_debug_report.c \
diff --git a/src/vulkan/wsi/meson.build b/src/vulkan/wsi/meson.build
index bd0fd3cc53e..743631a6113 100644
--- a/src/vulkan/wsi/meson.build
+++ b/src/vulkan/wsi/meson.build
@@ -57,6 +57,16 @@ if with_platform_wayland
   ]
 endif
 
+if with_platform_display
+  vulkan_wsi_args += [
+'-DVK_USE_PLATFORM_DISPLAY_KHR',
+  ]
+  files_vulkan_wsi += files(
+'wsi_common_display.c',
+