<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 14, 2017 at 4:08 AM, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, 2017-02-14 at 11:42 +0000, Alex Smith wrote: > Hi Iago, > > On 14 February 2017 at 11:19, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: > > On Tue, 2017-02-14 at 09:46 +0000, Alex Smith wrote: > > > On 14 February 2017 at 08:45, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> > > wrote: > > > > On Mon, 2017-02-13 at 16:29 +0000, Lionel Landwerlin wrote: > > > > > Run this by our CI earlier today and got a few failures : > > > > > > > > > > dEQP-VK.image.load_store.buffer.r8g8b8a8_snorm > > > > > dEQP-VK.image.load_store.buffer.r8g8b8a8_unorm > > > > > dEQP- > > VK.image.format_reinterpret.buffer.r32_uint_r8g8b8a8_snorm > > > > > dEQP- > > > > VK.image.format_reinterpret.buffer.r8g8b8a8_uint_r8g8b8a8_unorm > > > > > dEQP- > > > > VK.image.format_reinterpret.buffer.r8g8b8a8_sint_r8g8b8a8_snorm > > > > > dEQP- > > > > VK.image.format_reinterpret.buffer.r8g8b8a8_sint_r8g8b8a8_unorm > > > > > dEQP- > > VK.image.format_reinterpret.buffer.r32_sint_r8g8b8a8_unorm > > > > > dEQP- > > VK.image.format_reinterpret.buffer.r32_sint_r8g8b8a8_snorm > > > > > dEQP- > > VK.image.format_reinterpret.buffer.r32_uint_r8g8b8a8_unorm <div><div class="m_-5719535575661193194h5">> > > > > dEQP- > > > > > > VK.image.format_reinterpret.buffer.r8g8b8a8_snorm_r8g8b8a8_unorm > > > > > dEQP-VK.image.store.buffer.r8g8b8a8_unorm > > > > > dEQP-VK.image.store.buffer.r8g8b8a8_snorm > > > > > > > > > > I'm not quite sure why, it seems our backend discards format > > > > layout > > > > > qualifiers when we have writeonly set. > > > > > > > > For what is worth, I see a lot more regressions from > > > > image.load_store > > > > tests with this patch. Some of these get fixed with the second > > > > patch, > > > > but even then I see all of dEQP- > > > > VK.image.load_store.*.r32g32b32a32.* > > > > regressing (with Lionel's comment to patch 2 fixed) > > > These are all fixed by applying Jason's patch > > > "anv/apply_pipeline_layout: Set image.write_only to false" [1] > > before > > > this one. I've just tested locally and see no failures with both > > that > > > patch and this one applied. I'll include that patch when I resend > > the > > > series. > > > > Right, I can confirm this too. The second patch in the series, even > > with Lionel's comment fixed, does seem to add regressions even with > > Jason's patch though. > Were these failures on buffer-related tests (e.g. dEQP- > VK.image.load_store.buffer.*)? > > If so, I've fixed these in the new version of the patch I submitted a > few hours ago - buffer views also needed to be changed to handle > write-only accesses in the same way as image views. With the new > patch I see no failures on the dEQP-VK.image.* tests. </div></div>Yes, I can confirm that your v2 passes all of dEQP- VK.image.{load_}store.* on my end too. Also, I have done a quick test with some CTS tests removing the image format for the writeonly images in the dEQP-VK.image.store.* tests and they all seem to pass (this needs the patch I sent to the list today), so I think it looks good. </blockquote><div> </div><div>I've run the series through our CI system and also hacked up the dEQP-VK.image.store.* tests to not provide a format (just to make sure that works) and everything checks out. I pushed it in the following order: 1/3 (my patch), Iago's patch, 2/3, 3/3 (Alex's patches). </div><div>Thanks, Alex, for the feature! And thanks to Lionel and Iago for careful review and testing! Good work. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Iago <div class="m_-5719535575661193194HOEnZb"><div class="m_-5719535575661193194h5"> > Thanks, > Alex > > > > > Iago > > > > > Alex > > > > > > [1] <a href="https://lists.freedesktop.org/archives/mesa-dev/2017-February" rel="noreferrer" target="_blank">https://lists.freedesktop.org/archives/mesa-dev/2017-February</a> > > /144 > > > 167.html > > > > > > > > > > > Iago > > > > > > > > > - > > > > > Lionel > > > > > > > > > > On 13/02/17 16:10, Jason Ekstrand wrote: > > > > > > Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> > > > > > > > > > > > > On Thu, Feb 9, 2017 at 8:06 AM, Alex Smith <asmith@feralint > > erac > > > > tive > > > > > > .com> wrote: > > > > > > > Allow that capability if the driver indicates that it is > > > > > > > supported, and > > > > > > > flag whether images are read-only/write-only in the > > > > nir_variable > > > > > > > (based > > > > > > > on the NonReadable and NonWritable decorations), which > > > > drivers > > > > > > > may need > > > > > > > to implement this. > > > > > > > > > > > > > > Signed-off-by: Alex Smith <<a href="mailto:asmith@feralinteractive.com" target="_blank">asmith@feralinteractive.com</a>> > > > > > > > --- > > > > > > > src/compiler/spirv/nir_spirv.h | 1 + > > > > > > > src/compiler/spirv/spirv_to_nir.c | 5 ++++- > > > > > > > src/compiler/spirv/vtn_variables.c | 5 ++++- > > > > > > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/src/compiler/spirv/nir_spirv.h > > > > > > > b/src/compiler/spirv/nir_spirv.h > > > > > > > index e0ebc62..e43e9b5 100644 > > > > > > > --- a/src/compiler/spirv/nir_spirv.h > > > > > > > +++ b/src/compiler/spirv/nir_spirv.h > > > > > > > @@ -49,6 +49,7 @@ struct nir_spirv_supported_extensions { > > > > > > > bool image_ms_array; > > > > > > > bool tessellation; > > > > > > > bool draw_parameters; > > > > > > > + bool image_write_without_format; > > > > > > > }; > > > > > > > > > > > > > > nir_function *spirv_to_nir(const uint32_t *words, size_t > > > > > > > word_count, > > > > > > > diff --git a/src/compiler/spirv/spirv_to_nir.c > > > > > > > b/src/compiler/spirv/spirv_to_nir.c > > > > > > > index 416e12a..3f77ddf 100644 > > > > > > > --- a/src/compiler/spirv/spirv_to_nir.c > > > > > > > +++ b/src/compiler/spirv/spirv_to_nir.c > > > > > > > @@ -2666,7 +2666,6 @@ > > vtn_handle_preamble_instruction(struct > > > > > > > vtn_builder *b, SpvOp opcode, > > > > > > > case SpvCapabilityMinLod: > > > > > > > case SpvCapabilityTransformFeedback: > > > > > > > case SpvCapabilityStorageImageReadWithoutFormat: > > > > > > > - case SpvCapabilityStorageImageWriteWithoutFormat: > > > > > > > vtn_warn("Unsupported SPIR-V capability: %s", > > > > > > > spirv_capability_to_string(cap)); > > > > > > > break; > > > > > > > @@ -2702,6 +2701,10 @@ > > vtn_handle_preamble_instruction(struct > > > > > > > vtn_builder *b, SpvOp opcode, > > > > > > > spv_check_supported(draw_parameters, cap); > > > > > > > break; > > > > > > > > > > > > > > + case SpvCapabilityStorageImageWriteWithoutFormat: > > > > > > > + spv_check_supported(image_write_without_format, > > > > cap); > > > > > > > + break; > > > > > > > + > > > > > > > default: > > > > > > > unreachable("Unhandled capability"); > > > > > > > } > > > > > > > diff --git a/src/compiler/spirv/vtn_variables.c > > > > > > > b/src/compiler/spirv/vtn_variables.c > > > > > > > index 098cfb5..d7d882e 100644 > > > > > > > --- a/src/compiler/spirv/vtn_variables.c > > > > > > > +++ b/src/compiler/spirv/vtn_variables.c > > > > > > > @@ -1054,8 +1054,12 @@ apply_var_decoration(struct > > > > vtn_builder > > > > > > > *b, nir_variable *nir_var, > > > > > > > assert(nir_var->constant_initializer != NULL); > > > > > > > nir_var->data.read_only = true; > > > > > > > break; > > > > > > > + case SpvDecorationNonReadable: > > > > > > > + nir_var->data.image.write_only = true; > > > > > > > + break; > > > > > > > case SpvDecorationNonWritable: > > > > > > > nir_var->data.read_only = true; > > > > > > > + nir_var->data.image.read_only = true; > > > > > > > break; > > > > > > > case SpvDecorationComponent: > > > > > > > nir_var->data.location_frac = dec->literals[0]; > > > > > > > @@ -1107,7 +1111,6 @@ apply_var_decoration(struct > > vtn_builder > > > > *b, > > > > > > > nir_variable *nir_var, > > > > > > > case SpvDecorationAliased: > > > > > > > case SpvDecorationVolatile: > > > > > > > case SpvDecorationCoherent: > > > > > > > - case SpvDecorationNonReadable: > > > > > > > case SpvDecorationUniform: > > > > > > > case SpvDecorationStream: > > > > > > > case SpvDecorationOffset: > > > > > > > -- > > > > > > > 2.7.4 > > > > > > > > > > > > > > _______________________________________________ > > > > > > > mesa-dev mailing list > > > > > > > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> > > > > > > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > mesa-dev mailing list > > > > > > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> > > > > > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > > _______________________________________________ > > > > > mesa-dev mailing list > > > > > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> > > > > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > > > </div></div></blockquote></div> </div></div>