[-next,05/15] drm/vmwgfx: Fix atomic mode set check
diff mbox

Message ID 20180703191500.2374-6-thellstrom@vmware.com
State New
Headers show

Commit Message

Thomas Hellstrom July 3, 2018, 7:14 p.m. UTC
From: Sinclair Yeh <syeh@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@vmware.com>
Reviewed-by: Deepak Rawat <drawat@vmware.com>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 40 +++++++++++++++++++----------
 1 file changed, 26 insertions(+), 14 deletions(-)

Comments

Daniel Vetter July 4, 2018, 8:35 a.m. UTC | #1
On Tue, Jul 03, 2018 at 09:14:50PM +0200, Thomas Hellstrom wrote:
> From: Sinclair Yeh <syeh@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@vmware.com>
> Reviewed-by: Deepak Rawat <drawat@vmware.com>
> Signed-off-by: Thomas Hellstrom <thellstrom@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Thomas Hellstrom July 4, 2018, 9:35 a.m. UTC | #2
Hi,

On 07/04/2018 10:35 AM, Daniel Vetter wrote:
> On Tue, Jul 03, 2018 at 09:14:50PM +0200, Thomas Hellstrom wrote:
>> From: Sinclair Yeh <syeh@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@vmware.com>
>> Reviewed-by: Deepak Rawat <drawat@vmware.com>
>> Signed-off-by: Thomas Hellstrom <thellstrom@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
Thanks for pointing this out. This is actually addressed in a later 
patch by Deepak which chooses the second alternative. We should perhaps 
have squashed these patches, but keeping track internally becomes easier 
this way.

/Thomas

Patch
diff mbox

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) {
+		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 = {