[v1,10/14] drm/msm/disp/dpu: add supports of DSC encoder v1.2 engine

Commit Message

Kuogee Hsieh Jan. 23, 2023, 6:24 p.m. UTC

DSC V1.2 encoder engine is newly added hardware module. This patch
add support functions to configure and enable DSC V1.2 encoder engine.

Signed-off-by: Kuogee Hsieh <[e-mail]>
---
 drivers/gpu/drm/msm/Makefile                   |   1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c    |   2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  60 +++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c     |  23 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h     |  23 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 371 +++++++++++++++++++++++++
 6 files changed, 463 insertions(+), 17 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c

Comments

Marijn Suijten Jan. 23, 2023, 8:11 p.m. UTC | #1

add support for*

drm/msm/dpu*

On 2023-01-23 10:24:30, Kuogee Hsieh wrote:
> DSC V1.2 encoder engine is newly added hardware module. This patch
> add support functions to configure and enable DSC V1.2 encoder engine.
> 
> Signed-off-by: Kuogee Hsieh <[e-mail]>
> ---
>  drivers/gpu/drm/msm/Makefile                   |   1 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c    |   2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  60 +++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c     |  23 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h     |  23 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 371 +++++++++++++++++++++++++
>  6 files changed, 463 insertions(+), 17 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 28cf52b..271c29a15 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_dsc_helper.o \
>  	disp/dpu1/dpu_hw_interrupts.o \
>  	disp/dpu1/dpu_hw_intf.o \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 7f4a439..901e317 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1821,7 +1821,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
>  				     u32 initial_lines)
>  {
>  	if (hw_dsc->ops.dsc_config)
> -		hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines);
> +		hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines, false);

As usual, an enum is better: readers have no idea what a free-floating
bool means.

>  
>  	if (hw_dsc->ops.dsc_config_thresh)
>  		hw_dsc->ops.dsc_config_thresh(hw_dsc, dsc);
> 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 978e3bd..7b0b092 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.
>   */
>  
> @@ -11,6 +11,7 @@
>  #include <linux/bug.h>
>  #include <linux/bitmap.h>
>  #include <linux/err.h>
> +#include "dpu_hw_mdss.h"

Unused if you remove the unused DECLARE_BITMAP(dsc_pair_mask, DSC_MAX).

>  
>  /**
>   * Max hardware block count: For ex: max 12 SSPP pipes or
> @@ -182,6 +183,7 @@ enum {
>   * @DPU_PINGPONG_TE2        Additional tear check block for split pipes
>   * @DPU_PINGPONG_SPLIT      PP block supports split fifo
>   * @DPU_PINGPONG_SLAVE      PP block is a suitable slave for split fifo
> + * @DPU_PINGPONG_DSC,       Display stream compression blocks
>   * @DPU_PINGPONG_DITHER,    Dither blocks
>   * @DPU_PINGPONG_MAX
>   */
> @@ -190,10 +192,32 @@ enum {
>  	DPU_PINGPONG_TE2,
>  	DPU_PINGPONG_SPLIT,
>  	DPU_PINGPONG_SLAVE,
> +	DPU_PINGPONG_DSC,

This is not used.

>  	DPU_PINGPONG_DITHER,
>  	DPU_PINGPONG_MAX
>  };
>  
> +
> +/** DSC sub-blocks/features

Newline between /** and the text.

> + * @DPU_DSC_OUTPUT_CTRL         Supports the control of the pp id which gets
> + *                              the pixel output from this DSC.

The original comment is much more concise, can we keep it?

> + * @DPU_DSC_HW_REV_1_1          dsc block supports dsc 1.1 only
> + * @DPU_DSC_HW_REV_1_2          dsc block supports dsc 1.1 and 1.2

Capitalize DSC just like elsewhere.

> + * @DPU_DSC_NATIVE_422_EN,      Supports native422 and native420 encoding
> + * @DPU_DSC_ENC,                DSC encoder sub block
> + * @DPU_DSC_CTL,                DSC ctl sub block

No need for trailing commas in doc comments; if anything replace them
with colons?

> + * @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_ENC,
> +	DPU_DSC_CTL,

These two enum values only have a meaning within the dpu_hw_dsc_1_2.c
file, and have nothing to do with the other feature flags/block
description.  Please move them there (and give _dsc_subblk_offset a
proper enum type).

> +	DPU_DSC_MAX
> +};
> +
>  /**
>   * CTL sub-blocks
>   * @DPU_CTL_SPLIT_DISPLAY:	CTL supports video mode split display
> @@ -276,15 +300,6 @@ enum {
>  };
>  
>  /**
> - * DSC features
> - * @DPU_DSC_OUTPUT_CTRL       Configure which PINGPONG block gets
> - *                            the pixel output from this DSC.
> - */
> -enum {
> -	DPU_DSC_OUTPUT_CTRL = 0x1,

Did this have to move?

> -};
> -
> -/**
>   * MACRO DPU_HW_BLK_INFO - information of HW blocks inside DPU
>   * @name:              string name for debug purposes
>   * @id:                enum identifying this block
> @@ -346,6 +361,14 @@ struct dpu_pp_blk {
>  };
>  
>  /**
> + * struct dpu_dsc_blk : DSC Encoder sub-blk information

Use a hyphen here and everywhere else:
https://docs.kernel.org/doc-guide/kernel-doc.html

> + * @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 {
> @@ -403,6 +426,7 @@ struct dpu_rotation_cfg {
>   * @pixel_ram_size     size of latency hiding and de-tiling buffer in bytes
>   * @max_hdeci_exp      max horizontal decimation supported (max is 2^value)
>   * @max_vdeci_exp      max vertical decimation supported (max is 2^value)
> + * @max_dsc_width      max dsc line width support.

DSC*

>   */
>  struct dpu_caps {
>  	u32 max_mixer_width;
> @@ -419,6 +443,7 @@ struct dpu_caps {
>  	u32 pixel_ram_size;
>  	u32 max_hdeci_exp;
>  	u32 max_vdeci_exp;
> +	u32 max_dsc_width;

This is never read.

>  };
>  
>  /**
> @@ -494,9 +519,20 @@ struct dpu_dspp_sub_blks {
>  struct dpu_pingpong_sub_blks {
>  	struct dpu_pp_blk te;
>  	struct dpu_pp_blk te2;
> +	struct dpu_pp_blk dsc;

Unused.

>  	struct dpu_pp_blk dither;
>  };
>  
> +
> +/**
> + * struct dpu_dsc_sub_blks : DSC sub-blks
> + *

A sub-block of sub-blocks?  Use the documentation to explain what this
is for, describe @enc and @ctl.

> + */
> +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
>   */
> @@ -641,10 +677,14 @@ 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
> + * @dsc_pair_mask:     Bitmask of DSCs that can be controlled by same CTL
>   */
>  struct dpu_dsc_cfg {
>  	DPU_HW_BLK_INFO;
> +	DECLARE_BITMAP(dsc_pair_mask, DSC_MAX);

This bitmask is unused.

> +	const struct dpu_dsc_sub_blks *sblk;
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> index 619926d..51e8890 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
>   * Copyright (c) 2020-2022, Linaro Limited
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
>   */
>  
>  #include "dpu_kms.h"
> @@ -41,10 +42,11 @@ static void dpu_hw_dsc_disable(struct dpu_hw_dsc *dsc)
>  static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>  			      struct drm_dsc_config *dsc,
>  			      u32 mode,
> -			      u32 initial_lines)
> +			      u32 initial_lines,
> +			      bool ich_reset_override)
>  {
>  	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
> -	u32 data;
> +	u32 data, lsb, bpp;
>  	u32 slice_last_group_size;
>  	u32 det_thresh_flatness;
>  	bool is_cmd_mode = !(mode & DSC_MODE_VIDEO);
> @@ -58,7 +60,14 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>  	data = (initial_lines << 20);
>  	data |= ((slice_last_group_size - 1) << 18);
>  	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
> -	data |= (dsc->bits_per_pixel << 8);
> +	data |= dsc->bits_per_pixel << 12;
> +	lsb = dsc->bits_per_pixel % 4;
> +	bpp = dsc->bits_per_pixel / 4;
> +	bpp *= 4;
> +	bpp <<= 4;
> +	bpp |= lsb;
> +
> +	data |= bpp << 8;

Why are you re-adding this nonsense?  It was removed in [1] _and_ does
not account for bits_per_pixel _already being in x.4 format_. This will
regress existing hardware.

[1]: https://lore.kernel.org/linux-arm-msm/[e-mail]/

>  	data |= (dsc->block_pred_enable << 7);
>  	data |= (dsc->line_buf_depth << 3);
>  	data |= (dsc->simple_422 << 2);
> @@ -221,7 +230,13 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
>  
>  	c->idx = idx;
>  	c->caps = cfg;
> -	_setup_dsc_ops(&c->ops, c->caps->features);
> +
> +	if (test_bit(DPU_DSC_HW_REV_1_1, &c->caps->features))
> +		_setup_dsc_ops(&c->ops, c->caps->features);
> +	else if (test_bit(DPU_DSC_HW_REV_1_2, &c->caps->features))
> +		dpu_dsc_1_2_setup_ops(&c->ops, c->caps->features);
> +	else
> +		_setup_dsc_ops(&c->ops, c->caps->features);
>  
>  	return c;
>  }
> 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 ae9b5db..a48f572 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
> @@ -33,7 +36,8 @@ struct dpu_hw_dsc_ops {
>  	void (*dsc_config)(struct dpu_hw_dsc *hw_dsc,
>  			   struct drm_dsc_config *dsc,
>  			   u32 mode,
> -			   u32 initial_lines);
> +			   u32 initial_lines,
> +			   bool ich_reset_override);
>  
>  	/**
>  	 * dsc_config_thresh - programs panel thresholds
> @@ -43,6 +47,12 @@ struct dpu_hw_dsc_ops {
>  	void (*dsc_config_thresh)(struct dpu_hw_dsc *hw_dsc,
>  				  struct drm_dsc_config *dsc);
>  
> +	/**
> +	 * bind_pingpong_blk - enable/disable the connection with pp

Inherit docs from the enum.

> +	 * @hw_dsc: Pointer to dsc context

DSC*

> +	 * @enable: enable/disable connection
> +	 * @pp: pingpong blk id

It's documentation, write out block fully.

> +	 */
>  	void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
>  				  bool enable,
>  				  enum dpu_pingpong pp);
> @@ -51,6 +61,7 @@ struct dpu_hw_dsc_ops {
>  struct dpu_hw_dsc {
>  	struct dpu_hw_blk base;
>  	struct dpu_hw_blk_reg_map hw;
> +	struct dpu_hw_ctl *hw_ctl;

Unused.

>  
>  	/* dsc */
>  	enum dpu_dsc idx;
> @@ -76,9 +87,17 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
>   */
>  void dpu_hw_dsc_destroy(struct dpu_hw_dsc *dsc);
>  
> +/**
> + * dpu_hw_dsc - convert base object dpu_hw_base to container
> + * @hw: Pointer to base hardware block
> + * return: Pointer to hardware block container
> + */
>  static inline struct dpu_hw_dsc *to_dpu_hw_dsc(struct dpu_hw_blk *hw)
>  {
>  	return container_of(hw, struct dpu_hw_dsc, base);
>  }
>  
> +void dpu_dsc_1_2_setup_ops(struct dpu_hw_dsc_ops *ops,
> +		const unsigned long features);
> +
>  #endif /* _DPU_HW_DSC_H */
> 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..2be74ae
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
> @@ -0,0 +1,371 @@
> +// 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 "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_BUF_THRESH_1        0x6C
> +#define DSC_RC_BUF_THRESH_2        0x70
> +#define DSC_RC_BUF_THRESH_3        0x74
> +#define DSC_RC_MIN_QP_0            0x78
> +#define DSC_RC_MIN_QP_1            0x7C
> +#define DSC_RC_MIN_QP_2            0x80
> +#define DSC_RC_MAX_QP_0            0x84
> +#define DSC_RC_MAX_QP_1            0x88
> +#define DSC_RC_MAX_QP_2             0x8C
> +#define DSC_RC_RANGE_BPG_OFFSETS_0  0x90
> +#define DSC_RC_RANGE_BPG_OFFSETS_1  0x94
> +#define DSC_RC_RANGE_BPG_OFFSETS_2  0x98

Reindent to line this back up.

> +
> +/* 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 int _dsc_calc_ob_max_addr(struct dpu_hw_dsc *hw_dsc, int num_ss)
> +{
> +	enum dpu_dsc idx;
> +
> +	idx = hw_dsc->idx;
> +
> +	if (!(hw_dsc->caps->features & BIT(DPU_DSC_NATIVE_422_EN))) {

Why not swap the bodies instead of inverting this.

> +		if (num_ss == 1)
> +			return 2399;
> +		else if (num_ss == 2)
> +			return 1199;
> +	} else {
> +		if (num_ss == 1)
> +			return 1199;
> +		else if (num_ss == 2)
> +			return 599;
> +	}
> +	return 0;
> +}
> +
> +static inline int _dsc_subblk_offset(struct dpu_hw_dsc *hw_dsc, int s_id,
> +		u32 *idx)
> +{
> +	const struct dpu_dsc_sub_blks *sblk;
> +
> +	if (!hw_dsc)
> +		return -EINVAL;
> +
> +	*idx = 0;
> +
> +	sblk = hw_dsc->caps->sblk;
> +
> +	switch (s_id) {
> +
> +	case DPU_DSC_ENC:
> +		*idx = sblk->enc.base;
> +		break;
> +	case DPU_DSC_CTL:
> +		*idx = sblk->ctl.base;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void dpu_hw_dsc_disable_1_2(struct dpu_hw_dsc *hw_dsc)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	u32 idx;

Can we rename these to offset or subblk_offset or something more clear?

> +
> +	if (!hw_dsc)
> +		return;
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
> +		return;

These error checks are excessive: you pass in a non-null hw_dsc and
known enum constant - _dsc_subblk_offset should perhaps not return
errors at all.

> +
> +	hw = &hw_dsc->hw;
> +	DPU_REG_WRITE(hw, DSC_CFG + idx, 0);

Swap the arguments to + so that it's clear that DSC_CFG is a register on
the subblock offset denoted by "idx", not the other way around.

> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
> +		return;
> +
> +	DPU_REG_WRITE(hw, ENC_DF_CTRL + idx, 0);
> +	DPU_REG_WRITE(hw, DSC_MAIN_CONF + idx, 0);
> +}
> +
> +static void dpu_hw_dsc_config_1_2(struct dpu_hw_dsc *hw_dsc,
> +		struct drm_dsc_config *dsc, u32 mode,
> +		u32 initial_lines, bool ich_reset_override)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	struct msm_display_dsc_info *dsc_info;
> +	u32 idx;
> +	u32 data = 0;
> +	u32 bpp;
> +	void __iomem *off;
> +
> +	if (!hw_dsc || !dsc)
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +
> +	dsc_info = to_msm_dsc_info(dsc);
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
> +		return;
> +
> +	if (mode & DSC_MODE_SPLIT_PANEL)
> +		data |= BIT(0);
> +
> +	if (mode & DSC_MODE_MULTIPLEX)
> +		data |= BIT(1);

These are well known bitwise definitions for a reason, data |= mode will
do (or out DSC_MODE_VIDEO since you have to shift that one at BIT(9).

> +
> +	data |= (dsc_info->num_active_ss_per_enc & 0x3) << 7;
> +
> +	DPU_REG_WRITE(hw, DSC_CMN_MAIN_CNF, data);
> +
> +	data = (dsc_info->initial_lines & 0xff);

You already get initial_lines passed as function argument, but ignore
it?

> +	data |= ((mode & DSC_MODE_VIDEO) ? 1 : 0) << 9;

Yuck. if (mode & DSC_MODE_VIDEO) data |= BIT(9);.

> +	if (ich_reset_override)
> +		data |= 0xC00; // set bit 10 and 11

Instead of a comment, make this self-describing BIT(10) | BIT(11) code.

> +	data |= (_dsc_calc_ob_max_addr(hw_dsc, dsc_info->num_active_ss_per_enc) << 18);
> +
> +	DPU_REG_WRITE(hw, ENC_DF_CTRL + idx, 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 above, don't forget to read the documentation on this field:

    Target bits per pixel with 4 fractional bits, bits_per_pixel << 4

> +	/* 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);

Either wrap everything or nothing in ().

> +	data |= (dsc->line_buf_depth & 0xf) << 6;
> +	data |= dsc->convert_rgb << 4;
> +	data |= dsc->bits_per_component & 0xf;
> +
> +	DPU_REG_WRITE(hw, DSC_MAIN_CONF + idx, data);
> +
> +	data = (dsc->pic_width & 0xffff) |
> +		((dsc->pic_height & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_PICTURE_SIZE + idx, data);
> +
> +	data = (dsc->slice_width & 0xffff) |
> +		((dsc->slice_height & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_SLICE_SIZE + idx, data);
> +
> +	DPU_REG_WRITE(hw, DSC_MISC_SIZE + idx,
> +			(dsc->slice_chunk_size) & 0xffff);
> +
> +	data = (dsc->initial_xmit_delay & 0xffff) |
> +		((dsc->initial_dec_delay & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_HRD_DELAYS + idx, data);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_SCALE + idx,
> +			dsc->initial_scale_value & 0x3f);
> +
> +	data = (dsc->scale_increment_interval & 0xffff) |
> +		((dsc->scale_decrement_interval & 0x7ff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_SCALE_INC_DEC + idx, data);
> +
> +	data = (dsc->first_line_bpg_offset & 0x1f) |
> +		((dsc->second_line_bpg_offset & 0x1f) << 5);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_1 + idx, data);
> +
> +	data = (dsc->nfl_bpg_offset & 0xffff) |
> +		((dsc->slice_bpg_offset & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_2 + idx, data);
> +
> +	data = (dsc->initial_offset & 0xffff) |
> +		((dsc->final_offset & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_3 + idx, data);
> +
> +	data = (dsc->nsl_bpg_offset & 0xffff) |
> +		((dsc->second_line_offset_adj & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_4 + idx, data);
> +
> +	data = (dsc->flatness_min_qp & 0x1f);
> +	data |= (dsc->flatness_max_qp & 0x1f) << 5;
> +	data |= (dsc_info->det_thresh_flatness & 0xff) << 10;

dpu_hw_dsc.c computes this on the fly.  After removing that, and
using initial_lines from the function parameters, only
dsc_info->num_active_ss_per_enc remains.  Do you really need that
msm_display_dsc_info struct here, do you need it at all?

> +
> +	DPU_REG_WRITE(hw, DSC_FLATNESS_QP + idx, data);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_MODEL_SIZE + idx,
> +			(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, DSC_RC_CONFIG + idx, data);
> +
> +	/* program the dsc wrapper */
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
> +		return;
> +
> +	off = hw->blk_addr + idx;
> +
> +	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);
> +
> +	if (dsc_info->dsc_4hsmerge_en) {
> +		data |= dsc_info->dsc_4hsmerge_padding << 18;
> +		data |= dsc_info->dsc_4hsmerge_alignment << 22;
> +		data |= BIT(16);
> +	}
> +
> +	DPU_REG_WRITE(hw, DSC_CFG + idx, data);
> +
> +//	DPU_REG_WRITE(hw, DSC_DATA_IN_SWAP + idx, 0x14e5);

No commented-out code please, especially not with //

> +}
> +
> +static 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;
> +	struct msm_display_dsc_info *dsc_info;
> +	u32 idx, off;
> +	int i, j = 0;
> +	struct drm_dsc_rc_range_parameters *rc;
> +	u32 data = 0, min_qp = 0, max_qp = 0, bpg_off = 0;
> +
> +	if (!hw_dsc || !dsc)
> +		return;
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +
> +	dsc_info = to_msm_dsc_info(dsc);
> +
> +	rc = dsc->rc_range_params;
> +
> +	off = 0;
> +	for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
> +		data |= dsc->rc_buf_thresh[i] << (8*j);

Lack of spaces does not make this multiplication any prettier to read.

* has precedence over << but it's better to replicate the () below as
well.

> +		j++;
> +		if ((j == 4) || (i == DSC_NUM_BUF_RANGES - 2)) {
> +			DPU_REG_WRITE(hw, DSC_RC_BUF_THRESH_0 + idx + off,
> +					data);
> +			off += 4;
> +			j = 0;
> +			data = 0;
> +		}
> +	}
> +
> +	off = 0;
> +	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> +		min_qp |= (rc[i].range_min_qp & 0x1f) << 5*j;
> +		max_qp |= (rc[i].range_max_qp & 0x1f) << 5*j;
> +		bpg_off |= (rc[i].range_bpg_offset & 0x3f) << 6*j;

These values _must_ already be masked to be useful in
drm_dsc_compute_rc_parameters(), no need to mask them again just like
the v1.1 block implementation.

> +		j++;
> +		if (j == 5) {
> +			DPU_REG_WRITE(hw, DSC_RC_MIN_QP_0 + idx + off,
> +					min_qp);
> +			DPU_REG_WRITE(hw, DSC_RC_MAX_QP_0 + idx + off,
> +					max_qp);
> +			DPU_REG_WRITE(hw,
> +					DSC_RC_RANGE_BPG_OFFSETS_0 + idx + off,
> +					bpg_off);
> +			off += 4;
> +			j = 0;
> +			min_qp = 0;
> +			max_qp = 0;
> +			bpg_off = 0;
> +		}
> +	}
> +}
> +
> +static void dpu_hw_dsc_bind_pingpong_blk_1_2(
> +		struct dpu_hw_dsc *hw_dsc,
> +		bool enable,
> +		const enum dpu_pingpong pp)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	int idx;
> +	int mux_cfg = 0xF; /* Disabled */

Lowercase hex (and anywhere else if I skipped any).

> +
> +	if (!hw_dsc)
> +		return;

As with the v1.1 implementation, we don't check this, and your function
below also checks it (but it does not need to).

> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +	if (enable)
> +		mux_cfg = (pp - PINGPONG_0) & 0x7;
> +
> +	DPU_REG_WRITE(hw, DSC_CTL + idx, mux_cfg);
> +}
> +
> +void dpu_dsc_1_2_setup_ops(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;
> +}
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

All in all you really need to revise and clean your patches before
sending them to the lists; these are already far too many comments and
nits, and massively take away from reviewing code behaviour which I have
not even started with after looking at 1 out of 14 patches :(

- Marijn

Dmitry Baryshkov Jan. 24, 2023, 7:44 a.m. UTC | #2

On 23/01/2023 20:24, Kuogee Hsieh wrote:
> DSC V1.2 encoder engine is newly added hardware module. This patch
> add support functions to configure and enable DSC V1.2 encoder engine.
> 
> Signed-off-by: Kuogee Hsieh <[e-mail]>
> ---
>   drivers/gpu/drm/msm/Makefile                   |   1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c    |   2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  60 +++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c     |  23 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h     |  23 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 371 +++++++++++++++++++++++++
>   6 files changed, 463 insertions(+), 17 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 28cf52b..271c29a15 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_dsc_helper.o \
>   	disp/dpu1/dpu_hw_interrupts.o \
>   	disp/dpu1/dpu_hw_intf.o \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 7f4a439..901e317 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1821,7 +1821,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
>   				     u32 initial_lines)
>   {
>   	if (hw_dsc->ops.dsc_config)
> -		hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines);
> +		hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines, false);
>   
>   	if (hw_dsc->ops.dsc_config_thresh)
>   		hw_dsc->ops.dsc_config_thresh(hw_dsc, dsc);
> 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 978e3bd..7b0b092 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.
>    */
>   
> @@ -11,6 +11,7 @@
>   #include <linux/bug.h>
>   #include <linux/bitmap.h>
>   #include <linux/err.h>
> +#include "dpu_hw_mdss.h"
>   
>   /**
>    * Max hardware block count: For ex: max 12 SSPP pipes or
> @@ -182,6 +183,7 @@ enum {
>    * @DPU_PINGPONG_TE2        Additional tear check block for split pipes
>    * @DPU_PINGPONG_SPLIT      PP block supports split fifo
>    * @DPU_PINGPONG_SLAVE      PP block is a suitable slave for split fifo
> + * @DPU_PINGPONG_DSC,       Display stream compression blocks
>    * @DPU_PINGPONG_DITHER,    Dither blocks
>    * @DPU_PINGPONG_MAX
>    */
> @@ -190,10 +192,32 @@ enum {
>   	DPU_PINGPONG_TE2,
>   	DPU_PINGPONG_SPLIT,
>   	DPU_PINGPONG_SLAVE,
> +	DPU_PINGPONG_DSC,
>   	DPU_PINGPONG_DITHER,
>   	DPU_PINGPONG_MAX
>   };
>   
> +
> +/** DSC sub-blocks/features
> + * @DPU_DSC_OUTPUT_CTRL         Supports the control of the pp id which gets
> + *                              the pixel output from this DSC.
> + * @DPU_DSC_HW_REV_1_1          dsc block supports dsc 1.1 only
> + * @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_ENC,                DSC encoder sub block
> + * @DPU_DSC_CTL,                DSC ctl sub block
> + * @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_ENC,
> +	DPU_DSC_CTL,
> +	DPU_DSC_MAX
> +};
> +
>   /**
>    * CTL sub-blocks
>    * @DPU_CTL_SPLIT_DISPLAY:	CTL supports video mode split display
> @@ -276,15 +300,6 @@ enum {
>   };
>   
>   /**
> - * DSC features
> - * @DPU_DSC_OUTPUT_CTRL       Configure which PINGPONG block gets
> - *                            the pixel output from this DSC.
> - */
> -enum {
> -	DPU_DSC_OUTPUT_CTRL = 0x1,
> -};
> -

Any reason for this move?

> -/**
>    * MACRO DPU_HW_BLK_INFO - information of HW blocks inside DPU
>    * @name:              string name for debug purposes
>    * @id:                enum identifying this block
> @@ -346,6 +361,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 {
> @@ -403,6 +426,7 @@ struct dpu_rotation_cfg {
>    * @pixel_ram_size     size of latency hiding and de-tiling buffer in bytes
>    * @max_hdeci_exp      max horizontal decimation supported (max is 2^value)
>    * @max_vdeci_exp      max vertical decimation supported (max is 2^value)
> + * @max_dsc_width      max dsc line width support.
>    */
>   struct dpu_caps {
>   	u32 max_mixer_width;
> @@ -419,6 +443,7 @@ struct dpu_caps {
>   	u32 pixel_ram_size;
>   	u32 max_hdeci_exp;
>   	u32 max_vdeci_exp;
> +	u32 max_dsc_width;
>   };
>   
>   /**
> @@ -494,9 +519,20 @@ struct dpu_dspp_sub_blks {
>   struct dpu_pingpong_sub_blks {
>   	struct dpu_pp_blk te;
>   	struct dpu_pp_blk te2;
> +	struct dpu_pp_blk dsc;
>   	struct dpu_pp_blk dither;
>   };
>   
> +
> +/**
> + * struct dpu_dsc_sub_blks : DSC sub-blks
> + *
> + */
> +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
>    */
> @@ -641,10 +677,14 @@ 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
> + * @dsc_pair_mask:     Bitmask of DSCs that can be controlled by same CTL
>    */
>   struct dpu_dsc_cfg {
>   	DPU_HW_BLK_INFO;
> +	DECLARE_BITMAP(dsc_pair_mask, DSC_MAX);

Please change this from the bitmap to enum dpu_dsc instance

> +	const struct dpu_dsc_sub_blks *sblk;
>   };
>   
>   /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> index 619926d..51e8890 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   /*
>    * Copyright (c) 2020-2022, Linaro Limited
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
>    */
>   
>   #include "dpu_kms.h"
> @@ -41,10 +42,11 @@ static void dpu_hw_dsc_disable(struct dpu_hw_dsc *dsc)
>   static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>   			      struct drm_dsc_config *dsc,
>   			      u32 mode,
> -			      u32 initial_lines)
> +			      u32 initial_lines,
> +			      bool ich_reset_override)
>   {
>   	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
> -	u32 data;
> +	u32 data, lsb, bpp;
>   	u32 slice_last_group_size;
>   	u32 det_thresh_flatness;
>   	bool is_cmd_mode = !(mode & DSC_MODE_VIDEO);
> @@ -58,7 +60,14 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>   	data = (initial_lines << 20);
>   	data |= ((slice_last_group_size - 1) << 18);
>   	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
> -	data |= (dsc->bits_per_pixel << 8);
> +	data |= dsc->bits_per_pixel << 12;
> +	lsb = dsc->bits_per_pixel % 4;
> +	bpp = dsc->bits_per_pixel / 4;
> +	bpp *= 4;
> +	bpp <<= 4;
> +	bpp |= lsb;
> +

NAK. This was changed by the commit d3c1a8663d0d ("drm/msm/dpu1: Account 
for DSC's bits_per_pixel having 4 fractional bits"). It removed the code 
that you are trying to bring back.

> +	data |= bpp << 8;
>   	data |= (dsc->block_pred_enable << 7);
>   	data |= (dsc->line_buf_depth << 3);
>   	data |= (dsc->simple_422 << 2);
> @@ -221,7 +230,13 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
>   
>   	c->idx = idx;
>   	c->caps = cfg;
> -	_setup_dsc_ops(&c->ops, c->caps->features);
> +
> +	if (test_bit(DPU_DSC_HW_REV_1_1, &c->caps->features))
> +		_setup_dsc_ops(&c->ops, c->caps->features);
> +	else if (test_bit(DPU_DSC_HW_REV_1_2, &c->caps->features))
> +		dpu_dsc_1_2_setup_ops(&c->ops, c->caps->features);
> +	else
> +		_setup_dsc_ops(&c->ops, c->caps->features);

Can we handle this in a more sensible way, please. E.g. let RM check the 
flags and then call either dpu_hw_dsc_1_1_init() or dpu_hw_dsc_1_2_init()?

Granted that to generations of DSC blocks are _that_ different it might 
even make sense to handle them separately in the HW catalog too, but I 
wouldn't insist on that.

>   
>   	return c;
>   }
> 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 ae9b5db..a48f572 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
> @@ -33,7 +36,8 @@ struct dpu_hw_dsc_ops {
>   	void (*dsc_config)(struct dpu_hw_dsc *hw_dsc,
>   			   struct drm_dsc_config *dsc,
>   			   u32 mode,
> -			   u32 initial_lines);
> +			   u32 initial_lines,
> +			   bool ich_reset_override);

Documentation?

>   
>   	/**
>   	 * dsc_config_thresh - programs panel thresholds
> @@ -43,6 +47,12 @@ struct dpu_hw_dsc_ops {
>   	void (*dsc_config_thresh)(struct dpu_hw_dsc *hw_dsc,
>   				  struct drm_dsc_config *dsc);
>   
> +	/**
> +	 * bind_pingpong_blk - enable/disable the connection with pp
> +	 * @hw_dsc: Pointer to dsc context
> +	 * @enable: enable/disable connection
> +	 * @pp: pingpong blk id
> +	 */
>   	void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
>   				  bool enable,
>   				  enum dpu_pingpong pp);
> @@ -51,6 +61,7 @@ struct dpu_hw_dsc_ops {
>   struct dpu_hw_dsc {
>   	struct dpu_hw_blk base;
>   	struct dpu_hw_blk_reg_map hw;
> +	struct dpu_hw_ctl *hw_ctl;

Why? This is not used by the rest of the patch.

>   
>   	/* dsc */
>   	enum dpu_dsc idx;
> @@ -76,9 +87,17 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
>    */
>   void dpu_hw_dsc_destroy(struct dpu_hw_dsc *dsc);
>   
> +/**
> + * dpu_hw_dsc - convert base object dpu_hw_base to container
> + * @hw: Pointer to base hardware block
> + * return: Pointer to hardware block container
> + */

You are adding docs for the obvious items (which is unnecessary and just 
clobbers the code), but leaving out important items (for which docs 
might be necessary).

>   static inline struct dpu_hw_dsc *to_dpu_hw_dsc(struct dpu_hw_blk *hw)
>   {
>   	return container_of(hw, struct dpu_hw_dsc, base);
>   }
>   
> +void dpu_dsc_1_2_setup_ops(struct dpu_hw_dsc_ops *ops,
> +		const unsigned long features);
> +
>   #endif /* _DPU_HW_DSC_H */
> 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..2be74ae
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
> @@ -0,0 +1,371 @@
> +// 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 "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_BUF_THRESH_1        0x6C
> +#define DSC_RC_BUF_THRESH_2        0x70
> +#define DSC_RC_BUF_THRESH_3        0x74
> +#define DSC_RC_MIN_QP_0            0x78
> +#define DSC_RC_MIN_QP_1            0x7C
> +#define DSC_RC_MIN_QP_2            0x80
> +#define DSC_RC_MAX_QP_0            0x84
> +#define DSC_RC_MAX_QP_1            0x88
> +#define DSC_RC_MAX_QP_2             0x8C
> +#define DSC_RC_RANGE_BPG_OFFSETS_0  0x90
> +#define DSC_RC_RANGE_BPG_OFFSETS_1  0x94
> +#define DSC_RC_RANGE_BPG_OFFSETS_2  0x98
> +
> +/* 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 int _dsc_calc_ob_max_addr(struct dpu_hw_dsc *hw_dsc, int num_ss)
> +{
> +	enum dpu_dsc idx;
> +
> +	idx = hw_dsc->idx;
> +
> +	if (!(hw_dsc->caps->features & BIT(DPU_DSC_NATIVE_422_EN))) {
> +		if (num_ss == 1)
> +			return 2399;
> +		else if (num_ss == 2)
> +			return 1199;
> +	} else {
> +		if (num_ss == 1)
> +			return 1199;
> +		else if (num_ss == 2)
> +			return 599;
> +	}
> +	return 0;

int max_addr = 2400 / num_ss;
if (hw_dsc->caps->features & BIT(DPU_DSC_NATIVE_422_EN))
     max_addr /= 2;

return max_addr -1;

Isn't this nicer?

> +}
> +
> +static inline int _dsc_subblk_offset(struct dpu_hw_dsc *hw_dsc, int s_id,
> +		u32 *idx)
> +{
> +	const struct dpu_dsc_sub_blks *sblk;
> +
> +	if (!hw_dsc)
> +		return -EINVAL;
> +
> +	*idx = 0;
> +
> +	sblk = hw_dsc->caps->sblk;
> +
> +	switch (s_id) {
> +
> +	case DPU_DSC_ENC:
> +		*idx = sblk->enc.base;
> +		break;
> +	case DPU_DSC_CTL:
> +		*idx = sblk->ctl.base;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void dpu_hw_dsc_disable_1_2(struct dpu_hw_dsc *hw_dsc)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	u32 idx;
> +
> +	if (!hw_dsc)
> +		return;
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +	DPU_REG_WRITE(hw, DSC_CFG + idx, 0);
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
> +		return;
> +
> +	DPU_REG_WRITE(hw, ENC_DF_CTRL + idx, 0);
> +	DPU_REG_WRITE(hw, DSC_MAIN_CONF + idx, 0);
> +}
> +
> +static void dpu_hw_dsc_config_1_2(struct dpu_hw_dsc *hw_dsc,
> +		struct drm_dsc_config *dsc, u32 mode,
> +		u32 initial_lines, bool ich_reset_override)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	struct msm_display_dsc_info *dsc_info;
> +	u32 idx;
> +	u32 data = 0;
> +	u32 bpp;
> +	void __iomem *off;
> +
> +	if (!hw_dsc || !dsc)
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +
> +	dsc_info = to_msm_dsc_info(dsc);
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
> +		return;
> +
> +	if (mode & DSC_MODE_SPLIT_PANEL)
> +		data |= BIT(0);
> +
> +	if (mode & DSC_MODE_MULTIPLEX)
> +		data |= BIT(1);
> +
> +	data |= (dsc_info->num_active_ss_per_enc & 0x3) << 7;
> +
> +	DPU_REG_WRITE(hw, DSC_CMN_MAIN_CNF, data);
> +
> +	data = (dsc_info->initial_lines & 0xff);
> +	data |= ((mode & DSC_MODE_VIDEO) ? 1 : 0) << 9;
> +	if (ich_reset_override)
> +		data |= 0xC00; // set bit 10 and 11

Comment style is wrong. The comment is useless: from 0xc00 we see which 
bits are being set. We'd better know why. Please add corresponding 
defines to the bits and bitfields in these function.

> +	data |= (_dsc_calc_ob_max_addr(hw_dsc, dsc_info->num_active_ss_per_enc) << 18);
> +
> +	DPU_REG_WRITE(hw, ENC_DF_CTRL + idx, 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, DSC_MAIN_CONF + idx, data);
> +
> +	data = (dsc->pic_width & 0xffff) |
> +		((dsc->pic_height & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_PICTURE_SIZE + idx, data);
> +
> +	data = (dsc->slice_width & 0xffff) |
> +		((dsc->slice_height & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_SLICE_SIZE + idx, data);
> +
> +	DPU_REG_WRITE(hw, DSC_MISC_SIZE + idx,
> +			(dsc->slice_chunk_size) & 0xffff);
> +
> +	data = (dsc->initial_xmit_delay & 0xffff) |
> +		((dsc->initial_dec_delay & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_HRD_DELAYS + idx, data);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_SCALE + idx,
> +			dsc->initial_scale_value & 0x3f);
> +
> +	data = (dsc->scale_increment_interval & 0xffff) |
> +		((dsc->scale_decrement_interval & 0x7ff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_SCALE_INC_DEC + idx, data);
> +
> +	data = (dsc->first_line_bpg_offset & 0x1f) |
> +		((dsc->second_line_bpg_offset & 0x1f) << 5);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_1 + idx, data);
> +
> +	data = (dsc->nfl_bpg_offset & 0xffff) |
> +		((dsc->slice_bpg_offset & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_2 + idx, data);
> +
> +	data = (dsc->initial_offset & 0xffff) |
> +		((dsc->final_offset & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_3 + idx, data);
> +
> +	data = (dsc->nsl_bpg_offset & 0xffff) |
> +		((dsc->second_line_offset_adj & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_4 + idx, data);
> +
> +	data = (dsc->flatness_min_qp & 0x1f);
> +	data |= (dsc->flatness_max_qp & 0x1f) << 5;
> +	data |= (dsc_info->det_thresh_flatness & 0xff) << 10;
> +
> +	DPU_REG_WRITE(hw, DSC_FLATNESS_QP + idx, data);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_MODEL_SIZE + idx,
> +			(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, DSC_RC_CONFIG + idx, data);
> +
> +	/* program the dsc wrapper */
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
> +		return;
> +
> +	off = hw->blk_addr + idx;
> +
> +	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);
> +
> +	if (dsc_info->dsc_4hsmerge_en) {
> +		data |= dsc_info->dsc_4hsmerge_padding << 18;
> +		data |= dsc_info->dsc_4hsmerge_alignment << 22;
> +		data |= BIT(16);
> +	}
> +
> +	DPU_REG_WRITE(hw, DSC_CFG + idx, data);
> +
> +//	DPU_REG_WRITE(hw, DSC_DATA_IN_SWAP + idx, 0x14e5);

Is this necessary or not? If not, please drop it.

> +}
> +
> +static 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;
> +	struct msm_display_dsc_info *dsc_info;
> +	u32 idx, off;
> +	int i, j = 0;
> +	struct drm_dsc_rc_range_parameters *rc;
> +	u32 data = 0, min_qp = 0, max_qp = 0, bpg_off = 0;
> +
> +	if (!hw_dsc || !dsc)
> +		return;
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +
> +	dsc_info = to_msm_dsc_info(dsc);
> +
> +	rc = dsc->rc_range_params;
> +
> +	off = 0;
> +	for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
> +		data |= dsc->rc_buf_thresh[i] << (8*j);
> +		j++;
> +		if ((j == 4) || (i == DSC_NUM_BUF_RANGES - 2)) {
> +			DPU_REG_WRITE(hw, DSC_RC_BUF_THRESH_0 + idx + off,
> +					data);
> +			off += 4;
> +			j = 0;
> +			data = 0;
> +		}
> +	}
> +
> +	off = 0;
> +	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> +		min_qp |= (rc[i].range_min_qp & 0x1f) << 5*j;
> +		max_qp |= (rc[i].range_max_qp & 0x1f) << 5*j;
> +		bpg_off |= (rc[i].range_bpg_offset & 0x3f) << 6*j;
> +		j++;
> +		if (j == 5) {
> +			DPU_REG_WRITE(hw, DSC_RC_MIN_QP_0 + idx + off,
> +					min_qp);
> +			DPU_REG_WRITE(hw, DSC_RC_MAX_QP_0 + idx + off,
> +					max_qp);
> +			DPU_REG_WRITE(hw,
> +					DSC_RC_RANGE_BPG_OFFSETS_0 + idx + off,
> +					bpg_off);
> +			off += 4;
> +			j = 0;
> +			min_qp = 0;
> +			max_qp = 0;
> +			bpg_off = 0;
> +		}
> +	}
> +}
> +
> +static void dpu_hw_dsc_bind_pingpong_blk_1_2(
> +		struct dpu_hw_dsc *hw_dsc,
> +		bool enable,
> +		const enum dpu_pingpong pp)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	int idx;
> +	int mux_cfg = 0xF; /* Disabled */
> +
> +	if (!hw_dsc)
> +		return;
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +	if (enable)
> +		mux_cfg = (pp - PINGPONG_0) & 0x7;
> +
> +	DPU_REG_WRITE(hw, DSC_CTL + idx, mux_cfg);
> +}
> +
> +void dpu_dsc_1_2_setup_ops(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;
> +}

Neil Armstrong Jan. 24, 2023, 3:18 p.m. UTC | #3

On 23/01/2023 19:24, Kuogee Hsieh wrote:
> DSC V1.2 encoder engine is newly added hardware module. This patch
> add support functions to configure and enable DSC V1.2 encoder engine.
> 
> Signed-off-by: Kuogee Hsieh <[e-mail]>
> ---
>   drivers/gpu/drm/msm/Makefile                   |   1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c    |   2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  60 +++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c     |  23 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h     |  23 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 371 +++++++++++++++++++++++++
>   6 files changed, 463 insertions(+), 17 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 28cf52b..271c29a15 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_dsc_helper.o \
>   	disp/dpu1/dpu_hw_interrupts.o \
>   	disp/dpu1/dpu_hw_intf.o \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 7f4a439..901e317 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1821,7 +1821,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
>   				     u32 initial_lines)
>   {
>   	if (hw_dsc->ops.dsc_config)
> -		hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines);
> +		hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines, false);
>   
>   	if (hw_dsc->ops.dsc_config_thresh)
>   		hw_dsc->ops.dsc_config_thresh(hw_dsc, dsc);
> 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 978e3bd..7b0b092 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.
>    */
>   
> @@ -11,6 +11,7 @@
>   #include <linux/bug.h>
>   #include <linux/bitmap.h>
>   #include <linux/err.h>
> +#include "dpu_hw_mdss.h"
>   
>   /**
>    * Max hardware block count: For ex: max 12 SSPP pipes or
> @@ -182,6 +183,7 @@ enum {
>    * @DPU_PINGPONG_TE2        Additional tear check block for split pipes
>    * @DPU_PINGPONG_SPLIT      PP block supports split fifo
>    * @DPU_PINGPONG_SLAVE      PP block is a suitable slave for split fifo
> + * @DPU_PINGPONG_DSC,       Display stream compression blocks
>    * @DPU_PINGPONG_DITHER,    Dither blocks
>    * @DPU_PINGPONG_MAX
>    */
> @@ -190,10 +192,32 @@ enum {
>   	DPU_PINGPONG_TE2,
>   	DPU_PINGPONG_SPLIT,
>   	DPU_PINGPONG_SLAVE,
> +	DPU_PINGPONG_DSC,
>   	DPU_PINGPONG_DITHER,
>   	DPU_PINGPONG_MAX
>   };
>   
> +
> +/** DSC sub-blocks/features
> + * @DPU_DSC_OUTPUT_CTRL         Supports the control of the pp id which gets
> + *                              the pixel output from this DSC.
> + * @DPU_DSC_HW_REV_1_1          dsc block supports dsc 1.1 only
> + * @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_ENC,                DSC encoder sub block
> + * @DPU_DSC_CTL,                DSC ctl sub block
> + * @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_ENC,
> +	DPU_DSC_CTL,
> +	DPU_DSC_MAX
> +};
> +
>   /**
>    * CTL sub-blocks
>    * @DPU_CTL_SPLIT_DISPLAY:	CTL supports video mode split display
> @@ -276,15 +300,6 @@ enum {
>   };
>   
>   /**
> - * DSC features
> - * @DPU_DSC_OUTPUT_CTRL       Configure which PINGPONG block gets
> - *                            the pixel output from this DSC.
> - */
> -enum {
> -	DPU_DSC_OUTPUT_CTRL = 0x1,
> -};
> -
> -/**
>    * MACRO DPU_HW_BLK_INFO - information of HW blocks inside DPU
>    * @name:              string name for debug purposes
>    * @id:                enum identifying this block
> @@ -346,6 +361,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 {
> @@ -403,6 +426,7 @@ struct dpu_rotation_cfg {
>    * @pixel_ram_size     size of latency hiding and de-tiling buffer in bytes
>    * @max_hdeci_exp      max horizontal decimation supported (max is 2^value)
>    * @max_vdeci_exp      max vertical decimation supported (max is 2^value)
> + * @max_dsc_width      max dsc line width support.
>    */
>   struct dpu_caps {
>   	u32 max_mixer_width;
> @@ -419,6 +443,7 @@ struct dpu_caps {
>   	u32 pixel_ram_size;
>   	u32 max_hdeci_exp;
>   	u32 max_vdeci_exp;
> +	u32 max_dsc_width;
>   };
>   
>   /**
> @@ -494,9 +519,20 @@ struct dpu_dspp_sub_blks {
>   struct dpu_pingpong_sub_blks {
>   	struct dpu_pp_blk te;
>   	struct dpu_pp_blk te2;
> +	struct dpu_pp_blk dsc;
>   	struct dpu_pp_blk dither;
>   };
>   
> +
> +/**
> + * struct dpu_dsc_sub_blks : DSC sub-blks
> + *
> + */
> +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
>    */
> @@ -641,10 +677,14 @@ 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
> + * @dsc_pair_mask:     Bitmask of DSCs that can be controlled by same CTL
>    */
>   struct dpu_dsc_cfg {
>   	DPU_HW_BLK_INFO;
> +	DECLARE_BITMAP(dsc_pair_mask, DSC_MAX);
> +	const struct dpu_dsc_sub_blks *sblk;
>   };
>   
>   /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> index 619926d..51e8890 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   /*
>    * Copyright (c) 2020-2022, Linaro Limited
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
>    */
>   
>   #include "dpu_kms.h"
> @@ -41,10 +42,11 @@ static void dpu_hw_dsc_disable(struct dpu_hw_dsc *dsc)
>   static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>   			      struct drm_dsc_config *dsc,
>   			      u32 mode,
> -			      u32 initial_lines)
> +			      u32 initial_lines,
> +			      bool ich_reset_override)
>   {
>   	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
> -	u32 data;
> +	u32 data, lsb, bpp;
>   	u32 slice_last_group_size;
>   	u32 det_thresh_flatness;
>   	bool is_cmd_mode = !(mode & DSC_MODE_VIDEO);
> @@ -58,7 +60,14 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>   	data = (initial_lines << 20);
>   	data |= ((slice_last_group_size - 1) << 18);
>   	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
> -	data |= (dsc->bits_per_pixel << 8);
> +	data |= dsc->bits_per_pixel << 12;
> +	lsb = dsc->bits_per_pixel % 4;
> +	bpp = dsc->bits_per_pixel / 4;
> +	bpp *= 4;
> +	bpp <<= 4;
> +	bpp |= lsb;
> +
> +	data |= bpp << 8;
>   	data |= (dsc->block_pred_enable << 7);
>   	data |= (dsc->line_buf_depth << 3);
>   	data |= (dsc->simple_422 << 2);
> @@ -221,7 +230,13 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
>   
>   	c->idx = idx;
>   	c->caps = cfg;
> -	_setup_dsc_ops(&c->ops, c->caps->features);
> +
> +	if (test_bit(DPU_DSC_HW_REV_1_1, &c->caps->features))
> +		_setup_dsc_ops(&c->ops, c->caps->features);
> +	else if (test_bit(DPU_DSC_HW_REV_1_2, &c->caps->features))
> +		dpu_dsc_1_2_setup_ops(&c->ops, c->caps->features);
> +	else
> +		_setup_dsc_ops(&c->ops, c->caps->features);
>   
>   	return c;
>   }
> 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 ae9b5db..a48f572 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
> @@ -33,7 +36,8 @@ struct dpu_hw_dsc_ops {
>   	void (*dsc_config)(struct dpu_hw_dsc *hw_dsc,
>   			   struct drm_dsc_config *dsc,
>   			   u32 mode,
> -			   u32 initial_lines);
> +			   u32 initial_lines,
> +			   bool ich_reset_override);
>   
>   	/**
>   	 * dsc_config_thresh - programs panel thresholds
> @@ -43,6 +47,12 @@ struct dpu_hw_dsc_ops {
>   	void (*dsc_config_thresh)(struct dpu_hw_dsc *hw_dsc,
>   				  struct drm_dsc_config *dsc);
>   
> +	/**
> +	 * bind_pingpong_blk - enable/disable the connection with pp
> +	 * @hw_dsc: Pointer to dsc context
> +	 * @enable: enable/disable connection
> +	 * @pp: pingpong blk id
> +	 */
>   	void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
>   				  bool enable,
>   				  enum dpu_pingpong pp);
> @@ -51,6 +61,7 @@ struct dpu_hw_dsc_ops {
>   struct dpu_hw_dsc {
>   	struct dpu_hw_blk base;
>   	struct dpu_hw_blk_reg_map hw;
> +	struct dpu_hw_ctl *hw_ctl;
>   
>   	/* dsc */
>   	enum dpu_dsc idx;
> @@ -76,9 +87,17 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
>    */
>   void dpu_hw_dsc_destroy(struct dpu_hw_dsc *dsc);
>   
> +/**
> + * dpu_hw_dsc - convert base object dpu_hw_base to container
> + * @hw: Pointer to base hardware block
> + * return: Pointer to hardware block container
> + */
>   static inline struct dpu_hw_dsc *to_dpu_hw_dsc(struct dpu_hw_blk *hw)
>   {
>   	return container_of(hw, struct dpu_hw_dsc, base);
>   }
>   
> +void dpu_dsc_1_2_setup_ops(struct dpu_hw_dsc_ops *ops,
> +		const unsigned long features);
> +
>   #endif /* _DPU_HW_DSC_H */
> 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..2be74ae
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
> @@ -0,0 +1,371 @@
> +// 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 "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_BUF_THRESH_1        0x6C
> +#define DSC_RC_BUF_THRESH_2        0x70
> +#define DSC_RC_BUF_THRESH_3        0x74
> +#define DSC_RC_MIN_QP_0            0x78
> +#define DSC_RC_MIN_QP_1            0x7C
> +#define DSC_RC_MIN_QP_2            0x80
> +#define DSC_RC_MAX_QP_0            0x84
> +#define DSC_RC_MAX_QP_1            0x88
> +#define DSC_RC_MAX_QP_2             0x8C
> +#define DSC_RC_RANGE_BPG_OFFSETS_0  0x90
> +#define DSC_RC_RANGE_BPG_OFFSETS_1  0x94
> +#define DSC_RC_RANGE_BPG_OFFSETS_2  0x98
> +
> +/* 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 int _dsc_calc_ob_max_addr(struct dpu_hw_dsc *hw_dsc, int num_ss)
> +{
> +	enum dpu_dsc idx;
> +
> +	idx = hw_dsc->idx;
> +
> +	if (!(hw_dsc->caps->features & BIT(DPU_DSC_NATIVE_422_EN))) {
> +		if (num_ss == 1)
> +			return 2399;
> +		else if (num_ss == 2)
> +			return 1199;
> +	} else {
> +		if (num_ss == 1)
> +			return 1199;
> +		else if (num_ss == 2)
> +			return 599;
> +	}
> +	return 0;
> +}
> +
> +static inline int _dsc_subblk_offset(struct dpu_hw_dsc *hw_dsc, int s_id,
> +		u32 *idx)
> +{
> +	const struct dpu_dsc_sub_blks *sblk;
> +
> +	if (!hw_dsc)
> +		return -EINVAL;
> +
> +	*idx = 0;
> +
> +	sblk = hw_dsc->caps->sblk;
> +
> +	switch (s_id) {
> +
> +	case DPU_DSC_ENC:
> +		*idx = sblk->enc.base;
> +		break;
> +	case DPU_DSC_CTL:
> +		*idx = sblk->ctl.base;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void dpu_hw_dsc_disable_1_2(struct dpu_hw_dsc *hw_dsc)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	u32 idx;
> +
> +	if (!hw_dsc)
> +		return;
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +	DPU_REG_WRITE(hw, DSC_CFG + idx, 0);
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
> +		return;
> +
> +	DPU_REG_WRITE(hw, ENC_DF_CTRL + idx, 0);
> +	DPU_REG_WRITE(hw, DSC_MAIN_CONF + idx, 0);
> +}
> +
> +static void dpu_hw_dsc_config_1_2(struct dpu_hw_dsc *hw_dsc,
> +		struct drm_dsc_config *dsc, u32 mode,
> +		u32 initial_lines, bool ich_reset_override)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	struct msm_display_dsc_info *dsc_info;
> +	u32 idx;
> +	u32 data = 0;
> +	u32 bpp;
> +	void __iomem *off;
> +
> +	if (!hw_dsc || !dsc)
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +
> +	dsc_info = to_msm_dsc_info(dsc);


Please don't do that, the architecture of dsc_info is crap, if you *really* need some
values that are not part of drm_dsc, then pass this *new" struct as parameter of dsc_config()
and change the dsc_config() declaration, it's not a problem.

> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
> +		return;
> +
> +	if (mode & DSC_MODE_SPLIT_PANEL)
> +		data |= BIT(0);
> +
> +	if (mode & DSC_MODE_MULTIPLEX)
> +		data |= BIT(1);
> +
> +	data |= (dsc_info->num_active_ss_per_enc & 0x3) << 7;
> +
> +	DPU_REG_WRITE(hw, DSC_CMN_MAIN_CNF, data);
> +
> +	data = (dsc_info->initial_lines & 0xff);
> +	data |= ((mode & DSC_MODE_VIDEO) ? 1 : 0) << 9;
> +	if (ich_reset_override)
> +		data |= 0xC00; // set bit 10 and 11
> +	data |= (_dsc_calc_ob_max_addr(hw_dsc, dsc_info->num_active_ss_per_enc) << 18);
> +
> +	DPU_REG_WRITE(hw, ENC_DF_CTRL + idx, 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, DSC_MAIN_CONF + idx, data);
> +
> +	data = (dsc->pic_width & 0xffff) |
> +		((dsc->pic_height & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_PICTURE_SIZE + idx, data);
> +
> +	data = (dsc->slice_width & 0xffff) |
> +		((dsc->slice_height & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_SLICE_SIZE + idx, data);
> +
> +	DPU_REG_WRITE(hw, DSC_MISC_SIZE + idx,
> +			(dsc->slice_chunk_size) & 0xffff);
> +
> +	data = (dsc->initial_xmit_delay & 0xffff) |
> +		((dsc->initial_dec_delay & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_HRD_DELAYS + idx, data);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_SCALE + idx,
> +			dsc->initial_scale_value & 0x3f);
> +
> +	data = (dsc->scale_increment_interval & 0xffff) |
> +		((dsc->scale_decrement_interval & 0x7ff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_SCALE_INC_DEC + idx, data);
> +
> +	data = (dsc->first_line_bpg_offset & 0x1f) |
> +		((dsc->second_line_bpg_offset & 0x1f) << 5);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_1 + idx, data);
> +
> +	data = (dsc->nfl_bpg_offset & 0xffff) |
> +		((dsc->slice_bpg_offset & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_2 + idx, data);
> +
> +	data = (dsc->initial_offset & 0xffff) |
> +		((dsc->final_offset & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_3 + idx, data);
> +
> +	data = (dsc->nsl_bpg_offset & 0xffff) |
> +		((dsc->second_line_offset_adj & 0xffff) << 16);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_4 + idx, data);
> +
> +	data = (dsc->flatness_min_qp & 0x1f);
> +	data |= (dsc->flatness_max_qp & 0x1f) << 5;
> +	data |= (dsc_info->det_thresh_flatness & 0xff) << 10;
> +
> +	DPU_REG_WRITE(hw, DSC_FLATNESS_QP + idx, data);
> +
> +	DPU_REG_WRITE(hw, DSC_RC_MODEL_SIZE + idx,
> +			(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, DSC_RC_CONFIG + idx, data);
> +
> +	/* program the dsc wrapper */
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
> +		return;
> +
> +	off = hw->blk_addr + idx;
> +
> +	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);
> +
> +	if (dsc_info->dsc_4hsmerge_en) {
> +		data |= dsc_info->dsc_4hsmerge_padding << 18;
> +		data |= dsc_info->dsc_4hsmerge_alignment << 22;
> +		data |= BIT(16);
> +	}
> +
> +	DPU_REG_WRITE(hw, DSC_CFG + idx, data);
> +
> +//	DPU_REG_WRITE(hw, DSC_DATA_IN_SWAP + idx, 0x14e5);
> +}
> +
> +static 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;
> +	struct msm_display_dsc_info *dsc_info;
> +	u32 idx, off;
> +	int i, j = 0;
> +	struct drm_dsc_rc_range_parameters *rc;
> +	u32 data = 0, min_qp = 0, max_qp = 0, bpg_off = 0;
> +
> +	if (!hw_dsc || !dsc)
> +		return;
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +
> +	dsc_info = to_msm_dsc_info(dsc);
> +
> +	rc = dsc->rc_range_params;
> +
> +	off = 0;
> +	for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
> +		data |= dsc->rc_buf_thresh[i] << (8*j);
> +		j++;
> +		if ((j == 4) || (i == DSC_NUM_BUF_RANGES - 2)) {
> +			DPU_REG_WRITE(hw, DSC_RC_BUF_THRESH_0 + idx + off,
> +					data);
> +			off += 4;
> +			j = 0;
> +			data = 0;
> +		}
> +	}
> +
> +	off = 0;
> +	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> +		min_qp |= (rc[i].range_min_qp & 0x1f) << 5*j;
> +		max_qp |= (rc[i].range_max_qp & 0x1f) << 5*j;
> +		bpg_off |= (rc[i].range_bpg_offset & 0x3f) << 6*j;
> +		j++;
> +		if (j == 5) {
> +			DPU_REG_WRITE(hw, DSC_RC_MIN_QP_0 + idx + off,
> +					min_qp);
> +			DPU_REG_WRITE(hw, DSC_RC_MAX_QP_0 + idx + off,
> +					max_qp);
> +			DPU_REG_WRITE(hw,
> +					DSC_RC_RANGE_BPG_OFFSETS_0 + idx + off,
> +					bpg_off);
> +			off += 4;
> +			j = 0;
> +			min_qp = 0;
> +			max_qp = 0;
> +			bpg_off = 0;
> +		}
> +	}
> +}
> +
> +static void dpu_hw_dsc_bind_pingpong_blk_1_2(
> +		struct dpu_hw_dsc *hw_dsc,
> +		bool enable,
> +		const enum dpu_pingpong pp)
> +{
> +	struct dpu_hw_blk_reg_map *hw;
> +	int idx;
> +	int mux_cfg = 0xF; /* Disabled */
> +
> +	if (!hw_dsc)
> +		return;
> +
> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
> +		return;
> +
> +	hw = &hw_dsc->hw;
> +	if (enable)
> +		mux_cfg = (pp - PINGPONG_0) & 0x7;
> +
> +	DPU_REG_WRITE(hw, DSC_CTL + idx, mux_cfg);
> +}
> +
> +void dpu_dsc_1_2_setup_ops(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;
> +}

Kuogee Hsieh Jan. 24, 2023, 11:52 p.m. UTC | #4

On 1/23/2023 12:11 PM, Marijn Suijten wrote:
> add support for*
>
> drm/msm/dpu*
>
> On 2023-01-23 10:24:30, Kuogee Hsieh wrote:
>> DSC V1.2 encoder engine is newly added hardware module. This patch
>> add support functions to configure and enable DSC V1.2 encoder engine.
>>
>> Signed-off-by: Kuogee Hsieh <[e-mail]>
>> ---
>>   drivers/gpu/drm/msm/Makefile                   |   1 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c    |   2 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  60 +++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c     |  23 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h     |  23 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 371 +++++++++++++++++++++++++
>>   6 files changed, 463 insertions(+), 17 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 28cf52b..271c29a15 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_dsc_helper.o \
>>   	disp/dpu1/dpu_hw_interrupts.o \
>>   	disp/dpu1/dpu_hw_intf.o \
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 7f4a439..901e317 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1821,7 +1821,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
>>   				     u32 initial_lines)
>>   {
>>   	if (hw_dsc->ops.dsc_config)
>> -		hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines);
>> +		hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines, false);
> As usual, an enum is better: readers have no idea what a free-floating
> bool means.
>
>>   
>>   	if (hw_dsc->ops.dsc_config_thresh)
>>   		hw_dsc->ops.dsc_config_thresh(hw_dsc, dsc);
>> 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 978e3bd..7b0b092 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.
>>    */
>>   
>> @@ -11,6 +11,7 @@
>>   #include <linux/bug.h>
>>   #include <linux/bitmap.h>
>>   #include <linux/err.h>
>> +#include "dpu_hw_mdss.h"
> Unused if you remove the unused DECLARE_BITMAP(dsc_pair_mask, DSC_MAX).
>
>>   
>>   /**
>>    * Max hardware block count: For ex: max 12 SSPP pipes or
>> @@ -182,6 +183,7 @@ enum {
>>    * @DPU_PINGPONG_TE2        Additional tear check block for split pipes
>>    * @DPU_PINGPONG_SPLIT      PP block supports split fifo
>>    * @DPU_PINGPONG_SLAVE      PP block is a suitable slave for split fifo
>> + * @DPU_PINGPONG_DSC,       Display stream compression blocks
>>    * @DPU_PINGPONG_DITHER,    Dither blocks
>>    * @DPU_PINGPONG_MAX
>>    */
>> @@ -190,10 +192,32 @@ enum {
>>   	DPU_PINGPONG_TE2,
>>   	DPU_PINGPONG_SPLIT,
>>   	DPU_PINGPONG_SLAVE,
>> +	DPU_PINGPONG_DSC,
> This is not used.
>
>>   	DPU_PINGPONG_DITHER,
>>   	DPU_PINGPONG_MAX
>>   };
>>   
>> +
>> +/** DSC sub-blocks/features
> Newline between /** and the text.
>
>> + * @DPU_DSC_OUTPUT_CTRL         Supports the control of the pp id which gets
>> + *                              the pixel output from this DSC.
> The original comment is much more concise, can we keep it?
>
>> + * @DPU_DSC_HW_REV_1_1          dsc block supports dsc 1.1 only
>> + * @DPU_DSC_HW_REV_1_2          dsc block supports dsc 1.1 and 1.2
> Capitalize DSC just like elsewhere.
>
>> + * @DPU_DSC_NATIVE_422_EN,      Supports native422 and native420 encoding
>> + * @DPU_DSC_ENC,                DSC encoder sub block
>> + * @DPU_DSC_CTL,                DSC ctl sub block
> No need for trailing commas in doc comments; if anything replace them
> with colons?
>
>> + * @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_ENC,
>> +	DPU_DSC_CTL,
> These two enum values only have a meaning within the dpu_hw_dsc_1_2.c
> file, and have nothing to do with the other feature flags/block
> description.  Please move them there (and give _dsc_subblk_offset a
> proper enum type).
>
>> +	DPU_DSC_MAX
>> +};
>> +
>>   /**
>>    * CTL sub-blocks
>>    * @DPU_CTL_SPLIT_DISPLAY:	CTL supports video mode split display
>> @@ -276,15 +300,6 @@ enum {
>>   };
>>   
>>   /**
>> - * DSC features
>> - * @DPU_DSC_OUTPUT_CTRL       Configure which PINGPONG block gets
>> - *                            the pixel output from this DSC.
>> - */
>> -enum {
>> -	DPU_DSC_OUTPUT_CTRL = 0x1,
> Did this have to move?
>
>> -};
>> -
>> -/**
>>    * MACRO DPU_HW_BLK_INFO - information of HW blocks inside DPU
>>    * @name:              string name for debug purposes
>>    * @id:                enum identifying this block
>> @@ -346,6 +361,14 @@ struct dpu_pp_blk {
>>   };
>>   
>>   /**
>> + * struct dpu_dsc_blk : DSC Encoder sub-blk information
> Use a hyphen here and everywhere else:
> https://docs.kernel.org/doc-guide/kernel-doc.html
>
>> + * @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 {
>> @@ -403,6 +426,7 @@ struct dpu_rotation_cfg {
>>    * @pixel_ram_size     size of latency hiding and de-tiling buffer in bytes
>>    * @max_hdeci_exp      max horizontal decimation supported (max is 2^value)
>>    * @max_vdeci_exp      max vertical decimation supported (max is 2^value)
>> + * @max_dsc_width      max dsc line width support.
> DSC*
>
>>    */
>>   struct dpu_caps {
>>   	u32 max_mixer_width;
>> @@ -419,6 +443,7 @@ struct dpu_caps {
>>   	u32 pixel_ram_size;
>>   	u32 max_hdeci_exp;
>>   	u32 max_vdeci_exp;
>> +	u32 max_dsc_width;
> This is never read.
>
>>   };
>>   
>>   /**
>> @@ -494,9 +519,20 @@ struct dpu_dspp_sub_blks {
>>   struct dpu_pingpong_sub_blks {
>>   	struct dpu_pp_blk te;
>>   	struct dpu_pp_blk te2;
>> +	struct dpu_pp_blk dsc;
> Unused.
>
>>   	struct dpu_pp_blk dither;
>>   };
>>   
>> +
>> +/**
>> + * struct dpu_dsc_sub_blks : DSC sub-blks
>> + *
> A sub-block of sub-blocks?  Use the documentation to explain what this
> is for, describe @enc and @ctl.
>
>> + */
>> +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
>>    */
>> @@ -641,10 +677,14 @@ 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
>> + * @dsc_pair_mask:     Bitmask of DSCs that can be controlled by same CTL
>>    */
>>   struct dpu_dsc_cfg {
>>   	DPU_HW_BLK_INFO;
>> +	DECLARE_BITMAP(dsc_pair_mask, DSC_MAX);
> This bitmask is unused.
>
>> +	const struct dpu_dsc_sub_blks *sblk;
>>   };
>>   
>>   /**
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>> index 619926d..51e8890 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>>   /*
>>    * Copyright (c) 2020-2022, Linaro Limited
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
>>    */
>>   
>>   #include "dpu_kms.h"
>> @@ -41,10 +42,11 @@ static void dpu_hw_dsc_disable(struct dpu_hw_dsc *dsc)
>>   static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>>   			      struct drm_dsc_config *dsc,
>>   			      u32 mode,
>> -			      u32 initial_lines)
>> +			      u32 initial_lines,
>> +			      bool ich_reset_override)
>>   {
>>   	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
>> -	u32 data;
>> +	u32 data, lsb, bpp;
>>   	u32 slice_last_group_size;
>>   	u32 det_thresh_flatness;
>>   	bool is_cmd_mode = !(mode & DSC_MODE_VIDEO);
>> @@ -58,7 +60,14 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>>   	data = (initial_lines << 20);
>>   	data |= ((slice_last_group_size - 1) << 18);
>>   	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
>> -	data |= (dsc->bits_per_pixel << 8);
>> +	data |= dsc->bits_per_pixel << 12;
>> +	lsb = dsc->bits_per_pixel % 4;
>> +	bpp = dsc->bits_per_pixel / 4;
>> +	bpp *= 4;
>> +	bpp <<= 4;
>> +	bpp |= lsb;
>> +
>> +	data |= bpp << 8;
> Why are you re-adding this nonsense?  It was removed in [1] _and_ does
> not account for bits_per_pixel _already being in x.4 format_. This will
> regress existing hardware.
>
> [1]: https://lore.kernel.org/linux-arm-msm/[e-mail]/
>
>>   	data |= (dsc->block_pred_enable << 7);
>>   	data |= (dsc->line_buf_depth << 3);
>>   	data |= (dsc->simple_422 << 2);
>> @@ -221,7 +230,13 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
>>   
>>   	c->idx = idx;
>>   	c->caps = cfg;
>> -	_setup_dsc_ops(&c->ops, c->caps->features);
>> +
>> +	if (test_bit(DPU_DSC_HW_REV_1_1, &c->caps->features))
>> +		_setup_dsc_ops(&c->ops, c->caps->features);
>> +	else if (test_bit(DPU_DSC_HW_REV_1_2, &c->caps->features))
>> +		dpu_dsc_1_2_setup_ops(&c->ops, c->caps->features);
>> +	else
>> +		_setup_dsc_ops(&c->ops, c->caps->features);
>>   
>>   	return c;
>>   }
>> 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 ae9b5db..a48f572 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
>> @@ -33,7 +36,8 @@ struct dpu_hw_dsc_ops {
>>   	void (*dsc_config)(struct dpu_hw_dsc *hw_dsc,
>>   			   struct drm_dsc_config *dsc,
>>   			   u32 mode,
>> -			   u32 initial_lines);
>> +			   u32 initial_lines,
>> +			   bool ich_reset_override);
>>   
>>   	/**
>>   	 * dsc_config_thresh - programs panel thresholds
>> @@ -43,6 +47,12 @@ struct dpu_hw_dsc_ops {
>>   	void (*dsc_config_thresh)(struct dpu_hw_dsc *hw_dsc,
>>   				  struct drm_dsc_config *dsc);
>>   
>> +	/**
>> +	 * bind_pingpong_blk - enable/disable the connection with pp
> Inherit docs from the enum.
>
>> +	 * @hw_dsc: Pointer to dsc context
> DSC*
>
>> +	 * @enable: enable/disable connection
>> +	 * @pp: pingpong blk id
> It's documentation, write out block fully.
>
>> +	 */
>>   	void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
>>   				  bool enable,
>>   				  enum dpu_pingpong pp);
>> @@ -51,6 +61,7 @@ struct dpu_hw_dsc_ops {
>>   struct dpu_hw_dsc {
>>   	struct dpu_hw_blk base;
>>   	struct dpu_hw_blk_reg_map hw;
>> +	struct dpu_hw_ctl *hw_ctl;
> Unused.
>
>>   
>>   	/* dsc */
>>   	enum dpu_dsc idx;
>> @@ -76,9 +87,17 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
>>    */
>>   void dpu_hw_dsc_destroy(struct dpu_hw_dsc *dsc);
>>   
>> +/**
>> + * dpu_hw_dsc - convert base object dpu_hw_base to container
>> + * @hw: Pointer to base hardware block
>> + * return: Pointer to hardware block container
>> + */
>>   static inline struct dpu_hw_dsc *to_dpu_hw_dsc(struct dpu_hw_blk *hw)
>>   {
>>   	return container_of(hw, struct dpu_hw_dsc, base);
>>   }
>>   
>> +void dpu_dsc_1_2_setup_ops(struct dpu_hw_dsc_ops *ops,
>> +		const unsigned long features);
>> +
>>   #endif /* _DPU_HW_DSC_H */
>> 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..2be74ae
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
>> @@ -0,0 +1,371 @@
>> +// 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 "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_BUF_THRESH_1        0x6C
>> +#define DSC_RC_BUF_THRESH_2        0x70
>> +#define DSC_RC_BUF_THRESH_3        0x74
>> +#define DSC_RC_MIN_QP_0            0x78
>> +#define DSC_RC_MIN_QP_1            0x7C
>> +#define DSC_RC_MIN_QP_2            0x80
>> +#define DSC_RC_MAX_QP_0            0x84
>> +#define DSC_RC_MAX_QP_1            0x88
>> +#define DSC_RC_MAX_QP_2             0x8C
>> +#define DSC_RC_RANGE_BPG_OFFSETS_0  0x90
>> +#define DSC_RC_RANGE_BPG_OFFSETS_1  0x94
>> +#define DSC_RC_RANGE_BPG_OFFSETS_2  0x98
> Reindent to line this back up.
>
>> +
>> +/* 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 int _dsc_calc_ob_max_addr(struct dpu_hw_dsc *hw_dsc, int num_ss)
>> +{
>> +	enum dpu_dsc idx;
>> +
>> +	idx = hw_dsc->idx;
>> +
>> +	if (!(hw_dsc->caps->features & BIT(DPU_DSC_NATIVE_422_EN))) {
> Why not swap the bodies instead of inverting this.
>
>> +		if (num_ss == 1)
>> +			return 2399;
>> +		else if (num_ss == 2)
>> +			return 1199;
>> +	} else {
>> +		if (num_ss == 1)
>> +			return 1199;
>> +		else if (num_ss == 2)
>> +			return 599;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static inline int _dsc_subblk_offset(struct dpu_hw_dsc *hw_dsc, int s_id,
>> +		u32 *idx)
>> +{
>> +	const struct dpu_dsc_sub_blks *sblk;
>> +
>> +	if (!hw_dsc)
>> +		return -EINVAL;
>> +
>> +	*idx = 0;
>> +
>> +	sblk = hw_dsc->caps->sblk;
>> +
>> +	switch (s_id) {
>> +
>> +	case DPU_DSC_ENC:
>> +		*idx = sblk->enc.base;
>> +		break;
>> +	case DPU_DSC_CTL:
>> +		*idx = sblk->ctl.base;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void dpu_hw_dsc_disable_1_2(struct dpu_hw_dsc *hw_dsc)
>> +{
>> +	struct dpu_hw_blk_reg_map *hw;
>> +	u32 idx;
> Can we rename these to offset or subblk_offset or something more clear?
>
>> +
>> +	if (!hw_dsc)
>> +		return;
>> +
>> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
>> +		return;
> These error checks are excessive: you pass in a non-null hw_dsc and
> known enum constant - _dsc_subblk_offset should perhaps not return
> errors at all.
>
>> +
>> +	hw = &hw_dsc->hw;
>> +	DPU_REG_WRITE(hw, DSC_CFG + idx, 0);
> Swap the arguments to + so that it's clear that DSC_CFG is a register on
> the subblock offset denoted by "idx", not the other way around.
>
>> +
>> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
>> +		return;
>> +
>> +	DPU_REG_WRITE(hw, ENC_DF_CTRL + idx, 0);
>> +	DPU_REG_WRITE(hw, DSC_MAIN_CONF + idx, 0);
>> +}
>> +
>> +static void dpu_hw_dsc_config_1_2(struct dpu_hw_dsc *hw_dsc,
>> +		struct drm_dsc_config *dsc, u32 mode,
>> +		u32 initial_lines, bool ich_reset_override)
>> +{
>> +	struct dpu_hw_blk_reg_map *hw;
>> +	struct msm_display_dsc_info *dsc_info;
>> +	u32 idx;
>> +	u32 data = 0;
>> +	u32 bpp;
>> +	void __iomem *off;
>> +
>> +	if (!hw_dsc || !dsc)
>> +		return;
>> +
>> +	hw = &hw_dsc->hw;
>> +
>> +	dsc_info = to_msm_dsc_info(dsc);
>> +
>> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
>> +		return;
>> +
>> +	if (mode & DSC_MODE_SPLIT_PANEL)
>> +		data |= BIT(0);
>> +
>> +	if (mode & DSC_MODE_MULTIPLEX)
>> +		data |= BIT(1);
> These are well known bitwise definitions for a reason, data |= mode will
> do (or out DSC_MODE_VIDEO since you have to shift that one at BIT(9).
>
>> +
>> +	data |= (dsc_info->num_active_ss_per_enc & 0x3) << 7;
>> +
>> +	DPU_REG_WRITE(hw, DSC_CMN_MAIN_CNF, data);
>> +
>> +	data = (dsc_info->initial_lines & 0xff);
> You already get initial_lines passed as function argument, but ignore
> it?
>
>> +	data |= ((mode & DSC_MODE_VIDEO) ? 1 : 0) << 9;
> Yuck. if (mode & DSC_MODE_VIDEO) data |= BIT(9);.
>
>> +	if (ich_reset_override)
>> +		data |= 0xC00; // set bit 10 and 11
> Instead of a comment, make this self-describing BIT(10) | BIT(11) code.
>
>> +	data |= (_dsc_calc_ob_max_addr(hw_dsc, dsc_info->num_active_ss_per_enc) << 18);
>> +
>> +	DPU_REG_WRITE(hw, ENC_DF_CTRL + idx, 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 above, don't forget to read the documentation on this field:
>
>      Target bits per pixel with 4 fractional bits, bits_per_pixel << 4
>
>> +	/* 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);
> Either wrap everything or nothing in ().
>
>> +	data |= (dsc->line_buf_depth & 0xf) << 6;
>> +	data |= dsc->convert_rgb << 4;
>> +	data |= dsc->bits_per_component & 0xf;
>> +
>> +	DPU_REG_WRITE(hw, DSC_MAIN_CONF + idx, data);
>> +
>> +	data = (dsc->pic_width & 0xffff) |
>> +		((dsc->pic_height & 0xffff) << 16);
>> +
>> +	DPU_REG_WRITE(hw, DSC_PICTURE_SIZE + idx, data);
>> +
>> +	data = (dsc->slice_width & 0xffff) |
>> +		((dsc->slice_height & 0xffff) << 16);
>> +
>> +	DPU_REG_WRITE(hw, DSC_SLICE_SIZE + idx, data);
>> +
>> +	DPU_REG_WRITE(hw, DSC_MISC_SIZE + idx,
>> +			(dsc->slice_chunk_size) & 0xffff);
>> +
>> +	data = (dsc->initial_xmit_delay & 0xffff) |
>> +		((dsc->initial_dec_delay & 0xffff) << 16);
>> +
>> +	DPU_REG_WRITE(hw, DSC_HRD_DELAYS + idx, data);
>> +
>> +	DPU_REG_WRITE(hw, DSC_RC_SCALE + idx,
>> +			dsc->initial_scale_value & 0x3f);
>> +
>> +	data = (dsc->scale_increment_interval & 0xffff) |
>> +		((dsc->scale_decrement_interval & 0x7ff) << 16);
>> +
>> +	DPU_REG_WRITE(hw, DSC_RC_SCALE_INC_DEC + idx, data);
>> +
>> +	data = (dsc->first_line_bpg_offset & 0x1f) |
>> +		((dsc->second_line_bpg_offset & 0x1f) << 5);
>> +
>> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_1 + idx, data);
>> +
>> +	data = (dsc->nfl_bpg_offset & 0xffff) |
>> +		((dsc->slice_bpg_offset & 0xffff) << 16);
>> +
>> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_2 + idx, data);
>> +
>> +	data = (dsc->initial_offset & 0xffff) |
>> +		((dsc->final_offset & 0xffff) << 16);
>> +
>> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_3 + idx, data);
>> +
>> +	data = (dsc->nsl_bpg_offset & 0xffff) |
>> +		((dsc->second_line_offset_adj & 0xffff) << 16);
>> +
>> +	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_4 + idx, data);
>> +
>> +	data = (dsc->flatness_min_qp & 0x1f);
>> +	data |= (dsc->flatness_max_qp & 0x1f) << 5;
>> +	data |= (dsc_info->det_thresh_flatness & 0xff) << 10;
> dpu_hw_dsc.c computes this on the fly.  After removing that, and
> using initial_lines from the function parameters, only
> dsc_info->num_active_ss_per_enc remains.  Do you really need that
> msm_display_dsc_info struct here, do you need it at all?

I ported these code from our down stream code base.

I make it work first, then clean it up will follow.

I submit it for review since it looks like you guy like to have code sooner.

yes, eliminate msm_display_dsc_info is my next target and hope it can be 
done.





>
>> +
>> +	DPU_REG_WRITE(hw, DSC_FLATNESS_QP + idx, data);
>> +
>> +	DPU_REG_WRITE(hw, DSC_RC_MODEL_SIZE + idx,
>> +			(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, DSC_RC_CONFIG + idx, data);
>> +
>> +	/* program the dsc wrapper */
>> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
>> +		return;
>> +
>> +	off = hw->blk_addr + idx;
>> +
>> +	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);
>> +
>> +	if (dsc_info->dsc_4hsmerge_en) {
>> +		data |= dsc_info->dsc_4hsmerge_padding << 18;
>> +		data |= dsc_info->dsc_4hsmerge_alignment << 22;
>> +		data |= BIT(16);
>> +	}
>> +
>> +	DPU_REG_WRITE(hw, DSC_CFG + idx, data);
>> +
>> +//	DPU_REG_WRITE(hw, DSC_DATA_IN_SWAP + idx, 0x14e5);
> No commented-out code please, especially not with //
>
>> +}
>> +
>> +static 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;
>> +	struct msm_display_dsc_info *dsc_info;
>> +	u32 idx, off;
>> +	int i, j = 0;
>> +	struct drm_dsc_rc_range_parameters *rc;
>> +	u32 data = 0, min_qp = 0, max_qp = 0, bpg_off = 0;
>> +
>> +	if (!hw_dsc || !dsc)
>> +		return;
>> +
>> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
>> +		return;
>> +
>> +	hw = &hw_dsc->hw;
>> +
>> +	dsc_info = to_msm_dsc_info(dsc);
>> +
>> +	rc = dsc->rc_range_params;
>> +
>> +	off = 0;
>> +	for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
>> +		data |= dsc->rc_buf_thresh[i] << (8*j);
> Lack of spaces does not make this multiplication any prettier to read.
>
> * has precedence over << but it's better to replicate the () below as
> well.
>
>> +		j++;
>> +		if ((j == 4) || (i == DSC_NUM_BUF_RANGES - 2)) {
>> +			DPU_REG_WRITE(hw, DSC_RC_BUF_THRESH_0 + idx + off,
>> +					data);
>> +			off += 4;
>> +			j = 0;
>> +			data = 0;
>> +		}
>> +	}
>> +
>> +	off = 0;
>> +	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
>> +		min_qp |= (rc[i].range_min_qp & 0x1f) << 5*j;
>> +		max_qp |= (rc[i].range_max_qp & 0x1f) << 5*j;
>> +		bpg_off |= (rc[i].range_bpg_offset & 0x3f) << 6*j;
> These values _must_ already be masked to be useful in
> drm_dsc_compute_rc_parameters(), no need to mask them again just like
> the v1.1 block implementation.
>
>> +		j++;
>> +		if (j == 5) {
>> +			DPU_REG_WRITE(hw, DSC_RC_MIN_QP_0 + idx + off,
>> +					min_qp);
>> +			DPU_REG_WRITE(hw, DSC_RC_MAX_QP_0 + idx + off,
>> +					max_qp);
>> +			DPU_REG_WRITE(hw,
>> +					DSC_RC_RANGE_BPG_OFFSETS_0 + idx + off,
>> +					bpg_off);
>> +			off += 4;
>> +			j = 0;
>> +			min_qp = 0;
>> +			max_qp = 0;
>> +			bpg_off = 0;
>> +		}
>> +	}
>> +}
>> +
>> +static void dpu_hw_dsc_bind_pingpong_blk_1_2(
>> +		struct dpu_hw_dsc *hw_dsc,
>> +		bool enable,
>> +		const enum dpu_pingpong pp)
>> +{
>> +	struct dpu_hw_blk_reg_map *hw;
>> +	int idx;
>> +	int mux_cfg = 0xF; /* Disabled */
> Lowercase hex (and anywhere else if I skipped any).
>
>> +
>> +	if (!hw_dsc)
>> +		return;
> As with the v1.1 implementation, we don't check this, and your function
> below also checks it (but it does not need to).
>
>> +	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
>> +		return;
>> +
>> +	hw = &hw_dsc->hw;
>> +	if (enable)
>> +		mux_cfg = (pp - PINGPONG_0) & 0x7;
>> +
>> +	DPU_REG_WRITE(hw, DSC_CTL + idx, mux_cfg);
>> +}
>> +
>> +void dpu_dsc_1_2_setup_ops(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;
>> +}
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> All in all you really need to revise and clean your patches before
> sending them to the lists; these are already far too many comments and
> nits, and massively take away from reviewing code behaviour which I have
> not even started with after looking at 1 out of 14 patches :(
>
> - Marijn

Marijn Suijten Jan. 30, 2023, 8:16 p.m. UTC | #5

On 2023-01-24 15:52:46, Kuogee Hsieh wrote:

<snip>

If only replying to a small chunk somewhere in the middle of a diff
and/or large review, please cut out unnecessary bits to make your reply
easier to find :)

> >> +	data = (dsc->flatness_min_qp & 0x1f);
> >> +	data |= (dsc->flatness_max_qp & 0x1f) << 5;
> >> +	data |= (dsc_info->det_thresh_flatness & 0xff) << 10;
> > dpu_hw_dsc.c computes this on the fly.  After removing that, and
> > using initial_lines from the function parameters, only
> > dsc_info->num_active_ss_per_enc remains.  Do you really need that
> > msm_display_dsc_info struct here, do you need it at all?
> 
> I ported these code from our down stream code base.
> 
> I make it work first, then clean it up will follow.
> 
> I submit it for review since it looks like you guy like to have code sooner.

Correct, I was looking forward to these patches albeit complete with the
promised DSI support from Jessica, which still seems to be pending.

When sending patches to that extent, with the intent of getting quick
turnaround but knowing that they are not ready for prime time yet (or
were they, based on your "submit it for review" mention? Don't you mean
testing?), please annotate the series with an RFC tag accompanied with a
description what still needs to be done and why.  That would have saved
a great deal of comments and review.

> yes, eliminate msm_display_dsc_info is my next target and hope it can be 
> done.

Thank you.  Again, if that was the intent from the get-go, that's
perfect material to put in an RFC series' cover letter.

- Marijn

Abhinav Kumar Jan. 30, 2023, 9:22 p.m. UTC | #6

Hi Marijn

On 1/30/2023 12:16 PM, Marijn Suijten wrote:
> On 2023-01-24 15:52:46, Kuogee Hsieh wrote:
> 
> <snip>
> 
> If only replying to a small chunk somewhere in the middle of a diff
> and/or large review, please cut out unnecessary bits to make your reply
> easier to find :)
> 
>>>> +	data = (dsc->flatness_min_qp & 0x1f);
>>>> +	data |= (dsc->flatness_max_qp & 0x1f) << 5;
>>>> +	data |= (dsc_info->det_thresh_flatness & 0xff) << 10;
>>> dpu_hw_dsc.c computes this on the fly.  After removing that, and
>>> using initial_lines from the function parameters, only
>>> dsc_info->num_active_ss_per_enc remains.  Do you really need that
>>> msm_display_dsc_info struct here, do you need it at all?
>>
>> I ported these code from our down stream code base.
>>
>> I make it work first, then clean it up will follow.
>>
>> I submit it for review since it looks like you guy like to have code sooner.
> 
> Correct, I was looking forward to these patches albeit complete with the
> promised DSI support from Jessica, which still seems to be pending.
> 

DSI support is still being worked upon.

I dont think we promised DSC 1.2 will come with DSI together in the same 
series. It was always going to be DSC 1.2 + DP followed by another 
series from Jessica for DSI.

Lets set the expectations right.

Thanks

Abhinav
> When sending patches to that extent, with the intent of getting quick
> turnaround but knowing that they are not ready for prime time yet (or
> were they, based on your "submit it for review" mention? Don't you mean
> testing?), please annotate the series with an RFC tag accompanied with a
> description what still needs to be done and why.  That would have saved
> a great deal of comments and review.
> 
>> yes, eliminate msm_display_dsc_info is my next target and hope it can be
>> done.
> 
> Thank you.  Again, if that was the intent from the get-go, that's
> perfect material to put in an RFC series' cover letter.
> 
> - Marijn

Marijn Suijten Jan. 30, 2023, 10:31 p.m. UTC | #7

Abhinav,

On 2023-01-30 13:22:03, Abhinav Kumar wrote:
> Hi Marijn
> 
> On 1/30/2023 12:16 PM, Marijn Suijten wrote:
> > On 2023-01-24 15:52:46, Kuogee Hsieh wrote:
> > 
> > <snip>
> > 
> > If only replying to a small chunk somewhere in the middle of a diff
> > and/or large review, please cut out unnecessary bits to make your reply
> > easier to find :)
> > 
> >>>> +	data = (dsc->flatness_min_qp & 0x1f);
> >>>> +	data |= (dsc->flatness_max_qp & 0x1f) << 5;
> >>>> +	data |= (dsc_info->det_thresh_flatness & 0xff) << 10;
> >>> dpu_hw_dsc.c computes this on the fly.  After removing that, and
> >>> using initial_lines from the function parameters, only
> >>> dsc_info->num_active_ss_per_enc remains.  Do you really need that
> >>> msm_display_dsc_info struct here, do you need it at all?
> >>
> >> I ported these code from our down stream code base.
> >>
> >> I make it work first, then clean it up will follow.
> >>
> >> I submit it for review since it looks like you guy like to have code sooner.
> > 
> > Correct, I was looking forward to these patches albeit complete with the
> > promised DSI support from Jessica, which still seems to be pending.
> > 
> 
> DSI support is still being worked upon.
> 
> I dont think we promised DSC 1.2 will come with DSI together in the same 
> series. It was always going to be DSC 1.2 + DP followed by another 
> series from Jessica for DSI.
> 
> Lets set the expectations right.

Not saying that these patches were promised as part of this series (as
said, "which still seem to be pending"), just making clear that this
series if of no use to me (no hurry to get the code in my hands sooner)
until the DSI patches are also shared which I would have started working
on myself if I didn't know QUIC was picking it up to distract from the
current v1.1 broken-ness on SM8150 and SM8250.

To set my (and at least Neil's) expectations straight as well: DSC 1.2
HW support should come in a separate series without DP support.  Smaller
series (not to mention appropriately split-up patches) lead to a
decrease in scope, less dependencies and hopefully more efficient v2 -
for all involved.

- Marijn

Abhinav Kumar Jan. 30, 2023, 10:39 p.m. UTC | #8

On 1/30/2023 2:31 PM, Marijn Suijten wrote:
> Abhinav,
> 
> On 2023-01-30 13:22:03, Abhinav Kumar wrote:
>> Hi Marijn
>>
>> On 1/30/2023 12:16 PM, Marijn Suijten wrote:
>>> On 2023-01-24 15:52:46, Kuogee Hsieh wrote:
>>>
>>> <snip>
>>>
>>> If only replying to a small chunk somewhere in the middle of a diff
>>> and/or large review, please cut out unnecessary bits to make your reply
>>> easier to find :)
>>>
>>>>>> +	data = (dsc->flatness_min_qp & 0x1f);
>>>>>> +	data |= (dsc->flatness_max_qp & 0x1f) << 5;
>>>>>> +	data |= (dsc_info->det_thresh_flatness & 0xff) << 10;
>>>>> dpu_hw_dsc.c computes this on the fly.  After removing that, and
>>>>> using initial_lines from the function parameters, only
>>>>> dsc_info->num_active_ss_per_enc remains.  Do you really need that
>>>>> msm_display_dsc_info struct here, do you need it at all?
>>>>
>>>> I ported these code from our down stream code base.
>>>>
>>>> I make it work first, then clean it up will follow.
>>>>
>>>> I submit it for review since it looks like you guy like to have code sooner.
>>>
>>> Correct, I was looking forward to these patches albeit complete with the
>>> promised DSI support from Jessica, which still seems to be pending.
>>>
>>
>> DSI support is still being worked upon.
>>
>> I dont think we promised DSC 1.2 will come with DSI together in the same
>> series. It was always going to be DSC 1.2 + DP followed by another
>> series from Jessica for DSI.
>>
>> Lets set the expectations right.
> 
> Not saying that these patches were promised as part of this series (as
> said, "which still seem to be pending"), just making clear that this
> series if of no use to me (no hurry to get the code in my hands sooner)
> until the DSI patches are also shared which I would have started working
> on myself if I didn't know QUIC was picking it up to distract from the
> current v1.1 broken-ness on SM8150 and SM8250.
> 

This is being by Quic for everyone's benefit. So that we can land a 
working DSC 1.2 solution for DSI as a working example for all future 
panels. We only took it up to help others like you and linaro team to 
give a working example of a DSC 1.2 panel with command mode in upstream.


> To set my (and at least Neil's) expectations straight as well: DSC 1.2
> HW support should come in a separate series without DP support.  Smaller
> series (not to mention appropriately split-up patches) lead to a
> decrease in scope, less dependencies and hopefully more efficient v2 -
> for all involved.
> 

As I already wrote earlier, we will fix the mistakes of v1, make v2 
better and it will be split up better. But DSC 1.2 HW support had to be 
pushed along with DP or DSI to show its working. We chose DP to go with 
it as it aligns better with our upstream plans.


> - Marijn

Patch

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 28cf52b..271c29a15 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_dsc_helper.o \
 	disp/dpu1/dpu_hw_interrupts.o \
 	disp/dpu1/dpu_hw_intf.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 7f4a439..901e317 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1821,7 +1821,7 @@  static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
 				     u32 initial_lines)
 {
 	if (hw_dsc->ops.dsc_config)
-		hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines);
+		hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines, false);
 
 	if (hw_dsc->ops.dsc_config_thresh)
 		hw_dsc->ops.dsc_config_thresh(hw_dsc, dsc);
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 978e3bd..7b0b092 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.
  */
 
@@ -11,6 +11,7 @@ 
 #include <linux/bug.h>
 #include <linux/bitmap.h>
 #include <linux/err.h>
+#include "dpu_hw_mdss.h"
 
 /**
  * Max hardware block count: For ex: max 12 SSPP pipes or
@@ -182,6 +183,7 @@  enum {
  * @DPU_PINGPONG_TE2        Additional tear check block for split pipes
  * @DPU_PINGPONG_SPLIT      PP block supports split fifo
  * @DPU_PINGPONG_SLAVE      PP block is a suitable slave for split fifo
+ * @DPU_PINGPONG_DSC,       Display stream compression blocks
  * @DPU_PINGPONG_DITHER,    Dither blocks
  * @DPU_PINGPONG_MAX
  */
@@ -190,10 +192,32 @@  enum {
 	DPU_PINGPONG_TE2,
 	DPU_PINGPONG_SPLIT,
 	DPU_PINGPONG_SLAVE,
+	DPU_PINGPONG_DSC,
 	DPU_PINGPONG_DITHER,
 	DPU_PINGPONG_MAX
 };
 
+
+/** DSC sub-blocks/features
+ * @DPU_DSC_OUTPUT_CTRL         Supports the control of the pp id which gets
+ *                              the pixel output from this DSC.
+ * @DPU_DSC_HW_REV_1_1          dsc block supports dsc 1.1 only
+ * @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_ENC,                DSC encoder sub block
+ * @DPU_DSC_CTL,                DSC ctl sub block
+ * @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_ENC,
+	DPU_DSC_CTL,
+	DPU_DSC_MAX
+};
+
 /**
  * CTL sub-blocks
  * @DPU_CTL_SPLIT_DISPLAY:	CTL supports video mode split display
@@ -276,15 +300,6 @@  enum {
 };
 
 /**
- * DSC features
- * @DPU_DSC_OUTPUT_CTRL       Configure which PINGPONG block gets
- *                            the pixel output from this DSC.
- */
-enum {
-	DPU_DSC_OUTPUT_CTRL = 0x1,
-};
-
-/**
  * MACRO DPU_HW_BLK_INFO - information of HW blocks inside DPU
  * @name:              string name for debug purposes
  * @id:                enum identifying this block
@@ -346,6 +361,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 {
@@ -403,6 +426,7 @@  struct dpu_rotation_cfg {
  * @pixel_ram_size     size of latency hiding and de-tiling buffer in bytes
  * @max_hdeci_exp      max horizontal decimation supported (max is 2^value)
  * @max_vdeci_exp      max vertical decimation supported (max is 2^value)
+ * @max_dsc_width      max dsc line width support.
  */
 struct dpu_caps {
 	u32 max_mixer_width;
@@ -419,6 +443,7 @@  struct dpu_caps {
 	u32 pixel_ram_size;
 	u32 max_hdeci_exp;
 	u32 max_vdeci_exp;
+	u32 max_dsc_width;
 };
 
 /**
@@ -494,9 +519,20 @@  struct dpu_dspp_sub_blks {
 struct dpu_pingpong_sub_blks {
 	struct dpu_pp_blk te;
 	struct dpu_pp_blk te2;
+	struct dpu_pp_blk dsc;
 	struct dpu_pp_blk dither;
 };
 
+
+/**
+ * struct dpu_dsc_sub_blks : DSC sub-blks
+ *
+ */
+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
  */
@@ -641,10 +677,14 @@  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
+ * @dsc_pair_mask:     Bitmask of DSCs that can be controlled by same CTL
  */
 struct dpu_dsc_cfg {
 	DPU_HW_BLK_INFO;
+	DECLARE_BITMAP(dsc_pair_mask, DSC_MAX);
+	const struct dpu_dsc_sub_blks *sblk;
 };
 
 /**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
index 619926d..51e8890 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -1,6 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0-only
 /*
  * Copyright (c) 2020-2022, Linaro Limited
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
  */
 
 #include "dpu_kms.h"
@@ -41,10 +42,11 @@  static void dpu_hw_dsc_disable(struct dpu_hw_dsc *dsc)
 static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
 			      struct drm_dsc_config *dsc,
 			      u32 mode,
-			      u32 initial_lines)
+			      u32 initial_lines,
+			      bool ich_reset_override)
 {
 	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
-	u32 data;
+	u32 data, lsb, bpp;
 	u32 slice_last_group_size;
 	u32 det_thresh_flatness;
 	bool is_cmd_mode = !(mode & DSC_MODE_VIDEO);
@@ -58,7 +60,14 @@  static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
 	data = (initial_lines << 20);
 	data |= ((slice_last_group_size - 1) << 18);
 	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
-	data |= (dsc->bits_per_pixel << 8);
+	data |= dsc->bits_per_pixel << 12;
+	lsb = dsc->bits_per_pixel % 4;
+	bpp = dsc->bits_per_pixel / 4;
+	bpp *= 4;
+	bpp <<= 4;
+	bpp |= lsb;
+
+	data |= bpp << 8;
 	data |= (dsc->block_pred_enable << 7);
 	data |= (dsc->line_buf_depth << 3);
 	data |= (dsc->simple_422 << 2);
@@ -221,7 +230,13 @@  struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
 
 	c->idx = idx;
 	c->caps = cfg;
-	_setup_dsc_ops(&c->ops, c->caps->features);
+
+	if (test_bit(DPU_DSC_HW_REV_1_1, &c->caps->features))
+		_setup_dsc_ops(&c->ops, c->caps->features);
+	else if (test_bit(DPU_DSC_HW_REV_1_2, &c->caps->features))
+		dpu_dsc_1_2_setup_ops(&c->ops, c->caps->features);
+	else
+		_setup_dsc_ops(&c->ops, c->caps->features);
 
 	return c;
 }
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 ae9b5db..a48f572 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
@@ -33,7 +36,8 @@  struct dpu_hw_dsc_ops {
 	void (*dsc_config)(struct dpu_hw_dsc *hw_dsc,
 			   struct drm_dsc_config *dsc,
 			   u32 mode,
-			   u32 initial_lines);
+			   u32 initial_lines,
+			   bool ich_reset_override);
 
 	/**
 	 * dsc_config_thresh - programs panel thresholds
@@ -43,6 +47,12 @@  struct dpu_hw_dsc_ops {
 	void (*dsc_config_thresh)(struct dpu_hw_dsc *hw_dsc,
 				  struct drm_dsc_config *dsc);
 
+	/**
+	 * bind_pingpong_blk - enable/disable the connection with pp
+	 * @hw_dsc: Pointer to dsc context
+	 * @enable: enable/disable connection
+	 * @pp: pingpong blk id
+	 */
 	void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
 				  bool enable,
 				  enum dpu_pingpong pp);
@@ -51,6 +61,7 @@  struct dpu_hw_dsc_ops {
 struct dpu_hw_dsc {
 	struct dpu_hw_blk base;
 	struct dpu_hw_blk_reg_map hw;
+	struct dpu_hw_ctl *hw_ctl;
 
 	/* dsc */
 	enum dpu_dsc idx;
@@ -76,9 +87,17 @@  struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
  */
 void dpu_hw_dsc_destroy(struct dpu_hw_dsc *dsc);
 
+/**
+ * dpu_hw_dsc - convert base object dpu_hw_base to container
+ * @hw: Pointer to base hardware block
+ * return: Pointer to hardware block container
+ */
 static inline struct dpu_hw_dsc *to_dpu_hw_dsc(struct dpu_hw_blk *hw)
 {
 	return container_of(hw, struct dpu_hw_dsc, base);
 }
 
+void dpu_dsc_1_2_setup_ops(struct dpu_hw_dsc_ops *ops,
+		const unsigned long features);
+
 #endif /* _DPU_HW_DSC_H */
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..2be74ae
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
@@ -0,0 +1,371 @@ 
+// 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 "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_BUF_THRESH_1        0x6C
+#define DSC_RC_BUF_THRESH_2        0x70
+#define DSC_RC_BUF_THRESH_3        0x74
+#define DSC_RC_MIN_QP_0            0x78
+#define DSC_RC_MIN_QP_1            0x7C
+#define DSC_RC_MIN_QP_2            0x80
+#define DSC_RC_MAX_QP_0            0x84
+#define DSC_RC_MAX_QP_1            0x88
+#define DSC_RC_MAX_QP_2             0x8C
+#define DSC_RC_RANGE_BPG_OFFSETS_0  0x90
+#define DSC_RC_RANGE_BPG_OFFSETS_1  0x94
+#define DSC_RC_RANGE_BPG_OFFSETS_2  0x98
+
+/* 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 int _dsc_calc_ob_max_addr(struct dpu_hw_dsc *hw_dsc, int num_ss)
+{
+	enum dpu_dsc idx;
+
+	idx = hw_dsc->idx;
+
+	if (!(hw_dsc->caps->features & BIT(DPU_DSC_NATIVE_422_EN))) {
+		if (num_ss == 1)
+			return 2399;
+		else if (num_ss == 2)
+			return 1199;
+	} else {
+		if (num_ss == 1)
+			return 1199;
+		else if (num_ss == 2)
+			return 599;
+	}
+	return 0;
+}
+
+static inline int _dsc_subblk_offset(struct dpu_hw_dsc *hw_dsc, int s_id,
+		u32 *idx)
+{
+	const struct dpu_dsc_sub_blks *sblk;
+
+	if (!hw_dsc)
+		return -EINVAL;
+
+	*idx = 0;
+
+	sblk = hw_dsc->caps->sblk;
+
+	switch (s_id) {
+
+	case DPU_DSC_ENC:
+		*idx = sblk->enc.base;
+		break;
+	case DPU_DSC_CTL:
+		*idx = sblk->ctl.base;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void dpu_hw_dsc_disable_1_2(struct dpu_hw_dsc *hw_dsc)
+{
+	struct dpu_hw_blk_reg_map *hw;
+	u32 idx;
+
+	if (!hw_dsc)
+		return;
+
+	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
+		return;
+
+	hw = &hw_dsc->hw;
+	DPU_REG_WRITE(hw, DSC_CFG + idx, 0);
+
+	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
+		return;
+
+	DPU_REG_WRITE(hw, ENC_DF_CTRL + idx, 0);
+	DPU_REG_WRITE(hw, DSC_MAIN_CONF + idx, 0);
+}
+
+static void dpu_hw_dsc_config_1_2(struct dpu_hw_dsc *hw_dsc,
+		struct drm_dsc_config *dsc, u32 mode,
+		u32 initial_lines, bool ich_reset_override)
+{
+	struct dpu_hw_blk_reg_map *hw;
+	struct msm_display_dsc_info *dsc_info;
+	u32 idx;
+	u32 data = 0;
+	u32 bpp;
+	void __iomem *off;
+
+	if (!hw_dsc || !dsc)
+		return;
+
+	hw = &hw_dsc->hw;
+
+	dsc_info = to_msm_dsc_info(dsc);
+
+	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
+		return;
+
+	if (mode & DSC_MODE_SPLIT_PANEL)
+		data |= BIT(0);
+
+	if (mode & DSC_MODE_MULTIPLEX)
+		data |= BIT(1);
+
+	data |= (dsc_info->num_active_ss_per_enc & 0x3) << 7;
+
+	DPU_REG_WRITE(hw, DSC_CMN_MAIN_CNF, data);
+
+	data = (dsc_info->initial_lines & 0xff);
+	data |= ((mode & DSC_MODE_VIDEO) ? 1 : 0) << 9;
+	if (ich_reset_override)
+		data |= 0xC00; // set bit 10 and 11
+	data |= (_dsc_calc_ob_max_addr(hw_dsc, dsc_info->num_active_ss_per_enc) << 18);
+
+	DPU_REG_WRITE(hw, ENC_DF_CTRL + idx, 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, DSC_MAIN_CONF + idx, data);
+
+	data = (dsc->pic_width & 0xffff) |
+		((dsc->pic_height & 0xffff) << 16);
+
+	DPU_REG_WRITE(hw, DSC_PICTURE_SIZE + idx, data);
+
+	data = (dsc->slice_width & 0xffff) |
+		((dsc->slice_height & 0xffff) << 16);
+
+	DPU_REG_WRITE(hw, DSC_SLICE_SIZE + idx, data);
+
+	DPU_REG_WRITE(hw, DSC_MISC_SIZE + idx,
+			(dsc->slice_chunk_size) & 0xffff);
+
+	data = (dsc->initial_xmit_delay & 0xffff) |
+		((dsc->initial_dec_delay & 0xffff) << 16);
+
+	DPU_REG_WRITE(hw, DSC_HRD_DELAYS + idx, data);
+
+	DPU_REG_WRITE(hw, DSC_RC_SCALE + idx,
+			dsc->initial_scale_value & 0x3f);
+
+	data = (dsc->scale_increment_interval & 0xffff) |
+		((dsc->scale_decrement_interval & 0x7ff) << 16);
+
+	DPU_REG_WRITE(hw, DSC_RC_SCALE_INC_DEC + idx, data);
+
+	data = (dsc->first_line_bpg_offset & 0x1f) |
+		((dsc->second_line_bpg_offset & 0x1f) << 5);
+
+	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_1 + idx, data);
+
+	data = (dsc->nfl_bpg_offset & 0xffff) |
+		((dsc->slice_bpg_offset & 0xffff) << 16);
+
+	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_2 + idx, data);
+
+	data = (dsc->initial_offset & 0xffff) |
+		((dsc->final_offset & 0xffff) << 16);
+
+	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_3 + idx, data);
+
+	data = (dsc->nsl_bpg_offset & 0xffff) |
+		((dsc->second_line_offset_adj & 0xffff) << 16);
+
+	DPU_REG_WRITE(hw, DSC_RC_OFFSETS_4 + idx, data);
+
+	data = (dsc->flatness_min_qp & 0x1f);
+	data |= (dsc->flatness_max_qp & 0x1f) << 5;
+	data |= (dsc_info->det_thresh_flatness & 0xff) << 10;
+
+	DPU_REG_WRITE(hw, DSC_FLATNESS_QP + idx, data);
+
+	DPU_REG_WRITE(hw, DSC_RC_MODEL_SIZE + idx,
+			(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, DSC_RC_CONFIG + idx, data);
+
+	/* program the dsc wrapper */
+	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
+		return;
+
+	off = hw->blk_addr + idx;
+
+	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);
+
+	if (dsc_info->dsc_4hsmerge_en) {
+		data |= dsc_info->dsc_4hsmerge_padding << 18;
+		data |= dsc_info->dsc_4hsmerge_alignment << 22;
+		data |= BIT(16);
+	}
+
+	DPU_REG_WRITE(hw, DSC_CFG + idx, data);
+
+//	DPU_REG_WRITE(hw, DSC_DATA_IN_SWAP + idx, 0x14e5);
+}
+
+static 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;
+	struct msm_display_dsc_info *dsc_info;
+	u32 idx, off;
+	int i, j = 0;
+	struct drm_dsc_rc_range_parameters *rc;
+	u32 data = 0, min_qp = 0, max_qp = 0, bpg_off = 0;
+
+	if (!hw_dsc || !dsc)
+		return;
+
+	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_ENC, &idx))
+		return;
+
+	hw = &hw_dsc->hw;
+
+	dsc_info = to_msm_dsc_info(dsc);
+
+	rc = dsc->rc_range_params;
+
+	off = 0;
+	for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
+		data |= dsc->rc_buf_thresh[i] << (8*j);
+		j++;
+		if ((j == 4) || (i == DSC_NUM_BUF_RANGES - 2)) {
+			DPU_REG_WRITE(hw, DSC_RC_BUF_THRESH_0 + idx + off,
+					data);
+			off += 4;
+			j = 0;
+			data = 0;
+		}
+	}
+
+	off = 0;
+	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
+		min_qp |= (rc[i].range_min_qp & 0x1f) << 5*j;
+		max_qp |= (rc[i].range_max_qp & 0x1f) << 5*j;
+		bpg_off |= (rc[i].range_bpg_offset & 0x3f) << 6*j;
+		j++;
+		if (j == 5) {
+			DPU_REG_WRITE(hw, DSC_RC_MIN_QP_0 + idx + off,
+					min_qp);
+			DPU_REG_WRITE(hw, DSC_RC_MAX_QP_0 + idx + off,
+					max_qp);
+			DPU_REG_WRITE(hw,
+					DSC_RC_RANGE_BPG_OFFSETS_0 + idx + off,
+					bpg_off);
+			off += 4;
+			j = 0;
+			min_qp = 0;
+			max_qp = 0;
+			bpg_off = 0;
+		}
+	}
+}
+
+static void dpu_hw_dsc_bind_pingpong_blk_1_2(
+		struct dpu_hw_dsc *hw_dsc,
+		bool enable,
+		const enum dpu_pingpong pp)
+{
+	struct dpu_hw_blk_reg_map *hw;
+	int idx;
+	int mux_cfg = 0xF; /* Disabled */
+
+	if (!hw_dsc)
+		return;
+
+	if (_dsc_subblk_offset(hw_dsc, DPU_DSC_CTL, &idx))
+		return;
+
+	hw = &hw_dsc->hw;
+	if (enable)
+		mux_cfg = (pp - PINGPONG_0) & 0x7;
+
+	DPU_REG_WRITE(hw, DSC_CTL + idx, mux_cfg);
+}
+
+void dpu_dsc_1_2_setup_ops(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;
+}