Re: [PATCH 2/2] drm/amd/display: Fix HDMI deep color output for DCE 6-11.

[Mario Kleiner <mario.kleiner.de@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 11999 bytes --]

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:

> 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:
>
>> 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@gmail.com> 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@gmail.com>
>> >> Cc: Harry Wentland <harry.wentland@amd.com>
>> >
>> > 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@amd.com>
>>
>> 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@lists.freedesktop.org
>> >>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C598b2b5e841940b8c27708d8c15aa80d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637471942408643835%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=b9C3Xi%2F2RmNsGyGUN5oBF8U%2BuGyt2w4jUZ2dK8NM4AY%3D&amp;reserved=0
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> >
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C598b2b5e841940b8c27708d8c15aa80d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637471942408643835%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=b9C3Xi%2F2RmNsGyGUN5oBF8U%2BuGyt2w4jUZ2dK8NM4AY%3D&amp;reserved=0
>> >
>>
>>

[-- Attachment #1.2: Type: text/html, Size: 16476 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel