On Wed, 2017-02-15 at 10:24 -0800, Jason Ekstrand wrote:
> On Wed, Feb 15, 2017 at 10:09 AM, Juan A. Suarez Romero <jasua...@igalia.com> 
> wrote:
> > According to Ivybridge PRM, Volume 4 Part 1 p73, signed integer formats
> > 
> > cannot be multisampled.
> > 
> > 
> > 
> > Also in the same PRM p63, any format with greater than 64 bits per
> > 
> > element cannot be multisampled.
> > 
> > 
> > 
> > This fixes the following Vulkan CTS tests in Haswell:
> > 
> > 
> > 
> > dEQP-VK.memory.requirements.image.regular_tiling_optimal
> > 
> > dEQP-VK.memory.requirements.image.transient_tiling_optimal
> > 
> > 
> > 
> > It also fixes crashes in the following Vulkan CTS tests in Haswell 
> > (becoming now
> > 
> > skip):
> > 
> > 
> > 
> > dEQP-VK.glsl.texture_functions.query.texturesamples.isampler2dms_fragment
> > 
> > dEQP-VK.glsl.texture_functions.query.texturesamples.isampler2dms_vertex
> > 
> > dEQP-VK.glsl.texture_functions.query.texturesamples.isampler2dmsarray_fragment
> > 
> > dEQP-VK.glsl.texture_functions.query.texturesamples.isampler2dmsarray_vertex
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.64x64_1.r16g16_sint.samples_4
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.64x64_1.r16g16_sint.samples_8
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.64x64_1.r32g32b32a32_sfloat.samples_4
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.64x64_1.r32g32b32a32_sfloat.samples_8
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.64x64_4.r16g16_sint.samples_4
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.64x64_4.r16g16_sint.samples_8
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.64x64_4.r32g32b32a32_sfloat.samples_4
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.64x64_4.r32g32b32a32_sfloat.samples_8
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.79x31_1.r16g16_sint.samples_4
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.79x31_1.r16g16_sint.samples_8
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.79x31_1.r32g32b32a32_sfloat.samples_4
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.79x31_1.r32g32b32a32_sfloat.samples_8
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.79x31_4.r16g16_sint.samples_4
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.79x31_4.r16g16_sint.samples_8
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.79x31_4.r32g32b32a32_sfloat.samples_4
> > 
> > dEQP-VK.pipeline.multisample.sampled_image.79x31_4.r32g32b32a32_sfloat.samples_8
> > 
> > 
> > 
> > Signed-off-by: Juan A. Suarez Romero <jasua...@igalia.com>
> > 
> > ---
> > 
> >  src/intel/vulkan/anv_device.c  |  4 ++--
> > 
> >  src/intel/vulkan/anv_formats.c | 32 ++++++++++++++++++++++++++++++++
> > 
> >  2 files changed, 34 insertions(+), 2 deletions(-)
> > 
> > 
> > 
> > diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> > 
> > index d1a6cc8..a8ab8c3 100644
> > 
> > --- a/src/intel/vulkan/anv_device.c
> > 
> > +++ b/src/intel/vulkan/anv_device.c
> > 
> > @@ -622,12 +622,12 @@ void anv_GetPhysicalDeviceProperties(
> > 
> >        .maxFramebufferWidth                      = (1 << 14),
> > 
> >        .maxFramebufferHeight                     = (1 << 14),
> > 
> >        .maxFramebufferLayers                     = (1 << 11),
> > 
> > -      .framebufferColorSampleCounts             = sample_counts,
> > 
> > +      .framebufferColorSampleCounts             = devinfo->gen == 7 ? 
> > VK_SAMPLE_COUNT_1_BIT : sample_counts,
> > 
> >        .framebufferDepthSampleCounts             = sample_counts,
> > 
> >        .framebufferStencilSampleCounts           = sample_counts,
> > 
> >        .framebufferNoAttachmentsSampleCounts     = sample_counts,
> > 
> >        .maxColorAttachments                      = MAX_RTS,
> > 
> > -      .sampledImageColorSampleCounts            = sample_counts,
> > 
> > +      .sampledImageColorSampleCounts            = devinfo->gen == 7 ? 
> > VK_SAMPLE_COUNT_1_BIT : sample_counts,
> 
> The Vulkan spec says we're supposed to support at least 1x and 4x on both of 
> these.
> 

You're right. I didn't notice about this requirement.
> So I guess the real question is what's the best choice that will let apps 
> run.  My feeling is that setting these fields to 1x and 4x and then just 
> dying horribly if they use an integer format is probably actually the better 
> choice.  That said, we should definitely make sure we're dying horribly if 
> they use an integer format!

I agree. Right now we are already crashing if we try it to use that 4x 
multisampling with a SINT format. With this patch, at least if user asks if 
this combination is supported with vkGetPhysicalDeviceImageFormatProperties(), 
we will return FALSE.
The bad part is that we are somewhat breaking the spec: sampleCounts returned 
by that function must be a superset of framebufferColorSampleCounts; but we are 
returning 1x when framebufferColorSampleCounts is 1x and 4x. But I think we can 
live with it.
I'll send a V2 version.

>  
> >        .sampledImageIntegerSampleCounts          = VK_SAMPLE_COUNT_1_BIT,
> > 
> >        .sampledImageDepthSampleCounts            = sample_counts,
> > 
> >        .sampledImageStencilSampleCounts          = sample_counts,
> > 
> > diff --git a/src/intel/vulkan/anv_formats.c b/src/intel/vulkan/anv_formats.c
> > 
> > index 6005791..3eac931 100644
> > 
> > --- a/src/intel/vulkan/anv_formats.c
> > 
> > +++ b/src/intel/vulkan/anv_formats.c
> > 
> > @@ -561,6 +561,38 @@ anv_get_image_format_properties(
> > 
> >        sampleCounts = 
> > isl_device_get_sample_counts(&physical_device->isl_dev);
> > 
> >     }
> > 
> > 
> > 
> > +   /*
> > 
> > +    * From the Ivybridge PRM, Volume 4 Part 1 p73, SURFACE_STATE, Number of
> > 
> > +    * Multisamples:
> > 
> > +    *
> > 
> > +    *    - This field must be set to MULTISAMPLECOUNT_1 for SINT MSRTs when
> > 
> > +    *      all RT channels are not written.
> > 
> > +    *
> > 
> > +    * And errata from the Ivybridge PRM, Volume 4 Part 1 p77,
> > 
> > +    * RENDER_SURFACE_STATE, MCS Enable:
> > 
> > +    *
> > 
> > +    *   This field must be set to 0 [MULTISAMPLECOUNT_1] for all SINT MSRTs
> > 
> > +    *   when all RT channels are not written.
> > 
> > +    *
> > 
> > +    * Note that the above SINT restrictions apply only to *MSRTs* (that is,
> > 
> > +    * *multisampled* render targets). The restrictions seem to permit an 
> > MCS
> > 
> > +    * if the render target is singlesampled.
> > 
> > +    *
> > 
> > +    * From the IvyBridge PRM, Volume 4 Part 1 p63, SURFACE_STATE, Surface
> > 
> > +    * Format:
> > 
> > +    *
> > 
> > +    *    If Number of Multisamples is set to a value other than
> > 
> > +    *    MULTISAMPLECOUNT_1, this field cannot be set to the following
> > 
> > +    *    formats: any format with greater than 64 bits per element, if 
> > Number
> > 
> > +    *    of Multisamples is MULTISAMPLECOUNT_8, any compressed texture 
> > format
> > 
> > +    *    (BC*), and any YCRCB* format.
> > 
> > +    */
> > 
> > +   if (physical_device->info.gen < 8 &&
> > 
> > +       (isl_format_has_sint_channel(anv_formats[info->format].isl_format) 
> > ||
> > 
> > +        isl_format_get_layout(anv_formats[info->format].isl_format)->bpb > 
> > 64)) {
> > 
> > +      sampleCounts = VK_SAMPLE_COUNT_1_BIT;
> 
> We have this very helpful function in isl called 
> isl_format_supports_multisampling which I don't believe has the SINT 
> restriction yet.  I think the better thing to do would be to add the SINT 
> restriction to that helper and use it rather than hand-coding it here.
>  

Nice. I'll add it the check and use this function instead.
> > +   }
> > 
> > +
> > 
> >     if (info->usage & (VK_IMAGE_USAGE_TRANSFER_SRC_BIT |
> > 
> >                        VK_IMAGE_USAGE_TRANSFER_DST_BIT)) {
> > 
> >        /* Accept transfers on anything we can sample from or renderer to. */
> > 
> > --
> > 
> > 2.9.3
> > 
> > 
> > 
> > 
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to