Re: [PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Nov 21, 2018 at 06:46:57PM +0200, Ville Syrjälä wrote:
> On Wed, Nov 21, 2018 at 05:22:49PM +0100, Daniel Vetter wrote:
> > On Wed, Nov 21, 2018 at 5:16 PM Ville Syrjälä
> > <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Nov 21, 2018 at 04:19:36PM +0100, Daniel Vetter wrote:
> > > > On Wed, Nov 21, 2018 at 01:37:51PM +0200, Ville Syrjälä wrote:
> > > > > On Wed, Nov 21, 2018 at 10:27:27AM +0100, Daniel Vetter wrote:
> > > > > > On Mon, Nov 12, 2018 at 06:59:45PM +0200, Ville Syrjala wrote:
> > > > > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > > > >
> > > > > > > On i965gm we need to adjust max_vblank_count dynamically
> > > > > > > depending on whether the TV encoder is used or not. To
> > > > > > > that end add a per-crtc max_vblank_count that takes
> > > > > > > precedence over its device wide counterpart. The driver
> > > > > > > can now call drm_crtc_set_max_vblank_count() to configure
> > > > > > > the per-crtc value before calling drm_vblank_on().
> > > > > > >
> > > > > > > Also looks like there was some discussion about exynos needing
> > > > > > > similar treatment.
> > > > > > >
> > > > > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > > > > Cc: Inki Dae <inki.dae@xxxxxxxxxxx>
> > > > > > > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++++++++++++----
> > > > > > >  include/drm/drm_vblank.h     |  8 ++++++++
> > > > > > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > > > > index 98e091175921..c3abbdca8aba 100644
> > > > > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > > > > @@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> > > > > > >         write_sequnlock(&vblank->seqlock);
> > > > > > >  }
> > > > > > >
> > > > > > > +static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
> > > > > > > +{
> > > > > > > +       struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > > > > > > +
> > > > > > > +       return vblank->max_vblank_count ?: dev->max_vblank_count;
> > > > > > > +}
> > > > > > > +
> > > > > > >  /*
> > > > > > >   * "No hw counter" fallback implementation of .get_vblank_counter() hook,
> > > > > > >   * if there is no useable hardware frame counter available.
> > > > > > >   */
> > > > > > >  static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
> > > > > > >  {
> > > > > > > -       WARN_ON_ONCE(dev->max_vblank_count != 0);
> > > > > > > +       WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > > > >         ktime_t t_vblank;
> > > > > > >         int count = DRM_TIMESTAMP_MAXRETRIES;
> > > > > > >         int framedur_ns = vblank->framedur_ns;
> > > > > > > +       u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
> > > > > > >
> > > > > > >         /*
> > > > > > >          * Interrupts were disabled prior to this call, so deal with counter
> > > > > > > @@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > > > >                 rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
> > > > > > >         } while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> > > > > > >
> > > > > > > -       if (dev->max_vblank_count != 0) {
> > > > > > > +       if (max_vblank_count) {
> > > > > > >                 /* trust the hw counter when it's around */
> > > > > > > -               diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
> > > > > > > +               diff = (cur_vblank - vblank->last) & max_vblank_count;
> > > > > > >         } else if (rc && framedur_ns) {
> > > > > > >                 u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> > > > > > >
> > > > > > > @@ -258,7 +266,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > > > >                       pipe, vblank->count, diff, cur_vblank, vblank->last);
> > > > > > >
> > > > > > >         if (diff == 0) {
> > > > > > > -               WARN_ON_ONCE(cur_vblank != vblank->last);
> > > > > > > +               WARN_ON_ONCE(max_vblank_count &&
> > > > > > > +                            cur_vblank != vblank->last);
> > > > > >
> > > > > > Unrelated bugfix for this warning? Should be a separate patch I think, or
> > > > > > I'm missing something.
> > > > >
> > > > > Ah, yeah this was due to a quirk of i965gm hardware. The hw counter
> > > > > does work until the exact point when we enable TV encoder. Thus we
> > > > > will get non-zero values up to that point, and since the TV encoder
> > > > > isn't yet throttling the pipe it presumably runs at the oversample
> > > > > clock so our timestamp based estimates can give us a diff==0 even
> > > > > though the pipe did indeed pass a vblank already. I forgot to
> > > > > note this in the commit message.
> > > > >
> > > > > I think we can handle this three ways:
> > > > > 1. do what I do here and just let the mismatch slip through
> > > > > 2. force i915_get_vblank_counter() to return 0 always when the
> > > > >    TV encoder is going to be used
> > > > > 3. don't call drm_crtc_set_max_vblank_count() before drm_vblank_on()
> > > > >    and instead delay it until just before we enable the TV encoder
> > > > >
> > > > > I think option 3 is overly complicated to consider seriously. So
> > > > > option 1 or option 2 is what I think we should do. For whatever
> > > > > reason I went with option 1 here, but maybe option 2 is better
> > > > > since it would be all contained within i915...
> > > >
> > > > Delay drm_crtc_vblank_on until the vblank is stable? That seems like the
> > > > semantically clean solution to me, instead of hacking around in core code
> > > > when drivers leak garbage out ...
> > >
> > > We need a vblank wait before turning on the TV encoder. Chicken vs. egg.
> > 
> > Gah.
> > 
> > Ok I think I think having the hack here makes sense then, but split
> > out as a separate patch with separate justification. It's a bit a
> > tricky thing that deserves to be higlighted (and easier way to found
> > the explanation with git blame in case it's ever needed).
> 
> Or we go with option 2. It doesn't seem too bad actually.
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 019cb685986c..26d86aedc7ac 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -822,11 +822,26 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
>  static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>  {
>         struct drm_i915_private *dev_priv = to_i915(dev);
> +       struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +       const struct drm_display_mode *mode = vblank->hwmode;
>         i915_reg_t high_frame, low_frame;
>         u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> -       const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
>         unsigned long irqflags;
>  
> +       /*
> +        * On i965gm TV output the frame counter works up to
> +        * the point when we enable the TV encoder. After that the
> +        * frame counter ceases to work and reads zero. We need a
> +        * vblank wait before enabling the TV encoder and so we
> +        * have to enable vblank interrupts while the frame counter
> +        * is still in a working state. However the core vblank code
> +        * does not like us returning non-zero frame counter values
> +        * when we've told it that we don't have a working frame
> +        * counter. Thus we must stop non-zero values leaking out.
> +        */
> +       if (!vblank->max_vblank_count)
> +               return 0;

Hm yeah, looks reasonable actually. And less hacks in the shared code.
-Daniel

> +
>         htotal = mode->crtc_htotal;
>         hsync_start = mode->crtc_hsync_start;
>         vbl_start = mode->crtc_vblank_start;
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux