Re: [Freedreno] [PATCH 3/3] drm/msm/dsi: Enable DATABUS_WIDEN for DSI command mode

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 6/14/2023 2:56 AM, Marijn Suijten wrote:
> On 2023-06-13 18:57:13, Jessica Zhang wrote:
>> DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
>> 48 bits of compressed data per pclk instead of 24.
>>
>> For all chipsets that support this mode, enable it whenever DSC is
>> enabled as recommend by the hardware programming guide.
>>
>> Only enable this for command mode as we are currently unable to validate
>> it for video mode.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>
>> Note: The dsi.xml.h changes were generated using the headergen2 script in
>> envytools [1], but the changes to the copyright and rules-ng-ng source file
>> paths were dropped.
>>
>> [1] https://github.com/freedreno/envytools/
> 
> More interesting would be a link to the Mesa MR upstreaming this
> bitfield to dsi.xml [2] (which I have not found on my own yet).
> 
> [2]: https://gitlab.freedesktop.org/mesa/mesa/-/blame/main/src/freedreno/registers/dsi/dsi.xml
> 

Thats because we havent submitted a MR yet for this on mesa.

Generally, our team does not have legal permissions yet for mesa MRs 
other than mesa drm because we got permissions for the modetest.

Rob/Dmitry, can one of you pls help with the corresponding mesa MR for this?

The xml file change was autogenerated so this patch can go in.

>>   drivers/gpu/drm/msm/dsi/dsi.xml.h  |  1 +
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
>>   2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> index a4a154601114..2a7d980e12c3 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> @@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
>>   	return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
>>   }
>>   #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE			0x00010000
>> +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN			0x00100000
>>
>>   #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL			0x000001b8
>>   #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK		0x0000003f
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 5d7b4409e4e9..1da5238e7105 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -927,6 +927,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>   	u32 hdisplay = mode->hdisplay;
>>   	u32 wc;
>>   	int ret;
>> +	bool widebus_supported = msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
>> +			msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
>> +
>>
>>   	DBG("");
>>
>> @@ -973,8 +976,15 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>   		 *
>>   		 * hdisplay will be divided by 3 here to account for the fact
>>   		 * that DPU sends 3 bytes per pclk cycle to DSI.
>> +		 *
>> +		 * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
>> +		 * instead of 3
> 
> So this should allow us to divide pclk by 2, or have much lower latency?
> Otherwise it'll tick enough times to transmit the data twice.
> 
> Note that I brought up the exact same concerns when you used the 3:1
> ratio from dsi_bpp / dsc_bpp in your pclk reduction patch (instad of the
> number of bits/bytes that DPU sends to DSI per pclk), but no-one has
> replied to my inquiry yet.
> 

Ideally yes, we could have done pclk/2 on uncompressed pixels but we are 
not going to add support for widebus on DSI without DSC as that is not 
recommended in our docs.

So this cannot be done.

We tried our best to respond and explain to all your queries both on the 
bug and the patch but i guess it just kept coming :)

I am going to try one more time to explain it in the documentation change.

>>   		 */
>> -		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>> +		if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) && widebus_supported)
>> +			hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 6);
>> +		else
>> +			hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> 
> Nit: I wonder if this is more concise when written as:
> 
>      u32 bytes_per_pclk;
>      ...
>      if (!video && widebus)
>          bytes_per_pclk = 6;
>      else
>          bytes_per_pclk = 3;
> 
>      hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc),
>                              bytes_per_pclk);
> 
> That is less duplication, **and** the value becomes self-documenting!
> 

Sure, no concerns with making this change.

>> +
>>   		h_total += hdisplay;
>>   		ha_end = ha_start + hdisplay;
>>   	}
>> @@ -1027,6 +1037,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>   		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
>>   			DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
>>   			DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
>> +
>> +		if (msm_host->dsc && widebus_supported) {
> 
> Can we also support widebus for uncompressed streams (sending 2 pixels
> of bpp=24 per pclk), and if so, is that something you want to add in the
> future (a comment would be nice)?

No, we cannot support widebus on uncompressed streams on DSI so we wont 
be adding that.

> 
>> +			u32 mdp_ctrl2 = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
>> +
>> +			mdp_ctrl2 |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
>> +			dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, mdp_ctrl2);
>> +		}
> 
> Same comment as on your BURST_MODE patch (which this'll conflict with):
> does this belong to the timing setup or is it better moved to
> dsi_ctrl_config?
> 
> - Marijn
> 
>>   	}
>>   }
>>
>>
>> --
>> 2.40.1
>>