Re: [Intel-gfx] [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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [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 = >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 = >hwmode;
>>> + else
>>> + mode = >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
Re: [Intel-gfx] [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 = >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 = >hwmode;
>> +else
>> +mode = >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
Re: [Intel-gfx] [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 = >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 = >hwmode;
> + else
> + mode = >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 <
