[PATCH] drm_atomic_helper: Copy/paste fix for calling already disabled planes

[Daniel Vetter]

On Thu, Nov 20, 2014 at 07:59:15PM -0800, Jasper St. Pierre wrote:
> This code was in drm_plane_helper, but missing from drm_atomic_helper,
> causing various crashes when the plane was already disabled. Just copy
> over the quick return there to prevent a crash.
> 
> Signed-off-by: Jasper St. Pierre <jstpierre at mecheye.net>
> Reviewed-by: Rob Clark <robdclark at gmail.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index fad2b93..d96dac3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1246,6 +1246,11 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane)
>  	struct drm_plane_state *plane_state;
>  	int ret = 0;
>  
> +	/* crtc helpers love to call disable functions for already disabled hw
> +	 * functions. So cope with that. */

Comment here is misleading since this isn't called by the crtc helpers but
directly by the drm core code to handle the setplane ioctl.

Now the real problem with this is that it's at the wrong spot. If we
really need to filter this then it should be done in the atomic_commit
function as a noop and not here. This is just the legacy ->disable_plane
entry hook, userspace could still throw multiple disable plane calls at
the driver. And they would get past this check.

As-is it's actually a design feature of the atomic plane helpers that they
don't filter out any no-op updates, but just pass the new state into the
->atomic_update hook (through the plane->state pointer). We could extend
that (Thierry is iirc working ona an ->atomic_disable callback), and then
it would make sense to have some filtering in the helpers. But if we add
that it must be done in drm_atomic_helper_commit_planes not here.

So NACKed from me.
-Daniel

> +	if (!plane->crtc)
> +		return 0;
> +
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch