Re: [PATCH 6/6] drm/i915: Sanitize drm crtc vblank counter after DC reset frame count.

From: Daniel Vetter <daniel@ffwll.ch>
To: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>,
	DRI mailing list <dri-devel@lists.freedesktop.org>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH 6/6] drm/i915: Sanitize drm crtc vblank counter after DC reset frame count.
Date: Mon, 8 Aug 2016 10:12:47 +0200	[thread overview]
Message-ID: <20160808081247.GT6232@phenom.ffwll.local> (raw)
In-Reply-To: <CABVU7+vrUZx_cqSmLtCVD2-BaVpTLqDhW9tSi3hdppknSyKoFw@mail.gmail.com>

On Fri, Aug 05, 2016 at 02:49:44PM -0700, Rodrigo Vivi wrote:
> On Thu, Aug 4, 2016 at 1:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Aug 03, 2016 at 02:33:39PM -0700, Rodrigo Vivi wrote:
> >> DC state reset the frame counter that is a read-only register.
> >>
> >> So, besides blocking DC state on vblank let's restore the
> >> drm crtc vblank counter to a place we know it is reliable.
> >>
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 4efe20c..82d6896 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -2759,7 +2759,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
> >>  static void gen9_prepare_vblank(struct drm_device *dev, unsigned int pipe)
> >>  {
> >>       struct drm_i915_private *dev_priv = to_i915(dev);
> >> +     struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> >>       intel_display_power_get(dev_priv, POWER_DOMAIN_VBLANK);
> >> +     drm_crtc_vblank_sanitize_counter(crtc);
> >
> > I think this should be done within the platform power code. Otherwise
> > something else might keep the system out of dc states, but then we might
> > wreak havoc by calling this function here.
> 
> This is safe here because DC is handled as a power_well... so as long
> as there is one domain holding its reference DC will be off.
> 
> Also this needs to be in a place we are sure that vblanks are yet disabled.
> 
> Now it comes back to that point on how to make sure that we only run 1
> prepare pre-enable...
> 
> multiple prepare/unprepares will keep trying to resync it useless and
> if we have that WARN_ON we will get flooded....

Yeah, my comment was under the assumption that there can be multiple
prepare/unprepare, and then it makes sense to reuse the refcounting we
already have. Still not sure it'll make sense to implement
prepare/unprepare refcounting in the vblank code, it'll be fairly tricky
in there. And for use useless, since our power well code already has
refcounting.
-Daniel
> 
> > -Daniel
> >
> >>  }
> >>
> >>  static void gen9_unprepare_vblank(struct drm_device *dev, unsigned int pipe)
> >> --
> >> 2.4.3
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel