[Mesa-dev] [PATCH v3] vulkan: Fix compilation on older platforms

[Danylo Piliaiev]

Hi Emil,

On 24.07.18 17:10, Emil Velikov wrote:
> Hi Danylo,
>
> On 13 July 2018 at 14:57, Danylo Piliaiev <danylo.piliaiev at gmail.com> wrote:
>> Make xlease automatically enabled only if xcb-randr >= 1.13,
>> check its version if manually enabled.
>>
>> Enable VK_EXT_display_control only when libdrm >= 2.4.89
>>
>> Check for DRM_EVENT_CONTEXT_VERSION >= 4 to use sequence_handler.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107170
>>            https://bugs.freedesktop.org/show_bug.cgi?id=106972
>>            https://bugs.freedesktop.org/show_bug.cgi?id=107176
>>
>> v2: - Add 'protect="VK_USE_DISPLAY_CONTROL"' attribute to
>>         VK_EXT_display_control in vk.xml
>>      - Add support for 'protect' attribute to anv_entrypoints_gen
>>         (copied from radv_entrypoints_gen)
>>      - Turn #if into #ifdef
>>      - Remove unnecessary pkg-config call from meson build (Dylan Baker)
>>
>> v3: by Dylan Baker
>>      - Remove previously added changes to vk.xml and entrypoints
>>         generation because vk.xml is meant to be pulled from the external
>>         source.
>>
>> Signed-off-by: Danylo Piliaiev <danylo.piliaiev at globallogic.com>
>> ---
>>   configure.ac                           | 29 +++++++++++++-------------
>>   meson.build                            | 10 ++++++++-
>>   src/amd/vulkan/radv_extensions.py      |  9 +++++++-
>>   src/amd/vulkan/radv_wsi_display.c      |  5 ++---
>>   src/intel/vulkan/anv_extensions.py     |  2 +-
>>   src/intel/vulkan/anv_extensions_gen.py |  7 +++++++
>>   src/intel/vulkan/anv_wsi_display.c     |  4 ++--
>>   src/vulkan/wsi/wsi_common_display.c    |  8 +++++--
>>   src/vulkan/wsi/wsi_common_display.h    |  3 ++-
>>   9 files changed, 51 insertions(+), 26 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index f135d05736..0b04525014 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -82,6 +82,8 @@ LIBDRM_FREEDRENO_REQUIRED=2.4.92
>>   LIBDRM_ETNAVIV_REQUIRED=2.4.89
>>   LIBDRM_VC4_REQUIRED=2.4.89
>>
>> +LIBDRM_CRT_SEQUENCE_REQUIRED=2.4.89
>> +
> There is no need for yet another version - just bump LIBDRM_REQUIRED.
Bumping LIBDRM_REQUIRED would mean that older platforms won't be supported.
I don't know how the decisions about support of older platforms are made 
but
from my point of view bumping required libdrm version due to one vulkan 
extension
may be overkill.
>>   dnl Versions for external dependencies
>>   DRI2PROTO_REQUIRED=2.8
>>   GLPROTO_REQUIRED=1.4.14
>> @@ -97,6 +99,7 @@ XCBDRI2_REQUIRED=1.8
>>   XCBDRI3_MODIFIERS_REQUIRED=1.13
>>   XCBGLX_REQUIRED=1.8.1
>>   XCBPRESENT_MODIFIERS_REQUIRED=1.13
>> +XCBRANDR_XLEASE_REQUIRED=1.13
> Thinking out loud: I wonder if we cannot use a single variable for all
> the xcb 1.13 bits.
>
>>   XDAMAGE_REQUIRED=1.1
>>   XSHMFENCE_REQUIRED=1.1
>>   XVMC_REQUIRED=1.0.6
>> @@ -1874,20 +1877,6 @@ if test x"$enable_dri3" = xyes; then
>>       fi
>>   fi
>>
>> -
>> -if echo "$platforms" | grep -q 'x11' && echo "$platforms" | grep -q 'drm'; then
>> -    have_xlease=yes
>> -else
>> -    have_xlease=no
>> -fi
>> -
>> -if test x"$have_xlease" = xyes; then
>> -    randr_modules="x11-xcb xcb-randr"
>> -    PKG_CHECK_MODULES([XCB_RANDR], [$randr_modules])
>> -    xlib_randr_modules="xrandr"
>> -    PKG_CHECK_MODULES([XLIB_RANDR], [$xlib_randr_modules])
>> -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')
>> @@ -1905,14 +1894,24 @@ xno)
>>       ;;
>>   *)
>>       if echo "$platforms" | grep -q 'x11' && echo "$platforms" | grep -q 'drm'; then
>> -        enable_xlib_lease=yes
>> +        xlease_modules="x11-xcb xcb-randr >= $XCBRANDR_XLEASE_REQUIRED xrandr"
>> +        PKG_CHECK_EXISTS([$xlease_modules], [enable_xlib_lease=yes], [enable_xlib_lease=no])
>>       else
>>           enable_xlib_lease=no
>>       fi
>>   esac
>>
>> +if test x"$enable_xlib_lease" = xyes; then
>> +    randr_modules="x11-xcb xcb-randr >= $XCBRANDR_XLEASE_REQUIRED"
>> +    PKG_CHECK_MODULES([XCB_RANDR], [$randr_modules])
>> +    xlib_randr_modules="xrandr"
>> +    PKG_CHECK_MODULES([XLIB_RANDR], [$xlib_randr_modules])
>> +fi
>> +
>>   AM_CONDITIONAL(HAVE_XLIB_LEASE, test "x$enable_xlib_lease" = xyes)
>>
>> +PKG_CHECK_EXISTS([libdrm >= $LIBDRM_CRT_SEQUENCE_REQUIRED], [DEFINES="${DEFINES} -DVK_USE_DISPLAY_CONTROL"], [])
>> +
> Why do we need the new define? From a quick look we should be above to
> reuse VK_USE_PLATFORM_DISPLAY_KHR, we simply need the ifdef guards.
If we will not bump global libdrm version requirement new define is 
necessary because
rest of the code guarded by VK_USE_PLATFORM_DISPLAY_KHR works fine with 
older libdrm versions.
> The comments are more or less also applicable for meson.
>
> HTH
> Emil