From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function Date: Wed, 30 Jul 2014 10:22:12 +0200 Message-ID: <20140730082212.GH4747@phenom.ffwll.local> References: <1406669543-31213-1-git-send-email-daniel.vetter@ffwll.ch> <1406669543-31213-8-git-send-email-daniel.vetter@ffwll.ch> <53D85F95.3050502@daenzer.net> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <53D85F95.3050502@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: intel-gfx@lists.freedesktop.org On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel D=E4nzer wrote: > On 30.07.2014 06:32, Daniel Vetter wrote: > > As usual in both a crtc index and a struct drm_crtc * version. > > = > > The function assumes that no one drivers their display below 10Hz, and > > it will complain if the vblank wait takes longer than that. > > = > > v2: Also check dev->max_vblank_counter since some drivers register a > > fake get_vblank_counter function. > = > What does that refer to? Can't find any other reference to > max_vblank_counter in the patch. Oops, that was from an intermediate version. The v3: text somehow got lost where I've switched the code from directly calling the ->get_counter callback to using drm_vblank_counter, which will dtrt even when there's not hw counter available. Will augment the commit message. > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > > index 0de123afdb34..76024fdde452 100644 > > --- a/drivers/gpu/drm/drm_irq.c > > +++ b/drivers/gpu/drm/drm_irq.c > > @@ -999,6 +999,51 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc) > > EXPORT_SYMBOL(drm_crtc_vblank_put); > > = > > /** > > + * drm_vblank_wait - wait for one vblank > > + * @dev: DRM device > > + * @crtc: crtc index > > + * > > + * This waits for one vblank to pass on @crtc, using the irq driver in= terfaces. > > + * It is a failure to call this when the vblank irq for @crtc is disab= le, e.g. > = > Spelling: 'disabled' > = > = > > + * due to lack of driver support or because the crtc is off. > > + */ > > +void drm_vblank_wait(struct drm_device *dev, int crtc) > > +{ > > + int ret; > > + u32 last; > > + > > + ret =3D drm_vblank_get(dev, crtc); > > + if (WARN_ON(ret)) > > + return; > > + > > + last =3D drm_vblank_count(dev, crtc); > > + > > +#define C (last !=3D drm_vblank_count(dev, crtc)) > > + ret =3D wait_event_timeout(dev->vblank[crtc].queue, > > + C, msecs_to_jiffies(100)); > > + > > + WARN_ON(ret =3D=3D 0); > > +#undef C > = > What's the point of the C macro? Usually the conditions tend to overflow the 80 char limit, so I've adopted that pattern of extracting it into a local #define. I think it'll fit here, so will roll in. > = > = > > + drm_vblank_put(dev, crtc); > > +} > > +EXPORT_SYMBOL(drm_vblank_wait); > > + > > +/** > > + * drm_crtc_vblank_wait - wait for one vblank > > + * @crtc: DRM crtc > > + * > > + * This waits for one vblank to pass on @crtc, using the irq driver in= terfaces. > > + * It is a failure to call this when the vblank irq for @crtc is disab= le, e.g. > = > Same typo as above. > = > = > > + * due to lack of driver support or because the crtc is off. > > + */ > > +void drm_crtc_vblank_wait(struct drm_crtc *crtc) > > +{ > > + drm_vblank_wait(crtc->dev, drm_crtc_index(crtc)); > > +} > > +EXPORT_SYMBOL(drm_crtc_vblank_wait); > > + > > +/** > = > Maybe the function names should be *_vblank_wait_next() or something to > clarify the purpose and reduce potential confusion versus drm_wait_vblank= (). Yeah that name is just transferred from the i915 driver. What about drm_wait_one_vblank()/drm_crtc_wait_one_vblank()? At least to my ear vblank_wait_next sounds backwards. > Looks good to me other than that. If you're ok with the name suggestion I'll polish & resend, thanks for the comments. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch