Message-ID: <47dfe81e-08ab-4fea-85fe-8c0a1d76bb78@ideasonboard.com>
Date: Wed, 8 Oct 2025 17:15:38 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Devarsh Thakkar <devarsht@...com>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Andrzej Hajda <andrzej.hajda@...el.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Jyri Sarha <jyri.sarha@....fi>
Subject: Re: [PATCH 00/29] drm: Implement state readout support

Hi,

On 08/10/2025 16:57, Maxime Ripard wrote:
> Hi Tomi,
> 
> Thanks for having a look.
> 
> On Wed, Oct 08, 2025 at 04:07:57PM +0300, Tomi Valkeinen wrote:
>> On 02/09/2025 11:32, Maxime Ripard wrote:
>>> Hi,
>>>
>>> Here's a series that implement what i915 calls "fastboot", ie,
>>> initializing the initial KMS state from the hardware state at boot, to
>>> skip the first modeset if the firmware already set up the display.
>>>
>>> This series creates the infrastructure in KMS to create that state by
>>> relying on driver specific hooks. It also implements some infrastructure
>>> to check during non-blocking commits that the readout helpers work
>>> properly by reading out the state that was just committed and comparing
>>> it to what was supposed to be commited.
>>>
>>> This relies on another set of driver hooks to compare the entities
>>> states, with helpers providing the default implementation.
>>>
>>> It then implements the readout support in the TIDSS driver, and was
>>> tested with the SK-AM62 board. This board in particular is pretty
>>> interesting, since it relies on an DPI to HDMI bridge, and uses the
>>> drm_bridge_connector infrastructure.
>>>
>>> So the readout works with the current state of the art on embedded-ish
>>> platforms.
>>>
>>> The whole thing feels a bit clunky at the moment:
>>>
>>>   - The initial state buildup ties everything together in a state in the
>>>     old state pointer. It's useful for the initial readout because
>>>     accessors can then use the usual state accessors to look into the
>>>     state of other entities. But one of the argument for it was also
>>>     that for state comparison, it allows to compare the new state
>>>     (committed) to the old state (readout). It doesn't really work in
>>>     practice, since in such a case the old state contains the previous
>>>     hardware state to be freed, and thus we would end up with a memory
>>>     leak
>>>
>>>   - The framebuffer refcounting is broken.
>>>
>>>   - The tidss atomic_flush waits for the go bit on the initial
>>>     modesetting, except that if the state is readout we didn't commit
>>>     anything and the driver will wait forever, eventually resulting in
>>>     commit timeout
>>
>> Isn't atomic flush part of the modeset? Why is it called if there's no
>> modeset.
> 
> No, atomic_flush is ran when we update the planes, so it will trigger
> here on the first page flip.
> 
>>>   - The tidss_crtc_state fields are not read properly at the moment
>>>     either.
>>
>> Just because no implemented, or was there something funny with them? I
>> guess there's some reverse-mapping that needs to be done.
> 
> The bus_format field isn't read properly, I wasn't quite sure what was
> going on there.
> 
> And also, for bridges, I've yet to figure out a way to read / find the
> input/output formats.
> 
>>> The main thing works though: the state is picked up properly, doesn't
>>> trigger a modeset if what was programmed is the one the first modeset
>>> tries to pick as well, will switch properly if it isn't, etc.
>>
>> This is pretty interesting work. I haven't tested, and I'm sure it still
>> breaks in a million ways if used with anything else but the HW you're
>> using =).
> 
> Thanks :D
> 
>> This is related to the boot-splash screen work I've been working on for
>> quite a while, although at a different stage.
> 
> As far as I'm concerned, once this lands, your work isn't needed at all.

No, I'm quite sure something alike my patches are needed. Without my
patch (drm/tidss: Add some support for splash-screen):

- the driver will do a hw reset at probe time, and you lose the splash
screen
- nothing has a reference to the related clocks, so they may be turned off

>> In my patches I have been trying to avoid hw reset, so that if the DSS
>> has been set up by the bootloader, we'll just let it run until we get
>> a modeset.
> 
> We really only need to power up the hardware around
> drm_mode_config_reset.

If by "power up" you mean set things up (it's powered up already by the
bootloader), yes. I think that's somewhat similar to what I did in my
patch wrt. handling the reset, but with your series we'll skip the reset
too.

> Once we're done, either the hardware will be active or inactive, but
> we'll know for sure.

We still need to have some early code to make sure we keep the clocks
enabled until the userspace does a modeset. Or would
drm_mode_config_reset be called at probe time?

>> And now your series would potentially remove that modeset too, so, in
>> theory, we could get up to X/Weston from bootloader with just a single
>> modeset in the bootloader.
>>
>> Of course, fbdev/simpledrm will mess things up there. I had some hacks
>> for fbdev too, to retain the bootsplash image, but it was just hacking.
> 
> And it works also with fbdev, since fbdev or whatever will trigger a
> new commit we can compare to.

Sorry, meant simplefb. There's a different problem there: if the
bootloader has set up a boot splash, simpledrm will allocate a new empty
framebuffer so we get a black screen, or in case of simplefb, it can be
made to use the existing memory buffer, but if fb console is enabled, it
will clear the buffer.

So maybe there won't be a modeset-related-temporary-blanking when using
your series, but the core issue of keeping a stable logo on the screen
up until X/Weston is, afaik, unsolved (unless you disable simplefb/drm
and thus also fbconsole).

 Tomi

Powered by blists - more mailing lists