Re: [PATCH] drm/stm: dsi: relax mode_valid clock tolerance
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 > > > > 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 signature.asc Description: PGP signature
Re: [PATCH] drm/stm: dsi: relax mode_valid clock tolerance
Hi Sean, thanks for your patch. Tested-by: Yannick Fertre 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 --- 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
Re: [PATCH] drm/stm: dsi: relax mode_valid clock tolerance
On Wed, May 15, 2024 at 08:39:49AM UTC, Yannick FERTRE wrote: > Hi Sean, > > thanks for your patch. > > Tested-by: Yannick Fertre > > 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 > > > --- > > 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 > > >
Re: [PATCH] drm/stm: dsi: relax mode_valid clock tolerance
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 > > > --- > > > 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 signature.asc Description: PGP signature
Re: [PATCH] drm/stm: dsi: relax mode_valid clock tolerance
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 > > --- > > 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
Re: [PATCH] drm/stm: dsi: relax mode_valid clock tolerance
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 > --- > 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 signature.asc Description: PGP signature
Re: [PATCH] drm/stm: dsi: relax mode_valid clock tolerance
On Mon, Apr 22, 2024 at 4:06 PM 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 > > --- > 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
Re: [PATCH] drm/stm: dsi: relax mode_valid clock tolerance
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 > --- 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 >