All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC 1/2] drm: add bitmask property type
@ 2012-03-30  1:02 Rob Clark
  2012-03-30  1:02 ` [PATCH RFC 2/2] drm: add plane properties Rob Clark
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Rob Clark @ 2012-03-30  1:02 UTC (permalink / raw)
  To: dri-devel; +Cc: Rob Clark, patches

From: Rob Clark <rob@ti.com>

A bitmask property is similar to an enum.  The enum value is a bit
position (0-63), and valid property values consist of a mask of
zero or more of (1 << enum_val[n]).

TODO: word commit msg better
TODO: maybe "flags" would be a better name for the property type?
---
See https://github.com/robclark/kernel-omap4/commit/970b7bb95993fc43b4977976bf8005dc2e1a4ad3#L6R411
for an example usage.  In this case combinations of "x-invert", "y-invert"
and "xy-flip" can express all possible combinations of rotations of
multiples of 90 degrees plus mirroring.  Which is sufficient for an
xrandr v1.2 rotation support.  For arbitrary transforms in xrandr v1.3
a different property with a transform matrix (if supported by the
driver) should be used.

Note: I've not finished updating libdrm and ddx driver to use this
yet, so other than compile and boot test, you can consider this as
untested.  But I figure that it is at least worthwhile to send as
an RFC at this point to get feedback.

 drivers/gpu/drm/drm_crtc.c |   46 +++++++++++++++++++++++++++++++++++++++++--
 include/drm/drm_crtc.h     |    3 ++
 include/drm/drm_mode.h     |    1 +
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 95c7ab2..2b462f6 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2713,6 +2713,33 @@ struct drm_property *drm_property_create_enum(struct drm_device *dev, int flags,
 }
 EXPORT_SYMBOL(drm_property_create_enum);
 
+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_values)
+{
+	struct drm_property *property;
+	int i, ret;
+
+	flags |= DRM_MODE_PROP_BITMASK;
+
+	property = drm_property_create(dev, flags, name, num_values);
+	if (!property)
+		return NULL;
+
+	for (i = 0; i < num_values; i++) {
+		ret = drm_property_add_enum(property, i,
+				      props[i].type,
+				      props[i].name);
+		if (ret) {
+			drm_property_destroy(dev, property);
+			return NULL;
+		}
+	}
+
+	return property;
+}
+EXPORT_SYMBOL(drm_property_create_bitmask);
+
 struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
 					 const char *name,
 					 uint64_t min, uint64_t max)
@@ -2737,7 +2764,14 @@ int drm_property_add_enum(struct drm_property *property, int index,
 {
 	struct drm_property_enum *prop_enum;
 
-	if (!(property->flags & DRM_MODE_PROP_ENUM))
+	if (!(property->flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)))
+		return -EINVAL;
+
+	/*
+	 * Bitmask enum properties have the additional constraint of values
+	 * from 0 to 63
+	 */
+	if ((property->flags & DRM_MODE_PROP_BITMASK) && (value > 63))
 		return -EINVAL;
 
 	if (!list_empty(&property->enum_blob_list)) {
@@ -2881,7 +2915,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
 	}
 	property = obj_to_property(obj);
 
-	if (property->flags & DRM_MODE_PROP_ENUM) {
+	if (property->flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)) {
 		list_for_each_entry(prop_enum, &property->enum_blob_list, head)
 			enum_count++;
 	} else if (property->flags & DRM_MODE_PROP_BLOB) {
@@ -2906,7 +2940,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
 	}
 	out_resp->count_values = value_count;
 
-	if (property->flags & DRM_MODE_PROP_ENUM) {
+	if (property->flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)) {
 		if ((out_resp->count_enum_blobs >= enum_count) && enum_count) {
 			copied = 0;
 			enum_ptr = (struct drm_mode_property_enum __user *)(unsigned long)out_resp->enum_blob_ptr;
@@ -3063,6 +3097,12 @@ static bool drm_property_change_is_valid(struct drm_property *property,
 		if (value > property->values[1])
 			return false;
 		return true;
+	} else if (property->flags & DRM_MODE_PROP_BITMASK) {
+		int i;
+		__u64 valid_mask = 0;
+		for (i = 0; i < property->num_values; i++)
+			valid_mask |= (1 << property->values[i]);
+		return !(value & ~valid_mask);
 	} else {
 		int i;
 		for (i = 0; i < property->num_values; i++)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 32e9c51..28e9a78 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -940,6 +940,9 @@ extern struct drm_property *drm_property_create_enum(struct drm_device *dev, int
 					 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,
+					 const struct drm_prop_enum_list *props, int num_values);
 struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
 					 const char *name,
 					 uint64_t min, uint64_t max);
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index de5de2a..3190dfe 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -228,6 +228,7 @@ struct drm_mode_get_connector {
 #define DRM_MODE_PROP_IMMUTABLE	(1<<2)
 #define DRM_MODE_PROP_ENUM	(1<<3) /* enumerated type with text strings */
 #define DRM_MODE_PROP_BLOB	(1<<4)
+#define DRM_MODE_PROP_BITMASK	(1<<5) /* bitmask of enumerated types */
 
 struct drm_mode_property_enum {
 	__u64 value;
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH RFC 2/2] drm: add plane properties
  2012-03-30  1:02 [PATCH RFC 1/2] drm: add bitmask property type Rob Clark
@ 2012-03-30  1:02 ` Rob Clark
  2012-03-30  1:15 ` [PATCH RFC 1/2] drm: add bitmask property type Rob Clark
  2012-03-30  8:44 ` Chris Wilson
  2 siblings, 0 replies; 9+ messages in thread
From: Rob Clark @ 2012-03-30  1:02 UTC (permalink / raw)
  To: dri-devel; +Cc: Rob Clark, patches

From: Rob Clark <rob@ti.com>

The omapdrm driver uses this for setting per-overlay rotation.  It
is likely also useful for setting YUV->RGB colorspace conversion
matrix, etc.
---
 drivers/gpu/drm/drm_crtc.c |   19 +++++++++++++++++++
 include/drm/drm_crtc.h     |    5 +++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 2b462f6..b158b72 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -606,6 +606,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 	if (ret)
 		goto out;
 
+	plane->base.properties = &plane->properties;
 	plane->dev = dev;
 	plane->funcs = funcs;
 	plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
@@ -3163,6 +3164,21 @@ static int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
 	return ret;
 }
 
+static int drm_mode_plane_set_obj_prop(struct drm_mode_object *obj,
+				      struct drm_property *property,
+				      uint64_t value)
+{
+	int ret = -EINVAL;
+	struct drm_plane *plane = obj_to_plane(obj);
+
+	if (plane->funcs->set_property)
+		ret = plane->funcs->set_property(plane, property, value);
+	if (!ret)
+		drm_object_property_set_value(obj, property, value);
+
+	return ret;
+}
+
 int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 				      struct drm_file *file_priv)
 {
@@ -3268,6 +3284,9 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 	case DRM_MODE_OBJECT_CRTC:
 		ret = drm_mode_crtc_set_obj_prop(arg_obj, property, arg->value);
 		break;
+	case DRM_MODE_OBJECT_PLANE:
+		ret = drm_mode_plane_set_obj_prop(arg_obj, property, arg->value);
+		break;
 	}
 
 out:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 28e9a78..b5017c9 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -618,6 +618,9 @@ struct drm_plane_funcs {
 			    uint32_t src_w, uint32_t src_h);
 	int (*disable_plane)(struct drm_plane *plane);
 	void (*destroy)(struct drm_plane *plane);
+
+	int (*set_property)(struct drm_plane *plane,
+			    struct drm_property *property, uint64_t val);
 };
 
 /**
@@ -657,6 +660,8 @@ struct drm_plane {
 
 	const struct drm_plane_funcs *funcs;
 	void *helper_private;
+
+	struct drm_object_properties properties;
 };
 
 /**
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 1/2] drm: add bitmask property type
  2012-03-30  1:02 [PATCH RFC 1/2] drm: add bitmask property type Rob Clark
  2012-03-30  1:02 ` [PATCH RFC 2/2] drm: add plane properties Rob Clark
@ 2012-03-30  1:15 ` Rob Clark
  2012-03-30 10:37   ` Ville Syrjälä
  2012-03-30  8:44 ` Chris Wilson
  2 siblings, 1 reply; 9+ messages in thread
From: Rob Clark @ 2012-03-30  1:15 UTC (permalink / raw)
  To: dri-devel; +Cc: Rob Clark, patches

On Thu, Mar 29, 2012 at 8:02 PM, Rob Clark <rob.clark@linaro.org> wrote:
> From: Rob Clark <rob@ti.com>
>
> A bitmask property is similar to an enum.  The enum value is a bit
> position (0-63), and valid property values consist of a mask of
> zero or more of (1 << enum_val[n]).
>
> TODO: word commit msg better
> TODO: maybe "flags" would be a better name for the property type?
> ---
> See https://github.com/robclark/kernel-omap4/commit/970b7bb95993fc43b4977976bf8005dc2e1a4ad3#L6R411
> for an example usage.  In this case combinations of "x-invert", "y-invert"
> and "xy-flip" can express all possible combinations of rotations of
> multiples of 90 degrees plus mirroring.  Which is sufficient for an
> xrandr v1.2 rotation support.  For arbitrary transforms in xrandr v1.3
> a different property with a transform matrix (if supported by the
> driver) should be used.

oh, and this shows the mapping between xrandr rotation/reflection mask
and x-invert/y-invert/xy-flip:
https://github.com/robclark/xf86-video-omap/commit/87ffbaf9d282831bf03da457e6f6c4e45a0d6b2b#L0R222

The other option is of course to make the rotation bitmask mirror the
xrandr rotation mask values, which might be a better option to support
drivers which only provide rotation and not mirroring.  I'm ok with
either option, whatever others prefer.

BR,
-R

>
> Note: I've not finished updating libdrm and ddx driver to use this
> yet, so other than compile and boot test, you can consider this as
> untested.  But I figure that it is at least worthwhile to send as
> an RFC at this point to get feedback.
>
>  drivers/gpu/drm/drm_crtc.c |   46 +++++++++++++++++++++++++++++++++++++++++--
>  include/drm/drm_crtc.h     |    3 ++
>  include/drm/drm_mode.h     |    1 +
>  3 files changed, 47 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 95c7ab2..2b462f6 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2713,6 +2713,33 @@ struct drm_property *drm_property_create_enum(struct drm_device *dev, int flags,
>  }
>  EXPORT_SYMBOL(drm_property_create_enum);
>
> +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_values)
> +{
> +       struct drm_property *property;
> +       int i, ret;
> +
> +       flags |= DRM_MODE_PROP_BITMASK;
> +
> +       property = drm_property_create(dev, flags, name, num_values);
> +       if (!property)
> +               return NULL;
> +
> +       for (i = 0; i < num_values; i++) {
> +               ret = drm_property_add_enum(property, i,
> +                                     props[i].type,
> +                                     props[i].name);
> +               if (ret) {
> +                       drm_property_destroy(dev, property);
> +                       return NULL;
> +               }
> +       }
> +
> +       return property;
> +}
> +EXPORT_SYMBOL(drm_property_create_bitmask);
> +
>  struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
>                                         const char *name,
>                                         uint64_t min, uint64_t max)
> @@ -2737,7 +2764,14 @@ int drm_property_add_enum(struct drm_property *property, int index,
>  {
>        struct drm_property_enum *prop_enum;
>
> -       if (!(property->flags & DRM_MODE_PROP_ENUM))
> +       if (!(property->flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)))
> +               return -EINVAL;
> +
> +       /*
> +        * Bitmask enum properties have the additional constraint of values
> +        * from 0 to 63
> +        */
> +       if ((property->flags & DRM_MODE_PROP_BITMASK) && (value > 63))
>                return -EINVAL;
>
>        if (!list_empty(&property->enum_blob_list)) {
> @@ -2881,7 +2915,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>        }
>        property = obj_to_property(obj);
>
> -       if (property->flags & DRM_MODE_PROP_ENUM) {
> +       if (property->flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)) {
>                list_for_each_entry(prop_enum, &property->enum_blob_list, head)
>                        enum_count++;
>        } else if (property->flags & DRM_MODE_PROP_BLOB) {
> @@ -2906,7 +2940,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>        }
>        out_resp->count_values = value_count;
>
> -       if (property->flags & DRM_MODE_PROP_ENUM) {
> +       if (property->flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)) {
>                if ((out_resp->count_enum_blobs >= enum_count) && enum_count) {
>                        copied = 0;
>                        enum_ptr = (struct drm_mode_property_enum __user *)(unsigned long)out_resp->enum_blob_ptr;
> @@ -3063,6 +3097,12 @@ static bool drm_property_change_is_valid(struct drm_property *property,
>                if (value > property->values[1])
>                        return false;
>                return true;
> +       } else if (property->flags & DRM_MODE_PROP_BITMASK) {
> +               int i;
> +               __u64 valid_mask = 0;
> +               for (i = 0; i < property->num_values; i++)
> +                       valid_mask |= (1 << property->values[i]);
> +               return !(value & ~valid_mask);
>        } else {
>                int i;
>                for (i = 0; i < property->num_values; i++)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 32e9c51..28e9a78 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -940,6 +940,9 @@ extern struct drm_property *drm_property_create_enum(struct drm_device *dev, int
>                                         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,
> +                                        const struct drm_prop_enum_list *props, int num_values);
>  struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
>                                         const char *name,
>                                         uint64_t min, uint64_t max);
> diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
> index de5de2a..3190dfe 100644
> --- a/include/drm/drm_mode.h
> +++ b/include/drm/drm_mode.h
> @@ -228,6 +228,7 @@ struct drm_mode_get_connector {
>  #define DRM_MODE_PROP_IMMUTABLE        (1<<2)
>  #define DRM_MODE_PROP_ENUM     (1<<3) /* enumerated type with text strings */
>  #define DRM_MODE_PROP_BLOB     (1<<4)
> +#define DRM_MODE_PROP_BITMASK  (1<<5) /* bitmask of enumerated types */
>
>  struct drm_mode_property_enum {
>        __u64 value;
> --
> 1.7.9.1
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 1/2] drm: add bitmask property type
  2012-03-30  1:02 [PATCH RFC 1/2] drm: add bitmask property type Rob Clark
  2012-03-30  1:02 ` [PATCH RFC 2/2] drm: add plane properties Rob Clark
  2012-03-30  1:15 ` [PATCH RFC 1/2] drm: add bitmask property type Rob Clark
@ 2012-03-30  8:44 ` Chris Wilson
  2 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2012-03-30  8:44 UTC (permalink / raw)
  To: Rob Clark, dri-devel; +Cc: Rob Clark, patches

On Thu, 29 Mar 2012 20:02:45 -0500, Rob Clark <rob.clark@linaro.org> wrote:
> +	} else if (property->flags & DRM_MODE_PROP_BITMASK) {
> +		int i;
> +		__u64 valid_mask = 0;
> +		for (i = 0; i < property->num_values; i++)
> +			valid_mask |= (1 << property->values[i]);
This need to be 1LL << property->values[i].
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 1/2] drm: add bitmask property type
  2012-03-30  1:15 ` [PATCH RFC 1/2] drm: add bitmask property type Rob Clark
@ 2012-03-30 10:37   ` Ville Syrjälä
  2012-03-30 11:00     ` Marcus Lorentzon
                       ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Ville Syrjälä @ 2012-03-30 10:37 UTC (permalink / raw)
  To: Rob Clark; +Cc: Rob Clark, dri-devel, patches

On Thu, Mar 29, 2012 at 08:15:48PM -0500, Rob Clark wrote:
> On Thu, Mar 29, 2012 at 8:02 PM, Rob Clark <rob.clark@linaro.org> wrote:
> > From: Rob Clark <rob@ti.com>
> >
> > A bitmask property is similar to an enum.  The enum value is a bit
> > position (0-63), and valid property values consist of a mask of
> > zero or more of (1 << enum_val[n]).
> >
> > TODO: word commit msg better
> > TODO: maybe "flags" would be a better name for the property type?
> > ---
> > See https://github.com/robclark/kernel-omap4/commit/970b7bb95993fc43b4977976bf8005dc2e1a4ad3#L6R411
> > for an example usage.  In this case combinations of "x-invert", "y-invert"
> > and "xy-flip" can express all possible combinations of rotations of
> > multiples of 90 degrees plus mirroring.  Which is sufficient for an
> > xrandr v1.2 rotation support.  For arbitrary transforms in xrandr v1.3
> > a different property with a transform matrix (if supported by the
> > driver) should be used.
> 
> oh, and this shows the mapping between xrandr rotation/reflection mask
> and x-invert/y-invert/xy-flip:
> https://github.com/robclark/xf86-video-omap/commit/87ffbaf9d282831bf03da457e6f6c4e45a0d6b2b#L0R222
> 
> The other option is of course to make the rotation bitmask mirror the
> xrandr rotation mask values, which might be a better option to support
> drivers which only provide rotation and not mirroring.  I'm ok with
> either option, whatever others prefer.

I would prefer something like that, but we shouldn't just blindly copy
the exact bit definitions. The xrandr reflection vs. rotation always
seemed to be backwards to me. To me it feels more natural to first
rotate, and then reflect/mirror. But it could be that I've just been
influenced by specific hardware designs.


Now that there seems to be a spur of activity in the property area,
could we come up with some way to better control what properties get
added, and how they're defined. Currently every driver is free to
define whatever ad-hoc properties they wish.

I would suggest we either A) define some namespace for standard
properties, or B) introduce some new property mechanism that actually
uses integer property IDs. In either case new properties or changes to
existing standard properties should be carefully reviewed. I'm sort of
partial to option B, since doing a gazillion string comparisons when
pushing lots of properties to the driver seems like a pointless
waste of cycles. OTOH option A seems a little less revolutionary.

In any case, the current mess just doesn't allow any kind of truly
generic user space code to be written.

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 1/2] drm: add bitmask property type
  2012-03-30 10:37   ` Ville Syrjälä
@ 2012-03-30 11:00     ` Marcus Lorentzon
  2012-03-30 12:25     ` Rob Clark
  2012-03-30 16:59     ` Paulo Zanoni
  2 siblings, 0 replies; 9+ messages in thread
From: Marcus Lorentzon @ 2012-03-30 11:00 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Rob Clark, patches, dri-devel, Rob Clark

On 03/30/2012 12:37 PM, Ville Syrjälä wrote:
> On Thu, Mar 29, 2012 at 08:15:48PM -0500, Rob Clark wrote:
>> On Thu, Mar 29, 2012 at 8:02 PM, Rob Clark<rob.clark@linaro.org>  wrote:
>>> From: Rob Clark<rob@ti.com>
>>>
>>> A bitmask property is similar to an enum.  The enum value is a bit
>>> position (0-63), and valid property values consist of a mask of
>>> zero or more of (1<<  enum_val[n]).
>>>
>>> TODO: word commit msg better
>>> TODO: maybe "flags" would be a better name for the property type?
>>> ---
>>> See https://github.com/robclark/kernel-omap4/commit/970b7bb95993fc43b4977976bf8005dc2e1a4ad3#L6R411
>>> for an example usage.  In this case combinations of "x-invert", "y-invert"
>>> and "xy-flip" can express all possible combinations of rotations of
>>> multiples of 90 degrees plus mirroring.  Which is sufficient for an
>>> xrandr v1.2 rotation support.  For arbitrary transforms in xrandr v1.3
>>> a different property with a transform matrix (if supported by the
>>> driver) should be used.
>> oh, and this shows the mapping between xrandr rotation/reflection mask
>> and x-invert/y-invert/xy-flip:
>> https://github.com/robclark/xf86-video-omap/commit/87ffbaf9d282831bf03da457e6f6c4e45a0d6b2b#L0R222
>>
>> The other option is of course to make the rotation bitmask mirror the
>> xrandr rotation mask values, which might be a better option to support
>> drivers which only provide rotation and not mirroring.  I'm ok with
>> either option, whatever others prefer.
> I would prefer something like that, but we shouldn't just blindly copy
> the exact bit definitions. The xrandr reflection vs. rotation always
> seemed to be backwards to me. To me it feels more natural to first
> rotate, and then reflect/mirror. But it could be that I've just been
> influenced by specific hardware designs.
Exactly, and I would prefer a separate rotation (0,90,180,270) and 
mirror (true,false) property. I see no reason to merge these using a 
special bitmask property type.
And if there is an issue with atomic commit of rotation and mirror, this 
does not solve the general atomic commit issue. And if we solve that, we 
don't need to merge two properties in one.
>
> Now that there seems to be a spur of activity in the property area,
> could we come up with some way to better control what properties get
> added, and how they're defined. Currently every driver is free to
> define whatever ad-hoc properties they wish.
>
> I would suggest we either A) define some namespace for standard
> properties, or B) introduce some new property mechanism that actually
> uses integer property IDs. In either case new properties or changes to
> existing standard properties should be carefully reviewed. I'm sort of
> partial to option B, since doing a gazillion string comparisons when
> pushing lots of properties to the driver seems like a pointless
> waste of cycles. OTOH option A seems a little less revolutionary.
>
> In any case, the current mess just doesn't allow any kind of truly
> generic user space code to be written.

+1, I think a general mechanism for standard properties would be helpful 
for applications that want to expose a dynamic interface with settings 
supplied from kernel.
Option A would be easy to add, but option B would make it easy to do 
atomic commit with normal params and properties (as described in 
previous email). So why not A+B? Add ids to make setting multiple 
properties easier and add a name prefix. I assume these property names 
are IDs and not something intended to be shown to the user directly 
(i18n). And having standard property names would allow UI to include 
translations for these properties. Non standard properties could just be 
shown using their ID.

/BR
/Marcus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 1/2] drm: add bitmask property type
  2012-03-30 10:37   ` Ville Syrjälä
  2012-03-30 11:00     ` Marcus Lorentzon
@ 2012-03-30 12:25     ` Rob Clark
  2012-03-30 16:59     ` Paulo Zanoni
  2 siblings, 0 replies; 9+ messages in thread
From: Rob Clark @ 2012-03-30 12:25 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: dri-devel, patches

On Fri, Mar 30, 2012 at 5:37 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Mar 29, 2012 at 08:15:48PM -0500, Rob Clark wrote:
>> On Thu, Mar 29, 2012 at 8:02 PM, Rob Clark <rob.clark@linaro.org> wrote:
>> > From: Rob Clark <rob@ti.com>
>> >
>> > A bitmask property is similar to an enum.  The enum value is a bit
>> > position (0-63), and valid property values consist of a mask of
>> > zero or more of (1 << enum_val[n]).
>> >
>> > TODO: word commit msg better
>> > TODO: maybe "flags" would be a better name for the property type?
>> > ---
>> > See https://github.com/robclark/kernel-omap4/commit/970b7bb95993fc43b4977976bf8005dc2e1a4ad3#L6R411
>> > for an example usage.  In this case combinations of "x-invert", "y-invert"
>> > and "xy-flip" can express all possible combinations of rotations of
>> > multiples of 90 degrees plus mirroring.  Which is sufficient for an
>> > xrandr v1.2 rotation support.  For arbitrary transforms in xrandr v1.3
>> > a different property with a transform matrix (if supported by the
>> > driver) should be used.
>>
>> oh, and this shows the mapping between xrandr rotation/reflection mask
>> and x-invert/y-invert/xy-flip:
>> https://github.com/robclark/xf86-video-omap/commit/87ffbaf9d282831bf03da457e6f6c4e45a0d6b2b#L0R222
>>
>> The other option is of course to make the rotation bitmask mirror the
>> xrandr rotation mask values, which might be a better option to support
>> drivers which only provide rotation and not mirroring.  I'm ok with
>> either option, whatever others prefer.
>
> I would prefer something like that, but we shouldn't just blindly copy
> the exact bit definitions. The xrandr reflection vs. rotation always
> seemed to be backwards to me. To me it feels more natural to first
> rotate, and then reflect/mirror. But it could be that I've just been
> influenced by specific hardware designs.

well, I'd sort of prefer not having two levels of translating rotation
bitmask into something else (ie. userspace xrandr -> kms, then kernel
kms -> hw)..  so I'd prefer to stick to xrandr bitmasks rather than
just shuffle some bits around for the sake of it..  the only advantage
I like with x-invert/y-invert/xy-flip is that it doesn't have invalid
bitmask combinations (ie. RR_Rotate_0 | RR_Rotate_90)

>
> Now that there seems to be a spur of activity in the property area,
> could we come up with some way to better control what properties get
> added, and how they're defined. Currently every driver is free to
> define whatever ad-hoc properties they wish.

yeah, basically this was why I was bouncing the idea of rotation
related properties off the list.. I'd rather use values in omapdrm
that we could align on for other drivers

BR,
-R

> I would suggest we either A) define some namespace for standard
> properties, or B) introduce some new property mechanism that actually
> uses integer property IDs. In either case new properties or changes to
> existing standard properties should be carefully reviewed. I'm sort of
> partial to option B, since doing a gazillion string comparisons when
> pushing lots of properties to the driver seems like a pointless
> waste of cycles. OTOH option A seems a little less revolutionary.
>
> In any case, the current mess just doesn't allow any kind of truly
> generic user space code to be written.
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 1/2] drm: add bitmask property type
  2012-03-30 10:37   ` Ville Syrjälä
  2012-03-30 11:00     ` Marcus Lorentzon
  2012-03-30 12:25     ` Rob Clark
@ 2012-03-30 16:59     ` Paulo Zanoni
  2012-03-30 17:16       ` Paulo Zanoni
  2 siblings, 1 reply; 9+ messages in thread
From: Paulo Zanoni @ 2012-03-30 16:59 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Rob Clark, patches, dri-devel, Rob Clark

2012/3/30 Ville Syrjälä <ville.syrjala@linux.intel.com>:
>
> I would suggest we either A) define some namespace for standard
> properties, or B) introduce some new property mechanism that actually
> uses integer property IDs. In either case new properties or changes to
> existing standard properties should be carefully reviewed. I'm sort of
> partial to option B, since doing a gazillion string comparisons when
> pushing lots of properties to the driver seems like a pointless
> waste of cycles. OTOH option A seems a little less revolutionary.
>
> In any case, the current mess just doesn't allow any kind of truly
> generic user space code to be written.

While DRM doesn't have standard properties, RandR has:
http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt#n1624
 . They define "standard (connector) properties", and also define
"mandatory" and "optional" properties. And "mandatory" properties have
a "mandatory since" to indicate the protocol version made the
properties mandatory. The protocol also defines that driver-specific
properties should start with "_". Can't we try to add some document
(or header file) defining the standard properties and add a way to
distinguish between? Documentation/drm/properties.txt?

BTW, renaming current properties is not an option, right? Or are the
property names not considered part of the ABI? Current code relies on
those names, so if they change... If we consider the possibility of
renaming properties, I'd suggest just copying the RandR
standardization rules...

Here is a list of property names currently used under drivers/gpu/drm
(list may be smaller than reality):

- drm:
"bottom margin"
"brightness"
"contrast"
"dirty"
"dithering"
"DPMS"
"EDID"
"flicker reduction"
"hue"
"left margin"
"mode"
"overscan"
"right margin"
"saturation"
"scaling mode"
"select subconnector"
"subconnector"
"top margin"

- gma500:
"backlight"
"bottom_margin"
"brightness"
"contrast"
"dot_crawl"
"flicker_filter"
"flicker"filter_2d"
"flicker_filter_adaptive"
"hpos"
"hue"
"left_margin"
"mode"
"right_margin"
"saturation"
"sharpness"
"top_margin"
"tv_chroma_filter"
"tv_luma_filter"
"vpos"

- i2c:
"scale"

- i915:
"audio"
"bottom_margin"
"brightness"
"Broadcast RGB"
"contrast"
"dot_crawl"
"flicker_filter"
"flicker_filter_2d"
"flicker_filter_adaptive"
"hpos"
"hue"
"left_margin"
"mode"
"right_margin"
"rotation"
"saturation"
"sharpness"
"top_margin"
"tv_chroma_filter"
"tv_luma_filter"
"underscan hborder"
"underscan vborder"
"vpos"

- nouveau:
"color vibrance"
"dithering depth"
"dithering mode"
"underscan"
"underscan hborder"
"underscan vborder"
"vibrant hue"

- radeon:
"coherent"
"load detection"
"tmds_pll"
"tv standard"
"underscan"
"underscan hborder"
"underscan vborder"

Cheers,
Paulo

-- 
Paulo Zanoni

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 1/2] drm: add bitmask property type
  2012-03-30 16:59     ` Paulo Zanoni
@ 2012-03-30 17:16       ` Paulo Zanoni
  0 siblings, 0 replies; 9+ messages in thread
From: Paulo Zanoni @ 2012-03-30 17:16 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Rob Clark, patches, dri-devel, Rob Clark

2012/3/30 Paulo Zanoni <przanoni@gmail.com>:
> Can't we try to add some document
> (or header file) defining the standard properties and add a way to
> distinguish between? Documentation/drm/properties.txt?
>

After looking at the list names, maybe we should define that standard
properties should be named with "BIG LETTERS ONLY" and non-standard
should be not-only-BIG-LETTERS? This will be backwards-compatible if
we define EDID and DPMS as standard properties (and they don't need to
be mandatory).


-- 
Paulo Zanoni

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2012-03-30 17:16 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-30  1:02 [PATCH RFC 1/2] drm: add bitmask property type Rob Clark
2012-03-30  1:02 ` [PATCH RFC 2/2] drm: add plane properties Rob Clark
2012-03-30  1:15 ` [PATCH RFC 1/2] drm: add bitmask property type Rob Clark
2012-03-30 10:37   ` Ville Syrjälä
2012-03-30 11:00     ` Marcus Lorentzon
2012-03-30 12:25     ` Rob Clark
2012-03-30 16:59     ` Paulo Zanoni
2012-03-30 17:16       ` Paulo Zanoni
2012-03-30  8:44 ` Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.