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

[Paul Cercueil]

Le vendredi 06 novembre 2026 à 15:38 +0100, Olivier Louvignes a écrit :
> 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?

Is there really a point in enabling the clock on-demand? Why not keep it enabled
always? It makes the driver more complex and I don't see a real benefit.

> 
> > > +	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?

Ok. Then I guess keep the current check.

> > > +		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.

Ok.

> > 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.

Yeah, I forgot that the devm action would cause a clk_disable_unprepare() to be
called on unprobe, which wouldn't work here. So ignore that.

As for the clock running for as long as the driver is bound, I don't think
that's much of a problem.

> > 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.

Great.

Cheers,
-Paul