[Intel-gfx] [PATCH 2/4] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip

[Ville Syrjälä]

On Fri, Jun 12, 2015 at 02:27:54PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 12, 2015 at 12:31:37PM +0200, Maarten Lankhorst wrote:
> > Op 12-06-15 om 12:16 schreef Ville Syrjälä:
> > > On Fri, Jun 12, 2015 at 11:15:40AM +0200, Maarten Lankhorst wrote:
> > >> Use a full atomic call instead. intel_crtc_page_flip will still
> > >> have to live until async updates are allowed.
> > >>
> > >> This doesn't seem to be a regression from the convert to atomic,
> > >> part 3 patch. During GPU reset it fixes the following warning:
> > >>
> > >>  ------------[ cut here ]------------
> > >> WARNING: CPU: 0 PID: 752 at drivers/gpu/drm/drm_crtc.c:5337 drm_mode_page_flip_ioctl+0x27b/0x360()
> > >> Modules linked in: i915
> > >> CPU: 0 PID: 752 Comm: Xorg Not tainted 4.1.0-rc7-patser+ #4090
> > >> Hardware name: NUC5i7RYB, BIOS RYBDWi35.86A.0246.2015.0309.1355 03/09/2015
> > >>  ffffffff81c90866 ffff8800d87c3ca8 ffffffff817f7d87 0000000080000001
> > >>  0000000000000000 ffff8800d87c3ce8 ffffffff81084955 ffff880000000000
> > >>  ffff8800d87c3dc0 ffff8800d93d1208 0000000000000000 ffff8800b7d1f3e0
> > >> Call Trace:
> > >>  [<ffffffff817f7d87>] dump_stack+0x4f/0x7b
> > >>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
> > >>  [<ffffffff81084a35>] warn_slowpath_null+0x15/0x20
> > >>  [<ffffffff8146dffb>] drm_mode_page_flip_ioctl+0x27b/0x360
> > >>  [<ffffffff8145ccb0>] drm_ioctl+0x1a0/0x6a0
> > >>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> > >>  [<ffffffff812e5540>] ? avc_has_perm+0x20/0x280
> > >>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> > >>  [<ffffffff811ea0f8>] do_vfs_ioctl+0x2f8/0x530
> > >>  [<ffffffff811f6001>] ? expand_files+0x261/0x270
> > >>  [<ffffffff812e7c16>] ? selinux_file_ioctl+0x56/0x100
> > >>  [<ffffffff811ea3b1>] SyS_ioctl+0x81/0xa0
> > >>  [<ffffffff81801b97>] system_call_fastpath+0x12/0x6f
> > >> ---[ end trace 9ce834560085bd64 ]---
> > >>
> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++++++++-
> > >>  1 file changed, 28 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > >> index 7abaffeda7ce..cdf6549c8e74 100644
> > >> --- a/drivers/gpu/drm/i915/intel_display.c
> > >> +++ b/drivers/gpu/drm/i915/intel_display.c
> > >> @@ -11618,8 +11618,35 @@ free_work:
> > >>  	kfree(work);
> > >>  
> > >>  	if (ret == -EIO) {
> > >> +		struct drm_atomic_state *state;
> > >> +		struct drm_plane_state *plane_state;
> > >> +
> > >>  out_hang:
> > >> -		ret = intel_plane_restore(primary);
> > > So what exactly is wrong with intel_plane_restore() (ie.
> > > drm_plane_helper_update())? Shouldn't you fix that instead of spreading
> > > the uglyness here?
> > >
> > intel_plane_restore uses the transitional helpers. This is currently used for
> > setting color key, enabling sprite planes after a modeset and this function.
> > - Color key will be made atomic by making ckey part of the plane state.
> 
> Well what function do you plan to call there? The same should work here
> no?

To elaborate a bit more. My main gripe here is all this boilerplate we
need. I hope we're planning to abstract that away a bit so we don't have
to keep repeating it everywhere.

So maybe it could look something like this:

ret = prep_plane_update()
set stuff in the state
ret = check_and_commit()

That way the EDEADLOCK mess etc. would be hidden inside the
prep part.

In the meantime I'd at least stuff this thing into separate functions so
that the ugly if (!ret) stuff would go away.

> 
> > - Sprite plane updates after modeset will be made unnecessary by calling
> >   drm_atomic_helper_commit_planes_on_crtc after .crtc_enable.
> > - This function needs to update plane->fb, and intel_plane_restore doesn't provide that.
> > 
> > That leaves only this function calling intel_plane_restore, if we can get rid of it
> > there will be no need of transitional helper calls any more.
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC