diff mbox series

[v3,12/13] drm/msm/dsi: Add support for DSC configuration

Message ID 20211116062256.2417186-13-vkoul@kernel.org (mailing list archive)
State New, archived
Headers show
Series drm/msm: Add Display Stream Compression Support | expand

Commit Message

Vinod Koul Nov. 16, 2021, 6:22 a.m. UTC
When DSC is enabled, we need to configure DSI registers accordingly and
configure the respective stream compression registers.

Add support to calculate the register setting based on DSC params and
timing information and configure these registers.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/gpu/drm/msm/dsi/dsi.xml.h  |  10 +++
 drivers/gpu/drm/msm/dsi/dsi_host.c | 113 ++++++++++++++++++++++++++++-
 2 files changed, 122 insertions(+), 1 deletion(-)

Comments

kernel test robot Nov. 18, 2021, 9:47 p.m. UTC | #1
Hi Vinod,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v5.16-rc1 next-20211118]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vinod-Koul/drm-msm-Add-Display-Stream-Compression-Support/20211116-142602
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: arm64-randconfig-r036-20211118 (attached as .config)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/0d90631d88b4295b0612892e62110dd3b11c9d78
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vinod-Koul/drm-msm-Add-Display-Stream-Compression-Support/20211116-142602
        git checkout 0d90631d88b4295b0612892e62110dd3b11c9d78
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/msm/dsi/dsi_host.c:1039:13: warning: variable 'reg_ctrl' set but not used [-Wunused-but-set-variable]
                           u32 reg, reg_ctrl, reg_ctrl2;
                                    ^
   1 warning generated.


vim +/reg_ctrl +1039 drivers/gpu/drm/msm/dsi/dsi_host.c

   924	
   925	static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
   926	{
   927		struct drm_display_mode *mode = msm_host->mode;
   928		u32 hs_start = 0, vs_start = 0; /* take sync start as 0 */
   929		u32 h_total = mode->htotal;
   930		u32 v_total = mode->vtotal;
   931		u32 hs_end = mode->hsync_end - mode->hsync_start;
   932		u32 vs_end = mode->vsync_end - mode->vsync_start;
   933		u32 ha_start = h_total - mode->hsync_start;
   934		u32 ha_end = ha_start + mode->hdisplay;
   935		u32 va_start = v_total - mode->vsync_start;
   936		u32 va_end = va_start + mode->vdisplay;
   937		u32 hdisplay = mode->hdisplay;
   938		u32 wc;
   939	
   940		DBG("");
   941	
   942		/*
   943		 * For bonded DSI mode, the current DRM mode has
   944		 * the complete width of the panel. Since, the complete
   945		 * panel is driven by two DSI controllers, the horizontal
   946		 * timings have to be split between the two dsi controllers.
   947		 * Adjust the DSI host timing values accordingly.
   948		 */
   949		if (is_bonded_dsi) {
   950			h_total /= 2;
   951			hs_end /= 2;
   952			ha_start /= 2;
   953			ha_end /= 2;
   954			hdisplay /= 2;
   955		}
   956	
   957		if (msm_host->dsc) {
   958			struct msm_display_dsc_config *dsc = msm_host->dsc;
   959	
   960			/* update dsc params with timing params */
   961			dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
   962			DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);
   963	
   964			/* we do the calculations for dsc parameters here so that
   965			 * panel can use these parameters
   966			 */
   967			dsi_populate_dsc_params(dsc);
   968	
   969			/* Divide the display by 3 but keep back/font porch and
   970			 * pulse width same
   971			 */
   972			h_total -= hdisplay;
   973			hdisplay /= 3;
   974			h_total += hdisplay;
   975			ha_end = ha_start + hdisplay;
   976		}
   977	
   978		if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
   979			if (msm_host->dsc) {
   980				struct msm_display_dsc_config *dsc = msm_host->dsc;
   981				u32 reg, intf_width, slice_per_intf;
   982				u32 total_bytes_per_intf;
   983	
   984				/* first calculate dsc parameters and then program
   985				 * compress mode registers
   986				 */
   987				intf_width = hdisplay;
   988				slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
   989	
   990				dsc->drm->slice_count = 1;
   991				dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
   992				total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
   993	
   994				dsc->eol_byte_num = total_bytes_per_intf % 3;
   995				dsc->pclk_per_line =  DIV_ROUND_UP(total_bytes_per_intf, 3);
   996				dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
   997				dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
   998	
   999				reg = dsc->bytes_per_pkt << 16;
  1000				reg |= (0x0b << 8);    /* dtype of compressed image */
  1001	
  1002				/* pkt_per_line:
  1003				 * 0 == 1 pkt
  1004				 * 1 == 2 pkt
  1005				 * 2 == 4 pkt
  1006				 * 3 pkt is not supported
  1007				 * above translates to ffs() - 1
  1008				 */
  1009				reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
  1010	
  1011				dsc->eol_byte_num = total_bytes_per_intf % 3;
  1012				reg |= dsc->eol_byte_num << 4;
  1013				reg |= 1;
  1014	
  1015				dsi_write(msm_host,
  1016					  REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
  1017			}
  1018	
  1019			dsi_write(msm_host, REG_DSI_ACTIVE_H,
  1020				DSI_ACTIVE_H_START(ha_start) |
  1021				DSI_ACTIVE_H_END(ha_end));
  1022			dsi_write(msm_host, REG_DSI_ACTIVE_V,
  1023				DSI_ACTIVE_V_START(va_start) |
  1024				DSI_ACTIVE_V_END(va_end));
  1025			dsi_write(msm_host, REG_DSI_TOTAL,
  1026				DSI_TOTAL_H_TOTAL(h_total - 1) |
  1027				DSI_TOTAL_V_TOTAL(v_total - 1));
  1028	
  1029			dsi_write(msm_host, REG_DSI_ACTIVE_HSYNC,
  1030				DSI_ACTIVE_HSYNC_START(hs_start) |
  1031				DSI_ACTIVE_HSYNC_END(hs_end));
  1032			dsi_write(msm_host, REG_DSI_ACTIVE_VSYNC_HPOS, 0);
  1033			dsi_write(msm_host, REG_DSI_ACTIVE_VSYNC_VPOS,
  1034				DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
  1035				DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
  1036		} else {		/* command mode */
  1037			if (msm_host->dsc) {
  1038				struct msm_display_dsc_config *dsc = msm_host->dsc;
> 1039				u32 reg, reg_ctrl, reg_ctrl2;
  1040				u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
  1041	
  1042				reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
  1043				reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
  1044	
  1045				slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
  1046				bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
  1047							      dsc->drm->bits_per_pixel, 8);
  1048				dsc->drm->slice_chunk_size = bytes_in_slice;
  1049				total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
  1050				dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
  1051	
  1052				reg = 0x39 << 8;
  1053				reg |= ffs(dsc->pkt_per_line) << 6;
  1054	
  1055				dsc->eol_byte_num = total_bytes_per_intf % 3;
  1056				reg |= dsc->eol_byte_num << 4;
  1057				reg |= 1;
  1058	
  1059				reg_ctrl |= reg;
  1060				reg_ctrl2 |= bytes_in_slice;
  1061	
  1062				dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
  1063				dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
  1064			}
  1065	
  1066			/* image data and 1 byte write_memory_start cmd */
  1067			if (!msm_host->dsc)
  1068				wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
  1069			else
  1070				wc = mode->hdisplay / 2 + 1;
  1071	
  1072			dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
  1073				DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
  1074				DSI_CMD_MDP_STREAM0_CTRL_VIRTUAL_CHANNEL(
  1075						msm_host->channel) |
  1076				DSI_CMD_MDP_STREAM0_CTRL_DATA_TYPE(
  1077						MIPI_DSI_DCS_LONG_WRITE));
  1078	
  1079			dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
  1080				DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
  1081				DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
  1082		}
  1083	}
  1084	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Dmitry Baryshkov Nov. 24, 2021, 4:21 p.m. UTC | #2
On 16/11/2021 09:22, Vinod Koul wrote:
> When DSC is enabled, we need to configure DSI registers accordingly and
> configure the respective stream compression registers.
> 
> Add support to calculate the register setting based on DSC params and
> timing information and configure these registers.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>   drivers/gpu/drm/msm/dsi/dsi.xml.h  |  10 +++
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 113 ++++++++++++++++++++++++++++-
>   2 files changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> index 49b551ad1bff..c1c85df58c4b 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> @@ -706,4 +706,14 @@ static inline uint32_t DSI_VERSION_MAJOR(uint32_t val)
>   #define REG_DSI_CPHY_MODE_CTRL					0x000002d4
>   
>   
> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL			0x0000029c
> +
> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL2			0x000002a0
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL			0x000002a4
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2			0x000002a8
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL3			0x000002ac
> +
>   #endif /* DSI_XML */
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 31d385d8d834..2c14c36f0b3d 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -908,6 +908,20 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
>   		dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
>   }
>   
> +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> +				  int pic_width, int pic_height)
> +{
> +	if (!dsc || !pic_width || !pic_height) {
> +		pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> +		return -EINVAL;
> +	}
> +
> +	dsc->drm->pic_width = pic_width;
> +	dsc->drm->pic_height = pic_height;
> +
> +	return 0;
> +}
> +
>   static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   {
>   	struct drm_display_mode *mode = msm_host->mode;
> @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   		hdisplay /= 2;
>   	}
>   
> +	if (msm_host->dsc) {
> +		struct msm_display_dsc_config *dsc = msm_host->dsc;
> +
> +		/* update dsc params with timing params */
> +		dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> +		DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);
> +
> +		/* we do the calculations for dsc parameters here so that
> +		 * panel can use these parameters
> +		 */
> +		dsi_populate_dsc_params(dsc);
> +
> +		/* Divide the display by 3 but keep back/font porch and
> +		 * pulse width same
> +		 */
> +		h_total -= hdisplay;
> +		hdisplay /= 3;
> +		h_total += hdisplay;
> +		ha_end = ha_start + hdisplay;
> +	}
> +
>   	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> +		if (msm_host->dsc) {
> +			struct msm_display_dsc_config *dsc = msm_host->dsc;
> +			u32 reg, intf_width, slice_per_intf;
> +			u32 total_bytes_per_intf;
> +
> +			/* first calculate dsc parameters and then program
> +			 * compress mode registers
> +			 */
> +			intf_width = hdisplay;
> +			slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
> +
> +			dsc->drm->slice_count = 1;
> +			dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
> +			total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> +
> +			dsc->eol_byte_num = total_bytes_per_intf % 3;
> +			dsc->pclk_per_line =  DIV_ROUND_UP(total_bytes_per_intf, 3);
> +			dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
> +			dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> +
> +			reg = dsc->bytes_per_pkt << 16;
> +			reg |= (0x0b << 8);    /* dtype of compressed image */
> +
> +			/* pkt_per_line:
> +			 * 0 == 1 pkt
> +			 * 1 == 2 pkt
> +			 * 2 == 4 pkt
> +			 * 3 pkt is not supported
> +			 * above translates to ffs() - 1
> +			 */
> +			reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
> +
> +			dsc->eol_byte_num = total_bytes_per_intf % 3;
> +			reg |= dsc->eol_byte_num << 4;
> +			reg |= 1;
> +
> +			dsi_write(msm_host,
> +				  REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> +		}
> +
>   		dsi_write(msm_host, REG_DSI_ACTIVE_H,
>   			DSI_ACTIVE_H_START(ha_start) |
>   			DSI_ACTIVE_H_END(ha_end));
> @@ -959,8 +1034,40 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   			DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
>   			DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
>   	} else {		/* command mode */
> +		if (msm_host->dsc) {
> +			struct msm_display_dsc_config *dsc = msm_host->dsc;
> +			u32 reg, reg_ctrl, reg_ctrl2;
> +			u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
> +
> +			reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> +			reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> +
> +			slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
> +			bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
> +						      dsc->drm->bits_per_pixel, 8);
> +			dsc->drm->slice_chunk_size = bytes_in_slice;
> +			total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> +			dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> +
> +			reg = 0x39 << 8;
> +			reg |= ffs(dsc->pkt_per_line) << 6;
> +
> +			dsc->eol_byte_num = total_bytes_per_intf % 3;
> +			reg |= dsc->eol_byte_num << 4;
> +			reg |= 1;
> +
> +			reg_ctrl |= reg;
> +			reg_ctrl2 |= bytes_in_slice;
> +
> +			dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> +			dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> +		}
> +
>   		/* image data and 1 byte write_memory_start cmd */
> -		wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> +		if (!msm_host->dsc)
> +			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> +		else
> +			wc = mode->hdisplay / 2 + 1;
>   
>   		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>   			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
> @@ -2051,9 +2158,13 @@ int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>   {
>   	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>   	const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
> +	struct msm_drm_private *priv;
>   	int ret;
>   
>   	msm_host->dev = dev;
> +	priv = dev->dev_private;
> +	priv->dsc = msm_host->dsc;

I have been thinking about this piece. I highly dislike the priv->dsc 
field for multiple reasons.

Please correct me if I'm wrong, we use it for several reasons:
- to check if DSC is requested at all
- to store the dsc_mask

The DSC mask should be calculated basing on dpu_encoder_virt->hw_dsc[] 
values, so it can be removed from msm_display_dsc_config.

To check whether DSC is enabled, I'd suggest the following:

- Add use_dsc flag to struct msm_display_info.
   This way it would be generic to all possible encoders which can use DSC.

- Add struct msm_dsi_has_dsc_panel() function.
   It checks whether msm_host has ->dsc data. Feel free to change the 
name of the function to better suit your style.

- Call msm_dsi_has_dsc_panel() from _dpu_kms_initialize_dsi().
   If DSC is requested, set info->use_dsc.

- In dpu_encoder_setup store use_dsc in struct dpu_encoder_virt() and 
use it later instead of checking priv->dsc.

WDYT?


> +
>   	ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K);
>   	if (ret) {
>   		pr_err("%s: alloc tx gem obj failed, %d\n", __func__, ret);
>
Dmitry Baryshkov Nov. 24, 2021, 4:27 p.m. UTC | #3
On 24/11/2021 19:21, Dmitry Baryshkov wrote:
> On 16/11/2021 09:22, Vinod Koul wrote:
>> When DSC is enabled, we need to configure DSI registers accordingly and
>> configure the respective stream compression registers.
>>
>> Add support to calculate the register setting based on DSC params and
>> timing information and configure these registers.
>>
>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi.xml.h  |  10 +++
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 113 ++++++++++++++++++++++++++++-
>>   2 files changed, 122 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h 
>> b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> index 49b551ad1bff..c1c85df58c4b 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> @@ -706,4 +706,14 @@ static inline uint32_t DSI_VERSION_MAJOR(uint32_t 
>> val)
>>   #define REG_DSI_CPHY_MODE_CTRL                    0x000002d4
>> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL            0x0000029c
>> +
>> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL2            0x000002a0
>> +
>> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL            0x000002a4
>> +
>> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2            0x000002a8
>> +
>> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL3            0x000002ac
>> +
>>   #endif /* DSI_XML */
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 31d385d8d834..2c14c36f0b3d 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -908,6 +908,20 @@ static void dsi_ctrl_config(struct msm_dsi_host 
>> *msm_host, bool enable,
>>           dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
>>   }
>> +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
>> +                  int pic_width, int pic_height)
>> +{
>> +    if (!dsc || !pic_width || !pic_height) {
>> +        pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", 
>> pic_width, pic_height);
>> +        return -EINVAL;
>> +    }
>> +
>> +    dsc->drm->pic_width = pic_width;
>> +    dsc->drm->pic_height = pic_height;
>> +
>> +    return 0;
>> +}
>> +
>>   static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool 
>> is_bonded_dsi)
>>   {
>>       struct drm_display_mode *mode = msm_host->mode;
>> @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>           hdisplay /= 2;
>>       }
>> +    if (msm_host->dsc) {
>> +        struct msm_display_dsc_config *dsc = msm_host->dsc;
>> +
>> +        /* update dsc params with timing params */
>> +        dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
>> +        DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, 
>> dsc->drm->pic_height);
>> +
>> +        /* we do the calculations for dsc parameters here so that
>> +         * panel can use these parameters
>> +         */
>> +        dsi_populate_dsc_params(dsc);
>> +
>> +        /* Divide the display by 3 but keep back/font porch and
>> +         * pulse width same
>> +         */
>> +        h_total -= hdisplay;
>> +        hdisplay /= 3;
>> +        h_total += hdisplay;
>> +        ha_end = ha_start + hdisplay;
>> +    }
>> +
>>       if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
>> +        if (msm_host->dsc) {
>> +            struct msm_display_dsc_config *dsc = msm_host->dsc;
>> +            u32 reg, intf_width, slice_per_intf;
>> +            u32 total_bytes_per_intf;
>> +
>> +            /* first calculate dsc parameters and then program
>> +             * compress mode registers
>> +             */
>> +            intf_width = hdisplay;
>> +            slice_per_intf = DIV_ROUND_UP(intf_width, 
>> dsc->drm->slice_width);
>> +
>> +            dsc->drm->slice_count = 1;
>> +            dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width 
>> * 8, 8);
>> +            total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
>> +
>> +            dsc->eol_byte_num = total_bytes_per_intf % 3;
>> +            dsc->pclk_per_line =  DIV_ROUND_UP(total_bytes_per_intf, 3);
>> +            dsc->bytes_per_pkt = dsc->bytes_in_slice * 
>> dsc->drm->slice_count;
>> +            dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
>> +
>> +            reg = dsc->bytes_per_pkt << 16;
>> +            reg |= (0x0b << 8);    /* dtype of compressed image */
>> +
>> +            /* pkt_per_line:
>> +             * 0 == 1 pkt
>> +             * 1 == 2 pkt
>> +             * 2 == 4 pkt
>> +             * 3 pkt is not supported
>> +             * above translates to ffs() - 1
>> +             */
>> +            reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
>> +
>> +            dsc->eol_byte_num = total_bytes_per_intf % 3;
>> +            reg |= dsc->eol_byte_num << 4;
>> +            reg |= 1;
>> +
>> +            dsi_write(msm_host,
>> +                  REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
>> +        }
>> +
>>           dsi_write(msm_host, REG_DSI_ACTIVE_H,
>>               DSI_ACTIVE_H_START(ha_start) |
>>               DSI_ACTIVE_H_END(ha_end));
>> @@ -959,8 +1034,40 @@ static void dsi_timing_setup(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>               DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
>>               DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
>>       } else {        /* command mode */
>> +        if (msm_host->dsc) {
>> +            struct msm_display_dsc_config *dsc = msm_host->dsc;
>> +            u32 reg, reg_ctrl, reg_ctrl2;
>> +            u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
>> +
>> +            reg_ctrl = dsi_read(msm_host, 
>> REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
>> +            reg_ctrl2 = dsi_read(msm_host, 
>> REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
>> +
>> +            slice_per_intf = DIV_ROUND_UP(hdisplay, 
>> dsc->drm->slice_width);
>> +            bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
>> +                              dsc->drm->bits_per_pixel, 8);
>> +            dsc->drm->slice_chunk_size = bytes_in_slice;
>> +            total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
>> +            dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
>> +
>> +            reg = 0x39 << 8;
>> +            reg |= ffs(dsc->pkt_per_line) << 6;
>> +
>> +            dsc->eol_byte_num = total_bytes_per_intf % 3;
>> +            reg |= dsc->eol_byte_num << 4;
>> +            reg |= 1;
>> +
>> +            reg_ctrl |= reg;
>> +            reg_ctrl2 |= bytes_in_slice;
>> +
>> +            dsi_write(msm_host, 
>> REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
>> +            dsi_write(msm_host, 
>> REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
>> +        }
>> +
>>           /* image data and 1 byte write_memory_start cmd */
>> -        wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>> +        if (!msm_host->dsc)
>> +            wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>> +        else
>> +            wc = mode->hdisplay / 2 + 1;
>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>> @@ -2051,9 +2158,13 @@ int msm_dsi_host_modeset_init(struct 
>> mipi_dsi_host *host,
>>   {
>>       struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>>       const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
>> +    struct msm_drm_private *priv;
>>       int ret;
>>       msm_host->dev = dev;
>> +    priv = dev->dev_private;
>> +    priv->dsc = msm_host->dsc;
> 
> I have been thinking about this piece. I highly dislike the priv->dsc 
> field for multiple reasons.
> 
> Please correct me if I'm wrong, we use it for several reasons:
> - to check if DSC is requested at all
> - to store the dsc_mask
> 
> The DSC mask should be calculated basing on dpu_encoder_virt->hw_dsc[] 
> values, so it can be removed from msm_display_dsc_config.
> 
> To check whether DSC is enabled, I'd suggest the following:
> 
> - Add use_dsc flag to struct msm_display_info.
>    This way it would be generic to all possible encoders which can use DSC.
> 
> - Add struct msm_dsi_has_dsc_panel() function.
>    It checks whether msm_host has ->dsc data. Feel free to change the 
> name of the function to better suit your style.
> 
> - Call msm_dsi_has_dsc_panel() from _dpu_kms_initialize_dsi().
>    If DSC is requested, set info->use_dsc.
> 
> - In dpu_encoder_setup store use_dsc in struct dpu_encoder_virt() and 
> use it later instead of checking priv->dsc.

I forgot about the patch 2, which actually uses priv->dsc data. The 
overall idea would be the same, get data pointer from msm_dsi and pass 
it through the msm_display_info.

> 
> WDYT?
> 
> 
>> +
>>       ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K);
>>       if (ret) {
>>           pr_err("%s: alloc tx gem obj failed, %d\n", __func__, ret);
>>
> 
>
Marijn Suijten Dec. 11, 2021, 12:03 a.m. UTC | #4
Hi Vinod,

On 2021-11-16 11:52:55, Vinod Koul wrote:
> When DSC is enabled, we need to configure DSI registers accordingly and
> configure the respective stream compression registers.
> 
> Add support to calculate the register setting based on DSC params and
> timing information and configure these registers.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.xml.h  |  10 +++
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 113 ++++++++++++++++++++++++++++-
>  2 files changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> index 49b551ad1bff..c1c85df58c4b 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> @@ -706,4 +706,14 @@ static inline uint32_t DSI_VERSION_MAJOR(uint32_t val)
>  #define REG_DSI_CPHY_MODE_CTRL					0x000002d4
>  
>  
> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL			0x0000029c
> +
> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL2			0x000002a0
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL			0x000002a4
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2			0x000002a8
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL3			0x000002ac

I presume you are aware that these files are autogenerated, but there
does not seem to be any link to patches adding these registers to the
XML files in either envytools to mesa, nor could I find any merge/pull
requests on the matter.  Would you mind posting those?  Before doing so
though, consider the comment below about register mapping.

> +
>  #endif /* DSI_XML */
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 31d385d8d834..2c14c36f0b3d 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -908,6 +908,20 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
>  		dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
>  }
>  
> +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> +				  int pic_width, int pic_height)

This function - adopted from downstream - does not seem to perform a
whole lot, especially without the modulo checks against the slice size.
Perhaps it can be inlined?

> +{
> +	if (!dsc || !pic_width || !pic_height) {
> +		pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> +		return -EINVAL;
> +	}
> +
> +	dsc->drm->pic_width = pic_width;
> +	dsc->drm->pic_height = pic_height;
> +
> +	return 0;
> +}
> +
>  static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  {
>  	struct drm_display_mode *mode = msm_host->mode;
> @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  		hdisplay /= 2;
>  	}
>  
> +	if (msm_host->dsc) {
> +		struct msm_display_dsc_config *dsc = msm_host->dsc;
> +
> +		/* update dsc params with timing params */
> +		dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> +		DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);

This seems to be pretty non-standard and perhaps unnecessary debug code,
with a stray dash in there.  Is is needed here, and if so how about
using %dx%d\n to format width and height?

> +
> +		/* we do the calculations for dsc parameters here so that
> +		 * panel can use these parameters
> +		 */
> +		dsi_populate_dsc_params(dsc);
> +
> +		/* Divide the display by 3 but keep back/font porch and
> +		 * pulse width same
> +		 */

A more general nit on the comments in this patch series: it is
appreciated if comments explain the rationale rather than - or in
addition to - merely paraphrasing the code that follows.

> +		h_total -= hdisplay;
> +		hdisplay /= 3;
> +		h_total += hdisplay;
> +		ha_end = ha_start + hdisplay;
> +	}
> +
>  	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> +		if (msm_host->dsc) {
> +			struct msm_display_dsc_config *dsc = msm_host->dsc;
> +			u32 reg, intf_width, slice_per_intf;
> +			u32 total_bytes_per_intf;
> +
> +			/* first calculate dsc parameters and then program
> +			 * compress mode registers
> +			 */
> +			intf_width = hdisplay;
> +			slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
> +
> +			dsc->drm->slice_count = 1;
> +			dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);

If I am not mistaken this is the same value as dsc->drm->slice_width,
since a multiple of 8 is inherently "a multiple of 8" and hence needs no
rounding when divided by 8 again.

Also note that the cmdmode variant below uses bits_per_pixel here; is
that discrepancy intended?

> +			total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> +
> +			dsc->eol_byte_num = total_bytes_per_intf % 3;
> +			dsc->pclk_per_line =  DIV_ROUND_UP(total_bytes_per_intf, 3);
> +			dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
> +			dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> +
> +			reg = dsc->bytes_per_pkt << 16;
> +			reg |= (0x0b << 8);    /* dtype of compressed image */
> +
> +			/* pkt_per_line:
> +			 * 0 == 1 pkt
> +			 * 1 == 2 pkt
> +			 * 2 == 4 pkt
> +			 * 3 pkt is not supported
> +			 * above translates to ffs() - 1
> +			 */
> +			reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
> +
> +			dsc->eol_byte_num = total_bytes_per_intf % 3;

This was already calculated and assigned just a couple lines above.

> +			reg |= dsc->eol_byte_num << 4;
> +			reg |= 1;

Note that the XML register file exists to map out the layout of these
registers, including bit offset, size, and (enum) constant values.  It
is appreciated if you can replace all these magical shifts and magic
flags/bits with the appropriate enum constants and constructor
functions, after mapping them out in the XML file.

> +
> +			dsi_write(msm_host,
> +				  REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> +		}
> +
>  		dsi_write(msm_host, REG_DSI_ACTIVE_H,
>  			DSI_ACTIVE_H_START(ha_start) |
>  			DSI_ACTIVE_H_END(ha_end));
> @@ -959,8 +1034,40 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  			DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
>  			DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
>  	} else {		/* command mode */
> +		if (msm_host->dsc) {
> +			struct msm_display_dsc_config *dsc = msm_host->dsc;
> +			u32 reg, reg_ctrl, reg_ctrl2;
> +			u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
> +
> +			reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> +			reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);

Shouldn't old values be masked out first, before writing new bits or
values below?  The video-mode variant doesn't read back old register
values.

> +
> +			slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
> +			bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
> +						      dsc->drm->bits_per_pixel, 8);
> +			dsc->drm->slice_chunk_size = bytes_in_slice;
> +			total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> +			dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> +
> +			reg = 0x39 << 8;

Same comment about moving magic constants and shifts into the XML file.

> +			reg |= ffs(dsc->pkt_per_line) << 6;

Doesn't the calculation need -1 here just like video mode?

Thanks!

- Marijn

> +
> +			dsc->eol_byte_num = total_bytes_per_intf % 3;
> +			reg |= dsc->eol_byte_num << 4;
> +			reg |= 1;
> +
> +			reg_ctrl |= reg;
> +			reg_ctrl2 |= bytes_in_slice;
> +
> +			dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> +			dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> +		}
> +
>  		/* image data and 1 byte write_memory_start cmd */
> -		wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> +		if (!msm_host->dsc)
> +			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> +		else
> +			wc = mode->hdisplay / 2 + 1;
>  
>  		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>  			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
> @@ -2051,9 +2158,13 @@ int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>  {
>  	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>  	const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
> +	struct msm_drm_private *priv;
>  	int ret;
>  
>  	msm_host->dev = dev;
> +	priv = dev->dev_private;
> +	priv->dsc = msm_host->dsc;
> +
>  	ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K);
>  	if (ret) {
>  		pr_err("%s: alloc tx gem obj failed, %d\n", __func__, ret);
> -- 
> 2.31.1
>
Vinod Koul Feb. 17, 2022, 11:14 a.m. UTC | #5
Hi Marijn,

On 11-12-21, 01:03, Marijn Suijten wrote:

> > +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> > +				  int pic_width, int pic_height)
> 
> This function - adopted from downstream - does not seem to perform a
> whole lot, especially without the modulo checks against the slice size.
> Perhaps it can be inlined?

Most of the code here is :)

This was split from downstream code to check and update dimension. We
can inline this, or should we leave that to compiler. I am not a very
big fan of inlining...

> 
> > +{
> > +	if (!dsc || !pic_width || !pic_height) {
> > +		pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> > +		return -EINVAL;
> > +	}
> > +
> > +	dsc->drm->pic_width = pic_width;
> > +	dsc->drm->pic_height = pic_height;
> > +
> > +	return 0;
> > +}
> > +
> >  static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >  {
> >  	struct drm_display_mode *mode = msm_host->mode;
> > @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >  		hdisplay /= 2;
> >  	}
> >  
> > +	if (msm_host->dsc) {
> > +		struct msm_display_dsc_config *dsc = msm_host->dsc;
> > +
> > +		/* update dsc params with timing params */
> > +		dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> > +		DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);
> 
> This seems to be pretty non-standard and perhaps unnecessary debug code,
> with a stray dash in there.  Is is needed here, and if so how about
> using %dx%d\n to format width and height?

I can update that, sure...

> 
> > +
> > +		/* we do the calculations for dsc parameters here so that
> > +		 * panel can use these parameters
> > +		 */
> > +		dsi_populate_dsc_params(dsc);
> > +
> > +		/* Divide the display by 3 but keep back/font porch and
> > +		 * pulse width same
> > +		 */
> 
> A more general nit on the comments in this patch series: it is
> appreciated if comments explain the rationale rather than - or in
> addition to - merely paraphrasing the code that follows.

Yes it might be the case here, but in this case I wanted to explicitly
point out hat we need to divide display by 3...

> 
> > +		h_total -= hdisplay;
> > +		hdisplay /= 3;
> > +		h_total += hdisplay;
> > +		ha_end = ha_start + hdisplay;
> > +	}
> > +
> >  	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> > +		if (msm_host->dsc) {
> > +			struct msm_display_dsc_config *dsc = msm_host->dsc;
> > +			u32 reg, intf_width, slice_per_intf;
> > +			u32 total_bytes_per_intf;
> > +
> > +			/* first calculate dsc parameters and then program
> > +			 * compress mode registers
> > +			 */
> > +			intf_width = hdisplay;
> > +			slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
> > +
> > +			dsc->drm->slice_count = 1;
> > +			dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
> 
> If I am not mistaken this is the same value as dsc->drm->slice_width,
> since a multiple of 8 is inherently "a multiple of 8" and hence needs no
> rounding when divided by 8 again.

Yes this doesnt look right, I will update

> Also note that the cmdmode variant below uses bits_per_pixel here; is
> that discrepancy intended?

Nope both should use bits_per_pixel..

> > +			total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> > +
> > +			dsc->eol_byte_num = total_bytes_per_intf % 3;
> > +			dsc->pclk_per_line =  DIV_ROUND_UP(total_bytes_per_intf, 3);
> > +			dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
> > +			dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> > +
> > +			reg = dsc->bytes_per_pkt << 16;
> > +			reg |= (0x0b << 8);    /* dtype of compressed image */
> > +
> > +			/* pkt_per_line:
> > +			 * 0 == 1 pkt
> > +			 * 1 == 2 pkt
> > +			 * 2 == 4 pkt
> > +			 * 3 pkt is not supported
> > +			 * above translates to ffs() - 1
> > +			 */
> > +			reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
> > +
> > +			dsc->eol_byte_num = total_bytes_per_intf % 3;
> 
> This was already calculated and assigned just a couple lines above.

Yup, dropped now.

> 
> > +			reg |= dsc->eol_byte_num << 4;
> > +			reg |= 1;
> 
> Note that the XML register file exists to map out the layout of these
> registers, including bit offset, size, and (enum) constant values.  It
> is appreciated if you can replace all these magical shifts and magic
> flags/bits with the appropriate enum constants and constructor
> functions, after mapping them out in the XML file.

Yeah I am trying to get those details, if I manage to get it, will
update for sure as Dmitry already pointed in MESA PR.

> > +
> > +			dsi_write(msm_host,
> > +				  REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> > +		}
> > +
> >  		dsi_write(msm_host, REG_DSI_ACTIVE_H,
> >  			DSI_ACTIVE_H_START(ha_start) |
> >  			DSI_ACTIVE_H_END(ha_end));
> > @@ -959,8 +1034,40 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >  			DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
> >  			DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
> >  	} else {		/* command mode */
> > +		if (msm_host->dsc) {
> > +			struct msm_display_dsc_config *dsc = msm_host->dsc;
> > +			u32 reg, reg_ctrl, reg_ctrl2;
> > +			u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
> > +
> > +			reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> > +			reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> 
> Shouldn't old values be masked out first, before writing new bits or
> values below?  The video-mode variant doesn't read back old register
> values.

This follows downstream where the registers are read, modified and
written back

> > +
> > +			slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
> > +			bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
> > +						      dsc->drm->bits_per_pixel, 8);
> > +			dsc->drm->slice_chunk_size = bytes_in_slice;
> > +			total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> > +			dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> > +
> > +			reg = 0x39 << 8;
> 
> Same comment about moving magic constants and shifts into the XML file.

yes if we get details of bits

> 
> > +			reg |= ffs(dsc->pkt_per_line) << 6;
> 
> Doesn't the calculation need -1 here just like video mode?

yes will update now
Marijn Suijten Feb. 17, 2022, 3:11 p.m. UTC | #6
Hi Vinod,

Thanks for taking time to go through this review, please find some
clarifications below.

On 2022-02-17 16:44:04, Vinod Koul wrote:
> Hi Marijn,
> 
> On 11-12-21, 01:03, Marijn Suijten wrote:
> 
> > > +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> > > +				  int pic_width, int pic_height)
> > 
> > This function - adopted from downstream - does not seem to perform a
> > whole lot, especially without the modulo checks against the slice size.
> > Perhaps it can be inlined?
> 
> Most of the code here is :)
> 
> This was split from downstream code to check and update dimension. We
> can inline this, or should we leave that to compiler. I am not a very
> big fan of inlining...

It doesn't seem beneficial to code readability to have this function,
which is only called just once and also has the same struct members read
in a `DBG()` directly, abstracted away to a function.  Not really
concerned about generated code/performance FWIW.

Also note that the caller isn't checking the `-EINVAL` result...

> > 
> > > +{
> > > +	if (!dsc || !pic_width || !pic_height) {
> > > +		pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	dsc->drm->pic_width = pic_width;
> > > +	dsc->drm->pic_height = pic_height;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > >  {
> > >  	struct drm_display_mode *mode = msm_host->mode;
> > > @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > >  		hdisplay /= 2;
> > >  	}
> > >  
> > > +	if (msm_host->dsc) {
> > > +		struct msm_display_dsc_config *dsc = msm_host->dsc;
> > > +
> > > +		/* update dsc params with timing params */
> > > +		dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);

That is, the result code here should be checked (or function inlined).

> > > +		DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);
> > 
> > This seems to be pretty non-standard and perhaps unnecessary debug code,
> > with a stray dash in there.  Is is needed here, and if so how about
> > using %dx%d\n to format width and height?
> 
> I can update that, sure...
> 
> > 
> > > +
> > > +		/* we do the calculations for dsc parameters here so that
> > > +		 * panel can use these parameters
> > > +		 */
> > > +		dsi_populate_dsc_params(dsc);
> > > +
> > > +		/* Divide the display by 3 but keep back/font porch and
> > > +		 * pulse width same
> > > +		 */
> > 
> > A more general nit on the comments in this patch series: it is
> > appreciated if comments explain the rationale rather than - or in
> > addition to - merely paraphrasing the code that follows.
> 
> Yes it might be the case here, but in this case I wanted to explicitly
> point out hat we need to divide display by 3...

The main point here is justifying _why_ there's a division by 3 for the
active portion of the signal, I presume that's the compression ratio
(having not read into the DSC compression standard yet at all)?

> > 
> > > +		h_total -= hdisplay;
> > > +		hdisplay /= 3;
> > > +		h_total += hdisplay;
> > > +		ha_end = ha_start + hdisplay;
> > > +	}
> > > +
> > >  	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> > > +		if (msm_host->dsc) {
> > > +			struct msm_display_dsc_config *dsc = msm_host->dsc;
> > > +			u32 reg, intf_width, slice_per_intf;
> > > +			u32 total_bytes_per_intf;
> > > +
> > > +			/* first calculate dsc parameters and then program
> > > +			 * compress mode registers
> > > +			 */
> > > +			intf_width = hdisplay;
> > > +			slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
> > > +
> > > +			dsc->drm->slice_count = 1;
> > > +			dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
> > 
> > If I am not mistaken this is the same value as dsc->drm->slice_width,
> > since a multiple of 8 is inherently "a multiple of 8" and hence needs no
> > rounding when divided by 8 again.
> 
> Yes this doesnt look right, I will update
> 
> > Also note that the cmdmode variant below uses bits_per_pixel here; is
> > that discrepancy intended?
> 
> Nope both should use bits_per_pixel..
> 
> > > +			total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> > > +
> > > +			dsc->eol_byte_num = total_bytes_per_intf % 3;
> > > +			dsc->pclk_per_line =  DIV_ROUND_UP(total_bytes_per_intf, 3);
> > > +			dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
> > > +			dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> > > +
> > > +			reg = dsc->bytes_per_pkt << 16;
> > > +			reg |= (0x0b << 8);    /* dtype of compressed image */
> > > +
> > > +			/* pkt_per_line:
> > > +			 * 0 == 1 pkt
> > > +			 * 1 == 2 pkt
> > > +			 * 2 == 4 pkt
> > > +			 * 3 pkt is not supported
> > > +			 * above translates to ffs() - 1
> > > +			 */
> > > +			reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
> > > +
> > > +			dsc->eol_byte_num = total_bytes_per_intf % 3;
> > 
> > This was already calculated and assigned just a couple lines above.
> 
> Yup, dropped now.
> 
> > 
> > > +			reg |= dsc->eol_byte_num << 4;
> > > +			reg |= 1;
> > 
> > Note that the XML register file exists to map out the layout of these
> > registers, including bit offset, size, and (enum) constant values.  It
> > is appreciated if you can replace all these magical shifts and magic
> > flags/bits with the appropriate enum constants and constructor
> > functions, after mapping them out in the XML file.
> 
> Yeah I am trying to get those details, if I manage to get it, will
> update for sure as Dmitry already pointed in MESA PR.

That'd be lovely.  In any case, even if you can't get the meaning for
all these bits, offsets and constants, it's still desired to at least
add them to the XML as "unknown" or something (including offset in the
name).  Then perhaps the details can be filled in over time while
keeping the driver free of magic constants.  See for example how some of
the Adreno register XML descriptions set this up.

For example, without those details we can already state that
`eol_byte_num` has an offset of 4 in the MODE_CTL register, and probably
a size of 2 bits since its value is always modulo 3.  Likewise
pkt_per_line can be turned into an `<enum>` and so on for the other
segments that build up this register.  Let me know in the mesa MR if you
need assistance/suggestions for mapping out the registers in XML.

> > > +
> > > +			dsi_write(msm_host,
> > > +				  REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> > > +		}
> > > +
> > >  		dsi_write(msm_host, REG_DSI_ACTIVE_H,
> > >  			DSI_ACTIVE_H_START(ha_start) |
> > >  			DSI_ACTIVE_H_END(ha_end));
> > > @@ -959,8 +1034,40 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > >  			DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
> > >  			DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
> > >  	} else {		/* command mode */
> > > +		if (msm_host->dsc) {
> > > +			struct msm_display_dsc_config *dsc = msm_host->dsc;
> > > +			u32 reg, reg_ctrl, reg_ctrl2;
> > > +			u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
> > > +
> > > +			reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> > > +			reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> > 
> > Shouldn't old values be masked out first, before writing new bits or
> > values below?  The video-mode variant doesn't read back old register
> > values.
> 
> This follows downstream where the registers are read, modified and
> written back

Are you sure about this?  The register values are never cleared, meaning
that only bits get added through the `|=` below but never unset - unless
downstream clears these registers elsewhere before ending up in (their
equivalent of) dsi_timing_setup.

> > > +
> > > +			slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
> > > +			bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
> > > +						      dsc->drm->bits_per_pixel, 8);
> > > +			dsc->drm->slice_chunk_size = bytes_in_slice;
> > > +			total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> > > +			dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> > > +
> > > +			reg = 0x39 << 8;
> > 
> > Same comment about moving magic constants and shifts into the XML file.
> 
> yes if we get details of bits

As mentioned above, even without those details this constant can be
represented in the XML, as belonging to offset `8` of the register.

> > 
> > > +			reg |= ffs(dsc->pkt_per_line) << 6;
> > 
> > Doesn't the calculation need -1 here just like video mode?
> 
> yes will update now

Thanks.  I forgot to mention: there seem to be a lot of similarities
between the video and commandmode computations, can those possibly be
factored out of the if-else to save on duplication and accidental
mismatches like these?

- Marijn
Vinod Koul March 22, 2022, 5:16 p.m. UTC | #7
On 17-02-22, 16:11, Marijn Suijten wrote:
> Hi Vinod,
> 
> Thanks for taking time to go through this review, please find some
> clarifications below.
> 
> On 2022-02-17 16:44:04, Vinod Koul wrote:
> > Hi Marijn,
> > 
> > On 11-12-21, 01:03, Marijn Suijten wrote:
> > 
> > > > +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> > > > +				  int pic_width, int pic_height)
> > > 
> > > This function - adopted from downstream - does not seem to perform a
> > > whole lot, especially without the modulo checks against the slice size.
> > > Perhaps it can be inlined?
> > 
> > Most of the code here is :)
> > 
> > This was split from downstream code to check and update dimension. We
> > can inline this, or should we leave that to compiler. I am not a very
> > big fan of inlining...
> 
> It doesn't seem beneficial to code readability to have this function,
> which is only called just once and also has the same struct members read
> in a `DBG()` directly, abstracted away to a function.  Not really
> concerned about generated code/performance FWIW.
> 
> Also note that the caller isn't checking the `-EINVAL` result...

I have made this void inline.

> > > 
> > > > +{
> > > > +	if (!dsc || !pic_width || !pic_height) {
> > > > +		pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	dsc->drm->pic_width = pic_width;
> > > > +	dsc->drm->pic_height = pic_height;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > > >  {
> > > >  	struct drm_display_mode *mode = msm_host->mode;
> > > > @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > > >  		hdisplay /= 2;
> > > >  	}
> > > >  
> > > > +	if (msm_host->dsc) {
> > > > +		struct msm_display_dsc_config *dsc = msm_host->dsc;
> > > > +
> > > > +		/* update dsc params with timing params */
> > > > +		dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> 
> That is, the result code here should be checked (or function inlined).

This function return void, so no point in checking

> > > > +
> > > > +		/* we do the calculations for dsc parameters here so that
> > > > +		 * panel can use these parameters
> > > > +		 */
> > > > +		dsi_populate_dsc_params(dsc);
> > > > +
> > > > +		/* Divide the display by 3 but keep back/font porch and
> > > > +		 * pulse width same
> > > > +		 */
> > > 
> > > A more general nit on the comments in this patch series: it is
> > > appreciated if comments explain the rationale rather than - or in
> > > addition to - merely paraphrasing the code that follows.
> > 
> > Yes it might be the case here, but in this case I wanted to explicitly
> > point out hat we need to divide display by 3...
> 
> The main point here is justifying _why_ there's a division by 3 for the
> active portion of the signal, I presume that's the compression ratio
> (having not read into the DSC compression standard yet at all)?

I have updated this comment

> > > > +		if (msm_host->dsc) {
> > > > +			struct msm_display_dsc_config *dsc = msm_host->dsc;
> > > > +			u32 reg, reg_ctrl, reg_ctrl2;
> > > > +			u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
> > > > +
> > > > +			reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> > > > +			reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> > > 
> > > Shouldn't old values be masked out first, before writing new bits or
> > > values below?  The video-mode variant doesn't read back old register
> > > values.
> > 
> > This follows downstream where the registers are read, modified and
> > written back
> 
> Are you sure about this?  The register values are never cleared, meaning
> that only bits get added through the `|=` below but never unset - unless
> downstream clears these registers elsewhere before ending up in (their
> equivalent of) dsi_timing_setup.

I have modified video mode to write and not read now. For command mode
all bits are set to some value so no need to mask old values for that

> Thanks.  I forgot to mention: there seem to be a lot of similarities
> between the video and commandmode computations, can those possibly be
> factored out of the if-else to save on duplication and accidental
> mismatches like these?

Thanks, this was a good suggestion and am happy to report that I have
incorporated this and indeed code looks better
Marijn Suijten March 22, 2022, 6:59 p.m. UTC | #8
On 2022-03-22 22:46:50, Vinod Koul wrote:
> On 17-02-22, 16:11, Marijn Suijten wrote:
> > Hi Vinod,
> > 
> > Thanks for taking time to go through this review, please find some
> > clarifications below.
> > 
> > On 2022-02-17 16:44:04, Vinod Koul wrote:
> > > Hi Marijn,
> > > 
> > > On 11-12-21, 01:03, Marijn Suijten wrote:
> > > 
> > > > > +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> > > > > +				  int pic_width, int pic_height)
> > > > 
> > > > This function - adopted from downstream - does not seem to perform a
> > > > whole lot, especially without the modulo checks against the slice size.
> > > > Perhaps it can be inlined?
> > > 
> > > Most of the code here is :)
> > > 
> > > This was split from downstream code to check and update dimension. We
> > > can inline this, or should we leave that to compiler. I am not a very
> > > big fan of inlining...
> > 
> > It doesn't seem beneficial to code readability to have this function,
> > which is only called just once and also has the same struct members read
> > in a `DBG()` directly, abstracted away to a function.  Not really
> > concerned about generated code/performance FWIW.
> > 
> > Also note that the caller isn't checking the `-EINVAL` result...
> 
> I have made this void inline.

Perhaps there is a misunderstanding here: with inlining I am referring
to the process of transplanting the _function body_ to the only
call-site, not adding the `inline` keyword nor changing this to `void`.

The checks that make this function return `-EINVAL` seem valid, so the
caller should check it instead of removing the return?

> > > > 
> > > > > +{
> > > > > +	if (!dsc || !pic_width || !pic_height) {
> > > > > +		pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	dsc->drm->pic_width = pic_width;
> > > > > +	dsc->drm->pic_height = pic_height;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > > > >  {
> > > > >  	struct drm_display_mode *mode = msm_host->mode;
> > > > > @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > > > >  		hdisplay /= 2;
> > > > >  	}
> > > > >  
> > > > > +	if (msm_host->dsc) {
> > > > > +		struct msm_display_dsc_config *dsc = msm_host->dsc;
> > > > > +
> > > > > +		/* update dsc params with timing params */
> > > > > +		dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> > 
> > That is, the result code here should be checked (or function inlined).
> 
> This function return void, so no point in checking

It isn't returning `void` in the current patch series that my email is
reviewing, hence explicitly mentioning here that it may have been
overlooked.

Please only convert this to `void` if you are sure that the clause that
originally made `dsi_dsc_update_pic_dim()` return `-EINVAL` on invalid
input is unreachable (if, for example, you moved this check to another
location, say here in `dsi_timing_setup`).

Alas, it's pretty tricky to reason and pose assumptions about code that
I cannot see; we should probably continue this discussion in the next
patch revision depending on how it looks :)

> > [..]
> > Thanks.  I forgot to mention: there seem to be a lot of similarities
> > between the video and commandmode computations, can those possibly be
> > factored out of the if-else to save on duplication and accidental
> > mismatches like these?
> 
> Thanks, this was a good suggestion and am happy to report that I have
> incorporated this and indeed code looks better

Thank you for applying this and the other comments, glad to hear the
code is shaping up and looking forward to the next revision!

- Marijn
Vinod Koul March 23, 2022, 10:57 a.m. UTC | #9
On 22-03-22, 19:59, Marijn Suijten wrote:
> On 2022-03-22 22:46:50, Vinod Koul wrote:
> > On 17-02-22, 16:11, Marijn Suijten wrote:
> > > Hi Vinod,
> > > 
> > > Thanks for taking time to go through this review, please find some
> > > clarifications below.
> > > 
> > > On 2022-02-17 16:44:04, Vinod Koul wrote:
> > > > Hi Marijn,
> > > > 
> > > > On 11-12-21, 01:03, Marijn Suijten wrote:
> > > > 
> > > > > > +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> > > > > > +				  int pic_width, int pic_height)
> > > > > 
> > > > > This function - adopted from downstream - does not seem to perform a
> > > > > whole lot, especially without the modulo checks against the slice size.
> > > > > Perhaps it can be inlined?
> > > > 
> > > > Most of the code here is :)
> > > > 
> > > > This was split from downstream code to check and update dimension. We
> > > > can inline this, or should we leave that to compiler. I am not a very
> > > > big fan of inlining...
> > > 
> > > It doesn't seem beneficial to code readability to have this function,
> > > which is only called just once and also has the same struct members read
> > > in a `DBG()` directly, abstracted away to a function.  Not really
> > > concerned about generated code/performance FWIW.
> > > 
> > > Also note that the caller isn't checking the `-EINVAL` result...
> > 
> > I have made this void inline.
> 
> Perhaps there is a misunderstanding here: with inlining I am referring
> to the process of transplanting the _function body_ to the only
> call-site, not adding the `inline` keyword nor changing this to `void`.
> 
> The checks that make this function return `-EINVAL` seem valid, so the
> caller should check it instead of removing the return?

Okay somehow I misunderstood then, let me see how the code looks in this
case then
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
index 49b551ad1bff..c1c85df58c4b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
@@ -706,4 +706,14 @@  static inline uint32_t DSI_VERSION_MAJOR(uint32_t val)
 #define REG_DSI_CPHY_MODE_CTRL					0x000002d4
 
 
+#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL			0x0000029c
+
+#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL2			0x000002a0
+
+#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL			0x000002a4
+
+#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2			0x000002a8
+
+#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL3			0x000002ac
+
 #endif /* DSI_XML */
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 31d385d8d834..2c14c36f0b3d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -908,6 +908,20 @@  static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
 		dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
 }
 
+static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
+				  int pic_width, int pic_height)
+{
+	if (!dsc || !pic_width || !pic_height) {
+		pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
+		return -EINVAL;
+	}
+
+	dsc->drm->pic_width = pic_width;
+	dsc->drm->pic_height = pic_height;
+
+	return 0;
+}
+
 static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 {
 	struct drm_display_mode *mode = msm_host->mode;
@@ -940,7 +954,68 @@  static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		hdisplay /= 2;
 	}
 
+	if (msm_host->dsc) {
+		struct msm_display_dsc_config *dsc = msm_host->dsc;
+
+		/* update dsc params with timing params */
+		dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
+		DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);
+
+		/* we do the calculations for dsc parameters here so that
+		 * panel can use these parameters
+		 */
+		dsi_populate_dsc_params(dsc);
+
+		/* Divide the display by 3 but keep back/font porch and
+		 * pulse width same
+		 */
+		h_total -= hdisplay;
+		hdisplay /= 3;
+		h_total += hdisplay;
+		ha_end = ha_start + hdisplay;
+	}
+
 	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
+		if (msm_host->dsc) {
+			struct msm_display_dsc_config *dsc = msm_host->dsc;
+			u32 reg, intf_width, slice_per_intf;
+			u32 total_bytes_per_intf;
+
+			/* first calculate dsc parameters and then program
+			 * compress mode registers
+			 */
+			intf_width = hdisplay;
+			slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
+
+			dsc->drm->slice_count = 1;
+			dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
+			total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
+
+			dsc->eol_byte_num = total_bytes_per_intf % 3;
+			dsc->pclk_per_line =  DIV_ROUND_UP(total_bytes_per_intf, 3);
+			dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
+			dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
+
+			reg = dsc->bytes_per_pkt << 16;
+			reg |= (0x0b << 8);    /* dtype of compressed image */
+
+			/* pkt_per_line:
+			 * 0 == 1 pkt
+			 * 1 == 2 pkt
+			 * 2 == 4 pkt
+			 * 3 pkt is not supported
+			 * above translates to ffs() - 1
+			 */
+			reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
+
+			dsc->eol_byte_num = total_bytes_per_intf % 3;
+			reg |= dsc->eol_byte_num << 4;
+			reg |= 1;
+
+			dsi_write(msm_host,
+				  REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
+		}
+
 		dsi_write(msm_host, REG_DSI_ACTIVE_H,
 			DSI_ACTIVE_H_START(ha_start) |
 			DSI_ACTIVE_H_END(ha_end));
@@ -959,8 +1034,40 @@  static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 			DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
 			DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
 	} else {		/* command mode */
+		if (msm_host->dsc) {
+			struct msm_display_dsc_config *dsc = msm_host->dsc;
+			u32 reg, reg_ctrl, reg_ctrl2;
+			u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
+
+			reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
+			reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
+
+			slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
+			bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
+						      dsc->drm->bits_per_pixel, 8);
+			dsc->drm->slice_chunk_size = bytes_in_slice;
+			total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
+			dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
+
+			reg = 0x39 << 8;
+			reg |= ffs(dsc->pkt_per_line) << 6;
+
+			dsc->eol_byte_num = total_bytes_per_intf % 3;
+			reg |= dsc->eol_byte_num << 4;
+			reg |= 1;
+
+			reg_ctrl |= reg;
+			reg_ctrl2 |= bytes_in_slice;
+
+			dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
+			dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
+		}
+
 		/* image data and 1 byte write_memory_start cmd */
-		wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
+		if (!msm_host->dsc)
+			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
+		else
+			wc = mode->hdisplay / 2 + 1;
 
 		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
 			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
@@ -2051,9 +2158,13 @@  int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
 {
 	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
 	const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
+	struct msm_drm_private *priv;
 	int ret;
 
 	msm_host->dev = dev;
+	priv = dev->dev_private;
+	priv->dsc = msm_host->dsc;
+
 	ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K);
 	if (ret) {
 		pr_err("%s: alloc tx gem obj failed, %d\n", __func__, ret);