It can be seen in drm_atomic_get_private_obj_state() that ptr will never be NULL, so skip the check for that case.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 3 --- include/drm/drm_atomic.h | 9 +++------ 2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8bd5a1c4c7e8..29525bffdad9 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -182,9 +182,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) for (i = 0; i < state->num_private_objs; i++) { struct drm_private_obj *obj = state->private_objs[i].ptr;
- if (!obj) - continue; - obj->funcs->atomic_destroy_state(obj, state->private_objs[i].state); state->private_objs[i].ptr = NULL; diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 5834580d75bc..6fae95f28e10 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -768,8 +768,7 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); ((obj) = (__state)->private_objs[__i].ptr, \ (old_obj_state) = (__state)->private_objs[__i].old_state, \ (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \ - (__i)++) \ - for_each_if (obj) + (__i)++)
/** * for_each_old_private_obj_in_state - iterate over all private objects in an atomic update @@ -787,8 +786,7 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i) < (__state)->num_private_objs && \ ((obj) = (__state)->private_objs[__i].ptr, \ (old_obj_state) = (__state)->private_objs[__i].old_state, 1); \ - (__i)++) \ - for_each_if (obj) + (__i)++)
/** * for_each_new_private_obj_in_state - iterate over all private objects in an atomic update @@ -806,8 +804,7 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i) < (__state)->num_private_objs && \ ((obj) = (__state)->private_objs[__i].ptr, \ (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \ - (__i)++) \ - for_each_if (obj) + (__i)++)
/** * drm_atomic_crtc_needs_modeset - compute combined modeset need
Commit 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") assumed incorrectly that if only 1 plane is matched in the loop, the variables will be set to that plane. In reality we reset them to NULL every time a new plane was iterated. This behavior is surprising, so fix this by making the for loops only assign the variables on a match.
Cc: Dmitry Osipenko digetx@gmail.com Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- include/drm/drm_atomic.h | 85 ++++++++++++++++++++++++------------------------ 1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 6fae95f28e10..5afd6e364fb6 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -585,12 +585,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \ for ((__i) = 0; \ - (__i) < (__state)->num_connector && \ - ((connector) = (__state)->connectors[__i].ptr, \ - (old_connector_state) = (__state)->connectors[__i].old_state, \ - (new_connector_state) = (__state)->connectors[__i].new_state, 1); \ - (__i)++) \ - for_each_if (connector) + (__i) < (__state)->num_connector; \ + (__i)++) \ + for_each_if ((__state)->connectors[__i].ptr && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (old_connector_state) = (__state)->connectors[__i].old_state, \ + (new_connector_state) = (__state)->connectors[__i].new_state, 1))
/** * for_each_old_connector_in_state - iterate over all connectors in an atomic update @@ -606,11 +606,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ for ((__i) = 0; \ - (__i) < (__state)->num_connector && \ - ((connector) = (__state)->connectors[__i].ptr, \ - (old_connector_state) = (__state)->connectors[__i].old_state, 1); \ - (__i)++) \ - for_each_if (connector) + (__i) < (__state)->num_connector; \ + (__i)++) \ + for_each_if ((__state)->connectors[__i].ptr && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (old_connector_state) = (__state)->connectors[__i].old_state, 1))
/** * for_each_new_connector_in_state - iterate over all connectors in an atomic update @@ -626,11 +626,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ for ((__i) = 0; \ - (__i) < (__state)->num_connector && \ - ((connector) = (__state)->connectors[__i].ptr, \ - (new_connector_state) = (__state)->connectors[__i].new_state, 1); \ - (__i)++) \ - for_each_if (connector) + (__i) < (__state)->num_connector; \ + (__i)++) \ + for_each_if ((__state)->connectors[__i].ptr && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (new_connector_state) = (__state)->connectors[__i].new_state, 1))
/** * for_each_oldnew_crtc_in_state - iterate over all CRTCs in an atomic update @@ -646,12 +646,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ for ((__i) = 0; \ - (__i) < (__state)->dev->mode_config.num_crtc && \ - ((crtc) = (__state)->crtcs[__i].ptr, \ - (old_crtc_state) = (__state)->crtcs[__i].old_state, \ - (new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \ + (__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \ - for_each_if (crtc) + for_each_if ((__state)->crtcs[__i].ptr && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (old_crtc_state) = (__state)->crtcs[__i].old_state, \ + (new_crtc_state) = (__state)->crtcs[__i].new_state, 1))
/** * for_each_old_crtc_in_state - iterate over all CRTCs in an atomic update @@ -666,11 +666,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i) \ for ((__i) = 0; \ - (__i) < (__state)->dev->mode_config.num_crtc && \ - ((crtc) = (__state)->crtcs[__i].ptr, \ - (old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \ + (__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \ - for_each_if (crtc) + for_each_if ((__state)->crtcs[__i].ptr && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (old_crtc_state) = (__state)->crtcs[__i].old_state, 1))
/** * for_each_new_crtc_in_state - iterate over all CRTCs in an atomic update @@ -685,11 +685,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i) \ for ((__i) = 0; \ - (__i) < (__state)->dev->mode_config.num_crtc && \ - ((crtc) = (__state)->crtcs[__i].ptr, \ - (new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \ + (__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \ - for_each_if (crtc) + for_each_if ((__state)->crtcs[__i].ptr && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (new_crtc_state) = (__state)->crtcs[__i].new_state, 1))
/** * for_each_oldnew_plane_in_state - iterate over all planes in an atomic update @@ -705,12 +705,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ for ((__i) = 0; \ - (__i) < (__state)->dev->mode_config.num_total_plane && \ - ((plane) = (__state)->planes[__i].ptr, \ - (old_plane_state) = (__state)->planes[__i].old_state, \ - (new_plane_state) = (__state)->planes[__i].new_state, 1); \ + (__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \ - for_each_if (plane) + for_each_if ((__state)->planes[__i].ptr && \ + ((plane) = (__state)->planes[__i].ptr, \ + (old_plane_state) = (__state)->planes[__i].old_state,\ + (new_plane_state) = (__state)->planes[__i].new_state, 1))
/** * for_each_old_plane_in_state - iterate over all planes in an atomic update @@ -725,12 +725,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \ for ((__i) = 0; \ - (__i) < (__state)->dev->mode_config.num_total_plane && \ - ((plane) = (__state)->planes[__i].ptr, \ - (old_plane_state) = (__state)->planes[__i].old_state, 1); \ + (__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \ - for_each_if (plane) - + for_each_if ((__state)->planes[__i].ptr && \ + ((plane) = (__state)->planes[__i].ptr, \ + (old_plane_state) = (__state)->planes[__i].old_state, 1)) /** * for_each_new_plane_in_state - iterate over all planes in an atomic update * @__state: &struct drm_atomic_state pointer @@ -744,11 +743,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \ for ((__i) = 0; \ - (__i) < (__state)->dev->mode_config.num_total_plane && \ - ((plane) = (__state)->planes[__i].ptr, \ - (new_plane_state) = (__state)->planes[__i].new_state, 1); \ + (__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \ - for_each_if (plane) + for_each_if ((__state)->planes[__i].ptr && \ + ((plane) = (__state)->planes[__i].ptr, \ + (new_plane_state) = (__state)->planes[__i].new_state, 1))
/** * for_each_oldnew_private_obj_in_state - iterate over all private objects in an atomic update
On Wed, Sep 27, 2017 at 10:35:32AM +0200, Maarten Lankhorst wrote:
Commit 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") assumed incorrectly that if only 1 plane is matched in the loop, the variables will be set to that plane. In reality we reset them to NULL every time a new plane was iterated. This behavior is surprising, so fix this by making the for loops only assign the variables on a match.
Maybe explain a bit more how that's confusing, namely when we have not added all the planes/crtc/connector to the state, and there's a few NULL ones after the last one we iterated. Which then breaks the assumption that the pointers will hold the values from the last loop iteration, which holds true for all other for_each macros we're using (well except the iterator pointer itself, but that one really is entirely internal).
With that bit of blabla added to the commit message:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Cc: Dmitry Osipenko digetx@gmail.com Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
include/drm/drm_atomic.h | 85 ++++++++++++++++++++++++------------------------ 1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 6fae95f28e10..5afd6e364fb6 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -585,12 +585,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, \(new_connector_state) = (__state)->connectors[__i].new_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, \(new_connector_state) = (__state)->connectors[__i].new_state, 1))/**
- for_each_old_connector_in_state - iterate over all connectors in an atomic update
@@ -606,11 +606,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, 1))/**
- for_each_new_connector_in_state - iterate over all connectors in an atomic update
@@ -626,11 +626,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(new_connector_state) = (__state)->connectors[__i].new_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(new_connector_state) = (__state)->connectors[__i].new_state, 1))/**
- for_each_oldnew_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -646,12 +646,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1))/**
- for_each_old_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -666,11 +666,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, 1))/**
- for_each_new_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -685,11 +685,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1))/**
- for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
@@ -705,12 +705,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state,\(new_plane_state) = (__state)->planes[__i].new_state, 1))/**
- for_each_old_plane_in_state - iterate over all planes in an atomic update
@@ -725,12 +725,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, 1))/**
- for_each_new_plane_in_state - iterate over all planes in an atomic update
- @__state: &struct drm_atomic_state pointer
@@ -744,11 +743,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(new_plane_state) = (__state)->planes[__i].new_state, 1))/**
- for_each_oldnew_private_obj_in_state - iterate over all private objects in an atomic update
-- 2.14.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 27.09.2017 11:35, Maarten Lankhorst wrote:
Commit 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") assumed incorrectly that if only 1 plane is matched in the loop, the variables will be set to that plane. In reality we reset them to NULL every time a new plane was iterated. This behavior is surprising, so fix this by making the for loops only assign the variables on a match.
Cc: Dmitry Osipenko digetx@gmail.com Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
include/drm/drm_atomic.h | 85 ++++++++++++++++++++++++------------------------ 1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 6fae95f28e10..5afd6e364fb6 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -585,12 +585,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, \(new_connector_state) = (__state)->connectors[__i].new_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, \(new_connector_state) = (__state)->connectors[__i].new_state, 1))/**
- for_each_old_connector_in_state - iterate over all connectors in an atomic update
@@ -606,11 +606,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, 1))/**
- for_each_new_connector_in_state - iterate over all connectors in an atomic update
@@ -626,11 +626,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(new_connector_state) = (__state)->connectors[__i].new_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(new_connector_state) = (__state)->connectors[__i].new_state, 1))/**
- for_each_oldnew_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -646,12 +646,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1))/**
- for_each_old_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -666,11 +666,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, 1))/**
- for_each_new_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -685,11 +685,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1))/**
- for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
@@ -705,12 +705,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state,\(new_plane_state) = (__state)->planes[__i].new_state, 1))/**
- for_each_old_plane_in_state - iterate over all planes in an atomic update
@@ -725,12 +725,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, 1))/**
- for_each_new_plane_in_state - iterate over all planes in an atomic update
- @__state: &struct drm_atomic_state pointer
@@ -744,11 +743,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(new_plane_state) = (__state)->planes[__i].new_state, 1))/**
- for_each_oldnew_private_obj_in_state - iterate over all private objects in an atomic update
This variant also works.
Tested-by: Dmitry Osipenko digetx@gmail.com
Op 27-09-17 om 14:53 schreef Dmitry Osipenko:
On 27.09.2017 11:35, Maarten Lankhorst wrote:
Commit 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") assumed incorrectly that if only 1 plane is matched in the loop, the variables will be set to that plane. In reality we reset them to NULL every time a new plane was iterated. This behavior is surprising, so fix this by making the for loops only assign the variables on a match.
Cc: Dmitry Osipenko digetx@gmail.com Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
include/drm/drm_atomic.h | 85 ++++++++++++++++++++++++------------------------ 1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 6fae95f28e10..5afd6e364fb6 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -585,12 +585,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, \(new_connector_state) = (__state)->connectors[__i].new_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, \(new_connector_state) = (__state)->connectors[__i].new_state, 1))/**
- for_each_old_connector_in_state - iterate over all connectors in an atomic update
@@ -606,11 +606,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, 1))/**
- for_each_new_connector_in_state - iterate over all connectors in an atomic update
@@ -626,11 +626,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(new_connector_state) = (__state)->connectors[__i].new_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(new_connector_state) = (__state)->connectors[__i].new_state, 1))/**
- for_each_oldnew_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -646,12 +646,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1))/**
- for_each_old_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -666,11 +666,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, 1))/**
- for_each_new_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -685,11 +685,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1))/**
- for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
@@ -705,12 +705,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state,\(new_plane_state) = (__state)->planes[__i].new_state, 1))/**
- for_each_old_plane_in_state - iterate over all planes in an atomic update
@@ -725,12 +725,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, 1))/**
- for_each_new_plane_in_state - iterate over all planes in an atomic update
- @__state: &struct drm_atomic_state pointer
@@ -744,11 +743,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(new_plane_state) = (__state)->planes[__i].new_state, 1))/**
- for_each_oldnew_private_obj_in_state - iterate over all private objects in an atomic update
This variant also works.
Tested-by: Dmitry Osipenko digetx@gmail.com
Forgot to hit push, pushed now. :)
On 06.10.2017 12:07, Maarten Lankhorst wrote:
Op 27-09-17 om 14:53 schreef Dmitry Osipenko:
On 27.09.2017 11:35, Maarten Lankhorst wrote:
Commit 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") assumed incorrectly that if only 1 plane is matched in the loop, the variables will be set to that plane. In reality we reset them to NULL every time a new plane was iterated. This behavior is surprising, so fix this by making the for loops only assign the variables on a match.
Cc: Dmitry Osipenko digetx@gmail.com Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
include/drm/drm_atomic.h | 85 ++++++++++++++++++++++++------------------------ 1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 6fae95f28e10..5afd6e364fb6 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -585,12 +585,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, \(new_connector_state) = (__state)->connectors[__i].new_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, \(new_connector_state) = (__state)->connectors[__i].new_state, 1))/**
- for_each_old_connector_in_state - iterate over all connectors in an atomic update
@@ -606,11 +606,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(old_connector_state) = (__state)->connectors[__i].old_state, 1))/**
- for_each_new_connector_in_state - iterate over all connectors in an atomic update
@@ -626,11 +626,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->num_connector && \((connector) = (__state)->connectors[__i].ptr, \(new_connector_state) = (__state)->connectors[__i].new_state, 1); \(__i)++) \for_each_if (connector)
(__i) < (__state)->num_connector; \(__i)++) \for_each_if ((__state)->connectors[__i].ptr && \((connector) = (__state)->connectors[__i].ptr, \(new_connector_state) = (__state)->connectors[__i].new_state, 1))/**
- for_each_oldnew_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -646,12 +646,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1))/**
- for_each_old_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -666,11 +666,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, 1))/**
- for_each_new_crtc_in_state - iterate over all CRTCs in an atomic update
@@ -685,11 +685,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \
for_each_if (crtc)
for_each_if ((__state)->crtcs[__i].ptr && \((crtc) = (__state)->crtcs[__i].ptr, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1))/**
- for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
@@ -705,12 +705,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state,\(new_plane_state) = (__state)->planes[__i].new_state, 1))/**
- for_each_old_plane_in_state - iterate over all planes in an atomic update
@@ -725,12 +725,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, 1))/**
- for_each_new_plane_in_state - iterate over all planes in an atomic update
- @__state: &struct drm_atomic_state pointer
@@ -744,11 +743,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); */ #define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \ for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \
(__i) < (__state)->dev->mode_config.num_total_plane; \ (__i)++) \
for_each_if (plane)
for_each_if ((__state)->planes[__i].ptr && \((plane) = (__state)->planes[__i].ptr, \(new_plane_state) = (__state)->planes[__i].new_state, 1))/**
- for_each_oldnew_private_obj_in_state - iterate over all private objects in an atomic update
This variant also works.
Tested-by: Dmitry Osipenko digetx@gmail.com
Forgot to hit push, pushed now. :)
Awesome, thank you :)
On Wed, Sep 27, 2017 at 10:35:31AM +0200, Maarten Lankhorst wrote:
It can be seen in drm_atomic_get_private_obj_state() that ptr will never be NULL, so skip the check for that case.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_atomic.c | 3 --- include/drm/drm_atomic.h | 9 +++------ 2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8bd5a1c4c7e8..29525bffdad9 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -182,9 +182,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) for (i = 0; i < state->num_private_objs; i++) { struct drm_private_obj *obj = state->private_objs[i].ptr;
if (!obj)continue;- obj->funcs->atomic_destroy_state(obj, state->private_objs[i].state); state->private_objs[i].ptr = NULL;
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 5834580d75bc..6fae95f28e10 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -768,8 +768,7 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); ((obj) = (__state)->private_objs[__i].ptr, \ (old_obj_state) = (__state)->private_objs[__i].old_state, \ (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \
(__i)++) \for_each_if (obj)
(__i)++)/**
- for_each_old_private_obj_in_state - iterate over all private objects in an atomic update
@@ -787,8 +786,7 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i) < (__state)->num_private_objs && \ ((obj) = (__state)->private_objs[__i].ptr, \ (old_obj_state) = (__state)->private_objs[__i].old_state, 1); \
(__i)++) \for_each_if (obj)
(__i)++)/**
- for_each_new_private_obj_in_state - iterate over all private objects in an atomic update
@@ -806,8 +804,7 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i) < (__state)->num_private_objs && \ ((obj) = (__state)->private_objs[__i].ptr, \ (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \
(__i)++) \for_each_if (obj)
(__i)++)/**
- drm_atomic_crtc_needs_modeset - compute combined modeset need
-- 2.14.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Dmitry Osipenko -
Maarten Lankhorst