From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com ([134.134.136.100]:9677 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726941AbeKVDWN (ORCPT ); Wed, 21 Nov 2018 22:22:13 -0500 Date: Wed, 21 Nov 2018 18:46:57 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Daniel Vetter Cc: intel-gfx , dri-devel , stable , Inki Dae Subject: Re: [PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count Message-ID: <20181121164657.GH9144@intel.com> References: <20181112170000.27531-1-ville.syrjala@linux.intel.com> <20181112170000.27531-2-ville.syrjala@linux.intel.com> <20181121092727.GH4266@phenom.ffwll.local> <20181121113751.GA9144@intel.com> <20181121151936.GU4266@phenom.ffwll.local> <20181121161649.GG9144@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: stable-owner@vger.kernel.org List-ID: 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� > 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� > > > > > > > > > > > > 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@vger.kernel.org > > > > > > Cc: Inki Dae > > > > > > Cc: Daniel Vetter > > > > > > Signed-off-by: Ville Syrj�l� > > > > > > --- > > > > > > 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; + htotal = mode->crtc_htotal; hsync_start = mode->crtc_hsync_start; vbl_start = mode->crtc_vblank_start; -- Ville Syrj�l� Intel