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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 24 Jun 2024 11:56:58 +0200
From: neil.armstrong@...aro.org
To: Jonas Karlman <jonas@...boo.se>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>, Robert Foss <rfoss@...nel.org>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jernej Skrabec <jernej.skrabec@...il.com>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/8] drm: bridge: dw_hdmi: Call poweron/poweroff from
 atomic enable/disable

On 24/06/2024 11:41, Jonas Karlman wrote:
> 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@...boo.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.

Yeah I know, I was worried for other platforms using rxsense

> 
> 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.

Hmm, but I thought the code wouldn't poweron the bridge is rxsense was 0
even if a mode was set, this won't work like this anymore right ?

Neil

> 
> 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);
>>
>