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

[Daniel Vetter]

On Mon, Mar 30, 2020 at 05:51:26PM -0400, Lyude Paul wrote:
> I am glad that my explanation of vblanks made sense! Some comments below on
> things I think we could improve here
> 
> On Mon, 2020-03-30 at 20:57 +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)
> > 
> > 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>
> > Acked-by: Thomas Zimmermann <tzimmermann at suse.de>
> > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > Cc: Daniel Vetter <daniel 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 | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index bcf346b3e486..ec2c2083b186 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -41,6 +41,23 @@
> >  /**
> >   * 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
> > + * (the extra scanlines are sometimes used by HDMI audio and friends).
> > + * The period where the extra scanlines are "scanned out" is referred
> > + * to as the vertical blanking period (vblank for short).
> 
> I'd reword this, starting from "(the extra scanlines…" (I'd also remove the
> paranthesis):
> 
>     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.
> 
> I'd also add a simple ascii-art diagram here, since this might make a lot more
> sense to some people if there's a visual reference. Probably something like
> this (feel free to get a little creative)
> 
>      ⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽
>     |                                                |
>     |                                                |
>     |                   New frame                    |
>     |                                                |
>     |↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓|
>     |~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~| ← Scanline, updates the
>     |↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓|   frame as it travels
>     |                                                |   down (AKA "scans out")
>     |                                                |
>     |                                                |
>     |                   Old frame                    |
>     |                                                |
>     |                                                |
>     |                                                |
>     |                                                |
>     |                                                |   physical bottom of
>     |⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽| ← display
>     ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆
>     ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆ ← vertical blanking
>     ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆   region
>      ------------------------------------------------

Oh if we go with a nice asci-art picture, can we please also make a note
that top of the frame is where the corrected/high-precision timestamp
should match?
-Daniel

> > + *
> > + * On a lot of display hardware, programming needs to take effect during
> > the
> > + * vertical blanking period so that settings like gamma, what frame being
> 
> s/what frame being/which frame is being/
> 
> > + * scanned out, etc. can be safely changed without showing visual tearing
> > + * on the screen. In some unforgiving hardware, some of this programming
> > has
> > + * to both start and end in the same vblank - vertical blanking period.
> 
> You can just say vblank here, since we already explained what the vertical
> blanking period is up above.
> 
> Alex Deucher pointed out to me that it's probably also worth mentioning that not
> all hardware actually fires off the vblank interrupt at the start of the
> vertical blank, depending on the hardware the interrupt could also happen a few
> scanlines after the start of vblank, a few scanlines before the start, somewhere
> in the middle, at the end of the vblank, etc.
> 
> Other then that, this looks great so far! Feel free to cc me in the respin for
> this patch and I'll be happy to give my r-b.
> 
> > + *
> >   * 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
> -- 
> Cheers,
> 	Lyude Paul (she/her)
> 	Associate Software Engineer at Red Hat
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch