On Thu, Dec 05, 2024 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
> Il 05/12/24 13:56, Dmitry Baryshkov ha scritto:
> > On Thu, Dec 05, 2024 at 12:45:17PM +0100, AngeloGioacchino Del Regno wrote:
> > > Add support for the newer HDMI-TX (Encoder) v2 and DDC v2 IPs
> > > found in MediaTek's MT8195, MT8188 SoC and their variants, and
> > > including support for display modes up to 4k60 and for HDMI
> > > Audio, as per the HDMI 2.0 spec.
> > >
> > > HDCP and CEC functionalities are also supported by this hardware,
> > > but are not included in this commit and that also poses a slight
> > > difference between the V2 and V1 controllers in how they handle
> > > Hotplug Detection (HPD).
> > >
> > > While the v1 controller was using the CEC controller to check
> > > HDMI cable connection and disconnection, in this driver the v2
> > > one does not.
> > >
> > > This is due to the fact that on parts with v2 designs, like the
> > > MT8195 SoC, there is one CEC controller shared between the HDMI
> > > Transmitter (HDMI-TX) and Receiver (HDMI-RX): before eventually
> > > adding support to use the CEC HW to wake up the HDMI controllers
> > > it is necessary to have support for one TX, one RX *and* for both
> > > at the same time.
> > >
> > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/mediatek/Kconfig | 8 +
> > > drivers/gpu/drm/mediatek/Makefile | 4 +
> > > drivers/gpu/drm/mediatek/mtk_hdmi_common.c | 5 +
> > > drivers/gpu/drm/mediatek/mtk_hdmi_common.h | 1 +
> > > drivers/gpu/drm/mediatek/mtk_hdmi_ddc_v2.c | 403 +++++
> > > drivers/gpu/drm/mediatek/mtk_hdmi_regs_v2.h | 263 ++++
> > > drivers/gpu/drm/mediatek/mtk_hdmi_v2.c | 1488 +++++++++++++++++++
> > > 7 files changed, 2172 insertions(+)
> > > create mode 100644 drivers/gpu/drm/mediatek/mtk_hdmi_ddc_v2.c
> > > create mode 100644 drivers/gpu/drm/mediatek/mtk_hdmi_regs_v2.h
> > > create mode 100644 drivers/gpu/drm/mediatek/mtk_hdmi_v2.c
> > >
>
> ..snip..
>
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_v2.c b/drivers/gpu/drm/mediatek/mtk_hdmi_v2.c
> > > new file mode 100644
> > > index 000000000000..05cbfc45be54
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/mediatek/mtk_hdmi_v2.c
> > > @@ -0,0 +1,1488 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
>
> ..snip..
>
> > > +static int mtk_hdmi_v2_setup_audio_infoframe(struct mtk_hdmi *hdmi)
> > > +{
> > > + struct hdmi_codec_params *params = &hdmi->aud_param.codec_params;
> > > + struct hdmi_audio_infoframe frame;
> > > + u8 buffer[14];
> > > + ssize_t ret;
> > > +
> > > + memcpy(&frame, ¶ms->cea, sizeof(frame));
> > > +
> > > + ret = hdmi_audio_infoframe_pack(&frame, buffer, sizeof(buffer));
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + mtk_hdmi_v2_hw_write_audio_infoframe(hdmi, buffer);
> >
> > This should be handled via HDMI Connector framework too. There is
> > already an interface to set / clear audio infoframes.
> > als I have been working on an interface to make HDMI codec
> > implementation more generic, see [1]. Your comments are appreciated.
> >
> > [1] https://patchwork.freedesktop.org/series/134927/
> >
>
> I didn't go for that because I was afraid that your series wouldn't actually
> land in the same window as this driver.
There were two points in my comment (sorry for being not explicit
enough). One is to point to the HDMI codec infra (and you are right,
it's not yet in the mergeable state). Second one is to use
drm_atomic_helper_connector_hdmi_update_audio_infoframe() and
drm_atomic_helper_connector_hdmi_clear_audio_infoframe() if possible (it
might be hard to do it - in fact it being hard kind of forced me to
work on the HDMI codec infra).
> I am at this point puzzled to whether I should add a commit on top of this
> submission that switches this driver to using the generic HDMI Codec infra
> (which could also be helpful as guidance to others trying to do the same on
> other drivers, I guess), or if I should just rebase on top of that.
I'd say, a completely separate patch might be even better, I can then
integrate it into the HDMI codec patchset, adding a soft dependency on
your series. If for some reason the codec lands earlier than your
patchset, the patch can be dropped and reintegrated into your series. Or
just committed separately.
> I'd rather add a commit on top to mainly avoid risking to delay this driver,
> but if you're absolutely certain that you can get your series merged in the
> same window as this driver I have *zero* problems in just rebasing on top.
>
> Besides...
>
> I'll find the time to review your series - I'm sad it didn't land earlier as
> I would've written a bit less code otherwise :-)
>
> > > +
> > > + return 0;
> > > +}
> > > +
>
> ..snip..
>
> > > +
> > > + /* Enable the controller at attach time for HPD/Pord feedback */
> > > + ret = mtk_hdmi_v2_enable(hdmi);
> > > + if (ret)
> > > + return ret;
> >
> > This looks like a hack. Please use .hpd_enable() / .hpd_disable()
> > callbacks.
> >
>
> ...but there's no way to power on only the HPD - you can either power on
> the entire controller, or nothing.
>
> I can't call mtk_hdmi_v2_enable/disable() from the HPD callbacks.
Why? Add a powerup refcount if necessary.
>
> Perhaps the comment should have been
>
> "Enable the controller attach time also allows HPD/Pord feedback"?
I think it should still be explicit call from those callbacks. Consider
a platform using external GPIO to implement HPD. Then the HDMI
controller can stay disabled if it is not necessary.
>
> > > +
> > > + /* Enable Hotplug and Pord pins internal debouncing */
> > > + regmap_set_bits(hdmi->regs, HPD_DDC_CTRL,
> > > + HPD_DDC_HPD_DBNC_EN | HPD_DDC_PORD_DBNC_EN);
> > > +
> > > + irq_clear_status_flags(hdmi->irq, IRQ_NOAUTOEN);
> > > + enable_irq(hdmi->irq);
> > > +
> > > + /*
> > > + * Check if any HDMI monitor was connected before probing this driver
> > > + * and/or attaching the bridge, without debouncing: if so, we want to
> > > + * notify the DRM so that we start outputting an image ASAP.
> > > + * Note that calling the ISR thread function will also perform a HW
> > > + * registers write that enables both the HPD and Pord interrupts.
> > > + */
> > > + __mtk_hdmi_v2_isr_thread(hdmi);
> > > +
> > > + return 0;
> > > +}
> > > +
>
> ..snip..
>
> > > +static void mtk_hdmi_v2_bridge_pre_enable(struct drm_bridge *bridge,
> > > + struct drm_bridge_state *old_state)
> > > +{
> > > + struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> > > + struct drm_atomic_state *state = old_state->base.state;
> > > + struct drm_connector_state *conn_state;
> > > + union phy_configure_opts opts = {
> > > + .dp = { .link_rate = hdmi->mode.clock * KILO }
> > > + };
> > > +
> > > + /* Retrieve the connector through the atomic state */
> > > + hdmi->curr_conn = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> > > +
> > > + conn_state = drm_atomic_get_new_connector_state(state, hdmi->curr_conn);
> > > + if (WARN_ON(!conn_state))
> > > + return;
> > > +
> > > + /*
> > > + * Preconfigure the HDMI controller and the HDMI PHY at pre_enable
> > > + * stage to make sure that this IP is ready and clocked before the
> > > + * mtk_dpi gets powered on and before it enables the output.
> > > + */
> > > + hdmi->dvi_mode = !hdmi->curr_conn->display_info.is_hdmi;
> >
> > Can you access display_info directly instead?
> >
>
> Nice catch. Yeah, I should've done that from the beginning. Fixed for v3.
>
> > > + mtk_hdmi_v2_output_set_display_mode(hdmi, &hdmi->mode);
> > > +
> > > + /* Reconfigure phy clock link with appropriate rate */
> > > + phy_configure(hdmi->phy, &opts);
> > > +
> > > + /* Power on the PHY here to make sure that DPI_HDMI is clocked */
> > > + phy_power_on(hdmi->phy);
> > > +
> > > + hdmi->powered = true;
> > > +}
> > > +
> > > +static void mtk_hdmi_v2_bridge_enable(struct drm_bridge *bridge,
> > > + struct drm_bridge_state *old_state)
> > > +{
> > > + struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> > > + struct drm_atomic_state *state = old_state->base.state;
> > > + int ret;
> > > +
> > > + ret = drm_atomic_helper_connector_hdmi_update_infoframes(hdmi->curr_conn, state);
> > > + if (ret)
> > > + dev_err(hdmi->dev, "Could not update infoframes: %d\n", ret);
> > > +
> > > + mtk_hdmi_v2_hw_vid_mute(hdmi, false);
> > > + mtk_hdmi_v2_hw_aud_mute(hdmi, false);
> > > +
> > > + /* signal the connect event to audio codec */
> > > + mtk_hdmi_v2_handle_plugged_change(hdmi, true);
> >
> > I think it was agreed that this should be called from the hotplug path,
> > see the (linked) HDMI codec infrastructure patchset and dicussions for
> > the previous revisions.
> >
>
> Okay, I'll check that out.
>
> > > +
> > > + hdmi->enabled = true;
> > > +}
> > > +
>
> ..snip..
>
> > > +
> > > +static int mtk_hdmi_v2_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
> > > + const struct drm_display_mode *mode,
> > > + unsigned long long tmds_rate)
> > > +{
> > > + if (mode->clock < MTK_HDMI_V2_CLOCK_MIN)
> > > + return MODE_CLOCK_LOW;
> > > + else if (mode->clock > MTK_HDMI_V2_CLOCK_MAX)
> > > + return MODE_CLOCK_HIGH;
> > > + else
> > > + return MODE_OK;
> > > +}
> > > +
> > > +static enum drm_mode_status
> > > +mtk_hdmi_v2_bridge_mode_valid(struct drm_bridge *bridge,
> > > + const struct drm_display_info *info,
> > > + const struct drm_display_mode *mode)
> > > +{
> > > + unsigned long long rate;
> > > +
> > > + rate = drm_hdmi_compute_mode_clock(mode, 8, HDMI_COLORSPACE_RGB);
> > > + return mtk_hdmi_v2_hdmi_tmds_char_rate_valid(bridge, mode, rate);
> > > +}
> >
> > Please rebase on top of https://patchwork.freedesktop.org/series/140193/
> > There should be no need to do this manually.
> >
>
> Oh finally there's a helper for this. Cool, will rebase! Thanks!
And it doesn't even add an extra dependency for you.
>
> > > +
> > > +static int mtk_hdmi_v2_hdmi_clear_infoframe(struct drm_bridge *bridge,
> > > + enum hdmi_infoframe_type type)
> > > +{
> > > + struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> > > + u32 reg_start, reg_end;
> > > +
> > > + switch (type) {
> > > + case HDMI_INFOFRAME_TYPE_AUDIO:
> > > + regmap_clear_bits(hdmi->regs, TOP_INFO_EN, AUD_EN | AUD_EN_WR);
> > > + regmap_clear_bits(hdmi->regs, TOP_INFO_RPT, AUD_RPT_EN);
> > > + reg_start = TOP_AIF_HEADER;
> > > + reg_end = TOP_AIF_PKT03;
> > > + break;
> > > + case HDMI_INFOFRAME_TYPE_AVI:
> > > + regmap_clear_bits(hdmi->regs, TOP_INFO_EN, AVI_EN_WR | AVI_EN);
> > > + regmap_clear_bits(hdmi->regs, TOP_INFO_RPT, AVI_RPT_EN);
> > > + reg_start = TOP_AVI_HEADER;
> > > + reg_end = TOP_AVI_PKT05;
> > > + break;
> > > + case HDMI_INFOFRAME_TYPE_SPD:
> > > + regmap_clear_bits(hdmi->regs, TOP_INFO_EN, SPD_EN_WR | SPD_EN);
> > > + regmap_clear_bits(hdmi->regs, TOP_INFO_RPT, SPD_RPT_EN);
> > > + reg_start = TOP_SPDIF_HEADER;
> > > + reg_end = TOP_SPDIF_PKT07;
> > > + break;
> > > + case HDMI_INFOFRAME_TYPE_VENDOR:
> > > + regmap_clear_bits(hdmi->regs, TOP_INFO_EN, VSIF_EN_WR | VSIF_EN);
> > > + regmap_clear_bits(hdmi->regs, TOP_INFO_RPT, VSIF_RPT_EN);
> > > + reg_start = TOP_VSIF_HEADER;
> > > + reg_end = TOP_VSIF_PKT07;
> > > + break;
> > > + case HDMI_INFOFRAME_TYPE_DRM:
> > > + default:
> > > + return 0;
> > > + };
> > > +
> > > + for (; reg_start <= reg_end; reg_start += 4)
> > > + regmap_write(hdmi->regs, reg_start, 0);
> >
> > Interesting. Usually sending of the infoframe is controlled by a
> > register of a kind.
> >
>
> "This callback clears the infoframes in the hardware during commit"
>
> ...and that's exactly what this function does: clears the infoframes
> in the HW and stops the RPT, making the HW ready for the "next round".
>
> Did I get the documentation wrong?
> Should this function *send* an all-zero infoframe to the external display?
No, it should stop sending the callback. My point was that usually there
is a separate register which controls the infoframes. You code just
clears the header (which probably is handled as disabling in the IP
core).
>
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_hdmi_v2_hdmi_write_infoframe(struct drm_bridge *bridge,
> > > + enum hdmi_infoframe_type type,
> > > + const u8 *buffer, size_t len)
> > > +{
> > > + struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> > > +
> > > + switch (type) {
> > > + case HDMI_INFOFRAME_TYPE_AUDIO:
> > > + mtk_hdmi_v2_hw_write_audio_infoframe(hdmi, buffer);
> > > + break;
> > > + case HDMI_INFOFRAME_TYPE_AVI:
> > > + mtk_hdmi_v2_hw_write_avi_infoframe(hdmi, buffer);
> > > + break;
> > > + case HDMI_INFOFRAME_TYPE_SPD:
> > > + mtk_hdmi_v2_hw_write_spd_infoframe(hdmi, buffer);
> > > + break;
> > > + case HDMI_INFOFRAME_TYPE_VENDOR:
> > > + mtk_hdmi_v2_hw_write_vendor_infoframe(hdmi, buffer);
> > > + break;
> > > + case HDMI_INFOFRAME_TYPE_DRM:
> > > + default:
> > > + dev_err(hdmi->dev, "Unsupported HDMI infoframe type %u\n", type);
> > > + break;
> > > + };
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_hdmi_v2_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);
> > > +}
> >
> > Note to myself, probably we can move this to drm_bridge_connector too.
> >
> > > +
> > > +static int mtk_hdmi_v2_set_abist(struct mtk_hdmi *hdmi, bool enable)
> > > +{
> > > + struct drm_display_mode *mode = &hdmi->mode;
> > > + int abist_format = -EINVAL;
> > > + bool interlaced;
> > > +
> > > + if (!enable) {
> > > + regmap_clear_bits(hdmi->regs, TOP_CFG00, HDMI_ABIST_ENABLE);
> > > + return 0;
> > > + }
> > > +
> > > + if (!mode->hdisplay || !mode->vdisplay)
> > > + return -EINVAL;
> > > +
> > > + interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
> >
> > The interlaced modes should be filtered, unless you also set
> > bridge->interlace_allowed to true.
> >
>
> Noted. Many Thanks for the review!
>
> I'll wait until next week for more feedback before sending a v3.
--
With best wishes
Dmitry
