It's time to kill off for_each_obj_in_state, and convert the remainder to for_each_(old/new/both)_obj_in_state.
Some patches have been upstreamed, these are the remaining few with compile fixes.
Maarten Lankhorst (7): drm/atomic: Use new iterator macros in drm_atomic_helper_wait_for_flip_done, again. drm/atomic: Clean up drm_atomic_helper_async_check drm/rcar-du: Use new iterator macros, v2. drm/omapdrm: Fix omap_atomic_wait_for_completion drm/msm: Convert to use new iterator macros, v2. drm/nouveau: Convert nouveau to use new iterator macros, v2. drm/atomic: Remove deprecated accessor macros
drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++------------------ drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 4 +- drivers/gpu/drm/msm/msm_atomic.c | 18 ++++---- drivers/gpu/drm/nouveau/nv50_display.c | 72 ++++++++++++++++--------------- drivers/gpu/drm/omapdrm/omap_drv.c | 6 +-- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 57 ++++++++++++------------- include/drm/drm_atomic.h | 75 --------------------------------- include/drm/drm_connector.h | 3 +- include/drm/drm_crtc.h | 8 ++-- include/drm/drm_plane.h | 8 ++-- 10 files changed, 104 insertions(+), 205 deletions(-)
for_each_obj_in_state is about to be removed, so use the correct new iterator macro.
I renamed the variable to 'unused', but forgot to convert drm_atomic_helper_wait_for_flip_done to the new iterator macro, so make it work this time.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Sean Paul seanpaul@chromium.org Cc: David Airlie airlied@linux.ie --- drivers/gpu/drm/drm_atomic_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 70146f809db5..a46b51291006 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1270,7 +1270,7 @@ void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev, struct drm_crtc *crtc; int i;
- for_each_crtc_in_state(old_state, crtc, unused, i) { + for_each_new_crtc_in_state(old_state, crtc, unused, i) { struct drm_crtc_commit *commit = old_state->crtcs[i].commit; int ret;
On Wed, Jul 19, 2017 at 04:39:14PM +0200, Maarten Lankhorst wrote:
for_each_obj_in_state is about to be removed, so use the correct new iterator macro.
I renamed the variable to 'unused', but forgot to convert drm_atomic_helper_wait_for_flip_done to the new iterator macro, so make it work this time.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Sean Paul seanpaul@chromium.org Cc: David Airlie airlied@linux.ie
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_atomic_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 70146f809db5..a46b51291006 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1270,7 +1270,7 @@ void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev, struct drm_crtc *crtc; int i;
- for_each_crtc_in_state(old_state, crtc, unused, i) {
- for_each_new_crtc_in_state(old_state, crtc, unused, i) { struct drm_crtc_commit *commit = old_state->crtcs[i].commit; int ret;
-- 2.11.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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) { - 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);
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?
for_each_obj_in_state is about to be removed, so use the correct new iterator macros.
Also look at new_plane_state instead of plane->state when looking up the hw planes in use. They should be the same except when reallocating, (in which case this code is skipped) and we should really stop looking at obj->state whenever possible.
Changes since v1: - Actually compile correctly.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: linux-renesas-soc@vger.kernel.org --- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 57 ++++++++++++++++----------------- 1 file changed, 28 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index dcde6288da6c..50fd793c38d1 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -51,12 +51,9 @@ */
static bool rcar_du_plane_needs_realloc(struct rcar_du_plane *plane, + const struct rcar_du_plane_state *cur_state, struct rcar_du_plane_state *new_state) { - struct rcar_du_plane_state *cur_state; - - cur_state = to_rcar_plane_state(plane->plane.state); - /* Lowering the number of planes doesn't strictly require reallocation * as the extra hardware plane will be freed when committing, but doing * so could lead to more fragmentation. @@ -141,16 +138,17 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, unsigned int groups = 0; unsigned int i; struct drm_plane *drm_plane; - struct drm_plane_state *drm_plane_state; + struct drm_plane_state *old_drm_plane_state, *new_drm_plane_state;
/* Check if hardware planes need to be reallocated. */ - for_each_plane_in_state(state, drm_plane, drm_plane_state, i) { - struct rcar_du_plane_state *plane_state; + for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state, new_drm_plane_state, i) { + struct rcar_du_plane_state *old_plane_state, *new_plane_state; struct rcar_du_plane *plane; unsigned int index;
plane = to_rcar_plane(drm_plane); - plane_state = to_rcar_plane_state(drm_plane_state); + old_plane_state = to_rcar_plane_state(old_drm_plane_state); + new_plane_state = to_rcar_plane_state(new_drm_plane_state);
dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes); @@ -159,19 +157,19 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, * the full reallocation procedure. Just mark the hardware * plane(s) as freed. */ - if (!plane_state->format) { + if (!new_plane_state->format) { dev_dbg(rcdu->dev, "%s: plane is being disabled\n", __func__); index = plane - plane->group->planes; group_freed_planes[plane->group->index] |= 1 << index; - plane_state->hwindex = -1; + new_plane_state->hwindex = -1; continue; }
/* If the plane needs to be reallocated mark it as such, and * mark the hardware plane(s) as free. */ - if (rcar_du_plane_needs_realloc(plane, plane_state)) { + if (rcar_du_plane_needs_realloc(plane, old_plane_state, new_plane_state)) { dev_dbg(rcdu->dev, "%s: plane needs reallocation\n", __func__); groups |= 1 << plane->group->index; @@ -179,7 +177,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
index = plane - plane->group->planes; group_freed_planes[plane->group->index] |= 1 << index; - plane_state->hwindex = -1; + new_plane_state->hwindex = -1; } }
@@ -204,7 +202,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
for (i = 0; i < group->num_planes; ++i) { struct rcar_du_plane *plane = &group->planes[i]; - struct rcar_du_plane_state *plane_state; + struct rcar_du_plane_state *new_plane_state; struct drm_plane_state *s;
s = drm_atomic_get_plane_state(state, &plane->plane); @@ -226,16 +224,16 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, continue; }
- plane_state = to_rcar_plane_state(plane->plane.state); - used_planes |= rcar_du_plane_hwmask(plane_state); + new_plane_state = to_rcar_plane_state(s); + used_planes |= rcar_du_plane_hwmask(new_plane_state);
dev_dbg(rcdu->dev, "%s: plane (%u,%tu) uses %u hwplanes (index %d)\n", __func__, plane->group->index, plane - plane->group->planes, - plane_state->format ? - plane_state->format->planes : 0, - plane_state->hwindex); + new_plane_state->format ? + new_plane_state->format->planes : 0, + new_plane_state->hwindex); }
group_free_planes[index] = 0xff & ~used_planes; @@ -246,15 +244,16 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, }
/* Reallocate hardware planes for each plane that needs it. */ - for_each_plane_in_state(state, drm_plane, drm_plane_state, i) { - struct rcar_du_plane_state *plane_state; + for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state, new_drm_plane_state, i) { + struct rcar_du_plane_state *old_plane_state, *new_plane_state; struct rcar_du_plane *plane; unsigned int crtc_planes; unsigned int free; int idx;
plane = to_rcar_plane(drm_plane); - plane_state = to_rcar_plane_state(drm_plane_state); + old_plane_state = to_rcar_plane_state(old_drm_plane_state); + new_plane_state = to_rcar_plane_state(new_drm_plane_state);
dev_dbg(rcdu->dev, "%s: allocating plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes); @@ -262,8 +261,8 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, /* Skip planes that are being disabled or don't need to be * reallocated. */ - if (!plane_state->format || - !rcar_du_plane_needs_realloc(plane, plane_state)) + if (!new_plane_state->format || + !rcar_du_plane_needs_realloc(plane, old_plane_state, new_plane_state)) continue;
/* Try to allocate the plane from the free planes currently @@ -271,15 +270,15 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, * group and thus minimize flicker. If it fails fall back to * allocating from all free planes. */ - crtc_planes = to_rcar_crtc(plane_state->state.crtc)->index % 2 + crtc_planes = to_rcar_crtc(new_plane_state->state.crtc)->index % 2 ? plane->group->dptsr_planes : ~plane->group->dptsr_planes; free = group_free_planes[plane->group->index];
- idx = rcar_du_plane_hwalloc(plane, plane_state, + idx = rcar_du_plane_hwalloc(plane, new_plane_state, free & crtc_planes); if (idx < 0) - idx = rcar_du_plane_hwalloc(plane, plane_state, + idx = rcar_du_plane_hwalloc(plane, new_plane_state, free); if (idx < 0) { dev_dbg(rcdu->dev, "%s: no available hardware plane\n", @@ -288,12 +287,12 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, }
dev_dbg(rcdu->dev, "%s: allocated %u hwplanes (index %u)\n", - __func__, plane_state->format->planes, idx); + __func__, new_plane_state->format->planes, idx);
- plane_state->hwindex = idx; + new_plane_state->hwindex = idx;
group_free_planes[plane->group->index] &= - ~rcar_du_plane_hwmask(plane_state); + ~rcar_du_plane_hwmask(new_plane_state);
dev_dbg(rcdu->dev, "%s: group %u free planes mask 0x%02x\n", __func__, plane->group->index,
On Wed, Jul 19, 2017 at 04:39:16PM +0200, Maarten Lankhorst wrote:
for_each_obj_in_state is about to be removed, so use the correct new iterator macros.
Also look at new_plane_state instead of plane->state when looking up the hw planes in use. They should be the same except when reallocating, (in which case this code is skipped) and we should really stop looking at obj->state whenever possible.
Changes since v1:
- Actually compile correctly.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: linux-renesas-soc@vger.kernel.org
Didn't compile-test, but looks correct now.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/rcar-du/rcar_du_plane.c | 57 ++++++++++++++++----------------- 1 file changed, 28 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index dcde6288da6c..50fd793c38d1 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -51,12 +51,9 @@ */
static bool rcar_du_plane_needs_realloc(struct rcar_du_plane *plane,
const struct rcar_du_plane_state *cur_state, struct rcar_du_plane_state *new_state){
- struct rcar_du_plane_state *cur_state;
- cur_state = to_rcar_plane_state(plane->plane.state);
- /* Lowering the number of planes doesn't strictly require reallocation
- as the extra hardware plane will be freed when committing, but doing
- so could lead to more fragmentation.
@@ -141,16 +138,17 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, unsigned int groups = 0; unsigned int i; struct drm_plane *drm_plane;
- struct drm_plane_state *drm_plane_state;
struct drm_plane_state *old_drm_plane_state, *new_drm_plane_state;
/* Check if hardware planes need to be reallocated. */
- for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
struct rcar_du_plane_state *plane_state;
for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state, new_drm_plane_state, i) {
struct rcar_du_plane_state *old_plane_state, *new_plane_state;struct rcar_du_plane *plane; unsigned int index;
plane = to_rcar_plane(drm_plane);
plane_state = to_rcar_plane_state(drm_plane_state);
old_plane_state = to_rcar_plane_state(old_drm_plane_state);new_plane_state = to_rcar_plane_state(new_drm_plane_state);dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes);
@@ -159,19 +157,19 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, * the full reallocation procedure. Just mark the hardware * plane(s) as freed. */
if (!plane_state->format) {
if (!new_plane_state->format) { dev_dbg(rcdu->dev, "%s: plane is being disabled\n", __func__); index = plane - plane->group->planes; group_freed_planes[plane->group->index] |= 1 << index;
plane_state->hwindex = -1;
new_plane_state->hwindex = -1; continue;}
/* If the plane needs to be reallocated mark it as such, and
- mark the hardware plane(s) as free.
*/
if (rcar_du_plane_needs_realloc(plane, plane_state)) {
if (rcar_du_plane_needs_realloc(plane, old_plane_state, new_plane_state)) { dev_dbg(rcdu->dev, "%s: plane needs reallocation\n", __func__); groups |= 1 << plane->group->index;@@ -179,7 +177,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
index = plane - plane->group->planes; group_freed_planes[plane->group->index] |= 1 << index;
plane_state->hwindex = -1;
new_plane_state->hwindex = -1;@@ -204,7 +202,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
for (i = 0; i < group->num_planes; ++i) { struct rcar_du_plane *plane = &group->planes[i];
struct rcar_du_plane_state *plane_state;
struct rcar_du_plane_state *new_plane_state; struct drm_plane_state *s; s = drm_atomic_get_plane_state(state, &plane->plane);@@ -226,16 +224,16 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, continue; }
plane_state = to_rcar_plane_state(plane->plane.state);used_planes |= rcar_du_plane_hwmask(plane_state);
new_plane_state = to_rcar_plane_state(s);used_planes |= rcar_du_plane_hwmask(new_plane_state); dev_dbg(rcdu->dev, "%s: plane (%u,%tu) uses %u hwplanes (index %d)\n", __func__, plane->group->index, plane - plane->group->planes,
plane_state->format ?plane_state->format->planes : 0,plane_state->hwindex);
new_plane_state->format ?new_plane_state->format->planes : 0,new_plane_state->hwindex);}
group_free_planes[index] = 0xff & ~used_planes;
@@ -246,15 +244,16 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, }
/* Reallocate hardware planes for each plane that needs it. */
- for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
struct rcar_du_plane_state *plane_state;
for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state, new_drm_plane_state, i) {
struct rcar_du_plane_state *old_plane_state, *new_plane_state;struct rcar_du_plane *plane; unsigned int crtc_planes; unsigned int free; int idx;
plane = to_rcar_plane(drm_plane);
plane_state = to_rcar_plane_state(drm_plane_state);
old_plane_state = to_rcar_plane_state(old_drm_plane_state);new_plane_state = to_rcar_plane_state(new_drm_plane_state);dev_dbg(rcdu->dev, "%s: allocating plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes);
@@ -262,8 +261,8 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, /* Skip planes that are being disabled or don't need to be * reallocated. */
if (!plane_state->format ||!rcar_du_plane_needs_realloc(plane, plane_state))
if (!new_plane_state->format ||!rcar_du_plane_needs_realloc(plane, old_plane_state, new_plane_state)) continue;/* Try to allocate the plane from the free planes currently
@@ -271,15 +270,15 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, * group and thus minimize flicker. If it fails fall back to * allocating from all free planes. */
crtc_planes = to_rcar_crtc(plane_state->state.crtc)->index % 2
crtc_planes = to_rcar_crtc(new_plane_state->state.crtc)->index % 2 ? plane->group->dptsr_planes : ~plane->group->dptsr_planes;
idx = rcar_du_plane_hwalloc(plane, plane_state,
idx = rcar_du_plane_hwalloc(plane, new_plane_state, free & crtc_planes);
idx = rcar_du_plane_hwalloc(plane, plane_state,
idx = rcar_du_plane_hwalloc(plane, new_plane_state, free);@@ -288,12 +287,12 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, }
dev_dbg(rcdu->dev, "%s: allocated %u hwplanes (index %u)\n",
__func__, plane_state->format->planes, idx);
__func__, new_plane_state->format->planes, idx);
plane_state->hwindex = idx;
new_plane_state->hwindex = idx;group_free_planes[plane->group->index] &=
~rcar_du_plane_hwmask(plane_state);
~rcar_du_plane_hwmask(new_plane_state);dev_dbg(rcdu->dev, "%s: group %u free planes mask 0x%02x\n", __func__, plane->group->index,
-- 2.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Maarten,
Thank you for the patch.
On Wednesday 19 Jul 2017 16:39:16 Maarten Lankhorst wrote:
for_each_obj_in_state is about to be removed, so use the correct new iterator macros.
Also look at new_plane_state instead of plane->state when looking up the hw planes in use. They should be the same except when reallocating, (in which case this code is skipped) and we should really stop looking at obj->state whenever possible.
Changes since v1:
- Actually compile correctly.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: linux-renesas-soc@vger.kernel.org
drivers/gpu/drm/rcar-du/rcar_du_plane.c | 57 ++++++++++++++--------------- 1 file changed, 28 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index dcde6288da6c..50fd793c38d1 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -51,12 +51,9 @@ */
static bool rcar_du_plane_needs_realloc(struct rcar_du_plane *plane,
const struct rcar_du_plane_state *cur_state,
I would call this old_state to be consistent with the naming scheme used in the subsystem.
struct rcar_du_plane_state *new_state)
The function doesn't need the plane argument anymore, it can be removed.
{
- struct rcar_du_plane_state *cur_state;
- cur_state = to_rcar_plane_state(plane->plane.state);
- /* Lowering the number of planes doesn't strictly require reallocation
- as the extra hardware plane will be freed when committing, but doing
- so could lead to more fragmentation.
@@ -141,16 +138,17 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, unsigned int groups = 0; unsigned int i; struct drm_plane *drm_plane;
- struct drm_plane_state *drm_plane_state;
- struct drm_plane_state *old_drm_plane_state, *new_drm_plane_state;
One variable declaration per line please (here and below).
/* Check if hardware planes need to be reallocated. */
- for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
struct rcar_du_plane_state *plane_state;
- for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state,
new_drm_plane_state, i) {
Could you please break lines to keep them below the 80 columns limit (here and below) ?
struct rcar_du_plane_state *old_plane_state, *new_plane_state;struct rcar_du_plane *plane; unsigned int index;
plane = to_rcar_plane(drm_plane);
plane_state = to_rcar_plane_state(drm_plane_state);
old_plane_state = to_rcar_plane_state(old_drm_plane_state);new_plane_state = to_rcar_plane_state(new_drm_plane_state);dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes);
[snip]
Feel free to use this version of the patch.
From 21ea2b1e4dcdfb1fd0ff44776434219793e0ef75 Mon Sep 17 00:00:00 2001
From: Maarten Lankhorst maarten.lankhorst@linux.intel.com Date: Wed, 12 Jul 2017 12:43:40 +0200 Subject: [PATCH v3] drm: rcar-du: Use new iterator macros
for_each_obj_in_state is about to be removed, so use the correct new iterator macros.
Also look at new_plane_state instead of plane->state when looking up the hw planes in use. They should be the same except when reallocating, (in which case this code is skipped) and we should really stop looking at obj->state whenever possible.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- Changes since v2:
- Removed plane argument to rcar_du_plane_needs_realloc() - Avoid lines longer than 80 columns when possible - Declare one variable per line
Changes since v1:
- Actually compile correctly. --- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 72 +++++++++++++++++---------------- 1 file changed, 38 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index dcde6288da6c..4bad0b79d3f2 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -50,23 +50,20 @@ * automatically when the core swaps the old and new states. */
-static bool rcar_du_plane_needs_realloc(struct rcar_du_plane *plane, - struct rcar_du_plane_state *new_state) +static bool rcar_du_plane_needs_realloc( + const struct rcar_du_plane_state *old_state, + const struct rcar_du_plane_state *new_state) { - struct rcar_du_plane_state *cur_state; - - cur_state = to_rcar_plane_state(plane->plane.state); - /* Lowering the number of planes doesn't strictly require reallocation * as the extra hardware plane will be freed when committing, but doing * so could lead to more fragmentation. */ - if (!cur_state->format || - cur_state->format->planes != new_state->format->planes) + if (!old_state->format || + old_state->format->planes != new_state->format->planes) return true;
/* Reallocate hardware planes if the source has changed. */ - if (cur_state->source != new_state->source) + if (old_state->source != new_state->source) return true;
return false; @@ -141,16 +138,20 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, unsigned int groups = 0; unsigned int i; struct drm_plane *drm_plane; - struct drm_plane_state *drm_plane_state; + struct drm_plane_state *old_drm_plane_state; + struct drm_plane_state *new_drm_plane_state;
/* Check if hardware planes need to be reallocated. */ - for_each_plane_in_state(state, drm_plane, drm_plane_state, i) { - struct rcar_du_plane_state *plane_state; + for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state, + new_drm_plane_state, i) { + struct rcar_du_plane_state *old_plane_state; + struct rcar_du_plane_state *new_plane_state; struct rcar_du_plane *plane; unsigned int index;
plane = to_rcar_plane(drm_plane); - plane_state = to_rcar_plane_state(drm_plane_state); + old_plane_state = to_rcar_plane_state(old_drm_plane_state); + new_plane_state = to_rcar_plane_state(new_drm_plane_state);
dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes); @@ -159,19 +160,19 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, * the full reallocation procedure. Just mark the hardware * plane(s) as freed. */ - if (!plane_state->format) { + if (!new_plane_state->format) { dev_dbg(rcdu->dev, "%s: plane is being disabled\n", __func__); index = plane - plane->group->planes; group_freed_planes[plane->group->index] |= 1 << index; - plane_state->hwindex = -1; + new_plane_state->hwindex = -1; continue; }
/* If the plane needs to be reallocated mark it as such, and * mark the hardware plane(s) as free. */ - if (rcar_du_plane_needs_realloc(plane, plane_state)) { + if (rcar_du_plane_needs_realloc(old_plane_state, new_plane_state)) { dev_dbg(rcdu->dev, "%s: plane needs reallocation\n", __func__); groups |= 1 << plane->group->index; @@ -179,7 +180,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
index = plane - plane->group->planes; group_freed_planes[plane->group->index] |= 1 << index; - plane_state->hwindex = -1; + new_plane_state->hwindex = -1; } }
@@ -204,7 +205,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
for (i = 0; i < group->num_planes; ++i) { struct rcar_du_plane *plane = &group->planes[i]; - struct rcar_du_plane_state *plane_state; + struct rcar_du_plane_state *new_plane_state; struct drm_plane_state *s;
s = drm_atomic_get_plane_state(state, &plane->plane); @@ -226,16 +227,16 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, continue; }
- plane_state = to_rcar_plane_state(plane->plane.state); - used_planes |= rcar_du_plane_hwmask(plane_state); + new_plane_state = to_rcar_plane_state(s); + used_planes |= rcar_du_plane_hwmask(new_plane_state);
dev_dbg(rcdu->dev, "%s: plane (%u,%tu) uses %u hwplanes (index %d)\n", __func__, plane->group->index, plane - plane->group->planes, - plane_state->format ? - plane_state->format->planes : 0, - plane_state->hwindex); + new_plane_state->format ? + new_plane_state->format->planes : 0, + new_plane_state->hwindex); }
group_free_planes[index] = 0xff & ~used_planes; @@ -246,15 +247,18 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, }
/* Reallocate hardware planes for each plane that needs it. */ - for_each_plane_in_state(state, drm_plane, drm_plane_state, i) { - struct rcar_du_plane_state *plane_state; + for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state, + new_drm_plane_state, i) { + struct rcar_du_plane_state *old_plane_state; + struct rcar_du_plane_state *new_plane_state; struct rcar_du_plane *plane; unsigned int crtc_planes; unsigned int free; int idx;
plane = to_rcar_plane(drm_plane); - plane_state = to_rcar_plane_state(drm_plane_state); + old_plane_state = to_rcar_plane_state(old_drm_plane_state); + new_plane_state = to_rcar_plane_state(new_drm_plane_state);
dev_dbg(rcdu->dev, "%s: allocating plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes); @@ -262,8 +266,8 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, /* Skip planes that are being disabled or don't need to be * reallocated. */ - if (!plane_state->format || - !rcar_du_plane_needs_realloc(plane, plane_state)) + if (!new_plane_state->format || + !rcar_du_plane_needs_realloc(old_plane_state, new_plane_state)) continue;
/* Try to allocate the plane from the free planes currently @@ -271,15 +275,15 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, * group and thus minimize flicker. If it fails fall back to * allocating from all free planes. */ - crtc_planes = to_rcar_crtc(plane_state->state.crtc)->index % 2 + crtc_planes = to_rcar_crtc(new_plane_state->state.crtc)->index % 2 ? plane->group->dptsr_planes : ~plane->group->dptsr_planes; free = group_free_planes[plane->group->index];
- idx = rcar_du_plane_hwalloc(plane, plane_state, + idx = rcar_du_plane_hwalloc(plane, new_plane_state, free & crtc_planes); if (idx < 0) - idx = rcar_du_plane_hwalloc(plane, plane_state, + idx = rcar_du_plane_hwalloc(plane, new_plane_state, free); if (idx < 0) { dev_dbg(rcdu->dev, "%s: no available hardware plane\n", @@ -288,12 +292,12 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, }
dev_dbg(rcdu->dev, "%s: allocated %u hwplanes (index %u)\n", - __func__, plane_state->format->planes, idx); + __func__, new_plane_state->format->planes, idx);
- plane_state->hwindex = idx; + new_plane_state->hwindex = idx;
group_free_planes[plane->group->index] &= - ~rcar_du_plane_hwmask(plane_state); + ~rcar_du_plane_hwmask(new_plane_state);
dev_dbg(rcdu->dev, "%s: group %u free planes mask 0x%02x\n", __func__, plane->group->index,
Hi Maarten,
On Wednesday 26 Jul 2017 14:53:33 Laurent Pinchart wrote:
On Wednesday 19 Jul 2017 16:39:16 Maarten Lankhorst wrote:
for_each_obj_in_state is about to be removed, so use the correct new iterator macros.
Also look at new_plane_state instead of plane->state when looking up the hw planes in use. They should be the same except when reallocating, (in which case this code is skipped) and we should really stop looking at obj->state whenever possible.
Changes since v1:
- Actually compile correctly.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: linux-renesas-soc@vger.kernel.org
I plan to send a pull request to Dave in the middle of next week with a bunch of rcar-du-drm patches that will generate a few conflicts with this one. They're all simple to solve as they're located in comments, but that will be annoying nonetheless. I can include a rebased version of this patch in my pull request if it can help, depending on when you plan to get this series merged in drm-misc-next.
Use the new iterator macro and look for crtc_state->active instead of enable, only crtc_state->enable implies that vblanks will happen.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/omapdrm/omap_drv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 022029ea6972..66d3c6bfd6a8 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -57,13 +57,13 @@ static void omap_fb_output_poll_changed(struct drm_device *dev) static void omap_atomic_wait_for_completion(struct drm_device *dev, struct drm_atomic_state *old_state) { - struct drm_crtc_state *old_crtc_state; + struct drm_crtc_state *new_crtc_state; struct drm_crtc *crtc; unsigned int i; int ret;
- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { - if (!crtc->state->enable) + for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) { + if (!new_crtc_state->active) continue;
ret = omap_crtc_wait_pending(crtc);
On Wed, Jul 19, 2017 at 04:39:17PM +0200, Maarten Lankhorst wrote:
Use the new iterator macro and look for crtc_state->active instead of enable, only crtc_state->enable implies that vblanks will happen.
s/enable/active/, since enable only means logically enabled (aka resources reserved). With that my r-b holds. -Daniel
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/omapdrm/omap_drv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 022029ea6972..66d3c6bfd6a8 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -57,13 +57,13 @@ static void omap_fb_output_poll_changed(struct drm_device *dev) static void omap_atomic_wait_for_completion(struct drm_device *dev, struct drm_atomic_state *old_state) {
- struct drm_crtc_state *old_crtc_state;
- struct drm_crtc_state *new_crtc_state; struct drm_crtc *crtc; unsigned int i; int ret;
- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
if (!crtc->state->enable)
for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
if (!new_crtc_state->active) continue;ret = omap_crtc_wait_pending(crtc);
-- 2.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 25-07-17 om 10:27 schreef Daniel Vetter:
On Wed, Jul 19, 2017 at 04:39:17PM +0200, Maarten Lankhorst wrote:
Use the new iterator macro and look for crtc_state->active instead of enable, only crtc_state->enable implies that vblanks will happen.
s/enable/active/, since enable only means logically enabled (aka resources reserved). With that my r-b holds. -Daniel
Ok thanks. I've pushed patch 1, 4, 5 and 6.
I'll wait a bit longer for the conflict in patch 3, and for a better solution on async in patch 2.
It seems the nouveau patches caused a bit of a conflict in nv50_disp_atomic_commit_tail because of better event handling compared to drm-misc, I've fixed up drm-tip.
for_each_obj_in_state is about to be removed, so convert to the new iterator macros.
Just like in omap, use crtc_state->active instead of crtc_state->enable when waiting for completion.
Changes since v1: - Fix compilation.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Rob Clark robdclark@gmail.com Cc: Archit Taneja architt@codeaurora.org Cc: Vincent Abriou vincent.abriou@st.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Russell King rmk+kernel@armlinux.org.uk Cc: Rob Herring robh@kernel.org Cc: Markus Elfring elfring@users.sourceforge.net Cc: Sushmita Susheelendra ssusheel@codeaurora.org Cc: linux-arm-msm@vger.kernel.org Cc: freedreno@lists.freedesktop.org Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Tested-by: Archit Taneja architt@codeaurora.org --- drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 4 ++-- drivers/gpu/drm/msm/msm_atomic.c | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c index bcd1f5cac72c..f7f087419ed8 100644 --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c @@ -114,7 +114,7 @@ static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st mdp4_enable(mdp4_kms);
/* see 119ecb7fd */ - for_each_crtc_in_state(state, crtc, crtc_state, i) + for_each_new_crtc_in_state(state, crtc, crtc_state, i) drm_crtc_vblank_get(crtc); }
@@ -126,7 +126,7 @@ static void mdp4_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s struct drm_crtc_state *crtc_state;
/* see 119ecb7fd */ - for_each_crtc_in_state(state, crtc, crtc_state, i) + for_each_new_crtc_in_state(state, crtc, crtc_state, i) drm_crtc_vblank_put(crtc);
mdp4_disable(mdp4_kms); diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index badfa8717317..025d454163b0 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -84,13 +84,13 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev, struct drm_atomic_state *old_state) { struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *new_crtc_state; struct msm_drm_private *priv = old_state->dev->dev_private; struct msm_kms *kms = priv->kms; int i;
- for_each_crtc_in_state(old_state, crtc, crtc_state, i) { - if (!crtc->state->enable) + for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) { + if (!new_crtc_state->active) continue;
kms->funcs->wait_for_crtc_commit_done(kms, crtc); @@ -195,7 +195,7 @@ int msm_atomic_commit(struct drm_device *dev, struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_plane *plane; - struct drm_plane_state *plane_state; + struct drm_plane_state *old_plane_state, *new_plane_state; int i, ret;
ret = drm_atomic_helper_prepare_planes(dev, state); @@ -211,19 +211,19 @@ int msm_atomic_commit(struct drm_device *dev, /* * Figure out what crtcs we have: */ - for_each_crtc_in_state(state, crtc, crtc_state, i) + for_each_new_crtc_in_state(state, crtc, crtc_state, i) c->crtc_mask |= drm_crtc_mask(crtc);
/* * Figure out what fence to wait for: */ - for_each_plane_in_state(state, plane, plane_state, i) { - if ((plane->state->fb != plane_state->fb) && plane_state->fb) { - struct drm_gem_object *obj = msm_framebuffer_bo(plane_state->fb, 0); + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { + if ((new_plane_state->fb != old_plane_state->fb) && new_plane_state->fb) { + struct drm_gem_object *obj = msm_framebuffer_bo(new_plane_state->fb, 0); struct msm_gem_object *msm_obj = to_msm_bo(obj); struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv);
- drm_atomic_set_fence_for_plane(plane_state, fence); + drm_atomic_set_fence_for_plane(new_plane_state, fence); } }
Use the new atomic iterator macros, the old ones are about to be removed. With the new macros, it's more easy to get old and new state so get them from the macros instead of from obj->state.
Changes since v1: - Don't mix up old and new state. (danvet) - Rebase on top of interruptible swap_state changes.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ben Skeggs bskeggs@redhat.com Cc: nouveau@lists.freedesktop.org --- drivers/gpu/drm/nouveau/nv50_display.c | 72 +++++++++++++++++----------------- 1 file changed, 37 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c index 747c99c1e474..7abfb561b00c 100644 --- a/drivers/gpu/drm/nouveau/nv50_display.c +++ b/drivers/gpu/drm/nouveau/nv50_display.c @@ -2103,7 +2103,7 @@ nv50_head_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state)
NV_ATOMIC(drm, "%s atomic_check %d\n", crtc->name, asyh->state.active); if (asyh->state.active) { - for_each_connector_in_state(asyh->state.state, conn, conns, i) { + for_each_new_connector_in_state(asyh->state.state, conn, conns, i) { if (conns->crtc == crtc) { asyc = nouveau_conn_atom(conns); break; @@ -3905,9 +3905,9 @@ static void nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *new_crtc_state; struct drm_crtc *crtc; - struct drm_plane_state *plane_state; + struct drm_plane_state *new_plane_state; struct drm_plane *plane; struct nouveau_drm *drm = nouveau_drm(dev); struct nv50_disp *disp = nv50_disp(dev); @@ -3926,8 +3926,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) mutex_lock(&disp->mutex);
/* Disable head(s). */ - for_each_crtc_in_state(state, crtc, crtc_state, i) { - struct nv50_head_atom *asyh = nv50_head_atom(crtc->state); + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + struct nv50_head_atom *asyh = nv50_head_atom(new_crtc_state); struct nv50_head *head = nv50_head(crtc);
NV_ATOMIC(drm, "%s: clr %04x (set %04x)\n", crtc->name, @@ -3940,8 +3940,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) }
/* Disable plane(s). */ - for_each_plane_in_state(state, plane, plane_state, i) { - struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state); + for_each_new_plane_in_state(state, plane, new_plane_state, i) { + struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state); struct nv50_wndw *wndw = nv50_wndw(plane);
NV_ATOMIC(drm, "%s: clr %02x (set %02x)\n", plane->name, @@ -4006,8 +4006,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) }
/* Update head(s). */ - for_each_crtc_in_state(state, crtc, crtc_state, i) { - struct nv50_head_atom *asyh = nv50_head_atom(crtc->state); + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + struct nv50_head_atom *asyh = nv50_head_atom(new_crtc_state); struct nv50_head *head = nv50_head(crtc);
NV_ATOMIC(drm, "%s: set %04x (clr %04x)\n", crtc->name, @@ -4019,14 +4019,14 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) } }
- for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (crtc->state->event) + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + if (new_crtc_state->event) drm_crtc_vblank_get(crtc); }
/* Update plane(s). */ - for_each_plane_in_state(state, plane, plane_state, i) { - struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state); + for_each_new_plane_in_state(state, plane, new_plane_state, i) { + struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state); struct nv50_wndw *wndw = nv50_wndw(plane);
NV_ATOMIC(drm, "%s: set %02x (clr %02x)\n", plane->name, @@ -4056,23 +4056,23 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) mutex_unlock(&disp->mutex);
/* Wait for HW to signal completion. */ - for_each_plane_in_state(state, plane, plane_state, i) { - struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state); + for_each_new_plane_in_state(state, plane, new_plane_state, i) { + struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state); struct nv50_wndw *wndw = nv50_wndw(plane); int ret = nv50_wndw_wait_armed(wndw, asyw); if (ret) NV_ERROR(drm, "%s: timeout\n", plane->name); }
- for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (crtc->state->event) { + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + if (new_crtc_state->event) { unsigned long flags; /* Get correct count/ts if racing with vblank irq */ drm_crtc_accurate_vblank_count(crtc); spin_lock_irqsave(&crtc->dev->event_lock, flags); - drm_crtc_send_vblank_event(crtc, crtc->state->event); + drm_crtc_send_vblank_event(crtc, new_crtc_state->event); spin_unlock_irqrestore(&crtc->dev->event_lock, flags); - crtc->state->event = NULL; + new_crtc_state->event = NULL; drm_crtc_vblank_put(crtc); } } @@ -4097,7 +4097,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, { struct nouveau_drm *drm = nouveau_drm(dev); struct nv50_disp *disp = nv50_disp(dev); - struct drm_plane_state *plane_state; + struct drm_plane_state *old_plane_state; struct drm_plane *plane; struct drm_crtc *crtc; bool active = false; @@ -4127,9 +4127,10 @@ nv50_disp_atomic_commit(struct drm_device *dev, if (ret) goto err_cleanup;
- for_each_plane_in_state(state, plane, plane_state, i) { - struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane_state); + for_each_old_plane_in_state(state, plane, old_plane_state, i) { + struct nv50_wndw_atom *asyw = nv50_wndw_atom(old_plane_state); struct nv50_wndw *wndw = nv50_wndw(plane); + if (asyw->set.image) { asyw->ntfy.handle = wndw->dmac->sync.handle; asyw->ntfy.offset = wndw->ntfy; @@ -4192,18 +4193,19 @@ nv50_disp_outp_atomic_add(struct nv50_atom *atom, struct drm_encoder *encoder)
static int nv50_disp_outp_atomic_check_clr(struct nv50_atom *atom, - struct drm_connector *connector) + struct drm_connector_state *old_connector_state) { - struct drm_encoder *encoder = connector->state->best_encoder; - struct drm_crtc_state *crtc_state; + struct drm_encoder *encoder = old_connector_state->best_encoder; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_crtc *crtc; struct nv50_outp_atom *outp;
- if (!(crtc = connector->state->crtc)) + if (!(crtc = old_connector_state->crtc)) return 0;
- crtc_state = drm_atomic_get_existing_crtc_state(&atom->state, crtc); - if (crtc->state->active && drm_atomic_crtc_needs_modeset(crtc_state)) { + old_crtc_state = drm_atomic_get_old_crtc_state(&atom->state, crtc); + new_crtc_state = drm_atomic_get_new_crtc_state(&atom->state, crtc); + if (old_crtc_state->active && drm_atomic_crtc_needs_modeset(new_crtc_state)) { outp = nv50_disp_outp_atomic_add(atom, encoder); if (IS_ERR(outp)) return PTR_ERR(outp); @@ -4224,15 +4226,15 @@ nv50_disp_outp_atomic_check_set(struct nv50_atom *atom, struct drm_connector_state *connector_state) { struct drm_encoder *encoder = connector_state->best_encoder; - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *new_crtc_state; struct drm_crtc *crtc; struct nv50_outp_atom *outp;
if (!(crtc = connector_state->crtc)) return 0;
- crtc_state = drm_atomic_get_existing_crtc_state(&atom->state, crtc); - if (crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state)) { + new_crtc_state = drm_atomic_get_new_crtc_state(&atom->state, crtc); + if (new_crtc_state->active && drm_atomic_crtc_needs_modeset(new_crtc_state)) { outp = nv50_disp_outp_atomic_add(atom, encoder); if (IS_ERR(outp)) return PTR_ERR(outp); @@ -4248,7 +4250,7 @@ static int nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { struct nv50_atom *atom = nv50_atom(state); - struct drm_connector_state *connector_state; + struct drm_connector_state *old_connector_state, *new_connector_state; struct drm_connector *connector; int ret, i;
@@ -4256,12 +4258,12 @@ nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) if (ret) return ret;
- for_each_connector_in_state(state, connector, connector_state, i) { - ret = nv50_disp_outp_atomic_check_clr(atom, connector); + for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) { + ret = nv50_disp_outp_atomic_check_clr(atom, old_connector_state); if (ret) return ret;
- ret = nv50_disp_outp_atomic_check_set(atom, connector_state); + ret = nv50_disp_outp_atomic_check_set(atom, new_connector_state); if (ret) return ret; }
On Wed, Jul 19, 2017 at 04:39:19PM +0200, Maarten Lankhorst wrote:
Use the new atomic iterator macros, the old ones are about to be removed. With the new macros, it's more easy to get old and new state so get them from the macros instead of from obj->state.
Changes since v1:
- Don't mix up old and new state. (danvet)
- Rebase on top of interruptible swap_state changes.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ben Skeggs bskeggs@redhat.com Cc: nouveau@lists.freedesktop.org
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/nouveau/nv50_display.c | 72 +++++++++++++++++----------------- 1 file changed, 37 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c index 747c99c1e474..7abfb561b00c 100644 --- a/drivers/gpu/drm/nouveau/nv50_display.c +++ b/drivers/gpu/drm/nouveau/nv50_display.c @@ -2103,7 +2103,7 @@ nv50_head_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state)
NV_ATOMIC(drm, "%s atomic_check %d\n", crtc->name, asyh->state.active); if (asyh->state.active) {
for_each_connector_in_state(asyh->state.state, conn, conns, i) {
for_each_new_connector_in_state(asyh->state.state, conn, conns, i) { if (conns->crtc == crtc) { asyc = nouveau_conn_atom(conns); break;@@ -3905,9 +3905,9 @@ static void nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) { struct drm_device *dev = state->dev;
- struct drm_crtc_state *crtc_state;
- struct drm_crtc_state *new_crtc_state; struct drm_crtc *crtc;
- struct drm_plane_state *plane_state;
- struct drm_plane_state *new_plane_state; struct drm_plane *plane; struct nouveau_drm *drm = nouveau_drm(dev); struct nv50_disp *disp = nv50_disp(dev);
@@ -3926,8 +3926,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) mutex_lock(&disp->mutex);
/* Disable head(s). */
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
struct nv50_head_atom *asyh = nv50_head_atom(crtc->state);
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
struct nv50_head_atom *asyh = nv50_head_atom(new_crtc_state);struct nv50_head *head = nv50_head(crtc);
NV_ATOMIC(drm, "%s: clr %04x (set %04x)\n", crtc->name,
@@ -3940,8 +3940,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) }
/* Disable plane(s). */
- for_each_plane_in_state(state, plane, plane_state, i) {
struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state);
for_each_new_plane_in_state(state, plane, new_plane_state, i) {
struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state);struct nv50_wndw *wndw = nv50_wndw(plane);
NV_ATOMIC(drm, "%s: clr %02x (set %02x)\n", plane->name,
@@ -4006,8 +4006,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) }
/* Update head(s). */
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
struct nv50_head_atom *asyh = nv50_head_atom(crtc->state);
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
struct nv50_head_atom *asyh = nv50_head_atom(new_crtc_state);struct nv50_head *head = nv50_head(crtc);
NV_ATOMIC(drm, "%s: set %04x (clr %04x)\n", crtc->name,
@@ -4019,14 +4019,14 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) } }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (crtc->state->event)
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
if (new_crtc_state->event) drm_crtc_vblank_get(crtc);}
/* Update plane(s). */
- for_each_plane_in_state(state, plane, plane_state, i) {
struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state);
for_each_new_plane_in_state(state, plane, new_plane_state, i) {
struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state);struct nv50_wndw *wndw = nv50_wndw(plane);
NV_ATOMIC(drm, "%s: set %02x (clr %02x)\n", plane->name,
@@ -4056,23 +4056,23 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) mutex_unlock(&disp->mutex);
/* Wait for HW to signal completion. */
- for_each_plane_in_state(state, plane, plane_state, i) {
struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state);
- for_each_new_plane_in_state(state, plane, new_plane_state, i) {
struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state);
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (crtc->state->event) {
- for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
if (new_crtc_state->event) { unsigned long flags; /* Get correct count/ts if racing with vblank irq */ drm_crtc_accurate_vblank_count(crtc); spin_lock_irqsave(&crtc->dev->event_lock, flags);
drm_crtc_send_vblank_event(crtc, crtc->state->event);
drm_crtc_send_vblank_event(crtc, new_crtc_state->event); spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
crtc->state->event = NULL;
new_crtc_state->event = NULL; drm_crtc_vblank_put(crtc);@@ -4097,7 +4097,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, { struct nouveau_drm *drm = nouveau_drm(dev); struct nv50_disp *disp = nv50_disp(dev);
- struct drm_plane_state *plane_state;
- struct drm_plane_state *old_plane_state; struct drm_plane *plane; struct drm_crtc *crtc; bool active = false;
@@ -4127,9 +4127,10 @@ nv50_disp_atomic_commit(struct drm_device *dev, if (ret) goto err_cleanup;
- for_each_plane_in_state(state, plane, plane_state, i) {
struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane_state);
- for_each_old_plane_in_state(state, plane, old_plane_state, i) {
struct nv50_wndw_atom *asyw = nv50_wndw_atom(old_plane_state);- if (asyw->set.image) { asyw->ntfy.handle = wndw->dmac->sync.handle; asyw->ntfy.offset = wndw->ntfy;
@@ -4192,18 +4193,19 @@ nv50_disp_outp_atomic_add(struct nv50_atom *atom, struct drm_encoder *encoder)
static int nv50_disp_outp_atomic_check_clr(struct nv50_atom *atom,
struct drm_connector *connector)
struct drm_connector_state *old_connector_state){
- struct drm_encoder *encoder = connector->state->best_encoder;
- struct drm_crtc_state *crtc_state;
- struct drm_encoder *encoder = old_connector_state->best_encoder;
- struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_crtc *crtc; struct nv50_outp_atom *outp;
- if (!(crtc = connector->state->crtc))
- if (!(crtc = old_connector_state->crtc)) return 0;
- crtc_state = drm_atomic_get_existing_crtc_state(&atom->state, crtc);
- if (crtc->state->active && drm_atomic_crtc_needs_modeset(crtc_state)) {
- old_crtc_state = drm_atomic_get_old_crtc_state(&atom->state, crtc);
- new_crtc_state = drm_atomic_get_new_crtc_state(&atom->state, crtc);
- if (old_crtc_state->active && drm_atomic_crtc_needs_modeset(new_crtc_state)) { outp = nv50_disp_outp_atomic_add(atom, encoder); if (IS_ERR(outp)) return PTR_ERR(outp);
@@ -4224,15 +4226,15 @@ nv50_disp_outp_atomic_check_set(struct nv50_atom *atom, struct drm_connector_state *connector_state) { struct drm_encoder *encoder = connector_state->best_encoder;
- struct drm_crtc_state *crtc_state;
struct drm_crtc_state *new_crtc_state; struct drm_crtc *crtc; struct nv50_outp_atom *outp;
if (!(crtc = connector_state->crtc)) return 0;
- crtc_state = drm_atomic_get_existing_crtc_state(&atom->state, crtc);
- if (crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state)) {
- new_crtc_state = drm_atomic_get_new_crtc_state(&atom->state, crtc);
- if (new_crtc_state->active && drm_atomic_crtc_needs_modeset(new_crtc_state)) { outp = nv50_disp_outp_atomic_add(atom, encoder); if (IS_ERR(outp)) return PTR_ERR(outp);
@@ -4248,7 +4250,7 @@ static int nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { struct nv50_atom *atom = nv50_atom(state);
- struct drm_connector_state *connector_state;
- struct drm_connector_state *old_connector_state, *new_connector_state; struct drm_connector *connector; int ret, i;
@@ -4256,12 +4258,12 @@ nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) if (ret) return ret;
- for_each_connector_in_state(state, connector, connector_state, i) {
ret = nv50_disp_outp_atomic_check_clr(atom, connector);
- for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
ret = nv50_disp_outp_atomic_check_clr(atom, old_connector_state);
ret = nv50_disp_outp_atomic_check_set(atom, connector_state);
ret = nv50_disp_outp_atomic_check_set(atom, new_connector_state);-- 2.11.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Now that the last users have been converted, we can finally get rid of for_each_obj_in_state, we have better macros to replace them with.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Sean Paul seanpaul@chromium.org Cc: David Airlie airlied@linux.ie --- include/drm/drm_atomic.h | 75 --------------------------------------------- include/drm/drm_connector.h | 3 +- include/drm/drm_crtc.h | 8 ++--- include/drm/drm_plane.h | 8 ++--- 4 files changed, 9 insertions(+), 85 deletions(-)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 7cd0f303f5a3..679e746f0459 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -563,31 +563,6 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
/** - * for_each_connector_in_state - iterate over all connectors in an atomic update - * @__state: &struct drm_atomic_state pointer - * @connector: &struct drm_connector iteration cursor - * @connector_state: &struct drm_connector_state iteration cursor - * @__i: int iteration cursor, for macro-internal use - * - * This iterates over all connectors in an atomic update. Note that before the - * software state is committed (by calling drm_atomic_helper_swap_state(), this - * points to the new state, while afterwards it points to the old state. Due to - * this tricky confusion this macro is deprecated. - * - * FIXME: - * - * Replace all usage of this with one of the explicit iterators below and then - * remove this macro. - */ -#define for_each_connector_in_state(__state, connector, connector_state, __i) \ - for ((__i) = 0; \ - (__i) < (__state)->num_connector && \ - ((connector) = (__state)->connectors[__i].ptr, \ - (connector_state) = (__state)->connectors[__i].state, 1); \ - (__i)++) \ - for_each_if (connector) - -/** * for_each_oldnew_connector_in_state - iterate over all connectors in an atomic update * @__state: &struct drm_atomic_state pointer * @connector: &struct drm_connector iteration cursor @@ -651,31 +626,6 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); for_each_if (connector)
/** - * for_each_crtc_in_state - iterate over all connectors in an atomic update - * @__state: &struct drm_atomic_state pointer - * @crtc: &struct drm_crtc iteration cursor - * @crtc_state: &struct drm_crtc_state iteration cursor - * @__i: int iteration cursor, for macro-internal use - * - * This iterates over all CRTCs in an atomic update. Note that before the - * software state is committed (by calling drm_atomic_helper_swap_state(), this - * points to the new state, while afterwards it points to the old state. Due to - * this tricky confusion this macro is deprecated. - * - * FIXME: - * - * Replace all usage of this with one of the explicit iterators below and then - * remove this macro. - */ -#define for_each_crtc_in_state(__state, crtc, crtc_state, __i) \ - for ((__i) = 0; \ - (__i) < (__state)->dev->mode_config.num_crtc && \ - ((crtc) = (__state)->crtcs[__i].ptr, \ - (crtc_state) = (__state)->crtcs[__i].state, 1); \ - (__i)++) \ - for_each_if (crtc_state) - -/** * for_each_oldnew_crtc_in_state - iterate over all CRTCs in an atomic update * @__state: &struct drm_atomic_state pointer * @crtc: &struct drm_crtc iteration cursor @@ -735,31 +685,6 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); for_each_if (crtc)
/** - * for_each_plane_in_state - iterate over all planes in an atomic update - * @__state: &struct drm_atomic_state pointer - * @plane: &struct drm_plane iteration cursor - * @plane_state: &struct drm_plane_state iteration cursor - * @__i: int iteration cursor, for macro-internal use - * - * This iterates over all planes in an atomic update. Note that before the - * software state is committed (by calling drm_atomic_helper_swap_state(), this - * points to the new state, while afterwards it points to the old state. Due to - * this tricky confusion this macro is deprecated. - * - * FIXME: - * - * Replace all usage of this with one of the explicit iterators below and then - * remove this macro. - */ -#define for_each_plane_in_state(__state, plane, plane_state, __i) \ - for ((__i) = 0; \ - (__i) < (__state)->dev->mode_config.num_total_plane && \ - ((plane) = (__state)->planes[__i].ptr, \ - (plane_state) = (__state)->planes[__i].state, 1); \ - (__i)++) \ - for_each_if (plane_state) - -/** * for_each_oldnew_plane_in_state - iterate over all planes in an atomic update * @__state: &struct drm_atomic_state pointer * @plane: &struct drm_plane iteration cursor diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 4bc088269d05..e3d89feb1def 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -890,8 +890,7 @@ struct drm_connector { * This is protected by @drm_mode_config.connection_mutex. Note that * nonblocking atomic commits access the current connector state without * taking locks. Either by going through the &struct drm_atomic_state - * pointers, see for_each_connector_in_state(), - * for_each_oldnew_connector_in_state(), + * pointers, see for_each_oldnew_connector_in_state(), * for_each_old_connector_in_state() and * for_each_new_connector_in_state(). Or through careful ordering of * atomic commit operations as implemented in the atomic helpers, see diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3a911a64c257..c4c949ea20da 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -807,10 +807,10 @@ struct drm_crtc { * This is protected by @mutex. Note that nonblocking atomic commits * access the current CRTC state without taking locks. Either by going * through the &struct drm_atomic_state pointers, see - * for_each_crtc_in_state(), for_each_oldnew_crtc_in_state(), - * for_each_old_crtc_in_state() and for_each_new_crtc_in_state(). Or - * through careful ordering of atomic commit operations as implemented - * in the atomic helpers, see &struct drm_crtc_commit. + * for_each_oldnew_crtc_in_state(), for_each_old_crtc_in_state() and + * for_each_new_crtc_in_state(). Or through careful ordering of atomic + * commit operations as implemented in the atomic helpers, see + * &struct drm_crtc_commit. */ struct drm_crtc_state *state;
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 9ab3e7044812..a1b3aa5d1223 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -514,10 +514,10 @@ struct drm_plane { * This is protected by @mutex. Note that nonblocking atomic commits * access the current plane state without taking locks. Either by going * through the &struct drm_atomic_state pointers, see - * for_each_plane_in_state(), for_each_oldnew_plane_in_state(), - * for_each_old_plane_in_state() and for_each_new_plane_in_state(). Or - * through careful ordering of atomic commit operations as implemented - * in the atomic helpers, see &struct drm_crtc_commit. + * for_each_oldnew_plane_in_state(), for_each_old_plane_in_state() and + * for_each_new_plane_in_state(). Or through careful ordering of atomic + * commit operations as implemented in the atomic helpers, see + * &struct drm_crtc_commit. */ struct drm_plane_state *state;
On Wed, Jul 19, 2017 at 04:39:20PM +0200, Maarten Lankhorst wrote:
Now that the last users have been converted, we can finally get rid of for_each_obj_in_state, we have better macros to replace them with.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Sean Paul seanpaul@chromium.org Cc: David Airlie airlied@linux.ie
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
include/drm/drm_atomic.h | 75 --------------------------------------------- include/drm/drm_connector.h | 3 +- include/drm/drm_crtc.h | 8 ++--- include/drm/drm_plane.h | 8 ++--- 4 files changed, 9 insertions(+), 85 deletions(-)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 7cd0f303f5a3..679e746f0459 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -563,31 +563,6 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
/**
- for_each_connector_in_state - iterate over all connectors in an atomic update
- @__state: &struct drm_atomic_state pointer
- @connector: &struct drm_connector iteration cursor
- @connector_state: &struct drm_connector_state iteration cursor
- @__i: int iteration cursor, for macro-internal use
- This iterates over all connectors in an atomic update. Note that before the
- software state is committed (by calling drm_atomic_helper_swap_state(), this
- points to the new state, while afterwards it points to the old state. Due to
- this tricky confusion this macro is deprecated.
- FIXME:
- Replace all usage of this with one of the explicit iterators below and then
- remove this macro.
- */
-#define for_each_connector_in_state(__state, connector, connector_state, __i) \
- for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(connector_state) = (__state)->connectors[__i].state, 1); \(__i)++) \for_each_if (connector)-/**
- for_each_oldnew_connector_in_state - iterate over all connectors in an atomic update
- @__state: &struct drm_atomic_state pointer
- @connector: &struct drm_connector iteration cursor
@@ -651,31 +626,6 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); for_each_if (connector)
/**
- for_each_crtc_in_state - iterate over all connectors in an atomic update
- @__state: &struct drm_atomic_state pointer
- @crtc: &struct drm_crtc iteration cursor
- @crtc_state: &struct drm_crtc_state iteration cursor
- @__i: int iteration cursor, for macro-internal use
- This iterates over all CRTCs in an atomic update. Note that before the
- software state is committed (by calling drm_atomic_helper_swap_state(), this
- points to the new state, while afterwards it points to the old state. Due to
- this tricky confusion this macro is deprecated.
- FIXME:
- Replace all usage of this with one of the explicit iterators below and then
- remove this macro.
- */
-#define for_each_crtc_in_state(__state, crtc, crtc_state, __i) \
- for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(crtc_state) = (__state)->crtcs[__i].state, 1); \(__i)++) \for_each_if (crtc_state)-/**
- for_each_oldnew_crtc_in_state - iterate over all CRTCs in an atomic update
- @__state: &struct drm_atomic_state pointer
- @crtc: &struct drm_crtc iteration cursor
@@ -735,31 +685,6 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); for_each_if (crtc)
/**
- for_each_plane_in_state - iterate over all planes in an atomic update
- @__state: &struct drm_atomic_state pointer
- @plane: &struct drm_plane iteration cursor
- @plane_state: &struct drm_plane_state iteration cursor
- @__i: int iteration cursor, for macro-internal use
- This iterates over all planes in an atomic update. Note that before the
- software state is committed (by calling drm_atomic_helper_swap_state(), this
- points to the new state, while afterwards it points to the old state. Due to
- this tricky confusion this macro is deprecated.
- FIXME:
- Replace all usage of this with one of the explicit iterators below and then
- remove this macro.
- */
-#define for_each_plane_in_state(__state, plane, plane_state, __i) \
- for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(plane_state) = (__state)->planes[__i].state, 1); \(__i)++) \for_each_if (plane_state)-/**
- for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
- @__state: &struct drm_atomic_state pointer
- @plane: &struct drm_plane iteration cursor
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 4bc088269d05..e3d89feb1def 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -890,8 +890,7 @@ struct drm_connector { * This is protected by @drm_mode_config.connection_mutex. Note that * nonblocking atomic commits access the current connector state without * taking locks. Either by going through the &struct drm_atomic_state
* pointers, see for_each_connector_in_state(),* for_each_oldnew_connector_in_state(),
* pointers, see for_each_oldnew_connector_in_state(),
- for_each_old_connector_in_state() and
- for_each_new_connector_in_state(). Or through careful ordering of
- atomic commit operations as implemented in the atomic helpers, see
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3a911a64c257..c4c949ea20da 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -807,10 +807,10 @@ struct drm_crtc { * This is protected by @mutex. Note that nonblocking atomic commits * access the current CRTC state without taking locks. Either by going * through the &struct drm_atomic_state pointers, see
* for_each_crtc_in_state(), for_each_oldnew_crtc_in_state(),* for_each_old_crtc_in_state() and for_each_new_crtc_in_state(). Or* through careful ordering of atomic commit operations as implemented* in the atomic helpers, see &struct drm_crtc_commit.
* for_each_oldnew_crtc_in_state(), for_each_old_crtc_in_state() and* for_each_new_crtc_in_state(). Or through careful ordering of atomic* commit operations as implemented in the atomic helpers, see* &struct drm_crtc_commit.diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 9ab3e7044812..a1b3aa5d1223 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -514,10 +514,10 @@ struct drm_plane { * This is protected by @mutex. Note that nonblocking atomic commits * access the current plane state without taking locks. Either by going * through the &struct drm_atomic_state pointers, see
* for_each_plane_in_state(), for_each_oldnew_plane_in_state(),* for_each_old_plane_in_state() and for_each_new_plane_in_state(). Or* through careful ordering of atomic commit operations as implemented* in the atomic helpers, see &struct drm_crtc_commit.
* for_each_oldnew_plane_in_state(), for_each_old_plane_in_state() and* for_each_new_plane_in_state(). Or through careful ordering of atomic* commit operations as implemented in the atomic helpers, see* &struct drm_crtc_commit.-- 2.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Laurent Pinchart -
Maarten Lankhorst