On Tue, 18 Jun 2024 at 12:56, Jayesh Choudhary <j-choudhary@xxxxxx> 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@xxxxxx> > >> --- > >> 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. 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. > > 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@xxxxxx/#25801801 > > Warm Regards, > Jayesh > > [...] -- With best wishes Dmitry
