On Fri, Nov 21, 2014 at 3:31 AM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxx> >> Reviewed-by: Rob Clark <robdclark@xxxxxxxxx> >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
