mbox series

[v3,0/7] add DSC 1.2 dpu supports

Message ID 1683061382-32651-1-git-send-email-quic_khsieh@quicinc.com
Headers show
Series add DSC 1.2 dpu supports | expand

Message

Kuogee Hsieh May 2, 2023, 9:02 p.m. UTC
This series adds the DPU side changes to support DSC 1.2 encoder. This
was validated with both DSI DSC 1.2 panel and DP DSC 1.2 monitor.
The DSI and DP parts will be pushed later on top of this change.
This seriel is rebase on [1], [2] and catalog fixes from [3].

[1]: https://patchwork.freedesktop.org/series/116851/
[2]: https://patchwork.freedesktop.org/series/116615/
[3]: https://patchwork.freedesktop.org/series/112332/


Abhinav Kumar (2):
  drm/msm/dpu: add dsc blocks for remaining chipsets in catalog
  drm/msm/dpu: add DSC 1.2 hw blocks for relevant chipsets

Kuogee Hsieh (5):
  drm/msm/dpu: add DPU_PINGPONG_DSC feature bit
  drm/msm/dpu: add DPU_PINGPONG_DSC bits into PP_BLK and PP_BLK_TE
    marcos
  drm/msm/dpu: add PINGPONG_NONE to disconnect DSC from PINGPONG
  drm/msm/dpu: add support for DSC encoder v1.2 engine
  drm/msm/dpu: separate DSC flush update out of interface

 drivers/gpu/drm/msm/Makefile                       |   1 +
 .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h    |  19 +-
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h |   8 +-
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h |  26 +-
 .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h    |  35 +-
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h |  26 +-
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h |   4 +-
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h |   2 +-
 .../drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h    |   2 +-
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h |  14 +
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h |   7 +
 .../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h   |  16 +
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h |  14 +
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h |  14 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        |  16 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |  35 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |  36 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c         |  22 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h         |  10 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c         |   7 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h         |  15 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c     | 383 +++++++++++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h        |   3 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c    |   9 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             |   7 +-
 25 files changed, 649 insertions(+), 82 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c

Comments

Dmitry Baryshkov May 2, 2023, 9:14 p.m. UTC | #1
On 03/05/2023 00:02, Kuogee Hsieh wrote:
> This series adds the DPU side changes to support DSC 1.2 encoder. This
> was validated with both DSI DSC 1.2 panel and DP DSC 1.2 monitor.
> The DSI and DP parts will be pushed later on top of this change.
> This seriel is rebase on [1], [2] and catalog fixes from [3].

Changelog?
Dmitry Baryshkov May 2, 2023, 9:38 p.m. UTC | #2
On 03/05/2023 00:03, Kuogee Hsieh wrote:
> Add support for DSC 1.2 by providing the necessary hooks to program
> the DPU DSC 1.2 encoder.
> 
> Changes in v3:
> -- fixed kernel test rebot report that "__iomem *off" is declared but not
>     used at dpu_hw_dsc_config_1_2()
> -- unrolling thresh loops
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/Makefile                   |   1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  34 ++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h     |  14 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 383 +++++++++++++++++++++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c         |   7 +-
>   5 files changed, 435 insertions(+), 4 deletions(-)
>   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index b814fc8..b9af5e4 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -65,6 +65,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
>   	disp/dpu1/dpu_hw_catalog.o \
>   	disp/dpu1/dpu_hw_ctl.o \
>   	disp/dpu1/dpu_hw_dsc.o \
> +	disp/dpu1/dpu_hw_dsc_1_2.o \
>   	disp/dpu1/dpu_hw_interrupts.o \
>   	disp/dpu1/dpu_hw_intf.o \
>   	disp/dpu1/dpu_hw_lm.o \
> 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 c07a6b6..b410a85 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.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, 2020 The Linux Foundation. All rights reserved.
>    */
>   
> @@ -243,12 +243,20 @@ enum {
>   };
>   
>   /**
> - * DSC features
> + * DSC sub-blocks/features
>    * @DPU_DSC_OUTPUT_CTRL       Configure which PINGPONG block gets
>    *                            the pixel output from this DSC.
> + * @DPU_DSC_HW_REV_1_1        DSC block supports dsc 1.1 only

Do we need a separate feature flag for this? IIRC, 1.1 is a common 
baseline. Do you plan to reuse the same interface for other compression 
blocks (e.g. FBC or VDC-M, if they were to be supported at some point)?

> + * @DPU_DSC_HW_REV_1_2        DSC block supports dsc 1.1 and 1.2
> + * @DPU_DSC_NATIVE_422_EN     Supports native422 and native420 encoding
> + * @DPU_DSC_MAX
>    */
>   enum {
>   	DPU_DSC_OUTPUT_CTRL = 0x1,
> +	DPU_DSC_HW_REV_1_1,
> +	DPU_DSC_HW_REV_1_2,
> +	DPU_DSC_NATIVE_422_EN,
> +	DPU_DSC_MAX
>   };
>   
>   /**
> @@ -313,6 +321,14 @@ struct dpu_pp_blk {
>   };
>   
>   /**
> + * struct dpu_dsc_blk - DSC Encoder sub-blk information
> + * @info:   HW register and features supported by this sub-blk
> + */
> +struct dpu_dsc_blk {
> +	DPU_HW_SUBBLK_INFO;
> +};
> +
> +/**
>    * enum dpu_qos_lut_usage - define QoS LUT use cases
>    */
>   enum dpu_qos_lut_usage {
> @@ -461,6 +477,17 @@ struct dpu_pingpong_sub_blks {
>   };
>   
>   /**
> + * struct dpu_dsc_sub_blks - DSC sub-blks
> + * @enc: DSC encoder sub block
> + * @ctl: DSC controller sub block
> + *
> + */
> +struct dpu_dsc_sub_blks {
> +	struct dpu_dsc_blk enc;
> +	struct dpu_dsc_blk ctl;
> +};
> +
> +/**
>    * dpu_clk_ctrl_type - Defines top level clock control signals
>    */
>   enum dpu_clk_ctrl_type {
> @@ -614,10 +641,13 @@ struct dpu_merge_3d_cfg  {
>    * struct dpu_dsc_cfg - information of DSC blocks
>    * @id                 enum identifying this block
>    * @base               register offset of this block
> + * @len:               length of hardware block
>    * @features           bit mask identifying sub-blocks/features
> + * @sblk               sub-blocks information
>    */
>   struct dpu_dsc_cfg {
>   	DPU_HW_BLK_INFO;
> +	const struct dpu_dsc_sub_blks *sblk;
>   };
>   
>   /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
> index 138080a..bdff74d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
> @@ -1,5 +1,8 @@
>   /* SPDX-License-Identifier: GPL-2.0-only */
> -/* Copyright (c) 2020-2022, Linaro Limited */
> +/*
> + * Copyright (c) 2020-2022, Linaro Limited
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
> + */
>   
>   #ifndef _DPU_HW_DSC_H
>   #define _DPU_HW_DSC_H
> @@ -69,6 +72,15 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(const struct dpu_dsc_cfg *cfg,
>   		void __iomem *addr);
>   
>   /**
> + * dpu_hw_dsc_init_1_2 - initializes the v1.2 DSC hw driver block
> + * @cfg:  DSC catalog entry for which driver object is required
> + * @addr: Mapped register io address of MDP
> + * Returns: Error code or allocated dpu_hw_dsc context
> + */
> +struct dpu_hw_dsc *dpu_hw_dsc_init_1_2(const struct dpu_dsc_cfg *cfg,
> +		void __iomem *addr);
> +
> +/**
>    * dpu_hw_dsc_destroy - destroys dsc driver context
>    * @dsc:   Pointer to dsc driver context returned by dpu_hw_dsc_init
>    */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
> new file mode 100644
> index 00000000..617c7f3
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
> @@ -0,0 +1,383 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
> + */
> +
> +#include <drm/display/drm_dsc_helper.h>
> +
> +#include "dpu_kms.h"
> +#include "dpu_hw_catalog.h"
> +#include "dpu_hwio.h"
> +#include "dpu_hw_mdss.h"
> +#include "dpu_hw_dsc.h"
> +
> +#define DSC_CMN_MAIN_CNF           0x00
> +
> +/* DPU_DSC_ENC register offsets */
> +#define ENC_DF_CTRL                0x00
> +#define ENC_GENERAL_STATUS         0x04
> +#define ENC_HSLICE_STATUS          0x08
> +#define ENC_OUT_STATUS             0x0C
> +#define ENC_INT_STAT               0x10
> +#define ENC_INT_CLR                0x14
> +#define ENC_INT_MASK               0x18
> +#define DSC_MAIN_CONF              0x30
> +#define DSC_PICTURE_SIZE           0x34
> +#define DSC_SLICE_SIZE             0x38
> +#define DSC_MISC_SIZE              0x3C
> +#define DSC_HRD_DELAYS             0x40
> +#define DSC_RC_SCALE               0x44
> +#define DSC_RC_SCALE_INC_DEC       0x48
> +#define DSC_RC_OFFSETS_1           0x4C
> +#define DSC_RC_OFFSETS_2           0x50
> +#define DSC_RC_OFFSETS_3           0x54
> +#define DSC_RC_OFFSETS_4           0x58
> +#define DSC_FLATNESS_QP            0x5C
> +#define DSC_RC_MODEL_SIZE          0x60
> +#define DSC_RC_CONFIG              0x64
> +#define DSC_RC_BUF_THRESH_0        0x68
> +#define DSC_RC_MIN_QP_0            0x78
> +#define DSC_RC_MAX_QP_0            0x84
> +#define DSC_RC_RANGE_BPG_OFFSETS_0 0x90
> +
> +/* DPU_DSC_CTL register offsets */
> +#define DSC_CTL                    0x00
> +#define DSC_CFG                    0x04
> +#define DSC_DATA_IN_SWAP           0x08
> +#define DSC_CLK_CTRL               0x0C
> +
> +static inline int _dsc_calc_ob_max_addr(struct dpu_hw_dsc *hw_dsc, int num_ss, bool native_422)
> +{
> +	int max_addr = 2400 / num_ss;
> +
> +	if ((hw_dsc->caps->features & BIT(DPU_DSC_NATIVE_422_EN)) && native_422)
> +		max_addr /= 2;

Do we need to check features bit here? I think it should be checked 
earlier, during atomic_check or mode_valid and at this point the driver 
should check just the dsc->native_422.

> +
> +	return max_addr - 1;
> +};
> +
> +static inline void dpu_hw_dsc_disable_1_2(struct dpu_hw_dsc *hw_dsc)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	u32 offset;
> +
> +	if (!hw_dsc)
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +	offset = hw_dsc->caps->sblk->ctl.base;
> +	DPU_REG_WRITE(hw, offset + DSC_CFG, 0);
> +
> +	offset = hw_dsc->caps->sblk->enc.base;
> +	DPU_REG_WRITE(hw, offset + ENC_DF_CTRL, 0);
> +	DPU_REG_WRITE(hw, offset + DSC_MAIN_CONF, 0);
> +}
> +
> +static inline void dpu_hw_dsc_config_1_2(struct dpu_hw_dsc *hw_dsc,
> +			      struct drm_dsc_config *dsc,
> +			      u32 mode,
> +			      u32 initial_lines)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	u32 offset;
> +	u32 data = 0;
> +	u32 det_thresh_flatness;
> +	u32 num_active_ss_per_enc;
> +	u32 bpp;
> +
> +	if (!hw_dsc || !dsc)
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +
> +	offset = hw_dsc->caps->sblk->enc.base;
> +
> +	if (mode & DSC_MODE_SPLIT_PANEL)
> +		data |= BIT(0);
> +
> +	if (mode & DSC_MODE_MULTIPLEX)
> +		data |= BIT(1);
> +
> +	num_active_ss_per_enc = dsc->slice_count;
> +	if (mode & DSC_MODE_MULTIPLEX)
> +		num_active_ss_per_enc = dsc->slice_count >> 1;
> +
> +	data |= (num_active_ss_per_enc & 0x3) << 7;
> +
> +	DPU_REG_WRITE(hw, DSC_CMN_MAIN_CNF, data);
> +
> +	data = (initial_lines & 0xff);
> +
> +	if (mode & DSC_MODE_VIDEO)
> +		data |= BIT(9);
> +
> +	data |= (_dsc_calc_ob_max_addr(hw_dsc, num_active_ss_per_enc, dsc->native_422) << 18);
> +
> +	DPU_REG_WRITE(hw, offset + ENC_DF_CTRL, data);
> +
> +	data = (dsc->dsc_version_minor & 0xf) << 28;
> +	if (dsc->dsc_version_minor == 0x2) {
> +		if (dsc->native_422)
> +			data |= BIT(22);
> +		if (dsc->native_420)
> +			data |= BIT(21);
> +	}
> +
> +	bpp = dsc->bits_per_pixel;
> +	/* as per hw requirement bpp should be programmed
> +	 * twice the actual value in case of 420 or 422 encoding
> +	 */
> +	if (dsc->native_422 || dsc->native_420)
> +		bpp = 2 * bpp;
> +	data |= (dsc->block_pred_enable ? 1 : 0) << 20;
> +	data |= bpp << 10;
> +	data |= (dsc->line_buf_depth & 0xf) << 6;
> +	data |= dsc->convert_rgb << 4;
> +	data |= dsc->bits_per_component & 0xf;
> +
> +	DPU_REG_WRITE(hw, offset + DSC_MAIN_CONF, data);
> +
> +	data = (dsc->pic_width & 0xffff) |
> +		((dsc->pic_height & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_PICTURE_SIZE, data);
> +
> +	data = (dsc->slice_width & 0xffff) |
> +		((dsc->slice_height & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_SLICE_SIZE, data);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_MISC_SIZE,
> +			(dsc->slice_chunk_size) & 0xffff);
> +
> +	data = (dsc->initial_xmit_delay & 0xffff) |
> +		((dsc->initial_dec_delay & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_HRD_DELAYS, data);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_RC_SCALE,
> +			dsc->initial_scale_value & 0x3f);
> +
> +	data = (dsc->scale_increment_interval & 0xffff) |
> +		((dsc->scale_decrement_interval & 0x7ff) << 16);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_RC_SCALE_INC_DEC, data);
> +
> +	data = (dsc->first_line_bpg_offset & 0x1f) |
> +		((dsc->second_line_bpg_offset & 0x1f) << 5);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_RC_OFFSETS_1, data);
> +
> +	data = (dsc->nfl_bpg_offset & 0xffff) |
> +		((dsc->slice_bpg_offset & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_RC_OFFSETS_2, data);
> +
> +	data = (dsc->initial_offset & 0xffff) |
> +		((dsc->final_offset & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_RC_OFFSETS_3, data);
> +
> +	data = (dsc->nsl_bpg_offset & 0xffff) |
> +		((dsc->second_line_offset_adj & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_RC_OFFSETS_4, data);
> +
> +	data = (dsc->flatness_min_qp & 0x1f);
> +	data |= (dsc->flatness_max_qp & 0x1f) << 5;
> +
> +	det_thresh_flatness = drm_dsc_calculate_flatness_det_thresh(dsc);
> +	data |= (det_thresh_flatness & 0xff) << 10;
> +
> +	DPU_REG_WRITE(hw, offset + DSC_FLATNESS_QP, data);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_RC_MODEL_SIZE,
> +			(dsc->rc_model_size) & 0xffff);
> +
> +	data = dsc->rc_edge_factor & 0xf;
> +	data |= (dsc->rc_quant_incr_limit0 & 0x1f) << 8;
> +	data |= (dsc->rc_quant_incr_limit1 & 0x1f) << 13;
> +	data |= (dsc->rc_tgt_offset_high & 0xf) << 20;
> +	data |= (dsc->rc_tgt_offset_low & 0xf) << 24;
> +
> +	DPU_REG_WRITE(hw, offset + DSC_RC_CONFIG, data);
> +
> +	/* program the dsc wrapper */
> +	offset = hw_dsc->caps->sblk->ctl.base;
> +
> +	data = BIT(0); /* encoder enable */
> +	if (dsc->native_422)
> +		data |= BIT(8);
> +	else if (dsc->native_420)
> +		data |= BIT(9);
> +	if (!dsc->convert_rgb)
> +		data |= BIT(10);
> +	if (dsc->bits_per_component == 8)
> +		data |= BIT(11);
> +	if (mode & DSC_MODE_SPLIT_PANEL)
> +		data |= BIT(12);
> +	if (mode & DSC_MODE_MULTIPLEX)
> +		data |= BIT(13);
> +	if (!(mode & DSC_MODE_VIDEO))
> +		data |= BIT(17);
> +
> +	DPU_REG_WRITE(hw, offset + DSC_CFG, data);
> +}
> +
> +static inline void dpu_hw_dsc_config_thresh_1_2(struct dpu_hw_dsc *hw_dsc,
> +					struct drm_dsc_config *dsc)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	u32 offset, off;
> +	struct drm_dsc_rc_range_parameters *rc;
> +
> +	if (!hw_dsc || !dsc)
> +		return;
> +
> +	offset = hw_dsc->caps->sblk->enc.base;
> +
> +	hw = &hw_dsc->hw;
> +
> +	rc = dsc->rc_range_params;
> +
> +	/*
> +	 * With BUF_THRESH -- 14 in total
> +	 * each register contains 4 thresh values with the last register
> +	 * containing only 2 thresh values
> +	 */
> +	off = 0;
> +	DPU_REG_WRITE(hw, offset + DSC_RC_BUF_THRESH_0 + off,
> +					(dsc->rc_buf_thresh[0] << 0) |
> +					(dsc->rc_buf_thresh[1] << 8) |
> +					(dsc->rc_buf_thresh[2] << 16) |
> +					(dsc->rc_buf_thresh[3] << 24));
> +	off += 4;

Please use proper indexed registers instead of manipulating the offset. 
Otherwise it's impossible to understand, which register is being written 
at this point.

> +	DPU_REG_WRITE(hw, offset + DSC_RC_BUF_THRESH_0 + off,
> +					(dsc->rc_buf_thresh[4] << 0) |
> +					(dsc->rc_buf_thresh[5] << 8) |
> +					(dsc->rc_buf_thresh[6] << 16) |
> +					(dsc->rc_buf_thresh[7] << 24));
> +	off += 4;
> +	DPU_REG_WRITE(hw, offset + DSC_RC_BUF_THRESH_0 + off,
> +					(dsc->rc_buf_thresh[8] << 0) |
> +					(dsc->rc_buf_thresh[9] << 8) |
> +					(dsc->rc_buf_thresh[10] << 16) |
> +					(dsc->rc_buf_thresh[11] << 24));
> +	off += 4;
> +	DPU_REG_WRITE(hw, offset + DSC_RC_BUF_THRESH_0 + off,
> +					(dsc->rc_buf_thresh[12] << 0) |
> +					(dsc->rc_buf_thresh[13] << 8));
> +
> +	/*
> +	 * with min/max_QP -- 5 bits each
> +	 * each register contains 5 min_qp or max_qp for total of 15
> +	 *
> +	 * With BPG_OFFSET -- 6 bits each
> +	 * each register contains 5 BPG_offset for total of 15
> +	 */
> +	off = 0;
> +	DPU_REG_WRITE(hw, offset + DSC_RC_MIN_QP_0 + off,
> +					(rc[0].range_min_qp << 0) |
> +					(rc[1].range_min_qp << 5) |
> +					(rc[2].range_min_qp << 10) |
> +					(rc[3].range_min_qp << 15) |
> +					(rc[4].range_min_qp << 20));
> +	DPU_REG_WRITE(hw, offset + DSC_RC_MAX_QP_0 + off,
> +					(rc[0].range_max_qp << 0) |
> +					(rc[1].range_max_qp << 5) |
> +					(rc[2].range_max_qp << 10) |
> +					(rc[3].range_max_qp << 15) |
> +					(rc[4].range_max_qp << 20));
> +	DPU_REG_WRITE(hw, offset + DSC_RC_RANGE_BPG_OFFSETS_0 + off,
> +					(rc[0].range_bpg_offset << 0) |
> +					(rc[1].range_bpg_offset << 6) |
> +					(rc[2].range_bpg_offset << 12) |
> +					(rc[3].range_bpg_offset << 18) |
> +					(rc[4].range_bpg_offset << 24));
> +
> +	off += 4;
> +	DPU_REG_WRITE(hw, offset + DSC_RC_MIN_QP_0 + off,
> +					(rc[5].range_min_qp << 0) |
> +					(rc[6].range_min_qp << 5) |
> +					(rc[7].range_min_qp << 10) |
> +					(rc[8].range_min_qp << 15) |
> +					(rc[9].range_min_qp << 20));
> +	DPU_REG_WRITE(hw, offset + DSC_RC_MAX_QP_0 + off,
> +					(rc[5].range_max_qp << 0) |
> +					(rc[6].range_max_qp << 5) |
> +					(rc[7].range_max_qp << 10) |
> +					(rc[8].range_max_qp << 15) |
> +					(rc[9].range_max_qp << 20));
> +	DPU_REG_WRITE(hw, offset + DSC_RC_RANGE_BPG_OFFSETS_0 + off,
> +					(rc[5].range_bpg_offset << 0) |
> +					(rc[6].range_bpg_offset << 6) |
> +					(rc[7].range_bpg_offset << 12) |
> +					(rc[8].range_bpg_offset << 18) |
> +					(rc[9].range_bpg_offset << 24));
> +
> +	off += 4;
> +	DPU_REG_WRITE(hw, offset + DSC_RC_MIN_QP_0 + off,
> +					(rc[10].range_min_qp << 0) |
> +					(rc[11].range_min_qp << 5) |
> +					(rc[12].range_min_qp << 10) |
> +					(rc[13].range_min_qp << 15) |
> +					(rc[14].range_min_qp << 20));
> +	DPU_REG_WRITE(hw, offset + DSC_RC_MAX_QP_0 + off,
> +					(rc[10].range_max_qp << 0) |
> +					(rc[11].range_max_qp << 5) |
> +					(rc[12].range_max_qp << 10) |
> +					(rc[13].range_max_qp << 15) |
> +					(rc[14].range_max_qp << 20));
> +	DPU_REG_WRITE(hw, offset + DSC_RC_RANGE_BPG_OFFSETS_0 + off,
> +					(rc[10].range_bpg_offset << 0) |
> +					(rc[11].range_bpg_offset << 6) |
> +					(rc[12].range_bpg_offset << 12) |
> +					(rc[13].range_bpg_offset << 18) |
> +					(rc[14].range_bpg_offset << 24));
> +}
> +
> +static inline void dpu_hw_dsc_bind_pingpong_blk_1_2(
> +		struct dpu_hw_dsc *hw_dsc,
> +		const enum dpu_pingpong pp)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	int offset;
> +	int mux_cfg = 0xf; /* Disabled */
> +
> +	offset = hw_dsc->caps->sblk->ctl.base;
> +
> +	hw = &hw_dsc->hw;
> +	if (pp)
> +		mux_cfg = (pp - PINGPONG_0) & 0x7;
> +
> +	DPU_REG_WRITE(hw, offset + DSC_CTL, mux_cfg);
> +}
> +
> +static void _setup_dcs_ops_1_2(struct dpu_hw_dsc_ops *ops,
> +		const unsigned long features)
> +{
> +	ops->dsc_disable = dpu_hw_dsc_disable_1_2;
> +	ops->dsc_config = dpu_hw_dsc_config_1_2;
> +	ops->dsc_config_thresh = dpu_hw_dsc_config_thresh_1_2;
> +	ops->dsc_bind_pingpong_blk = dpu_hw_dsc_bind_pingpong_blk_1_2;
> +}
> +
> +struct dpu_hw_dsc *dpu_hw_dsc_init_1_2(const struct dpu_dsc_cfg *cfg,
> +				   void __iomem *addr)
> +{
> +	struct dpu_hw_dsc *c;
> +
> +	c = kzalloc(sizeof(*c), GFP_KERNEL);
> +	if (!c)
> +		return ERR_PTR(-ENOMEM);
> +
> +	c->hw.blk_addr = addr + cfg->base;
> +	c->hw.log_mask = DPU_DBG_MASK_DSC;
> +
> +	c->idx = cfg->id;
> +	c->caps = cfg;
> +	_setup_dcs_ops_1_2(&c->ops, c->caps->features);
> +
> +	return c;
> +}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 3452f88..b2f618f6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   /*
>    * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>    */
>   
>   #define pr_fmt(fmt)	"[drm:%s] " fmt, __func__
> @@ -250,7 +251,11 @@ int dpu_rm_init(struct dpu_rm *rm,
>   		struct dpu_hw_dsc *hw;
>   		const struct dpu_dsc_cfg *dsc = &cat->dsc[i];
>   
> -		hw = dpu_hw_dsc_init(dsc, mmio);
> +		if (test_bit(DPU_DSC_HW_REV_1_2, &dsc->features))
> +			hw = dpu_hw_dsc_init_1_2(dsc, mmio);
> +		else
> +			hw = dpu_hw_dsc_init(dsc, mmio);
> +
>   		if (IS_ERR_OR_NULL(hw)) {
>   			rc = PTR_ERR(hw);
>   			DPU_ERROR("failed dsc object creation: err %d\n", rc);
Dmitry Baryshkov May 2, 2023, 10:42 p.m. UTC | #3
On 03/05/2023 00:02, Kuogee Hsieh wrote:
> At legacy chipsets, it required DPU_PINGPONG_DSC bit be set to indicate
> pingpong ops functions are required to complete DSC data path setup if
> this chipset has DSC hardware block presented. This patch add
> DPU_PINGPONG_DSC bit to both PP_BLK and PP_BLK_TE marcos if it has DSC
> hardware block presented.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h    | 12 +++++-----
>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h |  8 +++----
>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 26 ++++++++++------------
>   .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h    | 24 ++++++++++----------
>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 26 ++++++++++------------
>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h |  4 ++--
>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h |  2 +-
>   .../drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h    |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |  8 +++----
>   9 files changed, 54 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> index 17f821c..b7cd746 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> @@ -112,16 +112,16 @@ static const struct dpu_lm_cfg msm8998_lm[] = {
>   };
>   
>   static const struct dpu_pingpong_cfg msm8998_pp[] = {
> -	PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 0, sdm845_pp_sblk_te,
> -			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
> +	PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, BIT(DPU_PINGPONG_DSC), 0,
> +			sdm845_pp_sblk_te, DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12)),
> -	PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 0, sdm845_pp_sblk_te,
> -			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
> +	PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, BIT(DPU_PINGPONG_DSC), 0,
> +			sdm845_pp_sblk_te, DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13)),
> -	PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, sdm845_pp_sblk,
> +	PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, 0, sdm845_pp_sblk,
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14)),
> -	PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, sdm845_pp_sblk,
> +	PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, 0, sdm845_pp_sblk,
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15)),

Just to doublecheck: why don't we have DPU_PINGPONG_DSC for PP_3/_4? We 
do have them on sdm845. Is it because we should not use DSC with thos 
PINGPONG blocks?

>   };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> index ceca741..8888bd9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> @@ -110,16 +110,16 @@ static const struct dpu_lm_cfg sdm845_lm[] = {
>   };
>   
>   static const struct dpu_pingpong_cfg sdm845_pp[] = {
> -	PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 0, sdm845_pp_sblk_te,
> +	PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk_te,
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12)),
> -	PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 0, sdm845_pp_sblk_te,
> +	PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk_te,
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13)),
> -	PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, sdm845_pp_sblk,
> +	PP_BLK("pingpong_2", PINGPONG_2, 0x71000, BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk,
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14)),
> -	PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, sdm845_pp_sblk,
> +	PP_BLK("pingpong_3", PINGPONG_3, 0x71800, BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk,
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
>   			DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15)),
>

[skipped the rest, looks good to me]
Kuogee Hsieh May 2, 2023, 11:49 p.m. UTC | #4
On 5/2/2023 2:38 PM, Dmitry Baryshkov wrote:
> On 03/05/2023 00:03, Kuogee Hsieh wrote:
>> Add support for DSC 1.2 by providing the necessary hooks to program
>> the DPU DSC 1.2 encoder.
>>
>> Changes in v3:
>> -- fixed kernel test rebot report that "__iomem *off" is declared but 
>> not
>>     used at dpu_hw_dsc_config_1_2()
>> -- unrolling thresh loops
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/Makefile                   |   1 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  34 ++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h     |  14 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 383 
>> +++++++++++++++++++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c         |   7 +-
>>   5 files changed, 435 insertions(+), 4 deletions(-)
>>   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
>>
>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>> index b814fc8..b9af5e4 100644
>> --- a/drivers/gpu/drm/msm/Makefile
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -65,6 +65,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
>>       disp/dpu1/dpu_hw_catalog.o \
>>       disp/dpu1/dpu_hw_ctl.o \
>>       disp/dpu1/dpu_hw_dsc.o \
>> +    disp/dpu1/dpu_hw_dsc_1_2.o \
>>       disp/dpu1/dpu_hw_interrupts.o \
>>       disp/dpu1/dpu_hw_intf.o \
>>       disp/dpu1/dpu_hw_lm.o \
>> 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 c07a6b6..b410a85 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.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, 2020 The Linux Foundation. All rights 
>> reserved.
>>    */
>>   @@ -243,12 +243,20 @@ enum {
>>   };
>>     /**
>> - * DSC features
>> + * DSC sub-blocks/features
>>    * @DPU_DSC_OUTPUT_CTRL       Configure which PINGPONG block gets
>>    *                            the pixel output from this DSC.
>> + * @DPU_DSC_HW_REV_1_1        DSC block supports dsc 1.1 only
>
> Do we need a separate feature flag for this? IIRC, 1.1 is a common 
> baseline. Do you plan to reuse the same interface for other 
> compression blocks (e.g. FBC or VDC-M, if they were to be supported at 
> some point)?
>
>> + * @DPU_DSC_HW_REV_1_2        DSC block supports dsc 1.1 and 1.2
>> + * @DPU_DSC_NATIVE_422_EN     Supports native422 and native420 encoding
>> + * @DPU_DSC_MAX
>>    */
>>   enum {
>>       DPU_DSC_OUTPUT_CTRL = 0x1,
>> +    DPU_DSC_HW_REV_1_1,
>> +    DPU_DSC_HW_REV_1_2,
>> +    DPU_DSC_NATIVE_422_EN,
>> +    DPU_DSC_MAX
>>   };
>>     /**
>> @@ -313,6 +321,14 @@ struct dpu_pp_blk {
>>   };
>>     /**
>> + * struct dpu_dsc_blk - DSC Encoder sub-blk information
>> + * @info:   HW register and features supported by this sub-blk
>> + */
>> +struct dpu_dsc_blk {
>> +    DPU_HW_SUBBLK_INFO;
>> +};
>> +
>> +/**
>>    * enum dpu_qos_lut_usage - define QoS LUT use cases
>>    */
>>   enum dpu_qos_lut_usage {
>> @@ -461,6 +477,17 @@ struct dpu_pingpong_sub_blks {
>>   };
>>     /**
>> + * struct dpu_dsc_sub_blks - DSC sub-blks
>> + * @enc: DSC encoder sub block
>> + * @ctl: DSC controller sub block
>> + *
>> + */
>> +struct dpu_dsc_sub_blks {
>> +    struct dpu_dsc_blk enc;
>> +    struct dpu_dsc_blk ctl;
>> +};
>> +
>> +/**
>>    * dpu_clk_ctrl_type - Defines top level clock control signals
>>    */
>>   enum dpu_clk_ctrl_type {
>> @@ -614,10 +641,13 @@ struct dpu_merge_3d_cfg  {
>>    * struct dpu_dsc_cfg - information of DSC blocks
>>    * @id                 enum identifying this block
>>    * @base               register offset of this block
>> + * @len:               length of hardware block
>>    * @features           bit mask identifying sub-blocks/features
>> + * @sblk               sub-blocks information
>>    */
>>   struct dpu_dsc_cfg {
>>       DPU_HW_BLK_INFO;
>> +    const struct dpu_dsc_sub_blks *sblk;
>>   };
>>     /**
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
>> index 138080a..bdff74d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
>> @@ -1,5 +1,8 @@
>>   /* SPDX-License-Identifier: GPL-2.0-only */
>> -/* Copyright (c) 2020-2022, Linaro Limited */
>> +/*
>> + * Copyright (c) 2020-2022, Linaro Limited
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>> reserved
>> + */
>>     #ifndef _DPU_HW_DSC_H
>>   #define _DPU_HW_DSC_H
>> @@ -69,6 +72,15 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(const struct 
>> dpu_dsc_cfg *cfg,
>>           void __iomem *addr);
>>     /**
>> + * dpu_hw_dsc_init_1_2 - initializes the v1.2 DSC hw driver block
>> + * @cfg:  DSC catalog entry for which driver object is required
>> + * @addr: Mapped register io address of MDP
>> + * Returns: Error code or allocated dpu_hw_dsc context
>> + */
>> +struct dpu_hw_dsc *dpu_hw_dsc_init_1_2(const struct dpu_dsc_cfg *cfg,
>> +        void __iomem *addr);
>> +
>> +/**
>>    * dpu_hw_dsc_destroy - destroys dsc driver context
>>    * @dsc:   Pointer to dsc driver context returned by dpu_hw_dsc_init
>>    */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
>> new file mode 100644
>> index 00000000..617c7f3
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
>> @@ -0,0 +1,383 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>> reserved
>> + */
>> +
>> +#include <drm/display/drm_dsc_helper.h>
>> +
>> +#include "dpu_kms.h"
>> +#include "dpu_hw_catalog.h"
>> +#include "dpu_hwio.h"
>> +#include "dpu_hw_mdss.h"
>> +#include "dpu_hw_dsc.h"
>> +
>> +#define DSC_CMN_MAIN_CNF           0x00
>> +
>> +/* DPU_DSC_ENC register offsets */
>> +#define ENC_DF_CTRL                0x00
>> +#define ENC_GENERAL_STATUS         0x04
>> +#define ENC_HSLICE_STATUS          0x08
>> +#define ENC_OUT_STATUS             0x0C
>> +#define ENC_INT_STAT               0x10
>> +#define ENC_INT_CLR                0x14
>> +#define ENC_INT_MASK               0x18
>> +#define DSC_MAIN_CONF              0x30
>> +#define DSC_PICTURE_SIZE           0x34
>> +#define DSC_SLICE_SIZE             0x38
>> +#define DSC_MISC_SIZE              0x3C
>> +#define DSC_HRD_DELAYS             0x40
>> +#define DSC_RC_SCALE               0x44
>> +#define DSC_RC_SCALE_INC_DEC       0x48
>> +#define DSC_RC_OFFSETS_1           0x4C
>> +#define DSC_RC_OFFSETS_2           0x50
>> +#define DSC_RC_OFFSETS_3           0x54
>> +#define DSC_RC_OFFSETS_4           0x58
>> +#define DSC_FLATNESS_QP            0x5C
>> +#define DSC_RC_MODEL_SIZE          0x60
>> +#define DSC_RC_CONFIG              0x64
>> +#define DSC_RC_BUF_THRESH_0        0x68
>> +#define DSC_RC_MIN_QP_0            0x78
>> +#define DSC_RC_MAX_QP_0            0x84
>> +#define DSC_RC_RANGE_BPG_OFFSETS_0 0x90
>> +
>> +/* DPU_DSC_CTL register offsets */
>> +#define DSC_CTL                    0x00
>> +#define DSC_CFG                    0x04
>> +#define DSC_DATA_IN_SWAP           0x08
>> +#define DSC_CLK_CTRL               0x0C
>> +
>> +static inline int _dsc_calc_ob_max_addr(struct dpu_hw_dsc *hw_dsc, 
>> int num_ss, bool native_422)
>> +{
>> +    int max_addr = 2400 / num_ss;
>> +
>> +    if ((hw_dsc->caps->features & BIT(DPU_DSC_NATIVE_422_EN)) && 
>> native_422)
>> +        max_addr /= 2;
>
> Do we need to check features bit here? I think it should be checked 
> earlier, during atomic_check or mode_valid and at this point the 
> driver should check just the dsc->native_422.

I made mistake here.

this buffer size is decided base on encoder hardware core support 
NATIVE_422 or not. If it supports it, then need divided by two.

Therefore we should check only feature bit (DPU_DSC_NATIVE_422_EN).

It is nothing to do with format of content.

This match with down stream code.

I will correct this at next spin.


>
>> +
>> +    return max_addr - 1;
>> +};
>> +
>> +static inline void dpu_hw_dsc_disable_1_2(struct dpu_hw_dsc *hw_dsc)
>> +{
>> +    struct dpu_hw_blk_reg_map *hw;
>> +    u32 offset;
>> +
>> +    if (!hw_dsc)
>> +        return;
>> +
>> +    hw = &hw_dsc->hw;
>> +    offset = hw_dsc->caps->sblk->ctl.base;
>> +    DPU_REG_WRITE(hw, offset + DSC_CFG, 0);
>> +
>> +    offset = hw_dsc->caps->sblk->enc.base;
>> +    DPU_REG_WRITE(hw, offset + ENC_DF_CTRL, 0);
>> +    DPU_REG_WRITE(hw, offset + DSC_MAIN_CONF, 0);
>> +}
>> +
>> +static inline void dpu_hw_dsc_config_1_2(struct dpu_hw_dsc *hw_dsc,
>> +                  struct drm_dsc_config *dsc,
>> +                  u32 mode,
>> +                  u32 initial_lines)
>> +{
>> +    struct dpu_hw_blk_reg_map *hw;
>> +    u32 offset;
>> +    u32 data = 0;
>> +    u32 det_thresh_flatness;
>> +    u32 num_active_ss_per_enc;
>> +    u32 bpp;
>> +
>> +    if (!hw_dsc || !dsc)
>> +        return;
>> +
>> +    hw = &hw_dsc->hw;
>> +
>> +    offset = hw_dsc->caps->sblk->enc.base;
>> +
>> +    if (mode & DSC_MODE_SPLIT_PANEL)
>> +        data |= BIT(0);
>> +
>> +    if (mode & DSC_MODE_MULTIPLEX)
>> +        data |= BIT(1);
>> +
>> +    num_active_ss_per_enc = dsc->slice_count;
>> +    if (mode & DSC_MODE_MULTIPLEX)
>> +        num_active_ss_per_enc = dsc->slice_count >> 1;
>> +
>> +    data |= (num_active_ss_per_enc & 0x3) << 7;
>> +
>> +    DPU_REG_WRITE(hw, DSC_CMN_MAIN_CNF, data);
>> +
>> +    data = (initial_lines & 0xff);
>> +
>> +    if (mode & DSC_MODE_VIDEO)
>> +        data |= BIT(9);
>> +
>> +    data |= (_dsc_calc_ob_max_addr(hw_dsc, num_active_ss_per_enc, 
>> dsc->native_422) << 18);
>> +
>> +    DPU_REG_WRITE(hw, offset + ENC_DF_CTRL, data);
>> +
>> +    data = (dsc->dsc_version_minor & 0xf) << 28;
>> +    if (dsc->dsc_version_minor == 0x2) {
>> +        if (dsc->native_422)
>> +            data |= BIT(22);
>> +        if (dsc->native_420)
>> +            data |= BIT(21);
>> +    }
>> +
>> +    bpp = dsc->bits_per_pixel;
>> +    /* as per hw requirement bpp should be programmed
>> +     * twice the actual value in case of 420 or 422 encoding
>> +     */
>> +    if (dsc->native_422 || dsc->native_420)
>> +        bpp = 2 * bpp;
>> +    data |= (dsc->block_pred_enable ? 1 : 0) << 20;
>> +    data |= bpp << 10;
>> +    data |= (dsc->line_buf_depth & 0xf) << 6;
>> +    data |= dsc->convert_rgb << 4;
>> +    data |= dsc->bits_per_component & 0xf;
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_MAIN_CONF, data);
>> +
>> +    data = (dsc->pic_width & 0xffff) |
>> +        ((dsc->pic_height & 0xffff) << 16);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_PICTURE_SIZE, data);
>> +
>> +    data = (dsc->slice_width & 0xffff) |
>> +        ((dsc->slice_height & 0xffff) << 16);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_SLICE_SIZE, data);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_MISC_SIZE,
>> +            (dsc->slice_chunk_size) & 0xffff);
>> +
>> +    data = (dsc->initial_xmit_delay & 0xffff) |
>> +        ((dsc->initial_dec_delay & 0xffff) << 16);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_HRD_DELAYS, data);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_SCALE,
>> +            dsc->initial_scale_value & 0x3f);
>> +
>> +    data = (dsc->scale_increment_interval & 0xffff) |
>> +        ((dsc->scale_decrement_interval & 0x7ff) << 16);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_SCALE_INC_DEC, data);
>> +
>> +    data = (dsc->first_line_bpg_offset & 0x1f) |
>> +        ((dsc->second_line_bpg_offset & 0x1f) << 5);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_OFFSETS_1, data);
>> +
>> +    data = (dsc->nfl_bpg_offset & 0xffff) |
>> +        ((dsc->slice_bpg_offset & 0xffff) << 16);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_OFFSETS_2, data);
>> +
>> +    data = (dsc->initial_offset & 0xffff) |
>> +        ((dsc->final_offset & 0xffff) << 16);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_OFFSETS_3, data);
>> +
>> +    data = (dsc->nsl_bpg_offset & 0xffff) |
>> +        ((dsc->second_line_offset_adj & 0xffff) << 16);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_OFFSETS_4, data);
>> +
>> +    data = (dsc->flatness_min_qp & 0x1f);
>> +    data |= (dsc->flatness_max_qp & 0x1f) << 5;
>> +
>> +    det_thresh_flatness = drm_dsc_calculate_flatness_det_thresh(dsc);
>> +    data |= (det_thresh_flatness & 0xff) << 10;
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_FLATNESS_QP, data);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_MODEL_SIZE,
>> +            (dsc->rc_model_size) & 0xffff);
>> +
>> +    data = dsc->rc_edge_factor & 0xf;
>> +    data |= (dsc->rc_quant_incr_limit0 & 0x1f) << 8;
>> +    data |= (dsc->rc_quant_incr_limit1 & 0x1f) << 13;
>> +    data |= (dsc->rc_tgt_offset_high & 0xf) << 20;
>> +    data |= (dsc->rc_tgt_offset_low & 0xf) << 24;
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_CONFIG, data);
>> +
>> +    /* program the dsc wrapper */
>> +    offset = hw_dsc->caps->sblk->ctl.base;
>> +
>> +    data = BIT(0); /* encoder enable */
>> +    if (dsc->native_422)
>> +        data |= BIT(8);
>> +    else if (dsc->native_420)
>> +        data |= BIT(9);
>> +    if (!dsc->convert_rgb)
>> +        data |= BIT(10);
>> +    if (dsc->bits_per_component == 8)
>> +        data |= BIT(11);
>> +    if (mode & DSC_MODE_SPLIT_PANEL)
>> +        data |= BIT(12);
>> +    if (mode & DSC_MODE_MULTIPLEX)
>> +        data |= BIT(13);
>> +    if (!(mode & DSC_MODE_VIDEO))
>> +        data |= BIT(17);
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_CFG, data);
>> +}
>> +
>> +static inline void dpu_hw_dsc_config_thresh_1_2(struct dpu_hw_dsc 
>> *hw_dsc,
>> +                    struct drm_dsc_config *dsc)
>> +{
>> +    struct dpu_hw_blk_reg_map *hw;
>> +    u32 offset, off;
>> +    struct drm_dsc_rc_range_parameters *rc;
>> +
>> +    if (!hw_dsc || !dsc)
>> +        return;
>> +
>> +    offset = hw_dsc->caps->sblk->enc.base;
>> +
>> +    hw = &hw_dsc->hw;
>> +
>> +    rc = dsc->rc_range_params;
>> +
>> +    /*
>> +     * With BUF_THRESH -- 14 in total
>> +     * each register contains 4 thresh values with the last register
>> +     * containing only 2 thresh values
>> +     */
>> +    off = 0;
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_BUF_THRESH_0 + off,
>> +                    (dsc->rc_buf_thresh[0] << 0) |
>> +                    (dsc->rc_buf_thresh[1] << 8) |
>> +                    (dsc->rc_buf_thresh[2] << 16) |
>> +                    (dsc->rc_buf_thresh[3] << 24));
>> +    off += 4;
>
> Please use proper indexed registers instead of manipulating the 
> offset. Otherwise it's impossible to understand, which register is 
> being written at this point.
>
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_BUF_THRESH_0 + off,
>> +                    (dsc->rc_buf_thresh[4] << 0) |
>> +                    (dsc->rc_buf_thresh[5] << 8) |
>> +                    (dsc->rc_buf_thresh[6] << 16) |
>> +                    (dsc->rc_buf_thresh[7] << 24));
>> +    off += 4;
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_BUF_THRESH_0 + off,
>> +                    (dsc->rc_buf_thresh[8] << 0) |
>> +                    (dsc->rc_buf_thresh[9] << 8) |
>> +                    (dsc->rc_buf_thresh[10] << 16) |
>> +                    (dsc->rc_buf_thresh[11] << 24));
>> +    off += 4;
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_BUF_THRESH_0 + off,
>> +                    (dsc->rc_buf_thresh[12] << 0) |
>> +                    (dsc->rc_buf_thresh[13] << 8));
>> +
>> +    /*
>> +     * with min/max_QP -- 5 bits each
>> +     * each register contains 5 min_qp or max_qp for total of 15
>> +     *
>> +     * With BPG_OFFSET -- 6 bits each
>> +     * each register contains 5 BPG_offset for total of 15
>> +     */
>> +    off = 0;
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_MIN_QP_0 + off,
>> +                    (rc[0].range_min_qp << 0) |
>> +                    (rc[1].range_min_qp << 5) |
>> +                    (rc[2].range_min_qp << 10) |
>> +                    (rc[3].range_min_qp << 15) |
>> +                    (rc[4].range_min_qp << 20));
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_MAX_QP_0 + off,
>> +                    (rc[0].range_max_qp << 0) |
>> +                    (rc[1].range_max_qp << 5) |
>> +                    (rc[2].range_max_qp << 10) |
>> +                    (rc[3].range_max_qp << 15) |
>> +                    (rc[4].range_max_qp << 20));
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_RANGE_BPG_OFFSETS_0 + off,
>> +                    (rc[0].range_bpg_offset << 0) |
>> +                    (rc[1].range_bpg_offset << 6) |
>> +                    (rc[2].range_bpg_offset << 12) |
>> +                    (rc[3].range_bpg_offset << 18) |
>> +                    (rc[4].range_bpg_offset << 24));
>> +
>> +    off += 4;
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_MIN_QP_0 + off,
>> +                    (rc[5].range_min_qp << 0) |
>> +                    (rc[6].range_min_qp << 5) |
>> +                    (rc[7].range_min_qp << 10) |
>> +                    (rc[8].range_min_qp << 15) |
>> +                    (rc[9].range_min_qp << 20));
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_MAX_QP_0 + off,
>> +                    (rc[5].range_max_qp << 0) |
>> +                    (rc[6].range_max_qp << 5) |
>> +                    (rc[7].range_max_qp << 10) |
>> +                    (rc[8].range_max_qp << 15) |
>> +                    (rc[9].range_max_qp << 20));
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_RANGE_BPG_OFFSETS_0 + off,
>> +                    (rc[5].range_bpg_offset << 0) |
>> +                    (rc[6].range_bpg_offset << 6) |
>> +                    (rc[7].range_bpg_offset << 12) |
>> +                    (rc[8].range_bpg_offset << 18) |
>> +                    (rc[9].range_bpg_offset << 24));
>> +
>> +    off += 4;
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_MIN_QP_0 + off,
>> +                    (rc[10].range_min_qp << 0) |
>> +                    (rc[11].range_min_qp << 5) |
>> +                    (rc[12].range_min_qp << 10) |
>> +                    (rc[13].range_min_qp << 15) |
>> +                    (rc[14].range_min_qp << 20));
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_MAX_QP_0 + off,
>> +                    (rc[10].range_max_qp << 0) |
>> +                    (rc[11].range_max_qp << 5) |
>> +                    (rc[12].range_max_qp << 10) |
>> +                    (rc[13].range_max_qp << 15) |
>> +                    (rc[14].range_max_qp << 20));
>> +    DPU_REG_WRITE(hw, offset + DSC_RC_RANGE_BPG_OFFSETS_0 + off,
>> +                    (rc[10].range_bpg_offset << 0) |
>> +                    (rc[11].range_bpg_offset << 6) |
>> +                    (rc[12].range_bpg_offset << 12) |
>> +                    (rc[13].range_bpg_offset << 18) |
>> +                    (rc[14].range_bpg_offset << 24));
>> +}
>> +
>> +static inline void dpu_hw_dsc_bind_pingpong_blk_1_2(
>> +        struct dpu_hw_dsc *hw_dsc,
>> +        const enum dpu_pingpong pp)
>> +{
>> +    struct dpu_hw_blk_reg_map *hw;
>> +    int offset;
>> +    int mux_cfg = 0xf; /* Disabled */
>> +
>> +    offset = hw_dsc->caps->sblk->ctl.base;
>> +
>> +    hw = &hw_dsc->hw;
>> +    if (pp)
>> +        mux_cfg = (pp - PINGPONG_0) & 0x7;
>> +
>> +    DPU_REG_WRITE(hw, offset + DSC_CTL, mux_cfg);
>> +}
>> +
>> +static void _setup_dcs_ops_1_2(struct dpu_hw_dsc_ops *ops,
>> +        const unsigned long features)
>> +{
>> +    ops->dsc_disable = dpu_hw_dsc_disable_1_2;
>> +    ops->dsc_config = dpu_hw_dsc_config_1_2;
>> +    ops->dsc_config_thresh = dpu_hw_dsc_config_thresh_1_2;
>> +    ops->dsc_bind_pingpong_blk = dpu_hw_dsc_bind_pingpong_blk_1_2;
>> +}
>> +
>> +struct dpu_hw_dsc *dpu_hw_dsc_init_1_2(const struct dpu_dsc_cfg *cfg,
>> +                   void __iomem *addr)
>> +{
>> +    struct dpu_hw_dsc *c;
>> +
>> +    c = kzalloc(sizeof(*c), GFP_KERNEL);
>> +    if (!c)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    c->hw.blk_addr = addr + cfg->base;
>> +    c->hw.log_mask = DPU_DBG_MASK_DSC;
>> +
>> +    c->idx = cfg->id;
>> +    c->caps = cfg;
>> +    _setup_dcs_ops_1_2(&c->ops, c->caps->features);
>> +
>> +    return c;
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> index 3452f88..b2f618f6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>>   /*
>>    * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>>    */
>>     #define pr_fmt(fmt)    "[drm:%s] " fmt, __func__
>> @@ -250,7 +251,11 @@ int dpu_rm_init(struct dpu_rm *rm,
>>           struct dpu_hw_dsc *hw;
>>           const struct dpu_dsc_cfg *dsc = &cat->dsc[i];
>>   -        hw = dpu_hw_dsc_init(dsc, mmio);
>> +        if (test_bit(DPU_DSC_HW_REV_1_2, &dsc->features))
>> +            hw = dpu_hw_dsc_init_1_2(dsc, mmio);
>> +        else
>> +            hw = dpu_hw_dsc_init(dsc, mmio);
>> +
>>           if (IS_ERR_OR_NULL(hw)) {
>>               rc = PTR_ERR(hw);
>>               DPU_ERROR("failed dsc object creation: err %d\n", rc);
>
Marijn Suijten May 3, 2023, 8:35 a.m. UTC | #5
Nit: drop leading space, use singular support*.

On 2023-05-02 14:02:55, Kuogee Hsieh wrote:
> This series adds the DPU side changes to support DSC 1.2 encoder. This
> was validated with both DSI DSC 1.2 panel and DP DSC 1.2 monitor.
> The DSI and DP parts will be pushed later on top of this change.
> This seriel is rebase on [1], [2] and catalog fixes from [3].

rebased*
> 
> [1]: https://patchwork.freedesktop.org/series/116851/
> [2]: https://patchwork.freedesktop.org/series/116615/
> [3]: https://patchwork.freedesktop.org/series/112332/

Was it only rebased on the catalog fixes from my INTF TE [3] support
series?  I cannot get patch 2/7 of this series to apply cleanly on top
of the entire INTF TE series because of a conflict with INTF TE patch
14/22 [4].

Patch 3/7 ("add DPU_PINGPONG_DSC bits into PP_BLK and PP_BLK_TE marcos")
doesn't apply either, probably because I removed PP_BLK_TE entirely in
that same ([4]) patch.

[4]: https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-14-27ce1a5ab5c6@somainline.org/

We should discuss in what order the series are going to be picked, so
that it is all compatible.

- Marijn

> Abhinav Kumar (2):
>   drm/msm/dpu: add dsc blocks for remaining chipsets in catalog
>   drm/msm/dpu: add DSC 1.2 hw blocks for relevant chipsets
> 
> Kuogee Hsieh (5):
>   drm/msm/dpu: add DPU_PINGPONG_DSC feature bit
>   drm/msm/dpu: add DPU_PINGPONG_DSC bits into PP_BLK and PP_BLK_TE
>     marcos
>   drm/msm/dpu: add PINGPONG_NONE to disconnect DSC from PINGPONG
>   drm/msm/dpu: add support for DSC encoder v1.2 engine
>   drm/msm/dpu: separate DSC flush update out of interface
> 
>  drivers/gpu/drm/msm/Makefile                       |   1 +
>  .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h    |  19 +-
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h |   8 +-
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h |  26 +-
>  .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h    |  35 +-
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h |  26 +-
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h |   4 +-
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h |   2 +-
>  .../drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h    |   2 +-
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h |  14 +
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h |   7 +
>  .../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h   |  16 +
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h |  14 +
>  .../gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h |  14 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        |  16 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |  35 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |  36 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c         |  22 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h         |  10 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c         |   7 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h         |  15 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c     | 383 +++++++++++++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h        |   3 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c    |   9 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             |   7 +-
>  25 files changed, 649 insertions(+), 82 deletions(-)
>  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Kuogee Hsieh May 3, 2023, 5:45 p.m. UTC | #6
On 5/2/2023 3:42 PM, Dmitry Baryshkov wrote:
> On 03/05/2023 00:02, Kuogee Hsieh wrote:
>> At legacy chipsets, it required DPU_PINGPONG_DSC bit be set to indicate
>> pingpong ops functions are required to complete DSC data path setup if
>> this chipset has DSC hardware block presented. This patch add
>> DPU_PINGPONG_DSC bit to both PP_BLK and PP_BLK_TE marcos if it has DSC
>> hardware block presented.
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h    | 12 +++++-----
>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h |  8 +++----
>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 26 
>> ++++++++++------------
>>   .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h    | 24 
>> ++++++++++----------
>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 26 
>> ++++++++++------------
>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h |  4 ++--
>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h |  2 +-
>>   .../drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h    |  2 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |  8 +++----
>>   9 files changed, 54 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> index 17f821c..b7cd746 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> @@ -112,16 +112,16 @@ static const struct dpu_lm_cfg msm8998_lm[] = {
>>   };
>>     static const struct dpu_pingpong_cfg msm8998_pp[] = {
>> -    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 0, sdm845_pp_sblk_te,
>> -            DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>> +    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 
>> BIT(DPU_PINGPONG_DSC), 0,
>> +            sdm845_pp_sblk_te, DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12)),
>> -    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 0, sdm845_pp_sblk_te,
>> -            DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>> +    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 
>> BIT(DPU_PINGPONG_DSC), 0,
>> +            sdm845_pp_sblk_te, DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13)),
>> -    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, sdm845_pp_sblk,
>> +    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, 0, sdm845_pp_sblk,
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14)),
>> -    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, sdm845_pp_sblk,
>> +    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, 0, sdm845_pp_sblk,
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15)),
>
> Just to doublecheck: why don't we have DPU_PINGPONG_DSC for PP_3/_4? 
> We do have them on sdm845. Is it because we should not use DSC with 
> thos PINGPONG blocks?
>
I think it only have two DSPP connect to pp blocks
>>   };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> index ceca741..8888bd9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> @@ -110,16 +110,16 @@ static const struct dpu_lm_cfg sdm845_lm[] = {
>>   };
>>     static const struct dpu_pingpong_cfg sdm845_pp[] = {
>> -    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 0, sdm845_pp_sblk_te,
>> +    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 
>> BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk_te,
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12)),
>> -    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 0, sdm845_pp_sblk_te,
>> +    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 
>> BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk_te,
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13)),
>> -    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, sdm845_pp_sblk,
>> +    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, BIT(DPU_PINGPONG_DSC), 
>> 0, sdm845_pp_sblk,
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14)),
>> -    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, sdm845_pp_sblk,
>> +    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, BIT(DPU_PINGPONG_DSC), 
>> 0, sdm845_pp_sblk,
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15)),
>>
>
> [skipped the rest, looks good to me]
>
Abhinav Kumar May 3, 2023, 6:54 p.m. UTC | #7
On 5/2/2023 2:42 PM, Dmitry Baryshkov wrote:
> On 03/05/2023 00:03, Kuogee Hsieh wrote:
>> From: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>> Add DSC 1.2 hardware blocks to the catalog with necessary sub-block and
>> feature flag information.  Each display compression engine (DCE) contains
>> dual hard slice DSC encoders so both share same base address but with
>> its own different sub block address.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Minor question below.
> 
>> ---
>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 14 +++++++++++
>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h |  7 ++++++
>>   .../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h   | 16 +++++++++++++
>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 14 +++++++++++
>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 14 +++++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     | 27 
>> ++++++++++++++++++++--
>>   6 files changed, 90 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> index 4f6a965..f98c2a5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> @@ -153,6 +153,18 @@ static const struct dpu_merge_3d_cfg 
>> sm8350_merge_3d[] = {
>>       MERGE_3D_BLK("merge_3d_2", MERGE_3D_2, 0x50000),
>>   };
>> +/*
>> + * NOTE: Each display compression engine (DCE) contains dual hard
>> + * slice DSC encoders so both share same base address but with
>> + * its own different sub block address.
>> + */
>> +static const struct dpu_dsc_cfg sm8350_dsc[] = {
>> +    DSC_BLK_1_2("dce_0", DSC_0, 0x80000, 0x100, 0, dsc_sblk_0),
>> +    DSC_BLK_1_2("dce_0", DSC_1, 0x80000, 0x100, 0, dsc_sblk_1),
>> +    DSC_BLK_1_2("dce_1", DSC_2, 0x81000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_0),
>> +    DSC_BLK_1_2("dce_1", DSC_3, 0x81000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_1),
>> +};
>> +
>>   static const struct dpu_intf_cfg sm8350_intf[] = {
>>       INTF_BLK("intf_0", INTF_0, 0x34000, 0x280, INTF_DP, 
>> MSM_DP_CONTROLLER_0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>>       INTF_BLK("intf_1", INTF_1, 0x35000, 0x2c4, INTF_DSI, 0, 24, 
>> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
>> @@ -205,6 +217,8 @@ const struct dpu_mdss_cfg dpu_sm8350_cfg = {
>>       .dspp = sm8350_dspp,
>>       .pingpong_count = ARRAY_SIZE(sm8350_pp),
>>       .pingpong = sm8350_pp,
>> +    .dsc = sm8350_dsc,
>> +    .dsc_count = ARRAY_SIZE(sm8350_dsc),
>>       .merge_3d_count = ARRAY_SIZE(sm8350_merge_3d),
>>       .merge_3d = sm8350_merge_3d,
>>       .intf_count = ARRAY_SIZE(sm8350_intf),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> index 6b2c7ea..3fd0498a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> @@ -93,6 +93,11 @@ static const struct dpu_pingpong_cfg sc7280_pp[] = {
>>       PP_BLK_DITHER("pingpong_3", PINGPONG_3, 0x6c000, 0, 
>> sc7280_pp_sblk, -1, -1),
>>   };
>> +/* NOTE: sc7280 only has one dsc hard slice encoder */
>> +static const struct dpu_dsc_cfg sc7280_dsc[] = {
>> +    DSC_BLK_1_2("dce_0", DSC_0, 0x80000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_0),
>> +};
>> +
>>   static const struct dpu_intf_cfg sc7280_intf[] = {
>>       INTF_BLK("intf_0", INTF_0, 0x34000, 0x280, INTF_DP, 
>> MSM_DP_CONTROLLER_0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>>       INTF_BLK("intf_1", INTF_1, 0x35000, 0x2c4, INTF_DSI, 0, 24, 
>> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
>> @@ -142,6 +147,8 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
>>       .mixer = sc7280_lm,
>>       .pingpong_count = ARRAY_SIZE(sc7280_pp),
>>       .pingpong = sc7280_pp,
>> +    .dsc_count = ARRAY_SIZE(sc7280_dsc),
>> +    .dsc = sc7280_dsc,
>>       .intf_count = ARRAY_SIZE(sc7280_intf),
>>       .intf = sc7280_intf,
>>       .vbif_count = ARRAY_SIZE(sdm845_vbif),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> index 706d0f1..ce583eb 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> @@ -141,6 +141,20 @@ static const struct dpu_merge_3d_cfg 
>> sc8280xp_merge_3d[] = {
>>       MERGE_3D_BLK("merge_3d_2", MERGE_3D_2, 0x50000),
>>   };
>> +/*
>> + * NOTE: Each display compression engine (DCE) contains dual hard
>> + * slice DSC encoders so both share same base address but with
>> + * its own different sub block address.
>> + */
>> +static const struct dpu_dsc_cfg sc8280xp_dsc[] = {
>> +    DSC_BLK_1_2("dce_0", DSC_0, 0x80000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_0),
>> +    DSC_BLK_1_2("dce_0", DSC_1, 0x80000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_1),
>> +    DSC_BLK_1_2("dce_1", DSC_2, 0x81000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_0),
>> +    DSC_BLK_1_2("dce_1", DSC_3, 0x81000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_1),
>> +    DSC_BLK_1_2("dce_2", DSC_4, 0x82000, 0x100, 0, dsc_sblk_0),
>> +    DSC_BLK_1_2("dce_2", DSC_5, 0x82000, 0x100, 0, dsc_sblk_1),
> 
> Just checking: all other platforms have non-422 encoders first, then the 
> 422-enabled ones. Is there any reason why this platform has them in the 
> opposite order?
> 

This turned out to be a mistake due to a confusion in the internal 
documents.

So this should be updated to :

DSC_0 pair -> only RGB
DSC_1 pair -> DPU_DSC_NATIVE_422_EN
DSC_2 pair -> only RGB

There was a generic statement in the document that with 3 DSC pairs 
first two are YUV 422 capable and third one is RGB. I assumed that was 
true for all chipsets with 3 DSC pairs and made this change but i was 
wrong as that was specific to some other chipset not present in this 
catalog yet.

Rest of the chipsets are correct as they have only two DSC pairs.

Will update this one.

>> +};
>> +
>>   /* TODO: INTF 3, 8 and 7 are used for MST, marked as INTF_NONE for 
>> now */
>>   static const struct dpu_intf_cfg sc8280xp_intf[] = {
>>       INTF_BLK("intf_0", INTF_0, 0x34000, 0x280, INTF_DP, 
>> MSM_DP_CONTROLLER_0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>> @@ -196,6 +210,8 @@ const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
>>       .dspp = sc8280xp_dspp,
>>       .pingpong_count = ARRAY_SIZE(sc8280xp_pp),
>>       .pingpong = sc8280xp_pp,
>> +    .dsc = sc8280xp_dsc,
>> +    .dsc_count = ARRAY_SIZE(sc8280xp_dsc),
>>       .merge_3d_count = ARRAY_SIZE(sc8280xp_merge_3d),
>>       .merge_3d = sc8280xp_merge_3d,
>>       .intf_count = ARRAY_SIZE(sc8280xp_intf),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> index 4ecb3df..3950e7b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> @@ -161,6 +161,18 @@ static const struct dpu_merge_3d_cfg 
>> sm8450_merge_3d[] = {
>>       MERGE_3D_BLK("merge_3d_3", MERGE_3D_3, 0x65f00),
>>   };
>> +/*
>> + * NOTE: Each display compression engine (DCE) contains dual hard
>> + * slice DSC encoders so both share same base address but with
>> + * its own different sub block address.
>> + */
>> +static const struct dpu_dsc_cfg sm8450_dsc[] = {
>> +    DSC_BLK_1_2("dce_0", DSC_0, 0x80000, 0x100, 0, dsc_sblk_0),
>> +    DSC_BLK_1_2("dce_0", DSC_1, 0x80000, 0x100, 0, dsc_sblk_1),
>> +    DSC_BLK_1_2("dce_1", DSC_2, 0x81000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_0),
>> +    DSC_BLK_1_2("dce_1", DSC_3, 0x81000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_1),
>> +};
>> +
>>   static const struct dpu_intf_cfg sm8450_intf[] = {
>>       INTF_BLK("intf_0", INTF_0, 0x34000, 0x280, INTF_DP, 
>> MSM_DP_CONTROLLER_0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>>       INTF_BLK("intf_1", INTF_1, 0x35000, 0x300, INTF_DSI, 0, 24, 
>> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
>> @@ -213,6 +225,8 @@ const struct dpu_mdss_cfg dpu_sm8450_cfg = {
>>       .dspp = sm8450_dspp,
>>       .pingpong_count = ARRAY_SIZE(sm8450_pp),
>>       .pingpong = sm8450_pp,
>> +    .dsc = sm8450_dsc,
>> +    .dsc_count = ARRAY_SIZE(sm8450_dsc),
>>       .merge_3d_count = ARRAY_SIZE(sm8450_merge_3d),
>>       .merge_3d = sm8450_merge_3d,
>>       .intf_count = ARRAY_SIZE(sm8450_intf),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> index d0ab351..1b3f542 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> @@ -165,6 +165,18 @@ static const struct dpu_merge_3d_cfg 
>> sm8550_merge_3d[] = {
>>       MERGE_3D_BLK("merge_3d_3", MERGE_3D_3, 0x66700),
>>   };
>> +/*
>> + * NOTE: Each display compression engine (DCE) contains dual hard
>> + * slice DSC encoders so both share same base address but with
>> + * its own different sub block address.
>> + */
>> +static const struct dpu_dsc_cfg sm8550_dsc[] = {
>> +    DSC_BLK_1_2("dce_0", DSC_0, 0x80000, 0x100, 0, dsc_sblk_0),
>> +    DSC_BLK_1_2("dce_0", DSC_1, 0x80000, 0x100, 0, dsc_sblk_1),
>> +    DSC_BLK_1_2("dce_1", DSC_2, 0x81000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_0),
>> +    DSC_BLK_1_2("dce_1", DSC_3, 0x81000, 0x100, 
>> BIT(DPU_DSC_NATIVE_422_EN), dsc_sblk_1),
>> +};
>> +
>>   static const struct dpu_intf_cfg sm8550_intf[] = {
>>       INTF_BLK("intf_0", INTF_0, 0x34000, 0x280, INTF_DP, 
>> MSM_DP_CONTROLLER_0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>>       /* TODO TE sub-blocks for intf1 & intf2 */
>> @@ -218,6 +230,8 @@ const struct dpu_mdss_cfg dpu_sm8550_cfg = {
>>       .dspp = sm8550_dspp,
>>       .pingpong_count = ARRAY_SIZE(sm8550_pp),
>>       .pingpong = sm8550_pp,
>> +    .dsc = sm8550_dsc,
>> +    .dsc_count = ARRAY_SIZE(sm8550_dsc),
>>       .merge_3d_count = ARRAY_SIZE(sm8550_merge_3d),
>>       .merge_3d = sm8550_merge_3d,
>>       .intf_count = ARRAY_SIZE(sm8550_intf),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> index 6ea1e9d..83c0cd9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -1,6 +1,6 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>>   /* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> - * Copyright (c) 2022. Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + * Copyright (c) 2022-2023, Qualcomm Innovation Center, Inc. All 
>> rights reserved.
>>    */
>>   #define pr_fmt(fmt)    "[drm:%s:%d] " fmt, __func__, __LINE__
>> @@ -536,11 +536,34 @@ static const struct dpu_pingpong_sub_blks 
>> sc7280_pp_sblk = {
>>   /*************************************************************
>>    * DSC sub blocks config
>>    *************************************************************/
>> +static const struct dpu_dsc_sub_blks dsc_sblk_0 = {
>> +    .enc = {.base = 0x100, .len = 0x100},
>> +    .ctl = {.base = 0xF00, .len = 0x10},
>> +};
>> +
>> +static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
>> +    .enc = {.base = 0x200, .len = 0x100},
>> +    .ctl = {.base = 0xF80, .len = 0x10},
>> +};
>> +
>>   #define DSC_BLK(_name, _id, _base, _features) \
>>       {\
>>       .name = _name, .id = _id, \
>>       .base = _base, .len = 0x140, \
>> -    .features = _features, \
>> +    .features = BIT(DPU_DSC_HW_REV_1_1) | _features, \
>> +    }
>> +
>> +/*
>> + * NOTE: Each display compression engine (DCE) contains dual hard
>> + * slice DSC encoders so both share same base address but with
>> + * its own different sub block address.
>> + */
>> +#define DSC_BLK_1_2(_name, _id, _base, _len, _features, _sblk) \
>> +    {\
>> +    .name = _name, .id = _id, \
>> +    .base = _base, .len = _len, \
>> +    .features = BIT(DPU_DSC_HW_REV_1_2) | _features, \
>> +    .sblk = &_sblk, \
>>       }
>>   /*************************************************************
>
Dmitry Baryshkov May 3, 2023, 6:55 p.m. UTC | #8
On 03/05/2023 20:45, Kuogee Hsieh wrote:
> 
> On 5/2/2023 3:42 PM, Dmitry Baryshkov wrote:
>> On 03/05/2023 00:02, Kuogee Hsieh wrote:
>>> At legacy chipsets, it required DPU_PINGPONG_DSC bit be set to indicate
>>> pingpong ops functions are required to complete DSC data path setup if
>>> this chipset has DSC hardware block presented. This patch add
>>> DPU_PINGPONG_DSC bit to both PP_BLK and PP_BLK_TE marcos if it has DSC
>>> hardware block presented.
>>>
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> ---
>>>   .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h    | 12 +++++-----
>>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h |  8 +++----
>>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 26 
>>> ++++++++++------------
>>>   .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h    | 24 
>>> ++++++++++----------
>>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 26 
>>> ++++++++++------------
>>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h |  4 ++--
>>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h |  2 +-
>>>   .../drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h    |  2 +-
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |  8 +++----
>>>   9 files changed, 54 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>>> index 17f821c..b7cd746 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>>> @@ -112,16 +112,16 @@ static const struct dpu_lm_cfg msm8998_lm[] = {
>>>   };
>>>     static const struct dpu_pingpong_cfg msm8998_pp[] = {
>>> -    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 0, sdm845_pp_sblk_te,
>>> -            DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>>> +    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 
>>> BIT(DPU_PINGPONG_DSC), 0,
>>> +            sdm845_pp_sblk_te, DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12)),
>>> -    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 0, sdm845_pp_sblk_te,
>>> -            DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>>> +    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 
>>> BIT(DPU_PINGPONG_DSC), 0,
>>> +            sdm845_pp_sblk_te, DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13)),
>>> -    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, sdm845_pp_sblk,
>>> +    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, 0, sdm845_pp_sblk,
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14)),
>>> -    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, sdm845_pp_sblk,
>>> +    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, 0, sdm845_pp_sblk,
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15)),
>>
>> Just to doublecheck: why don't we have DPU_PINGPONG_DSC for PP_3/_4? 
>> We do have them on sdm845. Is it because we should not use DSC with 
>> thos PINGPONG blocks?
>>
> I think it only have two DSPP connect to pp blocks

So, can they be connected to PP3/4 or not?

>>>   };
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>>> index ceca741..8888bd9 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>>> @@ -110,16 +110,16 @@ static const struct dpu_lm_cfg sdm845_lm[] = {
>>>   };
>>>     static const struct dpu_pingpong_cfg sdm845_pp[] = {
>>> -    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 0, sdm845_pp_sblk_te,
>>> +    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 
>>> BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk_te,
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12)),
>>> -    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 0, sdm845_pp_sblk_te,
>>> +    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 
>>> BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk_te,
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13)),
>>> -    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, sdm845_pp_sblk,
>>> +    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, BIT(DPU_PINGPONG_DSC), 
>>> 0, sdm845_pp_sblk,
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14)),
>>> -    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, sdm845_pp_sblk,
>>> +    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, BIT(DPU_PINGPONG_DSC), 
>>> 0, sdm845_pp_sblk,
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15)),
>>>
>>
>> [skipped the rest, looks good to me]
>>
Kuogee Hsieh May 3, 2023, 7:58 p.m. UTC | #9
On 5/3/2023 11:55 AM, Dmitry Baryshkov wrote:
> On 03/05/2023 20:45, Kuogee Hsieh wrote:
>>
>> On 5/2/2023 3:42 PM, Dmitry Baryshkov wrote:
>>> On 03/05/2023 00:02, Kuogee Hsieh wrote:
>>>> At legacy chipsets, it required DPU_PINGPONG_DSC bit be set to 
>>>> indicate
>>>> pingpong ops functions are required to complete DSC data path setup if
>>>> this chipset has DSC hardware block presented. This patch add
>>>> DPU_PINGPONG_DSC bit to both PP_BLK and PP_BLK_TE marcos if it has DSC
>>>> hardware block presented.
>>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> ---
>>>>   .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h    | 12 +++++-----
>>>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h |  8 +++----
>>>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 26 
>>>> ++++++++++------------
>>>>   .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h    | 24 
>>>> ++++++++++----------
>>>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 26 
>>>> ++++++++++------------
>>>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h |  4 ++--
>>>>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h |  2 +-
>>>>   .../drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h    |  2 +-
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |  8 +++----
>>>>   9 files changed, 54 insertions(+), 58 deletions(-)
>>>>
>>>> diff --git 
>>>> a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>>>> index 17f821c..b7cd746 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>>>> @@ -112,16 +112,16 @@ static const struct dpu_lm_cfg msm8998_lm[] = {
>>>>   };
>>>>     static const struct dpu_pingpong_cfg msm8998_pp[] = {
>>>> -    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 0, 
>>>> sdm845_pp_sblk_te,
>>>> -            DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>>>> +    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 
>>>> BIT(DPU_PINGPONG_DSC), 0,
>>>> +            sdm845_pp_sblk_te, DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12)),
>>>> -    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 0, 
>>>> sdm845_pp_sblk_te,
>>>> -            DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>>>> +    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 
>>>> BIT(DPU_PINGPONG_DSC), 0,
>>>> +            sdm845_pp_sblk_te, DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13)),
>>>> -    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, sdm845_pp_sblk,
>>>> +    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, 0, sdm845_pp_sblk,
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14)),
>>>> -    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, sdm845_pp_sblk,
>>>> +    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, 0, sdm845_pp_sblk,
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15)),
>>>
>>> Just to doublecheck: why don't we have DPU_PINGPONG_DSC for PP_3/_4? 
>>> We do have them on sdm845. Is it because we should not use DSC with 
>>> thos PINGPONG blocks?
>>>
>> I think it only have two DSPP connect to pp blocks
>
> So, can they be connected to PP3/4 or not?

no, my previous reply is not correct.

original i though pp_3/_4 are for write back.

but this not correct, 2 dspp can connect to pp_3/_4 also.

I will add DPU_PINGPONG_DSC to pp_3/_4.


>
>>>>   };
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>>>> index ceca741..8888bd9 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>>>> @@ -110,16 +110,16 @@ static const struct dpu_lm_cfg sdm845_lm[] = {
>>>>   };
>>>>     static const struct dpu_pingpong_cfg sdm845_pp[] = {
>>>> -    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 0, 
>>>> sdm845_pp_sblk_te,
>>>> +    PP_BLK_TE("pingpong_0", PINGPONG_0, 0x70000, 
>>>> BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk_te,
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12)),
>>>> -    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 0, 
>>>> sdm845_pp_sblk_te,
>>>> +    PP_BLK_TE("pingpong_1", PINGPONG_1, 0x70800, 
>>>> BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk_te,
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13)),
>>>> -    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 0, sdm845_pp_sblk,
>>>> +    PP_BLK("pingpong_2", PINGPONG_2, 0x71000, 
>>>> BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk,
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14)),
>>>> -    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 0, sdm845_pp_sblk,
>>>> +    PP_BLK("pingpong_3", PINGPONG_3, 0x71800, 
>>>> BIT(DPU_PINGPONG_DSC), 0, sdm845_pp_sblk,
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
>>>>               DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15)),
>>>>
>>>
>>> [skipped the rest, looks good to me]
>>>
>