Lyude - I added a "Co-developed-by: ..." Can I get your s-o-b on the patch, to document that you are OK with this to go in. Needed on top of an r-b.
v2: - committed acked/reviewed patches - significantly updated the vbalnk patch based on a lot of good inputs - updated the writeback patch - wired drm_writeback.h to kernel-doc
Sam
Sam Ravnborg (3): drm/vblank: Add intro to documentation drm: writeback: document callbacks drm/writeback: wire drm_writeback.h to kernel-doc
Documentation/gpu/drm-kms.rst | 3 ++ drivers/gpu/drm/drm_vblank.c | 53 ++++++++++++++++++++++++++++++++ include/drm/drm_modeset_helper_vtables.h | 27 ++++++++++++++++ include/drm/drm_writeback.h | 9 ++++++ 4 files changed, 92 insertions(+)
Lyude Paul wrote a very good intro to vblank here: https://lore.kernel.org/dri-devel/faf63d8a9ed23c16af69762f59d0dca6b2bf085f.c...
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@ravnborg.org Co-developed-by: Lyude Paul lyude@redhat.com Cc: Lyude Paul lyude@redhat.com Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@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 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
On Mon, 2020-04-06 at 21:47 +0200, Sam Ravnborg wrote:
s/have/has/
Other then that:
Reviewed-by: Lyude Paul lyude@redhat.com
Am 06.04.20 um 21:47 schrieb Sam Ravnborg:
Acked-by: Thomas Zimmermann tzimmermann@suse.de
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:
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.
Best regards, Liviu
On Mon, Apr 6, 2020 at 3:48 PM Sam Ravnborg sam@ravnborg.org wrote:
For historical reference, the vertical blanking period was designed to give the electron gun (on CRTs) enough time to move back to the top of the screen to start scanning out the next frame. Similar for horizontal blanking periods. They were designed to give the electron gun enough time to move back to the other side of the screen to start scanning the next scanline. Might be worth adding something like that. Either way: Reviewed-by: Alex Deucher alexander.deucher@amd.com
Hi Alex.
On Tue, Apr 07, 2020 at 10:42:46AM -0400, Alex Deucher wrote:
Thanks. Added this nice historical lesson and committed and pushed out. And to all, thanks for the inputs. We managed to improve this piece of the documetation.
Sam
Document the callbacks: drm_connector_helper_funcs.prepare_writeback_job drm_connector_helper_funcs.cleanup_writeback_job
The documentation was pulled from the changelong introducing the callbacks, originally written by Laurent.
Adding the missing documentation fixes the following warnings: drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs' drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
v2: - Fix formatting (Daniel) - Drop changelog text and add reference (Daniel) - Improve grammar. and use "operation" (Laurent)
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie --- include/drm/drm_modeset_helper_vtables.h | 27 ++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 7c20b1c8b6a7..421a30f08463 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -1075,8 +1075,35 @@ struct drm_connector_helper_funcs { void (*atomic_commit)(struct drm_connector *connector, struct drm_connector_state *state);
+ /** + * @prepare_writeback_job: + * + * As writeback jobs contain a framebuffer, drivers may need to + * prepare and clean them up the same way they can prepare and + * clean up framebuffers for planes. This optional connector operation + * is used to support the preparation of writeback jobs. The job + * prepare operation is called from drm_atomic_helper_prepare_planes() + * for struct &drm_writeback_connector connectors only. + * + * This operation is optional. + * + * This callback is used by the atomic modeset helpers. + */ int (*prepare_writeback_job)(struct drm_writeback_connector *connector, struct drm_writeback_job *job); + /** + * @cleanup_writeback_job: + * + * This optional connector operation is used to support the + * cleanup of writeback jobs. The job cleanup operation is called + * from the existing drm_writeback_cleanup_job() function, invoked + * both when destroying the job as part of an aborted commit, or when + * the job completes. + * + * This operation is optional. + * + * This callback is used by the atomic modeset helpers. + */ void (*cleanup_writeback_job)(struct drm_writeback_connector *connector, struct drm_writeback_job *job); };
On Mon, Apr 06, 2020 at 09:47:45PM +0200, Sam Ravnborg wrote:
Reviewed-by: Liviu Dudau liviu.dudau@arm.com
Thanks! Liviu
drm_writeback.h included a lot of nice kernel-doc comments. Wire it up so the header file is included in the kernel-doc generated documentation.
Added a few simple comments to the two structs so they get picked up by kernel-doc.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Cc: Brian Starkey brian.starkey@arm.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Maxime Ripard mripard@kernel.org Cc: Maxime Ripard mripard@kernel.org --- Documentation/gpu/drm-kms.rst | 3 +++ include/drm/drm_writeback.h | 9 +++++++++ 2 files changed, 12 insertions(+)
diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index e1f685015807..397314d08f77 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -397,6 +397,9 @@ Connector Functions Reference Writeback Connectors --------------------
+.. kernel-doc:: include/drm/drm_writeback.h + :internal: + .. kernel-doc:: drivers/gpu/drm/drm_writeback.c :doc: overview
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 777c14c847f0..9697d2714d2a 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -15,7 +15,13 @@ #include <drm/drm_encoder.h> #include <linux/workqueue.h>
+/** + * struct drm_writeback_connector - DRM writeback connector + */ struct drm_writeback_connector { + /** + * @base: base drm_connector object + */ struct drm_connector base;
/** @@ -78,6 +84,9 @@ struct drm_writeback_connector { char timeline_name[32]; };
+/** + * struct drm_writeback_job - DRM writeback job + */ struct drm_writeback_job { /** * @connector:
On Mon, Apr 06, 2020 at 09:47:46PM +0200, Sam Ravnborg wrote:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter -
Liviu Dudau -
Lyude Paul -
Sam Ravnborg -
Thomas Zimmermann