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

[Rob Clark]

On Fri, Nov 21, 2014 at 3:31 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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.

I guess I'm (a) not entirely sure what the point of a no-op disable
is, and (b) quite sure how you plane to make a
'drm_modeset_legacy_acquire_ctx(plane->crtc)' work if plane->crtc is
NULL..

BR,
-R


> 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
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel