[v2,1/2] drm: support feature masks in drm_core_check_feature()
diff mbox series

Message ID 20200122140259.12086-1-jani.nikula@intel.com
State New
Headers show
Series
  • [v2,1/2] drm: support feature masks in drm_core_check_feature()
Related show

Commit Message

Jani Nikula Jan. 22, 2020, 2:02 p.m. UTC
Allow a mask of features to be passed to drm_core_check_feature(). All
features in the mask are required.

v2:
- fix kernel-doc (Ville)
- add an extra variable for clarity (Ville)

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 include/drm/drm_drv.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Thomas Zimmermann Jan. 22, 2020, 2:16 p.m. UTC | #1
Hi Jani

Am 22.01.20 um 15:02 schrieb Jani Nikula:
> Allow a mask of features to be passed to drm_core_check_feature(). All
> features in the mask are required.
> 
> v2:
> - fix kernel-doc (Ville)
> - add an extra variable for clarity (Ville)
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  include/drm/drm_drv.h | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index cf13470810a5..f18e19f3f2d0 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -826,16 +826,20 @@ static inline bool drm_dev_is_unplugged(struct drm_device *dev)
>  /**
>   * drm_core_check_feature - check driver feature flags
>   * @dev: DRM device to check
> - * @feature: feature flag
> + * @features: feature flag(s)
>   *
>   * This checks @dev for driver features, see &drm_driver.driver_features,
>   * &drm_device.driver_features, and the various &enum drm_driver_feature flags.
>   *
> - * Returns true if the @feature is supported, false otherwise.
> + * Returns true if all features in the @features mask are supported, false
> + * otherwise.
>   */
> -static inline bool drm_core_check_feature(const struct drm_device *dev, u32 feature)
> +static inline bool drm_core_check_feature(const struct drm_device *dev,
> +					  u32 features)

It's misnamed now. I'd add a new function, say
drm_core_check_all_features(), which makes the purpose clear.

Best regards
Thomas

>  {
> -	return dev->driver->driver_features & dev->driver_features & feature;
> +	u32 supported = dev->driver->driver_features & dev->driver_features;
> +
> +	return features && (supported & features) == features;
>  }
>  
>  /**
>
Jani Nikula Jan. 22, 2020, 2:27 p.m. UTC | #2
On Wed, 22 Jan 2020, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi Jani
>
> Am 22.01.20 um 15:02 schrieb Jani Nikula:
>> Allow a mask of features to be passed to drm_core_check_feature(). All
>> features in the mask are required.
>> 
>> v2:
>> - fix kernel-doc (Ville)
>> - add an extra variable for clarity (Ville)
>> 
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  include/drm/drm_drv.h | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index cf13470810a5..f18e19f3f2d0 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -826,16 +826,20 @@ static inline bool drm_dev_is_unplugged(struct drm_device *dev)
>>  /**
>>   * drm_core_check_feature - check driver feature flags
>>   * @dev: DRM device to check
>> - * @feature: feature flag
>> + * @features: feature flag(s)
>>   *
>>   * This checks @dev for driver features, see &drm_driver.driver_features,
>>   * &drm_device.driver_features, and the various &enum drm_driver_feature flags.
>>   *
>> - * Returns true if the @feature is supported, false otherwise.
>> + * Returns true if all features in the @features mask are supported, false
>> + * otherwise.
>>   */
>> -static inline bool drm_core_check_feature(const struct drm_device *dev, u32 feature)
>> +static inline bool drm_core_check_feature(const struct drm_device *dev,
>> +					  u32 features)
>
> It's misnamed now. I'd add a new function, say
> drm_core_check_all_features(), which makes the purpose clear.

We don't really need another function. We need this one to check all the
features. But I'd rather not do the mass rename of all call sites for no
real benefit.

BR,
Jani.


>
> Best regards
> Thomas
>
>>  {
>> -	return dev->driver->driver_features & dev->driver_features & feature;
>> +	u32 supported = dev->driver->driver_features & dev->driver_features;
>> +
>> +	return features && (supported & features) == features;
>>  }
>>  
>>  /**
>>
Thomas Zimmermann Jan. 22, 2020, 3:31 p.m. UTC | #3
Hi

Am 22.01.20 um 15:27 schrieb Jani Nikula:
> On Wed, 22 Jan 2020, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi Jani
>>
>> Am 22.01.20 um 15:02 schrieb Jani Nikula:
>>> Allow a mask of features to be passed to drm_core_check_feature(). All
>>> features in the mask are required.
>>>
>>> v2:
>>> - fix kernel-doc (Ville)
>>> - add an extra variable for clarity (Ville)
>>>
>>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>  include/drm/drm_drv.h | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>> index cf13470810a5..f18e19f3f2d0 100644
>>> --- a/include/drm/drm_drv.h
>>> +++ b/include/drm/drm_drv.h
>>> @@ -826,16 +826,20 @@ static inline bool drm_dev_is_unplugged(struct drm_device *dev)
>>>  /**
>>>   * drm_core_check_feature - check driver feature flags
>>>   * @dev: DRM device to check
>>> - * @feature: feature flag
>>> + * @features: feature flag(s)
>>>   *
>>>   * This checks @dev for driver features, see &drm_driver.driver_features,
>>>   * &drm_device.driver_features, and the various &enum drm_driver_feature flags.
>>>   *
>>> - * Returns true if the @feature is supported, false otherwise.
>>> + * Returns true if all features in the @features mask are supported, false
>>> + * otherwise.
>>>   */
>>> -static inline bool drm_core_check_feature(const struct drm_device *dev, u32 feature)
>>> +static inline bool drm_core_check_feature(const struct drm_device *dev,
>>> +					  u32 features)
>>
>> It's misnamed now. I'd add a new function, say
>> drm_core_check_all_features(), which makes the purpose clear.
> 
> We don't really need another function. We need this one to check all the
> features. But I'd rather not do the mass rename of all call sites for no
> real benefit.

My point is: does the function check for all of the given features or
any combination of them? The function's name doesn't tell you.

Best regards
Thomas

> 
> BR,
> Jani.
> 
> 
>>
>> Best regards
>> Thomas
>>
>>>  {
>>> -	return dev->driver->driver_features & dev->driver_features & feature;
>>> +	u32 supported = dev->driver->driver_features & dev->driver_features;
>>> +
>>> +	return features && (supported & features) == features;
>>>  }
>>>  
>>>  /**
>>>
>

Patch
diff mbox series

diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index cf13470810a5..f18e19f3f2d0 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -826,16 +826,20 @@  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
 /**
  * drm_core_check_feature - check driver feature flags
  * @dev: DRM device to check
- * @feature: feature flag
+ * @features: feature flag(s)
  *
  * This checks @dev for driver features, see &drm_driver.driver_features,
  * &drm_device.driver_features, and the various &enum drm_driver_feature flags.
  *
- * Returns true if the @feature is supported, false otherwise.
+ * Returns true if all features in the @features mask are supported, false
+ * otherwise.
  */
-static inline bool drm_core_check_feature(const struct drm_device *dev, u32 feature)
+static inline bool drm_core_check_feature(const struct drm_device *dev,
+					  u32 features)
 {
-	return dev->driver->driver_features & dev->driver_features & feature;
+	u32 supported = dev->driver->driver_features & dev->driver_features;
+
+	return features && (supported & features) == features;
 }
 
 /**