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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> 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;
--
Regards,
Laurent Pinchart
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
