Re: [PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Aradhya Bhatia <a-bhatia1@ti.com>]

On 26/06/24 18:37, Maxime Ripard wrote:
> On Wed, Jun 26, 2024 at 02:28:57PM GMT, Tomi Valkeinen wrote:
>> On 22/06/2024 14:09, Aradhya Bhatia wrote:
>>> Move the bridge pre_enable call before crtc enable, and the bridge
>>> post_disable call after the crtc disable.
>>>
>>> The sequence of enable after this patch will look like:
>>>
>>> 	bridge[n]_pre_enable
>>> 	...
>>> 	bridge[1]_pre_enable
>>>
>>> 	crtc_enable
>>> 	encoder_enable
>>>
>>> 	bridge[1]_enable
>>> 	...
>>> 	bridge[n]__enable
>>>
>>> and vice-versa for the bridge chain disable sequence.
>>>
>>> The definition of bridge pre_enable hook says that,
>>> "The display pipe (i.e. clocks and timing signals) feeding this bridge
>>> will not yet be running when this callback is called".
>>>
>>> Since CRTC is also a source feeding the bridge, it should not be enabled
>>> before the bridges in the pipeline are pre_enabled. Fix that by
>>> re-ordering the sequence of bridge pre_enable and bridge post_disable.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>   drivers/gpu/drm/drm_atomic_helper.c | 165 ++++++++++++++++++----------
>>>   include/drm/drm_atomic_helper.h     |   7 ++
>>>   2 files changed, 114 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index fb97b51b38f1..e8ad08634f58 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -74,6 +74,7 @@
>>>    * also shares the &struct drm_plane_helper_funcs function table with the plane
>>>    * helpers.
>>>    */
>>> +
>>
>> Extra change.
>>
>>>   static void
>>>   drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>>>   				struct drm_plane_state *old_plane_state,
>>> @@ -1122,11 +1123,11 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
>>>   }
>>>   static void
>>> -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>> +disable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
>>> +			    enum bridge_chain_operation_type op_type)
>>>   {
>>>   	struct drm_connector *connector;
>>>   	struct drm_connector_state *old_conn_state, *new_conn_state;
>>> -	struct drm_crtc *crtc;
>>>   	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>>   	int i;
>>> @@ -1163,32 +1164,55 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>>   		if (WARN_ON(!encoder))
>>>   			continue;
>>> -		funcs = encoder->helper_private;
>>> -
>>> -		drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
>>> -			       encoder->base.id, encoder->name);
>>> -
>>>   		/*
>>>   		 * Each encoder has at most one connector (since we always steal
>>>   		 * it away), so we won't call disable hooks twice.
>>>   		 */
>>>   		bridge = drm_bridge_chain_get_first_bridge(encoder);
>>> -		drm_atomic_bridge_chain_disable(bridge, old_state);
>>> -		/* Right function depends upon target state. */
>>> -		if (funcs) {
>>> -			if (funcs->atomic_disable)
>>> -				funcs->atomic_disable(encoder, old_state);
>>> -			else if (new_conn_state->crtc && funcs->prepare)
>>> -				funcs->prepare(encoder);
>>> -			else if (funcs->disable)
>>> -				funcs->disable(encoder);
>>> -			else if (funcs->dpms)
>>> -				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>>> -		}
>>> +		switch (op_type) {
>>> +		case DRM_ENCODER_BRIDGE_DISABLE:
>>> +			funcs = encoder->helper_private;
>>> +
>>> +			drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
>>> +				       encoder->base.id, encoder->name);
>>> +
>>> +			drm_atomic_bridge_chain_disable(bridge, old_state);
>>> +
>>> +			/* Right function depends upon target state. */
>>> +			if (funcs) {
>>> +				if (funcs->atomic_disable)
>>> +					funcs->atomic_disable(encoder, old_state);
>>> +				else if (new_conn_state->crtc && funcs->prepare)
>>> +					funcs->prepare(encoder);
>>> +				else if (funcs->disable)
>>> +					funcs->disable(encoder);
>>> +				else if (funcs->dpms)
>>> +					funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>>> +			}
>>> +
>>> +			break;
>>> +
>>> +		case DRM_BRIDGE_POST_DISABLE:
>>> +			drm_atomic_bridge_chain_post_disable(bridge, old_state);
>>> -		drm_atomic_bridge_chain_post_disable(bridge, old_state);
>>> +			break;
>>> +
>>> +		default:
>>> +			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
>>> +			break;
>>> +		}
>>>   	}
>>> +}
>>> +
>>> +static void
>>> +disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>> +{
>>> +	struct drm_crtc *crtc;
>>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>> +	int i;
>>> +
>>> +	disable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_DISABLE);
>>>   	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>>>   		const struct drm_crtc_helper_funcs *funcs;
>>> @@ -1234,6 +1258,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>>   		if (ret == 0)
>>>   			drm_crtc_vblank_put(crtc);
>>>   	}
>>> +
>>> +	disable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_POST_DISABLE);
>>>   }
>>>   /**
>>> @@ -1445,6 +1471,64 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>>>   	}
>>>   }
>>> +static void
>>> +enable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
>>> +			   enum bridge_chain_operation_type op_type)
>>> +{
>>> +	struct drm_connector *connector;
>>> +	struct drm_connector_state *new_conn_state;
>>> +	int i;
>>> +
>>> +	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
>>> +		const struct drm_encoder_helper_funcs *funcs;
>>> +		struct drm_encoder *encoder;
>>> +		struct drm_bridge *bridge;
>>> +
>>> +		if (!new_conn_state->best_encoder)
>>> +			continue;
>>> +
>>> +		if (!new_conn_state->crtc->state->active ||
>>> +		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
>>> +			continue;
>>> +
>>> +		encoder = new_conn_state->best_encoder;
>>> +
>>> +		/*
>>> +		 * Each encoder has at most one connector (since we always steal
>>> +		 * it away), so we won't call enable hooks twice.
>>> +		 */
>>> +		bridge = drm_bridge_chain_get_first_bridge(encoder);
>>> +
>>> +		switch (op_type) {
>>> +		case DRM_BRIDGE_PRE_ENABLE:
>>> +			drm_atomic_bridge_chain_pre_enable(bridge, old_state);
>>> +			break;
>>> +
>>> +		case DRM_ENCODER_BRIDGE_ENABLE:
>>> +			funcs = encoder->helper_private;
>>> +
>>> +			drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
>>> +				       encoder->base.id, encoder->name);
>>> +
>>> +			if (funcs) {
>>> +				if (funcs->atomic_enable)
>>> +					funcs->atomic_enable(encoder, old_state);
>>> +				else if (funcs->enable)
>>> +					funcs->enable(encoder);
>>> +				else if (funcs->commit)
>>> +					funcs->commit(encoder);
>>> +			}
>>> +
>>> +			drm_atomic_bridge_chain_enable(bridge, old_state);
>>> +			break;
>>> +
>>> +		default:
>>> +			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
>>> +			break;
>>> +		}
>>> +	}
>>> +}
>>> +
>>>   /**
>>>    * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
>>>    * @dev: DRM device
>>> @@ -1465,10 +1549,10 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>>   	struct drm_crtc *crtc;
>>>   	struct drm_crtc_state *old_crtc_state;
>>>   	struct drm_crtc_state *new_crtc_state;
>>> -	struct drm_connector *connector;
>>> -	struct drm_connector_state *new_conn_state;
>>>   	int i;
>>> +	enable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_PRE_ENABLE);
>>> +
>>>   	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>>>   		const struct drm_crtc_helper_funcs *funcs;
>>> @@ -1491,42 +1575,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>>   		}
>>>   	}
>>> -	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
>>> -		const struct drm_encoder_helper_funcs *funcs;
>>> -		struct drm_encoder *encoder;
>>> -		struct drm_bridge *bridge;
>>> -
>>> -		if (!new_conn_state->best_encoder)
>>> -			continue;
>>> -
>>> -		if (!new_conn_state->crtc->state->active ||
>>> -		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
>>> -			continue;
>>> -
>>> -		encoder = new_conn_state->best_encoder;
>>> -		funcs = encoder->helper_private;
>>> -
>>> -		drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
>>> -			       encoder->base.id, encoder->name);
>>> -
>>> -		/*
>>> -		 * Each encoder has at most one connector (since we always steal
>>> -		 * it away), so we won't call enable hooks twice.
>>> -		 */
>>> -		bridge = drm_bridge_chain_get_first_bridge(encoder);
>>> -		drm_atomic_bridge_chain_pre_enable(bridge, old_state);
>>> -
>>> -		if (funcs) {
>>> -			if (funcs->atomic_enable)
>>> -				funcs->atomic_enable(encoder, old_state);
>>> -			else if (funcs->enable)
>>> -				funcs->enable(encoder);
>>> -			else if (funcs->commit)
>>> -				funcs->commit(encoder);
>>> -		}
>>> -
>>> -		drm_atomic_bridge_chain_enable(bridge, old_state);
>>> -	}
>>> +	enable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_ENABLE);
>>>   	drm_atomic_helper_commit_writebacks(dev, old_state);
>>>   }
>>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>>> index 9aa0a05aa072..b45a175a9f8a 100644
>>> --- a/include/drm/drm_atomic_helper.h
>>> +++ b/include/drm/drm_atomic_helper.h
>>> @@ -43,6 +43,13 @@
>>>    */
>>>   #define DRM_PLANE_NO_SCALING (1<<16)
>>> +enum bridge_chain_operation_type {
>>> +	DRM_BRIDGE_PRE_ENABLE,
>>> +	DRM_BRIDGE_POST_DISABLE,
>>> +	DRM_ENCODER_BRIDGE_ENABLE,
>>> +	DRM_ENCODER_BRIDGE_DISABLE,
>>> +};
>>
>> Why are the last two "DRM_ENCODER"?

I do agree that the naming is odd. It's supposed to mean both, encoder
and bridge.

When we are enabling/disabling bridges, the encoders are also getting
enabled/disabled right before/after.

But in case of pre_enable/post_disable its only the bridges that are
being operated on.

>>
>> I don't like the enum... Having "enum bridge_chain_operation_type" as a
>> parameter to a function looks like one can pass any of the enum's values,
>> which is not the case.
>>
>> How about an enum with just two values:
>>
>> DRM_BRIDGE_PRE_ENABLE_POST_DISABLE
>> DRM_BRIDGE_ENABLE_DISABLE

Yes, I can combine them like this.

> 
> I think I'd go a step further and ask why do we need that rework in the
> first place. We had a discussion about changing the time the CRTC is
> enabled compared to bridges, but it's not clear, nor explained, why we
> need to do that rework in the first place.
> 

We did discuss this, which resulted in a bunch of duplicated code in
v2[0]. The same block of code was required before the CRTC enable, as
well as after. Hence this patch.

Maybe all of this should have been explained better. =)

> It should even be two patches imo.
> 

Yeah, I can split this in 2 patches.


Regards
Aradhya

[0]:
https://lore.kernel.org/all/20240530093621.1925863-9-a-bhatia1@ti.com/