| Message ID | 20240322104732.2327060-1-sean@geanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/stm: dsi: relax mode_valid clock tolerance | expand |
On Fri, Mar 22, 2024 at 11:47:31AM +0100, Sean Nyekjaer wrote: > When using the DSI interface via DSI2LVDS bridge, it seems a bit harsh > to reguire the requested and the actual px clock to be within > 50Hz. A typical LVDS display requires the px clock to be within +-10%. > > In case for HDMI .5% tolerance is required. > > Fixes: e01356d18273 ("drm/stm: dsi: provide the implementation of mode_valid()") > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > --- Any feedback on this? > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > index d5f8c923d7bc..97936b0ef702 100644 > --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > @@ -322,8 +322,6 @@ dw_mipi_dsi_phy_get_timing(void *priv_data, unsigned int lane_mbps, > return 0; > } > > -#define CLK_TOLERANCE_HZ 50 > - > static enum drm_mode_status > dw_mipi_dsi_stm_mode_valid(void *priv_data, > const struct drm_display_mode *mode, > @@ -375,9 +373,10 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data, > /* > * Filter modes according to the clock value, particularly useful for > * hdmi modes that require precise pixel clocks. > + * Check that px_clock is within .5% tolerance. > */ > - if (px_clock_hz < target_px_clock_hz - CLK_TOLERANCE_HZ || > - px_clock_hz > target_px_clock_hz + CLK_TOLERANCE_HZ) > + if (px_clock_hz < mult_frac(target_px_clock_hz, 995, 1000) || > + px_clock_hz > mult_frac(target_px_clock_hz, 1005, 1000)) > return MODE_CLOCK_RANGE; > > /* sync packets are codes as DSI short packets (4 bytes) */ > -- > 2.44.0 >
On Mon, Apr 22, 2024 at 4:06 PM Sean Nyekjaer <sean@geanix.com> wrote: > > On Fri, Mar 22, 2024 at 11:47:31AM +0100, Sean Nyekjaer wrote: > > When using the DSI interface via DSI2LVDS bridge, it seems a bit harsh > > to reguire the requested and the actual px clock to be within > > 50Hz. A typical LVDS display requires the px clock to be within +-10%. > > > > In case for HDMI .5% tolerance is required. > > > > Fixes: e01356d18273 ("drm/stm: dsi: provide the implementation of mode_valid()") > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > --- > Any feedback on this? > > > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > index d5f8c923d7bc..97936b0ef702 100644 > > --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > @@ -322,8 +322,6 @@ dw_mipi_dsi_phy_get_timing(void *priv_data, unsigned int lane_mbps, > > return 0; > > } > > > > -#define CLK_TOLERANCE_HZ 50 > > - > > static enum drm_mode_status > > dw_mipi_dsi_stm_mode_valid(void *priv_data, > > const struct drm_display_mode *mode, > > @@ -375,9 +373,10 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data, > > /* > > * Filter modes according to the clock value, particularly useful for > > * hdmi modes that require precise pixel clocks. > > + * Check that px_clock is within .5% tolerance. > > */ > > - if (px_clock_hz < target_px_clock_hz - CLK_TOLERANCE_HZ || > > - px_clock_hz > target_px_clock_hz + CLK_TOLERANCE_HZ) > > + if (px_clock_hz < mult_frac(target_px_clock_hz, 995, 1000) || > > + px_clock_hz > mult_frac(target_px_clock_hz, 1005, 1000)) > > return MODE_CLOCK_RANGE; > > > > /* sync packets are codes as DSI short packets (4 bytes) */ > > -- > > 2.44.0 > > > Reviewed-by: Robert Foss <rfoss@kernel.org>
Hi, Sorry, my previous review didn't go through. On Fri, Mar 22, 2024 at 11:47:31AM +0100, Sean Nyekjaer wrote: > When using the DSI interface via DSI2LVDS bridge, it seems a bit harsh > to reguire the requested and the actual px clock to be within > 50Hz. A typical LVDS display requires the px clock to be within +-10%. > > In case for HDMI .5% tolerance is required. > > Fixes: e01356d18273 ("drm/stm: dsi: provide the implementation of mode_valid()") > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > --- > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > index d5f8c923d7bc..97936b0ef702 100644 > --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > @@ -322,8 +322,6 @@ dw_mipi_dsi_phy_get_timing(void *priv_data, unsigned int lane_mbps, > return 0; > } > > -#define CLK_TOLERANCE_HZ 50 > - > static enum drm_mode_status > dw_mipi_dsi_stm_mode_valid(void *priv_data, > const struct drm_display_mode *mode, > @@ -375,9 +373,10 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data, > /* > * Filter modes according to the clock value, particularly useful for > * hdmi modes that require precise pixel clocks. > + * Check that px_clock is within .5% tolerance. > */ > - if (px_clock_hz < target_px_clock_hz - CLK_TOLERANCE_HZ || > - px_clock_hz > target_px_clock_hz + CLK_TOLERANCE_HZ) > + if (px_clock_hz < mult_frac(target_px_clock_hz, 995, 1000) || > + px_clock_hz > mult_frac(target_px_clock_hz, 1005, 1000)) > return MODE_CLOCK_RANGE; I wonder if it's not something that should be made into a helper. We have a couple of drivers doing it already, so it might be worth creating a function that checks for a given struct clk pointer and pixel clock if it's within parameters. Maxime
Hi, On Wed, Apr 24, 2024 at 09:21:17AM UTC, Maxime Ripard wrote: > Hi, > > Sorry, my previous review didn't go through. > > On Fri, Mar 22, 2024 at 11:47:31AM +0100, Sean Nyekjaer wrote: > > When using the DSI interface via DSI2LVDS bridge, it seems a bit harsh > > to reguire the requested and the actual px clock to be within > > 50Hz. A typical LVDS display requires the px clock to be within +-10%. > > > > In case for HDMI .5% tolerance is required. > > > > Fixes: e01356d18273 ("drm/stm: dsi: provide the implementation of mode_valid()") > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > --- > > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > index d5f8c923d7bc..97936b0ef702 100644 > > --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > @@ -322,8 +322,6 @@ dw_mipi_dsi_phy_get_timing(void *priv_data, unsigned int lane_mbps, > > return 0; > > } > > > > -#define CLK_TOLERANCE_HZ 50 > > - > > static enum drm_mode_status > > dw_mipi_dsi_stm_mode_valid(void *priv_data, > > const struct drm_display_mode *mode, > > @@ -375,9 +373,10 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data, > > /* > > * Filter modes according to the clock value, particularly useful for > > * hdmi modes that require precise pixel clocks. > > + * Check that px_clock is within .5% tolerance. > > */ > > - if (px_clock_hz < target_px_clock_hz - CLK_TOLERANCE_HZ || > > - px_clock_hz > target_px_clock_hz + CLK_TOLERANCE_HZ) > > + if (px_clock_hz < mult_frac(target_px_clock_hz, 995, 1000) || > > + px_clock_hz > mult_frac(target_px_clock_hz, 1005, 1000)) > > return MODE_CLOCK_RANGE; > > I wonder if it's not something that should be made into a helper. We > have a couple of drivers doing it already, so it might be worth creating > a function that checks for a given struct clk pointer and pixel clock if > it's within parameters. > > Maxime Yes agree, if the same calculation is happening other places. I can't identify where it happens though. Would that helper function exist in drivers/gpu/drm/drm_hdmi_helper.c ? I will give the implementation a shot. /Sean
Hi, On Mon, Apr 29, 2024 at 10:17:45AM +0200, Sean Nyekjaer wrote: > On Wed, Apr 24, 2024 at 09:21:17AM UTC, Maxime Ripard wrote: > > Hi, > > > > Sorry, my previous review didn't go through. > > > > On Fri, Mar 22, 2024 at 11:47:31AM +0100, Sean Nyekjaer wrote: > > > When using the DSI interface via DSI2LVDS bridge, it seems a bit harsh > > > to reguire the requested and the actual px clock to be within > > > 50Hz. A typical LVDS display requires the px clock to be within +-10%. > > > > > > In case for HDMI .5% tolerance is required. > > > > > > Fixes: e01356d18273 ("drm/stm: dsi: provide the implementation of mode_valid()") > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > > --- > > > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > > index d5f8c923d7bc..97936b0ef702 100644 > > > --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > > +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > > @@ -322,8 +322,6 @@ dw_mipi_dsi_phy_get_timing(void *priv_data, unsigned int lane_mbps, > > > return 0; > > > } > > > > > > -#define CLK_TOLERANCE_HZ 50 > > > - > > > static enum drm_mode_status > > > dw_mipi_dsi_stm_mode_valid(void *priv_data, > > > const struct drm_display_mode *mode, > > > @@ -375,9 +373,10 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data, > > > /* > > > * Filter modes according to the clock value, particularly useful for > > > * hdmi modes that require precise pixel clocks. > > > + * Check that px_clock is within .5% tolerance. > > > */ > > > - if (px_clock_hz < target_px_clock_hz - CLK_TOLERANCE_HZ || > > > - px_clock_hz > target_px_clock_hz + CLK_TOLERANCE_HZ) > > > + if (px_clock_hz < mult_frac(target_px_clock_hz, 995, 1000) || > > > + px_clock_hz > mult_frac(target_px_clock_hz, 1005, 1000)) > > > return MODE_CLOCK_RANGE; > > > > I wonder if it's not something that should be made into a helper. We > > have a couple of drivers doing it already, so it might be worth creating > > a function that checks for a given struct clk pointer and pixel clock if > > it's within parameters. > > > > Maxime > > Yes agree, if the same calculation is happening other places. > I can't identify where it happens though. sun4i has one: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_rgb.c#L123 > Would that helper function exist in drivers/gpu/drm/drm_hdmi_helper.c ? There's nothing related to HDMI per se, so in drm_modes is probably better. Maxime
Hi Sean, thanks for your patch. Tested-by: Yannick Fertre <yannick.fertre@foss.st.com> I think that a helper could be useful in simplifying this part. This might be reworked when a new helper will be implemented. Best regards On 4/22/24 16:05, Sean Nyekjaer wrote: > On Fri, Mar 22, 2024 at 11:47:31AM +0100, Sean Nyekjaer wrote: >> When using the DSI interface via DSI2LVDS bridge, it seems a bit harsh >> to reguire the requested and the actual px clock to be within >> 50Hz. A typical LVDS display requires the px clock to be within +-10%. >> >> In case for HDMI .5% tolerance is required. >> >> Fixes: e01356d18273 ("drm/stm: dsi: provide the implementation of mode_valid()") >> Signed-off-by: Sean Nyekjaer <sean@geanix.com> >> --- > Any feedback on this? > >> drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c >> index d5f8c923d7bc..97936b0ef702 100644 >> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c >> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c >> @@ -322,8 +322,6 @@ dw_mipi_dsi_phy_get_timing(void *priv_data, unsigned int lane_mbps, >> return 0; >> } >> >> -#define CLK_TOLERANCE_HZ 50 >> - >> static enum drm_mode_status >> dw_mipi_dsi_stm_mode_valid(void *priv_data, >> const struct drm_display_mode *mode, >> @@ -375,9 +373,10 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data, >> /* >> * Filter modes according to the clock value, particularly useful for >> * hdmi modes that require precise pixel clocks. >> + * Check that px_clock is within .5% tolerance. >> */ >> - if (px_clock_hz < target_px_clock_hz - CLK_TOLERANCE_HZ || >> - px_clock_hz > target_px_clock_hz + CLK_TOLERANCE_HZ) >> + if (px_clock_hz < mult_frac(target_px_clock_hz, 995, 1000) || >> + px_clock_hz > mult_frac(target_px_clock_hz, 1005, 1000)) >> return MODE_CLOCK_RANGE; >> >> /* sync packets are codes as DSI short packets (4 bytes) */ >> -- >> 2.44.0 >>
On Wed, May 15, 2024 at 08:39:49AM UTC, Yannick FERTRE wrote: > Hi Sean, > > thanks for your patch. > > Tested-by: Yannick Fertre <yannick.fertre@foss.st.com> > > I think that a helper could be useful in simplifying this part. > This might be reworked when a new helper will be implemented. > > Best regards Hi Yannick, Will this mean that this will patch will go in? I still have plans to do the helper, but I'm limited on time :) /Sean > > > On 4/22/24 16:05, Sean Nyekjaer wrote: > > On Fri, Mar 22, 2024 at 11:47:31AM +0100, Sean Nyekjaer wrote: > > > When using the DSI interface via DSI2LVDS bridge, it seems a bit harsh > > > to reguire the requested and the actual px clock to be within > > > 50Hz. A typical LVDS display requires the px clock to be within +-10%. > > > > > > In case for HDMI .5% tolerance is required. > > > > > > Fixes: e01356d18273 ("drm/stm: dsi: provide the implementation of mode_valid()") > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > > --- > > Any feedback on this? > > > > > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > > index d5f8c923d7bc..97936b0ef702 100644 > > > --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > > +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > > > @@ -322,8 +322,6 @@ dw_mipi_dsi_phy_get_timing(void *priv_data, unsigned int lane_mbps, > > > return 0; > > > } > > > -#define CLK_TOLERANCE_HZ 50 > > > - > > > static enum drm_mode_status > > > dw_mipi_dsi_stm_mode_valid(void *priv_data, > > > const struct drm_display_mode *mode, > > > @@ -375,9 +373,10 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data, > > > /* > > > * Filter modes according to the clock value, particularly useful for > > > * hdmi modes that require precise pixel clocks. > > > + * Check that px_clock is within .5% tolerance. > > > */ > > > - if (px_clock_hz < target_px_clock_hz - CLK_TOLERANCE_HZ || > > > - px_clock_hz > target_px_clock_hz + CLK_TOLERANCE_HZ) > > > + if (px_clock_hz < mult_frac(target_px_clock_hz, 995, 1000) || > > > + px_clock_hz > mult_frac(target_px_clock_hz, 1005, 1000)) > > > return MODE_CLOCK_RANGE; > > > /* sync packets are codes as DSI short packets (4 bytes) */ > > > -- > > > 2.44.0 > > >
Hi, On Wed, May 15, 2024 at 09:42:34AM +0200, Sean Nyekjaer wrote: > On Wed, May 15, 2024 at 08:39:49AM UTC, Yannick FERTRE wrote: > > Hi Sean, > > > > thanks for your patch. > > > > Tested-by: Yannick Fertre <yannick.fertre@foss.st.com> > > > > I think that a helper could be useful in simplifying this part. > > This might be reworked when a new helper will be implemented. > > > > Best regards > > Hi Yannick, > > Will this mean that this will patch will go in? > > I still have plans to do the helper, but I'm limited on time :) I still think we should work on the helper and merge that directly. It's been broken for a while anyway so it's not like it's a regression anyway. Maxime
diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c index d5f8c923d7bc..97936b0ef702 100644 --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c @@ -322,8 +322,6 @@ dw_mipi_dsi_phy_get_timing(void *priv_data, unsigned int lane_mbps, return 0; } -#define CLK_TOLERANCE_HZ 50 - static enum drm_mode_status dw_mipi_dsi_stm_mode_valid(void *priv_data, const struct drm_display_mode *mode, @@ -375,9 +373,10 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data, /* * Filter modes according to the clock value, particularly useful for * hdmi modes that require precise pixel clocks. + * Check that px_clock is within .5% tolerance. */ - if (px_clock_hz < target_px_clock_hz - CLK_TOLERANCE_HZ || - px_clock_hz > target_px_clock_hz + CLK_TOLERANCE_HZ) + if (px_clock_hz < mult_frac(target_px_clock_hz, 995, 1000) || + px_clock_hz > mult_frac(target_px_clock_hz, 1005, 1000)) return MODE_CLOCK_RANGE; /* sync packets are codes as DSI short packets (4 bytes) */
When using the DSI interface via DSI2LVDS bridge, it seems a bit harsh to reguire the requested and the actual px clock to be within 50Hz. A typical LVDS display requires the px clock to be within +-10%. In case for HDMI .5% tolerance is required. Fixes: e01356d18273 ("drm/stm: dsi: provide the implementation of mode_valid()") Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)