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

[Thomas Zimmermann]

Hi,

just a few more nits below.

Am 30.03.20 um 23:51 schrieb Lyude Paul:
> 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
>      ------------------------------------------------
>> + *
>> + * 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/

I still had read it twice in either variant. Maybe:

s/what frame being scanned out/the displayed image buffer

> 
>> + * scanned out, etc. can be safely changed without showing visual tearing

I think tearing refers specifically to mid-frame buffer flips. Maybe

s/tearing/artifacts

or

s/tearing/distortion

Best regards
Thomas

>> + * 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

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200331/f717da67/attachment-0001.sig>