[PATCH v2 0/3] drm/stm: dsi: relax mode_valid clock tolerance

[PATCH v2 0/3] drm/stm: dsi: relax mode_valid clock tolerance

[Sean Nyekjaer]

Introduce helper function to check if the pixel clock is within
tolerance.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes in v2:
- Introduce helper function drm_mode_validate_mode()
- drm/stm: use helper function for mode validation
- drm/sun4i: use helper function for mode validation
- Link to v1: https://lore.kernel.org/r/20240322104732.2327060-1-sean@geanix.com

---
Sean Nyekjaer (3):
      drm/modes: introduce drm_mode_validate_mode() helper function
      drm/sun4i: use drm_mode_validate_mode() helper function
      drm/stm: dsi: use drm_mode_validate_mode() helper function

 drivers/gpu/drm/drm_modes.c           | 34 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 12 +++++-------
 drivers/gpu/drm/sun4i/sun4i_rgb.c     | 22 ++++------------------
 include/drm/drm_modes.h               |  2 ++
 4 files changed, 45 insertions(+), 25 deletions(-)
---
base-commit: 28eb75e178d389d325f1666e422bc13bbbb9804c
change-id: 20241125-dsi-relax-1414baf6cd74

Best regards,
-- 
Sean Nyekjaer <sean@geanix.com>

[PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Sean Nyekjaer]

Check if the required pixel clock is in within .5% range of the
desired pixel clock.
This will match the requirement for HDMI where a .5% tolerance is allowed.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/gpu/drm/drm_modes.c | 34 ++++++++++++++++++++++++++++++++++
 include/drm/drm_modes.h     |  2 ++
 2 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 6ba167a3346134072d100af0adbbe9b49e970769..4068b904759bf80502efde6e4d977b297f5d5359 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1623,6 +1623,40 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 }
 EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
 
+/**
+ * drm_mode_validate_mode
+ * @mode: mode to check
+ * @rounded_rate: output pixel clock
+ *
+ * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
+ * CVT spec reuses that tolerance in its examples, so it looks to be a
+ * good default tolerance for the EDID-based modes. Define it to 5 per
+ * mille to avoid floating point operations.
+ *
+ * Returns:
+ * The mode status
+ */
+enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
+					    unsigned long long rounded_rate)
+{
+	enum drm_mode_status status;
+	unsigned long long rate = mode->clock * 1000;
+	unsigned long long lowest, highest;
+
+	lowest = rate * (1000 - 5);
+	do_div(lowest, 1000);
+	if (rounded_rate < lowest)
+		return MODE_CLOCK_LOW;
+
+	highest = rate * (1000 + 5);
+	do_div(highest, 1000);
+	if (rounded_rate > highest)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+EXPORT_SYMBOL(drm_mode_validate_mode);
+
 static enum drm_mode_status
 drm_mode_validate_basic(const struct drm_display_mode *mode)
 {
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index b9bb92e4b0295a5cbe0eb0da13e77449ff04f51d..4b638992f3e50d2aba5088644744457d72dbe10a 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -549,6 +549,8 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1,
 			      const struct drm_display_mode *mode2);
 bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 					const struct drm_display_mode *mode2);
+enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
+					    unsigned long long rounded_rate);
 
 /* for use by the crtc helper probe functions */
 enum drm_mode_status drm_mode_validate_driver(struct drm_device *dev,

-- 
2.46.2

[PATCH v2 2/3] drm/sun4i: use drm_mode_validate_mode() helper function

[Sean Nyekjaer]

Use new helper function for HDMI mode validation

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index dfb6acc42f02efc40f36914e2925510cd8056d0b..4f8100e32769cf31c25f5dd849a18f5b77376090 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -51,14 +51,6 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector)
 	return drm_panel_get_modes(rgb->panel, connector);
 }
 
-/*
- * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
- * CVT spec reuses that tolerance in its examples, so it looks to be a
- * good default tolerance for the EDID-based modes. Define it to 5 per
- * mille to avoid floating point operations.
- */
-#define SUN4I_RGB_DOTCLOCK_TOLERANCE_PER_MILLE	5
-
 static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
 						 const struct drm_display_mode *mode)
 {
@@ -67,8 +59,8 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
 	u32 hsync = mode->hsync_end - mode->hsync_start;
 	u32 vsync = mode->vsync_end - mode->vsync_start;
 	unsigned long long rate = mode->clock * 1000;
-	unsigned long long lowest, highest;
 	unsigned long long rounded_rate;
+	int ret;
 
 	DRM_DEBUG_DRIVER("Validating modes...\n");
 
@@ -122,15 +114,9 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
 	tcon->dclk_max_div = 127;
 	rounded_rate = clk_round_rate(tcon->dclk, rate);
 
-	lowest = rate * (1000 - SUN4I_RGB_DOTCLOCK_TOLERANCE_PER_MILLE);
-	do_div(lowest, 1000);
-	if (rounded_rate < lowest)
-		return MODE_CLOCK_LOW;
-
-	highest = rate * (1000 + SUN4I_RGB_DOTCLOCK_TOLERANCE_PER_MILLE);
-	do_div(highest, 1000);
-	if (rounded_rate > highest)
-		return MODE_CLOCK_HIGH;
+	ret = drm_mode_validate_mode(mode, rounded_rate);
+	if (ret)
+		return ret;
 
 out:
 	DRM_DEBUG_DRIVER("Clock rate OK\n");

-- 
2.46.2

[PATCH v2 3/3] drm/stm: dsi: use drm_mode_validate_mode() helper function

[Sean Nyekjaer]

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.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
index b20123854c4ad7b3a2cc973a26fc10fd433e8d09..7b32abe0d4f582eea1fbbacad48c84199be3fa23 100644
--- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
+++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
@@ -484,8 +484,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,
@@ -525,7 +523,7 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data,
 	}
 
 	if (!(mode_flags & MIPI_DSI_MODE_VIDEO_BURST)) {
-		unsigned int px_clock_hz, target_px_clock_hz, lane_mbps;
+		unsigned int px_clock_hz, lane_mbps;
 		int dsi_short_packet_size_px, hfp, hsync, hbp, delay_to_lp;
 		struct dw_mipi_dsi_dphy_timing dphy_timing;
 
@@ -533,14 +531,14 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data,
 		pll_out_khz = dsi_pll_get_clkout_khz(pll_in_khz, idf, ndiv, odf);
 
 		px_clock_hz = DIV_ROUND_CLOSEST_ULL(1000ULL * pll_out_khz * lanes, bpp);
-		target_px_clock_hz = mode->clock * 1000;
 		/*
 		 * Filter modes according to the clock value, particularly useful for
 		 * hdmi modes that require precise pixel clocks.
 		 */
-		if (px_clock_hz < target_px_clock_hz - CLK_TOLERANCE_HZ ||
-		    px_clock_hz > target_px_clock_hz + CLK_TOLERANCE_HZ)
-			return MODE_CLOCK_RANGE;
+
+		ret = drm_mode_validate_mode(mode, px_clock_hz);
+		if (ret)
+			return ret;
 
 		/* sync packets are codes as DSI short packets (4 bytes) */
 		dsi_short_packet_size_px = DIV_ROUND_UP(4 * BITS_PER_BYTE, bpp);

-- 
2.46.2

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

Hi Sean,

On Mon, Nov 25, 2024 at 02:49:26PM +0100, Sean Nyekjaer wrote:
> Check if the required pixel clock is in within .5% range of the
> desired pixel clock.
> This will match the requirement for HDMI where a .5% tolerance is allowed.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/gpu/drm/drm_modes.c | 34 ++++++++++++++++++++++++++++++++++
>  include/drm/drm_modes.h     |  2 ++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 6ba167a3346134072d100af0adbbe9b49e970769..4068b904759bf80502efde6e4d977b297f5d5359 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1623,6 +1623,40 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  }
>  EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
>  
> +/**
> + * drm_mode_validate_mode
> + * @mode: mode to check
> + * @rounded_rate: output pixel clock
> + *
> + * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
> + * CVT spec reuses that tolerance in its examples, so it looks to be a
> + * good default tolerance for the EDID-based modes. Define it to 5 per
> + * mille to avoid floating point operations.
> + *
> + * Returns:
> + * The mode status
> + */
> +enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
> +					    unsigned long long rounded_rate)
> +{
> +	enum drm_mode_status status;
> +	unsigned long long rate = mode->clock * 1000;
> +	unsigned long long lowest, highest;
> +
> +	lowest = rate * (1000 - 5);
> +	do_div(lowest, 1000);
> +	if (rounded_rate < lowest)
> +		return MODE_CLOCK_LOW;
> +
> +	highest = rate * (1000 + 5);
> +	do_div(highest, 1000);
> +	if (rounded_rate > highest)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +EXPORT_SYMBOL(drm_mode_validate_mode);

Thanks a lot for doing that!

I wonder about the naming though (and prototype). I doesn't really
validates a mode, but rather makes sure that a given rate is a good
approximation of a pixel clock. So maybe something like
drm_mode_check_pixel_clock?

We probably need some kunit tests here too.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 2/3] drm/sun4i: use drm_mode_validate_mode() helper function

[Maxime Ripard]

On Mon, 25 Nov 2024 14:49:27 +0100, Sean Nyekjaer wrote:
> Use new helper function for HDMI mode validation
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH v2 3/3] drm/stm: dsi: use drm_mode_validate_mode() helper function

[Maxime Ripard]

On Mon, 25 Nov 2024 14:49:28 +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.
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH v2 2/3] drm/sun4i: use drm_mode_validate_mode() helper function

[Chen-Yu Tsai]

On Mon, Nov 25, 2024 at 9:50 PM Sean Nyekjaer <sean@geanix.com> wrote:
>
> Use new helper function for HDMI mode validation

This is a bit misleading since this is actually the DPI or parallel
output encoder, not HDMI. HDMI is in drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
and drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c

ChenYu

> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_rgb.c | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> index dfb6acc42f02efc40f36914e2925510cd8056d0b..4f8100e32769cf31c25f5dd849a18f5b77376090 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> @@ -51,14 +51,6 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector)
>         return drm_panel_get_modes(rgb->panel, connector);
>  }
>
> -/*
> - * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
> - * CVT spec reuses that tolerance in its examples, so it looks to be a
> - * good default tolerance for the EDID-based modes. Define it to 5 per
> - * mille to avoid floating point operations.
> - */
> -#define SUN4I_RGB_DOTCLOCK_TOLERANCE_PER_MILLE 5
> -
>  static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
>                                                  const struct drm_display_mode *mode)
>  {
> @@ -67,8 +59,8 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
>         u32 hsync = mode->hsync_end - mode->hsync_start;
>         u32 vsync = mode->vsync_end - mode->vsync_start;
>         unsigned long long rate = mode->clock * 1000;
> -       unsigned long long lowest, highest;
>         unsigned long long rounded_rate;
> +       int ret;
>
>         DRM_DEBUG_DRIVER("Validating modes...\n");
>
> @@ -122,15 +114,9 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
>         tcon->dclk_max_div = 127;
>         rounded_rate = clk_round_rate(tcon->dclk, rate);
>
> -       lowest = rate * (1000 - SUN4I_RGB_DOTCLOCK_TOLERANCE_PER_MILLE);
> -       do_div(lowest, 1000);
> -       if (rounded_rate < lowest)
> -               return MODE_CLOCK_LOW;
> -
> -       highest = rate * (1000 + SUN4I_RGB_DOTCLOCK_TOLERANCE_PER_MILLE);
> -       do_div(highest, 1000);
> -       if (rounded_rate > highest)
> -               return MODE_CLOCK_HIGH;
> +       ret = drm_mode_validate_mode(mode, rounded_rate);
> +       if (ret)
> +               return ret;
>
>  out:
>         DRM_DEBUG_DRIVER("Clock rate OK\n");
>
> --
> 2.46.2
>
>

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Sean Nyekjaer]

Hi Maxime,

On Mon, Nov 25, 2024 at 05:00:56PM +0100, Maxime Ripard wrote:
> Hi Sean,
> 
> On Mon, Nov 25, 2024 at 02:49:26PM +0100, Sean Nyekjaer wrote:
> > Check if the required pixel clock is in within .5% range of the
> > desired pixel clock.
> > This will match the requirement for HDMI where a .5% tolerance is allowed.
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> >  drivers/gpu/drm/drm_modes.c | 34 ++++++++++++++++++++++++++++++++++
> >  include/drm/drm_modes.h     |  2 ++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 6ba167a3346134072d100af0adbbe9b49e970769..4068b904759bf80502efde6e4d977b297f5d5359 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1623,6 +1623,40 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> >  }
> >  EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
> >  
> > +/**
> > + * drm_mode_validate_mode
> > + * @mode: mode to check
> > + * @rounded_rate: output pixel clock
> > + *
> > + * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
> > + * CVT spec reuses that tolerance in its examples, so it looks to be a
> > + * good default tolerance for the EDID-based modes. Define it to 5 per
> > + * mille to avoid floating point operations.
> > + *
> > + * Returns:
> > + * The mode status
> > + */
> > +enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
> > +					    unsigned long long rounded_rate)
> > +{
> > +	enum drm_mode_status status;
> > +	unsigned long long rate = mode->clock * 1000;
> > +	unsigned long long lowest, highest;
> > +
> > +	lowest = rate * (1000 - 5);
> > +	do_div(lowest, 1000);
> > +	if (rounded_rate < lowest)
> > +		return MODE_CLOCK_LOW;
> > +
> > +	highest = rate * (1000 + 5);
> > +	do_div(highest, 1000);
> > +	if (rounded_rate > highest)
> > +		return MODE_CLOCK_HIGH;
> > +
> > +	return MODE_OK;
> > +}
> > +EXPORT_SYMBOL(drm_mode_validate_mode);
> 
> Thanks a lot for doing that!
> 
> I wonder about the naming though (and prototype). I doesn't really
> validates a mode, but rather makes sure that a given rate is a good
> approximation of a pixel clock. So maybe something like
> drm_mode_check_pixel_clock?

Naming is hard :) I will use drm_mode_check_pixel_clock() for V2.

Would it make sense to have the pixel clock requirement as a input
parameter? For HDMI it is 0.5% and in my case the LVDS panel 10%.

enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
					    unsigned long long rounded_rate, unsigned tolerance)
?

And maybe a drm_mode_validate_mode_hdmi() with the default tolerance of
.5%?
> 
> We probably need some kunit tests here too.

Good idea, will be my first :)

/Sean

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3124 bytes --]

Hi,

On Tue, Nov 26, 2024 at 08:36:00AM +0100, Sean Nyekjaer wrote:
> On Mon, Nov 25, 2024 at 05:00:56PM +0100, Maxime Ripard wrote:
> > On Mon, Nov 25, 2024 at 02:49:26PM +0100, Sean Nyekjaer wrote:
> > > Check if the required pixel clock is in within .5% range of the
> > > desired pixel clock.
> > > This will match the requirement for HDMI where a .5% tolerance is allowed.
> > > 
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > ---
> > >  drivers/gpu/drm/drm_modes.c | 34 ++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_modes.h     |  2 ++
> > >  2 files changed, 36 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > > index 6ba167a3346134072d100af0adbbe9b49e970769..4068b904759bf80502efde6e4d977b297f5d5359 100644
> > > --- a/drivers/gpu/drm/drm_modes.c
> > > +++ b/drivers/gpu/drm/drm_modes.c
> > > @@ -1623,6 +1623,40 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> > >  }
> > >  EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
> > >  
> > > +/**
> > > + * drm_mode_validate_mode
> > > + * @mode: mode to check
> > > + * @rounded_rate: output pixel clock
> > > + *
> > > + * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
> > > + * CVT spec reuses that tolerance in its examples, so it looks to be a
> > > + * good default tolerance for the EDID-based modes. Define it to 5 per
> > > + * mille to avoid floating point operations.
> > > + *
> > > + * Returns:
> > > + * The mode status
> > > + */
> > > +enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
> > > +					    unsigned long long rounded_rate)
> > > +{
> > > +	enum drm_mode_status status;
> > > +	unsigned long long rate = mode->clock * 1000;
> > > +	unsigned long long lowest, highest;
> > > +
> > > +	lowest = rate * (1000 - 5);
> > > +	do_div(lowest, 1000);
> > > +	if (rounded_rate < lowest)
> > > +		return MODE_CLOCK_LOW;
> > > +
> > > +	highest = rate * (1000 + 5);
> > > +	do_div(highest, 1000);
> > > +	if (rounded_rate > highest)
> > > +		return MODE_CLOCK_HIGH;
> > > +
> > > +	return MODE_OK;
> > > +}
> > > +EXPORT_SYMBOL(drm_mode_validate_mode);
> > 
> > Thanks a lot for doing that!
> > 
> > I wonder about the naming though (and prototype). I doesn't really
> > validates a mode, but rather makes sure that a given rate is a good
> > approximation of a pixel clock. So maybe something like
> > drm_mode_check_pixel_clock?
> 
> Naming is hard :) I will use drm_mode_check_pixel_clock() for V2.
> 
> Would it make sense to have the pixel clock requirement as a input
> parameter? For HDMI it is 0.5%

This code was only used for panels so far. It reuses the same tolerance
than HDMI because we couldn't come up with anything better, but it
should totally apply to other things.

> and in my case the LVDS panel 10%.

10% is a lot, and I'm not sure we'll want that. The framerate being
anywhere between 54 and 66 fps will trip a lot of applications too.

Why do you need such a big tolerance?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Raphael Gallais-Pou]

On 11/26/24 08:36, Sean Nyekjaer wrote:
> Hi Maxime,
>
> On Mon, Nov 25, 2024 at 05:00:56PM +0100, Maxime Ripard wrote:
>> Hi Sean,
>>
>> On Mon, Nov 25, 2024 at 02:49:26PM +0100, Sean Nyekjaer wrote:
>>> Check if the required pixel clock is in within .5% range of the
>>> desired pixel clock.
>>> This will match the requirement for HDMI where a .5% tolerance is allowed.
>>>
>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>> ---
>>>  drivers/gpu/drm/drm_modes.c | 34 ++++++++++++++++++++++++++++++++++
>>>  include/drm/drm_modes.h     |  2 ++
>>>  2 files changed, 36 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>>> index 6ba167a3346134072d100af0adbbe9b49e970769..4068b904759bf80502efde6e4d977b297f5d5359 100644
>>> --- a/drivers/gpu/drm/drm_modes.c
>>> +++ b/drivers/gpu/drm/drm_modes.c
>>> @@ -1623,6 +1623,40 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>>>  }
>>>  EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
>>>  
>>> +/**
>>> + * drm_mode_validate_mode
>>> + * @mode: mode to check
>>> + * @rounded_rate: output pixel clock
>>> + *
>>> + * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
>>> + * CVT spec reuses that tolerance in its examples, so it looks to be a
>>> + * good default tolerance for the EDID-based modes. Define it to 5 per
>>> + * mille to avoid floating point operations.
>>> + *
>>> + * Returns:
>>> + * The mode status
>>> + */
>>> +enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
>>> +					    unsigned long long rounded_rate)
>>> +{
>>> +	enum drm_mode_status status;
>>> +	unsigned long long rate = mode->clock * 1000;
>>> +	unsigned long long lowest, highest;
>>> +
>>> +	lowest = rate * (1000 - 5);
>>> +	do_div(lowest, 1000);
>>> +	if (rounded_rate < lowest)
>>> +		return MODE_CLOCK_LOW;
>>> +
>>> +	highest = rate * (1000 + 5);
>>> +	do_div(highest, 1000);
>>> +	if (rounded_rate > highest)
>>> +		return MODE_CLOCK_HIGH;
>>> +
>>> +	return MODE_OK;
>>> +}
>>> +EXPORT_SYMBOL(drm_mode_validate_mode);
Hi Sean, Maxime,
>> Thanks a lot for doing that!
>>
>> I wonder about the naming though (and prototype). I doesn't really
>> validates a mode, but rather makes sure that a given rate is a good
>> approximation of a pixel clock. So maybe something like
>> drm_mode_check_pixel_clock?
> Naming is hard :) I will use drm_mode_check_pixel_clock() for V2.
>
> Would it make sense to have the pixel clock requirement as a input
> parameter? For HDMI it is 0.5% and in my case the LVDS panel 10%.
>
> enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
> 					    unsigned long long rounded_rate, unsigned tolerance)
> ?


IMO adding the tolerance as input parameter is a good idea.  This would useful
other than for HDMI pixel clock validation (and LVDS in your case).

Best regards,
Raphaël

>
> And maybe a drm_mode_validate_mode_hdmi() with the default tolerance of
> .5%?
>> We probably need some kunit tests here too.
> Good idea, will be my first :)
>
> /Sean

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3368 bytes --]

On Tue, Nov 26, 2024 at 09:47:17AM +0100, Raphael Gallais-Pou wrote:
> 
> On 11/26/24 08:36, Sean Nyekjaer wrote:
> > Hi Maxime,
> >
> > On Mon, Nov 25, 2024 at 05:00:56PM +0100, Maxime Ripard wrote:
> >> Hi Sean,
> >>
> >> On Mon, Nov 25, 2024 at 02:49:26PM +0100, Sean Nyekjaer wrote:
> >>> Check if the required pixel clock is in within .5% range of the
> >>> desired pixel clock.
> >>> This will match the requirement for HDMI where a .5% tolerance is allowed.
> >>>
> >>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> >>> ---
> >>>  drivers/gpu/drm/drm_modes.c | 34 ++++++++++++++++++++++++++++++++++
> >>>  include/drm/drm_modes.h     |  2 ++
> >>>  2 files changed, 36 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> >>> index 6ba167a3346134072d100af0adbbe9b49e970769..4068b904759bf80502efde6e4d977b297f5d5359 100644
> >>> --- a/drivers/gpu/drm/drm_modes.c
> >>> +++ b/drivers/gpu/drm/drm_modes.c
> >>> @@ -1623,6 +1623,40 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> >>>  }
> >>>  EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
> >>>  
> >>> +/**
> >>> + * drm_mode_validate_mode
> >>> + * @mode: mode to check
> >>> + * @rounded_rate: output pixel clock
> >>> + *
> >>> + * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
> >>> + * CVT spec reuses that tolerance in its examples, so it looks to be a
> >>> + * good default tolerance for the EDID-based modes. Define it to 5 per
> >>> + * mille to avoid floating point operations.
> >>> + *
> >>> + * Returns:
> >>> + * The mode status
> >>> + */
> >>> +enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
> >>> +					    unsigned long long rounded_rate)
> >>> +{
> >>> +	enum drm_mode_status status;
> >>> +	unsigned long long rate = mode->clock * 1000;
> >>> +	unsigned long long lowest, highest;
> >>> +
> >>> +	lowest = rate * (1000 - 5);
> >>> +	do_div(lowest, 1000);
> >>> +	if (rounded_rate < lowest)
> >>> +		return MODE_CLOCK_LOW;
> >>> +
> >>> +	highest = rate * (1000 + 5);
> >>> +	do_div(highest, 1000);
> >>> +	if (rounded_rate > highest)
> >>> +		return MODE_CLOCK_HIGH;
> >>> +
> >>> +	return MODE_OK;
> >>> +}
> >>> +EXPORT_SYMBOL(drm_mode_validate_mode);
> Hi Sean, Maxime,
> >> Thanks a lot for doing that!
> >>
> >> I wonder about the naming though (and prototype). I doesn't really
> >> validates a mode, but rather makes sure that a given rate is a good
> >> approximation of a pixel clock. So maybe something like
> >> drm_mode_check_pixel_clock?
> > Naming is hard :) I will use drm_mode_check_pixel_clock() for V2.
> >
> > Would it make sense to have the pixel clock requirement as a input
> > parameter? For HDMI it is 0.5% and in my case the LVDS panel 10%.
> >
> > enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
> > 					    unsigned long long rounded_rate, unsigned tolerance)
> > ?
> 
> 
> IMO adding the tolerance as input parameter is a good idea.  This
> would useful other than for HDMI pixel clock validation (and LVDS in
> your case).

It depends on the intent. If it's justified, why not. If it's to
workaround another issue, absolutely not. And so, generally speaking, I
don't think it's a good idea.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Jani Nikula]

On Mon, 25 Nov 2024, Maxime Ripard <mripard@kernel.org> wrote:
> I wonder about the naming though (and prototype). I doesn't really
> validates a mode, but rather makes sure that a given rate is a good
> approximation of a pixel clock. So maybe something like
> drm_mode_check_pixel_clock?

Quoting myself from a few weeks back:

"""
Random programming thought of the day: "check" is generally a terrible
word in a function name.

Checking stuff is great, but what do you expect to happen if the check
passes/fails? Do you expect the function to return on fail, or throw an
exception? Or just log about it? If you return a value, what should the
return value mean? It's hard to know without looking it up.

Prefer predicates instead, is_stuff_okay() is better than
check_stuff(). Or assert_stuff() if you don't return on failures.
"""


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Sean Nyekjaer]

Hi,

On Tue, Nov 26, 2024 at 09:38:55AM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Nov 26, 2024 at 08:36:00AM +0100, Sean Nyekjaer wrote:
> > On Mon, Nov 25, 2024 at 05:00:56PM +0100, Maxime Ripard wrote:
> > > On Mon, Nov 25, 2024 at 02:49:26PM +0100, Sean Nyekjaer wrote:
> > > > Check if the required pixel clock is in within .5% range of the
> > > > desired pixel clock.
> > > > This will match the requirement for HDMI where a .5% tolerance is allowed.
> > > > 
> > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_modes.c | 34 ++++++++++++++++++++++++++++++++++
> > > >  include/drm/drm_modes.h     |  2 ++
> > > >  2 files changed, 36 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > > > index 6ba167a3346134072d100af0adbbe9b49e970769..4068b904759bf80502efde6e4d977b297f5d5359 100644
> > > > --- a/drivers/gpu/drm/drm_modes.c
> > > > +++ b/drivers/gpu/drm/drm_modes.c
> > > > @@ -1623,6 +1623,40 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> > > >  }
> > > >  EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
> > > >  
> > > > +/**
> > > > + * drm_mode_validate_mode
> > > > + * @mode: mode to check
> > > > + * @rounded_rate: output pixel clock
> > > > + *
> > > > + * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
> > > > + * CVT spec reuses that tolerance in its examples, so it looks to be a
> > > > + * good default tolerance for the EDID-based modes. Define it to 5 per
> > > > + * mille to avoid floating point operations.
> > > > + *
> > > > + * Returns:
> > > > + * The mode status
> > > > + */
> > > > +enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
> > > > +					    unsigned long long rounded_rate)
> > > > +{
> > > > +	enum drm_mode_status status;
> > > > +	unsigned long long rate = mode->clock * 1000;
> > > > +	unsigned long long lowest, highest;
> > > > +
> > > > +	lowest = rate * (1000 - 5);
> > > > +	do_div(lowest, 1000);
> > > > +	if (rounded_rate < lowest)
> > > > +		return MODE_CLOCK_LOW;
> > > > +
> > > > +	highest = rate * (1000 + 5);
> > > > +	do_div(highest, 1000);
> > > > +	if (rounded_rate > highest)
> > > > +		return MODE_CLOCK_HIGH;
> > > > +
> > > > +	return MODE_OK;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_mode_validate_mode);
> > > 
> > > Thanks a lot for doing that!
> > > 
> > > I wonder about the naming though (and prototype). I doesn't really
> > > validates a mode, but rather makes sure that a given rate is a good
> > > approximation of a pixel clock. So maybe something like
> > > drm_mode_check_pixel_clock?
> > 
> > Naming is hard :) I will use drm_mode_check_pixel_clock() for V2.
> > 
> > Would it make sense to have the pixel clock requirement as a input
> > parameter? For HDMI it is 0.5%
> 
> This code was only used for panels so far. It reuses the same tolerance
> than HDMI because we couldn't come up with anything better, but it
> should totally apply to other things.
> 
> > and in my case the LVDS panel 10%.
> 
> 10% is a lot, and I'm not sure we'll want that. The framerate being
> anywhere between 54 and 66 fps will trip a lot of applications too.
> 
> Why do you need such a big tolerance?

I don't need it, it was just from the datasheet for the LVDS panel :)

> 
> Maxime

/Sean

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3928 bytes --]

On Tue, Nov 26, 2024 at 12:34:26PM +0100, Sean Nyekjaer wrote:
> Hi,
> 
> On Tue, Nov 26, 2024 at 09:38:55AM +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Nov 26, 2024 at 08:36:00AM +0100, Sean Nyekjaer wrote:
> > > On Mon, Nov 25, 2024 at 05:00:56PM +0100, Maxime Ripard wrote:
> > > > On Mon, Nov 25, 2024 at 02:49:26PM +0100, Sean Nyekjaer wrote:
> > > > > Check if the required pixel clock is in within .5% range of the
> > > > > desired pixel clock.
> > > > > This will match the requirement for HDMI where a .5% tolerance is allowed.
> > > > > 
> > > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_modes.c | 34 ++++++++++++++++++++++++++++++++++
> > > > >  include/drm/drm_modes.h     |  2 ++
> > > > >  2 files changed, 36 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > > > > index 6ba167a3346134072d100af0adbbe9b49e970769..4068b904759bf80502efde6e4d977b297f5d5359 100644
> > > > > --- a/drivers/gpu/drm/drm_modes.c
> > > > > +++ b/drivers/gpu/drm/drm_modes.c
> > > > > @@ -1623,6 +1623,40 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> > > > >  }
> > > > >  EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
> > > > >  
> > > > > +/**
> > > > > + * drm_mode_validate_mode
> > > > > + * @mode: mode to check
> > > > > + * @rounded_rate: output pixel clock
> > > > > + *
> > > > > + * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
> > > > > + * CVT spec reuses that tolerance in its examples, so it looks to be a
> > > > > + * good default tolerance for the EDID-based modes. Define it to 5 per
> > > > > + * mille to avoid floating point operations.
> > > > > + *
> > > > > + * Returns:
> > > > > + * The mode status
> > > > > + */
> > > > > +enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
> > > > > +					    unsigned long long rounded_rate)
> > > > > +{
> > > > > +	enum drm_mode_status status;
> > > > > +	unsigned long long rate = mode->clock * 1000;
> > > > > +	unsigned long long lowest, highest;
> > > > > +
> > > > > +	lowest = rate * (1000 - 5);
> > > > > +	do_div(lowest, 1000);
> > > > > +	if (rounded_rate < lowest)
> > > > > +		return MODE_CLOCK_LOW;
> > > > > +
> > > > > +	highest = rate * (1000 + 5);
> > > > > +	do_div(highest, 1000);
> > > > > +	if (rounded_rate > highest)
> > > > > +		return MODE_CLOCK_HIGH;
> > > > > +
> > > > > +	return MODE_OK;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_mode_validate_mode);
> > > > 
> > > > Thanks a lot for doing that!
> > > > 
> > > > I wonder about the naming though (and prototype). I doesn't really
> > > > validates a mode, but rather makes sure that a given rate is a good
> > > > approximation of a pixel clock. So maybe something like
> > > > drm_mode_check_pixel_clock?
> > > 
> > > Naming is hard :) I will use drm_mode_check_pixel_clock() for V2.
> > > 
> > > Would it make sense to have the pixel clock requirement as a input
> > > parameter? For HDMI it is 0.5%
> > 
> > This code was only used for panels so far. It reuses the same tolerance
> > than HDMI because we couldn't come up with anything better, but it
> > should totally apply to other things.
> > 
> > > and in my case the LVDS panel 10%.
> > 
> > 10% is a lot, and I'm not sure we'll want that. The framerate being
> > anywhere between 54 and 66 fps will trip a lot of applications too.
> > 
> > Why do you need such a big tolerance?
> 
> I don't need it, it was just from the datasheet for the LVDS panel :)

So you mean the panel accepts a pixel clock within +/- 10%?

That makes sense, but then we should also adjust the mode timings to
match so we still keep 60fps. There's much more to *that* than the
helpers you try to create though, so let's keep it aside for now.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

On Tue, Nov 26, 2024 at 12:16:34PM +0200, Jani Nikula wrote:
> On Mon, 25 Nov 2024, Maxime Ripard <mripard@kernel.org> wrote:
> > I wonder about the naming though (and prototype). I doesn't really
> > validates a mode, but rather makes sure that a given rate is a good
> > approximation of a pixel clock. So maybe something like
> > drm_mode_check_pixel_clock?
> 
> Quoting myself from a few weeks back:
> 
> """
> Random programming thought of the day: "check" is generally a terrible
> word in a function name.
> 
> Checking stuff is great, but what do you expect to happen if the check
> passes/fails? Do you expect the function to return on fail, or throw an
> exception? Or just log about it? If you return a value, what should the
> return value mean? It's hard to know without looking it up.
> 
> Prefer predicates instead, is_stuff_okay() is better than
> check_stuff(). Or assert_stuff() if you don't return on failures.
> """

Both is_stuff_okay() or assert_stuff() return a boolean in my mind. If
you want to return a mode status enum, I don't think they are better
names.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Sean Nyekjaer]

On Tue, Nov 26, 2024 at 01:09:10PM +0100, Maxime Ripard wrote:
> On Tue, Nov 26, 2024 at 12:34:26PM +0100, Sean Nyekjaer wrote:
> > Hi,
> > 
> > On Tue, Nov 26, 2024 at 09:38:55AM +0100, Maxime Ripard wrote:
> > > Hi,
> > > 
> > > On Tue, Nov 26, 2024 at 08:36:00AM +0100, Sean Nyekjaer wrote:
> > > > On Mon, Nov 25, 2024 at 05:00:56PM +0100, Maxime Ripard wrote:
> > > > > On Mon, Nov 25, 2024 at 02:49:26PM +0100, Sean Nyekjaer wrote:

[...]

> > > > > 
> > > > > Thanks a lot for doing that!
> > > > > 
> > > > > I wonder about the naming though (and prototype). I doesn't really
> > > > > validates a mode, but rather makes sure that a given rate is a good
> > > > > approximation of a pixel clock. So maybe something like
> > > > > drm_mode_check_pixel_clock?
> > > > 
> > > > Naming is hard :) I will use drm_mode_check_pixel_clock() for V2.
> > > > 
> > > > Would it make sense to have the pixel clock requirement as a input
> > > > parameter? For HDMI it is 0.5%
> > > 
> > > This code was only used for panels so far. It reuses the same tolerance
> > > than HDMI because we couldn't come up with anything better, but it
> > > should totally apply to other things.
> > > 
> > > > and in my case the LVDS panel 10%.
> > > 
> > > 10% is a lot, and I'm not sure we'll want that. The framerate being
> > > anywhere between 54 and 66 fps will trip a lot of applications too.
> > > 
> > > Why do you need such a big tolerance?
> > 
> > I don't need it, it was just from the datasheet for the LVDS panel :)
> 
> So you mean the panel accepts a pixel clock within +/- 10%?

Yes :)

> 
> That makes sense, but then we should also adjust the mode timings to
> match so we still keep 60fps. There's much more to *that* than the
> helpers you try to create though, so let's keep it aside for now.

Ok

> 
> Maxime

/Sean

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Jani Nikula]

On Tue, 26 Nov 2024, Maxime Ripard <mripard@kernel.org> wrote:
> On Tue, Nov 26, 2024 at 12:16:34PM +0200, Jani Nikula wrote:
>> On Mon, 25 Nov 2024, Maxime Ripard <mripard@kernel.org> wrote:
>> > I wonder about the naming though (and prototype). I doesn't really
>> > validates a mode, but rather makes sure that a given rate is a good
>> > approximation of a pixel clock. So maybe something like
>> > drm_mode_check_pixel_clock?
>> 
>> Quoting myself from a few weeks back:
>> 
>> """
>> Random programming thought of the day: "check" is generally a terrible
>> word in a function name.
>> 
>> Checking stuff is great, but what do you expect to happen if the check
>> passes/fails? Do you expect the function to return on fail, or throw an
>> exception? Or just log about it? If you return a value, what should the
>> return value mean? It's hard to know without looking it up.
>> 
>> Prefer predicates instead, is_stuff_okay() is better than
>> check_stuff(). Or assert_stuff() if you don't return on failures.
>> """
>
> Both is_stuff_okay() or assert_stuff() return a boolean in my mind. If
> you want to return a mode status enum, I don't think they are better
> names.

Most functions returning enum drm_mode_status are called
something_something_mode_valid(). Not check something.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Sean Nyekjaer]

Hi Maxime,

On Tue, Nov 26, 2024 at 08:36:01AM +0100, Sean Nyekjaer wrote:
> Hi Maxime,
> 

[...]

> > 
> > We probably need some kunit tests here too.
> 
> Good idea, will be my first :)
> 

Would something like this work?

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index 294773342e71..26e4ff02df85 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -1364,6 +1364,7 @@ static void drm_test_check_output_bpc_format_display_rgb_only(struct kunit *test
 	struct drm_connector_state *conn_state;
 	struct drm_display_info *info;
 	struct drm_display_mode *preferred;
+	enum drm_mode_status mode_status;
 	unsigned long long rate;
 	struct drm_connector *conn;
 	struct drm_device *drm;
@@ -1408,6 +1409,9 @@ static void drm_test_check_output_bpc_format_display_rgb_only(struct kunit *test
 	rate = drm_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_YUV422);
 	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
 
+	mode_status = drm_mode_check_pixel_clock(preferred, rate);
+	KUNIT_ASSERT_EQ(test, mode_status, MODE_OK);
+
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	ret = light_up_connector(test, drm, crtc, conn, preferred, ctx);

/Sean

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]

On Tue, Nov 26, 2024 at 02:24:12PM +0200, Jani Nikula wrote:
> On Tue, 26 Nov 2024, Maxime Ripard <mripard@kernel.org> wrote:
> > On Tue, Nov 26, 2024 at 12:16:34PM +0200, Jani Nikula wrote:
> >> On Mon, 25 Nov 2024, Maxime Ripard <mripard@kernel.org> wrote:
> >> > I wonder about the naming though (and prototype). I doesn't really
> >> > validates a mode, but rather makes sure that a given rate is a good
> >> > approximation of a pixel clock. So maybe something like
> >> > drm_mode_check_pixel_clock?
> >> 
> >> Quoting myself from a few weeks back:
> >> 
> >> """
> >> Random programming thought of the day: "check" is generally a terrible
> >> word in a function name.
> >> 
> >> Checking stuff is great, but what do you expect to happen if the check
> >> passes/fails? Do you expect the function to return on fail, or throw an
> >> exception? Or just log about it? If you return a value, what should the
> >> return value mean? It's hard to know without looking it up.
> >> 
> >> Prefer predicates instead, is_stuff_okay() is better than
> >> check_stuff(). Or assert_stuff() if you don't return on failures.
> >> """
> >
> > Both is_stuff_okay() or assert_stuff() return a boolean in my mind. If
> > you want to return a mode status enum, I don't think they are better
> > names.
> 
> Most functions returning enum drm_mode_status are called
> something_something_mode_valid(). Not check something.

But it doesn't check whether the mode is valid or not. It checks whether
a given clock rate is within reasonable tolerance from the expected
pixel clock.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 269 bytes --]

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1505 bytes --]

On Tue, Nov 26, 2024 at 02:30:53PM +0100, Sean Nyekjaer wrote:
> Hi Maxime,
> 
> On Tue, Nov 26, 2024 at 08:36:01AM +0100, Sean Nyekjaer wrote:
> > Hi Maxime,
> > 
> 
> [...]
> 
> > > 
> > > We probably need some kunit tests here too.
> > 
> > Good idea, will be my first :)
> > 
> 
> Would something like this work?
> 
> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index 294773342e71..26e4ff02df85 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -1364,6 +1364,7 @@ static void drm_test_check_output_bpc_format_display_rgb_only(struct kunit *test
>  	struct drm_connector_state *conn_state;
>  	struct drm_display_info *info;
>  	struct drm_display_mode *preferred;
> +	enum drm_mode_status mode_status;
>  	unsigned long long rate;
>  	struct drm_connector *conn;
>  	struct drm_device *drm;
> @@ -1408,6 +1409,9 @@ static void drm_test_check_output_bpc_format_display_rgb_only(struct kunit *test
>  	rate = drm_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_YUV422);
>  	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
>  
> +	mode_status = drm_mode_check_pixel_clock(preferred, rate);
> +	KUNIT_ASSERT_EQ(test, mode_status, MODE_OK);
> +

Not really. You need to test that function, and ideally only that
function, with a bunch of random inputs, including the error conditions.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 3/3] drm/stm: dsi: use drm_mode_validate_mode() helper function

[Raphael Gallais-Pou]

On 11/25/24 14:49, 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. Signed-off-by: Sean Nyekjaer <sean@geanix.com>---


Hi Sean,

Acked-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>

Thanks,
Raphaël

Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode() helper function

[Raphael Gallais-Pou]

On 11/25/24 14:49, Sean Nyekjaer wrote:
> Check if the required pixel clock is in within .5% range of the
> desired pixel clock.
> This will match the requirement for HDMI where a .5% tolerance is allowed.
>
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/gpu/drm/drm_modes.c | 34 ++++++++++++++++++++++++++++++++++
>  include/drm/drm_modes.h     |  2 ++
>  2 files changed, 36 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 6ba167a3346134072d100af0adbbe9b49e970769..4068b904759bf80502efde6e4d977b297f5d5359 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1623,6 +1623,40 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  }
>  EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
>  
> +/**
> + * drm_mode_validate_mode
> + * @mode: mode to check
> + * @rounded_rate: output pixel clock
> + *
> + * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
> + * CVT spec reuses that tolerance in its examples, so it looks to be a
> + * good default tolerance for the EDID-based modes. Define it to 5 per
> + * mille to avoid floating point operations.
> + *
> + * Returns:
> + * The mode status
> + */
> +enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
> +					    unsigned long long rounded_rate)
> +{
> +	enum drm_mode_status status;
> +	unsigned long long rate = mode->clock * 1000;
> +	unsigned long long lowest, highest;
> +
> +	lowest = rate * (1000 - 5);
> +	do_div(lowest, 1000);
> +	if (rounded_rate < lowest)
> +		return MODE_CLOCK_LOW;
> +
> +	highest = rate * (1000 + 5);
> +	do_div(highest, 1000);
> +	if (rounded_rate > highest)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +EXPORT_SYMBOL(drm_mode_validate_mode);

Hi,

With an agreement reached by everyone on the name of this function:

Reviewed-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>

Regards,
Raphaël

> +
>  static enum drm_mode_status
>  drm_mode_validate_basic(const struct drm_display_mode *mode)
>  {
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index b9bb92e4b0295a5cbe0eb0da13e77449ff04f51d..4b638992f3e50d2aba5088644744457d72dbe10a 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -549,6 +549,8 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1,
>  			      const struct drm_display_mode *mode2);
>  bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  					const struct drm_display_mode *mode2);
> +enum drm_mode_status drm_mode_validate_mode(const struct drm_display_mode *mode,
> +					    unsigned long long rounded_rate);
>  
>  /* for use by the crtc helper probe functions */
>  enum drm_mode_status drm_mode_validate_driver(struct drm_device *dev,
>