linux-kernel - Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <725aafb8-abfa-40c0-967a-62122206f736@foss.st.com>
Date: Tue, 26 Nov 2024 09:47:17 +0100
From: Raphael Gallais-Pou <raphael.gallais-pou@...s.st.com>
To: Sean Nyekjaer <sean@...nix.com>, Maxime Ripard <mripard@...nel.org>
CC: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Thomas Zimmermann
	<tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>, Simona Vetter
	<simona@...ll.ch>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec
	<jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        Yannick
 Fertre <yannick.fertre@...s.st.com>,
        Philippe Cornu
	<philippe.cornu@...s.st.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        <dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-sunxi@...ts.linux.dev>,
        <linux-stm32@...md-mailman.stormreply.com>
Subject: Re: [PATCH v2 1/3] drm/modes: introduce drm_mode_validate_mode()
 helper function


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@...nix.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