On Wed, Mar 22, 2017 at 12:33:35PM +0200, Jani Nikula wrote:
> On Wed, 22 Mar 2017, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> > There's really no reason for anything more:
> > - Calling this while the crtc vblank stuff isn't set up is a driver
> > bug. Those places arlready DRM_ERROR.
> > - Calling this when the crtc is off is either a driver bug (callin
> > drm_crtc_handle_vblank at the wrong time) or a core bug (for
> > anything else). Again, we DRM_ERROR.
> > - EINVAL is checked at higher levels already, and if we'd use struct
> > drm_crtc * instead of (dev, pipe) it would be real obvious that
> > those are again core bugs.
> >
> > The only valid failure mode is crap hardware that couldn't sample a
> > useful timestamp, to ask the core to just grab a not-so-accurate
> > timestampe. Bool is perfectly fine for that.
>
> FWIW, I'm not all that thrilled about changing int returns to bools,
> because it changes the check for success from !ret to ret, and the
> compiler won't help you. Sure, there aren't many call sites for the
> function, but, case in point, you forgot to change that lone call
> site. Though it still works by coincidence.
The old call site treated > 0 as success, and 0 or anything negative as
failure. That the lone caller still works is the reason why I've switched
to bool, because besides all the funny values that the caller gets and
ignreos, it already is.
Or did I miss something?
-Daniel
>
> BR,
> Jani.
>
> >
> > Cc: Mario Kleiner <mario.kleiner@xxxxxxxxxxxxxxxx>
> > Cc: Eric Anholt <eric@xxxxxxxxxx>
> > Cc: Rob Clark <robdclark@xxxxxxxxx>
> > Cc: linux-arm-msm@xxxxxxxxxxxxxxx
> > Cc: freedreno@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > Cc: Christian König <christian.koenig@xxxxxxx>
> > Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 8 +++----
> > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 14 ++++++------
> > drivers/gpu/drm/drm_irq.c | 37 +++++++++++--------------------
> > drivers/gpu/drm/i915/i915_irq.c | 8 +++----
> > drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 12 +++++-----
> > drivers/gpu/drm/nouveau/nouveau_display.c | 4 ++--
> > drivers/gpu/drm/nouveau/nouveau_display.h | 4 ++--
> > drivers/gpu/drm/radeon/radeon_drv.c | 8 +++----
> > drivers/gpu/drm/radeon/radeon_kms.c | 14 ++++++------
> > drivers/gpu/drm/vc4/vc4_crtc.c | 2 +-
> > drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
> > include/drm/drmP.h | 1 -
> > include/drm/drm_drv.h | 7 +++---
> > include/drm/drm_irq.h | 10 ++++-----
> > 14 files changed, 59 insertions(+), 72 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index c8860f05bfb9..acd8631d8024 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1768,10 +1768,10 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon);
> > u32 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
> > int amdgpu_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> > void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> > -int amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> > - int *max_error,
> > - struct timeval *vblank_time,
> > - unsigned flags);
> > +bool amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> > + int *max_error,
> > + struct timeval *vblank_time,
> > + unsigned flags);
> > long amdgpu_kms_compat_ioctl(struct file *filp, unsigned int cmd,
> > unsigned long arg);
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > index 55f951bcc91f..ac42f707c046 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > @@ -838,19 +838,19 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
> > *
> > * Gets the timestamp on the requested crtc based on the
> > * scanout position. (all asics).
> > - * Returns postive status flags on success, negative error on failure.
> > + * Returns true on success, false on failure.
> > */
> > -int amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> > - int *max_error,
> > - struct timeval *vblank_time,
> > - unsigned flags)
> > +bool amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> > + int *max_error,
> > + struct timeval *vblank_time,
> > + unsigned flags)
> > {
> > struct drm_crtc *crtc;
> > struct amdgpu_device *adev = dev->dev_private;
> >
> > if (pipe >= dev->num_crtcs) {
> > DRM_ERROR("Invalid crtc %u\n", pipe);
> > - return -EINVAL;
> > + return false;
> > }
> >
> > /* Get associated drm_crtc: */
> > @@ -859,7 +859,7 @@ int amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> > /* This can occur on driver load if some component fails to
> > * initialize completely and driver is unloaded */
> > DRM_ERROR("Uninitialized crtc %d\n", pipe);
> > - return -EINVAL;
> > + return false;
> > }
> >
> > /* Helper routine in DRM core does all the work: */
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 4b0c7475ed13..2121ea29e1b2 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -722,43 +722,32 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
> > * active. Higher level code is expected to handle this.
> > *
> > * Returns:
> > - * Negative value on error, failure or if not supported in current
> > - * video mode:
> > - *
> > - * -EINVAL Invalid CRTC.
> > - * -EAGAIN Temporary unavailable, e.g., called before initial modeset.
> > - * -ENOTSUPP Function not supported in current display mode.
> > - * -EIO Failed, e.g., due to failed scanout position query.
> > - *
> > - * Returns or'ed positive status flags on success:
> > - *
> > - * DRM_VBLANKTIME_SCANOUTPOS_METHOD - Signal this method used for timestamping.
> > - * DRM_VBLANKTIME_INVBL - Timestamp taken while scanout was in vblank interval.
> > *
> > + * Returns true on success, and false on failure, i.e. when no accurate
> > + * timestamp could be acquired.
> > */
> > -int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> > - unsigned int pipe,
> > - int *max_error,
> > - struct timeval *vblank_time,
> > - unsigned flags,
> > - const struct drm_display_mode *mode)
> > +bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> > + unsigned int pipe,
> > + int *max_error,
> > + struct timeval *vblank_time,
> > + unsigned flags,
> > + const struct drm_display_mode *mode)
> > {
> > struct timeval tv_etime;
> > ktime_t stime, etime;
> > unsigned int vbl_status;
> > - int ret = DRM_VBLANKTIME_SCANOUTPOS_METHOD;
> > int vpos, hpos, i;
> > int delta_ns, duration_ns;
> >
> > if (pipe >= dev->num_crtcs) {
> > DRM_ERROR("Invalid crtc %u\n", pipe);
> > - return -EINVAL;
> > + return false;
> > }
> >
> > /* 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");
> > - return -EIO;
> > + return false;
> > }
> >
> > /* If mode timing undefined, just return as no-op:
> > @@ -766,7 +755,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> > */
> > if (mode->crtc_clock == 0) {
> > DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe);
> > - return -EAGAIN;
> > + return false;
> > }
> >
> > /* Get current scanout position with system timestamp.
> > @@ -790,7 +779,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> > if (!(vbl_status & DRM_SCANOUTPOS_VALID)) {
> > DRM_DEBUG("crtc %u : scanoutpos query failed [0x%x].\n",
> > pipe, vbl_status);
> > - return -EIO;
> > + return false;
> > }
> >
> > /* Compute uncertainty in timestamp of scanout position query. */
> > @@ -834,7 +823,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> > (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
> > duration_ns/1000, i);
> >
> > - return ret;
> > + return true;
> > }
> > EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 26aa1c170b43..df149d159ce7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -964,7 +964,7 @@ int intel_get_crtc_scanline(struct intel_crtc *crtc)
> > return position;
> > }
> >
> > -static int i915_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> > +static bool i915_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> > int *max_error,
> > struct timeval *vblank_time,
> > unsigned flags)
> > @@ -974,19 +974,19 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> >
> > if (pipe >= INTEL_INFO(dev_priv)->num_pipes) {
> > DRM_ERROR("Invalid crtc %u\n", pipe);
> > - return -EINVAL;
> > + return false;
> > }
> >
> > /* Get drm_crtc to timestamp: */
> > crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > if (crtc == NULL) {
> > DRM_ERROR("Invalid crtc %u\n", pipe);
> > - return -EINVAL;
> > + return false;
> > }
> >
> > if (!crtc->base.hwmode.crtc_clock) {
> > DRM_DEBUG_KMS("crtc %u is disabled\n", pipe);
> > - return -EBUSY;
> > + return false;
> > }
> >
> > /* Helper routine in DRM core does all the work: */
> > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> > index 41ccd2a15d3c..655700eb42ba 100644
> > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> > @@ -595,23 +595,23 @@ static int mdp5_get_scanoutpos(struct drm_device *dev, unsigned int pipe,
> > return ret;
> > }
> >
> > -static int mdp5_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> > - int *max_error,
> > - struct timeval *vblank_time,
> > - unsigned flags)
> > +static bool mdp5_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> > + int *max_error,
> > + struct timeval *vblank_time,
> > + unsigned flags)
> > {
> > struct msm_drm_private *priv = dev->dev_private;
> > struct drm_crtc *crtc;
> >
> > if (pipe < 0 || pipe >= priv->num_crtcs) {
> > DRM_ERROR("Invalid crtc %d\n", pipe);
> > - return -EINVAL;
> > + return false;
> > }
> >
> > crtc = priv->crtcs[pipe];
> > if (!crtc) {
> > DRM_ERROR("Invalid crtc %d\n", pipe);
> > - return -EINVAL;
> > + return false;
> > }
> >
> > return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> > index 33269c7df30f..42f18b0b9c43 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> > @@ -156,7 +156,7 @@ nouveau_display_scanoutpos(struct drm_device *dev, unsigned int pipe,
> > return 0;
> > }
> >
> > -int
> > +bool
> > nouveau_display_vblstamp(struct drm_device *dev, unsigned int pipe,
> > int *max_error, struct timeval *time, unsigned flags)
> > {
> > @@ -174,7 +174,7 @@ nouveau_display_vblstamp(struct drm_device *dev, unsigned int pipe,
> > }
> > }
> >
> > - return -EINVAL;
> > + return false;
> > }
> >
> > static void
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
> > index 4a75df06c139..bc9d1e7b0117 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_display.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_display.h
> > @@ -71,8 +71,8 @@ void nouveau_display_vblank_disable(struct drm_device *, unsigned int);
> > int nouveau_display_scanoutpos(struct drm_device *, unsigned int,
> > unsigned int, int *, int *, ktime_t *,
> > ktime_t *, const struct drm_display_mode *);
> > -int nouveau_display_vblstamp(struct drm_device *, unsigned int, int *,
> > - struct timeval *, unsigned);
> > +bool nouveau_display_vblstamp(struct drm_device *, unsigned int, int *,
> > + struct timeval *, unsigned);
> >
> > int nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > struct drm_pending_vblank_event *event,
> > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> > index 312436a8d9e5..6d79b5c2805b 100644
> > --- a/drivers/gpu/drm/radeon/radeon_drv.c
> > +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> > @@ -114,10 +114,10 @@ int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon);
> > u32 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
> > int radeon_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> > void radeon_disable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> > -int radeon_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> > - int *max_error,
> > - struct timeval *vblank_time,
> > - unsigned flags);
> > +bool radeon_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> > + int *max_error,
> > + struct timeval *vblank_time,
> > + unsigned flags);
> > void radeon_driver_irq_preinstall_kms(struct drm_device *dev);
> > int radeon_driver_irq_postinstall_kms(struct drm_device *dev);
> > void radeon_driver_irq_uninstall_kms(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> > index a32a62e03a44..9afe72010c64 100644
> > --- a/drivers/gpu/drm/radeon/radeon_kms.c
> > +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> > @@ -868,25 +868,25 @@ void radeon_disable_vblank_kms(struct drm_device *dev, int crtc)
> > *
> > * Gets the timestamp on the requested crtc based on the
> > * scanout position. (all asics).
> > - * Returns postive status flags on success, negative error on failure.
> > + * Returns true on success, false on failure.
> > */
> > -int radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
> > - int *max_error,
> > - struct timeval *vblank_time,
> > - unsigned flags)
> > +bool radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
> > + int *max_error,
> > + struct timeval *vblank_time,
> > + unsigned flags)
> > {
> > struct drm_crtc *drmcrtc;
> > struct radeon_device *rdev = dev->dev_private;
> >
> > if (crtc < 0 || crtc >= dev->num_crtcs) {
> > DRM_ERROR("Invalid crtc %d\n", crtc);
> > - return -EINVAL;
> > + return false;
> > }
> >
> > /* Get associated drm_crtc: */
> > drmcrtc = &rdev->mode_info.crtcs[crtc]->base;
> > if (!drmcrtc)
> > - return -EINVAL;
> > + return false;
> >
> > /* Helper routine in DRM core does all the work: */
> > return drm_calc_vbltimestamp_from_scanoutpos(dev, crtc, max_error,
> > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> > index 24edd0c22cc9..74260990b6bf 100644
> > --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> > @@ -270,7 +270,7 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
> > return ret;
> > }
> >
> > -int vc4_crtc_get_vblank_timestamp(struct drm_device *dev, unsigned int crtc_id,
> > +bool vc4_crtc_get_vblank_timestamp(struct drm_device *dev, unsigned int crtc_id,
> > int *max_error, struct timeval *vblank_time,
> > unsigned flags)
> > {
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index dffce6293d87..8a5d0e12ee02 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -450,7 +450,7 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
> > unsigned int flags, int *vpos, int *hpos,
> > ktime_t *stime, ktime_t *etime,
> > const struct drm_display_mode *mode);
> > -int vc4_crtc_get_vblank_timestamp(struct drm_device *dev, unsigned int crtc_id,
> > +bool vc4_crtc_get_vblank_timestamp(struct drm_device *dev, unsigned int crtc_id,
> > int *max_error, struct timeval *vblank_time,
> > unsigned flags);
> >
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 27ecfcdbf752..6304de7e041e 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -322,7 +322,6 @@ struct pci_controller;
> >
> > /* Flags and return codes for get_vblank_timestamp() driver function. */
> > #define DRM_CALLED_FROM_VBLIRQ 1
> > -#define DRM_VBLANKTIME_SCANOUTPOS_METHOD (1 << 0)
> >
> > /* get_scanout_position() return flags */
> > #define DRM_SCANOUTPOS_VALID (1 << 0)
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index d0b5f363bfa1..da78e248d9d8 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -316,11 +316,10 @@ struct drm_driver {
> > *
> > * Returns:
> > *
> > - * Zero if timestamping isn't supported in current display mode or a
> > - * negative number on failure. A positive status code on success,
> > - * which describes how the vblank_time timestamp was computed.
> > + * True on success, false on failure, which means the core should
> > + * fallback to a simple timestamp taken in drm_crtc_handle_vblank().
> > */
> > - int (*get_vblank_timestamp) (struct drm_device *dev, unsigned int pipe,
> > + bool (*get_vblank_timestamp) (struct drm_device *dev, unsigned int pipe,
> > int *max_error,
> > struct timeval *vblank_time,
> > unsigned flags);
> > diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
> > index cf0be6594c8c..f0d5ccf9b282 100644
> > --- a/include/drm/drm_irq.h
> > +++ b/include/drm/drm_irq.h
> > @@ -153,11 +153,11 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc);
> > void drm_vblank_cleanup(struct drm_device *dev);
> > u32 drm_accurate_vblank_count(struct drm_crtc *crtc);
> >
> > -int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> > - unsigned int pipe, int *max_error,
> > - struct timeval *vblank_time,
> > - unsigned flags,
> > - const struct drm_display_mode *mode);
> > +bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> > + unsigned int pipe, int *max_error,
> > + struct timeval *vblank_time,
> > + unsigned flags,
> > + const struct drm_display_mode *mode);
> > void drm_calc_timestamping_constants(struct drm_crtc *crtc,
> > const struct drm_display_mode *mode);
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
