| Message ID | 20170725080122.20548-3-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 25-07-17 om 10:01 schreef Daniel Vetter: > Atomic drivers only use the property value store for immutable (i.e. > can't be set by userspace, but the kernel can still adjust it) > properties. The only tricky part is the removal of the update in > drm_atomic_helper_update_legacy_modeset_state(). > > This was added in > > commit 8c10342cb48f3140d9abeadcfd2fa6625d447282 (tag: topic/drm-misc-2015-07-28) > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Date: Mon Jul 27 13:24:29 2015 +0200 > > drm/atomic: Update legacy DPMS state during modesets, v3. > > by copying it from the i915 code, where it was originally added in > > commit 68d3472047a572936551f8ff0b6f4016c5a1fdef > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Thu Sep 6 22:08:35 2012 +0200 > > drm/i915: update dpms property in set_mode > > for the legacy modeset code. The reason we needed this hack was that > i915 didn't yet set DRIVER_ATOMIC, and we checked for that instead of > the newer-ish drm_drv_uses_atomic_modeset(), which avoids such > troubles. With the correct feature checks this isn't needed anymore at > all. > > Also make sure that drivers don't accidentally get this wrong by > making the exported version of drm_object_property_get_value() only > work for legacy drivers. Only gma500 uses it anyway. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 --- > drivers/gpu/drm/drm_connector.c | 3 +-- > drivers/gpu/drm/drm_crtc.c | 2 +- > drivers/gpu/drm/drm_mode_object.c | 49 +++++++++++++++++++++++-------------- > drivers/gpu/drm/drm_plane.c | 2 +- > 5 files changed, 33 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 7582bbc5decc..4a960c741e35 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -921,16 +921,12 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, > crtc = new_conn_state->crtc; > if ((!crtc && old_conn_state->crtc) || > (crtc && drm_atomic_crtc_needs_modeset(crtc->state))) { > - struct drm_property *dpms_prop = > - dev->mode_config.dpms_property; > int mode = DRM_MODE_DPMS_OFF; > > if (crtc && crtc->state->active) > mode = DRM_MODE_DPMS_ON; > > connector->dpms = mode; > - drm_object_property_set_value(&connector->base, > - dpms_prop, mode); > } > } > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 8072e6e4c62c..349104eadefe 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1225,8 +1225,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj, > } else if (connector->funcs->set_property) > ret = connector->funcs->set_property(connector, property, value); > > - /* store the property value if successful */ > - if (!ret) > + if (!ret && drm_drv_uses_atomic_modeset(property->dev)) > drm_object_property_set_value(&connector->base, property, value); > return ret; > } > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 5af25ce5bf7c..7d4fcdd34342 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -736,7 +736,7 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj, > > if (crtc->funcs->set_property) > ret = crtc->funcs->set_property(crtc, property, value); > - if (!ret) > + if (!ret && drm_drv_uses_atomic_modeset(property->dev)) > drm_object_property_set_value(obj, property, value); You've inverted the checks here for connector and crtc. :) Otherwise looks sane, fix this and you can add my r-b. On second thought, as penance you should add kms_properties to the fast-feedback testlist. ;) > return ret; > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c > index da9a9adbcc98..92743a796bf0 100644 > --- a/drivers/gpu/drm/drm_mode_object.c > +++ b/drivers/gpu/drm/drm_mode_object.c > @@ -233,6 +233,9 @@ int drm_object_property_set_value(struct drm_mode_object *obj, > { > int i; > > + WARN_ON(drm_drv_uses_atomic_modeset(property->dev) && > + !(property->flags & DRM_MODE_PROP_IMMUTABLE)); > + > for (i = 0; i < obj->properties->count; i++) { > if (obj->properties->properties[i] == property) { > obj->properties->values[i] = val; > @@ -244,24 +247,7 @@ int drm_object_property_set_value(struct drm_mode_object *obj, > } > EXPORT_SYMBOL(drm_object_property_set_value); > > -/** > - * drm_object_property_get_value - retrieve the value of a property > - * @obj: drm mode object to get property value from > - * @property: property to retrieve > - * @val: storage for the property value > - * > - * This function retrieves the softare state of the given property for the given > - * property. Since there is no driver callback to retrieve the current property > - * value this might be out of sync with the hardware, depending upon the driver > - * and property. > - * > - * Atomic drivers should never call this function directly, the core will read > - * out property values through the various ->atomic_get_property callbacks. > - * > - * Returns: > - * Zero on success, error code on failure. > - */ > -int drm_object_property_get_value(struct drm_mode_object *obj, > +int __drm_object_property_get_value(struct drm_mode_object *obj, > struct drm_property *property, uint64_t *val) > { > int i; > @@ -284,6 +270,31 @@ int drm_object_property_get_value(struct drm_mode_object *obj, > > return -EINVAL; > } > + > +/** > + * drm_object_property_get_value - retrieve the value of a property > + * @obj: drm mode object to get property value from > + * @property: property to retrieve > + * @val: storage for the property value > + * > + * This function retrieves the softare state of the given property for the given > + * property. Since there is no driver callback to retrieve the current property > + * value this might be out of sync with the hardware, depending upon the driver > + * and property. > + * > + * Atomic drivers should never call this function directly, the core will read > + * out property values through the various ->atomic_get_property callbacks. > + * > + * Returns: > + * Zero on success, error code on failure. > + */ > +int drm_object_property_get_value(struct drm_mode_object *obj, > + struct drm_property *property, uint64_t *val) > +{ > + WARN_ON(drm_drv_uses_atomic_modeset(property->dev)); > + > + return __drm_object_property_get_value(obj, property, val); > +} > EXPORT_SYMBOL(drm_object_property_get_value); > > /* helper for getconnector and getproperties ioctls */ > @@ -302,7 +313,7 @@ int drm_mode_object_get_properties(struct drm_mode_object *obj, bool atomic, > continue; > > if (*arg_count_props > count) { > - ret = drm_object_property_get_value(obj, prop, &val); > + ret = __drm_object_property_get_value(obj, prop, &val); > if (ret) > return ret; > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 5dc8c4350602..b4ffd0455200 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -330,7 +330,7 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, > > if (plane->funcs->set_property) > ret = plane->funcs->set_property(plane, property, value); > - if (!ret) > + if (!ret && drm_drv_uses_atomic_modeset(property->dev)) > drm_object_property_set_value(obj, property, value); > > return ret;
Hi Daniel, On Tuesday 25 Jul 2017 10:01:16 Daniel Vetter wrote: > Atomic drivers only use the property value store for immutable (i.e. > can't be set by userspace, but the kernel can still adjust it) > properties. The only tricky part is the removal of the update in > drm_atomic_helper_update_legacy_modeset_state(). > > This was added in > > commit 8c10342cb48f3140d9abeadcfd2fa6625d447282 (tag: > topic/drm-misc-2015-07-28) Author: Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> > Date: Mon Jul 27 13:24:29 2015 +0200 > > drm/atomic: Update legacy DPMS state during modesets, v3. > > by copying it from the i915 code, where it was originally added in > > commit 68d3472047a572936551f8ff0b6f4016c5a1fdef > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Thu Sep 6 22:08:35 2012 +0200 > > drm/i915: update dpms property in set_mode > > for the legacy modeset code. The reason we needed this hack was that > i915 didn't yet set DRIVER_ATOMIC, and we checked for that instead of > the newer-ish drm_drv_uses_atomic_modeset(), which avoids such > troubles. With the correct feature checks this isn't needed anymore at > all. > > Also make sure that drivers don't accidentally get this wrong by > making the exported version of drm_object_property_get_value() only > work for legacy drivers. Only gma500 uses it anyway. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 --- > drivers/gpu/drm/drm_connector.c | 3 +-- > drivers/gpu/drm/drm_crtc.c | 2 +- > drivers/gpu/drm/drm_mode_object.c | 49 ++++++++++++++++++++-------------- > drivers/gpu/drm/drm_plane.c | 2 +- > 5 files changed, 33 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c index 7582bbc5decc..4a960c741e35 > 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -921,16 +921,12 @@ drm_atomic_helper_update_legacy_modeset_state(struct > drm_device *dev, crtc = new_conn_state->crtc; > if ((!crtc && old_conn_state->crtc) || > (crtc && drm_atomic_crtc_needs_modeset(crtc->state))) { > - struct drm_property *dpms_prop = > - dev->mode_config.dpms_property; > int mode = DRM_MODE_DPMS_OFF; > > if (crtc && crtc->state->active) > mode = DRM_MODE_DPMS_ON; > > connector->dpms = mode; > - drm_object_property_set_value(&connector->base, > - dpms_prop, mode); > } > } > > diff --git a/drivers/gpu/drm/drm_connector.c > b/drivers/gpu/drm/drm_connector.c index 8072e6e4c62c..349104eadefe 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1225,8 +1225,7 @@ int drm_mode_connector_set_obj_prop(struct > drm_mode_object *obj, } else if (connector->funcs->set_property) > ret = connector->funcs->set_property(connector, property, value); > > - /* store the property value if successful */ > - if (!ret) > + if (!ret && drm_drv_uses_atomic_modeset(property->dev)) > drm_object_property_set_value(&connector->base, property, value); > return ret; > } > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 5af25ce5bf7c..7d4fcdd34342 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -736,7 +736,7 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object > *obj, > > if (crtc->funcs->set_property) > ret = crtc->funcs->set_property(crtc, property, value); > - if (!ret) > + if (!ret && drm_drv_uses_atomic_modeset(property->dev)) > drm_object_property_set_value(obj, property, value); > > return ret; > diff --git a/drivers/gpu/drm/drm_mode_object.c > b/drivers/gpu/drm/drm_mode_object.c index da9a9adbcc98..92743a796bf0 100644 > --- a/drivers/gpu/drm/drm_mode_object.c > +++ b/drivers/gpu/drm/drm_mode_object.c > @@ -233,6 +233,9 @@ int drm_object_property_set_value(struct drm_mode_object > *obj, { > int i; > > + WARN_ON(drm_drv_uses_atomic_modeset(property->dev) && > + !(property->flags & DRM_MODE_PROP_IMMUTABLE)); It would have been nice to remove the calls to drm_object_property_set_value() for the dpms property from drivers before adding this :-/ Three drivers (rcar- du, shmobile and fsl-dcu) initialize the connector's DPMS property to OFF using this call (the default being ON). Following the DPMS code paths always give me a headache, so if you know by heart how I should replace the set property call, I'm all ears :-) > for (i = 0; i < obj->properties->count; i++) { > if (obj->properties->properties[i] == property) { > obj->properties->values[i] = val; > @@ -244,24 +247,7 @@ int drm_object_property_set_value(struct > drm_mode_object *obj, } > EXPORT_SYMBOL(drm_object_property_set_value); > > -/** > - * drm_object_property_get_value - retrieve the value of a property > - * @obj: drm mode object to get property value from > - * @property: property to retrieve > - * @val: storage for the property value > - * > - * This function retrieves the softare state of the given property for the > given - * property. Since there is no driver callback to retrieve the > current property - * value this might be out of sync with the hardware, > depending upon the driver - * and property. > - * > - * Atomic drivers should never call this function directly, the core will > read - * out property values through the various ->atomic_get_property > callbacks. - * > - * Returns: > - * Zero on success, error code on failure. > - */ > -int drm_object_property_get_value(struct drm_mode_object *obj, > +int __drm_object_property_get_value(struct drm_mode_object *obj, > struct drm_property *property, uint64_t *val) > { > int i; > @@ -284,6 +270,31 @@ int drm_object_property_get_value(struct > drm_mode_object *obj, > > return -EINVAL; > } > + > +/** > + * drm_object_property_get_value - retrieve the value of a property > + * @obj: drm mode object to get property value from > + * @property: property to retrieve > + * @val: storage for the property value > + * > + * This function retrieves the softare state of the given property for the > given + * property. Since there is no driver callback to retrieve the > current property + * value this might be out of sync with the hardware, > depending upon the driver + * and property. > + * > + * Atomic drivers should never call this function directly, the core will > read + * out property values through the various ->atomic_get_property > callbacks. + * > + * Returns: > + * Zero on success, error code on failure. > + */ > +int drm_object_property_get_value(struct drm_mode_object *obj, > + struct drm_property *property, uint64_t *val) > +{ > + WARN_ON(drm_drv_uses_atomic_modeset(property->dev)); > + > + return __drm_object_property_get_value(obj, property, val); > +} > EXPORT_SYMBOL(drm_object_property_get_value); > > /* helper for getconnector and getproperties ioctls */ > @@ -302,7 +313,7 @@ int drm_mode_object_get_properties(struct > drm_mode_object *obj, bool atomic, continue; > > if (*arg_count_props > count) { > - ret = drm_object_property_get_value(obj, prop, &val); > + ret = __drm_object_property_get_value(obj, prop, &val); > if (ret) > return ret; > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 5dc8c4350602..b4ffd0455200 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -330,7 +330,7 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, > > if (plane->funcs->set_property) > ret = plane->funcs->set_property(plane, property, value); > - if (!ret) > + if (!ret && drm_drv_uses_atomic_modeset(property->dev)) > drm_object_property_set_value(obj, property, value); > > return ret;
On Sat, Aug 12, 2017 at 12:20 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Tuesday 25 Jul 2017 10:01:16 Daniel Vetter wrote: >> diff --git a/drivers/gpu/drm/drm_mode_object.c >> b/drivers/gpu/drm/drm_mode_object.c index da9a9adbcc98..92743a796bf0 100644 >> --- a/drivers/gpu/drm/drm_mode_object.c >> +++ b/drivers/gpu/drm/drm_mode_object.c >> @@ -233,6 +233,9 @@ int drm_object_property_set_value(struct drm_mode_object >> *obj, { >> int i; >> >> + WARN_ON(drm_drv_uses_atomic_modeset(property->dev) && >> + !(property->flags & DRM_MODE_PROP_IMMUTABLE)); > > It would have been nice to remove the calls to drm_object_property_set_value() > for the dpms property from drivers before adding this :-/ Three drivers (rcar- > du, shmobile and fsl-dcu) initialize the connector's DPMS property to OFF > using this call (the default being ON). > > Following the DPMS code paths always give me a headache, so if you know by > heart how I should replace the set property call, I'm all ears :-) Remove, it doesn't do anything for kms drivers. The value these calls update was never consulted when reading the property, instead drm_atomic_connector_get_property() directly looks at drm_connector->dpms. These calls where only needed for legacy modeset. Also note that the default _ON is the right one, since DPMS state is in addition to modeset state. dpms should actually be set to _ON when you enable the display using a modeset call, atomic takes care of that in drm_atomic_helper_update_legacy_modeset_state(). Aka we found a using this WARN_ON! And if you think the above explanation should be somewhere in kerneldoc, pls add :-) -Daniel
On Monday 14 Aug 2017 09:25:47 Daniel Vetter wrote: > On Sat, Aug 12, 2017 at 12:20 AM, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > On Tuesday 25 Jul 2017 10:01:16 Daniel Vetter wrote: > >> diff --git a/drivers/gpu/drm/drm_mode_object.c > >> b/drivers/gpu/drm/drm_mode_object.c index da9a9adbcc98..92743a796bf0 > >> 100644 > >> --- a/drivers/gpu/drm/drm_mode_object.c > >> +++ b/drivers/gpu/drm/drm_mode_object.c > >> @@ -233,6 +233,9 @@ int drm_object_property_set_value(struct > >> drm_mode_object *obj, { > >> > >> int i; > >> > >> + WARN_ON(drm_drv_uses_atomic_modeset(property->dev) && > >> + !(property->flags & DRM_MODE_PROP_IMMUTABLE)); > > > > It would have been nice to remove the calls to > > drm_object_property_set_value() for the dpms property from drivers before > > adding this :-/ Three drivers (rcar- du, shmobile and fsl-dcu) initialize > > the connector's DPMS property to OFF using this call (the default being > > ON). > > > > Following the DPMS code paths always give me a headache, so if you know by > > heart how I should replace the set property call, I'm all ears :-) > > Remove, it doesn't do anything for kms drivers. The value these calls > update was never consulted when reading the property, instead > drm_atomic_connector_get_property() directly looks at > drm_connector->dpms. But DRM_IOCTL_MODE_OBJ_GETPROPERTIES doesn't, it gets the property value directly, doesn't it ? > These calls where only needed for legacy modeset. Also note that the default > _ON is the right one, since DPMS state is in addition to modeset state. dpms > should actually be set to _ON when you enable the display using a modeset > call, atomic takes care of that in > drm_atomic_helper_update_legacy_modeset_state(). Sure, but the connectors should start disabled, shouldn't they ? > Aka we found a using this WARN_ON! > > And if you think the above explanation should be somewhere in > kerneldoc, pls add :-)
On Mon, Aug 14, 2017 at 01:32:07PM +0300, Laurent Pinchart wrote: > On Monday 14 Aug 2017 09:25:47 Daniel Vetter wrote: > > On Sat, Aug 12, 2017 at 12:20 AM, Laurent Pinchart > > > > <laurent.pinchart@ideasonboard.com> wrote: > > > On Tuesday 25 Jul 2017 10:01:16 Daniel Vetter wrote: > > >> diff --git a/drivers/gpu/drm/drm_mode_object.c > > >> b/drivers/gpu/drm/drm_mode_object.c index da9a9adbcc98..92743a796bf0 > > >> 100644 > > >> --- a/drivers/gpu/drm/drm_mode_object.c > > >> +++ b/drivers/gpu/drm/drm_mode_object.c > > >> @@ -233,6 +233,9 @@ int drm_object_property_set_value(struct > > >> drm_mode_object *obj, { > > >> > > >> int i; > > >> > > >> + WARN_ON(drm_drv_uses_atomic_modeset(property->dev) && > > >> + !(property->flags & DRM_MODE_PROP_IMMUTABLE)); > > > > > > It would have been nice to remove the calls to > > > drm_object_property_set_value() for the dpms property from drivers before > > > adding this :-/ Three drivers (rcar- du, shmobile and fsl-dcu) initialize > > > the connector's DPMS property to OFF using this call (the default being > > > ON). > > > > > > Following the DPMS code paths always give me a headache, so if you know by > > > heart how I should replace the set property call, I'm all ears :-) > > > > Remove, it doesn't do anything for kms drivers. The value these calls > > update was never consulted when reading the property, instead > > drm_atomic_connector_get_property() directly looks at > > drm_connector->dpms. > > But DRM_IOCTL_MODE_OBJ_GETPROPERTIES doesn't, it gets the property value > directly, doesn't it ? It redirects to the atomic machinery, so should all work out. > > These calls where only needed for legacy modeset. Also note that the default > > _ON is the right one, since DPMS state is in addition to modeset state. dpms > > should actually be set to _ON when you enable the display using a modeset > > call, atomic takes care of that in > > drm_atomic_helper_update_legacy_modeset_state(). > > Sure, but the connectors should start disabled, shouldn't they ? Sure, and they are, because connector_state->crtc == NULL. DPMS only matters if your connector is connected, which at boot-up/reset state it isnt. Yes DPMS is one of those thing which with hindsight I'd like to never have happened, it's probably the most quirky uabi we have :-/ If you have userspace that gets confused by this we need to: a) either fix your userspace b) if we decide to fix the kernel, fix it for everyone (i.e. in drm_connector_init). Given that the "standard kms driver" (aka i915.ko) doesn't do that, I don't think we need b). -Daniel
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 7582bbc5decc..4a960c741e35 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -921,16 +921,12 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, crtc = new_conn_state->crtc; if ((!crtc && old_conn_state->crtc) || (crtc && drm_atomic_crtc_needs_modeset(crtc->state))) { - struct drm_property *dpms_prop = - dev->mode_config.dpms_property; int mode = DRM_MODE_DPMS_OFF; if (crtc && crtc->state->active) mode = DRM_MODE_DPMS_ON; connector->dpms = mode; - drm_object_property_set_value(&connector->base, - dpms_prop, mode); } } diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 8072e6e4c62c..349104eadefe 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1225,8 +1225,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj, } else if (connector->funcs->set_property) ret = connector->funcs->set_property(connector, property, value); - /* store the property value if successful */ - if (!ret) + if (!ret && drm_drv_uses_atomic_modeset(property->dev)) drm_object_property_set_value(&connector->base, property, value); return ret; } diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 5af25ce5bf7c..7d4fcdd34342 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -736,7 +736,7 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj, if (crtc->funcs->set_property) ret = crtc->funcs->set_property(crtc, property, value); - if (!ret) + if (!ret && drm_drv_uses_atomic_modeset(property->dev)) drm_object_property_set_value(obj, property, value); return ret; diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index da9a9adbcc98..92743a796bf0 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -233,6 +233,9 @@ int drm_object_property_set_value(struct drm_mode_object *obj, { int i; + WARN_ON(drm_drv_uses_atomic_modeset(property->dev) && + !(property->flags & DRM_MODE_PROP_IMMUTABLE)); + for (i = 0; i < obj->properties->count; i++) { if (obj->properties->properties[i] == property) { obj->properties->values[i] = val; @@ -244,24 +247,7 @@ int drm_object_property_set_value(struct drm_mode_object *obj, } EXPORT_SYMBOL(drm_object_property_set_value); -/** - * drm_object_property_get_value - retrieve the value of a property - * @obj: drm mode object to get property value from - * @property: property to retrieve - * @val: storage for the property value - * - * This function retrieves the softare state of the given property for the given - * property. Since there is no driver callback to retrieve the current property - * value this might be out of sync with the hardware, depending upon the driver - * and property. - * - * Atomic drivers should never call this function directly, the core will read - * out property values through the various ->atomic_get_property callbacks. - * - * Returns: - * Zero on success, error code on failure. - */ -int drm_object_property_get_value(struct drm_mode_object *obj, +int __drm_object_property_get_value(struct drm_mode_object *obj, struct drm_property *property, uint64_t *val) { int i; @@ -284,6 +270,31 @@ int drm_object_property_get_value(struct drm_mode_object *obj, return -EINVAL; } + +/** + * drm_object_property_get_value - retrieve the value of a property + * @obj: drm mode object to get property value from + * @property: property to retrieve + * @val: storage for the property value + * + * This function retrieves the softare state of the given property for the given + * property. Since there is no driver callback to retrieve the current property + * value this might be out of sync with the hardware, depending upon the driver + * and property. + * + * Atomic drivers should never call this function directly, the core will read + * out property values through the various ->atomic_get_property callbacks. + * + * Returns: + * Zero on success, error code on failure. + */ +int drm_object_property_get_value(struct drm_mode_object *obj, + struct drm_property *property, uint64_t *val) +{ + WARN_ON(drm_drv_uses_atomic_modeset(property->dev)); + + return __drm_object_property_get_value(obj, property, val); +} EXPORT_SYMBOL(drm_object_property_get_value); /* helper for getconnector and getproperties ioctls */ @@ -302,7 +313,7 @@ int drm_mode_object_get_properties(struct drm_mode_object *obj, bool atomic, continue; if (*arg_count_props > count) { - ret = drm_object_property_get_value(obj, prop, &val); + ret = __drm_object_property_get_value(obj, prop, &val); if (ret) return ret; diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 5dc8c4350602..b4ffd0455200 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -330,7 +330,7 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, if (plane->funcs->set_property) ret = plane->funcs->set_property(plane, property, value); - if (!ret) + if (!ret && drm_drv_uses_atomic_modeset(property->dev)) drm_object_property_set_value(obj, property, value); return ret;
Atomic drivers only use the property value store for immutable (i.e. can't be set by userspace, but the kernel can still adjust it) properties. The only tricky part is the removal of the update in drm_atomic_helper_update_legacy_modeset_state(). This was added in commit 8c10342cb48f3140d9abeadcfd2fa6625d447282 (tag: topic/drm-misc-2015-07-28) Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Date: Mon Jul 27 13:24:29 2015 +0200 drm/atomic: Update legacy DPMS state during modesets, v3. by copying it from the i915 code, where it was originally added in commit 68d3472047a572936551f8ff0b6f4016c5a1fdef Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Thu Sep 6 22:08:35 2012 +0200 drm/i915: update dpms property in set_mode for the legacy modeset code. The reason we needed this hack was that i915 didn't yet set DRIVER_ATOMIC, and we checked for that instead of the newer-ish drm_drv_uses_atomic_modeset(), which avoids such troubles. With the correct feature checks this isn't needed anymore at all. Also make sure that drivers don't accidentally get this wrong by making the exported version of drm_object_property_get_value() only work for legacy drivers. Only gma500 uses it anyway. Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 4 --- drivers/gpu/drm/drm_connector.c | 3 +-- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_mode_object.c | 49 +++++++++++++++++++++++-------------- drivers/gpu/drm/drm_plane.c | 2 +- 5 files changed, 33 insertions(+), 27 deletions(-)