Re: [PATCH v3 04/17] drm/imagination: Add skeleton PowerVR driver
Hi Maxime,
Thank you for your feedback (comments below).
On Fri, 2023-07-07 at 14:46 +0200, Maxime Ripard wrote:
> Hi,
>
> [I just noticed I dropped the Cc list, resending]
>
> Thanks for contributing this driver, it's awesome to see it moving
> forward.
>
> And congrats on the documentation too, it's not often we see a driver
> that well documented on its v3.
>
> I've stripped some parts of the patch that weren't relevant to my
> review.
>
> On Tue, Jun 13, 2023 at 03:47:47PM +0100, Sarah Walker wrote:
> > +static __always_inline struct pvr_device *
> > +to_pvr_device(struct drm_device *drm_dev)
> > +{
> > + return container_of(drm_dev, struct pvr_device, base);
> > +}
>
> For that kind of helpers, we're slowly transitioning to using a macro
> and container_of_const. This allows to work with const-ness context.
Ack
>
> > +static int
> > +pvr_probe(struct platform_device *plat_dev)
> > +{
> > + struct pvr_device *pvr_dev;
> > + struct drm_device *drm_dev;
> > + int err;
> > +
> > + pvr_dev = devm_drm_dev_alloc(_dev->dev, _drm_driver,
> > + struct pvr_device, base);
> > + if (IS_ERR(pvr_dev)) {
> > + err = IS_ERR(pvr_dev);
>
> PTR_ERR?
Good catch :)
>
> > + goto err_out;
>
> The general pattern here is to return directly here, it's simpler to
> follow.
Ack
>
> > + }
> > + drm_dev = _dev->base;
> > +
> > + platform_set_drvdata(plat_dev, drm_dev);
> > +
> > + err = drm_dev_register(drm_dev, 0);
> > + if (err)
> > + goto err_out;
>
> I guess it would be simpler here too, but I think you're going to move
> things around anyway?
>
> > +static const struct of_device_id dt_match[] = {
> > + { .compatible = "ti,am62-gpu", .data = NULL },
> > + { .compatible = "img,powervr-seriesaxe", .data = NULL },
>
> Do you really need both? The binding you documented requires both to be
> there, so I think you can either match on the more specific one (and
> extend that list when needed) or match the more generic one and be done
> with it once and for all. Having both is redundant.
We'll drop the more specific one in the next version.
>
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, dt_match);
> > +
> > +static struct platform_driver pvr_driver = {
> > + .probe = pvr_probe,
> > + .remove = pvr_remove,
> > + .driver = {
> > + .name = PVR_DRIVER_NAME,
> > + .of_match_table = dt_match,
> > + },
> > +};
> > +module_platform_driver(pvr_driver);
> > +
> > +MODULE_AUTHOR("Imagination Technologies Ltd.");
> > +MODULE_DESCRIPTION(PVR_DRIVER_DESC);
> > +MODULE_LICENSE("Dual MIT/GPL");
> > +MODULE_IMPORT_NS(DMA_BUF);
> > +MODULE_FIRMWARE("powervr/rogue_4.40.2.51_v1.fw");
> > +MODULE_FIRMWARE("powervr/rogue_33.15.11.3_v1.fw");
>
> That one should probably be moved to the firmware patch?
Ack
>
> > diff --git a/drivers/gpu/drm/imagination/pvr_drv.h
> > b/drivers/gpu/drm/imagination/pvr_drv.h
> > new file mode 100644
> > index ..8e6f4a4dde3f
> > --- /dev/null
> > +++ b/drivers/gpu/drm/imagination/pvr_drv.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> > +
> > +#ifndef PVR_DRV_H
> > +#define PVR_DRV_H
> > +
> > +#include "linux/compiler_attributes.h"
> > +#include
> > +
> > +#define PVR_DRIVER_NAME "powervr"
> > +#define PVR_DRIVER_DESC "Imagination PowerVR Graphics"
>
> Do you intend to support the SGX and Rogue GPUs with this driver? If
> not, mentioning the generation/architecture name somewhere would be
> nice.
We don't currently have any plans to support SGX ourselves, so we'll update
this, and any other places, to clarify things.
Thanks
Frank
>
> Maxime
Re: [PATCH v3 04/17] drm/imagination: Add skeleton PowerVR driver
Hi,
[I just noticed I dropped the Cc list, resending]
Thanks for contributing this driver, it's awesome to see it moving
forward.
And congrats on the documentation too, it's not often we see a driver
that well documented on its v3.
I've stripped some parts of the patch that weren't relevant to my
review.
On Tue, Jun 13, 2023 at 03:47:47PM +0100, Sarah Walker wrote:
> +static __always_inline struct pvr_device *
> +to_pvr_device(struct drm_device *drm_dev)
> +{
> + return container_of(drm_dev, struct pvr_device, base);
> +}
For that kind of helpers, we're slowly transitioning to using a macro
and container_of_const. This allows to work with const-ness context.
> +static int
> +pvr_probe(struct platform_device *plat_dev)
> +{
> + struct pvr_device *pvr_dev;
> + struct drm_device *drm_dev;
> + int err;
> +
> + pvr_dev = devm_drm_dev_alloc(_dev->dev, _drm_driver,
> + struct pvr_device, base);
> + if (IS_ERR(pvr_dev)) {
> + err = IS_ERR(pvr_dev);
PTR_ERR?
> + goto err_out;
The general pattern here is to return directly here, it's simpler to
follow.
> + }
> + drm_dev = _dev->base;
> +
> + platform_set_drvdata(plat_dev, drm_dev);
> +
> + err = drm_dev_register(drm_dev, 0);
> + if (err)
> + goto err_out;
I guess it would be simpler here too, but I think you're going to move
things around anyway?
> +static const struct of_device_id dt_match[] = {
> + { .compatible = "ti,am62-gpu", .data = NULL },
> + { .compatible = "img,powervr-seriesaxe", .data = NULL },
Do you really need both? The binding you documented requires both to be
there, so I think you can either match on the more specific one (and
extend that list when needed) or match the more generic one and be done
with it once and for all. Having both is redundant.
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, dt_match);
> +
> +static struct platform_driver pvr_driver = {
> + .probe = pvr_probe,
> + .remove = pvr_remove,
> + .driver = {
> + .name = PVR_DRIVER_NAME,
> + .of_match_table = dt_match,
> + },
> +};
> +module_platform_driver(pvr_driver);
> +
> +MODULE_AUTHOR("Imagination Technologies Ltd.");
> +MODULE_DESCRIPTION(PVR_DRIVER_DESC);
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_IMPORT_NS(DMA_BUF);
> +MODULE_FIRMWARE("powervr/rogue_4.40.2.51_v1.fw");
> +MODULE_FIRMWARE("powervr/rogue_33.15.11.3_v1.fw");
That one should probably be moved to the firmware patch?
> diff --git a/drivers/gpu/drm/imagination/pvr_drv.h
> b/drivers/gpu/drm/imagination/pvr_drv.h
> new file mode 100644
> index ..8e6f4a4dde3f
> --- /dev/null
> +++ b/drivers/gpu/drm/imagination/pvr_drv.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> +
> +#ifndef PVR_DRV_H
> +#define PVR_DRV_H
> +
> +#include "linux/compiler_attributes.h"
> +#include
> +
> +#define PVR_DRIVER_NAME "powervr"
> +#define PVR_DRIVER_DESC "Imagination PowerVR Graphics"
Do you intend to support the SGX and Rogue GPUs with this driver? If
not, mentioning the generation/architecture name somewhere would be
nice.
Maxime
signature.asc
Description: PGP signature
[PATCH v3 04/17] drm/imagination: Add skeleton PowerVR driver
This adds the basic skeleton of the driver. The driver registers
itself with DRM on probe. Ioctl handlers are currently implemented
as stubs.
Signed-off-by: Sarah Walker
---
MAINTAINERS | 1 +
drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/imagination/Kconfig | 15 +
drivers/gpu/drm/imagination/Makefile | 9 +
drivers/gpu/drm/imagination/pvr_device.h | 169
drivers/gpu/drm/imagination/pvr_drv.c| 530 +++
drivers/gpu/drm/imagination/pvr_drv.h| 22 +
8 files changed, 749 insertions(+)
create mode 100644 drivers/gpu/drm/imagination/Kconfig
create mode 100644 drivers/gpu/drm/imagination/Makefile
create mode 100644 drivers/gpu/drm/imagination/pvr_device.h
create mode 100644 drivers/gpu/drm/imagination/pvr_drv.c
create mode 100644 drivers/gpu/drm/imagination/pvr_drv.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 3ad9de48a463..4a6107ec91cb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10090,6 +10090,7 @@ M: Sarah Walker
M: Donald Robson
S: Supported
F: Documentation/devicetree/bindings/gpu/img,powervr.yaml
+F: drivers/gpu/drm/imagination/
F: include/uapi/drm/pvr_drm.h
IMON SOUNDGRAPH USB IR RECEIVER
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index bb2e48cc6cd6..dc3fb60eec6f 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -365,6 +365,8 @@ source "drivers/gpu/drm/solomon/Kconfig"
source "drivers/gpu/drm/sprd/Kconfig"
+source "drivers/gpu/drm/imagination/Kconfig"
+
config DRM_HYPERV
tristate "DRM Support for Hyper-V synthetic video device"
depends on DRM && PCI && MMU && HYPERV
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index ba1a4878fb55..a1418574ccc4 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -195,3 +195,4 @@ obj-y += gud/
obj-$(CONFIG_DRM_HYPERV) += hyperv/
obj-y += solomon/
obj-$(CONFIG_DRM_SPRD) += sprd/
+obj-$(CONFIG_DRM_POWERVR) += imagination/
diff --git a/drivers/gpu/drm/imagination/Kconfig
b/drivers/gpu/drm/imagination/Kconfig
new file mode 100644
index ..9cda99f66a8d
--- /dev/null
+++ b/drivers/gpu/drm/imagination/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0 OR MIT
+# Copyright (c) 2022 Imagination Technologies Ltd.
+
+config DRM_POWERVR
+ tristate "Imagination Technologies PowerVR Graphics"
+ depends on ARM64
+ depends on DRM
+ select DRM_GEM_SHMEM_HELPER
+ select DRM_SCHED
+ select FW_LOADER
+ help
+ Choose this option if you have a system that has an Imagination
+ Technologies PowerVR Rogue GPU.
+
+ If "M" is selected, the module will be called powervr.
diff --git a/drivers/gpu/drm/imagination/Makefile
b/drivers/gpu/drm/imagination/Makefile
new file mode 100644
index ..62ccf0ccbd51
--- /dev/null
+++ b/drivers/gpu/drm/imagination/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0 OR MIT
+# Copyright (c) 2022 Imagination Technologies Ltd.
+
+subdir-ccflags-y := -I$(srctree)/$(src)
+
+powervr-y := \
+ pvr_drv.o \
+
+obj-$(CONFIG_DRM_POWERVR) += powervr.o
diff --git a/drivers/gpu/drm/imagination/pvr_device.h
b/drivers/gpu/drm/imagination/pvr_device.h
new file mode 100644
index ..3d2865d726b8
--- /dev/null
+++ b/drivers/gpu/drm/imagination/pvr_device.h
@@ -0,0 +1,169 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/* Copyright (c) 2022 Imagination Technologies Ltd. */
+
+#ifndef PVR_DEVICE_H
+#define PVR_DEVICE_H
+
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * struct pvr_device - powervr-specific wrapper for drm_device
+ */
+struct pvr_device {
+ /**
+* @base: The underlying drm_device.
+*
+* Do not access this member directly, instead call
+* from_pvr_device().
+*/
+ struct drm_device base;
+};
+
+/**
+ * struct pvr_file - powervr-specific data to be assigned to
+ * drm_file.driver_priv
+ */
+struct pvr_file {
+ /**
+* @file: A reference to the parent drm_file.
+*
+* Do not access this member directly, instead call from_pvr_file().
+*/
+ struct drm_file *file;
+
+ /**
+* @pvr_dev: A reference to the powervr-specific wrapper for the
+* associated device. Saves on repeated calls to
+* to_pvr_device().
+*/
+ struct pvr_device *pvr_dev;
+};
+
+static __always_inline struct drm_device *
+from_pvr_device(struct pvr_device *pvr_dev)
+{
+ return _dev->base;
+}
+
+static __always_inline struct pvr_device *
+to_pvr_device(struct drm_device *drm_dev)
+{
+ return container_of(drm_dev, struct pvr_device, base);
+}
+
+static __always_inline struct drm_file *
+from_pvr_file(struct pvr_file *pvr_file)
