<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<pre>Thanks for the patch.

Tested-by: Yannick Fertré <a class="moz-txt-link-rfc2396E" href="mailto:yannick.fertre@st.com"><yannick.fertre@st.com></a> 

BR
Yannick Fertré
</pre>
<br>
<div class="moz-cite-prefix">On 1/10/20 10:21 AM, Thomas Zimmermann wrote:<br>
</div>
<blockquote type="cite" cite="mid:20200110092127.27847-2-tzimmermann@suse.de">
<pre class="moz-quote-pre" wrap="">The new callback get_scanout_position() reads the current location of
the scanout process. The operation is currentyl located in struct
drm_driver, but really belongs to the CRTC. Drivers will be converted
in separate patches.

Signed-off-by: Thomas Zimmermann <a class="moz-txt-link-rfc2396E" href="mailto:tzimmermann@suse.de"><tzimmermann@suse.de></a>
---
 drivers/gpu/drm/drm_vblank.c             | 24 ++++++++----
 include/drm/drm_drv.h                    |  7 +---
 include/drm/drm_modeset_helper_vtables.h | 47 ++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 1659b13b178c..c12f0b333e14 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -30,6 +30,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_framebuffer.h>
+#include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_print.h>
 #include <drm/drm_vblank.h>
 
@@ -590,7 +591,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
  * Implements calculation of exact vblank timestamps from given drm_display_mode
  * timings and current video scanout position of a CRTC. This can be directly
  * used as the &drm_driver.get_vblank_timestamp implementation of a kms driver
- * if &drm_driver.get_scanout_position is implemented.
+ * if &drm_crtc_helper_funcs.get_scanout_position is implemented.
  *
  * The current implementation only handles standard video modes. For double scan
  * and interlaced modes the driver is supposed to adjust the hardware mode
@@ -632,8 +633,9 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
        }
 
        /* Scanout position query not supported? Should not happen. */
-       if (!dev->driver->get_scanout_position) {
-               DRM_ERROR("Called from driver w/o get_scanout_position()!?\n");
+   if (!dev->driver->get_scanout_position ||
+       !crtc->helper_private->get_scanout_position) {
+           DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n");
                return false;
        }
 
@@ -664,11 +666,17 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
                 * Get vertical and horizontal scanout position vpos, hpos,
                 * and bounding timestamps stime, etime, pre/post query.
                 */
-               vbl_status = dev->driver->get_scanout_position(dev, pipe,
-                                                              in_vblank_irq,
-                                                              &vpos, &hpos,
-                                                              &stime, &etime,
-                                                              mode);
+           if (crtc->helper_private->get_scanout_position) {
+                   vbl_status =
+                           crtc->helper_private->get_scanout_position(
+                                   crtc, in_vblank_irq, &vpos, &hpos,
+                                   &stime, &etime, mode);
+           } else {
+                   vbl_status =
+                           dev->driver->get_scanout_position(
+                                   dev, pipe, in_vblank_irq, &vpos,
+                                   &hpos, &stime, &etime, mode);
+           }
 
                /* Return as no-op if scanout query unsupported or failed. */
                if (!vbl_status) {
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index cf13470810a5..d0049e5786fc 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -362,11 +362,8 @@ struct drm_driver {
         * True on success, false if a reliable scanout position counter could
         * not be read out.
         *
-        * FIXME:
-        *
-        * Since this is a helper to implement @get_vblank_timestamp, we should
-        * move it to &struct drm_crtc_helper_funcs, like all the other
-        * helper-internal hooks.
+    * This is deprecated and should not be used by new drivers.
+    * Use &drm_crtc_helper_funcs.get_scanout_position instead.
         */
        bool (*get_scanout_position) (struct drm_device *dev, unsigned int pipe,
                                      bool in_vblank_irq, int *vpos, int *hpos,
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 5a87f1bd7a3f..e398512bfd5f 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -450,6 +450,53 @@ struct drm_crtc_helper_funcs {
         */
        void (*atomic_disable)(struct drm_crtc *crtc,
                               struct drm_crtc_state *old_crtc_state);
+
+   /**
+    * @get_scanout_position:
+    *
+    * Called by vblank timestamping code.
+    *
+    * Returns the current display scanout position from a CRTC and an
+    * optional accurate ktime_get() timestamp of when the position was
+    * measured. Note that this is a helper callback which is only used
+    * if a driver uses drm_calc_vbltimestamp_from_scanoutpos() for the
+    * @drm_driver.get_vblank_timestamp callback.
+    *
+    * Parameters:
+    *
+    * crtc:
+    *     The CRTC.
+    * in_vblank_irq:
+    *     True when called from drm_crtc_handle_vblank(). Some drivers
+    *     need to apply some workarounds for gpu-specific vblank irq
+    *     quirks if the flag is set.
+    * vpos:
+    *     Target location for current vertical scanout position.
+    * hpos:
+    *     Target location for current horizontal scanout position.
+    * stime:
+    *     Target location for timestamp taken immediately before
+    *     scanout position query. Can be NULL to skip timestamp.
+    * etime:
+    *     Target location for timestamp taken immediately after
+    *     scanout position query. Can be NULL to skip timestamp.
+    * mode:
+    *     Current display timings.
+    *
+    * Returns vpos as a positive number while in active scanout area.
+    * Returns vpos as a negative number inside vblank, counting the number
+    * of scanlines to go until end of vblank, e.g., -1 means "one scanline
+    * until start of active scanout / end of vblank."
+    *
+    * Returns:
+    *
+    * True on success, false if a reliable scanout position counter could
+    * not be read out.
+    */
+   bool (*get_scanout_position)(struct drm_crtc *crtc,
+                                bool in_vblank_irq, int *vpos, int *hpos,
+                                ktime_t *stime, ktime_t *etime,
+                                const struct drm_display_mode *mode);
 };
 
 /**
</pre>
</blockquote>
<br>
</body>
</html>