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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
> Op 08-07-15 om 10:55 schreef Daniel Vetter:
> > 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.
> Is that a ack or nack?

I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've implemented once
we switch over to atomic props. Since atm we only have atomic props which
get updated in pageflips we shouldn't have serious problems here yet (for
setting the rotation prop to 0° again when fbdev starts up).

Or do I miss something still here?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel