Hi,
There's still the scrambler issue we discussed on v15, but I have some
more comments.
On Tue, Jul 02, 2024 at 08:22:36PM GMT, Sandor Yu wrote:
> +enum drm_connector_status cdns_mhdp8501_detect(struct cdns_mhdp8501_device *mhdp)
> +{
> + u8 hpd = 0xf;
> +
> + hpd = cdns_mhdp8501_read_hpd(mhdp);
> + if (hpd == 1)
> + return connector_status_connected;
> + else if (hpd == 0)
> + return connector_status_disconnected;
> +
> + dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n", hpd);
> + return connector_status_unknown;
> +}
> +
> +static void hotplug_work_func(struct work_struct *work)
> +{
> + struct cdns_mhdp8501_device *mhdp = container_of(work,
> + struct cdns_mhdp8501_device,
> + hotplug_work.work);
> + enum drm_connector_status status = cdns_mhdp8501_detect(mhdp);
> +
> + drm_bridge_hpd_notify(&mhdp->bridge, status);
> +
> + if (status == connector_status_connected) {
> + /* Cable connected */
> + DRM_INFO("HDMI/DP Cable Plug In\n");
> + enable_irq(mhdp->irq[IRQ_OUT]);
> + } else if (status == connector_status_disconnected) {
> + /* Cable Disconnected */
> + DRM_INFO("HDMI/DP Cable Plug Out\n");
> + enable_irq(mhdp->irq[IRQ_IN]);
> + }
> +}
You shouldn't play with the interrupt being enabled here: hotplug
interrupts should always enabled.
If you can't for some reason, the reason should be documented in your
driver.
> + /* Mailbox protect for HDMI PHY access */
> + mutex_lock(&mhdp->mbox_mutex);
> + ret = phy_init(mhdp->phy);
> + mutex_unlock(&mhdp->mbox_mutex);
> + if (ret) {
> + dev_err(dev, "Failed to initialize PHY: %d\n", ret);
> + goto clk_disable;
> + }
> +
> + /* Mailbox protect for HDMI PHY access */
> + mutex_lock(&mhdp->mbox_mutex);
> + ret = phy_set_mode(mhdp->phy, phy_mode);
> + mutex_unlock(&mhdp->mbox_mutex);
> + if (ret) {
> + dev_err(dev, "Failed to configure PHY: %d\n", ret);
> + goto clk_disable;
> + }
Why do you need a shared mutex between the phy and HDMI controller?
> +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;
> +
> + /* Mailbox protect for HDMI PHY access */
> + mutex_lock(&mhdp->mbox_mutex);
> + ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
> + mutex_unlock(&mhdp->mbox_mutex);
> + 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;
> +
> + /* MAX support pixel clock rate 594MHz */
> + if (mode->clock > 594000)
> + return MODE_CLOCK_HIGH;
This needs to be in the tmds_char_rate_valid function
> + 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);
It will already be called by the core so this is redundant.
> +static void cdns_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_state)
> +{
> + struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> + struct drm_atomic_state *state = old_state->base.state;
> + struct drm_connector *connector;
> + struct video_info *video = &mhdp->video_info;
> + struct drm_crtc_state *crtc_state;
> + struct drm_connector_state *conn_state;
> + struct drm_display_mode *mode = &mhdp->mode;
> + union phy_configure_opts phy_cfg;
> + int ret;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(state,
> + bridge->encoder);
> + if (WARN_ON(!connector))
> + return;
> +
> + mhdp->curr_conn = connector;
> +
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + if (WARN_ON(!conn_state))
> + return;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> + if (WARN_ON(!crtc_state))
> + return;
> +
> + video->color_fmt = conn_state->hdmi.output_format;
> + video->bpc = conn_state->hdmi.output_bpc;
> +
> + drm_mode_copy(&mhdp->mode, &crtc_state->adjusted_mode);
Why do you need a copy of all these fields? You should pass the
connector / bridge state around and not copy these fields.
> + /* video mode check */
> + if (mode->clock == 0 || mode->hdisplay == 0 || mode->vdisplay == 0)
> + return;
This should be checked in atomic_check, but I'm pretty sure it's redundant.
> + dev_dbg(mhdp->dev, "Mode: %dx%dp%d\n",
> + mode->hdisplay, mode->vdisplay, mode->clock);
> +
> + drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
> +
> + /* Line swapping */
> + cdns_mhdp_reg_write(&mhdp->base, LANES_CONFIG, 0x00400000 | mhdp->lane_mapping);
> +
> + mhdp->hdmi.char_rate = drm_hdmi_compute_mode_clock(mode,
> + mhdp->video_info.bpc,
> + mhdp->video_info.color_fmt);
The TMDS char rate is already available in the connector_state so there
no need to recompute it.
> + phy_cfg.hdmi.tmds_char_rate = mhdp->hdmi.char_rate;
And you shouldn't store a copy either.
> + /* Mailbox protect for HDMI PHY access */
> + mutex_lock(&mhdp->mbox_mutex);
> + ret = phy_configure(mhdp->phy, &phy_cfg);
> + mutex_unlock(&mhdp->mbox_mutex);
> + if (ret) {
> + dev_err(mhdp->dev, "%s: phy_configure() failed: %d\n",
> + __func__, ret);
> + return;
> + }
> +
> + cdns_hdmi_sink_config(mhdp);
> +
> + ret = cdns_hdmi_ctrl_init(mhdp);
> + if (ret < 0) {
> + dev_err(mhdp->dev, "%s, ret = %d\n", __func__, ret);
> + return;
> + }
> +
> + /* Config GCP */
> + if (mhdp->video_info.bpc == 8)
> + cdns_hdmi_disable_gcp(mhdp);
> + else
> + cdns_hdmi_enable_gcp(mhdp);
> +
> + ret = cdns_hdmi_mode_config(mhdp, mode, &mhdp->video_info);
> + if (ret < 0) {
> + dev_err(mhdp->dev, "CDN_API_HDMITX_SetVic_blocking ret = %d\n", ret);
> + return;
> + }
> +}
> +
> +static int cdns_hdmi_bridge_clear_infoframe(struct drm_bridge *bridge,
> + enum hdmi_infoframe_type type)
> +{
> + return 0;
> +}
Either implement it or don't, but an empty function is dead code.
> +static int cdns_hdmi_bridge_write_infoframe(struct drm_bridge *bridge,
> + enum hdmi_infoframe_type type,
> + const u8 *buffer, size_t len)
> +{
> + struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> +
> + switch (type) {
> + case HDMI_INFOFRAME_TYPE_AVI:
> + cdns_hdmi_config_infoframe(mhdp, 0, len, buffer, HDMI_INFOFRAME_TYPE_AVI);
> + break;
> + case HDMI_INFOFRAME_TYPE_SPD:
> + cdns_hdmi_config_infoframe(mhdp, 1, len, buffer, HDMI_INFOFRAME_TYPE_SPD);
> + break;
> + case HDMI_INFOFRAME_TYPE_VENDOR:
> + cdns_hdmi_config_infoframe(mhdp, 2, len, buffer, HDMI_INFOFRAME_TYPE_VENDOR);
> + break;
> + default:
> + dev_dbg(mhdp->dev, "Unsupported infoframe type %x\n", type);
> + }
> +
> + return 0;
> +}
> +
> +static int cdns_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + return drm_atomic_helper_connector_hdmi_check(conn_state->connector, conn_state->state);
> +}
You should also call your mode_valid function here.
Maxime
Attachment:
signature.asc
Description: PGP signature
