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 <[email protected]> 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/[email protected]?part=31
