Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

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

Op 07-07-15 om 18:43 schreef Daniel Vetter:
> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
>>>>>> This allows the first atomic call during hw init to be a real modeset,
>>>>>> which is useful for forcing a recalculation.
>>>>> fbcon is optional, you can't rely on anything being done in any specific
>>>>> way. What exactly do you need this for, what's the implications?
>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
>>> the legacy entry points with atomic helpers, and then even plane_disable
>>> will be a full atomic modeset.
>>>
>>> What did fall apart with just touching properties/planes now?
>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
>> if a modeset is needed after the first atomic call. Right now because
>> intel_modeset_compute_config is only called in set_config so this works as expected.
>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
>> and if the final mode is different this will introduce a double modeset.
> For expensive properties (i.e. a no-op changes causes something that takes
> time like modeset or vblank wait) we need to make sure we filter them out
> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
> the existing legacy set_prop functions should all filter out no-op changes
> themselves. If we don't do that for rotation then that's a bug.
>
> Same for disabling planes harder, that shouldn't take time. Especially
> since fbcon only force-disable non-primary plane, and for driver load
> that's the exact thing we already do in the driver anyway.

Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic_helper.h>
+#include "drm_crtc_internal.h"
 #include <linux/fence.h>
 
 /**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
 {
 	struct drm_atomic_state *state;
 	struct drm_crtc_state *crtc_state;
-	int ret = 0;
+	uint64_t retval;
+	int ret;
+
+	ret = drm_atomic_get_property(&crtc->base, property, &retval);
+	if (!ret && val == retval)
+		return 0;
 
 	state = drm_atomic_state_alloc(crtc->dev);
 	if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
 {
 	struct drm_atomic_state *state;
 	struct drm_plane_state *plane_state;
-	int ret = 0;
+	uint64_t retval;
+	int ret;
+
+	ret = drm_atomic_get_property(&plane->base, property, &retval);
+	if (!ret && val == retval)
+		return 0;
 
 	state = drm_atomic_state_alloc(plane->dev);
 	if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
 {
 	struct drm_atomic_state *state;
 	struct drm_connector_state *connector_state;
-	int ret = 0;
+	uint64_t retval;
+	int ret;
+
+	ret = drm_atomic_get_property(&connector->base, property, &retval);
+	if (!ret && val == retval)
+		return 0;
 
 	state = drm_atomic_state_alloc(connector->dev);
 	if (!state)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 424c83323aaa..5bab7bff8a15 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1327,7 +1327,8 @@ void drm_plane_force_disable(struct drm_plane *plane)
 {
 	int ret;
 
-	if (!plane->fb)
+	if ((plane->state && !plane->state->fb) ||
+	    (!plane->state && !plane->fb))
 		return;
 
 	plane->old_fb = plane->fb;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel