| Message ID | 20221001190807.358691-2-marijn.suijten@somainline.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm: Fix math issues in MSM DSC implementation | expand |
On 1.10.2022 21:08, Marijn Suijten wrote: > Multiplying a value by 2 and adding 1 to it always results in a value > that is uneven, and that 1 gets truncated immediately when performing > integer division by 2 again. There is no "rounding" possible here. > > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data") > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@somainline.org> Konrad > drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 8e4bc586c262..e05bae647431 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -1864,12 +1864,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc) > data = 2048 * (dsc->rc_model_size - dsc->initial_offset + num_extra_mux_bits); > dsc->slice_bpg_offset = DIV_ROUND_UP(data, groups_total); > > - /* bpp * 16 + 0.5 */ > - data = dsc->bits_per_pixel * 16; > - data *= 2; > - data++; > - data /= 2; > - target_bpp_x16 = data; > + target_bpp_x16 = dsc->bits_per_pixel * 16; > > data = (dsc->initial_xmit_delay * target_bpp_x16) / 16; > final_value = dsc->rc_model_size - data + num_extra_mux_bits;
On Sat, Oct 01, 2022 at 09:08:03PM +0200, Marijn Suijten wrote: > Multiplying a value by 2 and adding 1 to it always results in a value > that is uneven, and that 1 gets truncated immediately when performing > integer division by 2 again. There is no "rounding" possible here. > > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data") > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> Reviewed-by: Bjorn Andersson <andersson@kernel.org> > --- > drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 8e4bc586c262..e05bae647431 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -1864,12 +1864,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc) > data = 2048 * (dsc->rc_model_size - dsc->initial_offset + num_extra_mux_bits); > dsc->slice_bpg_offset = DIV_ROUND_UP(data, groups_total); > > - /* bpp * 16 + 0.5 */ > - data = dsc->bits_per_pixel * 16; > - data *= 2; > - data++; > - data /= 2; > - target_bpp_x16 = data; > + target_bpp_x16 = dsc->bits_per_pixel * 16; > > data = (dsc->initial_xmit_delay * target_bpp_x16) / 16; > final_value = dsc->rc_model_size - data + num_extra_mux_bits; > -- > 2.37.3 >
On 10/1/2022 12:08 PM, Marijn Suijten wrote: > Multiplying a value by 2 and adding 1 to it always results in a value > that is uneven, and that 1 gets truncated immediately when performing > integer division by 2 again. There is no "rounding" possible here. > > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data") > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 8e4bc586c262..e05bae647431 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -1864,12 +1864,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc) > data = 2048 * (dsc->rc_model_size - dsc->initial_offset + num_extra_mux_bits); > dsc->slice_bpg_offset = DIV_ROUND_UP(data, groups_total); > > - /* bpp * 16 + 0.5 */ > - data = dsc->bits_per_pixel * 16; > - data *= 2; > - data++; > - data /= 2; > - target_bpp_x16 = data; > + target_bpp_x16 = dsc->bits_per_pixel * 16; > Since this patch is titled, "remove useless math", even the target_bpp_x16 math looks redundant to me, first we do target_bpp_x16 = dsc->bits_per_pixel * 16; then in the next line we do data = (dsc->initial_xmit_delay * target_bpp_x16) / 16; the *16 and /16 will cancel out here. Instead we can just do data = (dsc->initial_xmit_delay * dsc->drm->bits_per_pixel) ? > data = (dsc->initial_xmit_delay * target_bpp_x16) / 16; > final_value = dsc->rc_model_size - data + num_extra_mux_bits;
On 2022-10-04 07:33:49, Abhinav Kumar wrote: > > > On 10/1/2022 12:08 PM, Marijn Suijten wrote: > > Multiplying a value by 2 and adding 1 to it always results in a value > > that is uneven, and that 1 gets truncated immediately when performing > > integer division by 2 again. There is no "rounding" possible here. > > > > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data") > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > --- > > drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > > index 8e4bc586c262..e05bae647431 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > > @@ -1864,12 +1864,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc) > > data = 2048 * (dsc->rc_model_size - dsc->initial_offset + num_extra_mux_bits); > > dsc->slice_bpg_offset = DIV_ROUND_UP(data, groups_total); > > > > - /* bpp * 16 + 0.5 */ > > - data = dsc->bits_per_pixel * 16; > > - data *= 2; > > - data++; > > - data /= 2; > > - target_bpp_x16 = data; > > + target_bpp_x16 = dsc->bits_per_pixel * 16; > > > Since this patch is titled, "remove useless math", even the > target_bpp_x16 math looks redundant to me, > > first we do > > target_bpp_x16 = dsc->bits_per_pixel * 16; > > then in the next line we do > > data = (dsc->initial_xmit_delay * target_bpp_x16) / 16; > > the *16 and /16 will cancel out here. > > Instead we can just do > > data = (dsc->initial_xmit_delay * dsc->drm->bits_per_pixel) ? Thanks, good catch! I might have been so focused on explaining the effect of this patch and uselessness of the proposed `+ 0.5` rounding here that I missed this intermediate variable now becoming redundant as well. Corrected for v2! - Marijn > > data = (dsc->initial_xmit_delay * target_bpp_x16) / 16; > > final_value = dsc->rc_model_size - data + num_extra_mux_bits;
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 8e4bc586c262..e05bae647431 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -1864,12 +1864,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc) data = 2048 * (dsc->rc_model_size - dsc->initial_offset + num_extra_mux_bits); dsc->slice_bpg_offset = DIV_ROUND_UP(data, groups_total); - /* bpp * 16 + 0.5 */ - data = dsc->bits_per_pixel * 16; - data *= 2; - data++; - data /= 2; - target_bpp_x16 = data; + target_bpp_x16 = dsc->bits_per_pixel * 16; data = (dsc->initial_xmit_delay * target_bpp_x16) / 16; final_value = dsc->rc_model_size - data + num_extra_mux_bits;
Multiplying a value by 2 and adding 1 to it always results in a value that is uneven, and that 1 gets truncated immediately when performing integer division by 2 again. There is no "rounding" possible here. Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data") Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> --- drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)