On Tue, Sep 03, 2024 at 06:07:25AM GMT, Sandor Yu wrote: > Hi Maxime, > > Thanks for your comments, > > > -----Original Message----- > > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of > > Maxime Ripard > > Sent: 2024年7月2日 21:25 > > To: Sandor Yu <sandor.yu@xxxxxxx> > > Cc: dmitry.baryshkov@xxxxxxxxxx; andrzej.hajda@xxxxxxxxx; > > neil.armstrong@xxxxxxxxxx; Laurent Pinchart > > <laurent.pinchart@xxxxxxxxxxxxxxxx>; jonas@xxxxxxxxx; > > jernej.skrabec@xxxxxxxxx; airlied@xxxxxxxxx; daniel@xxxxxxxx; > > robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; > > shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; > > vkoul@xxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > > linux-kernel@xxxxxxxxxxxxxxx; linux-phy@xxxxxxxxxxxxxxxxxxx; > > kernel@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Oliver Brown > > <oliver.brown@xxxxxxx>; alexander.stein@xxxxxxxxxxxxxxx; > > sam@xxxxxxxxxxxx > > Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 > > DP/HDMI driver > > > > 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. > > iMX8MQ have two HPD interrupters, one for plugout and the other for plugin, > because they could not be masked, so we have to enable one and disable the other. > I will add more comments here. Right, but why do you need to enable and disable them? Do you get spurious interrupts? > > > > > + /* 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? > > Both PHY and HDMI controller could access to the HDMI firmware by mailbox, > So add mutex to avoid race condition. That should be handled at either the phy or mailbox level, not in your hdmi driver. > > > > > +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 > This clock rate check is covered by function tmds_char_rate_valid() > It could be removed if keep function tmds_char_rate_valid() be called by mode_valid. Yeah, it's not something you should have to duplicate. > > > > > + 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. > > mode_valid function is use to filter the mode list in drm_helper_probe_single_connector_modes(), > if function cdns_hdmi_tmds_char_rate_valid() is not called, unsupported modes will in mode list. It's probably something we should deal with in the core somehow. I'm not entirely sure how to reconcile drm_bridge_connector and the hdmi framework there, but we should at the very least provide a mode_valid helper for bridges. > > > > > +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. > > > OK, mode will be removed, and it will replace by drm_connector_state. > > > > + /* 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. > OK, It will be removed. > > > > > + 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. > OK, variable char_rate will be removed and will use the drm_connector_state-> hdmi.tmds_char_rate > > > > > > + /* 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. > Must have function hdmi_clear_infoframe when set DRM_BRIDGE_OP_HDMI flag in &drm_bridge->ops, > otherwise the drm_bridge_connector_init() will fail. > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/drm_bridge_connector.c?h=next-20240902#n419 That's a strong hint that you should implement it, not workaround it :) Maxime
Attachment:
signature.asc
Description: PGP signature