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;