[PATCH v2 1/3] drm/vblank: Add intro to documentation

[Liviu Dudau]

Hi Sam,

Sorry for jumping in late on this, I have just a small suggestion:

On Mon, Apr 06, 2020 at 09:47:44PM +0200, Sam Ravnborg wrote:
> Lyude Paul wrote a very good intro to vblank here:
> https://lore.kernel.org/dri-devel/faf63d8a9ed23c16af69762f59d0dca6b2bf085f.camel@redhat.com/T/#mce6480be738160e9d07c5d023e88fd78d7a06d27
> 
> Add this to the intro chapter in drm_vblank.c so others
> can benefit from it too.
> 
> v2:
>   - Reworded to improve readability (Thomas)
> 
> v3:
>   - Added nice ascii drawing from Lyude (Lyude)
>   - Added referende to high-precision timestamp (Daniel)
>   - Improved grammar (Thomas)
>   - Combined it all and made kernel-doc happy
>   - Dropped any a-b, r-b do to the amount of changes
> 
> Signed-off-by: Sam Ravnborg <sam at ravnborg.org>
> Co-developed-by: Lyude Paul <lyude at redhat.com>
> Cc: Lyude Paul <lyude at redhat.com>
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Maxime Ripard <mripard at kernel.org>
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> Cc: David Airlie <airlied at linux.ie>
> ---
>  drivers/gpu/drm/drm_vblank.c | 53 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index bcf346b3e486..9633092c9ad5 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -41,6 +41,59 @@
>  /**
>   * DOC: vblank handling
>   *
> + * From the computer's perspective, every time the monitor displays
> + * a new frame the scanout engine have "scanned out" the display image
> + * from top to bottom, one row of pixels at a time.
> + * The current row of pixels is referred to as the current scanline.
> + *
> + * In addition to the display's visible area, there's usually a couple of
> + * extra scanlines which aren't actually displayed on the screen.
> + * These extra scanlines don't contain image data and are occasionally used
> + * for features like audio and infoframes. The region made up of these
> + * scanlines is referred to as the vertical blanking region, or vblank for
> + * short.
> + *
> + * ::
> + *
> + *
> + *    physical →   ⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽
> + *    top of      |                                        |
> + *    display     |                                        |
> + *                |               New frame                |
> + *                |                                        |
> + *                |↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓|
> + *                |~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~| ← Scanline, updates
> + *                |↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓|   the frame as it
> + *                |                                        |   travels down
> + *                |                                        |   ("scan out")
> + *                |                                        |
> + *                |               Old frame                |
> + *                |                                        |
> + *                |                                        |
> + *                |                                        |
> + *                |                                        |   physical
> + *                |                                        |   bottom of
> + *    vertical    |⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽| ← display
> + *    blanking    ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆
> + *    region   →  ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆
> + *                ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆
> + *    start of →   ⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽
> + *    new frame
> + *
> + * "Physical top of display" is the reference point for the high-precision/
> + * corrected timestamp.
> + *
> + * On a lot of display hardware, programming needs to take effect during the
> + * vertical blanking period so that settings like gamma, the image buffer
> + * buffer to be scanned out, etc. can safely be changed without showing
> + * any visual artifacts on the screen. In some unforgiving hardware, some of
> + * this programming has to both start and end in the same vblank.

The mention of vblank interrupt in the next paragraph seems a bit sudden to me. Maybe
you can add at the end of the paragraph above something like:

   To help with the timing of the hardware programming, an interrupt is usually
   available to notify the driver when it can start the updating of registers.


> + *
> + * The vblank interrupt may be fired at different points depending on the
> + * hardware. Some hardware implementations will fire the interrupt when the
> + * new frame start, other implementations will fire the interrupt at different
> + * points in time.
> + *
>   * Vertical blanking plays a major role in graphics rendering. To achieve
>   * tear-free display, users must synchronize page flips and/or rendering to
>   * vertical blanking. The DRM API offers ioctls to perform page flips
> -- 
> 2.20.1
> 

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯