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
