[PATCH 1/4] drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC (fix video mode DSC)

[PATCH 1/4] drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC (fix video mode DSC)

[Jonathan Marek]

Video mode DSC won't work if this field is not set correctly. Set it to fix
video mode DSC (for slice_per_pkt==1 cases at least).

Fixes: 08802f515c3 ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 9d355cdc3ec1..bddc57726fb9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -847,6 +847,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	u32 slice_per_intf, total_bytes_per_intf;
 	u32 pkt_per_line;
 	u32 eol_byte_num;
+	u32 bytes_per_pkt;
 
 	/* first calculate dsc parameters and then program
 	 * compress mode registers
@@ -854,6 +855,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
 
 	total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
+	bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
 
 	eol_byte_num = total_bytes_per_intf % 3;
 
@@ -891,6 +893,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
 		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
 	} else {
+		reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_WC(bytes_per_pkt);
 		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
 	}
 }
-- 
2.26.1

[PATCH 2/4] drm/msm/dsi: add a comment to explain pkt_per_line encoding

[Jonathan Marek]

Make it clear why the pkt_per_line value is being "divided by 2".

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index bddc57726fb9..2ea2fc105fbf 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -875,6 +875,8 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	/* DSI_VIDEO_COMPRESSION_MODE & DSI_COMMAND_COMPRESSION_MODE
 	 * registers have similar offsets, so for below common code use
 	 * DSI_VIDEO_COMPRESSION_MODE_XXXX for setting bits
+	 *
+	 * pkt_per_line is log2 encoded, >>1 works for supported values (1,2,4)
 	 */
 	reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_PKT_PER_LINE(pkt_per_line >> 1);
 	reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_EOL_BYTE_NUM(eol_byte_num);
-- 
2.26.1

[PATCH 3/4] drm/msm/dsi: support DSC configurations with slice_per_pkt > 1

[Jonathan Marek]

Add a dsc_slice_per_pkt field to mipi_dsi_device struct and the necessary
changes to msm driver to support this field.

Note that the removed "pkt_per_line = slice_per_intf * slice_per_pkt"
comment is incorrect.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 25 ++++++++++---------------
 include/drm/drm_mipi_dsi.h         |  1 +
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 2ea2fc105fbf..7284346ab787 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -161,6 +161,7 @@ struct msm_dsi_host {
 
 	struct drm_display_mode *mode;
 	struct drm_dsc_config *dsc;
+	unsigned int dsc_slice_per_pkt;
 
 	/* connected device info */
 	unsigned int channel;
@@ -855,17 +856,10 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
 
 	total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
-	bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
+	bytes_per_pkt = dsc->slice_chunk_size * msm_host->dsc_slice_per_pkt;
 
 	eol_byte_num = total_bytes_per_intf % 3;
-
-	/*
-	 * Typically, pkt_per_line = slice_per_intf * slice_per_pkt.
-	 *
-	 * Since the current driver only supports slice_per_pkt = 1,
-	 * pkt_per_line will be equal to slice per intf for now.
-	 */
-	pkt_per_line = slice_per_intf;
+	pkt_per_line = slice_per_intf / msm_host->dsc_slice_per_pkt;
 
 	if (is_cmd_mode) /* packet data type */
 		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
@@ -1002,12 +996,8 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		else
 			/*
 			 * When DSC is enabled, WC = slice_chunk_size * slice_per_pkt + 1.
-			 * Currently, the driver only supports default value of slice_per_pkt = 1
-			 *
-			 * TODO: Expand mipi_dsi_device struct to hold slice_per_pkt info
-			 *       and adjust DSC math to account for slice_per_pkt.
 			 */
-			wc = msm_host->dsc->slice_chunk_size + 1;
+			wc = msm_host->dsc->slice_chunk_size * msm_host->dsc_slice_per_pkt + 1;
 
 		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
 			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
@@ -1634,8 +1624,13 @@ static int dsi_host_attach(struct mipi_dsi_host *host,
 	msm_host->lanes = dsi->lanes;
 	msm_host->format = dsi->format;
 	msm_host->mode_flags = dsi->mode_flags;
-	if (dsi->dsc)
+	if (dsi->dsc) {
 		msm_host->dsc = dsi->dsc;
+		msm_host->dsc_slice_per_pkt = dsi->dsc_slice_per_pkt;
+		/* for backwards compatibility, assume 1 if not set */
+		if (!msm_host->dsc_slice_per_pkt)
+			msm_host->dsc_slice_per_pkt = 1;
+	}
 
 	/* Some gpios defined in panel DT need to be controlled by host */
 	ret = dsi_host_init_panel_gpios(msm_host, &dsi->dev);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index c9df0407980c..3e32fa52d94b 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -193,6 +193,7 @@ struct mipi_dsi_device {
 	unsigned long hs_rate;
 	unsigned long lp_rate;
 	struct drm_dsc_config *dsc;
+	unsigned int dsc_slice_per_pkt;
 };
 
 #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
-- 
2.26.1

[PATCH 4/4] drm/msm/dsi: fix DSC for the bonded DSI case

[Jonathan Marek]

For the bonded DSI case, DSC pic_width and timing calculations should use
the width of a single panel instead of the total combined width.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/gpu/drm/msm/dsi/dsi.h         |  3 ++-
 drivers/gpu/drm/msm/dsi/dsi_host.c    | 20 +++++++++++---------
 drivers/gpu/drm/msm/dsi/dsi_manager.c |  2 +-
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 28379b1af63f..3a641e69447c 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -93,7 +93,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host);
 int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
 				  const struct drm_display_mode *mode);
 enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
-					    const struct drm_display_mode *mode);
+					    const struct drm_display_mode *mode,
+					    bool is_bonded_dsi);
 unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host);
 int msm_dsi_host_register(struct mipi_dsi_host *host);
 void msm_dsi_host_unregister(struct mipi_dsi_host *host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 7284346ab787..a6286eb9d006 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -938,8 +938,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 			       mode->hdisplay, mode->vdisplay);
 			return;
 		}
-
-		dsc->pic_width = mode->hdisplay;
+		dsc->pic_width = hdisplay;
 		dsc->pic_height = mode->vdisplay;
 		DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height);
 
@@ -950,6 +949,11 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		if (ret)
 			return;
 
+		if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
+			dsi_update_dsc_timing(msm_host, false, hdisplay);
+		else
+			dsi_update_dsc_timing(msm_host, true, hdisplay);
+
 		/* Divide the display by 3 but keep back/font porch and
 		 * pulse width same
 		 */
@@ -966,9 +970,6 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 	}
 
 	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
-		if (msm_host->dsc)
-			dsi_update_dsc_timing(msm_host, false, mode->hdisplay);
-
 		dsi_write(msm_host, REG_DSI_ACTIVE_H,
 			DSI_ACTIVE_H_START(ha_start) |
 			DSI_ACTIVE_H_END(ha_end));
@@ -987,9 +988,6 @@ 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)
-			dsi_update_dsc_timing(msm_host, true, mode->hdisplay);
-
 		/* image data and 1 byte write_memory_start cmd */
 		if (!msm_host->dsc)
 			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
@@ -2487,7 +2485,8 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
 }
 
 enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
-					    const struct drm_display_mode *mode)
+					    const struct drm_display_mode *mode,
+					    bool is_bonded_dsi)
 {
 	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
 	struct drm_dsc_config *dsc = msm_host->dsc;
@@ -2497,6 +2496,9 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
 	if (!msm_host->dsc)
 		return MODE_OK;
 
+	if (is_bonded_dsi)
+		pic_width = mode->hdisplay / 2;
+
 	if (pic_width % dsc->slice_width) {
 		pr_err("DSI: pic_width %d has to be multiple of slice %d\n",
 		       pic_width, dsc->slice_width);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 896f369fdd53..2ca1a7ca3659 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -455,7 +455,7 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
 			return MODE_ERROR;
 	}
 
-	return msm_dsi_host_check_dsc(host, mode);
+	return msm_dsi_host_check_dsc(host, mode, IS_BONDED_DSI());
 }
 
 static const struct drm_bridge_funcs dsi_mgr_bridge_funcs = {
-- 
2.26.1

Re: [PATCH 4/4] drm/msm/dsi: fix DSC for the bonded DSI case

[Marijn Suijten]

On what hardware have you been testing this?  Dmitry and I have a stack of
patches to resolve support for Active CTL programming on newer hardware (DPU
5.0+ IIRC), where a single CTL is responsible for programming multiple INTF and
DSC blocks as used in bonded DSI.

On 2023-11-14 12:42:16, Jonathan Marek wrote:
> For the bonded DSI case, DSC pic_width and timing calculations should use
> the width of a single panel instead of the total combined width.
> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.h         |  3 ++-
>  drivers/gpu/drm/msm/dsi/dsi_host.c    | 20 +++++++++++---------
>  drivers/gpu/drm/msm/dsi/dsi_manager.c |  2 +-
>  3 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 28379b1af63f..3a641e69447c 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -93,7 +93,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host);
>  int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
>  				  const struct drm_display_mode *mode);
>  enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
> -					    const struct drm_display_mode *mode);
> +					    const struct drm_display_mode *mode,
> +					    bool is_bonded_dsi);
>  unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host);
>  int msm_dsi_host_register(struct mipi_dsi_host *host);
>  void msm_dsi_host_unregister(struct mipi_dsi_host *host);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 7284346ab787..a6286eb9d006 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -938,8 +938,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  			       mode->hdisplay, mode->vdisplay);
>  			return;
>  		}
> -
> -		dsc->pic_width = mode->hdisplay;
> +		dsc->pic_width = hdisplay;

In my testing and debugging on CMDmode panels downstream this value/register
was always programmed to the _full_ width of the bonded panel.  Is that maybe
different for video mode?

>  		dsc->pic_height = mode->vdisplay;
>  		DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height);
>  
> @@ -950,6 +949,11 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  		if (ret)
>  			return;
>  
> +		if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
> +			dsi_update_dsc_timing(msm_host, false, hdisplay);
> +		else
> +			dsi_update_dsc_timing(msm_host, true, hdisplay);
> +

Such cleanups (which appear unrelated) should probably be posted as separate
patches.

- Marijn

>  		/* Divide the display by 3 but keep back/font porch and
>  		 * pulse width same
>  		 */
> @@ -966,9 +970,6 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  	}
>  
>  	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> -		if (msm_host->dsc)
> -			dsi_update_dsc_timing(msm_host, false, mode->hdisplay);
> -
>  		dsi_write(msm_host, REG_DSI_ACTIVE_H,
>  			DSI_ACTIVE_H_START(ha_start) |
>  			DSI_ACTIVE_H_END(ha_end));
> @@ -987,9 +988,6 @@ 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)
> -			dsi_update_dsc_timing(msm_host, true, mode->hdisplay);
> -
>  		/* image data and 1 byte write_memory_start cmd */
>  		if (!msm_host->dsc)
>  			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> @@ -2487,7 +2485,8 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
>  }
>  
>  enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
> -					    const struct drm_display_mode *mode)
> +					    const struct drm_display_mode *mode,
> +					    bool is_bonded_dsi)
>  {
>  	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>  	struct drm_dsc_config *dsc = msm_host->dsc;
> @@ -2497,6 +2496,9 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>  	if (!msm_host->dsc)
>  		return MODE_OK;
>  
> +	if (is_bonded_dsi)
> +		pic_width = mode->hdisplay / 2;
> +
>  	if (pic_width % dsc->slice_width) {
>  		pr_err("DSI: pic_width %d has to be multiple of slice %d\n",
>  		       pic_width, dsc->slice_width);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 896f369fdd53..2ca1a7ca3659 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -455,7 +455,7 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>  			return MODE_ERROR;
>  	}
>  
> -	return msm_dsi_host_check_dsc(host, mode);
> +	return msm_dsi_host_check_dsc(host, mode, IS_BONDED_DSI());
>  }
>  
>  static const struct drm_bridge_funcs dsi_mgr_bridge_funcs = {
> -- 
> 2.26.1
> 

Re: [PATCH 4/4] drm/msm/dsi: fix DSC for the bonded DSI case

[Jonathan Marek]

On 11/14/23 1:28 PM, Marijn Suijten wrote:
> On what hardware have you been testing this?  Dmitry and I have a stack of
> patches to resolve support for Active CTL programming on newer hardware (DPU
> 5.0+ IIRC), where a single CTL is responsible for programming multiple INTF and
> DSC blocks as used in bonded DSI.
> 

I am also using DPU 6+ but I won't be posting patches for DPU to support 
this as I am not using the upstream DPU codebase.

> On 2023-11-14 12:42:16, Jonathan Marek wrote:
>> For the bonded DSI case, DSC pic_width and timing calculations should use
>> the width of a single panel instead of the total combined width.
>>
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi.h         |  3 ++-
>>   drivers/gpu/drm/msm/dsi/dsi_host.c    | 20 +++++++++++---------
>>   drivers/gpu/drm/msm/dsi/dsi_manager.c |  2 +-
>>   3 files changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
>> index 28379b1af63f..3a641e69447c 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>> @@ -93,7 +93,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host);
>>   int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
>>   				  const struct drm_display_mode *mode);
>>   enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>> -					    const struct drm_display_mode *mode);
>> +					    const struct drm_display_mode *mode,
>> +					    bool is_bonded_dsi);
>>   unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host);
>>   int msm_dsi_host_register(struct mipi_dsi_host *host);
>>   void msm_dsi_host_unregister(struct mipi_dsi_host *host);
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 7284346ab787..a6286eb9d006 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -938,8 +938,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>   			       mode->hdisplay, mode->vdisplay);
>>   			return;
>>   		}
>> -
>> -		dsc->pic_width = mode->hdisplay;
>> +		dsc->pic_width = hdisplay;
> 
> In my testing and debugging on CMDmode panels downstream this value/register
> was always programmed to the _full_ width of the bonded panel.  Is that maybe
> different for video mode?
> 

downstream dual DSI panel timings are specified for a single panel 
("qcom,mdss-dsi-panel-width" is for a single panel, not both panels)

>>   		dsc->pic_height = mode->vdisplay;
>>   		DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height);
>>   
>> @@ -950,6 +949,11 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>   		if (ret)
>>   			return;
>>   
>> +		if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>> +			dsi_update_dsc_timing(msm_host, false, hdisplay);
>> +		else
>> +			dsi_update_dsc_timing(msm_host, true, hdisplay);
>> +
> 
> Such cleanups (which appear unrelated) should probably be posted as separate
> patches.
> 
> - Marijn
> 

Its not unrelated, dsi_update_dsc_timing call is moved up so it can use 
the single-panel "hdisplay" value before it gets adjusted for DSC.

>>   		/* Divide the display by 3 but keep back/font porch and
>>   		 * pulse width same
>>   		 */
>> @@ -966,9 +970,6 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>   	}
>>   
>>   	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
>> -		if (msm_host->dsc)
>> -			dsi_update_dsc_timing(msm_host, false, mode->hdisplay);
>> -
>>   		dsi_write(msm_host, REG_DSI_ACTIVE_H,
>>   			DSI_ACTIVE_H_START(ha_start) |
>>   			DSI_ACTIVE_H_END(ha_end));
>> @@ -987,9 +988,6 @@ 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)
>> -			dsi_update_dsc_timing(msm_host, true, mode->hdisplay);
>> -
>>   		/* image data and 1 byte write_memory_start cmd */
>>   		if (!msm_host->dsc)
>>   			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>> @@ -2487,7 +2485,8 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
>>   }
>>   
>>   enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>> -					    const struct drm_display_mode *mode)
>> +					    const struct drm_display_mode *mode,
>> +					    bool is_bonded_dsi)
>>   {
>>   	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>>   	struct drm_dsc_config *dsc = msm_host->dsc;
>> @@ -2497,6 +2496,9 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>>   	if (!msm_host->dsc)
>>   		return MODE_OK;
>>   
>> +	if (is_bonded_dsi)
>> +		pic_width = mode->hdisplay / 2;
>> +
>>   	if (pic_width % dsc->slice_width) {
>>   		pr_err("DSI: pic_width %d has to be multiple of slice %d\n",
>>   		       pic_width, dsc->slice_width);
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> index 896f369fdd53..2ca1a7ca3659 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> @@ -455,7 +455,7 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>>   			return MODE_ERROR;
>>   	}
>>   
>> -	return msm_dsi_host_check_dsc(host, mode);
>> +	return msm_dsi_host_check_dsc(host, mode, IS_BONDED_DSI());
>>   }
>>   
>>   static const struct drm_bridge_funcs dsi_mgr_bridge_funcs = {
>> -- 
>> 2.26.1
>>

Re: [PATCH 4/4] drm/msm/dsi: fix DSC for the bonded DSI case

[Marijn Suijten]

On 2023-11-14 14:00:19, Jonathan Marek wrote:
> On 11/14/23 1:28 PM, Marijn Suijten wrote:
> > On what hardware have you been testing this?  Dmitry and I have a stack of
> > patches to resolve support for Active CTL programming on newer hardware (DPU
> > 5.0+ IIRC), where a single CTL is responsible for programming multiple INTF and
> > DSC blocks as used in bonded DSI.
> > 
> 
> I am also using DPU 6+ but I won't be posting patches for DPU to support 
> this as I am not using the upstream DPU codebase.

Oh that is an odd situation!  At least glad to hear we aren't completely
duplicating our efforts :)

> > On 2023-11-14 12:42:16, Jonathan Marek wrote:
> >> For the bonded DSI case, DSC pic_width and timing calculations should use
> >> the width of a single panel instead of the total combined width.
> >>
> >> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> >> ---
> >>   drivers/gpu/drm/msm/dsi/dsi.h         |  3 ++-
> >>   drivers/gpu/drm/msm/dsi/dsi_host.c    | 20 +++++++++++---------
> >>   drivers/gpu/drm/msm/dsi/dsi_manager.c |  2 +-
> >>   3 files changed, 14 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> >> index 28379b1af63f..3a641e69447c 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> >> @@ -93,7 +93,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host);
> >>   int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
> >>   				  const struct drm_display_mode *mode);
> >>   enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
> >> -					    const struct drm_display_mode *mode);
> >> +					    const struct drm_display_mode *mode,
> >> +					    bool is_bonded_dsi);
> >>   unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host);
> >>   int msm_dsi_host_register(struct mipi_dsi_host *host);
> >>   void msm_dsi_host_unregister(struct mipi_dsi_host *host);
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> index 7284346ab787..a6286eb9d006 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> @@ -938,8 +938,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >>   			       mode->hdisplay, mode->vdisplay);
> >>   			return;
> >>   		}
> >> -
> >> -		dsc->pic_width = mode->hdisplay;
> >> +		dsc->pic_width = hdisplay;
> > 
> > In my testing and debugging on CMDmode panels downstream this value/register
> > was always programmed to the _full_ width of the bonded panel.  Is that maybe
> > different for video mode?
> > 
> 
> downstream dual DSI panel timings are specified for a single panel 
> ("qcom,mdss-dsi-panel-width" is for a single panel, not both panels)

_dual panels_?  In my case I have a "single panel" that is driven by two
"bonded" DSI hosts, just to achieve enough bandwidth.

Indeed my downstream DTS has qcom,mdss-dsi-panel-width set to half the total
panel width, but I recall seeing the full width in the register dump.  I'll scan
through my logs and see if I can back this up.

> >>   		dsc->pic_height = mode->vdisplay;
> >>   		DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height);
> >>   
> >> @@ -950,6 +949,11 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >>   		if (ret)
> >>   			return;
> >>   
> >> +		if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
> >> +			dsi_update_dsc_timing(msm_host, false, hdisplay);
> >> +		else
> >> +			dsi_update_dsc_timing(msm_host, true, hdisplay);

Another thought: it's probably clearer to write:

	bool is_cmd_mode = msm_host->mode_flags & MIPI_DSI_MODE_VIDEO;
	dsi_update_dsc_timing(msm_host, is_cmd_mode, hdisplay);

> >> +
> > 
> > Such cleanups (which appear unrelated) should probably be posted as separate
> > patches.
> > 
> > - Marijn
> > 
> 
> Its not unrelated, dsi_update_dsc_timing call is moved up so it can use 
> the single-panel "hdisplay" value before it gets adjusted for DSC.

This reply was mostly expected after not looking at the original code folded in
the diff, and pretty much solidifies my point: it's a hidden semantical change
that's not immediately obvious from reading the patch, and why I'd like to see
this split up in a few smaller patches.

> >>   		/* Divide the display by 3 but keep back/font porch and
> >>   		 * pulse width same
> >>   		 */
> >> @@ -966,9 +970,6 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >>   	}
> >>   
> >>   	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> >> -		if (msm_host->dsc)
> >> -			dsi_update_dsc_timing(msm_host, false, mode->hdisplay);
> >> -
> >>   		dsi_write(msm_host, REG_DSI_ACTIVE_H,
> >>   			DSI_ACTIVE_H_START(ha_start) |
> >>   			DSI_ACTIVE_H_END(ha_end));
> >> @@ -987,9 +988,6 @@ 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)
> >> -			dsi_update_dsc_timing(msm_host, true, mode->hdisplay);
> >> -
> >>   		/* image data and 1 byte write_memory_start cmd */
> >>   		if (!msm_host->dsc)
> >>   			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;

Regarding another patch: cmdmode calculates and uses word count here, but video
mode does it as part of timing calculations?

- Marijn

> >> @@ -2487,7 +2485,8 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
> >>   }
> >>   
> >>   enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
> >> -					    const struct drm_display_mode *mode)
> >> +					    const struct drm_display_mode *mode,
> >> +					    bool is_bonded_dsi)
> >>   {
> >>   	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> >>   	struct drm_dsc_config *dsc = msm_host->dsc;
> >> @@ -2497,6 +2496,9 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
> >>   	if (!msm_host->dsc)
> >>   		return MODE_OK;
> >>   
> >> +	if (is_bonded_dsi)
> >> +		pic_width = mode->hdisplay / 2;
> >> +
> >>   	if (pic_width % dsc->slice_width) {
> >>   		pr_err("DSI: pic_width %d has to be multiple of slice %d\n",
> >>   		       pic_width, dsc->slice_width);
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >> index 896f369fdd53..2ca1a7ca3659 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >> @@ -455,7 +455,7 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
> >>   			return MODE_ERROR;
> >>   	}
> >>   
> >> -	return msm_dsi_host_check_dsc(host, mode);
> >> +	return msm_dsi_host_check_dsc(host, mode, IS_BONDED_DSI());
> >>   }
> >>   
> >>   static const struct drm_bridge_funcs dsi_mgr_bridge_funcs = {
> >> -- 
> >> 2.26.1
> >>