Re: [PATCH] drm/vboxvideo: Move vboxvideo driver out of staging

[Sam Ravnborg <sam@ravnborg.org>]

Hi Hans.

Just a bunch of random observations that I hope you find use of.

	Sam

> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index cb88528e7b10..6b4d6c957da8 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -315,6 +315,8 @@ source "drivers/gpu/drm/tve200/Kconfig"
>  
>  source "drivers/gpu/drm/xen/Kconfig"
>  
> +source "drivers/gpu/drm/vboxvideo/Kconfig"
> +
>  # Keep legacy drivers last
>  
>  menuconfig DRM_LEGACY
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index a6771cef85e2..133606802300 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -107,3 +107,4 @@ obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
>  obj-$(CONFIG_DRM_PL111) += pl111/
>  obj-$(CONFIG_DRM_TVE200) += tve200/
>  obj-$(CONFIG_DRM_XEN) += xen/
> +obj-$(CONFIG_DRM_VBOXVIDEO) += vboxvideo/

Is it the most logical place to add this in the end?
You are asking for merge problems if you do so, but then
the drm drivers does not seem to follow any good order
so this may be fine.

> +++ b/drivers/gpu/drm/vboxvideo/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +ccflags-y := -Iinclude/drm
This should not be required.
If required then fix the offending #include

> +
> +vboxvideo-y :=  hgsmi_base.o modesetting.o vbva_base.o \
> +		vbox_drv.o vbox_fb.o vbox_hgsmi.o vbox_irq.o vbox_main.o \
> +		vbox_mode.o vbox_prime.o vbox_ttm.o
> +
> +obj-$(CONFIG_DRM_VBOXVIDEO) += vboxvideo.o
> diff --git a/drivers/gpu/drm/vboxvideo/hgsmi_base.c b/drivers/gpu/drm/vboxvideo/hgsmi_base.c
> new file mode 100644
> index 000000000000..361d3193258e
> --- /dev/null
> +++ b/drivers/gpu/drm/vboxvideo/hgsmi_base.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: MIT
> +/* Copyright (C) 2006-2017 Oracle Corporation */
2018?
General comment for all files.

> +	union {
> +		/* Opaque placeholder to make the union 8 bytes. */
> +		u8 header_data[8];
Further down you have 2 x u32, so the union is guaranteed to be 64 bits.
So header_data is redundant.
> +
> +		/* HGSMI_BUFFER_HEADER_F_SEQ_SINGLE */
> +		struct {
> +			u32 reserved1;	/* A reserved field, initialize to 0. */
> +			u32 reserved2;	/* A reserved field, initialize to 0. */
> +		} buffer;



> --- /dev/null
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (C) 2013-2017 Oracle Corporation
> + * This file is based on ast_drv.c
> + * Copyright 2012 Red Hat Inc.
> + * Authors: Dave Airlie <airlied@redhat.com>
> + *          Michael Thayer <michael.thayer@oracle.com,
> + *          Hans de Goede <hdegoede@redhat.com>
> + */
> +#include <linux/module.h>
> +#include <linux/console.h>
> +#include <linux/vt_kern.h>
> +
> +#include <drm/drmP.h>
We are trying to get rid of drmP - replace with more specific includes.
Goes for all uses of drmP.h


> +
> +static int vbox_modeset = -1;
> +
> +MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
> +module_param_named(modeset, vbox_modeset, int, 0400);
> +
> +static struct drm_driver driver;
> +
> +static const struct pci_device_id pciidlist[] = {
> +	{ 0x80ee, 0xbeef, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
> +	{ 0, 0, 0},
> +};
Use PCI_DEVICE()?


> +static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> +	struct vbox_private *vbox;
> +	int ret = 0;
> +
> +	if (!vbox_check_supported(VBE_DISPI_ID_HGSMI))
> +		return -ENODEV;
> +
> +	vbox = kzalloc(sizeof(*vbox), GFP_KERNEL);
> +	if (!vbox)
> +		return -ENOMEM;
> +
> +	ret = drm_dev_init(&vbox->ddev, &driver, &pdev->dev);
> +	if (ret) {
> +		kfree(vbox);
> +		return ret;
> +	}
> +
> +	vbox->ddev.pdev = pdev;
> +	vbox->ddev.dev_private = vbox;
> +	pci_set_drvdata(pdev, vbox);
> +	mutex_init(&vbox->hw_mutex);
> +
> +	ret = pci_enable_device(pdev);
> +	if (ret)
> +		goto err_dev_put;
> +
> +	ret = vbox_hw_init(vbox);
> +	if (ret)
> +		goto err_pci_disable;
> +
> +	ret = vbox_mm_init(vbox);
> +	if (ret)
> +		goto err_hw_fini;
> +
> +	ret = vbox_mode_init(vbox);
> +	if (ret)
> +		goto err_mm_fini;
> +
> +	ret = vbox_irq_init(vbox);
> +	if (ret)
> +		goto err_mode_fini;
> +
> +	ret = drm_fb_helper_fbdev_setup(&vbox->ddev, &vbox->fb_helper,
> +					&vbox_fb_helper_funcs, 32,
> +					vbox->num_crtcs);
> +	if (ret)
> +		goto err_irq_fini;
> +
> +	ret = drm_dev_register(&vbox->ddev, 0);
> +	if (ret)
> +		goto err_fbdev_fini;
> +
> +	return 0;
> +
> +err_fbdev_fini:
> +	vbox_fbdev_fini(vbox);
> +err_irq_fini:
> +	vbox_irq_fini(vbox);
> +err_mode_fini:
> +	vbox_mode_fini(vbox);
> +err_mm_fini:
> +	vbox_mm_fini(vbox);
> +err_hw_fini:
> +	vbox_hw_fini(vbox);
> +err_pci_disable:
> +	pci_disable_device(pdev);
> +err_dev_put:
> +	drm_dev_put(&vbox->ddev);

kfree(vbox) missing?

> +	return ret;
> +}
> +
> +
> +static int vbox_pm_suspend(struct device *dev)
> +{
> +	struct vbox_private *vbox = dev_get_drvdata(dev);
> +	int error;
> +
> +	error = drm_mode_config_helper_suspend(&vbox->ddev);
> +	if (error)
> +		return error;
> +
> +	pci_save_state(vbox->ddev.pdev);
> +	pci_disable_device(vbox->ddev.pdev);
> +	pci_set_power_state(vbox->ddev.pdev, PCI_D3hot);
> +
> +	return 0;
> +}
> +
> +static int vbox_pm_resume(struct device *dev)
> +{
> +	struct vbox_private *vbox = dev_get_drvdata(dev);
> +
> +	if (pci_enable_device(vbox->ddev.pdev))
> +		return -EIO;
> +
> +	return drm_mode_config_helper_resume(&vbox->ddev);
> +}
> +
> +static int vbox_pm_freeze(struct device *dev)
> +{
> +	struct vbox_private *vbox = dev_get_drvdata(dev);
> +
> +	return drm_mode_config_helper_suspend(&vbox->ddev);
> +}
> +
> +static int vbox_pm_thaw(struct device *dev)
> +{
> +	struct vbox_private *vbox = dev_get_drvdata(dev);
> +
> +	return drm_mode_config_helper_resume(&vbox->ddev);
> +}
> +
> +static int vbox_pm_poweroff(struct device *dev)
> +{
> +	struct vbox_private *vbox = dev_get_drvdata(dev);
> +
> +	return drm_mode_config_helper_suspend(&vbox->ddev);
> +}
> +
> +static const struct dev_pm_ops vbox_pm_ops = {
> +	.suspend = vbox_pm_suspend,
> +	.resume = vbox_pm_resume,
> +	.freeze = vbox_pm_freeze,
> +	.thaw = vbox_pm_thaw,
> +	.poweroff = vbox_pm_poweroff,
> +	.restore = vbox_pm_resume,
> +};
Should all this be inside #ifdef CONFIG_PM_SLEEP?

> +
> +static struct pci_driver vbox_pci_driver = {
> +	.name = DRIVER_NAME,
> +	.id_table = pciidlist,
> +	.probe = vbox_pci_probe,
> +	.remove = vbox_pci_remove,
> +	.driver.pm = &vbox_pm_ops,
> +};
> +
> +static const struct file_operations vbox_fops = {
> +	.owner = THIS_MODULE,
> +	.open = drm_open,
> +	.release = drm_release,
> +	.unlocked_ioctl = drm_ioctl,
> +	.mmap = vbox_mmap,
> +	.poll = drm_poll,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl = drm_compat_ioctl,
> +#endif

The ifdef is not needed. See drm_ioctl.h


> +
> +MODULE_AUTHOR("Oracle Corporation");
Add yourself?

> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("GPL and additional rights");
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.h b/drivers/gpu/drm/vboxvideo/vbox_drv.h


> +	/* Must be first; or we must define our own release callback */
> +	struct drm_device ddev;
> +	struct drm_fb_helper fb_helper;
> +	struct vbox_framebuffer afb;
> +
> +	u8 __iomem *guest_heap;
> +	u8 __iomem *vbva_buffers;
> +	struct gen_pool *guest_pool;
> +	struct vbva_buf_ctx *vbva_info;
> +	bool any_pitch;
> +	u32 num_crtcs;
> +	/* Amount of available VRAM, including space used for buffers. */
> +	u32 full_vram_size;
> +	/* Amount of available VRAM, not including space used for buffers. */
> +	u32 available_vram_size;
> +	/* Array of structures for receiving mode hints. */
> +	struct vbva_modehint *last_mode_hints;
> +
> +	int fb_mtrr;

This member is only used to:
vbox->fb_mtrr = arch_phys_wc_add(pci_resource_start(dev->pdev, 0),
+                                        pci_resource_len(dev->pdev, 0));

and later
arch_phys_wc_del(vbox->fb_mtrr);

And is not used for anything else.
Looks like some left-over that can be dropped.



> +
> +	struct {
> +		struct drm_global_reference mem_global_ref;
> +		struct ttm_bo_global_ref bo_global_ref;
> +		struct ttm_bo_device bdev;
> +	} ttm;
> +
> +	struct mutex hw_mutex; /* protects modeset and accel/vbva accesses */
> +	/*
> +	 * We decide whether or not user-space supports display hot-plug
> +	 * depending on whether they react to a hot-plug event after the initial
> +	 * mode query.
> +	 */
> +	bool initial_mode_queried;
> +	struct work_struct hotplug_work;
> +	u32 input_mapping_width;
> +	u32 input_mapping_height;
> +	/*
> +	 * Is user-space using an X.Org-style layout of one large frame-buffer
> +	 * encompassing all screen ones or is the fbdev console active?
> +	 */
> +	bool single_framebuffer;
> +	u8 cursor_data[CURSOR_DATA_SIZE];
> +};
> +
> +#undef CURSOR_PIXEL_COUNT
> +#undef CURSOR_DATA_SIZE
Why these and not the others?

> +	 */
> +	u32 width;
> +	u32 height;
> +	u32 x;
> +	u32 y;
> +};
> +
> +struct vbox_encoder {
> +	struct drm_encoder base;
> +};
> +
> +#define to_vbox_crtc(x) container_of(x, struct vbox_crtc, base)
> +#define to_vbox_connector(x) container_of(x, struct vbox_connector, base)
> +#define to_vbox_encoder(x) container_of(x, struct vbox_encoder, base)
> +#define to_vbox_framebuffer(x) container_of(x, struct vbox_framebuffer, base)
> +
> +bool vbox_check_supported(u16 id);
> +int vbox_hw_init(struct vbox_private *vbox);
> +void vbox_hw_fini(struct vbox_private *vbox);
> +
> +int vbox_mode_init(struct vbox_private *vbox);
> +void vbox_mode_fini(struct vbox_private *vbox);
> +
> +#define DRM_MODE_FB_CMD drm_mode_fb_cmd2

This define looks like letfover from something old.
Drop it and spell out the type.

> diff --git a/drivers/gpu/drm/vboxvideo/vbox_fb.c b/drivers/gpu/drm/vboxvideo/vbox_fb.c
> new file mode 100644
> index 000000000000..8041d0c46a6b
> --- /dev/null
> +++ b/drivers/gpu/drm/vboxvideo/vbox_fb.c
> @@ -0,0 +1,166 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (C) 2013-2017 Oracle Corporation
> + * This file is based on ast_fb.c
> + * Copyright 2012 Red Hat Inc.
> + * Authors: Dave Airlie <airlied@redhat.com>
> + *          Michael Thayer <michael.thayer@oracle.com,
> + */
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/mm.h>
> +#include <linux/tty.h>
> +#include <linux/sysrq.h>
> +#include <linux/delay.h>
> +#include <linux/fb.h>
> +#include <linux/init.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +
> +#include "vbox_drv.h"
> +#include "vboxvideo.h"
> +
> +#ifdef CONFIG_DRM_KMS_FB_HELPER
> +static struct fb_deferred_io vbox_defio = {
> +	.delay = HZ / 30,
> +	.deferred_io = drm_fb_helper_deferred_io,
> +};
> +#endif
> +
> +static struct fb_ops vboxfb_ops = {
> +	.owner = THIS_MODULE,
> +	.fb_check_var = drm_fb_helper_check_var,
> +	.fb_set_par = drm_fb_helper_set_par,
> +	.fb_fillrect = drm_fb_helper_sys_fillrect,
> +	.fb_copyarea = drm_fb_helper_sys_copyarea,
> +	.fb_imageblit = drm_fb_helper_sys_imageblit,
> +	.fb_pan_display = drm_fb_helper_pan_display,
> +	.fb_blank = drm_fb_helper_blank,
> +	.fb_setcmap = drm_fb_helper_setcmap,
> +	.fb_debug_enter = drm_fb_helper_debug_enter,
> +	.fb_debug_leave = drm_fb_helper_debug_leave,
> +};
Use DRM_FB_HELPER_DEFAULT_OPS?

> +
> +static struct vbox_crtc *vbox_crtc_init(struct drm_device *dev, unsigned int i)
> +{
> +	struct vbox_private *vbox =
> +		container_of(dev, struct vbox_private, ddev);
> +	struct drm_plane *cursor = NULL;
> +	struct vbox_crtc *vbox_crtc;
> +	struct drm_plane *primary;
> +	u32 caps = 0;
> +	int ret;
> +
...

> +	primary = vbox_create_plane(vbox, 1 << i, DRM_PLANE_TYPE_PRIMARY);
> +	if (IS_ERR(primary)) {
> +		ret = PTR_ERR(primary);
> +		goto free_mem;
> +	}
primary is a pointer, so it is wrong to use PTR_ERR() on a pointer.
This will just cast this to a long that again is casted to a pointer.

It would be simpler to declare ret a void * to PTR_ERR();


> +
> +	if ((caps & VBOX_VBVA_CURSOR_CAPABILITY_HARDWARE)) {
> +		cursor = vbox_create_plane(vbox, 1 << i, DRM_PLANE_TYPE_CURSOR);
> +		if (IS_ERR(cursor)) {
> +			ret = PTR_ERR(cursor);
> +			goto clean_primary;
> +		}
> +	} else {
> +		DRM_WARN("VirtualBox host is too old, no cursor support\n");
> +	}
> +
> +	vbox_crtc->crtc_id = i;
> +
> +	ret = drm_crtc_init_with_planes(dev, &vbox_crtc->base, primary, cursor,
> +					&vbox_crtc_funcs, NULL);
> +	if (ret)
> +		goto clean_cursor;
> +
> +	drm_mode_crtc_set_gamma_size(&vbox_crtc->base, 256);
> +	drm_crtc_helper_add(&vbox_crtc->base, &vbox_crtc_helper_funcs);
> +
> +	return vbox_crtc;
> +
> +clean_cursor:
> +	if (cursor) {
> +		drm_plane_cleanup(cursor);
> +		kfree(cursor);
> +	}
> +clean_primary:
> +	drm_plane_cleanup(primary);
> +	kfree(primary);
> +free_mem:
> +	kfree(vbox_crtc);
> +	return ERR_PTR(ret);
> +}
> +
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel