[PATCH v2 00/21] Enable Colorspace connector property in amdgpu

Tue Mar 21 13:12:19 UTC 2023

FWIW, I still think this series is good (minus the UAPI changes) and
would allow us to work on user space HDR support without custom
kernels.

On Fri, Jan 13, 2023 at 5:24 PM Harry Wentland <harry.wentland at amd.com> wrote:
>
> This patchset enables the DP and HDMI infoframe properties
> in amdgpu.
>
> The first two patches are not completely related to the rest. The
> first patch allows for HDR_OUTPUT_METADATA with EOTFs that are
> unknown in the kernel.
>
> The second one prints a connector's max_bpc as part of the atomic
> state debugfs print.
>
> The following patches rework the connector colorspace code to
> 1) allow for easy printing of the colorspace in the drm_atomic
>    state debugfs, and
> 2) allow drivers to specify the supported colorspaces on a
>    connector.
>
> The rest of the patches deal with the Colorspace enablement
> in amdgpu.
>
> Why do drivers need to specify supported colorspaces? The amdgpu
> driver needs support for RGB-to-YCbCr conversion when we drive
> the display in YCbCr. This is currently not implemented for all
> colorspaces.
>
> Since the Colorspace property didn't have an IGT test I added
> one to kms_hdr. The relevant patchset can be found on the IGT
> mailing list or on
> https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/tree/hdr-colorimetry
>
> We tested v1 of the patchset and confirmed that the infoframes
> are as expected for both DP and HDMI when running the IGT
> colorimetry tests.
>
> Open Items
> ----------
>
> A couple comments from Pekka about colorspace documentation are
> left unaddressed. I hope they won't block merging this set but
> should still be addressed separately.
>
> Pekka's questions really got me thinking of how this colorspace
> property should be used and working with it more closely with
> Joshua who is enabling HDR in gamescope made me wonder even more.
>
> Uma, is there a (canonical, upstream) userspace that uses this
> property that I can look at to understand more?
>
> One of the key challenges that is currently not addressed is that
> userspace is expected to pick a colorspace format straight from the
> list of definitions out of the DP or HDMI spec. But the kernel
> driver are the ones deciding on the output encoding (RGB, YCBCR444,
> YCBCR420, etc.). So there is no way for userspace to decide correctly
> between, for example, BT2020_RGB, BT2020_CYCC, BT2020_YCC.
>
> So we end up in a scenario where gamescope sets BT2020_RGB but we
> output YCBCR444 so have to correct the colorspace value to
> BT2020_YCC. This in turn breaks the colorspace IGT tests I
> wrote. I don't think "fixing" the IGT tests to accept this is
> the right thing to do.
>
> The way it stands this patchset allows us to specify the output
> colorspace on amdgpu and we try to do the right thing, but I don't
> thing the way the colorspace property is defined is right. We're trying
> to expose things to userspace that should be under driver control. A
> much better approach would be to give userspace options for colorspace
> that are not tied to DP or HDMI specs, i.e., sRGB, BT709, BT2020, etc.,
> and have the driver do the right thing to fill the infoframe, e.g., by
> picking BT2020_YCC if the requested colorspace is BT2020 and the
> is YCBCR444.
>
> If no upstream userspace currently makes use of this property I
> can make that change, i.e., no longer tie the colorspace property
> directly to the infoframe and reduce the options to sRGB, BT709,
> BT601, and BT2020 (and possibly opRGB).
>
> v2:
> - Tested with DP and HDMI analyzers
> - Confirmed driver will fallback to lower bpc when needed
> - Dropped hunk to set HDMI AVI infoframe as it was a no-op
> - Fixed BT.2020 YCbCr colorimetry (JoshuaAshton)
> - Simplify initialization of supported colorspaces (Jani)
> - Fix kerneldoc (kernel test robot)
>
> Cc: Pekka Paalanen <ppaalanen at gmail.com>
> Cc: Sebastian Wick <sebastian.wick at redhat.com>
> Cc: Vitaly.Prosyak at amd.com
> Cc: Uma Shankar <uma.shankar at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Joshua Ashton <joshua at froggi.es>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Michel Dänzer <michel.daenzer at mailbox.org>
> Cc: dri-devel at lists.freedesktop.org
> Cc: amd-gfx at lists.freedesktop.org
>
> Harry Wentland (16):
>   drm/display: Don't block HDR_OUTPUT_METADATA on unknown EOTF
>   drm/connector: print max_requested_bpc in state debugfs
>   drm/connector: Drop COLORIMETRY_NO_DATA
>   drm/connector: Convert DRM_MODE_COLORIMETRY to enum
>   drm/connector: Pull out common create_colorspace_property code
>   drm/connector: Allow drivers to pass list of supported colorspaces
>   drm/connector: Print connector colorspace in state debugfs
>   drm/amd/display: Always pass connector_state to stream validation
>   drm/amd/display: Register Colorspace property for DP and HDMI
>   drm/amd/display: Signal mode_changed if colorspace changed
>   drm/amd/display: Send correct DP colorspace infopacket
>   drm/amd/display: Add support for explicit BT601_YCC
>   drm/amd/display: Add debugfs for testing output colorspace
>   drm/amd/display: Add default case for output_color_space switch
>   drm/amd/display: Don't restrict bpc to 8 bpc
>   drm/amd/display: Format input and output CSC matrix
>
> Joshua Ashton (5):
>   drm/amd/display: Always set crtcinfo from create_stream_for_sink
>   drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not
>     RGB
>   drm/amd/display: Refactor avi_info_frame colorimetry determination
>   drm/amd/display: Calculate output_color_space after pixel encoding
>     adjustment
>   drm/amd/display: Fix COLOR_SPACE_YCBCR2020_TYPE matrix
>
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  90 ++++++---
>  .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c |  57 ++++++
>  .../drm/amd/display/dc/core/dc_hw_sequencer.c |  38 ++--
>  .../gpu/drm/amd/display/dc/core/dc_resource.c |  28 ++-
>  drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h   |  54 +++--
>  drivers/gpu/drm/display/drm_hdmi_helper.c     |   8 +-
>  drivers/gpu/drm/drm_atomic.c                  |   2 +
>  drivers/gpu/drm/drm_connector.c               | 189 ++++++++++--------
>  .../gpu/drm/i915/display/intel_connector.c    |   4 +-
>  drivers/gpu/drm/vc4/vc4_hdmi.c                |   2 +-
>  include/drm/display/drm_dp.h                  |   2 +-
>  include/drm/drm_connector.h                   |  57 +++---
>  12 files changed, 345 insertions(+), 186 deletions(-)
>
> --
> 2.39.0
>