Message-ID: <56696616.20805@arm.com>
Date:	Thu, 10 Dec 2015 11:46:30 +0000
From:	Robin Murphy <robin.murphy@....com>
To:	Liviu Dudau <Liviu.Dudau@....com>
Cc:	David Airlie <airlied@...ux.ie>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Sudeep Holla <sudeep.holla@....com>,
	Jon Medhurst <tixy@...aro.org>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Rob Herring <robh@...nel.org>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Pawel Moll <pawel.moll@....com>, Arnd Bergmann <arnd@...db.de>,
	Olof Johansson <olof@...om.net>,
	Punit Agrawal <punit.agrawal@....com>,
	DRI devel <dri-devel@...ts.freedesktop.org>,
	devicetree <devicetree@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LAKML <linux-arm-kernel@...ts.infradead.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 2/4] drm: Add support for ARM's HDLCD controller.

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@...ux.ie>
>>> Cc: Robin Murphy <robin.murphy@....com>
>>
>> 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@....com>
>>> Acked-by: Daniel Vetter <daniel.vetter@...ll.ch>
>>> ---
>>>   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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists