public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
To: "daniel@ffwll.ch" <daniel@ffwll.ch>
Cc: "dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Jose.Abreu@synopsys.com" <Jose.Abreu@synopsys.com>,
	"linux-snps-arc@lists.infradead.org" 
	<linux-snps-arc@lists.infradead.org>
Subject: Re: [PATCH 1/4 v3] drm: Add support of ARC PGU display controller
Date: Thu, 17 Mar 2016 20:27:10 +0000	[thread overview]
Message-ID: <1458246430.3134.14.camel@synopsys.com> (raw)
In-Reply-To: <20160315155940.GL14170@phenom.ffwll.local>

Hi Daniel,

On Tue, 2016-03-15 at 16:59 +0100, Daniel Vetter wrote:
> On Tue, Mar 15, 2016 at 03:24:46PM +0000, Alexey Brodkin wrote:
> > On Tue, 2016-03-15 at 09:10 +0100, Daniel Vetter wrote:
> > > On Mon, Mar 14, 2016 at 11:15:59AM +0000, Alexey Brodkin wrote:
> > > > On Mon, 2016-03-14 at 08:00 +0100, Daniel Vetter wrote:
> > > > > On Fri, Mar 11, 2016 at 06:42:36PM +0300, Alexey Brodkin wrote:
> > > > > > 
> > > > > > +static struct drm_driver arcpgu_drm_driver = {
> > > > > > +	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
> > > > > > +			   DRIVER_ATOMIC,
> > > > > > +	.preclose = arcpgu_preclose,
> > > > > > +	.lastclose = arcpgu_lastclose,
> > > > > > +	.name = "drm-arcpgu",
> > > > > > +	.desc = "ARC PGU Controller",
> > > > > > +	.date = "20160219",
> > > > > > +	.major = 1,
> > > > > > +	.minor = 0,
> > > > > > +	.patchlevel = 0,
> > > > > > +	.fops = &arcpgu_drm_ops,
> > > > > > +	.load = arcpgu_load,
> > > > > > +	.unload = arcpgu_unload,
> > > > > Load and unload hooks are deprecated (it's a classic midlayer mistake).
> > > > > Please use drm_dev_alloc/register pairs directly instead, and put your
> > > > > device setup code in-between. Similar for unloading. There's a bunch of
> > > > > example drivers converted already.
> > > > Ok I took "atmel-hlcdc" as example.
> > > > And that's interesting.
> > > > 
> > > > If I put my arcpgu_load() in between drm_dev_alloc() and
> > > > drm_dev_register() then I'm getting this on the driver probe:
> > > > ---------------------------------->8-------------------------------
> > > > [drm] Initialized drm 1.1.0 20060810
> > > > arcpgu e0017000.pgu: arc_pgu ID: 0xabbabaab
> > > > ------------[ cut here ]------------
> > > > WARNING: CPU: 0 PID: 1 at lib/kobject.c:244 kobject_add_internal+0x17c/0x498()
> > > > kobject_add_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 #17
> > > > 
> > > > Stack Trace:
> > > >   arc_unwind_core.constprop.1+0xa4/0x110
> > > >   warn_slowpath_fmt+0x6e/0xfc
> > > >   kobject_add_internal+0x17c/0x498
> > > >   kobject_add+0x98/0xe4
> > > >   device_add+0xc6/0x734
> > > >   device_create_with_groups+0x12a/0x144
> > > >   drm_sysfs_connector_add+0x54/0xe8
> > > >   arcpgu_drm_hdmi_init+0xd4/0x17c
> > > >   arcpgu_probe+0x138/0x24c
> > > >   platform_drv_probe+0x2e/0x6c
> > > >   really_probe+0x212/0x35c
> > > >   __driver_attach+0x90/0x94
> > > >   bus_for_each_dev+0x46/0x80
> > > >   bus_add_driver+0x14e/0x1b4
> > > >   driver_register+0x64/0x108
> > > >   do_one_initcall+0x86/0x194
> > > >   kernel_init_freeable+0xf0/0x188
> > > > ---[ end trace c67166ad43ddcce2 ]---
> > > > [drm:drm_sysfs_connector_add] adding "HDMI-A-1" to sysfs
> > > > [drm:drm_sysfs_connector_add] *ERROR* 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
> > > > ---------------------------------->8-------------------------------
> > > > 
> > > > But if I move arcpgu_load() after drm_dev_register() then everything
> > > > starts properly and I may see HDMI screen works perfectly fine.
> > > > 
> > > > Any thoughts?
> > > Oops, yeah missed that detail. If you look at atmel it has a loop to
> > > register all the drm connectors _after_ calling drm_dev_register().
> > > Totally forgot about that. Can you pls
> > > - Extract a new drm_connector_register_all() function
> > >   (atmel_hlcdc_dc_connector_plug_all seems to be the best template),
> > >   including kerneldoc.
> > > - Adjust kerneldoc of drm_dev_register() to mention
> > >   drm_connector_register_all() and that ordering constraint.
> > > - Roll that helper out to all the drivers that currently hand-roll it (one
> > >   patch per driver).
> > > 
> > > I know a bit of work but imo not too much, and by doing some small
> > > refactoring every time someone stumbles over a drm pitfall we keep the
> > > subsystem clean&easy to understand. You're up for this? Would be a prep
> > > series, I'll happily review it to get it merged fast. Just a few weeks ago
> > > I merged 20+ patches to make ->mode_fixup hooks optional and remove dummy
> > > ones all over the subsystem, in other words: You'll have my full attention
> > > ;-)
> > Sure, I'm ready to pay that price :)
> > Stay tuned and patches will follow.
> Awesome, looking forward to your patches.

Sorry it took longer for me to finally put my hands on that work but anyways.

I'm looking now at how drivers use existing drm_connector_unplug_all() and
their implementation of what would be drm_connector_plug_all() and see
in some implementations people wraps both helpers with
mutex_{lock|unlock}(&dev->mode_config.mutex). But not everybody does this.

So essentially my questions are:
 [1] If it's necessary to get hold of that mutex before execution of either helper?
 [2] If this is really necessary then IMHO it makes sense to move mutex_lock/unlock
     in helpers itself, right?

-Alexey

  reply	other threads:[~2016-03-17 20:27 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-11 15:42 [PATCH 0/4 v3] drm: Add support of ARC PGU display controller Alexey Brodkin
2016-03-11 15:42 ` [PATCH 1/4 " Alexey Brodkin
2016-03-11 16:45   ` kbuild test robot
2016-03-11 16:45   ` [PATCH] drm: fix platform_no_drv_owner.cocci warnings kbuild test robot
2016-03-14  7:00   ` [PATCH 1/4 v3] drm: Add support of ARC PGU display controller Daniel Vetter
2016-03-14 11:15     ` Alexey Brodkin
2016-03-15  8:10       ` Daniel Vetter
2016-03-15 15:24         ` Alexey Brodkin
2016-03-15 15:59           ` Daniel Vetter
2016-03-17 20:27             ` Alexey Brodkin [this message]
2016-03-18  8:02               ` Daniel Vetter
2016-03-18  8:11                 ` Alexey Brodkin
2016-03-18 17:23                   ` Daniel Vetter
2016-03-11 15:42 ` [PATCH 2/4 v3] drm: Add DT bindings documentation for " Alexey Brodkin
2016-03-21 13:04   ` Rob Herring
2016-03-24 14:57     ` Alexey Brodkin
2016-03-11 15:42 ` [PATCH 3/4 v3] arc: axs10x - add support of ARC PGU Alexey Brodkin
2016-03-11 15:42 ` [PATCH 4/4 v3] MAINTAINERS: Add maintainer for ARC PGU display controller Alexey Brodkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1458246430.3134.14.camel@synopsys.com \
    --to=alexey.brodkin@synopsys.com \
    --cc=Jose.Abreu@synopsys.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-snps-arc@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox