Re: [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 08-07-15 om 22:12 schreef Daniel Vetter:
> On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
>> 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.
> How does that happen? For setting the rotation property we should just
> duplicate the current crtc state. Since there's no mode changing (they
> should match perfectly no matter how botched the reconstruction is) there
> shouldn't be any need to recompute the config completely and discover that
> there's a mismatch. Which means we'll just do the plane update (which
> might do a few silly mmios but shouldn't block) and that's it.
>
> At least that's what I'd expect - where does this fall apart?
If crtc is active and primary fb visible, and converted to atomic:

restore_fbdev_mode() ->
	drm_mode_plane_set_obj_prop() ->
		drm_atomic_helper_plane_set_property() ->
			drm_atomic_get_plane_state() ->
				drm_atomic_get_crtc_state()
crtc state is part of the state, intel_modeset_pipe_config performs
the initial check if modeset's needed. Lets assume yes:

			modeset()

		drm_mode_set_config_internal() ->
			modeset()

Boom double modeset. :(

The alternative solution is making a atomic version of restore_fbdev_mode,
but that would break drivers that are only partially converted to atomic,
like i915 with i915.nuclear_pageflip=true before the convert to atomic commit.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx