Re: [PATCH v3 11/12] drm: Set property to return invalid for unsupported arguments for bitmask property

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Feb 06, 2014 at 08:34:52PM +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> DRM will not propagate the set_property call for bitmask drm
> properties if they are not supported by underlying driver.
> 
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> Tested-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 4f5e408..4c92741 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3420,6 +3420,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  	struct drm_mode_object *arg_obj;
>  	struct drm_mode_object *prop_obj;
>  	struct drm_property *property;
> +	struct drm_property_enum *prop_enum;
> +	bool supported_val = false;
>  	int ret = -EINVAL;
>  	int i;
>  
> @@ -3451,6 +3453,22 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  	}
>  	property = obj_to_property(prop_obj);
>  
> +	if (property->flags | DRM_MODE_PROP_BITMASK) {
> +		if (!list_empty(&property->enum_blob_list)) {
> +			list_for_each_entry(prop_enum,
> +					&property->enum_blob_list, head) {
> +				if (BIT(prop_enum->value) == arg->value) {
> +					supported_val = true;
> +					break;
> +				}
> +			}
> +		}
> +		if (!supported_val) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +	}
> +
>  	if (!drm_property_change_is_valid(property, arg->value))

drm_property_change_is_valid() should take care of this, except I think
my original patch to pass the supported_bits information is a bit buggy.
It will allocate the values[] array based on all possible rotation bits,
but then it will populate it sparsely based on the supported_bits.

Apparently that needs to be changed to allocate only the required amount
of space in values[]. I think that should make
drm_property_change_is_valid() correctly detect the valid bits.

Actually I think even my buggy patch should mostly work, it'll just
always accept DRM_ROTATE_0 even if that wasnt't one of the
supported_bits. That's because since values[] is zeroed initially.

Oh, I see your patch does actually a bit more than that. It would reject
any bitmask property change with multiple bits set. That's not what we
want, as the driver is the only one that can decide which combination(s)
of bits are in fact valid.

-- 
Ville Syrjälä
Intel OTC