Re: [PATCH v7 31/34] drm/tidss: Implement hw_reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/tidss: moving dispc_reset_hw() to tidss_hw_reset() breaks hardware reset
- [High] drm/tidss: dispc_reset_hw() never called on probe and fails when PM enabled
--

commit b4ac51170032bf695c274ab86a54a2de58078be4
Author: Maxime Ripard <mripard@kernel.org>

drm/tidss: Implement hw_reset

This commit renames dispc_init_hw() to dispc_reset_hw(), removes its
unconditional call from dispc_init(), and wires it up to
drm_mode_config_funcs.hw_reset via tidss_hw_reset().

> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 5943a5f254321..fd33edd809770 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2909,7 +2909,7 @@ static int dispc_softreset(struct dispc_device *dispc)
>  	return 0;
>  }
>  
> -static int dispc_init_hw(struct dispc_device *dispc)
> +int dispc_reset_hw(struct dispc_device *dispc)
>  {
>  	struct device *dev = dispc->dev;
>  	int ret;
> @@ -3059,10 +3059,6 @@ int dispc_init(struct tidss_device *tidss)
>  	of_property_read_u32(dispc->dev->of_node, "max-memory-bandwidth",
>  			     &dispc->memory_bandwidth_limit);
>  
> -	r = dispc_init_hw(dispc);
> -	if (r)
> -		return r;
> -
>  	tidss->dispc = dispc;
>  
>  	return 0;

[Severity: High]
Does removing dispc_init_hw() from dispc_init() leave the DISPC hardware
unreset during probe and before tidss_irq_install() registers the interrupt
handler?

In drivers/gpu/drm/tidss/tidss_drv.c, tidss_probe() calls
drm_mode_config_reset(ddev) rather than
drm_mode_config_create_initial_state(ddev):

drivers/gpu/drm/tidss/tidss_drv.c:tidss_probe() {
	...
	ret = tidss_irq_install(ddev, irq);
	if (ret) {
		dev_err(dev, "tidss_irq_install failed: %d\n", ret);
		goto err_runtime_suspend;
	}

	drm_kms_helper_poll_init(ddev);

	drm_mode_config_reset(ddev);
	...
}

Because drm_mode_config_reset() in drivers/gpu/drm/drm_mode_config.c only
invokes per-CRTC helper_funcs->hw_reset and does not call
dev->mode_config.funcs->hw_reset(dev) (which is only called by
drm_mode_config_create_initial_state()), tidss_hw_reset() and
dispc_reset_hw() are never called during probe. This also leaves any active
bootloader display or interrupt state unmasked when tidss_irq_install()
requests the IRQ line.

[ ... ]

> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index 70c14c3be10d5..3b97c08c4f714 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -116,10 +117,23 @@ static int tidss_atomic_check(struct drm_device *ddev,
>  	return 0;
>  }
>  
> +static void tidss_hw_reset(struct drm_device *dev)
> +{
> +	struct tidss_device *tidss = to_tidss(dev);
> +	int ret;
> +
> +	ret = dispc_reset_hw(tidss->dispc);
> +	if (ret)
> +		drm_warn(dev, "Couldn't reset dispc");
> +
> +	drm_mode_config_helper_hw_reset(dev);
> +}
> +
>  static const struct drm_mode_config_funcs mode_config_funcs = {
>  	.fb_create = drm_gem_fb_create,
>  	.atomic_check = tidss_atomic_check,
>  	.atomic_commit = drm_atomic_helper_commit,
> +	.hw_reset = tidss_hw_reset,
>  };

[Severity: High]
When tidss_hw_reset() is called during mode_config initialization (such as via
drm_mode_config_create_initial_state()), won't dispc_reset_hw() fail with
-EAGAIN when CONFIG_PM is enabled, or wipe out programmed register state when
CONFIG_PM is disabled?

In drivers/gpu/drm/tidss/tidss_drv.c, tidss_probe() enables runtime PM (or
manually calls dispc_runtime_resume() when !CONFIG_PM) before modeset
initialization and reset:

drivers/gpu/drm/tidss/tidss_drv.c:tidss_probe() {
	...
	pm_runtime_enable(dev);

	pm_runtime_set_autosuspend_delay(dev, 1000);
	pm_runtime_use_autosuspend(dev);

#ifndef CONFIG_PM
	/* If we don't have PM, we need to call resume manually */
	dispc_runtime_resume(tidss->dispc);
#endif

	ret = tidss_modeset_init(tidss);
	...
}

However, dispc_reset_hw() uses pm_runtime_set_active() and
pm_runtime_set_suspended(), which only work while runtime PM is disabled:

drivers/gpu/drm/tidss/tidss_dispc.c:dispc_reset_hw() {
	struct device *dev = dispc->dev;
	int ret;

	ret = pm_runtime_set_active(dev);
	if (ret) {
		dev_err(dev, "Failed to set DSS PM to active\n");
		return ret;
	}
	...
}

Once pm_runtime_enable(dev) has been called (so dev->power.disable_depth == 0
and dev->power.runtime_error == 0), __pm_runtime_set_status() in
drivers/base/power/runtime.c rejects pm_runtime_set_active(dev) with -EAGAIN:

drivers/base/power/runtime.c:__pm_runtime_set_status() {
	...
	if (dev->power.runtime_error || dev->power.disable_depth)
		dev->power.disable_depth++;
	else
		error = -EAGAIN;
	...
}

As a result, under CONFIG_PM=y, dispc_reset_hw() aborts immediately before
enabling dispc->fclk or calling dispc_softreset(). Conversely, under
CONFIG_PM=n, dispc_runtime_resume() and tidss_irq_install() have already
configured the initial registers and interrupt masks before tidss_hw_reset()
runs, so executing dispc_softreset() at that point resets DSS_SYSCONFIG and
wipes out the programmed register and IRQ state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-drm-state-readout-v7-0-25e5163d75c3@kernel.org?part=31