On Mon, Dec 09, 2024 at 08:38:01AM +0000, Sandor Yu wrote:
>
>
> > -----Original Message-----
> > From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > Sent: 2024年11月27日 3:21
> > To: Sandor Yu <sandor.yu@xxxxxxx>
> > Cc: 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;
> > mripard@xxxxxxxxxx; kernel@xxxxxxxxxxxxxx; dl-linux-imx
> > <linux-imx@xxxxxxx>; Oliver Brown <oliver.brown@xxxxxxx>;
> > alexander.stein@xxxxxxxxxxxxxxx; sam@xxxxxxxxxxxx
> > Subject: [EXT] Re: [PATCH v19 4/8] drm: bridge: Cadence: Add MHDP8501
> > DP/HDMI driver
Please strip such headers from your response emails. They take a lot of
space and are useless. See how it's usually handled by other email
clients.
> >
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report
> > this email' button
> >
> >
> > On Tue, Nov 26, 2024 at 10:11:49PM +0800, Sandor Yu wrote:
> > > Add a new DRM DisplayPort and HDMI bridge driver for Candence
> > MHDP8501
> > > used in i.MX8MQ SOC. MHDP8501 could support HDMI or DisplayPort
> > > standards according embedded Firmware running in the uCPU.
> > >
> > > For iMX8MQ SOC, the DisplayPort/HDMI FW was loaded and activated by
> > > SOC's ROM code. Bootload binary included respective specific firmware
> > > is required.
> > >
> > > Driver will check display connector type and
> > > then load the corresponding driver.
> > >
> > > Signed-off-by: Sandor Yu <Sandor.yu@xxxxxxx>
> > > ---
[...]
> > > +
> > > +static int reset_pipe(struct drm_crtc *crtc)
> > > +{
> > > + struct drm_atomic_state *state;
> > > + struct drm_crtc_state *crtc_state;
> > > + struct drm_modeset_acquire_ctx ctx;
> > > + int ret;
> > > +
> > > + state = drm_atomic_state_alloc(crtc->dev);
> > > + if (!state)
> > > + return -ENOMEM;
> > > +
> > > + drm_modeset_acquire_init(&ctx, 0);
> > > +
> > > + state->acquire_ctx = &ctx;
> > > +
> > > + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > > + if (IS_ERR(crtc_state)) {
> > > + ret = PTR_ERR(crtc_state);
> > > + goto out;
> > > + }
> > > +
> > > + crtc_state->connectors_changed = true;
> > > +
> > > + ret = drm_atomic_commit(state);
> >
> > I'd say, this looks like a hack to me.
>
> Basically, those code followed the sequence as vc4 HDMI and i915 according review comments in v15.
> HDMI driver should " Reenabling the scrambler when a display is disconnected and reconnected".
>
> To be honest, I'm not 100% convinced by this implementation,
> as the code doesn't seem to align perfectly with the current DRM framework.
> However, adding this feature would indeed address the issue of applications (like modetest) that don't handle HPD events
> but still require cable plug/unplug support when operating in scrambler-enabled video modes.
Ack. This matches vc4's reset_pipe().
>
> >
> > > +out:
> > > + drm_atomic_state_put(state);
> > > + drm_modeset_drop_locks(&ctx);
> > > + drm_modeset_acquire_fini(&ctx);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +void cdns_hdmi_reset_link(struct cdns_mhdp8501_device *mhdp)
> > > +{
> > > + struct drm_connector *connector = mhdp->curr_conn;
> > > + const struct drm_edid *drm_edid;
> > > + struct drm_connector_state *conn_state;
> > > + struct drm_crtc_state *crtc_state;
> > > + struct drm_crtc *crtc;
> > > +
> > > + if (!connector)
> > > + return;
> > > +
> > > + drm_edid = drm_edid_read_custom(connector,
> > cdns_hdmi_get_edid_block, mhdp);
> > > + drm_edid_connector_update(connector, drm_edid);
> >
> > Why?
>
> MHDP8501 HDMI support scrambling.low_rates.
> The scrambler status can change when switching HDMI display monitors, even if the video mode stays the same.
> The MHDP8501's HDMI code updates the EDID data to accommodate displays with different scrambler capabilities.
Ack. I'd suggest renaming cdns_hdmi_reset_link() to
cdns_hdmi_handle_hotplug() or any other similar name.
>
> >
> > > +
> > > + if (!drm_edid)
> > > + return;
> > > +
> > > + drm_edid_free(drm_edid);
> > > +
> > > + conn_state = connector->state;
> > > + crtc = conn_state->crtc;
> > > + if (!crtc)
> > > + return;
> > > +
> > > + crtc_state = crtc->state;
> > > + if (!crtc_state->active)
> > > + return;
> > > +
> > > + /*
> > > + * HDMI 2.0 says that one should not send scrambled data
> > > + * prior to configuring the sink scrambling, and that
> > > + * TMDS clock/data transmission should be suspended when
> > > + * changing the TMDS clock rate in the sink. So let's
> > > + * just do a full modeset here, even though some sinks
> > > + * would be perfectly happy if were to just reconfigure
> > > + * the SCDC settings on the fly.
> > > + */
> > > + reset_pipe(crtc);
> >
> > We are not supposed to cause full-modeset flicker if we can avoid it. Is
> > it the case here?
>
> My implementation differs slightly from the VC4 HDMI in that it doesn't check if the current video mode requires enabling the scrambler.
> This is because we considered a scenario where the previous display was operating at 1080p60 with scrambling enabled,
> and then the cable was plugged into a display that doesn't support scrambling at low rates.
> This case also necessitates a full modeset.
Well, if this is a part of the hotplug, then of course there are no
flickering issues as it happens during hotplug.
>
> I haven't found any video flickering issues in my actual tests.
>
> >
> > > +}
> > > +
> > > +static int cdns_hdmi_i2c_write(struct cdns_mhdp8501_device *mhdp,
> > > + struct i2c_msg *msgs)
> > > +{
> > > + u8 msg[5], reg[5];
> > > + int ret;
> > > +
> > > + msg[0] = msgs->addr;
> > > + msg[1] = msgs->buf[0];
> > > + msg[2] = 0;
> > > + msg[3] = 1;
> > > + msg[4] = msgs->buf[1];
> > > +
> > > + ret = cdns_mhdp_mailbox_send_recv(&mhdp->base,
> > > +
> > MB_MODULE_ID_HDMI_TX, HDMI_TX_WRITE,
> > > + sizeof(msg), msg,
> > sizeof(reg), reg);
> > > + if (ret) {
> > > + dev_err(mhdp->dev, "I2C write failed: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + if (reg[0] != 0)
> > > + return -EINVAL;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int cdns_hdmi_i2c_read(struct cdns_mhdp8501_device *mhdp,
> > > + struct i2c_msg *msgs, int num)
> > > +{
> > > + u8 msg[4], reg[5];
> > > + u8 addr, offset, *buf, len;
> > > + int ret, i;
> > > +
> > > + for (i = 0; i < num; i++) {
> > > + if (msgs[i].flags & I2C_M_RD) {
> > > + addr = msgs[i].addr;
> > > + buf = msgs[i].buf;
> > > + len = msgs[i].len;
> > > + } else {
> > > + offset = msgs[i].buf[0];
> > > + }
> > > + }
> > > +
> > > + msg[0] = addr;
> > > + msg[1] = offset;
> > > + put_unaligned_be16(len, msg + 2);
> > > +
> > > + ret = cdns_mhdp_mailbox_send_recv_multi(&mhdp->base,
> > > +
> > MB_MODULE_ID_HDMI_TX, HDMI_TX_READ,
> > > + sizeof(msg), msg,
> > > + HDMI_TX_READ,
> > > + sizeof(reg), reg,
> > > + len, buf);
> > > + if (ret) {
> > > + dev_err(mhdp->dev, "I2c Read failed: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +#define SCDC_I2C_SLAVE_ADDRESS 0x54
> > > +static int cdns_hdmi_i2c_xfer(struct i2c_adapter *adap,
> > > + struct i2c_msg *msgs, int num)
> > > +{
> > > + struct cdns_mhdp8501_device *mhdp = i2c_get_adapdata(adap);
> > > + struct cdns_hdmi_i2c *i2c = mhdp->hdmi.i2c;
> > > + int i, ret = 0;
> > > +
> > > + /* Only support SCDC I2C Read/Write */
> > > + for (i = 0; i < num; i++) {
> > > + if (msgs[i].addr != SCDC_I2C_SLAVE_ADDRESS) {
> > > + dev_err(mhdp->dev, "ADDR=%0x is not
> > supported\n", msgs[i].addr);
> > > + return -EINVAL;
> >
> > Why?
>
> MHDP FW offers mailbox APIs for SCDC register access but no direct I2C APIs.
> Individual I2C registers can be read/written using HDMI general register APIs,
> but block reads (e.g., EDID) are not supported, so it is not a full function I2C.
> i2c_adapter was implemented only for reuse drm_scdc_XXX functions.
Please put this info in the comment. From your 'Only support foo' it's
not obvious if the hw/fw doesn't support it or if you are artifically
limiting it on the driver's side.
>
--
With best wishes
Dmitry
