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

Reply via email to