[PATCH 04/13] drm: add atomic_set_property wrappers
Daniel Vetter
daniel at ffwll.ch
Wed Dec 17 05:43:44 PST 2014
On Tue, Dec 16, 2014 at 06:05:32PM -0500, Rob Clark wrote:
> As we add properties for all the standard plane/crtc/connector
> attributes (in preperation for the atomic ioctl), we are going to want
> to handle core state in core (rather than per driver). Intercepting the
> core properties will be easier if the atomic_set_property vfuncs are not
> called directly, but instead have a mandatory wrapper function (which
> will later serve as the point to intercept core properties).
>
> Signed-off-by: Rob Clark <robdclark at gmail.com>
Two suggestions for comment improvements below. With that addressed:
Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> drivers/gpu/drm/drm_atomic.c | 78 +++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_atomic_helper.c | 12 +++---
> include/drm/drm_atomic.h | 9 +++++
> include/drm/drm_crtc.h | 3 ++
> 4 files changed, 96 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index ff5f034..1261ade 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -217,6 +217,28 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
> EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>
> /**
> + * drm_atomic_crtc_set_property - set property on connector
> + * @crtc: the drm CRTC to set a property on
> + * @state: the state object to update with the new property value
> + * @property: the property to set
> + * @val: the new property value
> + *
> + * Use this instead of calling crtc->atomic_set_property directly
Maybe clarify a bit more here. What about
"This function directly processes generic properties (both mandatory and
optional ones) and calls into the driver's ->atomic_set_property hook for
anything else. To esnure consistent behavior this wrapper must be used
instead of calling the driver hook directly, even for driver private
properties."
A bit much perhaps, but kerneldoc is cheap ;-)
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> + struct drm_crtc_state *state, struct drm_property *property,
> + uint64_t val)
> +{
> + if (crtc->funcs->atomic_set_property)
> + return crtc->funcs->atomic_set_property(crtc, state, property, val);
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_atomic_crtc_set_property);
> +
> +/**
> * drm_atomic_get_plane_state - get plane state
> * @state: global atomic state object
> * @plane: plane to get state object for
> @@ -272,6 +294,28 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
> EXPORT_SYMBOL(drm_atomic_get_plane_state);
>
> /**
> + * drm_atomic_plane_set_property - set property on plane
> + * @plane: the drm plane to set a property on
> + * @state: the state object to update with the new property value
> + * @property: the property to set
> + * @val: the new property value
> + *
> + * Use this instead of calling plane->atomic_set_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_plane_set_property(struct drm_plane *plane,
> + struct drm_plane_state *state, struct drm_property *property,
> + uint64_t val)
> +{
> + if (plane->funcs->atomic_set_property)
> + return plane->funcs->atomic_set_property(plane, state, property, val);
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_atomic_plane_set_property);
> +
> +/**
> * drm_atomic_get_connector_state - get connector state
> * @state: global atomic state object
> * @connector: connector to get state object for
> @@ -343,6 +387,40 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
> EXPORT_SYMBOL(drm_atomic_get_connector_state);
>
> /**
> + * drm_atomic_connector_set_property - set property on connector.
> + * @connector: the drm connector to set a property on
> + * @state: the state object to update with the new property value
> + * @property: the property to set
> + * @val: the new property value
> + *
> + * Use this instead of calling connector->atomic_set_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_connector_set_property(struct drm_connector *connector,
> + struct drm_connector_state *state, struct drm_property *property,
> + uint64_t val)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_mode_config *config = &dev->mode_config;
> +
> + if (property == config->dpms_property) {
> + /* setting DPMS property requires special handling, which
> + * is done in legacy setprop path for us. Disallow (for
> + * now?) atomic writes to DPMS property:
> + */
Argh, I still need to write those patches for dpms in atomic. But imo a
better comment would be:
/*
* Per-connector is a legacy-only feature, disallow for atomic
* updates.
*/
I'll extend the comment once the atomic dpms is there.
> + return -EINVAL;
> + } else if (connector->funcs->atomic_set_property) {
> + return connector->funcs->atomic_set_property(connector,
> + state, property, val);
> + } else {
> + return -EINVAL;
> + }
> +}
> +EXPORT_SYMBOL(drm_atomic_connector_set_property);
> +
> +/**
> * drm_atomic_set_crtc_for_plane - set crtc for plane
> * @state: the incoming atomic state
> * @plane: the plane whose incoming state to update
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4a78a77..d42fdb1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1558,8 +1558,8 @@ retry:
> goto fail;
> }
>
> - ret = crtc->funcs->atomic_set_property(crtc, crtc_state,
> - property, val);
> + ret = drm_atomic_crtc_set_property(crtc, crtc_state,
> + property, val);
> if (ret)
> goto fail;
>
> @@ -1617,8 +1617,8 @@ retry:
> goto fail;
> }
>
> - ret = plane->funcs->atomic_set_property(plane, plane_state,
> - property, val);
> + ret = drm_atomic_plane_set_property(plane, plane_state,
> + property, val);
> if (ret)
> goto fail;
>
> @@ -1676,8 +1676,8 @@ retry:
> goto fail;
> }
>
> - ret = connector->funcs->atomic_set_property(connector, connector_state,
> - property, val);
> + ret = drm_atomic_connector_set_property(connector, connector_state,
> + property, val);
> if (ret)
> goto fail;
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index ad22295..b0834dc 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -38,12 +38,21 @@ void drm_atomic_state_free(struct drm_atomic_state *state);
> struct drm_crtc_state * __must_check
> drm_atomic_get_crtc_state(struct drm_atomic_state *state,
> struct drm_crtc *crtc);
> +int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> + struct drm_crtc_state *state, struct drm_property *property,
> + uint64_t val);
> struct drm_plane_state * __must_check
> drm_atomic_get_plane_state(struct drm_atomic_state *state,
> struct drm_plane *plane);
> +int drm_atomic_plane_set_property(struct drm_plane *plane,
> + struct drm_plane_state *state, struct drm_property *property,
> + uint64_t val);
> struct drm_connector_state * __must_check
> drm_atomic_get_connector_state(struct drm_atomic_state *state,
> struct drm_connector *connector);
> +int drm_atomic_connector_set_property(struct drm_connector *connector,
> + struct drm_connector_state *state, struct drm_property *property,
> + uint64_t val);
>
> int __must_check
> drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 61ab3e5..1b71f60 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -300,6 +300,7 @@ struct drm_crtc_state {
> * @atomic_duplicate_state: duplicate the atomic state for this CRTC
> * @atomic_destroy_state: destroy an atomic state for this CRTC
> * @atomic_set_property: set a property on an atomic state for this CRTC
> + * (do not call directly, use drm_atomic_crtc_set_property())
> *
> * 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
> @@ -483,6 +484,7 @@ struct drm_connector_state {
> * @atomic_duplicate_state: duplicate the atomic state for this connector
> * @atomic_destroy_state: destroy an atomic state for this connector
> * @atomic_set_property: set a property on an atomic state for this connector
> + * (do not call directly, use drm_atomic_connector_set_property())
> *
> * Each CRTC may have one or more connectors attached to it. The functions
> * below allow the core DRM code to control connectors, enumerate available modes,
> @@ -743,6 +745,7 @@ struct drm_plane_state {
> * @atomic_duplicate_state: duplicate the atomic state for this plane
> * @atomic_destroy_state: destroy an atomic state for this plane
> * @atomic_set_property: set a property on an atomic state for this plane
> + * (do not call directly, use drm_atomic_plane_set_property())
> */
> struct drm_plane_funcs {
> int (*update_plane)(struct drm_plane *plane,
> --
> 2.1.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list