Skip to content
Snippets Groups Projects

Screen is corrupted because of incorrect calculation of first_line_bpg_offset for DSC 1.1 (2nd report)

    • Closed Issue created by Gareth Yu @gyu3

    Continue #9102 (closed). The syndrome does still happen. The change below can solve it. The reason is because MIPI DSI 1.2 supports DSC 1.1, https://vesa.org/vesa-display-compression-codecs/. In the original condition, the calculation is wrong. Please check it.

    diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
index 6e7151346382..9f1c339986d4 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -320,7 +320,7 @@ int intel_dsc_compute_params(struct intel_crtc_state *pipe_config)
         * upto uncompressed bpp-1, hence add calculations for all the rc
         * parameters
         */
-       if (DISPLAY_VER(dev_priv) >= 13) {
+       if ((DISPLAY_VER(dev_priv) >= 13) && (vdsc_cfg->dsc_version_minor == 2)) {
                calculate_rc_params(vdsc_cfg);
        } else {
                if ((compressed_bpp == 8 ||
    Edited
    To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

    Child items 0

    • No child items are currently open.

    Activity

    • Gareth Yu changed the description

      changed the description

    • Gareth Yu changed the description

      changed the description

    • Ramadevi Gandi added feature: display/DSC label

      added feature: display/DSC label

    • Gareth Yu
      Gareth Yu @gyu3 ·
      Author

      Besides, DSC 1.2a is supported since Gen 14 because the display IP of Gen 13 is as well as Gen 12. Please check it.

      diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
index 6e7151346382..9f1c339986d4 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -320,7 +320,7 @@ int intel_dsc_compute_params(struct intel_crtc_state *pipe_config)
         * upto uncompressed bpp-1, hence add calculations for all the rc
         * parameters
         */
-       if (DISPLAY_VER(dev_priv) >= 13) {
+       if ((DISPLAY_VER(dev_priv) >= 14) && (vdsc_cfg->dsc_version_minor == 2)) {
                calculate_rc_params(vdsc_cfg);
        } else {
                if ((compressed_bpp == 8 ||
    • Gareth Yu
      Gareth Yu @gyu3 ·
      Author

      The patch is also in https://patchwork.freedesktop.org/series/145136/#rev1.

    • surajk8
      surajk8 @surajk8 ·
      Developer

      Please provide separate dmesg logs for the failing condition and the one in which it works well the patch provided causes a regression on machines with DSC 1.1 which were passing previously leads me to believe this is not the correct solution. Will have to investigate this further.

      @gyu3 @tsengwil

      Edited by surajk8
      • William Tseng
        William Tseng @tsengwil ·
        Reporter

        This is about the commit "db514cac" for clarifying the rc parameters calculation for DSC 1.1.

        If slice_height is 40, the rc parameter first_line_bpg_offset derived from the commit will be 15 for 8bpp/8bpc, which is not equal to the value shown in Table E-5, DSC 1.1. So, the commit message "...The calculations used here are from VESA C model for DSC 1.1" does not seem quite right.

        Shall bpp and bpc be considered in the function calculate_rc_params() in order to be compatible with DSC 1.1?

        Edited by William Tseng
      • surajk8
        surajk8 @surajk8 ·
        Developer

        The values calculated by Cmodel wont be an exact match but should be close by to the value of table e-5

      • William Tseng
        William Tseng @tsengwil ·
        Reporter

        Maybe the different value has no impact on eDP but it does cause this issue found on a MIPI DSI panel. Can we make further changes and adapt to DSI panel, for example, adapting the rc parameters in gen11_dsi_dsc_compute_config()?

      • surajk8
        surajk8 @surajk8 ·
        Developer

        I am guessing there maybe some restrictions for DSI panel we are missing the problem is that we need to find which is the exact parameter that ends up causing this issue, next would be the finding the documentation thats asks us to apply these restrictions. Currently creating a patch for dsi to log the dsc params which we do for eDp and DP but for some reason didn't see it in the dmesg logs which were provided makes me believe the code for that does not exist yet

      • Gareth Yu
        Gareth Yu @gyu3 ·
        Author

        Sure! Once your patch is ready, we'll provide the new log here.

      • surajk8
        surajk8 @surajk8 ·
        Developer

        @gyu3 are you using the latest upstream kernel to test this since i see that the prints should have shown up with the rest of the crtc_state_dump but dont make me think an older kernel may have been used.

        Can you check if you have the following commit 1de99ff7

        Edited by surajk8
      • Gareth Yu
        Gareth Yu @gyu3 ·
        Author

        bad-0225.txt

        good-0225.txt

        DRM log was moved to /sys/kernel/tracing/instances/drm/trace. The attached 2 files are for good case and bad case.

      • surajk8
        surajk8 @surajk8 ·
        Developer

        Saw quite a lot of difference but also anomalies in the print between the working case and non working case like dsc minor version being 0.0 or the rc buf threshold and other calculations being totally different from the working case

        WORKING CASE

        <...>-1 [002] ..... 1.217539: drm_trace_printf: (null) dsc-dss: compressed-bpp:8.0000, slice-count: 0, split: no

        <...>-1 [002] ..... 1.218097: drm_trace_printf: (null) dsc-cfg: version: 0.0, picture: w=1600, h=2560, slice: count=0, w=1600, h=40, size=1600

        <...>-1 [002] ..... 1.218652: drm_trace_printf: (null) dsc-cfg: mode: block-pred=yes, vbr=no, rgb=yes, simple-422=no, native-422=no, native-420=no <...>-1 [002] ..... 1.219207: drm_trace_printf: (null) dsc-cfg: color-depth: uncompressed-bpc=8, compressed-bpp=8.0000 line-buf-bpp=9

        <...>-1 [002] ..... 1.219760: drm_trace_printf: (null) dsc-cfg: rc-model: size=8192, bits=0, mux-word-size: 0, initial-delays: xmit=512, dec=1057

        <...>-1 [002] ..... 1.220313: drm_trace_printf: (null) dsc-cfg: offsets: initial=6144, final=4320, slice-bpg=218

        <...>-1 [002] ..... 1.220868: drm_trace_printf: (null) dsc-cfg: line-bpg-offsets: first=12, non-first=631, second=0, non-second=0, second-adj=0

        <...>-1 [002] ..... 1.221422: drm_trace_printf: (null) dsc-cfg: rc-tgt-offsets: low=0, high=0, rc-edge-factor: 0, rc-quant-incr-limits: [0]=11, [1]=11

        <...>-1 [002] ..... 1.221978: drm_trace_printf: (null) dsc-cfg: initial-scale: 32, scale-intervals: increment=1488, decrement=22

        <...>-1 [002] ..... 1.222533: drm_trace_printf: (null) dsc-cfg: flatness: min-qp=3, max-qp=12

        <...>-1 [002] ..... 1.223086: drm_trace_printf: (null) dsc-cfg: rc-level: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14

        <...>-1 [002] ..... 1.223642: drm_trace_printf: (null) dsc-cfg: rc-buf-thresh: 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0

        <...>-1 [002] ..... 1.224199: drm_trace_printf: (null) dsc-cfg: rc-min-qp: 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0

        <...>-1 [002] ..... 1.224753: drm_trace_printf: (null) dsc-cfg: rc-max-qp: 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0

        <...>-1 [002] ..... 1.225309: drm_trace_printf: (null) dsc-cfg: rc-bpg-offset: 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0

        <...>-1 [002] ..... 1.225863: drm_trace_printf: (null) [CRTC:134:pipe B] enable: no [setup_hw_state]

        <...>-1 [002] ..... 1.226417: drm_trace_printf: (null) [CRTC:186:pipe C] enable: no [setup_hw_state]

        <...>-1 [002] ..... 1.226437: drm_trace_printf: 0000:00:02.0 [drm:skl_wm_get_hw_state_and_sanitize] [CRTC:82:pipe A] dbuf slices 0x3, ddb (0 - 2048), active pipes 0x1, mbus joined: no

        <...>-1 [002] ..... 1.226442: drm_trace_printf: 0000:00:02.0 [drm:skl_wm_get_hw_state_and_sanitize] [CRTC:134:pipe B] dbuf slices 0x0, ddb (0 - 0), active pipes 0x1, mbus joined: no

        <...>-1 [002] ..... 1.226445: drm_trace_printf: 0000:00:02.0 [drm:skl_wm_get_hw_state_and_sanitize] [CRTC:186:pipe C] dbuf slices 0x0, ddb (0 - 0), active pipes 0x1, mbus joined: no

        <...>-1 [002] ..... 1.226464: drm_trace_printf: 0000:00:02.0 [drm:skl_get_initial_plane_config] pipe A/plane 1A with fb: size=1600x2560@32, offset=0, pitch 6400, size 0xfa0000

        <...>-1 [002] ..... 1.226473: drm_trace_printf: 0000:00:02.0 [drm:_i915_gem_object_stolen_init] creating preallocated stolen object: stolen_offset=0x0000000000000000, size=0x0000000000fa0000

        <...>-1 [002] ..... 1.226908: drm_trace_printf: 0000:00:02.0 [drm:intel_initial_plane_config] Initial plane fb bound to 0x0 in the ggtt (original 0x0)

        NON WORKING CASE

        <...>-1 [000] ..... 1.322533: drm_trace_printf: (null) dsc-dss: compressed-bpp:8.0000, slice-count: 1, split: no

        <...>-1 [000] ..... 1.323096: drm_trace_printf: (null) dsc-cfg: version: 1.1, picture: w=1600, h=2560, slice: count=0, w=1600, h=40, size=1600

        <...>-1 [000] ..... 1.323661: drm_trace_printf: (null) dsc-cfg: mode: block-pred=yes, vbr=no, rgb=yes, simple-422=no, native-422=no, native-420=no <...>-1 [000] ..... 1.324233: drm_trace_printf: (null) dsc-cfg: color-depth: uncompressed-bpc=8, compressed-bpp=8.0000 line-buf-bpp=9

        <...>-1 [000] ..... 1.324795: drm_trace_printf: (null) dsc-cfg: rc-model: size=8192, bits=13624, mux-word-size: 48, initial-delays: xmit=512, dec=1191

        <...>-1 [000] ..... 1.325356: drm_trace_printf: (null) dsc-cfg: offsets: initial=6144, final=4320, slice-bpg=218

        <...>-1 [000] ..... 1.325918: drm_trace_printf: (null) dsc-cfg: line-bpg-offsets: first=14, non-first=736, second=0, non-second=0, second-adj=0

        <...>-1 [000] ..... 1.326480: drm_trace_printf: (null) dsc-cfg: rc-tgt-offsets: low=0, high=0, rc-edge-factor: 0, rc-quant-incr-limits: [0]=11, [1]=11

        <...>-1 [000] ..... 1.327042: drm_trace_printf: (null) dsc-cfg: initial-scale: 32, scale-intervals: increment=1324, decrement=22

        <...>-1 [000] ..... 1.327603: drm_trace_printf: (null) dsc-cfg: flatness: min-qp=3, max-qp=12 <...>-1 [000] ..... 1.328165: drm_trace_printf: (null) dsc-cfg: rc-level: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14

        <...>-1 [000] ..... 1.328725: drm_trace_printf: (null) dsc-cfg: rc-buf-thresh: 14, 28, 42, 56, 70, 84, 98,105,112,119,121,123,125,126

        <...>-1 [000] ..... 1.329298: drm_trace_printf: (null) dsc-cfg: rc-min-qp: 0, 0, 2, 2, 4, 4, 4, 4, 4, 5, 5, 6, 6, 9, 13

        <...>-1 [000] ..... 1.329860: drm_trace_printf: (null) dsc-cfg: rc-max-qp: 4, 5, 7, 7, 8, 8, 8, 9, 10, 11, 11, 12, 12, 13, 14

        <...>-1 [000] ..... 1.330424: drm_trace_printf: (null) dsc-cfg: rc-bpg-offset: 2, 0, 0, 62, 60, 58, 56, 56, 56, 54, 54, 54, 52, 52, 52

        <...>-1 [000] ..... 1.330987: drm_trace_printf: (null) [PLANE:32:plane 1A] fb: [FB:193] 1600x2560 format = XR24 little-endian (0x34325258) modifier = 0x0, visible: yes

        Will need to cross check the model for DSC 1.1 and also any special DSI mentions

        FYI @aknautiyal

        Edited by surajk8
      • Gareth Yu
        Gareth Yu @gyu3 ·
        Author

        In the good case as well as the bad case, 2 version information are shown. The first one shows version 0.0, the second one shows 1.1.

        good case: 1.218097: drm_trace_printf: (null) dsc-cfg: version: 0.0, picture: w=1600, h=2560, slice: count=0, w=1600, h=40, size=1600

        1.319427: drm_trace_printf: (null) dsc-cfg: version: 1.1, picture: w=1600, h=2560, slice: count=0, w=1600, h=40, size=1600

        bad case: 1.219082: drm_trace_printf: (null) dsc-cfg: version: 0.0, picture: w=1600, h=2560, slice: count=0, w=1600, h=40, size=1600

        1.323096: drm_trace_printf: (null) dsc-cfg: version: 1.1, picture: w=1600, h=2560, slice: count=0, w=1600, h=40, size=1600

      • Please register or sign in to reply
    • William Tseng
      William Tseng @tsengwil ·
      Reporter

      Maybe we could ignore the part of dsc-cfg, in which version is 0.0 and focus on the other part with version 1.1. It is observed the version number is changed to 1.1 after the log message.

      "drm_trace_printf: 0000:00:02.0 [drm:gen11_dsi_initial_fastset_check] Forcing full modeset due to DSC being enabled".

      As the following, if comparing the two dsc-cfg configurations for the working and not working cases correspondingly, all the differences will be the values for "bits" and "dec" for rc-model, "first" and "non-first" for line-bpg-offsets, "increment" for initial-scale, rc-min-qp with the levels 2, 3, 4, 5, 6, 7, 8, 9, 11, 12, and 13, rc-max-qp with the levels 0, 3, 5, 6, 11, 12, 14, and rc-bpg-offset with level 11.

      1. dsc-cfg for working case

            <...>-1       [002] .....     1.319427: drm_trace_printf: (null) dsc-cfg: version: 1.1, picture: w=1600, h=2560, slice: count=0, w=1600, h=40, size=1600
    <...>-1       [002] .....     1.319999: drm_trace_printf: (null) dsc-cfg: mode: block-pred=yes, vbr=no, rgb=yes, simple-422=no, native-422=no, native-420=no
    <...>-1       [002] .....     1.320561: drm_trace_printf: (null) dsc-cfg: color-depth: uncompressed-bpc=8, compressed-bpp=8.0000 line-buf-bpp=9
    <...>-1       [002] .....     1.321122: drm_trace_printf: (null) dsc-cfg: rc-model: size=8192, bits=12552, mux-word-size: 48, initial-delays: xmit=512, dec=1057
    <...>-1       [002] .....     1.321684: drm_trace_printf: (null) dsc-cfg: offsets: initial=6144, final=4320, slice-bpg=218
    <...>-1       [002] .....     1.322246: drm_trace_printf: (null) dsc-cfg: line-bpg-offsets: first=12, non-first=631, second=0, non-second=0, second-adj=0
    <...>-1       [002] .....     1.322808: drm_trace_printf: (null) dsc-cfg: rc-tgt-offsets: low=0, high=0, rc-edge-factor: 0, rc-quant-incr-limits: [0]=11, [1]=11
    <...>-1       [002] .....     1.323371: drm_trace_printf: (null) dsc-cfg: initial-scale: 32, scale-intervals: increment=1488, decrement=22
    <...>-1       [002] .....     1.323934: drm_trace_printf: (null) dsc-cfg: flatness: min-qp=3, max-qp=12
    <...>-1       [002] .....     1.324496: drm_trace_printf: (null) dsc-cfg: rc-level:         0,  1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 11, 12, 13, 14
    <...>-1       [002] .....     1.325057: drm_trace_printf: (null) dsc-cfg: rc-buf-thresh:   14, 28, 42, 56, 70, 84, 98,105,112,119,121,123,125,126
    <...>-1       [002] .....     1.325620: drm_trace_printf: (null) dsc-cfg: rc-min-qp:        0,  0,  1,  1,  3,  3,  3,  3,  3,  3,  5,  5,  5,  7, 13
    <...>-1       [002] .....     1.326185: drm_trace_printf: (null) dsc-cfg: rc-max-qp:        4,  4,  5,  6,  7,  7,  7,  8,  9, 10, 11, 12, 13, 13, 15
    <...>-1       [002] .....     1.326748: drm_trace_printf: (null) dsc-cfg: rc-bpg-offset:    2,  0,  0, 62, 60, 58, 56, 56, 56, 54, 54, 52, 52, 52, 52

      2. dsc-cfg for not working case

            <...>-1       [000] .....     1.323096: drm_trace_printf: (null) dsc-cfg: version: 1.1, picture: w=1600, h=2560, slice: count=0, w=1600, h=40, size=1600
    <...>-1       [000] .....     1.323661: drm_trace_printf: (null) dsc-cfg: mode: block-pred=yes, vbr=no, rgb=yes, simple-422=no, native-422=no, native-420=no
    <...>-1       [000] .....     1.324233: drm_trace_printf: (null) dsc-cfg: color-depth: uncompressed-bpc=8, compressed-bpp=8.0000 line-buf-bpp=9
    <...>-1       [000] .....     1.324795: drm_trace_printf: (null) dsc-cfg: rc-model: size=8192, bits=13624, mux-word-size: 48, initial-delays: xmit=512, dec=1191
    <...>-1       [000] .....     1.325356: drm_trace_printf: (null) dsc-cfg: offsets: initial=6144, final=4320, slice-bpg=218
    <...>-1       [000] .....     1.325918: drm_trace_printf: (null) dsc-cfg: line-bpg-offsets: first=14, non-first=736, second=0, non-second=0, second-adj=0
    <...>-1       [000] .....     1.326480: drm_trace_printf: (null) dsc-cfg: rc-tgt-offsets: low=0, high=0, rc-edge-factor: 0, rc-quant-incr-limits: [0]=11, [1]=11
    <...>-1       [000] .....     1.327042: drm_trace_printf: (null) dsc-cfg: initial-scale: 32, scale-intervals: increment=1324, decrement=22
    <...>-1       [000] .....     1.327603: drm_trace_printf: (null) dsc-cfg: flatness: min-qp=3, max-qp=12
    <...>-1       [000] .....     1.328165: drm_trace_printf: (null) dsc-cfg: rc-level:         0,  1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 11, 12, 13, 14
    <...>-1       [000] .....     1.328725: drm_trace_printf: (null) dsc-cfg: rc-buf-thresh:   14, 28, 42, 56, 70, 84, 98,105,112,119,121,123,125,126
    <...>-1       [000] .....     1.329298: drm_trace_printf: (null) dsc-cfg: rc-min-qp:        0,  0,  2,  2,  4,  4,  4,  4,  4,  5,  5,  6,  6,  9, 13
    <...>-1       [000] .....     1.329860: drm_trace_printf: (null) dsc-cfg: rc-max-qp:        4,  5,  7,  7,  8,  8,  8,  9, 10, 11, 11, 12, 12, 13, 14
    <...>-1       [000] .....     1.330424: drm_trace_printf: (null) dsc-cfg: rc-bpg-offset:    2,  0,  0, 62, 60, 58, 56, 56, 56, 54, 54, 54, 52, 52, 52

      According to the log messages, the working case has the values which are equal to the ones (At 8bpp/8bpc) in Table E-5, DSC 1.1, in terms of first_line_bpg_offset and MinQp and MaxQp of rc_range_parameters[0:14].

      @surajk8 could you take a look about the finding and share your thoughts?

      Edited by William Tseng
    • William Tseng
      William Tseng @tsengwil ·
      Reporter

      @surajk8 and @aknautiyal Have you checked the model for DSC 1.1 and found the differences between the working case and not working case?

      • surajk8
        surajk8 @surajk8 ·
        Developer

        This basically boils down to individual DSI panel vendors hardcoding values in the VBT for DSC 1.1 because of which they are completely ignoring the params sent by Source. The driver is in accordance to VESA DSC Model. More info in the below HSD

        16011641294

        I have sent a patch that fixes this and hopefully does not cause a regression elsewhere

        https://patchwork.freedesktop.org/series/145561/

      • William Tseng
        William Tseng @tsengwil ·
        Reporter

        Thanks, Suraj. To clarify, so the dsc-cfg parameters for the failing case are also conforming with the specification DSC 1.1?

      • surajk8
        surajk8 @surajk8 ·
        Developer

        Yes they do the parameters may not be the same as shown in table but are not far off to cause such corruption.

      • William Tseng
        William Tseng @tsengwil ·
        Reporter

        Noted, thank you.
        Could you share the specification where the calculation is defined and can derive the parameters for the failing case? Maybe DSI panel vendor should be aware of it.

      • Please register or sign in to reply
      • Gareth Yu
        Gareth Yu @gyu3 ·
        Author

        @surajk8 I have no questions about calculate_rc_params but don't know why to have "DISPLAY_VER(dev_priv) >= 13". According your comments, it only need to check the dsc version of sink to solve this issue, doesn't it?

        Quote from https://lore.kernel.org/all/MW4PR11MB6761EF8A44F8A1F488FEF93DE3CC2@MW4PR11MB6761.namprd11.prod.outlook.com/:

> > -	if (DISPLAY_VER(dev_priv) >= 13) {
> > +	if (DISPLAY_VER(dev_priv) >= 13 &&
> 
> Please change to "if (DISPLAY_VER(dev_priv) >= 14 &&" because MTL 
> begins to support DSC 1.2a according bspec 49259.

As mentioned in the gitlab issue calculate_rc_params works for both DSC 1.1 and 1.2a because of which
DISPLAY_VER>=13 works check commit message db514cac.
The reason we saw the issue in DSI panels was individual DSI panel vendors hardcoding DSC params and not the driver Miscalculating them which would also explain why it worked for all other panels in platforms >=13.
So won't be changing the display_ver() check.

Regards,
Suraj Kandpal
        Edited by Gareth Yu
      • surajk8
        surajk8 @surajk8 ·
        Developer

        1. DISPLAY_VER() check already existed and wasn't something that was added to solve this issue.

        2. This check was previously added because that is when our platform/source side got the capability to handle values that are calculated rather than using the static config tables. Also the capability to handle fractional bits is when display_ver() >= 13

        @aknautiyal

      • Gareth Yu
        Gareth Yu @gyu3 ·
        Author

        @demarchi Please provide your comments if DISPLAY_VER() is still needed or not because you created the initial patch.

      • surajk8
        surajk8 @surajk8 ·
        Developer

        FYI the comment right above where we call calculate_rc_params also states the reason why DISPLAY_VER check is needed

        /* * From XE_LPD onwards we supports compression bpps in steps of 1 * upto uncompressed bpp-1, hence add calculations for all the rc * parameters */

      • Please register or sign in to reply
    • Gareth Yu
      Gareth Yu @gyu3 ·
      Author

      Understood.

      Thanks

    • Gareth Yu closed

      closed

    • surajk8 mentioned in commit c63d00e3

      mentioned in commit c63d00e3

    Please register or sign in to reply