Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] The DC8000 panel enable, CRTC begin, and CRTC flush operations use incorrect registers and bits, directly contradicting the commit message and breaking hardware functionality. - [Medium] The hw_irqs variable is passed to regmap_read() uninitialized and used without checking for read failures. Pre-existing issues: - [High] The DC8200 primary plane disable function erroneously sets the enable bit instead of clearing it, keeping the plane active. - [High] The IRQ handler unconditionally dereferences dc->drm_dev, but the interrupt is enabled before dc->drm_dev is allocated and remains active after it is freed. -- commit a518460571afb18a0b4ff8e14af894a66907ebde Author: Joey Lu <[email protected]> drm/verisilicon: add DC8000 (DCUltraLite) display controller support The Nuvoton MA35D1 SoC integrates a Verisilicon DCUltraLite display controller (DC8000 generation) whose register layout differs from the DC8200 in several important ways: > 1. No CONFIG_EX commit path: framebuffer updates use the enable (bit 0) > and reset (bit 4) bits in FB_CONFIG instead of the DC8200 staging > registers (FB_CONFIG_EX, FB_TOP_LEFT, FB_BOTTOM_RIGHT, > FB_BLEND_CONFIG, PANEL_CONFIG_EX). > > 2. No PANEL_START register: panel output starts when > PANEL_CONFIG.RUNNING is set; there is no multi-display sync start > register. [Severity: High] Do the functions implemented in this patch use the correct registers and bits as described in the commit message? In vs_dc8000_panel_enable_ex(), the code sets a reset bit (VSDC_FB_CONFIG_RESET) in VSDC_FB_CONFIG instead of writing to PANEL_CONFIG. Additionally, vs_dc8000_crtc_begin() and vs_dc8000_crtc_flush() use an unmentioned VALID bit (VSDC_FB_CONFIG_VALID), and clear it after updates complete, which seems to invert normal latch semantics. Could this mismatch prevent the display panel from enabling properly? > 3. Different IRQ registers: DCUltraLite uses DISP_IRQ_STA (0x147C) / > DISP_IRQ_EN (0x1480) versus DC8200's TOP_IRQ_ACK (0x0010) / > TOP_IRQ_EN (0x0014). > > 4. Simpler clock topology: only 'core' (bus gate) and 'pix0' (pixel > divider) clocks; no axi or ahb clocks required. > > Signed-off-by: Joey Lu <[email protected]> > diff --git a/drivers/gpu/drm/verisilicon/Makefile > b/drivers/gpu/drm/verisilicon/Makefile > index 9d4cd16452fa1..d2fd8e4dff242 100644 > --- a/drivers/gpu/drm/verisilicon/Makefile > +++ b/drivers/gpu/drm/verisilicon/Makefile > @@ -1,6 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0-only > > -verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_dc8200.o vs_drm.o > vs_hwdb.o \ > +verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_dc8200.o vs_dc8000.o > vs_drm.o vs_hwdb.o \ [Severity: High] This is a pre-existing issue, but while reviewing the driver files included here, I noticed a potential problem in vs_dc8200.c:vs_dc8200_primary_plane_disable_ex(). The function appears to call regmap_set_bits() on VSDC_FB_CONFIG_EX_FB_EN instead of regmap_clear_bits(): vs_dc8200.c:vs_dc8200_primary_plane_disable_ex() { ... regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), VSDC_FB_CONFIG_EX_FB_EN); ... } Does this erroneously keep the primary plane active when the DRM framework expects it to be turned off, potentially breaking power management? > vs_plane.o vs_primary_plane.o vs_cursor_plane.o > > obj-$(CONFIG_DRM_VERISILICON_DC) += verisilicon-dc.o [ ... ] > diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c > b/drivers/gpu/drm/verisilicon/vs_dc.c > index fd1f5fe67a68b..9499fffbca58e 100644 > --- a/drivers/gpu/drm/verisilicon/vs_dc.c > +++ b/drivers/gpu/drm/verisilicon/vs_dc.c > @@ -134,7 +134,10 @@ static int vs_dc_probe(struct platform_device *pdev) > dev_info(dev, "Found DC%x rev %x customer %x\n", dc->identity.model, > dc->identity.revision, dc->identity.customer_id); > > - dc->funcs = &vs_dc8200_funcs; > + if (dc->identity.generation == VSDC_GEN_DC8200) > + dc->funcs = &vs_dc8200_funcs; > + else > + dc->funcs = &vs_dc8000_funcs; [Severity: High] This is a pre-existing issue, but I noticed a potential lifecycle regression in vs_dc_probe() just below this block. The function calls devm_request_irq() to register vs_dc_irq_handler() before calling vs_drm_initialize(), which is responsible for allocating and assigning dc->drm_dev. If an interrupt fires during this window, for example if the bootloader left VSYNC interrupts enabled, the handler is invoked: vs_drm.c:vs_drm_handle_irq() { ... if (dc->drm_dev->crtcs[i]) drm_crtc_handle_vblank(&dc->drm_dev->crtcs[i]->base); ... } Will this unconditionally dereference dc->drm_dev and cause a NULL pointer dereference before vs_drm_initialize() has completed? > > if (port_count > dc->identity.display_count) { > dev_err(dev, "too many downstream ports than HW capability\n"); [ ... ] > diff --git a/drivers/gpu/drm/verisilicon/vs_dc8000.c > b/drivers/gpu/drm/verisilicon/vs_dc8000.c > new file mode 100644 > index 0000000000000..fbe0fa516cac9 > --- /dev/null > +++ b/drivers/gpu/drm/verisilicon/vs_dc8000.c > @@ -0,0 +1,86 @@ [ ... ] > +static u32 vs_dc8000_irq_ack(struct vs_dc *dc) > +{ > + u32 hw_irqs, unified = 0; > + unsigned int i; > + > + regmap_read(dc->regs, VSDC_DISP_IRQ_STA, &hw_irqs); [Severity: Medium] Is it safe to use hw_irqs here if regmap_read() fails? If regmap_read() returns an error code, hw_irqs will remain uninitialized stack memory. The function then proceeds to bitwise-AND this uninitialized value to determine which VSYNC interrupts fired: for (i = 0; i < VSDC_MAX_OUTPUTS; i++) { if (hw_irqs & VSDC_DISP_IRQ_VSYNC(i)) unified |= VSDC_IRQ_VSYNC(i); } Could this result in spurious VSYNC events being dispatched to the DRM core if a read failure occurs? > + > + for (i = 0; i < VSDC_MAX_OUTPUTS; i++) { > + if (hw_irqs & VSDC_DISP_IRQ_VSYNC(i)) > + unified |= VSDC_IRQ_VSYNC(i); > + } > + > + return unified; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
