Re: [PATCH 3/4] drm/imx: imx-tve: probe DDC before claiming the connector is connected

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Missing polling configuration for dynamic hotplug detection.

Pre-existing issues:
- [High] Missing -EPROBE_DEFER handling undermines the intended fix under normal probe ordering.
--

--- Patch [3]: [PATCH 3/4] drm/imx: imx-tve: probe DDC before claiming the connector is connected ---
commit 496cfa19c398d86d2f77e8a07cbe96a51f14ca0a
Author: Dmitry Baryshkov <[email protected]>

drm/imx: imx-tve: probe DDC before claiming the connector is connected

This commit adds a detect_ctx hook to the TVE connector using the DDC bus to
detect whether a monitor is attached. It aims to fix a CRTC timeout issue
caused by unconditionally lighting up the VGA output when nothing is
connected.

> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-tve.c b/drivers/gpu/drm/imx/ipuv3/imx-tve.c
> index e861b8b9d8fa8..11ac69561eb81 100644
> --- a/drivers/gpu/drm/imx/ipuv3/imx-tve.c
> +++ b/drivers/gpu/drm/imx/ipuv3/imx-tve.c
> @@ -322,6 +322,7 @@ static const struct drm_connector_funcs imx_tve_connector_funcs = {
>  static const struct drm_connector_helper_funcs imx_tve_connector_helper_funcs = {
>  	.get_modes = imx_tve_connector_get_modes,
>  	.mode_valid = imx_tve_connector_mode_valid,
> +	.detect_ctx = drm_connector_helper_detect_from_ddc,

[Severity: Medium]
Since this adds dynamic detection through the detect_ctx callback, shouldn't
we also configure the connector polling flags in imx_tve_bind()?

It looks like the connector is initialized here:

imx_tve_bind() {
    ...
    ret = drm_connector_init_with_ddc(drm, connector,
                                      &imx_tve_connector_funcs,
                                      DRM_MODE_CONNECTOR_VGA, tve->ddc);
    ...
}

Without setting connector->polled = DRM_CONNECTOR_POLL_CONNECT |
DRM_CONNECTOR_POLL_DISCONNECT, the DRM core's periodic poll worker will
ignore this connector. Will this prevent dynamic detection if a user plugs
in a monitor after boot?

[Severity: High]
This is a pre-existing issue, but could probe ordering undermine this fix?

The patch relies on tve->ddc being initialized to accurately report the
connector status. However, imx_tve_probe() does not currently handle probe
deferral:

imx_tve_probe() {
    ...
    tve->ddc = of_find_i2c_adapter_by_node(ddc_node);
    of_node_put(ddc_node);
    if (tve->ddc) {
    ...
}

If the I2C driver hasn't probed yet, tve->ddc will be left as NULL instead
of returning -EPROBE_DEFER.

If detect_ctx is later called with a NULL DDC adapter,
drm_connector_helper_detect_from_ddc() will return connector_status_unknown.
Because the DRM core treats unknown as connected when fallback modes are
involved, does this mean the CRTC timeout bug will reoccur whenever the I2C
driver probes after imx-tve?

>  };
>  
>  static const struct drm_encoder_helper_funcs imx_tve_encoder_helper_funcs = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3