| Message ID | 20230818-samsung-dsim-v1-5-b39716db6b7a@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge | expand |
On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter <m.tretter@pengutronix.de> wrote: > > Calculating the byte_clk in kHz is imprecise for a hs_clock of 55687500 > Hz, which may be used with a pixel clock of 74.25 MHz with mode > 1920x1080-30. > > Fix the calculation by using HZ instead of kHZ. > > This requires to change the type to u64 to prevent overflows of the > integer type. > I would argue this needs a fixes tag since the previous calculations were not accurately generated for many resolutions and refresh rates. Reviewed-by: Adam Ford <aford173@gmail.com> #imx8mm-beacon Tested-by: Adam Ford <aford173@gmail.com> #imx8mm-beacon > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > --- > drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c > index 459be953be55..eb7aca2b9ab7 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -973,10 +973,12 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi) > u32 reg; > > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) { > - int byte_clk_khz = dsi->hs_clock / 1000 / 8; > - int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk_khz, m->clock); > - int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) * byte_clk_khz, m->clock); > - int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk_khz, m->clock); > + u64 byte_clk = dsi->hs_clock / 8; > + u64 pix_clk = m->clock * 1000; > + > + int hfp = DIV64_U64_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk, pix_clk); > + int hbp = DIV64_U64_ROUND_UP((m->htotal - m->hsync_end) * byte_clk, pix_clk); > + int hsa = DIV64_U64_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk, pix_clk); > > /* remove packet overhead when possible */ > hfp = max(hfp - 6, 0); > > -- > 2.39.2 >
Hi, On 28.08.23 17:59, Michael Tretter wrote: > Calculating the byte_clk in kHz is imprecise for a hs_clock of 55687500 > Hz, which may be used with a pixel clock of 74.25 MHz with mode > 1920x1080-30. > > Fix the calculation by using HZ instead of kHZ. > > This requires to change the type to u64 to prevent overflows of the > integer type. > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > --- > drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c > index 459be953be55..eb7aca2b9ab7 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -973,10 +973,12 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi) > u32 reg; > > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) { > - int byte_clk_khz = dsi->hs_clock / 1000 / 8; > - int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk_khz, m->clock); > - int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) * byte_clk_khz, m->clock); > - int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk_khz, m->clock); > + u64 byte_clk = dsi->hs_clock / 8; > + u64 pix_clk = m->clock * 1000; > + > + int hfp = DIV64_U64_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk, pix_clk); > + int hbp = DIV64_U64_ROUND_UP((m->htotal - m->hsync_end) * byte_clk, pix_clk); > + int hsa = DIV64_U64_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk, pix_clk); Wouldn't it make sense to use the videomode structure here? > > /* remove packet overhead when possible */ > hfp = max(hfp - 6, 0); > Best regards, Maxim
On Mon, 04 Sep 2023 16:28:37 +0200, Maxim Schwalm wrote: > On 28.08.23 17:59, Michael Tretter wrote: > > Calculating the byte_clk in kHz is imprecise for a hs_clock of 55687500 > > Hz, which may be used with a pixel clock of 74.25 MHz with mode > > 1920x1080-30. > > > > Fix the calculation by using HZ instead of kHZ. > > > > This requires to change the type to u64 to prevent overflows of the > > integer type. > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > --- > > drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c > > index 459be953be55..eb7aca2b9ab7 100644 > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > > @@ -973,10 +973,12 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi) > > u32 reg; > > > > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) { > > - int byte_clk_khz = dsi->hs_clock / 1000 / 8; > > - int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk_khz, m->clock); > > - int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) * byte_clk_khz, m->clock); > > - int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk_khz, m->clock); > > + u64 byte_clk = dsi->hs_clock / 8; > > + u64 pix_clk = m->clock * 1000; > > + > > + int hfp = DIV64_U64_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk, pix_clk); > > + int hbp = DIV64_U64_ROUND_UP((m->htotal - m->hsync_end) * byte_clk, pix_clk); > > + int hsa = DIV64_U64_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk, pix_clk); > > Wouldn't it make sense to use the videomode structure here? That would introduce a dependency on VIDEOMODE_HELPERS to avoid four explicit calculations of the pixel clock in Hz and the porches in pixels. I don't think that's really worth it. Michael > > > > > /* remove packet overhead when possible */ > > hfp = max(hfp - 6, 0); > > > > Best regards, > Maxim >
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index 459be953be55..eb7aca2b9ab7 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -973,10 +973,12 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi) u32 reg; if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) { - int byte_clk_khz = dsi->hs_clock / 1000 / 8; - int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk_khz, m->clock); - int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) * byte_clk_khz, m->clock); - int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk_khz, m->clock); + u64 byte_clk = dsi->hs_clock / 8; + u64 pix_clk = m->clock * 1000; + + int hfp = DIV64_U64_ROUND_UP((m->hsync_start - m->hdisplay) * byte_clk, pix_clk); + int hbp = DIV64_U64_ROUND_UP((m->htotal - m->hsync_end) * byte_clk, pix_clk); + int hsa = DIV64_U64_ROUND_UP((m->hsync_end - m->hsync_start) * byte_clk, pix_clk); /* remove packet overhead when possible */ hfp = max(hfp - 6, 0);
Calculating the byte_clk in kHz is imprecise for a hs_clock of 55687500 Hz, which may be used with a pixel clock of 74.25 MHz with mode 1920x1080-30. Fix the calculation by using HZ instead of kHZ. This requires to change the type to u64 to prevent overflows of the integer type. Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> --- drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)