From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= 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 Message-ID: <20170330134157.GI30290@intel.com> References: <20170322083617.13361-15-daniel.vetter@ffwll.ch> <20170322205612.26619-1-daniel.vetter@ffwll.ch> <20170330120326.GG30290@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Received: from mga04.intel.com ([192.55.52.120]:28431 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933569AbdC3NmC (ORCPT ); Thu, 30 Mar 2017 09:42:02 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Daniel Vetter Cc: Intel Graphics Development , linux-arm-msm , DRI Development , Ben Skeggs , Mario Kleiner , Alex Deucher , Daniel Vetter , freedreno@lists.freedesktop.org, Christian =?iso-8859-1?Q?K=F6nig?= 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ä > 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