diff mbox series

[v3,04/27] drm: Don't test for IRQ support in VBLANK ioctls

Message ID 20210624072916.27703-5-tzimmermann@suse.de (mailing list archive)
State Superseded
Headers show
Series Deprecate struct drm_device.irq_enabled | expand

Commit Message

Thomas Zimmermann June 24, 2021, 7:28 a.m. UTC
For KMS drivers, replace the IRQ check in VBLANK ioctls with a check for
vblank support. IRQs might be enabled wthout vblanking being supported.

This change also removes the DRM framework's only dependency on IRQ state
for non-legacy drivers. For legacy drivers with userspace modesetting,
the original test remains in drm_wait_vblank_ioctl().

v3:
	* optimize test in drm_wait_vblank_ioctl() for KMS case (Liviu)
	* update docs for drm_irq_uninstall()
v2:
	* keep the old test for legacy drivers in
	  drm_wait_vblank_ioctl() (Daniel)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c    | 13 ++++---------
 drivers/gpu/drm/drm_vblank.c | 16 ++++++++++++----
 2 files changed, 16 insertions(+), 13 deletions(-)

Comments

Jani Nikula June 24, 2021, 8:06 a.m. UTC | #1
On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> For KMS drivers, replace the IRQ check in VBLANK ioctls with a check for
> vblank support. IRQs might be enabled wthout vblanking being supported.
>
> This change also removes the DRM framework's only dependency on IRQ state
> for non-legacy drivers. For legacy drivers with userspace modesetting,
> the original test remains in drm_wait_vblank_ioctl().
>
> v3:
> 	* optimize test in drm_wait_vblank_ioctl() for KMS case (Liviu)
> 	* update docs for drm_irq_uninstall()
> v2:
> 	* keep the old test for legacy drivers in
> 	  drm_wait_vblank_ioctl() (Daniel)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c    | 13 ++++---------
>  drivers/gpu/drm/drm_vblank.c | 16 ++++++++++++----
>  2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index c3bd664ea733..945dd82e2ea3 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -74,10 +74,8 @@
>   * only supports devices with a single interrupt on the main device stored in
>   * &drm_device.dev and set as the device paramter in drm_dev_alloc().
>   *
> - * These IRQ helpers are strictly optional. Drivers which roll their own only
> - * need to set &drm_device.irq_enabled to signal the DRM core that vblank
> - * interrupts are working. Since these helpers don't automatically clean up the
> - * requested interrupt like e.g. devm_request_irq() they're not really
> + * These IRQ helpers are strictly optional. Since these helpers don't automatically
> + * clean up the requested interrupt like e.g. devm_request_irq() they're not really
>   * recommended.
>   */
>  
> @@ -91,9 +89,7 @@
>   * and after the installation.
>   *
>   * This is the simplified helper interface provided for drivers with no special
> - * needs. Drivers which need to install interrupt handlers for multiple
> - * interrupts must instead set &drm_device.irq_enabled to signal the DRM core
> - * that vblank interrupts are available.
> + * needs.
>   *
>   * @irq must match the interrupt number that would be passed to request_irq(),
>   * if called directly instead of using this helper function.
> @@ -156,8 +152,7 @@ EXPORT_SYMBOL(drm_irq_install);
>   *
>   * Calls the driver's &drm_driver.irq_uninstall function and unregisters the IRQ
>   * handler.  This should only be called by drivers which used drm_irq_install()
> - * to set up their interrupt handler. Other drivers must only reset
> - * &drm_device.irq_enabled to false.
> + * to set up their interrupt handler.
>   *
>   * Note that for kernel modesetting drivers it is a bug if this function fails.
>   * The sanity checks are only to catch buggy user modesetting drivers which call
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 3417e1ac7918..10fe16bafcb6 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  	unsigned int pipe_index;
>  	unsigned int flags, pipe, high_pipe;
>  
> -	if (!dev->irq_enabled)
> -		return -EOPNOTSUPP;
> +#if defined(CONFIG_DRM_LEGACY)
> +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
> +		if (!dev->irq_enabled)
> +			return -EOPNOTSUPP;
> +	} else /* if DRIVER_MODESET */
> +#endif
> +	{
> +		if (!drm_dev_has_vblank(dev))
> +			return -EOPNOTSUPP;
> +	}

Sheesh I hate this kind of inline #ifdefs.

Two alternate suggestions that I believe should be as just efficient:

1) The more verbose:

#if defined(CONFIG_DRM_LEGACY)
static bool drm_wait_vblank_supported(struct drm_device *dev)
{
	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
		return dev->irq_enabled;
	else
		return drm_dev_has_vblank(dev);
}
#else
static bool drm_wait_vblank_supported(struct drm_device *dev)
{
	return drm_dev_has_vblank(dev);
}
#endif

2) The more compact:

static bool drm_wait_vblank_supported(struct drm_device *dev)
{
	if  (IS_ENABLED(CONFIG_DRM_LEGACY) && unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
		return dev->irq_enabled;
	else
		return drm_dev_has_vblank(dev);
}

Then, in drm_wait_vblank_ioctl().

	if (!drm_wait_vblank_supported(dev))
		return -EOPNOTSUPP;

The compiler should do the right thing without any explicit inline
keywords etc.

BR,
Jani.

>  
>  	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
>  		return -EINVAL;
> @@ -2023,7 +2031,7 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> -	if (!dev->irq_enabled)
> +	if (!drm_dev_has_vblank(dev))
>  		return -EOPNOTSUPP;
>  
>  	crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id);
> @@ -2082,7 +2090,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> -	if (!dev->irq_enabled)
> +	if (!drm_dev_has_vblank(dev))
>  		return -EOPNOTSUPP;
>  
>  	crtc = drm_crtc_find(dev, file_priv, queue_seq->crtc_id);
Thomas Zimmermann June 24, 2021, 8:19 a.m. UTC | #2
Hi

Am 24.06.21 um 10:06 schrieb Jani Nikula:
> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> For KMS drivers, replace the IRQ check in VBLANK ioctls with a check for
>> vblank support. IRQs might be enabled wthout vblanking being supported.
>>
>> This change also removes the DRM framework's only dependency on IRQ state
>> for non-legacy drivers. For legacy drivers with userspace modesetting,
>> the original test remains in drm_wait_vblank_ioctl().
>>
>> v3:
>> 	* optimize test in drm_wait_vblank_ioctl() for KMS case (Liviu)
>> 	* update docs for drm_irq_uninstall()
>> v2:
>> 	* keep the old test for legacy drivers in
>> 	  drm_wait_vblank_ioctl() (Daniel)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>   drivers/gpu/drm/drm_irq.c    | 13 ++++---------
>>   drivers/gpu/drm/drm_vblank.c | 16 ++++++++++++----
>>   2 files changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index c3bd664ea733..945dd82e2ea3 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -74,10 +74,8 @@
>>    * only supports devices with a single interrupt on the main device stored in
>>    * &drm_device.dev and set as the device paramter in drm_dev_alloc().
>>    *
>> - * These IRQ helpers are strictly optional. Drivers which roll their own only
>> - * need to set &drm_device.irq_enabled to signal the DRM core that vblank
>> - * interrupts are working. Since these helpers don't automatically clean up the
>> - * requested interrupt like e.g. devm_request_irq() they're not really
>> + * These IRQ helpers are strictly optional. Since these helpers don't automatically
>> + * clean up the requested interrupt like e.g. devm_request_irq() they're not really
>>    * recommended.
>>    */
>>   
>> @@ -91,9 +89,7 @@
>>    * and after the installation.
>>    *
>>    * This is the simplified helper interface provided for drivers with no special
>> - * needs. Drivers which need to install interrupt handlers for multiple
>> - * interrupts must instead set &drm_device.irq_enabled to signal the DRM core
>> - * that vblank interrupts are available.
>> + * needs.
>>    *
>>    * @irq must match the interrupt number that would be passed to request_irq(),
>>    * if called directly instead of using this helper function.
>> @@ -156,8 +152,7 @@ EXPORT_SYMBOL(drm_irq_install);
>>    *
>>    * Calls the driver's &drm_driver.irq_uninstall function and unregisters the IRQ
>>    * handler.  This should only be called by drivers which used drm_irq_install()
>> - * to set up their interrupt handler. Other drivers must only reset
>> - * &drm_device.irq_enabled to false.
>> + * to set up their interrupt handler.
>>    *
>>    * Note that for kernel modesetting drivers it is a bug if this function fails.
>>    * The sanity checks are only to catch buggy user modesetting drivers which call
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index 3417e1ac7918..10fe16bafcb6 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>>   	unsigned int pipe_index;
>>   	unsigned int flags, pipe, high_pipe;
>>   
>> -	if (!dev->irq_enabled)
>> -		return -EOPNOTSUPP;
>> +#if defined(CONFIG_DRM_LEGACY)
>> +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
>> +		if (!dev->irq_enabled)
>> +			return -EOPNOTSUPP;
>> +	} else /* if DRIVER_MODESET */
>> +#endif
>> +	{
>> +		if (!drm_dev_has_vblank(dev))
>> +			return -EOPNOTSUPP;
>> +	}
> 
> Sheesh I hate this kind of inline #ifdefs.

I don't like them either. I guess I'll go with suggestion 2. Thanks for 
the feedback.

Best regards
Thomas

> 
> Two alternate suggestions that I believe should be as just efficient:
> 
> 1) The more verbose:
> 
> #if defined(CONFIG_DRM_LEGACY)
> static bool drm_wait_vblank_supported(struct drm_device *dev)
> {
> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> 		return dev->irq_enabled;
> 	else
> 		return drm_dev_has_vblank(dev);
> }
> #else
> static bool drm_wait_vblank_supported(struct drm_device *dev)
> {
> 	return drm_dev_has_vblank(dev);
> }
> #endif
> 
> 2) The more compact:
> 
> static bool drm_wait_vblank_supported(struct drm_device *dev)
> {
> 	if  (IS_ENABLED(CONFIG_DRM_LEGACY) && unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> 		return dev->irq_enabled;
> 	else
> 		return drm_dev_has_vblank(dev);
> }
> 
> Then, in drm_wait_vblank_ioctl().
> 
> 	if (!drm_wait_vblank_supported(dev))
> 		return -EOPNOTSUPP;
> 
> The compiler should do the right thing without any explicit inline
> keywords etc.
> 
> BR,
> Jani.
> 
>>   
>>   	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
>>   		return -EINVAL;
>> @@ -2023,7 +2031,7 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> -	if (!dev->irq_enabled)
>> +	if (!drm_dev_has_vblank(dev))
>>   		return -EOPNOTSUPP;
>>   
>>   	crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id);
>> @@ -2082,7 +2090,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> -	if (!dev->irq_enabled)
>> +	if (!drm_dev_has_vblank(dev))
>>   		return -EOPNOTSUPP;
>>   
>>   	crtc = drm_crtc_find(dev, file_priv, queue_seq->crtc_id);
>
Thomas Zimmermann June 24, 2021, 8:28 a.m. UTC | #3
Hi

Am 24.06.21 um 10:06 schrieb Jani Nikula:
> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> For KMS drivers, replace the IRQ check in VBLANK ioctls with a check for
>> vblank support. IRQs might be enabled wthout vblanking being supported.
>>
>> This change also removes the DRM framework's only dependency on IRQ state
>> for non-legacy drivers. For legacy drivers with userspace modesetting,
>> the original test remains in drm_wait_vblank_ioctl().
>>
>> v3:
>> 	* optimize test in drm_wait_vblank_ioctl() for KMS case (Liviu)
>> 	* update docs for drm_irq_uninstall()
>> v2:
>> 	* keep the old test for legacy drivers in
>> 	  drm_wait_vblank_ioctl() (Daniel)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>   drivers/gpu/drm/drm_irq.c    | 13 ++++---------
>>   drivers/gpu/drm/drm_vblank.c | 16 ++++++++++++----
>>   2 files changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index c3bd664ea733..945dd82e2ea3 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -74,10 +74,8 @@
>>    * only supports devices with a single interrupt on the main device stored in
>>    * &drm_device.dev and set as the device paramter in drm_dev_alloc().
>>    *
>> - * These IRQ helpers are strictly optional. Drivers which roll their own only
>> - * need to set &drm_device.irq_enabled to signal the DRM core that vblank
>> - * interrupts are working. Since these helpers don't automatically clean up the
>> - * requested interrupt like e.g. devm_request_irq() they're not really
>> + * These IRQ helpers are strictly optional. Since these helpers don't automatically
>> + * clean up the requested interrupt like e.g. devm_request_irq() they're not really
>>    * recommended.
>>    */
>>   
>> @@ -91,9 +89,7 @@
>>    * and after the installation.
>>    *
>>    * This is the simplified helper interface provided for drivers with no special
>> - * needs. Drivers which need to install interrupt handlers for multiple
>> - * interrupts must instead set &drm_device.irq_enabled to signal the DRM core
>> - * that vblank interrupts are available.
>> + * needs.
>>    *
>>    * @irq must match the interrupt number that would be passed to request_irq(),
>>    * if called directly instead of using this helper function.
>> @@ -156,8 +152,7 @@ EXPORT_SYMBOL(drm_irq_install);
>>    *
>>    * Calls the driver's &drm_driver.irq_uninstall function and unregisters the IRQ
>>    * handler.  This should only be called by drivers which used drm_irq_install()
>> - * to set up their interrupt handler. Other drivers must only reset
>> - * &drm_device.irq_enabled to false.
>> + * to set up their interrupt handler.
>>    *
>>    * Note that for kernel modesetting drivers it is a bug if this function fails.
>>    * The sanity checks are only to catch buggy user modesetting drivers which call
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index 3417e1ac7918..10fe16bafcb6 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>>   	unsigned int pipe_index;
>>   	unsigned int flags, pipe, high_pipe;
>>   
>> -	if (!dev->irq_enabled)
>> -		return -EOPNOTSUPP;
>> +#if defined(CONFIG_DRM_LEGACY)
>> +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
>> +		if (!dev->irq_enabled)
>> +			return -EOPNOTSUPP;
>> +	} else /* if DRIVER_MODESET */
>> +#endif
>> +	{
>> +		if (!drm_dev_has_vblank(dev))
>> +			return -EOPNOTSUPP;
>> +	}
> 
> Sheesh I hate this kind of inline #ifdefs.
> 
> Two alternate suggestions that I believe should be as just efficient:

Or how about:

static bool drm_wait_vblank_supported(struct drm_device *dev)

{

if defined(CONFIG_DRM_LEGACY)
	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))

		return dev->irq_enabled;

#endif
	return drm_dev_has_vblank(dev);

}


?

It's inline, but still readable.

Best regards
Thomas

> 
> 1) The more verbose:
> 
> #if defined(CONFIG_DRM_LEGACY)
> static bool drm_wait_vblank_supported(struct drm_device *dev)
> {
> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> 		return dev->irq_enabled;
> 	else
> 		return drm_dev_has_vblank(dev);
> }
> #else
> static bool drm_wait_vblank_supported(struct drm_device *dev)
> {
> 	return drm_dev_has_vblank(dev);
> }
> #endif
> 
> 2) The more compact:
> 
> static bool drm_wait_vblank_supported(struct drm_device *dev)
> {
> 	if  (IS_ENABLED(CONFIG_DRM_LEGACY) && unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> 		return dev->irq_enabled;
> 	else
> 		return drm_dev_has_vblank(dev);
> }
> 
> Then, in drm_wait_vblank_ioctl().
> 
> 	if (!drm_wait_vblank_supported(dev))
> 		return -EOPNOTSUPP;
> 
> The compiler should do the right thing without any explicit inline
> keywords etc.
> 
> BR,
> Jani.
> 
>>   
>>   	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
>>   		return -EINVAL;
>> @@ -2023,7 +2031,7 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> -	if (!dev->irq_enabled)
>> +	if (!drm_dev_has_vblank(dev))
>>   		return -EOPNOTSUPP;
>>   
>>   	crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id);
>> @@ -2082,7 +2090,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> -	if (!dev->irq_enabled)
>> +	if (!drm_dev_has_vblank(dev))
>>   		return -EOPNOTSUPP;
>>   
>>   	crtc = drm_crtc_find(dev, file_priv, queue_seq->crtc_id);
>
Jani Nikula June 24, 2021, 8:51 a.m. UTC | #4
On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 24.06.21 um 10:06 schrieb Jani Nikula:
>> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>>> index 3417e1ac7918..10fe16bafcb6 100644
>>> --- a/drivers/gpu/drm/drm_vblank.c
>>> +++ b/drivers/gpu/drm/drm_vblank.c
>>> @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>>>   	unsigned int pipe_index;
>>>   	unsigned int flags, pipe, high_pipe;
>>>   
>>> -	if (!dev->irq_enabled)
>>> -		return -EOPNOTSUPP;
>>> +#if defined(CONFIG_DRM_LEGACY)
>>> +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
>>> +		if (!dev->irq_enabled)
>>> +			return -EOPNOTSUPP;
>>> +	} else /* if DRIVER_MODESET */
>>> +#endif
>>> +	{
>>> +		if (!drm_dev_has_vblank(dev))
>>> +			return -EOPNOTSUPP;
>>> +	}
>> 
>> Sheesh I hate this kind of inline #ifdefs.
>> 
>> Two alternate suggestions that I believe should be as just efficient:
>
> Or how about:
>
> static bool drm_wait_vblank_supported(struct drm_device *dev)
>
> {
>
> if defined(CONFIG_DRM_LEGACY)
> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>
> 		return dev->irq_enabled;
>
> #endif
> 	return drm_dev_has_vblank(dev);
>
> }
>
>
> ?
>
> It's inline, but still readable.

It's definitely better than the original, but it's unclear to me why
you'd prefer this over option 2) below. I guess the only reason I can
think of is emphasizing the conditional compilation. However,
IS_ENABLED() is widely used in this manner specifically to avoid inline
#if, and the compiler optimizes it away.

BR,
Jani.


>
> Best regards
> Thomas
>
>> 
>> 1) The more verbose:
>> 
>> #if defined(CONFIG_DRM_LEGACY)
>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>> {
>> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>> 		return dev->irq_enabled;
>> 	else
>> 		return drm_dev_has_vblank(dev);
>> }
>> #else
>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>> {
>> 	return drm_dev_has_vblank(dev);
>> }
>> #endif
>> 
>> 2) The more compact:
>> 
>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>> {
>> 	if  (IS_ENABLED(CONFIG_DRM_LEGACY) && unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>> 		return dev->irq_enabled;
>> 	else
>> 		return drm_dev_has_vblank(dev);
>> }
>> 
>> Then, in drm_wait_vblank_ioctl().
>> 
>> 	if (!drm_wait_vblank_supported(dev))
>> 		return -EOPNOTSUPP;
>> 
>> The compiler should do the right thing without any explicit inline
>> keywords etc.
>> 
>> BR,
>> Jani.
>> 
>>>   
>>>   	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
>>>   		return -EINVAL;
>>> @@ -2023,7 +2031,7 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>   		return -EOPNOTSUPP;
>>>   
>>> -	if (!dev->irq_enabled)
>>> +	if (!drm_dev_has_vblank(dev))
>>>   		return -EOPNOTSUPP;
>>>   
>>>   	crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id);
>>> @@ -2082,7 +2090,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
>>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>   		return -EOPNOTSUPP;
>>>   
>>> -	if (!dev->irq_enabled)
>>> +	if (!drm_dev_has_vblank(dev))
>>>   		return -EOPNOTSUPP;
>>>   
>>>   	crtc = drm_crtc_find(dev, file_priv, queue_seq->crtc_id);
>>
Thomas Zimmermann June 24, 2021, 9:07 a.m. UTC | #5
Hi

Am 24.06.21 um 10:51 schrieb Jani Nikula:
> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 24.06.21 um 10:06 schrieb Jani Nikula:
>>> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>>>> index 3417e1ac7918..10fe16bafcb6 100644
>>>> --- a/drivers/gpu/drm/drm_vblank.c
>>>> +++ b/drivers/gpu/drm/drm_vblank.c
>>>> @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>>>>    	unsigned int pipe_index;
>>>>    	unsigned int flags, pipe, high_pipe;
>>>>    
>>>> -	if (!dev->irq_enabled)
>>>> -		return -EOPNOTSUPP;
>>>> +#if defined(CONFIG_DRM_LEGACY)
>>>> +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
>>>> +		if (!dev->irq_enabled)
>>>> +			return -EOPNOTSUPP;
>>>> +	} else /* if DRIVER_MODESET */
>>>> +#endif
>>>> +	{
>>>> +		if (!drm_dev_has_vblank(dev))
>>>> +			return -EOPNOTSUPP;
>>>> +	}
>>>
>>> Sheesh I hate this kind of inline #ifdefs.
>>>
>>> Two alternate suggestions that I believe should be as just efficient:
>>
>> Or how about:
>>
>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>>
>> {
>>
>> if defined(CONFIG_DRM_LEGACY)
>> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>>
>> 		return dev->irq_enabled;
>>
>> #endif
>> 	return drm_dev_has_vblank(dev);
>>
>> }
>>
>>
>> ?
>>
>> It's inline, but still readable.
> 
> It's definitely better than the original, but it's unclear to me why
> you'd prefer this over option 2) below. I guess the only reason I can
> think of is emphasizing the conditional compilation. However,
> IS_ENABLED() is widely used in this manner specifically to avoid inline
> #if, and the compiler optimizes it away.

It's simply more readable to me as the condition is simpler. But option 
2 is also ok.

Best regards
Thomas

> 
> BR,
> Jani.
> 
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> 1) The more verbose:
>>>
>>> #if defined(CONFIG_DRM_LEGACY)
>>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>>> {
>>> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>>> 		return dev->irq_enabled;
>>> 	else
>>> 		return drm_dev_has_vblank(dev);
>>> }
>>> #else
>>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>>> {
>>> 	return drm_dev_has_vblank(dev);
>>> }
>>> #endif
>>>
>>> 2) The more compact:
>>>
>>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>>> {
>>> 	if  (IS_ENABLED(CONFIG_DRM_LEGACY) && unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>>> 		return dev->irq_enabled;
>>> 	else
>>> 		return drm_dev_has_vblank(dev);
>>> }
>>>
>>> Then, in drm_wait_vblank_ioctl().
>>>
>>> 	if (!drm_wait_vblank_supported(dev))
>>> 		return -EOPNOTSUPP;
>>>
>>> The compiler should do the right thing without any explicit inline
>>> keywords etc.
>>>
>>> BR,
>>> Jani.
>>>
>>>>    
>>>>    	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
>>>>    		return -EINVAL;
>>>> @@ -2023,7 +2031,7 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>>>>    	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>> -	if (!dev->irq_enabled)
>>>> +	if (!drm_dev_has_vblank(dev))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>>    	crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id);
>>>> @@ -2082,7 +2090,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
>>>>    	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>> -	if (!dev->irq_enabled)
>>>> +	if (!drm_dev_has_vblank(dev))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>>    	crtc = drm_crtc_find(dev, file_priv, queue_seq->crtc_id);
>>>
>
Liviu Dudau June 24, 2021, 10:27 a.m. UTC | #6
On Thu, Jun 24, 2021 at 11:07:57AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 24.06.21 um 10:51 schrieb Jani Nikula:
> > On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > > Hi
> > > 
> > > Am 24.06.21 um 10:06 schrieb Jani Nikula:
> > > > On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > > index 3417e1ac7918..10fe16bafcb6 100644
> > > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > > @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
> > > > >    	unsigned int pipe_index;
> > > > >    	unsigned int flags, pipe, high_pipe;
> > > > > -	if (!dev->irq_enabled)
> > > > > -		return -EOPNOTSUPP;
> > > > > +#if defined(CONFIG_DRM_LEGACY)
> > > > > +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
> > > > > +		if (!dev->irq_enabled)
> > > > > +			return -EOPNOTSUPP;
> > > > > +	} else /* if DRIVER_MODESET */
> > > > > +#endif
> > > > > +	{
> > > > > +		if (!drm_dev_has_vblank(dev))
> > > > > +			return -EOPNOTSUPP;
> > > > > +	}
> > > > 
> > > > Sheesh I hate this kind of inline #ifdefs.
> > > > 
> > > > Two alternate suggestions that I believe should be as just efficient:
> > > 
> > > Or how about:
> > > 
> > > static bool drm_wait_vblank_supported(struct drm_device *dev)
> > > 
> > > {
> > > 
> > > if defined(CONFIG_DRM_LEGACY)
> > > 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> > > 
> > > 		return dev->irq_enabled;
> > > 
> > > #endif
> > > 	return drm_dev_has_vblank(dev);
> > > 
> > > }
> > > 
> > > 
> > > ?
> > > 
> > > It's inline, but still readable.
> > 
> > It's definitely better than the original, but it's unclear to me why
> > you'd prefer this over option 2) below. I guess the only reason I can
> > think of is emphasizing the conditional compilation. However,
> > IS_ENABLED() is widely used in this manner specifically to avoid inline
> > #if, and the compiler optimizes it away.
> 
> It's simply more readable to me as the condition is simpler. But option 2 is
> also ok.

Either option looks good to me.

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Thanks for doing that!
Liviu

> 
> Best regards
> Thomas
> 
> > 
> > BR,
> > Jani.
> > 
> > 
> > > 
> > > Best regards
> > > Thomas
> > > 
> > > > 
> > > > 1) The more verbose:
> > > > 
> > > > #if defined(CONFIG_DRM_LEGACY)
> > > > static bool drm_wait_vblank_supported(struct drm_device *dev)
> > > > {
> > > > 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> > > > 		return dev->irq_enabled;
> > > > 	else
> > > > 		return drm_dev_has_vblank(dev);
> > > > }
> > > > #else
> > > > static bool drm_wait_vblank_supported(struct drm_device *dev)
> > > > {
> > > > 	return drm_dev_has_vblank(dev);
> > > > }
> > > > #endif
> > > > 
> > > > 2) The more compact:
> > > > 
> > > > static bool drm_wait_vblank_supported(struct drm_device *dev)
> > > > {
> > > > 	if  (IS_ENABLED(CONFIG_DRM_LEGACY) && unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> > > > 		return dev->irq_enabled;
> > > > 	else
> > > > 		return drm_dev_has_vblank(dev);
> > > > }
> > > > 
> > > > Then, in drm_wait_vblank_ioctl().
> > > > 
> > > > 	if (!drm_wait_vblank_supported(dev))
> > > > 		return -EOPNOTSUPP;
> > > > 
> > > > The compiler should do the right thing without any explicit inline
> > > > keywords etc.
> > > > 
> > > > BR,
> > > > Jani.
> > > > 
> > > > >    	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
> > > > >    		return -EINVAL;
> > > > > @@ -2023,7 +2031,7 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> > > > >    	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > > > >    		return -EOPNOTSUPP;
> > > > > -	if (!dev->irq_enabled)
> > > > > +	if (!drm_dev_has_vblank(dev))
> > > > >    		return -EOPNOTSUPP;
> > > > >    	crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id);
> > > > > @@ -2082,7 +2090,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> > > > >    	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > > > >    		return -EOPNOTSUPP;
> > > > > -	if (!dev->irq_enabled)
> > > > > +	if (!drm_dev_has_vblank(dev))
> > > > >    		return -EOPNOTSUPP;
> > > > >    	crtc = drm_crtc_find(dev, file_priv, queue_seq->crtc_id);
> > > > 
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
Thierry Reding June 24, 2021, 12:03 p.m. UTC | #7
On Thu, Jun 24, 2021 at 11:07:57AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 24.06.21 um 10:51 schrieb Jani Nikula:
> > On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > > Hi
> > > 
> > > Am 24.06.21 um 10:06 schrieb Jani Nikula:
> > > > On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > > index 3417e1ac7918..10fe16bafcb6 100644
> > > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > > @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
> > > > >    	unsigned int pipe_index;
> > > > >    	unsigned int flags, pipe, high_pipe;
> > > > > -	if (!dev->irq_enabled)
> > > > > -		return -EOPNOTSUPP;
> > > > > +#if defined(CONFIG_DRM_LEGACY)
> > > > > +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
> > > > > +		if (!dev->irq_enabled)
> > > > > +			return -EOPNOTSUPP;
> > > > > +	} else /* if DRIVER_MODESET */
> > > > > +#endif
> > > > > +	{
> > > > > +		if (!drm_dev_has_vblank(dev))
> > > > > +			return -EOPNOTSUPP;
> > > > > +	}
> > > > 
> > > > Sheesh I hate this kind of inline #ifdefs.
> > > > 
> > > > Two alternate suggestions that I believe should be as just efficient:
> > > 
> > > Or how about:
> > > 
> > > static bool drm_wait_vblank_supported(struct drm_device *dev)
> > > 
> > > {
> > > 
> > > if defined(CONFIG_DRM_LEGACY)
> > > 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> > > 
> > > 		return dev->irq_enabled;
> > > 
> > > #endif
> > > 	return drm_dev_has_vblank(dev);
> > > 
> > > }
> > > 
> > > 
> > > ?
> > > 
> > > It's inline, but still readable.
> > 
> > It's definitely better than the original, but it's unclear to me why
> > you'd prefer this over option 2) below. I guess the only reason I can
> > think of is emphasizing the conditional compilation. However,
> > IS_ENABLED() is widely used in this manner specifically to avoid inline
> > #if, and the compiler optimizes it away.
> 
> It's simply more readable to me as the condition is simpler. But option 2 is
> also ok.

Perhaps do something like this, then:

	if (IS_ENABLED(CONFIG_DRM_LEGACY)) {
		if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
			return dev->irq_enabled;
	}

	return drm_dev_has_vblank(dev);

That's about just as readable as the variant involving the preprocessor
but has all the benefits of not using the preprocessor.

Thierry
Jani Nikula June 24, 2021, 12:36 p.m. UTC | #8
On Thu, 24 Jun 2021, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Jun 24, 2021 at 11:07:57AM +0200, Thomas Zimmermann wrote:
>> Hi
>> 
>> Am 24.06.21 um 10:51 schrieb Jani Nikula:
>> > On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> > > Hi
>> > > 
>> > > Am 24.06.21 um 10:06 schrieb Jani Nikula:
>> > > > On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> > > > > index 3417e1ac7918..10fe16bafcb6 100644
>> > > > > --- a/drivers/gpu/drm/drm_vblank.c
>> > > > > +++ b/drivers/gpu/drm/drm_vblank.c
>> > > > > @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>> > > > >    	unsigned int pipe_index;
>> > > > >    	unsigned int flags, pipe, high_pipe;
>> > > > > -	if (!dev->irq_enabled)
>> > > > > -		return -EOPNOTSUPP;
>> > > > > +#if defined(CONFIG_DRM_LEGACY)
>> > > > > +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
>> > > > > +		if (!dev->irq_enabled)
>> > > > > +			return -EOPNOTSUPP;
>> > > > > +	} else /* if DRIVER_MODESET */
>> > > > > +#endif
>> > > > > +	{
>> > > > > +		if (!drm_dev_has_vblank(dev))
>> > > > > +			return -EOPNOTSUPP;
>> > > > > +	}
>> > > > 
>> > > > Sheesh I hate this kind of inline #ifdefs.
>> > > > 
>> > > > Two alternate suggestions that I believe should be as just efficient:
>> > > 
>> > > Or how about:
>> > > 
>> > > static bool drm_wait_vblank_supported(struct drm_device *dev)
>> > > 
>> > > {
>> > > 
>> > > if defined(CONFIG_DRM_LEGACY)
>> > > 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>> > > 
>> > > 		return dev->irq_enabled;
>> > > 
>> > > #endif
>> > > 	return drm_dev_has_vblank(dev);
>> > > 
>> > > }
>> > > 
>> > > 
>> > > ?
>> > > 
>> > > It's inline, but still readable.
>> > 
>> > It's definitely better than the original, but it's unclear to me why
>> > you'd prefer this over option 2) below. I guess the only reason I can
>> > think of is emphasizing the conditional compilation. However,
>> > IS_ENABLED() is widely used in this manner specifically to avoid inline
>> > #if, and the compiler optimizes it away.
>> 
>> It's simply more readable to me as the condition is simpler. But option 2 is
>> also ok.
>
> Perhaps do something like this, then:
>
> 	if (IS_ENABLED(CONFIG_DRM_LEGACY)) {
> 		if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> 			return dev->irq_enabled;
> 	}
>
> 	return drm_dev_has_vblank(dev);
>
> That's about just as readable as the variant involving the preprocessor
> but has all the benefits of not using the preprocessor.

Looks like a winner to me. :)

BR,
Jani.
Thomas Zimmermann June 24, 2021, 12:57 p.m. UTC | #9
Hi

Am 24.06.21 um 14:36 schrieb Jani Nikula:
> On Thu, 24 Jun 2021, Thierry Reding <thierry.reding@gmail.com> wrote:
>> On Thu, Jun 24, 2021 at 11:07:57AM +0200, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 24.06.21 um 10:51 schrieb Jani Nikula:
>>>> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>> Hi
>>>>>
>>>>> Am 24.06.21 um 10:06 schrieb Jani Nikula:
>>>>>> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>>>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>>>>>>> index 3417e1ac7918..10fe16bafcb6 100644
>>>>>>> --- a/drivers/gpu/drm/drm_vblank.c
>>>>>>> +++ b/drivers/gpu/drm/drm_vblank.c
>>>>>>> @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>>>>>>>     	unsigned int pipe_index;
>>>>>>>     	unsigned int flags, pipe, high_pipe;
>>>>>>> -	if (!dev->irq_enabled)
>>>>>>> -		return -EOPNOTSUPP;
>>>>>>> +#if defined(CONFIG_DRM_LEGACY)
>>>>>>> +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
>>>>>>> +		if (!dev->irq_enabled)
>>>>>>> +			return -EOPNOTSUPP;
>>>>>>> +	} else /* if DRIVER_MODESET */
>>>>>>> +#endif
>>>>>>> +	{
>>>>>>> +		if (!drm_dev_has_vblank(dev))
>>>>>>> +			return -EOPNOTSUPP;
>>>>>>> +	}
>>>>>>
>>>>>> Sheesh I hate this kind of inline #ifdefs.
>>>>>>
>>>>>> Two alternate suggestions that I believe should be as just efficient:
>>>>>
>>>>> Or how about:
>>>>>
>>>>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>>>>>
>>>>> {
>>>>>
>>>>> if defined(CONFIG_DRM_LEGACY)
>>>>> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>>>>>
>>>>> 		return dev->irq_enabled;
>>>>>
>>>>> #endif
>>>>> 	return drm_dev_has_vblank(dev);
>>>>>
>>>>> }
>>>>>
>>>>>
>>>>> ?
>>>>>
>>>>> It's inline, but still readable.
>>>>
>>>> It's definitely better than the original, but it's unclear to me why
>>>> you'd prefer this over option 2) below. I guess the only reason I can
>>>> think of is emphasizing the conditional compilation. However,
>>>> IS_ENABLED() is widely used in this manner specifically to avoid inline
>>>> #if, and the compiler optimizes it away.
>>>
>>> It's simply more readable to me as the condition is simpler. But option 2 is
>>> also ok.
>>
>> Perhaps do something like this, then:
>>
>> 	if (IS_ENABLED(CONFIG_DRM_LEGACY)) {
>> 		if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>> 			return dev->irq_enabled;
>> 	}
>>
>> 	return drm_dev_has_vblank(dev);
>>
>> That's about just as readable as the variant involving the preprocessor
>> but has all the benefits of not using the preprocessor.
> 
> Looks like a winner to me. :)

That's the most readable.

But I just remembered that irq_enabled will likely become legacy-only in 
the device structure. We'll need an ifdef variant then. :/

Best regards
Thomas

> 
> BR,
> Jani.
> 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c3bd664ea733..945dd82e2ea3 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -74,10 +74,8 @@ 
  * only supports devices with a single interrupt on the main device stored in
  * &drm_device.dev and set as the device paramter in drm_dev_alloc().
  *
- * These IRQ helpers are strictly optional. Drivers which roll their own only
- * need to set &drm_device.irq_enabled to signal the DRM core that vblank
- * interrupts are working. Since these helpers don't automatically clean up the
- * requested interrupt like e.g. devm_request_irq() they're not really
+ * These IRQ helpers are strictly optional. Since these helpers don't automatically
+ * clean up the requested interrupt like e.g. devm_request_irq() they're not really
  * recommended.
  */
 
@@ -91,9 +89,7 @@ 
  * and after the installation.
  *
  * This is the simplified helper interface provided for drivers with no special
- * needs. Drivers which need to install interrupt handlers for multiple
- * interrupts must instead set &drm_device.irq_enabled to signal the DRM core
- * that vblank interrupts are available.
+ * needs.
  *
  * @irq must match the interrupt number that would be passed to request_irq(),
  * if called directly instead of using this helper function.
@@ -156,8 +152,7 @@  EXPORT_SYMBOL(drm_irq_install);
  *
  * Calls the driver's &drm_driver.irq_uninstall function and unregisters the IRQ
  * handler.  This should only be called by drivers which used drm_irq_install()
- * to set up their interrupt handler. Other drivers must only reset
- * &drm_device.irq_enabled to false.
+ * to set up their interrupt handler.
  *
  * Note that for kernel modesetting drivers it is a bug if this function fails.
  * The sanity checks are only to catch buggy user modesetting drivers which call
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 3417e1ac7918..10fe16bafcb6 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1748,8 +1748,16 @@  int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 	unsigned int pipe_index;
 	unsigned int flags, pipe, high_pipe;
 
-	if (!dev->irq_enabled)
-		return -EOPNOTSUPP;
+#if defined(CONFIG_DRM_LEGACY)
+	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
+		if (!dev->irq_enabled)
+			return -EOPNOTSUPP;
+	} else /* if DRIVER_MODESET */
+#endif
+	{
+		if (!drm_dev_has_vblank(dev))
+			return -EOPNOTSUPP;
+	}
 
 	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
 		return -EINVAL;
@@ -2023,7 +2031,7 @@  int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
-	if (!dev->irq_enabled)
+	if (!drm_dev_has_vblank(dev))
 		return -EOPNOTSUPP;
 
 	crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id);
@@ -2082,7 +2090,7 @@  int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
-	if (!dev->irq_enabled)
+	if (!drm_dev_has_vblank(dev))
 		return -EOPNOTSUPP;
 
 	crtc = drm_crtc_find(dev, file_priv, queue_seq->crtc_id);