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

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

[Jessica Zhang]

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

This includes:

1) Adjusting pclk_rate to account for compression
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 in command mode.

Note: Changes that add DSC v1.2 support for video mode will be posted
with the DP support changes.

Depends-on: "add DSC 1.2 dpu supports" [1] and "Introduce MSM-specific
DSC helpers" [2]

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

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
Changes in v2:
- Changed pclk math to only divide hdisplay by compression ratio
- Reworded word count TODO comment
- Make DATA_COMPRESS an INTF flag
- Read INTF_CFG2 before setting DATA_COMRPESS register
- Reworded commit messages and cover letter for clarity
- Link to v1: https://lore.kernel.org/r/20230405-add-dsc-support-v1-0-6bc6f03ae735@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 DPU_INTF_DATA_COMPRESS feature flag
      drm/msm/dpu: Set DATA_COMPRESS for command mode

 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  3 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |  2 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 11 +++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  2 ++
 drivers/gpu/drm/msm/dsi/dsi_host.c                 | 26 +++++++++++++++++-----
 6 files changed, 40 insertions(+), 6 deletions(-)
---
base-commit: 70e08302e024bfac485b12972099237f7f39d829
change-id: 20230405-add-dsc-support-fe130ba49841

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

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

[Jessica Zhang]

Adjust the pclk rate to divide hdisplay by the compression ratio when DSC
is enabled.

Changes in v2:
- Adjusted pclk_rate math to divide only the hdisplay value by
  compression ratio

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 17 +++++++++++++----
 1 file changed, 13 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..0e5778e8091f 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,14 @@ 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 hdisplay by compression ratio */
+	if (dsc) {
+		int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * msm_dsc_get_bpp_int(dsc),
+				dsc->bits_per_component * 3);
+		int fps = DIV_ROUND_UP(pclk_rate, mode->htotal * mode->vtotal);
+		pclk_rate = (new_hdisplay + (mode->htotal - mode->hdisplay)) * mode->vtotal * fps;
+	}
+
 	return pclk_rate;
 }
 
@@ -585,7 +594,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 +613,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 +643,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 v2 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.

Changes in v2:
- "drm_panel" -> "mipi_dsi_device" in TODO comment

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 0e5778e8091f..f6fb32e2223c 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -999,7 +999,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 mipi_dsi_device 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 v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Jessica Zhang]

Add DATA_COMPRESS feature flag to DPU INTF block.

In DPU 7.x and later, DSC/DCE enablement registers have been moved from
PINGPONG to INTF.

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 feature flag.

Changes in v2:
- Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 7944481d0a33..c74051906d05 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -104,7 +104,7 @@
 #define INTF_SC7180_MASK \
 	(BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | BIT(DPU_INTF_STATUS_SUPPORTED))
 
-#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
+#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_DATA_COMPRESS)
 
 #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
 			 BIT(DPU_WB_UBWC) | \
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 4eda2cc847ef..01c65f940f2a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -185,6 +185,7 @@ enum {
  * @DPU_DATA_HCTL_EN                Allows data to be transferred at different rate
  *                                  than video timing
  * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS register
+ * @DPU_INTF_DATA_COMPRESS          INTF block has DATA_COMPRESS register
  * @DPU_INTF_MAX
  */
 enum {
@@ -192,6 +193,7 @@ enum {
 	DPU_INTF_TE,
 	DPU_DATA_HCTL_EN,
 	DPU_INTF_STATUS_SUPPORTED,
+	DPU_INTF_DATA_COMPRESS,
 	DPU_INTF_MAX
 };
 

-- 
2.40.1

[PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode

[Jessica Zhang]

Add a DPU INTF op to set DATA_COMPRESS register for command mode panels if
the DPU_INTF_DATA_COMPRESS feature flag is set. This flag needs to be
enabled in order for DSC v1.2 to work.

Note: These changes are for command mode only. Video mode changes will
be posted along with the DSC v1.2 support for DP.

Changes in v2:
- Fixed whitespace issue in macro definition
- Read INTF_CONFIG2 before writing to DATA_COMPRESS bit
- Only set dpu_hw_intf_ops.data_compress if DATA_COMPRESS feature is set
- Removed `inline` from dpu_hw_intf_enable_compression declaration

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  3 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 11 +++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  2 ++
 3 files changed, 16 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 d8ed85a238af..1a4c20f02312 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
@@ -68,6 +68,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
 				phys_enc->hw_intf,
 				true,
 				phys_enc->hw_pp->idx);
+
+	if (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 6485500eedb8..322c55a5042c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -91,6 +91,14 @@
 
 #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
 #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
+#define INTF_CFG2_DCE_DATA_COMPRESS     BIT(12)
+
+static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
+{
+	u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
+
+	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2 | INTF_CFG2_DCE_DATA_COMPRESS);
+}
 
 static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 		const struct intf_timing_params *p,
@@ -542,6 +550,9 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
 		ops->vsync_sel = dpu_hw_intf_vsync_sel;
 		ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
 	}
+
+	if (cap & BIT(DPU_INTF_DATA_COMPRESS))
+		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 73b0885918f8..a8def68a5ec2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -70,6 +70,7 @@ struct intf_status {
  * @get_autorefresh:            Retrieve autorefresh config from hardware
  *                              Return: 0 on success, -ETIMEDOUT on timeout
  * @vsync_sel:                  Select vsync signal for tear-effect configuration
+ * @enable_compression: Enable data compression
  */
 struct dpu_hw_intf_ops {
 	void (*setup_timing_gen)(struct dpu_hw_intf *intf,
@@ -107,6 +108,7 @@ struct dpu_hw_intf_ops {
 	 * Disable autorefresh if enabled
 	 */
 	void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay);
+	void (*enable_compression)(struct dpu_hw_intf *intf);
 };
 
 struct dpu_hw_intf {

-- 
2.40.1

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

[Jessica Zhang]

On 5/5/2023 2:23 PM, Jessica Zhang wrote:
> Adjust the pclk rate to divide hdisplay by the compression ratio when DSC
> is enabled.
> 
> Changes in v2:
> - Adjusted pclk_rate math to divide only the hdisplay value by
>    compression ratio
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 17 +++++++++++++----
>   1 file changed, 13 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..0e5778e8091f 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,14 @@ 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 hdisplay by compression ratio */
> +	if (dsc) {
> +		int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * msm_dsc_get_bpp_int(dsc),
> +				dsc->bits_per_component * 3);
> +		int fps = DIV_ROUND_UP(pclk_rate, mode->htotal * mode->vtotal);

Should've used drm_mode_vrefresh() here... Will spin a v3 with that 
change (along with any additional comments)

> +		pclk_rate = (new_hdisplay + (mode->htotal - mode->hdisplay)) * mode->vtotal * fps;
> +	}
> +
>   	return pclk_rate;
>   }
>   
> @@ -585,7 +594,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 +613,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 +643,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 v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Marijn Suijten]

On 2023-05-05 14:23:50, Jessica Zhang wrote:
> Add DATA_COMPRESS feature flag to DPU INTF block.
> 
> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
> PINGPONG to INTF.
> 
> 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 feature flag.

Irrelevant.  Even though core_rev was still in mainline until recently,
we always hardcoded the features in the catalog and only used core_rev
to select a dpu_mdss_cfg catalog entry.  There is no "if version >= X
then enable feature Y" logic, this manually-enabled feature flag is the
only, correct way to do it.

> Changes in v2:
> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>

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

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index 7944481d0a33..c74051906d05 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -104,7 +104,7 @@
>  #define INTF_SC7180_MASK \
>  	(BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | BIT(DPU_INTF_STATUS_SUPPORTED))
>  
> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_DATA_COMPRESS)

Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
erroneously also receive this feature flag and write the new
DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).

[1]: https://lore.kernel.org/linux-arm-msm/80b46fcb-d6d0-1998-c273-5401fa924c7d@linaro.org/T/#u

Depending on who lands first, this flag should be split.

I still see value in inlining and removing these defines, though that
brings a host of other complexity.

- Marijn

>  #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>  			 BIT(DPU_WB_UBWC) | \
> 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 4eda2cc847ef..01c65f940f2a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -185,6 +185,7 @@ enum {
>   * @DPU_DATA_HCTL_EN                Allows data to be transferred at different rate
>   *                                  than video timing
>   * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS register
> + * @DPU_INTF_DATA_COMPRESS          INTF block has DATA_COMPRESS register
>   * @DPU_INTF_MAX
>   */
>  enum {
> @@ -192,6 +193,7 @@ enum {
>  	DPU_INTF_TE,
>  	DPU_DATA_HCTL_EN,
>  	DPU_INTF_STATUS_SUPPORTED,
> +	DPU_INTF_DATA_COMPRESS,
>  	DPU_INTF_MAX
>  };
>  
> 
> -- 
> 2.40.1
> 

Re: [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode

[Marijn Suijten]

On 2023-05-05 14:23:51, Jessica Zhang wrote:
> Add a DPU INTF op to set DATA_COMPRESS register for command mode panels if
> the DPU_INTF_DATA_COMPRESS feature flag is set. This flag needs to be
> enabled in order for DSC v1.2 to work.
> 
> Note: These changes are for command mode only. Video mode changes will
> be posted along with the DSC v1.2 support for DP.

Nit: the "command mode" parts of both paragraphs only apply to the call
in dpu_encoder_phys_cmd, right?  If so, and the INTF op remains the same
for video mode (but only the call needs to be added to the
dpu_encoder_phy_vid), make this a bit more clear in your commit message.

> Changes in v2:
> - Fixed whitespace issue in macro definition
> - Read INTF_CONFIG2 before writing to DATA_COMPRESS bit
> - Only set dpu_hw_intf_ops.data_compress if DATA_COMPRESS feature is set
> - Removed `inline` from dpu_hw_intf_enable_compression declaration
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  3 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 11 +++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  2 ++
>  3 files changed, 16 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 d8ed85a238af..1a4c20f02312 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
> @@ -68,6 +68,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>  				phys_enc->hw_intf,
>  				true,
>  				phys_enc->hw_pp->idx);
> +
> +	if (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 6485500eedb8..322c55a5042c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -91,6 +91,14 @@
>  
>  #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
>  #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
> +#define INTF_CFG2_DCE_DATA_COMPRESS     BIT(12)
> +
> +static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
> +{
> +	u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
> +
> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2 | INTF_CFG2_DCE_DATA_COMPRESS);

I'm not sure if it's more idiomatic to write:

    intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;

On a separate line.

> +}

Move the function close to the bottom of this file.  Right now all the
functions are defined approximately in the same order as they're listed
in the header and assigned in _setup_intf_ops().

>  
>  static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>  		const struct intf_timing_params *p,
> @@ -542,6 +550,9 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
>  		ops->vsync_sel = dpu_hw_intf_vsync_sel;
>  		ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>  	}
> +
> +	if (cap & BIT(DPU_INTF_DATA_COMPRESS))
> +		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 73b0885918f8..a8def68a5ec2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -70,6 +70,7 @@ struct intf_status {
>   * @get_autorefresh:            Retrieve autorefresh config from hardware
>   *                              Return: 0 on success, -ETIMEDOUT on timeout
>   * @vsync_sel:                  Select vsync signal for tear-effect configuration
> + * @enable_compression: Enable data compression

Indent to match above.

- Marijn

>   */
>  struct dpu_hw_intf_ops {
>  	void (*setup_timing_gen)(struct dpu_hw_intf *intf,
> @@ -107,6 +108,7 @@ struct dpu_hw_intf_ops {
>  	 * Disable autorefresh if enabled
>  	 */
>  	void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay);
> +	void (*enable_compression)(struct dpu_hw_intf *intf);
>  };
>  
>  struct dpu_hw_intf {
> 
> -- 
> 2.40.1
> 

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Dmitry Baryshkov]

On 07/05/2023 19:00, Marijn Suijten wrote:
> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>
>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
>> PINGPONG to INTF.
>>
>> 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 feature flag.
> 
> Irrelevant.  Even though core_rev was still in mainline until recently,
> we always hardcoded the features in the catalog and only used core_rev
> to select a dpu_mdss_cfg catalog entry.  There is no "if version >= X
> then enable feature Y" logic, this manually-enabled feature flag is the
> only, correct way to do it.
> 
>> Changes in v2:
>> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> index 7944481d0a33..c74051906d05 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -104,7 +104,7 @@
>>   #define INTF_SC7180_MASK \
>>   	(BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | BIT(DPU_INTF_STATUS_SUPPORTED))
>>   
>> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_DATA_COMPRESS)
> 
> Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
> to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
> erroneously also receive this feature flag and write the new
> DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).

Yeah, that's why I had the idea of including at least the DPU major in 
the mask name.

It looks like we should enable DPU_DATA_HCTL_EN at least for 
sm8150/sm8250 (and other DPU 6.x) too. I am not sure if it is present on 
sdm845.

> 
> [1]: https://lore.kernel.org/linux-arm-msm/80b46fcb-d6d0-1998-c273-5401fa924c7d@linaro.org/T/#u
> 
> Depending on who lands first, this flag should be split.
> 
> I still see value in inlining and removing these defines, though that
> brings a host of other complexity.
> 
> - Marijn
> 
>>   #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>>   			 BIT(DPU_WB_UBWC) | \
>> 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 4eda2cc847ef..01c65f940f2a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -185,6 +185,7 @@ enum {
>>    * @DPU_DATA_HCTL_EN                Allows data to be transferred at different rate
>>    *                                  than video timing
>>    * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS register
>> + * @DPU_INTF_DATA_COMPRESS          INTF block has DATA_COMPRESS register
>>    * @DPU_INTF_MAX
>>    */
>>   enum {
>> @@ -192,6 +193,7 @@ enum {
>>   	DPU_INTF_TE,
>>   	DPU_DATA_HCTL_EN,
>>   	DPU_INTF_STATUS_SUPPORTED,
>> +	DPU_INTF_DATA_COMPRESS,
>>   	DPU_INTF_MAX
>>   };
>>   
>>
>> -- 
>> 2.40.1
>>

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Konrad Dybcio]

On 7.05.2023 18:00, Marijn Suijten wrote:
> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>
>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
>> PINGPONG to INTF.
>>
>> 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 feature flag.
> 
> Irrelevant.  Even though core_rev was still in mainline until recently,
> we always hardcoded the features in the catalog and only used core_rev
> to select a dpu_mdss_cfg catalog entry.  There is no "if version >= X
> then enable feature Y" logic, this manually-enabled feature flag is the
> only, correct way to do it.
> 
>> Changes in v2:
>> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> index 7944481d0a33..c74051906d05 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -104,7 +104,7 @@
>>  #define INTF_SC7180_MASK \
>>  	(BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | BIT(DPU_INTF_STATUS_SUPPORTED))
>>  
>> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_DATA_COMPRESS)
> 
> Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
> to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
> erroneously also receive this feature flag and write the new
> DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
> 
> [1]: https://lore.kernel.org/linux-arm-msm/80b46fcb-d6d0-1998-c273-5401fa924c7d@linaro.org/T/#u
> 
> Depending on who lands first, this flag should be split.
I'll adapt my patches. Jessica, no changes required on your side.

> 
> I still see value in inlining and removing these defines, though that
> brings a host of other complexity.
right, we should totally do it after we settle down from the patch
flurry

Konrad
> 
> - Marijn
> 
>>  #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>>  			 BIT(DPU_WB_UBWC) | \
>> 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 4eda2cc847ef..01c65f940f2a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -185,6 +185,7 @@ enum {
>>   * @DPU_DATA_HCTL_EN                Allows data to be transferred at different rate
>>   *                                  than video timing
>>   * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS register
>> + * @DPU_INTF_DATA_COMPRESS          INTF block has DATA_COMPRESS register
>>   * @DPU_INTF_MAX
>>   */
>>  enum {
>> @@ -192,6 +193,7 @@ enum {
>>  	DPU_INTF_TE,
>>  	DPU_DATA_HCTL_EN,
>>  	DPU_INTF_STATUS_SUPPORTED,
>> +	DPU_INTF_DATA_COMPRESS,
>>  	DPU_INTF_MAX
>>  };
>>  
>>
>> -- 
>> 2.40.1
>>

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Jessica Zhang]

On 5/7/2023 9:00 AM, Marijn Suijten wrote:
> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>
>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
>> PINGPONG to INTF.
>>
>> 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 feature flag.
> 
> Irrelevant.  Even though core_rev was still in mainline until recently,
> we always hardcoded the features in the catalog and only used core_rev
> to select a dpu_mdss_cfg catalog entry.  There is no "if version >= X
> then enable feature Y" logic, this manually-enabled feature flag is the
> only, correct way to do it.

Hi Marijn,

Understood. FWIW, if we do find more register bit-level differences 
between HW versions in the future, it might make more sense to keep the 
HW catalog small and bring core_rev back, rather than keep adding these 
kinds of small differences to caps.

Thanks,

Jessica Zhang

> 
>> Changes in v2:
>> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> index 7944481d0a33..c74051906d05 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -104,7 +104,7 @@
>>   #define INTF_SC7180_MASK \
>>   	(BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | BIT(DPU_INTF_STATUS_SUPPORTED))
>>   
>> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_DATA_COMPRESS)
> 
> Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
> to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
> erroneously also receive this feature flag and write the new
> DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
> 
> [1]: https://lore.kernel.org/linux-arm-msm/80b46fcb-d6d0-1998-c273-5401fa924c7d@linaro.org/T/#u
> 
> Depending on who lands first, this flag should be split.
> 
> I still see value in inlining and removing these defines, though that
> brings a host of other complexity.
> 
> - Marijn
> 
>>   #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>>   			 BIT(DPU_WB_UBWC) | \
>> 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 4eda2cc847ef..01c65f940f2a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -185,6 +185,7 @@ enum {
>>    * @DPU_DATA_HCTL_EN                Allows data to be transferred at different rate
>>    *                                  than video timing
>>    * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS register
>> + * @DPU_INTF_DATA_COMPRESS          INTF block has DATA_COMPRESS register
>>    * @DPU_INTF_MAX
>>    */
>>   enum {
>> @@ -192,6 +193,7 @@ enum {
>>   	DPU_INTF_TE,
>>   	DPU_DATA_HCTL_EN,
>>   	DPU_INTF_STATUS_SUPPORTED,
>> +	DPU_INTF_DATA_COMPRESS,
>>   	DPU_INTF_MAX
>>   };
>>   
>>
>> -- 
>> 2.40.1
>>

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Marijn Suijten]

On 2023-05-07 22:21:35, Dmitry Baryshkov wrote:
<snip>
> > Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
> > to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
> > erroneously also receive this feature flag and write the new
> > DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
> 
> Yeah, that's why I had the idea of including at least the DPU major in 
> the mask name.

Yes please, that would be much more clear.  We could even drop the SoC
name altogether.

> It looks like we should enable DPU_DATA_HCTL_EN at least for 
> sm8150/sm8250 (and other DPU 6.x) too. I am not sure if it is present on 
> sdm845.

Agreed, thanks for sending that patch!

<snip>

- Marijn

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Marijn Suijten]

On 2023-05-08 14:46:10, Jessica Zhang wrote:
> 
> 
> On 5/7/2023 9:00 AM, Marijn Suijten wrote:
> > On 2023-05-05 14:23:50, Jessica Zhang wrote:
> >> Add DATA_COMPRESS feature flag to DPU INTF block.
> >>
> >> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
> >> PINGPONG to INTF.
> >>
> >> 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 feature flag.
> > 
> > Irrelevant.  Even though core_rev was still in mainline until recently,
> > we always hardcoded the features in the catalog and only used core_rev
> > to select a dpu_mdss_cfg catalog entry.  There is no "if version >= X
> > then enable feature Y" logic, this manually-enabled feature flag is the
> > only, correct way to do it.
> 
> Hi Marijn,
> 
> Understood.

Thanks if you can drop the paragraph.

> FWIW, if we do find more register bit-level differences 
> between HW versions in the future, it might make more sense to keep the 
> HW catalog small and bring core_rev back, rather than keep adding these 
> kinds of small differences to caps.

That is not up to me to decide, but I do agree that DPU is currently
"one big mess" where lots of things are hardcoded in the catalog (which
isn't a bad thing, these things won't change but it does make it harder
on us than if we could dynamically state "every DPU between these two
revisions"), and certain other things are/were read back from hardware
registers.

As well as the sub-block feature flags that pain us :)

- Marijn

> Thanks,
> 
> Jessica Zhang

<snip>

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Marijn Suijten]

On 2023-05-08 10:36:09, Konrad Dybcio wrote:
<snip>
> > Depending on who lands first, this flag should be split.
> I'll adapt my patches. Jessica, no changes required on your side.

Looks like no split is needed after moving HCTL to SC7180.  Your SM6375
series can use SC7180 instead of SC7280 now and everything will be right
without a change here.

> > I still see value in inlining and removing these defines, though that
> > brings a host of other complexity.
> right, we should totally do it after we settle down from the patch
> flurry

But we just got a nice velocity, I'd rather keep going :) - we can
totally address these cleanups somewhere along the line though.

- Marijn

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

[Marijn Suijten]

On 2023-05-05 14:49:08, Jessica Zhang wrote:
> On 5/5/2023 2:23 PM, Jessica Zhang wrote:
> > Adjust the pclk rate to divide hdisplay by the compression ratio when DSC
> > is enabled.
> > 
> > Changes in v2:
> > - Adjusted pclk_rate math to divide only the hdisplay value by
> >    compression ratio
> > 
> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> > ---
> >   drivers/gpu/drm/msm/dsi/dsi_host.c | 17 +++++++++++++----
> >   1 file changed, 13 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..0e5778e8091f 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,14 @@ 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 hdisplay by compression ratio */
> > +	if (dsc) {
> > +		int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * msm_dsc_get_bpp_int(dsc),
> > +				dsc->bits_per_component * 3);
> > +		int fps = DIV_ROUND_UP(pclk_rate, mode->htotal * mode->vtotal);
> 
> Should've used drm_mode_vrefresh() here... Will spin a v3 with that 
> change (along with any additional comments)

Perhaps unsigned long on some of these?  Overall the computations and
multi-lines look rather cluttered, perhaps (parts of) this is/are a
prime candidate to go into the new helpers?

Note that I cannot get the 4k mode working at 60Hz on one of my panels
(30Hz works with minor corruption), regardless of this patch.  See also:
https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1900031

> > +		pclk_rate = (new_hdisplay + (mode->htotal - mode->hdisplay)) * mode->vtotal * fps;
> > +	}
> > +
> >   	return pclk_rate;
> >   }
> >   
> > @@ -585,7 +594,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 +613,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 +643,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;

Let's rebase on top of "drm/msm/dsi: simplify pixel clk rate handling"
[1] to clean this up.

[1]: https://lore.kernel.org/linux-arm-msm/20230118130031.2345941-1-dmitry.baryshkov@linaro.org/

- Marijn

> >   	do_div(pclk_bpp, 8);
> >   	msm_host->src_clk_rate = pclk_bpp;
> >   
> > 
> > -- 
> > 2.40.1
> > 

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Dmitry Baryshkov]

On 09/05/2023 00:46, Jessica Zhang wrote:
> 
> 
> On 5/7/2023 9:00 AM, Marijn Suijten wrote:
>> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>>
>>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
>>> PINGPONG to INTF.
>>>
>>> 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 feature flag.
>>
>> Irrelevant.  Even though core_rev was still in mainline until recently,
>> we always hardcoded the features in the catalog and only used core_rev
>> to select a dpu_mdss_cfg catalog entry.  There is no "if version >= X
>> then enable feature Y" logic, this manually-enabled feature flag is the
>> only, correct way to do it.
> 
> Hi Marijn,
> 
> Understood. FWIW, if we do find more register bit-level differences 
> between HW versions in the future, it might make more sense to keep the 
> HW catalog small and bring core_rev back, rather than keep adding these 
> kinds of small differences to caps.

Let's see how it goes. Abhinav suggested that there might be feature 
differences inside the DPU generations (and even inside the single DPU 
major/minor combo). So I'm not sure what core_rev will bring us.

Let's land the platforms which are ready (or if there is anything close 
to be submitted). I'll post the next proposal for the catalog cleanups 
close to -rc4, when the dust settles then we can have one or two weaks 
for the discussion and polishing.

I'd like to consider:
- inlining foo_BLK macros, if that makes adding new features easier
- reformat of clk_ctrls
- maybe reintroduction of per-generation feature masks instead of 
keeping them named after the random SoC
- maybe a rework of mdss_irqs / INTFn_INTR. We already have this info in 
hw catalog.

Comments are appreciated.


> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>> Changes in v2:
>>> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>
>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>> index 7944481d0a33..c74051906d05 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>> @@ -104,7 +104,7 @@
>>>   #define INTF_SC7180_MASK \
>>>       (BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | 
>>> BIT(DPU_INTF_STATUS_SUPPORTED))
>>> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>>> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | 
>>> BIT(DPU_INTF_DATA_COMPRESS)
>>
>> Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
>> to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
>> erroneously also receive this feature flag and write the new
>> DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
>>
>> [1]: 
>> https://lore.kernel.org/linux-arm-msm/80b46fcb-d6d0-1998-c273-5401fa924c7d@linaro.org/T/#u
>>
>> Depending on who lands first, this flag should be split.
>>
>> I still see value in inlining and removing these defines, though that
>> brings a host of other complexity.
>>
>> - Marijn
>>
>>>   #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>>>                BIT(DPU_WB_UBWC) | \
>>> 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 4eda2cc847ef..01c65f940f2a 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>> @@ -185,6 +185,7 @@ enum {
>>>    * @DPU_DATA_HCTL_EN                Allows data to be transferred 
>>> at different rate
>>>    *                                  than video timing
>>>    * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS 
>>> register
>>> + * @DPU_INTF_DATA_COMPRESS          INTF block has DATA_COMPRESS 
>>> register
>>>    * @DPU_INTF_MAX
>>>    */
>>>   enum {
>>> @@ -192,6 +193,7 @@ enum {
>>>       DPU_INTF_TE,
>>>       DPU_DATA_HCTL_EN,
>>>       DPU_INTF_STATUS_SUPPORTED,
>>> +    DPU_INTF_DATA_COMPRESS,
>>>       DPU_INTF_MAX
>>>   };
>>>
>>> -- 
>>> 2.40.1
>>>

-- 
With best wishes
Dmitry

Re: [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode

[Jessica Zhang]

On 5/7/2023 9:06 AM, Marijn Suijten wrote:
> On 2023-05-05 14:23:51, Jessica Zhang wrote:
>> Add a DPU INTF op to set DATA_COMPRESS register for command mode panels if
>> the DPU_INTF_DATA_COMPRESS feature flag is set. This flag needs to be
>> enabled in order for DSC v1.2 to work.
>>
>> Note: These changes are for command mode only. Video mode changes will
>> be posted along with the DSC v1.2 support for DP.
> 
> Nit: the "command mode" parts of both paragraphs only apply to the call
> in dpu_encoder_phys_cmd, right?  If so, and the INTF op remains the same
> for video mode (but only the call needs to be added to the
> dpu_encoder_phy_vid), make this a bit more clear in your commit message.
> 
>> Changes in v2:
>> - Fixed whitespace issue in macro definition
>> - Read INTF_CONFIG2 before writing to DATA_COMPRESS bit
>> - Only set dpu_hw_intf_ops.data_compress if DATA_COMPRESS feature is set
>> - Removed `inline` from dpu_hw_intf_enable_compression declaration
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  3 +++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 11 +++++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  2 ++
>>   3 files changed, 16 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 d8ed85a238af..1a4c20f02312 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
>> @@ -68,6 +68,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>   				phys_enc->hw_intf,
>>   				true,
>>   				phys_enc->hw_pp->idx);
>> +
>> +	if (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 6485500eedb8..322c55a5042c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -91,6 +91,14 @@
>>   
>>   #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
>>   #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
>> +#define INTF_CFG2_DCE_DATA_COMPRESS     BIT(12)
>> +
>> +static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>> +{
>> +	u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>> +
>> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2 | INTF_CFG2_DCE_DATA_COMPRESS);
> 
> I'm not sure if it's more idiomatic to write:
> 
>      intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
> 
> On a separate line.

Hi Marijn,

Sounds good.

> 
>> +}
> 
> Move the function close to the bottom of this file.  Right now all the
> functions are defined approximately in the same order as they're listed
> in the header and assigned in _setup_intf_ops().

Acked.

> 
>>   
>>   static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>   		const struct intf_timing_params *p,
>> @@ -542,6 +550,9 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
>>   		ops->vsync_sel = dpu_hw_intf_vsync_sel;
>>   		ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>>   	}
>> +
>> +	if (cap & BIT(DPU_INTF_DATA_COMPRESS))
>> +		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 73b0885918f8..a8def68a5ec2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -70,6 +70,7 @@ struct intf_status {
>>    * @get_autorefresh:            Retrieve autorefresh config from hardware
>>    *                              Return: 0 on success, -ETIMEDOUT on timeout
>>    * @vsync_sel:                  Select vsync signal for tear-effect configuration
>> + * @enable_compression: Enable data compression
> 
> Indent to match above.

Sure, is the plan to correct the whitespace in the first half of the 
comment block in the future?

Thanks,

Jessica Zhang

> 
> - Marijn
> 
>>    */
>>   struct dpu_hw_intf_ops {
>>   	void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>> @@ -107,6 +108,7 @@ struct dpu_hw_intf_ops {
>>   	 * Disable autorefresh if enabled
>>   	 */
>>   	void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay);
>> +	void (*enable_compression)(struct dpu_hw_intf *intf);
>>   };
>>   
>>   struct dpu_hw_intf {
>>
>> -- 
>> 2.40.1
>>

Re: [Freedreno] [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode

[Jessica Zhang]

On 5/8/2023 4:17 PM, Jessica Zhang wrote:
> 
> 
> On 5/7/2023 9:06 AM, Marijn Suijten wrote:
>> On 2023-05-05 14:23:51, Jessica Zhang wrote:
>>> Add a DPU INTF op to set DATA_COMPRESS register for command mode 
>>> panels if
>>> the DPU_INTF_DATA_COMPRESS feature flag is set. This flag needs to be
>>> enabled in order for DSC v1.2 to work.
>>>
>>> Note: These changes are for command mode only. Video mode changes will
>>> be posted along with the DSC v1.2 support for DP.
>>
>> Nit: the "command mode" parts of both paragraphs only apply to the call
>> in dpu_encoder_phys_cmd, right?  If so, and the INTF op remains the same
>> for video mode (but only the call needs to be added to the
>> dpu_encoder_phy_vid), make this a bit more clear in your commit message.

(Sorry, forgot to address this comment in my initial reply)

The op will be available for video mode to use, but most likely video 
mode will set DATA_COMPRESS (or call dpu_hw_intf_enable_compression()) 
directly in dpu_hw_intf_setup_timing_engine().

Thanks,

Jessica Zhang

>>
>>> Changes in v2:
>>> - Fixed whitespace issue in macro definition
>>> - Read INTF_CONFIG2 before writing to DATA_COMPRESS bit
>>> - Only set dpu_hw_intf_ops.data_compress if DATA_COMPRESS feature is set
>>> - Removed `inline` from dpu_hw_intf_enable_compression declaration
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  3 +++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 11 +++++++++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  2 ++
>>>   3 files changed, 16 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 d8ed85a238af..1a4c20f02312 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
>>> @@ -68,6 +68,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>                   phys_enc->hw_intf,
>>>                   true,
>>>                   phys_enc->hw_pp->idx);
>>> +
>>> +    if (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 6485500eedb8..322c55a5042c 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> @@ -91,6 +91,14 @@
>>>   #define INTF_CFG2_DATABUS_WIDEN    BIT(0)
>>>   #define INTF_CFG2_DATA_HCTL_EN    BIT(4)
>>> +#define INTF_CFG2_DCE_DATA_COMPRESS     BIT(12)
>>> +
>>> +static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>>> +{
>>> +    u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>>> +
>>> +    DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2 | 
>>> INTF_CFG2_DCE_DATA_COMPRESS);
>>
>> I'm not sure if it's more idiomatic to write:
>>
>>      intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>
>> On a separate line.
> 
> Hi Marijn,
> 
> Sounds good.
> 
>>
>>> +}
>>
>> Move the function close to the bottom of this file.  Right now all the
>> functions are defined approximately in the same order as they're listed
>> in the header and assigned in _setup_intf_ops().
> 
> Acked.
> 
>>
>>>   static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>>           const struct intf_timing_params *p,
>>> @@ -542,6 +550,9 @@ static void _setup_intf_ops(struct 
>>> dpu_hw_intf_ops *ops,
>>>           ops->vsync_sel = dpu_hw_intf_vsync_sel;
>>>           ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>>>       }
>>> +
>>> +    if (cap & BIT(DPU_INTF_DATA_COMPRESS))
>>> +        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 73b0885918f8..a8def68a5ec2 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> @@ -70,6 +70,7 @@ struct intf_status {
>>>    * @get_autorefresh:            Retrieve autorefresh config from 
>>> hardware
>>>    *                              Return: 0 on success, -ETIMEDOUT on 
>>> timeout
>>>    * @vsync_sel:                  Select vsync signal for tear-effect 
>>> configuration
>>> + * @enable_compression: Enable data compression
>>
>> Indent to match above.
> 
> Sure, is the plan to correct the whitespace in the first half of the 
> comment block in the future?
> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>> - Marijn
>>
>>>    */
>>>   struct dpu_hw_intf_ops {
>>>       void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>>> @@ -107,6 +108,7 @@ struct dpu_hw_intf_ops {
>>>        * Disable autorefresh if enabled
>>>        */
>>>       void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t 
>>> encoder_id, u16 vdisplay);
>>> +    void (*enable_compression)(struct dpu_hw_intf *intf);
>>>   };
>>>   struct dpu_hw_intf {
>>>
>>> -- 
>>> 2.40.1
>>>

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Abhinav Kumar]

On 5/8/2023 4:08 PM, Dmitry Baryshkov wrote:
> On 09/05/2023 00:46, Jessica Zhang wrote:
>>
>>
>> On 5/7/2023 9:00 AM, Marijn Suijten wrote:
>>> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>>>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>>>
>>>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
>>>> PINGPONG to INTF.
>>>>
>>>> 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 feature flag.
>>>
>>> Irrelevant.  Even though core_rev was still in mainline until recently,
>>> we always hardcoded the features in the catalog and only used core_rev
>>> to select a dpu_mdss_cfg catalog entry.  There is no "if version >= X
>>> then enable feature Y" logic, this manually-enabled feature flag is the
>>> only, correct way to do it.
>>
>> Hi Marijn,
>>
>> Understood. FWIW, if we do find more register bit-level differences 
>> between HW versions in the future, it might make more sense to keep 
>> the HW catalog small and bring core_rev back, rather than keep adding 
>> these kinds of small differences to caps.
> 
> Let's see how it goes. Abhinav suggested that there might be feature 
> differences inside the DPU generations (and even inside the single DPU 
> major/minor combo). So I'm not sure what core_rev will bring us.
> 

It allows us to have if MDSS_REV() checks which are convenient for some 
calculations / bit programming which we dont want to expose in the 
catalog as they cannot be classified as a hw cap as such or atleast we 
dont want them to be classified as such.

> Let's land the platforms which are ready (or if there is anything close 
> to be submitted). I'll post the next proposal for the catalog cleanups 
> close to -rc4, when the dust settles then we can have one or two weaks 
> for the discussion and polishing.
> 
> I'd like to consider:
> - inlining foo_BLK macros, if that makes adding new features easier
> - reformat of clk_ctrls
> - maybe reintroduction of per-generation feature masks instead of 
> keeping them named after the random SoC
> - maybe a rework of mdss_irqs / INTFn_INTR. We already have this info in 
> hw catalog.
> 
> Comments are appreciated.
> 

I would say, lets wait for DSC to settle. Atleast the parts already on 
the list. Continuous rebase of features already on the list is becoming 
time consuming because of overlapping catalog reworks.

> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>> Changes in v2:
>>>> - Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature 
>>>> flag
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>
>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> index 7944481d0a33..c74051906d05 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> @@ -104,7 +104,7 @@
>>>>   #define INTF_SC7180_MASK \
>>>>       (BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE) | 
>>>> BIT(DPU_INTF_STATUS_SUPPORTED))
>>>> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>>>> +#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | 
>>>> BIT(DPU_INTF_DATA_COMPRESS)
>>>
>>> Konrad: Your SM6350/SM6375 series v3 [1] switched from INTF_SC7180_MASK
>>> to INTF_SC7280_MASK to enable HCTL on SM6375, but that will now
>>> erroneously also receive this feature flag and write the new
>>> DATA_COMPESS mask even if it's DPU 6.9 (< 7.x where it got added).
>>>
>>> [1]: 
>>> https://lore.kernel.org/linux-arm-msm/80b46fcb-d6d0-1998-c273-5401fa924c7d@linaro.org/T/#u 
>>>
>>>
>>> Depending on who lands first, this flag should be split.
>>>
>>> I still see value in inlining and removing these defines, though that
>>> brings a host of other complexity.
>>>
>>> - Marijn
>>>
>>>>   #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
>>>>                BIT(DPU_WB_UBWC) | \
>>>> 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 4eda2cc847ef..01c65f940f2a 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>> @@ -185,6 +185,7 @@ enum {
>>>>    * @DPU_DATA_HCTL_EN                Allows data to be transferred 
>>>> at different rate
>>>>    *                                  than video timing
>>>>    * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS 
>>>> register
>>>> + * @DPU_INTF_DATA_COMPRESS          INTF block has DATA_COMPRESS 
>>>> register
>>>>    * @DPU_INTF_MAX
>>>>    */
>>>>   enum {
>>>> @@ -192,6 +193,7 @@ enum {
>>>>       DPU_INTF_TE,
>>>>       DPU_DATA_HCTL_EN,
>>>>       DPU_INTF_STATUS_SUPPORTED,
>>>> +    DPU_INTF_DATA_COMPRESS,
>>>>       DPU_INTF_MAX
>>>>   };
>>>>
>>>> -- 
>>>> 2.40.1
>>>>
> 

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Dmitry Baryshkov]

On 09/05/2023 03:28, Abhinav Kumar wrote:
> 
> 
> On 5/8/2023 4:08 PM, Dmitry Baryshkov wrote:
>> On 09/05/2023 00:46, Jessica Zhang wrote:
>>>
>>>
>>> On 5/7/2023 9:00 AM, Marijn Suijten wrote:
>>>> On 2023-05-05 14:23:50, Jessica Zhang wrote:
>>>>> Add DATA_COMPRESS feature flag to DPU INTF block.
>>>>>
>>>>> In DPU 7.x and later, DSC/DCE enablement registers have been moved 
>>>>> from
>>>>> PINGPONG to INTF.
>>>>>
>>>>> 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 feature flag.
>>>>
>>>> Irrelevant.  Even though core_rev was still in mainline until recently,
>>>> we always hardcoded the features in the catalog and only used core_rev
>>>> to select a dpu_mdss_cfg catalog entry.  There is no "if version >= X
>>>> then enable feature Y" logic, this manually-enabled feature flag is the
>>>> only, correct way to do it.
>>>
>>> Hi Marijn,
>>>
>>> Understood. FWIW, if we do find more register bit-level differences 
>>> between HW versions in the future, it might make more sense to keep 
>>> the HW catalog small and bring core_rev back, rather than keep adding 
>>> these kinds of small differences to caps.
>>
>> Let's see how it goes. Abhinav suggested that there might be feature 
>> differences inside the DPU generations (and even inside the single DPU 
>> major/minor combo). So I'm not sure what core_rev will bring us.
>>
> 
> It allows us to have if MDSS_REV() checks which are convenient for some 
> calculations / bit programming which we dont want to expose in the 
> catalog as they cannot be classified as a hw cap as such or atleast we 
> dont want them to be classified as such.
> 
>> Let's land the platforms which are ready (or if there is anything 
>> close to be submitted). I'll post the next proposal for the catalog 
>> cleanups close to -rc4, when the dust settles then we can have one or 
>> two weaks for the discussion and polishing.
>>
>> I'd like to consider:
>> - inlining foo_BLK macros, if that makes adding new features easier
>> - reformat of clk_ctrls
>> - maybe reintroduction of per-generation feature masks instead of 
>> keeping them named after the random SoC
>> - maybe a rework of mdss_irqs / INTFn_INTR. We already have this info 
>> in hw catalog.
>>
>> Comments are appreciated.
>>
> 
> I would say, lets wait for DSC to settle. Atleast the parts already on 
> the list. Continuous rebase of features already on the list is becoming 
> time consuming because of overlapping catalog reworks.

As I wrote, -rc4. Until that time, I'd expect DSC to be settled and 
accepted.

-- 
With best wishes
Dmitry

Re: [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode

[Marijn Suijten]

On 2023-05-08 16:17:54, Jessica Zhang wrote:
<snip>
> >> 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 73b0885918f8..a8def68a5ec2 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> >> @@ -70,6 +70,7 @@ struct intf_status {
> >>    * @get_autorefresh:            Retrieve autorefresh config from hardware
> >>    *                              Return: 0 on success, -ETIMEDOUT on timeout
> >>    * @vsync_sel:                  Select vsync signal for tear-effect configuration
> >> + * @enable_compression: Enable data compression
> > 
> > Indent to match above.
> 
> Sure, is the plan to correct the whitespace in the first half of the 
> comment block in the future?

I couldn't see the first part of the block in the diff context here, but
indeed that's a broken disaster so we will have to fix that up :(

I think it is fine to leave the latter ones as it is, as long as it is
consistent:

- Only using spaces;
- Colon directly after the word (and an @ before it, see kerneldoc
  specification);
- Aligned to existing entries.

- Marijn

Re: [Freedreno] [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode

[Marijn Suijten]

On 2023-05-08 17:00:12, Jessica Zhang wrote:
> On 5/8/2023 4:17 PM, Jessica Zhang wrote:
> > On 5/7/2023 9:06 AM, Marijn Suijten wrote:
> >> On 2023-05-05 14:23:51, Jessica Zhang wrote:
> >>> Add a DPU INTF op to set DATA_COMPRESS register for command mode 
> >>> panels if
> >>> the DPU_INTF_DATA_COMPRESS feature flag is set. This flag needs to be
> >>> enabled in order for DSC v1.2 to work.
> >>>
> >>> Note: These changes are for command mode only. Video mode changes will
> >>> be posted along with the DSC v1.2 support for DP.
> >>
> >> Nit: the "command mode" parts of both paragraphs only apply to the call
> >> in dpu_encoder_phys_cmd, right?  If so, and the INTF op remains the same
> >> for video mode (but only the call needs to be added to the
> >> dpu_encoder_phy_vid), make this a bit more clear in your commit message.
> 
> (Sorry, forgot to address this comment in my initial reply)
> 
> The op will be available for video mode to use, but most likely video 
> mode will set DATA_COMPRESS (or call dpu_hw_intf_enable_compression()) 
> directly in dpu_hw_intf_setup_timing_engine().

Sounsds good, if you can clarify something along those lines so that it
is clear that the call is valid on video mode too, and that the callback
is also available there.

e.g. 
- Drop "for command mode panels" from "op to set DATA_COMPRESS
  register";
- "Note: the op is currently called for command-mode encoders only,
  video mode changes..."

Thanks!

- Marijn

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Marijn Suijten]

On 2023-05-09 02:08:52, Dmitry Baryshkov wrote:
> On 09/05/2023 00:46, Jessica Zhang wrote:
> > 
> > 
> > On 5/7/2023 9:00 AM, Marijn Suijten wrote:
> >> On 2023-05-05 14:23:50, Jessica Zhang wrote:
> >>> Add DATA_COMPRESS feature flag to DPU INTF block.
> >>>
> >>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
> >>> PINGPONG to INTF.
> >>>
> >>> 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 feature flag.
> >>
> >> Irrelevant.  Even though core_rev was still in mainline until recently,
> >> we always hardcoded the features in the catalog and only used core_rev
> >> to select a dpu_mdss_cfg catalog entry.  There is no "if version >= X
> >> then enable feature Y" logic, this manually-enabled feature flag is the
> >> only, correct way to do it.
> > 
> > Hi Marijn,
> > 
> > Understood. FWIW, if we do find more register bit-level differences 
> > between HW versions in the future, it might make more sense to keep the 
> > HW catalog small and bring core_rev back, rather than keep adding these 
> > kinds of small differences to caps.
> 
> Let's see how it goes. Abhinav suggested that there might be feature 
> differences inside the DPU generations (and even inside the single DPU 
> major/minor combo). So I'm not sure what core_rev will bring us.
> 
> Let's land the platforms which are ready (or if there is anything close 
> to be submitted).

You mean waiting for catalog changes on the list specifically, so the
DSC series as well as SM6350/SM6375?  I do intend to send SM6125 now
that the INTF TE series (required for it, as well as for SM63**) seems
to be generally accepted, but have been quite busy with the DSC series
on the list as we're now unblocking many Xperia's to finally have
display!

The catalog addition is "pretty much ready", let me know if you'd like
it to be sent in prior to your cleanup.

> I'll post the next proposal for the catalog cleanups 
> close to -rc4, when the dust settles then we can have one or two weaks 
> for the discussion and polishing.
> 
> I'd like to consider:
> - inlining foo_BLK macros, if that makes adding new features easier

Sounds like a good idea.

> - reformat of clk_ctrls
> - maybe reintroduction of per-generation feature masks instead of 
> keeping them named after the random SoC

Yes that would make things more clear.  Or we can inline them entirely
though that might accidentally diverge SoCs and revisions.

> - maybe a rework of mdss_irqs / INTFn_INTR. We already have this info in 
> hw catalog.

Sounds good as well, that should get rid of some "duplication".

- Marijn

Re: [PATCH v2 3/4] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag

[Dmitry Baryshkov]

On Tue, 9 May 2023 at 10:00, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2023-05-09 02:08:52, Dmitry Baryshkov wrote:
> > On 09/05/2023 00:46, Jessica Zhang wrote:
> > >
> > >
> > > On 5/7/2023 9:00 AM, Marijn Suijten wrote:
> > >> On 2023-05-05 14:23:50, Jessica Zhang wrote:
> > >>> Add DATA_COMPRESS feature flag to DPU INTF block.
> > >>>
> > >>> In DPU 7.x and later, DSC/DCE enablement registers have been moved from
> > >>> PINGPONG to INTF.
> > >>>
> > >>> 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 feature flag.
> > >>
> > >> Irrelevant.  Even though core_rev was still in mainline until recently,
> > >> we always hardcoded the features in the catalog and only used core_rev
> > >> to select a dpu_mdss_cfg catalog entry.  There is no "if version >= X
> > >> then enable feature Y" logic, this manually-enabled feature flag is the
> > >> only, correct way to do it.
> > >
> > > Hi Marijn,
> > >
> > > Understood. FWIW, if we do find more register bit-level differences
> > > between HW versions in the future, it might make more sense to keep the
> > > HW catalog small and bring core_rev back, rather than keep adding these
> > > kinds of small differences to caps.
> >
> > Let's see how it goes. Abhinav suggested that there might be feature
> > differences inside the DPU generations (and even inside the single DPU
> > major/minor combo). So I'm not sure what core_rev will bring us.
> >
> > Let's land the platforms which are ready (or if there is anything close
> > to be submitted).
>
> You mean waiting for catalog changes on the list specifically, so the
> DSC series as well as SM6350/SM6375?  I do intend to send SM6125 now
> that the INTF TE series (required for it, as well as for SM63**) seems
> to be generally accepted, but have been quite busy with the DSC series
> on the list as we're now unblocking many Xperia's to finally have
> display!

Yes, please send it, as you find time. No time pressure.

>
> The catalog addition is "pretty much ready", let me know if you'd like
> it to be sent in prior to your cleanup.

-- 
With best wishes
Dmitry

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

[Jessica Zhang]

On 5/8/2023 2:56 PM, Marijn Suijten wrote:
> On 2023-05-05 14:49:08, Jessica Zhang wrote:
>> On 5/5/2023 2:23 PM, Jessica Zhang wrote:
>>> Adjust the pclk rate to divide hdisplay by the compression ratio when DSC
>>> is enabled.
>>>
>>> Changes in v2:
>>> - Adjusted pclk_rate math to divide only the hdisplay value by
>>>     compression ratio
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 17 +++++++++++++----
>>>    1 file changed, 13 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..0e5778e8091f 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,14 @@ 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 hdisplay by compression ratio */
>>> +	if (dsc) {
>>> +		int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * msm_dsc_get_bpp_int(dsc),
>>> +				dsc->bits_per_component * 3);
>>> +		int fps = DIV_ROUND_UP(pclk_rate, mode->htotal * mode->vtotal);
>>
>> Should've used drm_mode_vrefresh() here... Will spin a v3 with that
>> change (along with any additional comments)
> 
> Perhaps unsigned long on some of these?  Overall the computations and
> multi-lines look rather cluttered, perhaps (parts of) this is/are a
> prime candidate to go into the new helpers?

Hi Marijn,

Sorry for the late reply, wanted to get the MSM DSC helpers series 
settled first before addressing these changes.

Sounds good, I'll put these calculations in a separate 
dsi_adjust_compressed_pclk() helper.

> 
> Note that I cannot get the 4k mode working at 60Hz on one of my panels
> (30Hz works with minor corruption), regardless of this patch.  See also:
> https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1900031
As discussed elsewhere, we suspect that this is unrelated to DSC 
specifically and might be an issue with the upstream driver not taking 
transfer time into account with calculating pclk_rate.

We will look into this as a separate issue.

> 
>>> +		pclk_rate = (new_hdisplay + (mode->htotal - mode->hdisplay)) * mode->vtotal * fps;
>>> +	}
>>> +
>>>    	return pclk_rate;
>>>    }
>>>    
>>> @@ -585,7 +594,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 +613,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 +643,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;
> 
> Let's rebase on top of "drm/msm/dsi: simplify pixel clk rate handling"
> [1] to clean this up.
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20230118130031.2345941-1-dmitry.baryshkov@linaro.org/

I've looked into this patch and have made a comment on it. Just have 
some reservations about it as it changes the functionality of a clk 
handler op.

I will hold off on rebasing and wait for that thread to resolve first.

Thanks,

Jessica Zhang

> 
> - Marijn
> 
>>>    	do_div(pclk_bpp, 8);
>>>    	msm_host->src_clk_rate = pclk_bpp;
>>>    
>>>
>>> -- 
>>> 2.40.1
>>>

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

[Marijn Suijten]

On 2023-05-19 12:04:00, Jessica Zhang wrote:
<snip>
> >>> +	/* If DSC is enabled, divide hdisplay by compression ratio */
> >>> +	if (dsc) {
> >>> +		int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * msm_dsc_get_bpp_int(dsc),
> >>> +				dsc->bits_per_component * 3);
> >>> +		int fps = DIV_ROUND_UP(pclk_rate, mode->htotal * mode->vtotal);
> >>
> >> Should've used drm_mode_vrefresh() here... Will spin a v3 with that
> >> change (along with any additional comments)
> > 
> > Perhaps unsigned long on some of these?  Overall the computations and
> > multi-lines look rather cluttered, perhaps (parts of) this is/are a
> > prime candidate to go into the new helpers?
> 
> Hi Marijn,
> 
> Sorry for the late reply, wanted to get the MSM DSC helpers series 
> settled first before addressing these changes.

No hurry and no worry, that is exactly why I requested this to be split
across multiple series so that we can make progress on that in isolation
(or rather, make progress on the first series in the chain before
iterating on the next).  On the other hand Dmitry made the right remark
that it does cause contention for some patches that only become relevant
in future series... but that's mostly down to how the patches are
distributed across series.

> Sounds good, I'll put these calculations in a separate 
> dsi_adjust_compressed_pclk() helper.

Not sure if "adjust" carries the meaning, but I'll leave it to you to
come up with an initial revision and then we can discuss.  I am mostly
curious if there are generic (DSI) timing rules that apply DRM-wide, or
if these would be MSM-specific.

Otherwise assigning them to properly named local variables is the
perfect way to self-document the code.

> > Note that I cannot get the 4k mode working at 60Hz on one of my panels
> > (30Hz works with minor corruption), regardless of this patch.  See also:
> > https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1900031
> As discussed elsewhere, we suspect that this is unrelated to DSC 
> specifically and might be an issue with the upstream driver not taking 
> transfer time into account with calculating pclk_rate.
> 
> We will look into this as a separate issue.

Yes that is very likely, but it is still a good idea to take into
account when looking into adjusting DSC timing: can we do that in any
sensible way without first accounting for transfer time?

<snip>
> >>>    	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;
> > 
> > Let's rebase on top of "drm/msm/dsi: simplify pixel clk rate handling"
> > [1] to clean this up.
> > 
> > [1]: https://lore.kernel.org/linux-arm-msm/20230118130031.2345941-1-dmitry.baryshkov@linaro.org/
> 
> I've looked into this patch and have made a comment on it. Just have 
> some reservations about it as it changes the functionality of a clk 
> handler op.
> 
> I will hold off on rebasing and wait for that thread to resolve first.

Looks like the resolution was to drop it, but we should still first
apply the following hunk from it so that this line in your patch can be
skipped entirely.

    -	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
    +	pclk_bpp = msm_host->pixel_clk_rate * bpp;

- Marijn