[PATCH 08/43] drm/fbdev: Add fbdev-shmem

[Thomas Zimmermann]

Hi

Am 13.03.24 um 10:03 schrieb Geert Uytterhoeven:
> Hi Thomas,
>
> On Wed, Mar 13, 2024 at 9:19 AM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>> Am 12.03.24 um 17:14 schrieb Geert Uytterhoeven:
>>> On Tue, Mar 12, 2024 at 4:48 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>>>> Add an fbdev emulation for SHMEM-based memory managers. The code is
>>>> similar to fbdev-generic, but does not require an addition shadow
>>>> buffer for mmap(). Fbdev-shmem operates directly on the buffer object's
>>>> SHMEM pages. Fbdev's deferred-I/O mechanism updates the hardware state
>>>> on write operations.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>> Thanks for your patch!
>>>
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/drm_fbdev_shmem.c
>>>> +static int drm_fbdev_shmem_helper_fb_probe(struct drm_fb_helper *fb_helper,
>>>> +                                          struct drm_fb_helper_surface_size *sizes)
>>>> +{
>>>> +       struct drm_client_dev *client = &fb_helper->client;
>>>> +       struct drm_device *dev = fb_helper->dev;
>>>> +       struct drm_client_buffer *buffer;
>>>> +       struct drm_gem_shmem_object *shmem;
>>>> +       struct drm_framebuffer *fb;
>>>> +       struct fb_info *info;
>>>> +       u32 format;
>>>> +       struct iosys_map map;
>>>> +       int ret;
>>>> +
>>>> +       drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
>>>> +                   sizes->surface_width, sizes->surface_height,
>>>> +                   sizes->surface_bpp);
>>>> +
>>>> +       format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
>>> Oops, one more caller of the imprecise
>>> let's-guess-the-format-from-bpp-and-depth machinery to get rid of...
>> Right, that has been discussed in another thread. I'll change this call
>> to the drm_driver_() function.
> You mean drm_driver_legacy_fb_format()? That has the same issues.

We have the video= parameter with its bpp argument. So we won't ever 
fully remove that function.

>
>>>> +       buffer = drm_client_framebuffer_create(client, sizes->surface_width,
>>>> +                                              sizes->surface_height, format);
>>> [...]
>>>
>>>> +}
>>>> +/**
>>>> + * drm_fbdev_shmem_setup() - Setup fbdev emulation for GEM SHMEM helpers
>>>> + * @dev: DRM device
>>>> + * @preferred_bpp: Preferred bits per pixel for the device.
>>>> + *                 32 is used if this is zero.
>>>> + *
>>>> + * This function sets up fbdev emulation for GEM DMA drivers that support
>>>> + * dumb buffers with a virtual address and that can be mmap'ed.
>>>> + * drm_fbdev_shmem_setup() shall be called after the DRM driver registered
>>>> + * the new DRM device with drm_dev_register().
>>>> + *
>>>> + * Restore, hotplug events and teardown are all taken care of. Drivers that do
>>>> + * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
>>>> + * Simple drivers might use drm_mode_config_helper_suspend().
>>>> + *
>>>> + * This function is safe to call even when there are no connectors present.
>>>> + * Setup will be retried on the next hotplug event.
>>>> + *
>>>> + * The fbdev is destroyed by drm_dev_unregister().
>>>> + */
>>>> +void drm_fbdev_shmem_setup(struct drm_device *dev, unsigned int preferred_bpp)
>>> As this is a completely new function, can we please get a
>>> preferred_format parameter instead?
>> An understandable question. But as it is, the patchset has a trivial
>> change in each driver. And the preferred_bpp parameter has the same
>> meaning as the bpp value in the video= parameter. So it's ok-ish for now.
> OK.
>
>> Using a format parameter here is really a much larger update and touches
>> the internals of the fbdev emulation. I'm not even sure that we should
>> have a parameter at all. Since in-kernel clients should behave like
>> userspace clients, we could try to figure out the format from the
>> driver's primary planes. That's a patchset of its own.
> How do you figure out "the" format from the driver's primary plane?
> Isn't that a list of formats (always including XR24) , so the driver
> still needs to specify a preferred format?

The list of formats for each plane is roughly sorted by preference. We 
can go through it and pick the first format that is supported by the 
fbdev code. That's likely how userspace would do it.

>
> A while ago, I had a look into replacing preferred_{depth,bpp} by
> preferred_format, but I was held back by the inconsistencies in some
> drivers (e.g. depth 24 vs. 32).  Perhaps an incremental approach
> (use preferred_format if available, else fall back to legacy
> preferred_{depth,bpp} handling) would be more suitable?

I have initial patches to move format selection from the fb_probe 
helpers into the shared helpers. That allows to remove the surface_depth 
and surface_bpp fields. That is at least a step into the right direction.

>
> FTR, my main use-case is letting fbdev emulation distinguish between
> DRM_FORMAT_Rx and DRM_FORMAT_Cx, which share the values of depth
> and bpp.

How are they used for the framebuffer console? Shouldn't it always be 
_Cx? _Rx is just monochrome AFAIU.

Best regards
Thomas

>
> Gr{oetje,eeting}s,
>
>                          Geert
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)