[PATCH v5 00/13] Enable Colorspace connector property in amdgpu

[Joshua Ashton]

Thanks Harry. Looks good.

Reviewed-by: Joshua Ashton <joshua at froggi.es>

- Joshie 🐸✨

On 6/6/23 21:25, Harry Wentland wrote:
> This patchset is based on Joshua's previous patchset [1], as well
> as my previous patchset [2].
> 
> It is
> - enabling support for the colorspace property in amdgpu, as well as
> - allowing drivers to specify the supported set of colorspaces, and
> 
> Colorspace, Infoframes, and YCbCr matrix
> ---------------------------------------
> 
> Even though the initial intent of the colorspace property was to set the
> colorspace field in the respective HDMI AVI and DP SDP infoframes that
> is not sufficient in all scenarios. For DP the colorspace information
> also affects the MSA (main stream attribute) packet. For YUV output the
> colorspace affects the RGB-to-YCbCr conversion matrix. The colorspace
> field of the infopackets also depends on the encoding used, which is
> something that is decided by the driver and not known to userspace.
> 
> For these reasons a driver will need to be able to select the supported
> colorspaces at property creation.
> 
> Note: There seems to be an understanding that the colorspace property
> should ONLY modify the infoframe. While this is current behavior and
> sufficient in some cases it is nowhere specified that this should be the
> only use of this property. As outlined above this limitation is not
> going to work in all cases.
> 
> This patchset does not affect current behavior for the drivers that
> implement this property: i915 and vc4.
> 
> In the future we might want to give userspace control over the encoding
> format on the wire, in particular to avoid use of YUV420 when image
> fidelity is important. This work would likely go hand in hand with a
> min_bpc property and wouldn't conflict with the work done in this
> patchset. I would expect this future work to tag along with a drm_crtc
> or drm_connector's Color Pipeline, similar to the one propsed for
> drm_plane [3].
> 
> Colorspace on crtc or connector?
> --------------------------------
> 
> There have been suggestions of programming 'colorspace' on the drm_crtc
> but I don't think the crtc is the right place for this property. The
> drm_plane and drm_crtc will be used to offload color processing that
> would normally be done via the GFX or other pipelines. The drm_connector
> controls the signalling with the display and ensures the wire format is
> appropriate for the encoding by programming the RGB-to-YCbCr matrix.
> 
> [1] https://patchwork.freedesktop.org/series/113632/
> [2] https://patchwork.freedesktop.org/series/111865/
> [3] https://lists.freedesktop.org/archives/dri-devel/2023-May/403173.html
> 
> 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)
> 
> v3:
> - Added documentation for colorspaces (Pekka, Joshua)
> - Split 'Allow drivers to pass list of supported colorspaces' patch
>    to pull out code to create common colorspace array and keep it separate
>    from change to create only supported colorspaces
> 
> v4:
> - Don't "deprecate" existing enum values
> - Fixes based on review comments throughout
> - Dropped Josh's RBs
> 
> v5:
> - Add documentation that drivers are free to pick appropriate
>    RGB or YCC variant
> 
> 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: Simon Ser <contact at emersion.fr>
> Cc: Melissa Wen <mwen at igalia.com>
> Cc: dri-devel at lists.freedesktop.org
> Cc: amd-gfx at lists.freedesktop.org
> 
> Harry Wentland (10):
>    drm/connector: Convert DRM_MODE_COLORIMETRY to enum
>    drm/connector: Pull out common create_colorspace_property code
>    drm/connector: Use common colorspace_names array
>    drm/connector: Print connector colorspace in state debugfs
>    drm/connector: Allow drivers to pass list of supported colorspaces
>    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 debugfs for testing output colorspace
> 
> Joshua Ashton (3):
>    drm/connector: Add enum documentation to drm_colorspace
>    drm/amd/display: Always set crtcinfo from create_stream_for_sink
>    drm/amd/display: Refactor avi_info_frame colorimetry determination
> 
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  84 ++++++---
>   .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c |  57 ++++++
>   .../gpu/drm/amd/display/dc/core/dc_resource.c |  28 +--
>   drivers/gpu/drm/drm_atomic.c                  |   1 +
>   drivers/gpu/drm/drm_connector.c               | 176 +++++++++++-------
>   .../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                   | 129 ++++++++++---
>   9 files changed, 349 insertions(+), 134 deletions(-)
> 
> --
> 2.41.0
>