Message-ID: <52845f6f-2593-4ecf-9f0c-f4e948c0e896@kwiboo.se>
Date: Mon, 24 Jun 2024 14:19:36 +0200
From: Jonas Karlman <jonas@...boo.se>
To: neil.armstrong@...aro.org
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

Hi Neil,

On 2024-06-24 11:56, neil.armstrong@...aro.org wrote:
> 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

>From what I can find only dw-hdmi.c and dw_hdmi_meson.c are caller of
dw_hdmi_setup_rx_sense(). For meson the same value is passed for both
hpd and rx_sense, everyone else hpd = HPD and rx_sense = RX_SENSE status.

My understanding and testing based on Rockchip has shown that rx_sense
has no significant impact before and after this change.

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

I may have been wrong here, there is a check for disabled here so it
should not have setup() before atomic_enable().

Still we should ensure configure happen in atomic_enable(). The
hpd_irq_event/hpd_notify calls will trigger a detect() and signal core
if the bridge should be enabled/disabled when connector status changes.

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

This is what I thought was happening, and the comment in code seem to
indicate that. However, the code only seem to care about HPD=1 status to
poweron() and possible both HPD=0 + RXSENSE=0 to poweroff().

To make matter more complex core also mainly care about detect() status
and most/all related drivers seem to only use the the HPD status.

So I would say that there should not be any changes in impact of the
rxsense signal.

Will also send a new patch in a v2 for dw_hdmi_irq() that may be related:

-		if (phy_stat & HDMI_PHY_HPD)
+		if ((intr_stat & HDMI_IH_PHY_STAT0_HPD) &&
+		    (phy_stat & HDMI_PHY_HPD))
			status = connector_status_connected;

or there may be a status_connected event triggered when rxsense goes
from 1 to 0 and a second one with status_disconnected shortly after when
hpd goes from 1 to 0 when the cable is removed.

Regards,
Jonas

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

Powered by blists - more mailing lists