[Intel-gfx] [PATCH] drm/atomic: Add connector atomic_check function.

[Maarten Lankhorst]

Op 06-04-17 om 03:27 schreef Pandiyan, Dhinakaran:
> On Wed, 2017-04-05 at 12:06 +0200, Daniel Vetter wrote:
>> On Wed, Apr 05, 2017 at 10:41:24AM +0200, Maarten Lankhorst wrote:
>>> The connector atomic check function may be called multiple times,
>>> or not at all. It's mostly useful for implementing properties but if you
>>> call check_modeset twice it can be used for other modeset related checks
>>> as well.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/drm_atomic_helper.c      | 25 +++++++++++++++++----
>>>  include/drm/drm_modeset_helper_vtables.h | 37 ++++++++++++++++++++++++++++++++
>>>  2 files changed, 58 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index c3994b4d5f32..d550e79e8347 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -459,10 +459,20 @@ mode_fixup(struct drm_atomic_state *state)
>>>   *
>>>   * Check the state object to see if the requested state is physically possible.
>>>   * This does all the crtc and connector related computations for an atomic
>>> - * update and adds any additional connectors needed for full modesets and calls
>>> - * down into &drm_crtc_helper_funcs.mode_fixup and
>>> - * &drm_encoder_helper_funcs.mode_fixup or
>>> - * &drm_encoder_helper_funcs.atomic_check functions of the driver backend.
>>> + * update and adds any additional connectors needed for full modesets. It calls
>>> + * the various per-object callbacks in the follow order:
>>> + *
>>> + * 1. &drm_connector_helper_funcs.atomic_best_encoder for determining the new encoder.
>>> + * 2. &drm_connector_helper_funcs.atomic_check to validate the connector state.
>>> + * 3. If it's determined a modeset is needed then all connectors on the affected crtc
>>> + *    crtc are added.
> Typo - 'crtc' typed twice.
>
> update_output_state which is called before _helper_check_modeset() also
> adds all affected connectors. Why is that not considered when you say
> affected connectors are added in Step 3? Is that because
> update_output_state() is in the legacy modeset path?
Yes. update_output_state is just a legacy function.

When a crtc is cloned, a atomic update may only disable 1 connector and leave the other one enabled. In this case there's a modeset
but all connectors are not checked yet.

Similar for changing crtc_state->active, which may also result in a modeset with no connectors added yet.
>
>>> + * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
>>> + * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
>>> + *    This function is only called when the encoder will be part of a configured crtc,
>>> + *    it must not be used for implementing connector property validation.
>>> + *    If this function is NULL, &drm_atomic_encoder_helper_funcs.mode_fixup is called
>>> + *    instead.
>>> + * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
>> Line too I think. And maybe insert empty lines between each item, to make
>> them stand out more. Shouldn't affect rendering, but better to recheck.
>>
>>>   *
>>>   * &drm_crtc_state.mode_changed is set when the input mode is changed.
>>>   * &drm_crtc_state.connectors_changed is set when a connector is added or
>>> @@ -522,6 +532,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>>>  		return ret;
>>>  
>>>  	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
>>> +		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
>>> +
>>>  		/*
>>>  		 * This only sets crtc->connectors_changed for routing changes,
>>>  		 * drivers must set crtc->connectors_changed themselves when
>>> @@ -539,6 +551,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>>>  			    new_connector_state->link_status)
>>>  				new_crtc_state->connectors_changed = true;
>>>  		}
>>> +
>>> +		if (funcs->atomic_check)
>>> +			ret = funcs->atomic_check(connector, new_connector_state);
>>> +		if (ret)
>>> +			return ret;
>>>  	}
>>>  
>>>  	/*
>>> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
>>> index b304950b402d..7b5dd909f189 100644
>>> --- a/include/drm/drm_modeset_helper_vtables.h
>>> +++ b/include/drm/drm_modeset_helper_vtables.h
>>> @@ -860,6 +860,43 @@ struct drm_connector_helper_funcs {
>>>  	 */
>>>  	struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
>>>  						   struct drm_connector_state *connector_state);
>>> +
>>> +	/**
>>> +	 * @atomic_check:
>>> +	 *
>>> +	 * Drivers should check connector properties in this
>>> +	 * hook. This function is called from &drm_atomic_helper_check_modeset.
>>> +	 *
>>> +	 * This function may not be called at all on a modeset or connector
>>> +	 * change, so it should only be used to validate properties.
> I did not understand this part - "function may not be called at all on a
> modeset or connector change". Why is that?
I meant moving one connector to a different crtc, if there are 2 connectors on one of the crtc the check function for the other one may not be called the first time around.

I guess that falls under modeset, so that part may be killed.
>>> +	 * If it's required to validate all connectors from there, call
>>> +	 * &drm_atomic_helper_check_modeset twice. This will result in
> s/twice/again ? 
Indeed.
>>> +	 * atomic_check possibly being called twice as well, which the callback
> &drm_connector_helper_funcs.atomic_check?
>
>
> I had to read this a couple of times to understand, but I guess, that's
> mostly me trying to piece things together. 
Yes.