Re: [PATCH 11/11] drm: atomic: Add MODE_ID property

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote:
> Atomic modesetting: now with modesetting support.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> Tested-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/drm_atomic.c | 12 +++++++++++-
>  drivers/gpu/drm/drm_crtc.c   |  7 +++++++
>  include/drm/drm_crtc.h       |  1 +
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 63fc58a..a4ab03a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -415,10 +415,18 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> +	int ret;
>  
> -	/* FIXME: Mode prop is missing, which also controls ->enable. */
>  	if (property == config->prop_active)
>  		state->active = val;
> +	else if (property == config->prop_mode_id) {
> +		struct drm_property_blob *mode =
> +			drm_property_lookup_blob(dev, val);
> +		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
> +		if (mode)
> +			drm_property_unreference_blob(mode);

Hm, maybe I need to revisit whether auto-clamping ->active is a good idea.
We need it for legacy helpers, but for atomic userspace this code means
depending upon whether active or mode_id is first in the prop list it will
get clamped or not, which isn't awesome.

Imo that's a good reason to remove the ->active clamping from
set_mode_pop_for_crtc. I guess we can keep it for set_mode_for_crtc since
that's only used internally and in legacy paths. Perhaps with a comment as
to why (and why not in set_mode_prop).

> +		return ret;
> +	}
>  	else if (crtc->funcs->atomic_set_property)
>  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>  	else
> @@ -443,6 +451,8 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  
>  	if (property == config->prop_active)
>  		*val = state->active;
> +	else if (property == config->prop_mode_id)
> +		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
>  	else if (crtc->funcs->atomic_get_property)
>  		return crtc->funcs->atomic_get_property(crtc, state, property, val);
>  	else
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index ee87045..8f18412 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -688,6 +688,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&crtc->base, config->prop_active, 0);
> +		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>  	}
>  
>  	return 0;
> @@ -1454,6 +1455,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_active = prop;
>  
> +	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> +			"MODE_ID", DRM_MODE_OBJECT_BLOB);

Ah, here we go. Why not DRM_MODE_PROP_BLOB? Imo confusing to userspace
that some prop blobs are objects and some are old blob props.
-Daniel

> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_mode_id = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c54fa4a..3b4d8a4 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1146,6 +1146,7 @@ struct drm_mode_config {
>  	struct drm_property *prop_fb_id;
>  	struct drm_property *prop_crtc_id;
>  	struct drm_property *prop_active;
> +	struct drm_property *prop_mode_id;
>  
>  	/* DVI-I properties */
>  	struct drm_property *dvi_i_subconnector_property;
> -- 
> 2.4.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel