Re: [PATCH 3/8] drm: Handle properties in the core for atomic drivers
On 07/25/2017 01:31 PM, Daniel Vetter wrote: The reason behind the original indirection through the helper functions was to allow existing drivers to overwrite how they handle properties. For example when a vendor-specific userspace had expectations that didn't match atomic. That seemed likely, since atomic is standardizing a _lot_ more of the behaviour of a kms driver. But 20 drivers later there's no such need at all. Worse, this forces all drivers to hook up the default behaviour, breaking userspace if they forget to do that. And it forces us to export a bunch of core function just for those helpers. And finally, these helpers are the last places using drm_atomic_legacy_backoff() and the implicit acquire_ctx. This patch here just implements the new behaviour and updates the docs. Follow-up patches will garbage-collect all the dead code. v2: Fixup docs even better! v3: Make it actually work ... Reviewed-by: Archit TanejaSigned-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic.c| 60 ++-- drivers/gpu/drm/drm_connector.c | 6 +- drivers/gpu/drm/drm_crtc_helper.c | 3 +- drivers/gpu/drm/drm_crtc_internal.h | 7 +++ drivers/gpu/drm/drm_mode_object.c | 110 +++- include/drm/drm_connector.h | 10 ++-- include/drm/drm_crtc.h | 6 +- include/drm/drm_plane.h | 6 +- 8 files changed, 158 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 01192dd3ed79..0fd14aff7add 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1864,9 +1864,60 @@ static struct drm_pending_vblank_event *create_vblank_event( return e; } -static int atomic_set_prop(struct drm_atomic_state *state, - struct drm_mode_object *obj, struct drm_property *prop, - uint64_t prop_value) +int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state, +struct drm_connector *connector, +int mode) +{ + struct drm_connector *tmp_connector; + struct drm_connector_state *new_conn_state; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + int i, ret, old_mode = connector->dpms; + bool active = false; + + ret = drm_modeset_lock(>dev->mode_config.connection_mutex, + state->acquire_ctx); + if (ret) + return ret; + + if (mode != DRM_MODE_DPMS_ON) + mode = DRM_MODE_DPMS_OFF; + connector->dpms = mode; + + crtc = connector->state->crtc; + if (!crtc) + goto out; + ret = drm_atomic_add_affected_connectors(state, crtc); + if (ret) + goto out; + + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) { + ret = PTR_ERR(crtc_state); + goto out; + } + + for_each_new_connector_in_state(state, tmp_connector, new_conn_state, i) { + if (new_conn_state->crtc != crtc) + continue; + if (tmp_connector->dpms == DRM_MODE_DPMS_ON) { + active = true; + break; + } + } + + crtc_state->active = active; + ret = drm_atomic_commit(state); +out: + if (ret != 0) + connector->dpms = old_mode; + return ret; +} + +int drm_atomic_set_property(struct drm_atomic_state *state, + struct drm_mode_object *obj, + struct drm_property *prop, + uint64_t prop_value) { struct drm_mode_object *ref; int ret; @@ -2286,7 +2337,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, goto out; } - ret = atomic_set_prop(state, obj, prop, prop_value); + ret = drm_atomic_set_property(state, obj, prop, + prop_value); if (ret) { drm_mode_object_put(obj); goto out; diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 349104eadefe..12371f184019 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -717,9 +717,9 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name, *drivers, it remaps to controlling the "ACTIVE" property on the CRTC the *connector is linked to. Drivers should never set this property directly, *it is handled by the DRM core by calling the _connector_funcs.dpms - * callback. Atomic drivers should implement this hook using - * drm_atomic_helper_connector_dpms(). This is the only property standard - * connector
[PATCH 3/8] drm: Handle properties in the core for atomic drivers
The reason behind the original indirection through the helper functions was to allow existing drivers to overwrite how they handle properties. For example when a vendor-specific userspace had expectations that didn't match atomic. That seemed likely, since atomic is standardizing a _lot_ more of the behaviour of a kms driver. But 20 drivers later there's no such need at all. Worse, this forces all drivers to hook up the default behaviour, breaking userspace if they forget to do that. And it forces us to export a bunch of core function just for those helpers. And finally, these helpers are the last places using drm_atomic_legacy_backoff() and the implicit acquire_ctx. This patch here just implements the new behaviour and updates the docs. Follow-up patches will garbage-collect all the dead code. v2: Fixup docs even better! v3: Make it actually work ... Signed-off-by: Daniel Vetter--- drivers/gpu/drm/drm_atomic.c| 60 ++-- drivers/gpu/drm/drm_connector.c | 6 +- drivers/gpu/drm/drm_crtc_helper.c | 3 +- drivers/gpu/drm/drm_crtc_internal.h | 7 +++ drivers/gpu/drm/drm_mode_object.c | 110 +++- include/drm/drm_connector.h | 10 ++-- include/drm/drm_crtc.h | 6 +- include/drm/drm_plane.h | 6 +- 8 files changed, 158 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 01192dd3ed79..0fd14aff7add 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1864,9 +1864,60 @@ static struct drm_pending_vblank_event *create_vblank_event( return e; } -static int atomic_set_prop(struct drm_atomic_state *state, - struct drm_mode_object *obj, struct drm_property *prop, - uint64_t prop_value) +int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state, +struct drm_connector *connector, +int mode) +{ + struct drm_connector *tmp_connector; + struct drm_connector_state *new_conn_state; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + int i, ret, old_mode = connector->dpms; + bool active = false; + + ret = drm_modeset_lock(>dev->mode_config.connection_mutex, + state->acquire_ctx); + if (ret) + return ret; + + if (mode != DRM_MODE_DPMS_ON) + mode = DRM_MODE_DPMS_OFF; + connector->dpms = mode; + + crtc = connector->state->crtc; + if (!crtc) + goto out; + ret = drm_atomic_add_affected_connectors(state, crtc); + if (ret) + goto out; + + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) { + ret = PTR_ERR(crtc_state); + goto out; + } + + for_each_new_connector_in_state(state, tmp_connector, new_conn_state, i) { + if (new_conn_state->crtc != crtc) + continue; + if (tmp_connector->dpms == DRM_MODE_DPMS_ON) { + active = true; + break; + } + } + + crtc_state->active = active; + ret = drm_atomic_commit(state); +out: + if (ret != 0) + connector->dpms = old_mode; + return ret; +} + +int drm_atomic_set_property(struct drm_atomic_state *state, + struct drm_mode_object *obj, + struct drm_property *prop, + uint64_t prop_value) { struct drm_mode_object *ref; int ret; @@ -2286,7 +2337,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, goto out; } - ret = atomic_set_prop(state, obj, prop, prop_value); + ret = drm_atomic_set_property(state, obj, prop, + prop_value); if (ret) { drm_mode_object_put(obj); goto out; diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 349104eadefe..12371f184019 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -717,9 +717,9 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name, * drivers, it remaps to controlling the "ACTIVE" property on the CRTC the * connector is linked to. Drivers should never set this property directly, * it is handled by the DRM core by calling the _connector_funcs.dpms - * callback. Atomic drivers should implement this hook using - * drm_atomic_helper_connector_dpms(). This is the only property standard - * connector property that userspace can change. + * callback. For atomic drivers the remapping to the "ACTIVE"