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

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

Hello Dmitry,

On 18/06/24 15:45, Dmitry Baryshkov wrote:
> On Tue, 18 Jun 2024 at 12:56, Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>
>> 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 didn't write that it's the wrong place for math. I wrote that you
> should not be modifying global structure.
> 
> So you have to subclass drm_bridge_state for the driver and store the
> value there. Or just add a helper function and call it from
> atomic_check(), mode_valid() and set_dsi_rate(). It really looks like
> a simpler solution here.
> 

Okay, instead of moving the set_dsi_rate, I will rename it to
calc_dsi_rate with integer return value which I would use in both
bridge enable to write the register value and atomic_check to check the
parameters eliminating the need to modify the pdata structure/ adding
new variable to the structure.

(Earlier I was trying to avoid calculation in both calls so I added
another variable to the structure and used that. But I get your point
now!)

I will re-order the patches to have an independent fix patch
addressing your concern in [2/2] patch
https://lore.kernel.org/all/CAA8EJpq2UkMn9ArSNaJcOyw28H4uUcRwvUqfUBBqSCALmozBrg@mail.gmail.com/

Also in the code they have been using 594MHz as mode clock limit.
I was using more relaxed value (600MHz) in atomic check but I will
switch to 594MHz to be in sync with the value that is used in the
driver.

> Note, there is a significant difference between mode_valid() and
> atomic_check(). The former function is used for filtering the modes,
> while the latter one is used for actually checking that the parameters
> passed from the client are correct.

[...]

Warm Regards,
Jayesh