Re: [Intel-gfx] [PATCH] drm/atomic: Make async plane update checks actually work as intended.

[Dmitry Osipenko]

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 
>>> Signed-off-by: Maarten Lankhorst 
>>> Reviewed-by: Gustavo Padovan 
>>> Reviewed-by: Daniel Vetter  #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] [] (drm_atomic_helper_async_check) from []
>> (drm_atomic_helper_check+0x4c/0x64)
>> [   70.138958] [] (drm_atomic_helper_check) from []
>> (drm_atomic_check_only+0x2f4/0x56c)
>> [   70.138969] [] (drm_atomic_check_only) from []
>> (drm_atomic_commit+0x10/0x50)
>> [   70.138979] [] (drm_atomic_commit) from []
>> (drm_atomic_helper_update_plane+0xf0/0x100)
>> [   70.138991] [] (drm_atomic_helper_update_plane) from 
>> []
>> (__setplane_internal+0x178/0x214)
>> [   70.139003] [] (__setplane_internal) from []
>> (drm_mode_cursor_universal+0x118/0x1a8)
>> [   70.139014] [] (drm_mode_cursor_universal) from []
>> (drm_mode_cursor_common+0x174/0x1ec)
>> [   70.139026] [] (drm_mode_cursor_common) from []
>> (drm_mode_cursor_ioctl+0x58/0x60)
>> [   70.139036] [] (drm_mode_cursor_ioctl) from []
>> (drm_ioctl+0x198/0x368)
>> [   70.139047] [] (drm_ioctl) from [] 
>> (do_vfs_ioctl+0x9c/0x8f0)
>> [   70.139058] [] (do_vfs_ioctl) from [] 
>> (SyS_ioctl+0x34/0x5c)
>> [   70.139071] [] (SyS_ioctl) from []
>> (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 
> Fixes: 669c9215afea ("drm/atomic: Make async 

Re: [Intel-gfx] [PATCH] drm/atomic: Make async plane update checks actually work as intended.

[Daniel Vetter]

On Mon, Sep 25, 2017 at 08:43:44AM +0200, 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 
> >> Signed-off-by: Maarten Lankhorst 
> >> Reviewed-by: Gustavo Padovan 
> >> Reviewed-by: Daniel Vetter  #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] [] (drm_atomic_helper_async_check) from 
> > []
> > (drm_atomic_helper_check+0x4c/0x64)
> > [   70.138958] [] (drm_atomic_helper_check) from []
> > (drm_atomic_check_only+0x2f4/0x56c)
> > [   70.138969] [] (drm_atomic_check_only) from []
> > (drm_atomic_commit+0x10/0x50)
> > [   70.138979] [] (drm_atomic_commit) from []
> > (drm_atomic_helper_update_plane+0xf0/0x100)
> > [   70.138991] [] (drm_atomic_helper_update_plane) from 
> > []
> > (__setplane_internal+0x178/0x214)
> > [   70.139003] [] (__setplane_internal) from []
> > (drm_mode_cursor_universal+0x118/0x1a8)
> > [   70.139014] [] (drm_mode_cursor_universal) from []
> > (drm_mode_cursor_common+0x174/0x1ec)
> > [   70.139026] [] (drm_mode_cursor_common) from []
> > (drm_mode_cursor_ioctl+0x58/0x60)
> > [   70.139036] [] (drm_mode_cursor_ioctl) from []
> > (drm_ioctl+0x198/0x368)
> > [   70.139047] [] (drm_ioctl) from [] 
> > (do_vfs_ioctl+0x9c/0x8f0)
> > [   70.139058] [] (do_vfs_ioctl) from [] 
> > (SyS_ioctl+0x34/0x5c)
> > [   70.139071] [] (SyS_ioctl) from []
> > (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.

Please don't in-line reply fixup patches, CI doesn't pick them up but
instead wants to add it to the patch series. Which won't apply because
most of it landed already. Please resend standalone.

> 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