diff mbox series

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

Message ID 20240312154834.26178-9-tzimmermann@suse.de
State Superseded
Headers show
Series drm: Provide fbdev emulation per memory manager | expand

Commit Message

Thomas Zimmermann March 12, 2024, 3:45 p.m. UTC
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@suse.de>
---
 drivers/gpu/drm/Makefile          |   1 +
 drivers/gpu/drm/drm_fbdev_shmem.c | 316 ++++++++++++++++++++++++++++++
 include/drm/drm_fbdev_shmem.h     |  15 ++
 3 files changed, 332 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_fbdev_shmem.c
 create mode 100644 include/drm/drm_fbdev_shmem.h

Comments

Geert Uytterhoeven March 12, 2024, 4:14 p.m. UTC | #1
Hi Thomas,

On Tue, Mar 12, 2024 at 4:48 PM Thomas Zimmermann <tzimmermann@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@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...

> +       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?

> +{
> +       struct drm_fb_helper *fb_helper;
> +       int ret;
> +
> +       drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
> +       drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
> +
> +       fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
> +       if (!fb_helper)
> +               return;
> +       drm_fb_helper_prepare(dev, fb_helper, preferred_bpp, &drm_fbdev_shmem_helper_funcs);
> +
> +       ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_shmem_client_funcs);
> +       if (ret) {
> +               drm_err(dev, "Failed to register client: %d\n", ret);
> +               goto err_drm_client_init;
> +       }
> +
> +       drm_client_register(&fb_helper->client);
> +
> +       return;
> +
> +err_drm_client_init:
> +       drm_fb_helper_unprepare(fb_helper);
> +       kfree(fb_helper);
> +}
> +EXPORT_SYMBOL(drm_fbdev_shmem_setup);


Gr{oetje,eeting}s,

                        Geert
Thomas Zimmermann March 13, 2024, 8:19 a.m. UTC | #2
Hi Geert

Am 12.03.24 um 17:14 schrieb Geert Uytterhoeven:
> Hi Thomas,
>
> On Tue, Mar 12, 2024 at 4:48 PM Thomas Zimmermann <tzimmermann@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@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.

>
>> +       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.

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.

Best regards
Thomas

>
>> +{
>> +       struct drm_fb_helper *fb_helper;
>> +       int ret;
>> +
>> +       drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
>> +       drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
>> +
>> +       fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
>> +       if (!fb_helper)
>> +               return;
>> +       drm_fb_helper_prepare(dev, fb_helper, preferred_bpp, &drm_fbdev_shmem_helper_funcs);
>> +
>> +       ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_shmem_client_funcs);
>> +       if (ret) {
>> +               drm_err(dev, "Failed to register client: %d\n", ret);
>> +               goto err_drm_client_init;
>> +       }
>> +
>> +       drm_client_register(&fb_helper->client);
>> +
>> +       return;
>> +
>> +err_drm_client_init:
>> +       drm_fb_helper_unprepare(fb_helper);
>> +       kfree(fb_helper);
>> +}
>> +EXPORT_SYMBOL(drm_fbdev_shmem_setup);
>
> Gr{oetje,eeting}s,
>
>                          Geert
>
Geert Uytterhoeven March 13, 2024, 9:03 a.m. UTC | #3
Hi Thomas,

On Wed, Mar 13, 2024 at 9:19 AM Thomas Zimmermann <tzimmermann@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@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@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.

> >> +       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?

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?

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.

Gr{oetje,eeting}s,

                        Geert
Thomas Zimmermann March 13, 2024, 9:24 a.m. UTC | #4
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@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@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@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
>
Geert Uytterhoeven March 13, 2024, 9:56 a.m. UTC | #5
Hi Thomas,

On Wed, Mar 13, 2024 at 10:24 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 13.03.24 um 10:03 schrieb Geert Uytterhoeven:
> > On Wed, Mar 13, 2024 at 9:19 AM Thomas Zimmermann <tzimmermann@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@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@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.

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@linux-m68k.org/
[2] "PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1"
    https://lore.kernel.org/r/cover.1689252746.git.geert@linux-m68k.org/

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index a73c04d2d7a39..50a2f0cffdac2 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -107,6 +107,7 @@  drm_dma_helper-$(CONFIG_DRM_KMS_HELPER) += drm_fb_dma_helper.o
 obj-$(CONFIG_DRM_GEM_DMA_HELPER) += drm_dma_helper.o
 
 drm_shmem_helper-y := drm_gem_shmem_helper.o
+drm_shmem_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fbdev_shmem.o
 obj-$(CONFIG_DRM_GEM_SHMEM_HELPER) += drm_shmem_helper.o
 
 drm_suballoc_helper-y := drm_suballoc.o
diff --git a/drivers/gpu/drm/drm_fbdev_shmem.c b/drivers/gpu/drm/drm_fbdev_shmem.c
new file mode 100644
index 0000000000000..f568efcb198d6
--- /dev/null
+++ b/drivers/gpu/drm/drm_fbdev_shmem.c
@@ -0,0 +1,316 @@ 
+// SPDX-License-Identifier: MIT
+
+#include <linux/fb.h>
+
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+
+#include <drm/drm_fbdev_shmem.h>
+
+/*
+ * struct fb_ops
+ */
+
+static int drm_fbdev_shmem_fb_open(struct fb_info *info, int user)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+
+	/* No need to take a ref for fbcon because it unbinds on unregister */
+	if (user && !try_module_get(fb_helper->dev->driver->fops->owner))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int drm_fbdev_shmem_fb_release(struct fb_info *info, int user)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+
+	if (user)
+		module_put(fb_helper->dev->driver->fops->owner);
+
+	return 0;
+}
+
+FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(drm_fbdev_shmem,
+				   drm_fb_helper_damage_range,
+				   drm_fb_helper_damage_area);
+
+static int drm_fbdev_shmem_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+	struct drm_framebuffer *fb = fb_helper->fb;
+	struct drm_gem_object *obj = drm_gem_fb_get_obj(fb, 0);
+	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+
+	if (shmem->map_wc)
+		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+
+	return fb_deferred_io_mmap(info, vma);
+}
+
+static void drm_fbdev_shmem_fb_destroy(struct fb_info *info)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+
+	if (!fb_helper->dev)
+		return;
+
+	drm_fb_helper_fini(fb_helper);
+
+	drm_client_buffer_vunmap(fb_helper->buffer);
+	drm_client_framebuffer_delete(fb_helper->buffer);
+	drm_client_release(&fb_helper->client);
+	drm_fb_helper_unprepare(fb_helper);
+	kfree(fb_helper);
+}
+
+static const struct fb_ops drm_fbdev_shmem_fb_ops = {
+	.owner = THIS_MODULE,
+	.fb_open = drm_fbdev_shmem_fb_open,
+	.fb_release = drm_fbdev_shmem_fb_release,
+	__FB_DEFAULT_DEFERRED_OPS_RDWR(drm_fbdev_shmem),
+	DRM_FB_HELPER_DEFAULT_OPS,
+	__FB_DEFAULT_DEFERRED_OPS_DRAW(drm_fbdev_shmem),
+	.fb_mmap = drm_fbdev_shmem_fb_mmap,
+	.fb_destroy = drm_fbdev_shmem_fb_destroy,
+};
+
+static struct page *drm_fbdev_shmem_get_page(struct fb_info *info, unsigned long offset)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+	struct drm_framebuffer *fb = fb_helper->fb;
+	struct drm_gem_object *obj = drm_gem_fb_get_obj(fb, 0);
+	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+	unsigned int i = offset >> PAGE_SHIFT;
+	struct page *page;
+
+	if (fb_WARN_ON_ONCE(info, offset > obj->size))
+		return NULL;
+
+	page = shmem->pages[i]; // protected by active vmap
+	if (page)
+		get_page(page);
+	fb_WARN_ON_ONCE(info, !page);
+
+	return page;
+}
+
+/*
+ * struct drm_fb_helper
+ */
+
+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);
+	buffer = drm_client_framebuffer_create(client, sizes->surface_width,
+					       sizes->surface_height, format);
+	if (IS_ERR(buffer))
+		return PTR_ERR(buffer);
+	shmem = to_drm_gem_shmem_obj(buffer->gem);
+
+	fb = buffer->fb;
+
+	ret = drm_client_buffer_vmap(buffer, &map);
+	if (ret) {
+		goto err_drm_client_buffer_delete;
+	} else if (drm_WARN_ON(dev, map.is_iomem)) {
+		ret = -ENODEV; /* I/O memory not supported; use generic emulation */
+		goto err_drm_client_buffer_delete;
+	}
+
+	fb_helper->buffer = buffer;
+	fb_helper->fb = fb;
+
+	info = drm_fb_helper_alloc_info(fb_helper);
+	if (IS_ERR(info)) {
+		ret = PTR_ERR(info);
+		goto err_drm_client_buffer_vunmap;
+	}
+
+	drm_fb_helper_fill_info(info, fb_helper, sizes);
+
+	info->fbops = &drm_fbdev_shmem_fb_ops;
+
+	/* screen */
+	info->flags |= FBINFO_VIRTFB; /* system memory */
+	if (!shmem->map_wc)
+		info->flags |= FBINFO_READS_FAST; /* signal caching */
+	info->screen_size = sizes->surface_height * fb->pitches[0];
+	info->screen_buffer = map.vaddr;
+	info->fix.smem_len = info->screen_size;
+
+	/* deferred I/O */
+	fb_helper->fbdefio.delay = HZ / 20;
+	fb_helper->fbdefio.get_page = drm_fbdev_shmem_get_page;
+	fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
+
+	info->fbdefio = &fb_helper->fbdefio;
+	ret = fb_deferred_io_init(info);
+	if (ret)
+		goto err_drm_fb_helper_release_info;
+
+	return 0;
+
+err_drm_fb_helper_release_info:
+	drm_fb_helper_release_info(fb_helper);
+err_drm_client_buffer_vunmap:
+	fb_helper->fb = NULL;
+	fb_helper->buffer = NULL;
+	drm_client_buffer_vunmap(buffer);
+err_drm_client_buffer_delete:
+	drm_client_framebuffer_delete(buffer);
+	return ret;
+}
+
+static int drm_fbdev_shmem_helper_fb_dirty(struct drm_fb_helper *helper,
+					   struct drm_clip_rect *clip)
+{
+	struct drm_device *dev = helper->dev;
+	int ret;
+
+	/* Call damage handlers only if necessary */
+	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
+		return 0;
+
+	if (helper->fb->funcs->dirty) {
+		ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
+		if (drm_WARN_ONCE(dev, ret, "Dirty helper failed: ret=%d\n", ret))
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct drm_fb_helper_funcs drm_fbdev_shmem_helper_funcs = {
+	.fb_probe = drm_fbdev_shmem_helper_fb_probe,
+	.fb_dirty = drm_fbdev_shmem_helper_fb_dirty,
+};
+
+/*
+ * struct drm_client_funcs
+ */
+
+static void drm_fbdev_shmem_client_unregister(struct drm_client_dev *client)
+{
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+
+	if (fb_helper->info) {
+		drm_fb_helper_unregister_info(fb_helper);
+	} else {
+		drm_client_release(&fb_helper->client);
+		drm_fb_helper_unprepare(fb_helper);
+		kfree(fb_helper);
+	}
+}
+
+static int drm_fbdev_shmem_client_restore(struct drm_client_dev *client)
+{
+	drm_fb_helper_lastclose(client->dev);
+
+	return 0;
+}
+
+static int drm_fbdev_shmem_client_hotplug(struct drm_client_dev *client)
+{
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+	struct drm_device *dev = client->dev;
+	int ret;
+
+	if (dev->fb_helper)
+		return drm_fb_helper_hotplug_event(dev->fb_helper);
+
+	ret = drm_fb_helper_init(dev, fb_helper);
+	if (ret)
+		goto err_drm_err;
+
+	if (!drm_drv_uses_atomic_modeset(dev))
+		drm_helper_disable_unused_functions(dev);
+
+	ret = drm_fb_helper_initial_config(fb_helper);
+	if (ret)
+		goto err_drm_fb_helper_fini;
+
+	return 0;
+
+err_drm_fb_helper_fini:
+	drm_fb_helper_fini(fb_helper);
+err_drm_err:
+	drm_err(dev, "fbdev-shmem: Failed to setup emulation (ret=%d)\n", ret);
+	return ret;
+}
+
+static const struct drm_client_funcs drm_fbdev_shmem_client_funcs = {
+	.owner		= THIS_MODULE,
+	.unregister	= drm_fbdev_shmem_client_unregister,
+	.restore	= drm_fbdev_shmem_client_restore,
+	.hotplug	= drm_fbdev_shmem_client_hotplug,
+};
+
+/**
+ * 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)
+{
+	struct drm_fb_helper *fb_helper;
+	int ret;
+
+	drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
+	drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
+
+	fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
+	if (!fb_helper)
+		return;
+	drm_fb_helper_prepare(dev, fb_helper, preferred_bpp, &drm_fbdev_shmem_helper_funcs);
+
+	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_shmem_client_funcs);
+	if (ret) {
+		drm_err(dev, "Failed to register client: %d\n", ret);
+		goto err_drm_client_init;
+	}
+
+	drm_client_register(&fb_helper->client);
+
+	return;
+
+err_drm_client_init:
+	drm_fb_helper_unprepare(fb_helper);
+	kfree(fb_helper);
+}
+EXPORT_SYMBOL(drm_fbdev_shmem_setup);
diff --git a/include/drm/drm_fbdev_shmem.h b/include/drm/drm_fbdev_shmem.h
new file mode 100644
index 0000000000000..fb43cadd1950c
--- /dev/null
+++ b/include/drm/drm_fbdev_shmem.h
@@ -0,0 +1,15 @@ 
+/* SPDX-License-Identifier: MIT */
+
+#ifndef DRM_FBDEV_SHMEM_H
+#define DRM_FBDEV_SHMEM_H
+
+struct drm_device;
+
+#ifdef CONFIG_DRM_FBDEV_EMULATION
+void drm_fbdev_shmem_setup(struct drm_device *dev, unsigned int preferred_bpp);
+#else
+static inline void drm_fbdev_shmem_setup(struct drm_device *dev, unsigned int preferred_bpp)
+{ }
+#endif
+
+#endif