Re: [PATCH 1/6] Revert "drm/atomic-helper: Fix leak in disable_all"

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Mar 20, 2018 at 09:17:52PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we're leaking fbs on load detect on account of nothing setting
> up plane->old_fb for the drm_atomic_clean_old_fb() call in
> drm_atomic_helper_commit_duplicated_state(). Removing the
> drm_atomic_clean_old_fb() call seems like the right call to me here.
> This does mean we end up leaking something via
> drm_atomic_helper_shutdown() though, but we'll fix that up in a
> different way.
> 
> This reverts commit 49d70aeaeca8f62b72b7712ecd1e29619a445866.

So the reason I went this way and not what you're suggesting in this patch
series is that i915 is the one and only driver noodling around with load
detect and gpu reset that needs this.

Imo it'd be better to fix up our load detect code to also set the old_fb
stuff up correctly, and add at least a long-term task to todo.rst to get
rid of all this. Inflicting a new parameter on all the other drives (like
you do in patch 2) is imo the wrong way round - we have 31 other atomic
drivers than i915.ko.
-Daniel

> 
> Cc: martin.peres@free.fr
> Cc: chris@chris-wilson.co.uk
> Cc: Dave Airlie <airlied@gmail.com> (v1)
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 18 ++----------------
>  1 file changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c35654591c12..c48f187d08de 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2914,7 +2914,6 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>  	struct drm_plane *plane;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc *crtc;
> -	unsigned plane_mask = 0;
>  	int ret, i;
>  
>  	state = drm_atomic_state_alloc(dev);
> @@ -2957,14 +2956,10 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>  			goto free;
>  
>  		drm_atomic_set_fb_for_plane(plane_state, NULL);
> -		plane_mask |= BIT(drm_plane_index(plane));
> -		plane->old_fb = plane->fb;
>  	}
>  
>  	ret = drm_atomic_commit(state);
>  free:
> -	if (plane_mask)
> -		drm_atomic_clean_old_fb(dev, plane_mask, ret);
>  	drm_atomic_state_put(state);
>  	return ret;
>  }
> @@ -3095,16 +3090,11 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
>  	struct drm_connector_state *new_conn_state;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *new_crtc_state;
> -	unsigned plane_mask = 0;
> -	struct drm_device *dev = state->dev;
> -	int ret;
>  
>  	state->acquire_ctx = ctx;
>  
> -	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> -		plane_mask |= BIT(drm_plane_index(plane));
> +	for_each_new_plane_in_state(state, plane, new_plane_state, i)
>  		state->planes[i].old_state = plane->state;
> -	}
>  
>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
>  		state->crtcs[i].old_state = crtc->state;
> @@ -3112,11 +3102,7 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
>  	for_each_new_connector_in_state(state, connector, new_conn_state, i)
>  		state->connectors[i].old_state = connector->state;
>  
> -	ret = drm_atomic_commit(state);
> -	if (plane_mask)
> -		drm_atomic_clean_old_fb(dev, plane_mask, ret);
> -
> -	return ret;
> +	return drm_atomic_commit(state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);
>  
> -- 
> 2.16.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx