dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/6] Add DSC v1.2 Support for DSI
@ 2023-06-09 22:57 Jessica Zhang
  2023-06-09 22:57 ` [PATCH v6 1/6] msm/drm/dsi: Round up DSC hdisplay calculation Jessica Zhang
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Jessica Zhang @ 2023-06-09 22:57 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
	David Airlie, Daniel Vetter, Marijn Suijten
  Cc: linux-arm-msm, linux-kernel, dri-devel, Konrad Dybcio,
	Jessica Zhang, freedreno

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

This includes:

1) Rounding up `hdisplay / 3` in dsc_timing_setup()
2) Adjusting pclk_rate to account for compression
3) Fixing incorrect uses of slice_count in DSI DSC calculations
4) 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]
 - "Introduce MSM-specific DSC helpers" [2]
 - "drm/msm/dsi: use mult_frac for pclk_bpp calculation" [3]

[1] https://patchwork.freedesktop.org/series/116789/
[2] https://patchwork.freedesktop.org/series/115833/
[3] https://patchwork.freedesktop.org/patch/538273/?series=118072&rev=1

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
Changes in v6:
- "Adjust" --> "Reduce" in pclk patch title (Marijn)
- dsi_adjust_compressed_pclk() --> dsi_adjust_pclk_for_compression()
  (Marijn)
- Moved dsi_adjust_pclk_for_compression() to before is_bonded_dsi pclk
  adjustment (Dmitry)
- Added documentation comments explaining the pclk_rate and hdisplay
  adjustments related to DSC (Dmitry)
- Only set DATA_COMPRESS bit if DSC is enabled (Abhinav)
- Rebased on top of latest msm-next-lumag branch
- Link to v5: https://lore.kernel.org/r/20230405-add-dsc-support-v5-0-028c10850491@quicinc.com

Changes in v5:
- Added newline before enable_compression() function pointer definition
- Rebased on top of "drm/msm/dsi: use mult_frac for pclk_bpp calculation"
- Reworded commit messages for clarity
- Dropped mentions of "soft slice" in commit messages
- "slice_per_packet" -> "slice_per_pkt"
- Picked up reviewed-by tags
- Link to v4: https://lore.kernel.org/r/20230405-add-dsc-support-v4-0-15daf84f8dcb@quicinc.com

Changes in v4:
- Clarified slice_per_pkt comment regarding pkt_per_line calculations
- Reworded commit message for "drm/msm/dsi: Remove incorrect references
  to slice_count"
- Wrapped INTF_SC7280_MASK macro definition in parentheses
- Fixed incorrect commit hash in "msm/drm/dsi: Round up DSC hdisplay
  calculation"
- Picked up Reviewed-by tag
- Link to v3: https://lore.kernel.org/r/20230405-add-dsc-support-v3-0-6e1d35a206b3@quicinc.com

Changes in v3:
- Added fix to round up hdisplay DSC adjustment
- Fixed inconsistent whitespace in dpu_hw_intf_ops comment doc
- Moved placement of dpu_hw_intf_enable_compression
- Picked up "drm/msm/dsi: Fix calculation for pkt_per_line" patch and
  squashed all slice_count fixes into a single patch
- Use drm_mode_vrefresh() to calculate adjusted pclk rate
- Moved compressed pclk adjustment to dsi_adjust_compressed_pclk() helper
- Rebased changes on top of updated dependencies
- Reworded commit message for "drm/msm/dpu: Set DATA_COMPRESS for
  command mode" for clarity
- Removed revision changelog in commit messages
- Link to v2: https://lore.kernel.org/r/20230405-add-dsc-support-v2-0-1072c70e9786@quicinc.com

Changes in v2:
- Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
- Changed pclk math to only divide hdisplay by compression ratio
- Reworded word count TODO comment
- Make DATA_COMPRESS an INTF flag
- Read INTF_CONFIG2 before writing to DATA_COMPRESS bit
- Fixed whitespace issue in macro definition
- Removed `inline` from dpu_hw_intf_enable_compression declaration
- Only set dpu_hw_intf_ops.data_compress if DATA_COMPRESS feature is set
- 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 (6):
      msm/drm/dsi: Round up DSC hdisplay calculation
      drm/msm/dsi: Reduce pclk rate for compression
      drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0
      drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2
      drm/msm/dsi: Remove incorrect references to slice_count
      drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations

 .../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        | 13 +++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  3 ++
 drivers/gpu/drm/msm/dsi/dsi_host.c                 | 59 +++++++++++++++++-----
 6 files changed, 67 insertions(+), 15 deletions(-)
---
base-commit: 8dc20f06b90af85c083866df73dae69236183b62
change-id: 20230405-add-dsc-support-fe130ba49841

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v6 1/6] msm/drm/dsi: Round up DSC hdisplay calculation
  2023-06-09 22:57 [PATCH v6 0/6] Add DSC v1.2 Support for DSI Jessica Zhang
@ 2023-06-09 22:57 ` Jessica Zhang
  2023-06-09 22:57 ` [PATCH v6 2/6] drm/msm/dsi: Reduce pclk rate for compression Jessica Zhang
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Jessica Zhang @ 2023-06-09 22:57 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
	David Airlie, Daniel Vetter, Marijn Suijten
  Cc: linux-arm-msm, linux-kernel, dri-devel, Konrad Dybcio,
	Jessica Zhang, freedreno

Currently, when compression is enabled, hdisplay is reduced via integer
division. This causes issues for modes where the original hdisplay is
not a multiple of 3.

To fix this, use DIV_ROUND_UP to divide hdisplay.

Suggested-by: Marijn Suijten <marijn.suijten@somainline.org>
Fixes: 08802f515c3cf ("drm/msm/dsi: Add support for DSC configuration")
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 1a99d75025dc..a448931af804 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -949,7 +949,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		 * pulse width same
 		 */
 		h_total -= hdisplay;
-		hdisplay = msm_dsc_get_bytes_per_line(msm_host->dsc) / 3;
+		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
 		h_total += hdisplay;
 		ha_end = ha_start + hdisplay;
 	}

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v6 2/6] drm/msm/dsi: Reduce pclk rate for compression
  2023-06-09 22:57 [PATCH v6 0/6] Add DSC v1.2 Support for DSI Jessica Zhang
  2023-06-09 22:57 ` [PATCH v6 1/6] msm/drm/dsi: Round up DSC hdisplay calculation Jessica Zhang
@ 2023-06-09 22:57 ` Jessica Zhang
  2023-06-09 23:09   ` Abhinav Kumar
  2023-06-09 22:57 ` [PATCH v6 3/6] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0 Jessica Zhang
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Jessica Zhang @ 2023-06-09 22:57 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
	David Airlie, Daniel Vetter, Marijn Suijten
  Cc: linux-arm-msm, linux-kernel, dri-devel, Konrad Dybcio,
	Jessica Zhang, freedreno

Adjust the pclk rate to divide hdisplay 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 | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a448931af804..98ea1da492c7 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -561,12 +561,27 @@ 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_adjust_pclk_for_compression(const struct drm_display_mode *mode,
+		const struct drm_dsc_config *dsc)
+{
+	int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
+			dsc->bits_per_component * 3);
+
+	int new_htotal = mode->htotal - mode->hdisplay + new_hdisplay;
+
+	return new_htotal * mode->vtotal * drm_mode_vrefresh(mode);
+}
+
+static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode,
+		const struct drm_dsc_config *dsc, bool is_bonded_dsi)
 {
 	unsigned long pclk_rate;
 
 	pclk_rate = mode->clock * 1000;
 
+	if (dsc)
+		pclk_rate = dsi_adjust_pclk_for_compression(mode, dsc);
+
 	/*
 	 * For bonded DSI mode, the current DRM mode has the complete width of the
 	 * panel. Since, the complete panel is driven by two DSI controllers,
@@ -585,7 +600,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);
 	unsigned long pclk_bpp;
 
 	if (lanes == 0) {
@@ -604,7 +619,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);
 

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v6 3/6] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0
  2023-06-09 22:57 [PATCH v6 0/6] Add DSC v1.2 Support for DSI Jessica Zhang
  2023-06-09 22:57 ` [PATCH v6 1/6] msm/drm/dsi: Round up DSC hdisplay calculation Jessica Zhang
  2023-06-09 22:57 ` [PATCH v6 2/6] drm/msm/dsi: Reduce pclk rate for compression Jessica Zhang
@ 2023-06-09 22:57 ` Jessica Zhang
  2023-06-09 22:57 ` [PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2 Jessica Zhang
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Jessica Zhang @ 2023-06-09 22:57 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
	David Airlie, Daniel Vetter, Marijn Suijten
  Cc: linux-arm-msm, linux-kernel, dri-devel, Konrad Dybcio,
	Jessica Zhang, freedreno

In DPU 7.x and later, DSC/DCE enablement registers have been moved from
PINGPONG to INTF. Thus, add a DPU_INTF_DATA_COMPRESS feature flag that will
be set if the DATA_COMPRESS register is in the INTF block.

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
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 008df60b00f0..36ba3f58dcdf 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -105,7 +105,7 @@
 	 BIT(DPU_INTF_STATUS_SUPPORTED) | \
 	 BIT(DPU_DATA_HCTL_EN))
 
-#define INTF_SC7280_MASK (INTF_SC7180_MASK)
+#define INTF_SC7280_MASK (INTF_SC7180_MASK | 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 d3598dd9d448..b860784ade72 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -181,6 +181,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 {
@@ -188,6 +189,7 @@ enum {
 	DPU_INTF_TE,
 	DPU_DATA_HCTL_EN,
 	DPU_INTF_STATUS_SUPPORTED,
+	DPU_INTF_DATA_COMPRESS,
 	DPU_INTF_MAX
 };
 

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2
  2023-06-09 22:57 [PATCH v6 0/6] Add DSC v1.2 Support for DSI Jessica Zhang
                   ` (2 preceding siblings ...)
  2023-06-09 22:57 ` [PATCH v6 3/6] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0 Jessica Zhang
@ 2023-06-09 22:57 ` Jessica Zhang
  2023-06-09 23:08   ` Abhinav Kumar
  2023-06-13 19:04   ` Marijn Suijten
  2023-06-09 22:57 ` [PATCH v6 5/6] drm/msm/dsi: Remove incorrect references to slice_count Jessica Zhang
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 16+ messages in thread
From: Jessica Zhang @ 2023-06-09 22:57 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
	David Airlie, Daniel Vetter, Marijn Suijten
  Cc: linux-arm-msm, linux-kernel, dri-devel, Konrad Dybcio,
	Jessica Zhang, freedreno

Add a DPU INTF op to set the DCE_DATA_COMPRESS bit to enable the
DCE/DSC 1.2 datapath

Note: For now, this op is called for command mode encoders only. Changes to
set DATA_COMPRESS for video mode encoders will be posted along with DSC
v1.2 support for DP.

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          | 13 +++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  3 +++
 3 files changed, 19 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 63ba0082b6ee..b856c6286c85 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
@@ -67,6 +67,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
 		phys_enc->hw_intf->ops.bind_pingpong_blk(
 				phys_enc->hw_intf,
 				phys_enc->hw_pp->idx);
+
+	if (intf_cfg.dsc != 0 && 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 530f82e34c1e..5b0f6627e29b 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,7 @@
 
 #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_setup_timing_engine(struct dpu_hw_intf *ctx,
@@ -512,6 +513,15 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
 
 }
 
+static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
+{
+	u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
+
+	intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
+
+	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
+}
+
 static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
 		unsigned long cap)
 {
@@ -532,6 +542,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 33895eca1211..99e21c4137f9 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,
@@ -106,6 +107,8 @@ 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v6 5/6] drm/msm/dsi: Remove incorrect references to slice_count
  2023-06-09 22:57 [PATCH v6 0/6] Add DSC v1.2 Support for DSI Jessica Zhang
                   ` (3 preceding siblings ...)
  2023-06-09 22:57 ` [PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2 Jessica Zhang
@ 2023-06-09 22:57 ` Jessica Zhang
  2023-06-09 22:57 ` [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations Jessica Zhang
  2023-06-15 11:31 ` [PATCH v6 0/6] Add DSC v1.2 Support for DSI Dmitry Baryshkov
  6 siblings, 0 replies; 16+ messages in thread
From: Jessica Zhang @ 2023-06-09 22:57 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
	David Airlie, Daniel Vetter, Marijn Suijten
  Cc: linux-arm-msm, linux-kernel, dri-devel, Konrad Dybcio,
	Jessica Zhang, freedreno

Currently, slice_count is being used to calculate word count and
pkt_per_line. Instead, these values should be calculated using slice per
packet, which is not the same as slice_count.

Slice count represents the number of 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 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 aforementioned calculations.

Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 98ea1da492c7..fb1d3a25765f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -863,18 +863,17 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	 */
 	slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
 
-	/*
-	 * If slice_count is greater than slice_per_intf
-	 * then default to 1. This can happen during partial
-	 * update.
-	 */
-	if (dsc->slice_count > slice_per_intf)
-		dsc->slice_count = 1;
-
 	total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
 
 	eol_byte_num = total_bytes_per_intf % 3;
-	pkt_per_line = slice_per_intf / dsc->slice_count;
+
+	/*
+	 * Typically, pkt_per_line = slice_per_intf * slice_per_pkt.
+	 *
+	 * Since the current driver only supports slice_per_pkt = 1,
+	 * pkt_per_line will be equal to slice per intf for now.
+	 */
+	pkt_per_line = slice_per_intf;
 
 	if (is_cmd_mode) /* packet data type */
 		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
@@ -998,7 +997,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_pkt + 1.
+			 * Currently, the driver only supports default value of slice_per_pkt = 1
+			 *
+			 * TODO: Expand mipi_dsi_device struct to hold slice_per_pkt info
+			 *       and adjust DSC math to account for slice_per_pkt.
+			 */
+			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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
  2023-06-09 22:57 [PATCH v6 0/6] Add DSC v1.2 Support for DSI Jessica Zhang
                   ` (4 preceding siblings ...)
  2023-06-09 22:57 ` [PATCH v6 5/6] drm/msm/dsi: Remove incorrect references to slice_count Jessica Zhang
@ 2023-06-09 22:57 ` Jessica Zhang
  2023-06-09 23:30   ` Dmitry Baryshkov
  2023-06-11 22:03   ` Marijn Suijten
  2023-06-15 11:31 ` [PATCH v6 0/6] Add DSC v1.2 Support for DSI Dmitry Baryshkov
  6 siblings, 2 replies; 16+ messages in thread
From: Jessica Zhang @ 2023-06-09 22:57 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
	David Airlie, Daniel Vetter, Marijn Suijten
  Cc: linux-arm-msm, linux-kernel, dri-devel, Konrad Dybcio,
	Jessica Zhang, freedreno

Add documentation comments explaining the pclk_rate and hdisplay math
related to DSC.

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

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index fb1d3a25765f..aeaadc18bc7b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -564,6 +564,13 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
 static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
 		const struct drm_dsc_config *dsc)
 {
+	/*
+	 * Adjust the pclk rate by calculating a new hdisplay proportional to
+	 * the compression ratio such that:
+	 *     new_hdisplay = old_hdisplay * target_bpp / source_bpp
+	 *
+	 * Porches need not be adjusted during compression.
+	 */
 	int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
 			dsc->bits_per_component * 3);
 
@@ -961,6 +968,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 
 		/* Divide the display by 3 but keep back/font porch and
 		 * pulse width same
+		 *
+		 * hdisplay will be divided by 3 here to account for the fact
+		 * that DPU sends 3 bytes per pclk cycle to DSI.
 		 */
 		h_total -= hdisplay;
 		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2
  2023-06-09 22:57 ` [PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2 Jessica Zhang
@ 2023-06-09 23:08   ` Abhinav Kumar
  2023-06-13 19:04   ` Marijn Suijten
  1 sibling, 0 replies; 16+ messages in thread
From: Abhinav Kumar @ 2023-06-09 23:08 UTC (permalink / raw)
  To: Jessica Zhang, Rob Clark, Dmitry Baryshkov, Sean Paul,
	David Airlie, Daniel Vetter, Marijn Suijten
  Cc: Konrad Dybcio, linux-arm-msm, freedreno, linux-kernel, dri-devel



On 6/9/2023 3:57 PM, Jessica Zhang wrote:
> Add a DPU INTF op to set the DCE_DATA_COMPRESS bit to enable the
> DCE/DSC 1.2 datapath
> 
> Note: For now, this op is called for command mode encoders only. Changes to
> set DATA_COMPRESS for video mode encoders will be posted along with DSC
> v1.2 support for DP.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 2/6] drm/msm/dsi: Reduce pclk rate for compression
  2023-06-09 22:57 ` [PATCH v6 2/6] drm/msm/dsi: Reduce pclk rate for compression Jessica Zhang
@ 2023-06-09 23:09   ` Abhinav Kumar
  0 siblings, 0 replies; 16+ messages in thread
From: Abhinav Kumar @ 2023-06-09 23:09 UTC (permalink / raw)
  To: Jessica Zhang, Rob Clark, Dmitry Baryshkov, Sean Paul,
	David Airlie, Daniel Vetter, Marijn Suijten
  Cc: Konrad Dybcio, linux-arm-msm, freedreno, linux-kernel, dri-devel



On 6/9/2023 3:57 PM, Jessica Zhang wrote:
> Adjust the pclk rate to divide hdisplay by the compression ratio when DSC
> is enabled.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
  2023-06-09 22:57 ` [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations Jessica Zhang
@ 2023-06-09 23:30   ` Dmitry Baryshkov
  2023-06-11 22:03   ` Marijn Suijten
  1 sibling, 0 replies; 16+ messages in thread
From: Dmitry Baryshkov @ 2023-06-09 23:30 UTC (permalink / raw)
  To: Jessica Zhang, Rob Clark, Abhinav Kumar, Sean Paul, David Airlie,
	Daniel Vetter, Marijn Suijten
  Cc: Konrad Dybcio, linux-arm-msm, freedreno, linux-kernel, dri-devel

On 10/06/2023 01:57, Jessica Zhang wrote:
> Add documentation comments explaining the pclk_rate and hdisplay math
> related to DSC.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index fb1d3a25765f..aeaadc18bc7b 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -564,6 +564,13 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
>   static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
>   		const struct drm_dsc_config *dsc)
>   {
> +	/*
> +	 * Adjust the pclk rate by calculating a new hdisplay proportional to
> +	 * the compression ratio such that:
> +	 *     new_hdisplay = old_hdisplay * target_bpp / source_bpp

I'd say `* compressed_bpp / uncompressed_bpp'. This is easier to follow 
than source and target.

> +	 *
> +	 * Porches need not be adjusted during compression.
> +	 */
>   	int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
>   			dsc->bits_per_component * 3);
>   
> @@ -961,6 +968,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   
>   		/* Divide the display by 3 but keep back/font porch and
>   		 * pulse width same
> +		 *
> +		 * hdisplay will be divided by 3 here to account for the fact
> +		 * that DPU sends 3 bytes per pclk cycle to DSI.
>   		 */
>   		h_total -= hdisplay;
>   		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> 

-- 
With best wishes
Dmitry


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
  2023-06-09 22:57 ` [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations Jessica Zhang
  2023-06-09 23:30   ` Dmitry Baryshkov
@ 2023-06-11 22:03   ` Marijn Suijten
  2023-06-12 17:26     ` [Freedreno] " Abhinav Kumar
  1 sibling, 1 reply; 16+ messages in thread
From: Marijn Suijten @ 2023-06-11 22:03 UTC (permalink / raw)
  To: Jessica Zhang
  Cc: freedreno, Sean Paul, Abhinav Kumar, dri-devel, linux-kernel,
	Konrad Dybcio, linux-arm-msm, Dmitry Baryshkov

On 2023-06-09 15:57:18, Jessica Zhang wrote:
> Add documentation comments explaining the pclk_rate and hdisplay math
> related to DSC.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index fb1d3a25765f..aeaadc18bc7b 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -564,6 +564,13 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
>  static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
>  		const struct drm_dsc_config *dsc)
>  {
> +	/*
> +	 * Adjust the pclk rate by calculating a new hdisplay proportional to
> +	 * the compression ratio such that:
> +	 *     new_hdisplay = old_hdisplay * target_bpp / source_bpp
> +	 *
> +	 * Porches need not be adjusted during compression.
> +	 */
>  	int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
>  			dsc->bits_per_component * 3);

I won't reiterate my original troubles with this logic and the comment
as that has well been described in v5 replies.

Just want to ask why this comment couldn't be added in patch 5/6
immediately when the logic is introduced?  Now readers won't have a clue
what is going on until they skip one patch ahead.

Furthermore it is lacking any explanation that this is a workaround for
cmd-mode, and that porches are currently used to represent "transfer
time" until those calculations are implemented.  At that point there is
no concept of "not adjusting porches for compressed signals" anymore.

>  
> @@ -961,6 +968,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  
>  		/* Divide the display by 3 but keep back/font porch and
>  		 * pulse width same
> +		 *
> +		 * hdisplay will be divided by 3 here to account for the fact
> +		 * that DPU sends 3 bytes per pclk cycle to DSI.
>  		 */
>  		h_total -= hdisplay;
>  		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);

Still very glad to have this, thank you for adding it.  Note that it
only further undermines the pclk adjustments, as I just explained in v5
review.

- Marijn

> 
> -- 
> 2.40.1
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Freedreno] [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
  2023-06-11 22:03   ` Marijn Suijten
@ 2023-06-12 17:26     ` Abhinav Kumar
  2023-06-12 18:04       ` Marijn Suijten
  0 siblings, 1 reply; 16+ messages in thread
From: Abhinav Kumar @ 2023-06-12 17:26 UTC (permalink / raw)
  To: Marijn Suijten, Jessica Zhang
  Cc: freedreno, linux-kernel, dri-devel, Konrad Dybcio, linux-arm-msm,
	Dmitry Baryshkov, Sean Paul



On 6/11/2023 3:03 PM, Marijn Suijten wrote:
> On 2023-06-09 15:57:18, Jessica Zhang wrote:
>> Add documentation comments explaining the pclk_rate and hdisplay math
>> related to DSC.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index fb1d3a25765f..aeaadc18bc7b 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -564,6 +564,13 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
>>   static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
>>   		const struct drm_dsc_config *dsc)
>>   {
>> +	/*
>> +	 * Adjust the pclk rate by calculating a new hdisplay proportional to
>> +	 * the compression ratio such that:
>> +	 *     new_hdisplay = old_hdisplay * target_bpp / source_bpp
>> +	 *
>> +	 * Porches need not be adjusted during compression.
>> +	 */
>>   	int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
>>   			dsc->bits_per_component * 3);
> 
> I won't reiterate my original troubles with this logic and the comment
> as that has well been described in v5 replies.
> 
> Just want to ask why this comment couldn't be added in patch 5/6
> immediately when the logic is introduced?  Now readers won't have a clue
> what is going on until they skip one patch ahead.
> 

Both myself and Dmitry discussed that in this particular case, we will 
add the documentation as a follow-up patch and merge it together. Not 
usually the process, but in this case, just decided to do it this way. 
The series will still be merged as one.

> Furthermore it is lacking any explanation that this is a workaround for
> cmd-mode, and that porches are currently used to represent "transfer
> time" until those calculations are implemented.  At that point there is
> no concept of "not adjusting porches for compressed signals" anymore.
> 

This is a much bigger topic and goes out of scope of this patch and 
series and I dont want to explain all that in this documentation patch.

If we explain that this is specific to command mode, what would the 
panel drivers fill out for porches . Obviously they cannot fill out a 0.

Coming to transfer time. Even if current panel drivers use 0 porches, 
the clock you get should still be sufficient for 60fps or a transfer 
time of 16.66ms.

Transfer time was a concept introduced for some specific command mode 
panels where we needed to finish transferring the frame even faster than 
16.66ms like 12ms or 13ms.

Yes, without that, upstream and downstream math doesnt match. But that 
doesnt mean its going to break the panels or that upstream math is 
wrong. If you think command mode porches should be 0, then this will 
give you the clk for 60fps. If you add some random porches, it will just 
give a faster clock.

Porches can be used instead of transfer time till we add that math but 
again, thats only needed for panels which need a faster transfer time 
than 16.66ms.

So we dont need to call this a workaround in my opinion at all (and hack 
as you called in v5 is totally out of proportion).

One could even argue that if the panel needs a transfer time faster than 
16.66ms, then the mode->clock should also be bumped up. Panels dont do 
that today either.

Hence, I am going to consider transfer time as an enhancement and not 
going to take that up in this series so I am not for adding that comment 
here.

And as I have explained, this patch is not a workaround either. Its just 
calculating the clock based on what we have today in the panel drivers.



>>   
>> @@ -961,6 +968,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>   
>>   		/* Divide the display by 3 but keep back/font porch and
>>   		 * pulse width same
>> +		 *
>> +		 * hdisplay will be divided by 3 here to account for the fact
>> +		 * that DPU sends 3 bytes per pclk cycle to DSI.
>>   		 */
>>   		h_total -= hdisplay;
>>   		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> 
> Still very glad to have this, thank you for adding it.  Note that it
> only further undermines the pclk adjustments, as I just explained in v5
> review.
> 
> - Marijn
> 
>>
>> -- 
>> 2.40.1
>>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Freedreno] [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
  2023-06-12 17:26     ` [Freedreno] " Abhinav Kumar
@ 2023-06-12 18:04       ` Marijn Suijten
  0 siblings, 0 replies; 16+ messages in thread
From: Marijn Suijten @ 2023-06-12 18:04 UTC (permalink / raw)
  To: Abhinav Kumar
  Cc: freedreno, linux-kernel, dri-devel, Konrad Dybcio, linux-arm-msm,
	Dmitry Baryshkov, Jessica Zhang, Sean Paul

On 2023-06-12 10:26:39, Abhinav Kumar wrote:
> 
> 
> On 6/11/2023 3:03 PM, Marijn Suijten wrote:
> > On 2023-06-09 15:57:18, Jessica Zhang wrote:
> >> Add documentation comments explaining the pclk_rate and hdisplay math
> >> related to DSC.
> >>
> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> index fb1d3a25765f..aeaadc18bc7b 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> @@ -564,6 +564,13 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
> >>   static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
> >>   		const struct drm_dsc_config *dsc)
> >>   {
> >> +	/*
> >> +	 * Adjust the pclk rate by calculating a new hdisplay proportional to
> >> +	 * the compression ratio such that:
> >> +	 *     new_hdisplay = old_hdisplay * target_bpp / source_bpp
> >> +	 *
> >> +	 * Porches need not be adjusted during compression.
> >> +	 */
> >>   	int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
> >>   			dsc->bits_per_component * 3);
> > 
> > I won't reiterate my original troubles with this logic and the comment
> > as that has well been described in v5 replies.
> > 
> > Just want to ask why this comment couldn't be added in patch 5/6
> > immediately when the logic is introduced?  Now readers won't have a clue
> > what is going on until they skip one patch ahead.
> > 
> 
> Both myself and Dmitry discussed that in this particular case, we will 
> add the documentation as a follow-up patch and merge it together. Not 
> usually the process, but in this case, just decided to do it this way. 
> The series will still be merged as one.

Just saying that it doesn't make much sense from a "reading the patches
after they've been merged" point of view, maybe there was a
misunderstanding here that Dmitry would have been okay with a followup
patch if the pclk patch got merged... But since it didn't, I would at
least prefer that to be squashed (but I am not the maintainer, so you'll
obviously take that with a grain of salt).

> > Furthermore it is lacking any explanation that this is a workaround for
> > cmd-mode, and that porches are currently used to represent "transfer
> > time" until those calculations are implemented.  At that point there is
> > no concept of "not adjusting porches for compressed signals" anymore.
> > 
> 
> This is a much bigger topic and goes out of scope of this patch and 
> series and I dont want to explain all that in this documentation patch.
> 
> If we explain that this is specific to command mode, what would the 
> panel drivers fill out for porches . Obviously they cannot fill out a 0.
> 
> Coming to transfer time. Even if current panel drivers use 0 porches, 
> the clock you get should still be sufficient for 60fps or a transfer 
> time of 16.66ms.
> 
> Transfer time was a concept introduced for some specific command mode 
> panels where we needed to finish transferring the frame even faster than 
> 16.66ms like 12ms or 13ms.

That probably explains why, for the same sofef01 Driver-IC for example,
but with slight variations in panels (and SoC...), I was able to achieve
60Hz on the Xperia 10 II with 0-porches, yet the Xperia 5 (and 10 III?)
require quite a bit more "pclk" to reach 60Hz.

(I don't think 10 III ever achieved 60Hz, but the clocktree and/or panel
 cmds might be wrong)

> Yes, without that, upstream and downstream math doesnt match. But that 
> doesnt mean its going to break the panels or that upstream math is 
> wrong. If you think command mode porches should be 0, then this will 
> give you the clk for 60fps. If you add some random porches, it will just 
> give a faster clock.

And exactly this little bit of text is what I'd like to see in a comment
:)

> Porches can be used instead of transfer time till we add that math but 
> again, thats only needed for panels which need a faster transfer time 
> than 16.66ms.

Same here, why can't we have this in a code comment?

> So we dont need to call this a workaround in my opinion at all (and hack 
> as you called in v5 is totally out of proportion).

Don't get me wrong, it is still a hack to only scale the hdisplay
portion without ever explaining in any comment why the porches are not
taken into account (the explanation you gave above is perfect!), instead
of calculating the right pclk from the get-go and ignoring the DRM mode
clock altogether.

> One could even argue that if the panel needs a transfer time faster than 
> 16.66ms, then the mode->clock should also be bumped up. Panels dont do 
> that today either.

But we cannot easily bump the `clock` member as that'd break the value
returned by drm_mode_vrefresh(), unless we also come up with a fake
porch in htotal or vtotal as that is what it will be divided by.

This ties into a recent discussion where "someone" mentioned that DRM
isn't really designed with CMD mode panels, in mind... and perhaps my
observations here are just scratching the surface?

> Hence, I am going to consider transfer time as an enhancement and not 
> going to take that up in this series so I am not for adding that comment 
> here.

No need to add a TODO stating that it needs to be added, but it'd be
good to at the very least explain ***why*** only the hdisplay portion is
scaled and not everything else?

> And as I have explained, this patch is not a workaround either. Its just 
> calculating the clock based on what we have today in the panel drivers.

Agree to disagree?  Beyond sending my take on these two patches as an
RFC, I don't think there's a point discussing this much furter as we
seem to disagree and misunderstand eachother on a fundamental level.
And we haven't even gotten into the "src_bpp / target_bpp" versus "24
bits per pclk" details yet.

- Marijn

> >> @@ -961,6 +968,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >>   
> >>   		/* Divide the display by 3 but keep back/font porch and
> >>   		 * pulse width same
> >> +		 *
> >> +		 * hdisplay will be divided by 3 here to account for the fact
> >> +		 * that DPU sends 3 bytes per pclk cycle to DSI.
> >>   		 */
> >>   		h_total -= hdisplay;
> >>   		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> > 
> > Still very glad to have this, thank you for adding it.  Note that it
> > only further undermines the pclk adjustments, as I just explained in v5
> > review.
> > 
> > - Marijn
> > 
> >>
> >> -- 
> >> 2.40.1
> >>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2
  2023-06-09 22:57 ` [PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2 Jessica Zhang
  2023-06-09 23:08   ` Abhinav Kumar
@ 2023-06-13 19:04   ` Marijn Suijten
  1 sibling, 0 replies; 16+ messages in thread
From: Marijn Suijten @ 2023-06-13 19:04 UTC (permalink / raw)
  To: Jessica Zhang
  Cc: freedreno, Sean Paul, Abhinav Kumar, dri-devel, linux-kernel,
	Konrad Dybcio, linux-arm-msm, Dmitry Baryshkov

On 2023-06-09 15:57:16, Jessica Zhang wrote:
> Add a DPU INTF op to set the DCE_DATA_COMPRESS bit to enable the
> DCE/DSC 1.2 datapath
> 
> Note: For now, this op is called for command mode encoders only. Changes to
> set DATA_COMPRESS for video mode encoders will be posted along with DSC
> v1.2 support for DP.
> 
> 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          | 13 +++++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  3 +++
>  3 files changed, 19 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 63ba0082b6ee..b856c6286c85 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
> @@ -67,6 +67,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>  		phys_enc->hw_intf->ops.bind_pingpong_blk(
>  				phys_enc->hw_intf,
>  				phys_enc->hw_pp->idx);
> +
> +	if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
> +		phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);

It was probably not necessary to drop this after adding dsc!=0:

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

>  }
>  
>  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 530f82e34c1e..5b0f6627e29b 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,7 @@
>  
>  #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_setup_timing_engine(struct dpu_hw_intf *ctx,
> @@ -512,6 +513,15 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>  
>  }
>  
> +static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
> +{
> +	u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
> +
> +	intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
> +
> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
> +}
> +
>  static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
>  		unsigned long cap)
>  {
> @@ -532,6 +542,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 33895eca1211..99e21c4137f9 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,
> @@ -106,6 +107,8 @@ 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
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 0/6] Add DSC v1.2 Support for DSI
  2023-06-09 22:57 [PATCH v6 0/6] Add DSC v1.2 Support for DSI Jessica Zhang
                   ` (5 preceding siblings ...)
  2023-06-09 22:57 ` [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations Jessica Zhang
@ 2023-06-15 11:31 ` Dmitry Baryshkov
  2023-06-15 12:25   ` Marijn Suijten
  6 siblings, 1 reply; 16+ messages in thread
From: Dmitry Baryshkov @ 2023-06-15 11:31 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Sean Paul, David Airlie, Daniel Vetter,
	Marijn Suijten, Jessica Zhang
  Cc: Konrad Dybcio, linux-arm-msm, freedreno, linux-kernel, dri-devel


On Fri, 09 Jun 2023 15:57:12 -0700, Jessica Zhang wrote:
> This is a series of changes for DSI to enable command mode support
> for DSC v1.2.
> 
> This includes:
> 
> 1) Rounding up `hdisplay / 3` in dsc_timing_setup()
> 2) Adjusting pclk_rate to account for compression
> 3) Fixing incorrect uses of slice_count in DSI DSC calculations
> 4) Setting the DATA_COMPRESS bit when DSC is enabled
> 
> [...]

Applied, thanks!

[1/6] msm/drm/dsi: Round up DSC hdisplay calculation
      https://gitlab.freedesktop.org/lumag/msm/-/commit/21bf617110ba
[2/6] drm/msm/dsi: Reduce pclk rate for compression
      https://gitlab.freedesktop.org/lumag/msm/-/commit/7c9e4a554d4a
[3/6] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0
      https://gitlab.freedesktop.org/lumag/msm/-/commit/22598cfc94bb
[4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2
      https://gitlab.freedesktop.org/lumag/msm/-/commit/1642b5803473
[5/6] drm/msm/dsi: Remove incorrect references to slice_count
      https://gitlab.freedesktop.org/lumag/msm/-/commit/155fa3a91d64

Note, patch 6 is skipped for now

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 0/6] Add DSC v1.2 Support for DSI
  2023-06-15 11:31 ` [PATCH v6 0/6] Add DSC v1.2 Support for DSI Dmitry Baryshkov
@ 2023-06-15 12:25   ` Marijn Suijten
  0 siblings, 0 replies; 16+ messages in thread
From: Marijn Suijten @ 2023-06-15 12:25 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: freedreno, Sean Paul, Abhinav Kumar, dri-devel, linux-kernel,
	Konrad Dybcio, linux-arm-msm, Jessica Zhang

On 2023-06-15 14:31:26, Dmitry Baryshkov wrote:
> 
> On Fri, 09 Jun 2023 15:57:12 -0700, Jessica Zhang wrote:
> > This is a series of changes for DSI to enable command mode support
> > for DSC v1.2.
> > 
> > This includes:
> > 
> > 1) Rounding up `hdisplay / 3` in dsc_timing_setup()
> > 2) Adjusting pclk_rate to account for compression
> > 3) Fixing incorrect uses of slice_count in DSI DSC calculations
> > 4) Setting the DATA_COMPRESS bit when DSC is enabled
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/6] msm/drm/dsi: Round up DSC hdisplay calculation
>       https://gitlab.freedesktop.org/lumag/msm/-/commit/21bf617110ba
> [2/6] drm/msm/dsi: Reduce pclk rate for compression
>       https://gitlab.freedesktop.org/lumag/msm/-/commit/7c9e4a554d4a
> [3/6] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0
>       https://gitlab.freedesktop.org/lumag/msm/-/commit/22598cfc94bb
> [4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2
>       https://gitlab.freedesktop.org/lumag/msm/-/commit/1642b5803473
> [5/6] drm/msm/dsi: Remove incorrect references to slice_count
>       https://gitlab.freedesktop.org/lumag/msm/-/commit/155fa3a91d64
> 
> Note, patch 6 is skipped for now

Note that we also haven't finished discussions on where the ratio in
patch 2/6 comes from and how that should be outlined in patch 6.
Related to the widebus patches which affect the ratio as well.

- Marijn

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2023-06-15 12:25 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-09 22:57 [PATCH v6 0/6] Add DSC v1.2 Support for DSI Jessica Zhang
2023-06-09 22:57 ` [PATCH v6 1/6] msm/drm/dsi: Round up DSC hdisplay calculation Jessica Zhang
2023-06-09 22:57 ` [PATCH v6 2/6] drm/msm/dsi: Reduce pclk rate for compression Jessica Zhang
2023-06-09 23:09   ` Abhinav Kumar
2023-06-09 22:57 ` [PATCH v6 3/6] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0 Jessica Zhang
2023-06-09 22:57 ` [PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2 Jessica Zhang
2023-06-09 23:08   ` Abhinav Kumar
2023-06-13 19:04   ` Marijn Suijten
2023-06-09 22:57 ` [PATCH v6 5/6] drm/msm/dsi: Remove incorrect references to slice_count Jessica Zhang
2023-06-09 22:57 ` [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations Jessica Zhang
2023-06-09 23:30   ` Dmitry Baryshkov
2023-06-11 22:03   ` Marijn Suijten
2023-06-12 17:26     ` [Freedreno] " Abhinav Kumar
2023-06-12 18:04       ` Marijn Suijten
2023-06-15 11:31 ` [PATCH v6 0/6] Add DSC v1.2 Support for DSI Dmitry Baryshkov
2023-06-15 12:25   ` Marijn Suijten

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).