Re: [PATCH v8 34/39] drm/tidss: Implement hw_reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/tidss: dispc_reset_hw() wipes initial config and skips
  irq_mask setup
- [Medium] drm/tidss: unchecked tidss_runtime_get() in dispc_reset_hw()
--

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

drm/tidss: Implement hw_reset

This commit moves the DISPC hardware soft reset out of dispc_init() into a
drm_mode_config_funcs.hw_reset callback and reorders tidss_probe() so that
drm_mode_config_create_initial_state() runs before tidss_irq_install().

> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index b5fb296fddccc..8720403af9ed3 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2931,45 +2931,23 @@ 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;
> +	struct tidss_device *tidss = dispc->tidss;
>  	int ret;
>  
> -	ret = pm_runtime_set_active(dev);
> -	if (ret) {
> -		dev_err(dev, "Failed to set DSS PM to active\n");
> -		return ret;
> -	}
> -
> -	ret = clk_prepare_enable(dispc->fclk);
> -	if (ret) {
> -		dev_err(dev, "Failed to enable DSS fclk\n");
> -		goto err_runtime_suspend;
> -	}
> +	/*
> +	 * The reset runs from drm_mode_config_create_initial_state(), so
> +	 * after the driver has enabled runtime PM. As
> +	 * pm_runtime_set_active() is only allowed while runtime PM is
> +	 * disabled, rely on the regular runtime PM references to power
> +	 * the device up for the reset instead.
> +	 */
> +	tidss_runtime_get(tidss);
>  
>  	ret = dispc_softreset(dispc);
> -	if (ret)
> -		goto err_clk_disable;
>  
> -	clk_disable_unprepare(dispc->fclk);
> -	ret = pm_runtime_set_suspended(dev);
> -	if (ret) {
> -		dev_err(dev, "Failed to set DSS PM to suspended\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -
> -err_clk_disable:
> -	clk_disable_unprepare(dispc->fclk);
> -
> -err_runtime_suspend:
> -	ret = pm_runtime_set_suspended(dev);
> -	if (ret) {
> -		dev_err(dev, "Failed to set DSS PM to suspended\n");
> -		return ret;
> -	}
> +	tidss_runtime_put(tidss);
>  
>  	return ret;
>  }

[Severity: Medium]
What happens if tidss_runtime_get() fails here?

tidss_runtime_get() wraps pm_runtime_resume_and_get(), which drops its usage
reference on error and leaves the power domain and dispc->fclk disabled.
Continuing on failure accesses unclocked DISPC registers in
dispc_softreset() and then underflows the runtime PM usage count in
tidss_runtime_put().

[ ... ]

> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index e258f656cf03d..409d599bb2ffd 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -151,6 +151,20 @@ static int tidss_probe(struct platform_device *pdev)
>  		goto err_runtime_suspend;
>  	}
>  
> +	drm_kms_helper_poll_init(ddev);
> +
> +	/*
> +	 * drm_mode_config_create_initial_state() resets the DISPC when
> +	 * the state readout isn't possible, which also clears the
> +	 * interrupt enable registers. Install the interrupts only once
> +	 * that's been done, so that the reset can't wipe them.
> +	 */
> +	ret = drm_mode_config_create_initial_state(ddev);
> +	if (ret) {
> +		dev_err(dev, "failed to create initial state: %d\n", ret);
> +		goto err_runtime_suspend;
> +	}
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
>  		ret = irq;
> @@ -164,14 +178,6 @@ static int tidss_probe(struct platform_device *pdev)
>  		goto err_runtime_suspend;
>  	}
>  
> -	drm_kms_helper_poll_init(ddev);
> -
> -	ret = drm_mode_config_create_initial_state(ddev);
> -	if (ret) {
> -		dev_err(dev, "failed to create initial state: %d\n", ret);
> -		goto err_irq_uninstall;
> -	}
> -
>  	ret = drm_dev_register(ddev, 0);

[Severity: High]
Does running dispc_reset_hw() via runtime PM and before tidss_irq_install()
wipe the initial DISPC configuration and leave tidss->irq_mask unprogrammed
in hardware?

drivers/gpu/drm/tidss/tidss_dispc.c:dispc_runtime_resume() {
    ...
	dispc_initial_config(dispc);

	dispc->is_enabled = true;

	tidss_irq_resume(dispc->tidss);
    ...
}

Previously, dispc_init_hw() ran before pm_runtime_enable() using
pm_runtime_set_active() and clk_prepare_enable() so that the soft reset
completed before dispc_runtime_resume() ever ran (as noted in
bc288a927815 ("drm/tidss: Fix dss reset")).  Now, tidss_runtime_get() in
dispc_reset_hw() triggers dispc_runtime_resume() first, and
dispc_softreset() immediately resets all DISPC registers back to their
defaults, wiping the plane thresholds, preload values, gamma shadow bits,
and J721E DISPC_CONNECTIONS programmed by dispc_initial_config().

In addition, tidss_irq_install() only populates tidss->irq_mask in memory
and relies on tidss_irq_resume() during dispc_runtime_resume() to program
the hardware IRQ enable registers.  Because dispc_runtime_resume() now runs
during drm_mode_config_create_initial_state() while tidss->irq_mask is still
0, and tidss_runtime_put() uses a 1000 ms autosuspend delay (or never
suspends when CONFIG_PM is disabled), the device stays runtime-active across
tidss_irq_install() and the initial modeset in drm_client_setup(), leaving
the hardware unconfigured and frame-done/underflow interrupts disabled in
hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-drm-state-readout-v8-0-cbe78624e1bc@kernel.org?part=34