Re: [PATCH 1/2] drm/vmwgfx: Fix up the implicit display unit handling

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 04, 2018 at 10:38:17PM +0000, Thomas Hellstrom wrote:
> Make the connector is_implicit property immutable.
> As far as we know, no user-space application is writing to it.
> 
> Also move the verification that all implicit display units scan out
> from the same framebuffer to atomic_check().
> 
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> Reviewed-by: Sinclair Yeh <syeh@vmware.com>
> Reviewed-by: Deepak Rawat <drawat@vmware.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h  |   2 -
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 230 ++++++++++++++---------------------
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |  16 +--
>  drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c  |   6 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |  38 +-----
>  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |  43 +------
>  6 files changed, 102 insertions(+), 233 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 59f614225bcd..ea2c22d92357 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -486,8 +486,6 @@ struct vmw_private {
>  	struct vmw_overlay *overlay_priv;
>  	struct drm_property *hotplug_mode_update_property;
>  	struct drm_property *implicit_placement_property;
> -	unsigned num_implicit;
> -	struct vmw_framebuffer *implicit_fb;
>  	struct mutex global_kms_state_mutex;
>  	spinlock_t cursor_lock;
>  	struct drm_atomic_state *suspend_state;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 05fb16733c5c..ccaec2cbabd2 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -456,21 +456,8 @@ int vmw_du_primary_plane_atomic_check(struct drm_plane *plane,
>  		struct drm_crtc *crtc = state->crtc;
>  		struct vmw_connector_state *vcs;
>  		struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> -		struct vmw_private *dev_priv = vmw_priv(crtc->dev);
> -		struct vmw_framebuffer *vfb = vmw_framebuffer_to_vfb(new_fb);
>  
>  		vcs = vmw_connector_state_to_vcs(du->connector.state);
> -
> -		/* Only one active implicit framebuffer at a time. */
> -		mutex_lock(&dev_priv->global_kms_state_mutex);
> -		if (vcs->is_implicit && dev_priv->implicit_fb &&
> -		    !(dev_priv->num_implicit == 1 && du->active_implicit)
> -		    && dev_priv->implicit_fb != vfb) {
> -			DRM_ERROR("Multiple implicit framebuffers "
> -				  "not supported.\n");
> -			ret = -EINVAL;
> -		}
> -		mutex_unlock(&dev_priv->global_kms_state_mutex);
>  	}
>  
>  
> @@ -1566,6 +1553,88 @@ static int vmw_kms_check_display_memory(struct drm_device *dev,
>  	return 0;
>  }
>  
> +/**
> + * vmw_crtc_state_and_lock - Return new or current crtc state with locked
> + * crtc mutex
> + * @state: The atomic state pointer containing the new atomic state
> + * @crtc: The crtc
> + *
> + * This function returns the new crtc state if it's part of the state update.
> + * Otherwise returns the current crtc state. It also makes sure that the
> + * crtc mutex is locked.
> + *
> + * Returns: A valid crtc state pointer or NULL. It may also return a
> + * pointer error, in particular -EDEADLK if locking needs to be rerun.
> + */
> +static struct drm_crtc_state *
> +vmw_crtc_state_and_lock(struct drm_atomic_state *state, struct drm_crtc *crtc)
> +{
> +	struct drm_crtc_state *crtc_state;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	if (crtc_state) {
> +		lockdep_assert_held(&crtc->mutex.mutex.base);
> +	} else {
> +		int ret = drm_modeset_lock(&crtc->mutex, state->acquire_ctx);
> +
> +		if (ret != 0 && ret != -EALREADY)
> +			return ERR_PTR(ret);
> +
> +		crtc_state = crtc->state;
> +	}
> +
> +	return crtc_state;
> +}
> +
> +/**
> + * vmw_kms_check_implicit - Verify that all implicit display units scan out
> + * from the same fb after the new state is committed.
> + * @dev: The drm_device.
> + * @state: The new state to be checked.
> + *
> + * Returns:
> + *   Zero on success,
> + *   -EINVAL on invalid state,
> + *   -EDEADLK if modeset locking needs to be rerun.
> + */
> +static int vmw_kms_check_implicit(struct drm_device *dev,
> +				  struct drm_atomic_state *state)
> +{
> +	struct drm_framebuffer *implicit_fb = NULL;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_plane_state *plane_state;
> +
> +	drm_for_each_crtc(crtc, dev) {
> +		struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> +
> +		if (!du->is_implicit)
> +			continue;
> +
> +		crtc_state = vmw_crtc_state_and_lock(state, crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +
> +		if (!crtc_state || !crtc_state->enable)
> +			continue;
> +
> +		/*
> +		 * Can't move primary planes across crtcs, so this is OK.
> +		 * It also means we don't need to take the plane mutex.
> +		 */
> +		plane_state = du->primary.state;
> +		if (plane_state->crtc != crtc)
> +			continue;
> +
> +		if (!implicit_fb)
> +			implicit_fb = plane_state->fb;
> +		else if (implicit_fb != plane_state->fb)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * vmw_kms_check_topology - Validates topology in drm_atomic_state
>   * @dev: DRM device
> @@ -1683,6 +1752,10 @@ vmw_kms_atomic_check_modeset(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> +	ret = vmw_kms_check_implicit(dev, state);
> +	if (ret)
> +		return ret;
> +
>  	if (!state->allow_modeset)
>  		return ret;
>  
> @@ -2277,13 +2350,7 @@ int vmw_du_connector_set_property(struct drm_connector *connector,
>  				  struct drm_property *property,
>  				  uint64_t val)
>  {
> -	struct vmw_display_unit *du = vmw_connector_to_du(connector);
> -	struct vmw_private *dev_priv = vmw_priv(connector->dev);
> -
> -	if (property == dev_priv->implicit_placement_property)
> -		du->is_implicit = val;
> -
> -	return 0;
> +	return -EINVAL;
>  }
>  
>  
> @@ -2302,25 +2369,7 @@ vmw_du_connector_atomic_set_property(struct drm_connector *connector,
>  				     struct drm_property *property,
>  				     uint64_t val)
>  {
> -	struct vmw_private *dev_priv = vmw_priv(connector->dev);
> -	struct vmw_connector_state *vcs = vmw_connector_state_to_vcs(state);
> -	struct vmw_display_unit *du = vmw_connector_to_du(connector);
> -
> -
> -	if (property == dev_priv->implicit_placement_property) {
> -		vcs->is_implicit = val;
> -
> -		/*
> -		 * We should really be doing a drm_atomic_commit() to
> -		 * commit the new state, but since this doesn't cause
> -		 * an immedate state change, this is probably ok
> -		 */
> -		du->is_implicit = vcs->is_implicit;
> -	} else {
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> +	return -EINVAL;
>  }

No need to keep the empty functions.

>  
>  
> @@ -2339,10 +2388,9 @@ vmw_du_connector_atomic_get_property(struct drm_connector *connector,
>  				     uint64_t *val)
>  {
>  	struct vmw_private *dev_priv = vmw_priv(connector->dev);
> -	struct vmw_connector_state *vcs = vmw_connector_state_to_vcs(state);
>  
>  	if (property == dev_priv->implicit_placement_property)
> -		*val = vcs->is_implicit;
> +		*val = vmw_connector_to_du(connector)->is_implicit;

This codepath will never be take with immutable props. So you can nuke
this and you must do the appropriate drm_property_set_value() somewhere
to make the prop value match .is_implicit.

>  	else {
>  		DRM_ERROR("Invalid Property %s\n", property->name);
>  		return -EINVAL;

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel