Re: [Freedreno] [PATCH 03/23] drm/i915: Don't use struct drm_driver.get_scanout_position()
On Fri, Jan 10, 2020 at 03:56:06PM +0200, Jani Nikula wrote: > On Fri, 10 Jan 2020, Thomas Zimmermann wrote: > > Hi > > > > Am 10.01.20 um 12:59 schrieb Jani Nikula: > >> On Fri, 10 Jan 2020, Thomas Zimmermann wrote: > >>> The callback struct drm_driver.get_scanout_position() is deprecated in > >>> favor of struct drm_crtc_helper_funcs.get_scanout_position(). > >>> > >>> i915 doesn't use CRTC helpers. The patch duplicates the caller > >>> drm_calc_vbltimestamp_from_scanoutpos() for i915, such that the callback > >>> function is not needed. > >>> > >>> Signed-off-by: Thomas Zimmermann > >>> --- > >>> drivers/gpu/drm/i915/i915_drv.c | 3 +- > >>> drivers/gpu/drm/i915/i915_irq.c | 117 ++-- > >>> drivers/gpu/drm/i915/i915_irq.h | 9 +-- > >>> 3 files changed, 119 insertions(+), 10 deletions(-) > >> > >> Not really enthusiastic about the diffstat in a "cleanup" series. > > > > Well, the cleanup is about the content of drm_driver :) > > > >> > >> I wonder if you could add a generic helper version of > >> drm_calc_vbltimestamp_from_scanoutpos where you pass the > >> get_scanout_position function as a parameter. Both > >> drm_calc_vbltimestamp_from_scanoutpos and the new > >> i915_calc_vbltimestamp_from_scanoutpos would then be fairly thin > >> wrappers passing in the relevant get_scanout_position function. > > > > Of course. Will be in v2 of the series. > > Please give Ville (Cc'd) a moment before sending v2 in case he wants to > chime in on this. Passing the function pointer was one option I considered for this a while back. Can't remeber what other solutions I condsidered. But I guess I didn't like any of them enough to make an actual patch. -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 03/23] drm/i915: Don't use struct drm_driver.get_scanout_position()
On Fri, 10 Jan 2020, Thomas Zimmermann wrote: > Hi > > Am 10.01.20 um 12:59 schrieb Jani Nikula: >> On Fri, 10 Jan 2020, Thomas Zimmermann wrote: >>> The callback struct drm_driver.get_scanout_position() is deprecated in >>> favor of struct drm_crtc_helper_funcs.get_scanout_position(). >>> >>> i915 doesn't use CRTC helpers. The patch duplicates the caller >>> drm_calc_vbltimestamp_from_scanoutpos() for i915, such that the callback >>> function is not needed. >>> >>> Signed-off-by: Thomas Zimmermann >>> --- >>> drivers/gpu/drm/i915/i915_drv.c | 3 +- >>> drivers/gpu/drm/i915/i915_irq.c | 117 ++-- >>> drivers/gpu/drm/i915/i915_irq.h | 9 +-- >>> 3 files changed, 119 insertions(+), 10 deletions(-) >> >> Not really enthusiastic about the diffstat in a "cleanup" series. > > Well, the cleanup is about the content of drm_driver :) > >> >> I wonder if you could add a generic helper version of >> drm_calc_vbltimestamp_from_scanoutpos where you pass the >> get_scanout_position function as a parameter. Both >> drm_calc_vbltimestamp_from_scanoutpos and the new >> i915_calc_vbltimestamp_from_scanoutpos would then be fairly thin >> wrappers passing in the relevant get_scanout_position function. > > Of course. Will be in v2 of the series. Please give Ville (Cc'd) a moment before sending v2 in case he wants to chime in on this. Thanks, Jani. > > Best regards > Thomas > >> >> This would reduce the almost identical duplication of the function in >> i915. >> >> BR, >> Jani. >> >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.c >>> b/drivers/gpu/drm/i915/i915_drv.c >>> index f7385abdd74b..4a0a7fb85c53 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.c >>> +++ b/drivers/gpu/drm/i915/i915_drv.c >>> @@ -2769,8 +2769,7 @@ static struct drm_driver driver = { >>> .gem_prime_export = i915_gem_prime_export, >>> .gem_prime_import = i915_gem_prime_import, >>> >>> - .get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos, >>> - .get_scanout_position = i915_get_crtc_scanoutpos, >>> + .get_vblank_timestamp = i915_calc_vbltimestamp_from_scanoutpos, >>> >>> .dumb_create = i915_gem_dumb_create, >>> .dumb_map_offset = i915_gem_dumb_mmap_offset, >>> diff --git a/drivers/gpu/drm/i915/i915_irq.c >>> b/drivers/gpu/drm/i915/i915_irq.c >>> index afc6aad9bf8c..99d0c3b0feae 100644 >>> --- a/drivers/gpu/drm/i915/i915_irq.c >>> +++ b/drivers/gpu/drm/i915/i915_irq.c >>> @@ -52,6 +52,11 @@ >>> #include "i915_trace.h" >>> #include "intel_pm.h" >>> >>> +/* Retry timestamp calculation up to 3 times to satisfy >>> + * drm_timestamp_precision before giving up. >>> + */ >>> +#define I915_TIMESTAMP_MAXRETRIES 3 >>> + >>> /** >>> * DOC: interrupt handling >>> * >>> @@ -762,10 +767,11 @@ static int __intel_get_crtc_scanline(struct >>> intel_crtc *crtc) >>> return (position + crtc->scanline_offset) % vtotal; >>> } >>> >>> -bool i915_get_crtc_scanoutpos(struct drm_device *dev, unsigned int index, >>> - bool in_vblank_irq, int *vpos, int *hpos, >>> - ktime_t *stime, ktime_t *etime, >>> - const struct drm_display_mode *mode) >>> +static bool i915_get_crtc_scanoutpos(struct drm_device *dev, >>> +unsigned int index, bool in_vblank_irq, >>> +int *vpos, int *hpos, >>> +ktime_t *stime, ktime_t *etime, >>> +const struct drm_display_mode *mode) >>> { >>> struct drm_i915_private *dev_priv = to_i915(dev); >>> struct intel_crtc *crtc = to_intel_crtc(drm_crtc_from_index(dev, >>> index)); >>> @@ -879,6 +885,109 @@ bool i915_get_crtc_scanoutpos(struct drm_device *dev, >>> unsigned int index, >>> return true; >>> } >>> >>> +bool i915_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, >>> + unsigned int pipe, >>> + int *max_error, >>> + ktime_t *vblank_time, >>> + bool in_vblank_irq) >>> +{ >>> + struct timespec64 ts_etime, ts_vblank_time; >>> + ktime_t stime, etime; >>> + bool vbl_status; >>> + struct drm_crtc *crtc; >>> + const struct drm_display_mode *mode; >>> + struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; >>> + int vpos, hpos, i; >>> + int delta_ns, duration_ns; >>> + >>> + crtc = drm_crtc_from_index(dev, pipe); >>> + >>> + if (pipe >= dev->num_crtcs || !crtc) { >>> + DRM_ERROR("Invalid crtc %u\n", pipe); >>> + return false; >>> + } >>> + >>> + if (drm_drv_uses_atomic_modeset(dev)) >>> + mode = &vblank->hwmode; >>> + else >>> + mode = &crtc->hwmode; >>> + >>> + /* If mode timing undefined, just return as no-op: >>> +* Happens during initial modesetting of a crtc. >>> +*/ >>> + if (mode->crtc_clock == 0) { >>> + DRM_DE
Re: [Freedreno] [PATCH 03/23] drm/i915: Don't use struct drm_driver.get_scanout_position()
Hi Am 10.01.20 um 12:59 schrieb Jani Nikula: > On Fri, 10 Jan 2020, Thomas Zimmermann wrote: >> The callback struct drm_driver.get_scanout_position() is deprecated in >> favor of struct drm_crtc_helper_funcs.get_scanout_position(). >> >> i915 doesn't use CRTC helpers. The patch duplicates the caller >> drm_calc_vbltimestamp_from_scanoutpos() for i915, such that the callback >> function is not needed. >> >> Signed-off-by: Thomas Zimmermann >> --- >> drivers/gpu/drm/i915/i915_drv.c | 3 +- >> drivers/gpu/drm/i915/i915_irq.c | 117 ++-- >> drivers/gpu/drm/i915/i915_irq.h | 9 +-- >> 3 files changed, 119 insertions(+), 10 deletions(-) > > Not really enthusiastic about the diffstat in a "cleanup" series. Well, the cleanup is about the content of drm_driver :) > > I wonder if you could add a generic helper version of > drm_calc_vbltimestamp_from_scanoutpos where you pass the > get_scanout_position function as a parameter. Both > drm_calc_vbltimestamp_from_scanoutpos and the new > i915_calc_vbltimestamp_from_scanoutpos would then be fairly thin > wrappers passing in the relevant get_scanout_position function. Of course. Will be in v2 of the series. Best regards Thomas > > This would reduce the almost identical duplication of the function in > i915. > > BR, > Jani. > >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c >> b/drivers/gpu/drm/i915/i915_drv.c >> index f7385abdd74b..4a0a7fb85c53 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -2769,8 +2769,7 @@ static struct drm_driver driver = { >> .gem_prime_export = i915_gem_prime_export, >> .gem_prime_import = i915_gem_prime_import, >> >> -.get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos, >> -.get_scanout_position = i915_get_crtc_scanoutpos, >> +.get_vblank_timestamp = i915_calc_vbltimestamp_from_scanoutpos, >> >> .dumb_create = i915_gem_dumb_create, >> .dumb_map_offset = i915_gem_dumb_mmap_offset, >> diff --git a/drivers/gpu/drm/i915/i915_irq.c >> b/drivers/gpu/drm/i915/i915_irq.c >> index afc6aad9bf8c..99d0c3b0feae 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -52,6 +52,11 @@ >> #include "i915_trace.h" >> #include "intel_pm.h" >> >> +/* Retry timestamp calculation up to 3 times to satisfy >> + * drm_timestamp_precision before giving up. >> + */ >> +#define I915_TIMESTAMP_MAXRETRIES 3 >> + >> /** >> * DOC: interrupt handling >> * >> @@ -762,10 +767,11 @@ static int __intel_get_crtc_scanline(struct intel_crtc >> *crtc) >> return (position + crtc->scanline_offset) % vtotal; >> } >> >> -bool i915_get_crtc_scanoutpos(struct drm_device *dev, unsigned int index, >> - bool in_vblank_irq, int *vpos, int *hpos, >> - ktime_t *stime, ktime_t *etime, >> - const struct drm_display_mode *mode) >> +static bool i915_get_crtc_scanoutpos(struct drm_device *dev, >> + unsigned int index, bool in_vblank_irq, >> + int *vpos, int *hpos, >> + ktime_t *stime, ktime_t *etime, >> + const struct drm_display_mode *mode) >> { >> struct drm_i915_private *dev_priv = to_i915(dev); >> struct intel_crtc *crtc = to_intel_crtc(drm_crtc_from_index(dev, >> index)); >> @@ -879,6 +885,109 @@ bool i915_get_crtc_scanoutpos(struct drm_device *dev, >> unsigned int index, >> return true; >> } >> >> +bool i915_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, >> +unsigned int pipe, >> +int *max_error, >> +ktime_t *vblank_time, >> +bool in_vblank_irq) >> +{ >> +struct timespec64 ts_etime, ts_vblank_time; >> +ktime_t stime, etime; >> +bool vbl_status; >> +struct drm_crtc *crtc; >> +const struct drm_display_mode *mode; >> +struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; >> +int vpos, hpos, i; >> +int delta_ns, duration_ns; >> + >> +crtc = drm_crtc_from_index(dev, pipe); >> + >> +if (pipe >= dev->num_crtcs || !crtc) { >> +DRM_ERROR("Invalid crtc %u\n", pipe); >> +return false; >> +} >> + >> +if (drm_drv_uses_atomic_modeset(dev)) >> +mode = &vblank->hwmode; >> +else >> +mode = &crtc->hwmode; >> + >> +/* If mode timing undefined, just return as no-op: >> + * Happens during initial modesetting of a crtc. >> + */ >> +if (mode->crtc_clock == 0) { >> +DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); >> +WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev)); >> + >> +return false; >> +} >> + >> +/* Get current scanout position with system timestamp. >> + * Re
Re: [Freedreno] [PATCH 03/23] drm/i915: Don't use struct drm_driver.get_scanout_position()
On Fri, 10 Jan 2020, Thomas Zimmermann wrote: > The callback struct drm_driver.get_scanout_position() is deprecated in > favor of struct drm_crtc_helper_funcs.get_scanout_position(). > > i915 doesn't use CRTC helpers. The patch duplicates the caller > drm_calc_vbltimestamp_from_scanoutpos() for i915, such that the callback > function is not needed. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/i915/i915_drv.c | 3 +- > drivers/gpu/drm/i915/i915_irq.c | 117 ++-- > drivers/gpu/drm/i915/i915_irq.h | 9 +-- > 3 files changed, 119 insertions(+), 10 deletions(-) Not really enthusiastic about the diffstat in a "cleanup" series. I wonder if you could add a generic helper version of drm_calc_vbltimestamp_from_scanoutpos where you pass the get_scanout_position function as a parameter. Both drm_calc_vbltimestamp_from_scanoutpos and the new i915_calc_vbltimestamp_from_scanoutpos would then be fairly thin wrappers passing in the relevant get_scanout_position function. This would reduce the almost identical duplication of the function in i915. BR, Jani. > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index f7385abdd74b..4a0a7fb85c53 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -2769,8 +2769,7 @@ static struct drm_driver driver = { > .gem_prime_export = i915_gem_prime_export, > .gem_prime_import = i915_gem_prime_import, > > - .get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos, > - .get_scanout_position = i915_get_crtc_scanoutpos, > + .get_vblank_timestamp = i915_calc_vbltimestamp_from_scanoutpos, > > .dumb_create = i915_gem_dumb_create, > .dumb_map_offset = i915_gem_dumb_mmap_offset, > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index afc6aad9bf8c..99d0c3b0feae 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -52,6 +52,11 @@ > #include "i915_trace.h" > #include "intel_pm.h" > > +/* Retry timestamp calculation up to 3 times to satisfy > + * drm_timestamp_precision before giving up. > + */ > +#define I915_TIMESTAMP_MAXRETRIES 3 > + > /** > * DOC: interrupt handling > * > @@ -762,10 +767,11 @@ static int __intel_get_crtc_scanline(struct intel_crtc > *crtc) > return (position + crtc->scanline_offset) % vtotal; > } > > -bool i915_get_crtc_scanoutpos(struct drm_device *dev, unsigned int index, > - bool in_vblank_irq, int *vpos, int *hpos, > - ktime_t *stime, ktime_t *etime, > - const struct drm_display_mode *mode) > +static bool i915_get_crtc_scanoutpos(struct drm_device *dev, > + unsigned int index, bool in_vblank_irq, > + int *vpos, int *hpos, > + ktime_t *stime, ktime_t *etime, > + const struct drm_display_mode *mode) > { > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_crtc *crtc = to_intel_crtc(drm_crtc_from_index(dev, > index)); > @@ -879,6 +885,109 @@ bool i915_get_crtc_scanoutpos(struct drm_device *dev, > unsigned int index, > return true; > } > > +bool i915_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, > + unsigned int pipe, > + int *max_error, > + ktime_t *vblank_time, > + bool in_vblank_irq) > +{ > + struct timespec64 ts_etime, ts_vblank_time; > + ktime_t stime, etime; > + bool vbl_status; > + struct drm_crtc *crtc; > + const struct drm_display_mode *mode; > + struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > + int vpos, hpos, i; > + int delta_ns, duration_ns; > + > + crtc = drm_crtc_from_index(dev, pipe); > + > + if (pipe >= dev->num_crtcs || !crtc) { > + DRM_ERROR("Invalid crtc %u\n", pipe); > + return false; > + } > + > + if (drm_drv_uses_atomic_modeset(dev)) > + mode = &vblank->hwmode; > + else > + mode = &crtc->hwmode; > + > + /* If mode timing undefined, just return as no-op: > + * Happens during initial modesetting of a crtc. > + */ > + if (mode->crtc_clock == 0) { > + DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); > + WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev)); > + > + return false; > + } > + > + /* Get current scanout position with system timestamp. > + * Repeat query up to DRM_TIMESTAMP_MAXRETRIES times > + * if single query takes longer than max_error nanoseconds. > + * > + * This guarantees a tight bound on maximum error if > + * code gets preempted or delayed for some reason. > + */ > +
[Freedreno] [PATCH 03/23] drm/i915: Don't use struct drm_driver.get_scanout_position()
The callback struct drm_driver.get_scanout_position() is deprecated in favor of struct drm_crtc_helper_funcs.get_scanout_position(). i915 doesn't use CRTC helpers. The patch duplicates the caller drm_calc_vbltimestamp_from_scanoutpos() for i915, such that the callback function is not needed. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/i915/i915_drv.c | 3 +- drivers/gpu/drm/i915/i915_irq.c | 117 ++-- drivers/gpu/drm/i915/i915_irq.h | 9 +-- 3 files changed, 119 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index f7385abdd74b..4a0a7fb85c53 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2769,8 +2769,7 @@ static struct drm_driver driver = { .gem_prime_export = i915_gem_prime_export, .gem_prime_import = i915_gem_prime_import, - .get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos, - .get_scanout_position = i915_get_crtc_scanoutpos, + .get_vblank_timestamp = i915_calc_vbltimestamp_from_scanoutpos, .dumb_create = i915_gem_dumb_create, .dumb_map_offset = i915_gem_dumb_mmap_offset, diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index afc6aad9bf8c..99d0c3b0feae 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -52,6 +52,11 @@ #include "i915_trace.h" #include "intel_pm.h" +/* Retry timestamp calculation up to 3 times to satisfy + * drm_timestamp_precision before giving up. + */ +#define I915_TIMESTAMP_MAXRETRIES 3 + /** * DOC: interrupt handling * @@ -762,10 +767,11 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) return (position + crtc->scanline_offset) % vtotal; } -bool i915_get_crtc_scanoutpos(struct drm_device *dev, unsigned int index, - bool in_vblank_irq, int *vpos, int *hpos, - ktime_t *stime, ktime_t *etime, - const struct drm_display_mode *mode) +static bool i915_get_crtc_scanoutpos(struct drm_device *dev, +unsigned int index, bool in_vblank_irq, +int *vpos, int *hpos, +ktime_t *stime, ktime_t *etime, +const struct drm_display_mode *mode) { struct drm_i915_private *dev_priv = to_i915(dev); struct intel_crtc *crtc = to_intel_crtc(drm_crtc_from_index(dev, index)); @@ -879,6 +885,109 @@ bool i915_get_crtc_scanoutpos(struct drm_device *dev, unsigned int index, return true; } +bool i915_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, + unsigned int pipe, + int *max_error, + ktime_t *vblank_time, + bool in_vblank_irq) +{ + struct timespec64 ts_etime, ts_vblank_time; + ktime_t stime, etime; + bool vbl_status; + struct drm_crtc *crtc; + const struct drm_display_mode *mode; + struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; + int vpos, hpos, i; + int delta_ns, duration_ns; + + crtc = drm_crtc_from_index(dev, pipe); + + if (pipe >= dev->num_crtcs || !crtc) { + DRM_ERROR("Invalid crtc %u\n", pipe); + return false; + } + + if (drm_drv_uses_atomic_modeset(dev)) + mode = &vblank->hwmode; + else + mode = &crtc->hwmode; + + /* If mode timing undefined, just return as no-op: +* Happens during initial modesetting of a crtc. +*/ + if (mode->crtc_clock == 0) { + DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); + WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev)); + + return false; + } + + /* Get current scanout position with system timestamp. +* Repeat query up to DRM_TIMESTAMP_MAXRETRIES times +* if single query takes longer than max_error nanoseconds. +* +* This guarantees a tight bound on maximum error if +* code gets preempted or delayed for some reason. +*/ + for (i = 0; i < I915_TIMESTAMP_MAXRETRIES; i++) { + /* +* Get vertical and horizontal scanout position vpos, hpos, +* and bounding timestamps stime, etime, pre/post query. +*/ + vbl_status = i915_get_crtc_scanoutpos(dev, pipe, in_vblank_irq, + &vpos, &hpos, &stime, + &etime, mode); + /* Return as no-op if scanout query unsupported or failed. */ + if (!vbl_status) { + DRM_DEBUG("crtc %u : scanoutpos query failed.\n", +