Re: [Freedreno] [PATCH 01/23] drm: Add get_scanout_position() to struct drm_crtc_helper_funcs

[Yannick FERTRE]

Thanks for the patch.

Tested-by: Yannick Fertré 

BR
Yannick Fertré


On 1/10/20 10:21 AM, Thomas Zimmermann wrote:

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 

---
 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 
 #include 
 #include 
+#include 
 #include 
 #include 

@@ -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 _driver.get_vblank_timestamp implementation of a kms driver
- * if _driver.get_scanout_position is implemented.
+ * if _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,
-  , ,
-  , ,
-  mode);
+   if (crtc->helper_private->get_scanout_position) {
+   vbl_status =
+   crtc->helper_private->get_scanout_position(
+   crtc, in_vblank_irq, , ,
+   , , mode);
+   } else {
+   vbl_status =
+   dev->driver->get_scanout_position(
+   dev, pipe, in_vblank_irq, ,
+   , , , 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  drm_crtc_helper_funcs, like all the other
-* helper-internal hooks.
+* This is deprecated and should not be used by new drivers.
+* Use _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 

Re: [Freedreno] [PATCH 08/23] drm/stm: Convert to struct drm_crtc_helper_funcs.get_scanout_position()

[Yannick FERTRE]

Thanks for the patch.

Tested-by: Yannick Fertré 

BR
Yannick Fertré


On 1/10/20 10:21 AM, Thomas Zimmermann wrote:

The callback struct drm_driver.get_scanout_position() is deprecated in
favor of struct drm_crtc_helper_funcs.get_scanout_position(). Convert stm
over.

Signed-off-by: Thomas Zimmermann 

---
 drivers/gpu/drm/stm/drv.c  |  1 -
 drivers/gpu/drm/stm/ltdc.c | 65 --
 drivers/gpu/drm/stm/ltdc.h |  5 ---
 3 files changed, 34 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 5a9f9aca8bc2..486985604109 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -72,7 +72,6 @@ static struct drm_driver drv_driver = {
.gem_prime_vmap = drm_gem_cma_prime_vmap,
.gem_prime_vunmap = drm_gem_cma_prime_vunmap,
.gem_prime_mmap = drm_gem_cma_prime_mmap,
-   .get_scanout_position = ltdc_crtc_scanoutpos,
.get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
 };

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index c2815e8ae1da..8b6d1a2252e3 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -636,38 +636,13 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc *crtc,
}
 }

-static const struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
-   .mode_valid = ltdc_crtc_mode_valid,
-   .mode_fixup = ltdc_crtc_mode_fixup,
-   .mode_set_nofb = ltdc_crtc_mode_set_nofb,
-   .atomic_flush = ltdc_crtc_atomic_flush,
-   .atomic_enable = ltdc_crtc_atomic_enable,
-   .atomic_disable = ltdc_crtc_atomic_disable,
-};
-
-static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
-{
-   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
-
-   DRM_DEBUG_DRIVER("\n");
-   reg_set(ldev->regs, LTDC_IER, IER_LIE);
-
-   return 0;
-}
-
-static void ltdc_crtc_disable_vblank(struct drm_crtc *crtc)
-{
-   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
-
-   DRM_DEBUG_DRIVER("\n");
-   reg_clear(ldev->regs, LTDC_IER, IER_LIE);
-}
-
-bool ltdc_crtc_scanoutpos(struct drm_device *ddev, unsigned int pipe,
- bool in_vblank_irq, int *vpos, int *hpos,
- ktime_t *stime, ktime_t *etime,
- const struct drm_display_mode *mode)
+static bool ltdc_crtc_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)
 {
+   struct drm_device *ddev = crtc->dev;
struct ltdc_device *ldev = ddev->dev_private;
int line, vactive_start, vactive_end, vtotal;

@@ -710,6 +685,34 @@ bool ltdc_crtc_scanoutpos(struct drm_device *ddev, 
unsigned int pipe,
return true;
 }

+static const struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
+   .mode_valid = ltdc_crtc_mode_valid,
+   .mode_fixup = ltdc_crtc_mode_fixup,
+   .mode_set_nofb = ltdc_crtc_mode_set_nofb,
+   .atomic_flush = ltdc_crtc_atomic_flush,
+   .atomic_enable = ltdc_crtc_atomic_enable,
+   .atomic_disable = ltdc_crtc_atomic_disable,
+   .get_scanout_position = ltdc_crtc_get_scanout_position,
+};
+
+static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
+{
+   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
+
+   DRM_DEBUG_DRIVER("\n");
+   reg_set(ldev->regs, LTDC_IER, IER_LIE);
+
+   return 0;
+}
+
+static void ltdc_crtc_disable_vblank(struct drm_crtc *crtc)
+{
+   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
+
+   DRM_DEBUG_DRIVER("\n");
+   reg_clear(ldev->regs, LTDC_IER, IER_LIE);
+}
+
 static const struct drm_crtc_funcs ltdc_crtc_funcs = {
.destroy = drm_crtc_cleanup,
.set_config = drm_atomic_helper_set_config,
diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
index a1ad0ae3b006..c5467d74e707 100644
--- a/drivers/gpu/drm/stm/ltdc.h
+++ b/drivers/gpu/drm/stm/ltdc.h
@@ -39,11 +39,6 @@ struct ltdc_device {
struct drm_atomic_state *suspend_state;
 };

-bool ltdc_crtc_scanoutpos(struct drm_device *dev, unsigned int pipe,
- bool in_vblank_irq, int *vpos, int *hpos,
- ktime_t *stime, ktime_t *etime,
- const struct drm_display_mode *mode);
-
 int ltdc_load(struct drm_device *ddev);
 void ltdc_unload(struct drm_device *ddev);
 void ltdc_suspend(struct drm_device *ddev);


___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 19/23] drm/stm: Convert to CRTC VBLANK callbacks

[Yannick FERTRE]

Thanks for the patch.

Tested-by: Yannick Fertré 

BR
Yannick Fertré


On 1/10/20 10:21 AM, Thomas Zimmermann wrote:

VBLANK callbacks in struct drm_driver are deprecated in favor of
their equivalents in struct drm_crtc_funcs. Convert stm over.

Signed-off-by: Thomas Zimmermann 

---
 drivers/gpu/drm/stm/drv.c  | 1 -
 drivers/gpu/drm/stm/ltdc.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 486985604109..ea9fcbdc68b3 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -72,7 +72,6 @@ static struct drm_driver drv_driver = {
.gem_prime_vmap = drm_gem_cma_prime_vmap,
.gem_prime_vunmap = drm_gem_cma_prime_vunmap,
.gem_prime_mmap = drm_gem_cma_prime_mmap,
-   .get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
 };

 static int drv_load(struct drm_device *ddev)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 8b6d1a2252e3..4fe9b033de1b 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -722,6 +722,7 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
.enable_vblank = ltdc_crtc_enable_vblank,
.disable_vblank = ltdc_crtc_disable_vblank,
+   .get_vblank_timestamp = drm_crtc_calc_vbltimestamp_from_scanoutpos,
.gamma_set = drm_atomic_helper_legacy_gamma_set,
 };



___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 23/23] drm: Cleanup VBLANK callbacks in struct drm_driver

[Yannick FERTRE]

Thanks for the patch.

Tested-by: Yannick Fertré 

BR
Yannick Fertré


On 1/10/20 10:21 AM, Thomas Zimmermann wrote:

All non-legacy users of VBLANK functions in struct drm_driver have been
converted to use the respective interfaces in struct drm_crtc_funcs. The
remaining users of VBLANK callbacks in struct drm_driver are legacy drivers
with userspace modesetting.

There are no users left of get_vblank_timestamp(), so the callback is
being removed. The other VBLANK callbacks are being moved to the legacy
section at the end of struct drm_driver.

Signed-off-by: Thomas Zimmermann 

---
 drivers/gpu/drm/drm_vblank.c |  39 +-
 include/drm/drm_drv.h| 101 ++-
 2 files changed, 17 insertions(+), 123 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 7cf436a4b908..ceff68474d4d 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -138,10 +138,9 @@ static u32 __get_vblank_counter(struct drm_device *dev, 
unsigned int pipe)

if (crtc->funcs->get_vblank_counter)
return crtc->funcs->get_vblank_counter(crtc);
-   }
-
-   if (dev->driver->get_vblank_counter)
+   } else if (dev->driver->get_vblank_counter) {
return dev->driver->get_vblank_counter(dev, pipe);
+   }

return drm_vblank_no_hw_counter(dev, pipe);
 }
@@ -334,8 +333,7 @@ u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
unsigned long flags;

WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) &&
- !crtc->funcs->get_vblank_timestamp &&
- !dev->driver->get_vblank_timestamp,
+ !crtc->funcs->get_vblank_timestamp,
  "This function requires support for accurate vblank 
timestamps.");

spin_lock_irqsave(>vblank_time_lock, flags);
@@ -357,13 +355,11 @@ static void __disable_vblank(struct drm_device *dev, 
unsigned int pipe)
if (WARN_ON(!crtc))
return;

-   if (crtc->funcs->disable_vblank) {
+   if (crtc->funcs->disable_vblank)
crtc->funcs->disable_vblank(crtc);
-   return;
-   }
+   } else {
+   dev->driver->disable_vblank(dev, pipe);
}
-
-   dev->driver->disable_vblank(dev, pipe);
 }

 /*
@@ -791,9 +787,6 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned 
int pipe,

ret = crtc->funcs->get_vblank_timestamp(crtc, _error,
tvblank, in_vblank_irq);
-   } else if (dev->driver->get_vblank_timestamp && (max_error > 0)) {
-   ret = dev->driver->get_vblank_timestamp(dev, pipe, _error,
-   tvblank, in_vblank_irq);
}

/* GPU high precision timestamp query unsupported or failed.
@@ -1016,9 +1009,11 @@ static int __enable_vblank(struct drm_device *dev, 
unsigned int pipe)

if (crtc->funcs->enable_vblank)
return crtc->funcs->enable_vblank(crtc);
+   } else if (dev->driver->enable_vblank) {
+   return dev->driver->enable_vblank(dev, pipe);
}

-   return dev->driver->enable_vblank(dev, pipe);
+   return -EINVAL;
 }

 static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
@@ -1109,13 +1104,10 @@ static bool __vblank_disable_immediate(struct 
drm_device *dev, unsigned int pipe
return false;

crtc = drm_crtc_from_index(dev, pipe);
-   if (crtc && crtc->funcs->get_vblank_timestamp)
-   return true;
-
-   if (dev->driver->get_vblank_timestamp)
-   return true;
+   if (!crtc || !crtc->funcs->get_vblank_timestamp)
+   return false;

-   return false;
+   return true;
 }

 static void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
@@ -1798,7 +1790,6 @@ static void drm_handle_vblank_events(struct drm_device 
*dev, unsigned int pipe)
struct drm_pending_vblank_event *e, *t;
ktime_t now;
u64 seq;
-   bool high_prec;

assert_spin_locked(>event_lock);

@@ -1818,10 +1809,8 @@ static void drm_handle_vblank_events(struct drm_device 
*dev, unsigned int pipe)
send_vblank_event(dev, e, seq, now);
}

-   high_prec = crtc->funcs->get_vblank_timestamp ||
-   dev->driver->get_vblank_timestamp;
-
-   trace_drm_vblank_event(pipe, seq, now, high_prec);
+   trace_drm_vblank_event(pipe, seq, now,
+  crtc->funcs->get_vblank_timestamp != NULL);
 }

 /**
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index b704e252f3b2..e290b3aca6eb 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -268,104 +268,6 @@ struct drm_driver {
 */
void 

Re: [Freedreno] [PATCH 09/23] drm: Remove struct drm_driver.get_scanout_position()

[Yannick FERTRE]

Thanks for the patch.

Tested-by: Yannick Fertré 

BR
Yannick Fertré


On 1/10/20 10:21 AM, Thomas Zimmermann wrote:

All users of struct drm_driver.get_scanout_position() have been
covnerted to the respective CRTC helper function. Remove the callback
from struct drm_driver.

Signed-off-by: Thomas Zimmermann 

---
 drivers/gpu/drm/drm_vblank.c | 13 ++---
 include/drm/drm_drv.h| 52 
 2 files changed, 2 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index c12f0b333e14..b84065911d69 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -633,8 +633,7 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct 
drm_device *dev,
}

/* Scanout position query not supported? Should not happen. */
-   if (!dev->driver->get_scanout_position ||
-   !crtc->helper_private->get_scanout_position) {
+   if (!crtc->helper_private->get_scanout_position) {
DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n");
return false;
}
@@ -666,17 +665,9 @@ 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.
 */
-   if (crtc->helper_private->get_scanout_position) {
-   vbl_status =
-   crtc->helper_private->get_scanout_position(
+   vbl_status = crtc->helper_private->get_scanout_position(
crtc, in_vblank_irq, , ,
, , mode);
-   } else {
-   vbl_status =
-   dev->driver->get_scanout_position(
-   dev, pipe, in_vblank_irq, ,
-   , , , 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 d0049e5786fc..b704e252f3b2 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -318,58 +318,6 @@ struct drm_driver {
 */
void (*disable_vblank) (struct drm_device *dev, unsigned int pipe);

-   /**
-* @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 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
-* @get_vblank_timestamp callback.
-*
-* Parameters:
-*
-* dev:
-* DRM device.
-* pipe:
-* Id of the crtc to query.
-* 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 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.
-*
-* This is deprecated and should not be used by new drivers.
-* Use _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,
- ktime_t *stime, ktime_t *etime,
- const struct drm_display_mode *mode);
-
/**
 * @get_vblank_timestamp:
 *


___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno