RE: [v3 3/5] drm/bridge: add psr support during panel bridge enable & disable sequence

[Sankeerth Billakanti (QUIC)]

Hi Dmitry,

>On 21/06/2022 13:53, Vinod Polimera wrote:
>> This change avoids panel prepare/unprepare based on self-refresh
>> state.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@xxxxxxxxxxx>
>> Signed-off-by: Kalyan Thota <quic_kalyant@xxxxxxxxxxx>
>> Signed-off-by: Vinod Polimera <quic_vpolimer@xxxxxxxxxxx>
>> ---
>>   drivers/gpu/drm/bridge/panel.c | 102
>+++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 98 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/panel.c
>> b/drivers/gpu/drm/bridge/panel.c index 59a3496..6b09ae0 100644
>> --- a/drivers/gpu/drm/bridge/panel.c
>> +++ b/drivers/gpu/drm/bridge/panel.c
>> @@ -41,6 +41,40 @@ static int panel_bridge_connector_get_modes(struct
>drm_connector *connector)
>>   	return drm_panel_get_modes(panel_bridge->panel, connector);
>>   }
>>
>> +static struct drm_crtc *bridge_drm_get_old_connector_crtc(struct
>drm_encoder *encoder,
>> +							struct
>drm_atomic_state *state) {
>> +	struct drm_connector *connector;
>> +	struct drm_connector_state *conn_state;
>> +
>> +	connector = drm_atomic_get_old_connector_for_encoder(state,
>encoder);
>> +	if (!connector)
>> +		return NULL;
>> +
>> +	conn_state = drm_atomic_get_old_connector_state(state,
>connector);
>> +	if (!conn_state)
>> +		return NULL;
>> +
>> +	return conn_state->crtc;
>> +}
>> +
>> +static struct drm_crtc *bridge_drm_get_new_connector_crtc(struct
>drm_encoder *encoder,
>> +							struct
>drm_atomic_state *state) {
>> +	struct drm_connector *connector;
>> +	struct drm_connector_state *conn_state;
>> +
>> +	connector = drm_atomic_get_new_connector_for_encoder(state,
>encoder);
>> +	if (!connector)
>> +		return NULL;
>> +
>> +	conn_state = drm_atomic_get_new_connector_state(state,
>connector);
>> +	if (!conn_state)
>> +		return NULL;
>> +
>> +	return conn_state->crtc;
>> +}
>
>As I wrote earlier, this should become generic drm helpers.
>

Yes, will move it.

>> +
>>   static const struct drm_connector_helper_funcs
>>   panel_bridge_connector_helper_funcs = {
>>   	.get_modes = panel_bridge_connector_get_modes, @@ -108,30
>+142,90
>> @@ static void panel_bridge_detach(struct drm_bridge *bridge)
>>   		drm_connector_cleanup(connector);
>>   }
>>
>> -static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge)
>> +static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>> +					struct drm_bridge_state
>*old_bridge_state)
>
>This must be a part of the previous patch?
>

Yes, it should be moved to that patch.

>>   {
>>   	struct panel_bridge *panel_bridge =
>> drm_bridge_to_panel_bridge(bridge);
>> +	struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> +	struct drm_encoder *encoder = bridge->encoder;
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *old_crtc_state;
>> +
>> +	crtc = bridge_drm_get_new_connector_crtc(encoder, old_state);
>> +	if (!crtc)
>> +		return;
>
>Why? And why do you ask for the new crtc from the old state?
>

If the previous bridge disable and post_disable calls were issued just to enter psr,
then the panel power and backlight will still be on.

We need to know the psr status of the old state of the crtc to decide whether to
enable the panel power or just early return.

old_state is the atomic_state object. Will change the variable name to atomic_state.

>> +
>> +	old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
>> +
>> +	/* Don't touch the panel if we're coming back from self refresh state
>*/
>> +	if (old_crtc_state && old_crtc_state->self_refresh_active)
>> +		return;
>>
>>   	drm_panel_prepare(panel_bridge->panel);
>>   }
>>
>> -static void panel_bridge_atomic_enable(struct drm_bridge *bridge)
>> +static void panel_bridge_atomic_enable(struct drm_bridge *bridge,
>> +					struct drm_bridge_state
>*old_bridge_state)
>>   {
>>   	struct panel_bridge *panel_bridge =
>> drm_bridge_to_panel_bridge(bridge);
>> +	struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> +	struct drm_encoder *encoder = bridge->encoder;
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *old_crtc_state;
>> +
>> +	crtc = bridge_drm_get_new_connector_crtc(encoder, old_state);
>> +	if (!crtc)
>> +		return;
>> +
>> +	old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
>> +
>> +	/* Don't touch the panel if we're coming back from self refresh state
>*/
>> +	if (old_crtc_state && old_crtc_state->self_refresh_active)
>> +		return;
>>
>>   	drm_panel_enable(panel_bridge->panel);
>>   }
>>
>> -static void panel_bridge_atomic_disable(struct drm_bridge *bridge)
>> +static void panel_bridge_atomic_disable(struct drm_bridge *bridge,
>> +					struct drm_bridge_state
>*old_bridge_state)
>>   {
>>   	struct panel_bridge *panel_bridge =
>> drm_bridge_to_panel_bridge(bridge);
>> +	struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> +	struct drm_encoder *encoder = bridge->encoder;
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *new_crtc_state;
>> +
>> +	crtc = bridge_drm_get_old_connector_crtc(encoder, old_state);
>> +	if (!crtc)
>> +		return;
>> +
>> +	new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
>
>This doesn't sound right too.
>

There is a risk of crtc being deallocated if the disable call came during screen off.
To be on safer side, we are getting the old crtc and check for the appropriate crtc state.
I believe the old_state variable name is causing a confusion. I will change the name to
atomic_state.

>> +
>> +	/* Don't do a full disable on PSR transitions if new state is self refresh
>state */
>> +	if (new_crtc_state && new_crtc_state->self_refresh_active)
>> +		return;
>>
>>   	drm_panel_disable(panel_bridge->panel);
>>   }
>>
>> -static void panel_bridge_atomic_post_disable(struct drm_bridge
>> *bridge)
>> +static void panel_bridge_atomic_post_disable(struct drm_bridge *bridge,
>> +					struct drm_bridge_state
>*old_bridge_state)
>>   {
>>   	struct panel_bridge *panel_bridge =
>> drm_bridge_to_panel_bridge(bridge);
>> +	struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> +	struct drm_encoder *encoder = bridge->encoder;
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *new_crtc_state;
>> +
>> +	crtc = bridge_drm_get_old_connector_crtc(encoder, old_state);
>> +	if (!crtc)
>> +		return;
>> +
>> +	new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
>> +
>> +	/* Don't do unprepare on PSR transitions if new state is self refresh
>state */
>> +	if (new_crtc_state && new_crtc_state->self_refresh_active)
>> +		return;
>>
>>   	drm_panel_unprepare(panel_bridge->panel);
>>   }
>
>
>--
>With best wishes
>Dmitry

Thank you,
Sankeerth