On Tue, Jul 03, 2018 at 09:14:50PM +0200, Thomas Hellstrom wrote:
> From: Sinclair Yeh <syeh@xxxxxxxxxx>
>
> vmw_kms_atomic_check_modeset() is currently checking config using the
> legacy state, which is updated after a commit has happened.
>
> This means vmw_kms_atomic_check_modeset() will reject an invalid config
> on the next update rather than the current one.
>
> Fix this by using the new states for config checking
>
> Signed-off-by: Sinclair Yeh <syeh@xxxxxxxxxx>
> Reviewed-by: Deepak Rawat <drawat@xxxxxxxxxx>
> Signed-off-by: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 40 +++++++++++++++++++----------
> 1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index e7a7a2e73bbf..6b8541f9215d 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -1526,33 +1526,45 @@ static int
> vmw_kms_atomic_check_modeset(struct drm_device *dev,
> struct drm_atomic_state *state)
> {
> - struct drm_crtc_state *crtc_state;
> + struct drm_crtc_state *new_crtc_state;
> + struct drm_plane_state *new_plane_state;
> + struct drm_plane *plane;
> struct drm_crtc *crtc;
> struct vmw_private *dev_priv = vmw_priv(dev);
> - int i;
> + int i, ret, cpp = 0;
>
> - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> - unsigned long requested_bb_mem = 0;
> + ret = drm_atomic_helper_check(dev, state);
>
> - if (dev_priv->active_display_unit == vmw_du_screen_target) {
> - struct drm_plane *plane = crtc->primary;
> - struct drm_plane_state *plane_state;
> + /* If this is not a STDU, then no more checking is necessary */
> + if (ret || dev_priv->active_display_unit != vmw_du_screen_target)
> + return ret;
>
> - plane_state = drm_atomic_get_new_plane_state(state, plane);
> + for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> + if (new_plane_state->fb) {
> + int current_cpp = new_plane_state->fb->pitches[0] /
> + new_plane_state->fb->width;
>
> - if (plane_state && plane_state->fb) {
> - int cpp = plane_state->fb->format->cpp[0];
> + if (cpp == 0)
> + cpp = current_cpp;
> + else if (current_cpp != cpp)
> + return -EINVAL;
> + }
> + }
>
> - requested_bb_mem += crtc->mode.hdisplay * cpp *
> - crtc->mode.vdisplay;
> - }
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
Note that the above too loops only walk over the changed states (by
default), not all planes/crtc. You either need to add them yourself (which
kinda defeats the point of the per-plane/crtc locks for doing pageflips in
parallel). Or you need to track things in a driver private structure
(which you'll only grab on modesets) and just update the delta.
I didn't check the entire code to see whether this is a real problem for
you, just wanted to point this out.
-Daniel
> + unsigned long requested_bb_mem = 0;
> +
> + if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
> + requested_bb_mem += new_crtc_state->mode.hdisplay *
> + new_crtc_state->mode.vdisplay *
> + cpp;
>
> if (requested_bb_mem > dev_priv->prim_bb_mem)
> return -EINVAL;
> }
> }
>
> - return drm_atomic_helper_check(dev, state);
> + return ret;
> }
>
> static const struct drm_mode_config_funcs vmw_kms_funcs = {
> --
> 2.18.0.rc1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
