Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

On Tuesday 29 October 2013 17:29:55 Rob Clark wrote:
> On Tue, Oct 29, 2013 at 4:50 PM, Tomasz Figa wrote:
> > On Tuesday 29 of October 2013 16:36:47 Sean Paul wrote:
> > > On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa wrote:
> >> > On Wednesday 23 of October 2013 12:09:06 Sean Paul wrote:
> >> >> On Wed, Oct 23, 2013 at 11:53 AM, Dave Airlie wrote:
> >> >> >>>>> I think we need to start considering a framework where
> >> >> >>>>> subdrivers just add drm objects themselves, then the toplevel
> >> >> >>>>> node is responsible for knowing that everything for the current
> >> >> >>>>> configuration is loaded.
> >> >> >>>> 
> >> >> >>>> It would be nice to specify the various pieces in dt, then have
> >> >> >>>> some type of drm notifier to the toplevel node when everything
> >> >> >>>> has been probed. Doing it in the dt would allow standalone
> >> >> >>>> drm_bridge/drm_panel drivers to be transparent as far as the
> >> >> >>>> device's drm driver is concerned.
> >> >> >>>> 
> >> >> >>>> Sean
> >> >> >>>> 
> >> >> >>>>> I realise we may need to make changes to the core drm to allow
> >> >> >>>>> this but we should probably start to create a strategy for
> >> >> >>>>> fixing the API issues that this throws up.
> >> >> >>>>> 
> >> >> >>>>> Note I'm not yet advocating for dynamic addition of nodes once
> >> >> >>>>> the device is in use, or removing them.
> >> >> >>> 
> >> >> >>> I do wonder if we had some sort of tag in the device tree for any
> >> >> >>> nodes involved in the display, and the core drm layer would read
> >> >> >>> that list, and when every driver registers tick things off, and
> >> >> >>> when the last one joins we get a callback and init the drm layer,
> >> >> >>> we'd of course have the basic drm layer setup prior to that so we
> >> >> >>> can add the objects as the drivers load. It might make development
> >> >> >>> a bit trickier as you'd need to make sure someone claimed
> >> >> >>> ownership of all the bits for init to proceed.
> >> >> >> 
> >> >> >> Yeah, that's basically what the strawman looked like in my head.
> >> >> >> 
> >> >> >> Instead of a property in each node, I was thinking of having a
> >> >> >> separate gfx pipe nodes that would have dt pointers to the various
> >> >> >> pieces involved in that pipe. This would allow us to associate
> >> >> >> standalone entities like bridges and panels with encoders in dt
> >> >> >> w/o doing it in the drm code. I *think* this should be Ok with the
> >> >> >> dt guys since it is still describing the hardware, but I think we'd
> >> >> >> have to make sure it wasn't drm-specific.
> >> >> > 
> >> >> > I suppose the question is how much dynamic pipeline construction
> >> >> > there is, even on things like radeon and i915 we have dynamic clock
> >> >> > generator to crtc to encoder setups, so I worry about static lists
> >> >> > per-pipe, so I still think just stating all these devices are needed
> >> >> > for display and a list of valid interconnections between them, then
> >> >> > we can have the generic code model drm crtc/encoders/connectors on
> >> >> > that list, and construct the possible_crtcs /possible_clones etc at
> >> >> > that stage.
> >> >> 
> >> >> I'm, without excuse, hopeless at devicetree, so there are probably
> >> >> some violations, but something like:
> >> >> 
> >> >> display-pipelines {
> >> >>   required-elements = <&bridge-a &panel-a &encoder-x &encoder-y
> >> >> &crtc-x &crtc-y>;
> >> >> 
> >> >>   pipe1 {
> >> >>     bridge = <&bridge-a>;
> >> >>     encoder = <&encoder-x>;
> >> >>     crtc = <&crtc-y>;
> >> >>   };
> >> >>   pipe2 {
> >> >>     encoder = <&encoder-x>;
> >> >>     crtc = <&crtc-x>;
> >> >>   };
> >> >>   pipe3 {
> >> >>     panel = <&panel-a>;
> >> >>     encoder = <&encoder-y>;
> >> >>     crtc = <&crtc-y>;
> >> >>   };
> >> >> };
> >> >> 
> >> >> I'm tempted to add connector to the pipe nodes as well, so it's
> >> >> obvious which connector should be used in cases where multiple
> >> >> entities in the pipe implement drm_connector. However, I'm not sure
> >> >> if that would be NACKed by dt people.
> >> >> 
> >> >> I'm also not sure if there are too many combinations for i915 and
> >> >> radeon to make this unreasonable. I suppose those devices could just
> >> >> use required-elements and leave the pipe nodes out.
> >> > 
> >> > Just to put my two cents in, as one of the people involved into "the
> >> > device tree movement", I'd say that instead of creating artifical
> >> > entities, such as display-pipelines and all of the pipeX'es, device
> >> > tree should represent relations between nodes.
> >> > 
> >> > According to the generic DT bindings we already have for
> >> > video-interfaces
> >> 
> >> > [1] your example connection layout would look as follows:
> >> Hi Tomasz
> >> Thanks for sending this along.
> >> 
> >> I think the general consensus is that each drm driver should be
> >> implemented as a singular driver. That is, N:1 binding to driver
> >> mapping, where there are N IP blocks. Optional devices (such as
> >> bridges, panels) probably make sense to spin off as standalone
> >> drivers.
> > 
> > I believe this is a huge step backwards from current kernel design
> > standards, which prefer modularity.

I'd like to adopt a middle ground position on this matter.

Without a doubt external I2C devices will have their own device tree node, 
separate from the display controller node(s). I believe we've settled on that.

At the other extreme, it makes no sense to handle IP cores that are tightly 
coupled with separate device instances (and possibly separate drivers). A 
display controller with a CRTC that includes a scaler that shares many 
configuration registers with the scanout engine shouldn't be created from two 
devices.

What's left to debate is everything in-between. I believe than an on-SoC HDMI 
encoder that is completely decoupled from the display controller (different 
reigster space, different clocks, different power domain, ...) should be 
modeled as a separate device, especially if that IP core is used in other SoCs 
with different display controllers. I'm also ready to ultimately accept the 
driver author's decision to tightly couple the two drivers. We can probably 
come up with several guidelines to help taking decisions, but there will be no 
strict one-size-fits-them-all rule we can apply here, especially before 
gaining experience from applying the model to several drivers. I'm not too 
worried, I believe we'll be able to take the right path along the way.

> But it makes things behave in the way that userspace expects, which is
> more important.
> 
> > Having multiple IPs being part of the DRM subsystem in a SoC, it would be
> > nice to have the possibility to compile just a subset of support for them
> > into the kernel and load rest of them as modules. (e.g. basic LCD
> > controller on a mobile phone compiled in and external connectors, like
> > HDMI as modules)
> 
> I think I've mentioned before, that userspace is not prepared to deal
> w/ crtc/encoder/connector's dynamically appearing/disappearing.
> Perhaps there are ways to improve that, but I haven't come across
> hardware where the hdmi block can actually be dynamically removed.

Userspace will need to prepare for that in the longer term, as we'll likely 
see more needs for highly dynamic pipelines (one such use case is partial 
reconfiguration in FPGA: albeit marginal today, we can't just ignore it in the 
long term). I don't want to solve this problem now though (even CDF made no 
attempt at doing so ;-)).

> > Not even saying that from development perspective, a huge single driver
> > would be much more difficult to test and debug, than several smaller
> > drivers, which could be developed separately.
> > 
> > Unless there is a misunderstanding here, I think this is broken.
> > 
> >> An example: exynos_drm_drv would be a platform_driver which implements
> >> drm_driver. On drm_load, it would enumerate the various dt nodes for
> >> its IP blocks and initialize them with direct calls (like
> >> exynos_drm_fimd_initialize). If the board uses a bridge (say for
> >> eDP->LVDS), that bridge driver would be a real driver with its own
> >> probe.
> >> 
> >> I think the ideal situation would be for the drm layer to manage the
> >> standalone drivers in a way that is transparent to the main driver,
> >> such that it doesn't need to know which type of hardware can hang off
> >> it. It will need to know if one exists since it might need to forego
> >> creating a connector, but it need not know anything else about it.
> >> 
> >> To accomplish this, I think we need:
> >>
> >> (1) Some way for drm to enumerate the standalone drivers, so it can
> >> know when all of them have been probed
> >> 
> >> (2) A drm registration function that's called by the standalone
> >> drivers once they're probed, and a hook with drm_device pointer called
> >> during drm_load for them to register their drm_* implementations
> >> 
> >> (3) Something that will allow for deferred probe if the main driver
> >> kicks off before the standalones are in, it would need to be called
> >> before drm_platform/pci_init

I believe we should defer probing of the sub-drivers only, as we could run 
into a circular dependency issue if we defer probing on all sides. The main 
driver should use a notification mechanism instead. This idea has been 
discussed during the kernel summit and seemed to not have caused a strong 
disagreement.

I will give this a try, given that I already have a working implementation as 
part of my CDF RFC. I "just" need to extract it as a standalone change (BTW, I 
wish people would have read the CDF RFC in a bit more details, as it contains 
ideas that have been proposed on the list by other developers during the last 
couple of weeks. It's both saddening and slightly offending to post ideas that 
get ignored because of disagreements on the big picture and see them proposed 
as brand new later on).

> >> I think we'll need to expand on the media bindings to achieve (1).
> > 
> > Could you elaborate on why you think so?
> > 
> > I believe the video interface bindings contain everything needed for this
> > case, except, of course, some device/bus specific parts, but those are to
> > be defined by separate device/bus specific bindings.

-- 
Regards,

Laurent Pinchart