On Sun, Sep 29, 2024 at 02:34:36AM GMT, Sandor Yu wrote:

quoted

quoted

+static void cdns_hdmi_sink_config(struct cdns_mhdp8501_device *mhdp)
+{
+	struct drm_display_info *display = &mhdp->curr_conn->display_info;
+	struct drm_connector_state *conn_state = mhdp->curr_conn->state;

That looks a bit hackish to me. We should probably provide a helper to get the
connector state the bridge is attached to.

How about code change to followed, is it more clear?
370         struct drm_connector *connector = mhdp->curr_conn;
371         struct drm_connector_state *conn_state = connector->state;
372         struct drm_display_info *display = &connector->display_info;
373         struct drm_scdc *scdc = &display->hdmi.scdc;

What I meant was that I wish bridges had a way to get their connector
pointer. It doesn't look like it's possible with drm_bridge_connector,
and we don't have access to drm_display_info anymore.

I don't really see a good way to do this yet, so maybe that kind of
workaround is ok. Eventually, I guess we'll have the scrambler setup in
the HDMI connector helpers anyway.

Dmitry, any idea?

quoted

quoted

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode) {
+	unsigned long long tmds_rate;
+
+	/* We don't support double-clocked and Interlaced modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||
+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;
+
+	if (mode->hdisplay > 3840)
+		return MODE_BAD_HVALUE;
+
+	if (mode->vdisplay > 2160)
+		return MODE_BAD_VVALUE;
+
+	tmds_rate = mode->clock * 1000ULL;
+	return cdns_hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate); }

Didn't we agree on creating a mode_valid helper?

In fact, now I'm no idea where should add the mode_valid helper function.

In struct drm_bridge_funcs, it had mode_valid() and hdmi_tmds_char_rate_valid().

If create a new mode_valid helper function in struct drm_connector_hdmi_funcs,
Is it appropriate to call another API function(tmds_char_rate_valid)
at the same level within this API function?

I'm not quite sure what you mean, but a reasonable approach to me would
be to turn drm_hdmi_state_helper.c hdmi_clock_valid into a public
function, and then call it from drm_bridge_connector mode_valid hook.

It's a similar discussion to the previous one really: in order to
implement it properly, we need access to drm_display_info.

Maxime