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

From: Thomas Zimmermann
Date: Mon Sep 26 2022 - 10:41:55 EST


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@xxxxxxxxxx> 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: OpenPGP_signature
Description: OpenPGP digital signature