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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks Harry. Looks good.

Reviewed-by: Joshua Ashton <joshua@xxxxxxxxx>

- 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@xxxxxxxxx>
Cc: Sebastian Wick <sebastian.wick@xxxxxxxxxx>
Cc: Vitaly.Prosyak@xxxxxxx
Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Cc: Joshua Ashton <joshua@xxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
Cc: Michel Dänzer <michel.daenzer@xxxxxxxxxxx>
Cc: Simon Ser <contact@xxxxxxxxxxx>
Cc: Melissa Wen <mwen@xxxxxxxxxx>
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx

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





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux