On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
> 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)
The reason I didn't do this is that a prop change might still result in no
hw state change (e.g. if you go automitic->explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause trouble
yet.
> 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;
Nah, atomic helpers should figure this out imo. Since if userspace does
the same (loop over all planes) then it won't go through force_disable.
-Daniel
>
> plane->old_fb = plane->fb;
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
