[PATCH 0/5] drm: Fix math issues in MSM DSC implementation

[PATCH 0/5] drm: Fix math issues in MSM DSC implementation

[Marijn Suijten]

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 an approach for dealing with
dsi_host setting negative values in range_bpg_offset, resulting in
overflow inside drm_dsc_pps_payload_pack().

Note that updating the static bpg_offset array to limit the size of
these negative values to 6 bits changes what would be written to the DPU
hardware at register(s) DSC_RANGE_BPG_OFFSET, hence the choice has been
made to cover up for this while packing the value into a smaller field
instead.

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

Marijn Suijten (5):
  drm/msm/dsi: Remove useless math in DSC calculation
  drm/msm/dsi: Remove repeated calculation of slice_per_intf
  drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits
  drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional
    bits
  drm/dsc: Prevent negative BPG offsets from shadowing adjacent
    bitfields

 drivers/gpu/drm/display/drm_dsc_helper.c   |  6 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 11 +-----
 drivers/gpu/drm/msm/dsi/dsi_host.c         | 45 ++++++++++++++--------
 3 files changed, 33 insertions(+), 29 deletions(-)

--
2.37.3

[PATCH 1/5] drm/msm/dsi: Remove useless math in DSC calculation

[Marijn Suijten]

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;
 
 	data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
 	final_value =  dsc->rc_model_size - data + num_extra_mux_bits;
-- 
2.37.3

Re: [PATCH 1/5] drm/msm/dsi: Remove useless math in DSC calculation

[Konrad Dybcio]

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;

Re: [PATCH 1/5] drm/msm/dsi: Remove useless math in DSC calculation

[Bjorn Andersson]

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
> 

Re: [Freedreno] [PATCH 1/5] drm/msm/dsi: Remove useless math in DSC calculation

[Abhinav Kumar]

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;

Re: [Freedreno] [PATCH 1/5] drm/msm/dsi: Remove useless math in DSC calculation

[Marijn Suijten]

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;

[PATCH 2/5] drm/msm/dsi: Remove repeated calculation of slice_per_intf

[Marijn Suijten]

slice_per_intf is already computed for intf_width, which holds the same
value as hdisplay.

Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e05bae647431..cb6f2fa11f58 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -842,7 +842,7 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
 static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mode, u32 hdisplay)
 {
 	struct drm_dsc_config *dsc = msm_host->dsc;
-	u32 reg, intf_width, reg_ctrl, reg_ctrl2;
+	u32 reg, reg_ctrl, reg_ctrl2;
 	u32 slice_per_intf, total_bytes_per_intf;
 	u32 pkt_per_line;
 	u32 bytes_in_slice;
@@ -851,8 +851,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	/* first calculate dsc parameters and then program
 	 * compress mode registers
 	 */
-	intf_width = hdisplay;
-	slice_per_intf = DIV_ROUND_UP(intf_width, dsc->slice_width);
+	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
 
 	/* If slice_per_pkt is greater than slice_per_intf
 	 * then default to 1. This can happen during partial
@@ -861,7 +860,6 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	if (slice_per_intf > dsc->slice_count)
 		dsc->slice_count = 1;
 
-	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
 	bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
 
 	dsc->slice_chunk_size = bytes_in_slice;
-- 
2.37.3

Re: [PATCH 2/5] drm/msm/dsi: Remove repeated calculation of slice_per_intf

[Konrad Dybcio]

On 1.10.2022 21:08, Marijn Suijten wrote:
> slice_per_intf is already computed for intf_width, which holds the same
> value as hdisplay.
> 
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> 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 | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index e05bae647431..cb6f2fa11f58 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -842,7 +842,7 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
>  static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mode, u32 hdisplay)
>  {
>  	struct drm_dsc_config *dsc = msm_host->dsc;
> -	u32 reg, intf_width, reg_ctrl, reg_ctrl2;
> +	u32 reg, reg_ctrl, reg_ctrl2;
>  	u32 slice_per_intf, total_bytes_per_intf;
>  	u32 pkt_per_line;
>  	u32 bytes_in_slice;
> @@ -851,8 +851,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  	/* first calculate dsc parameters and then program
>  	 * compress mode registers
>  	 */
> -	intf_width = hdisplay;
> -	slice_per_intf = DIV_ROUND_UP(intf_width, dsc->slice_width);
> +	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>  
>  	/* If slice_per_pkt is greater than slice_per_intf
>  	 * then default to 1. This can happen during partial
> @@ -861,7 +860,6 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  	if (slice_per_intf > dsc->slice_count)
>  		dsc->slice_count = 1;
>  
> -	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>  	bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
>  
>  	dsc->slice_chunk_size = bytes_in_slice;

Re: [PATCH 2/5] drm/msm/dsi: Remove repeated calculation of slice_per_intf

[Bjorn Andersson]

On Sat, Oct 01, 2022 at 09:08:04PM +0200, Marijn Suijten wrote:
> slice_per_intf is already computed for intf_width, which holds the same
> value as hdisplay.
> 
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index e05bae647431..cb6f2fa11f58 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -842,7 +842,7 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
>  static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mode, u32 hdisplay)
>  {
>  	struct drm_dsc_config *dsc = msm_host->dsc;
> -	u32 reg, intf_width, reg_ctrl, reg_ctrl2;
> +	u32 reg, reg_ctrl, reg_ctrl2;
>  	u32 slice_per_intf, total_bytes_per_intf;
>  	u32 pkt_per_line;
>  	u32 bytes_in_slice;
> @@ -851,8 +851,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  	/* first calculate dsc parameters and then program
>  	 * compress mode registers
>  	 */
> -	intf_width = hdisplay;
> -	slice_per_intf = DIV_ROUND_UP(intf_width, dsc->slice_width);
> +	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>  
>  	/* If slice_per_pkt is greater than slice_per_intf
>  	 * then default to 1. This can happen during partial
> @@ -861,7 +860,6 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  	if (slice_per_intf > dsc->slice_count)
>  		dsc->slice_count = 1;
>  
> -	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>  	bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
>  
>  	dsc->slice_chunk_size = bytes_in_slice;
> -- 
> 2.37.3
> 

Re: [PATCH 2/5] drm/msm/dsi: Remove repeated calculation of slice_per_intf

[Abhinav Kumar]

On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> slice_per_intf is already computed for intf_width, which holds the same
> value as hdisplay.
> 
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

LGTM,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index e05bae647431..cb6f2fa11f58 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -842,7 +842,7 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
>   static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mode, u32 hdisplay)
>   {
>   	struct drm_dsc_config *dsc = msm_host->dsc;
> -	u32 reg, intf_width, reg_ctrl, reg_ctrl2;
> +	u32 reg, reg_ctrl, reg_ctrl2;
>   	u32 slice_per_intf, total_bytes_per_intf;
>   	u32 pkt_per_line;
>   	u32 bytes_in_slice;
> @@ -851,8 +851,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>   	/* first calculate dsc parameters and then program
>   	 * compress mode registers
>   	 */
> -	intf_width = hdisplay;
> -	slice_per_intf = DIV_ROUND_UP(intf_width, dsc->slice_width);
> +	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>   
>   	/* If slice_per_pkt is greater than slice_per_intf
>   	 * then default to 1. This can happen during partial
> @@ -861,7 +860,6 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>   	if (slice_per_intf > dsc->slice_count)
>   		dsc->slice_count = 1;
>   
> -	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>   	bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
>   
>   	dsc->slice_chunk_size = bytes_in_slice;

[PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits

[Marijn Suijten]

drm_dsc_config's bits_per_pixel field holds a fractional value with 4
bits, which all panel drivers should adhere to for
drm_dsc_pps_payload_pack() to generate a valid payload.  All code in the
DSI driver here seems to assume that this field doesn't contain any
fractional bits, hence resulting in the wrong values being computed.
Since none of the calculations leave any room for fractional bits or
seem to indicate any possible area of support, disallow such values
altogether.

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 | 34 +++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index cb6f2fa11f58..42a5c9776f52 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -847,6 +847,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	u32 pkt_per_line;
 	u32 bytes_in_slice;
 	u32 eol_byte_num;
+	int bpp = dsc->bits_per_pixel >> 4;
+
+	if (dsc->bits_per_pixel & 0xf)
+		/* dsi_populate_dsc_params() already caught this case */
+		pr_err("DSI does not support fractional bits_per_pixel\n");
 
 	/* first calculate dsc parameters and then program
 	 * compress mode registers
@@ -860,7 +865,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	if (slice_per_intf > dsc->slice_count)
 		dsc->slice_count = 1;
 
-	bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
+	bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8);
 
 	dsc->slice_chunk_size = bytes_in_slice;
 
@@ -913,6 +918,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 	u32 va_end = va_start + mode->vdisplay;
 	u32 hdisplay = mode->hdisplay;
 	u32 wc;
+	int ret;
 
 	DBG("");
 
@@ -948,7 +954,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		/* we do the calculations for dsc parameters here so that
 		 * panel can use these parameters
 		 */
-		dsi_populate_dsc_params(dsc);
+		ret = dsi_populate_dsc_params(dsc);
+		if (ret)
+			return;
 
 		/* Divide the display by 3 but keep back/font porch and
 		 * pulse width same
@@ -1229,6 +1237,10 @@ static int dsi_cmd_dma_add(struct msm_dsi_host *msm_host,
 	if (packet.size < len)
 		memset(data + packet.size, 0xff, len - packet.size);
 
+	if (msg->type == MIPI_DSI_PICTURE_PARAMETER_SET)
+		print_hex_dump(KERN_DEBUG, "ALL:", DUMP_PREFIX_NONE,
+				16, 1, data, len, false);
+
 	if (cfg_hnd->ops->tx_buf_put)
 		cfg_hnd->ops->tx_buf_put(msm_host);
 
@@ -1786,6 +1798,12 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
 	int data;
 	int final_value, final_scale;
 	int i;
+	int bpp = dsc->bits_per_pixel >> 4;
+
+	if (dsc->bits_per_pixel & 0xf) {
+		pr_err("DSI does not support fractional bits_per_pixel\n");
+		return -EINVAL;
+	}
 
 	dsc->rc_model_size = 8192;
 	dsc->first_line_bpg_offset = 12;
@@ -1807,7 +1825,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
 	}
 
 	dsc->initial_offset = 6144; /* Not bpp 12 */
-	if (dsc->bits_per_pixel != 8)
+	if (bpp != 8)
 		dsc->initial_offset = 2048;	/* bpp = 12 */
 
 	mux_words_size = 48;		/* bpc == 8/10 */
@@ -1830,16 +1848,16 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
 	 * params are calculated
 	 */
 	groups_per_line = DIV_ROUND_UP(dsc->slice_width, 3);
-	dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8;
-	if ((dsc->slice_width * dsc->bits_per_pixel) % 8)
+	dsc->slice_chunk_size = dsc->slice_width * bpp / 8;
+	if ((dsc->slice_width * bpp) % 8)
 		dsc->slice_chunk_size++;
 
 	/* rbs-min */
 	min_rate_buffer_size =  dsc->rc_model_size - dsc->initial_offset +
-				dsc->initial_xmit_delay * dsc->bits_per_pixel +
+				dsc->initial_xmit_delay * bpp +
 				groups_per_line * dsc->first_line_bpg_offset;
 
-	hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, dsc->bits_per_pixel);
+	hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, bpp);
 
 	dsc->initial_dec_delay = hrd_delay - dsc->initial_xmit_delay;
 
@@ -1862,7 +1880,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);
 
-	target_bpp_x16 = dsc->bits_per_pixel * 16;
+	target_bpp_x16 = bpp * 16;
 
 	data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
 	final_value =  dsc->rc_model_size - data + num_extra_mux_bits;
-- 
2.37.3

Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits

[Konrad Dybcio]

On 1.10.2022 21:08, Marijn Suijten wrote:
> drm_dsc_config's bits_per_pixel field holds a fractional value with 4
> bits, which all panel drivers should adhere to for
> drm_dsc_pps_payload_pack() to generate a valid payload.  All code in the
> DSI driver here seems to assume that this field doesn't contain any
> fractional bits, hence resulting in the wrong values being computed.
> Since none of the calculations leave any room for fractional bits or
> seem to indicate any possible area of support, disallow such values
> altogether.
> 
> 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 | 34 +++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index cb6f2fa11f58..42a5c9776f52 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -847,6 +847,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  	u32 pkt_per_line;
>  	u32 bytes_in_slice;
>  	u32 eol_byte_num;
> +	int bpp = dsc->bits_per_pixel >> 4;
> +
> +	if (dsc->bits_per_pixel & 0xf)
> +		/* dsi_populate_dsc_params() already caught this case */
> +		pr_err("DSI does not support fractional bits_per_pixel\n");
>  
>  	/* first calculate dsc parameters and then program
>  	 * compress mode registers
> @@ -860,7 +865,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  	if (slice_per_intf > dsc->slice_count)
>  		dsc->slice_count = 1;
>  
> -	bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
> +	bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8);
>  
>  	dsc->slice_chunk_size = bytes_in_slice;
>  
> @@ -913,6 +918,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  	u32 va_end = va_start + mode->vdisplay;
>  	u32 hdisplay = mode->hdisplay;
>  	u32 wc;
> +	int ret;
>  
>  	DBG("");
>  
> @@ -948,7 +954,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  		/* we do the calculations for dsc parameters here so that
>  		 * panel can use these parameters
>  		 */
> -		dsi_populate_dsc_params(dsc);
> +		ret = dsi_populate_dsc_params(dsc);
> +		if (ret)
> +			return;
>  
>  		/* Divide the display by 3 but keep back/font porch and
>  		 * pulse width same
> @@ -1229,6 +1237,10 @@ static int dsi_cmd_dma_add(struct msm_dsi_host *msm_host,
>  	if (packet.size < len)
>  		memset(data + packet.size, 0xff, len - packet.size);
>  
> +	if (msg->type == MIPI_DSI_PICTURE_PARAMETER_SET)
> +		print_hex_dump(KERN_DEBUG, "ALL:", DUMP_PREFIX_NONE,
> +				16, 1, data, len, false);
> +
>  	if (cfg_hnd->ops->tx_buf_put)
>  		cfg_hnd->ops->tx_buf_put(msm_host);
>  
> @@ -1786,6 +1798,12 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>  	int data;
>  	int final_value, final_scale;
>  	int i;
> +	int bpp = dsc->bits_per_pixel >> 4;
> +
> +	if (dsc->bits_per_pixel & 0xf) {
> +		pr_err("DSI does not support fractional bits_per_pixel\n");
> +		return -EINVAL;
> +	}
>  
>  	dsc->rc_model_size = 8192;
>  	dsc->first_line_bpg_offset = 12;
> @@ -1807,7 +1825,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>  	}
>  
>  	dsc->initial_offset = 6144; /* Not bpp 12 */
> -	if (dsc->bits_per_pixel != 8)
> +	if (bpp != 8)
>  		dsc->initial_offset = 2048;	/* bpp = 12 */
>  
>  	mux_words_size = 48;		/* bpc == 8/10 */
> @@ -1830,16 +1848,16 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>  	 * params are calculated
>  	 */
>  	groups_per_line = DIV_ROUND_UP(dsc->slice_width, 3);
> -	dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8;
> -	if ((dsc->slice_width * dsc->bits_per_pixel) % 8)
> +	dsc->slice_chunk_size = dsc->slice_width * bpp / 8;
> +	if ((dsc->slice_width * bpp) % 8)
>  		dsc->slice_chunk_size++;
>  
>  	/* rbs-min */
>  	min_rate_buffer_size =  dsc->rc_model_size - dsc->initial_offset +
> -				dsc->initial_xmit_delay * dsc->bits_per_pixel +
> +				dsc->initial_xmit_delay * bpp +
>  				groups_per_line * dsc->first_line_bpg_offset;
>  
> -	hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, dsc->bits_per_pixel);
> +	hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, bpp);
>  
>  	dsc->initial_dec_delay = hrd_delay - dsc->initial_xmit_delay;
>  
> @@ -1862,7 +1880,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);
>  
> -	target_bpp_x16 = dsc->bits_per_pixel * 16;
> +	target_bpp_x16 = bpp * 16;
>  
>  	data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
>  	final_value =  dsc->rc_model_size - data + num_extra_mux_bits;

Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits

[Marijn Suijten]

Doing some self-review as these patches accrued some bit-rot while
waiting to be sent.

On 2022-10-01 21:08:05, Marijn Suijten wrote:
> drm_dsc_config's bits_per_pixel field holds a fractional value with 4
> bits, which all panel drivers should adhere to for
> drm_dsc_pps_payload_pack() to generate a valid payload.  All code in the
> DSI driver here seems to assume that this field doesn't contain any
> fractional bits, hence resulting in the wrong values being computed.
> Since none of the calculations leave any room for fractional bits or
> seem to indicate any possible area of support, disallow such values
> altogether.
> 
> 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 | 34 +++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index cb6f2fa11f58..42a5c9776f52 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -847,6 +847,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  	u32 pkt_per_line;
>  	u32 bytes_in_slice;
>  	u32 eol_byte_num;
> +	int bpp = dsc->bits_per_pixel >> 4;

This should have been u16 instead of int.

> +
> +	if (dsc->bits_per_pixel & 0xf)

Should there be a define for this mask?

> +		/* dsi_populate_dsc_params() already caught this case */
> +		pr_err("DSI does not support fractional bits_per_pixel\n");

This file mostly uses pr_err(), but it's probably cleaner to use
DRM_DEV_ERROR(&msm_host->pdev->dev, ...) even though it's not prevalent
yet?

>  
>  	/* first calculate dsc parameters and then program
>  	 * compress mode registers
> @@ -860,7 +865,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  	if (slice_per_intf > dsc->slice_count)
>  		dsc->slice_count = 1;
>  
> -	bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
> +	bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8);
>  
>  	dsc->slice_chunk_size = bytes_in_slice;
>  
> @@ -913,6 +918,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  	u32 va_end = va_start + mode->vdisplay;
>  	u32 hdisplay = mode->hdisplay;
>  	u32 wc;
> +	int ret;
>  
>  	DBG("");
>  
> @@ -948,7 +954,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  		/* we do the calculations for dsc parameters here so that
>  		 * panel can use these parameters
>  		 */
> -		dsi_populate_dsc_params(dsc);
> +		ret = dsi_populate_dsc_params(dsc);
> +		if (ret)
> +			return;
>  
>  		/* Divide the display by 3 but keep back/font porch and
>  		 * pulse width same
> @@ -1229,6 +1237,10 @@ static int dsi_cmd_dma_add(struct msm_dsi_host *msm_host,
>  	if (packet.size < len)
>  		memset(data + packet.size, 0xff, len - packet.size);
>  
> +	if (msg->type == MIPI_DSI_PICTURE_PARAMETER_SET)
> +		print_hex_dump(KERN_DEBUG, "ALL:", DUMP_PREFIX_NONE,
> +				16, 1, data, len, false);
> +
>  	if (cfg_hnd->ops->tx_buf_put)
>  		cfg_hnd->ops->tx_buf_put(msm_host);
>  
> @@ -1786,6 +1798,12 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>  	int data;
>  	int final_value, final_scale;
>  	int i;
> +	int bpp = dsc->bits_per_pixel >> 4;

Same u16 here.

- Marijn

> +
> +	if (dsc->bits_per_pixel & 0xf) {
> +		pr_err("DSI does not support fractional bits_per_pixel\n");
> +		return -EINVAL;
> +	}
>  
>  	dsc->rc_model_size = 8192;
>  	dsc->first_line_bpg_offset = 12;
> @@ -1807,7 +1825,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>  	}
>  
>  	dsc->initial_offset = 6144; /* Not bpp 12 */
> -	if (dsc->bits_per_pixel != 8)
> +	if (bpp != 8)
>  		dsc->initial_offset = 2048;	/* bpp = 12 */
>  
>  	mux_words_size = 48;		/* bpc == 8/10 */
> @@ -1830,16 +1848,16 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>  	 * params are calculated
>  	 */
>  	groups_per_line = DIV_ROUND_UP(dsc->slice_width, 3);
> -	dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8;
> -	if ((dsc->slice_width * dsc->bits_per_pixel) % 8)
> +	dsc->slice_chunk_size = dsc->slice_width * bpp / 8;
> +	if ((dsc->slice_width * bpp) % 8)
>  		dsc->slice_chunk_size++;
>  
>  	/* rbs-min */
>  	min_rate_buffer_size =  dsc->rc_model_size - dsc->initial_offset +
> -				dsc->initial_xmit_delay * dsc->bits_per_pixel +
> +				dsc->initial_xmit_delay * bpp +
>  				groups_per_line * dsc->first_line_bpg_offset;
>  
> -	hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, dsc->bits_per_pixel);
> +	hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, bpp);
>  
>  	dsc->initial_dec_delay = hrd_delay - dsc->initial_xmit_delay;
>  
> @@ -1862,7 +1880,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);
>  
> -	target_bpp_x16 = dsc->bits_per_pixel * 16;
> +	target_bpp_x16 = bpp * 16;
>  
>  	data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
>  	final_value =  dsc->rc_model_size - data + num_extra_mux_bits;
> -- 
> 2.37.3
> 

Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits

[Dmitry Baryshkov]

On Sat, 1 Oct 2022 at 22:08, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> drm_dsc_config's bits_per_pixel field holds a fractional value with 4
> bits, which all panel drivers should adhere to for
> drm_dsc_pps_payload_pack() to generate a valid payload.  All code in the
> DSI driver here seems to assume that this field doesn't contain any
> fractional bits, hence resulting in the wrong values being computed.
> Since none of the calculations leave any room for fractional bits or
> seem to indicate any possible area of support, disallow such values
> altogether.
>
> 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 | 34 +++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index cb6f2fa11f58..42a5c9776f52 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -847,6 +847,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>         u32 pkt_per_line;
>         u32 bytes_in_slice;
>         u32 eol_byte_num;
> +       int bpp = dsc->bits_per_pixel >> 4;
> +
> +       if (dsc->bits_per_pixel & 0xf)
> +               /* dsi_populate_dsc_params() already caught this case */
> +               pr_err("DSI does not support fractional bits_per_pixel\n");
>
>         /* first calculate dsc parameters and then program
>          * compress mode registers
> @@ -860,7 +865,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>         if (slice_per_intf > dsc->slice_count)
>                 dsc->slice_count = 1;
>
> -       bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
> +       bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8);


bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8 * 16); ?

>
>         dsc->slice_chunk_size = bytes_in_slice;
>
> @@ -913,6 +918,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>         u32 va_end = va_start + mode->vdisplay;
>         u32 hdisplay = mode->hdisplay;
>         u32 wc;
> +       int ret;
>
>         DBG("");
>
> @@ -948,7 +954,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>                 /* we do the calculations for dsc parameters here so that
>                  * panel can use these parameters
>                  */
> -               dsi_populate_dsc_params(dsc);
> +               ret = dsi_populate_dsc_params(dsc);
> +               if (ret)
> +                       return;
>
>                 /* Divide the display by 3 but keep back/font porch and
>                  * pulse width same
> @@ -1229,6 +1237,10 @@ static int dsi_cmd_dma_add(struct msm_dsi_host *msm_host,
>         if (packet.size < len)
>                 memset(data + packet.size, 0xff, len - packet.size);
>
> +       if (msg->type == MIPI_DSI_PICTURE_PARAMETER_SET)
> +               print_hex_dump(KERN_DEBUG, "ALL:", DUMP_PREFIX_NONE,
> +                               16, 1, data, len, false);
> +
>         if (cfg_hnd->ops->tx_buf_put)
>                 cfg_hnd->ops->tx_buf_put(msm_host);
>
> @@ -1786,6 +1798,12 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>         int data;
>         int final_value, final_scale;
>         int i;
> +       int bpp = dsc->bits_per_pixel >> 4;
> +
> +       if (dsc->bits_per_pixel & 0xf) {
> +               pr_err("DSI does not support fractional bits_per_pixel\n");
> +               return -EINVAL;
> +       }
>
>         dsc->rc_model_size = 8192;
>         dsc->first_line_bpg_offset = 12;
> @@ -1807,7 +1825,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>         }
>
>         dsc->initial_offset = 6144; /* Not bpp 12 */
> -       if (dsc->bits_per_pixel != 8)
> +       if (bpp != 8)
>                 dsc->initial_offset = 2048;     /* bpp = 12 */
>
>         mux_words_size = 48;            /* bpc == 8/10 */
> @@ -1830,16 +1848,16 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>          * params are calculated
>          */
>         groups_per_line = DIV_ROUND_UP(dsc->slice_width, 3);
> -       dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8;
> -       if ((dsc->slice_width * dsc->bits_per_pixel) % 8)
> +       dsc->slice_chunk_size = dsc->slice_width * bpp / 8;
> +       if ((dsc->slice_width * bpp) % 8)

One can use fixed point math here too:

dsc->slice_chunk_size = (dsc->slice_width * dsc->bits_per_pixel  + 8 *
16 - 1)/ (8 * 16);

>                 dsc->slice_chunk_size++;
>
>         /* rbs-min */
>         min_rate_buffer_size =  dsc->rc_model_size - dsc->initial_offset +
> -                               dsc->initial_xmit_delay * dsc->bits_per_pixel +
> +                               dsc->initial_xmit_delay * bpp +
>                                 groups_per_line * dsc->first_line_bpg_offset;
>
> -       hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, dsc->bits_per_pixel);
> +       hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, bpp);
>
>         dsc->initial_dec_delay = hrd_delay - dsc->initial_xmit_delay;
>
> @@ -1862,7 +1880,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);
>
> -       target_bpp_x16 = dsc->bits_per_pixel * 16;
> +       target_bpp_x16 = bpp * 16;
>
>         data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;

It looks like this can be replaced with the direct multiplication
instead, maybe with support for overflow/rounding.

>         final_value =  dsc->rc_model_size - data + num_extra_mux_bits;
> --
> 2.37.3
>


-- 
With best wishes
Dmitry

Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits

[Marijn Suijten]

On 2022-10-04 17:45:50, Dmitry Baryshkov wrote:
> On Sat, 1 Oct 2022 at 22:08, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> [..]
> > -       bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
> > +       bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8);
> 
> 
> bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8 * 16); ?

Not necessarily a fan of this, it "hides" the fact that we are dealing
with 4 fractional bits (1/16th precision, it is correct though); but
since this is the only use of `bpp` I can change it and document this
fact wiht a comment on top (including referencing the validation pointed
out in dsi_populate_dsc_params()).

Alternatively we can inline the `>> 4` here?

> >
> >         dsc->slice_chunk_size = bytes_in_slice;
> >
> > @@ -913,6 +918,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >         u32 va_end = va_start + mode->vdisplay;
> >         u32 hdisplay = mode->hdisplay;
> >         u32 wc;
> > +       int ret;
> >
> >         DBG("");
> >
> > @@ -948,7 +954,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >                 /* we do the calculations for dsc parameters here so that
> >                  * panel can use these parameters
> >                  */
> > -               dsi_populate_dsc_params(dsc);
> > +               ret = dsi_populate_dsc_params(dsc);
> > +               if (ret)
> > +                       return;
> >
> >                 /* Divide the display by 3 but keep back/font porch and
> >                  * pulse width same
> > @@ -1229,6 +1237,10 @@ static int dsi_cmd_dma_add(struct msm_dsi_host *msm_host,
> >         if (packet.size < len)
> >                 memset(data + packet.size, 0xff, len - packet.size);
> >
> > +       if (msg->type == MIPI_DSI_PICTURE_PARAMETER_SET)
> > +               print_hex_dump(KERN_DEBUG, "ALL:", DUMP_PREFIX_NONE,
> > +                               16, 1, data, len, false);
> > +
> >         if (cfg_hnd->ops->tx_buf_put)
> >                 cfg_hnd->ops->tx_buf_put(msm_host);
> >
> > @@ -1786,6 +1798,12 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
> >         int data;
> >         int final_value, final_scale;
> >         int i;
> > +       int bpp = dsc->bits_per_pixel >> 4;
> > +
> > +       if (dsc->bits_per_pixel & 0xf) {
> > +               pr_err("DSI does not support fractional bits_per_pixel\n");
> > +               return -EINVAL;
> > +       }
> >
> >         dsc->rc_model_size = 8192;
> >         dsc->first_line_bpg_offset = 12;
> > @@ -1807,7 +1825,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
> >         }
> >
> >         dsc->initial_offset = 6144; /* Not bpp 12 */
> > -       if (dsc->bits_per_pixel != 8)
> > +       if (bpp != 8)
> >                 dsc->initial_offset = 2048;     /* bpp = 12 */
> >
> >         mux_words_size = 48;            /* bpc == 8/10 */
> > @@ -1830,16 +1848,16 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
> >          * params are calculated
> >          */
> >         groups_per_line = DIV_ROUND_UP(dsc->slice_width, 3);
> > -       dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8;
> > -       if ((dsc->slice_width * dsc->bits_per_pixel) % 8)
> > +       dsc->slice_chunk_size = dsc->slice_width * bpp / 8;
> > +       if ((dsc->slice_width * bpp) % 8)
> 
> One can use fixed point math here too:
> 
> dsc->slice_chunk_size = (dsc->slice_width * dsc->bits_per_pixel  + 8 *
> 16 - 1)/ (8 * 16);

Good catch, this is effectively a DIV_ROUND_UP() that we happened to
call bytes_in_slice above...

Shall I tackle this in the same patch, or insert another cleanup patch?

In fact dsi_populate_dsc_params() is called first (this comment),
followed by dsi_update_dsc_timing(), meaning that slice_chunk_size is
already provided and shouldn't be recomputed.

> >                 dsc->slice_chunk_size++;
> >
> >         /* rbs-min */
> >         min_rate_buffer_size =  dsc->rc_model_size - dsc->initial_offset +
> > -                               dsc->initial_xmit_delay * dsc->bits_per_pixel +
> > +                               dsc->initial_xmit_delay * bpp +
> >                                 groups_per_line * dsc->first_line_bpg_offset;
> >
> > -       hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, dsc->bits_per_pixel);
> > +       hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, bpp);
> >
> >         dsc->initial_dec_delay = hrd_delay - dsc->initial_xmit_delay;
> >
> > @@ -1862,7 +1880,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);
> >
> > -       target_bpp_x16 = dsc->bits_per_pixel * 16;
> > +       target_bpp_x16 = bpp * 16;
> >
> >         data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
> 
> It looks like this can be replaced with the direct multiplication
> instead, maybe with support for overflow/rounding.

Thanks, Abhinav pointed out the same in patch 1/5 which originally
cleaned up most - but apparently not all! - of the math here.  I don't
think this value should ever overlow, nor does this `* 16 / 16` have any
effect on rounding (that'd be `/ 16 * 16`).

> >         final_value =  dsc->rc_model_size - data + num_extra_mux_bits;
> > --
> > 2.37.3
> >
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits

[Dmitry Baryshkov]

On 05/10/2022 01:35, Marijn Suijten wrote:
> On 2022-10-04 17:45:50, Dmitry Baryshkov wrote:
>> On Sat, 1 Oct 2022 at 22:08, Marijn Suijten
>> <marijn.suijten@somainline.org> wrote:
>> [..]
>>> -       bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
>>> +       bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8);
>>
>>
>> bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8 * 16); ?
> 
> Not necessarily a fan of this, it "hides" the fact that we are dealing
> with 4 fractional bits (1/16th precision, it is correct though); but
> since this is the only use of `bpp` I can change it and document this
> fact wiht a comment on top (including referencing the validation pointed
> out in dsi_populate_dsc_params()).
> 
> Alternatively we can inline the `>> 4` here?

No, I don't think so. If we shift by 4 bits, we'd loose the fractional 
part. DIV_ROUND_UP( .... , 8 * 16) ensures that we round it up rather 
than just dropping it.

> 
>>>
>>>          dsc->slice_chunk_size = bytes_in_slice;
>>>
>>> @@ -913,6 +918,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>          u32 va_end = va_start + mode->vdisplay;
>>>          u32 hdisplay = mode->hdisplay;
>>>          u32 wc;
>>> +       int ret;
>>>
>>>          DBG("");
>>>
>>> @@ -948,7 +954,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>                  /* we do the calculations for dsc parameters here so that
>>>                   * panel can use these parameters
>>>                   */
>>> -               dsi_populate_dsc_params(dsc);
>>> +               ret = dsi_populate_dsc_params(dsc);
>>> +               if (ret)
>>> +                       return;
>>>
>>>                  /* Divide the display by 3 but keep back/font porch and
>>>                   * pulse width same
>>> @@ -1229,6 +1237,10 @@ static int dsi_cmd_dma_add(struct msm_dsi_host *msm_host,
>>>          if (packet.size < len)
>>>                  memset(data + packet.size, 0xff, len - packet.size);
>>>
>>> +       if (msg->type == MIPI_DSI_PICTURE_PARAMETER_SET)
>>> +               print_hex_dump(KERN_DEBUG, "ALL:", DUMP_PREFIX_NONE,
>>> +                               16, 1, data, len, false);
>>> +
>>>          if (cfg_hnd->ops->tx_buf_put)
>>>                  cfg_hnd->ops->tx_buf_put(msm_host);
>>>
>>> @@ -1786,6 +1798,12 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>>>          int data;
>>>          int final_value, final_scale;
>>>          int i;
>>> +       int bpp = dsc->bits_per_pixel >> 4;
>>> +
>>> +       if (dsc->bits_per_pixel & 0xf) {
>>> +               pr_err("DSI does not support fractional bits_per_pixel\n");
>>> +               return -EINVAL;
>>> +       }
>>>
>>>          dsc->rc_model_size = 8192;
>>>          dsc->first_line_bpg_offset = 12;
>>> @@ -1807,7 +1825,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>>>          }
>>>
>>>          dsc->initial_offset = 6144; /* Not bpp 12 */
>>> -       if (dsc->bits_per_pixel != 8)
>>> +       if (bpp != 8)
>>>                  dsc->initial_offset = 2048;     /* bpp = 12 */
>>>
>>>          mux_words_size = 48;            /* bpc == 8/10 */
>>> @@ -1830,16 +1848,16 @@ static int dsi_populate_dsc_params(struct drm_dsc_config *dsc)
>>>           * params are calculated
>>>           */
>>>          groups_per_line = DIV_ROUND_UP(dsc->slice_width, 3);
>>> -       dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8;
>>> -       if ((dsc->slice_width * dsc->bits_per_pixel) % 8)
>>> +       dsc->slice_chunk_size = dsc->slice_width * bpp / 8;
>>> +       if ((dsc->slice_width * bpp) % 8)
>>
>> One can use fixed point math here too:
>>
>> dsc->slice_chunk_size = (dsc->slice_width * dsc->bits_per_pixel  + 8 *
>> 16 - 1)/ (8 * 16);
> 
> Good catch, this is effectively a DIV_ROUND_UP() that we happened to
> call bytes_in_slice above...
> 
> Shall I tackle this in the same patch, or insert another cleanup patch?

It's up to you. I usually prefer separate patches, even if just to ease 
bisecting between unrelated changes.

> 
> In fact dsi_populate_dsc_params() is called first (this comment),
> followed by dsi_update_dsc_timing(), meaning that slice_chunk_size is
> already provided and shouldn't be recomputed.
> 
>>>                  dsc->slice_chunk_size++;
>>>
>>>          /* rbs-min */
>>>          min_rate_buffer_size =  dsc->rc_model_size - dsc->initial_offset +
>>> -                               dsc->initial_xmit_delay * dsc->bits_per_pixel +
>>> +                               dsc->initial_xmit_delay * bpp +
>>>                                  groups_per_line * dsc->first_line_bpg_offset;
>>>
>>> -       hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, dsc->bits_per_pixel);
>>> +       hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, bpp);
>>>
>>>          dsc->initial_dec_delay = hrd_delay - dsc->initial_xmit_delay;
>>>
>>> @@ -1862,7 +1880,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);
>>>
>>> -       target_bpp_x16 = dsc->bits_per_pixel * 16;
>>> +       target_bpp_x16 = bpp * 16;
>>>
>>>          data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
>>
>> It looks like this can be replaced with the direct multiplication
>> instead, maybe with support for overflow/rounding.
> 
> Thanks, Abhinav pointed out the same in patch 1/5 which originally
> cleaned up most - but apparently not all! - of the math here.  I don't
> think this value should ever overlow, nor does this `* 16 / 16` have any
> effect on rounding (that'd be `/ 16 * 16`).

Ack

> 
>>>          final_value =  dsc->rc_model_size - data + num_extra_mux_bits;
>>> --
>>> 2.37.3
>>>
>>
>>
>> -- 
>> With best wishes
>> Dmitry

-- 
With best wishes
Dmitry

Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits

[Marijn Suijten]

On 2022-10-05 01:40:12, Dmitry Baryshkov wrote:
> On 05/10/2022 01:35, Marijn Suijten wrote:
> > On 2022-10-04 17:45:50, Dmitry Baryshkov wrote:
> >> On Sat, 1 Oct 2022 at 22:08, Marijn Suijten
> >> <marijn.suijten@somainline.org> wrote:
> >> [..]
> >>> -       bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8);
> >>> +       bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8);
> >>
> >>
> >> bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8 * 16); ?
> > 
> > Not necessarily a fan of this, it "hides" the fact that we are dealing
> > with 4 fractional bits (1/16th precision, it is correct though); but
> > since this is the only use of `bpp` I can change it and document this
> > fact wiht a comment on top (including referencing the validation pointed
> > out in dsi_populate_dsc_params()).
> > 
> > Alternatively we can inline the `>> 4` here?
> 
> No, I don't think so. If we shift by 4 bits, we'd loose the fractional 
> part. DIV_ROUND_UP( .... , 8 * 16) ensures that we round it up rather 
> than just dropping it.

I'd still keep the `-EINVAL` on `if (dsc->bits_per_pixel & 0xf)` to
guarantee that there is no fractional part.
After all, as explained in the patch description, none of this code /
the DSI driver in general seems to be able to handle fractional bits per
pixel.

> >>> [..]
> >>> -       dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8;
> >>> -       if ((dsc->slice_width * dsc->bits_per_pixel) % 8)
> >>> +       dsc->slice_chunk_size = dsc->slice_width * bpp / 8;
> >>> +       if ((dsc->slice_width * bpp) % 8)
> >>
> >> One can use fixed point math here too:
> >>
> >> dsc->slice_chunk_size = (dsc->slice_width * dsc->bits_per_pixel  + 8 *
> >> 16 - 1)/ (8 * 16);
> > 
> > Good catch, this is effectively a DIV_ROUND_UP() that we happened to
> > call bytes_in_slice above...
> > 
> > Shall I tackle this in the same patch, or insert another cleanup patch?
> 
> It's up to you. I usually prefer separate patches, even if just to ease 
> bisecting between unrelated changes.

Same feeling here, and have already set it up that way; added two extra
patches to 1. replace this with DIV_ROUND_UP() and 2. remove the
recalculation of slice_chunk_size (disguised as bytes_in_slice) above.

- Marijn

[PATCH 4/5] drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional bits

[Marijn Suijten]

According to the comment this DPU register contains the bits per pixel
as a 6.4 fractional value, conveniently matching the contents of
bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
bits.  However, the downstream source this implementation was
copy-pasted from has its bpp field stored _without_ fractional part.

This makes the entire convoluted math obsolete as it is impossible to
pull those 4 fractional bits out of thin air, by somehow trying to reuse
the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).

The rest of the code merely attempts to keep the integer part a multiple
of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
12; already filling up those bits anyway (but not on downstream).

Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
index f2ddcfb6f7ee..3662df698dae 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -42,7 +42,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
 			      u32 initial_lines)
 {
 	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
-	u32 data, lsb, bpp;
+	u32 data;
 	u32 slice_last_group_size;
 	u32 det_thresh_flatness;
 	bool is_cmd_mode = !(mode & DSC_MODE_VIDEO);
@@ -56,14 +56,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
 	data = (initial_lines << 20);
 	data |= ((slice_last_group_size - 1) << 18);
 	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
-	data |= dsc->bits_per_pixel << 12;
-	lsb = dsc->bits_per_pixel % 4;
-	bpp = dsc->bits_per_pixel / 4;
-	bpp *= 4;
-	bpp <<= 4;
-	bpp |= lsb;
-
-	data |= bpp << 8;
+	data |= (dsc->bits_per_pixel << 8);
 	data |= (dsc->block_pred_enable << 7);
 	data |= (dsc->line_buf_depth << 3);
 	data |= (dsc->simple_422 << 2);
-- 
2.37.3

Re: [PATCH 4/5] drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional bits

[Dmitry Baryshkov]

On Sat, 1 Oct 2022 at 22:08, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> According to the comment this DPU register contains the bits per pixel
> as a 6.4 fractional value, conveniently matching the contents of
> bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
> bits.  However, the downstream source this implementation was
> copy-pasted from has its bpp field stored _without_ fractional part.
>
> This makes the entire convoluted math obsolete as it is impossible to
> pull those 4 fractional bits out of thin air, by somehow trying to reuse
> the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
>
> The rest of the code merely attempts to keep the integer part a multiple
> of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
> 12; already filling up those bits anyway (but not on downstream).
>
> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional bits

[Abhinav Kumar]

On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> According to the comment this DPU register contains the bits per pixel
> as a 6.4 fractional value, conveniently matching the contents of
> bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
> bits.  However, the downstream source this implementation was
> copy-pasted from has its bpp field stored _without_ fractional part.
> 
> This makes the entire convoluted math obsolete as it is impossible to
> pull those 4 fractional bits out of thin air, by somehow trying to reuse
> the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
> 
> The rest of the code merely attempts to keep the integer part a multiple
> of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
> 12; already filling up those bits anyway (but not on downstream).
> 
> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

Many of this bugs are because the downstream code from which this 
implementation was derived wasnt the latest perhaps?

Earlier, downstream had its own DSC struct maybe leading to this 
redundant math but now we have migrated over to use the upstream struct 
drm_dsc_config.

That being said, this patch LGTM
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> index f2ddcfb6f7ee..3662df698dae 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> @@ -42,7 +42,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>   			      u32 initial_lines)
>   {
>   	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
> -	u32 data, lsb, bpp;
> +	u32 data;
>   	u32 slice_last_group_size;
>   	u32 det_thresh_flatness;
>   	bool is_cmd_mode = !(mode & DSC_MODE_VIDEO);
> @@ -56,14 +56,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>   	data = (initial_lines << 20);
>   	data |= ((slice_last_group_size - 1) << 18);
>   	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
> -	data |= dsc->bits_per_pixel << 12;
> -	lsb = dsc->bits_per_pixel % 4;
> -	bpp = dsc->bits_per_pixel / 4;
> -	bpp *= 4;
> -	bpp <<= 4;
> -	bpp |= lsb;
> -
> -	data |= bpp << 8;
> +	data |= (dsc->bits_per_pixel << 8);
>   	data |= (dsc->block_pred_enable << 7);
>   	data |= (dsc->line_buf_depth << 3);
>   	data |= (dsc->simple_422 << 2);

Re: [PATCH 4/5] drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional bits

[Marijn Suijten]

On 2022-10-04 10:03:07, Abhinav Kumar wrote:
> 
> 
> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> > According to the comment this DPU register contains the bits per pixel
> > as a 6.4 fractional value, conveniently matching the contents of
> > bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
> > bits.  However, the downstream source this implementation was
> > copy-pasted from has its bpp field stored _without_ fractional part.
> > 
> > This makes the entire convoluted math obsolete as it is impossible to
> > pull those 4 fractional bits out of thin air, by somehow trying to reuse
> > the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
> > 
> > The rest of the code merely attempts to keep the integer part a multiple
> > of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
> > 12; already filling up those bits anyway (but not on downstream).
> > 
> > Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> Many of this bugs are because the downstream code from which this 
> implementation was derived wasnt the latest perhaps?

Perhaps, this code is "identical" to what I'm looking at in some
downstream 4.14 / 4.19, where the upstream struct for DSC either wasn't
there or wasn't used.  We have to find and address these bugs one by one
to make our panels work, and this series gets one platform (sdm845) down
but has more work pending for others (sm8250 has my current focus).

Or are you suggesting to "redo" the DSC integration work based on a
(much) newer display techpack (SDE driver)?

> Earlier, downstream had its own DSC struct maybe leading to this 
> redundant math but now we have migrated over to use the upstream struct 
> drm_dsc_config.

Found the 3-year-old `disp: msm: use upstream dsc config data` commit
that makes this change.  It carries a similar comment:

    /* integer bpp support only */

The superfluous math was howerver removed earlier, in:

    disp: msm: fix dsc parameters related to 10 bpc 10 bpp

- Marijn

> That being said, this patch LGTM
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH 4/5] drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional bits

[Abhinav Kumar]

On 10/4/2022 3:11 PM, Marijn Suijten wrote:
> On 2022-10-04 10:03:07, Abhinav Kumar wrote:
>>
>>
>> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
>>> According to the comment this DPU register contains the bits per pixel
>>> as a 6.4 fractional value, conveniently matching the contents of
>>> bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
>>> bits.  However, the downstream source this implementation was
>>> copy-pasted from has its bpp field stored _without_ fractional part.
>>>
>>> This makes the entire convoluted math obsolete as it is impossible to
>>> pull those 4 fractional bits out of thin air, by somehow trying to reuse
>>> the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
>>>
>>> The rest of the code merely attempts to keep the integer part a multiple
>>> of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
>>> 12; already filling up those bits anyway (but not on downstream).
>>>
>>> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>
>> Many of this bugs are because the downstream code from which this
>> implementation was derived wasnt the latest perhaps?
> 
> Perhaps, this code is "identical" to what I'm looking at in some
> downstream 4.14 / 4.19, where the upstream struct for DSC either wasn't
> there or wasn't used.  We have to find and address these bugs one by one
> to make our panels work, and this series gets one platform (sdm845) down
> but has more work pending for others (sm8250 has my current focus).
> 
> Or are you suggesting to "redo" the DSC integration work based on a
> (much) newer display techpack (SDE driver)?

There is no need to redo the DSC integration now.

The code I am referring to is here :

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#L240

So with respect to the redundant math in patches 1/3/4/5 of this series, 
I dont see all the redundant math anymore in this calculation.

This is what i meant by my comment.

When DSC changes were pushed, they were indeed validated on sdm845 
devices by Vinod so there was a certain level of confidence on those 
changes.

At this point, we should just consider these as bug-fixes for upstream 
and keep going. A full redo is not required.

At some point in the next couple of months, we plan to add DSC 1.2 
support to MSM.

We will check for any missing changes (if any after this series of 
yours) and push those as part of that.

> 
>> Earlier, downstream had its own DSC struct maybe leading to this
>> redundant math but now we have migrated over to use the upstream struct
>> drm_dsc_config.
> 
> Found the 3-year-old `disp: msm: use upstream dsc config data` commit
> that makes this change.  It carries a similar comment:
> 
>      /* integer bpp support only */
> 
> The superfluous math was howerver removed earlier, in:
> 
>      disp: msm: fix dsc parameters related to 10 bpc 10 bpp
> 
> - Marijn
> 
>> That being said, this patch LGTM
>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH 4/5] drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional bits

[Marijn Suijten]

On 2022-10-05 07:19:11, Abhinav Kumar wrote:
> > [..]
> > 
> > Or are you suggesting to "redo" the DSC integration work based on a
> > (much) newer display techpack (SDE driver)?
> 
> There is no need to redo the DSC integration now.
> 
> The code I am referring to is here :
> 
> 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#L240
> 
> So with respect to the redundant math in patches 1/3/4/5 of this series, 
> I dont see all the redundant math anymore in this calculation.
> 
> This is what i meant by my comment.

It all seems to have had a nice clean-up.  What I meant is that it might
have been more efficient to copy-paste the cleaned-up, improved
downstream implementation instead of individually trying to find and
address all issues; either by running into these bugs on upstream (the
way this patch series came to be), or by comparing the new/improved
downstream with upstream.

> When DSC changes were pushed, they were indeed validated on sdm845 
> devices by Vinod so there was a certain level of confidence on those 
> changes.

Some branches seemed to have a display driver without the DCS PPS
command, or with the command commented out (relying on the panel being
configured for DSC by the bootloader).  The "4 fractional bits" issue
might have gone unnoticed since the panel driver was writing, and both
the DSI and DPU1 drivers were reading this field without those
fractional bits.

It's only a small bug (but with disastrous results on panel drivers with
proper DCS PPS command), the rest is cruft that was copied from
downstream but not filtered out in development nor review.

> At this point, we should just consider these as bug-fixes for upstream 
> and keep going. A full redo is not required.

Ack, at least that doesn't make this series/work obsolete :)

> At some point in the next couple of months, we plan to add DSC 1.2 
> support to MSM.

That's appreciated as all devices I have here (on newer SoCs with DSC
1.2) also have high-resolution, high-fps panels that need DSC to
function correctly.
We'll see who gets to it first though :)

> We will check for any missing changes (if any after this series of 
> yours) and push those as part of that.

There are a few, but it's hard to say until the panel is fully working.
Current focus is on sm8250.
We can discuss this at a more informal pace in #linux-msm if you're
interested.

- Marijn

[PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

[Marijn Suijten]

msm's dsi_host specifies 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 parameter when the field is only expected 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 all values that constitute the RC Range
parameter fields to their expected size.

Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
index c869c6e51e2b..2e7ef242685d 100644
--- a/drivers/gpu/drm/display/drm_dsc_helper.c
+++ b/drivers/gpu/drm/display/drm_dsc_helper.c
@@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
 	 */
 	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
 		pps_payload->rc_range_parameters[i] =
-			cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
+			cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
 				     DSC_PPS_RC_RANGE_MINQP_SHIFT) |
-				    (dsc_cfg->rc_range_params[i].range_max_qp <<
+				    ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
 				     DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
-				    (dsc_cfg->rc_range_params[i].range_bpg_offset));
+				    (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
 	}
 
 	/* PPS 88 */
-- 
2.37.3

Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

[Marijn Suijten]

On 2022-10-01 21:08:07, Marijn Suijten wrote:
> msm's dsi_host specifies 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 parameter when the field is only expected 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 all values that constitute the RC Range
> parameter fields to their expected size.
> 
> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> index c869c6e51e2b..2e7ef242685d 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
>  	 */
>  	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
>  		pps_payload->rc_range_parameters[i] =
> -			cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
> +			cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
>  				     DSC_PPS_RC_RANGE_MINQP_SHIFT) |
> -				    (dsc_cfg->rc_range_params[i].range_max_qp <<
> +				    ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
>  				     DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
> -				    (dsc_cfg->rc_range_params[i].range_bpg_offset));
> +				    (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));

Pre-empting the reviews: I was contemplating whether to use FIELD_PREP
here, given that it's not yet used anywhere else in this file.  For that
I'd remove the existing _SHIFT definitions and replace them with:

	#define DSC_PPS_RC_RANGE_MINQP_MASK		GENMASK(15, 11)
	#define DSC_PPS_RC_RANGE_MAXQP_MASK		GENMASK(10, 6)
	#define DSC_PPS_RC_RANGE_BPG_OFFSET_MASK	GENMASK(5, 0)

And turn this section of code into:

	cpu_to_be16(FIELD_PREP(DSC_PPS_RC_RANGE_MINQP_MASK,
			       dsc_cfg->rc_range_params[i].range_min_qp) |
		    FIELD_PREP(DSC_PPS_RC_RANGE_MAXQP_MASK,
			       dsc_cfg->rc_range_params[i].range_max_qp) |
		    FIELD_PREP(DSC_PPS_RC_RANGE_BPG_OFFSET_MASK,
			       dsc_cfg->rc_range_params[i].range_bpg_offset));

Is that okay/recommended?

- Marijn

>  	}
>  
>  	/* PPS 88 */
> -- 
> 2.37.3
> 

Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

[Dmitry Baryshkov]

On Sat, 1 Oct 2022 at 23:23, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2022-10-01 21:08:07, Marijn Suijten wrote:
> > msm's dsi_host specifies 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 parameter when the field is only expected 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 all values that constitute the RC Range
> > parameter fields to their expected size.
> >
> > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> >  drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> > index c869c6e51e2b..2e7ef242685d 100644
> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> > @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
> >        */
> >       for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> >               pps_payload->rc_range_parameters[i] =
> > -                     cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
> > +                     cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
> >                                    DSC_PPS_RC_RANGE_MINQP_SHIFT) |
> > -                                 (dsc_cfg->rc_range_params[i].range_max_qp <<
> > +                                 ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
> >                                    DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
> > -                                 (dsc_cfg->rc_range_params[i].range_bpg_offset));
> > +                                 (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
>
> Pre-empting the reviews: I was contemplating whether to use FIELD_PREP
> here, given that it's not yet used anywhere else in this file.  For that
> I'd remove the existing _SHIFT definitions and replace them with:
>
>         #define DSC_PPS_RC_RANGE_MINQP_MASK             GENMASK(15, 11)
>         #define DSC_PPS_RC_RANGE_MAXQP_MASK             GENMASK(10, 6)
>         #define DSC_PPS_RC_RANGE_BPG_OFFSET_MASK        GENMASK(5, 0)
>
> And turn this section of code into:
>
>         cpu_to_be16(FIELD_PREP(DSC_PPS_RC_RANGE_MINQP_MASK,
>                                dsc_cfg->rc_range_params[i].range_min_qp) |
>                     FIELD_PREP(DSC_PPS_RC_RANGE_MAXQP_MASK,
>                                dsc_cfg->rc_range_params[i].range_max_qp) |
>                     FIELD_PREP(DSC_PPS_RC_RANGE_BPG_OFFSET_MASK,
>                                dsc_cfg->rc_range_params[i].range_bpg_offset));
>
> Is that okay/recommended?

This is definitely easier to review. However if you do not want to use
FIELD_PREP, it would be better to split this into a series of `data |=
something` assignments terminated with the rc_range_parameters[i]
assignment.

>
> - Marijn
>
> >       }
> >
> >       /* PPS 88 */
> > --
> > 2.37.3
> >



-- 
With best wishes
Dmitry

Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

[Marijn Suijten]

On 2022-10-04 17:41:07, Dmitry Baryshkov wrote:
> On Sat, 1 Oct 2022 at 23:23, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> [..]
> > Pre-empting the reviews: I was contemplating whether to use FIELD_PREP
> > here, given that it's not yet used anywhere else in this file.  For that
> > I'd remove the existing _SHIFT definitions and replace them with:
> >
> >         #define DSC_PPS_RC_RANGE_MINQP_MASK             GENMASK(15, 11)
> >         #define DSC_PPS_RC_RANGE_MAXQP_MASK             GENMASK(10, 6)
> >         #define DSC_PPS_RC_RANGE_BPG_OFFSET_MASK        GENMASK(5, 0)
> >
> > And turn this section of code into:
> >
> >         cpu_to_be16(FIELD_PREP(DSC_PPS_RC_RANGE_MINQP_MASK,
> >                                dsc_cfg->rc_range_params[i].range_min_qp) |
> >                     FIELD_PREP(DSC_PPS_RC_RANGE_MAXQP_MASK,
> >                                dsc_cfg->rc_range_params[i].range_max_qp) |
> >                     FIELD_PREP(DSC_PPS_RC_RANGE_BPG_OFFSET_MASK,
> >                                dsc_cfg->rc_range_params[i].range_bpg_offset));
> >
> > Is that okay/recommended?
> 
> This is definitely easier to review. However if you do not want to use
> FIELD_PREP, it would be better to split this into a series of `data |=
> something` assignments terminated with the rc_range_parameters[i]
> assignment.

Anything is fine by me, I have no strong opinion on this and rather
leave it up to the maintainers.  However, FIELD_PREP gives us concise
`#define`s through a single `GENMASK()` carrying both the shift and
mask/field-width.
At the same time these genmask definitions map more clearly to the
layout comment right above:

	/*
	 * For DSC sink programming the RC Range parameter fields
	 * are as follows: Min_qp[15:11], max_qp[10:6], offset[5:0]
	 */

If switching to `data |=` however, I've been recommended to not use
FIELD_PREP but fulyl write out `data |= (value & MASK) << SHIFT`
instead.

Perhaps a more important question is how to apply this consistently
throughout the function?

- Marijn

Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

[Abhinav Kumar]

On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> msm's dsi_host specifies 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 parameter when the field is only expected 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 all values that constitute the RC Range
> parameter fields to their expected size.
> 
> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> index c869c6e51e2b..2e7ef242685d 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
>   	 */
>   	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
>   		pps_payload->rc_range_parameters[i] =
> -			cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
> +			cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
>   				     DSC_PPS_RC_RANGE_MINQP_SHIFT) |
> -				    (dsc_cfg->rc_range_params[i].range_max_qp <<
> +				    ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
>   				     DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
> -				    (dsc_cfg->rc_range_params[i].range_bpg_offset));
> +				    (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
>   	}
>   

Looking at some examples of this for other vendors, they have managed to 
limit the value to 6 bits in their drivers:

https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L532

https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dsc/rc_calc_dpi.c#L87

Perhaps, msm should do the same thing instead of the helper change.

If you want to move to helper, other drivers need to be changed too to 
remove duplicate & 0x3f.

FWIW, this too has already been fixed in the latest downstream driver too.


Thanks

Abhinav

>   	/* PPS 88 */

Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

[Marijn Suijten]

On 2022-10-04 13:22:25, Abhinav Kumar wrote:
> 
> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> > msm's dsi_host specifies 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 parameter when the field is only expected 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 all values that constitute the RC Range
> > parameter fields to their expected size.
> > 
> > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> >   drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> > index c869c6e51e2b..2e7ef242685d 100644
> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> > @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
> >   	 */
> >   	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> >   		pps_payload->rc_range_parameters[i] =
> > -			cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
> > +			cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
> >   				     DSC_PPS_RC_RANGE_MINQP_SHIFT) |
> > -				    (dsc_cfg->rc_range_params[i].range_max_qp <<
> > +				    ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
> >   				     DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
> > -				    (dsc_cfg->rc_range_params[i].range_bpg_offset));
> > +				    (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
> >   	}
> >   
> 
> Looking at some examples of this for other vendors, they have managed to 
> limit the value to 6 bits in their drivers:
> 
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L532
> 
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dsc/rc_calc_dpi.c#L87
> 
> Perhaps, msm should do the same thing instead of the helper change.

Thanks, I should have done my due-diligence and look up how other
drivers dealt with this, but wasn't immediately expecting negative
values elsewhere.

Alas, as explained in the cover letter I opted to perform the masking in
the PPS packing code as the DSC block code also reads these values, and
would suddenly write 6-bit intead of 8-bit values to the
DSC_RANGE_BPG_OFFSET registers.  Quick testing on the mentioned sdm845
platform shows no regressions, but I'm not sure if that's safe to rely
on?

> If you want to move to helper, other drivers need to be changed too to 
> remove duplicate & 0x3f.

Sure, we only have to confirm whether those drivers also read back the
value(s) in rc_range_params, and expect / allow this to be 8 instead of
6 bits.

> FWIW, this too has already been fixed in the latest downstream driver too.

What is this supposed to mean?  Is there a downstream DPU project that
has pending patches needing to be upstreamed?  Or is the downstream SDE,
techpack/display, or whatever it is called nowadays, slowly using more
DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack()
helper function as pointed out in an earlier mail?

Offtopic: are SDE and DPU growing closer together, hopefully achieving
feature parity allowing the SDE project to be dropped in favour of a
fully upstreamed DPU driver for day-one out-of-the-box mainline support
for new SoCs (as long as work is published and on its way upstream)?

- Marijn

Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

[Abhinav Kumar]

On 10/4/2022 2:57 PM, Marijn Suijten wrote:
> On 2022-10-04 13:22:25, Abhinav Kumar wrote:
>>
>> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
>>> msm's dsi_host specifies 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 parameter when the field is only expected 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 all values that constitute the RC Range
>>> parameter fields to their expected size.
>>>
>>> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>> ---
>>>    drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
>>> index c869c6e51e2b..2e7ef242685d 100644
>>> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
>>> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
>>> @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
>>>    	 */
>>>    	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
>>>    		pps_payload->rc_range_parameters[i] =
>>> -			cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
>>> +			cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
>>>    				     DSC_PPS_RC_RANGE_MINQP_SHIFT) |
>>> -				    (dsc_cfg->rc_range_params[i].range_max_qp <<
>>> +				    ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
>>>    				     DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
>>> -				    (dsc_cfg->rc_range_params[i].range_bpg_offset));
>>> +				    (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
>>>    	}
>>>    
>>
>> Looking at some examples of this for other vendors, they have managed to
>> limit the value to 6 bits in their drivers:
>>
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L532
>>
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dsc/rc_calc_dpi.c#L87
>>
>> Perhaps, msm should do the same thing instead of the helper change.
> 
> Thanks, I should have done my due-diligence and look up how other
> drivers dealt with this, but wasn't immediately expecting negative
> values elsewhere.
> 
> Alas, as explained in the cover letter I opted to perform the masking in
> the PPS packing code as the DSC block code also reads these values, and
> would suddenly write 6-bit intead of 8-bit values to the
> DSC_RANGE_BPG_OFFSET registers.  Quick testing on the mentioned sdm845
> platform shows no regressions, but I'm not sure if that's safe to rely
> on?

I looked up the MDP_DSC_0_RANGE_BPG_OFFSET_* registers.
They take only a 6-bit value according to the SW documentation ( bits 5:0 )

It was always expecting only a 6-bit value and not 8.

So this change is safe.

> 
>> If you want to move to helper, other drivers need to be changed too to
>> remove duplicate & 0x3f.
> 
> Sure, we only have to confirm whether those drivers also read back the
> value(s) in rc_range_params, and expect / allow this to be 8 instead of
> 6 bits.
> 
>> FWIW, this too has already been fixed in the latest downstream driver too.
> 
> What is this supposed to mean?  Is there a downstream DPU project that
> has pending patches needing to be upstreamed?  Or is the downstream SDE,
> techpack/display, or whatever it is called nowadays, slowly using more
> DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack()
> helper function as pointed out in an earlier mail?
> 

No, what I meant was, the version of downstream driver based on which 
the upstream DSC was made seems to be an older version. Downstream 
drivers keep getting updated and we always keep trying to align with 
upstream structs.

This is true not just for DSC but even other blocks.

So as part of that effort, we started using struct drm_dsc_config . That 
change was made on newer chipsets. But the downstream SW on sdm845 based 
on which the DSC was upstreamed seems like didnt have that. Hence all 
this redundant math happened.

So this comment was more of a explanation about why this issue happened 
even though latest downstream didnt have this issue.

> Offtopic: are SDE and DPU growing closer together, hopefully achieving
> feature parity allowing the SDE project to be dropped in favour of a
> fully upstreamed DPU driver for day-one out-of-the-box mainline support
> for new SoCs (as long as work is published and on its way upstream)?
> 

There is still a lot of gap between SDE and DPU drivers at this point. 
We keep trying to upstream as many features as possible to minimize the 
gap but there is still a lot of work to do.


> - Marijn

Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

[Marijn Suijten]

On 2022-10-04 15:31:10, Abhinav Kumar wrote:
> 
> 
> On 10/4/2022 2:57 PM, Marijn Suijten wrote:
> > [..]
> > Alas, as explained in the cover letter I opted to perform the masking in
> > the PPS packing code as the DSC block code also reads these values, and
> > would suddenly write 6-bit intead of 8-bit values to the
> > DSC_RANGE_BPG_OFFSET registers.  Quick testing on the mentioned sdm845
> > platform shows no regressions, but I'm not sure if that's safe to rely
> > on?
> 
> I looked up the MDP_DSC_0_RANGE_BPG_OFFSET_* registers.
> They take only a 6-bit value according to the SW documentation ( bits 5:0 )
> 
> It was always expecting only a 6-bit value and not 8.
> 
> So this change is safe.

Ack, I think that implies I should make this change and move the masks
to the DSI driver?

> >> If you want to move to helper, other drivers need to be changed too to
> >> remove duplicate & 0x3f.
> > 
> > Sure, we only have to confirm whether those drivers also read back the
> > value(s) in rc_range_params, and expect / allow this to be 8 instead of
> > 6 bits.
> > 
> >> FWIW, this too has already been fixed in the latest downstream driver too.
> > 
> > What is this supposed to mean?  Is there a downstream DPU project that
> > has pending patches needing to be upstreamed?  Or is the downstream SDE,
> > techpack/display, or whatever it is called nowadays, slowly using more
> > DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack()
> > helper function as pointed out in an earlier mail?
> > 
> 
> No, what I meant was, the version of downstream driver based on which 
> the upstream DSC was made seems to be an older version. Downstream 
> drivers keep getting updated and we always keep trying to align with 
> upstream structs.
> 
> This is true not just for DSC but even other blocks.
> 
> So as part of that effort, we started using struct drm_dsc_config . That 
> change was made on newer chipsets. But the downstream SW on sdm845 based 
> on which the DSC was upstreamed seems like didnt have that. Hence all 
> this redundant math happened.
> 
> So this comment was more of a explanation about why this issue happened 
> even though latest downstream didnt have this issue.

Thanks, I understood most of that but wasn't aware these exact "issues"
were also addressed downstream (by i.e. also using the upstream
structs).

> > Offtopic: are SDE and DPU growing closer together, hopefully achieving
> > feature parity allowing the SDE project to be dropped in favour of a
> > fully upstreamed DPU driver for day-one out-of-the-box mainline support
> > for new SoCs (as long as work is published and on its way upstream)?
> > 
> 
> There is still a lot of gap between SDE and DPU drivers at this point. 
> We keep trying to upstream as many features as possible to minimize the 
> gap but there is still a lot of work to do.

Glad to hear, but that sounds like a very hard to close gap unless
downstream "just works on DPU" instead of having parallel development on
two "competing" drivers for the exact same hardware.

- Marijn

Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

[Abhinav Kumar]

On 10/4/2022 3:39 PM, Marijn Suijten wrote:
> On 2022-10-04 15:31:10, Abhinav Kumar wrote:
>>
>>
>> On 10/4/2022 2:57 PM, Marijn Suijten wrote:
>>> [..]
>>> Alas, as explained in the cover letter I opted to perform the masking in
>>> the PPS packing code as the DSC block code also reads these values, and
>>> would suddenly write 6-bit intead of 8-bit values to the
>>> DSC_RANGE_BPG_OFFSET registers.  Quick testing on the mentioned sdm845
>>> platform shows no regressions, but I'm not sure if that's safe to rely
>>> on?
>>
>> I looked up the MDP_DSC_0_RANGE_BPG_OFFSET_* registers.
>> They take only a 6-bit value according to the SW documentation ( bits 5:0 )
>>
>> It was always expecting only a 6-bit value and not 8.
>>
>> So this change is safe.
> 
> Ack, I think that implies I should make this change and move the masks
> to the DSI driver?

hmm .... downstream seems to have the & just before the reg write

https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde/sde_hw_dsc_1_2.c#L310

But yes, we can move this to the dsi calculation to match what others 
are doing. I am fine with that.

> 
>>>> If you want to move to helper, other drivers need to be changed too to
>>>> remove duplicate & 0x3f.
>>>
>>> Sure, we only have to confirm whether those drivers also read back the
>>> value(s) in rc_range_params, and expect / allow this to be 8 instead of
>>> 6 bits.
>>>
>>>> FWIW, this too has already been fixed in the latest downstream driver too.
>>>
>>> What is this supposed to mean?  Is there a downstream DPU project that
>>> has pending patches needing to be upstreamed?  Or is the downstream SDE,
>>> techpack/display, or whatever it is called nowadays, slowly using more
>>> DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack()
>>> helper function as pointed out in an earlier mail?
>>>
>>
>> No, what I meant was, the version of downstream driver based on which
>> the upstream DSC was made seems to be an older version. Downstream
>> drivers keep getting updated and we always keep trying to align with
>> upstream structs.
>>
>> This is true not just for DSC but even other blocks.
>>
>> So as part of that effort, we started using struct drm_dsc_config . That
>> change was made on newer chipsets. But the downstream SW on sdm845 based
>> on which the DSC was upstreamed seems like didnt have that. Hence all
>> this redundant math happened.
>>
>> So this comment was more of a explanation about why this issue happened
>> even though latest downstream didnt have this issue.
> 
> Thanks, I understood most of that but wasn't aware these exact "issues"
> were also addressed downstream (by i.e. also using the upstream
> structs).
> 

Even I wasnt. When I was reviewing this series, it seemed like a valid 
change so I checked the latest downstream code like i always do to see 
whether and how this is handled and found that these issues were 
addressed there so thought i would update that here.

>>> Offtopic: are SDE and DPU growing closer together, hopefully achieving
>>> feature parity allowing the SDE project to be dropped in favour of a
>>> fully upstreamed DPU driver for day-one out-of-the-box mainline support
>>> for new SoCs (as long as work is published and on its way upstream)?
>>>
>>
>> There is still a lot of gap between SDE and DPU drivers at this point.
>> We keep trying to upstream as many features as possible to minimize the
>> gap but there is still a lot of work to do.
> 
> Glad to hear, but that sounds like a very hard to close gap unless
> downstream "just works on DPU" instead of having parallel development on
> two "competing" drivers for the exact same hardware.
> 
Its not really parallel development OR competing drivers.
The design of these features downstream (not just DSC but many others) 
is quite different because some of the downstream designs wont get 
accepted upstream as its tightly coupled with our 
compositor/device-tree. Thats where we are slowly trying to implement 
these in an upstream compatible way.

BTW, that being said. Its not always the case that every issue seen 
upstream has already been fixed downstream. In fact, on some occasions 
we found something fixed in upstream in a better way and ported them 
downstream too.

Thanks

Abhinav
> - Marijn

Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

[Marijn Suijten]

On 2022-10-05 08:33:23, Abhinav Kumar wrote:
[..]
> hmm .... downstream seems to have the & just before the reg write
> 
> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde/sde_hw_dsc_1_2.c#L310

https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde/sde_hw_dsc.c#L156

The reference code for NON-v1.2 doesn't do this here, but you already
confirmed by the docs - and I confirmed by testing a set of size #1 -
that the register is fine with having only 6 bits.

> But yes, we can move this to the dsi calculation to match what others 
> are doing. I am fine with that.

Thanks, done that in v2.

> > [..]
> 
> Even I wasnt. When I was reviewing this series, it seemed like a valid 
> change so I checked the latest downstream code like i always do to see 
> whether and how this is handled and found that these issues were 
> addressed there so thought i would update that here.

Thanks for confirming that it was done in the correct/same way :)

> > [..]
> Its not really parallel development OR competing drivers.
> The design of these features downstream (not just DSC but many others) 
> is quite different because some of the downstream designs wont get 
> accepted upstream as its tightly coupled with our 
> compositor/device-tree. Thats where we are slowly trying to implement 
> these in an upstream compatible way.

But this is what it feels like.

To me this sounds like downstream is more of a staging / prototyping
area that is actively used in production, while the driver
implementation is fleshed out and slowly brought to mainline in a
careful and controlled manner.

That's not a bad thing, but it does mean that mainline always lags
behind in terms of features and hardware support.  At least I'm glad
to hear that downstream is slowly using more DRM primitives, and the
driver is becoming more digestible as well.

> BTW, that being said. Its not always the case that every issue seen 
> upstream has already been fixed downstream. In fact, on some occasions 
> we found something fixed in upstream in a better way and ported them 
> downstream too.

I wasn't expecting anything else, as different drivers have inevitably
different details and different bugs.  The issues this series fixes
weren't applicable to the downstream kernel because it (at the time)
wasn't even using this drm_dsc_config struct with different semantics.
Regardless, it's good to hear that fixes are transplanted in both ways
even if it does mean extra overhead maintaining and keeping tabs on two
drivers at once.

- Marijn

Re: [PATCH 0/5] drm: Fix math issues in MSM DSC implementation

[Vinod Koul]

On 01-10-22, 21:08, 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 an approach for dealing with
> dsi_host setting negative values in range_bpg_offset, resulting in
> overflow inside drm_dsc_pps_payload_pack().
> 
> Note that updating the static bpg_offset array to limit the size of
> these negative values to 6 bits changes what would be written to the DPU
> hardware at register(s) DSC_RANGE_BPG_OFFSET, hence the choice has been
> made to cover up for this while packing the value into a smaller field
> instead.

Thanks for fixing these. I dont have my pixel3 availble but changes lgtm

Reviewed-by: Vinod Koul <vkoul@kernel.org>

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

Does it need two dsi lanes?

-- 
~Vinod

Re: [PATCH 0/5] drm: Fix math issues in MSM DSC implementation

[Marijn Suijten]

On 2022-10-04 10:12:58, Vinod Koul wrote:
> On 01-10-22, 21:08, 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 an approach for dealing with
> > dsi_host setting negative values in range_bpg_offset, resulting in
> > overflow inside drm_dsc_pps_payload_pack().
> > 
> > Note that updating the static bpg_offset array to limit the size of
> > these negative values to 6 bits changes what would be written to the DPU
> > hardware at register(s) DSC_RANGE_BPG_OFFSET, hence the choice has been
> > made to cover up for this while packing the value into a smaller field
> > instead.
> 
> Thanks for fixing these. I dont have my pixel3 availble but changes lgtm
> 
> Reviewed-by: Vinod Koul <vkoul@kernel.org>

Thanks; any comment on the self-review I sent in for patch 3 and 5?

> > Altogether this series is responsible for solving _all_ Display Stream
> > Compression issues and artifacts on the Sony Tama (sdm845) Akatsuki
> > smartphone (2880x1440p).
> 
> Does it need two dsi lanes?

This panel has the default of four dsi data lanes enabled:

https://github.com/sonyxperiadev/kernel/blob/f956fbd9a234033bd18234d456a2c32c126b38f3/arch/arm64/boot/dts/qcom/dsi-panel-somc-akatsuki.dtsi#L74-L77

Unless you are referring to dual-dsi (ctrl/phy); this panel doesn't have
a dual connection, but I do have devices on sm8350/sm8450 with a
"4k"@120Hz display that have this, in case you want it to be tested?

However, for the time being I'm focussing on a similar panel (4 data
lanes, single DSI ctrl/phy) on sm8250 which keeps showing corrupted /
garbled data and resulting in ping-pong timeouts.  I haven't yet
confirmed if this is due to the "integration" of the pingpong block with
the intf (since relevant registers and interrupts still seem to be
accessible), a mismatching resource topology, or a misconfiguration
elswhere.  Relevant panel dts if you're interested:

https://github.com/sonyxperiadev/kernel/blob/e70161ec43b147b0b02578d05ab64552fd2df2cd/arch/arm64/boot/dts/somc/dsi-panel-sofef03_m-fhd_plus.dtsi

- Marijn