[PATCH -next 05/15] drm/vmwgfx: Fix atomic mode set check

[Daniel Vetter]

On Tue, Jul 03, 2018 at 09:14:50PM +0200, Thomas Hellstrom wrote:
> From: Sinclair Yeh <syeh at vmware.com>
> 
> 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 at vmware.com>
> Reviewed-by: Deepak Rawat <drawat at vmware.com>
> Signed-off-by: Thomas Hellstrom <thellstrom at vmware.com>
> ---
>  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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch