[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

✓ Fi.CI.BAT: success for series starting with [1/6] drm: Reject replacing property enum values

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm: Reject replacing property enum values
URL   : https://patchwork.freedesktop.org/series/39465/
State : success

== Summary ==

Series 39465v1 series starting with [1/6] drm: Reject replacing property enum values
https://patchwork.freedesktop.org/api/1.0/series/39465/revisions/1/mbox/

---- Known issues:

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713
Test prime_vgem:
        Subgroup basic-fence-flip:
                fail       -> PASS       (fi-ilk-650) fdo#104008

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:423s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:425s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:373s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:512s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:281s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:491s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:495s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:483s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:470s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:405s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:572s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:590s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:410s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:292s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:517s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:402s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:413s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:468s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:417s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:468s
fi-kbl-7560u     total:108  pass:104  dwarn:0   dfail:0   fail:0   skip:3  
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:463s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:507s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:589s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:437s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:519s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:534s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:496s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:486s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:424s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:522s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:396s

dc9f7f3e78b982ee999e018cd51c2df4240515cf drm-tip: 2018y-03m-06d-16h-12m-59s UTC integration manifest
d1dea82c33b7 drm: Reject bad property flag combinations
b774a71d30fe drm: Make property flags u32
0e0109bc93e3 drm/uapi: Deprecate DRM_MODE_PROP_PENDING
ff5973f3a330 drm: WARN when trying to add enum value > 63 to a bitmask property
f8f70aa3a389 drm: WARN when trying add enum values to non-enum/bitmask properties
35933b28d4af drm: Reject replacing property enum values

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8247/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

✗ Fi.CI.IGT: warning for series starting with [1/6] drm: Reject replacing property enum values

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm: Reject replacing property enum values
URL   : https://patchwork.freedesktop.org/series/39465/
State : warning

== Summary ==

---- Possible new issues:

Test kms_chv_cursor_fail:
        Subgroup pipe-a-64x64-top-edge:
                dmesg-warn -> PASS       (shard-hsw)
        Subgroup pipe-c-64x64-right-edge:
                pass       -> DMESG-WARN (shard-apl)

---- Known issues:

Test gem_eio:
        Subgroup in-flight-contexts:
                pass       -> INCOMPLETE (shard-apl) fdo#105341
Test kms_atomic_transition:
        Subgroup 1x-modeset-transitions:
                pass       -> FAIL       (shard-apl) fdo#103207
Test kms_chv_cursor_fail:
        Subgroup pipe-b-256x256-right-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185
Test kms_cursor_legacy:
        Subgroup 2x-long-flip-vs-cursor-atomic:
                fail       -> PASS       (shard-hsw) fdo#104873
        Subgroup flip-vs-cursor-toggle:
                fail       -> PASS       (shard-hsw) fdo#102670
Test kms_flip:
        Subgroup plain-flip-ts-check:
                fail       -> PASS       (shard-hsw) fdo#100368 +2
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-spr-indfb-fullscreen:
                fail       -> PASS       (shard-apl) fdo#101623 +1
Test pm_lpsp:
        Subgroup screens-disabled:
                pass       -> FAIL       (shard-hsw) fdo#104941

fdo#105341 https://bugs.freedesktop.org/show_bug.cgi?id=105341
fdo#103207 https://bugs.freedesktop.org/show_bug.cgi?id=103207
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#104941 https://bugs.freedesktop.org/show_bug.cgi?id=104941

shard-apl        total:3381 pass:1779 dwarn:2   dfail:0   fail:7   skip:1591 time:11922s
shard-hsw        total:3467 pass:1771 dwarn:1   dfail:0   fail:3   skip:1691 time:11848s
shard-snb        total:3467 pass:1363 dwarn:1   dfail:0   fail:2   skip:2101 time:6972s
Blacklisted hosts:
shard-kbl        total:3394 pass:1908 dwarn:3   dfail:0   fail:8   skip:1474 time:9081s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8247/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.IGT: warning for series starting with [1/6] drm: Reject replacing property enum values

[Ville Syrjälä]

On Tue, Mar 06, 2018 at 09:49:29PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/6] drm: Reject replacing property enum values
> URL   : https://patchwork.freedesktop.org/series/39465/
> State : warning
> 
> == Summary ==
> 
> ---- Possible new issues:
> 
> Test kms_chv_cursor_fail:
>         Subgroup pipe-a-64x64-top-edge:
>                 dmesg-warn -> PASS       (shard-hsw)
>         Subgroup pipe-c-64x64-right-edge:
>                 pass       -> DMESG-WARN (shard-apl)

Unrelated pipe crc vs. vblank warn:

[   39.323847] WARNING: CPU: 1 PID: 0 at drivers/gpu/drm/drm_vblank.c:620 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0

> 
> ---- Known issues:
> 
> Test gem_eio:
>         Subgroup in-flight-contexts:
>                 pass       -> INCOMPLETE (shard-apl) fdo#105341
> Test kms_atomic_transition:
>         Subgroup 1x-modeset-transitions:
>                 pass       -> FAIL       (shard-apl) fdo#103207
> Test kms_chv_cursor_fail:
>         Subgroup pipe-b-256x256-right-edge:
>                 dmesg-warn -> PASS       (shard-snb) fdo#105185
> Test kms_cursor_legacy:
>         Subgroup 2x-long-flip-vs-cursor-atomic:
>                 fail       -> PASS       (shard-hsw) fdo#104873
>         Subgroup flip-vs-cursor-toggle:
>                 fail       -> PASS       (shard-hsw) fdo#102670
> Test kms_flip:
>         Subgroup plain-flip-ts-check:
>                 fail       -> PASS       (shard-hsw) fdo#100368 +2
> Test kms_frontbuffer_tracking:
>         Subgroup fbc-1p-primscrn-spr-indfb-fullscreen:
>                 fail       -> PASS       (shard-apl) fdo#101623 +1
> Test pm_lpsp:
>         Subgroup screens-disabled:
>                 pass       -> FAIL       (shard-hsw) fdo#104941
> 
> fdo#105341 https://bugs.freedesktop.org/show_bug.cgi?id=105341
> fdo#103207 https://bugs.freedesktop.org/show_bug.cgi?id=103207
> fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
> fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
> fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
> fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
> fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
> fdo#104941 https://bugs.freedesktop.org/show_bug.cgi?id=104941
> 
> shard-apl        total:3381 pass:1779 dwarn:2   dfail:0   fail:7   skip:1591 time:11922s
> shard-hsw        total:3467 pass:1771 dwarn:1   dfail:0   fail:3   skip:1691 time:11848s
> shard-snb        total:3467 pass:1363 dwarn:1   dfail:0   fail:2   skip:2101 time:6972s
> Blacklisted hosts:
> shard-kbl        total:3394 pass:1908 dwarn:3   dfail:0   fail:8   skip:1474 time:9081s
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8247/shards.html

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
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