Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

color_space_helper: Allow 8- and 10-bit fmts for the sRGB color space #175

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Dec 21, 2023

Most texture formats should be presentable on the default sRGB color space, in particular 10-bit formats for higher bit depth and detail than the typical 8-bit.

A couple notes:

  • This currently only affects VK_KHR_display as there is a needsWorkaround case in PhysicalDevice::GetSurfaceFormats() that returns exclusively RGBA8 and BGRA8 in the windowed/compositor case;
  • Before this patch, A2B10G10R10 wasn't in the list returned by vkGetPhysicalDeviceSurfaceFormatsKHR() but worked anyway on VK_KHR_display without validation errors;
  • Using the A2B10G10R10 format inside a 10-bit compositor works anyway, albeit with validation layer errors;
  • RADV also allows 10-bit sRGB swapchains, but only in a compositor, not in VK_KHR_display;
  • Windows also does not report support for this 10-bit sRGB format combination, but it works anyway (also with validation layer errors);
  • By using Fmt_All, the R16G16B16A16_UNORM and R16G16B16A16_SFLOAT formats become available for the sRGB color space, but creating a swapchain with them leaves a black screen. If this is truly unsupported, perhaps there should be a new Fmt_8bpc_10bpc?
  • This was previously Fmt_All already before commit 02e867e ("Update xgl from commit 2aeb0b25") but it is assumed that sub-commit "Fix ColorspaceHelper's lookup table so it only reports formats that are legal" changed this behaviour. At least 10-bit sRGB should be retained?

(When saying "it works", I mean that it works as intended, i.e. any 8-bit banding previously observable is gone when using a 10-bit swapchain)

@amdvlk-admin
Copy link
Collaborator

Can one of the admins verify this patch?

@jinjianrong jinjianrong requested a review from a team January 9, 2024 12:07
@amdvlk-admin
Copy link
Collaborator

Test summary for commit 8dbca14

CTS tests (Failed: 267/207843)
  • Built with version 1.3.5.2
  • Rhel 8.9, Gfx10
    • Passed: 30909/69309 (44.6%)
    • Failed: 267/69309 (0.4%)

      Failures:

      FAILURE: dEQP-VK.binding_model.buffer_device_address.set3.depth1.basessbo.convertchecku64.nostore.replay.scalar.frag
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.buffer_device_address.set3.depth2.baseubo.load.store.multi.scalar.vert
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.buffer_device_address.set3.depth3.basessbo.convert.nostore.multi.scalar.vert
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.buffer_device_address.set3.depth3.basessbo.crossconvertu2p.nostore.single.std140.vert
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.buffer_device_address.set7.depth1.baseubo.load.nostore.replay.std140.comp
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.descriptor_buffer.max.graphics_tesc_sampler1_resource7
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.descriptor_buffer.robust.null_descriptor.graphics_geom_storage_buffer
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.descriptor_copy.graphics.uniform_buffer_1
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.descriptor_copy.graphics.uniform_texel_buffer_array2
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.descriptor_update.samplerless.sampled_img_sampler_zero
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.mutable_descriptor.single.sampler.update_copy.mutable_source.normal_source.pool_no_types.pre_update.no_array.frag
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.mutable_descriptor.single.switches.storage_buffer_sampler.update_copy.mutable_source.normal_source.pool_same_types.pre_update.no_array.comp
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.mutable_descriptor.single.switches.uniform_buffer_storage_image.update_copy.nonmutable_source.normal_source.pool_no_types.pre_update.no_array.vert
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.shader_access.primary_cmd_buf.with_push_template.combined_image_sampler_mutable.vertex.multiple_discontiguous_descriptors.2d_base_slice
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.shader_access.secondary_cmd_buf.uniform_texel_buffer.tess_eval.multiple_discontiguous_descriptor_sets.multiple_arbitrary_descriptors.offset_zero
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.shader_access.secondary_cmd_buf.with_template.sampler_immutable.vertex_fragment.multiple_contiguous_descriptors.3d
      Stack trace: Flake
      
      FAILURE: dEQP-VK.clipping.user_defined.clip_cull_distance.vert.8
      Stack trace: Flake
      
      FAILURE: dEQP-VK.clipping.user_defined.clip_distance.vert_geom.6_fragmentshader_read
      Stack trace: Fail
      
      FAILURE: dEQP-VK.conditional_rendering.conditional_ignore.clear_condition_host_memory_inherited_expect_execution
      Stack trace: Fail
      
      FAILURE: dEQP-VK.conditional_rendering.conditional_ignore.clear_condition_host_memory_inherited_expect_noop_inverted
      Stack trace: Fail
      ...
      

    • Not Supported: 38133/69309 (55.0%)
    • Warnings: 0/69309 (0.0%)
    Ubuntu navi3x, Srdcvk
    • Passed: 35161/69241 (50.8%)
    • Failed: 0/69241 (0.0%)
    • Not Supported: 34080/69241 (49.2%)
    • Warnings: 0/69241 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35241/69293 (50.9%)
    • Failed: 0/69293 (0.0%)
    • Not Supported: 34052/69293 (49.1%)
    • Warnings: 0/69293 (0.0%)

@JoeWangAMD
Copy link

That commit will make “R16G16B16A16_UNORM and R16G16B16A16_SFLOAT formats become available for the sRGB color space”, but currently Linux display only supports 32bits formats.

Need to define a “new Fmt_8bpc_10bpc” for format “VK_FORMAT_B8G8R8A8_UNORM”, “VK_FORMAT_B8G8R8A8_SRGB” and “VK_FORMAT_A2R10G10B10_UNORM_PACK32”. Please don’t use “Fmt_All” directly.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jan 17, 2024

@JoeWangAMD these would be the plane formats supported by amdgpu, correct? Is there a concrete list that only allows a few plane formats for the sRGB color space?

EDIT: For future reference some of this info might come from https://drmdb.emersion.fr/snapshots/e0ffff941ce7.

Let's introduce the suggested Fmt_8bpc_10bpc group and expand this later? We can also name it Fmt_KnownSRGB to expand it when new formats appear?

@MarijnS95 MarijnS95 marked this pull request as ready for review January 17, 2024 10:25
@MarijnS95 MarijnS95 changed the title color_space_helper: Allow all texture formats for the sRGB color space color_space_helper: Allow 8- and 10-bit fmts for the sRGB color space Jan 17, 2024
@JoeWangAMD
Copy link

I don't have a concrete list currently, but at least we have proved the following formats are supported.
“VK_FORMAT_B8G8R8A8_UNORM”, “VK_FORMAT_B8G8R8A8_SRGB” and “VK_FORMAT_A2R10G10B10_UNORM_PACK32”.

Suggest adding the format restrictions for the 10bits. Thanks. Sample code below.
static const uint32_t colorspaceLookupSize = sizeof(colorspaceLookup) / sizeof(colorspaceLookup[0]);
@@ -126,6 +131,10 @@ ColorSpaceHelper::FmtSupport ColorSpaceHelper::GetBitFormat(Pal::ChNumFormat pal
break;
case 10:
fmt = Fmt_10bpc;

  •        if (Pal::Formats::IsUnorm(palFormat))
    
  •        {
    
  •            fmt = Fmt_10bpc_unorm;
    
  •        }
    

@MarijnS95
Copy link
Contributor Author

Hi @JoeWangAMD, while related to color spaces that should not be too important for this PR (the VK_COLOR_SPACE_SRGB_NONLINEAR_KHR should be usable with both UNORM and SRGB texture formats theoretically; I have not validated this though).

I have however pushed the split in a separate commit, and use it in non-sRGB 10-bit formats.

Fmt_8bpc = Fmt_8bpc_srgb | Fmt_8bpc_unorm,
Fmt_10bpc = Fmt_10bpc_srgb | Fmt_10bpc_unorm,
Fmt_16bpc = Fmt_16bpc_unorm | Fmt_16bpc_sfloat,
Fmt_KnownHDR = Fmt_10bpc | Fmt_11bpc | Fmt_12bpc | Fmt_16bpc,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that 10-bit sRGB, while technically invalid for TfHlg (where this KnownHDR format is used), is still HDR. Should we change TfHlg to use a new Fmt, without the srgb variant?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your information @MarijnS95 . Could you help change "Fmt_KnownHDR = Fmt_10bpc_unorm| Fmt_11bpc | Fmt_12bpc | Fmt_16bpc" in this commit? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoeWangAMD sounds good! We can also decide to rename it to Fmt_KnownHLG, is that more clear or would you prefer to stick with KnownHDR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoeWangAMD I've updated this and also added Fmt_10bpc_srgb to Fmt_All to complement this change.

I haven't renamed it to HLG as this flag is also used in the IsFmtHdr() function.

Most texture formats should be presentable on the default sRGB color
space, in particular 10-bit formats for higher bit depth and detail than
the typical 8-bit.

A couple notes:
- This currently only affects `VK_KHR_display` as there is a
  `needsWorkaround` case in `PhysicalDevice::GetSurfaceFormats()` that
  returns exclusively RGBA8 and BGRA8 in the windowed/compositor case;
- Before this patch, `A2B10G10R10` wasn't in the list returned by
  `vkGetPhysicalDeviceSurfaceFormatsKHR()` but worked anyway on
  `VK_KHR_display` _without_ validation errors;
- Using the `A2B10G10R10` format inside a 10-bit compositor works
  anyway, albeit with validation layer errors;
- RADV also allows 10-bit sRGB swapchains, but only in a compositor, not
  in `VK_KHR_display`;
- Windows also does not report support for this 10-bit sRGB format
  combination, but it works anyway (also with validation layer errors);
- This was previously `Fmt_All` already before commit 02e867e ("Update
  xgl from commit 2aeb0b25") but it is assumed that sub-commit "Fix
  ColorspaceHelper's lookup table so it only reports formats that are
  legal" changed this behaviour. At least 10-bit sRGB should be retained?

(When saying "it works", I mean that it works as intended, i.e.
any 8-bit banding previously observable is gone when using a 10-bit
swapchain)
@amdvlk-admin
Copy link
Collaborator

Test summary for commit 03b1852

CTS tests (Failed: 0/142491)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35943/71229 (50.5%)
    • Failed: 0/71229 (0.0%)
    • Not Supported: 35286/71229 (49.5%)
    • Warnings: 0/71229 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35779/71262 (50.2%)
    • Failed: 0/71262 (0.0%)
    • Not Supported: 35483/71262 (49.8%)
    • Warnings: 0/71262 (0.0%)

@qiaojbao qiaojbao merged commit 44902e7 into GPUOpen-Drivers:dev Feb 19, 2024
1 check passed
@MarijnS95 MarijnS95 deleted the 10-bit-srgb branch March 10, 2024 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants