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

[Sean Paul]

On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa <tomasz.figa at gmail.com> wrote:
> Hi,
>
> On Wednesday 23 of October 2013 12:09:06 Sean Paul wrote:
>> On Wed, Oct 23, 2013 at 11:53 AM, Dave Airlie <airlied at gmail.com> 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.

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 think we'll need to expand on the media bindings to achieve (1). I'm
not sure what the best way to tease out the standalone devices would
be.

Sean



> panel-a {
>         /* Single input port */
>         port {
>                 panel_a: endpoint at 0 {
>                         remote-endpoint = <&encoder_y_out>;
>                 };
>         };
> };
>
> bridge-a {
>         ports {
>                 /* Input port */
>                 port at 0 {
>                         bridge_a_in: endpoint at 0 {
>                                 remote-endpoint = <&encoder_x_out>;
>                         };
>                 };
>
>                 /*
>                  * Since it is a bridge, port at 1 should be probably
>                  * present here as well...
>                  */
>         };
> };
>
> encoder-x {
>         ports {
>                 /* Input port */
>                 port at 0 {
>                         encoder_x_in0: endpoint at 0 {
>                                 remote-endpoint = <&crtc_x>;
>                         };
>
>                         encoder_x_in1: endpoint at 1 {
>                                 remote-endpoint = <&crtc_y_out0>;
>                         };
>                 };
>
>                 /* Output port */
>                 port at 1 {
>                         encoder_x_out: endpoint at 0 {
>                                 remote-endpoint = <&bridge_a_in>;
>                         };
>                 };
>         };
> };
>
> encoder-y {
>         ports {
>                 /* Input port */
>                 port at 0 {
>                         encoder_y_in: endpoint at 0 {
>                                 remote-endpoint = <&encoder_y_in>;
>                         };
>                 };
>
>                 /* Output port */
>                 port at 1 {
>                         encoder_y_out: endpoint at 0 {
>                                 remote-endpoint = <&panel_a>;
>                         };
>                 };
>         };
> };
>
> crtc-x {
>         /* Single output port */
>         port {
>                 crtc_x: endpoint at 0 {
>                         remote-endpoint = <&encoder_x_in0>;
>                 };
>         };
> };
>
> crtc-y {
>         /* Single output port */
>         port {
>                 crtc_y_out0: endpoint at 0 {
>                         remote-endpoint = <&encoder_x_in1>;
>                 };
>
>                 crtc_y_out1: endpoint at 1 {
>                         remote-endpoint = <&encoder_y_in>;
>                 };
>         };
> };
>
> If I didn't make any typos, then the nodes above should not only represent
> fully the layout of your sample video pipelines, but also map connections
> between video entities with their physical endpoints, allowing respective
> drivers to properly configure any muxes based purely on DT data. Of course
> this also includes dependencies between all display entities.
>
> [1] Documentation/devicetree/bindings/media/video-interfaces.txt
>
> Best regards,
> Tomasz
>