Re: [PATCH v2 6/6] drm/cirrus: rewrite and modernize driver.

From: Sam Ravnborg
Date: Thu Apr 04 2019 - 13:41:22 EST

Next message: Dmitry Torokhov: "Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name"
Previous message: Joel Fernandes: "Re: [RFC PATCH 0/7] Early task context tracking"
In reply to: Gerd Hoffmann: "[PATCH v2 6/6] drm/cirrus: rewrite and modernize driver."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxx>

> 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@xxxxxxxxx>
> + */
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 }

Next message: Dmitry Torokhov: "Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name"
Previous message: Joel Fernandes: "Re: [RFC PATCH 0/7] Early task context tracking"
In reply to: Gerd Hoffmann: "[PATCH v2 6/6] drm/cirrus: rewrite and modernize driver."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]