Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752882AbcCRIML (ORCPT ); Fri, 18 Mar 2016 04:12:11 -0400 Received: from smtprelay.synopsys.com ([198.182.60.111]:57112 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750856AbcCRILz convert rfc822-to-8bit (ORCPT ); Fri, 18 Mar 2016 04:11:55 -0400 From: Alexey Brodkin To: "daniel@ffwll.ch" CC: "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "Jose.Abreu@synopsys.com" , "linux-snps-arc@lists.infradead.org" Subject: Re: [PATCH 1/4 v3] drm: Add support of ARC PGU display controller Thread-Topic: [PATCH 1/4 v3] drm: Add support of ARC PGU display controller Thread-Index: AQHRe6yu3aXUJ9z4NEywOEDLxFTdJ59Ydi2AgABHe4CAAV6igIAAeTUAgAAJwACAA29nAIAAwmaAgAACe4A= Date: Fri, 18 Mar 2016 08:11:49 +0000 Message-ID: <1458288709.12484.15.camel@synopsys.com> References: <1457710959-9562-1-git-send-email-abrodkin@synopsys.com> <1457710959-9562-2-git-send-email-abrodkin@synopsys.com> <20160314070009.GN14170@phenom.ffwll.local> <1457954159.3306.24.camel@synopsys.com> <20160315081056.GS14170@phenom.ffwll.local> <1458055486.4174.16.camel@synopsys.com> <20160315155940.GL14170@phenom.ffwll.local> <1458246430.3134.14.camel@synopsys.com> <20160318080257.GD14170@phenom.ffwll.local> In-Reply-To: <20160318080257.GD14170@phenom.ffwll.local> Accept-Language: en-US, ru-RU Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.121.8.125] Content-Type: text/plain; charset="utf-7" Content-ID: <708BB0E3995728438A13FDCF4B40FE33@internal.synopsys.com> Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9913 Lines: 137 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