From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Keith Packard" 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 Message-ID: <871shpc0r8.fsf@keithp.com> References: <20180210044516.16944-1-keithp@keithp.com> <20180210044516.16944-2-keithp@keithp.com> <20180212152704.cchww2vlowyjnd4i@imgtec.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0808103422==" Return-path: In-Reply-To: <20180212152704.cchww2vlowyjnd4i@imgtec.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Eric Engestrom Cc: mesa-dev@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0808103422== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Eric Engestrom 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 +=3D [ >> + '-DVK_USE_PLATFORM_DISPLAY_KHR', >> + ] > > Nit: this can be a simple > radv_flags +=3D '-DVK_USE_PLATFORM_DISPLAY_KHR' > >> +if with_platform_display >> + vulkan_wsi_args +=3D [ >> + '-DVK_USE_PLATFORM_DISPLAY_KHR', >> + ] > > Ditto: > vulkan_wsi_args +=3D '-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. =2D-=20 =2Dkeith --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEw4O3eCVWE9/bQJ2R2yIaaQAAABEFAlqCEisACgkQ2yIaaQAA ABFmBA/+JsEGkYpPRIiFsWBw/HHYAdOLKgp9SVAd2zzc2IkZxSZOYllnH6UpD/gC W3ZxdUVeBTY1hDU/R1tS9LgiyUdDCUSe9rCGJOgMcfMTey7p/0AGikf6oY9/tv8L c9ZVujl1FDcz7jZa3rGddiP1YVAA8JCgKxFqGAHC1TsHn4QatfYzZrNokdnLyuNI clzd5CUIwwp4KCIkFxzq8Pwd1vK8p1NiEEBduXSJc6nnozePQAdZCADA1f6nmdnk ixAPjdxVdnlevu9XgLVVoy03+Lkt25HeTGXhA/mK6mipb9Aiwmq9xqwAI3wWDXtV rBGlT+x8Q97Lq1At4JWdXSyQ6ZWLv5P6NcdnZKPKozs2z+ZzvfhzlU9Fq3FJg5TU lCfHLT6muew4bA83mkDybe0ZkRRyJVYsPkkNMmsqchDL9TLVW3nBsR7/fkj9+fOf HjGKDE6YmLWICIVk1bUGPCDID6xxgRX28oZcMXZDcid3C1xJgaZVNBKCFr61+3V0 voTv0Gcz6mXaeD/gIEp3b6Nv9ZU5HO3dSsjVeLhE/x7ON0MDIeTdZaSzdxZsOqWC 5A6fAwKAneq/z5oUxSNJMHmRTErc5WD0P4lg4IgU3KLrHfldjTnMSHgV+PAHylMU +az6eGwc641g3jp16TMZOZDqTTVg6Z2qTetjOa15SR8Yu/pzgic= =+FqD -----END PGP SIGNATURE----- --=-=-=-- --===============0808103422== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0808103422==--