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@xxxxxxxxxx>
> Reviewed-by: Sinclair Yeh <syeh@xxxxxxxxxx>
> Reviewed-by: Deepak Rawat <drawat@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
