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

[Maxime Ripard <mripard@kernel.org>]

[-- 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 --]