Re: [PATCH 1/4 v3] drm: Add support of ARC PGU display controller
From: Alexey Brodkin
Date: Fri Mar 18 2016 - 04:12:15 EST
Hi Daniel,
On Fri, 2016-03-18 at 09:02 +-0100, Daniel Vetter wrote:
+AD4- On Thu, Mar 17, 2016 at 08:27:10PM +-0000, Alexey Brodkin wrote:
+AD4- +AD4-
+AD4- +AD4- Hi Daniel,
+AD4- +AD4-
+AD4- +AD4- On Tue, 2016-03-15 at 16:59 +-0100, Daniel Vetter wrote:
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- On Tue, Mar 15, 2016 at 03:24:46PM +-0000, Alexey Brodkin wrote:
+AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- On Tue, 2016-03-15 at 09:10 +-0100, Daniel Vetter wrote:
+AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- On Mon, Mar 14, 2016 at 11:15:59AM +-0000, Alexey Brodkin wrote:
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- On Mon, 2016-03-14 at 08:00 +-0100, Daniel Vetter wrote:
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- On Fri, Mar 11, 2016 at 06:42:36PM +-0300, Alexey Brodkin wrote:
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA-
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +-static struct drm+AF8-driver arcpgu+AF8-drm+AF8-driver +AD0- +AHs-
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .driver+AF8-features +AD0- DRIVER+AF8-MODESET +AHw- DRIVER+AF8-GEM +AHw- DRIVER+AF8-PRIME +AHw-
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- +AKAAoACg-DRIVER+AF8-ATOMIC,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .preclose +AD0- arcpgu+AF8-preclose,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .lastclose +AD0- arcpgu+AF8-lastclose,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .name +AD0- +ACI-drm-arcpgu+ACI-,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .desc +AD0- +ACI-ARC PGU Controller+ACI-,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .date +AD0- +ACI-20160219+ACI-,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .major +AD0- 1,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .minor +AD0- 0,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .patchlevel +AD0- 0,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .fops +AD0- +ACY-arcpgu+AF8-drm+AF8-ops,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .load +AD0- arcpgu+AF8-load,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +- .unload +AD0- arcpgu+AF8-unload,
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Load and unload hooks are deprecated (it's a classic midlayer mistake).
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Please use drm+AF8-dev+AF8-alloc/register pairs directly instead, and put your
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- device setup code in-between. Similar for unloading. There's a bunch of
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AD4- example drivers converted already.
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Ok I took +ACI-atmel-hlcdc+ACI- as example.
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- And that's interesting.
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- If I put my+AKA-arcpgu+AF8-load() in between+AKA-drm+AF8-dev+AF8-alloc() and
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- drm+AF8-dev+AF8-register() then I'm getting this on the driver probe:
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- ----------------------------------+AD4-8-------------------------------
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AFs-drm+AF0- Initialized drm 1.1.0 20060810
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- arcpgu e0017000.pgu: arc+AF8-pgu ID: 0xabbabaab
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- ------------+AFs- cut here +AF0-------------
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- WARNING: CPU: 0 PID: 1 at lib/kobject.c:244 kobject+AF8-add+AF8-internal+-0x17c/0x498()
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- kobject+AF8-add+AF8-internal failed for card0-HDMI-A-1 (error: -2 parent: card0)
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Modules linked in:
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- CPU: 0 PID: 1 Comm: swapper Not tainted 4.5.0-rc3-01062-ga447822-dirty +ACM-17
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Stack Trace:
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- arc+AF8-unwind+AF8-core.constprop.1+-0xa4/0x110
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- warn+AF8-slowpath+AF8-fmt+-0x6e/0xfc
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- kobject+AF8-add+AF8-internal+-0x17c/0x498
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- kobject+AF8-add+-0x98/0xe4
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- device+AF8-add+-0xc6/0x734
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- device+AF8-create+AF8-with+AF8-groups+-0x12a/0x144
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- drm+AF8-sysfs+AF8-connector+AF8-add+-0x54/0xe8
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- arcpgu+AF8-drm+AF8-hdmi+AF8-init+-0xd4/0x17c
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- arcpgu+AF8-probe+-0x138/0x24c
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- platform+AF8-drv+AF8-probe+-0x2e/0x6c
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- really+AF8-probe+-0x212/0x35c
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- +AF8AXw-driver+AF8-attach+-0x90/0x94
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- bus+AF8-for+AF8-each+AF8-dev+-0x46/0x80
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- bus+AF8-add+AF8-driver+-0x14e/0x1b4
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- driver+AF8-register+-0x64/0x108
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- do+AF8-one+AF8-initcall+-0x86/0x194
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AKA- kernel+AF8-init+AF8-freeable+-0xf0/0x188
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- ---+AFs- end trace c67166ad43ddcce2 +AF0----
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AFs-drm:drm+AF8-sysfs+AF8-connector+AF8-add+AF0- adding +ACI-HDMI-A-1+ACI- to sysfs
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- +AFs-drm:drm+AF8-sysfs+AF8-connector+AF8-add+AF0- +ACo-ERROR+ACo- failed to register connector device: -2
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- arcpgu e0017000.pgu: failed to regiter DRM connector and helper funcs
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- arcpgu: probe of e0017000.pgu failed with error -2
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- ----------------------------------+AD4-8-------------------------------
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- But if I move arcpgu+AF8-load() after drm+AF8-dev+AF8-register() then everything
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- starts properly and I may see HDMI screen works perfectly fine.
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- +AD4- Any thoughts?
+AD4- +AD4- +AD4- +AD4- +AD4- Oops, yeah missed that detail. If you look at atmel it has a loop to
+AD4- +AD4- +AD4- +AD4- +AD4- register all the drm connectors +AF8-after+AF8- calling drm+AF8-dev+AF8-register().
+AD4- +AD4- +AD4- +AD4- +AD4- Totally forgot about that. Can you pls
+AD4- +AD4- +AD4- +AD4- +AD4- - Extract a new drm+AF8-connector+AF8-register+AF8-all() function
+AD4- +AD4- +AD4- +AD4- +AD4- +AKA- (atmel+AF8-hlcdc+AF8-dc+AF8-connector+AF8-plug+AF8-all seems to be the best template),
+AD4- +AD4- +AD4- +AD4- +AD4- +AKA- including kerneldoc.
+AD4- +AD4- +AD4- +AD4- +AD4- - Adjust kerneldoc of drm+AF8-dev+AF8-register() to mention
+AD4- +AD4- +AD4- +AD4- +AD4- +AKA- drm+AF8-connector+AF8-register+AF8-all() and that ordering constraint.
+AD4- +AD4- +AD4- +AD4- +AD4- - Roll that helper out to all the drivers that currently hand-roll it (one
+AD4- +AD4- +AD4- +AD4- +AD4- +AKA- patch per driver).
+AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- I know a bit of work but imo not too much, and by doing some small
+AD4- +AD4- +AD4- +AD4- +AD4- refactoring every time someone stumbles over a drm pitfall we keep the
+AD4- +AD4- +AD4- +AD4- +AD4- subsystem clean+ACY-easy to understand. You're up for this? Would be a prep
+AD4- +AD4- +AD4- +AD4- +AD4- series, I'll happily review it to get it merged fast. Just a few weeks ago
+AD4- +AD4- +AD4- +AD4- +AD4- I merged 20+- patches to make -+AD4-mode+AF8-fixup hooks optional and remove dummy
+AD4- +AD4- +AD4- +AD4- +AD4- ones all over the subsystem, in other words: You'll have my full attention
+AD4- +AD4- +AD4- +AD4- +AD4- +ADs--)
+AD4- +AD4- +AD4- +AD4- Sure, I'm ready to pay that price :)
+AD4- +AD4- +AD4- +AD4- Stay tuned and patches will follow.
+AD4- +AD4- +AD4- Awesome, looking forward to your patches.
+AD4- +AD4- Sorry it took longer for me to finally put my hands on that work but anyways.
+AD4- +AD4-
+AD4- +AD4- I'm looking now at how drivers use existing+AKA-drm+AF8-connector+AF8-unplug+AF8-all() and
+AD4- +AD4- their implementation of what would be+AKA-drm+AF8-connector+AF8-plug+AF8-all() and see
+AD4- +AD4- in some implementations people wraps both helpers with
+AD4- +AD4- mutex+AF8Aew-lock+AHw-unlock+AH0-(+ACY-dev-+AD4-mode+AF8-config.mutex). But not everybody does this.
+AD4- +AD4-
+AD4- +AD4- So essentially my questions are:
+AD4- +AD4- +AKAAWw-1+AF0- If it's necessary to get hold of that mutex before execution of either helper?
+AD4- In plug+AF8-all I think so, unplug+AF8-all has a FIXME comment about how locking
+AD4- against sysfs is horrible and it's all going to blow up. But we did
+AD4- recently change the connector sysfs files, so maybe that's fixed now. Not
+AD4- sure.
+AD4-
+AD4- +AD4- +AKAAWw-2+AF0- If this is really necessary then IMHO it makes sense to move mutex+AF8-lock/unlock
+AD4- +AD4- +AKA- +AKA- +AKA-in helpers itself, right?
+AD4- Yeah, locking in the helper makes imo sense.
Hm, now I'm even more confused :)
Above you're not sure if this locking is safe (at least on unplug) but here you say
it makes sense (not mentioning the case - both helpers or only on +ACI-plugging+ACI-).
I may indeed try to embed mutex locks in both helpers but what if it works for me but
others will get broken stuff?
+AD4- Aside: I'd vote for
+AD4- register+AF8-all/unregister+AF8-all for consistency with drm+AF8-connector+AF8-register.
+AD4- register/unregister has a very clear meaning of +ACI-publish the object to
+AD4- userspace/other kernel subsystems+ACI-. plug/unplug is confusing in DRM
+AD4- because of hotplug.
Well but what should happen to existing unplug() then?
Should I rename it to unregister+AF8-all() in the same change?
-Alexey