Re: [PATCH 29/70] drm/tilcdc: Move hardware reset to CRTC creation

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

New issues:
- [High] Moving the hardware reset logic to `tilcdc_crtc_create()` causes an unconditional 500ms timeout and a potential fatal bus fault if the bootloader left the display enabled, because it waits for an interrupt before the IRQ handler is registered.

Pre-existing issues:
- [High] The hardware reset sequence unconditionally writes to a V2-only interrupt enable register, which fails to enable the framedone interrupt on V1 hardware and can cause register corruption.
--

--- Patch [29]: [PATCH 29/70] drm/tilcdc: Move hardware reset to CRTC creation ---
commit d53692ddb744ac620f653bb0b29134bf757b3e82
Author: Maxime Ripard <[email protected]>

drm/tilcdc: Move hardware reset to CRTC creation

The tilcdc crtc reset hook performs both atomic state initialization
and hardware-level register writes. This patch moves the hardware
reset sequence to the CRTC creation path since it is not related to
atomic state initialization.

[ ... ]

> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index e07ecb36a27a8..910580c4dada6 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c

[ ... ]

> @@ -1005,5 +977,24 @@ int tilcdc_crtc_create(struct drm_device *dev)
>  
>  	drm_crtc_helper_add(crtc, &tilcdc_crtc_helper_funcs);
>  
> +	/* Turn the raster off if it for some reason is on. */
> +	pm_runtime_get_sync(dev->dev);
> +	if (tilcdc_read(dev, LCDC_RASTER_CTRL_REG) & LCDC_RASTER_ENABLE) {
> +		/* Enable DMA Frame Done Interrupt */
> +		tilcdc_write(dev, LCDC_INT_ENABLE_SET_REG, LCDC_FRAME_DONE);

[Severity: High]
This is a pre-existing issue, but does this write corrupt V1 hardware?

The LCDC_INT_ENABLE_SET_REG is specific to V2 hardware. On V1 hardware,
writing to this register accesses an undefined register address.

For V1, shouldn't the framedone interrupt be enabled via
LCDC_V1_FRAME_DONE_INT_ENA within LCDC_RASTER_CTRL_REG instead? This
unconditional write might fail to enable the interrupt on V1 controllers,
leading to a guaranteed timeout in the wait_event_timeout() call below.

> +		tilcdc_clear_irqstatus(dev, 0xffffffff);
> +
> +		tilcdc_crtc->frame_done = false;
> +		tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
> +
> +		ret = wait_event_timeout(tilcdc_crtc->frame_done_wq,
> +					 tilcdc_crtc->frame_done,
> +					 msecs_to_jiffies(500));

[Severity: High]
Will this unconditionally time out during boot if the bootloader left the
display enabled?

In the driver initialization sequence, tilcdc_crtc_create() executes before
the IRQ handler is registered in tilcdc_pdev_probe():

tilcdc_pdev_probe()
  ...
  ret = tilcdc_crtc_create(ddev);
  ...
  ret = tilcdc_irq_install(ddev, priv->irq);

Since the IRQ handler isn't registered yet when this code runs,
wait_event_timeout() will wait for an interrupt that cannot be handled,
causing a 500ms boot delay.

Could this also cause a kernel panic once the IRQ is finally registered?

When the timeout finishes, pm_runtime_put_sync() disables the device clocks,
leaving the unhandled interrupt pending in hardware. When
tilcdc_irq_install() later requests the IRQ, the handler will fire
immediately and attempt to read the status register while the IP's clocks
are gated, leading to an imprecise external abort.

> +		if (ret == 0)
> +			drm_err(dev, "%s: timeout waiting for framedone\n",
> +				__func__);
> +	}
> +	pm_runtime_put_sync(dev->dev);
> +
>  	return 0;
>  }

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