diff mbox series

[v1,12/14] drm/msm/disp/dpu1: revise timing engine programming to work for DSC

Message ID 1674498274-6010-13-git-send-email-quic_khsieh@quicinc.com (mailing list archive)
State New, archived
Headers show
Series add display port DSC feature | expand

Commit Message

Kuogee Hsieh Jan. 23, 2023, 6:24 p.m. UTC
Current implementation timing engine programming does not consider
compression factors. This patch add consideration of DSC factors
while programming timing engine.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |   2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |  14 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 132 +++++++++++++--------
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  10 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |   6 +-
 5 files changed, 110 insertions(+), 54 deletions(-)

Comments

Dmitry Baryshkov Jan. 23, 2023, 9:39 p.m. UTC | #1
On 23/01/2023 20:24, Kuogee Hsieh wrote:
> Current implementation timing engine programming does not consider
> compression factors. This patch add consideration of DSC factors
> while programming timing engine.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |   2 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |  14 ++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 132 +++++++++++++--------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  10 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |   6 +-
>   5 files changed, 110 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index 2d864f9..3330e185 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -279,6 +279,8 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>   	if (phys_enc->hw_pp->merge_3d)
>   		intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
>   
> +	phys_enc->hw_intf->hw_rev = phys_enc->dpu_kms->core_rev;
> +
>   	spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
>   	phys_enc->hw_intf->ops.setup_timing_gen(phys_enc->hw_intf,
>   			&timing_params, fmt);
> 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 7b0b092..c6ee789 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -43,16 +43,22 @@
>   #define DPU_HW_VER_500	DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>   #define DPU_HW_VER_501	DPU_HW_VER(5, 0, 1) /* sm8150 v2.0 */
>   #define DPU_HW_VER_510	DPU_HW_VER(5, 1, 1) /* sc8180 */
> -#define DPU_HW_VER_600	DPU_HW_VER(6, 0, 0) /* sm8250 */
> +#define DPU_HW_VER_600	DPU_HW_VER(6, 0, 0) /* sm8250, kona */
>   #define DPU_HW_VER_620	DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>   #define DPU_HW_VER_630	DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>   #define DPU_HW_VER_650	DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
> -#define DPU_HW_VER_700	DPU_HW_VER(7, 0, 0) /* sm8350 */
> +#define DPU_HW_VER_700	DPU_HW_VER(7, 0, 0) /* sm8350, lahaina */
>   #define DPU_HW_VER_720	DPU_HW_VER(7, 2, 0) /* sc7280 */
>   #define DPU_HW_VER_800	DPU_HW_VER(8, 0, 0) /* sc8280xp */
> -#define DPU_HW_VER_810	DPU_HW_VER(8, 1, 0) /* sm8450 */
> +#define DPU_HW_VER_810	DPU_HW_VER(8, 1, 0) /* sm8450, waipio */
>   #define DPU_HW_VER_900	DPU_HW_VER(9, 0, 0) /* sm8550 */

No.

>   
> +/* Avoid using below IS_XXX macros outside catalog, use feature bit instead */
> +#define IS_DPU_MAJOR_SAME(rev1, rev2)   \
> +		(DPU_HW_MAJOR((rev1)) == DPU_HW_MAJOR((rev2)))
> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
> +		(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
> +
>   #define IS_MSM8996_TARGET(rev) IS_DPU_MAJOR_MINOR_SAME((rev), DPU_HW_VER_170)
>   #define IS_MSM8998_TARGET(rev) IS_DPU_MAJOR_MINOR_SAME((rev), DPU_HW_VER_300)
>   #define IS_SDM845_TARGET(rev) IS_DPU_MAJOR_MINOR_SAME((rev), DPU_HW_VER_400)
> @@ -240,6 +246,7 @@ enum {
>    * @DPU_INTF_INPUT_CTRL         Supports the setting of pp block from which
>    *                              pixel data arrives to this INTF
>    * @DPU_INTF_TE                 INTF block has TE configuration support
> + * @DPU_INTF_TE_ALIGN_VSYNC     INTF block has POMS Align vsync support
>    * @DPU_DATA_HCTL_EN            Allows data to be transferred at different rate
>                                   than video timing
>    * @DPU_INTF_MAX
> @@ -247,6 +254,7 @@ enum {
>   enum {
>   	DPU_INTF_INPUT_CTRL = 0x1,
>   	DPU_INTF_TE,
> +	DPU_INTF_TE_ALIGN_VSYNC,
>   	DPU_DATA_HCTL_EN,
>   	DPU_INTF_MAX
>   };
> 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 7ce66bf..238efdb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -1,6 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   /*
> - * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2022-2023, Qualcomm Innovation Center, Inc. All rights reserved.
>    * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>    */
>   
> @@ -44,6 +44,7 @@
>   #define   INTF_DEFLICKER_STRNG_COEFF    0x0F4
>   #define   INTF_DEFLICKER_WEAK_COEFF     0x0F8
>   
> +#define   INTF_REG_SPLIT_LINK           0x080
>   #define   INTF_DSI_CMD_MODE_TRIGGER_EN  0x084
>   #define   INTF_PANEL_FORMAT             0x090
>   #define   INTF_TPG_ENABLE               0x100
> @@ -65,9 +66,9 @@
>   
>   #define INTF_CFG_ACTIVE_H_EN	BIT(29)
>   #define INTF_CFG_ACTIVE_V_EN	BIT(30)
> -
>   #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
>   #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
> +#define INTF_CFG2_ALIGN_VSYNC_TO_TE BIT(16)
>   
>   #define INTF_MISR_CTRL			0x180
>   #define INTF_MISR_SIGNATURE		0x184
> @@ -91,6 +92,16 @@ static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
>   	return ERR_PTR(-EINVAL);
>   }
>   
> +static inline void _check_and_set_comp_bit(struct dpu_hw_intf *ctx,
> +		bool dsc_4hs_merge, bool compression_en, u32 *intf_cfg2)
> +{
> +	if (((DPU_HW_MAJOR(ctx->hw_rev) >= DPU_HW_MAJOR(DPU_HW_VER_700)) && compression_en)
> +		|| (IS_DPU_MAJOR_SAME(ctx->hw_rev, DPU_HW_VER_600) && dsc_4hs_merge))
> +		(*intf_cfg2) |= BIT(12);
> +	else if (!compression_en)
> +		(*intf_cfg2) &= ~BIT(12);

NAK. Make this into a sensible API rather than poking at DPU revision. 
Not to mention that this BIT(12) should be defined somewhere.

> +}
> +
>   static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>   		const struct intf_timing_params *p,
>   		const struct dpu_format *fmt)
> @@ -113,82 +124,96 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>   	/* read interface_cfg */
>   	intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
>   
> -	if (ctx->cap->type == INTF_DP)
> +	if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP)

You remember, we have been here. INTF_EDP is for old (msm8974,  apq8084) 
eDP.

>   		dp_intf = true;
>   
>   	hsync_period = p->hsync_pulse_width + p->h_back_porch + p->width +
> -	p->h_front_porch;
> +			p->h_front_porch;
>   	vsync_period = p->vsync_pulse_width + p->v_back_porch + p->height +
> -	p->v_front_porch;
> +			p->v_front_porch;
>   
>   	display_v_start = ((p->vsync_pulse_width + p->v_back_porch) *
> -	hsync_period) + p->hsync_skew;
> +			hsync_period) + p->hsync_skew;
>   	display_v_end = ((vsync_period - p->v_front_porch) * hsync_period) +
> -	p->hsync_skew - 1;
> +			p->hsync_skew - 1;

As usual. A mixture of formatting changes and data changes. Please turn 
this into reviewable changes. Split them into atomic commits. Rewriting 
the whole function is not revieweable and has no way to go in.

> +
> +	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
>   
>   	hsync_start_x = p->h_back_porch + p->hsync_pulse_width;
>   	hsync_end_x = hsync_period - p->h_front_porch - 1;
>   
> -	if (p->width != p->xres) { /* border fill added */
> -		active_h_start = hsync_start_x;
> -		active_h_end = active_h_start + p->xres - 1;
> -	} else {
> -		active_h_start = 0;
> -		active_h_end = 0;
> -	}
> -
> -	if (p->height != p->yres) { /* border fill added */
> -		active_v_start = display_v_start;
> -		active_v_end = active_v_start + (p->yres * hsync_period) - 1;
> -	} else {
> -		active_v_start = 0;
> -		active_v_end = 0;
> -	}
> -
> -	if (active_h_end) {
> -		active_hctl = (active_h_end << 16) | active_h_start;
> -		intf_cfg |= INTF_CFG_ACTIVE_H_EN;
> -	} else {
> -		active_hctl = 0;
> -	}
> -
> -	if (active_v_end)
> -		intf_cfg |= INTF_CFG_ACTIVE_V_EN;
> -
> -	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
> -	display_hctl = (hsync_end_x << 16) | hsync_start_x;
> -
>   	/*
>   	 * DATA_HCTL_EN controls data timing which can be different from
>   	 * video timing. It is recommended to enable it for all cases, except
>   	 * if compression is enabled in 1 pixel per clock mode
>   	 */
> +	if (!p->compression_en || p->wide_bus_en)
> +		intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> +
>   	if (p->wide_bus_en)
> -		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
> +		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>   
> +	/*
> +	 * If widebus is disabled:
> +	 * For uncompressed stream, the data is valid for the entire active
> +	 * window period.
> +	 * For compressed stream, data is valid for a shorter time period
> +	 * inside the active window depending on the compression ratio.
> +	 *
> +	 * If widebus is enabled:
> +	 * For uncompressed stream, data is valid for only half the active
> +	 * window, since the data rate is doubled in this mode.
> +	 * p->width holds the adjusted width for DP but unadjusted width for DSI
> +	 * For compressed stream, data validity window needs to be adjusted for
> +	 * compression ratio and then further halved.
> +	 */
>   	data_width = p->width;
>   
> +	if (p->compression_en) {
> +		if (p->wide_bus_en)
> +			data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 6);
> +		else
> +			data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
> +	} else if (!dp_intf && p->wide_bus_en) {
> +		data_width = p->width >> 1;
> +	} else {
> +		data_width = p->width;
> +	}
> +
>   	hsync_data_start_x = hsync_start_x;
>   	hsync_data_end_x =  hsync_start_x + data_width - 1;
>   
> +	display_hctl = (hsync_end_x << 16) | hsync_start_x;
>   	display_data_hctl = (hsync_data_end_x << 16) | hsync_data_start_x;
>   
>   	if (dp_intf) {
>   		/* DP timing adjustment */
>   		display_v_start += p->hsync_pulse_width + p->h_back_porch;
>   		display_v_end   -= p->h_front_porch;
> +	}
> +
> +	intf_cfg |= INTF_CFG_ACTIVE_H_EN;
> +	intf_cfg |= INTF_CFG_ACTIVE_V_EN;
> +	active_h_start = hsync_start_x;
> +	active_h_end = active_h_start + p->xres - 1;
> +	active_v_start = display_v_start;
> +	active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>   
> -		active_h_start = hsync_start_x;
> -		active_h_end = active_h_start + p->xres - 1;
> -		active_v_start = display_v_start;
> -		active_v_end = active_v_start + (p->yres * hsync_period) - 1;
> +	active_hctl = (active_h_end << 16) | active_h_start;
>   
> -		active_hctl = (active_h_end << 16) | active_h_start;
> +	if (dp_intf) {
>   		display_hctl = active_hctl;
>   
> -		intf_cfg |= INTF_CFG_ACTIVE_H_EN | INTF_CFG_ACTIVE_V_EN;
> +		if (p->compression_en) {
> +			active_data_hctl = (hsync_start_x + p->extra_dto_cycles) << 16;
> +			active_data_hctl += hsync_start_x;
> +
> +			display_data_hctl = active_data_hctl;
> +		}
>   	}
>   
> +	_check_and_set_comp_bit(ctx, p->dsc_4hs_merge, p->compression_en, &intf_cfg2);
> +
>   	den_polarity = 0;
>   	if (ctx->cap->type == INTF_HDMI) {
>   		hsync_polarity = p->yres >= 720 ? 0 : 1;
> @@ -202,7 +227,7 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>   	}
>   	polarity_ctl = (den_polarity << 2) | /*  DEN Polarity  */
>   		(vsync_polarity << 1) | /* VSYNC Polarity */
> -		(hsync_polarity << 0);  /* HSYNC Polarity */
> +		 (hsync_polarity << 0);  /* HSYNC Polarity */
>   
>   	if (!DPU_FORMAT_IS_YUV(fmt))
>   		panel_format = (fmt->bits[C0_G_Y] |
> @@ -216,6 +241,17 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>   				(COLOR_8BIT << 4) |
>   				(0x21 << 8));
>   
> +	if (p->wide_bus_en)
> +		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
> +
> +	/* Synchronize timing engine enable to TE */
> +	if ((ctx->cap->features & BIT(DPU_INTF_TE_ALIGN_VSYNC))
> +			&& p->poms_align_vsync)
> +		intf_cfg2 |= INTF_CFG2_ALIGN_VSYNC_TO_TE;
> +
> +	if (ctx->cfg.split_link_en)
> +		DPU_REG_WRITE(c, INTF_REG_SPLIT_LINK, 0x3);
> +
>   	DPU_REG_WRITE(c, INTF_HSYNC_CTL, hsync_ctl);
>   	DPU_REG_WRITE(c, INTF_VSYNC_PERIOD_F0, vsync_period * hsync_period);
>   	DPU_REG_WRITE(c, INTF_VSYNC_PULSE_WIDTH_F0,
> @@ -233,11 +269,9 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>   	DPU_REG_WRITE(c, INTF_FRAME_LINE_COUNT_EN, 0x3);
>   	DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg);
>   	DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format);
> -	if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
> -		DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
> -		DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
> -		DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
> -	}
> +	DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
> +	DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
> +	DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
>   }
>   
>   static void dpu_hw_intf_enable_timing_engine(
> 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 643dd10..57be86d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -1,6 +1,6 @@
>   /* SPDX-License-Identifier: GPL-2.0-only */
>   /*
> - * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2022-2023, Qualcomm Innovation Center, Inc. All rights reserved.
>    * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>    */
>   
> @@ -10,6 +10,7 @@
>   #include "dpu_hw_catalog.h"
>   #include "dpu_hw_mdss.h"
>   #include "dpu_hw_util.h"
> +#include "dpu_hw_top.h"
>   
>   struct dpu_hw_intf;
>   
> @@ -33,6 +34,11 @@ struct intf_timing_params {
>   	u32 hsync_skew;
>   
>   	bool wide_bus_en;
> +	bool compression_en;
> +	u32 extra_dto_cycles;   /* for DP only */
> +	bool dsc_4hs_merge;     /* DSC 4HS merge */
> +	bool poms_align_vsync;  /* poms with vsync aligned */
> +	u32 dce_bytes_per_line;
>   };
>   
>   struct intf_prog_fetch {
> @@ -86,11 +92,13 @@ struct dpu_hw_intf_ops {
>   
>   struct dpu_hw_intf {
>   	struct dpu_hw_blk_reg_map hw;
> +	u32 hw_rev;	/* mdss hw_rev */
>   
>   	/* intf */
>   	enum dpu_intf idx;
>   	const struct dpu_intf_cfg *cap;
>   	const struct dpu_mdss_cfg *mdss;
> +	struct split_pipe_cfg cfg;
>   
>   	/* ops */
>   	struct dpu_hw_intf_ops ops;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
> index a1a9e44..1212fa2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
> @@ -1,5 +1,7 @@
>   /* SPDX-License-Identifier: GPL-2.0-only */
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/*
> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>    */
>   
>   #ifndef _DPU_HW_TOP_H
> @@ -34,12 +36,14 @@ struct traffic_shaper_cfg {
>    * @intf      : Interface id for main control path
>    * @split_flush_en: Allows both the paths to be flushed when master path is
>    *              flushed
> + * @split_link_en:  Check if split link is enabled
>    */
>   struct split_pipe_cfg {
>   	bool en;
>   	enum dpu_intf_mode mode;
>   	enum dpu_intf intf;
>   	bool split_flush_en;
> +	bool split_link_en;
>   };
>   
>   /**
Dmitry Baryshkov Jan. 24, 2023, 9:11 a.m. UTC | #2
On 23/01/2023 20:24, Kuogee Hsieh wrote:
> Current implementation timing engine programming does not consider
> compression factors. This patch add consideration of DSC factors
> while programming timing engine.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |   2 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |  14 ++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 132 +++++++++++++--------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  10 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |   6 +-
>   5 files changed, 110 insertions(+), 54 deletions(-)
> 

[skipped]

> @@ -113,82 +124,96 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>   	/* read interface_cfg */
>   	intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
>   
> -	if (ctx->cap->type == INTF_DP)
> +	if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP)
>   		dp_intf = true;
>   
>   	hsync_period = p->hsync_pulse_width + p->h_back_porch + p->width +
> -	p->h_front_porch;
> +			p->h_front_porch;
>   	vsync_period = p->vsync_pulse_width + p->v_back_porch + p->height +
> -	p->v_front_porch;
> +			p->v_front_porch;

Actually I went on through the history and found the previous 
submission, https://patchwork.freedesktop.org/patch/471505/.
Exactly the same piece of code. Did you expect that the comments will be 
different this time?

I really hoped that at that time we already went through this. But it 
seems I was wrong. That series went through v10 or v12 before being 
accepted. And it was just adding wide_bus_en. Back at that time we 
lightly discussed that the code will receive compression support. But I 
never expected to see the original submission again.

It might sound bad, but could you please find somebody who can do 
internal review for you? Good internal review.

That said, I really do not expect to see v2 before the whole series is 
reworked, restructured and prepared for the review on your side.

>   
>   	display_v_start = ((p->vsync_pulse_width + p->v_back_porch) *
> -	hsync_period) + p->hsync_skew;
> +			hsync_period) + p->hsync_skew;
>   	display_v_end = ((vsync_period - p->v_front_porch) * hsync_period) +
> -	p->hsync_skew - 1;
> +			p->hsync_skew - 1;
> +
> +	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
>   
>   	hsync_start_x = p->h_back_porch + p->hsync_pulse_width;
>   	hsync_end_x = hsync_period - p->h_front_porch - 1;
>   
> -	if (p->width != p->xres) { /* border fill added */
> -		active_h_start = hsync_start_x;
> -		active_h_end = active_h_start + p->xres - 1;
> -	} else {
> -		active_h_start = 0;
> -		active_h_end = 0;
> -	}
> -
> -	if (p->height != p->yres) { /* border fill added */
> -		active_v_start = display_v_start;
> -		active_v_end = active_v_start + (p->yres * hsync_period) - 1;
> -	} else {
> -		active_v_start = 0;
> -		active_v_end = 0;
> -	}
> -
> -	if (active_h_end) {
> -		active_hctl = (active_h_end << 16) | active_h_start;
> -		intf_cfg |= INTF_CFG_ACTIVE_H_EN;
> -	} else {
> -		active_hctl = 0;
> -	}
> -
> -	if (active_v_end)
> -		intf_cfg |= INTF_CFG_ACTIVE_V_EN;
> -
> -	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
> -	display_hctl = (hsync_end_x << 16) | hsync_start_x;
> -
>   	/*
>   	 * DATA_HCTL_EN controls data timing which can be different from
>   	 * video timing. It is recommended to enable it for all cases, except
>   	 * if compression is enabled in 1 pixel per clock mode
>   	 */
> +	if (!p->compression_en || p->wide_bus_en)
> +		intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> +
>   	if (p->wide_bus_en)
> -		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
> +		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>   
> +	/*
> +	 * If widebus is disabled:
> +	 * For uncompressed stream, the data is valid for the entire active
> +	 * window period.
> +	 * For compressed stream, data is valid for a shorter time period
> +	 * inside the active window depending on the compression ratio.
> +	 *
> +	 * If widebus is enabled:
> +	 * For uncompressed stream, data is valid for only half the active
> +	 * window, since the data rate is doubled in this mode.
> +	 * p->width holds the adjusted width for DP but unadjusted width for DSI
> +	 * For compressed stream, data validity window needs to be adjusted for
> +	 * compression ratio and then further halved.
> +	 */
>   	data_width = p->width;
>   
> +	if (p->compression_en) {
> +		if (p->wide_bus_en)
> +			data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 6);
> +		else
> +			data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
> +	} else if (!dp_intf && p->wide_bus_en) {
> +		data_width = p->width >> 1;
> +	} else {
> +		data_width = p->width;
> +	}
> +
>   	hsync_data_start_x = hsync_start_x;
>   	hsync_data_end_x =  hsync_start_x + data_width - 1;
>   
> +	display_hctl = (hsync_end_x << 16) | hsync_start_x;
>   	display_data_hctl = (hsync_data_end_x << 16) | hsync_data_start_x;
>   
>   	if (dp_intf) {
>   		/* DP timing adjustment */
>   		display_v_start += p->hsync_pulse_width + p->h_back_porch;
>   		display_v_end   -= p->h_front_porch;
> +	}
> +
> +	intf_cfg |= INTF_CFG_ACTIVE_H_EN;
> +	intf_cfg |= INTF_CFG_ACTIVE_V_EN;
> +	active_h_start = hsync_start_x;
> +	active_h_end = active_h_start + p->xres - 1;
> +	active_v_start = display_v_start;
> +	active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>   
> -		active_h_start = hsync_start_x;
> -		active_h_end = active_h_start + p->xres - 1;
> -		active_v_start = display_v_start;
> -		active_v_end = active_v_start + (p->yres * hsync_period) - 1;
> +	active_hctl = (active_h_end << 16) | active_h_start;
>   
> -		active_hctl = (active_h_end << 16) | active_h_start;
> +	if (dp_intf) {
>   		display_hctl = active_hctl;
>   
> -		intf_cfg |= INTF_CFG_ACTIVE_H_EN | INTF_CFG_ACTIVE_V_EN;
> +		if (p->compression_en) {
> +			active_data_hctl = (hsync_start_x + p->extra_dto_cycles) << 16;
> +			active_data_hctl += hsync_start_x;
> +
> +			display_data_hctl = active_data_hctl;
> +		}
>   	}
>   
> +	_check_and_set_comp_bit(ctx, p->dsc_4hs_merge, p->compression_en, &intf_cfg2);
> +
>   	den_polarity = 0;
>   	if (ctx->cap->type == INTF_HDMI) {
>   		hsync_polarity = p->yres >= 720 ? 0 : 1;
> @@ -202,7 +227,7 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
Kuogee Hsieh Jan. 24, 2023, 5:55 p.m. UTC | #3
On 1/24/2023 1:11 AM, Dmitry Baryshkov wrote:
> On 23/01/2023 20:24, Kuogee Hsieh wrote:
>> Current implementation timing engine programming does not consider
>> compression factors. This patch add consideration of DSC factors
>> while programming timing engine.
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |   2 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |  14 ++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 132 
>> +++++++++++++--------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  10 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |   6 +-
>>   5 files changed, 110 insertions(+), 54 deletions(-)
>>
>
> [skipped]
>
>> @@ -113,82 +124,96 @@ static void 
>> dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>       /* read interface_cfg */
>>       intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
>>   -    if (ctx->cap->type == INTF_DP)
>> +    if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP)
>>           dp_intf = true;
>>         hsync_period = p->hsync_pulse_width + p->h_back_porch + 
>> p->width +
>> -    p->h_front_porch;
>> +            p->h_front_porch;
>>       vsync_period = p->vsync_pulse_width + p->v_back_porch + 
>> p->height +
>> -    p->v_front_porch;
>> +            p->v_front_porch;
>
> Actually I went on through the history and found the previous 
> submission, https://patchwork.freedesktop.org/patch/471505/.
> Exactly the same piece of code. Did you expect that the comments will 
> be different this time?
>
> I really hoped that at that time we already went through this. But it 
> seems I was wrong. That series went through v10 or v12 before being 
> accepted. And it was just adding wide_bus_en. Back at that time we 
> lightly discussed that the code will receive compression support. But 
> I never expected to see the original submission again.
>
> It might sound bad, but could you please find somebody who can do 
> internal review for you? Good internal review.
>
> That said, I really do not expect to see v2 before the whole series is 
> reworked, restructured and prepared for the review on your side.

This timing engine code is derived from our downstream code directly and 
it has been used at many mobile devices by many vendors for many years 
already.

On the other words, it had been tested very thorough and works on 
dsi/dp/hdmi/dsc/widebus applications.

When i brought dsc v1.2 over, I just merged it over and did not consider 
too much.

Can we adapt this code so that both upstream and down stream shared same 
timing engine programming so that easier to maintain?





>
>>         display_v_start = ((p->vsync_pulse_width + p->v_back_porch) *
>> -    hsync_period) + p->hsync_skew;
>> +            hsync_period) + p->hsync_skew;
>>       display_v_end = ((vsync_period - p->v_front_porch) * 
>> hsync_period) +
>> -    p->hsync_skew - 1;
>> +            p->hsync_skew - 1;
>> +
>> +    hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
>>         hsync_start_x = p->h_back_porch + p->hsync_pulse_width;
>>       hsync_end_x = hsync_period - p->h_front_porch - 1;
>>   -    if (p->width != p->xres) { /* border fill added */
>> -        active_h_start = hsync_start_x;
>> -        active_h_end = active_h_start + p->xres - 1;
>> -    } else {
>> -        active_h_start = 0;
>> -        active_h_end = 0;
>> -    }
>> -
>> -    if (p->height != p->yres) { /* border fill added */
>> -        active_v_start = display_v_start;
>> -        active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>> -    } else {
>> -        active_v_start = 0;
>> -        active_v_end = 0;
>> -    }
>> -
>> -    if (active_h_end) {
>> -        active_hctl = (active_h_end << 16) | active_h_start;
>> -        intf_cfg |= INTF_CFG_ACTIVE_H_EN;
>> -    } else {
>> -        active_hctl = 0;
>> -    }
>> -
>> -    if (active_v_end)
>> -        intf_cfg |= INTF_CFG_ACTIVE_V_EN;
>> -
>> -    hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
>> -    display_hctl = (hsync_end_x << 16) | hsync_start_x;
>> -
>>       /*
>>        * DATA_HCTL_EN controls data timing which can be different from
>>        * video timing. It is recommended to enable it for all cases, 
>> except
>>        * if compression is enabled in 1 pixel per clock mode
>>        */
>> +    if (!p->compression_en || p->wide_bus_en)
>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
>> +
>>       if (p->wide_bus_en)
>> -        intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
>> +        intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>>   +    /*
>> +     * If widebus is disabled:
>> +     * For uncompressed stream, the data is valid for the entire active
>> +     * window period.
>> +     * For compressed stream, data is valid for a shorter time period
>> +     * inside the active window depending on the compression ratio.
>> +     *
>> +     * If widebus is enabled:
>> +     * For uncompressed stream, data is valid for only half the active
>> +     * window, since the data rate is doubled in this mode.
>> +     * p->width holds the adjusted width for DP but unadjusted width 
>> for DSI
>> +     * For compressed stream, data validity window needs to be 
>> adjusted for
>> +     * compression ratio and then further halved.
>> +     */
>>       data_width = p->width;
>>   +    if (p->compression_en) {
>> +        if (p->wide_bus_en)
>> +            data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 6);
>> +        else
>> +            data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
>> +    } else if (!dp_intf && p->wide_bus_en) {
>> +        data_width = p->width >> 1;
>> +    } else {
>> +        data_width = p->width;
>> +    }
>> +
>>       hsync_data_start_x = hsync_start_x;
>>       hsync_data_end_x =  hsync_start_x + data_width - 1;
>>   +    display_hctl = (hsync_end_x << 16) | hsync_start_x;
>>       display_data_hctl = (hsync_data_end_x << 16) | hsync_data_start_x;
>>         if (dp_intf) {
>>           /* DP timing adjustment */
>>           display_v_start += p->hsync_pulse_width + p->h_back_porch;
>>           display_v_end   -= p->h_front_porch;
>> +    }
>> +
>> +    intf_cfg |= INTF_CFG_ACTIVE_H_EN;
>> +    intf_cfg |= INTF_CFG_ACTIVE_V_EN;
>> +    active_h_start = hsync_start_x;
>> +    active_h_end = active_h_start + p->xres - 1;
>> +    active_v_start = display_v_start;
>> +    active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>>   -        active_h_start = hsync_start_x;
>> -        active_h_end = active_h_start + p->xres - 1;
>> -        active_v_start = display_v_start;
>> -        active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>> +    active_hctl = (active_h_end << 16) | active_h_start;
>>   -        active_hctl = (active_h_end << 16) | active_h_start;
>> +    if (dp_intf) {
>>           display_hctl = active_hctl;
>>   -        intf_cfg |= INTF_CFG_ACTIVE_H_EN | INTF_CFG_ACTIVE_V_EN;
>> +        if (p->compression_en) {
>> +            active_data_hctl = (hsync_start_x + p->extra_dto_cycles) 
>> << 16;
>> +            active_data_hctl += hsync_start_x;
>> +
>> +            display_data_hctl = active_data_hctl;
>> +        }
>>       }
>>   +    _check_and_set_comp_bit(ctx, p->dsc_4hs_merge, 
>> p->compression_en, &intf_cfg2);
>> +
>>       den_polarity = 0;
>>       if (ctx->cap->type == INTF_HDMI) {
>>           hsync_polarity = p->yres >= 720 ? 0 : 1;
>> @@ -202,7 +227,7 @@ static void 
>> dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>
Dmitry Baryshkov Jan. 24, 2023, 9:37 p.m. UTC | #4
On 24/01/2023 19:55, Kuogee Hsieh wrote:
> 
> On 1/24/2023 1:11 AM, Dmitry Baryshkov wrote:
>> On 23/01/2023 20:24, Kuogee Hsieh wrote:
>>> Current implementation timing engine programming does not consider
>>> compression factors. This patch add consideration of DSC factors
>>> while programming timing engine.
>>>
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> ---
>>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |   2 +
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |  14 ++-
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 132 
>>> +++++++++++++--------
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  10 +-
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |   6 +-
>>>   5 files changed, 110 insertions(+), 54 deletions(-)
>>>
>>
>> [skipped]
>>
>>> @@ -113,82 +124,96 @@ static void 
>>> dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>>       /* read interface_cfg */
>>>       intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
>>>   -    if (ctx->cap->type == INTF_DP)
>>> +    if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP)
>>>           dp_intf = true;
>>>         hsync_period = p->hsync_pulse_width + p->h_back_porch + 
>>> p->width +
>>> -    p->h_front_porch;
>>> +            p->h_front_porch;
>>>       vsync_period = p->vsync_pulse_width + p->v_back_porch + 
>>> p->height +
>>> -    p->v_front_porch;
>>> +            p->v_front_porch;
>>
>> Actually I went on through the history and found the previous 
>> submission, https://patchwork.freedesktop.org/patch/471505/.
>> Exactly the same piece of code. Did you expect that the comments will 
>> be different this time?
>>
>> I really hoped that at that time we already went through this. But it 
>> seems I was wrong. That series went through v10 or v12 before being 
>> accepted. And it was just adding wide_bus_en. Back at that time we 
>> lightly discussed that the code will receive compression support. But 
>> I never expected to see the original submission again.
>>
>> It might sound bad, but could you please find somebody who can do 
>> internal review for you? Good internal review.
>>
>> That said, I really do not expect to see v2 before the whole series is 
>> reworked, restructured and prepared for the review on your side.
> 
> This timing engine code is derived from our downstream code directly and 
> it has been used at many mobile devices by many vendors for many years 
> already.
> 
> On the other words, it had been tested very thorough and works on 
> dsi/dp/hdmi/dsc/widebus applications.

As far as I understand, it has been tested on the recent generations of 
the hardware. I doubt that anybody retests new techpack drops on 
previous hardware generations. Correct?

When was the last time this particular code drop was tested on 
INTF_HDMI? I think it was back in the 4.4 era. Newer vendor kernels do 
not have hdmi-staging, so at least the claim of testing this codepiece 
on HDMI is not correct.

What is the earliest chip that has been driven by this particular code 
instance?

> When i brought dsc v1.2 over, I just merged it over and did not consider 
> too much.
> 
> Can we adapt this code so that both upstream and down stream shared same 
> timing engine programming so that easier to maintain?

We have been discussing exactly the same piece of code a year ago. Could 
you please recheck the comments that were provided to your patches. And 
I actually mean that. There were 12 iterations of wide bus patchset. 
Timing engine programming patch had 8. I do not want to start again from 
the very beginning.

The basic idea is that you have to evolve the code rather than flushing 
us with the 'latest and greatest code dump'. Split this into individual 
atomic changes that we can review. Provide justification (= motivation) 
for each change. Previously we haven't seen them.

We know that current function works. We must be able to assume that new 
instance doesn't break things. Or, if something breaks, understand which 
particular change broke it. Consider the case that your patch breaks 
msm8998. Or sdm845. How can we cope? Would you be able to spot the place 
which did that? I know I wouldn't. The only way would be to revert the 
patch completely. And inherently the whole series.

>>
>>>         display_v_start = ((p->vsync_pulse_width + p->v_back_porch) *
>>> -    hsync_period) + p->hsync_skew;
>>> +            hsync_period) + p->hsync_skew;
>>>       display_v_end = ((vsync_period - p->v_front_porch) * 
>>> hsync_period) +
>>> -    p->hsync_skew - 1;
>>> +            p->hsync_skew - 1;
>>> +
>>> +    hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
>>>         hsync_start_x = p->h_back_porch + p->hsync_pulse_width;
>>>       hsync_end_x = hsync_period - p->h_front_porch - 1;
>>>   -    if (p->width != p->xres) { /* border fill added */
>>> -        active_h_start = hsync_start_x;
>>> -        active_h_end = active_h_start + p->xres - 1;
>>> -    } else {
>>> -        active_h_start = 0;
>>> -        active_h_end = 0;
>>> -    }
>>> -
>>> -    if (p->height != p->yres) { /* border fill added */
>>> -        active_v_start = display_v_start;
>>> -        active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>>> -    } else {
>>> -        active_v_start = 0;
>>> -        active_v_end = 0;
>>> -    }
>>> -
>>> -    if (active_h_end) {
>>> -        active_hctl = (active_h_end << 16) | active_h_start;
>>> -        intf_cfg |= INTF_CFG_ACTIVE_H_EN;
>>> -    } else {
>>> -        active_hctl = 0;
>>> -    }
>>> -
>>> -    if (active_v_end)
>>> -        intf_cfg |= INTF_CFG_ACTIVE_V_EN;
>>> -
>>> -    hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
>>> -    display_hctl = (hsync_end_x << 16) | hsync_start_x;
>>> -
>>>       /*
>>>        * DATA_HCTL_EN controls data timing which can be different from
>>>        * video timing. It is recommended to enable it for all cases, 
>>> except
>>>        * if compression is enabled in 1 pixel per clock mode
>>>        */
>>> +    if (!p->compression_en || p->wide_bus_en)
>>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
>>> +
>>>       if (p->wide_bus_en)
>>> -        intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
>>> +        intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>>>   +    /*
>>> +     * If widebus is disabled:
>>> +     * For uncompressed stream, the data is valid for the entire active
>>> +     * window period.
>>> +     * For compressed stream, data is valid for a shorter time period
>>> +     * inside the active window depending on the compression ratio.
>>> +     *
>>> +     * If widebus is enabled:
>>> +     * For uncompressed stream, data is valid for only half the active
>>> +     * window, since the data rate is doubled in this mode.
>>> +     * p->width holds the adjusted width for DP but unadjusted width 
>>> for DSI
>>> +     * For compressed stream, data validity window needs to be 
>>> adjusted for
>>> +     * compression ratio and then further halved.
>>> +     */
>>>       data_width = p->width;
>>>   +    if (p->compression_en) {
>>> +        if (p->wide_bus_en)
>>> +            data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 6);
>>> +        else
>>> +            data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
>>> +    } else if (!dp_intf && p->wide_bus_en) {
>>> +        data_width = p->width >> 1;
>>> +    } else {
>>> +        data_width = p->width;
>>> +    }
>>> +
>>>       hsync_data_start_x = hsync_start_x;
>>>       hsync_data_end_x =  hsync_start_x + data_width - 1;
>>>   +    display_hctl = (hsync_end_x << 16) | hsync_start_x;
>>>       display_data_hctl = (hsync_data_end_x << 16) | hsync_data_start_x;
>>>         if (dp_intf) {
>>>           /* DP timing adjustment */
>>>           display_v_start += p->hsync_pulse_width + p->h_back_porch;
>>>           display_v_end   -= p->h_front_porch;
>>> +    }
>>> +
>>> +    intf_cfg |= INTF_CFG_ACTIVE_H_EN;
>>> +    intf_cfg |= INTF_CFG_ACTIVE_V_EN;
>>> +    active_h_start = hsync_start_x;
>>> +    active_h_end = active_h_start + p->xres - 1;
>>> +    active_v_start = display_v_start;
>>> +    active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>>>   -        active_h_start = hsync_start_x;
>>> -        active_h_end = active_h_start + p->xres - 1;
>>> -        active_v_start = display_v_start;
>>> -        active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>>> +    active_hctl = (active_h_end << 16) | active_h_start;
>>>   -        active_hctl = (active_h_end << 16) | active_h_start;
>>> +    if (dp_intf) {
>>>           display_hctl = active_hctl;
>>>   -        intf_cfg |= INTF_CFG_ACTIVE_H_EN | INTF_CFG_ACTIVE_V_EN;
>>> +        if (p->compression_en) {
>>> +            active_data_hctl = (hsync_start_x + p->extra_dto_cycles) 
>>> << 16;
>>> +            active_data_hctl += hsync_start_x;
>>> +
>>> +            display_data_hctl = active_data_hctl;
>>> +        }
>>>       }
>>>   +    _check_and_set_comp_bit(ctx, p->dsc_4hs_merge, 
>>> p->compression_en, &intf_cfg2);
>>> +
>>>       den_polarity = 0;
>>>       if (ctx->cap->type == INTF_HDMI) {
>>>           hsync_polarity = p->yres >= 720 ? 0 : 1;
>>> @@ -202,7 +227,7 @@ static void 
>>> dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>
Marijn Suijten Jan. 24, 2023, 11:36 p.m. UTC | #5
On 2023-01-24 09:55:24, Kuogee Hsieh wrote:

<snip>

> This timing engine code is derived from our downstream code directly and 
> it has been used at many mobile devices by many vendors for many years 
> already.
> 
> On the other words, it had been tested very thorough and works on 
> dsi/dp/hdmi/dsc/widebus applications.

And the code already in mainline has seen 12 rounds of review, with a
focus on inter-SoC compatibility.  Regardless of that, we have processes
to make changes on mainline: formatting changes (when actually making an
improvement) go separate from semantic changes.  Bugfixes are clearly
described in individual patches with Fixes: tags.  If you really think
the code has to be as proposed in this patch, follow Dmitry's advice and
split this accordingly.

> When i brought dsc v1.2 over, I just merged it over and did not consider 
> too much.

And that is exactly what is wrong with this *entire* series: copying
over downstream code without "considering too much", stomping over
previous review and even reverting bugfixes [1] [2] without giving it
ANY ATTENTION in your patch description.  That's unacceptable and
insulting to contributors and reviewers.  Full stop.  Or did you expect
us to turn a blind eye?  This is mainline, not some techpack playground.

[1]: https://lore.kernel.org/linux-arm-msm/20230123201133.zzt2zbyaw3pfkzi6@SoMainline.org/
[2]: https://lore.kernel.org/linux-arm-msm/20221026182824.876933-10-marijn.suijten@somainline.org/

> Can we adapt this code so that both upstream and down stream shared same 
> timing engine programming so that easier to maintain?

Easy, I've said this before in IRC and will state it again: stop this
techpack nonsense and focus on upstream-first.  When something passes
mainline review (and please don't bother maintainers and reviewers with
series like this) it is inevitably good enough to be copied to
techpack... at which point techpack becomes worthless as you can just
backport a mainline patch or use a recent-enough kernel.


tl;dr: it seems like you nor anyone involved in pre-reviewing/vetting
this series is familiar with upstream guidelines.  Follow the global
advice from Dmitry [3] to reach a more efficient v2, and please don't
let this run to v10 (or beyond) again.

One suggestion to improve efficiency: split off the DPU v1.2 hardware
block addition (and related changes) into a separate series.  A smaller
series (and properly split patches!) will give everyone less moving
parts to worry about, and paves the way for DSI support without blocking
on DP.

[3]: https://lore.kernel.org/linux-arm-msm/47c83e8c-09f1-d1dd-ca79-574122638256@linaro.org/

- Marijn
Neil Armstrong Jan. 25, 2023, 9:08 a.m. UTC | #6
On 25/01/2023 00:36, Marijn Suijten wrote:
> On 2023-01-24 09:55:24, Kuogee Hsieh wrote:
> 
> <snip>
> 
>> This timing engine code is derived from our downstream code directly and
>> it has been used at many mobile devices by many vendors for many years
>> already.
>>
>> On the other words, it had been tested very thorough and works on
>> dsi/dp/hdmi/dsc/widebus applications.
> 
> And the code already in mainline has seen 12 rounds of review, with a
> focus on inter-SoC compatibility.  Regardless of that, we have processes
> to make changes on mainline: formatting changes (when actually making an
> improvement) go separate from semantic changes.  Bugfixes are clearly
> described in individual patches with Fixes: tags.  If you really think
> the code has to be as proposed in this patch, follow Dmitry's advice and
> split this accordingly.
> 
>> When i brought dsc v1.2 over, I just merged it over and did not consider
>> too much.
> 
> And that is exactly what is wrong with this *entire* series: copying
> over downstream code without "considering too much", stomping over
> previous review and even reverting bugfixes [1] [2] without giving it
> ANY ATTENTION in your patch description.  That's unacceptable and
> insulting to contributors and reviewers.  Full stop.  Or did you expect
> us to turn a blind eye?  This is mainline, not some techpack playground.
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20230123201133.zzt2zbyaw3pfkzi6@SoMainline.org/
> [2]: https://lore.kernel.org/linux-arm-msm/20221026182824.876933-10-marijn.suijten@somainline.org/
> 
>> Can we adapt this code so that both upstream and down stream shared same
>> timing engine programming so that easier to maintain?
> 
> Easy, I've said this before in IRC and will state it again: stop this
> techpack nonsense and focus on upstream-first.  When something passes
> mainline review (and please don't bother maintainers and reviewers with
> series like this) it is inevitably good enough to be copied to
> techpack... at which point techpack becomes worthless as you can just
> backport a mainline patch or use a recent-enough kernel.
> 
> 
> tl;dr: it seems like you nor anyone involved in pre-reviewing/vetting
> this series is familiar with upstream guidelines.  Follow the global
> advice from Dmitry [3] to reach a more efficient v2, and please don't
> let this run to v10 (or beyond) again.
> 
> One suggestion to improve efficiency: split off the DPU v1.2 hardware
> block addition (and related changes) into a separate series.  A smaller
> series (and properly split patches!) will give everyone less moving
> parts to worry about, and paves the way for DSI support without blocking
> on DP.

Yes to split DSC 1.2 integration and DP+DSC in 2 patchsets, with the various
fixes not necessary to make DP+DSC work in separate patches.

Be aware the rule is to make sure each single change doesn't break boot and builds
without warning, the rule is to make sure each single kernel change can be built
and doesn't break booting. And build the code with "W=1" to the make parameter to
trigger advanced GCC warnings.

This rule exists to permit running a git bisect to determine which commit introduces
a regression.

And the second most important rule is: a single change per patch, and a clear description
of the change in the commit message.
If your message needs a "change this and also change this" then it's wrong and it must be reworked.

Do incremental changes, like introduce a new struct, then use it afterwards.

Neil

> 
> [3]: https://lore.kernel.org/linux-arm-msm/47c83e8c-09f1-d1dd-ca79-574122638256@linaro.org/
> 
> - Marijn
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 2d864f9..3330e185 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -279,6 +279,8 @@  static void dpu_encoder_phys_vid_setup_timing_engine(
 	if (phys_enc->hw_pp->merge_3d)
 		intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
 
+	phys_enc->hw_intf->hw_rev = phys_enc->dpu_kms->core_rev;
+
 	spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
 	phys_enc->hw_intf->ops.setup_timing_gen(phys_enc->hw_intf,
 			&timing_params, fmt);
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 7b0b092..c6ee789 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -43,16 +43,22 @@ 
 #define DPU_HW_VER_500	DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
 #define DPU_HW_VER_501	DPU_HW_VER(5, 0, 1) /* sm8150 v2.0 */
 #define DPU_HW_VER_510	DPU_HW_VER(5, 1, 1) /* sc8180 */
-#define DPU_HW_VER_600	DPU_HW_VER(6, 0, 0) /* sm8250 */
+#define DPU_HW_VER_600	DPU_HW_VER(6, 0, 0) /* sm8250, kona */
 #define DPU_HW_VER_620	DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
 #define DPU_HW_VER_630	DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
 #define DPU_HW_VER_650	DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */
-#define DPU_HW_VER_700	DPU_HW_VER(7, 0, 0) /* sm8350 */
+#define DPU_HW_VER_700	DPU_HW_VER(7, 0, 0) /* sm8350, lahaina */
 #define DPU_HW_VER_720	DPU_HW_VER(7, 2, 0) /* sc7280 */
 #define DPU_HW_VER_800	DPU_HW_VER(8, 0, 0) /* sc8280xp */
-#define DPU_HW_VER_810	DPU_HW_VER(8, 1, 0) /* sm8450 */
+#define DPU_HW_VER_810	DPU_HW_VER(8, 1, 0) /* sm8450, waipio */
 #define DPU_HW_VER_900	DPU_HW_VER(9, 0, 0) /* sm8550 */
 
+/* Avoid using below IS_XXX macros outside catalog, use feature bit instead */
+#define IS_DPU_MAJOR_SAME(rev1, rev2)   \
+		(DPU_HW_MAJOR((rev1)) == DPU_HW_MAJOR((rev2)))
+#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
+		(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
+
 #define IS_MSM8996_TARGET(rev) IS_DPU_MAJOR_MINOR_SAME((rev), DPU_HW_VER_170)
 #define IS_MSM8998_TARGET(rev) IS_DPU_MAJOR_MINOR_SAME((rev), DPU_HW_VER_300)
 #define IS_SDM845_TARGET(rev) IS_DPU_MAJOR_MINOR_SAME((rev), DPU_HW_VER_400)
@@ -240,6 +246,7 @@  enum {
  * @DPU_INTF_INPUT_CTRL         Supports the setting of pp block from which
  *                              pixel data arrives to this INTF
  * @DPU_INTF_TE                 INTF block has TE configuration support
+ * @DPU_INTF_TE_ALIGN_VSYNC     INTF block has POMS Align vsync support
  * @DPU_DATA_HCTL_EN            Allows data to be transferred at different rate
                                 than video timing
  * @DPU_INTF_MAX
@@ -247,6 +254,7 @@  enum {
 enum {
 	DPU_INTF_INPUT_CTRL = 0x1,
 	DPU_INTF_TE,
+	DPU_INTF_TE_ALIGN_VSYNC,
 	DPU_DATA_HCTL_EN,
 	DPU_INTF_MAX
 };
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 7ce66bf..238efdb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -1,6 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2022-2023, Qualcomm Innovation Center, Inc. All rights reserved.
  * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
  */
 
@@ -44,6 +44,7 @@ 
 #define   INTF_DEFLICKER_STRNG_COEFF    0x0F4
 #define   INTF_DEFLICKER_WEAK_COEFF     0x0F8
 
+#define   INTF_REG_SPLIT_LINK           0x080
 #define   INTF_DSI_CMD_MODE_TRIGGER_EN  0x084
 #define   INTF_PANEL_FORMAT             0x090
 #define   INTF_TPG_ENABLE               0x100
@@ -65,9 +66,9 @@ 
 
 #define INTF_CFG_ACTIVE_H_EN	BIT(29)
 #define INTF_CFG_ACTIVE_V_EN	BIT(30)
-
 #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
 #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
+#define INTF_CFG2_ALIGN_VSYNC_TO_TE BIT(16)
 
 #define INTF_MISR_CTRL			0x180
 #define INTF_MISR_SIGNATURE		0x184
@@ -91,6 +92,16 @@  static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
 	return ERR_PTR(-EINVAL);
 }
 
+static inline void _check_and_set_comp_bit(struct dpu_hw_intf *ctx,
+		bool dsc_4hs_merge, bool compression_en, u32 *intf_cfg2)
+{
+	if (((DPU_HW_MAJOR(ctx->hw_rev) >= DPU_HW_MAJOR(DPU_HW_VER_700)) && compression_en)
+		|| (IS_DPU_MAJOR_SAME(ctx->hw_rev, DPU_HW_VER_600) && dsc_4hs_merge))
+		(*intf_cfg2) |= BIT(12);
+	else if (!compression_en)
+		(*intf_cfg2) &= ~BIT(12);
+}
+
 static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 		const struct intf_timing_params *p,
 		const struct dpu_format *fmt)
@@ -113,82 +124,96 @@  static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 	/* read interface_cfg */
 	intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
 
-	if (ctx->cap->type == INTF_DP)
+	if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP)
 		dp_intf = true;
 
 	hsync_period = p->hsync_pulse_width + p->h_back_porch + p->width +
-	p->h_front_porch;
+			p->h_front_porch;
 	vsync_period = p->vsync_pulse_width + p->v_back_porch + p->height +
-	p->v_front_porch;
+			p->v_front_porch;
 
 	display_v_start = ((p->vsync_pulse_width + p->v_back_porch) *
-	hsync_period) + p->hsync_skew;
+			hsync_period) + p->hsync_skew;
 	display_v_end = ((vsync_period - p->v_front_porch) * hsync_period) +
-	p->hsync_skew - 1;
+			p->hsync_skew - 1;
+
+	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
 
 	hsync_start_x = p->h_back_porch + p->hsync_pulse_width;
 	hsync_end_x = hsync_period - p->h_front_porch - 1;
 
-	if (p->width != p->xres) { /* border fill added */
-		active_h_start = hsync_start_x;
-		active_h_end = active_h_start + p->xres - 1;
-	} else {
-		active_h_start = 0;
-		active_h_end = 0;
-	}
-
-	if (p->height != p->yres) { /* border fill added */
-		active_v_start = display_v_start;
-		active_v_end = active_v_start + (p->yres * hsync_period) - 1;
-	} else {
-		active_v_start = 0;
-		active_v_end = 0;
-	}
-
-	if (active_h_end) {
-		active_hctl = (active_h_end << 16) | active_h_start;
-		intf_cfg |= INTF_CFG_ACTIVE_H_EN;
-	} else {
-		active_hctl = 0;
-	}
-
-	if (active_v_end)
-		intf_cfg |= INTF_CFG_ACTIVE_V_EN;
-
-	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
-	display_hctl = (hsync_end_x << 16) | hsync_start_x;
-
 	/*
 	 * DATA_HCTL_EN controls data timing which can be different from
 	 * video timing. It is recommended to enable it for all cases, except
 	 * if compression is enabled in 1 pixel per clock mode
 	 */
+	if (!p->compression_en || p->wide_bus_en)
+		intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
+
 	if (p->wide_bus_en)
-		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
+		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
 
+	/*
+	 * If widebus is disabled:
+	 * For uncompressed stream, the data is valid for the entire active
+	 * window period.
+	 * For compressed stream, data is valid for a shorter time period
+	 * inside the active window depending on the compression ratio.
+	 *
+	 * If widebus is enabled:
+	 * For uncompressed stream, data is valid for only half the active
+	 * window, since the data rate is doubled in this mode.
+	 * p->width holds the adjusted width for DP but unadjusted width for DSI
+	 * For compressed stream, data validity window needs to be adjusted for
+	 * compression ratio and then further halved.
+	 */
 	data_width = p->width;
 
+	if (p->compression_en) {
+		if (p->wide_bus_en)
+			data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 6);
+		else
+			data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
+	} else if (!dp_intf && p->wide_bus_en) {
+		data_width = p->width >> 1;
+	} else {
+		data_width = p->width;
+	}
+
 	hsync_data_start_x = hsync_start_x;
 	hsync_data_end_x =  hsync_start_x + data_width - 1;
 
+	display_hctl = (hsync_end_x << 16) | hsync_start_x;
 	display_data_hctl = (hsync_data_end_x << 16) | hsync_data_start_x;
 
 	if (dp_intf) {
 		/* DP timing adjustment */
 		display_v_start += p->hsync_pulse_width + p->h_back_porch;
 		display_v_end   -= p->h_front_porch;
+	}
+
+	intf_cfg |= INTF_CFG_ACTIVE_H_EN;
+	intf_cfg |= INTF_CFG_ACTIVE_V_EN;
+	active_h_start = hsync_start_x;
+	active_h_end = active_h_start + p->xres - 1;
+	active_v_start = display_v_start;
+	active_v_end = active_v_start + (p->yres * hsync_period) - 1;
 
-		active_h_start = hsync_start_x;
-		active_h_end = active_h_start + p->xres - 1;
-		active_v_start = display_v_start;
-		active_v_end = active_v_start + (p->yres * hsync_period) - 1;
+	active_hctl = (active_h_end << 16) | active_h_start;
 
-		active_hctl = (active_h_end << 16) | active_h_start;
+	if (dp_intf) {
 		display_hctl = active_hctl;
 
-		intf_cfg |= INTF_CFG_ACTIVE_H_EN | INTF_CFG_ACTIVE_V_EN;
+		if (p->compression_en) {
+			active_data_hctl = (hsync_start_x + p->extra_dto_cycles) << 16;
+			active_data_hctl += hsync_start_x;
+
+			display_data_hctl = active_data_hctl;
+		}
 	}
 
+	_check_and_set_comp_bit(ctx, p->dsc_4hs_merge, p->compression_en, &intf_cfg2);
+
 	den_polarity = 0;
 	if (ctx->cap->type == INTF_HDMI) {
 		hsync_polarity = p->yres >= 720 ? 0 : 1;
@@ -202,7 +227,7 @@  static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 	}
 	polarity_ctl = (den_polarity << 2) | /*  DEN Polarity  */
 		(vsync_polarity << 1) | /* VSYNC Polarity */
-		(hsync_polarity << 0);  /* HSYNC Polarity */
+		 (hsync_polarity << 0);  /* HSYNC Polarity */
 
 	if (!DPU_FORMAT_IS_YUV(fmt))
 		panel_format = (fmt->bits[C0_G_Y] |
@@ -216,6 +241,17 @@  static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 				(COLOR_8BIT << 4) |
 				(0x21 << 8));
 
+	if (p->wide_bus_en)
+		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
+
+	/* Synchronize timing engine enable to TE */
+	if ((ctx->cap->features & BIT(DPU_INTF_TE_ALIGN_VSYNC))
+			&& p->poms_align_vsync)
+		intf_cfg2 |= INTF_CFG2_ALIGN_VSYNC_TO_TE;
+
+	if (ctx->cfg.split_link_en)
+		DPU_REG_WRITE(c, INTF_REG_SPLIT_LINK, 0x3);
+
 	DPU_REG_WRITE(c, INTF_HSYNC_CTL, hsync_ctl);
 	DPU_REG_WRITE(c, INTF_VSYNC_PERIOD_F0, vsync_period * hsync_period);
 	DPU_REG_WRITE(c, INTF_VSYNC_PULSE_WIDTH_F0,
@@ -233,11 +269,9 @@  static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 	DPU_REG_WRITE(c, INTF_FRAME_LINE_COUNT_EN, 0x3);
 	DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg);
 	DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format);
-	if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
-		DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
-		DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
-		DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
-	}
+	DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
+	DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
+	DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
 }
 
 static void dpu_hw_intf_enable_timing_engine(
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 643dd10..57be86d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -1,6 +1,6 @@ 
 /* SPDX-License-Identifier: GPL-2.0-only */
 /*
- * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2022-2023, Qualcomm Innovation Center, Inc. All rights reserved.
  * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
  */
 
@@ -10,6 +10,7 @@ 
 #include "dpu_hw_catalog.h"
 #include "dpu_hw_mdss.h"
 #include "dpu_hw_util.h"
+#include "dpu_hw_top.h"
 
 struct dpu_hw_intf;
 
@@ -33,6 +34,11 @@  struct intf_timing_params {
 	u32 hsync_skew;
 
 	bool wide_bus_en;
+	bool compression_en;
+	u32 extra_dto_cycles;   /* for DP only */
+	bool dsc_4hs_merge;     /* DSC 4HS merge */
+	bool poms_align_vsync;  /* poms with vsync aligned */
+	u32 dce_bytes_per_line;
 };
 
 struct intf_prog_fetch {
@@ -86,11 +92,13 @@  struct dpu_hw_intf_ops {
 
 struct dpu_hw_intf {
 	struct dpu_hw_blk_reg_map hw;
+	u32 hw_rev;	/* mdss hw_rev */
 
 	/* intf */
 	enum dpu_intf idx;
 	const struct dpu_intf_cfg *cap;
 	const struct dpu_mdss_cfg *mdss;
+	struct split_pipe_cfg cfg;
 
 	/* ops */
 	struct dpu_hw_intf_ops ops;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
index a1a9e44..1212fa2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
@@ -1,5 +1,7 @@ 
 /* SPDX-License-Identifier: GPL-2.0-only */
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/*
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef _DPU_HW_TOP_H
@@ -34,12 +36,14 @@  struct traffic_shaper_cfg {
  * @intf      : Interface id for main control path
  * @split_flush_en: Allows both the paths to be flushed when master path is
  *              flushed
+ * @split_link_en:  Check if split link is enabled
  */
 struct split_pipe_cfg {
 	bool en;
 	enum dpu_intf_mode mode;
 	enum dpu_intf intf;
 	bool split_flush_en;
+	bool split_link_en;
 };
 
 /**