Re: [PATCH 2/4] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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@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.

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx