Re: [PATCH v5 03/13] drm: client: add splash client

[Thomas Zimmermann]

Hi

Am 21.10.26 um 02:50 schrieb Màxim Pedraza Padilla:
> Hi Thomas,
>
> Thanks a lot for the thorough review. I was preparing v6 when it came
> in; I will hold it back and work all of this in first. Answers below.
>
> Am 20.10.26 um 12:28 schrieb Thomas Zimmermann:
> > This lengthy comment should be a list of per-revision changes, like this:
> >
> > v5:
> > - exit when firmware BMP is missing
> > - foo
> > - bla
> >
> > and so on. We keep these in the commit message right before the tags.
> Will do.
>
> > > +	select DRM_CLIENT_LIB if DRM_CLIENT_SPLASH
> > >    	select DRM_CLIENT_LIB if DRM_CLIENT_LOG
> > Alphabetic order please
> OK.
>
> > > +	  The color can be overridden through the drm_client_lib.splash_color
> > > +	  kernel command line parameter.
> > No extra module parameters please.
> [...]
>
> > As I said, no module parameter for colors.  The color is something that
> > the SoC firmware vendor would know at build-time. There is no reason for
> > the user to modify this at runtime.
> Sorry, I should have answered this the first time rather than keep
> the parameter without a word. Let me explain why I would like to keep
> the command line, for the colour and also for the parameters patch 12
> adds, and you tell me whether it holds.
>
> The build-time defaults cover what a product normally shows. The
> parameters are for what is only known at boot: the bootloader finds an
> update pending, a failed boot count or a recovery request, or reads the
> product variant from an EEPROM, and wants the splash to say so, with
> another logo or a red or amber background. Any bootloader can do that
> by adding to the command line: a setenv in a U-Boot script, a recovery
> entry in GRUB or systemd-boot. Changing the device tree instead needs
> fdt support in the bootloader and knowledge of the node, and is not
> possible at all when the device tree is signed along with the kernel
> in a FIT image. On x86 with the BGRT there is no device tree, and the
> command line is the only way.

I'd just like to say that these are not good uses of the splash screen.  
They also move the goal from having a boot-up logo to 
reporting/debugging boot-up problems.

> There is precedent for this kind of setting: video=...,rotate=,
> fbcon=rotate: and fbcon=logo-pos: are all about how the boot screen
> looks, and console= wins over stdout-path the way these win over the
> device tree.

video= has its uses, but the others seem misguided. Rotation should be 
auto-configured in some way; logo-pos shouldn't even be a thing IMHO.


> So my proposal for v6: the defaults are set at build time, as you ask
> (the colour already is, and the BMP's file name becomes a Kconfig
> string), and the parameters stay, only to override them for one boot.
> If that is still a no, I will drop them all, patch 12 included.

Please drop the parameters for now. If we want to give status messages 
from boot loader to kernel, we should first evaluate how to do that 
correctly. This involves people from the various supported 
architectures. Once we have support for such a channel, we can try to 
use it on any DRM client and build a shared solution.

I'm not against reporting boot-loader messages, but it seems to require 
a lot more thought that what can be done here in this series.


> > How could we find out about the bmp's file name? I assume it is just a
> > file in the initfs, which we have to know about?
> It is requested with request_firmware_nowait(), so it is looked up like
> any other firmware: built into the kernel with EXTRA_FIRMWARE, in the
> initramfs, or under /lib/firmware once the root file system is there.
> The name is fixed by whoever builds the image, so
>
> > In any case, this should again not be module parameter. Please make it
> > build-time configurable via Kconfig.
> a Kconfig string fits it well for the default name; v6 will have it.
> splash_bmp= would then only pick another file for one boot, such as
> one of several images built in with EXTRA_FIRMWARE, as above.
>
> > > +} __packed;
> > These types are already in efifb.c can we move them into a header file.
> > include/video/bmp.h should work.
> Yes, I'll add a patch that moves them there and makes efifb use the
> header. The header keeps the __le types, as BMP is little endian
> whatever the CPU, so efifb will read the fields through
> get_unaligned_le*() like the splash does; no change on the little
> endian systems EFI runs on. That patch touches fbdev, so it will go to
> Helge and Peter as well.

Makes sense.

> > > +	*data = fw->data;
> > > +	*size = fw->size;
> > Why does this work?
> >
> > We no longer hold a lock on fw. If a concurrent thread calls
> > release_firmware() on splash->fw, data and size might become invalid.
> The only release_firmware() on splash->fw is in
> drm_splash_image_cleanup(), which runs in the render thread itself
> once it has drawn, and at unregister after the thread has been
> stopped. The firmware callback stores the firmware under fw_lock, or
> releases it itself if the thread is already done, and never touches it
> again. So nobody can release it while the thread reads from it; the
> lock is only there for the hand-over from the callback. I'll put that
> in a comment, since it is not obvious from the code.

Thanks.

> > > +	drm_info(client->dev, "splash: request %s as firmware", splash_bmp);
> > drm_dbg() here. drm_info() is log spam.
> OK, here and in the other places you pointed out.
>
> > > +	int i;
> > format_count is an unsigned int. i should lik
> OK.


Oh, I just noticed that my comment got truncated. I meant to that "i 
should likely by unsigned int as well".


> > Values like px_width are deprecated because they don't work in the
> > general case.  Rather pass the format as drm_format_into and do the
> > px_width lookup within the drm_splash_fill().
> OK.

At some point I'd like to have a nice, shared drawing interface that all 
the clients can use easily.


> > > +			scolor = *(const u32 *)(&sbuf8[src_offset + 3 * x]);
> > Please build scolor from the individual entries in the color table. This
> > type casting is fragile. It also only works on little endian with
> > unaligned loads AFAIU.
> Agreed. It also read one byte past the end of the image on the last
> pixel. v6 already reads each pixel with get_unaligned_le24(), which
> builds it from the three bytes, in this patch rather than in a later
> one as in v5.
>
> > > +	bmp_width = (s32)get_unaligned_le32(&dib_header->width);
> > > +	bmp_height = (s32)get_unaligned_le32(&dib_header->height);
> > Why not make these variables unsigned?
> [...]
>
> > > +	bmp_invert_y = (bmp_height > 0);
> > > +	bmp_height = abs(bmp_height);
> > That's the answer to my question above, i guess?   But what about bmp_width?
> Yes. Both are signed in the BMP header, as in efifb's struct
> bmp_dib_header. A negative width has no meaning, and keeping it signed
> lets the format check reject it as such (bmp_width <= 0) rather than
> see a huge width that only the size checks further down would stop.
> I can read the width as unsigned and check it against S32_MAX instead
> if you prefer.

It's ok.

> > > +		 * On x86 some firmwares use a low non native resolution for
> > 'Some firmware on x86 uses a non-native resolution for'
> >
> > > +		 * the display when they have shown some text messages. While
> > 'the display if it has shown some text messages.'
> OK, thanks.
>
> > The entire if-else block below should be a function drm_splash_blit().
> > Please also see my comment on using px_width.
> OK. Together with your comment on patch 9, the blit will become a
> function from this patch on.
>
> > The client receives a hotplug whenever the attached display changes.
> > Returning here would likely break anything after the initial one.
> >
> > Best is to kill the rendering thread, rebuild the list of modesets and
> > reinit.
> Right, today an output plugged in later stays dark, and a restore puts
> back the modesets of the first hotplug. v6 will rebuild on every
> hotplug, keeping the image around so that an output plugged in later
> gets it too.

It's probably ok to address things in small steps. DRM's common client 
interfaces could be easier to use and getting it all right is fairly 
complicated, I think.

Best regards
Thomas


> > Why does it use a rendering threat?  Wouldn't it be much easier to
> > schedule a worker?  You can also try to put it on the power-efficient
> > work queue.
> The thread mostly waits: for the BMP firmware and for the next
> redraw. A work item queued from the hotplug and from the firmware
> callback covers both, and makes the start and stop paths simpler,
> which is where v3 had its use-after-frees. I'll move it to
> system_power_efficient_wq.
>
> > I don't think this is right. (?) If there's no fb, the restore should
> > disable the display. Just run the commit below.
> Fair enough. The early return was there because committing modesets
> that still had a mode but no framebuffer failed with -EINVAL and a
> WARN. v6 already clears the mode of an output that gets no buffer;
> I'll do the same when the buffers are freed, drop the early return,
> and let the commit turn the outputs off.
>
> > > +	mutex_destroy(&splash->hotplug_lock);
> > This line belongs into drm_splash_client_unregister(). The free callback
> > is only for free-ing the client instance.
> OK.
>
> > > +	drm_dbg(dev, "Unregistered with drm splash");
> > 'free', not 'unregister'
> OK.
>
> Best regards,
> Màxim

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)