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@xxxxxxxxxx/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@xxxxxxxxxxxx> >> Co-developed-by: Lyude Paul <lyude@xxxxxxxxxx> >> Cc: Lyude Paul <lyude@xxxxxxxxxx> >> Acked-by: Thomas Zimmermann <tzimmermann@xxxxxxx> >> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> >> Cc: Daniel Vetter <daniel@xxxxxxxx> >> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >> Cc: Maxime Ripard <mripard@xxxxxxxxxx> >> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> >> Cc: David Airlie <airlied@xxxxxxxx> >> --- >> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
