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

Message

Marijn Suijten Oct. 26, 2022, 6:28 p.m. UTC

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/[e-mail]/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/[e-mail]/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/[e-mail]/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

Comments

Abhinav Kumar Oct. 28, 2022, 6:33 p.m. UTC | #1

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/[e-mail]/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/[e-mail]/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/[e-mail]/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.

Thanks

Abhinav

Marijn Suijten Oct. 28, 2022, 8:09 p.m. UTC | #2

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/[e-mail]/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/[e-mail]/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/[e-mail]/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=[e-mail]/

- Marijn

Abhinav Kumar Oct. 28, 2022, 10:06 p.m. UTC | #3

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/[e-mail]/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/[e-mail]/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/[e-mail]/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=[e-mail]/
> 
> - Marijn

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