[Mesa-dev] [PATCH 1/7] vulkan: Add KHR_display extension to anv and radv using DRM

[Keith Packard]

Eric Engestrom <eric.engestrom at imgtec.com> writes:

> copy/paste error: s/drm/display/

That's actually intentional -- any system which supports 'drm' can
support the display backend as it's based on that. Maybe it should use a
different test? (note that I haven't been using the autotools backend
recently, so it may not be great at this point. At least my current tree
builds?)

>> +if with_platform_display
>> +  radv_flags += [
>> +    '-DVK_USE_PLATFORM_DISPLAY_KHR',
>> +  ]
>
> Nit: this can be a simple
>   radv_flags += '-DVK_USE_PLATFORM_DISPLAY_KHR'
>
>> +if with_platform_display
>> +  vulkan_wsi_args += [
>> +    '-DVK_USE_PLATFORM_DISPLAY_KHR',
>> +  ]
>
> Ditto:
>   vulkan_wsi_args += '-DVK_USE_PLATFORM_DISPLAY_KHR'


Oh, good point -- I'd split out the RANDR and DISPLAY bits but didn't
simplify the meson stuff.

> Nit: src/amd/vulkan/ uses tabs for indent, you used spaces.

Thanks; I'll reconfigure my environment so that it uses tabs in that
directory, and re-indent the changes.

>> +#define MM_PER_PIXEL     (1.0/96.0 * 25.4)
>
> unused

Good catch; those got left in both anv and radv directories after some
refactoring.

>> +#if 0
>
> `#if DEBUG` maybe?

Could do; I could also just strip out the printf debugging, but when
you're doing timing-sensitive stuff at that level, printf debugging is
pretty useful.

>> +#define wsi_display_debug(...) fprintf(stderr, __VA_ARGS__)
>> +#define wsi_display_debug_code(...)     __VA_ARGS__
>
> that 2nd one is unused

It gets used in a later patch.

>> +   if (wsi) {
>
> if (!wsi) return;
> and carry on without the extra indent

Yeah, would probably look cleaner.

> I don't know enough for this to be an actual review though, but the rest
> of this file looks reasonable to me :)

Thanks for reviewing the basic formatting and structure though; I
totally missed the tabs/spaces issue.

-- 
-keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180212/5a08cd9d/attachment.sig>