Re: [PATCH 10/11] drm/atomic: Add current-mode blob to CRTC state

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, May 22, 2015 at 01:34:53PM +0100, Daniel Stone wrote:
> Add a blob property tracking the current mode to the CRTC state, and
> ensure it is properly updated and referenced.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> Tested-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 62 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++---
>  drivers/gpu/drm/drm_crtc.c          |  3 +-
>  include/drm/drm_crtc.h              |  3 ++
>  4 files changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 67c251f..63fc58a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -301,6 +301,8 @@ EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  				 struct drm_display_mode *mode)
>  {
> +	struct drm_mode_modeinfo umode;
> +
>  	/* Early return for no change. */
>  	if (!mode && !state->enable)
>  		return 0;
> @@ -308,9 +310,20 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  	    memcmp(&state->mode, mode, sizeof(*mode)) == 0)
>  		return 0;
>  
> +	if (state->mode_blob)
> +		drm_property_unreference_blob(state->mode_blob);
> +	state->mode_blob = NULL;
>  	state->mode_changed = true;
>  
>  	if (mode) {
> +		drm_mode_convert_to_umode(&umode, mode);
> +		state->mode_blob =
> +			drm_property_create_blob(state->crtc->dev,
> +		                                 sizeof(umode),
> +		                                 &umode);
> +		if (!state->mode_blob)
> +			return -ENOMEM;

IS_ERR and PTR_ERR? Dan Carpenter will catch this as soon as I'll apply
;-)

> +
>  		drm_mode_copy(&state->mode, mode);
>  		state->enable = true;
>  	} else {
> @@ -330,6 +343,55 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
>  
> +/**
> + * drm_atomic_set_mode_prop_for_crtc - set mode for CRTC
> + * @state: the CRTC whose incoming state to update
> + * @blob: pointer to blob property to use for mode
> + *
> + * Set a mode (originating from a blob property) on the desired CRTC state.
> + * This function will take a reference on the blob property for the CRTC state,
> + * and release the reference held on the state's existing mode property, if any
> + * was set.
> + */
> +int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
> +                                      struct drm_property_blob *blob)
> +{
> +	if (blob == state->mode_blob)
> +		return 0;
> +
> +	if (state->mode_blob)
> +		drm_property_unreference_blob(state->mode_blob);
> +	state->mode_blob = NULL;
> +
> +	if (blob) {
> +		if (blob->length != sizeof(struct drm_mode_modeinfo) ||
> +		    drm_mode_convert_umode(&state->mode,
> +		                           (const struct drm_mode_modeinfo *)
> +		                            blob->data))
> +			return -EINVAL;
> +
> +		state->mode_blob = drm_property_reference_blob(blob);
> +		state->mode_changed = true;
> +		state->enable = true;
> +	} else {
> +		memset(&state->mode, 0, sizeof(state->mode));
> +
> +		if (state->enable)
> +			state->mode_changed = true;

Same comment as in the previous patch: mode_changed isnt' the core atomic
code's business.

> +		state->enable = false;
> +		state->active = false;
> +	}
> +
> +	if (state->enable)
> +		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> +				 state->mode.name, state);
> +	else
> +		DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n",
> +				 state);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc);
>  
>  /**
>   * drm_atomic_crtc_set_property - set property on CRTC
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 70ce243..5d596d9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2027,6 +2027,8 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
>   */
>  void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
>  {
> +	if (crtc->state && crtc->state->mode_blob)
> +		drm_property_unreference_blob(crtc->state->mode_blob);
>  	kfree(crtc->state);
>  	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
>  
> @@ -2048,6 +2050,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>  {
>  	memcpy(state, crtc->state, sizeof(*state));
>  
> +	if (state->mode_blob)
> +		drm_property_reference_blob(state->mode_blob);
>  	state->mode_changed = false;
>  	state->active_changed = false;
>  	state->planes_changed = false;
> @@ -2090,11 +2094,8 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
>  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
>  					    struct drm_crtc_state *state)
>  {
> -	/*
> -	 * This is currently a placeholder so that drivers that subclass the
> -	 * state will automatically do the right thing if code is ever added
> -	 * to this function.
> -	 */
> +	if (state->mode_blob)
> +		drm_property_unreference_blob(state->mode_blob);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 9263b22..ee87045 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1961,7 +1961,8 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  		crtc_resp->x = crtc->primary->state->src_x >> 16;
>  		crtc_resp->y = crtc->primary->state->src_y >> 16;
>  		if (crtc->state->enable) {
> -			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
> +			memcpy(&crtc_resp->mode, &crtc->state->mode_blob->data,
> +			       sizeof(crtc_resp->mode));

Why this change? While transitioning I'd expect that some driver (probably
the one with hw state readout) will get this wrong, imo safer to keep
looking at crtc->state->mode.

lgtm otherwise.
-Daniel

>  			crtc_resp->mode_valid = 1;
>  
>  		} else {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 72b60db..c54fa4a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -299,6 +299,9 @@ struct drm_crtc_state {
>  
>  	struct drm_display_mode mode;
>  
> +	/* blob property to expose current mode to atomic userspace */
> +	struct drm_property_blob *mode_blob;
> +
>  	struct drm_pending_vblank_event *event;
>  
>  	struct drm_atomic_state *state;
> -- 
> 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