[PATCH 3/9] drm: Add simplekms driver

[Thomas Zimmermann]

Hi

Am 29.06.20 um 11:06 schrieb Daniel Vetter:
> 
>> +					   ARRAY_SIZE(simplekms_formats),
>> +					   simplekms_format_modifiers,
>> +					   connector);
>> +	if (ret)
>> +		return ret;
>> +
>> +	drm_mode_config_reset(dev);
> 
> This breaks fastboot. I think ideally we'd have the state represent
> everything is on, and allocate an fb + buffer with the current contents of
> the framebuffer. Since we can allocate an fb that matches this shouldn't
> be a problem, just a raw memcpy_fromio should do the job.
> 
> Having a nice new simplekms drm driver and then losing fastboot feels like
> slightly off tradeoff.
> 
> Maybe in a follow-up patch, but before fbcon setup? Since ideally fbcon
> also takes over the already existing framebuffer we allocated, so that as
> long as nothing clears the fb (i.e. fbcon is quiet) we'd preserve the
> original framebuffer throughout the boot-up sequence.

I recently looked at how to implement this and it seems fairly complicated.

What we want it to adopt the current mode config into fbcon (and 
probably other in-kernel clients). The kernel client code uses it's own 
file instance to allocate the framebuffer objects against. So we cannot 
read-out the framebuffer state here. We'd ideally do this in the fbdev code.

I read through the proposal for read-out helpers. i915 seems to have 
lots of special cases. Can we adopt a simplified version that is just 
good enough to get the initial state for fbdev?

Best regards
Thomas

> 
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Init / Cleanup
>> + */
>> +
>> +static void simplekms_device_cleanup(struct simplekms_device* sdev)
>> +{
>> +	struct drm_device *dev = &sdev->dev;
>> +
>> +	drm_dev_unregister(dev);
> 
> I'd inline this, I guess there was once more before you switched
> everything over to devm_
> 
>> +}
>> +
>> +static struct simplekms_device *
>> +simplekms_device_create(struct drm_driver *drv, struct platform_device *pdev)
>> +{
>> +	struct simplekms_device *sdev;
>> +	int ret;
>> +
>> +	sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simplekms_device,
>> +				  dev);
>> +	if (IS_ERR(sdev))
>> +		return ERR_CAST(sdev);
>> +	sdev->pdev = pdev;
>> +
>> +	ret = simplekms_device_init_fb(sdev);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +	ret = simplekms_device_init_mm(sdev);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +	ret = simplekms_device_init_modeset(sdev);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	return sdev;
>> +}
>> +
>> +/*
>> + * DRM driver
>> + */
>> +
>> +DEFINE_DRM_GEM_FOPS(simplekms_fops);
>> +
>> +static struct drm_driver simplekms_driver = {
>> +	DRM_GEM_SHMEM_DRIVER_OPS,
>> +	.name			= DRIVER_NAME,
>> +	.desc			= DRIVER_DESC,
>> +	.date			= DRIVER_DATE,
>> +	.major			= DRIVER_MAJOR,
>> +	.minor			= DRIVER_MINOR,
>> +	.driver_features	= DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
>> +	.fops			= &simplekms_fops,
>> +};
>> +
>> +/*
>> + * Platform driver
>> + */
>> +
>> +static int simplekms_probe(struct platform_device *pdev)
>> +{
>> +	struct simplekms_device *sdev;
>> +	struct drm_device *dev;
>> +	int ret;
>> +
>> +	sdev = simplekms_device_create(&simplekms_driver, pdev);
>> +	if (IS_ERR(sdev))
>> +		return PTR_ERR(sdev);
>> +	dev = &sdev->dev;
>> +
>> +	ret = drm_dev_register(dev, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int simplekms_remove(struct platform_device *pdev)
>> +{
>> +	struct simplekms_device *sdev = platform_get_drvdata(pdev);
>> +
>> +	simplekms_device_cleanup(sdev);
> 
> If you add the ->disable hook then a comment here that we don't want to
> shut down to allow fastboot would be nice.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver simplekms_platform_driver = {
>> +	.driver = {
>> +		.name = "simple-framebuffer", /* connect to sysfb */
>> +	},
>> +	.probe = simplekms_probe,
>> +	.remove = simplekms_remove,
>> +};
>> +
>> +module_platform_driver(simplekms_platform_driver);
>> +
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.27.0
>>
> 
> Cheers, Daniel
> 

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20210210/182ec3d5/attachment-0001.sig>