From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 07/12] drm/irq: kerneldoc polish Date: Thu, 15 May 2014 11:55:25 +0200 Message-ID: <20140515095525.GO8790@phenom.ffwll.local> References: <1400093477-3217-1-git-send-email-daniel.vetter@ffwll.ch> <1400093477-3217-8-git-send-email-daniel.vetter@ffwll.ch> <53744614.90003@daenzer.net> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-we0-f175.google.com (mail-we0-f175.google.com [74.125.82.175]) by gabe.freedesktop.org (Postfix) with ESMTP id 09A186EDE1 for ; Thu, 15 May 2014 02:55:30 -0700 (PDT) Received: by mail-we0-f175.google.com with SMTP id t61so793821wes.6 for ; Thu, 15 May 2014 02:55:29 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53744614.90003@daenzer.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Michel =?iso-8859-1?Q?D=E4nzer?= Cc: Daniel Vetter , Intel Graphics Development , DRI Development List-Id: dri-devel@lists.freedesktop.org On Thu, May 15, 2014 at 01:44:04PM +0900, Michel D=E4nzer wrote: > On 15.05.2014 03:51, Daniel Vetter wrote: > > @@ -964,8 +1005,13 @@ EXPORT_SYMBOL(drm_vblank_off); > > = > > /** > > * drm_vblank_on - enable vblank events on a CRTC > > - * @dev: DRM device > > + * @dev: drm device > > * @crtc: CRTC in question > > + * > > + * This functions restores the vblank interrupt state captured with > > + * drm_vblank_off() again. Note that calls to drm_vblank_on() and > > + * drm_vblank_off() can be unbalanced and so can also be unconditional= y called > > + * in driver load code to reflect the current hardware state of the cr= tc. > = > Given this description, maybe something like drm_vblank_save/restore > would describe better what these functions do than drm_vblank_off/on? It also enables/disables the vblank machinery itself, which at least in i915 will be important shortly - we slowly switch over our code to be more interrupt driven, including the initial plane enabling/disabling in modeset changes. > > @@ -981,11 +1027,26 @@ EXPORT_SYMBOL(drm_vblank_on); > > = > > /** > > * drm_vblank_pre_modeset - account for vblanks across mode sets > > - * @dev: DRM device > > + * @dev: drm device > > * @crtc: CRTC in question > > * > > * Account for vblank events across mode setting events, which will li= kely > > * reset the hardware frame counter. > > + * > > + * This is done by grabbing a temporary vblank reference to ensure tha= t the > > + * vblank interrupt keeps running across the modeset sequence. With th= is the > > + * software-side vblank frame counting will ensure that there are no j= umps or > > + * discontinuities. > > + * > > + * Unfortunately this approach is racy and also doesn't work when the = vblank > > + * interrupt stops running, e.g. across system suspend resume. It is t= herefore > > + * highly recommended that drivers use the newer drm_vblank_off() and > > + * drm_vblank_on() instead. drm_vblank_pre_modeset() only works correc= tly when > > + * using "cooked" software vblank frame counters and not relying on an= y hardware > > + * counters. > = > That last statement is not true IME with radeon[0]. > = > Basically, it sounds to me like drm_vblank_off/on do more or less what > drm_vblank_pre/post_modeset are intended to do (e.g. the latter can also > be nested arbitrarily). Still not really sure why we need two sets of > these instead of fixing any problems in one set. The problem is that the driver situation is a mess. So the right plan imo is: 1) Create new functions that work, beat on them. 2) Convert all drivers. 3) Rip out the old stuff. I've tried to look into this a bit and decided that this isn't something I can do without the hardware at hand and a few solid tests. So this series is 1) done for i915, with an rfc for 2). Also there's the issue of old ums using the same stuff and I think we shouldn't touch that can of worms at all. > [0] Though we certainly don't have as rigorous testing for that as you > guys do in intel-gpu-tools. Any chance some of that could be moved to > somewhere more generic? igt is mostly ready - what we need is some rather thin abstraction for the kms tests to run also on other drivers, with dumb objects. Otherwise all the test framework can cope with random bits not being there and skipping those subtests, e.g. the CRC based stuff. I don't want to extract the generic parts of the tests into a different repo (at least not if there's not _lots_ of people working on them) since the code sharing we currently do between tests is fairly massive. But I'm willing to deal with the hassle of supporting other drivers for e.g. the kms tests. And I think it would make a nice gsoc project. But it's definitely not something I can throw intel resource (or too much of my own time) at ;-) Having a shared set of tests which clearly spells out all the corner cases and tests for races would imo be awesome and greatly improve the overall health of the drm/kms drivers and wider ecosystem. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch