From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=185.119.170.32 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1793977617; b=Y/UC9Xzyug02scilTNtMhGDA/2mpgbLtLDqLjM5ZqBRvqX/1Ss3LJJPE7HKkqogCosGw YHmKGOl76YrdKoqk2zDPyFlpgPKpQTwfPU/o3tyFPCieADSNsUtAoolxCw+m5c83zZ3L+ x1G5hVcWskCqloPiO9FoZnIkCOQNAWjVn1Ey87qMz0tRzTsJXFMxCsiLZW80D8vKtqJ7/ oqcwCObR+L/PQJmpkHy226hyJ/9kFeq8EqnzanW0TSXi4jpxXrStRt1bcAqeBfCyfzsi9 mWSg8Y3gtF6jT5aeN62fxiqDqdE6dK9qeEUlo+YEpl1qXq/wY3tYX6XgZRO+U5S4EXw== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1793977617; h=DMARC-Filter:Message-ID:Subject:From:To:Cc:Date:In-Reply-To: References:Content-Type:Content-Transfer-Encoding:MIME-Version; bh=gqJdpxgraavV9cayMwotUN97OO7P7jUb+2s38dDaeHo=; b=33eMxmeW6/Guszm/7g4kxeiYd49H1vIP91d068658HvekUQbclzjxSJYEDUDgjkQX2z5 3NfIvqMx535uZ8DhIrw7NHSYqu4DE1AATshUizfbTqmj5YLPCYrw3mjYHxE7yzK9yBRLH r3ZOXEQeyETwVPZGhmm8Y2n2w3lvHDZ5MloId8vuKg0FP5KsrNovwFcKwPBNtFNDLKWva OyuhdoARXmqF/WfmUn6O+AQvLPwc/ADsz7MUNvT0gTO7fiSj/mfeb89QSeKwsL+ptCeTH WUu3SGWOiqcN82XbxqlWuDh0YsKoVvj82WwQJe2L087AlojQSAAMRiM8pAhTXw/ENTw== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass header.from=crapouillou.net; spf=pass smtp.mailfrom=crapouillou.net; arc=none smtp.remote-ip=185.119.170.32 Received: from aposti.net (aposti.net [185.119.170.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DC864606A32C; Fri, 6 Nov 2026 15:06:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.2 smtp.subspace.kernel.org DC864606A32C Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=crapouillou.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=crapouillou.net Message-ID: <71aa432f71144f446dba2037ab5664eea3569fdc.camel@crapouillou.net> Subject: Re: [PATCH v3 3/5] drm/ingenic: Add a driver for the X1600 display controller From: Paul Cercueil To: Olivier Louvignes , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , Neil Armstrong Cc: David Airlie , Simona Vetter , Jessica Zhang , Rob Herring , Krzysztof Kozlowski , Conor Dooley , "H . Nikolaus Schaller" , Thomas Petazzoni , dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org Date: Fri, 06 Nov 2026 16:06:50 +0100 In-Reply-To: <20261106143827.15926-1-olivier@mgcrea.io> References: <20261106112000.1612-4-olivier@mgcrea.io> <0a2e8ea782fa819a547e0a3075303bd74a7fde72.camel@crapouillou.net> <20261106143827.15926-1-olivier@mgcrea.io> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 Le vendredi 06 novembre 2026 =C3=A0 15:38 +0100, Olivier Louvignes a =C3=A9= crit=C2=A0: > Hi Paul, >=20 > On Fri, Nov 06, 2026 at 02:34:08PM +0100, Paul Cercueil wrote: > > > + struct drm_display_mode *mode =3D &crtc->state->adjusted_mode; > >=20 > > This bothers me a bit, shouldn't it be using 'crtc_state'? >=20 > Yes. v4 takes the mode from crtc_state. >=20 > > > + writel(readl(lcd->base + X1600_LCD_ST), lcd->base + > > > X1600_LCD_CLR_ST); > >=20 > > Not fond of the writel(readl()) nesting here. >=20 > OK, a local in v4, here and in probe. >=20 > > > + writel(val, lcd->base + X1600_LCD_DISP_COM); > >=20 > > This whole block above looks like out of place. Why not do it in > > x1600_lcd_encoder_atomic_mode_set()? > >=20 > > Bonus point, you wouldn't need the 'bus_flags' and 'color_order' variab= les. >=20 > 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. >=20 > 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 en= abled always? It makes the driver more complex and I don't see a real benefit. >=20 > > > + if (pixels > X1600_LCD_MAX_PIXELS) > > > + return MODE_MEM; > >=20 > > The max resolution is 1280x720 so you should check mode->hdisplay / mod= e- > > > vdisplay individually. Otherwise bogus resolutions like 1920x480 woul= d be > > accepted. >=20 > 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); > >=20 > > spin_lock() can sleep with CONFIG_PREEMPT_RT so it looks wrong in an IR= Q > > handler. I *think* you don't need to lock it here anyway, as the spinlo= ck is > > locked in other places using spin_lock_irq(). >=20 > 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). >=20 > 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. >=20 > 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() t= o 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). >=20 > Will do, it's in drm-misc-next already. >=20 > I'll leave v3 a few more days for the drm-misc side before sending v4. Great. Cheers, -Paul