On 25.09.2017 09:43, Maarten Lankhorst wrote: > Op 24-09-17 om 16:33 schreef Dmitry Osipenko: >> On 04.09.2017 13:48, Maarten Lankhorst wrote: >>> By always keeping track of the last commit in plane_state, we know >>> whether there is an active update on the plane or not. With that >>> information we can reject the fast update, and force the slowpath >>> to be used as was originally intended. >>> >>> We cannot use plane_state->crtc->state here, because this only mentions >>> the most recent commit for the crtc, but not the planes that were part >>> of it. We specifically care about what the last commit involving this >>> plane is, which can only be tracked with a pointer in the plane state. >>> >>> Changes since v1: >>> - Clean up the whole function here, instead of partially earlier. >>> - Add mention in the commit message why we need commit in plane_state. >>> - Swap plane->state in intel_legacy_cursor_update, instead of >>> reassigning all variables. With this commit We know that the cursor >>> is not part of any active commits so this hack can be removed. >>> >>> Cc: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >>> Reviewed-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx> >>> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> #v1 >>> --- >>> drivers/gpu/drm/drm_atomic_helper.c | 53 ++++++++++-------------------------- >>> drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++------- >>> include/drm/drm_plane.h | 5 ++-- >>> 3 files changed, 35 insertions(+), 50 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>> index c81d46927a74..a2cd432d8b2d 100644 >>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>> @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev, >>> { >>> struct drm_crtc *crtc; >>> struct drm_crtc_state *crtc_state; >>> - struct drm_crtc_commit *commit; >>> - struct drm_plane *__plane, *plane = NULL; >>> - struct drm_plane_state *__plane_state, *plane_state = NULL; >>> + struct drm_plane *plane; >>> + struct drm_plane_state *old_plane_state, *new_plane_state; >>> const struct drm_plane_helper_funcs *funcs; >>> - int i, j, n_planes = 0; >>> + int i, n_planes = 0; >>> >>> for_each_new_crtc_in_state(state, crtc, crtc_state, i) { >>> if (drm_atomic_crtc_needs_modeset(crtc_state)) >>> return -EINVAL; >>> } >>> >>> - for_each_new_plane_in_state(state, __plane, __plane_state, i) { >>> + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) >>> n_planes++; >>> - plane = __plane; >>> - plane_state = __plane_state; >>> - } >>> >>> /* FIXME: we support only single plane updates for now */ >>> - if (!plane || n_planes != 1) >>> + if (n_planes != 1) >>> return -EINVAL; >>> >>> - if (!plane_state->crtc) >>> + if (!new_plane_state->crtc) >>> return -EINVAL; >>> >> Hello, >> >> It looks like a NULL-checking of new_plane_state is missed here. >> >> [ 70.138947] [<c03dd670>] (drm_atomic_helper_async_check) from [<c03e0424>] >> (drm_atomic_helper_check+0x4c/0x64) >> [ 70.138958] [<c03e0424>] (drm_atomic_helper_check) from [<c03fb6d4>] >> (drm_atomic_check_only+0x2f4/0x56c) >> [ 70.138969] [<c03fb6d4>] (drm_atomic_check_only) from [<c03fb95c>] >> (drm_atomic_commit+0x10/0x50) >> [ 70.138979] [<c03fb95c>] (drm_atomic_commit) from [<c03dde50>] >> (drm_atomic_helper_update_plane+0xf0/0x100) >> [ 70.138991] [<c03dde50>] (drm_atomic_helper_update_plane) from [<c0403548>] >> (__setplane_internal+0x178/0x214) >> [ 70.139003] [<c0403548>] (__setplane_internal) from [<c04036fc>] >> (drm_mode_cursor_universal+0x118/0x1a8) >> [ 70.139014] [<c04036fc>] (drm_mode_cursor_universal) from [<c0403900>] >> (drm_mode_cursor_common+0x174/0x1ec) >> [ 70.139026] [<c0403900>] (drm_mode_cursor_common) from [<c0403fa4>] >> (drm_mode_cursor_ioctl+0x58/0x60) >> [ 70.139036] [<c0403fa4>] (drm_mode_cursor_ioctl) from [<c03eb0b4>] >> (drm_ioctl+0x198/0x368) >> [ 70.139047] [<c03eb0b4>] (drm_ioctl) from [<c015fffc>] (do_vfs_ioctl+0x9c/0x8f0) >> [ 70.139058] [<c015fffc>] (do_vfs_ioctl) from [<c0160884>] (SyS_ioctl+0x34/0x5c) >> [ 70.139071] [<c0160884>] (SyS_ioctl) from [<c000f900>] >> (ret_fast_syscall+0x0/0x48) >> >> It's triggered by Tegra DRM driver on Xorg's startup. I also should notice that >> currently Tegra DRM doesn't have a working HW cursor, I'm going to send out >> Tegra cursor patches sometime soon. > > Oops.. I messed up there.. for_each_new_plane_in_state overwrites the state internally.. > ----->8----- > for_each_oldnew_plane_in_state overwrites the iterator values internally, so we cannot rely > on it being set to the last valid plane. This was causing a NULL pointer deref when someone > tries to use the code. Save the plane and use the accessor functions to pull out the relevant > plane state. > > Cc: Dmitry Osipenko <digetx@xxxxxxxxx> > Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.") > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > --- It works! Thank you. Tested-by: Dmitry Osipenko <digetx@xxxxxxxxx> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 32fb61169b4f..8573feaea8c0 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1388,23 +1388,30 @@ int drm_atomic_helper_async_check(struct drm_device *dev, > { > struct drm_crtc *crtc; > struct drm_crtc_state *crtc_state; > - struct drm_plane *plane; > + struct drm_plane *plane = NULL, *__plane; > struct drm_plane_state *old_plane_state, *new_plane_state; > const struct drm_plane_helper_funcs *funcs; > - int i, n_planes = 0; > + int i; > > for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > if (drm_atomic_crtc_needs_modeset(crtc_state)) > return -EINVAL; > } > > - for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) > - n_planes++; > + for_each_new_plane_in_state(state, __plane, new_plane_state, i) { > + /* FIXME: we support only single plane updates for now */ > + if (plane) > + return -EINVAL; > + > + plane = __plane; > + } > > - /* FIXME: we support only single plane updates for now */ > - if (n_planes != 1) > + if (!plane) > return -EINVAL; > > + old_plane_state = drm_atomic_get_old_plane_state(state, plane); > + new_plane_state = drm_atomic_get_new_plane_state(state, plane); > + > if (!new_plane_state->crtc) > return -EINVAL; > > -- Dmitry _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx