Hi,
Thank you for the review.
On 18-10-18 20:12, Sam Ravnborg wrote:
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.
Right, just tagging the entry to the end seems to be what
all new driver additions do, so I'm just following that
example here.
+++ 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
Fixed in the set of cleanup patches which I'm sending out
after this email.
Note these cleanup patches apply to the staging version of
the driver, I will do a new version of this patch after
someone has found the time to do a more thorough review.
+
+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.
I do not work for Oracle, so its not up to me to extend the copyright
claim.
+ 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.
The header_data member is used in the code.
+
+ /* 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@xxxxxxxxxx>
+ * Michael Thayer <michael.thayer@xxxxxxxxxx,
+ * Hans de Goede <hdegoede@xxxxxxxxxx>
+ */
+#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
Fixed in the cleanup series.
+
+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()?
Ack, fixed in the cleanup series.
+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?
No, we do not define a release callback in our drm_driver (ops)
struct, so drm_dev_release() from drivers/gpu/drm/drm_drv.c,
which gets called on the last drm_dev_put, will call
kfree() on our embedded struct drm_device, also see
the definition of the vbox_private struct, which has:
struct vbox_private {
/* Must be first; or we must define our own release callback */
struct drm_device ddev;
This is a standard way of doing this for drm drivers.
+ 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?
Yes your right, will fix in the cleanup series.
+
+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
Ack, fixed.
+
+MODULE_AUTHOR("Oracle Corporation");
Add yourself?
Yes good idea, done.
+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.
This sets up the MTRR registers to set the cache-policy for the video-mem to
write-combined, see:
https://www.kernel.org/doc/htmldocs/kernel-api/API-arch-phys-wc-add.html
If you look under drivers/gpu/drm you will so more drivers doing this.
The return value not being used for anything but cleanup is normal.
+
+ 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?
Because these are only intended to determine the size of the cursor_data
member and not intended for use in the driver code.
+ */
+ 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.
Ack, fixed.
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@xxxxxxxxxx>
+ * Michael Thayer <michael.thayer@xxxxxxxxxx,
+ */
+#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?
Yes good, idea, done.
+
+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.
PTR_ERR stands for pointer to error and ret is an int, so this is fine.
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);
+}
+
Regards,
Hans
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
