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

From: "Keith Packard" <keithp@keithp.com>
To: Eric Engestrom <eric.engestrom@imgtec.com>
Cc: mesa-dev@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/7] vulkan: Add KHR_display extension to anv and radv using DRM
Date: Mon, 12 Feb 2018 14:16:11 -0800	[thread overview]
Message-ID: <871shpc0r8.fsf@keithp.com> (raw)
In-Reply-To: <20180212152704.cchww2vlowyjnd4i@imgtec.com>

[-- Attachment #1.1: Type: text/plain, Size: 1940 bytes --]

Eric Engestrom <eric.engestrom@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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel