* [PATCH v5 0/5] Add DSC v1.2 Support for DSI
@ 2023-05-23 1:08 Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 1/5] msm/drm/dsi: Round up DSC hdisplay calculation Jessica Zhang
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Jessica Zhang @ 2023-05-23 1:08 UTC (permalink / raw)
To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
David Airlie, Daniel Vetter, Marijn Suijten
Cc: Konrad Dybcio, linux-arm-msm, dri-devel, freedreno, linux-kernel,
Jessica Zhang
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 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 (5):
msm/drm/dsi: Round up DSC hdisplay calculation
drm/msm/dsi: Adjust 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
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 +-
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 | 49 +++++++++++++++-------
6 files changed, 58 insertions(+), 15 deletions(-)
---
base-commit: 12a0bf73039bd760c5e78d08109882aa628cce8c
change-id: 20230405-add-dsc-support-fe130ba49841
Best regards,
--
Jessica Zhang <quic_jesszhan@quicinc.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 1/5] msm/drm/dsi: Round up DSC hdisplay calculation
2023-05-23 1:08 [PATCH v5 0/5] Add DSC v1.2 Support for DSI Jessica Zhang
@ 2023-05-23 1:08 ` Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression Jessica Zhang
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Jessica Zhang @ 2023-05-23 1:08 UTC (permalink / raw)
To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
David Airlie, Daniel Vetter, Marijn Suijten
Cc: Konrad Dybcio, linux-arm-msm, dri-devel, freedreno, linux-kernel,
Jessica Zhang
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] 14+ messages in thread
* [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
2023-05-23 1:08 [PATCH v5 0/5] Add DSC v1.2 Support for DSI Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 1/5] msm/drm/dsi: Round up DSC hdisplay calculation Jessica Zhang
@ 2023-05-23 1:08 ` Jessica Zhang
2023-06-08 20:36 ` Marijn Suijten
2023-05-23 1:08 ` [PATCH v5 3/5] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0 Jessica Zhang
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Jessica Zhang @ 2023-05-23 1:08 UTC (permalink / raw)
To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
David Airlie, Daniel Vetter, Marijn Suijten
Cc: Konrad Dybcio, linux-arm-msm, dri-devel, freedreno, linux-kernel,
Jessica Zhang
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..88f370dd2ea1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -561,7 +561,18 @@ 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_compressed_pclk(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);
+
+ return (new_hdisplay + (mode->htotal - mode->hdisplay))
+ * 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;
@@ -576,6 +587,10 @@ 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)
+ pclk_rate = dsi_adjust_compressed_pclk(mode, dsc);
+
return pclk_rate;
}
@@ -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] 14+ messages in thread
* [PATCH v5 3/5] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0
2023-05-23 1:08 [PATCH v5 0/5] Add DSC v1.2 Support for DSI Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 1/5] msm/drm/dsi: Round up DSC hdisplay calculation Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression Jessica Zhang
@ 2023-05-23 1:08 ` Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 4/5] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2 Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 5/5] drm/msm/dsi: Remove incorrect references to slice_count Jessica Zhang
4 siblings, 0 replies; 14+ messages in thread
From: Jessica Zhang @ 2023-05-23 1:08 UTC (permalink / raw)
To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
David Airlie, Daniel Vetter, Marijn Suijten
Cc: Konrad Dybcio, linux-arm-msm, dri-devel, freedreno, linux-kernel,
Jessica Zhang
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 | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
2 files changed, 4 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 243399d09ffe..09c8c1672910 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,8 @@
#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 11610f7d3150..334e4ab7281a 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 4/5] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2
2023-05-23 1:08 [PATCH v5 0/5] Add DSC v1.2 Support for DSI Jessica Zhang
` (2 preceding siblings ...)
2023-05-23 1:08 ` [PATCH v5 3/5] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0 Jessica Zhang
@ 2023-05-23 1:08 ` Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 5/5] drm/msm/dsi: Remove incorrect references to slice_count Jessica Zhang
4 siblings, 0 replies; 14+ messages in thread
From: Jessica Zhang @ 2023-05-23 1:08 UTC (permalink / raw)
To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
David Airlie, Daniel Vetter, Marijn Suijten
Cc: Konrad Dybcio, linux-arm-msm, dri-devel, freedreno, linux-kernel,
Jessica Zhang
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.
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
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 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..a462c6780e6e 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,
const struct intf_timing_params *p,
@@ -522,6 +523,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)
{
@@ -542,6 +552,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..5e8e103e29ce 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,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] 14+ messages in thread
* [PATCH v5 5/5] drm/msm/dsi: Remove incorrect references to slice_count
2023-05-23 1:08 [PATCH v5 0/5] Add DSC v1.2 Support for DSI Jessica Zhang
` (3 preceding siblings ...)
2023-05-23 1:08 ` [PATCH v5 4/5] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2 Jessica Zhang
@ 2023-05-23 1:08 ` Jessica Zhang
4 siblings, 0 replies; 14+ messages in thread
From: Jessica Zhang @ 2023-05-23 1:08 UTC (permalink / raw)
To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
David Airlie, Daniel Vetter, Marijn Suijten
Cc: Konrad Dybcio, linux-arm-msm, dri-devel, freedreno, linux-kernel,
Jessica Zhang
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 88f370dd2ea1..919760740bba 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] 14+ messages in thread
* Re: [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
2023-05-23 1:08 ` [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression Jessica Zhang
@ 2023-06-08 20:36 ` Marijn Suijten
2023-06-09 0:56 ` [Freedreno] " Jessica Zhang
2023-06-09 16:58 ` Dmitry Baryshkov
0 siblings, 2 replies; 14+ messages in thread
From: Marijn Suijten @ 2023-06-08 20:36 UTC (permalink / raw)
To: Jessica Zhang
Cc: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
David Airlie, Daniel Vetter, Konrad Dybcio, linux-arm-msm,
dri-devel, freedreno, linux-kernel
Same title suggestion as earlier: s/adjust/reduce
On 2023-05-22 18:08:56, 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>
> ---
> 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..88f370dd2ea1 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -561,7 +561,18 @@ 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_compressed_pclk(const struct drm_display_mode *mode,
Nit: adjust_pclk_for_compression
As discussed before we realized that this change is more-or-less a hack,
since downstream calculates pclk quite differently - at least for
command-mode panels. Do you still intend to land this patch this way,
or go the proper route by introducing the right math from the get-go?
Or is the math at least correct for video-mode panels?
This function requires a documentation comment to explain that all.
> + const struct drm_dsc_config *dsc)
> +{
> + int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if
bits_per_component==8 is assumed. In fact, it then becomes identical
to the following line in dsi_host.c which you added previously:
hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
If not, what is the difference between these two calculations? Maybe
they both need to be in a properly-named helper.
> + dsc->bits_per_component * 3);
As we established in the drm/msm issue [2] there is currently a
confusion whether this /3 (and the /3 in dsi_timing_setup) come from the
ratio between dsi_get_bpp() and dsc->bpp or something else. Can you
clarify that with constants and comments?
[2]: https://gitlab.freedesktop.org/drm/msm/-/issues/24
> +
> + return (new_hdisplay + (mode->htotal - mode->hdisplay))
> + * mode->vtotal * drm_mode_vrefresh(mode);
As clarified in [1] I was not necessarily suggesting to move this math
to a separate helper, but to also use a few more properly-named
intermediate variables to not have multi-line math and self-documenting
code. These lines could be split to be much more clear.
[1]: https://lore.kernel.org/linux-arm-msm/u4x2vldkzsokfcpbkz3dtwcllbdk4ljcz6kzuaxt5frx6g76o5@uku6abewvye7/
> +}
> +
> +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;
>
> @@ -576,6 +587,10 @@ 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)
> + pclk_rate = dsi_adjust_compressed_pclk(mode, dsc);
The confusion with this comment (and the reason the aforementioned
discussion [2] carried on so long) stems from the fact a division makes
sense for a bit/byte clock, but not for a pixel clock: we still intend
to send the same number of pixels, just spending less bytes on them. So
as you clarify the /3 above, can you also clarify that here or drop this
comment completely when the function is correctly documented instead?
- Marijn
> +
> return pclk_rate;
> }
>
> @@ -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 [flat|nested] 14+ messages in thread
* Re: [Freedreno] [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
2023-06-08 20:36 ` Marijn Suijten
@ 2023-06-09 0:56 ` Jessica Zhang
2023-06-09 1:09 ` Abhinav Kumar
2023-06-11 21:59 ` Marijn Suijten
2023-06-09 16:58 ` Dmitry Baryshkov
1 sibling, 2 replies; 14+ messages in thread
From: Jessica Zhang @ 2023-06-09 0:56 UTC (permalink / raw)
To: Marijn Suijten
Cc: freedreno, Sean Paul, Abhinav Kumar, dri-devel, linux-kernel,
Konrad Dybcio, Rob Clark, Daniel Vetter, linux-arm-msm,
Dmitry Baryshkov, David Airlie
On 6/8/2023 1:36 PM, Marijn Suijten wrote:
> Same title suggestion as earlier: s/adjust/reduce
Hi Marijn,
Acked.
>
> On 2023-05-22 18:08:56, 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>
>> ---
>> 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..88f370dd2ea1 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -561,7 +561,18 @@ 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_compressed_pclk(const struct drm_display_mode *mode,
>
> Nit: adjust_pclk_for_compression
Acked.
>
> As discussed before we realized that this change is more-or-less a hack,
> since downstream calculates pclk quite differently - at least for
> command-mode panels. Do you still intend to land this patch this way,
> or go the proper route by introducing the right math from the get-go?
> Or is the math at least correct for video-mode panels?
Sorry but can you please clarify what exactly is incorrect or different
about this math when compared to downstream? And, if you think that this
math is incorrect, what exactly has to be changed to make it the "right
math"?
We've already shown step-by-step [1] not only how the resulting pclk
from the downstream code matches out upstream calculations, but also how
the inclusion of porches in the upstream math would make up for the fact
that upstream has no concept of transfer time [2].
If the lack of transfer time in the upstream math is the issue, I
believe that that's not within the scope of this series, as transfer
time is not something specific to DSC.
Moreover, as stated in an earlier revision [3], there is no way to
validate DSC over DSI for video mode. As far as I know, we do not have a
way to validate video mode datapath for DSI in general.
[1] https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1936144
[2] https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1945792
[3]
https://patchwork.freedesktop.org/patch/535117/?series=117219&rev=1#comment_970492
>
> This function requires a documentation comment to explain that all.
>
>> + const struct drm_dsc_config *dsc)
>> +{
>> + int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
>
> This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if
> bits_per_component==8 is assumed. In fact, it then becomes identical
> to the following line in dsi_host.c which you added previously:
>
> hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>
> If not, what is the difference between these two calculations? Maybe
> they both need to be in a properly-named helper.
While the math technically matches up (assuming, also, that
mode->hdisplay == slice_width * slice_count for all cases), there are
conceptual differences between the pclk and hdisplay calculations.
Just to reiterate what was already said on IRC:
In the pclk calculation, we're multiplying pclk by the compression ratio
(which would be target_bpp / src_bpp) -- please note that here, we
calculate src_bpp by doing bpc * 3.
In the hdisplay calculation, we calculate the bytes per line and divide
by 3 (bytes) to account for the fact that we can only process 3 bytes
per pclk cycle.
So while I understand why you would want to put this behind a shared
helper, I think abstracting the pclk and hdisplay math away would
obfuscate the conceptual difference between the 2 calculations.
>
>> + dsc->bits_per_component * 3);
>
> As we established in the drm/msm issue [2] there is currently a
> confusion whether this /3 (and the /3 in dsi_timing_setup) come from the
> ratio between dsi_get_bpp() and dsc->bpp or something else. Can you
> clarify that with constants and comments?
Sure, we are planning to add a patch to the end of this series
documenting the math.
>
> [2]: https://gitlab.freedesktop.org/drm/msm/-/issues/24
>
>> +
>> + return (new_hdisplay + (mode->htotal - mode->hdisplay))
>> + * mode->vtotal * drm_mode_vrefresh(mode);
>
> As clarified in [1] I was not necessarily suggesting to move this math
> to a separate helper, but to also use a few more properly-named
> intermediate variables to not have multi-line math and self-documenting
> code. These lines could be split to be much more clear.
Acked.
>
> [1]: https://lore.kernel.org/linux-arm-msm/u4x2vldkzsokfcpbkz3dtwcllbdk4ljcz6kzuaxt5frx6g76o5@uku6abewvye7/
>
>> +}
>> +
>> +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;
>>
>> @@ -576,6 +587,10 @@ 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)
>> + pclk_rate = dsi_adjust_compressed_pclk(mode, dsc);
>
> The confusion with this comment (and the reason the aforementioned
> discussion [2] carried on so long) stems from the fact a division makes
> sense for a bit/byte clock, but not for a pixel clock: we still intend
> to send the same number of pixels, just spending less bytes on them. So
> as you clarify the /3 above, can you also clarify that here or drop this
> comment completely when the function is correctly documented instead?
Acked.
Thanks,
Jessica Zhang
>
> - Marijn
>
>> +
>> return pclk_rate;
>> }
>>
>> @@ -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 [flat|nested] 14+ messages in thread
* Re: [Freedreno] [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
2023-06-09 0:56 ` [Freedreno] " Jessica Zhang
@ 2023-06-09 1:09 ` Abhinav Kumar
2023-06-11 21:19 ` Marijn Suijten
2023-06-11 21:59 ` Marijn Suijten
1 sibling, 1 reply; 14+ messages in thread
From: Abhinav Kumar @ 2023-06-09 1:09 UTC (permalink / raw)
To: Jessica Zhang, Marijn Suijten
Cc: freedreno, Sean Paul, dri-devel, linux-kernel, Konrad Dybcio,
Rob Clark, Daniel Vetter, linux-arm-msm, Dmitry Baryshkov,
David Airlie
On 6/8/2023 5:56 PM, Jessica Zhang wrote:
>
>
> On 6/8/2023 1:36 PM, Marijn Suijten wrote:
>> Same title suggestion as earlier: s/adjust/reduce
>
> Hi Marijn,
>
> Acked.
>
>>
>> On 2023-05-22 18:08:56, 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>
>>> ---
>>> 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..88f370dd2ea1 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -561,7 +561,18 @@ 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_compressed_pclk(const struct
>>> drm_display_mode *mode,
>>
>> Nit: adjust_pclk_for_compression
>
> Acked.
>
>>
>> As discussed before we realized that this change is more-or-less a hack,
>> since downstream calculates pclk quite differently - at least for
>> command-mode panels. Do you still intend to land this patch this way,
>> or go the proper route by introducing the right math from the get-go?
>> Or is the math at least correct for video-mode panels?
>
> Sorry but can you please clarify what exactly is incorrect or different
> about this math when compared to downstream? And, if you think that this
> math is incorrect, what exactly has to be changed to make it the "right
> math"?
>
Agree with Jessica, just calling the math a hack without explaining why
you think it is so is not justified especially when a great detail of
explanation was given on the bug. Sorry but its a bit harsh on the
developers.
> We've already shown step-by-step [1] not only how the resulting pclk
> from the downstream code matches out upstream calculations, but also how
> the inclusion of porches in the upstream math would make up for the fact
> that upstream has no concept of transfer time [2].
>
> If the lack of transfer time in the upstream math is the issue, I
> believe that that's not within the scope of this series, as transfer
> time is not something specific to DSC.
>
> Moreover, as stated in an earlier revision [3], there is no way to
> validate DSC over DSI for video mode. As far as I know, we do not have a
> way to validate video mode datapath for DSI in general.
>
> [1] https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1936144
> [2] https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1945792
> [3]
> https://patchwork.freedesktop.org/patch/535117/?series=117219&rev=1#comment_970492
>
>>
>> This function requires a documentation comment to explain that all.
>>
>>> + const struct drm_dsc_config *dsc)
>>> +{
>>> + int new_hdisplay = DIV_ROUND_UP(mode->hdisplay *
>>> drm_dsc_get_bpp_int(dsc),
>>
>> This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if
>> bits_per_component==8 is assumed. In fact, it then becomes identical
>> to the following line in dsi_host.c which you added previously:
>>
>> hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc),
>> 3);
>>
>> If not, what is the difference between these two calculations? Maybe
>> they both need to be in a properly-named helper.
>
> While the math technically matches up (assuming, also, that
> mode->hdisplay == slice_width * slice_count for all cases), there are
> conceptual differences between the pclk and hdisplay calculations.
>
> Just to reiterate what was already said on IRC:
>
> In the pclk calculation, we're multiplying pclk by the compression ratio
> (which would be target_bpp / src_bpp) -- please note that here, we
> calculate src_bpp by doing bpc * 3.
>
> In the hdisplay calculation, we calculate the bytes per line and divide
> by 3 (bytes) to account for the fact that we can only process 3 bytes
> per pclk cycle.
>
> So while I understand why you would want to put this behind a shared
> helper, I think abstracting the pclk and hdisplay math away would
> obfuscate the conceptual difference between the 2 calculations.
>
>>
>>> + dsc->bits_per_component * 3);
>>
>> As we established in the drm/msm issue [2] there is currently a
>> confusion whether this /3 (and the /3 in dsi_timing_setup) come from the
>> ratio between dsi_get_bpp() and dsc->bpp or something else. Can you
>> clarify that with constants and comments?
>
> Sure, we are planning to add a patch to the end of this series
> documenting the math.
>
>>
>> [2]: https://gitlab.freedesktop.org/drm/msm/-/issues/24
>>
>>> +
>>> + return (new_hdisplay + (mode->htotal - mode->hdisplay))
>>> + * mode->vtotal * drm_mode_vrefresh(mode);
>>
>> As clarified in [1] I was not necessarily suggesting to move this math
>> to a separate helper, but to also use a few more properly-named
>> intermediate variables to not have multi-line math and self-documenting
>> code. These lines could be split to be much more clear.
>
> Acked.
>
>>
>> [1]:
>> https://lore.kernel.org/linux-arm-msm/u4x2vldkzsokfcpbkz3dtwcllbdk4ljcz6kzuaxt5frx6g76o5@uku6abewvye7/
>>
>>> +}
>>> +
>>> +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;
>>> @@ -576,6 +587,10 @@ 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)
>>> + pclk_rate = dsi_adjust_compressed_pclk(mode, dsc);
>>
>> The confusion with this comment (and the reason the aforementioned
>> discussion [2] carried on so long) stems from the fact a division makes
>> sense for a bit/byte clock, but not for a pixel clock: we still intend
>> to send the same number of pixels, just spending less bytes on them. So
>> as you clarify the /3 above, can you also clarify that here or drop this
>> comment completely when the function is correctly documented instead?
>
> Acked.
>
> Thanks,
>
> Jessica Zhang
>
>>
>> - Marijn
>>
>>> +
>>> return pclk_rate;
>>> }
>>> @@ -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 [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
2023-06-08 20:36 ` Marijn Suijten
2023-06-09 0:56 ` [Freedreno] " Jessica Zhang
@ 2023-06-09 16:58 ` Dmitry Baryshkov
2023-06-09 17:24 ` Jessica Zhang
2023-06-11 21:42 ` Marijn Suijten
1 sibling, 2 replies; 14+ messages in thread
From: Dmitry Baryshkov @ 2023-06-09 16:58 UTC (permalink / raw)
To: Marijn Suijten, Jessica Zhang
Cc: Rob Clark, Abhinav Kumar, Sean Paul, David Airlie, Daniel Vetter,
Konrad Dybcio, linux-arm-msm, dri-devel, freedreno, linux-kernel
On 08/06/2023 23:36, Marijn Suijten wrote:
> Same title suggestion as earlier: s/adjust/reduce
>
> On 2023-05-22 18:08:56, 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>
>> ---
>> 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..88f370dd2ea1 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -561,7 +561,18 @@ 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_compressed_pclk(const struct drm_display_mode *mode,
>
> Nit: adjust_pclk_for_compression
>
> As discussed before we realized that this change is more-or-less a hack,
> since downstream calculates pclk quite differently - at least for
> command-mode panels. Do you still intend to land this patch this way,
> or go the proper route by introducing the right math from the get-go?
> Or is the math at least correct for video-mode panels?
Can we please postpone the cmd-vs-video discussion? Otherwise I will
reserve myself a right to push a patch dropping CMD mode support until
somebody comes with a proper way to handle CMD clock calculation.
It is off-topic for the sake of DSC 1.2 support. Yes, all CMD panel
timings are a kind of a hack and should be improved. No, we can not do
this as a part of this series. I think everybody agrees that with the
current way of calculating CMD panel timings, this function does some
kind of a trick.
>
> This function requires a documentation comment to explain that all.
>
>> + const struct drm_dsc_config *dsc)
>> +{
>> + int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
>
> This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if
> bits_per_component==8 is assumed. In fact, it then becomes identical
> to the following line in dsi_host.c which you added previously:
>
> hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
This would imply a simple /3, but as far as I understand it is not
correct here.
>
> If not, what is the difference between these two calculations? Maybe
> they both need to be in a properly-named helper.
>
>> + dsc->bits_per_component * 3);
I hope to see a documentation patch to be posted, telling that this
scales hdisplay and thus pclk by the factor of compressed_bpp /
uncompressed_bpp.
This is not how it is usually done, but I would accept a separate
documentation patch going over the calculation here and in
dsi_timing_setup (and maybe other unobvious cases, if there is anything
left).
>
> As we established in the drm/msm issue [2] there is currently a
> confusion whether this /3 (and the /3 in dsi_timing_setup) come from the
> ratio between dsi_get_bpp() and dsc->bpp or something else. Can you
> clarify that with constants and comments?
>
> [2]: https://gitlab.freedesktop.org/drm/msm/-/issues/24
>
>> +
>> + return (new_hdisplay + (mode->htotal - mode->hdisplay))
>> + * mode->vtotal * drm_mode_vrefresh(mode);
>
> As clarified in [1] I was not necessarily suggesting to move this math
> to a separate helper, but to also use a few more properly-named
> intermediate variables to not have multi-line math and self-documenting
> code. These lines could be split to be much more clear.
I think it's fine more or less. One pair of parenthesis is unnecessary,
but that's mostly it. Maybe `new_htotal' variable would make some sense.
Also, please excuse me if this was discussed somewhere. This calculation
means that only the displayed data is compressed, but porches are not
touched. Correct?
>
> [1]: https://lore.kernel.org/linux-arm-msm/u4x2vldkzsokfcpbkz3dtwcllbdk4ljcz6kzuaxt5frx6g76o5@uku6abewvye7/
>
>> +}
>> +
>> +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;
>>
>> @@ -576,6 +587,10 @@ 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)
>> + pclk_rate = dsi_adjust_compressed_pclk(mode, dsc);
Looking for the perfection, I'd also move the pclk adjustment to come
before the is_bonded_dsi check.
>
> The confusion with this comment (and the reason the aforementioned
> discussion [2] carried on so long) stems from the fact a division makes
> sense for a bit/byte clock, but not for a pixel clock: we still intend
> to send the same number of pixels, just spending less bytes on them. So
> as you clarify the /3 above, can you also clarify that here or drop this
> comment completely when the function is correctly documented instead?
>
> - Marijn
>
>> +
>> return pclk_rate;
>> }
>>
>> @@ -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
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
2023-06-09 16:58 ` Dmitry Baryshkov
@ 2023-06-09 17:24 ` Jessica Zhang
2023-06-11 21:42 ` Marijn Suijten
1 sibling, 0 replies; 14+ messages in thread
From: Jessica Zhang @ 2023-06-09 17:24 UTC (permalink / raw)
To: Dmitry Baryshkov, Marijn Suijten
Cc: Rob Clark, Abhinav Kumar, Sean Paul, David Airlie, Daniel Vetter,
Konrad Dybcio, linux-arm-msm, dri-devel, freedreno, linux-kernel
On 6/9/2023 9:58 AM, Dmitry Baryshkov wrote:
> On 08/06/2023 23:36, Marijn Suijten wrote:
>> Same title suggestion as earlier: s/adjust/reduce
>>
>> On 2023-05-22 18:08:56, 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>
>>> ---
>>> 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..88f370dd2ea1 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -561,7 +561,18 @@ 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_compressed_pclk(const struct
>>> drm_display_mode *mode,
>>
>> Nit: adjust_pclk_for_compression
>>
>> As discussed before we realized that this change is more-or-less a hack,
>> since downstream calculates pclk quite differently - at least for
>> command-mode panels. Do you still intend to land this patch this way,
>> or go the proper route by introducing the right math from the get-go?
>> Or is the math at least correct for video-mode panels?
>
> Can we please postpone the cmd-vs-video discussion? Otherwise I will
> reserve myself a right to push a patch dropping CMD mode support until
> somebody comes with a proper way to handle CMD clock calculation.
>
>
> It is off-topic for the sake of DSC 1.2 support. Yes, all CMD panel
> timings are a kind of a hack and should be improved. No, we can not do
> this as a part of this series. I think everybody agrees that with the
> current way of calculating CMD panel timings, this function does some
> kind of a trick.
>
>>
>> This function requires a documentation comment to explain that all.
>>
>>> + const struct drm_dsc_config *dsc)
>>> +{
>>> + int new_hdisplay = DIV_ROUND_UP(mode->hdisplay *
>>> drm_dsc_get_bpp_int(dsc),
>>
>> This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if
>> bits_per_component==8 is assumed. In fact, it then becomes identical
>> to the following line in dsi_host.c which you added previously:
>>
>> hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc),
>> 3);
>
> This would imply a simple /3, but as far as I understand it is not
> correct here.
>
>>
>> If not, what is the difference between these two calculations? Maybe
>> they both need to be in a properly-named helper.
>>
>>> + dsc->bits_per_component * 3);
>
> I hope to see a documentation patch to be posted, telling that this
> scales hdisplay and thus pclk by the factor of compressed_bpp /
> uncompressed_bpp.
>
> This is not how it is usually done, but I would accept a separate
> documentation patch going over the calculation here and in
> dsi_timing_setup (and maybe other unobvious cases, if there is anything
> left).
>
>>
>> As we established in the drm/msm issue [2] there is currently a
>> confusion whether this /3 (and the /3 in dsi_timing_setup) come from the
>> ratio between dsi_get_bpp() and dsc->bpp or something else. Can you
>> clarify that with constants and comments?
>>
>> [2]: https://gitlab.freedesktop.org/drm/msm/-/issues/24
>>
>>> +
>>> + return (new_hdisplay + (mode->htotal - mode->hdisplay))
>>> + * mode->vtotal * drm_mode_vrefresh(mode);
>>
>> As clarified in [1] I was not necessarily suggesting to move this math
>> to a separate helper, but to also use a few more properly-named
>> intermediate variables to not have multi-line math and self-documenting
>> code. These lines could be split to be much more clear.
>
> I think it's fine more or less. One pair of parenthesis is unnecessary,
> but that's mostly it. Maybe `new_htotal' variable would make some sense.
>
> Also, please excuse me if this was discussed somewhere. This calculation
> means that only the displayed data is compressed, but porches are not
> touched. Correct?
Hi Dmitry,
Correct, we will apply the compression ratio to only the hdisplay but
keep the porches as is.
>
>>
>> [1]:
>> https://lore.kernel.org/linux-arm-msm/u4x2vldkzsokfcpbkz3dtwcllbdk4ljcz6kzuaxt5frx6g76o5@uku6abewvye7/
>>
>>> +}
>>> +
>>> +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;
>>> @@ -576,6 +587,10 @@ 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)
>>> + pclk_rate = dsi_adjust_compressed_pclk(mode, dsc);
>
> Looking for the perfection, I'd also move the pclk adjustment to come
> before the is_bonded_dsi check.
Acked.
Thanks,
Jessica Zhang
>
>>
>> The confusion with this comment (and the reason the aforementioned
>> discussion [2] carried on so long) stems from the fact a division makes
>> sense for a bit/byte clock, but not for a pixel clock: we still intend
>> to send the same number of pixels, just spending less bytes on them. So
>> as you clarify the /3 above, can you also clarify that here or drop this
>> comment completely when the function is correctly documented instead?
>>
>> - Marijn
>>
>>> +
>>> return pclk_rate;
>>> }
>>> @@ -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
>>>
>
> --
> With best wishes
> Dmitry
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Freedreno] [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
2023-06-09 1:09 ` Abhinav Kumar
@ 2023-06-11 21:19 ` Marijn Suijten
0 siblings, 0 replies; 14+ messages in thread
From: Marijn Suijten @ 2023-06-11 21:19 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Jessica Zhang, freedreno, Sean Paul, dri-devel, linux-kernel,
Konrad Dybcio, Rob Clark, Daniel Vetter, linux-arm-msm,
Dmitry Baryshkov, David Airlie
On 2023-06-08 18:09:57, Abhinav Kumar wrote:
<snip>
> >> As discussed before we realized that this change is more-or-less a hack,
> >> since downstream calculates pclk quite differently - at least for
> >> command-mode panels. Do you still intend to land this patch this way,
> >> or go the proper route by introducing the right math from the get-go?
> >> Or is the math at least correct for video-mode panels?
> >
> > Sorry but can you please clarify what exactly is incorrect or different
> > about this math when compared to downstream? And, if you think that this
> > math is incorrect, what exactly has to be changed to make it the "right
> > math"?
> >
>
> Agree with Jessica, just calling the math a hack without explaining why
> you think it is so is not justified especially when a great detail of
> explanation was given on the bug. Sorry but its a bit harsh on the
> developers.
We discussed this in detail so I'm not quite sure why that suddenly
needs to be reiterated again?
- Marijn
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
2023-06-09 16:58 ` Dmitry Baryshkov
2023-06-09 17:24 ` Jessica Zhang
@ 2023-06-11 21:42 ` Marijn Suijten
1 sibling, 0 replies; 14+ messages in thread
From: Marijn Suijten @ 2023-06-11 21:42 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Jessica Zhang, Rob Clark, Abhinav Kumar, Sean Paul, David Airlie,
Daniel Vetter, Konrad Dybcio, linux-arm-msm, dri-devel, freedreno,
linux-kernel
On 2023-06-09 19:58:00, Dmitry Baryshkov wrote:
> On 08/06/2023 23:36, Marijn Suijten wrote:
> > Same title suggestion as earlier: s/adjust/reduce
> >
> > On 2023-05-22 18:08:56, 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>
> >> ---
> >> 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..88f370dd2ea1 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> @@ -561,7 +561,18 @@ 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_compressed_pclk(const struct drm_display_mode *mode,
> >
> > Nit: adjust_pclk_for_compression
> >
> > As discussed before we realized that this change is more-or-less a hack,
> > since downstream calculates pclk quite differently - at least for
> > command-mode panels. Do you still intend to land this patch this way,
> > or go the proper route by introducing the right math from the get-go?
> > Or is the math at least correct for video-mode panels?
>
> Can we please postpone the cmd-vs-video discussion?
If you had read Jessica's reply (and the discussions thus far) this
patch is intended for CMD-mode:
Moreover, as stated in an earlier revision [3], there is no way to
validate DSC over DSI for video mode. As far as I know, we do not
have a way to validate video mode datapath for DSI in general.
Hence my hopefully-relevant question whether this workaround - to reduce
the hdisplay portion that comprises the full pclk signal - is at the
very least right for video mode? After all, CMD mode doesn't have
porches so it makes no sense to account for those (except that it
currently pretends to represent the "transfer time").
Furthermore there is *zero* documentation describing this workaround,
not even in v6.
> Otherwise I will reserve myself a right to push a patch dropping CMD
> mode support until somebody comes with a proper way to handle CMD
> clock calculation.
Please do. That should force me or someone else to submit the right
calculations instead of repeatedly getting stuck in this loop of
complaints and incomprehensible patches with no fix in sight.
> It is off-topic for the sake of DSC 1.2 support. Yes, all CMD panel
> timings are a kind of a hack and should be improved. No, we can not do
> this as a part of this series. I think everybody agrees that with the
> current way of calculating CMD panel timings, this function does some
> kind of a trick.
I understand that you want to be pragrmatic about this situation, but it
seems wrong to build new math on top of fundamentally broken values. If
there's one thing I learned while contributing to mainline here, it is
that sometimes things are fundamentally broken and you cannot
ship/contribute a shiny new feature before first fixing the
fundamentals. What makes this different?
> > This function requires a documentation comment to explain that all.
> >
> >> + const struct drm_dsc_config *dsc)
> >> +{
> >> + int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
> >
> > This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if
> > bits_per_component==8 is assumed. In fact, it then becomes identical
> > to the following line in dsi_host.c which you added previously:
> >
> > hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>
> This would imply a simple /3, but as far as I understand it is not
> correct here.
If you read Jessica's comment on that duplicate line in v6, that is
exactly what is happening:
* hdisplay will be divided by 3 here to account for the fact
* that DPU sends 3 bytes per pclk cycle to DSI.
That comment acknowledges that it is the same: but why isn't this? Why
is bits_per_component (number of bits per pixel component _in the source
picture_) suddenly introduced in this pclk calculation while it is not
used anywhere else?
> > If not, what is the difference between these two calculations? Maybe
> > they both need to be in a properly-named helper.
> >
> >> + dsc->bits_per_component * 3);
How is this different from dsi_get_bpp()?
> I hope to see a documentation patch to be posted, telling that this
> scales hdisplay and thus pclk by the factor of compressed_bpp /
> uncompressed_bpp.
>
> This is not how it is usually done, but I would accept a separate
> documentation patch going over the calculation here and in
> dsi_timing_setup (and maybe other unobvious cases, if there is anything
> left).
I'd love to see additional documentation for existing logic, but the
next revision of this patch could atomically add these comments while
adding the logic (seems to not have been done for v6...)?
> > As we established in the drm/msm issue [2] there is currently a
> > confusion whether this /3 (and the /3 in dsi_timing_setup) come from the
> > ratio between dsi_get_bpp() and dsc->bpp or something else. Can you
> > clarify that with constants and comments?
> >
> > [2]: https://gitlab.freedesktop.org/drm/msm/-/issues/24
> >
> >> +
> >> + return (new_hdisplay + (mode->htotal - mode->hdisplay))
> >> + * mode->vtotal * drm_mode_vrefresh(mode);
> >
> > As clarified in [1] I was not necessarily suggesting to move this math
> > to a separate helper, but to also use a few more properly-named
> > intermediate variables to not have multi-line math and self-documenting
> > code. These lines could be split to be much more clear.
>
> I think it's fine more or less. One pair of parenthesis is unnecessary,
> but that's mostly it. Maybe `new_htotal' variable would make some sense.
>
> Also, please excuse me if this was discussed somewhere. This calculation
> means that only the displayed data is compressed, but porches are not
> touched. Correct?
Porches don't exist in CMDmode (but they do in mainline, because
transfer time is not calculated yet). For video mode I don't know.
> > [1]: https://lore.kernel.org/linux-arm-msm/u4x2vldkzsokfcpbkz3dtwcllbdk4ljcz6kzuaxt5frx6g76o5@uku6abewvye7/
> >
> >> +}
> >> +
> >> +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;
> >>
> >> @@ -576,6 +587,10 @@ 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)
> >> + pclk_rate = dsi_adjust_compressed_pclk(mode, dsc);
>
> Looking for the perfection, I'd also move the pclk adjustment to come
> before the is_bonded_dsi check.
Perfection?
- Marijn
> > The confusion with this comment (and the reason the aforementioned
> > discussion [2] carried on so long) stems from the fact a division makes
> > sense for a bit/byte clock, but not for a pixel clock: we still intend
> > to send the same number of pixels, just spending less bytes on them. So
> > as you clarify the /3 above, can you also clarify that here or drop this
> > comment completely when the function is correctly documented instead?
> >
> > - Marijn
> >
> >> +
> >> return pclk_rate;
> >> }
> >>
> >> @@ -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
> >>
>
> --
> With best wishes
> Dmitry
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Freedreno] [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression
2023-06-09 0:56 ` [Freedreno] " Jessica Zhang
2023-06-09 1:09 ` Abhinav Kumar
@ 2023-06-11 21:59 ` Marijn Suijten
1 sibling, 0 replies; 14+ messages in thread
From: Marijn Suijten @ 2023-06-11 21:59 UTC (permalink / raw)
To: Jessica Zhang
Cc: freedreno, Sean Paul, Abhinav Kumar, dri-devel, linux-kernel,
Konrad Dybcio, Rob Clark, Daniel Vetter, linux-arm-msm,
Dmitry Baryshkov, David Airlie
On 2023-06-08 17:56:47, Jessica Zhang wrote:
<snip>
> > As discussed before we realized that this change is more-or-less a hack,
> > since downstream calculates pclk quite differently - at least for
> > command-mode panels. Do you still intend to land this patch this way,
> > or go the proper route by introducing the right math from the get-go?
> > Or is the math at least correct for video-mode panels?
>
> Sorry but can you please clarify what exactly is incorrect or different
> about this math when compared to downstream? And, if you think that this
> math is incorrect, what exactly has to be changed to make it the "right
> math"?
>
> We've already shown step-by-step [1] not only how the resulting pclk
> from the downstream code matches out upstream calculations, but also how
> the inclusion of porches in the upstream math would make up for the fact
> that upstream has no concept of transfer time [2].
But it doesn't match, unless one hardcodes the desired clock (and/or
tranfer_time calculations) in a panel driver and uses that to come up
with artificial porches in the DRM mode.
> If the lack of transfer time in the upstream math is the issue, I
> believe that that's not within the scope of this series, as transfer
> time is not something specific to DSC.
Yes, that is the issue, and there is zero documentation in this patch
describing that it is currently a workaround to rescale the hdisplay
portion.
After all, when there are no porches pretending to make up for the lack
of transfer time, this code will be obsolete.
> Moreover, as stated in an earlier revision [3], there is no way to
> validate DSC over DSI for video mode. As far as I know, we do not have a
> way to validate video mode datapath for DSI in general.
It was just a question wheter a calculation of this form is correct for
video mode, where porches are used and not - afaik - tranfer time?
> [1] https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1936144
> [2] https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1945792
> [3]
> https://patchwork.freedesktop.org/patch/535117/?series=117219&rev=1#comment_970492
>
> >
> > This function requires a documentation comment to explain that all.
> >
> >> + const struct drm_dsc_config *dsc)
> >> +{
> >> + int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
> >
> > This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if
> > bits_per_component==8 is assumed. In fact, it then becomes identical
> > to the following line in dsi_host.c which you added previously:
> >
> > hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> >
> > If not, what is the difference between these two calculations? Maybe
> > they both need to be in a properly-named helper.
>
> While the math technically matches up (assuming, also, that
> mode->hdisplay == slice_width * slice_count for all cases), there are
> conceptual differences between the pclk and hdisplay calculations.
>
> Just to reiterate what was already said on IRC:
>
> In the pclk calculation, we're multiplying pclk by the compression ratio
> (which would be target_bpp / src_bpp) -- please note that here, we
> calculate src_bpp by doing bpc * 3.
>
> In the hdisplay calculation, we calculate the bytes per line and divide
> by 3 (bytes) to account for the fact that we can only process 3 bytes
> per pclk cycle.
Your comment in v6 explains that hdisplay is divided by 3 because "DPU
sends 3 bytes per pclk cycle to DSI". That inherently describes **a
relation between hdisplay and pclk** so why is the math then different?
After all, pclk is calculated based on the number of bytes (after DSC
compression) that need to be sent from DPU to DSI... and that has
nothing to do with the number of source bytes.
Note that the original hdisplay neither has any notion of bytes: it is
the _number of horizontal pixels_.
> So while I understand why you would want to put this behind a shared
> helper, I think abstracting the pclk and hdisplay math away would
> obfuscate the conceptual difference between the 2 calculations.
That difference is still unclear, FWIW.
> >> + dsc->bits_per_component * 3);
And bits_per_component hasn't been used before yet... It is the number
of bits in the original image, so this could just be dsi_get_bpp() as
used elsewhere?
> >
> > As we established in the drm/msm issue [2] there is currently a
> > confusion whether this /3 (and the /3 in dsi_timing_setup) come from the
> > ratio between dsi_get_bpp() and dsc->bpp or something else. Can you
> > clarify that with constants and comments?
>
> Sure, we are planning to add a patch to the end of this series
> documenting the math.
Why can't you - at least for the new math introduced right here -
document it right from the get-go? Having a separate patch to add it
seems extraneous; though extra documentation for existing code is always
welcome.
- Marijn
<snip>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-06-11 21:59 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-23 1:08 [PATCH v5 0/5] Add DSC v1.2 Support for DSI Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 1/5] msm/drm/dsi: Round up DSC hdisplay calculation Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression Jessica Zhang
2023-06-08 20:36 ` Marijn Suijten
2023-06-09 0:56 ` [Freedreno] " Jessica Zhang
2023-06-09 1:09 ` Abhinav Kumar
2023-06-11 21:19 ` Marijn Suijten
2023-06-11 21:59 ` Marijn Suijten
2023-06-09 16:58 ` Dmitry Baryshkov
2023-06-09 17:24 ` Jessica Zhang
2023-06-11 21:42 ` Marijn Suijten
2023-05-23 1:08 ` [PATCH v5 3/5] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0 Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 4/5] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2 Jessica Zhang
2023-05-23 1:08 ` [PATCH v5 5/5] drm/msm/dsi: Remove incorrect references to slice_count Jessica Zhang
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 read-only IMAP folder(s) and NNTP newsgroup(s).