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

[Thomas Hellstrom]

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 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
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