Re:[PATCH v8] drm/rockchip: rk3066_hdmi: add sound support

["Andy Yan" <andyshrk@163.com>]

Hi Johan，

At 2024-07-03 02:23:24, "Johan Jonker" <jbx6244@gmail.com> wrote:
>Add sound support to the RK3066 HDMI driver.
>The HDMI TX audio source is connected to I2S_8CH.
>
>Signed-off-by: Zheng Yang <zhengyang@rock-chips.com>
>Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>---
>
>Changed V8:
>  return -EPROBE_DEFER as early as possible
>  move rk3066_hdmi_audio_codec_init() function after rk3066_hdmi_register()
>  restyle
>
>Changed V7:
>  rebase
>---
> drivers/gpu/drm/rockchip/Kconfig       |   2 +
> drivers/gpu/drm/rockchip/rk3066_hdmi.c | 296 +++++++++++++++++++++++--
> 2 files changed, 283 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
>index 1bf3e2829cd0..a32ee558408c 100644
>--- a/drivers/gpu/drm/rockchip/Kconfig
>+++ b/drivers/gpu/drm/rockchip/Kconfig
>@@ -102,6 +102,8 @@ config ROCKCHIP_RGB
> config ROCKCHIP_RK3066_HDMI
> 	bool "Rockchip specific extensions for RK3066 HDMI"
> 	depends on DRM_ROCKCHIP
>+	select SND_SOC_HDMI_CODEC if SND_SOC
>+	select SND_SOC_ROCKCHIP_I2S if SND_SOC
> 	help
> 	  This selects support for Rockchip SoC specific extensions
> 	  for the RK3066 HDMI driver. If you want to enable
>diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
>index 784de990da1b..296094a3c405 100644
>--- a/driver

.....

k3066_hdmi *hdmi, struct device *dev)
>+{
>+	struct hdmi_codec_pdata codec_data = {
>+		.i2s = 1,
>+		.ops = &audio_codec_ops,
>+		.max_i2s_channels = 8,
>+	};
>+
>+	hdmi->audio.channels = 2;
>+	hdmi->audio.sample_rate = 48000;
>+	hdmi->audio.sample_width = 16;
>+	hdmi->audio_enable = false;
>+	hdmi->audio_pdev =
>+		platform_device_register_data(dev,
>+					      HDMI_CODEC_DRV_NAME,
>+					      PLATFORM_DEVID_NONE,
>+					      &codec_data,
>+					      sizeof(codec_data));
>+
>+	return PTR_ERR_OR_ZERO(hdmi->audio_pdev);
>+}
>+
>+static int rk3066_hdmi_register(struct drm_device *drm, struct rk3066_hdmi *hdmi)
>+{
>+	struct drm_encoder *encoder = &hdmi->encoder.encoder;
>
> 	drm_encoder_helper_add(encoder, &rk3066_hdmi_encoder_helper_funcs);
> 	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
>@@ -740,6 +988,7 @@ static int rk3066_hdmi_bind(struct device *dev, struct device *master,
> {
> 	struct platform_device *pdev = to_platform_device(dev);
> 	struct drm_device *drm = data;
>+	struct drm_encoder *encoder;
> 	struct rk3066_hdmi *hdmi;
> 	int irq;
> 	int ret;
>@@ -748,8 +997,21 @@ static int rk3066_hdmi_bind(struct device *dev, struct device *master,
> 	if (!hdmi)
> 		return -ENOMEM;
>
>+	encoder = &hdmi->encoder.encoder;
>+
>+	encoder->possible_crtcs =
>+		drm_of_find_possible_crtcs(drm, dev->of_node);
>+
>+	/*
>+	 * If we failed to find the CRTC(s) which this encoder is
>+	 * supposed to be connected to, it's because the CRTC has
>+	 * not been registered yet.  Defer probing, and hope that
>+	 * the required CRTC is added later.
>+	 */
>+	if (encoder->possible_crtcs == 0)
>+		return -EPROBE_DEFER;
>+






Move  EPROBE_DEFER early still does not avoid the issue I mentioned in V7.

For the component based driver mode in drm，the probe of last subcomponent（component_add（）） will
trigger the bind callback of all subcomponent， any   -EPROBE_DEFER  return in the bind callback will lead
the  EPROBE_DEFER of the last subcomponent .probe.
For one example on rk3066:
LCDC0-->HDMI
LCDC1-->RGB-->panel
lcdc0(vop_probe)--->hdmi_bind-->audio register-->hdmi_codec_probe success-->lcdc1(vop_bind)-->
rockchip_rgb_init(defer by somehow,maybe panel register failed)--> lcdc0(vop_probe) return   EPROBE_DEFER.

As there is on success probe(hdmi_codec_probe) during the process, this will trigger a new deferred probe, see:
static int driver_probe_device(struct device_driver *drv, struct device *dev)
{
        int trigger_count = atomic_read(&deferred_trigger_count); 
        int ret;

        atomic_inc(&probe_count); 
        ret = __driver_probe_device(drv, dev);
        if (ret == -EPROBE_DEFER || ret == EPROBE_DEFER) {
                driver_deferred_probe_add(dev);

                /*
                 * Did a trigger occur while probing? Need to re-trigger if yes
                 */
                if (trigger_count != atomic_read(&deferred_trigger_count) &&
                    !defer_all_probes)
                        driver_deferred_probe_trigger();
        }
        atomic_dec(&probe_count);
        wake_up_all(&probe_waitqueue);
        return ret;
}

So the potentia of infinite loop porbe rise。

I think one possible solutiion is register the extra dirvers（hdmi audio，cec，hdcp）at the
encoder->funcs->late_register hook。
the late_register is called at the very last of drm_dev_register,  all the subcompent must
bind success if we finally run to this step.

> 	hdmi->dev = dev;
>-	hdmi->drm_dev = drm;
> 	hdmi->regs = devm_platform_ioremap_resource(pdev, 0);
> 	if (IS_ERR(hdmi->regs))
> 		return PTR_ERR(hdmi->regs);
>@@ -800,6 +1062,8 @@ static int rk3066_hdmi_bind(struct device *dev, struct device *master,
> 	if (ret)
> 		goto err_disable_i2c;
>
>+	rk3066_hdmi_audio_codec_init(hdmi, dev);
>+
> 	dev_set_drvdata(dev, hdmi);
>
> 	ret = devm_request_threaded_irq(dev, irq, rk3066_hdmi_hardirq,
>@@ -813,6 +1077,7 @@ static int rk3066_hdmi_bind(struct device *dev, struct device *master,
> 	return 0;
>
> err_cleanup_hdmi:
>+	platform_device_unregister(hdmi->audio_pdev);
> 	hdmi->connector.funcs->destroy(&hdmi->connector);
> 	hdmi->encoder.encoder.funcs->destroy(&hdmi->encoder.encoder);
> err_disable_i2c:
>@@ -828,6 +1093,7 @@ static void rk3066_hdmi_unbind(struct device *dev, struct device *master,
> {
> 	struct rk3066_hdmi *hdmi = dev_get_drvdata(dev);
>
>+	platform_device_unregister(hdmi->audio_pdev);
> 	hdmi->connector.funcs->destroy(&hdmi->connector);
> 	hdmi->encoder.encoder.funcs->destroy(&hdmi->encoder.encoder);
>
>--
>2.39.2