Hi Gerd.
Very nice diffstat - good work indeed!
I think it'll still be alot easier to review than a longish baby-step patch series.
Agreed.
Some random nits below. With the relevant parts addressed you can add my: Reviewed-by: Sam Ravnborg sam@ravnborg.org
new file mode 100644 index 000000000000..5060e3d854d3 --- /dev/null +++ b/drivers/gpu/drm/cirrus/cirrus.c @@ -0,0 +1,621 @@ +/*
- Copyright 2012-2019 Red Hat
- This file is subject to the terms and conditions of the GNU General
- Public License version 2. See the file COPYING in the main
- directory of this archive for more details.
- Authors: Matthew Garrett
Dave Airlie
Gerd Hoffmann
- Portions of this code derived from cirrusfb.c:
- drivers/video/cirrusfb.c - driver for Cirrus Logic chipsets
- Copyright 1999-2001 Jeff Garzik jgarzik@pobox.com
- */
Can we introduce an SPDX identifier? (I am not good at the license stuff, so I cannot tell)
+#include <linux/module.h> +#include <linux/pci.h> +#include <linux/console.h>
+#include <video/vga.h> +#include <video/cirrus.h>
+#include <drm/drm_drv.h> +#include <drm/drm_file.h> +#include <drm/drm_ioctl.h> +#include <drm/drm_vblank.h> +#include <drm/drm_connector.h>
+#include <drm/drm_fourcc.h> +#include <drm/drm_fb_helper.h> +#include <drm/drm_probe_helper.h> +#include <drm/drm_simple_kms_helper.h> +#include <drm/drm_gem_shmem_helper.h> +#include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_modeset_helper_vtables.h> +#include <drm/drm_damage_helper.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_atomic_state_helper.h>
Please sort again, you added a few new includes since last time
+struct cirrus_device {
- struct drm_device dev;
- struct drm_simple_display_pipe pipe;
- struct drm_connector conn;
- unsigned int cpp;
- unsigned int pitch;
- void __iomem *vram;
- void __iomem *mmio;
+};
+/* ------------------------------------------------------------------ */ +/*
- The meat of this driver. The core passes us a mode and we have to program
- it. The modesetting here is the bare minimum required to satisfy the qemu
- emulation of this hardware, and running this against a real device is
- likely to result in an inadequately programmed mode. We've already had
- the opportunity to modify the mode, so whatever we receive here should
- be something that can be correctly programmed and displayed
- */
+#define RREG8(reg) ioread8(((void __iomem *)cirrus->mmio) + (reg)) +#define WREG8(reg, v) iowrite8(v, ((void __iomem *)cirrus->mmio) + (reg)) +#define RREG32(reg) ioread32(((void __iomem *)cirrus->mmio) + (reg)) +#define WREG32(reg, v) iowrite32(v, ((void __iomem *)cirrus->mmio) + (reg))
The cast of cirrus->mmio to (void __iomem *) should not be necessary as this is the type is has in struct cirrus_device
There is not reason to use a define, use can use a static inline function
+#define SEQ_INDEX 4 +#define SEQ_DATA 5
+#define WREG_SEQ(reg, v) \
- do { \
WREG8(SEQ_INDEX, reg); \WREG8(SEQ_DATA, v); \- } while (0) \
This is only used once, drop the define?
+#define CRT_INDEX 0x14 +#define CRT_DATA 0x15
+#define WREG_CRT(reg, v) \
- do { \
WREG8(CRT_INDEX, reg); \WREG8(CRT_DATA, v); \- } while (0) \
static inline?
+#define GFX_INDEX 0xe +#define GFX_DATA 0xf
+#define WREG_GFX(reg, v) \
- do { \
WREG8(GFX_INDEX, reg); \WREG8(GFX_DATA, v); \- } while (0) \
Used twice - drop?
+#define VGA_DAC_MASK 0x6
+#define WREG_HDR(v) \
- do { \
RREG8(VGA_DAC_MASK); \RREG8(VGA_DAC_MASK); \RREG8(VGA_DAC_MASK); \RREG8(VGA_DAC_MASK); \WREG8(VGA_DAC_MASK, v); \- } while (0) \
Used once, drop?
+static int cirrus_convert_to(struct drm_framebuffer *fb) +{
- if (fb->format->cpp[0] == 4 && fb->pitches[0] > CIRRUS_MAX_PITCH) {
if (fb->width * 3 <= CIRRUS_MAX_PITCH)/* convert from XR24 to RG24 */return 3;else/* convert from XR24 to RG16 */return 2;- }
- return 0;
+}
+static int cirrus_cpp(struct drm_framebuffer *fb) +{
- int convert_cpp = cirrus_convert_to(fb);
- if (convert_cpp)
return convert_cpp;- return fb->format->cpp[0];
+}
+static int cirrus_pitch(struct drm_framebuffer *fb) +{
- int convert_cpp = cirrus_convert_to(fb);
- if (convert_cpp)
return convert_cpp * fb->width;- return fb->pitches[0];
+}
+static int cirrus_mode_set(struct cirrus_device *cirrus,
struct drm_display_mode *mode,struct drm_framebuffer *fb)+{
- int hsyncstart, hsyncend, htotal, hdispend;
- int vtotal, vdispend;
- int tmp;
- int sr07 = 0, hdr = 0;
- htotal = mode->htotal / 8;
- hsyncend = mode->hsync_end / 8;
- hsyncstart = mode->hsync_start / 8;
- hdispend = mode->hdisplay / 8;
- vtotal = mode->vtotal;
- vdispend = mode->vdisplay;
- vdispend -= 1;
- vtotal -= 2;
- htotal -= 5;
- hdispend -= 1;
- hsyncstart += 1;
- hsyncend += 1;
- WREG_CRT(VGA_CRTC_V_SYNC_END, 0x20);
- WREG_CRT(VGA_CRTC_H_TOTAL, htotal);
- WREG_CRT(VGA_CRTC_H_DISP, hdispend);
- WREG_CRT(VGA_CRTC_H_SYNC_START, hsyncstart);
- WREG_CRT(VGA_CRTC_H_SYNC_END, hsyncend);
- WREG_CRT(VGA_CRTC_V_TOTAL, vtotal & 0xff);
- WREG_CRT(VGA_CRTC_V_DISP_END, vdispend & 0xff);
- tmp = 0x40;
- if ((vdispend + 1) & 512)
tmp |= 0x20;- WREG_CRT(VGA_CRTC_MAX_SCAN, tmp);
- /*
* Overflow bits for values that don't fit in the standard registers*/- tmp = 16;
- if (vtotal & 256)
tmp |= 1;- if (vdispend & 256)
tmp |= 2;- if ((vdispend + 1) & 256)
tmp |= 8;- if (vtotal & 512)
tmp |= 32;- if (vdispend & 512)
tmp |= 64;- WREG_CRT(VGA_CRTC_OVERFLOW, tmp);
- tmp = 0;
- /* More overflow bits */
- if ((htotal + 5) & 64)
tmp |= 16;- if ((htotal + 5) & 128)
tmp |= 32;- if (vtotal & 256)
tmp |= 64;- if (vtotal & 512)
tmp |= 128;
For bit operations / numbers above consider to hexadecimal numbers to increase readability.
- WREG_CRT(CL_CRT1A, tmp);
- /* Disable Hercules/CGA compatibility */
- WREG_CRT(VGA_CRTC_MODE, 0x03);
- WREG8(SEQ_INDEX, 0x7);
- sr07 = RREG8(SEQ_DATA);
- sr07 &= 0xe0;
- hdr = 0;
- cirrus->cpp = cirrus_cpp(fb);
- switch (cirrus->cpp * 8) {
- case 8:
sr07 |= 0x11;break;- case 16:
sr07 |= 0x17;hdr = 0xc1;break;- case 24:
sr07 |= 0x15;hdr = 0xc5;break;- case 32:
sr07 |= 0x19;hdr = 0xc5;break;- default:
return -1;- }
- WREG_SEQ(0x7, sr07);
- /* Program the pitch */
- cirrus->pitch = cirrus_pitch(fb);
- tmp = cirrus->pitch / 8;
- WREG_CRT(VGA_CRTC_OFFSET, tmp);
- /* Enable extended blanking and pitch bits, and enable full memory */
- tmp = 0x22;
- tmp |= (cirrus->pitch >> 7) & 0x10;
- tmp |= (cirrus->pitch >> 6) & 0x40;
- WREG_CRT(0x1b, tmp);
- /* Enable high-colour modes */
- WREG_GFX(VGA_GFX_MODE, 0x40);
- /* And set graphics mode */
- WREG_GFX(VGA_GFX_MISC, 0x01);
- WREG_HDR(hdr);
- /* cirrus_crtc_do_set_base(crtc, old_fb, x, y, 0); */
- /* Unblank (needed on S3 resume, vgabios doesn't do it then) */
- outb(0x20, 0x3c0);
- return 0;
+}
+static void cirrus_pipe_update(struct drm_simple_display_pipe *pipe,
struct drm_plane_state *old_state)+{
- struct cirrus_device *cirrus = pipe->crtc.dev->dev_private;
- struct drm_plane_state *state = pipe->plane.state;
- struct drm_crtc *crtc = &pipe->crtc;
- struct drm_rect rect;
- if (pipe->plane.state->fb &&
cirrus->cpp != cirrus_cpp(pipe->plane.state->fb))cirrus_mode_set(cirrus, &crtc->mode,pipe->plane.state->fb);- if (drm_atomic_helper_damage_merged(old_state, state, &rect))
cirrus_fb_blit_rect(pipe->plane.state->fb, &rect);- if (crtc->state->event) {
spin_lock_irq(&crtc->dev->event_lock);drm_crtc_send_vblank_event(crtc, crtc->state->event);spin_unlock_irq(&crtc->dev->event_lock);crtc->state->event = NULL;
Should you set crtc->state->event = NULL; before spin_unlock_irq()?
+/* only bind to the cirrus chip in qemu */ +static const struct pci_device_id pciidlist[] = {
- { PCI_VENDOR_ID_CIRRUS, PCI_DEVICE_ID_CIRRUS_5446,
PCI_SUBVENDOR_ID_REDHAT_QUMRANET, PCI_SUBDEVICE_ID_QEMU,0, 0, 0 },
PCI_DEVICE_SUB(PCI_VENDOR_ID_CIRRUS, PCI_DEVICE_ID_CIRRUS_5446, PCI_SUBVENDOR_ID_REDHAT_QUMRANET, PCI_SUBDEVICE_ID_QEMU) ???? Hmm, looks like an alternative way to say the same, that is hardly much improvement?!?
- { PCI_VENDOR_ID_CIRRUS, PCI_DEVICE_ID_CIRRUS_5446,
PCI_VENDOR_ID_XEN, 0x0001,0, 0, 0 },- { /* end if list */}
Add space before final }