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

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

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-cv59FeDIM0c@public.gmane.org>
 > Cc: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

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)?

> Signed-off-by: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>
> Acked-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org>
> ---
>   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 ;)

> +	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.

Robin.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html