[PATCH 1/6] drm: Reject replacing property enum values

[PATCH 1/6] drm: Reject replacing property enum values

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If the property already has the enum value WARN and bail.
Replacing enum values doesn't make sense to me.

Throw out the pointless list_empty() while at it.

Cc: Daniel Vetter <daniel@ffwll.ch>
Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_property.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index c37ac41125b5..d77b0c4dc485 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -390,14 +390,9 @@ int drm_property_add_enum(struct drm_property *property, int index,
 			(value > 63))
 		return -EINVAL;
 
-	if (!list_empty(&property->enum_list)) {
-		list_for_each_entry(prop_enum, &property->enum_list, head) {
-			if (prop_enum->value == value) {
-				strncpy(prop_enum->name, name, DRM_PROP_NAME_LEN);
-				prop_enum->name[DRM_PROP_NAME_LEN-1] = '\0';
-				return 0;
-			}
-		}
+	list_for_each_entry(prop_enum, &property->enum_list, head) {
+		if (WARN_ON(prop_enum->value == value))
+			return -EINVAL;
 	}
 
 	prop_enum = kzalloc(sizeof(struct drm_property_enum), GFP_KERNEL);
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/6] drm: WARN when trying add enum values to non-enum/bitmask properties

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Trying to add enum values to non-enum/bitmask properties is a
programmer mistake. WARN to make sure the developers notice
their mistake.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_property.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index d77b0c4dc485..e676b1ecc705 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -378,8 +378,8 @@ int drm_property_add_enum(struct drm_property *property, int index,
 	if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
 		return -EINVAL;
 
-	if (!(drm_property_type_is(property, DRM_MODE_PROP_ENUM) ||
-			drm_property_type_is(property, DRM_MODE_PROP_BITMASK)))
+	if (WARN_ON(!drm_property_type_is(property, DRM_MODE_PROP_ENUM) &&
+		    !drm_property_type_is(property, DRM_MODE_PROP_BITMASK)))
 		return -EINVAL;
 
 	/*
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/6] drm: WARN when trying to add enum value > 63 to a bitmask property

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Enum values >63 with a bitmask property is a programmer error. WARN
when someone is attempting this.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_property.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index e676b1ecc705..019d1abb94ca 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -386,8 +386,8 @@ int drm_property_add_enum(struct drm_property *property, int index,
 	 * Bitmask enum properties have the additional constraint of values
 	 * from 0 to 63
 	 */
-	if (drm_property_type_is(property, DRM_MODE_PROP_BITMASK) &&
-			(value > 63))
+	if (WARN_ON(drm_property_type_is(property, DRM_MODE_PROP_BITMASK) &&
+		    value > 63))
 		return -EINVAL;
 
 	list_for_each_entry(prop_enum, &property->enum_list, head) {
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/6] drm/uapi: Deprecate DRM_MODE_PROP_PENDING

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

DRM_MODE_PROP_PENDING is not used anywhere (except printed out
by libdrm proptest/modetest).

This seems to be yet another thing blindly copied from xrandr.
Quoting from the protocol spec:
"If 'pending' is TRUE, changes made to property values with
 RRChangeOutputProperty will be saved in the pending property value
 and be automatically copied to the current value on the next
 RRSetCrtcConfig request involving the named output. If 'pending' is
 FALSE, changes are copied immediately."

So it was some kind of early idea for atomic property updates.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/uapi/drm/drm_mode.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index b5d7d9e0eff5..50bcf4214ff9 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -363,7 +363,7 @@ struct drm_mode_get_connector {
 	__u32 pad;
 };
 
-#define DRM_MODE_PROP_PENDING	(1<<0)
+#define DRM_MODE_PROP_PENDING	(1<<0) /* deprecated, do not use */
 #define DRM_MODE_PROP_RANGE	(1<<1)
 #define DRM_MODE_PROP_IMMUTABLE	(1<<2)
 #define DRM_MODE_PROP_ENUM	(1<<3) /* enumerated type with text strings */
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/6] drm: Make property flags u32

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The property flags are part of the uabi and we have 32 bits for them.
Pass them around as u32 internally as well, instead of a signed int.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_property.c | 41 +++++++++++++++++++++--------------------
 include/drm/drm_property.h     | 24 +++++++++++++-----------
 2 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index 019d1abb94ca..027a50e55e96 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -72,8 +72,9 @@ static bool drm_property_type_valid(struct drm_property *property)
  * Returns:
  * A pointer to the newly created property on success, NULL on failure.
  */
-struct drm_property *drm_property_create(struct drm_device *dev, int flags,
-					 const char *name, int num_values)
+struct drm_property *drm_property_create(struct drm_device *dev,
+					 u32 flags, const char *name,
+					 int num_values)
 {
 	struct drm_property *property = NULL;
 	int ret;
@@ -136,10 +137,10 @@ EXPORT_SYMBOL(drm_property_create);
  * Returns:
  * A pointer to the newly created property on success, NULL on failure.
  */
-struct drm_property *drm_property_create_enum(struct drm_device *dev, int flags,
-					 const char *name,
-					 const struct drm_prop_enum_list *props,
-					 int num_values)
+struct drm_property *drm_property_create_enum(struct drm_device *dev,
+					      u32 flags, const char *name,
+					      const struct drm_prop_enum_list *props,
+					      int num_values)
 {
 	struct drm_property *property;
 	int i, ret;
@@ -185,10 +186,10 @@ EXPORT_SYMBOL(drm_property_create_enum);
  * A pointer to the newly created property on success, NULL on failure.
  */
 struct drm_property *drm_property_create_bitmask(struct drm_device *dev,
-					 int flags, const char *name,
-					 const struct drm_prop_enum_list *props,
-					 int num_props,
-					 uint64_t supported_bits)
+						 u32 flags, const char *name,
+						 const struct drm_prop_enum_list *props,
+						 int num_props,
+						 uint64_t supported_bits)
 {
 	struct drm_property *property;
 	int i, ret, index = 0;
@@ -222,8 +223,8 @@ struct drm_property *drm_property_create_bitmask(struct drm_device *dev,
 EXPORT_SYMBOL(drm_property_create_bitmask);
 
 static struct drm_property *property_create_range(struct drm_device *dev,
-					 int flags, const char *name,
-					 uint64_t min, uint64_t max)
+						  u32 flags, const char *name,
+						  uint64_t min, uint64_t max)
 {
 	struct drm_property *property;
 
@@ -256,9 +257,9 @@ static struct drm_property *property_create_range(struct drm_device *dev,
  * Returns:
  * A pointer to the newly created property on success, NULL on failure.
  */
-struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
-					 const char *name,
-					 uint64_t min, uint64_t max)
+struct drm_property *drm_property_create_range(struct drm_device *dev,
+					       u32 flags, const char *name,
+					       uint64_t min, uint64_t max)
 {
 	return property_create_range(dev, DRM_MODE_PROP_RANGE | flags,
 			name, min, max);
@@ -285,8 +286,8 @@ EXPORT_SYMBOL(drm_property_create_range);
  * A pointer to the newly created property on success, NULL on failure.
  */
 struct drm_property *drm_property_create_signed_range(struct drm_device *dev,
-					 int flags, const char *name,
-					 int64_t min, int64_t max)
+						      u32 flags, const char *name,
+						      int64_t min, int64_t max)
 {
 	return property_create_range(dev, DRM_MODE_PROP_SIGNED_RANGE | flags,
 			name, I642U64(min), I642U64(max));
@@ -312,7 +313,7 @@ EXPORT_SYMBOL(drm_property_create_signed_range);
  * A pointer to the newly created property on success, NULL on failure.
  */
 struct drm_property *drm_property_create_object(struct drm_device *dev,
-						int flags, const char *name,
+						u32 flags, const char *name,
 						uint32_t type)
 {
 	struct drm_property *property;
@@ -348,8 +349,8 @@ EXPORT_SYMBOL(drm_property_create_object);
  * Returns:
  * A pointer to the newly created property on success, NULL on failure.
  */
-struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags,
-					      const char *name)
+struct drm_property *drm_property_create_bool(struct drm_device *dev,
+					      u32 flags, const char *name)
 {
 	return drm_property_create_range(dev, flags, name, 0, 1);
 }
diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
index 8a522b4bed40..937757a8a568 100644
--- a/include/drm/drm_property.h
+++ b/include/drm/drm_property.h
@@ -237,27 +237,29 @@ static inline bool drm_property_type_is(struct drm_property *property,
 	return property->flags & type;
 }
 
-struct drm_property *drm_property_create(struct drm_device *dev, int flags,
-					 const char *name, int num_values);
-struct drm_property *drm_property_create_enum(struct drm_device *dev, int flags,
-					      const char *name,
+struct drm_property *drm_property_create(struct drm_device *dev,
+					 u32 flags, const char *name,
+					 int num_values);
+struct drm_property *drm_property_create_enum(struct drm_device *dev,
+					      u32 flags, const char *name,
 					      const struct drm_prop_enum_list *props,
 					      int num_values);
 struct drm_property *drm_property_create_bitmask(struct drm_device *dev,
-						 int flags, const char *name,
+						 u32 flags, const char *name,
 						 const struct drm_prop_enum_list *props,
 						 int num_props,
 						 uint64_t supported_bits);
-struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
-					       const char *name,
+struct drm_property *drm_property_create_range(struct drm_device *dev,
+					       u32 flags, const char *name,
 					       uint64_t min, uint64_t max);
 struct drm_property *drm_property_create_signed_range(struct drm_device *dev,
-						      int flags, const char *name,
+						      u32 flags, const char *name,
 						      int64_t min, int64_t max);
 struct drm_property *drm_property_create_object(struct drm_device *dev,
-						int flags, const char *name, uint32_t type);
-struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags,
-					      const char *name);
+						u32 flags, const char *name,
+						uint32_t type);
+struct drm_property *drm_property_create_bool(struct drm_device *dev,
+					      u32 flags, const char *name);
 int drm_property_add_enum(struct drm_property *property, int index,
 			  uint64_t value, const char *name);
 void drm_property_destroy(struct drm_device *dev, struct drm_property *property);
-- 
2.16.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] drm: Reject bad property flag combinations

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pimp drm_property_type_valid() to check for more fails with the
property flags. Also make the check before adding the property,
and bail out if things look bad.

Since we're now chekcing for more than the type let's also
change the function name to drm_property_flags_valid().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_property.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index 027a50e55e96..6ac6ee41a6a3 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -50,11 +50,27 @@
  * IOCTL and in the get/set property IOCTL.
  */
 
-static bool drm_property_type_valid(struct drm_property *property)
+static bool drm_property_flags_valid(u32 flags)
 {
-	if (property->flags & DRM_MODE_PROP_EXTENDED_TYPE)
-		return !(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
-	return !!(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
+	u32 legacy_type = flags & DRM_MODE_PROP_LEGACY_TYPE;
+	u32 ext_type = flags & DRM_MODE_PROP_EXTENDED_TYPE;
+
+	/* Reject undefined/deprecated flags */
+	if (flags & ~(DRM_MODE_PROP_LEGACY_TYPE |
+		      DRM_MODE_PROP_EXTENDED_TYPE |
+		      DRM_MODE_PROP_IMMUTABLE |
+		      DRM_MODE_PROP_ATOMIC))
+		return false;
+
+	/* We want either a legacy type or an extended type, but not both */
+	if (!legacy_type == !ext_type)
+		return false;
+
+	/* Only one legacy type at a time please */
+	if (legacy_type && !is_power_of_2(legacy_type))
+		return false;
+
+	return true;
 }
 
 /**
@@ -79,6 +95,9 @@ struct drm_property *drm_property_create(struct drm_device *dev,
 	struct drm_property *property = NULL;
 	int ret;
 
+	if (WARN_ON(!drm_property_flags_valid(flags)))
+		return NULL;
+
 	if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
 		return NULL;
 
@@ -108,8 +127,6 @@ struct drm_property *drm_property_create(struct drm_device *dev,
 
 	list_add_tail(&property->head, &dev->mode_config.property_list);
 
-	WARN_ON(!drm_property_type_valid(property));
-
 	return property;
 fail:
 	kfree(property->values);
-- 
2.16.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm: Reject replacing property enum values

[Daniel Vetter]

On Tue, Mar 06, 2018 at 06:48:44PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If the property already has the enum value WARN and bail.
> Replacing enum values doesn't make sense to me.
> 
> Throw out the pointless list_empty() while at it.
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_property.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index c37ac41125b5..d77b0c4dc485 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -390,14 +390,9 @@ int drm_property_add_enum(struct drm_property *property, int index,
>  			(value > 63))
>  		return -EINVAL;
>  
> -	if (!list_empty(&property->enum_list)) {
> -		list_for_each_entry(prop_enum, &property->enum_list, head) {
> -			if (prop_enum->value == value) {
> -				strncpy(prop_enum->name, name, DRM_PROP_NAME_LEN);
> -				prop_enum->name[DRM_PROP_NAME_LEN-1] = '\0';
> -				return 0;
> -			}
> -		}
> +	list_for_each_entry(prop_enum, &property->enum_list, head) {
> +		if (WARN_ON(prop_enum->value == value))
> +			return -EINVAL;
>  	}
>  
>  	prop_enum = kzalloc(sizeof(struct drm_property_enum), GFP_KERNEL);
> -- 
> 2.16.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] drm: WARN when trying add enum values to non-enum/bitmask properties

[Daniel Vetter]

On Tue, Mar 06, 2018 at 06:48:45PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Trying to add enum values to non-enum/bitmask properties is a
> programmer mistake. WARN to make sure the developers notice
> their mistake.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_property.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index d77b0c4dc485..e676b1ecc705 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -378,8 +378,8 @@ int drm_property_add_enum(struct drm_property *property, int index,
>  	if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
>  		return -EINVAL;
>  
> -	if (!(drm_property_type_is(property, DRM_MODE_PROP_ENUM) ||
> -			drm_property_type_is(property, DRM_MODE_PROP_BITMASK)))
> +	if (WARN_ON(!drm_property_type_is(property, DRM_MODE_PROP_ENUM) &&
> +		    !drm_property_type_is(property, DRM_MODE_PROP_BITMASK)))
>  		return -EINVAL;
>  
>  	/*
> -- 
> 2.16.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: WARN when trying to add enum value > 63 to a bitmask property

[Daniel Vetter]

On Tue, Mar 06, 2018 at 06:48:46PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Enum values >63 with a bitmask property is a programmer error. WARN
> when someone is attempting this.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_property.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index e676b1ecc705..019d1abb94ca 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -386,8 +386,8 @@ int drm_property_add_enum(struct drm_property *property, int index,
>  	 * Bitmask enum properties have the additional constraint of values
>  	 * from 0 to 63
>  	 */
> -	if (drm_property_type_is(property, DRM_MODE_PROP_BITMASK) &&
> -			(value > 63))
> +	if (WARN_ON(drm_property_type_is(property, DRM_MODE_PROP_BITMASK) &&
> +		    value > 63))
>  		return -EINVAL;
>  
>  	list_for_each_entry(prop_enum, &property->enum_list, head) {
> -- 
> 2.16.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/uapi: Deprecate DRM_MODE_PROP_PENDING

[Daniel Vetter]

On Tue, Mar 06, 2018 at 06:48:47PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DRM_MODE_PROP_PENDING is not used anywhere (except printed out
> by libdrm proptest/modetest).
> 
> This seems to be yet another thing blindly copied from xrandr.
> Quoting from the protocol spec:
> "If 'pending' is TRUE, changes made to property values with
>  RRChangeOutputProperty will be saved in the pending property value
>  and be automatically copied to the current value on the next
>  RRSetCrtcConfig request involving the named output. If 'pending' is
>  FALSE, changes are copied immediately."
> 
> So it was some kind of early idea for atomic property updates.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  include/uapi/drm/drm_mode.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index b5d7d9e0eff5..50bcf4214ff9 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -363,7 +363,7 @@ struct drm_mode_get_connector {
>  	__u32 pad;
>  };
>  
> -#define DRM_MODE_PROP_PENDING	(1<<0)
> +#define DRM_MODE_PROP_PENDING	(1<<0) /* deprecated, do not use */
>  #define DRM_MODE_PROP_RANGE	(1<<1)
>  #define DRM_MODE_PROP_IMMUTABLE	(1<<2)
>  #define DRM_MODE_PROP_ENUM	(1<<3) /* enumerated type with text strings */
> -- 
> 2.16.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/6] drm: Make property flags u32

[Daniel Vetter]

On Tue, Mar 06, 2018 at 06:48:48PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The property flags are part of the uabi and we have 32 bits for them.
> Pass them around as u32 internally as well, instead of a signed int.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

u32 vs uint32_t (in struct drm_property) ftw, but who cares.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_property.c | 41 +++++++++++++++++++++--------------------
>  include/drm/drm_property.h     | 24 +++++++++++++-----------
>  2 files changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index 019d1abb94ca..027a50e55e96 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -72,8 +72,9 @@ static bool drm_property_type_valid(struct drm_property *property)
>   * Returns:
>   * A pointer to the newly created property on success, NULL on failure.
>   */
> -struct drm_property *drm_property_create(struct drm_device *dev, int flags,
> -					 const char *name, int num_values)
> +struct drm_property *drm_property_create(struct drm_device *dev,
> +					 u32 flags, const char *name,
> +					 int num_values)
>  {
>  	struct drm_property *property = NULL;
>  	int ret;
> @@ -136,10 +137,10 @@ EXPORT_SYMBOL(drm_property_create);
>   * Returns:
>   * A pointer to the newly created property on success, NULL on failure.
>   */
> -struct drm_property *drm_property_create_enum(struct drm_device *dev, int flags,
> -					 const char *name,
> -					 const struct drm_prop_enum_list *props,
> -					 int num_values)
> +struct drm_property *drm_property_create_enum(struct drm_device *dev,
> +					      u32 flags, const char *name,
> +					      const struct drm_prop_enum_list *props,
> +					      int num_values)
>  {
>  	struct drm_property *property;
>  	int i, ret;
> @@ -185,10 +186,10 @@ EXPORT_SYMBOL(drm_property_create_enum);
>   * A pointer to the newly created property on success, NULL on failure.
>   */
>  struct drm_property *drm_property_create_bitmask(struct drm_device *dev,
> -					 int flags, const char *name,
> -					 const struct drm_prop_enum_list *props,
> -					 int num_props,
> -					 uint64_t supported_bits)
> +						 u32 flags, const char *name,
> +						 const struct drm_prop_enum_list *props,
> +						 int num_props,
> +						 uint64_t supported_bits)
>  {
>  	struct drm_property *property;
>  	int i, ret, index = 0;
> @@ -222,8 +223,8 @@ struct drm_property *drm_property_create_bitmask(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_property_create_bitmask);
>  
>  static struct drm_property *property_create_range(struct drm_device *dev,
> -					 int flags, const char *name,
> -					 uint64_t min, uint64_t max)
> +						  u32 flags, const char *name,
> +						  uint64_t min, uint64_t max)
>  {
>  	struct drm_property *property;
>  
> @@ -256,9 +257,9 @@ static struct drm_property *property_create_range(struct drm_device *dev,
>   * Returns:
>   * A pointer to the newly created property on success, NULL on failure.
>   */
> -struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
> -					 const char *name,
> -					 uint64_t min, uint64_t max)
> +struct drm_property *drm_property_create_range(struct drm_device *dev,
> +					       u32 flags, const char *name,
> +					       uint64_t min, uint64_t max)
>  {
>  	return property_create_range(dev, DRM_MODE_PROP_RANGE | flags,
>  			name, min, max);
> @@ -285,8 +286,8 @@ EXPORT_SYMBOL(drm_property_create_range);
>   * A pointer to the newly created property on success, NULL on failure.
>   */
>  struct drm_property *drm_property_create_signed_range(struct drm_device *dev,
> -					 int flags, const char *name,
> -					 int64_t min, int64_t max)
> +						      u32 flags, const char *name,
> +						      int64_t min, int64_t max)
>  {
>  	return property_create_range(dev, DRM_MODE_PROP_SIGNED_RANGE | flags,
>  			name, I642U64(min), I642U64(max));
> @@ -312,7 +313,7 @@ EXPORT_SYMBOL(drm_property_create_signed_range);
>   * A pointer to the newly created property on success, NULL on failure.
>   */
>  struct drm_property *drm_property_create_object(struct drm_device *dev,
> -						int flags, const char *name,
> +						u32 flags, const char *name,
>  						uint32_t type)
>  {
>  	struct drm_property *property;
> @@ -348,8 +349,8 @@ EXPORT_SYMBOL(drm_property_create_object);
>   * Returns:
>   * A pointer to the newly created property on success, NULL on failure.
>   */
> -struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags,
> -					      const char *name)
> +struct drm_property *drm_property_create_bool(struct drm_device *dev,
> +					      u32 flags, const char *name)
>  {
>  	return drm_property_create_range(dev, flags, name, 0, 1);
>  }
> diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
> index 8a522b4bed40..937757a8a568 100644
> --- a/include/drm/drm_property.h
> +++ b/include/drm/drm_property.h
> @@ -237,27 +237,29 @@ static inline bool drm_property_type_is(struct drm_property *property,
>  	return property->flags & type;
>  }
>  
> -struct drm_property *drm_property_create(struct drm_device *dev, int flags,
> -					 const char *name, int num_values);
> -struct drm_property *drm_property_create_enum(struct drm_device *dev, int flags,
> -					      const char *name,
> +struct drm_property *drm_property_create(struct drm_device *dev,
> +					 u32 flags, const char *name,
> +					 int num_values);
> +struct drm_property *drm_property_create_enum(struct drm_device *dev,
> +					      u32 flags, const char *name,
>  					      const struct drm_prop_enum_list *props,
>  					      int num_values);
>  struct drm_property *drm_property_create_bitmask(struct drm_device *dev,
> -						 int flags, const char *name,
> +						 u32 flags, const char *name,
>  						 const struct drm_prop_enum_list *props,
>  						 int num_props,
>  						 uint64_t supported_bits);
> -struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
> -					       const char *name,
> +struct drm_property *drm_property_create_range(struct drm_device *dev,
> +					       u32 flags, const char *name,
>  					       uint64_t min, uint64_t max);
>  struct drm_property *drm_property_create_signed_range(struct drm_device *dev,
> -						      int flags, const char *name,
> +						      u32 flags, const char *name,
>  						      int64_t min, int64_t max);
>  struct drm_property *drm_property_create_object(struct drm_device *dev,
> -						int flags, const char *name, uint32_t type);
> -struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags,
> -					      const char *name);
> +						u32 flags, const char *name,
> +						uint32_t type);
> +struct drm_property *drm_property_create_bool(struct drm_device *dev,
> +					      u32 flags, const char *name);
>  int drm_property_add_enum(struct drm_property *property, int index,
>  			  uint64_t value, const char *name);
>  void drm_property_destroy(struct drm_device *dev, struct drm_property *property);
> -- 
> 2.16.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm: Reject bad property flag combinations

[Daniel Vetter]

On Tue, Mar 06, 2018 at 06:48:49PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Pimp drm_property_type_valid() to check for more fails with the
> property flags. Also make the check before adding the property,
> and bail out if things look bad.
> 
> Since we're now chekcing for more than the type let's also
> change the function name to drm_property_flags_valid().
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_property.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index 027a50e55e96..6ac6ee41a6a3 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -50,11 +50,27 @@
>   * IOCTL and in the get/set property IOCTL.
>   */
>  
> -static bool drm_property_type_valid(struct drm_property *property)
> +static bool drm_property_flags_valid(u32 flags)
>  {
> -	if (property->flags & DRM_MODE_PROP_EXTENDED_TYPE)
> -		return !(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
> -	return !!(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
> +	u32 legacy_type = flags & DRM_MODE_PROP_LEGACY_TYPE;
> +	u32 ext_type = flags & DRM_MODE_PROP_EXTENDED_TYPE;
> +
> +	/* Reject undefined/deprecated flags */
> +	if (flags & ~(DRM_MODE_PROP_LEGACY_TYPE |
> +		      DRM_MODE_PROP_EXTENDED_TYPE |
> +		      DRM_MODE_PROP_IMMUTABLE |
> +		      DRM_MODE_PROP_ATOMIC))
> +		return false;
> +
> +	/* We want either a legacy type or an extended type, but not both */
> +	if (!legacy_type == !ext_type)
> +		return false;
> +
> +	/* Only one legacy type at a time please */
> +	if (legacy_type && !is_power_of_2(legacy_type))
> +		return false;
> +
> +	return true;
>  }

I think this catches everything. Well except not-yet-assigned
EXTENDED_TYPE defines, but really we can overdo this :-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>  
>  /**
> @@ -79,6 +95,9 @@ struct drm_property *drm_property_create(struct drm_device *dev,
>  	struct drm_property *property = NULL;
>  	int ret;
>  
> +	if (WARN_ON(!drm_property_flags_valid(flags)))
> +		return NULL;
> +
>  	if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
>  		return NULL;
>  
> @@ -108,8 +127,6 @@ struct drm_property *drm_property_create(struct drm_device *dev,
>  
>  	list_add_tail(&property->head, &dev->mode_config.property_list);
>  
> -	WARN_ON(!drm_property_type_valid(property));
> -
>  	return property;
>  fail:
>  	kfree(property->values);
> -- 
> 2.16.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm: Reject bad property flag combinations

[Ville Syrjälä]

On Tue, Mar 06, 2018 at 07:22:51PM +0100, Daniel Vetter wrote:
> On Tue, Mar 06, 2018 at 06:48:49PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Pimp drm_property_type_valid() to check for more fails with the
> > property flags. Also make the check before adding the property,
> > and bail out if things look bad.
> > 
> > Since we're now chekcing for more than the type let's also
> > change the function name to drm_property_flags_valid().
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_property.c | 29 +++++++++++++++++++++++------
> >  1 file changed, 23 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> > index 027a50e55e96..6ac6ee41a6a3 100644
> > --- a/drivers/gpu/drm/drm_property.c
> > +++ b/drivers/gpu/drm/drm_property.c
> > @@ -50,11 +50,27 @@
> >   * IOCTL and in the get/set property IOCTL.
> >   */
> >  
> > -static bool drm_property_type_valid(struct drm_property *property)
> > +static bool drm_property_flags_valid(u32 flags)
> >  {
> > -	if (property->flags & DRM_MODE_PROP_EXTENDED_TYPE)
> > -		return !(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
> > -	return !!(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
> > +	u32 legacy_type = flags & DRM_MODE_PROP_LEGACY_TYPE;
> > +	u32 ext_type = flags & DRM_MODE_PROP_EXTENDED_TYPE;
> > +
> > +	/* Reject undefined/deprecated flags */
> > +	if (flags & ~(DRM_MODE_PROP_LEGACY_TYPE |
> > +		      DRM_MODE_PROP_EXTENDED_TYPE |
> > +		      DRM_MODE_PROP_IMMUTABLE |
> > +		      DRM_MODE_PROP_ATOMIC))
> > +		return false;
> > +
> > +	/* We want either a legacy type or an extended type, but not both */
> > +	if (!legacy_type == !ext_type)
> > +		return false;
> > +
> > +	/* Only one legacy type at a time please */
> > +	if (legacy_type && !is_power_of_2(legacy_type))
> > +		return false;
> > +
> > +	return true;
> >  }
> 
> I think this catches everything. Well except not-yet-assigned
> EXTENDED_TYPE defines, but really we can overdo this :-)

Hmm. Yeah, I guess that kind of a fail is fairly unlikely because the
defines won't be there. Would require the driver to basically pass in
utter garbage instead of just a bad combination of existing flags.

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks. Series pushed to drm-misc-next.

> >  
> >  /**
> > @@ -79,6 +95,9 @@ struct drm_property *drm_property_create(struct drm_device *dev,
> >  	struct drm_property *property = NULL;
> >  	int ret;
> >  
> > +	if (WARN_ON(!drm_property_flags_valid(flags)))
> > +		return NULL;
> > +
> >  	if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
> >  		return NULL;
> >  
> > @@ -108,8 +127,6 @@ struct drm_property *drm_property_create(struct drm_device *dev,
> >  
> >  	list_add_tail(&property->head, &dev->mode_config.property_list);
> >  
> > -	WARN_ON(!drm_property_type_valid(property));
> > -
> >  	return property;
> >  fail:
> >  	kfree(property->values);
> > -- 
> > 2.16.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel