[Intel-gfx] [PATCH 2/4] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip
Daniel Vetter
daniel at ffwll.ch
Mon Jun 15 09:44:01 PDT 2015
On Fri, Jun 12, 2015 at 03:17:04PM +0200, Maarten Lankhorst wrote:
> Op 12-06-15 om 13:43 schreef 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.
>
> Eventually color key should probably become a property, but how does this look?
>
> Adding color key with this function should be easy.
Yeah once everything goes properly through atomic props and helpers this
boilerplate state frobbing should go away. Until then we just have to bear
the cost of having tons of driver-specific logic all over the place in
i915. Note a whole lot we can do about that.
In the end I hope the only thing we really need to keep around is state
frobberry for the hw state readout (the force-restore should be doable
with drm atomic helpers to save/restore all atomic state) and in the load
detect code (that's truly driver private).
-Daniel
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7abaffeda7ce..cda85eca7174 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11431,6 +11431,53 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
> spin_unlock(&dev->event_lock);
> }
>
> +static int intel_update_plane_state(struct drm_plane *plane,
> + struct drm_modeset_acquire_ctx *acquire_ctx,
> + int (*update_fn)(struct drm_plane_state *, void *data),
> + void *data)
> +{
> + struct drm_atomic_state *state;
> + struct drm_plane_state *plane_state;
> + int ret;
> +
> + state = drm_atomic_state_alloc(plane->dev);
> + if (!state)
> + return -ENOMEM;
> +
> + state->acquire_ctx = acquire_ctx;
> +
> +retry:
> + plane_state = drm_atomic_get_plane_state(state, plane);
> + ret = PTR_ERR_OR_ZERO(plane_state);
> + if (!ret)
> + ret = update_fn(plane_state, data);
> +
> + if (!ret)
> + ret = drm_atomic_commit(state);
> +
> + if (ret == -EDEADLK) {
> + drm_modeset_backoff(state->acquire_ctx);
> + drm_atomic_state_clear(state);
> + goto retry;
> + }
> +
> + if (ret)
> + drm_atomic_state_free(state);
> +
> + return ret;
> +}
> +
> +static int
> +intel_crtc_page_flip_update_fb(struct drm_plane_state *plane_state, void *data)
> +{
> + drm_atomic_set_fb_for_plane(plane_state, data);
> +
> + if (WARN_ON(!plane_state->crtc))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static int intel_crtc_page_flip(struct drm_crtc *crtc,
> struct drm_framebuffer *fb,
> struct drm_pending_vblank_event *event,
> @@ -11619,7 +11666,8 @@ free_work:
>
> if (ret == -EIO) {
> out_hang:
> - ret = intel_plane_restore(primary);
> + ret = intel_update_plane_state(primary, drm_modeset_legacy_acquire_ctx(crtc), intel_crtc_page_flip_update_fb, fb);
> +
> if (ret == 0 && event) {
> spin_lock_irq(&dev->event_lock);
> drm_send_vblank_event(dev, pipe, event);
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list