On Sat, Jun 22, 2013 at 12:56:46PM +0200, Thierry Reding wrote: > On Thu, Jun 20, 2013 at 04:55:03PM +0200, Daniel Vetter wrote: > > On Thu, Jun 20, 2013 at 01:00:36PM +0200, Thierry Reding wrote: > > > On Thu, Jun 20, 2013 at 12:46:21PM +0200, Laurent Pinchart wrote: > > > > Hi Thierry, > > > > > > > > On Thursday 20 June 2013 12:40:26 Thierry Reding wrote: > > > > > On Thu, Jun 20, 2013 at 12:17:25PM +0200, Laurent Pinchart wrote: > > > > > > On Thursday 20 June 2013 12:10:47 Thierry Reding wrote: > > > > > > > On Wed, Jun 19, 2013 at 02:00:45PM +0200, Laurent Pinchart wrote: > > > > > > > > Signed-off-by: Laurent Pinchart > > > > > > > > <laurent.pinchart+rene...@ideasonboard.com> > > > > > > > > --- > > > > > > > > > > > > > > > > Documentation/DocBook/drm.tmpl | 14 ++++++++------ > > > > > > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > > > diff --git a/Documentation/DocBook/drm.tmpl > > > > > > > > b/Documentation/DocBook/drm.tmpl index f9df3b8..738b727 100644 > > > > > > > > --- a/Documentation/DocBook/drm.tmpl > > > > > > > > +++ b/Documentation/DocBook/drm.tmpl > > > > > > > > @@ -186,11 +186,12 @@ > > > > > > > > > > > > > > > > <varlistentry> > > > > > > > > > > > > > > > > > > > > > > > > <term>DRIVER_HAVE_IRQ</term><term>DRIVER_IRQ_SHARED</term > > > > > > > > > > > > > > > > > <listitem><para> > > > > > > > > > > > > > > > > - DRIVER_HAVE_IRQ indicates whether the driver has > > > > > > > > an IRQ > > > > > > > > handler. The - DRM core will automatically > > > > > > > > register an > > > > > > > > interrupt handler when the - flag is set. > > > > > > > > DRIVER_IRQ_SHARED > > > > > > > > indicates whether the device & - handler > > > > > > > > support > > > > > > > > shared > > > > > > > > IRQs (note that this is required of PCI - drivers). > > > > > > > > + DRIVER_HAVE_IRQ indicates whether the driver has > > > > > > > > an IRQ > > > > > > > > handler + managed by the DRM Core. The core will > > > > > > > > support > > > > > > > > simple IRQ handler + installation when the flag is > > > > > > > > set. > > > > > > > > The > > > > > > > > installation process is + described in <xref > > > > > > > > linkend="drm-irq-registration"/>.</para> + > > > > > > > > <para>DRIVER_IRQ_SHARED indicates whether the device & > > > > > > > > handler + > > > > > > > > > > > > > > > > support shared IRQs (note that this is required of PCI > > > > > > > > drivers).> > > > > > > > > > > > > > > > > </para></listitem> > > > > > > > > > > > > > > > > </varlistentry> > > > > > > > > <varlistentry> > > > > > > > > > > > > > > > > @@ -344,7 +345,8 @@ char *date;</synopsis> > > > > > > > > > > > > > > > > The DRM core tries to facilitate IRQ handler > > > > > > > > registration > > > > > > > > and > > > > > > > > unregistration by providing > > > > > > > > <function>drm_irq_install</function> and > > > > > > > > <function>drm_irq_uninstall</function> functions. > > > > > > > > Those > > > > > > > > functions only > > > > > > > > > > > > > > > > - support a single interrupt per device. > > > > > > > > + support a single interrupt per device, devices that > > > > > > > > use > > > > > > > > more > > > > > > > > than one + IRQs need to be handled manually. > > > > > > > > > > > > > > Perhaps this should mention that if you handle IRQ installation > > > > > > > manually > > > > > > > you also need to manually set drm->irq_enabled = 1, as otherwise > > > > > > > things > > > > > > > like DRM_IOCTL_WAIT_VBLANK won't work properly. > > > > > > > > > > > > That's only needed if DRIVER_HAVE_IRQ is set, otherwise the > > > > > > drm_wait_vblank() function skips the irq_enabled check. > > > > > > > > > > Not quite. There's another check for dev->irq_enabled in the > > > > > DRM_WAIT_ON() so it'll never actually block. > > > > > > > > Indeed. > > > > > > > > > Arguably this could be solved by making the DRM_WAIT_ON() condition > > > > > drop the > > > > > !dev->irq_enabled in case DRIVER_HAVE_IRQ isn't set. > > > > > > > > Or we could force drivers to set drm->irq_enabled, I'm fine with that > > > > as well. > > > > I can fix the documentation if that would be the preferred solution. > > > > > > Thinking about it some more, the latter seems more appropriate. Consider > > > some driver that doesn't set DRIVER_HAVE_IRQ but also doesn't initialize > > > interrupts manually. If so it'll cause DRM_IOCTL_WAIT_VBLANK to block > > > indefinitely. > > > > > > On the other hand perhaps the check at the beginning of drm_wait_vblank > > > should be improved. Maybe make it something like this: > > > > > > if (drm_core_check_feature(dev, DRIVER_HAVE_IRQ)) > > > if (!drm_dev_to_irq(dev)) > > > return -EINVAL; > > > > > > if (!dev->irq_enabled) > > > return -EINVAL; > > > > I think the vblank subsystem is ripe for rework, and I have a few plans > > for that. > > Would you mind sharing those plans so that maybe others can help out?
Oh, not yet solid enough to be called plans, but just a few goals: - Shift from using int pipe to struct drm_crtc *crtc for modeset drivers. This will be a bit of fun since we have to keep the old ums vblank interrupt going. Probably ends up just duplicating almost all the vblank code in core drm for the modesetting case. - Once that's done we can add solid locking (i.e. grabbing crtc->mutex at relevant places). Atm vblank handling at least in i915 is simply racy. Not a that big issue as long as userspace doesn't grab a vblank ref while doing modesets, but still. - Rework the high-precision timestamping stuff into something more clearly resembling a helper layer. Modern intel hw has a set of nice vblank/pageflip timestamp registers which could replace the current code completely and in a race-free manner. This way we could drop the vblank irq enabling hystersis on those platforms completely. We probably also want to push down the vblank handling for pageflips into drivers (since on the same new platforms we don't need to enable interrupts at all for pageflip timestamps). - Icing on the cake (and not sure whether really worth it): vblank interrupt handler and work item support. At least in i915 we have a few use cases (and more are planned) that need generic support to do stuff at/after the next n vblanks. We need both support for tight timing constrains (so probably run from the interrupt handler directly) and process context (e.g. for unpinnning after a pageflip completes). > > But till that happens I guess we could just roll forward with > > whatever hacks we currently have ... > > So that means the above sounds like a reasonable interim solution? Yeah, forcing drivers to set dev->irq_enabled is fine with me as an interim solution. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
