Hi Maarten,
(CC'ing Daniel)
Thank you for the patch.
On Monday 16 Jan 2017 10:37:41 Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++++---------------
> 1 file changed, 152 insertions(+), 141 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
[snip]
> @@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state,
> {
> struct drm_crtc_state *crtc_state;
> struct drm_connector *connector;
> - struct drm_connector_state *connector_state;
> + struct drm_connector_state *old_connector_state, *new_connector_state;
The kernel favours one variable declaration per line, and I think this file
does as well, at least mostly (this comment holds for the rest of this patch
and the other patches in the series).
> int i;
>
> - for_each_connector_in_state(state, connector, connector_state, i) {
> + for_each_oldnew_connector_in_state(state, connector,
> old_connector_state, new_connector_state, i) {
This is getting quite long, you could wrap the line.
> struct drm_crtc *encoder_crtc;
>
> - if (connector_state->best_encoder != encoder)
> + if (new_connector_state->best_encoder != encoder)
> continue;
>
> - encoder_crtc = connector->state->crtc;
> + encoder_crtc = old_connector_state->crtc;
>
> DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s],
stealing it\n",
> encoder->base.id, encoder->name,
> encoder_crtc->base.id, encoder_crtc->name);
>
> - set_best_encoder(state, connector_state, NULL);
> + set_best_encoder(state, new_connector_state, NULL);
>
> crtc_state = drm_atomic_get_existing_crtc_state(state,
encoder_crtc);
> crtc_state->connectors_changed = true;
[snip]
> @@ -516,14 +518,15 @@ drm_atomic_helper_check_modeset(struct drm_device
> *dev, if (ret)
> return ret;
>
> - for_each_connector_in_state(state, connector, connector_state, i) {
> + for_each_oldnew_connector_in_state(state, connector,
> old_connector_state, new_connector_state, i) {
Line wrap here too ?
> /*
> * This only sets crtc->connectors_changed for routing
changes,
> * drivers must set crtc->connectors_changed themselves when
> * connector properties need to be updated.
> */
> ret = update_connector_routing(state, connector,
> - connector_state);
> + old_connector_state,
> + new_connector_state);
> if (ret)
> return ret;
> }
[snip]
> @@ -599,15 +602,15 @@ drm_atomic_helper_check_planes(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 *plane_state, *old_plane_state;
In some location you use new_*_state and in others *_state. I believe this
should this be standardized to avoid confusion (the code is hard enough to
read as it is :-)).
Similarly, you sometimes use *_conn_state and sometimes *_connector_state.
That should be standardized as well.
> int i, ret = 0;
>
> - for_each_plane_in_state(state, plane, plane_state, i) {
> + for_each_oldnew_plane_in_state(state, plane, old_plane_state,
> plane_state, i) {
> const struct drm_plane_helper_funcs *funcs;
>
> funcs = plane->helper_private;
>
> - drm_atomic_helper_plane_changed(state, plane_state, plane);
> + drm_atomic_helper_plane_changed(state, old_plane_state,
> plane_state, plane);
>
> if (!funcs || !funcs->atomic_check)
> continue;
[snip]
> @@ -974,18 +977,18 @@ void drm_atomic_helper_commit_modeset_enables(struct
> drm_device *dev, }
> }
>
> - for_each_connector_in_state(old_state, connector, old_conn_state, i) {
> + for_each_new_connector_in_state(old_state, connector, conn_state, i) {
Not strictly related to this patch, but I've been wondering how this works,
given that we need to loop over connectors in the new state, not the old
state. After some investigation I've realized that both the old and new states
contain the same list of objects, as we don't keep a copy of the old global
atomic state. old_state was the new state before the state swap, and the swap
operation sets state->/objects/[*].state to the old state for all objects, but
doesn't modify the object pointers. This is possibly more of a question for
Daniel, should this be captured in the documentation somewhere (and is my
understanding correct) ?
> const struct drm_encoder_helper_funcs *funcs;
> struct drm_encoder *encoder;
>
> - if (!connector->state->best_encoder)
> + if (!conn_state->best_encoder)
> continue;
>
> - if (!connector->state->crtc->state->active ||
> - !drm_atomic_crtc_needs_modeset(connector->state->crtc-
>state))
> + if (!conn_state->crtc->state->active ||
> + !drm_atomic_crtc_needs_modeset(conn_state->crtc->state))
> continue;
>
> - encoder = connector->state->best_encoder;
> + encoder = conn_state->best_encoder;
> funcs = encoder->helper_private;
>
> DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
[snip]
> @@ -1921,12 +1919,19 @@ void drm_atomic_helper_cleanup_planes(struct
> drm_device *dev, struct drm_atomic_state *old_state)
> {
> struct drm_plane *plane;
> - struct drm_plane_state *plane_state;
> + struct drm_plane_state *plane_state, *new_plane_state;
> int i;
>
> - for_each_plane_in_state(old_state, plane, plane_state, i) {
> + for_each_oldnew_plane_in_state(old_state, plane, plane_state,
> new_plane_state, i) {
> const struct drm_plane_helper_funcs *funcs;
>
> + /*
> + * This might be called before swapping when commit is
aborted,
> + * in which case we have to free the new state.
Should this be "cleanup the new state" instead of "free the new state" ?
> + */
> + if (plane_state == plane->state)
> + plane_state = new_plane_state;
> +
> funcs = plane->helper_private;
>
> if (funcs->cleanup_fb)
[snip]
--
Regards,
Laurent Pinchart
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
