[PATCH v2 2/6] drm/atomic: Unconditionally call prepare_fb.

[Maarten Lankhorst]

Op 13-12-16 om 18:10 schreef Daniel Vetter:
> On Tue, Dec 13, 2016 at 03:13:54PM +0100, Maarten Lankhorst wrote:
>> Op 09-12-16 om 09:25 schreef Daniel Vetter:
>>> On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote:
>>>> Hi Daniel,
>>>>
>>>> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote:
>>>>> On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote:
>>>>>> Atomic drivers may set properties like rotation on the same fb, which
>>>>>> may require a call to prepare_fb even when framebuffer stays identical.
>>>>>>
>>>>>> Instead of handling all the special cases in the core, let the driver
>>>>>> decide when prepare_fb and cleanup_fb are noops.
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>>> I think this makes sense, but would be really good to get a pile of acks
>>>>> from driver maintainers on this one. Rob, Eric, Laurent, others?
>>>> This is all very nice, but it will introduce at least a performance 
>>>> regression, and possibly worse, until drivers get updated. There are 7 drivers 
>>>> implementing the .prepare_fb() callback (plus a bunch of drivers that probably 
>>>> should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this 
>>>> patch before they get fixed.
>>> Maarten's commit message is insufficient, since this is defacto a revert
>>> of
>>>
>>> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
>>> Author: Keith Packard <keithp at keithp.com>
>>> Date:   Sat Jun 4 01:16:22 2016 -0700
>>>
>>>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
>>>
>>> because that breaks stuff. We're simply going back to where we've been a
>>> few months ago. Since this is a regression fix, back to original
>>> behaviour, can you ack (assuming Maarten updates the commit message to
>>> reflect the nature of the commit here)?
>> Waiting on a reply, but what about this commit message for this patch?
>> ---
>> Atomic drivers may set properties like rotation on the same fb, which
>> may require a call to prepare_fb even when framebuffer stays identical.
>>
>> Instead of handling all the special cases in the core, let the driver
>> decide when prepare_fb and cleanup_fb are noops.
>>
>> This is a revert of:
>>
>> commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
>> Author: Keith Packard <keithp at keithp.com>
>> Date:   Sat Jun 4 01:16:22 2016 -0700
>>
>>     drm: Don't prepare or cleanup unchanging frame buffers [v3]
>>
>> The original commit mentions that this prevents waiting in i915 on all
>> previous rendering during cursor updates, but there are better ways to
>> fix this.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Yeah sounds good to me. Since we don't want to backport all the i915
> cursor patches no cc: stable on this. Also, this is only an issue for
> drivers which both have a cursor plane, and implement that cursor using
> universal planes (i.e. settting drm_crtc->cursor). Afaik the only two are
> vc4 and i915, and after this series both will have appropriate hacks (for
> now) to keep existing userspace happy.
> -Daniel

Same patch, reworded!
---8<---
Atomic drivers may set properties like rotation on the same fb, which
may require a call to prepare_fb even when framebuffer stays identical.

Instead of handling all the special cases in the core, let the driver
decide when prepare_fb and cleanup_fb are noops.

This is a revert of:

commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
Author: Keith Packard <keithp at keithp.com>
Date:   Sat Jun 4 01:16:22 2016 -0700

    drm: Don't prepare or cleanup unchanging frame buffers [v3]

The original commit mentions that this prevents waiting in i915 on all
previous rendering during cursor updates, but there are better ways to
fix this.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 23767df72615..d19563651e07 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 		funcs = plane->helper_private;
 
-		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
-			continue;
-
 		if (funcs->prepare_fb) {
 			ret = funcs->prepare_fb(plane, plane_state);
 			if (ret)
@@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 		if (j >= i)
 			continue;
 
-		if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc))
-			continue;
-
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
@@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 	for_each_plane_in_state(old_state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 
-		if (!drm_atomic_helper_framebuffer_changed(dev, old_state, plane_state->crtc))
-			continue;
-
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
-- 
2.7.4