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

[Maarten Lankhorst]

Op 08-07-15 om 19:52 schreef Daniel Vetter:
> 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?
Yes, if the hardware mode is incompatible with its calculated sw mode,
and we set a different mode from fbdev you get 2 modesets instead of 1.

First to make the mode compatible because of the rotate_0, second to set the new mode.

~Maarten