Re: [PATCH RFC 4/4] drm/fl2000: Add KMS display pipeline

[Angel Gabriel Crispin Valdivia]

Hi Thomas,

thank you for the review. I have applied all of it for v2 and tried it
on the adapter. One change has a cost that you should know about before
I send it, and the answers to your questions are below.

> I assume that XRGB2101010 is the hardware's native or preferred format?

It is not. The vendor's API has RGB888, RGB565 and RGB555, and the mode
bits I know of in the chip's control register are for 565, 555 and 332.
I have no datasheet, but nothing I have seen has a 10-bit mode. The
driver accepted the 10-bit formats and reduced them to 8 bits, because
Mutter renders in them.

v2 offers XRGB8888, converted to RGB888, and RGB565 as it is. I saw
RGB565 work at 1280x720 on HDMI and at 1024x768 on VGA.

Dropping the 10-bit formats has a visible cost here. With an amdgpu
laptop, kernel 7.2.11 and Mutter 51.1:

- With XRGB8888 and RGB565 only, gnome-shell logs "Zero-copy disabled
for /dev/dri/card2, import failed: drmModeAddFB2 failed (Invalid
argument) [...] format=0x30335241 (AR30)". mem_info_gtt_used went from
96 MB to 6251 of 6939 MB in under a minute on VGA at 1280x720, and was
at 1922 MB two minutes after boot on HDMI at 1920x1080. On an earlier
version of the driver, the same message was followed by a desktop
stall after about 30 minutes ("bind failed" in
amdgpu_dm_plane_helper_prepare_fb).
- With XRGB2101010 and ARGB2101010 accepted, there is no such message,
and GTT stays between 107 and 134 MB over 45 minutes.

This is Mutter's: issue #5101 reports its copy path leaking a framebuffer
per frame on a DisplayLink output. Its main branch already releases that
framebuffer, since 15 October, but no release has it yet, as 51.1 was
tagged on 12 October. So I plan to send v2 without these formats, as you
asked, and GNOME users will have this until the next Mutter release. Tell
me if you see it differently.

> How large a value could you write into the registers?

The register definitions I inherited from Artem Mygaiev's driver have
12-bit active and total fields, so 4095, and v2 takes its limits from
them instead of the number I had made up. I don't have the manual, and
reading back does not confirm it: these registers return all 32 bits as
written.

> Once that is up, do you receive interrupts? If so, it would better go
> after drm_mode_config_reset().

Yes. v2 only starts polling the interrupt endpoint after
drm_dev_register(), which also lets the state you asked to drop go away.

> I'm not quite sure what this does. It looks like it's code for the
> encoder/connector.

It makes the chip believe that a monitor is connected to its analog
sense input, whatever the input says. The driver keeps it set while HDMI
is driven, and clear on VGA, where it would hide the cable being
unplugged. The vendor driver only sets it while programming the HDMI
transmitter, and I have not tried that yet. It is in the encoder in v2.

> I think it's better to resume mode setting first before resuming
> hardware. Otherwise the incoming interrupt might run on a suspended
> pipeline. (Not sure about the resume logic.)

v2 starts the interrupts after drm_mode_config_helper_resume(), so no
event runs on a suspended pipeline. The chips are still set up before
it: the adapter may have lost power, the register caches have to be
dropped, and restoring the state writes to those registers. Tell me if
you would rather see it done differently.

Best regards,
Angel