Re: [PATCH v3 3/5] drm/ingenic: Add a driver for the X1600 display controller

[Olivier Louvignes]

Hi Paul,

On Fri, Nov 06, 2026 at 02:34:08PM +0100, Paul Cercueil wrote:
> > +	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>
> This bothers me a bit, shouldn't it be using 'crtc_state'?

Yes. v4 takes the mode from crtc_state.

> > +	writel(readl(lcd->base + X1600_LCD_ST), lcd->base +
> > X1600_LCD_CLR_ST);
>
> Not fond of the writel(readl()) nesting here.

OK, a local in v4, here and in probe.

> > +	writel(val, lcd->base + X1600_LCD_DISP_COM);
>
> This whole block above looks like out of place. Why not do it in
> x1600_lcd_encoder_atomic_mode_set()?
>
> Bonus point, you wouldn't need the 'bus_flags' and 'color_order' variables.

The controller only takes register writes while its clock runs, and the
clock is off when atomic_mode_set() is called: on a full modeset the
CRTC's atomic_disable() has stopped it, and atomic_enable() starts it
again afterwards. I checked it on the R1: with the clock gated (CLKGR0
bit 23), every register reads 0x80, and a write to TFT_HSYNC is lost,
the register still holds the old value once the clock is back.

So moving the block means starting the clock in atomic_mode_set() too,
or leaving it running while the CRTC is off. I'd rather keep the writes
in atomic_enable(), next to clk_prepare_enable(), and look the bus
format up in the new connector state there, so the two fields go
anyway. Would that work for you?

> > +	if (pixels > X1600_LCD_MAX_PIXELS)
> > +		return MODE_MEM;
>
> The max resolution is 1280x720 so you should check mode->hdisplay / mode-
> >vdisplay individually. Otherwise bogus resolutions like 1920x480 would be
> accepted.

Right for 1920x480, but 720 per side would refuse the R1's own panel,
which is 480x800 in portrait. The manual only says "frame size up to
720p". v4 can bound each side by 1280 and keep the 1280x720 pixel count,
would that do?

> > +		spin_lock(&lcd->drm.event_lock);
>
> spin_lock() can sleep with CONFIG_PREEMPT_RT so it looks wrong in an IRQ
> handler. I *think* you don't need to lock it here anyway, as the spinlock is
> locked in other places using spin_lock_irq().

On PREEMPT_RT the handler doesn't run in hard IRQ context:
force_irqthreads() is always true there, and the IRQ is requested
without IRQF_NO_THREAD, so the handler runs in a thread, where a
spinlock_t is fine (Documentation/locking/locktypes.rst).

And it needs the lock: drm_crtc_send_vblank_event() wants the caller to
hold event_lock, and lcd->event is shared with queue_event() and
atomic_disable(). Their spin_lock_irq() only keeps the handler off the
CPU they run on. armada, omapdrm and malidp take event_lock the same way
in their handlers. So I'd keep it.

> You enable it later in the probe, so you could just use devm_clk_get_enabled()
> here.

Probe only enables it to stop what the boot loader left running, and
disables it again before drm_dev_register(). From then on atomic_enable()
and atomic_disable() gate it. devm_clk_get_enabled() would keep it
running for as long as the driver is bound, display off included.

> I saw a patch from Maxime replacing these - you should be calling
> drm_mode_config_create_initial_state(drm) here (and handle the error).

Will do, it's in drm-misc-next already.

I'll leave v3 a few more days for the drm-misc side before sending v4.

Thanks for the review,
Olivier