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@xxxxxxx> wrote:
On 2021-01-25 12:57 p.m., Alex Deucher wrote:
> On Thu, Jan 21, 2021 at 1:17 AM Mario Kleiner
> <mario.kleiner.de@xxxxxxxxx> wrote:
>>
>> 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@xxxxxxxxx>
>> Cc: Harry Wentland <harry.wentland@xxxxxxx>
>
> These make sense. I've applied the series. I'll let the display guys
> gauge the other points in your cover letter.
>
> Alex
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@xxxxxxx>
Regards,
Nicholas Kazlauskas
>
>
>> ---
>> .../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)
>> --
>> 2.25.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://nam11.safelinks.protection.outlook.com/?url="">
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://nam11.safelinks.protection.outlook.com/?url="">
>
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
