Re: [Intel-gfx] [PATCH] drm/vblank: drop the mode argument from drm_calc_vbltimestamp_from_scanoutpos

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Intel Graphics Development" <intel-gfx@lists.freedesktop.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	"DRI Development" <dri-devel@lists.freedesktop.org>,
	"Ben Skeggs" <bskeggs@redhat.com>,
	"Mario Kleiner" <mario.kleiner@tuebingen.mpg.de>,
	"Alex Deucher" <alexander.deucher@amd.com>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	freedreno@lists.freedesktop.org,
	"Christian König" <christian.koenig@amd.com>
Subject: Re: [Intel-gfx] [PATCH] drm/vblank: drop the mode argument from drm_calc_vbltimestamp_from_scanoutpos
Date: Thu, 30 Mar 2017 16:41:57 +0300	[thread overview]
Message-ID: <20170330134157.GI30290@intel.com> (raw)
In-Reply-To: <CAKMK7uEqKbrzEiJntjv5=pYdu+YbR0M_g+um12DPjszTmEt2Mg@mail.gmail.com>

On Thu, Mar 30, 2017 at 03:27:57PM +0200, Daniel Vetter wrote:
> On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote:
> >> If we restrict this helper to only kms drivers (which is the case) we
> >> can look up the correct mode easily ourselves. But it's a bit tricky:
> >>
> >> - All legacy drivers look at crtc->hwmode. But that is update already
> >>   at the beginning of the modeset helper, which means when we disable
> >>   a pipe. Hence the final timestamps might be a bit off. But since
> >>   this is an existing bug I'm not going to change it, but just try to
> >>   be bug-for-bug compatible with the current code. This only applies
> >>   to radeon&amdgpu.
> >>
> >> - i915 tries to get it perfect by updating crtc->hwmode when the pipe
> >>   is off (i.e. vblank->enabled = false).
> >>
> >> - All other atomic drivers look at crtc->state->adjusted_mode. Those
> >>   that look at state->requested_mode simply don't adjust their mode,
> >>   so it's the same. That has two problems: Accessing crtc->state from
> >>   interrupt handling code is unsafe, and it's updated before we shut
> >>   down the pipe. For nonblocking modesets it's even worse.
> >>
> >> For atomic drivers try to implement what i915 does. To do that we add
> >> a new hwmode field to the vblank structure, and update it from
> >> drm_calc_timestamping_constants().
> >
> > i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which
> > this does not do for the new vblank->hwmode. I guess no one should
> > really end up in these codepaths with a disabled crtc, but parts of
> > drm_irq.c sort of look like they're expecting it to happen.
> >
> > So should we have some way to clear out the vblank->hwmode.crtc_clock
> > for disabled crtcs? And then maybe make some of these crtc_clock checks
> > WARN and eventually just nuke it all if it looks like nothing is hitting
> > those?
> 
> So the trouble is that with a pile of dpms on/off/on/off you could run
> drm_crtc_vblank_on/off a lot, without ever calling the
> drm_calc_vbltimestamps helper again to re-upload the mode. So I don't
> think we can clear vblank->hwmode.crtc_clock unfortunately in
> drm_crtc_vblank_off.

I was thinking that we'd just try to avoid making pontetially functional
changes here. Ie. reset where we currently reset, which I think is somewhere
in the atomic commit for i915, but definitely not in drm_crtc_vblank_off().

> 
> But what we could do (at least with atomic) is WARN in the vblank
> helper if it's called outside of drm_vblank_on/off ... Not sure how
> useful that is (it won't catch when a driver outright forgets to call
> these) or whether we have enough checks already. Would be a separate
> patch (can do ofc if we agree on what exactly).
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC