Hi,
these two patches fix non-working HDMI deep color output on DCE-8.3, AMD Mullins when amdgpu-kms is used with Displaycore force-enabled, ie. for radeon.cik_support=0 amdgpu.cik_support=1 amdgpu.dc=1: I suspect they might fix similar problems on other older asics of DCE-11.0 and earlier.
Patch 1/2 is a fix for some oddity i found while hunting for the HDMI deep color bug. It fixes what looks like an obvious mistake, but the fix did not improve or degrade anything, so maybe the hw doesn't care all too much about the wrong clamping/truncation mask? Anyway, it makes the code less confusing.
Patch 2/2 fixes HDMI deep color output at 10 bpc or 12 bpc output on AMD Mullins, DCE-8.3, where at output_bpc 10 or 12 the display would be scrambled. With the patch, the display looks correct, and photometer measurements on a HDR-10 monitor suggest we probably get the correct output signal. I found the fix by comparing DC against the classic amdgpu-kms and radeon-kms modesetting path, readding missing stuff.
Given other encoder/pixelclock setup functions than the ones used on DCE-8.3 showed the same omissions, i added missing code there as well, but i couldn't test it due to lack of hw, so i hope that fixes instead of breaks things on asic's other than DCE-8.3.
I also created a similar patch for DCE-11.2 and later, not included here, but during testing on a Raven Ridge DCN-1, the patch neither helped nor hurt. Output was correct without the patch, and adding the patch didn't change or break anything on DCN-1. Looking at disassembled AtomBios tables for DCN-1 and a DCE-11.2, i think AtomBios may not do much with the info that was missing, which would explain why the current upstream code seems to work fine without it? At least as verified on DCN-1. I can't test on DCE-11.2 or DCE-12 due to lack of hw with actual HDMI output. But it would be interesting for me to know what changed wrt. Atombios in later asic versions to make some of this setup apparently redundant in DC?
Do you test DC wrt. HDMI deep color starting at a specific DCE revision, given that the bug went unnoticed in DCE-8.3, but things seem to be fine on at least DCN-1?
Thanks, -mario
In set_clamp(), the comments and definitions for the COLOR_DEPTH_101010 and COLOR_DEPTH_121212 cases directly contradict the code comment which explains how this should work, whereas the COLOR_DEPTH_888 case is consistent with the code comments. Comment says the bitmask should be chosen to align to the top-most 10 or 12 MSB's on a 14 bit bus, but the implementation contradicts that: 10 bit case sets a mask for 12 bpc clamping, whereas 12 bit case sets a mask for 14 bpc clamping.
Note that during my limited testing on DCE-8.3 (HDMI deep color) and DCE-11.2 (DP deep color), this didn't have any obvious ill effects, neither did fixing it change anything obvious for the better, so this fix may be inconsequential on DCE, and just reduce the confusion of innocent bystanders when reading the code and trying to investigate problems with 10 bpc+ output.
Fixes: 4562236b3bc0 ("drm/amd/dc: Add dc display driver (v2)")
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com Cc: Harry Wentland harry.wentland@amd.com --- drivers/gpu/drm/amd/display/dc/dce/dce_transform.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c index 130a0a0c8332..68028ec995e7 100644 --- a/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c @@ -601,12 +601,12 @@ static void set_clamp( clamp_max = 0x3FC0; break; case COLOR_DEPTH_101010: - /* 10bit MSB aligned on 14 bit bus '11 1111 1111 1100' */ - clamp_max = 0x3FFC; + /* 10bit MSB aligned on 14 bit bus '11 1111 1111 0000' */ + clamp_max = 0x3FF0; break; case COLOR_DEPTH_121212: - /* 12bit MSB aligned on 14 bit bus '11 1111 1111 1111' */ - clamp_max = 0x3FFF; + /* 12bit MSB aligned on 14 bit bus '11 1111 1111 1100' */ + clamp_max = 0x3FFC; break; default: clamp_max = 0x3FC0;
This fixes corrupted display output in HDMI deep color 10/12 bpc mode at least as observed on AMD Mullins, DCE-8.3.
It will hopefully also provide fixes for other DCE's up to DCE-11, assuming those will need similar fixes, but i could not test that for HDMI due to lack of suitable hw, so viewer discretion is advised.
dce110_stream_encoder_hdmi_set_stream_attribute() is used for HDMI setup on all DCE's and is missing color_depth assignment.
dce110_program_pix_clk() is used for pixel clock setup on HDMI for DCE 6-11, and is missing color_depth assignment.
Additionally some of the underlying Atombios specific encoder and pixelclock setup functions are missing code which is in the classic amdgpu kms modesetting path and the in the radeon kms driver for DCE6/DCE8.
encoder_control_digx_v3() - Was missing setup code wrt. amdgpu and radeon kms classic drivers. Added here, but untested due to lack of suitable test hw.
encoder_control_digx_v4() - Added missing setup code. Successfully tested on AMD mullins / DCE-8.3 with HDMI deep color output at 10 bpc and 12 bpc.
Note that encoder_control_digx_v5() has proper setup code in place and is used, e.g., by DCE-11.2, but this code wasn't used for deep color setup due to the missing cntl.color_depth setup in the calling function for HDMI.
set_pixel_clock_v5() - Missing setup code wrt. classic amdgpu/radeon kms. Added here, but untested due to lack of hw.
set_pixel_clock_v6() - Missing setup code added. Successfully tested on AMD mullins DCE-8.3. This fixes corrupted display output at HDMI deep color output with 10 bpc or 12 bpc.
Fixes: 4562236b3bc0 ("drm/amd/dc: Add dc display driver (v2)")
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com Cc: Harry Wentland harry.wentland@amd.com --- .../drm/amd/display/dc/bios/command_table.c | 61 +++++++++++++++++++ .../drm/amd/display/dc/dce/dce_clock_source.c | 14 +++++ .../amd/display/dc/dce/dce_stream_encoder.c | 1 + 3 files changed, 76 insertions(+)
diff --git a/drivers/gpu/drm/amd/display/dc/bios/command_table.c b/drivers/gpu/drm/amd/display/dc/bios/command_table.c index 070459e3e407..afc10b954ffa 100644 --- a/drivers/gpu/drm/amd/display/dc/bios/command_table.c +++ b/drivers/gpu/drm/amd/display/dc/bios/command_table.c @@ -245,6 +245,23 @@ static enum bp_result encoder_control_digx_v3( cntl->enable_dp_audio); params.ucLaneNum = (uint8_t)(cntl->lanes_number);
+ switch (cntl->color_depth) { + case COLOR_DEPTH_888: + params.ucBitPerColor = PANEL_8BIT_PER_COLOR; + break; + case COLOR_DEPTH_101010: + params.ucBitPerColor = PANEL_10BIT_PER_COLOR; + break; + case COLOR_DEPTH_121212: + params.ucBitPerColor = PANEL_12BIT_PER_COLOR; + break; + case COLOR_DEPTH_161616: + params.ucBitPerColor = PANEL_16BIT_PER_COLOR; + break; + default: + break; + } + if (EXEC_BIOS_CMD_TABLE(DIGxEncoderControl, params)) result = BP_RESULT_OK;
@@ -274,6 +291,23 @@ static enum bp_result encoder_control_digx_v4( cntl->enable_dp_audio)); params.ucLaneNum = (uint8_t)(cntl->lanes_number);
+ switch (cntl->color_depth) { + case COLOR_DEPTH_888: + params.ucBitPerColor = PANEL_8BIT_PER_COLOR; + break; + case COLOR_DEPTH_101010: + params.ucBitPerColor = PANEL_10BIT_PER_COLOR; + break; + case COLOR_DEPTH_121212: + params.ucBitPerColor = PANEL_12BIT_PER_COLOR; + break; + case COLOR_DEPTH_161616: + params.ucBitPerColor = PANEL_16BIT_PER_COLOR; + break; + default: + break; + } + if (EXEC_BIOS_CMD_TABLE(DIGxEncoderControl, params)) result = BP_RESULT_OK;
@@ -1057,6 +1091,19 @@ static enum bp_result set_pixel_clock_v5( * driver choose program it itself, i.e. here we program it * to 888 by default. */ + if (bp_params->signal_type == SIGNAL_TYPE_HDMI_TYPE_A) + switch (bp_params->color_depth) { + case TRANSMITTER_COLOR_DEPTH_30: + /* yes this is correct, the atom define is wrong */ + clk.sPCLKInput.ucMiscInfo |= PIXEL_CLOCK_V5_MISC_HDMI_32BPP; + break; + case TRANSMITTER_COLOR_DEPTH_36: + /* yes this is correct, the atom define is wrong */ + clk.sPCLKInput.ucMiscInfo |= PIXEL_CLOCK_V5_MISC_HDMI_30BPP; + break; + default: + break; + }
if (EXEC_BIOS_CMD_TABLE(SetPixelClock, clk)) result = BP_RESULT_OK; @@ -1135,6 +1182,20 @@ static enum bp_result set_pixel_clock_v6( * driver choose program it itself, i.e. here we pass required * target rate that includes deep color. */ + if (bp_params->signal_type == SIGNAL_TYPE_HDMI_TYPE_A) + switch (bp_params->color_depth) { + case TRANSMITTER_COLOR_DEPTH_30: + clk.sPCLKInput.ucMiscInfo |= PIXEL_CLOCK_V6_MISC_HDMI_30BPP_V6; + break; + case TRANSMITTER_COLOR_DEPTH_36: + clk.sPCLKInput.ucMiscInfo |= PIXEL_CLOCK_V6_MISC_HDMI_36BPP_V6; + break; + case TRANSMITTER_COLOR_DEPTH_48: + clk.sPCLKInput.ucMiscInfo |= PIXEL_CLOCK_V6_MISC_HDMI_48BPP; + break; + default: + break; + }
if (EXEC_BIOS_CMD_TABLE(SetPixelClock, clk)) result = BP_RESULT_OK; diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c index fb733f573715..466f8f5803c9 100644 --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c @@ -871,6 +871,20 @@ static bool dce110_program_pix_clk( bp_pc_params.flags.SET_EXTERNAL_REF_DIV_SRC = pll_settings->use_external_clk;
+ switch (pix_clk_params->color_depth) { + case COLOR_DEPTH_101010: + bp_pc_params.color_depth = TRANSMITTER_COLOR_DEPTH_30; + break; + case COLOR_DEPTH_121212: + bp_pc_params.color_depth = TRANSMITTER_COLOR_DEPTH_36; + break; + case COLOR_DEPTH_161616: + bp_pc_params.color_depth = TRANSMITTER_COLOR_DEPTH_48; + break; + default: + break; + } + if (clk_src->bios->funcs->set_pixel_clock( clk_src->bios, &bp_pc_params) != BP_RESULT_OK) return false; diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_stream_encoder.c b/drivers/gpu/drm/amd/display/dc/dce/dce_stream_encoder.c index ada57f745fd7..19e380e0a330 100644 --- a/drivers/gpu/drm/amd/display/dc/dce/dce_stream_encoder.c +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_stream_encoder.c @@ -564,6 +564,7 @@ static void dce110_stream_encoder_hdmi_set_stream_attribute( cntl.enable_dp_audio = enable_audio; cntl.pixel_clock = actual_pix_clk_khz; cntl.lanes_number = LANE_COUNT_FOUR; + cntl.color_depth = crtc_timing->display_color_depth;
if (enc110->base.bp->funcs->encoder_control( enc110->base.bp, &cntl) != BP_RESULT_OK)
On Thu, Jan 21, 2021 at 1:17 AM Mario Kleiner mario.kleiner.de@gmail.com wrote:
These make sense. I've applied the series. I'll let the display guys gauge the other points in your cover letter.
Alex
On 2021-01-25 12:57 p.m., Alex Deucher wrote:
I don't have any concerns with this patch.
Even though it's already applied feel free to have my:
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
Regards, Nicholas Kazlauskas
Thanks Alex and Nicholas! Brings quite a bit of extra shiny to those older asics :)
Nicholas, any thoughts on my cover-letter wrt. why a similar patch (that I wrote and tested to no good or bad effect) not seem to be needed on DCN, and probably not DCE-11.2+ either? Is what is left in DC for those asic's just dead code? My Atombios disassembly sort of pointed into that direction, but reading disassembly is not easy on the brain, and my brain was getting quite mushy towards the end of digging through all the code. So some official statement would add peace of mind on my side. Is there a certain DCE version at which your team starts validating output precision / HDR etc. on hw?
Thanks, -mario
On Mon, Jan 25, 2021 at 8:16 PM Kazlauskas, Nicholas < nicholas.kazlauskas@amd.com> wrote:
Ping. Any bit of info appreciated wrt. the DCE-11.2+ situation. -mario
On Mon, Jan 25, 2021 at 8:24 PM Mario Kleiner mario.kleiner.de@gmail.com wrote:
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Kazlauskas, Nicholas -
Mario Kleiner