Re: [PATCH v2 1/2] drm/bridge: ti-sn65dsi86: Add atomic_check hook for the bridge

[Jayesh Choudhary <j-choudhary@ti.com>]

Hello Dmitry,

Thanks for the review.

On 18/06/24 14:29, Dmitry Baryshkov wrote:
> On Tue, Jun 18, 2024 at 01:44:17PM GMT, Jayesh Choudhary wrote:
>> Add the atomic_check hook to ensure that the parameters are within the
>> valid range.
>> As of now, dsi clock freqency is being calculated in bridge_enable but
>> this needs to be checked in atomic_check which is called before
>> bridge_enable so move this calculation to atomic_check and write the
>> register value in bridge_enable as it is.
>>
>> For now, add mode clock check for the max resolution supported by the
>> bridge as mentioned in the SN65DSI86 datasheet[0] and dsi clock range
>> check for SN_DSIA_CLK_FREQ_REG.
>> According to the datasheet[0], the minimum value for that reg is 0x08
>> and the maximum value is 0x96. So add check for that.
>>
>> [0]: <https://www.ti.com/lit/gpn/sn65dsi86>
>>
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 65 +++++++++++++++++++--------
>>   1 file changed, 46 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> index 84698a0b27a8..d13b42d7c512 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -113,6 +113,20 @@
>>   

[...]

>>   
>> +static int ti_sn_bridge_atomic_check(struct drm_bridge *bridge,
>> +				     struct drm_bridge_state *bridge_state,
>> +				     struct drm_crtc_state *crtc_state,
>> +				     struct drm_connector_state *conn_state)
>> +{
>> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>> +	struct drm_display_mode *mode = &crtc_state->mode;
>> +	unsigned int bit_rate_mhz, clk_freq_mhz;
>> +
>> +	/* Pixel clock check */
>> +	if (mode->clock > SN65DSI86_MAX_PIXEL_CLOCK_KHZ)
>> +		return -EINVAL;
>> +
>> +	bit_rate_mhz = (mode->clock / 1000) *
>> +			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
>> +	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
>> +
>> +	/* for each increment in dsi_clk_range, frequency increases by 5MHz */
>> +	pdata->dsi_clk_range = (MIN_DSI_CLK_FREQ_MHZ / 5) +
>> +		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
> 
> atomic_check might be called several times, it might be called to test
> the state. As such, it should not modify anything outside of the
> state variables.
> 

If not in atomic_check, then where should I move this calculation and check?
mode_valid with returning MODE_BAD in case of failure?

I had to move it from bridge_enable based on the comments on v1:
https://patchwork.kernel.org/project/dri-devel/patch/20240408073623.186489-1-j-choudhary@ti.com/#25801801

Warm Regards,
Jayesh

[...]