Re: [PATCH v6 2/7] drm/msm/hdmi: program HDMI timings during atomic_pre_enable

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

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