Re: [PATCH v2 2/7] drm/atomic: Clean up drm_atomic_helper_async_check

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 25-07-17 om 10:23 schreef Daniel Vetter:
> On Wed, Jul 19, 2017 at 04:39:15PM +0200, Maarten Lankhorst wrote:
>> Instead of assigning plane to __plane, iterate over plane
>> which is the same thing. Simplify the check for n_planes != 1,
>> at that point we know plane != NULL as well.
>>
>> Rename obj_state to new_obj_state, and get rid of the bogus stuff.
>> crtc->state->state is NEVER non-null, if it is, there is a bug.
>> We should definitely try to prevent async updates if there are flips
>> queued, but this code will simply not be executed and needs to be
>> rethought.
>>
>> Also remove the loop too, because we're trying to loop over the crtc until
>> we find plane_state->crtc == crtc, which we already know is non-zero.
>> It's dead code anyway. :)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Gustavo Padovan <gustavo.padovan@collabora.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 56 ++++++++++---------------------------
>>  1 file changed, 15 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index a46b51291006..c538528a794a 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1372,68 +1372,42 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>>  				   struct drm_atomic_state *state)
>>  {
>>  	struct drm_crtc *crtc;
>> -	struct drm_crtc_state *crtc_state;
>> -	struct drm_crtc_commit *commit;
>> -	struct drm_plane *__plane, *plane = NULL;
>> -	struct drm_plane_state *__plane_state, *plane_state = NULL;
>> +	struct drm_crtc_state *new_crtc_state;
>> +	struct drm_plane *plane;
>> +	struct drm_plane_state *new_plane_state;
>>  	const struct drm_plane_helper_funcs *funcs;
>> -	int i, j, n_planes = 0;
>> +	int i, n_planes = 0;
>>  
>> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>> -		if (drm_atomic_crtc_needs_modeset(crtc_state))
>> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>> +		if (drm_atomic_crtc_needs_modeset(new_crtc_state))
>>  			return -EINVAL;
>>  	}
>>  
>> -	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
>> +	for_each_new_plane_in_state(state, plane, new_plane_state, i)
>>  		n_planes++;
>> -		plane = __plane;
>> -		plane_state = __plane_state;
>> -	}
>>  
>>  	/* FIXME: we support only single plane updates for now */
>> -	if (!plane || n_planes != 1)
>> +	if (n_planes != 1)
>>  		return -EINVAL;
>>  
>> -	if (!plane_state->crtc)
>> +	if (!new_plane_state->crtc)
>>  		return -EINVAL;
>>  
>>  	funcs = plane->helper_private;
>>  	if (!funcs->atomic_async_update)
>>  		return -EINVAL;
>>  
>> -	if (plane_state->fence)
>> +	if (new_plane_state->fence)
>>  		return -EINVAL;
>>  
>>  	/*
>> -	 * Don't do an async update if there is an outstanding commit modifying
>> -	 * the plane.  This prevents our async update's changes from getting
>> -	 * overridden by a previous synchronous update's state.
>> +	 * FIXME: We should prevent an async update if there is an outstanding
>> +	 * commit modifying the plane.  This prevents our async update's
>> +	 * changes from getting overwritten by a previous synchronous update's
>> +	 * state.
>>  	 */
>> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>> -		if (plane->crtc != crtc)
>> -			continue;
>> -
>> -		spin_lock(&crtc->commit_lock);
>> -		commit = list_first_entry_or_null(&crtc->commit_list,
>> -						  struct drm_crtc_commit,
>> -						  commit_entry);
>> -		if (!commit) {
>> -			spin_unlock(&crtc->commit_lock);
>> -			continue;
>> -		}
>> -		spin_unlock(&crtc->commit_lock);
>> -
>> -		if (!crtc->state->state)
>> -			continue;
>> -
>> -		for_each_plane_in_state(crtc->state->state, __plane,
>> -					__plane_state, j) {
> I'm pretty sure this oopses, because crtc->state->state is NULL after
> commit. I think Gustavo needs to review this first (and write a nasty igt
> testcase to catch it) before we remove this. I think the correct check is
> to simply bail out if our current crtc has a pending commit (i.e.
> !list_empty(&crtc->commit_list) should be all we need to check.
It didn't oops. Right above it was a null check. It was never executed. :)

obj->state->state is always NULL. Excluding a brief moment during swap_state where this may oops,  this code willl never do a thing.
> But I think we need a testcase for this.
>
> Otherwise the patch looks ok I think.
> -Daniel
>
>
>> -			if (__plane == plane)
>> -				return -EINVAL;
>> -		}
>> -	}
>>  
>> -	return funcs->atomic_async_check(plane, plane_state);
>> +	return funcs->atomic_async_check(plane, new_plane_state);
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_async_check);
>>  
>> -- 
>> 2.11.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx