| Message ID | 1674498274-6010-13-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | add display port DSC feature | expand |
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; > }; > > /**
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,
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, >
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, >>
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
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 --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; }; /**
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(-)