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@xxxxxxx> 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@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: dri-devel@xxxxxxxxxxxxxxxxxxxxx > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > 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 >
