Re: [PATCH v2 10/33] drm/modes: Add a function to generate analog display modes

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Maxime Ripard <maxime@cerno.tech>
Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>,
	"Karol Herbst" <kherbst@redhat.com>,
	"David Airlie" <airlied@linux.ie>,
	nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	"Phil Elwell" <phil@raspberrypi.com>,
	"Emma Anholt" <emma@anholt.net>,
	"Samuel Holland" <samuel@sholland.org>,
	"Jernej Skrabec" <jernej.skrabec@gmail.com>,
	"Chen-Yu Tsai" <wens@csie.org>, "Ben Skeggs" <bskeggs@redhat.com>,
	linux-sunxi@lists.linux.dev, intel-gfx@lists.freedesktop.org,
	"Hans de Goede" <hdegoede@redhat.com>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	linux-arm-kernel@lists.infradead.org,
	"Tvrtko Ursulin" <tvrtko.ursulin@linux.intel.com>,
	"Dom Cobley" <dom@raspberrypi.com>,
	"Dave Stevenson" <dave.stevenson@raspberrypi.com>,
	linux-kernel@vger.kernel.org,
	"Mateusz Kwiatkowski" <kfyatek+publicgit@gmail.com>,
	"Noralf Trønnes" <noralf@tronnes.org>
Subject: Re: [PATCH v2 10/33] drm/modes: Add a function to generate analog display modes
Date: Mon, 26 Sep 2022 15:02:56 +0200	[thread overview]
Message-ID: <fb2dcbdd-057b-c3e6-0be7-3a8ee5822d4d@suse.de> (raw)
In-Reply-To: <20220926124259.o6fcfnxjy7gyzqz2@houat>

[-- Attachment #1.1: Type: text/plain, Size: 2453 bytes --]

Hi

Am 26.09.22 um 14:42 schrieb Maxime Ripard:
> On Mon, Sep 26, 2022 at 01:17:52PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 26.09.22 um 12:34 schrieb Geert Uytterhoeven:
>>> Hi Maxime,
>>>
>>> On Mon, Sep 26, 2022 at 12:17 PM Maxime Ripard <maxime@cerno.tech> wrote:
>>>> On Fri, Sep 23, 2022 at 11:05:48AM +0200, Thomas Zimmermann wrote:
>>>>>> +   /* 63.556us * 13.5MHz = 858 pixels */
>>>>>
>>>>> I kind of get what the comment wants to tell me, but the units don't add up.
>>>>
>>>> I'm not sure how it doesn't add up?
>>>>
>>>> We have a frequency in Hz (equivalent to s^-1) and a duration in s, so
>>>> the result ends up with no dimension, which is to be expected for a
>>>> number of periods?
>>>
>>> To make the units add up, it should be 13.5 Mpixel/s
>>> (which is what a pixel clock of 13.5 MHz really means ;-)
>>
>> Sort of. It leaves the time value as a magic number, which obfuscates what's
>> happening.
>>
>> The unit for htotal is pixels/scanline because if you multiply it with the
>> number of scanlines per frame (which is in vtotal), you get pixels/frame.
>> Multiplying with the frames per second results in the pixel clock in
>> pixels/second.
> 
> That's true, but both are true?

I'm not quite sure what you mean. I tried to say that this magic time 
value makes all this hard to see.

> 
>> That's a bit much for this comment. Hence, I suggested to remove these
>> comments entirely and document the relation among the numbers in a more
>> prominent location. The documentation for drm_display_mode would be a good
>> place, I guess.
> 
> I'm not sure I understand what it's about. It's an explicit requirement
> of PAL and NTSC, why would something so specific be in the generic
> definition of drm_display_mode?

Not just TV signals, it's the case for all displays were we control the 
electron beam in some way (VGA). Such documentation could therefore be 
added to DRM in an appropriate place. That makes it easier for newcomers 
to see why certain modes are defined the way they are. (At first, 
display modes can look like they are made up randomly.)

For your test cases, maybe simply refer to the relevant standard documents.

Best regards
Thomas

> 
> Maxime

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]