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/[email protected]/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/[email protected]/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/[email protected]/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
According to the `/* bpc 8 */` comment below only values for a
bits_per_component of 8 are currently hardcoded in place. This is
further confirmed by downstream sources [1] containing different
constants for other BPC values (and different initial_offset too,
with an extra dependency on bits_per_pixel). Prevent future mishaps by
explicitly disallowing any other bits_per_component value until the
right parameters are put in place and tested.
[1]: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde_dsc_helper.c#L110-139
Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Marijn Suijten <[email protected]>
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 8da27c740c1c..4bd8301d2049 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1783,6 +1783,11 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
return -EINVAL;
}
+ if (dsc->bits_per_component != 8) {
+ DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n");
+ return -EOPNOTSUPP;
+ }
+
dsc->rc_model_size = 8192;
dsc->first_line_bpg_offset = 12;
dsc->rc_edge_factor = 6;
--
2.38.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/[email protected]/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/[email protected]/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/[email protected]/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
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/[email protected]/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/[email protected]/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/[email protected]/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@mail.gmail.com/
- Marijn
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/[email protected]/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/[email protected]/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/[email protected]/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@mail.gmail.com/
>
> - Marijn
Alright, in that case we will take the whole series for -next and not in
-fixes.