Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

From: Daniel Vetter <daniel@ffwll.ch>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: linux-samsung-soc <linux-samsung-soc@vger.kernel.org>,
	Jingoo Han <jg1.han@samsung.com>, sunil joshi <joshi@samsung.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Ajay kumar <ajaynumb@gmail.com>,
	Thierry Reding <treding@nvidia.com>,
	Prashanth G <prashanth.g@samsung.com>,
	Ajay Kumar <ajaykumar.rs@samsung.com>
Subject: Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
Date: Fri, 31 Oct 2014 16:59:40 +0100	[thread overview]
Message-ID: <20141031155940.GS26941@phenom.ffwll.local> (raw)
In-Reply-To: <20141029091648.GC27900@ulmo>

On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:
> On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
> > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@chromium.org> wrote:
> > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > >>>   * @driver_private: pointer to the bridge driver's internal context
> > > > >>>   */
> > > > >>>  struct drm_bridge {
> > > > >>> -     struct drm_device *dev;
> > > > >>> +     struct device *dev;
> > > > >>
> > > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > > > >> drm structures still call it ->dev. Also, can't we use struct device_node
> > > > >> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > >>
> > > > >
> > > > > I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > > seems reasonable to keep the device_node instead.
> > > > 
> > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > drm_crtc drop the struct device and go directly to a struct
> > > > device_node. Since we don't really need the sturct device, the only
> > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > Should be all fairly simple to pull off with cocci.
> > > > 
> > > > Thierry?
> > > 
> > > The struct device * is in DRM panel because there's nothing device tree
> > > specific about the concept. Having a struct device_node * instead would
> > > indicate that it can only be used with a device tree, whereas the
> > > framework doesn't care the tiniest bit what type of device we have.
> > > 
> > > While the trend clearly is to use more device tree, I don't think we
> > > should make it impossible for anybody else to use these frameworks.
> > > 
> > > There are other advantages to keeping a struct device *, like having
> > > access to the proper device and its name. Also you get access to the
> > > device_node * via dev->of_node anyway. I don't see any advantage in
> > > switching to just a struct device_node *, only disadvantages.
> > 
> > Well the idea is to make the lookup key specific, and conditional on
> > #CONFIG_OF. If there's going to be another neat way to enumerate platform
> > devices then I think we should add that, too. Or maybe we should have a
> > void *platform_data or so.
> > 
> > The reason I really don't want a struct device * in core drm structures is
> > that two releases down the road people will have found tons of really
> > great ways to abuse them and re-create a midlayer. DRM core really should
> > only care about the sw objects and not be hw specific at all. Heck there's
> > not even an requirement to have any piece of actual hw, you could write a
> > completely fake drm driver (for e.g. testing like the new v4l driver).
> > 
> > Tbh I wonder a bit why we even have this registery embedded into the core
> > drm objects. Essentially the only thing you're doing is a list that maps
> > some platform specific key onto some subsystem specific driver structure
> > or fails the lookup. So instead of putting all these low-level details
> > into drm core structures can't we just have a generic hashtable/list for
> > this, plus some static inline helpers that cast the void * you get into
> > the one you want?
> > 
> > I also get the feeling that this really should be in the driver core (like
> > the component helpers), and that we should think a lot harder about
> > lifetimes and refcounting (see my other reply on that).
> 
> Yes, that sounds very useful indeed. Also see my reply to yours. =)

Just replying here with some of the irc discussions we've had. Since
drm_bridge/panel isn't a core drm interface object exposed to userspace
it's less critical. I still think that wasting a few brain cycles to have
a clear separation between the abstract interface object and how to bind
and unbind the pieces together is worthwhile, even though the cost when
getting it wrong is much less severe than in the case of a mandatory piece
of core infrastructure.

I think in general the recent armsoc motivated drm infrastructure lacks a
bit in attention to these details. E.g. the cma helpers are built into the
drm.ko module, but clearly are auxilliary library code. So they should be
pulled out and the headers clean up, so that we have a clear separation
between core and helpers. Otherwise someone will sooner or later screw up
and insert a helper depency into the core, and then we've started with the
midlayer mess. Same goes with drm_bridge/panel, which didn't even bother
to have separate headers from the core modeset header drm_crtc.h.

So would be great if someone could invest a bit of time into cleaning this
up. Writing proper api docs also helps a lot with achieving a clean and
sensible split ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel