| Message ID | 1386758111-3446-30-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 11, 2013 at 11:34:50AM +0100, Daniel Vetter wrote: > Checking for both an irq number _and_ whether it's enabled is > redundant. Also this will breakd drivers which do their own irq > management and just set dev->irq_enabled once the irq stuff is all set > up. I don't think it'll break such drivers because they shouldn't have DRIVER_HAVE_IRQ set in the first place. > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_irq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index c0c6bdedefef..85d88cadc543 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -1186,7 +1186,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, > unsigned int flags, seq, crtc, high_crtc; > > if (drm_core_check_feature(dev, DRIVER_HAVE_IRQ)) > - if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) > + if (!dev->irq_enabled) In fact, unbreaking drivers was one of the reasons I added this check a few months back (see commit 03f6509df921 'drm: Allow vblank support without DRIVER_HAVE_IRQ'). At the time I could've probably removed the call to drm_dev_to_irq() as well. I'm now thinking that perhaps we could remove the DRIVER_HAVE_IRQ check altogether now, given how I've had to explicitly make the Tegra driver set dev->irq_enabled since there are other places that require it to be set (amongst other things the conditional wait further below in the drm_wait_vblank() function). See commit 603f0cc9482e 'drm/tegra: Explicitly set irq_enabled'. Thierry
On Thu, Dec 12, 2013 at 12:29 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Dec 11, 2013 at 11:34:50AM +0100, Daniel Vetter wrote: >> Checking for both an irq number _and_ whether it's enabled is >> redundant. Also this will breakd drivers which do their own irq >> management and just set dev->irq_enabled once the irq stuff is all set >> up. > > I don't think it'll break such drivers because they shouldn't have > DRIVER_HAVE_IRQ set in the first place. Hm right. >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- >> drivers/gpu/drm/drm_irq.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c >> index c0c6bdedefef..85d88cadc543 100644 >> --- a/drivers/gpu/drm/drm_irq.c >> +++ b/drivers/gpu/drm/drm_irq.c >> @@ -1186,7 +1186,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, >> unsigned int flags, seq, crtc, high_crtc; >> >> if (drm_core_check_feature(dev, DRIVER_HAVE_IRQ)) >> - if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) >> + if (!dev->irq_enabled) > > In fact, unbreaking drivers was one of the reasons I added this check a > few months back (see commit 03f6509df921 'drm: Allow vblank support > without DRIVER_HAVE_IRQ'). At the time I could've probably removed the > call to drm_dev_to_irq() as well. > > I'm now thinking that perhaps we could remove the DRIVER_HAVE_IRQ check > altogether now, given how I've had to explicitly make the Tegra driver > set dev->irq_enabled since there are other places that require it to be > set (amongst other things the conditional wait further below in the > drm_wait_vblank() function). See commit 603f0cc9482e 'drm/tegra: > Explicitly set irq_enabled'. Yeah, I think I'll respin the patch and also drop the HAVE_IRQ check - this function here really should only care about dev->irq_enabled. And even that is kinda just a curtesy to dri1 drivers, for kms drivers interrupt support should always be available for vblank. Of course the driver can internally enabel/disable the vblank source on demand (like e.g. i915 does). I'll respin the patch. -Daniel
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index c0c6bdedefef..85d88cadc543 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1186,7 +1186,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, unsigned int flags, seq, crtc, high_crtc; if (drm_core_check_feature(dev, DRIVER_HAVE_IRQ)) - if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) + if (!dev->irq_enabled) return -EINVAL; if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
Checking for both an irq number _and_ whether it's enabled is redundant. Also this will breakd drivers which do their own irq management and just set dev->irq_enabled once the irq stuff is all set up. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)