Re: [PATCH v2 3/7] drm/vc4: Add KMS support for Raspberry Pi.

From: Stefan Wahren
Date: Wed Aug 19 2015 - 17:37:37 EST


Hi Eric,

only a few nits.

Am 18.08.2015 um 23:54 schrieb Eric Anholt:
This is the start of a full VC4 driver. Right now this just supports
configuring the display using a pre-existing video mode (because
changing the pixel clock isn't available yet, and doesn't work when it
is). However, this is enough for fbcon and bringing up X using
xf86-video-modesetting.

Signed-off-by: Eric Anholt <eric@xxxxxxxxxx>
---

v2: Drop FB_HELPER select thanks to Archit's patches. Do manual init
ordering instead of using the .load hook. Structure registration
more like tegra's, but still using the typical "component" code.
Drop no-op hooks for atomic_begin and mode_fixup() now that
they're optional. Drop sentinel in Makefile. Fix minor style
nits I noticed on another reread.

drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/vc4/Kconfig | 13 +
drivers/gpu/drm/vc4/Makefile | 17 +
drivers/gpu/drm/vc4/vc4_bo.c | 52 ++++
drivers/gpu/drm/vc4/vc4_crtc.c | 565 ++++++++++++++++++++++++++++++++++
drivers/gpu/drm/vc4/vc4_debugfs.c | 38 +++
drivers/gpu/drm/vc4/vc4_drv.c | 271 ++++++++++++++++
drivers/gpu/drm/vc4/vc4_drv.h | 120 ++++++++
drivers/gpu/drm/vc4/vc4_hdmi.c | 633 ++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/vc4/vc4_hvs.c | 161 ++++++++++
drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++
drivers/gpu/drm/vc4/vc4_plane.c | 320 +++++++++++++++++++
drivers/gpu/drm/vc4/vc4_regs.h | 562 +++++++++++++++++++++++++++++++++
14 files changed, 2839 insertions(+)
create mode 100644 drivers/gpu/drm/vc4/Kconfig
create mode 100644 drivers/gpu/drm/vc4/Makefile
create mode 100644 drivers/gpu/drm/vc4/vc4_bo.c
create mode 100644 drivers/gpu/drm/vc4/vc4_crtc.c
create mode 100644 drivers/gpu/drm/vc4/vc4_debugfs.c
create mode 100644 drivers/gpu/drm/vc4/vc4_drv.c
create mode 100644 drivers/gpu/drm/vc4/vc4_drv.h
create mode 100644 drivers/gpu/drm/vc4/vc4_hdmi.c
create mode 100644 drivers/gpu/drm/vc4/vc4_hvs.c
create mode 100644 drivers/gpu/drm/vc4/vc4_kms.c
create mode 100644 drivers/gpu/drm/vc4/vc4_plane.c
create mode 100644 drivers/gpu/drm/vc4/vc4_regs.h

[...]
+static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
+{
+ struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
+ struct drm_crtc_state *state = crtc->state;
+ struct drm_display_mode *mode = &state->adjusted_mode;
+ u32 vactive = (mode->vdisplay >>
+ ((mode->flags & DRM_MODE_FLAG_INTERLACE) ? 1 : 0));
+ u32 format = PV_CONTROL_FORMAT_24;
+ bool debug_dump_regs = false;
+
+ if (debug_dump_regs) {
+ DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc));
+ vc4_crtc_dump_regs(vc4_crtc);
+ }
+
+ /* This is where we would set the pixel clock. */
+
+ /* Reset the PV fifo. */
+ CRTC_WRITE(PV_CONTROL, 0);
+ CRTC_WRITE(PV_CONTROL, PV_CONTROL_FIFO_CLR | PV_CONTROL_EN);
+ CRTC_WRITE(PV_CONTROL, 0);
+
+ CRTC_WRITE(PV_HORZA,
+ VC4_SET_FIELD(mode->htotal - mode->hsync_end,
+ PV_HORZA_HBP) |
+ VC4_SET_FIELD(mode->hsync_end - mode->hsync_start,
+ PV_HORZA_HSYNC));
+ CRTC_WRITE(PV_HORZB,
+ VC4_SET_FIELD(mode->hsync_start - mode->hdisplay,
+ PV_HORZB_HFP) |
+ VC4_SET_FIELD(mode->hdisplay, PV_HORZB_HACTIVE));
+
+ CRTC_WRITE(PV_VERTA,
+ VC4_SET_FIELD(mode->vtotal - mode->vsync_end,
+ PV_VERTA_VBP) |
+ VC4_SET_FIELD(mode->vsync_end - mode->vsync_start,
+ PV_VERTA_VSYNC));
+ CRTC_WRITE(PV_VERTB,
+ VC4_SET_FIELD(mode->vsync_start - mode->vdisplay,
+ PV_VERTB_VFP) |
+ VC4_SET_FIELD(vactive, PV_VERTB_VACTIVE));
+ if (mode->flags & DRM_MODE_FLAG_INTERLACE) {
+ /* Write PV_VERTA_EVEN/VERTB_EVEN */
+ }

checkpatch complains here. Is this intended to have only a comment? Is it a TODO?

[...]
--- /dev/null
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -0,0 +1,120 @@
+/*
+ * Copyright (C) 2015 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "drmP.h"
+#include "drm_gem_cma_helper.h"
+
+struct vc4_dev {
+ struct drm_device *dev;
+
+ struct vc4_hdmi *hdmi;
+ struct vc4_hvs *hvs;
+ struct vc4_crtc *crtc[3];
+};
+
+static inline struct vc4_dev *
+to_vc4_dev(struct drm_device *dev)
+{
+ return (struct vc4_dev *)dev->dev_private;
+}
+
+struct vc4_bo {
+ struct drm_gem_cma_object base;
+};
+
+static inline struct vc4_bo *
+to_vc4_bo(struct drm_gem_object *bo)
+{
+ return (struct vc4_bo *)bo;
+}
+
+struct vc4_hvs {
+ struct platform_device *pdev;
+ void __iomem *regs;
+ void __iomem *dlist;
+};
+
+struct vc4_crtc {
+ struct drm_crtc base;
+ void __iomem *regs;
+
+ /* Which HVS channel we're using for our CRTC. */
+ int channel;
+
+ /* Pointer to the actual hardware display list memory for the
+ * crtc.
+ */
+ u32 __iomem *dlist;
+
+ u32 dlist_size; /* in dwords */
+
+ struct drm_pending_vblank_event *event;
+};
+
+static inline struct vc4_crtc *
+to_vc4_crtc(struct drm_crtc *crtc)
+{
+ return (struct vc4_crtc *)crtc;
+}
+
+struct vc4_plane {
+ struct drm_plane base;
+};
+
+static inline struct vc4_plane *
+to_vc4_plane(struct drm_plane *plane)
+{
+ return (struct vc4_plane *)plane;
+}
+
+#define HVS_READ(offset) readl(vc4->hvs->regs + offset)
+#define HVS_WRITE(offset, val) writel(val, vc4->hvs->regs + offset)
+
+/* vc4_bo.c */

I'm not sure about these filename references.

+void vc4_free_object(struct drm_gem_object *gem_obj);
+struct vc4_bo *vc4_bo_create(struct drm_device *dev, size_t size);
+int vc4_dumb_create(struct drm_file *file_priv,
+ struct drm_device *dev,
+ struct drm_mode_create_dumb *args);
+struct dma_buf *vc4_prime_export(struct drm_device *dev,
+ struct drm_gem_object *obj, int flags);
+
+/* vc4_crtc.c */
+extern struct platform_driver vc4_crtc_driver;
+int vc4_enable_vblank(struct drm_device *dev, int crtc_id);
+void vc4_disable_vblank(struct drm_device *dev, int crtc_id);
+void vc4_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file);
+int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
+
+/* vc4_debugfs.c */
+int vc4_debugfs_init(struct drm_minor *minor);
+void vc4_debugfs_cleanup(struct drm_minor *minor);
+
+/* vc4_drv.c */
+void __iomem *vc4_ioremap_regs(struct platform_device *dev, int index);
+
+/* vc4_hdmi.c */
+extern struct platform_driver vc4_hdmi_driver;
+struct drm_encoder *vc4_hdmi_encoder_init(struct drm_device *dev);
+struct drm_connector *vc4_hdmi_connector_init(struct drm_device *dev,
+ struct drm_encoder *encoder);
+int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused);
+
+/* vc4_hvs.c */
+extern struct platform_driver vc4_hvs_driver;
+void vc4_hvs_dump_state(struct drm_device *dev);
+int vc4_hvs_debugfs_regs(struct seq_file *m, void *unused);
+
+/* vc4_kms.c */
+int vc4_kms_load(struct drm_device *dev);
+
+/* vc4_plane.c */
+struct drm_plane *vc4_plane_init(struct drm_device *dev,
+ enum drm_plane_type type);
+u32 vc4_plane_write_dlist(struct drm_plane *plane, u32 __iomem *dlist);
+u32 vc4_plane_dlist_size(struct drm_plane_state *state);

[...]
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
new file mode 100644
index 0000000..0eff631
--- /dev/null
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -0,0 +1,562 @@
+/*
+ * Copyright  2014-2015 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define VC4_MASK(high, low) (((1 << ((high) - (low) + 1)) - 1) << (low))
+/* Using the GNU statement expression extension */
+#define VC4_SET_FIELD(value, field) \
+ ({ \
+ uint32_t fieldval = (value) << field##_SHIFT; \
+ WARN_ON((fieldval & ~field##_MASK) != 0); \
+ fieldval & field##_MASK; \
+ })
+
+#define VC4_GET_FIELD(word, field) (((word) & field##_MASK) >> \
+ field##_SHIFT)
+
+#define V3D_IDENT0 0x00000
+# define V3D_EXPECTED_IDENT0 \
+ ((2 << 24) | \
+ ('V' << 0) | \
+ ('3' << 8) | \
+ ('D' << 16))
+
+#define V3D_IDENT1 0x00004
+/* Multiples of 1kb */
+# define V3D_IDENT1_VPM_SIZE_MASK VC4_MASK(31, 28)
+# define V3D_IDENT1_VPM_SIZE_SHIFT 28
+# define V3D_IDENT1_NSEM_MASK VC4_MASK(23, 16)
+# define V3D_IDENT1_NSEM_SHIFT 16
+# define V3D_IDENT1_TUPS_MASK VC4_MASK(15, 12)
+# define V3D_IDENT1_TUPS_SHIFT 12
+# define V3D_IDENT1_QUPS_MASK VC4_MASK(11, 8)
+# define V3D_IDENT1_QUPS_SHIFT 8
+# define V3D_IDENT1_NSLC_MASK VC4_MASK(7, 4)
+# define V3D_IDENT1_NSLC_SHIFT 4
+# define V3D_IDENT1_REV_MASK VC4_MASK(3, 0)
+# define V3D_IDENT1_REV_SHIFT 0
+
+#define V3D_IDENT2 0x00008
+#define V3D_SCRATCH 0x00010
+#define V3D_L2CACTL 0x00020
+# define V3D_L2CACTL_L2CCLR (1 << 2)

Maybe you could use the BIT macro?

Thanks Stefan


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/