Re: [PATCH 29/50] drm/irq: simplify irq checks in drm_wait_vblank

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 29/50] drm/irq: simplify irq checks in drm_wait_vblank
Date: Thu, 12 Dec 2013 13:51:21 +0100	[thread overview]
Message-ID: <CAKMK7uHbXEusx0-Vi0ztt7UJmRxv7cBMw2FZvEXpuwNd9a-xJg@mail.gmail.com> (raw)
In-Reply-To: <20131212112941.GD11524@ulmo.nvidia.com>

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch