Re: [PATCH 01/22] drm/fbdev-dma: Implement fbdev emulation for GEM DMA helpers

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 10104 bytes --]

Hi Javier,

thanks for your review.

Am 09.03.23 um 12:04 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> Implement fbdev emulation that is optimized for drivers that use
>> DMA helpers. The buffers may no tbe moveable, may not require damage
> 
> "may not be"
> 
> Is may the correct verb here though? I guess you meant "shall not".

I cannnot say for sure, but I always thought 'may not' is a nicer term 
for 'must not'. But RFC2119 disagrees wrt to the use of 'may'. I'll 
change the wording to 'must not'.

> 
>> handling and have to be located in system memory. This allows fbdev
>> emulation to operate directly on the buffer and mmap it to userspace.
>>
>> Besides those constraints, the emulation works like in the generic
>> code. As an internal DRM client provides, it receives hotplug, restore
>> and unregister events. The DRM client is independent from the fbdev
>> probing, which runs on the first successful hotplug event.
>>
>> The emulation is part of the DMA helper module and not build unless
>> DMA helpers and fbdev emulation has been configured.
>>
>> Tested with vc4.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>> +static int drm_fbdev_dma_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_dma_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;
>> +}
>> +
> 
> These two functions are the same than what's used by the generic fbdev
> emulation. Maybe they could be moved to drivers/gpu/drm/drm_fb_helper.c
> and be reused ?

I deliberately did not share code between the existing generic fbdev 
emulation and the new one. A number of drivers come with their own fbdev 
code and need conversion to struct drm_client. I want to see if there 
really is a pattern can can be shared in a helper.

I've done 'premature helperization' before and had to undo it later on. 
The existing fbdev code is an example of that. I'm trying to not do this 
mistake again.

> 
>> +static int drm_fbdev_dma_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_dma_object *dma_obj;
>> +	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);
>> +	dma_obj = to_drm_gem_dma_obj(buffer->gem);
>> +
>> +	fb = buffer->fb;
>> +	if (drm_WARN_ON(dev, fb->funcs->dirty)) {
>> +		ret = -ENODEV; /* damage handling not supported; use generic emulation */
>> +		goto err_drm_client_buffer_delete;
>> +	}
> 
> Should we have a similar check for drm_fbdev_use_shadow_fb(fb_helper)
> and warn on ?

That function (and several others) will go away soon. After the fbdev 
code for DMA helpers has been merged, generic fbdev will go shadow-fb-only.

> 
>> +
>> +	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 */
> 
> I also wonder if here and above instead of the warn on, there should
> just be a normal check and print more verbose warning messages.

No, because it's a driver bug that should be fixed ASAP. The driver 
should call generic fbdev instead. A regular warning would be 
appropriate for a runtime error over which the driver has no control, 
such as a OOM.

> 
> [...]
> 
>> +static void drm_fbdev_dma_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);
>> +	}
>> +}
> 
> This is again the same than drm_fbdev_client_unregister() so I think
> that can be made a helper and shared bewteen the two implementations.

I've have the same discussion with Patrik when I sent such an updte for 
gma500. These functions are the same, but I think this will change.

Here in _unregister(), the kfree() expects struct drm_fb_helper. But 
other drivers will certainly have their own structures and then require 
their own unregister helpers.

> 
>> +
>> +static int drm_fbdev_dma_client_restore(struct drm_client_dev *client)
>> +{
>> +	drm_fb_helper_lastclose(client->dev);
>> +
>> +	return 0;
>> +}
> 
> Same for this one.

Maybe more sharable, but there will be a version that supports 
vgaswitcheroo on several drivers.

> 
>> +
>> +static int drm_fbdev_dma_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-dma: Failed to setup generic emulation (ret=%d)\n", ret);
>> +	return ret;
>> +}
> 
> And this one.

I think, we should try to remove drm_fb_helper_funcs and therefore merge 
probe into hotplug. Each fbdev emulation will then require its own 
_hotplug() function. So this code won't be shareable in the future.

As I outlined before, I intentionally didn't share this code because I 
expect that it will be 'un-shared' in the near future.

> 
>> +/**
>> + * drm_fbdev_dma_setup() - Setup fbdev emulation for GEM DMA helpers
>> + * @dev: DRM device
>> + * @preferred_bpp: Preferred bits per pixel for the device.
>> + *                 @dev->mode_config.preferred_depth 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_dma_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_dma_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_dma_helper_funcs);
>> +
>> +	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_dma_client_funcs);
>> +	if (ret) {
>> +		drm_err(dev, "Failed to register client: %d\n", ret);
>> +		goto err_drm_client_init;
>> +	}
>> +
>> +	ret = drm_fbdev_dma_client_hotplug(&fb_helper->client);
>> +	if (ret)
>> +		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>> +
>> +	drm_client_register(&fb_helper->client);
>> +
>> +	return;
>> +
>> +err_drm_client_init:
>> +	drm_fb_helper_unprepare(fb_helper);
>> +	kfree(fb_helper);
>> +}
>> +EXPORT_SYMBOL(drm_fbdev_dma_setup);
> 
> And this one could also be shared AFAICT if drm_fbdev_client_hotplug()
> is used instead.
> 
>> diff --git a/include/drm/drm_fbdev_dma.h b/include/drm/drm_fbdev_dma.h
>> new file mode 100644
>> index 000000000000..2da7ee784133
>> --- /dev/null
>> +++ b/include/drm/drm_fbdev_dma.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: MIT */
>> +
>> +#ifndef DRM_FBDEV_DMA_H
>> +#define DRM_FBDEV_DMA_H
>> +
>> +struct drm_device;
>> +
>> +#ifdef CONFIG_DRM_FBDEV_EMULATION
>> +void drm_fbdev_dma_setup(struct drm_device *dev, unsigned int preferred_bpp);
>> +#else
>> +static inline void drm_fbdev_dma_setup(struct drm_device *dev, unsigned int preferred_bpp)
>> +{ }
>> +#endif
>> +
>> +#endif
>> -- 
> 
> And you should be able to drop this header too if split the common
> helpers from drm_fbdev_generic.c or maybe I'm missing something ?

This is the header that drivers include to run DMA fbdev emulation. We 
cannot remove it.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic