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

[Daniel Vetter]

On Fri, Nov 21, 2014 at 07:17:48AM -0500, Rob Clark wrote:
> 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..

For a) atm atomic helpers just don't filter that out, and imo the core
should (since it's really tricky to figure out whether userspace has done
an elaborate noop ioctl when there's driver-specific properties and maybe
some autodetection involved).

For b) the commit message completely failed to mention the Oops. The fix
for that is


diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
index 474e4d12a2d8..93d28269e3bd 100644
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -209,7 +209,7 @@ EXPORT_SYMBOL(drm_modeset_lock_crtc);
 struct drm_modeset_acquire_ctx *
 drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc)
 {
-	if (crtc->acquire_ctx)
+	if (crtc && crtc->acquire_ctx)
 		return crtc->acquire_ctx;
 
 	WARN_ON(!crtc->dev->mode_config.acquire_ctx);

I somehow missed that implication of the "pick crtc acquire context or if
that's not there the global one".
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch