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

[Dmitry Baryshkov]

On Fri, Feb 07, 2025 at 12:11:55PM -0800, Abhinav Kumar wrote:
> 
> 
> 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@xxxxxxxxxx>
> > > > > > ---
> > > > > >     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@xxxxxxxxxxx/
> > > > 
> > > 
> > > 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.

After doing several tests, this doesn't result in a stable enough
behaviour. I'll follow your recommendataion and call
msm_hdmi_bridge_atomic_set_timings() in the beginning of
msm_hdmi_bridge_atomic_pre_enable().

-- 
With best wishes
Dmitry