| 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
| ||
|
Message-ID: <b7561a2c-653b-4c56-8bef-3ae76a729d7c@quicinc.com>
Date: Fri, 7 Feb 2025 12:11:55 -0800
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong
<neil.armstrong@...aro.org>,
Robert Foss <rfoss@...nel.org>,
Laurent Pinchart
<Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>,
Jernej
Skrabec <jernej.skrabec@...il.com>,
Maarten Lankhorst
<maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Rob Clark <robdclark@...il.com>,
Sean Paul <sean@...rly.run>,
Marijn Suijten
<marijn.suijten@...ainline.org>,
Simona Vetter <simona@...ll.ch>,
Simona
Vetter <simona.vetter@...ll.ch>,
<dri-devel@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
<freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 2/7] drm/msm/hdmi: program HDMI timings during
atomic_pre_enable
On 2/6/2025 5:19 PM, Dmitry Baryshkov wrote:
> On Thu, Feb 06, 2025 at 12:41:30PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 2/3/2025 4:59 PM, Dmitry Baryshkov wrote:
>>> On Mon, Feb 03, 2025 at 11:34:00AM -0800, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/24/2025 1:47 PM, Dmitry Baryshkov wrote:
>>>>> The mode_set callback is deprecated, it doesn't get the
>>>>> drm_bridge_state, just mode-related argumetns. Also Abhinav pointed out
>>>>> that HDMI timings should be programmed after setting up HDMI PHY and
>>>>> PLL. Rework the code to program HDMI timings at the end of
>>>>> atomic_pre_enable().
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>>>>> ---
>>>>> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 23 +++++++++++++++--------
>>>>> 1 file changed, 15 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>>>>> index d839c71091dcdc3b020fcbba8d698d58ee7fc749..d5ab1f74c0e6f47dc59872c016104e9a84d85e9e 100644
>>>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>>>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>>>>> @@ -126,15 +126,26 @@ static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
>>>>> hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL1, val);
>>>>> }
>>>>> +static void msm_hdmi_bridge_atomic_set_timings(struct hdmi *hdmi,
>>>>> + const struct drm_display_mode *mode);
>>>>> static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>>>>> struct drm_bridge_state *old_bridge_state)
>>>>> {
>>>>> + struct drm_atomic_state *state = old_bridge_state->base.state;
>>>>> struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>>>>> struct hdmi *hdmi = hdmi_bridge->hdmi;
>>>>> struct hdmi_phy *phy = hdmi->phy;
>>>>> + struct drm_encoder *encoder = bridge->encoder;
>>>>> + struct drm_connector *connector;
>>>>> + struct drm_connector_state *conn_state;
>>>>> + struct drm_crtc_state *crtc_state;
>>>>> DBG("power up");
>>>>> + connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
>>>>> + conn_state = drm_atomic_get_new_connector_state(state, connector);
>>>>> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
>>>>> +
>>>>> if (!hdmi->power_on) {
>>>>> msm_hdmi_phy_resource_enable(phy);
>>>>> msm_hdmi_power_on(bridge);
>>>>> @@ -151,6 +162,8 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>>>>> if (hdmi->hdcp_ctrl)
>>>>> msm_hdmi_hdcp_on(hdmi->hdcp_ctrl);
>>>>> +
>>>>> + msm_hdmi_bridge_atomic_set_timings(hdmi, &crtc_state->adjusted_mode);
>>>>> }
>>>>
>>>> This addresses my comment about setting up the HDMI timing registers before
>>>> setting up the timing engine registers.
>>>>
>>>> But prior to this change, mode_set was doing the same thing as
>>>> msm_hdmi_bridge_atomic_set_timings() which means
>>>> msm_hdmi_bridge_atomic_set_timings() should be called at the beginning of
>>>> pre_enable()?
>>>>
>>>> The controller is enabled in msm_hdmi_set_mode(). So this should be done
>>>> before that.
>>>
>>> In [1] you provided the following order:
>>>
>>> 1) setup HDMI PHY and PLL
>>> 2) setup HDMI video path correctly (HDMI timing registers)
>>> 3) setup timing generator to match the HDMI video in (2)
>>> 4) Enable timing engine
>>>
>>> This means htat msm_hdmi_bridge_atomic_set_timings() should come at the
>>> end of msm_hdmi_bridge_atomic_pre_enable(), not in the beginning /
>>> middle of it.
>>>
>>> [1] https://lore.kernel.org/dri-devel/8dd4a43e-d83c-1f36-21ff-61e13ff751e7@quicinc.com/
>>>
>>
>> Sequence given is correct and is exactly what is given in the docs. What is
>> somewhat not clear in the docs is the location of the enable of the HDMI
>> controller. This is not there in the above 4 steps. I am referring to the
>> enable bit being programmed in msm_hdmi_set_mode(). Ideally till we enable
>> the timing engine, it should be okay but what I wanted to do was to keep the
>> msm_hdmi_set_mode() as the last call in this function that way we program
>> everything and then enable the controller.
>>
>> This can be done in either way, move it to the beginning of the function or
>> move it right before msm_hdmi_set_mode(). I had suggested beginning because
>> thats how it was when things were still in mode_set.
>
> Well.. following your description it might be better to put it after PHY
> init. What do you think?
>
Are you referring to after msm_hdmi_phy_powerup()? Yes, thats fine too.
Powered by blists - more mailing lists