Re: [PATCH v4 00/10] drm/msm: Fix math issues in MSM DSC implementation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 10/28/2022 1:09 PM, Marijn Suijten wrote:
Hi Abhinav,

On 2022-10-28 11:33:21, Abhinav Kumar wrote:
Hi Marijn

On 10/26/2022 11:28 AM, Marijn Suijten wrote:
Various removals of complex yet unnecessary math, fixing all uses of
drm_dsc_config::bits_per_pixel to deal with the fact that this field
includes four fractional bits, and finally making sure that
range_bpg_offset contains values 6-bits wide to prevent overflows in
drm_dsc_pps_payload_pack().

Altogether this series is responsible for solving _all_ Display Stream
Compression issues and artifacts on the Sony Tama (sdm845) Akatsuki
smartphone (2880x1440p).

Changes since v3:
- Swap patch 7 and 8 to make sure msm_host is available inside
    dsi_populate_dsc_params();
- Reword patch 6 (Migrate to drm_dsc_compute_rc_parameters()) to more
    clearly explain why the FIXME wasn't solved initially, but why it can
    (and should!) be resolved now.

v3: https://lore.kernel.org/linux-arm-msm/20221009184824.457416-1-marijn.suijten@xxxxxxxxxxxxxx/T/#u

Changes since v2:
- Generalize mux_word_size setting depending on bits_per_component;
- Migrate DSI's DSC calculations to drm_dsc_compute_rc_parameters(),
    implicitly addressing existing math issues;
- Disallow any bits_per_component other than 8, until hardcoded values
    are updated and tested to support such cases.

v2: https://lore.kernel.org/linux-arm-msm/20221005181657.784375-1-marijn.suijten@xxxxxxxxxxxxxx/T/#u

Changes since v1:

- Propagate r-b's, except (obviously) in patches that were (heavily)
    modified;
- Remove accidental debug code in dsi_cmd_dma_add;
- Move Range BPG Offset masking out of DCS PPS packing, back into the
    DSI driver when it is assigned to drm_dsc_config (this series is now
    strictly focusing on drm/msm again);
- Replace modulo-check resulting in conditional increment with
    DIV_ROUND_UP;
- Remove repeated calculation of slice_chunk_size;
- Use u16 instead of int when handling bits_per_pixel;
- Use DRM_DEV_ERROR instead of pr_err in DSI code;
- Also remove redundant target_bpp_x16 variable.

v1: https://lore.kernel.org/linux-arm-msm/20221001190807.358691-1-marijn.suijten@xxxxxxxxxxxxxx/T/#u

Marijn Suijten (10):
    drm/msm/dsi: Remove useless math in DSC calculations
    drm/msm/dsi: Remove repeated calculation of slice_per_intf
    drm/msm/dsi: Use DIV_ROUND_UP instead of conditional increment on
      modulo
    drm/msm/dsi: Reuse earlier computed dsc->slice_chunk_size
    drm/msm/dsi: Appropriately set dsc->mux_word_size based on bpc
    drm/msm/dsi: Migrate to drm_dsc_compute_rc_parameters()
    drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits
    drm/msm/dsi: Disallow 8 BPC DSC configuration for alternative BPC
      values
    drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional
      bits
    drm/msm/dsi: Prevent signed BPG offsets from bleeding into adjacent
      bits

   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c |  11 +-
   drivers/gpu/drm/msm/dsi/dsi_host.c         | 121 ++++++---------------
   2 files changed, 37 insertions(+), 95 deletions(-)

--
2.38.1


To keep the -fixes cycle to have only critical fixes (others are
important too but are cleanups), I was thinking of absorbing patches
7,8,9 and 10 alone in the -fixes cycle and for patches 1-6, will go
through the 6.2 push.

Let me know if there are any concerns if we just take patches 7,8,9 and
10 separately.

Unfortunately that isn't going to cut it.  For starters patch 8 is only
introducing additional validation but as long as no panel drivers set
bpc != 8, this doesn't change anything: it is not a critical fix.

Then, more importantly, as discussed in v2 reviews it was preferred to
_not_ fix the broken code in dsi_populate_dsc_params() but migrate to
drm_dsc_compute_rc_parameters() directly [1].  As such patch 6 (which
performs the migration) is definitely a requirement for the fixes to be
complete.  Then again this patch looks weird when 5 is not applied,
since both refactor how dsc->mux_word_size is assigned.  At the same
time it cannot be cleanly applied without patch 1 (Remove useless math
in DSC calculations) nor patch 3 (Use DIV_ROUND_UP instead of
conditional increment on modulo), but I just realized that patch 3 is
now also useless as the code is being removed altogether while migrating
to drm_dsc_compute_rc_parameters().

Same for patch 4 (Reuse earlier computed dsc->slice_chunk_size): while
it may not seem obvious at first, the original code uses bits_per_pixel
without considering the fractional bits, again resulting invalid values.
Perhaps this should have been mentioned in the patch description, but I
did not want to create an even larger chain of references pointing back
and forth to future patches fixing the exact same bug.  Unfortunately
this patch doesn't apply cleanly without patch 2 (Remove repeated
calculation of slice_per_intf) either.

All in all, applying this series piecemeal requires careful
consideration which of the patches are actually fixing issues, and is
terribly tricky considering code cleanups touching the same code and
sitting right before the fixes (done intentionally to not distract diffs
in bugfixes being surrounded by odd looking code).

[1]: https://lore.kernel.org/linux-arm-msm/CAA8EJpr=0w0KReqNW2jP8DzvXLgo_o6bKmwMOed2sXb6d8HKhg@xxxxxxxxxxxxxx/

- Marijn

Alright, in that case we will take the whole series for -next and not in -fixes.



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux