| Message ID | 20170719143920.25685-3-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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
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
On Tue, Jul 25, 2017 at 11:11 AM, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Op 25-07-17 om 10:23 schreef Daniel Vetter: >> On Wed, Jul 19, 2017 at 04:39:15PM +0200, Maarten Lankhorst wrote: >>> /* >>> - * 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. Oh right. It's still completely buggy, and I'd like to fix that first, testcase included. Can you pls poke Gustavo a bit (or maybe he's on vacation)? -Daniel
Op 25-07-17 om 11:27 schreef Daniel Vetter: > On Tue, Jul 25, 2017 at 11:11 AM, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> Op 25-07-17 om 10:23 schreef Daniel Vetter: >>> On Wed, Jul 19, 2017 at 04:39:15PM +0200, Maarten Lankhorst wrote: >>>> /* >>>> - * 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. > Oh right. It's still completely buggy, and I'd like to fix that first, > testcase included. Can you pls poke Gustavo a bit (or maybe he's on > vacation)? > -Daniel The only thing we have atm excercising it is kms_cursor_legacy, but that doesn't check if flips can be overwritten. Perhaps we should make IGT tests a requirement for features in the future?
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) { - 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);
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(-)