drm/vmwgfx: Fix up the implicit display unit handling
Subsystems (3)
PATCH 1/2 drm/vmwgfx: Fix up the implicit display unit handling
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 <[email protected]>
Reviewed-by Sinclair Yeh <[email protected]>
Reviewed-by Deepak Rawat <[email protected]>
--- 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(-)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +0 -2
@@ -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;
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +91 -139
292 lines not on this page · raw message
drivers/gpu/drm/vmwgfx/vmwgfx_kms.h +2 -14
@@ -191,8 +191,6 @@ struct vmw_plane_state {struct vmw_connector_state {struct drm_connector_state base; -bool is_implicit; -/** * @gui_x: *
@@ -254,7 +252,6 @@ struct vmw_display_unit {int gui_x;int gui_y;bool is_implicit; -bool active_implicit;int set_gui_x;int set_gui_y; };
@@ -334,17 +331,8 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv,struct drm_crtc **p_crtc, struct drm_display_mode **p_mode);void vmw_guess_mode_timing (struct drm_display_mode *mode); -void vmw_kms_del_active (struct vmw_private *dev_priv, -struct vmw_display_unit *du); -void vmw_kms_add_active (struct vmw_private *dev_priv, -struct vmw_display_unit *du, -struct vmw_framebuffer *vfb); -bool vmw_kms_crtc_flippable (struct vmw_private *dev_priv, -struct drm_crtc *crtc); -void vmw_kms_update_implicit_fb (struct vmw_private *dev_priv, -struct drm_crtc *crtc); -void vmw_kms_create_implicit_placement_property (struct vmw_private *dev_priv, -bool immutable); +void vmw_kms_update_implicit_fb (struct vmw_private *dev_priv); +void vmw_kms_create_implicit_placement_property (struct vmw_private *dev_priv);/* Universal Plane Helpers */ void vmw_du_primary_plane_destroy (struct drm_plane *plane);
drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c +1 -5
@@ -417,7 +417,6 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)drm_plane_helper_add (cursor, &vmw_ldu_cursor_plane_helper_funcs); -vmw_du_connector_reset (connector); ret =drm_connector_init (dev, connector, &vmw_legacy_connector_funcs,DRM_MODE_CONNECTOR_VIRTUAL );
@@ -428,8 +427,6 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)drm_connector_helper_add (connector, &vmw_ldu_connector_helper_funcs); connector->status =vmw_du_connector_detect (connector, true); -vmw_connector_state_to_vcs (connector->state)->is_implicit = true; - ret =drm_encoder_init (dev, encoder, &vmw_legacy_encoder_funcs,DRM_MODE_ENCODER_VIRTUAL ,NULL );
@@ -448,7 +445,6 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit) goto err_free_encoder; } -vmw_du_crtc_reset (crtc); ret = drm_crtc_init_with_planes(dev, crtc, &ldu->base.primary, &ldu->base.cursor,
@@ -514,7 +510,7 @@ int vmw_kms_ldu_init_display(struct vmw_private *dev_priv)if (ret !=0 ) goto err_free; -vmw_kms_create_implicit_placement_property (dev_priv, true); +vmw_kms_create_implicit_placement_property (dev_priv);if (dev_priv->capabilities &SVGA_CAP_MULTIMON )for (i =0 ; i <VMWGFX_NUM_DISPLAY_UNITS ; ++i)
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c +5 -33
@@ -241,28 +241,20 @@ static void vmw_sou_crtc_mode_set_nofb(struct drm_crtc *crtc) sou->buffer = vps->bo; sou->buffer_size = vps->bo_size; -if (sou->base.is_implicit) { - x = crtc->x; - y = crtc->y; - }else { - conn_state = sou->base.connector.state; - vmw_conn_state =vmw_connector_state_to_vcs (conn_state); - - x = vmw_conn_state->gui_x; - y = vmw_conn_state->gui_y; - } + conn_state = sou->base.connector.state; + vmw_conn_state =vmw_connector_state_to_vcs (conn_state); + + x = vmw_conn_state->gui_x; + y = vmw_conn_state->gui_y; ret =vmw_sou_fifo_create (dev_priv, sou, x, y, &crtc->mode);if (ret)DRM_ERROR ("Failed to define Screen Object %dx%d\n" , crtc->x, crtc->y); -vmw_kms_add_active (dev_priv, &sou->base, vfb); }else { sou->buffer =NULL ; sou->buffer_size =0 ; - -vmw_kms_del_active (dev_priv, &sou->base); } }
@@ -323,21 +315,14 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,uint32_t flags, struct drm_modeset_acquire_ctx *ctx) { - struct vmw_private *dev_priv = vmw_priv(crtc->dev); int ret; -if (!vmw_kms_crtc_flippable (dev_priv, crtc)) -return -EINVAL ; - ret =drm_atomic_helper_page_flip (crtc, new_fb, event, flags, ctx);if (ret) {DRM_ERROR ("Page flip error %d.\n" , ret);return ret; } -if (vmw_crtc_to_du (crtc)->is_implicit) -vmw_kms_update_implicit_fb (dev_priv, crtc); -return ret; }
@@ -640,7 +625,6 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit) primary = &sou->base.primary; cursor = &sou->base.cursor; - sou->base.active_implicit = false; sou->base.pref_active = (unit ==0 ); sou->base.pref_width = dev_priv->initial_width; sou->base.pref_height = dev_priv->initial_height;
@@ -693,8 +677,6 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)drm_connector_helper_add (connector, &vmw_sou_connector_helper_funcs); connector->status =vmw_du_connector_detect (connector, true); -vmw_connector_state_to_vcs (connector->state)->is_implicit = false; - ret =drm_encoder_init (dev, encoder, &vmw_screen_object_encoder_funcs,DRM_MODE_ENCODER_VIRTUAL ,NULL );
@@ -733,12 +715,6 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit) dev->mode_config.suggested_x_property, 0);drm_object_attach_property (&connector->base, dev->mode_config.suggested_y_property,0 ); -if (dev_priv->implicit_placement_property) -drm_object_attach_property - (&connector->base, - dev_priv->implicit_placement_property, - sou->base.is_implicit); -return 0 ; err_free_unregister:
@@ -764,15 +740,11 @@ int vmw_kms_sou_init_display(struct vmw_private *dev_priv) } ret = -ENOMEM ; - dev_priv->num_implicit =0 ; - dev_priv->implicit_fb =NULL ; ret =drm_vblank_init (dev,VMWGFX_NUM_DISPLAY_UNITS );if (unlikely (ret !=0 ))return ret; -vmw_kms_create_implicit_placement_property (dev_priv, false); -for (i =0 ; i <VMWGFX_NUM_DISPLAY_UNITS ; ++i)vmw_sou_init (dev_priv, i);
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c +3 -40
91 lines not on this page · raw message
Signature · 6 lines
2.14.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 2/2 drm/vmwgfx: Fix a layout race-condition
This fixes a layout update race condition. We make sure
the crtc mutex is locked before we dereference crtc->state. Otherwise the state might change under us.
Since now we're already holding the crtc mutexes when reading the gui coordinates, protect them with the crtc mutexes rather than with the requested_layout mutex.
Signed-off-by Thomas Hellstrom <[email protected]>
Reviewed-by Deepak Rawat <[email protected]>
Reviewed-by Sinclair Yeh <[email protected]>
--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 - drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 9 ------ drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 64 ++++++++++++++++++++++--------------- 3 files changed, 38 insertions(+), 36 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +0 -1
@@ -665,7 +665,6 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)mutex_init (&dev_priv->cmdbuf_mutex);mutex_init (&dev_priv->release_mutex);mutex_init (&dev_priv->binding_mutex); -mutex_init (&dev_priv->requested_layout_mutex);mutex_init (&dev_priv->global_kms_state_mutex);ttm_lock_init (&dev_priv->reservation_sem);spin_lock_init (&dev_priv->resource_lock);
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +0 -9
@@ -467,15 +467,6 @@ struct vmw_private {uint32_t num_displays; -/* -* Currently requested_layout_mutex is used to protect the gui -* positionig state in display unit. With that use case currently this -* mutex is only taken during layout ioctl and atomic check_modeset. -* Other display unit state can be protected with this mutex but that -* needs careful consideration. -*/ -struct mutex requested_layout_mutex; -/* * Framebuffer info. */
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +38 -26
125 lines not on this page · raw message
Signature · 6 lines
2.14.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 187 lines · Thomas Hellstrom
Make the connector
is_implicitproperty 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 <[email protected]>Reviewed-by Sinclair Yeh <[email protected]>Reviewed-by Deepak Rawat <[email protected]>--- 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(-)drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +0
-2@@ -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;drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +87
-31@@ -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
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c vmw_du_connector_atomic_set_property(struct drm_connector *connector, · line 2373
Quoted 10 lines
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +1
-8-* 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.
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c vmw_du_connector_atomic_get_property(struct drm_connector *connector, · line 2393
Quoted 11 lines
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +1
-2@@ -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.
Quoted 3 lines
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +0
-0else {DRM_ERROR ("Invalid Property %s\n" , property->name);return -EINVAL ;
Signature · 6 lines
Ville Syrjälä Intel _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Thanks for reviewing, Ville.
I'll fix those up.
/Thomas
On 10/05/2018 12:04 PM, Ville Syrjälä wrote:
...
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel