[PATCH 0/4] Add DSC v1.2 Support for DSI

[PATCH 0/4] Add DSC v1.2 Support for DSI

[Jessica Zhang]

This is a series of changes for DSI to enable support for DSC v1.2.

This includes:

1) Dividing the pclk_rate by the compression ratio when DSC is enabled
2) Fixing the word count calculation for DSC
3) Setting the DATA_COMPRESS bit when DSC is enabled

With these changes (and the dependency below), DSC v1.2 should work over
DSI.

Depends-on: "add DSC 1.2 dpu supports" [1]

[1] https://patchwork.freedesktop.org/series/116789/

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
Jessica Zhang (4):
      drm/msm/dsi: Adjust pclk rate for compression
      drm/msm/dsi: Fix compressed word count calculation
      drm/msm/dpu: Add has_data_compress to dpu_caps
      drm/msm/dpu: Enable compression for command mode

 .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h |  1 +
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h |  1 +
 .../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h   |  1 +
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h |  1 +
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h |  1 +
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  4 ++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |  2 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        |  7 +++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  2 ++
 drivers/gpu/drm/msm/dsi/dsi_host.c                 | 23 +++++++++++++++++-----
 10 files changed, 38 insertions(+), 5 deletions(-)
---
base-commit: a4e4b4826fe482d5f63a0232bc6588da2edfa45b
change-id: 20230405-add-dsc-support-fe130ba49841

Best regards,
-- 
Jessica Zhang <quic_jesszhan@quicinc.com>

[PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression

[Jessica Zhang]

Divide the pclk rate by the compression ratio when DSC is enabled

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 43a5ec33eee8..35c69dbe5f6f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -561,7 +561,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
 	clk_disable_unprepare(msm_host->byte_clk);
 }
 
-static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi)
+static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode,
+		struct drm_dsc_config *dsc, bool is_bonded_dsi)
 {
 	unsigned long pclk_rate;
 
@@ -576,6 +577,11 @@ static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool
 	if (is_bonded_dsi)
 		pclk_rate /= 2;
 
+	/* If DSC is enabled, divide pclk by compression ratio */
+	if (dsc)
+		pclk_rate = DIV_ROUND_UP(pclk_rate,
+				dsc->bits_per_component * 3 / msm_dsc_get_bpp_int(dsc));
+
 	return pclk_rate;
 }
 
@@ -585,7 +591,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
 	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
 	u8 lanes = msm_host->lanes;
 	u32 bpp = dsi_get_bpp(msm_host->format);
-	unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
+	unsigned long pclk_rate = dsi_get_pclk_rate(mode, msm_host->dsc, is_bonded_dsi);
 	u64 pclk_bpp = (u64)pclk_rate * bpp;
 
 	if (lanes == 0) {
@@ -604,7 +610,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
 
 static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 {
-	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi);
+	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi);
 	msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi,
 							msm_host->mode);
 
@@ -634,7 +640,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 
 	dsi_calc_pclk(msm_host, is_bonded_dsi);
 
-	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
+	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi) * bpp;
 	do_div(pclk_bpp, 8);
 	msm_host->src_clk_rate = pclk_bpp;
 

-- 
2.40.1

[PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Jessica Zhang]

Currently, word count is calculated using slice_count. This is incorrect
as downstream uses slice per packet, which is different from
slice_count.

Slice count represents the number of soft slices per interface, and its
value will not always match that of slice per packet. For example, it is
possible to have cases where there are multiple soft slices per interface
but the panel specifies only one slice per packet.

Thus, use the default value of one slice per packet and remove slice_count
from the word count calculation.

Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 35c69dbe5f6f..b0d448ffb078 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -996,7 +996,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		if (!msm_host->dsc)
 			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
 		else
-			wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
+			/*
+			 * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1.
+			 * Currently, the driver only supports default value of slice_per_packet = 1
+			 *
+			 * TODO: Expand drm_panel struct to hold slice_per_packet info
+			 *       and adjust DSC math to account for slice_per_packet.
+			 */
+			wc = msm_host->dsc->slice_chunk_size + 1;
 
 		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
 			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |

-- 
2.40.1

[PATCH 3/4] drm/msm/dpu: Add has_data_compress to dpu_caps

[Jessica Zhang]

Add data_compress feature to DPU HW catalog.

In DPU 7.x and later, there is a DATA_COMPRESS register that must be set
within the DPU INTF block for DSC to work.

As core_rev (and related macros) was removed from the dpu_kms struct, the
most straightforward way to indicate the presence of this register would be
to have a flag in dpu_caps.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h   | 1 +
 drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h   | 1 +
 drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 1 +
 drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h   | 1 +
 drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h   | 1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h           | 2 ++
 6 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
index f98c2a5b0e87..4160a35ff20f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
@@ -15,6 +15,7 @@ static const struct dpu_caps sm8350_dpu_caps = {
 	.has_dim_layer = true,
 	.has_idle_pc = true,
 	.has_3d_merge = true,
+	.has_data_compress = true,
 	.max_linewidth = 4096,
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
index 3fd0498ab420..23230841a0d1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
@@ -13,6 +13,7 @@ static const struct dpu_caps sc7280_dpu_caps = {
 	.qseed_type = DPU_SSPP_SCALER_QSEED4,
 	.has_dim_layer = true,
 	.has_idle_pc = true,
+	.has_data_compress = true,
 	.max_linewidth = 2400,
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
index ce583eb14b06..c990406e4bca 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
@@ -15,6 +15,7 @@ static const struct dpu_caps sc8280xp_dpu_caps = {
 	.has_dim_layer = true,
 	.has_idle_pc = true,
 	.has_3d_merge = true,
+	.has_data_compress = true,
 	.max_linewidth = 5120,
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
index 3950e7b946a5..7094640e2fbf 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
@@ -15,6 +15,7 @@ static const struct dpu_caps sm8450_dpu_caps = {
 	.has_dim_layer = true,
 	.has_idle_pc = true,
 	.has_3d_merge = true,
+	.has_data_compress = true,
 	.max_linewidth = 5120,
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
index 1b3f5424aea8..970049559e02 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
@@ -15,6 +15,7 @@ static const struct dpu_caps sm8550_dpu_caps = {
 	.has_dim_layer = true,
 	.has_idle_pc = true,
 	.has_3d_merge = true,
+	.has_data_compress = true,
 	.max_linewidth = 5120,
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index b410a85c109c..c5bbd4ad6da8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -380,6 +380,7 @@ struct dpu_rotation_cfg {
  * @has_dim_layer      dim layer feature status
  * @has_idle_pc        indicate if idle power collapse feature is supported
  * @has_3d_merge       indicate if 3D merge is supported
+ * @has_data_compress  indicate if data compression is supported
  * @max_linewidth      max linewidth for sspp
  * @pixel_ram_size     size of latency hiding and de-tiling buffer in bytes
  * @max_hdeci_exp      max horizontal decimation supported (max is 2^value)
@@ -393,6 +394,7 @@ struct dpu_caps {
 	bool has_dim_layer;
 	bool has_idle_pc;
 	bool has_3d_merge;
+	bool has_data_compress;
 	/* SSPP limits */
 	u32 max_linewidth;
 	u32 pixel_ram_size;

-- 
2.40.1

[PATCH 4/4] drm/msm/dpu: Enable compression for command mode

[Jessica Zhang]

Add a dpu_hw_intf op to enable data compression.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
 3 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 74470d068622..4321a1aba17f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -72,6 +72,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
 				phys_enc->hw_intf,
 				true,
 				phys_enc->hw_pp->idx);
+
+	if (phys_enc->dpu_kms->catalog->caps->has_data_compress &&
+			phys_enc->hw_intf->ops.enable_compression)
+		phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
 }
 
 static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 671048a78801..4ce7ffdd7a05 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -64,10 +64,16 @@
 
 #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
 #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
+#define INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
 
 #define INTF_MISR_CTRL			0x180
 #define INTF_MISR_SIGNATURE		0x184
 
+static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
+{
+	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);
+}
+
 static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 		const struct intf_timing_params *p,
 		const struct dpu_format *fmt)
@@ -325,6 +331,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
 		ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
 	ops->setup_misr = dpu_hw_intf_setup_misr;
 	ops->collect_misr = dpu_hw_intf_collect_misr;
+	ops->enable_compression = dpu_hw_intf_enable_compression;
 }
 
 struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index 102c4f0e812b..99528c735368 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -60,6 +60,7 @@ struct intf_status {
  *                     feed pixels to this interface
  * @setup_misr: enable/disable MISR
  * @collect_misr: read MISR signature
+ * @enable_compression: Enable data compression
  */
 struct dpu_hw_intf_ops {
 	void (*setup_timing_gen)(struct dpu_hw_intf *intf,
@@ -82,6 +83,7 @@ struct dpu_hw_intf_ops {
 			const enum dpu_pingpong pp);
 	void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count);
 	int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value);
+	void (*enable_compression)(struct dpu_hw_intf *intf);
 };
 
 struct dpu_hw_intf {

-- 
2.40.1

Re: [PATCH 3/4] drm/msm/dpu: Add has_data_compress to dpu_caps

[Marijn Suijten]

On 2023-05-02 18:19:14, Jessica Zhang wrote:
> Add data_compress feature to DPU HW catalog.
> 
> In DPU 7.x and later, there is a DATA_COMPRESS register that must be set
> within the DPU INTF block for DSC to work.
> 
> As core_rev (and related macros) was removed from the dpu_kms struct, the
> most straightforward way to indicate the presence of this register would be
> to have a flag in dpu_caps.

This is a very generic name to have in the global dpu_caps for a very
specific register on the INTF block since DPU >= 7.0.0, and I doubt any
new catalog contributor will know how to fill this field.  After all,
DPU < 7.0.0 also has DCE but it is controlled via the PINGPONG block.

Instead, how about having it as a DPU_INTF_DATA_COMPRESS (or similar)
feature flag on the INTF block?  We do the same for other (register
related) features on the INTF block, and you did the same to disable DSC
callbacks on PP in [1].

In fact it seems that the DSC/DCE (enablement) registers have been moved
from PINGPONG to INTF in DPU 7.0.0.  Can you clarify in the patch
message for v2 that this is the case, and do the same in the linked
PINGPONG patch?  Perhaps these patches should be part of the same series
as they do not seem DSI-specific.

[1]: https://lore.kernel.org/linux-arm-msm/1683061382-32651-3-git-send-email-quic_khsieh@quicinc.com/

- Marijn

> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h   | 1 +
>  drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h   | 1 +
>  drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 1 +
>  drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h   | 1 +
>  drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h   | 1 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h           | 2 ++
>  6 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> index f98c2a5b0e87..4160a35ff20f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> @@ -15,6 +15,7 @@ static const struct dpu_caps sm8350_dpu_caps = {
>  	.has_dim_layer = true,
>  	.has_idle_pc = true,
>  	.has_3d_merge = true,
> +	.has_data_compress = true,
>  	.max_linewidth = 4096,
>  	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>  };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> index 3fd0498ab420..23230841a0d1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> @@ -13,6 +13,7 @@ static const struct dpu_caps sc7280_dpu_caps = {
>  	.qseed_type = DPU_SSPP_SCALER_QSEED4,
>  	.has_dim_layer = true,
>  	.has_idle_pc = true,
> +	.has_data_compress = true,
>  	.max_linewidth = 2400,
>  	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>  };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> index ce583eb14b06..c990406e4bca 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> @@ -15,6 +15,7 @@ static const struct dpu_caps sc8280xp_dpu_caps = {
>  	.has_dim_layer = true,
>  	.has_idle_pc = true,
>  	.has_3d_merge = true,
> +	.has_data_compress = true,
>  	.max_linewidth = 5120,
>  	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>  };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> index 3950e7b946a5..7094640e2fbf 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> @@ -15,6 +15,7 @@ static const struct dpu_caps sm8450_dpu_caps = {
>  	.has_dim_layer = true,
>  	.has_idle_pc = true,
>  	.has_3d_merge = true,
> +	.has_data_compress = true,
>  	.max_linewidth = 5120,
>  	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>  };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> index 1b3f5424aea8..970049559e02 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> @@ -15,6 +15,7 @@ static const struct dpu_caps sm8550_dpu_caps = {
>  	.has_dim_layer = true,
>  	.has_idle_pc = true,
>  	.has_3d_merge = true,
> +	.has_data_compress = true,
>  	.max_linewidth = 5120,
>  	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>  };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index b410a85c109c..c5bbd4ad6da8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -380,6 +380,7 @@ struct dpu_rotation_cfg {
>   * @has_dim_layer      dim layer feature status
>   * @has_idle_pc        indicate if idle power collapse feature is supported
>   * @has_3d_merge       indicate if 3D merge is supported
> + * @has_data_compress  indicate if data compression is supported
>   * @max_linewidth      max linewidth for sspp
>   * @pixel_ram_size     size of latency hiding and de-tiling buffer in bytes
>   * @max_hdeci_exp      max horizontal decimation supported (max is 2^value)
> @@ -393,6 +394,7 @@ struct dpu_caps {
>  	bool has_dim_layer;
>  	bool has_idle_pc;
>  	bool has_3d_merge;
> +	bool has_data_compress;
>  	/* SSPP limits */
>  	u32 max_linewidth;
>  	u32 pixel_ram_size;
> 
> -- 
> 2.40.1
> 

Re: [PATCH 4/4] drm/msm/dpu: Enable compression for command mode

[Marijn Suijten]

On 2023-05-02 18:19:15, Jessica Zhang wrote:
> Add a dpu_hw_intf op to enable data compression.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 74470d068622..4321a1aba17f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c

Can we have INTF DCE on video-mode encoders as well?

> @@ -72,6 +72,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>  				phys_enc->hw_intf,
>  				true,
>  				phys_enc->hw_pp->idx);
> +
> +	if (phys_enc->dpu_kms->catalog->caps->has_data_compress &&

As per my suggestion on patch 3/4, drop the flag and check above and
only check if the function is NULL (below).

> +			phys_enc->hw_intf->ops.enable_compression)
> +		phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>  }
>  
>  static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 671048a78801..4ce7ffdd7a05 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -64,10 +64,16 @@
>  
>  #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
>  #define INTF_CFG2_DATA_HCTL_EN	BIT(4)

These should probably be reindented to match the below... And the rest
of the defines use spaces instead of tabs.

> +#define INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
>  
>  #define INTF_MISR_CTRL			0x180
>  #define INTF_MISR_SIGNATURE		0x184

This does not seem to apply on top of:
https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/

>  
> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)

Why inline?  This is used as a pointer callback.

> +{
> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);

dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
double-buffered, or is that config **always** unused when DSI CMD mode
is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
the bitflag into the register, or write the whole thing at once in
dpu_hw_intf_setup_timing_engine()?

> +}
> +
>  static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>  		const struct intf_timing_params *p,
>  		const struct dpu_format *fmt)
> @@ -325,6 +331,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
>  		ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
>  	ops->setup_misr = dpu_hw_intf_setup_misr;
>  	ops->collect_misr = dpu_hw_intf_collect_misr;
> +	ops->enable_compression = dpu_hw_intf_enable_compression;

And per the same suggestion on patch 3/4, this is then wrapped in:

    if (cap & BIT(DPU_INTF_DATA_COMPRESS))

(or similary named) flag check.

>  }

This also doesn't seem to apply on top of the INTF TE [1] support
series, even though it depends on DSC 1.2 DPU support(s?) [2] which
mentions it was rebase(d) on top of that.

[1]: https://patchwork.freedesktop.org/series/112332/
[2]: https://patchwork.freedesktop.org/series/116789/

- Marijn

>  
>  struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index 102c4f0e812b..99528c735368 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -60,6 +60,7 @@ struct intf_status {
>   *                     feed pixels to this interface
>   * @setup_misr: enable/disable MISR
>   * @collect_misr: read MISR signature
> + * @enable_compression: Enable data compression
>   */
>  struct dpu_hw_intf_ops {
>  	void (*setup_timing_gen)(struct dpu_hw_intf *intf,
> @@ -82,6 +83,7 @@ struct dpu_hw_intf_ops {
>  			const enum dpu_pingpong pp);
>  	void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count);
>  	int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value);
> +	void (*enable_compression)(struct dpu_hw_intf *intf);
>  };
>  
>  struct dpu_hw_intf {
> 
> -- 
> 2.40.1
> 

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Dmitry Baryshkov]

On 03/05/2023 04:19, Jessica Zhang wrote:
> Currently, word count is calculated using slice_count. This is incorrect
> as downstream uses slice per packet, which is different from
> slice_count.
> 
> Slice count represents the number of soft slices per interface, and its
> value will not always match that of slice per packet. For example, it is
> possible to have cases where there are multiple soft slices per interface
> but the panel specifies only one slice per packet.
> 
> Thus, use the default value of one slice per packet and remove slice_count
> from the word count calculation.
> 
> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 35c69dbe5f6f..b0d448ffb078 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   		if (!msm_host->dsc)
>   			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>   		else
> -			wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
> +			/*
> +			 * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1.
> +			 * Currently, the driver only supports default value of slice_per_packet = 1
> +			 *
> +			 * TODO: Expand drm_panel struct to hold slice_per_packet info
> +			 *       and adjust DSC math to account for slice_per_packet.

slice_per_packet is not a part of the standard DSC, so I'm not sure how 
that can be implemented. And definitely we should not care about the 
drm_panel here. It should be either a part of drm_dsc_config, or 
mipi_dsi_device.

> +			 */
> +			wc = msm_host->dsc->slice_chunk_size + 1;
>   
>   		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>   			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression

[Dmitry Baryshkov]

On 03/05/2023 04:19, Jessica Zhang wrote:
> Divide the pclk rate by the compression ratio when DSC is enabled
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 43a5ec33eee8..35c69dbe5f6f 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -561,7 +561,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
>   	clk_disable_unprepare(msm_host->byte_clk);
>   }
>   
> -static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi)
> +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode,
> +		struct drm_dsc_config *dsc, bool is_bonded_dsi)
>   {
>   	unsigned long pclk_rate;
>   
> @@ -576,6 +577,11 @@ static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool
>   	if (is_bonded_dsi)
>   		pclk_rate /= 2;
>   
> +	/* If DSC is enabled, divide pclk by compression ratio */
> +	if (dsc)
> +		pclk_rate = DIV_ROUND_UP(pclk_rate,
> +				dsc->bits_per_component * 3 / msm_dsc_get_bpp_int(dsc));
> +

Don't we loose precision here?
Would DIV_ROUND_UP(pclk_rate * bpp, dsc->bpc * 3) be better?

>   	return pclk_rate;
>   }
>   
> @@ -585,7 +591,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
>   	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>   	u8 lanes = msm_host->lanes;
>   	u32 bpp = dsi_get_bpp(msm_host->format);
> -	unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
> +	unsigned long pclk_rate = dsi_get_pclk_rate(mode, msm_host->dsc, is_bonded_dsi);
>   	u64 pclk_bpp = (u64)pclk_rate * bpp;
>   
>   	if (lanes == 0) {
> @@ -604,7 +610,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
>   
>   static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   {
> -	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi);
> +	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi);
>   	msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi,
>   							msm_host->mode);
>   
> @@ -634,7 +640,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   
>   	dsi_calc_pclk(msm_host, is_bonded_dsi);
>   
> -	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
> +	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi) * bpp;
>   	do_div(pclk_bpp, 8);
>   	msm_host->src_clk_rate = pclk_bpp;
>   
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression

[Jessica Zhang]

On 5/3/2023 1:33 AM, Dmitry Baryshkov wrote:
> On 03/05/2023 04:19, Jessica Zhang wrote:
>> Divide the pclk rate by the compression ratio when DSC is enabled
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 43a5ec33eee8..35c69dbe5f6f 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -561,7 +561,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host 
>> *msm_host)
>>       clk_disable_unprepare(msm_host->byte_clk);
>>   }
>> -static unsigned long dsi_get_pclk_rate(const struct drm_display_mode 
>> *mode, bool is_bonded_dsi)
>> +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode 
>> *mode,
>> +        struct drm_dsc_config *dsc, bool is_bonded_dsi)
>>   {
>>       unsigned long pclk_rate;
>> @@ -576,6 +577,11 @@ static unsigned long dsi_get_pclk_rate(const 
>> struct drm_display_mode *mode, bool
>>       if (is_bonded_dsi)
>>           pclk_rate /= 2;
>> +    /* If DSC is enabled, divide pclk by compression ratio */
>> +    if (dsc)
>> +        pclk_rate = DIV_ROUND_UP(pclk_rate,
>> +                dsc->bits_per_component * 3 / msm_dsc_get_bpp_int(dsc));
>> +
> 
> Don't we loose precision here?
> Would DIV_ROUND_UP(pclk_rate * bpp, dsc->bpc * 3) be better?

Hi Dmitry,

Acked.

Thanks,

Jessica Zhang

> 
>>       return pclk_rate;
>>   }
>> @@ -585,7 +591,7 @@ unsigned long dsi_byte_clk_get_rate(struct 
>> mipi_dsi_host *host, bool is_bonded_d
>>       struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>>       u8 lanes = msm_host->lanes;
>>       u32 bpp = dsi_get_bpp(msm_host->format);
>> -    unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
>> +    unsigned long pclk_rate = dsi_get_pclk_rate(mode, msm_host->dsc, 
>> is_bonded_dsi);
>>       u64 pclk_bpp = (u64)pclk_rate * bpp;
>>       if (lanes == 0) {
>> @@ -604,7 +610,7 @@ unsigned long dsi_byte_clk_get_rate(struct 
>> mipi_dsi_host *host, bool is_bonded_d
>>   static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool 
>> is_bonded_dsi)
>>   {
>> -    msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, 
>> is_bonded_dsi);
>> +    msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, 
>> msm_host->dsc, is_bonded_dsi);
>>       msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, 
>> is_bonded_dsi,
>>                               msm_host->mode);
>> @@ -634,7 +640,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>       dsi_calc_pclk(msm_host, is_bonded_dsi);
>> -    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) 
>> * bpp;
>> +    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, 
>> is_bonded_dsi) * bpp;
>>       do_div(pclk_bpp, 8);
>>       msm_host->src_clk_rate = pclk_bpp;
>>
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Jessica Zhang]

On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
> On 03/05/2023 04:19, Jessica Zhang wrote:
>> Currently, word count is calculated using slice_count. This is incorrect
>> as downstream uses slice per packet, which is different from
>> slice_count.
>>
>> Slice count represents the number of soft slices per interface, and its
>> value will not always match that of slice per packet. For example, it is
>> possible to have cases where there are multiple soft slices per interface
>> but the panel specifies only one slice per packet.
>>
>> Thus, use the default value of one slice per packet and remove 
>> slice_count
>> from the word count calculation.
>>
>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to 
>> compute word count")
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 35c69dbe5f6f..b0d448ffb078 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>           if (!msm_host->dsc)
>>               wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>           else
>> -            wc = msm_host->dsc->slice_chunk_size * 
>> msm_host->dsc->slice_count + 1;
>> +            /*
>> +             * When DSC is enabled, WC = slice_chunk_size * 
>> slice_per_packet + 1.
>> +             * Currently, the driver only supports default value of 
>> slice_per_packet = 1
>> +             *
>> +             * TODO: Expand drm_panel struct to hold slice_per_packet 
>> info
>> +             *       and adjust DSC math to account for 
>> slice_per_packet.
> 
> slice_per_packet is not a part of the standard DSC, so I'm not sure how 
> that can be implemented. And definitely we should not care about the 
> drm_panel here. It should be either a part of drm_dsc_config, or 
> mipi_dsi_device.

Hi Dmitry,

IIRC slice per packet is given by the panel specs with the default value 
being 1 if no value is specified, so it might be better to have it as 
part of mipi_dsi_device.

Will update the TODO comment accordingly.

Thanks,

Jessica Zhang

> 
>> +             */
>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 3/4] drm/msm/dpu: Add has_data_compress to dpu_caps

[Jessica Zhang]

On 5/3/2023 12:07 AM, Marijn Suijten wrote:
> On 2023-05-02 18:19:14, Jessica Zhang wrote:
>> Add data_compress feature to DPU HW catalog.
>>
>> In DPU 7.x and later, there is a DATA_COMPRESS register that must be set
>> within the DPU INTF block for DSC to work.
>>
>> As core_rev (and related macros) was removed from the dpu_kms struct, the
>> most straightforward way to indicate the presence of this register would be
>> to have a flag in dpu_caps.
> 
> This is a very generic name to have in the global dpu_caps for a very
> specific register on the INTF block since DPU >= 7.0.0, and I doubt any
> new catalog contributor will know how to fill this field.  After all,
> DPU < 7.0.0 also has DCE but it is controlled via the PINGPONG block.
> 
> Instead, how about having it as a DPU_INTF_DATA_COMPRESS (or similar)
> feature flag on the INTF block?  We do the same for other (register
> related) features on the INTF block, and you did the same to disable DSC
> callbacks on PP in [1].

Hi Marijn,

Sounds good.

> 
> In fact it seems that the DSC/DCE (enablement) registers have been moved
> from PINGPONG to INTF in DPU 7.0.0.  Can you clarify in the patch
> message for v2 that this is the case, and do the same in the linked
> PINGPONG patch?  Perhaps these patches should be part of the same series
> as they do not seem DSI-specific.

Will make a note of the PP to INTF change in the commit message.

I would prefer to keep this patch in this series is because it is needed 
for DSI over command mode to work and the subsequent patch is 
specifically for command mode.

Thanks,

Jessica Zhang

> 
> [1]: https://lore.kernel.org/linux-arm-msm/1683061382-32651-3-git-send-email-quic_khsieh@quicinc.com/
> 
> - Marijn
> 
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h   | 1 +
>>   drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h   | 1 +
>>   drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 1 +
>>   drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h   | 1 +
>>   drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h   | 1 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h           | 2 ++
>>   6 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> index f98c2a5b0e87..4160a35ff20f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> @@ -15,6 +15,7 @@ static const struct dpu_caps sm8350_dpu_caps = {
>>   	.has_dim_layer = true,
>>   	.has_idle_pc = true,
>>   	.has_3d_merge = true,
>> +	.has_data_compress = true,
>>   	.max_linewidth = 4096,
>>   	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>   };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> index 3fd0498ab420..23230841a0d1 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> @@ -13,6 +13,7 @@ static const struct dpu_caps sc7280_dpu_caps = {
>>   	.qseed_type = DPU_SSPP_SCALER_QSEED4,
>>   	.has_dim_layer = true,
>>   	.has_idle_pc = true,
>> +	.has_data_compress = true,
>>   	.max_linewidth = 2400,
>>   	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>   };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> index ce583eb14b06..c990406e4bca 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> @@ -15,6 +15,7 @@ static const struct dpu_caps sc8280xp_dpu_caps = {
>>   	.has_dim_layer = true,
>>   	.has_idle_pc = true,
>>   	.has_3d_merge = true,
>> +	.has_data_compress = true,
>>   	.max_linewidth = 5120,
>>   	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>   };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> index 3950e7b946a5..7094640e2fbf 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> @@ -15,6 +15,7 @@ static const struct dpu_caps sm8450_dpu_caps = {
>>   	.has_dim_layer = true,
>>   	.has_idle_pc = true,
>>   	.has_3d_merge = true,
>> +	.has_data_compress = true,
>>   	.max_linewidth = 5120,
>>   	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>   };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> index 1b3f5424aea8..970049559e02 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> @@ -15,6 +15,7 @@ static const struct dpu_caps sm8550_dpu_caps = {
>>   	.has_dim_layer = true,
>>   	.has_idle_pc = true,
>>   	.has_3d_merge = true,
>> +	.has_data_compress = true,
>>   	.max_linewidth = 5120,
>>   	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>   };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> index b410a85c109c..c5bbd4ad6da8 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -380,6 +380,7 @@ struct dpu_rotation_cfg {
>>    * @has_dim_layer      dim layer feature status
>>    * @has_idle_pc        indicate if idle power collapse feature is supported
>>    * @has_3d_merge       indicate if 3D merge is supported
>> + * @has_data_compress  indicate if data compression is supported
>>    * @max_linewidth      max linewidth for sspp
>>    * @pixel_ram_size     size of latency hiding and de-tiling buffer in bytes
>>    * @max_hdeci_exp      max horizontal decimation supported (max is 2^value)
>> @@ -393,6 +394,7 @@ struct dpu_caps {
>>   	bool has_dim_layer;
>>   	bool has_idle_pc;
>>   	bool has_3d_merge;
>> +	bool has_data_compress;
>>   	/* SSPP limits */
>>   	u32 max_linewidth;
>>   	u32 pixel_ram_size;
>>
>> -- 
>> 2.40.1
>>

Re: [PATCH 4/4] drm/msm/dpu: Enable compression for command mode

[Jessica Zhang]

On 5/3/2023 12:28 AM, Marijn Suijten wrote:
> On 2023-05-02 18:19:15, Jessica Zhang wrote:
>> Add a dpu_hw_intf op to enable data compression.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>>   3 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index 74470d068622..4321a1aba17f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> 
> Can we have INTF DCE on video-mode encoders as well?

Hi Marijn,

Currently, there's no way to validate DSC for video mode as I've only 
made changes to support DSI for command mode. We are planning to post 
changes to support DSC over DP, which will include changes for video mode.

> 
>> @@ -72,6 +72,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>   				phys_enc->hw_intf,
>>   				true,
>>   				phys_enc->hw_pp->idx);
>> +
>> +	if (phys_enc->dpu_kms->catalog->caps->has_data_compress &&
> 
> As per my suggestion on patch 3/4, drop the flag and check above and
> only check if the function is NULL (below).

Acked.

> 
>> +			phys_enc->hw_intf->ops.enable_compression)
>> +		phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>>   }
>>   
>>   static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> index 671048a78801..4ce7ffdd7a05 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -64,10 +64,16 @@
>>   
>>   #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
>>   #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
> 
> These should probably be reindented to match the below... And the rest
> of the defines use spaces instead of tabs.

Fair point, though I think fixing the whitespace for these 2 macros 
specifically might be better in a more relevant series.

With that being said, I'll change the spacing of the DATA_COMPRESS bit 
to spaces instead of tabs.

> 
>> +#define INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
>>   
>>   #define INTF_MISR_CTRL			0x180
>>   #define INTF_MISR_SIGNATURE		0x184
> 
> This does not seem to apply on top of:
> https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/

Seems like I'm missing some patches from that series on my working 
branch. Will rebase on top of the full series for the v2.

> 
>>   
>> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
> 
> Why inline?  This is used as a pointer callback.

Acked, will remove the inline.

> 
>> +{
>> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);
> 
> dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
> double-buffered, or is that config **always** unused when DSI CMD mode
> is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
> the bitflag into the register, or write the whole thing at once in
> dpu_hw_intf_setup_timing_engine()?

For command mode, INTF_CONFIG2 is unused aside from setting 
DATA_COMPRESS for DSC.

Since setup_timing_engine() is only used for video mode, the 
corresponding changes will be made in the DSC v1.2 for DP changes.

> 
>> +}
>> +
>>   static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>   		const struct intf_timing_params *p,
>>   		const struct dpu_format *fmt)
>> @@ -325,6 +331,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
>>   		ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
>>   	ops->setup_misr = dpu_hw_intf_setup_misr;
>>   	ops->collect_misr = dpu_hw_intf_collect_misr;
>> +	ops->enable_compression = dpu_hw_intf_enable_compression;
> 
> And per the same suggestion on patch 3/4, this is then wrapped in:
> 
>      if (cap & BIT(DPU_INTF_DATA_COMPRESS))
> 
> (or similary named) flag check.

Acked.

Thanks,

Jessica Zhang

> 
>>   }
> 
> This also doesn't seem to apply on top of the INTF TE [1] support
> series, even though it depends on DSC 1.2 DPU support(s?) [2] which
> mentions it was rebase(d) on top of that.
> 
> [1]: https://patchwork.freedesktop.org/series/112332/
> [2]: https://patchwork.freedesktop.org/series/116789/
> 
> - Marijn
> 
>>   
>>   struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> index 102c4f0e812b..99528c735368 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -60,6 +60,7 @@ struct intf_status {
>>    *                     feed pixels to this interface
>>    * @setup_misr: enable/disable MISR
>>    * @collect_misr: read MISR signature
>> + * @enable_compression: Enable data compression
>>    */
>>   struct dpu_hw_intf_ops {
>>   	void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>> @@ -82,6 +83,7 @@ struct dpu_hw_intf_ops {
>>   			const enum dpu_pingpong pp);
>>   	void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count);
>>   	int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value);
>> +	void (*enable_compression)(struct dpu_hw_intf *intf);
>>   };
>>   
>>   struct dpu_hw_intf {
>>
>> -- 
>> 2.40.1
>>

Re: [PATCH 4/4] drm/msm/dpu: Enable compression for command mode

[Dmitry Baryshkov]

On 03/05/2023 22:04, Jessica Zhang wrote:
> 
> 
> On 5/3/2023 12:28 AM, Marijn Suijten wrote:
>> On 2023-05-02 18:19:15, Jessica Zhang wrote:
>>> Add a dpu_hw_intf op to enable data compression.
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>>>   3 files changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> index 74470d068622..4321a1aba17f 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>
>> Can we have INTF DCE on video-mode encoders as well?
> 
> Hi Marijn,
> 
> Currently, there's no way to validate DSC for video mode as I've only 
> made changes to support DSI for command mode. We are planning to post 
> changes to support DSC over DP, which will include changes for video mode.

If I remember correctly, HDK8350 panel should support DSC for both 
command and video modes.

> 
>>
>>> @@ -72,6 +72,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>                   phys_enc->hw_intf,
>>>                   true,
>>>                   phys_enc->hw_pp->idx);
>>> +
>>> +    if (phys_enc->dpu_kms->catalog->caps->has_data_compress &&
>>
>> As per my suggestion on patch 3/4, drop the flag and check above and
>> only check if the function is NULL (below).
> 
> Acked.
> 
>>
>>> +            phys_enc->hw_intf->ops.enable_compression)
>>> +        phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>>>   }
>>>   static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int 
>>> irq_idx)
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> index 671048a78801..4ce7ffdd7a05 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> @@ -64,10 +64,16 @@
>>>   #define INTF_CFG2_DATABUS_WIDEN    BIT(0)
>>>   #define INTF_CFG2_DATA_HCTL_EN    BIT(4)
>>
>> These should probably be reindented to match the below... And the rest
>> of the defines use spaces instead of tabs.
> 
> Fair point, though I think fixing the whitespace for these 2 macros 
> specifically might be better in a more relevant series.
> 
> With that being said, I'll change the spacing of the DATA_COMPRESS bit 
> to spaces instead of tabs.
> 
>>
>>> +#define INTF_CFG2_DCE_DATA_COMPRESS    BIT(12)
>>>   #define INTF_MISR_CTRL            0x180
>>>   #define INTF_MISR_SIGNATURE        0x184
>>
>> This does not seem to apply on top of:
>> https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/
> 
> Seems like I'm missing some patches from that series on my working 
> branch. Will rebase on top of the full series for the v2.
> 
>>
>>> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf 
>>> *ctx)
>>
>> Why inline?  This is used as a pointer callback.
> 
> Acked, will remove the inline.
> 
>>
>>> +{
>>> +    DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);
>>
>> dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
>> double-buffered, or is that config **always** unused when DSI CMD mode
>> is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
>> the bitflag into the register, or write the whole thing at once in
>> dpu_hw_intf_setup_timing_engine()?
> 
> For command mode, INTF_CONFIG2 is unused aside from setting 
> DATA_COMPRESS for DSC.
> 
> Since setup_timing_engine() is only used for video mode, the 
> corresponding changes will be made in the DSC v1.2 for DP changes.

So, for command mode panels is this the only bit that should be set in 
INTF_CFG2?
-- 
With best wishes
Dmitry

Re: [PATCH 4/4] drm/msm/dpu: Enable compression for command mode

[Marijn Suijten]

Hi Jessica,

On 2023-05-03 12:04:59, Jessica Zhang wrote:
> 
> 
> On 5/3/2023 12:28 AM, Marijn Suijten wrote:
> > On 2023-05-02 18:19:15, Jessica Zhang wrote:
> >> Add a dpu_hw_intf op to enable data compression.
> >>
> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
> >>   3 files changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> >> index 74470d068622..4321a1aba17f 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > 
> > Can we have INTF DCE on video-mode encoders as well?
> 
> Hi Marijn,
> 
> Currently, there's no way to validate DSC for video mode as I've only 
> made changes to support DSI for command mode. We are planning to post 
> changes to support DSC over DP, which will include changes for video mode.

Okay, but then mention so in the patch description (which is rather
short in this revision).

<snip>

> >>   #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
> >>   #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
> > 
> > These should probably be reindented to match the below... And the rest
> > of the defines use spaces instead of tabs.
> 
> Fair point, though I think fixing the whitespace for these 2 macros 
> specifically might be better in a more relevant series.

Yes, I have many patches to start cleaning these up, as well as all the
broken kerneldoc comments, but it's an uphill battle.  Not sure if I'll
get to it any time soon if at all.

> With that being said, I'll change the spacing of the DATA_COMPRESS bit 
> to spaces instead of tabs.

Thanks, that seems to be the most common format.

> >> +#define INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
> >>   
> >>   #define INTF_MISR_CTRL			0x180
> >>   #define INTF_MISR_SIGNATURE		0x184
> > 
> > This does not seem to apply on top of:
> > https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/
> 
> Seems like I'm missing some patches from that series on my working 
> branch. Will rebase on top of the full series for the v2.

Thanks, but do discuss with Abhinav/Dmitry which series will land first.

> >> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
> > 
> > Why inline?  This is used as a pointer callback.
> 
> Acked, will remove the inline.
> 
> > 
> >> +{
> >> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);
> > 
> > dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
> > double-buffered, or is that config **always** unused when DSI CMD mode
> > is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
> > the bitflag into the register, or write the whole thing at once in
> > dpu_hw_intf_setup_timing_engine()?
> 
> For command mode, INTF_CONFIG2 is unused aside from setting 
> DATA_COMPRESS for DSC.
> 
> Since setup_timing_engine() is only used for video mode, the 
> corresponding changes will be made in the DSC v1.2 for DP changes.

Ack, that makes sense.  However, is this a guarantee that nothing else
will write INTF_CONFIG2 in the future, or will we solve that problem
when it happens?  I'm afraid more config-bits get added to this register
in the future and might possibly race/overwrite each other.

- Marijn

<snip>

Re: [PATCH 3/4] drm/msm/dpu: Add has_data_compress to dpu_caps

[Marijn Suijten]

Hi Jessica,

On 2023-05-03 12:03:40, Jessica Zhang wrote:
> 
> 
> On 5/3/2023 12:07 AM, Marijn Suijten wrote:
> > On 2023-05-02 18:19:14, Jessica Zhang wrote:
> >> Add data_compress feature to DPU HW catalog.
> >>
> >> In DPU 7.x and later, there is a DATA_COMPRESS register that must be set
> >> within the DPU INTF block for DSC to work.
> >>
> >> As core_rev (and related macros) was removed from the dpu_kms struct, the
> >> most straightforward way to indicate the presence of this register would be
> >> to have a flag in dpu_caps.
> > 
> > This is a very generic name to have in the global dpu_caps for a very
> > specific register on the INTF block since DPU >= 7.0.0, and I doubt any
> > new catalog contributor will know how to fill this field.  After all,
> > DPU < 7.0.0 also has DCE but it is controlled via the PINGPONG block.
> > 
> > Instead, how about having it as a DPU_INTF_DATA_COMPRESS (or similar)
> > feature flag on the INTF block?  We do the same for other (register
> > related) features on the INTF block, and you did the same to disable DSC
> > callbacks on PP in [1].

(Note: I said "you" but meant Kuogee)

> Hi Marijn,
> 
> Sounds good.
> 
> > 
> > In fact it seems that the DSC/DCE (enablement) registers have been moved
> > from PINGPONG to INTF in DPU 7.0.0.  Can you clarify in the patch
> > message for v2 that this is the case, and do the same in the linked
> > PINGPONG patch?  Perhaps these patches should be part of the same series
> > as they do not seem DSI-specific.
> 
> Will make a note of the PP to INTF change in the commit message.

Thanks.

> I would prefer to keep this patch in this series is because it is needed 
> for DSI over command mode to work and the subsequent patch is 
> specifically for command mode.

That is fine, but do mention this in the commit message if it is
relevant here.  Otherwise only mention it as part of patch 4/4.

- Marijn

Re: [PATCH 4/4] drm/msm/dpu: Enable compression for command mode

[Jessica Zhang]

On 5/3/2023 12:51 PM, Dmitry Baryshkov wrote:
> On 03/05/2023 22:04, Jessica Zhang wrote:
>>
>>
>> On 5/3/2023 12:28 AM, Marijn Suijten wrote:
>>> On 2023-05-02 18:19:15, Jessica Zhang wrote:
>>>> Add a dpu_hw_intf op to enable data compression.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>>>>   3 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> index 74470d068622..4321a1aba17f 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>
>>> Can we have INTF DCE on video-mode encoders as well?
>>
>> Hi Marijn,
>>
>> Currently, there's no way to validate DSC for video mode as I've only 
>> made changes to support DSI for command mode. We are planning to post 
>> changes to support DSC over DP, which will include changes for video 
>> mode.
> 
> If I remember correctly, HDK8350 panel should support DSC for both 
> command and video modes.

Hi Dmitry,

Correct, however we are planning to submit the video mode changes with 
the DP DSC v1.2 changes.

My current panel driver/dt changes are for command mode, so we would 
have to spent time to also add video mode support. It would be faster to 
land the video mode changes with DP support as that's already a work in 
progress.

> 
>>
>>>
>>>> @@ -72,6 +72,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>>                   phys_enc->hw_intf,
>>>>                   true,
>>>>                   phys_enc->hw_pp->idx);
>>>> +
>>>> +    if (phys_enc->dpu_kms->catalog->caps->has_data_compress &&
>>>
>>> As per my suggestion on patch 3/4, drop the flag and check above and
>>> only check if the function is NULL (below).
>>
>> Acked.
>>
>>>
>>>> +            phys_enc->hw_intf->ops.enable_compression)
>>>> +        phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>>>>   }
>>>>   static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int 
>>>> irq_idx)
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> index 671048a78801..4ce7ffdd7a05 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> @@ -64,10 +64,16 @@
>>>>   #define INTF_CFG2_DATABUS_WIDEN    BIT(0)
>>>>   #define INTF_CFG2_DATA_HCTL_EN    BIT(4)
>>>
>>> These should probably be reindented to match the below... And the rest
>>> of the defines use spaces instead of tabs.
>>
>> Fair point, though I think fixing the whitespace for these 2 macros 
>> specifically might be better in a more relevant series.
>>
>> With that being said, I'll change the spacing of the DATA_COMPRESS bit 
>> to spaces instead of tabs.
>>
>>>
>>>> +#define INTF_CFG2_DCE_DATA_COMPRESS    BIT(12)
>>>>   #define INTF_MISR_CTRL            0x180
>>>>   #define INTF_MISR_SIGNATURE        0x184
>>>
>>> This does not seem to apply on top of:
>>> https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/
>>
>> Seems like I'm missing some patches from that series on my working 
>> branch. Will rebase on top of the full series for the v2.
>>
>>>
>>>> +static inline void dpu_hw_intf_enable_compression(struct 
>>>> dpu_hw_intf *ctx)
>>>
>>> Why inline?  This is used as a pointer callback.
>>
>> Acked, will remove the inline.
>>
>>>
>>>> +{
>>>> +    DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, 
>>>> INTF_CFG2_DCE_DATA_COMPRESS);
>>>
>>> dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
>>> double-buffered, or is that config **always** unused when DSI CMD mode
>>> is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
>>> the bitflag into the register, or write the whole thing at once in
>>> dpu_hw_intf_setup_timing_engine()?
>>
>> For command mode, INTF_CONFIG2 is unused aside from setting 
>> DATA_COMPRESS for DSC.
>>
>> Since setup_timing_engine() is only used for video mode, the 
>> corresponding changes will be made in the DSC v1.2 for DP changes.
> 
> So, for command mode panels is this the only bit that should be set in 
> INTF_CFG2?

Yep, outside of the changes in this patch, INTF_CONFIG2 is only used in 
the video mode setup_timing_engine() method.

Thanks,

Jessica Zhang

> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 3/4] drm/msm/dpu: Add has_data_compress to dpu_caps

[Jessica Zhang]

On 5/3/2023 4:03 PM, Marijn Suijten wrote:
> Hi Jessica,
> 
> On 2023-05-03 12:03:40, Jessica Zhang wrote:
>>
>>
>> On 5/3/2023 12:07 AM, Marijn Suijten wrote:
>>> On 2023-05-02 18:19:14, Jessica Zhang wrote:
>>>> Add data_compress feature to DPU HW catalog.
>>>>
>>>> In DPU 7.x and later, there is a DATA_COMPRESS register that must be set
>>>> within the DPU INTF block for DSC to work.
>>>>
>>>> As core_rev (and related macros) was removed from the dpu_kms struct, the
>>>> most straightforward way to indicate the presence of this register would be
>>>> to have a flag in dpu_caps.
>>>
>>> This is a very generic name to have in the global dpu_caps for a very
>>> specific register on the INTF block since DPU >= 7.0.0, and I doubt any
>>> new catalog contributor will know how to fill this field.  After all,
>>> DPU < 7.0.0 also has DCE but it is controlled via the PINGPONG block.
>>>
>>> Instead, how about having it as a DPU_INTF_DATA_COMPRESS (or similar)
>>> feature flag on the INTF block?  We do the same for other (register
>>> related) features on the INTF block, and you did the same to disable DSC
>>> callbacks on PP in [1].
> 
> (Note: I said "you" but meant Kuogee)
> 
>> Hi Marijn,
>>
>> Sounds good.
>>
>>>
>>> In fact it seems that the DSC/DCE (enablement) registers have been moved
>>> from PINGPONG to INTF in DPU 7.0.0.  Can you clarify in the patch
>>> message for v2 that this is the case, and do the same in the linked
>>> PINGPONG patch?  Perhaps these patches should be part of the same series
>>> as they do not seem DSI-specific.
>>
>> Will make a note of the PP to INTF change in the commit message.
> 
> Thanks.
> 
>> I would prefer to keep this patch in this series is because it is needed
>> for DSI over command mode to work and the subsequent patch is
>> specifically for command mode.
> 
> That is fine, but do mention this in the commit message if it is
> relevant here.  Otherwise only mention it as part of patch 4/4.

Acked.

Thanks,

Jessica Zhang

> 
> - Marijn

Re: [PATCH 4/4] drm/msm/dpu: Enable compression for command mode

[Jessica Zhang]

On 5/3/2023 4:00 PM, Marijn Suijten wrote:
> Hi Jessica,
> 
> On 2023-05-03 12:04:59, Jessica Zhang wrote:
>>
>>
>> On 5/3/2023 12:28 AM, Marijn Suijten wrote:
>>> On 2023-05-02 18:19:15, Jessica Zhang wrote:
>>>> Add a dpu_hw_intf op to enable data compression.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>>>>    3 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> index 74470d068622..4321a1aba17f 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>
>>> Can we have INTF DCE on video-mode encoders as well?
>>
>> Hi Marijn,
>>
>> Currently, there's no way to validate DSC for video mode as I've only
>> made changes to support DSI for command mode. We are planning to post
>> changes to support DSC over DP, which will include changes for video mode.
> 
> Okay, but then mention so in the patch description (which is rather
> short in this revision).

Acked.

> 
> <snip>
> 
>>>>    #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
>>>>    #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
>>>
>>> These should probably be reindented to match the below... And the rest
>>> of the defines use spaces instead of tabs.
>>
>> Fair point, though I think fixing the whitespace for these 2 macros
>> specifically might be better in a more relevant series.
> 
> Yes, I have many patches to start cleaning these up, as well as all the
> broken kerneldoc comments, but it's an uphill battle.  Not sure if I'll
> get to it any time soon if at all.
> 
>> With that being said, I'll change the spacing of the DATA_COMPRESS bit
>> to spaces instead of tabs.
> 
> Thanks, that seems to be the most common format.
> 
>>>> +#define INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
>>>>    
>>>>    #define INTF_MISR_CTRL			0x180
>>>>    #define INTF_MISR_SIGNATURE		0x184
>>>
>>> This does not seem to apply on top of:
>>> https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/
>>
>> Seems like I'm missing some patches from that series on my working
>> branch. Will rebase on top of the full series for the v2.
> 
> Thanks, but do discuss with Abhinav/Dmitry which series will land first.
> 
>>>> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>>>
>>> Why inline?  This is used as a pointer callback.
>>
>> Acked, will remove the inline.
>>
>>>
>>>> +{
>>>> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);
>>>
>>> dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
>>> double-buffered, or is that config **always** unused when DSI CMD mode
>>> is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
>>> the bitflag into the register, or write the whole thing at once in
>>> dpu_hw_intf_setup_timing_engine()?
>>
>> For command mode, INTF_CONFIG2 is unused aside from setting
>> DATA_COMPRESS for DSC.
>>
>> Since setup_timing_engine() is only used for video mode, the
>> corresponding changes will be made in the DSC v1.2 for DP changes.
> 
> Ack, that makes sense.  However, is this a guarantee that nothing else
> will write INTF_CONFIG2 in the future, or will we solve that problem
> when it happens?  I'm afraid more config-bits get added to this register
> in the future and might possibly race/overwrite each other.

That's a fair point. There's no guarantee that nothing else will set 
INTF_CONFIG2 for command mode in the future. I think it would be better 
to add a register read now instead of having to fix that issue in a 
future change.

Thanks,

Jessica Zhang

> 
> - Marijn
> 
> <snip>

Re: [PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression

[Marijn Suijten]

Title suggestion: use the wording "reduce pclk rate" :)

(Eventually "when DSC is enabled", instead of "for compression")

On 2023-05-02 18:19:12, Jessica Zhang wrote:
> Divide the pclk rate by the compression ratio when DSC is enabled
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Thank you so much for sending this.   The compression ratio was applied
to hdisplay, but not the clocks yet, and with this patch I get a massive
reduction in clock speeds on the Xperia XZ3, without regressions nor
affecting performance/fps:

          gcc_sys_noc_cpuss_ahb_clk       1        1        0    19200000          0     0  50000         Y
          gcc_cpuss_ahb_clk           1        1        0    19200000          0     0  50000         Y
    bi_tcxo                           6        6        0    19200000          0     0  50000         Y
       dsi0vco_clk                    1        1        0  [-1873793994-]{+1249195898+}          0     0  50000         Y
          dsi0_pll_out_div_clk        1        1        0   [-1873793994-]{+624597949+}          0     0  50000         Y
             dsi0_pll_post_out_div_clk       0        0        0   [-468448498-]{+156149487+}          0     0  50000         Y
             dsi0_pll_bit_clk         2        2        0   [-1873793994-]{+624597949+}          0     0  50000         Y
                dsi0_pclk_mux         1        1        0   [-1873793994-]{+624597949+}          0     0  50000         Y
                   dsi0_phy_pll_out_dsiclk       1        1        0   [-312298999-]{+104099659+}          0     0  50000         Y
                      disp_cc_mdss_pclk0_clk_src       1        1        0   [-312298999-]{+104099659+}          0     0  50000         Y
                         disp_cc_mdss_pclk0_clk       1        1        0   [-312298999-]{+104099659+}          0     0  50000         Y
                dsi0_pll_by_2_bit_clk       0        0        0   [-936896997-]{+312298974+}          0     0  50000         Y
                dsi0_phy_pll_out_byteclk       1        1        0    [-234224249-]{+78074743+}          0     0  50000         Y
                   disp_cc_mdss_byte0_clk_src       2        2        0    [-234224249-]{+78074743+}          0     0  50000         Y
                      disp_cc_mdss_byte0_div_clk_src       1        1        0    [-117112125-]{+39037372+}          0     0  50000         Y
                         disp_cc_mdss_byte0_intf_clk       1        1        0    [-117112125-]{+39037372+}          0     0  50000         Y
                      disp_cc_mdss_byte0_clk       1        1        0    [-234224249-]{+78074743+}          0     0  50000         Y
       gpu_cc_pll1                    0        0        0   500000097          0     0  50000         N
       disp_cc_mdss_dp_pixel_clk_src       0        0        0    19200000          0     0  50000         N
          disp_cc_mdss_dp_pixel_clk       0        0        0    19200000          0     0  50000         N

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 43a5ec33eee8..35c69dbe5f6f 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -561,7 +561,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
>  	clk_disable_unprepare(msm_host->byte_clk);
>  }
>  
> -static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi)
> +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode,

It is a bit unfortunate that this function is called so often with the
same parameters, doing the same calculation over and over.

> +		struct drm_dsc_config *dsc, bool is_bonded_dsi)
>  {
>  	unsigned long pclk_rate;
>  
> @@ -576,6 +577,11 @@ static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool
>  	if (is_bonded_dsi)
>  		pclk_rate /= 2;
>  
> +	/* If DSC is enabled, divide pclk by compression ratio */
> +	if (dsc)
> +		pclk_rate = DIV_ROUND_UP(pclk_rate,
> +				dsc->bits_per_component * 3 / msm_dsc_get_bpp_int(dsc));

Don't forget to mention that this series depends on the DSC helpers.  I
don't think the linked DSC 1.2 series depends on it (at least it doesn't
mention it):

https://lore.kernel.org/linux-arm-msm/20230329-rfc-msm-dsc-helper-v6-2-cb7f59f0f7fb@quicinc.com/

- Marijn

> +
>  	return pclk_rate;
>  }
>  
> @@ -585,7 +591,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
>  	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>  	u8 lanes = msm_host->lanes;
>  	u32 bpp = dsi_get_bpp(msm_host->format);
> -	unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
> +	unsigned long pclk_rate = dsi_get_pclk_rate(mode, msm_host->dsc, is_bonded_dsi);
>  	u64 pclk_bpp = (u64)pclk_rate * bpp;
>  
>  	if (lanes == 0) {
> @@ -604,7 +610,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
>  
>  static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  {
> -	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi);
> +	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi);
>  	msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi,
>  							msm_host->mode);
>  
> @@ -634,7 +640,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  
>  	dsi_calc_pclk(msm_host, is_bonded_dsi);
>  
> -	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
> +	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi) * bpp;
>  	do_div(pclk_bpp, 8);
>  	msm_host->src_clk_rate = pclk_bpp;
>  
> 
> -- 
> 2.40.1
> 

Re: [PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression

[Marijn Suijten]

On 2023-05-04 22:33:17, Marijn Suijten wrote:
> Title suggestion: use the wording "reduce pclk rate" :)
> 
> (Eventually "when DSC is enabled", instead of "for compression")
> 
> On 2023-05-02 18:19:12, Jessica Zhang wrote:
> > Divide the pclk rate by the compression ratio when DSC is enabled
> > 
> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> 
> Thank you so much for sending this.   The compression ratio was applied
> to hdisplay

In hindsight, on the note of hdisplay, dsi_timing_setup() actually only
divides the visual portion - that is hdisplay out of htotal - without
affecting the back and front porch.

Since this clock inside the mode is based on the full htotal * vtotal *
..., should we compensate for that and only divide the visual portion of
the clock signal by 3?  Otherwise we might not have enough clockticks to
perform the front and back porch (even though CMD mode doesn't really
have porches, I have yet to properly understand that part of the
signal).

- Marijn

> , but not the clocks yet, and with this patch I get a massive
> reduction in clock speeds on the Xperia XZ3, without regressions nor
> affecting performance/fps:
> 
>           gcc_sys_noc_cpuss_ahb_clk       1        1        0    19200000          0     0  50000         Y
>           gcc_cpuss_ahb_clk           1        1        0    19200000          0     0  50000         Y
>     bi_tcxo                           6        6        0    19200000          0     0  50000         Y
>        dsi0vco_clk                    1        1        0  [-1873793994-]{+1249195898+}          0     0  50000         Y
>           dsi0_pll_out_div_clk        1        1        0   [-1873793994-]{+624597949+}          0     0  50000         Y
>              dsi0_pll_post_out_div_clk       0        0        0   [-468448498-]{+156149487+}          0     0  50000         Y
>              dsi0_pll_bit_clk         2        2        0   [-1873793994-]{+624597949+}          0     0  50000         Y
>                 dsi0_pclk_mux         1        1        0   [-1873793994-]{+624597949+}          0     0  50000         Y
>                    dsi0_phy_pll_out_dsiclk       1        1        0   [-312298999-]{+104099659+}          0     0  50000         Y
>                       disp_cc_mdss_pclk0_clk_src       1        1        0   [-312298999-]{+104099659+}          0     0  50000         Y
>                          disp_cc_mdss_pclk0_clk       1        1        0   [-312298999-]{+104099659+}          0     0  50000         Y
>                 dsi0_pll_by_2_bit_clk       0        0        0   [-936896997-]{+312298974+}          0     0  50000         Y
>                 dsi0_phy_pll_out_byteclk       1        1        0    [-234224249-]{+78074743+}          0     0  50000         Y
>                    disp_cc_mdss_byte0_clk_src       2        2        0    [-234224249-]{+78074743+}          0     0  50000         Y
>                       disp_cc_mdss_byte0_div_clk_src       1        1        0    [-117112125-]{+39037372+}          0     0  50000         Y
>                          disp_cc_mdss_byte0_intf_clk       1        1        0    [-117112125-]{+39037372+}          0     0  50000         Y
>                       disp_cc_mdss_byte0_clk       1        1        0    [-234224249-]{+78074743+}          0     0  50000         Y
>        gpu_cc_pll1                    0        0        0   500000097          0     0  50000         N
>        disp_cc_mdss_dp_pixel_clk_src       0        0        0    19200000          0     0  50000         N
>           disp_cc_mdss_dp_pixel_clk       0        0        0    19200000          0     0  50000         N
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> > ---
> >  drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 43a5ec33eee8..35c69dbe5f6f 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -561,7 +561,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
> >  	clk_disable_unprepare(msm_host->byte_clk);
> >  }
> >  
> > -static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi)
> > +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode,
> 
> It is a bit unfortunate that this function is called so often with the
> same parameters, doing the same calculation over and over.
> 
> > +		struct drm_dsc_config *dsc, bool is_bonded_dsi)
> >  {
> >  	unsigned long pclk_rate;
> >  
> > @@ -576,6 +577,11 @@ static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool
> >  	if (is_bonded_dsi)
> >  		pclk_rate /= 2;
> >  
> > +	/* If DSC is enabled, divide pclk by compression ratio */
> > +	if (dsc)
> > +		pclk_rate = DIV_ROUND_UP(pclk_rate,
> > +				dsc->bits_per_component * 3 / msm_dsc_get_bpp_int(dsc));
> 
> Don't forget to mention that this series depends on the DSC helpers.  I
> don't think the linked DSC 1.2 series depends on it (at least it doesn't
> mention it):
> 
> https://lore.kernel.org/linux-arm-msm/20230329-rfc-msm-dsc-helper-v6-2-cb7f59f0f7fb@quicinc.com/
> 
> - Marijn
> 
> > +
> >  	return pclk_rate;
> >  }
> >  
> > @@ -585,7 +591,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
> >  	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> >  	u8 lanes = msm_host->lanes;
> >  	u32 bpp = dsi_get_bpp(msm_host->format);
> > -	unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
> > +	unsigned long pclk_rate = dsi_get_pclk_rate(mode, msm_host->dsc, is_bonded_dsi);
> >  	u64 pclk_bpp = (u64)pclk_rate * bpp;
> >  
> >  	if (lanes == 0) {
> > @@ -604,7 +610,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
> >  
> >  static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >  {
> > -	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi);
> > +	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi);
> >  	msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi,
> >  							msm_host->mode);
> >  
> > @@ -634,7 +640,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >  
> >  	dsi_calc_pclk(msm_host, is_bonded_dsi);
> >  
> > -	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
> > +	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi) * bpp;
> >  	do_div(pclk_bpp, 8);
> >  	msm_host->src_clk_rate = pclk_bpp;
> >  
> > 
> > -- 
> > 2.40.1
> > 

Re: [PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression

[Jessica Zhang]

On 5/4/2023 2:17 PM, Marijn Suijten wrote:
> On 2023-05-04 22:33:17, Marijn Suijten wrote:
>> Title suggestion: use the wording "reduce pclk rate" :)
>>
>> (Eventually "when DSC is enabled", instead of "for compression")
>>
>> On 2023-05-02 18:19:12, Jessica Zhang wrote:
>>> Divide the pclk rate by the compression ratio when DSC is enabled
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>
>> Thank you so much for sending this.   The compression ratio was applied
>> to hdisplay
> 
> In hindsight, on the note of hdisplay, dsi_timing_setup() actually only
> divides the visual portion - that is hdisplay out of htotal - without
> affecting the back and front porch.
> 
> Since this clock inside the mode is based on the full htotal * vtotal *
> ..., should we compensate for that and only divide the visual portion of
> the clock signal by 3?  Otherwise we might not have enough clockticks to
> perform the front and back porch (even though CMD mode doesn't really
> have porches, I have yet to properly understand that part of the
> signal).

Hi Marijn,

That's a fair point. Will change the pclk math accordingly.

Thanks,

Jessica Zhang

> 
> - Marijn
> 
>> , but not the clocks yet, and with this patch I get a massive
>> reduction in clock speeds on the Xperia XZ3, without regressions nor
>> affecting performance/fps:
>>
>>            gcc_sys_noc_cpuss_ahb_clk       1        1        0    19200000          0     0  50000         Y
>>            gcc_cpuss_ahb_clk           1        1        0    19200000          0     0  50000         Y
>>      bi_tcxo                           6        6        0    19200000          0     0  50000         Y
>>         dsi0vco_clk                    1        1        0  [-1873793994-]{+1249195898+}          0     0  50000         Y
>>            dsi0_pll_out_div_clk        1        1        0   [-1873793994-]{+624597949+}          0     0  50000         Y
>>               dsi0_pll_post_out_div_clk       0        0        0   [-468448498-]{+156149487+}          0     0  50000         Y
>>               dsi0_pll_bit_clk         2        2        0   [-1873793994-]{+624597949+}          0     0  50000         Y
>>                  dsi0_pclk_mux         1        1        0   [-1873793994-]{+624597949+}          0     0  50000         Y
>>                     dsi0_phy_pll_out_dsiclk       1        1        0   [-312298999-]{+104099659+}          0     0  50000         Y
>>                        disp_cc_mdss_pclk0_clk_src       1        1        0   [-312298999-]{+104099659+}          0     0  50000         Y
>>                           disp_cc_mdss_pclk0_clk       1        1        0   [-312298999-]{+104099659+}          0     0  50000         Y
>>                  dsi0_pll_by_2_bit_clk       0        0        0   [-936896997-]{+312298974+}          0     0  50000         Y
>>                  dsi0_phy_pll_out_byteclk       1        1        0    [-234224249-]{+78074743+}          0     0  50000         Y
>>                     disp_cc_mdss_byte0_clk_src       2        2        0    [-234224249-]{+78074743+}          0     0  50000         Y
>>                        disp_cc_mdss_byte0_div_clk_src       1        1        0    [-117112125-]{+39037372+}          0     0  50000         Y
>>                           disp_cc_mdss_byte0_intf_clk       1        1        0    [-117112125-]{+39037372+}          0     0  50000         Y
>>                        disp_cc_mdss_byte0_clk       1        1        0    [-234224249-]{+78074743+}          0     0  50000         Y
>>         gpu_cc_pll1                    0        0        0   500000097          0     0  50000         N
>>         disp_cc_mdss_dp_pixel_clk_src       0        0        0    19200000          0     0  50000         N
>>            disp_cc_mdss_dp_pixel_clk       0        0        0    19200000          0     0  50000         N
>>
>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 43a5ec33eee8..35c69dbe5f6f 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -561,7 +561,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
>>>   	clk_disable_unprepare(msm_host->byte_clk);
>>>   }
>>>   
>>> -static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi)
>>> +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode,
>>
>> It is a bit unfortunate that this function is called so often with the
>> same parameters, doing the same calculation over and over.
>>
>>> +		struct drm_dsc_config *dsc, bool is_bonded_dsi)
>>>   {
>>>   	unsigned long pclk_rate;
>>>   
>>> @@ -576,6 +577,11 @@ static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool
>>>   	if (is_bonded_dsi)
>>>   		pclk_rate /= 2;
>>>   
>>> +	/* If DSC is enabled, divide pclk by compression ratio */
>>> +	if (dsc)
>>> +		pclk_rate = DIV_ROUND_UP(pclk_rate,
>>> +				dsc->bits_per_component * 3 / msm_dsc_get_bpp_int(dsc));
>>
>> Don't forget to mention that this series depends on the DSC helpers.  I
>> don't think the linked DSC 1.2 series depends on it (at least it doesn't
>> mention it):
>>
>> https://lore.kernel.org/linux-arm-msm/20230329-rfc-msm-dsc-helper-v6-2-cb7f59f0f7fb@quicinc.com/
>>
>> - Marijn
>>
>>> +
>>>   	return pclk_rate;
>>>   }
>>>   
>>> @@ -585,7 +591,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
>>>   	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>>>   	u8 lanes = msm_host->lanes;
>>>   	u32 bpp = dsi_get_bpp(msm_host->format);
>>> -	unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
>>> +	unsigned long pclk_rate = dsi_get_pclk_rate(mode, msm_host->dsc, is_bonded_dsi);
>>>   	u64 pclk_bpp = (u64)pclk_rate * bpp;
>>>   
>>>   	if (lanes == 0) {
>>> @@ -604,7 +610,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
>>>   
>>>   static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>   {
>>> -	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi);
>>> +	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi);
>>>   	msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi,
>>>   							msm_host->mode);
>>>   
>>> @@ -634,7 +640,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>   
>>>   	dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>   
>>> -	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
>>> +	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi) * bpp;
>>>   	do_div(pclk_bpp, 8);
>>>   	msm_host->src_clk_rate = pclk_bpp;
>>>   
>>>
>>> -- 
>>> 2.40.1
>>>

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Abhinav Kumar]

On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
> On 03/05/2023 04:19, Jessica Zhang wrote:
>> Currently, word count is calculated using slice_count. This is incorrect
>> as downstream uses slice per packet, which is different from
>> slice_count.
>>
>> Slice count represents the number of soft slices per interface, and its
>> value will not always match that of slice per packet. For example, it is
>> possible to have cases where there are multiple soft slices per interface
>> but the panel specifies only one slice per packet.
>>
>> Thus, use the default value of one slice per packet and remove 
>> slice_count
>> from the word count calculation.
>>
>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to 
>> compute word count")
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 35c69dbe5f6f..b0d448ffb078 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>           if (!msm_host->dsc)
>>               wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>           else
>> -            wc = msm_host->dsc->slice_chunk_size * 
>> msm_host->dsc->slice_count + 1;
>> +            /*
>> +             * When DSC is enabled, WC = slice_chunk_size * 
>> slice_per_packet + 1.
>> +             * Currently, the driver only supports default value of 
>> slice_per_packet = 1
>> +             *
>> +             * TODO: Expand drm_panel struct to hold slice_per_packet 
>> info
>> +             *       and adjust DSC math to account for 
>> slice_per_packet.
> 
> slice_per_packet is not a part of the standard DSC, so I'm not sure how 
> that can be implemented. And definitely we should not care about the 
> drm_panel here. It should be either a part of drm_dsc_config, or 
> mipi_dsi_device.
>

This is not correct.

It is part of the DSI standard (not DSC standard). Please refer to 
Figure 40 "One Line Containing One Packet with Data from One or More 
Compressed Slices" and Figure 41 "One Line Containing More than One 
Compressed Pixel Stream Packet".

This has details about this. So I still stand by my point that this 
should be in the drm_panel.

>> +             */
>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>
> 

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Dmitry Baryshkov]

On 08/05/2023 23:09, Abhinav Kumar wrote:
> 
> 
> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
>> On 03/05/2023 04:19, Jessica Zhang wrote:
>>> Currently, word count is calculated using slice_count. This is incorrect
>>> as downstream uses slice per packet, which is different from
>>> slice_count.
>>>
>>> Slice count represents the number of soft slices per interface, and its
>>> value will not always match that of slice per packet. For example, it is
>>> possible to have cases where there are multiple soft slices per 
>>> interface
>>> but the panel specifies only one slice per packet.
>>>
>>> Thus, use the default value of one slice per packet and remove 
>>> slice_count
>>> from the word count calculation.
>>>
>>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to 
>>> compute word count")
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 35c69dbe5f6f..b0d448ffb078 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct msm_dsi_host 
>>> *msm_host, bool is_bonded_dsi)
>>>           if (!msm_host->dsc)
>>>               wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>>           else
>>> -            wc = msm_host->dsc->slice_chunk_size * 
>>> msm_host->dsc->slice_count + 1;
>>> +            /*
>>> +             * When DSC is enabled, WC = slice_chunk_size * 
>>> slice_per_packet + 1.
>>> +             * Currently, the driver only supports default value of 
>>> slice_per_packet = 1
>>> +             *
>>> +             * TODO: Expand drm_panel struct to hold 
>>> slice_per_packet info
>>> +             *       and adjust DSC math to account for 
>>> slice_per_packet.
>>
>> slice_per_packet is not a part of the standard DSC, so I'm not sure 
>> how that can be implemented. And definitely we should not care about 
>> the drm_panel here. It should be either a part of drm_dsc_config, or 
>> mipi_dsi_device.
>>
> 
> This is not correct.
> 
> It is part of the DSI standard (not DSC standard). Please refer to 
> Figure 40 "One Line Containing One Packet with Data from One or More 
> Compressed Slices" and Figure 41 "One Line Containing More than One 
> Compressed Pixel Stream Packet".

I have reviewed section 8.8.24 and Annex D of the DSI standard.

It is not clear to me, if we can get away with always using 
slice_per_packet = 1. What is the DSI sink's difference between Fig. 
40.(b) and Fig 41?

Are there are known panels that require slice_per_packet != 1? If so, we 
will have to implement support for such configurations.

> This has details about this. So I still stand by my point that this 
> should be in the drm_panel.

Note, the driver doesn't use drm_panel directly. So slices_per_packet 
should go to mipi_dsi_device instead (which in turn can be filled from 
e.g. drm_panel or from any other source).

> 
>>> +             */
>>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>>
>>

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Abhinav Kumar]

On 5/8/2023 4:27 PM, Dmitry Baryshkov wrote:
> On 08/05/2023 23:09, Abhinav Kumar wrote:
>>
>>
>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
>>> On 03/05/2023 04:19, Jessica Zhang wrote:
>>>> Currently, word count is calculated using slice_count. This is 
>>>> incorrect
>>>> as downstream uses slice per packet, which is different from
>>>> slice_count.
>>>>
>>>> Slice count represents the number of soft slices per interface, and its
>>>> value will not always match that of slice per packet. For example, 
>>>> it is
>>>> possible to have cases where there are multiple soft slices per 
>>>> interface
>>>> but the panel specifies only one slice per packet.
>>>>
>>>> Thus, use the default value of one slice per packet and remove 
>>>> slice_count
>>>> from the word count calculation.
>>>>
>>>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to 
>>>> compute word count")
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index 35c69dbe5f6f..b0d448ffb078 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct 
>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>           if (!msm_host->dsc)
>>>>               wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>>>           else
>>>> -            wc = msm_host->dsc->slice_chunk_size * 
>>>> msm_host->dsc->slice_count + 1;
>>>> +            /*
>>>> +             * When DSC is enabled, WC = slice_chunk_size * 
>>>> slice_per_packet + 1.
>>>> +             * Currently, the driver only supports default value of 
>>>> slice_per_packet = 1
>>>> +             *
>>>> +             * TODO: Expand drm_panel struct to hold 
>>>> slice_per_packet info
>>>> +             *       and adjust DSC math to account for 
>>>> slice_per_packet.
>>>
>>> slice_per_packet is not a part of the standard DSC, so I'm not sure 
>>> how that can be implemented. And definitely we should not care about 
>>> the drm_panel here. It should be either a part of drm_dsc_config, or 
>>> mipi_dsi_device.
>>>
>>
>> This is not correct.
>>
>> It is part of the DSI standard (not DSC standard). Please refer to 
>> Figure 40 "One Line Containing One Packet with Data from One or More 
>> Compressed Slices" and Figure 41 "One Line Containing More than One 
>> Compressed Pixel Stream Packet".
> 
> I have reviewed section 8.8.24 and Annex D of the DSI standard.
> 
> It is not clear to me, if we can get away with always using 
> slice_per_packet = 1. What is the DSI sink's difference between Fig. 
> 40.(b) and Fig 41?
> 

The difference is that in fig 40(b) there is only one packet of data 
(check closely, there is only one header).

In fig 41, there are multiple headers so its showing multiple packets.

> Are there are known panels that require slice_per_packet != 1? If so, we 
> will have to implement support for such configurations.
> 

Unless explicitly requested by the panel, we can use 1. From the device 
tree files of the panels we support downstream, I do see 
qcom,mdss-dsc-slice-per-pkt set to 2 for some panels. I dont know 
whether those panels will not work with 1. I really don't think any of 
the DSC panels working with MSM were upstreamed.

I think the one jessica will be posting (and merging) will be the first 
and that works with 1.

If there are other panels in the works which require 2 slice_per_pkt, I 
would wait to first see them on the list and if they cannot work with 1 
slice_per_pkt, add support for that.

>> This has details about this. So I still stand by my point that this 
>> should be in the drm_panel.
> 
> Note, the driver doesn't use drm_panel directly. So slices_per_packet 
> should go to mipi_dsi_device instead (which in turn can be filled from 
> e.g. drm_panel or from any other source).
> 
>>
>>>> +             */
>>>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>>>
>>>
> 

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Dmitry Baryshkov]

On 09/05/2023 03:45, Abhinav Kumar wrote:
> 
> 
> On 5/8/2023 4:27 PM, Dmitry Baryshkov wrote:
>> On 08/05/2023 23:09, Abhinav Kumar wrote:
>>>
>>>
>>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
>>>> On 03/05/2023 04:19, Jessica Zhang wrote:
>>>>> Currently, word count is calculated using slice_count. This is 
>>>>> incorrect
>>>>> as downstream uses slice per packet, which is different from
>>>>> slice_count.
>>>>>
>>>>> Slice count represents the number of soft slices per interface, and 
>>>>> its
>>>>> value will not always match that of slice per packet. For example, 
>>>>> it is
>>>>> possible to have cases where there are multiple soft slices per 
>>>>> interface
>>>>> but the panel specifies only one slice per packet.
>>>>>
>>>>> Thus, use the default value of one slice per packet and remove 
>>>>> slice_count
>>>>> from the word count calculation.
>>>>>
>>>>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to 
>>>>> compute word count")
>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>> index 35c69dbe5f6f..b0d448ffb078 100644
>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct 
>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>           if (!msm_host->dsc)
>>>>>               wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>>>>           else
>>>>> -            wc = msm_host->dsc->slice_chunk_size * 
>>>>> msm_host->dsc->slice_count + 1;
>>>>> +            /*
>>>>> +             * When DSC is enabled, WC = slice_chunk_size * 
>>>>> slice_per_packet + 1.
>>>>> +             * Currently, the driver only supports default value 
>>>>> of slice_per_packet = 1
>>>>> +             *
>>>>> +             * TODO: Expand drm_panel struct to hold 
>>>>> slice_per_packet info
>>>>> +             *       and adjust DSC math to account for 
>>>>> slice_per_packet.
>>>>
>>>> slice_per_packet is not a part of the standard DSC, so I'm not sure 
>>>> how that can be implemented. And definitely we should not care about 
>>>> the drm_panel here. It should be either a part of drm_dsc_config, or 
>>>> mipi_dsi_device.
>>>>
>>>
>>> This is not correct.
>>>
>>> It is part of the DSI standard (not DSC standard). Please refer to 
>>> Figure 40 "One Line Containing One Packet with Data from One or More 
>>> Compressed Slices" and Figure 41 "One Line Containing More than One 
>>> Compressed Pixel Stream Packet".
>>
>> I have reviewed section 8.8.24 and Annex D of the DSI standard.
>>
>> It is not clear to me, if we can get away with always using 
>> slice_per_packet = 1. What is the DSI sink's difference between Fig. 
>> 40.(b) and Fig 41?
>>
> 
> The difference is that in fig 40(b) there is only one packet of data 
> (check closely, there is only one header).
> 
> In fig 41, there are multiple headers so its showing multiple packets.

Yes, this is the description of the pictures. I mean what is the 
functional difference?

> 
>> Are there are known panels that require slice_per_packet != 1? If so, 
>> we will have to implement support for such configurations.
>>
> 
> Unless explicitly requested by the panel, we can use 1. From the device 
> tree files of the panels we support downstream, I do see 
> qcom,mdss-dsc-slice-per-pkt set to 2 for some panels. I dont know 
> whether those panels will not work with 1. I really don't think any of 
> the DSC panels working with MSM were upstreamed.
> 
> I think the one jessica will be posting (and merging) will be the first 
> and that works with 1.
> 
> If there are other panels in the works which require 2 slice_per_pkt, I 
> would wait to first see them on the list and if they cannot work with 1 
> slice_per_pkt, add support for that.

If slice_per_pkt change is localized - touching only few lines in DSI or 
in msm_helpers. Otherwise we should consider having that from the 
beginning (but hardcoded to 1 for now).

> 
>>> This has details about this. So I still stand by my point that this 
>>> should be in the drm_panel.
>>
>> Note, the driver doesn't use drm_panel directly. So slices_per_packet 
>> should go to mipi_dsi_device instead (which in turn can be filled from 
>> e.g. drm_panel or from any other source).
>>
>>>
>>>>> +             */
>>>>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>>>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>>>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>>>>
>>>>
>>

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Abhinav Kumar]

On 5/8/2023 5:47 PM, Dmitry Baryshkov wrote:
> On 09/05/2023 03:45, Abhinav Kumar wrote:
>>
>>
>> On 5/8/2023 4:27 PM, Dmitry Baryshkov wrote:
>>> On 08/05/2023 23:09, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
>>>>> On 03/05/2023 04:19, Jessica Zhang wrote:
>>>>>> Currently, word count is calculated using slice_count. This is 
>>>>>> incorrect
>>>>>> as downstream uses slice per packet, which is different from
>>>>>> slice_count.
>>>>>>
>>>>>> Slice count represents the number of soft slices per interface, 
>>>>>> and its
>>>>>> value will not always match that of slice per packet. For example, 
>>>>>> it is
>>>>>> possible to have cases where there are multiple soft slices per 
>>>>>> interface
>>>>>> but the panel specifies only one slice per packet.
>>>>>>
>>>>>> Thus, use the default value of one slice per packet and remove 
>>>>>> slice_count
>>>>>> from the word count calculation.
>>>>>>
>>>>>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to 
>>>>>> compute word count")
>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> index 35c69dbe5f6f..b0d448ffb078 100644
>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct 
>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>           if (!msm_host->dsc)
>>>>>>               wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>>>>>           else
>>>>>> -            wc = msm_host->dsc->slice_chunk_size * 
>>>>>> msm_host->dsc->slice_count + 1;
>>>>>> +            /*
>>>>>> +             * When DSC is enabled, WC = slice_chunk_size * 
>>>>>> slice_per_packet + 1.
>>>>>> +             * Currently, the driver only supports default value 
>>>>>> of slice_per_packet = 1
>>>>>> +             *
>>>>>> +             * TODO: Expand drm_panel struct to hold 
>>>>>> slice_per_packet info
>>>>>> +             *       and adjust DSC math to account for 
>>>>>> slice_per_packet.
>>>>>
>>>>> slice_per_packet is not a part of the standard DSC, so I'm not sure 
>>>>> how that can be implemented. And definitely we should not care 
>>>>> about the drm_panel here. It should be either a part of 
>>>>> drm_dsc_config, or mipi_dsi_device.
>>>>>
>>>>
>>>> This is not correct.
>>>>
>>>> It is part of the DSI standard (not DSC standard). Please refer to 
>>>> Figure 40 "One Line Containing One Packet with Data from One or More 
>>>> Compressed Slices" and Figure 41 "One Line Containing More than One 
>>>> Compressed Pixel Stream Packet".
>>>
>>> I have reviewed section 8.8.24 and Annex D of the DSI standard.
>>>
>>> It is not clear to me, if we can get away with always using 
>>> slice_per_packet = 1. What is the DSI sink's difference between Fig. 
>>> 40.(b) and Fig 41?
>>>
>>
>> The difference is that in fig 40(b) there is only one packet of data 
>> (check closely, there is only one header).
>>
>> In fig 41, there are multiple headers so its showing multiple packets.
> 
> Yes, this is the description of the pictures. I mean what is the 
> functional difference?
> 

 From the same section of the spec:

"This is one method that can segregate slice-widths
when the receiver has multiple instantiations of decoders and this 
packet structure is used to identify slice-width boundaries"

 From this it seems like when there are multiple decoders then having 
individual multiple packets helps to identify the boundaries correctly 
due to the header/checksum boundaries. How exactly this works on the 
panel side, needs more investigation but outside the boundary of this 
discussion. But I think overall, unless really requested by the panel 
one slice_per_pkt is more optimal as it reduces packet overhead.

>>
>>> Are there are known panels that require slice_per_packet != 1? If so, 
>>> we will have to implement support for such configurations.
>>>
>>
>> Unless explicitly requested by the panel, we can use 1. From the 
>> device tree files of the panels we support downstream, I do see 
>> qcom,mdss-dsc-slice-per-pkt set to 2 for some panels. I dont know 
>> whether those panels will not work with 1. I really don't think any of 
>> the DSC panels working with MSM were upstreamed.
>>
>> I think the one jessica will be posting (and merging) will be the 
>> first and that works with 1.
>>
>> If there are other panels in the works which require 2 slice_per_pkt, 
>> I would wait to first see them on the list and if they cannot work 
>> with 1 slice_per_pkt, add support for that.
> 
> If slice_per_pkt change is localized - touching only few lines in DSI or 
> in msm_helpers. Otherwise we should consider having that from the 
> beginning (but hardcoded to 1 for now).
> 

Yes, from what I see downstream it touches only few lines in DSI and MSM 
helper. So should not be hard from MSM standpoint even in future, but 
the core changes would need agreement from the core maintainers.

>>
>>>> This has details about this. So I still stand by my point that this 
>>>> should be in the drm_panel.
>>>
>>> Note, the driver doesn't use drm_panel directly. So slices_per_packet 
>>> should go to mipi_dsi_device instead (which in turn can be filled 
>>> from e.g. drm_panel or from any other source).
>>>
>>>>
>>>>>> +             */
>>>>>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>>>>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>>>>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>>>>>
>>>>>
>>>
> 

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Konrad Dybcio]

On 9.05.2023 01:27, Dmitry Baryshkov wrote:
> On 08/05/2023 23:09, Abhinav Kumar wrote:
>>
>>
>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
>>> On 03/05/2023 04:19, Jessica Zhang wrote:
>>>> Currently, word count is calculated using slice_count. This is incorrect
>>>> as downstream uses slice per packet, which is different from
>>>> slice_count.
>>>>
>>>> Slice count represents the number of soft slices per interface, and its
>>>> value will not always match that of slice per packet. For example, it is
>>>> possible to have cases where there are multiple soft slices per interface
>>>> but the panel specifies only one slice per packet.
>>>>
>>>> Thus, use the default value of one slice per packet and remove slice_count
>>>> from the word count calculation.
>>>>
>>>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index 35c69dbe5f6f..b0d448ffb078 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>           if (!msm_host->dsc)
>>>>               wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>>>           else
>>>> -            wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
>>>> +            /*
>>>> +             * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1.
>>>> +             * Currently, the driver only supports default value of slice_per_packet = 1
>>>> +             *
>>>> +             * TODO: Expand drm_panel struct to hold slice_per_packet info
>>>> +             *       and adjust DSC math to account for slice_per_packet.
>>>
>>> slice_per_packet is not a part of the standard DSC, so I'm not sure how that can be implemented. And definitely we should not care about the drm_panel here. It should be either a part of drm_dsc_config, or mipi_dsi_device.
>>>
>>
>> This is not correct.
>>
>> It is part of the DSI standard (not DSC standard). Please refer to Figure 40 "One Line Containing One Packet with Data from One or More Compressed Slices" and Figure 41 "One Line Containing More than One Compressed Pixel Stream Packet".
> 
> I have reviewed section 8.8.24 and Annex D of the DSI standard.
> 
> It is not clear to me, if we can get away with always using slice_per_packet = 1. What is the DSI sink's difference between Fig. 40.(b) and Fig 41?
> 
> Are there are known panels that require slice_per_packet != 1? If so, we will have to implement support for such configurations.
At least two different ones on expensive Xperias (souxp00_a+amb650wh07 and
sofef03_m)

Konrad
> 
>> This has details about this. So I still stand by my point that this should be in the drm_panel.
> 
> Note, the driver doesn't use drm_panel directly. So slices_per_packet should go to mipi_dsi_device instead (which in turn can be filled from e.g. drm_panel or from any other source).
> 
>>
>>>> +             */
>>>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>>>
>>>
> 

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Neil Armstrong]

On 09/05/2023 01:27, Dmitry Baryshkov wrote:
> On 08/05/2023 23:09, Abhinav Kumar wrote:
>>
>>
>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
>>> On 03/05/2023 04:19, Jessica Zhang wrote:
>>>> Currently, word count is calculated using slice_count. This is incorrect
>>>> as downstream uses slice per packet, which is different from
>>>> slice_count.
>>>>
>>>> Slice count represents the number of soft slices per interface, and its
>>>> value will not always match that of slice per packet. For example, it is
>>>> possible to have cases where there are multiple soft slices per interface
>>>> but the panel specifies only one slice per packet.
>>>>
>>>> Thus, use the default value of one slice per packet and remove slice_count
>>>> from the word count calculation.
>>>>
>>>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index 35c69dbe5f6f..b0d448ffb078 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>           if (!msm_host->dsc)
>>>>               wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>>>           else
>>>> -            wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
>>>> +            /*
>>>> +             * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1.
>>>> +             * Currently, the driver only supports default value of slice_per_packet = 1
>>>> +             *
>>>> +             * TODO: Expand drm_panel struct to hold slice_per_packet info
>>>> +             *       and adjust DSC math to account for slice_per_packet.
>>>
>>> slice_per_packet is not a part of the standard DSC, so I'm not sure how that can be implemented. And definitely we should not care about the drm_panel here. It should be either a part of drm_dsc_config, or mipi_dsi_device.
>>>
>>
>> This is not correct.
>>
>> It is part of the DSI standard (not DSC standard). Please refer to Figure 40 "One Line Containing One Packet with Data from One or More Compressed Slices" and Figure 41 "One Line Containing More than One Compressed Pixel Stream Packet".
> 
> I have reviewed section 8.8.24 and Annex D of the DSI standard.
> 
> It is not clear to me, if we can get away with always using slice_per_packet = 1. What is the DSI sink's difference between Fig. 40.(b) and Fig 41?
> 
> Are there are known panels that require slice_per_packet != 1? If so, we will have to implement support for such configurations.
> 
>> This has details about this. So I still stand by my point that this should be in the drm_panel.
> 
> Note, the driver doesn't use drm_panel directly. So slices_per_packet should go to mipi_dsi_device instead (which in turn can be filled from e.g. drm_panel or from any other source).

This is a big question, where should we set those parameters ?

It's an even bigger questions for panels optionally supporting DSC in Video or Command mode (like the vtdr6130),
how to select DSC or not ? DT is not an option.

Those should tied to a panel+controller tuple.

Neil

> 
>>
>>>> +             */
>>>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>>>
>>>
> 

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Konrad Dybcio]

On 9.05.2023 10:23, Neil Armstrong wrote:
> On 09/05/2023 01:27, Dmitry Baryshkov wrote:
>> On 08/05/2023 23:09, Abhinav Kumar wrote:
>>>
>>>
>>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
>>>> On 03/05/2023 04:19, Jessica Zhang wrote:
>>>>> Currently, word count is calculated using slice_count. This is incorrect
>>>>> as downstream uses slice per packet, which is different from
>>>>> slice_count.
>>>>>
>>>>> Slice count represents the number of soft slices per interface, and its
>>>>> value will not always match that of slice per packet. For example, it is
>>>>> possible to have cases where there are multiple soft slices per interface
>>>>> but the panel specifies only one slice per packet.
>>>>>
>>>>> Thus, use the default value of one slice per packet and remove slice_count
>>>>> from the word count calculation.
>>>>>
>>>>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>> index 35c69dbe5f6f..b0d448ffb078 100644
>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>           if (!msm_host->dsc)
>>>>>               wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>>>>           else
>>>>> -            wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
>>>>> +            /*
>>>>> +             * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1.
>>>>> +             * Currently, the driver only supports default value of slice_per_packet = 1
>>>>> +             *
>>>>> +             * TODO: Expand drm_panel struct to hold slice_per_packet info
>>>>> +             *       and adjust DSC math to account for slice_per_packet.
>>>>
>>>> slice_per_packet is not a part of the standard DSC, so I'm not sure how that can be implemented. And definitely we should not care about the drm_panel here. It should be either a part of drm_dsc_config, or mipi_dsi_device.
>>>>
>>>
>>> This is not correct.
>>>
>>> It is part of the DSI standard (not DSC standard). Please refer to Figure 40 "One Line Containing One Packet with Data from One or More Compressed Slices" and Figure 41 "One Line Containing More than One Compressed Pixel Stream Packet".
>>
>> I have reviewed section 8.8.24 and Annex D of the DSI standard.
>>
>> It is not clear to me, if we can get away with always using slice_per_packet = 1. What is the DSI sink's difference between Fig. 40.(b) and Fig 41?
>>
>> Are there are known panels that require slice_per_packet != 1? If so, we will have to implement support for such configurations.
>>
>>> This has details about this. So I still stand by my point that this should be in the drm_panel.
>>
>> Note, the driver doesn't use drm_panel directly. So slices_per_packet should go to mipi_dsi_device instead (which in turn can be filled from e.g. drm_panel or from any other source).
> 
> This is a big question, where should we set those parameters ?
> 
> It's an even bigger questions for panels optionally supporting DSC in Video or Command mode (like the vtdr6130),
> how to select DSC or not ? DT is not an option.
Compressed vs uncompressed modes, maybe? Would be nice to make this
togglable from userspace.. But then it may not scale for panels with e.g.
10 resolutions, all cmd/vid/dsc/nodsc


Konrad
> 
> Those should tied to a panel+controller tuple.
> 
> Neil
> 
>>
>>>
>>>>> +             */
>>>>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>>>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>>>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>>>>
>>>>
>>
> 

Re: [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Dmitry Baryshkov]

On 09/05/2023 11:54, Konrad Dybcio wrote:
> 
> 
> On 9.05.2023 10:23, Neil Armstrong wrote:
>> On 09/05/2023 01:27, Dmitry Baryshkov wrote:
>>> On 08/05/2023 23:09, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
>>>>> On 03/05/2023 04:19, Jessica Zhang wrote:
>>>>>> Currently, word count is calculated using slice_count. This is incorrect
>>>>>> as downstream uses slice per packet, which is different from
>>>>>> slice_count.
>>>>>>
>>>>>> Slice count represents the number of soft slices per interface, and its
>>>>>> value will not always match that of slice per packet. For example, it is
>>>>>> possible to have cases where there are multiple soft slices per interface
>>>>>> but the panel specifies only one slice per packet.
>>>>>>
>>>>>> Thus, use the default value of one slice per packet and remove slice_count
>>>>>> from the word count calculation.
>>>>>>
>>>>>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>>>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> index 35c69dbe5f6f..b0d448ffb078 100644
>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>            if (!msm_host->dsc)
>>>>>>                wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>>>>>            else
>>>>>> -            wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
>>>>>> +            /*
>>>>>> +             * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1.
>>>>>> +             * Currently, the driver only supports default value of slice_per_packet = 1
>>>>>> +             *
>>>>>> +             * TODO: Expand drm_panel struct to hold slice_per_packet info
>>>>>> +             *       and adjust DSC math to account for slice_per_packet.
>>>>>
>>>>> slice_per_packet is not a part of the standard DSC, so I'm not sure how that can be implemented. And definitely we should not care about the drm_panel here. It should be either a part of drm_dsc_config, or mipi_dsi_device.
>>>>>
>>>>
>>>> This is not correct.
>>>>
>>>> It is part of the DSI standard (not DSC standard). Please refer to Figure 40 "One Line Containing One Packet with Data from One or More Compressed Slices" and Figure 41 "One Line Containing More than One Compressed Pixel Stream Packet".
>>>
>>> I have reviewed section 8.8.24 and Annex D of the DSI standard.
>>>
>>> It is not clear to me, if we can get away with always using slice_per_packet = 1. What is the DSI sink's difference between Fig. 40.(b) and Fig 41?
>>>
>>> Are there are known panels that require slice_per_packet != 1? If so, we will have to implement support for such configurations.
>>>
>>>> This has details about this. So I still stand by my point that this should be in the drm_panel.
>>>
>>> Note, the driver doesn't use drm_panel directly. So slices_per_packet should go to mipi_dsi_device instead (which in turn can be filled from e.g. drm_panel or from any other source).
>>
>> This is a big question, where should we set those parameters ?
>>
>> It's an even bigger questions for panels optionally supporting DSC in Video or Command mode (like the vtdr6130),
>> how to select DSC or not ? DT is not an option.
> Compressed vs uncompressed modes, maybe? Would be nice to make this
> togglable from userspace.. But then it may not scale for panels with e.g.
> 10 resolutions, all cmd/vid/dsc/nodsc

Currently the panel/panel-bridge make decision on command vs video mode. 
We have no way to influence that decision. If you want to make that 
negotiable, I'd start with adding 
'cmd_supported/video_supported/dsc_supported' flags to struct 
mipi_dsi_hosts.

> 
> 
> Konrad
>>
>> Those should tied to a panel+controller tuple.
>>
>> Neil
>>
>>>
>>>>
>>>>>> +             */
>>>>>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>>>>>            dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>>>>>                DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>>>>>
>>>>>
>>>
>>

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Abhinav Kumar]

On 5/9/2023 4:42 AM, Dmitry Baryshkov wrote:
> On 09/05/2023 11:54, Konrad Dybcio wrote:
>>
>>
>> On 9.05.2023 10:23, Neil Armstrong wrote:
>>> On 09/05/2023 01:27, Dmitry Baryshkov wrote:
>>>> On 08/05/2023 23:09, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
>>>>>> On 03/05/2023 04:19, Jessica Zhang wrote:
>>>>>>> Currently, word count is calculated using slice_count. This is 
>>>>>>> incorrect
>>>>>>> as downstream uses slice per packet, which is different from
>>>>>>> slice_count.
>>>>>>>
>>>>>>> Slice count represents the number of soft slices per interface, 
>>>>>>> and its
>>>>>>> value will not always match that of slice per packet. For 
>>>>>>> example, it is
>>>>>>> possible to have cases where there are multiple soft slices per 
>>>>>>> interface
>>>>>>> but the panel specifies only one slice per packet.
>>>>>>>
>>>>>>> Thus, use the default value of one slice per packet and remove 
>>>>>>> slice_count
>>>>>>> from the word count calculation.
>>>>>>>
>>>>>>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size 
>>>>>>> to compute word count")
>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>>>>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> index 35c69dbe5f6f..b0d448ffb078 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct 
>>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>>            if (!msm_host->dsc)
>>>>>>>                wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 
>>>>>>> + 1;
>>>>>>>            else
>>>>>>> -            wc = msm_host->dsc->slice_chunk_size * 
>>>>>>> msm_host->dsc->slice_count + 1;
>>>>>>> +            /*
>>>>>>> +             * When DSC is enabled, WC = slice_chunk_size * 
>>>>>>> slice_per_packet + 1.
>>>>>>> +             * Currently, the driver only supports default value 
>>>>>>> of slice_per_packet = 1
>>>>>>> +             *
>>>>>>> +             * TODO: Expand drm_panel struct to hold 
>>>>>>> slice_per_packet info
>>>>>>> +             *       and adjust DSC math to account for 
>>>>>>> slice_per_packet.
>>>>>>
>>>>>> slice_per_packet is not a part of the standard DSC, so I'm not 
>>>>>> sure how that can be implemented. And definitely we should not 
>>>>>> care about the drm_panel here. It should be either a part of 
>>>>>> drm_dsc_config, or mipi_dsi_device.
>>>>>>
>>>>>
>>>>> This is not correct.
>>>>>
>>>>> It is part of the DSI standard (not DSC standard). Please refer to 
>>>>> Figure 40 "One Line Containing One Packet with Data from One or 
>>>>> More Compressed Slices" and Figure 41 "One Line Containing More 
>>>>> than One Compressed Pixel Stream Packet".
>>>>
>>>> I have reviewed section 8.8.24 and Annex D of the DSI standard.
>>>>
>>>> It is not clear to me, if we can get away with always using 
>>>> slice_per_packet = 1. What is the DSI sink's difference between Fig. 
>>>> 40.(b) and Fig 41?
>>>>
>>>> Are there are known panels that require slice_per_packet != 1? If 
>>>> so, we will have to implement support for such configurations.
>>>>
>>>>> This has details about this. So I still stand by my point that this 
>>>>> should be in the drm_panel.
>>>>
>>>> Note, the driver doesn't use drm_panel directly. So 
>>>> slices_per_packet should go to mipi_dsi_device instead (which in 
>>>> turn can be filled from e.g. drm_panel or from any other source).
>>>
>>> This is a big question, where should we set those parameters ?
>>>
>>> It's an even bigger questions for panels optionally supporting DSC in 
>>> Video or Command mode (like the vtdr6130),
>>> how to select DSC or not ? DT is not an option.
>> Compressed vs uncompressed modes, maybe? Would be nice to make this
>> togglable from userspace.. But then it may not scale for panels with e.g.
>> 10 resolutions, all cmd/vid/dsc/nodsc
> 
> Currently the panel/panel-bridge make decision on command vs video mode. 
> We have no way to influence that decision. If you want to make that 
> negotiable, I'd start with adding 
> 'cmd_supported/video_supported/dsc_supported' flags to struct 
> mipi_dsi_hosts.
> 

Right. Isn't that issue there even today that if a panel supports DSC in 
only one of the modes, we have no way to tell that as we have not 
encountered such a panel in upstream yet.

Also, fundamental question to folks who had panels requiring 
slice_per_pkt as 2,

if you had some panels which need a slice_per_pkt as 2, this support 
could have been added even earlier by someone who had these panels even 
in DSC 1.1.

If these panels are not yet upstreamed, may i please know why this is 
considered as a "breakage"? If they were working "somehow" due to 
incorrect math / panel settings /  DPU calculations, unfortunately we 
have to work towards bringing up these panels properly and upstreaming 
them rather than saying "oh, these panels were working somehow and now 
we need to keep it working".

>>
>>
>> Konrad
>>>
>>> Those should tied to a panel+controller tuple.
>>>
>>> Neil
>>>
>>>>
>>>>>
>>>>>>> +             */
>>>>>>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>>>>>>            dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>>>>>>                DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>>>>>>
>>>>>>
>>>>
>>>
> 

Re: [Freedreno] [PATCH 2/4] drm/msm/dsi: Fix compressed word count calculation

[Jessica Zhang]

On 5/9/2023 11:02 AM, Abhinav Kumar wrote:
> 
> 
> On 5/9/2023 4:42 AM, Dmitry Baryshkov wrote:
>> On 09/05/2023 11:54, Konrad Dybcio wrote:
>>>
>>>
>>> On 9.05.2023 10:23, Neil Armstrong wrote:
>>>> On 09/05/2023 01:27, Dmitry Baryshkov wrote:
>>>>> On 08/05/2023 23:09, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 5/3/2023 1:26 AM, Dmitry Baryshkov wrote:
>>>>>>> On 03/05/2023 04:19, Jessica Zhang wrote:
>>>>>>>> Currently, word count is calculated using slice_count. This is 
>>>>>>>> incorrect
>>>>>>>> as downstream uses slice per packet, which is different from
>>>>>>>> slice_count.
>>>>>>>>
>>>>>>>> Slice count represents the number of soft slices per interface, 
>>>>>>>> and its
>>>>>>>> value will not always match that of slice per packet. For 
>>>>>>>> example, it is
>>>>>>>> possible to have cases where there are multiple soft slices per 
>>>>>>>> interface
>>>>>>>> but the panel specifies only one slice per packet.
>>>>>>>>
>>>>>>>> Thus, use the default value of one slice per packet and remove 
>>>>>>>> slice_count
>>>>>>>> from the word count calculation.
>>>>>>>>
>>>>>>>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size 
>>>>>>>> to compute word count")
>>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 9 ++++++++-
>>>>>>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> index 35c69dbe5f6f..b0d448ffb078 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> @@ -996,7 +996,14 @@ static void dsi_timing_setup(struct 
>>>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>>>            if (!msm_host->dsc)
>>>>>>>>                wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 
>>>>>>>> + 1;
>>>>>>>>            else
>>>>>>>> -            wc = msm_host->dsc->slice_chunk_size * 
>>>>>>>> msm_host->dsc->slice_count + 1;
>>>>>>>> +            /*
>>>>>>>> +             * When DSC is enabled, WC = slice_chunk_size * 
>>>>>>>> slice_per_packet + 1.
>>>>>>>> +             * Currently, the driver only supports default 
>>>>>>>> value of slice_per_packet = 1
>>>>>>>> +             *
>>>>>>>> +             * TODO: Expand drm_panel struct to hold 
>>>>>>>> slice_per_packet info
>>>>>>>> +             *       and adjust DSC math to account for 
>>>>>>>> slice_per_packet.
>>>>>>>
>>>>>>> slice_per_packet is not a part of the standard DSC, so I'm not 
>>>>>>> sure how that can be implemented. And definitely we should not 
>>>>>>> care about the drm_panel here. It should be either a part of 
>>>>>>> drm_dsc_config, or mipi_dsi_device.
>>>>>>>
>>>>>>
>>>>>> This is not correct.
>>>>>>
>>>>>> It is part of the DSI standard (not DSC standard). Please refer to 
>>>>>> Figure 40 "One Line Containing One Packet with Data from One or 
>>>>>> More Compressed Slices" and Figure 41 "One Line Containing More 
>>>>>> than One Compressed Pixel Stream Packet".
>>>>>
>>>>> I have reviewed section 8.8.24 and Annex D of the DSI standard.
>>>>>
>>>>> It is not clear to me, if we can get away with always using 
>>>>> slice_per_packet = 1. What is the DSI sink's difference between 
>>>>> Fig. 40.(b) and Fig 41?
>>>>>
>>>>> Are there are known panels that require slice_per_packet != 1? If 
>>>>> so, we will have to implement support for such configurations.
>>>>>
>>>>>> This has details about this. So I still stand by my point that 
>>>>>> this should be in the drm_panel.
>>>>>
>>>>> Note, the driver doesn't use drm_panel directly. So 
>>>>> slices_per_packet should go to mipi_dsi_device instead (which in 
>>>>> turn can be filled from e.g. drm_panel or from any other source).
>>>>
>>>> This is a big question, where should we set those parameters ?
>>>>
>>>> It's an even bigger questions for panels optionally supporting DSC 
>>>> in Video or Command mode (like the vtdr6130),
>>>> how to select DSC or not ? DT is not an option.
>>> Compressed vs uncompressed modes, maybe? Would be nice to make this
>>> togglable from userspace.. But then it may not scale for panels with 
>>> e.g.
>>> 10 resolutions, all cmd/vid/dsc/nodsc
>>
>> Currently the panel/panel-bridge make decision on command vs video 
>> mode. We have no way to influence that decision. If you want to make 
>> that negotiable, I'd start with adding 
>> 'cmd_supported/video_supported/dsc_supported' flags to struct 
>> mipi_dsi_hosts.
>>
> 
> Right. Isn't that issue there even today that if a panel supports DSC in 
> only one of the modes, we have no way to tell that as we have not 
> encountered such a panel in upstream yet.
> 
> Also, fundamental question to folks who had panels requiring 
> slice_per_pkt as 2,
> 
> if you had some panels which need a slice_per_pkt as 2, this support 
> could have been added even earlier by someone who had these panels even 
> in DSC 1.1.
> 
> If these panels are not yet upstreamed, may i please know why this is 
> considered as a "breakage"? If they were working "somehow" due to 
> incorrect math / panel settings /  DPU calculations, unfortunately we 
> have to work towards bringing up these panels properly and upstreaming 
> them rather than saying "oh, these panels were working somehow and now 
> we need to keep it working".

I also want to add on top of Abhinav's point:

Currently, the panel I'm testing this series on only uses 
slice_per_pkt=1, so I have no way to testing slice_per_pkt > 1.

So in the case where we add support for slice_per_pkt > 1, I would 
prefer that be as a separate series as I would not be able to validate 
those changes on my current setup.

Thanks,

Jessica Zhang

> 
>>>
>>>
>>> Konrad
>>>>
>>>> Those should tied to a panel+controller tuple.
>>>>
>>>> Neil
>>>>
>>>>>
>>>>>>
>>>>>>>> +             */
>>>>>>>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>>>>>>>            dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>>>>>>>                DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>