Re: [PATCH v5 2/4] drm: Add support for ARM's HDLCD controller.

From: Robin Murphy
Date: Thu Dec 10 2015 - 06:46:47 EST

Next message: Sergei Shtylyov: "Re: [PATCH 4/9v2] usb: host: ehci.h: fix single statement macros"
Previous message: Wim de With: "[PATCH v2] staging: gdm72xx: add userspace data struct"
In reply to: Liviu Dudau: "Re: [PATCH v5 2/4] drm: Add support for ARM's HDLCD controller."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 08/12/15 16:52, Liviu Dudau wrote:

On Tue, Dec 08, 2015 at 04:25:27PM +0000, Robin Murphy wrote:

Hi Liviu,

On 07/12/15 12:11, Liviu Dudau wrote:

The HDLCD controller is a display controller that supports resolutions
up to 4096x4096 pixels. It is present on various development boards
produced by ARM Ltd and emulated by the latest Fast Models from the
company.

Cc: David Airlie <airlied@xxxxxxxx>
Cc: Robin Murphy <robin.murphy@xxxxxxx>

I've given this a spin on my Juno, and the first thing of note is this:

hdlcd 7ff60000.hdlcd: master bind failed: -517
hdlcd 7ff50000.hdlcd: master bind failed: -517
scpi_protocol scpi: SCP Protocol 1.0 Firmware 1.9.0 version
[drm] found ARM HDLCD version r0p0
tda998x 0-0070: Falling back to first CRTC
usb 1-1: new high-speed USB device number 2 using ehci-platform
tda998x 0-0070: found TDA19988
hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops)
[drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[drm] No driver support for vblank timestamp query.
------------[ cut here ]------------
WARNING: at drivers/gpu/drm/drm_atomic_helper.c:682
Modules linked in:

CPU: 2 PID: 98 Comm: kworker/u12:3 Tainted: G W 4.4.0-rc2+ #846
Hardware name: ARM Juno development board (r1) (DT)
Workqueue: deferwq deferred_probe_work_func
task: fffffe007ecb3700 ti: fffffe09409c8000 task.ti: fffffe09409c8000
PC is at drm_atomic_helper_update_legacy_modeset_state+0x1e8/0x1f0
LR is at drm_atomic_helper_commit_modeset_disables+0x1a8/0x388
pc : [<fffffe00004a4468>] lr : [<fffffe00004a59b8>] pstate: 20000045
sp : fffffe09409cb560
x29: fffffe09409cb560 x28: fffffe0940bf2800
x27: fffffe0940070000 x26: 0000000000000001
x25: fffffe0000be4b50 x24: fffffe00007ae820
x23: fffffe0000be4b50 x22: fffffe0940bd1000
x21: fffffe0940bd1000 x20: 0000000000000000
x19: fffffe0940968000 x18: fffffe0940c8091c
x17: 0000000000000007 x16: 0000000000000001
x15: fffffe0940c8016f x14: 0000003c00000000
x13: 0000000000000000 x12: 000004c9000004b4
x11: 000004b1000004c9 x10: 000004b0000004b0
x9 : 00000000000006f4 x8 : 000006a400000654
x7 : 000006f400000640 x6 : fffffe0940966480
x5 : fffffe0940968200 x4 : 0000000000000001
x3 : fffffe0940966a80 x2 : 0000000000000000
x1 : fffffe0940bd0900 x0 : fffffe0940bd0960

---[ end trace bdb6af69b29bf7ea ]---
Call trace:
[<fffffe00004a4468>]
drm_atomic_helper_update_legacy_modeset_state+0x1e8/0x1f0
[<fffffe00004a59b8>] drm_atomic_helper_commit_modeset_disables+0x1a8/0x388
[<fffffe00004a5c70>] drm_atomic_helper_commit+0xd8/0x140
[<fffffe00004ccce0>] hdlcd_atomic_commit+0x10/0x18
[<fffffe00004c9ef8>] drm_atomic_commit+0x40/0x70
[<fffffe00004a69b0>] restore_fbdev_mode+0x270/0x2b0
[<fffffe00004a8d64>] drm_fb_helper_restore_fbdev_mode_unlocked+0x34/0x90
[<fffffe00004a8dec>] drm_fb_helper_set_par+0x2c/0x60
[<fffffe000042f010>] fbcon_init+0x4d0/0x520
[<fffffe000046ec9c>] visual_init+0xac/0x108
[<fffffe000047060c>] do_bind_con_driver+0x1d4/0x3e8
[<fffffe0000470c14>] do_take_over_console+0x174/0x1e8
[<fffffe000042c38c>] do_fbcon_takeover+0x74/0x100
[<fffffe00004313b4>] fbcon_event_notify+0x77c/0x7d8
[<fffffe00000dc700>] notifier_call_chain+0x50/0x90
[<fffffe00000dcadc>] __blocking_notifier_call_chain+0x4c/0x90
[<fffffe00000dcb34>] blocking_notifier_call_chain+0x14/0x20
[<fffffe0000435204>] fb_notifier_call_chain+0x1c/0x28
[<fffffe0000437010>] register_framebuffer+0x1c0/0x2b8
[<fffffe00004a90a4>] drm_fb_helper_initial_config+0x284/0x3e8
[<fffffe00004a991c>] drm_fbdev_cma_init+0x94/0x148
[<fffffe00004ccfc4>] hdlcd_drm_bind+0x1d4/0x418
[<fffffe00004d15f0>] try_to_bring_up_master.part.2+0xc8/0x110
[<fffffe00004d1820>] component_add+0x90/0x108
[<fffffe00004ce6a8>] tda998x_probe+0x18/0x20
[<fffffe00005a6d24>] i2c_device_probe+0x164/0x228
[<fffffe00004d698c>] driver_probe_device+0x1ec/0x2f0
[<fffffe00004d6bc0>] __device_attach_driver+0x90/0xd8
[<fffffe00004d4b88>] bus_for_each_drv+0x58/0x98
[<fffffe00004d66e4>] __device_attach+0xc4/0x148
[<fffffe00004d6c58>] device_initial_probe+0x10/0x18
[<fffffe00004d5b9c>] bus_probe_device+0x94/0xa0
[<fffffe00004d6020>] deferred_probe_work_func+0x70/0xa8
[<fffffe00000d5b70>] process_one_work+0x138/0x378
[<fffffe00000d5ed4>] worker_thread+0x124/0x498
[<fffffe00000dbb54>] kthread+0xdc/0xf0
[<fffffe0000093980>] ret_from_fork+0x10/0x50
Console: switching to colour frame buffer device 150x100

which for reference, is the first one in that function:

...
/* clear out existing links and update dpms */
for_each_connector_in_state(old_state, connector, old_conn_state, i) {
if (connector->encoder) {
WARN_ON(!connector->encoder->crtc);
...

That's on 4.4-rc2 with this series plus the 3 tda998x patches from Russell's
patch system applied. Is there something else I'm missing or does this need
looking at (could it be related to that initial probe deferral)?

Yeah, you also need Thierry Reding's patch to not set the connector->encoder in
drivers.

http://lists.freedesktop.org/archives/dri-devel/2015-November/094576.html

Ah, right, I don't think that one was specifically called out anywhere, but it does indeed make the splat go away, thanks!

Signed-off-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
---
drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/arm/Kconfig | 29 ++
drivers/gpu/drm/arm/Makefile | 2 +
drivers/gpu/drm/arm/hdlcd_crtc.c | 329 ++++++++++++++++++++++
drivers/gpu/drm/arm/hdlcd_drv.c | 580 +++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/arm/hdlcd_drv.h | 42 +++
drivers/gpu/drm/arm/hdlcd_regs.h | 87 ++++++
8 files changed, 1072 insertions(+)
create mode 100644 drivers/gpu/drm/arm/Kconfig
create mode 100644 drivers/gpu/drm/arm/Makefile
create mode 100644 drivers/gpu/drm/arm/hdlcd_crtc.c
create mode 100644 drivers/gpu/drm/arm/hdlcd_drv.c
create mode 100644 drivers/gpu/drm/arm/hdlcd_drv.h
create mode 100644 drivers/gpu/drm/arm/hdlcd_regs.h

[...]

+static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc)
+{
+ unsigned int btpp, default_color = 0x00000000;
+ struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+ uint32_t pixel_format;
+ struct simplefb_format *format = NULL;
+ int i;
+
+#ifdef CONFIG_DRM_HDLCD_SHOW_UNDERRUN
+ default_color = 0x00ff0000; /* show underruns in red */
+#endif
+
+ pixel_format = crtc->primary->state->fb->pixel_format;
+
+ for (i = 0; i < ARRAY_SIZE(supported_formats); i++) {
+ if (supported_formats[i].fourcc == pixel_format)
+ format = &supported_formats[i];
+ }
+
+ if (WARN_ON(!format)) {
+ return 0;
+ }

nit: unnecessary braces.

+ /* HDLCD uses 'bytes per pixel', zero means 1 byte */
+ btpp = (format->bits_per_pixel + 7) / 8;
+ hdlcd_write(hdlcd, HDLCD_REG_PIXEL_FORMAT, (btpp - 1) << 3);
+
+ /*
+ * The format of the HDLCD_REG_<color>_SELECT register is:
+ * - bits[23:16] - default value for that color component
+ * - bits[11:8] - number of bits to extract for each color component
+ * - bits[4:0] - index of the lowest bit to extract
+ *
+ * The default color value is used when bits[11:8] are zero, when the
+ * pixel is outside the visible frame area or when there is a
+ * buffer underrun.
+ */
+ hdlcd_write(hdlcd, HDLCD_REG_RED_SELECT, default_color |
+ format->red.offset | (format->red.length & 0xf) << 8);
+ hdlcd_write(hdlcd, HDLCD_REG_GREEN_SELECT, default_color |
+ format->green.offset | (format->green.length & 0xf) << 8);
+ hdlcd_write(hdlcd, HDLCD_REG_BLUE_SELECT, default_color |
+ format->blue.offset | (format->blue.length & 0xf) << 8);

These would seem to be putting bits 23:16 from default_color into the
default field of every register, and indeed underruns show up as a very
white-looking shade of red for me ;)

Ooops, I better change that. Could you tell me how you trigger underruns
in your setup?

I cheat and put the SMMU into a state where it won't allow anything through at all, so it's the ultimate 'underrun'.

+ return 0;
+}

[...]

+static void hdlcd_fb_output_poll_changed(struct drm_device *drm)
+{
+ struct hdlcd_drm_private *hdlcd = drm->dev_private;
+
+ if (hdlcd->fbdev) {
+ drm_fbdev_cma_hotplug_event(hdlcd->fbdev);
+ }

nit: braces.

+}

[...]

+static irqreturn_t hdlcd_irq(int irq, void *arg)
+{
+ struct drm_device *drm = arg;
+ struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ unsigned long irq_status;
+
+ irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
+
+#ifdef CONFIG_DEBUG_FS
+ if (irq_status & HDLCD_INTERRUPT_UNDERRUN) {
+ atomic_inc(&hdlcd->buffer_underrun_count);
+ }
+ if (irq_status & HDLCD_INTERRUPT_DMA_END) {
+ atomic_inc(&hdlcd->dma_end_count);
+ }
+ if (irq_status & HDLCD_INTERRUPT_BUS_ERROR) {
+ atomic_inc(&hdlcd->bus_error_count);
+ }
+ if (irq_status & HDLCD_INTERRUPT_VSYNC) {
+ atomic_inc(&hdlcd->vsync_count);
+ }

nit: braces again (it's only because I'm too lazy to remove the newbie
checkpatch commit hook, and a manual merge of this into my SMMU dev tree
made that throw a fit)

+#endif
+ if (irq_status & HDLCD_INTERRUPT_VSYNC) {
+ bool events_sent = false;
+ unsigned long flags;
+ struct drm_pending_vblank_event *e, *t;
+
+ drm_crtc_handle_vblank(&hdlcd->crtc);
+
+ spin_lock_irqsave(&drm->event_lock, flags);
+ list_for_each_entry_safe(e, t, &hdlcd->event_list, base.link) {
+ list_del(&e->base.link);
+ drm_crtc_send_vblank_event(&hdlcd->crtc, e);
+ events_sent = true;
+ }
+ if (events_sent)
+ drm_crtc_vblank_put(&hdlcd->crtc);
+ spin_unlock_irqrestore(&drm->event_lock, flags);
+ }
+
+ /* acknowledge interrupt(s) */
+ hdlcd_write(hdlcd, HDLCD_REG_INT_CLEAR, irq_status);
+
+ return IRQ_HANDLED;
+}

Other than that though, it seems to do the job. I get a usable framebuffer
console and can boot to an X desktop with at least the ancient 1600x1200 DVI
monitor I have handy - the 1920x1080 HDMI one seems to get recognised OK but
the monitor itself doesn't like the signal and just locks up until I unplug
it, although I know that's more of a clock driver/firmware issue.

I have a TV that does the same. Yes, the SCPI clock that was picked for this
resolution is a standard one, but I bet that these monitors are slightly out
of spec. At least my TV lists two options for 1080p: one with 145MHz pixel
clock and another with 145.382MHz. But Linux decides to go interlaced anyway,
so I can't test without a hack in tda998x_drv.c.

Yeah, the fact that it goes unresponsive by continually trying to sync with the signal rather than just giving up and displaying the 'out of range' message suggests that's the case. Oddly, that one did used to work fine with the old hard-coded SCP clock. Oh well.

Robin.

Thanks for testing this.

Best regards,
Liviu

Robin.

--
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/

Next message: Sergei Shtylyov: "Re: [PATCH 4/9v2] usb: host: ehci.h: fix single statement macros"
Previous message: Wim de With: "[PATCH v2] staging: gdm72xx: add userspace data struct"
In reply to: Liviu Dudau: "Re: [PATCH v5 2/4] drm: Add support for ARM's HDLCD controller."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]