Re: [PATCH 1/4 v3] drm: Add support of ARC PGU display controller

From: Alexey Brodkin
Date: Mon Mar 14 2016 - 07:16:28 EST

Hi Daniel,

On Mon, 2016-03-14 at 08:00 +-0100, Daniel Vetter wrote:
+AD4- On Fri, Mar 11, 2016 at 06:42:36PM +-0300, Alexey Brodkin wrote:
+AD4- +AD4-
+AD4- +AD4- ARC PGU could be found on some development boards from Synopsys.
+AD4- +AD4- This is a simple byte streamer that reads data from a framebuffer
+AD4- +AD4- and sends data to the single encoder.
+AD4- +AD4-
+AD4- +AD4- Signed-off-by: Alexey Brodkin +ADw-abrodkin+AEA-synopsys.com+AD4-
+AD4- +AD4- Cc: David Airlie +ADw-airlied+AEA-linux.ie+AD4-
+AD4- +AD4- Cc: dri-devel+AEA-lists.freedesktop.org
+AD4- +AD4- Cc: linux-snps-arc+AEA-lists.infradead.org
+AD4- +AD4- Cc: Jose Abreu +ADw-joabreu+AEA-synopsys.com+AD4-
+AD4- +AD4- ---
+AD4- +AD4-
+AD4- +AD4- Changes v2 -+AD4- v3:
+AD4- +AD4- +AKAAKg- Improved failure path if arcpgu+AF8-connector wasn't allocated (thanks Jose).
+AD4- +AD4- +AKAAKg- Fixed driver building as module (reported by 0-DAY kernel test infrastruct.)
+AD4- +AD4- +AKAAKg- Implemented uncached mapping of user-space FB pages.
+AD4- +AD4-
+AD4- +AD4- No changes v1 -+AD4- v2.
+AD4- +AD4-
+AD4- Bunch of comments below to update your driver to latest styles and best
+AD4- practices.
+AD4-
+AD4- Cheers, Daniel

Thanks for doing that review+ACE-

+AD4- +AD4- +-
+AD4- +AD4- +-static void arc+AF8-pgu+AF8-crtc+AF8-atomic+AF8-flush(struct drm+AF8-crtc +ACo-crtc,
+AD4- +AD4- +- +AKAAoACgAKAAoACg-struct drm+AF8-crtc+AF8-state +ACo-state)
+AD4- +AD4- +-+AHs-
+AD4- +AD4- +-+AH0-
+AD4- +AD4- +-
+AD4- +AD4- +-static bool arc+AF8-pgu+AF8-crtc+AF8-mode+AF8-fixup(struct drm+AF8-crtc +ACo-crtc,
+AD4- +AD4- +- +AKAAoACgAKA-const struct drm+AF8-display+AF8-mode +ACo-mode,
+AD4- +AD4- +- +AKAAoACgAKA-struct drm+AF8-display+AF8-mode +ACo-adjusted+AF8-mode)
+AD4- +AD4- +-+AHs-
+AD4- +AD4- +- return true+ADs-
+AD4- +AD4- +-+AH0-
+AD4- You can drop the above 2 dummy functions.

Ok will do.

+AD4- +AD4-
+AD4- +AD4- +-
+AD4- +AD4- +-static const struct drm+AF8-crtc+AF8-helper+AF8-funcs arc+AF8-pgu+AF8-crtc+AF8-helper+AF8-funcs +AD0- +AHs-
+AD4- +AD4- +- .mode+AF8-fixup +AD0- arc+AF8-pgu+AF8-crtc+AF8-mode+AF8-fixup,
+AD4- +AD4- +- .mode+AF8-set +AD0- drm+AF8-helper+AF8-crtc+AF8-mode+AF8-set,
+AD4- +AD4- +- .mode+AF8-set+AF8-base +AD0- drm+AF8-helper+AF8-crtc+AF8-mode+AF8-set+AF8-base,
+AD4- +AD4- +- .mode+AF8-set+AF8-nofb +AD0- arc+AF8-pgu+AF8-crtc+AF8-mode+AF8-set+AF8-nofb,
+AD4- +AD4- +- .enable +AD0- arc+AF8-pgu+AF8-crtc+AF8-enable,
+AD4- +AD4- +- .disable +AD0- arc+AF8-pgu+AF8-crtc+AF8-disable,
+AD4- +AD4- +- .prepare +AD0- arc+AF8-pgu+AF8-crtc+AF8-disable,
+AD4- +AD4- +- .commit +AD0- arc+AF8-pgu+AF8-crtc+AF8-enable,
+AD4- +AD4- +- .atomic+AF8-check +AD0- arc+AF8-pgu+AF8-crtc+AF8-atomic+AF8-check,
+AD4- +AD4- +- .atomic+AF8-begin +AD0- arc+AF8-pgu+AF8-crtc+AF8-atomic+AF8-begin,
+AD4- +AD4- +- .atomic+AF8-flush +AD0- arc+AF8-pgu+AF8-crtc+AF8-atomic+AF8-flush,
+AD4- +AD4- +-+AH0AOw-
+AD4- +AD4- +-
+AD4- +AD4- +-static int arc+AF8-pgu+AF8-plane+AF8-atomic+AF8-check(struct drm+AF8-plane +ACo-plane,
+AD4- +AD4- +- +AKAAoACgAKAAoACg-struct drm+AF8-plane+AF8-state +ACo-state)
+AD4- +AD4- +-+AHs-
+AD4- +AD4- +- return 0+ADs-
+AD4- +AD4- +-+AH0-
+AD4- You don't need dummy functions for this.

Ditto.

+AD4- +AD4- +-
+AD4- +AD4- +-void arc+AF8-pgu+AF8-crtc+AF8-suspend(struct drm+AF8-crtc +ACo-crtc)
+AD4- +AD4- +-+AHs-
+AD4- +AD4- +- arc+AF8-pgu+AF8-crtc+AF8-disable(crtc)+ADs-
+AD4- +AD4- +-+AH0-
+AD4- +AD4- +-
+AD4- +AD4- +-void arc+AF8-pgu+AF8-crtc+AF8-resume(struct drm+AF8-crtc +ACo-crtc)
+AD4- +AD4- +-+AHs-
+AD4- +AD4- +- arc+AF8-pgu+AF8-crtc+AF8-enable(crtc)+ADs-
+AD4- +AD4- +-+AH0-
+AD4- Please use the atomic suspend/resume helper that Thierry recently merged.
+AD4- See the kerneldoc of drm+AF8-atomic+AF8-helper+AF8-suspend as a starting point for how
+AD4- it works and how it's supposed to be used.

Well looks like this is a reminder if dummy copy-paste.
We don't support PM in that driver yet, so I'll remove both functions
for now.

+AD4- +AD4- +-static int arcpgu+AF8-atomic+AF8-commit(struct drm+AF8-device +ACo-dev,
+AD4- +AD4- +- +AKAAoACgAKA-struct drm+AF8-atomic+AF8-state +ACo-state, bool async)
+AD4- +AD4- +-+AHs-
+AD4- +AD4- +- return drm+AF8-atomic+AF8-helper+AF8-commit(dev, state, false)+ADs-
+AD4- Note that this isn't really async if you ever get around to implement
+AD4- fence support or vblank support. Just fyi.

Ok but for now should I leave it as it is?

+AD4- +AD4- +-static struct drm+AF8-driver arcpgu+AF8-drm+AF8-driver +AD0- +AHs-
+AD4- +AD4- +- .driver+AF8-features +AD0- DRIVER+AF8-MODESET +AHw- DRIVER+AF8-GEM +AHw- DRIVER+AF8-PRIME +AHw-
+AD4- +AD4- +- +AKAAoACg-DRIVER+AF8-ATOMIC,
+AD4- +AD4- +- .preclose +AD0- arcpgu+AF8-preclose,
+AD4- +AD4- +- .lastclose +AD0- arcpgu+AF8-lastclose,
+AD4- +AD4- +- .name +AD0- +ACI-drm-arcpgu+ACI-,
+AD4- +AD4- +- .desc +AD0- +ACI-ARC PGU Controller+ACI-,
+AD4- +AD4- +- .date +AD0- +ACI-20160219+ACI-,
+AD4- +AD4- +- .major +AD0- 1,
+AD4- +AD4- +- .minor +AD0- 0,
+AD4- +AD4- +- .patchlevel +AD0- 0,
+AD4- +AD4- +- .fops +AD0- +ACY-arcpgu+AF8-drm+AF8-ops,
+AD4- +AD4- +- .load +AD0- arcpgu+AF8-load,
+AD4- +AD4- +- .unload +AD0- arcpgu+AF8-unload,
+AD4- Load and unload hooks are deprecated (it's a classic midlayer mistake).
+AD4- Please use drm+AF8-dev+AF8-alloc/register pairs directly instead, and put your
+AD4- device setup code in-between. Similar for unloading. There's a bunch of
+AD4- example drivers converted already.

Ok I took +ACI-atmel-hlcdc+ACI- as example.
And that's interesting.

If I put my+AKA-arcpgu+AF8-load() in between+AKA-drm+AF8-dev+AF8-alloc() and
drm+AF8-dev+AF8-register() then I'm getting this on the driver probe:
----------------------------------+AD4-8-------------------------------
+AFs-drm+AF0- Initialized drm 1.1.0 20060810
arcpgu e0017000.pgu: arc+AF8-pgu ID: 0xabbabaab
------------+AFs- cut here +AF0-------------
WARNING: CPU: 0 PID: 1 at lib/kobject.c:244 kobject+AF8-add+AF8-internal+-0x17c/0x498()
kobject+AF8-add+AF8-internal failed for card0-HDMI-A-1 (error: -2 parent: card0)
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.5.0-rc3-01062-ga447822-dirty +ACM-17

Stack Trace:
+AKA- arc+AF8-unwind+AF8-core.constprop.1+-0xa4/0x110
+AKA- warn+AF8-slowpath+AF8-fmt+-0x6e/0xfc
+AKA- kobject+AF8-add+AF8-internal+-0x17c/0x498
+AKA- kobject+AF8-add+-0x98/0xe4
+AKA- device+AF8-add+-0xc6/0x734
+AKA- device+AF8-create+AF8-with+AF8-groups+-0x12a/0x144
+AKA- drm+AF8-sysfs+AF8-connector+AF8-add+-0x54/0xe8
+AKA- arcpgu+AF8-drm+AF8-hdmi+AF8-init+-0xd4/0x17c
+AKA- arcpgu+AF8-probe+-0x138/0x24c
+AKA- platform+AF8-drv+AF8-probe+-0x2e/0x6c
+AKA- really+AF8-probe+-0x212/0x35c
+AKA- +AF8AXw-driver+AF8-attach+-0x90/0x94
+AKA- bus+AF8-for+AF8-each+AF8-dev+-0x46/0x80
+AKA- bus+AF8-add+AF8-driver+-0x14e/0x1b4
+AKA- driver+AF8-register+-0x64/0x108
+AKA- do+AF8-one+AF8-initcall+-0x86/0x194
+AKA- kernel+AF8-init+AF8-freeable+-0xf0/0x188
---+AFs- end trace c67166ad43ddcce2 +AF0----
+AFs-drm:drm+AF8-sysfs+AF8-connector+AF8-add+AF0- adding +ACI-HDMI-A-1+ACI- to sysfs
+AFs-drm:drm+AF8-sysfs+AF8-connector+AF8-add+AF0- +ACo-ERROR+ACo- failed to register connector device: -2
arcpgu e0017000.pgu: failed to regiter DRM connector and helper funcs
arcpgu: probe of e0017000.pgu failed with error -2
----------------------------------+AD4-8-------------------------------

But if I move arcpgu+AF8-load() after drm+AF8-dev+AF8-register() then everything
starts properly and I may see HDMI screen works perfectly fine.

Any thoughts?


+AD4- +AD4-
+AD4- +AD4- +- .dumb+AF8-create +AD0- drm+AF8-gem+AF8-cma+AF8-dumb+AF8-create,
+AD4- +AD4- +- .dumb+AF8-map+AF8-offset +AD0- drm+AF8-gem+AF8-cma+AF8-dumb+AF8-map+AF8-offset,
+AD4- +AD4- +- .dumb+AF8-destroy +AD0- drm+AF8-gem+AF8-dumb+AF8-destroy,
+AD4- +AD4- +- .get+AF8-vblank+AF8-counter +AD0- drm+AF8-vblank+AF8-no+AF8-hw+AF8-counter,
+AD4- +AD4- +- .prime+AF8-handle+AF8-to+AF8-fd +AD0- drm+AF8-gem+AF8-prime+AF8-handle+AF8-to+AF8-fd,
+AD4- +AD4- +- .prime+AF8-fd+AF8-to+AF8-handle +AD0- drm+AF8-gem+AF8-prime+AF8-fd+AF8-to+AF8-handle,
+AD4- +AD4- +- .gem+AF8-free+AF8-object +AD0- drm+AF8-gem+AF8-cma+AF8-free+AF8-object,
+AD4- +AD4- +- .gem+AF8-vm+AF8-ops +AD0- +ACY-drm+AF8-gem+AF8-cma+AF8-vm+AF8-ops,
+AD4- +AD4- +- .gem+AF8-prime+AF8-export +AD0- drm+AF8-gem+AF8-prime+AF8-export,
+AD4- +AD4- +- .gem+AF8-prime+AF8-import +AD0- drm+AF8-gem+AF8-prime+AF8-import,
+AD4- +AD4- +- .gem+AF8-prime+AF8-get+AF8-sg+AF8-table +AD0- drm+AF8-gem+AF8-cma+AF8-prime+AF8-get+AF8-sg+AF8-table,
+AD4- +AD4- +- .gem+AF8-prime+AF8-import+AF8-sg+AF8-table +AD0- drm+AF8-gem+AF8-cma+AF8-prime+AF8-import+AF8-sg+AF8-table,
+AD4- +AD4- +- .gem+AF8-prime+AF8-vmap +AD0- drm+AF8-gem+AF8-cma+AF8-prime+AF8-vmap,
+AD4- +AD4- +- .gem+AF8-prime+AF8-vunmap +AD0- drm+AF8-gem+AF8-cma+AF8-prime+AF8-vunmap,
+AD4- +AD4- +- .gem+AF8-prime+AF8-mmap +AD0- drm+AF8-gem+AF8-cma+AF8-prime+AF8-mmap,
+AD4- +AD4- +-+AH0AOw-
+AD4- +AD4- +-
+AD4- +AD4- +-static int arcpgu+AF8-probe(struct platform+AF8-device +ACo-pdev)
+AD4- +AD4- +-+AHs-
+AD4- +AD4- +- return drm+AF8-platform+AF8-init(+ACY-arcpgu+AF8-drm+AF8-driver, pdev)+ADs-
+AD4- ... or read the kerneldoc of this function, which also explains what you
+AD4- should do +ADs--)

Could you please point me to the relevant document?
I wasn't able to find anything related from the first glance :(

+AD4- +AD4- +-
+AD4- +AD4- +-/+ACo-
+AD4- +AD4- +- +ACo- This function is the only reason to have a copy of drm+AF8-fbdev+AF8-cma+AF8-init()
+AD4- +AD4- +- +ACo- here in this driver.
+AD4- +AD4- +- +ACo-
+AD4- +AD4- +- +ACo- In its turn this mmap() is required to mark user-space page as non-cached
+AD4- +AD4- +- +ACo- because it is just a mirror or real hardware frame-buffer.
+AD4- +AD4- +- +ACo-/
+AD4- +AD4- +-static int arcpgu+AF8-mmap(struct fb+AF8-info +ACo-info, struct vm+AF8-area+AF8-struct +ACo-vma)
+AD4- +AD4- +-+AHs-
+AD4- +AD4- +- vma-+AD4-vm+AF8-page+AF8-prot +AD0- pgprot+AF8-noncached(vma-+AD4-vm+AF8-page+AF8-prot)+ADs-
+AD4- +AD4- +- return vm+AF8-iomap+AF8-memory(vma, info-+AD4-fix.smem+AF8-start, info-+AD4-fix.smem+AF8-len)+ADs-
+AD4- +AD4- +-+AH0-
+AD4- This looks very fishy, no other drm driver even bothers with providing an
+AD4- fb+AF8-mmap hook. What exactly do you need this for? Assuming you've mmapped
+AD4- your fbcon drm+AF8-framebuffer correctly for kernel access things should just
+AD4- work ...

Indeed for kernel there's non need to that hack. Kernel deals directly with HW
frame-buffer area (that address we get from gem-+AD4-paddr). And so every byte written gets
picked up by PGU and is then rendered on the display.

But when user-space opens /dev/fb0 and mmaps() it deals with memory pages which
are by default (at least on ARC) marked as +ACI-cached+ACI-. I.e. user-space application
(I use that nice demo app+AKA-https://github.com/qtproject/qt/blob/4.8/examples/qws/framebuffer/main.c)
deals with frame-buffer via data cache. And that has 2 problems:
+AKAAWw-1+AF0- Since no explicit cache flush gets executed some data is left in data cache,
+AKA- +AKA- +AKA-i.e. some parts of the picture never reaches real PGU.
+AKA- +AKA- +AKA-See what happens on display -+AKA-http://imgur.com/iAbnnx3
+AKA- +AKA- +AKA-Those missing lines are exactly those 32-byte missing cache lines.
+AKAAWw-2+AF0- Even if we manage to flush data somehow massive amount of data that goes
+AKA- +AKA- +AKA-through data cache (let's sat 1080p+AEA-30Hz) will thrash it and as a result
+AKA- +AKA- +AKA-there will be no benefit for other cache users to use cache.

So we fix it simply marking pages mapped to user-space apps as uncached
that effectively routes all FB data directly to memry instead of polluting cache.

Hopefully that explanation makes sense.

-Alexey