Re: [PATCH 1/8] drm: bridge: dw_hdmi: Call poweron/poweroff from atomic enable/disable

[Jonas Karlman <jonas@kwiboo.se>]

Hi Neil,

On 2024-06-24 11:23, Neil Armstrong wrote:
> On 11/06/2024 17:50, Jonas Karlman wrote:
>> Change to only call poweron/poweroff from atomic_enable/atomic_disable
>> ops instead of trying to keep a bridge_is_on state and poweron/off in
>> the hotplug irq handler.
>>
>> A benefit of this is that drm mode_config mutex is always held at
>> poweron/off, something that may reduce the need for our own mutex.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 33 ++---------------------
>>   1 file changed, 2 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 9f2bc932c371..34bc6f4754b8 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -172,7 +172,6 @@ struct dw_hdmi {
>>   	enum drm_connector_force force;	/* mutex-protected force state */
>>   	struct drm_connector *curr_conn;/* current connector (only valid when !disabled) */
>>   	bool disabled;			/* DRM has disabled our bridge */
>> -	bool bridge_is_on;		/* indicates the bridge is on */
>>   	bool rxsense;			/* rxsense state */
>>   	u8 phy_mask;			/* desired phy int mask settings */
>>   	u8 mc_clkdis;			/* clock disable register */
>> @@ -2383,8 +2382,6 @@ static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi)
>>   
>>   static void dw_hdmi_poweron(struct dw_hdmi *hdmi)
>>   {
>> -	hdmi->bridge_is_on = true;
>> -
>>   	/*
>>   	 * The curr_conn field is guaranteed to be valid here, as this function
>>   	 * is only be called when !hdmi->disabled.
>> @@ -2398,30 +2395,6 @@ static void dw_hdmi_poweroff(struct dw_hdmi *hdmi)
>>   		hdmi->phy.ops->disable(hdmi, hdmi->phy.data);
>>   		hdmi->phy.enabled = false;
>>   	}
>> -
>> -	hdmi->bridge_is_on = false;
>> -}
>> -
>> -static void dw_hdmi_update_power(struct dw_hdmi *hdmi)
>> -{
>> -	int force = hdmi->force;
>> -
>> -	if (hdmi->disabled) {
>> -		force = DRM_FORCE_OFF;
>> -	} else if (force == DRM_FORCE_UNSPECIFIED) {
>> -		if (hdmi->rxsense)
>> -			force = DRM_FORCE_ON;
>> -		else
>> -			force = DRM_FORCE_OFF;
>> -	}
> 
> This means we always poweron the bridge even if rxsense is false ?

If I follow the logic there should not be any difference, beside that
power on now only happen in atomic_enable instead of sometime in irq
handler.

hdmi->rxsense is set to true based on hpd in dw_hdmi_setup_rx_sense().
For both meson and dw-hdmi this means HPD=1 and not rxsense=1.

drm core will call the force/detect ops and enable/disable based on
forced/HPD state, so I am not expecting any difference in how this
currently works.

This change to only poweron/setup in atomic_enable should also fix
issues/situations where dw-hdmi was poweron too early during bootup in
irq handler, before the drm driver was fully probed.

Regards,
Jonas

> 
> Neil
> 
>> -
>> -	if (force == DRM_FORCE_OFF) {
>> -		if (hdmi->bridge_is_on)
>> -			dw_hdmi_poweroff(hdmi);
>> -	} else {
>> -		if (!hdmi->bridge_is_on)
>> -			dw_hdmi_poweron(hdmi);
>> -	}
>>   }
>>   
>>   /*
>> @@ -2546,7 +2519,6 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
>>   
>>   	mutex_lock(&hdmi->mutex);
>>   	hdmi->force = connector->force;
>> -	dw_hdmi_update_power(hdmi);
>>   	dw_hdmi_update_phy_mask(hdmi);
>>   	mutex_unlock(&hdmi->mutex);
>>   }
>> @@ -2955,7 +2927,7 @@ static void dw_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
>>   	mutex_lock(&hdmi->mutex);
>>   	hdmi->disabled = true;
>>   	hdmi->curr_conn = NULL;
>> -	dw_hdmi_update_power(hdmi);
>> +	dw_hdmi_poweroff(hdmi);
>>   	dw_hdmi_update_phy_mask(hdmi);
>>   	handle_plugged_change(hdmi, false);
>>   	mutex_unlock(&hdmi->mutex);
>> @@ -2974,7 +2946,7 @@ static void dw_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
>>   	mutex_lock(&hdmi->mutex);
>>   	hdmi->disabled = false;
>>   	hdmi->curr_conn = connector;
>> -	dw_hdmi_update_power(hdmi);
>> +	dw_hdmi_poweron(hdmi);
>>   	dw_hdmi_update_phy_mask(hdmi);
>>   	handle_plugged_change(hdmi, true);
>>   	mutex_unlock(&hdmi->mutex);
>> @@ -3073,7 +3045,6 @@ void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
>>   		if (hpd)
>>   			hdmi->rxsense = true;
>>   
>> -		dw_hdmi_update_power(hdmi);
>>   		dw_hdmi_update_phy_mask(hdmi);
>>   	}
>>   	mutex_unlock(&hdmi->mutex);
>