The bpg_offset array contains negative BPG offsets which fill the full 8
bits of a char thanks to two's complement: this however results in those
bits bleeding into the next field when the value is packed into DSC PPS
by the drm_dsc_helper function, which only expects range_bpg_offset to
contain 6-bit wide values. As a consequence random slices appear
corrupted on-screen (tested on a Sony Tama Akatsuki device with sdm845).
Use AND operators to limit these two's complement values to 6 bits,
similar to the AMD and i915 drivers.
Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
Signed-off-by: Marijn Suijten <[email protected]>
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 46032c576a59..c5c2d70ac27d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1804,7 +1804,11 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
dsc->rc_range_params[i].range_min_qp = min_qp[i];
dsc->rc_range_params[i].range_max_qp = max_qp[i];
- dsc->rc_range_params[i].range_bpg_offset = bpg_offset[i];
+ /*
+ * Range BPG Offset contains two's-complement signed values that fill
+ * 8 bits, yet the registers and DCS PPS field are only 6 bits wide.
+ */
+ dsc->rc_range_params[i].range_bpg_offset = bpg_offset[i] & DSC_RANGE_BPG_OFFSET_MASK;
}
dsc->initial_offset = 6144; /* Not bpp 12 */
--
2.38.0
On 09/10/2022 21:53, Marijn Suijten wrote:
> The bpg_offset array contains negative BPG offsets which fill the full 8
> bits of a char thanks to two's complement: this however results in those
> bits bleeding into the next field when the value is packed into DSC PPS
> by the drm_dsc_helper function, which only expects range_bpg_offset to
> contain 6-bit wide values. As a consequence random slices appear
> corrupted on-screen (tested on a Sony Tama Akatsuki device with sdm845).
>
> Use AND operators to limit these two's complement values to 6 bits,
> similar to the AMD and i915 drivers.
>
> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> Signed-off-by: Marijn Suijten <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
Side note: the DSC params tables are more or less common between amd,
i916 and msm drivers. It might be worth moving them to the DSC helpers
from the individual drivers. This would mean such masks handling can go
into the helper too.
--
With best wishes
Dmitry
On 2022-10-09 22:14:16, Dmitry Baryshkov wrote:
> On 09/10/2022 21:53, Marijn Suijten wrote:
> > The bpg_offset array contains negative BPG offsets which fill the full 8
> > bits of a char thanks to two's complement: this however results in those
> > bits bleeding into the next field when the value is packed into DSC PPS
> > by the drm_dsc_helper function, which only expects range_bpg_offset to
> > contain 6-bit wide values. As a consequence random slices appear
> > corrupted on-screen (tested on a Sony Tama Akatsuki device with sdm845).
> >
> > Use AND operators to limit these two's complement values to 6 bits,
> > similar to the AMD and i915 drivers.
> >
> > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> > Signed-off-by: Marijn Suijten <[email protected]>
>
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
>
> Side note: the DSC params tables are more or less common between amd,
> i916 and msm drivers. It might be worth moving them to the DSC helpers
> from the individual drivers. This would mean such masks handling can go
> into the helper too.
I'll queue this up in my list and perhaps tackle it in the next round of
DSC fixes, assuming things don't get too big.
If there are no more reviews I'll respin v4 with your review picked up
and patch 7/10 reworked or reordered to have access to the msm_host
pointer added in 8/10 (see kernel test robot mail).
- Marijn
On 10/9/2022 11:53 AM, Marijn Suijten wrote:
> The bpg_offset array contains negative BPG offsets which fill the full 8
> bits of a char thanks to two's complement: this however results in those
> bits bleeding into the next field when the value is packed into DSC PPS
> by the drm_dsc_helper function, which only expects range_bpg_offset to
> contain 6-bit wide values. As a consequence random slices appear
> corrupted on-screen (tested on a Sony Tama Akatsuki device with sdm845).
>
> Use AND operators to limit these two's complement values to 6 bits,
> similar to the AMD and i915 drivers.
>
> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> Signed-off-by: Marijn Suijten <[email protected]>
Reviewed-by: Abhinav Kumar <[email protected]>
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 46032c576a59..c5c2d70ac27d 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1804,7 +1804,11 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
> for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> dsc->rc_range_params[i].range_min_qp = min_qp[i];
> dsc->rc_range_params[i].range_max_qp = max_qp[i];
> - dsc->rc_range_params[i].range_bpg_offset = bpg_offset[i];
> + /*
> + * Range BPG Offset contains two's-complement signed values that fill
> + * 8 bits, yet the registers and DCS PPS field are only 6 bits wide.
> + */
> + dsc->rc_range_params[i].range_bpg_offset = bpg_offset[i] & DSC_RANGE_BPG_OFFSET_MASK;
> }
>
> dsc->initial_offset = 6144; /* Not bpp 12 */