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

[Jonathan Marek <jonathan@marek.ca>]

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
>>