Re: [PATCH v3 4/7] drm/atomic: Fix atomic helpers to use the new iterator macros.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@linux.intel.com>
> ---
>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx