From: Paulo Zanoni paulo.r.zanoni@intel.com
Move code from drm_mode_connector_property_set_ioctl to a new function, so we can reuse this code when we add crtc properties.
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com --- drivers/gpu/drm/drm_crtc.c | 41 +++++++++++++++++++++-------------------- 1 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 5e818a8..cd155e9 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2917,6 +2917,26 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
+static int drm_property_change_is_valid(struct drm_property *property, + __u64 value) +{ + if (property->flags & DRM_MODE_PROP_IMMUTABLE) + return 0; + if (property->flags & DRM_MODE_PROP_RANGE) { + if (value < property->values[0]) + return 0; + if (value > property->values[1]) + return 0; + return 1; + } else { + int i; + for (i = 0; i < property->num_values; i++) + if (property->values[i] == value) + return 1; + return 0; + } +} + int drm_mode_connector_property_set_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -2953,28 +2973,9 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev, } property = obj_to_property(obj);
- if (property->flags & DRM_MODE_PROP_IMMUTABLE) + if (!drm_property_change_is_valid(property, out_resp->value)) goto out;
- if (property->flags & DRM_MODE_PROP_RANGE) { - if (out_resp->value < property->values[0]) - goto out; - - if (out_resp->value > property->values[1]) - goto out; - } else { - int found = 0; - for (i = 0; i < property->num_values; i++) { - if (property->values[i] == out_resp->value) { - found = 1; - break; - } - } - if (!found) { - goto out; - } - } - /* Do DPMS ourselves */ if (property == connector->dev->mode_config.dpms_property) { if (connector->funcs->dpms)
From: Paulo Zanoni paulo.r.zanoni@intel.com
Code based on the connector properties code.
Two new ioctls: - DRM_IOCTL_MODE_CRTC_GETPROPERTIES - DRM_IOCTL_MODE_CRTC_SETPROPERTY
The i915 driver needs this (for the rotation property). Other drivers might need this too.
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com --- drivers/gpu/drm/drm_crtc.c | 116 ++++++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_drv.c | 4 +- include/drm/drm.h | 2 + include/drm/drm_crtc.h | 22 ++++++++- include/drm/drm_mode.h | 13 +++++ 5 files changed, 155 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index cd155e9..5112921 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2685,6 +2685,22 @@ int drm_connector_attach_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_connector_attach_property);
+void drm_crtc_attach_property(struct drm_crtc *crtc, + struct drm_property *property, uint64_t init_val) +{ + int i; + + for (i = 0; i < DRM_CRTC_MAX_PROPERTY; i++) { + if (crtc->property_ids[i] == 0) { + crtc->property_ids[i] = property->base.id; + crtc->property_values[i] = init_val; + return; + } + } + BUG_ON(1); +} +EXPORT_SYMBOL(drm_crtc_attach_property); + int drm_connector_property_set_value(struct drm_connector *connector, struct drm_property *property, uint64_t value) { @@ -2992,6 +3008,106 @@ out: return ret; }
+int drm_mode_crtc_get_properties_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv) +{ + struct drm_mode_crtc_get_properties *arg = data; + struct drm_mode_object *obj; + struct drm_crtc *crtc; + int ret = 0; + int i; + int copied = 0; + int props_count = 0; + uint32_t __user *props_ptr; + uint64_t __user *prop_values_ptr; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + + mutex_lock(&dev->mode_config.mutex); + + obj = drm_mode_object_find(dev, arg->crtc_id, DRM_MODE_OBJECT_CRTC); + if (!obj) { + ret = -EINVAL; + goto out; + } + crtc = obj_to_crtc(obj); + + for (i = 0; i < DRM_CRTC_MAX_PROPERTY; i++) + if (crtc->property_ids[i] != 0) + props_count++; + + /* This ioctl is called twice, once to determine how much space is + * needed, and the 2nd time to fill it. */ + if ((arg->count_props >= props_count) && props_count) { + copied = 0; + props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr); + prop_values_ptr = (uint64_t __user *)(unsigned long) + (arg->prop_values_ptr); + for (i = 0; i < props_count; i++) { + if (put_user(crtc->property_ids[i], + props_ptr + copied)) { + ret = -EFAULT; + goto out; + } + if (put_user(crtc->property_values[i], + prop_values_ptr + copied)) { + ret = -EFAULT; + goto out; + } + } + } + arg->count_props = props_count; +out: + mutex_unlock(&dev->mode_config.mutex); + return ret; +} + +int drm_mode_crtc_set_property_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv) +{ + struct drm_mode_crtc_set_property *arg = data; + struct drm_mode_object *obj; + struct drm_property *property; + struct drm_crtc *crtc; + int ret = -EINVAL; + int i; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + + mutex_lock(&dev->mode_config.mutex); + + obj = drm_mode_object_find(dev, arg->crtc_id, DRM_MODE_OBJECT_CRTC); + if (!obj) + goto out; + crtc = obj_to_crtc(obj); + + for (i = 0; i < DRM_CRTC_MAX_PROPERTY; i++) + if (crtc->property_ids[i] == arg->prop_id) + break; + + if (i == DRM_CRTC_MAX_PROPERTY) + goto out; + + obj = drm_mode_object_find(dev, arg->prop_id, DRM_MODE_OBJECT_PROPERTY); + if (!obj) + goto out; + property = obj_to_property(obj); + + if (!drm_property_change_is_valid(property, arg->value)) + goto out; + + if (crtc->funcs->set_property) + ret = crtc->funcs->set_property(crtc, property, arg->value); + if (!ret) { + crtc->property_values[i] = arg->value; + } +out: + mutex_unlock(&dev->mode_config.mutex); + return ret; +} + int drm_mode_connector_attach_encoder(struct drm_connector *connector, struct drm_encoder *encoder) { diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index ebf7d3f..c6d00d9 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -159,7 +159,9 @@ static struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_DIRTYFB, drm_mode_dirtyfb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), - DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED) + DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_MODE_CRTC_GETPROPERTIES, drm_mode_crtc_get_properties_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_MODE_CRTC_SETPROPERTY, drm_mode_crtc_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED) };
#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) diff --git a/include/drm/drm.h b/include/drm/drm.h index 49d94ed..a51919f 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -718,6 +718,8 @@ struct drm_get_cap { #define DRM_IOCTL_MODE_GETPLANE DRM_IOWR(0xB6, struct drm_mode_get_plane) #define DRM_IOCTL_MODE_SETPLANE DRM_IOWR(0xB7, struct drm_mode_set_plane) #define DRM_IOCTL_MODE_ADDFB2 DRM_IOWR(0xB8, struct drm_mode_fb_cmd2) +#define DRM_IOCTL_MODE_CRTC_GETPROPERTIES DRM_IOWR(0xB9, struct drm_mode_crtc_get_properties) +#define DRM_IOCTL_MODE_CRTC_SETPROPERTY DRM_IOWR(0xBA, struct drm_mode_crtc_set_property)
/** * Device specific ioctls should only be in their respective headers diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 63e4fce..164a3a5 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -283,6 +283,8 @@ struct drm_encoder; struct drm_pending_vblank_event; struct drm_plane;
+#define DRM_CRTC_MAX_PROPERTY 16 + /** * drm_crtc_funcs - control CRTCs for a given device * @reset: reset CRTC after state has been invalidate (e.g. resume) @@ -297,7 +299,8 @@ struct drm_plane; * @mode_fixup: fixup proposed mode * @mode_set: set the desired mode on the CRTC * @gamma_set: specify color ramp for CRTC - * @destroy: deinit and free object. + * @destroy: deinit and free object + * @set_property: called when a property is changed * * The drm_crtc_funcs structure is the central CRTC management structure * in the DRM. Each CRTC controls one or more connectors (note that the name @@ -341,6 +344,9 @@ struct drm_crtc_funcs { int (*page_flip)(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event); + + int (*set_property)(struct drm_crtc *crtc, + struct drm_property *property, uint64_t val); };
/** @@ -360,6 +366,8 @@ struct drm_crtc_funcs { * @framedur_ns: precise line timing * @pixeldur_ns: precise pixel timing * @helper_private: mid-layer private data + * @property_ids: property tracking for this CRTC + * @property_values: property values * * Each CRTC may have one or more connectors associated with it. This structure * allows the CRTC to be controlled. @@ -395,6 +403,9 @@ struct drm_crtc {
/* if you are using the helper */ void *helper_private; + + u32 property_ids[DRM_CRTC_MAX_PROPERTY]; + uint64_t property_values[DRM_CRTC_MAX_PROPERTY]; };
@@ -902,6 +913,9 @@ extern bool drm_crtc_in_use(struct drm_crtc *crtc);
extern int drm_connector_attach_property(struct drm_connector *connector, struct drm_property *property, uint64_t init_val); +extern void drm_crtc_attach_property(struct drm_crtc *crtc, + struct drm_property *property, + uint64_t init_val); extern struct drm_property *drm_property_create(struct drm_device *dev, int flags, const char *name, int num_values); extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property); @@ -1005,6 +1019,12 @@ extern int drm_mode_mmap_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern int drm_mode_destroy_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +extern int drm_mode_crtc_get_properties_ioctl(struct drm_device *dev, + void *data, + struct drm_file *file_priv); +extern int drm_mode_crtc_set_property_ioctl(struct drm_device *dev, + void *data, + struct drm_file *file_priv);
extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, int *bpp); diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h index 2a2acda..890b930 100644 --- a/include/drm/drm_mode.h +++ b/include/drm/drm_mode.h @@ -252,6 +252,19 @@ struct drm_mode_connector_set_property { __u32 connector_id; };
+struct drm_mode_crtc_get_properties { + __u64 props_ptr; + __u64 prop_values_ptr; + __u32 count_props; + __u32 crtc_id; +}; + +struct drm_mode_crtc_set_property { + __u64 value; + __u32 prop_id; + __u32 crtc_id; +}; + struct drm_mode_get_blob { __u32 blob_id; __u32 length;
Three comments about the design are inline:
+void drm_crtc_attach_property(struct drm_crtc *crtc,
- struct drm_property *property, uint64_t init_val)
+{
- int i;
- for (i = 0; i < DRM_CRTC_MAX_PROPERTY; i++) {
- if (crtc->property_ids[i] == 0) {
- crtc->property_ids[i] = property->base.id;
- crtc->property_values[i] = init_val;
- return;
- }
- }
- BUG_ON(1);
I looked at drm_connector_attach_property and saw that instead of BUG_ON(1), it tries to return -EINVAL. The problem is that only zero callers check for the return value of drm_connector_attach_property. I can provide a patch for drm_connector_attach_property changing the -EINVAL for BUG_ON(1) if no one objects. Or I can also add -EINVAL to drm_crtc_attach_property and, to be consistent, not check for it :)
+int drm_mode_crtc_get_properties_ioctl(struct drm_device *dev, void *data,
- struct drm_file *file_priv)
+{
... (same function) ...
- for (i = 0; i < DRM_CRTC_MAX_PROPERTY; i++)
- if (crtc->property_ids[i] != 0)
- props_count++;
... (same function) ...
- for (i = 0; i < props_count; i++) {
- if (put_user(crtc->property_ids[i],
- props_ptr + copied)) {
- ret = -EFAULT;
- goto out;
- }
If you look at the connector properties (and now crtc properties), you'll see that they're stored in "property_ids" arrays, and invalid IDs are "0". There is code to add properties, but I didn't find code to remove them. But if you look at the connector code, you'll that it is implemented in a way that if the array is something like: [ valid_id, 0, valid_id, 0, 0, 0 ... ], the code will work (even with a zero in the middle of two valid ids). While this makes the code more resistant to mistakes, it also makes some loops longer (you don't need to stop at the first invalid id, you need to keep looking). And, as far as I found, we don't ever reach the [ valid, 0, valid ] case because we never remove properties. Even if we start removing properties, we can try to avoid the [valid, 0, valid] case. In my patch, this condition is not valid for the crtc properties, so we can make loops shorter (and I see I could still improve one of the loops). Which way do we prefer?
- DRM_IOCTL_DEF(DRM_IOCTL_MODE_CRTC_GETPROPERTIES, drm_mode_crtc_get_properties_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
- DRM_IOCTL_DEF(DRM_IOCTL_MODE_CRTC_SETPROPERTY, drm_mode_crtc_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED)
I'm not sure about how we want these flags. I just copied from connector properties ioctls.
On Mon, Jan 16, 2012 at 06:29:36PM -0200, Paulo Zanoni wrote:
Three comments about the design are inline:
+void drm_crtc_attach_property(struct drm_crtc *crtc,
- struct drm_property *property, uint64_t init_val)
+{
- int i;
- for (i = 0; i < DRM_CRTC_MAX_PROPERTY; i++) {
- if (crtc->property_ids[i] == 0) {
- crtc->property_ids[i] = property->base.id;
- crtc->property_values[i] = init_val;
- return;
- }
- }
- BUG_ON(1);
I looked at drm_connector_attach_property and saw that instead of BUG_ON(1), it tries to return -EINVAL. The problem is that only zero callers check for the return value of drm_connector_attach_property. I can provide a patch for drm_connector_attach_property changing the -EINVAL for BUG_ON(1) if no one objects. Or I can also add -EINVAL to drm_crtc_attach_property and, to be consistent, not check for it :)
Just a quick comment: WARN is generally highly preferred over BUG. Use the latter only when you know that the kernel _will_ go down in a horribly way and it's better to stop it doing so (e.g. for NULL pointer checks). -Daniel
From: Paulo Zanoni paulo.r.zanoni@intel.com
This property is needed by to inform the KVMr feature about our current rotation: whenever we change the rotation, we should change that property so that the KVMr knows that the screen is rotated.
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com --- drivers/gpu/drm/i915/i915_reg.h | 5 +++ drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 64 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c3afb78..49e4f10 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2324,6 +2324,11 @@ #define PIPECONF_INTERLACE_FIELD_0_ONLY (7 << 21) #define PIPECONF_INTERLACE_MASK (7 << 21) #define PIPECONF_CXSR_DOWNCLOCK (1<<16) +#define PIPECONF_ROTATION_MASK (3 << 14) +#define PIPECONF_ROTATION_0 (0 << 14) +#define PIPECONF_ROTATION_90 (1 << 14) +#define PIPECONF_ROTATION_180 (2 << 14) +#define PIPECONF_ROTATION_270 (3 << 14) #define PIPECONF_BPP_MASK (0x000000e0) #define PIPECONF_BPP_8 (0<<5) #define PIPECONF_BPP_10 (1<<5) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 29743de..7ec1b18 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7072,6 +7072,8 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) kfree(work); }
+ drm_property_destroy(dev, intel_crtc->rotation_property); + drm_crtc_cleanup(crtc);
kfree(intel_crtc); @@ -7529,6 +7531,49 @@ static void intel_crtc_reset(struct drm_crtc *crtc) intel_sanitize_modesetting(dev, intel_crtc->pipe, intel_crtc->plane); }
+static void intel_crtc_set_rotation(struct drm_crtc *crtc, + uint64_t rotation) +{ + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + int reg = PIPECONF(intel_crtc->pipe); + u32 val = I915_READ(reg); + + val &= ~PIPECONF_ROTATION_MASK; + + switch (rotation) { + case 0: + val |= PIPECONF_ROTATION_0; + break; + case 90: + val |= PIPECONF_ROTATION_90; + break; + case 180: + val |= PIPECONF_ROTATION_180; + break; + case 270: + val |= PIPECONF_ROTATION_270; + break; + default: + DRM_ERROR("Unsupported rotation: %Lu\n", rotation); + val |= PIPECONF_ROTATION_0; + } + + I915_WRITE(reg, val); +} + +static int intel_crtc_set_property(struct drm_crtc *crtc, + struct drm_property *property, + uint64_t val) +{ + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + + if (property == intel_crtc->rotation_property) + intel_crtc_set_rotation(crtc, val); + return 0; +} + static struct drm_crtc_helper_funcs intel_helper_funcs = { .dpms = intel_crtc_dpms, .mode_fixup = intel_crtc_mode_fixup, @@ -7547,12 +7592,14 @@ static const struct drm_crtc_funcs intel_crtc_funcs = { .set_config = drm_crtc_helper_set_config, .destroy = intel_crtc_destroy, .page_flip = intel_crtc_page_flip, + .set_property = intel_crtc_set_property, };
static void intel_crtc_init(struct drm_device *dev, int pipe) { drm_i915_private_t *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc; + struct drm_property *prop; int i;
intel_crtc = kzalloc(sizeof(struct intel_crtc) + (INTELFB_CONN_LIMIT * sizeof(struct drm_connector *)), GFP_KERNEL); @@ -7568,6 +7615,16 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) intel_crtc->lut_b[i] = i; }
+ prop = drm_property_create(dev, DRM_MODE_PROP_RANGE, "rotation", 2); + if (prop == NULL) { + DRM_ERROR("Failed to create rotation property!\n"); + BUG(); + } + prop->values[0] = 0; + prop->values[1] = 359; + drm_crtc_attach_property(&intel_crtc->base, prop, 0); + intel_crtc->rotation_property = prop; + /* Swap pipes & planes for FBC on pre-965 */ intel_crtc->pipe = pipe; intel_crtc->plane = pipe; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 5ac8a16..26bdc65 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -176,6 +176,8 @@ struct intel_crtc {
bool no_pll; /* tertiary pipe for IVB */ bool use_pll_a; + + struct drm_property *rotation_property; };
struct intel_plane {
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Paulo Zanoni