Re: [PATCH v1 7/7] drm/mediatek: Introduce HDMI/DDC v2 for MT8195/MT8188

[CK Hu (胡俊光)]

On Wed, 2024-12-04 at 15:44 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> Il 02/12/24 08:27, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Wed, 2024-11-20 at 13:45 +0100, AngeloGioacchino Del Regno wrote:
> > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > 
> > > 
> > > 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.
> > > 
> > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > > ---
> > 
> > [snip]
> > 
> > > +static int mtk_hdmi_v2_enable(struct mtk_hdmi *hdmi)
> > > +{
> > > +       int ret;
> > > +
> > > +       ret = pm_runtime_resume_and_get(hdmi->dev);
> > 
> > Why turn on power when attach?
> > Should hot-plug detection be always on so power on when attach?
> > If so, maybe we need to use CEC to detect hot-plug because CEC has lower power and HDMI power would not be always on.
> > As you describe previously, CEC just need some hack to support both HDMI Tx and HDMI Rx.
> > So there is a software solution to use CEC to detect hot-plug in a lower power mode.
> > And v1 use CEC to detect hot-plug, so it's worth to make v1 and v2 has the same behavior.
> > 
> 
> As I said, we can't use CEC to detect hotplug, because the same CEC controller
> can be used for both HDMI TX and RX at the same time.
> 
> If you use CEC to detect cable attach and you have both HDMI RX and HDMI TX drivers
> probed and ready, if you attach the cable to HDMI RX port, HDMI TX will try to get
> enabled and will error out.
> Same happens if you attach a cable to HDMI TX: HDMI RX will try to get enabled and
> will error out.
> 
> The only way to use CEC for hotplug detection is to detect twice: CEC for HDMI
> TX/RX controllers wakeup, then reset both, wait for autodetect in both controllers,
> suppress errors, and start signal transmit or receive.
> 
> While it is technically possible to do so, it's very complicated and hacky.
> 
> Please keep in mind that on the new SoCs (8188/8195) the CEC block is different
> from the one in the old SoCs (6795, 8173, etc) and requires an entirely new driver
> to get it working.
> 
> Using CEC for hotplug detection would mean that we need 3 full drivers:
> 1. HDMI TX
> 2. HDMI RX
> 3. CEC
> 
> This is because the CEC is shared, so we need to have all three to properly test
> the functionality - and this is not possible to do all at once.
> 
> To save some power (not much, though!), we can eventually go for your proposed
> CEC detection *in the future*, but really not right now - and we can do that only
> after upstreaming both HDMI TX and HDMI RX drivers.
> 
> The plan (time/effort pemitting!) would look like this:
> 
> Series 1 - Upstream HDMIv2 TX (HPD through HDMIv2 TX controller) - drivers/gpu/drm
> Series 2 - Upstream HDMIv2 RX (HPD through HDMIv2 RX controller) - drivers/media
> Series 3 - Upstream CEC driver (No HPD in CEC)                   - drivers/media
> Series 4 - Implement HPD in CEC and use it in HDMIv2 TX and HDMIv2 RX - media,drm
> 
> You can see how complicated this is - and I'm sure that you understand why that
> cannot be done all at once.
> 
> Adding up some: the HDMI driver is used only on IoT boards for now, which typically
> run off AC and not battery - so the (again) very little power consumption raise is
> not critical for now.

If you plan to upstream HDMI RX driver recently, it's OK not using CEC to detect hot-plug.
But for any behavior which is different with v1 and it's related to CEC, add comment about CEC.

Regards,
CK

> 
> Cheers,
> Angelo
> 
> > Regards,
> > CK
> > 
> > 
> > > +       if (ret) {
> > > +               dev_err(hdmi->dev, "Cannot resume HDMI\n");
> > > +               return ret;
> > > +       }
> > > +
> > > +       mtk_hdmi_v2_clk_enable(hdmi);
> > > +       mtk_hdmi_hw_reset(hdmi);
> > > +       mtk_hdmi_set_sw_hpd(hdmi, true);
> > > +
> > > +       return 0;
> > > +}
> > > +