Hi Thomas,
On Wed, Mar 13, 2024 at 10:24 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> Am 13.03.24 um 10:03 schrieb Geert Uytterhoeven:
> > On Wed, Mar 13, 2024 at 9:19 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> >> Am 12.03.24 um 17:14 schrieb Geert Uytterhoeven:
> >>> On Tue, Mar 12, 2024 at 4:48 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>
> >>> 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.
For that to work with monochrome and greyscale displays, it should
fall back to DRM_FORMAT_Rx (or _Dx) if depth <= 8 and DRM_FORMAT_Cx
is not supported by the underlying primary plane or driver.
Hmm, perhaps I should indeed implement the fallback in
drm_driver_legacy_fb_format() instead of drm_fb_helper_find_format()
(like I did in [1]).
> >>>> + 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.
OK.
> > 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.
Thanks, I am looking forward to that...
> > 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.
The fbdev console supports monochrome (including bold, underline, and
reverse video!) and grayscale text, too. I proposed adding support
for monochrome to the DRM fbdev emulation in [2]. Javier will be able
to make use of grayscale for the Solomon SSD132x controllers to improve
console performance.
[1] "[PATCH 7/8] drm/fb-helper: Add support for DRM_FORMAT_R1"
https://lore.kernel.org/r/ea0d68ef5630fe9748a11e50f6d79f79a768ebdb.1689252746.git.geert@xxxxxxxxxxxxxx/
[2] "PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1"
https://lore.kernel.org/r/cover.1689252746.git.geert@xxxxxxxxxxxxxx/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
