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@xxxxxxxxxxxxxxx>
>> Cc: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
