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

[Inki Dae]

> -Original Message-
> From: Sean Paul [mailto:seanpaul at chromium.org]
> Sent: Tuesday, November 05, 2013 12:44 AM
> To: Inki Dae
> Cc: Thierry Reding; Laurent Pinchart; dri-devel; Sylwester Nawrocki;
> St?phane Marchesin
> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> 
> On Mon, Nov 4, 2013 at 6:30 AM, Inki Dae  wrote:
> > 2013/11/4 Thierry Reding :
> >> On Tue, Oct 29, 2013 at 08:46:03PM -0700, St?phane Marchesin wrote:
> >>> On Tue, Oct 29, 2013 at 1:50 PM, Tomasz Figa 
> wrote:
> >>>
> >>> > Hi Sean,
> >>> >
> >>> > On Tuesday 29 of October 2013 16:36:47 Sean Paul wrote:
> >>> > > On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa
> 
> >>> > 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
> 
> >>> > 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
> >>> > > >> >&

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

[Inki Dae]

2013/11/4 Thierry Reding :
> On Tue, Oct 29, 2013 at 08:46:03PM -0700, St?phane Marchesin wrote:
>> On Tue, Oct 29, 2013 at 1:50 PM, Tomasz Figa  
>> wrote:
>>
>> > Hi Sean,
>> >
>> > On Tuesday 29 of October 2013 16:36:47 Sean Paul wrote:
>> > > On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa 
>> > 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 
>> > 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
>> 

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

[Daniel Vetter]

On Mon, Nov 04, 2013 at 01:52:33PM +0100, Thierry Reding wrote:
> On Mon, Nov 04, 2013 at 07:14:27AM -0500, Rob Clark wrote:
> > On Mon, Nov 4, 2013 at 5:10 AM, Thierry Reding  > gmail.com> wrote:
> > > On Tue, Oct 29, 2013 at 05:29:55PM -0400, Rob Clark wrote:
> > >> On Tue, Oct 29, 2013 at 4:50 PM, Tomasz Figa  
> > >> wrote:
> > > [...]
> > >> > I believe this is a huge step backwards from current kernel design
> > >> > standards, which prefer modularity.
> > >>
> > >> But it makes things behave in the way that userspace expects, which is
> > >> more important.
> > >
> > > Why would userspace care about the modularity of kernel drivers? The
> > > only thing that userspace should care about is whether there's a DRM
> > > device or not. How the kernel makes that happen should be completely
> > > irrelevant to userspace.
> > 
> > What I was referring to was userspace not expecting parts of the drm
> > (crtcs/encoders/connectors) driver to show up incrementally. You can
> > avoid that, but it is more of a hassle currently (ie. most drivers
> > that need to do this, including a few that I've written, end up
> > needing some form of
> > stuff-devices-in-global-variables-that-main-driver-checks-for).
> 
> I must have misunderstood then. I don't think adding hotplug of DRM
> subdevices is something we would want. And I don't think there's a
> requirement for that, either. Embedded devices usually have well-defined
> use-cases, so the configuration is rather static at runtime.
> 
> As for the global variables, you can do it properly. Granted, it might
> be more work than global variables, but keeping drivers separated does
> have advantages. Especially when the devices have completely separated
> register ranges or clocks or other resources, it's very natural to use
> one driver per device and glue them together with a composite device
> construct.

I'm pretty short of ripping out all the midlayer disasters in the drm
driver load path. As soon as that's done drivers can defer probing and do
all kinds of trick until everything is set up, and then as the very last
step register the drm device. Current wip stuff is at

http://cgit.freedesktop.org/~danvet/drm/log/?h=drm-init-cleanup

Leftover todo items:
- rip out bus->set_busid and block the setversion ioctl from doing stupid
  things on kms drivers. That's the last nail on the drm_bus coffin.
- Convert udl over to embedding drm_device and driver-controlled setup
  sequence and rip out drm_dev->usbdevice as a proof of concept. That
  should be the death-spell to drm_usb.c

I'll let you arm guys figure out how to the same for drm_platform.c ;-) On
a quick look there's only very few drivers that use
drm_dev->platform_device, which is the last piece to kill really.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Thierry Reding]

On Mon, Nov 04, 2013 at 07:14:27AM -0500, Rob Clark wrote:
> On Mon, Nov 4, 2013 at 5:10 AM, Thierry Reding  
> wrote:
> > On Tue, Oct 29, 2013 at 05:29:55PM -0400, Rob Clark wrote:
> >> On Tue, Oct 29, 2013 at 4:50 PM, Tomasz Figa  
> >> wrote:
> > [...]
> >> > I believe this is a huge step backwards from current kernel design
> >> > standards, which prefer modularity.
> >>
> >> But it makes things behave in the way that userspace expects, which is
> >> more important.
> >
> > Why would userspace care about the modularity of kernel drivers? The
> > only thing that userspace should care about is whether there's a DRM
> > device or not. How the kernel makes that happen should be completely
> > irrelevant to userspace.
> 
> What I was referring to was userspace not expecting parts of the drm
> (crtcs/encoders/connectors) driver to show up incrementally. You can
> avoid that, but it is more of a hassle currently (ie. most drivers
> that need to do this, including a few that I've written, end up
> needing some form of
> stuff-devices-in-global-variables-that-main-driver-checks-for).

I must have misunderstood then. I don't think adding hotplug of DRM
subdevices is something we would want. And I don't think there's a
requirement for that, either. Embedded devices usually have well-defined
use-cases, so the configuration is rather static at runtime.

As for the global variables, you can do it properly. Granted, it might
be more work than global variables, but keeping drivers separated does
have advantages. Especially when the devices have completely separated
register ranges or clocks or other resources, it's very natural to use
one driver per device and glue them together with a composite device
construct.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Thierry Reding]

On Wed, Oct 30, 2013 at 11:32:24AM -0400, Sean Paul wrote:
> On Tue, Oct 29, 2013 at 4:50 PM, Tomasz Figa  wrote:
> > Hi Sean,
> >
> > On Tuesday 29 of October 2013 16:36:47 Sean Paul wrote:
> >> On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa 
> > 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 
> > 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
> >> 

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

[Thierry Reding]

On Tue, Oct 29, 2013 at 08:46:03PM -0700, St?phane Marchesin wrote:
> On Tue, Oct 29, 2013 at 1:50 PM, Tomasz Figa  wrote:
> 
> > Hi Sean,
> >
> > On Tuesday 29 of October 2013 16:36:47 Sean Paul wrote:
> > > On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa 
> > 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 
> > 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 tr

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

[Thierry Reding]

On Wed, Oct 30, 2013 at 12:32:11AM +0100, Laurent Pinchart wrote:
> 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 

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

[Laurent Pinchart]

Hi Daniel,

On Wednesday 30 October 2013 16:53:22 Daniel Vetter wrote:
> On Wed, Oct 30, 2013 at 4:32 PM, Sean Paul  wrote:
> > Once all required nodes have been "claimed", the main driver's probe
> > would call drm_platform/pci_init to kick off load(). After load() has
> > finished, the drm layer would then call the various standalone driver
> > hooks that were previously registered when it claimed its node. These
> > hooks would allow the driver to register its
> > crtc/encoder/bridge/connector.
> 
> Just a quick comment on calling the ->driver_load callback: I plan to
> look again my "kill drm midlayer" series so that drivers are in full
> control of the load sequence. Then they could do whatever delayed
> loading the need to do by calling into optional helpers (hopefully
> shared with asoc and v4l and other aggregate devices madness) and the
> drm core simply does not need to care: The driver only
> registers/allocates the drm_device once it's ready to do so.

That would be great ! Please let me know if you would like me to test patches 
with the R-Car DU driver.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sean,

Sorry for the late reply.

On Wednesday 30 October 2013 11:56:18 Sean Paul wrote:
> On Wed, Oct 30, 2013 at 11:45 AM, Laurent Pinchart wrote:
> > On Wednesday 30 October 2013 11:32:24 Sean Paul 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:
> > [snip]
> > 
> >> >> 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).
> >> > 
> >> > 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.
> >> 
> >> AFAICT, there is no way for drm to enumerate all of the pieces that
> >> need probing before it loads (ie: how do you enumerate all device
> >> nodes with pipe {} subnode[s]). I've given this more thought, and I
> >> think the following could work without forcing unified/split drivers
> >> (ie: it can be left to the driver author to choose).
> >> 
> >> If there was some way for drm to know all of the pieces that need to
> >> be probed/initialized before calling drm_load, it could provide an API
> >> for various drivers to "claim" nodes. This API would accept the
> >> device_node being claimed as well as an initialize hook that will be
> >> called back to give the standalone driver a pointer to the drm_device.
> >> 
> >> The main drm driver, which is responsible for calling
> >> drm_platform/pci_init, would claim the nodes it plans on implementing
> >> in the probe. It would then check drm to see if all requred nodes had
> >> been claimed. If they have not been claimed, that probe would defer
> >> and try again later.
> >> 
> >> Once all required nodes have been "claimed", the main driver's probe
> >> would call drm_platform/pci_init to kick off load(). After load() has
> >> finished, the drm layer would then call the various standalone driver
> >> hooks that were previously registered when it claimed its node. These
> >> hooks would allow the driver to register its
> >> crtc/encoder/bridge/connector.
> >> 
> >> Multi-driver solutions could work within this framework, as could
> >> integrated ones. This would also allow things like bridge drivers to
> >> be completely transparent.
> > 
> > Have you all configured your spam filters to reject anything that is or
> > has
> > been related to CDF ?
> > 
> > Split in two patches, the first one adding the infrastructure, the second
> > one adding OF support.
> > 
> > http://git.linuxtv.org/pinchartl/fbdev.git/commitdiff/2d19e74ab8d86aaf5d54
> > c34c6bc940508f793512
> > http://git.linuxtv.org/pinchartl/fbdev.git/commitdiff/e8c4380ca4a6a62fa9d
> > 8bc340a6dcbd123b4f674
> > 
> > The code can be extracted as a stand-alone solution, either specific to
> > DRM, or at the struct device level. As the problem is not DRM-specific,
> > the later would probably make more sense (if I'm not mistaken Grant
> > Likely - CCed- mentioned during the kernel summit was in favor of adding
> > the code in the device core).
> > 
> > We've solved the exact same problem in V4L, do we *really* need to adopt
> > the NIH approach and reinvent the wheel ?
> 
> Laurent,
> I really don't care how the functionality gets in, or what form it takes.
> This isn't NIH, I just want something that can be merged.

Great :-)

> When we talked about CDF at plumbers, I thought the plan was to split it up
> into the logical pieces and integrate it into drm. I haven't seen any
> movement on this front, is that still your in

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

[Thierry Reding]

On Tue, Oct 29, 2013 at 05:29:55PM -0400, Rob Clark wrote:
> On Tue, Oct 29, 2013 at 4:50 PM, Tomasz Figa  wrote:
[...]
> > I believe this is a huge step backwards from current kernel design
> > standards, which prefer modularity.
> 
> But it makes things behave in the way that userspace expects, which is
> more important.

Why would userspace care about the modularity of kernel drivers? The
only thing that userspace should care about is whether there's a DRM
device or not. How the kernel makes that happen should be completely
irrelevant to userspace.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Thierry Reding]

On Wed, Oct 23, 2013 at 04:22:11PM +0100, Dave Airlie wrote:
> On Wed, Oct 23, 2013 at 3:45 PM, Sean Paul  wrote:
> > On Wed, Oct 23, 2013 at 10:29 AM, Dave Airlie  wrote:
> >>> As I mentioned earlier, display_ops is needed to have no any
> >>> dependency of drm framework directly like below,
> >>>
> >>>   DRM Framework
> >>>|
> >>> Exynos DRM Framework
> >>> /   |   \
> >>>  Real device drivers
> >>>
> >>> In particular, in case of ARM based DRM drivers with separated
> >>> devices, I still tend to think it's better design than that device
> >>> drivers implement the connector callbacks directly, but I will try to
> >>> consider what is the better way.
> >>>
> >>
> >> 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.

I'm curious whether anyone actually ever bothered to look at the Tegra
driver. It pretty much does all of this already.

Granted the hardware may be more friendly than on most other SoCs, but
essentially the problem is the same.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Sean Paul]

On Mon, Nov 4, 2013 at 6:30 AM, Inki Dae  wrote:
> 2013/11/4 Thierry Reding :
>> On Tue, Oct 29, 2013 at 08:46:03PM -0700, St?phane Marchesin wrote:
>>> On Tue, Oct 29, 2013 at 1:50 PM, Tomasz Figa  
>>> wrote:
>>>
>>> > Hi Sean,
>>> >
>>> > On Tuesday 29 of October 2013 16:36:47 Sean Paul wrote:
>>> > > On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa 
>>> > 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 
>>> > 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 d

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

[Rob Clark]

On Mon, Nov 4, 2013 at 5:10 AM, Thierry Reding  
wrote:
> On Tue, Oct 29, 2013 at 05:29:55PM -0400, Rob Clark wrote:
>> On Tue, Oct 29, 2013 at 4:50 PM, Tomasz Figa  
>> wrote:
> [...]
>> > I believe this is a huge step backwards from current kernel design
>> > standards, which prefer modularity.
>>
>> But it makes things behave in the way that userspace expects, which is
>> more important.
>
> Why would userspace care about the modularity of kernel drivers? The
> only thing that userspace should care about is whether there's a DRM
> device or not. How the kernel makes that happen should be completely
> irrelevant to userspace.

What I was referring to was userspace not expecting parts of the drm
(crtcs/encoders/connectors) driver to show up incrementally. You can
avoid that, but it is more of a hassle currently (ie. most drivers
that need to do this, including a few that I've written, end up
needing some form of
stuff-devices-in-global-variables-that-main-driver-checks-for).

BR,
-R

> Thierry

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

[Daniel Vetter]

On Wed, Oct 30, 2013 at 4:32 PM, Sean Paul  wrote:
> Once all required nodes have been "claimed", the main driver's probe
> would call drm_platform/pci_init to kick off load(). After load() has
> finished, the drm layer would then call the various standalone driver
> hooks that were previously registered when it claimed its node. These
> hooks would allow the driver to register its
> crtc/encoder/bridge/connector.

Just a quick comment on calling the ->driver_load callback: I plan to
look again my "kill drm midlayer" series so that drivers are in full
control of the load sequence. Then they could do whatever delayed
loading the need to do by calling into optional helpers (hopefully
shared with asoc and v4l and other aggregate devices madness) and the
drm core simply does not need to care: The driver only
registers/allocates the drm_device once it's ready to do so.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Laurent Pinchart]

On Wednesday 30 October 2013 11:32:24 Sean Paul 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:

[snip]

> >> 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).
> > 
> > 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.
> 
> AFAICT, there is no way for drm to enumerate all of the pieces that
> need probing before it loads (ie: how do you enumerate all device
> nodes with pipe {} subnode[s]). I've given this more thought, and I
> think the following could work without forcing unified/split drivers
> (ie: it can be left to the driver author to choose).
> 
> If there was some way for drm to know all of the pieces that need to
> be probed/initialized before calling drm_load, it could provide an API
> for various drivers to "claim" nodes. This API would accept the
> device_node being claimed as well as an initialize hook that will be
> called back to give the standalone driver a pointer to the drm_device.
> 
> The main drm driver, which is responsible for calling
> drm_platform/pci_init, would claim the nodes it plans on implementing
> in the probe. It would then check drm to see if all requred nodes had
> been claimed. If they have not been claimed, that probe would defer
> and try again later.
> 
> Once all required nodes have been "claimed", the main driver's probe
> would call drm_platform/pci_init to kick off load(). After load() has
> finished, the drm layer would then call the various standalone driver
> hooks that were previously registered when it claimed its node. These
> hooks would allow the driver to register its
> crtc/encoder/bridge/connector.
> 
> Multi-driver solutions could work within this framework, as could
> integrated ones. This would also allow things like bridge drivers to
> be completely transparent.

Have you all configured your spam filters to reject anything that is or has 
been related to CDF ?

Split in two patches, the first one adding the infrastructure, the second one 
adding OF support.

http://git.linuxtv.org/pinchartl/fbdev.git/commitdiff/2d19e74ab8d86aaf5d54c34c6bc940508f793512
http://git.linuxtv.org/pinchartl/fbdev.git/commitdiff/e8c4380ca4a6a62fa9d8bc340a6dcbd123b4f674

The code can be extracted as a stand-alone solution, either specific to DRM, 
or at the struct device level. As the problem is not DRM-specific, the later 
would probably make more sense (if I'm not mistaken Grant Likely - CCed- 
mentioned during the kernel summit was in favor of adding the code in the 
device core).

We've solved the exact same problem in V4L, do we *really* need to adopt the 
NIH approach and reinvent the wheel ?

> I hope that made sense ;)

-- 
Regards,

Laurent Pinchart

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

[Sean Paul]

On Wed, Oct 30, 2013 at 11:45 AM, Laurent Pinchart
 wrote:
> On Wednesday 30 October 2013 11:32:24 Sean Paul 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:
>
> [snip]
>
>> >> 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).
>> >
>> > 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.
>>
>> AFAICT, there is no way for drm to enumerate all of the pieces that
>> need probing before it loads (ie: how do you enumerate all device
>> nodes with pipe {} subnode[s]). I've given this more thought, and I
>> think the following could work without forcing unified/split drivers
>> (ie: it can be left to the driver author to choose).
>>
>> If there was some way for drm to know all of the pieces that need to
>> be probed/initialized before calling drm_load, it could provide an API
>> for various drivers to "claim" nodes. This API would accept the
>> device_node being claimed as well as an initialize hook that will be
>> called back to give the standalone driver a pointer to the drm_device.
>>
>> The main drm driver, which is responsible for calling
>> drm_platform/pci_init, would claim the nodes it plans on implementing
>> in the probe. It would then check drm to see if all requred nodes had
>> been claimed. If they have not been claimed, that probe would defer
>> and try again later.
>>
>> Once all required nodes have been "claimed", the main driver's probe
>> would call drm_platform/pci_init to kick off load(). After load() has
>> finished, the drm layer would then call the various standalone driver
>> hooks that were previously registered when it claimed its node. These
>> hooks would allow the driver to register its
>> crtc/encoder/bridge/connector.
>>
>> Multi-driver solutions could work within this framework, as could
>> integrated ones. This would also allow things like bridge drivers to
>> be completely transparent.
>
> Have you all configured your spam filters to reject anything that is or has
> been related to CDF ?
>
> Split in two patches, the first one adding the infrastructure, the second one
> adding OF support.
>
> http://git.linuxtv.org/pinchartl/fbdev.git/commitdiff/2d19e74ab8d86aaf5d54c34c6bc940508f793512
> http://git.linuxtv.org/pinchartl/fbdev.git/commitdiff/e8c4380ca4a6a62fa9d8bc340a6dcbd123b4f674
>
> The code can be extracted as a stand-alone solution, either specific to DRM,
> or at the struct device level. As the problem is not DRM-specific, the later
> would probably make more sense (if I'm not mistaken Grant Likely - CCed-
> mentioned during the kernel summit was in favor of adding the code in the
> device core).
>
> We've solved the exact same problem in V4L, do we *really* need to adopt the
> NIH approach and reinvent the wheel ?
>

Laurent,
I really don't care how the functionality gets in, or what form it
takes. This isn't NIH, I just want something that can be merged.

When we talked about CDF at plumbers, I thought the plan was to split
it up into the logical pieces and integrate it into drm. I haven't
seen any movement on this front, is that still your intention? If so,
I look forward to the patch.

Sean



>> I hope that made sense ;)
>
> --
> Regards,
>
> Laurent Pinchart
>

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

[Sean Paul]

On Tue, Oct 29, 2013 at 4:50 PM, Tomasz Figa  wrote:
> Hi Sean,
>
> On Tuesday 29 of October 2013 16:36:47 Sean Paul wrote:
>> On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa 
> 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 
> 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
>> impl

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

[Laurent Pinchart]

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

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

[Laurent Pinchart]

Hello,

On Tuesday 29 October 2013 20:47:44 Sylwester Nawrocki wrote:
> On 29/10/13 20:23, Tomasz Figa wrote:
> > On Tuesday 29 of October 2013 12:19:35 Olof Johansson wrote:
> > > It's a very deeply nested structure, I'm not sure there's a need to
> > > make a ports {} subnode really.

Agreed, this can be simplified. We've introduced the ports node to group 
multiple ports when the device tree node that contains them also has child 
devices (which is a pretty uncommon case). When this happens, we need 
potentially different #address-cells values for the ports and for the child 
devices. In all other case the ports node can be omitted and the port nodes 
placed directly as children of the device node.

> > > Also, I don't know if it makes sense to always name it remote-endpoint,
> > > or to use a more flexible name depending on what is actually connected,
> > > over which bus, etc.
> 
> I have been thinking about a 'bus_type' as an 'endpoint' node property.
> Currently the data bus type is derived from selected properties in
> endpoint node, which is IMO not good enough.

I agree, a bus type would be useful. I've also been thinking about adding a 
direction property for ports, although that information could be considered as 
device driver knowledge.

> I'm not sure if naming 'remote-endpoint' differently would be helpful, as it
> is now it seems easier to write a generic links parser.
> 
> Nevertheless I wish we have defined a bit simplified option in this binding
> right from the beginning.

It's not too late to clean it up and create a v2 :-)

> >> > But overall this looks sane-ish.
> > 
> > I fully agree with you. Personally I would take a bit different design
> > decisions when designing this bindings, but here I'm just pointing an
> > already defined binding that should suit the needs described in this
> > thread, to avoid reinventing the wheel.
> 
> The 'ports' node is optional. It needs to be used only if, e.g. bridge-a
> node contains device child nodes and these sub-nodes use 'reg' property.
> In such case #address-cells, #size-cells properties for 'port' nodes could
> be conflicting with those for the device child nodes.

I should have read Sylwester's e-mail completely before writing the same 
explanation above :-)

-- 
Regards,

Laurent Pinchart

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

[Tomasz Figa]

Hi Sean,

On Tuesday 29 of October 2013 16:36:47 Sean Paul wrote:
> On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa  
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  
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 ma

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

[Sylwester Nawrocki]

Hi,

On 29/10/13 20:23, Tomasz Figa wrote:
>> It's a very deeply nested structure, I'm not sure there's a need to
>> > make a ports {} subnode really.
>> > 
>> > Also, I don't know if it makes sense to always name it
>> > remote-endpoint, or to use a more flexible name depending on what is
>> > actually connected, over which bus, etc.

I have been thinking about a 'bus_type' as an 'endpoint' node property.
Currently the data bus type is derived from selected properties in
endpoint node, which is IMO not good enough.

I'm not sure if naming 'remote-endpoint' differently would be helpful,
as it is now it seems easier to write a generic links parser.

Nevertheless I wish we have defined a bit simplified option in this binding
right from the beginning.

>> > But overall this looks sane-ish.
>
> I fully agree with you. Personally I would take a bit different design 
> decisions when designing this bindings, but here I'm just pointing an 
> already defined binding that should suit the needs described in this 
> thread, to avoid reinventing the wheel.

The 'ports' node is optional. It needs to be used only if, e.g. bridge-a
node contains device child nodes and these sub-nodes use 'reg' property.
In such case #address-cells, #size-cells properties for 'port' nodes could
be conflicting with those for the device child nodes.

Thanks,
Sylwester

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

[Stéphane Marchesin]

On Tue, Oct 29, 2013 at 1:50 PM, Tomasz Figa  wrote:

> Hi Sean,
>
> On Tuesday 29 of October 2013 16:36:47 Sean Paul wrote:
> > On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa 
> 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 
> 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 f

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

[Tomasz Figa]

On Tuesday 29 of October 2013 12:19:35 Olof Johansson wrote:
> On Mon, Oct 28, 2013 at 4:13 PM, Tomasz Figa  
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  
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:
> > 
> > panel-a {
> > 
> > /* Single input port */
> > port {
> > 
> > panel_a: endpoint at 0 {
> > 
> > remote-endpoint = <&encoder_y_out>;
> > 
> > };
> > 
> >

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

[Rob Clark]

On Tue, Oct 29, 2013 at 4:50 PM, Tomasz Figa  wrote:
> Hi Sean,
>
> On Tuesday 29 of October 2013 16:36:47 Sean Paul wrote:
>> On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa 
> 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 
> 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
>> impl

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

[Sean Paul]

On Mon, Oct 28, 2013 at 7:13 PM, Tomasz Figa  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  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

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

[Olof Johansson]

On Mon, Oct 28, 2013 at 4:13 PM, Tomasz Figa  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  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:
>
> 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: endp

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

[Tomasz Figa]

Hi,

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:

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-endpoin

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

[Rob Clark]

On Thu, Oct 24, 2013 at 3:46 AM, Inki Dae  wrote:
> 2013/10/23 Rob Clark :
>> On Wed, Oct 23, 2013 at 9:18 AM, Inki Dae  wrote:
>>> Look at omapdrm, nouveau, and radeon drm drivers.
>>
>>
>> btw, please don't look at omapdrm as a "good" example in this regard.
>> The layering is really not a good idea and for a long time caused a
>> lot of problems, which we essentially solved by bypassing parts of the
>> layering.  I still think omapdss and omapdrm should be flattened into
>> a single drm driver, then net result would be a drop in # of lines of
>
> It seems that you proper to use duplicated driver. I mean... do you
> proper that omapdss driver is placed in drm framework? Otherwise, is
> there any way that omapdss and omapdrm can be flattened into a single
> drm driver without moving omapdss into drm framework? As I mentioned
> earlier, we wanted to reuse the existing panel driver so Exynos drm
> framework has the layer, wrappers to connector and encoder. Of course,
> for this, we have TODO works yet, and I still think it's better way to
> keep the wrapper if we should reuse the existing panel drivers.

If it were my decision, I would duplicate (+refactor) the code into
drm..  there ends up being some duplication between fbdev and drm, but
the benefit is less risk of breaking other subsystem (fbdev) in the
short term and more flexibility to refactor things to fit better into
how drm/kms works.

In the long term, fbdev stops being something we care about, so the
duplicate code is just a transient problem.  But what ends up in drm
we live with for a long time, so it is easier in the long run to not
have to care about both fbdev and drm with the same code.

BR,
-R

> The below would be one design for the case,
>
>
>   lcd panel 
> drivers
>   \ | /
>drm framework  -  drm_bridge
>   / | \
>   crtc
> drivers(display controller or hdmi)
>
>
> A header file of drm_bridge includes drm_panel structure having some
> callbacks related to connector, and some function to register crtc and
> panel driver's requests to the drm_bridge object. And the header file
> can be included in the existing panel drivers. In other words,
> drm_bridge will connect drm driver and lcd class based panel drivers.
>
> struct drm_bridge {
> struct list_head list;
> unsigned int type;
> struct drm_device *drm_dev;
> struct drm_panel *panel_ops;
> ...
> int (*drm_create_enc_conn)();
> };
>
> A drm_bridge object has drm_panel ops and drm_create_enc_conn
> callback. And once the crtc driver calls register function of the
> drm_bridge with drm_create_enc_conn callback pointer, a drm_crtc is
> created, and once the panel driver calls the register function with
> drm_panel ops, a encoder and a connector are created. At this time,
> drm_fb_helper_initial_config() can be called appropriately to connect
> crtc and connector, and to display framebuffer on lcd panel.
>
> The above is just my opinion for the case, and we are testing this way
> implementing it internally.
>
> Thanks,
> Inki Dae
>
>> code.  I wish there were folks like the Sean and St?phane who cared
>> enough to do this for omapdrm ;-)
>>
>> Other drivers have some modularity to separate code that is
>> significantly different across genarations (but what form that takes
>> differs depending on what how the hw differs across generations).
>> This isn't a bad thing.  But you need to look at the end result.  For
>> example how I split out the phy code for the mdp4 code in msm (hdmi
>> and dsi phy differ between otherwise similar 28nm and 45nm parts, but
>> the rest of the display controller blocks are basically the same).
>>
>> BR,
>> -R
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sean Paul]

On Mon, Oct 28, 2013 at 12:49 PM, Olof Johansson  wrote:
> On Wed, Oct 23, 2013 at 9:09 AM, 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:
>
> This is definitely worth discussing. I know device-tree but not DRM so
> let's see if we can figure this out together. :)
>
>>
>> display-pipelines {
>>   required-elements = <&bridge-a &panel-a &encoder-x &encoder-y
>> &crtc-x &crtc-y>;
>
> What's this supposed to mean? Are all these elements required to get
> DRM up on this particular platform? Or are they just enumerating all
> the possible devices that might be involved?
>

I was thinking that these would be all of the pieces that are required
before drm is loaded.

So, in the multiple device-drivers scenario, these would all register
themselves with drm on probe. drm would tick them off the list as they
probe until all have been accounted for. At this point, drm would load
and call them back with a pointer to the drm_device.

In the case where there is One True DRM Driver, where the various
devices are not standalone drivers (see ptn3460 patch for this model),
this would be the list of devices that drm is responsible for linking
in. To do this, it would need to call one hook when it probes (so it
can be deferred), and than an initialization hook later once
everything is probed and ready.

>>   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>;
>>   };
>
> Maybe it's the use of pipe as a keyword that's confusing me here, and
> my lack of DRM knowledge, but wouldn't it make more sense to focus on
> output connectors here? I.e. describe the outputs such as eDP, LVDS,
> HDMI. The bridge, such as edp-to-lvds, should possibly be specified
> under the edp node instead of in the drm node here.
>

Sure, I think "pipeX" is poor nomenclature on my part. It would make
more sense to have these node names translate to the actual connector
name. I was trying to be a bit opaque/non-drm-specific to avoid
describing software in the devicetree.

Again, using the ptn driver as an example, something like:

LVDS-1 {
connector = <&ptn3460>;

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

[Olof Johansson]

On Wed, Oct 23, 2013 at 9:09 AM, 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:

This is definitely worth discussing. I know device-tree but not DRM so
let's see if we can figure this out together. :)

>
> display-pipelines {
>   required-elements = <&bridge-a &panel-a &encoder-x &encoder-y
> &crtc-x &crtc-y>;

What's this supposed to mean? Are all these elements required to get
DRM up on this particular platform? Or are they just enumerating all
the possible devices that might be involved?

>   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>;
>   };

Maybe it's the use of pipe as a keyword that's confusing me here, and
my lack of DRM knowledge, but wouldn't it make more sense to focus on
output connectors here? I.e. describe the outputs such as eDP, LVDS,
HDMI. The bridge, such as edp-to-lvds, should possibly be specified
under the edp node instead of in the drm node here.

Also, while the hardware can allow a very flexible connection of
pipes, I'm guessing that in reality nearly all usage of these will
follow a few common patterns, so maybe it's not required to be able to
describe the full graph in device tree?

> 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.

As mentioned above, it might make sense to turn it around and describe
it around the connectors instead of the pipes.

> 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.


-Olof

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

[Inki Dae]

Sorry for some typos.

2013/10/24 Inki Dae :
> 2013/10/23 Rob Clark :
>> On Wed, Oct 23, 2013 at 9:18 AM, Inki Dae  wrote:
>>> Look at omapdrm, nouveau, and radeon drm drivers.
>>
>>
>> btw, please don't look at omapdrm as a "good" example in this regard.
>> The layering is really not a good idea and for a long time caused a
>> lot of problems, which we essentially solved by bypassing parts of the
>> layering.  I still think omapdss and omapdrm should be flattened into
>> a single drm driver, then net result would be a drop in # of lines of
>
> It seems that you proper to use duplicated driver. I mean... do you

s/proper/prefer

> proper that omapdss driver is placed in drm framework? Otherwise, is

s/proper/prefer

> there any way that omapdss and omapdrm can be flattened into a single
> drm driver without moving omapdss into drm framework? As I mentioned
> earlier, we wanted to reuse the existing panel driver so Exynos drm
> framework has the layer, wrappers to connector and encoder. Of course,
> for this, we have TODO works yet, and I still think it's better way to
> keep the wrapper if we should reuse the existing panel drivers.
>
> The below would be one design for the case,
>
>
>   lcd panel 
> drivers
>   \ | /
>drm framework  -  drm_bridge
>   / | \
>   crtc
> drivers(display controller or hdmi)
>
>
> A header file of drm_bridge includes drm_panel structure having some
> callbacks related to connector, and some function to register crtc and
> panel driver's requests to the drm_bridge object. And the header file
> can be included in the existing panel drivers. In other words,
> drm_bridge will connect drm driver and lcd class based panel drivers.
>
> struct drm_bridge {
> struct list_head list;
> unsigned int type;
> struct drm_device *drm_dev;
> struct drm_panel *panel_ops;
> ...
> int (*drm_create_enc_conn)();
> };
>
> A drm_bridge object has drm_panel ops and drm_create_enc_conn
> callback. And once the crtc driver calls register function of the
> drm_bridge with drm_create_enc_conn callback pointer, a drm_crtc is
> created, and once the panel driver calls the register function with
> drm_panel ops, a encoder and a connector are created. At this time,
> drm_fb_helper_initial_config() can be called appropriately to connect
> crtc and connector, and to display framebuffer on lcd panel.
>
> The above is just my opinion for the case, and we are testing this way
> implementing it internally.
>
> Thanks,
> Inki Dae
>
>> code.  I wish there were folks like the Sean and St?phane who cared
>> enough to do this for omapdrm ;-)
>>
>> Other drivers have some modularity to separate code that is
>> significantly different across genarations (but what form that takes
>> differs depending on what how the hw differs across generations).
>> This isn't a bad thing.  But you need to look at the end result.  For
>> example how I split out the phy code for the mdp4 code in msm (hdmi
>> and dsi phy differ between otherwise similar 28nm and 45nm parts, but
>> the rest of the display controller blocks are basically the same).
>>
>> BR,
>> -R
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Inki Dae]

2013/10/23 Rob Clark :
> On Wed, Oct 23, 2013 at 9:18 AM, Inki Dae  wrote:
>> Look at omapdrm, nouveau, and radeon drm drivers.
>
>
> btw, please don't look at omapdrm as a "good" example in this regard.
> The layering is really not a good idea and for a long time caused a
> lot of problems, which we essentially solved by bypassing parts of the
> layering.  I still think omapdss and omapdrm should be flattened into
> a single drm driver, then net result would be a drop in # of lines of

It seems that you proper to use duplicated driver. I mean... do you
proper that omapdss driver is placed in drm framework? Otherwise, is
there any way that omapdss and omapdrm can be flattened into a single
drm driver without moving omapdss into drm framework? As I mentioned
earlier, we wanted to reuse the existing panel driver so Exynos drm
framework has the layer, wrappers to connector and encoder. Of course,
for this, we have TODO works yet, and I still think it's better way to
keep the wrapper if we should reuse the existing panel drivers.

The below would be one design for the case,


  lcd panel drivers
  \ | /
   drm framework  -  drm_bridge
  / | \
  crtc
drivers(display controller or hdmi)


A header file of drm_bridge includes drm_panel structure having some
callbacks related to connector, and some function to register crtc and
panel driver's requests to the drm_bridge object. And the header file
can be included in the existing panel drivers. In other words,
drm_bridge will connect drm driver and lcd class based panel drivers.

struct drm_bridge {
struct list_head list;
unsigned int type;
struct drm_device *drm_dev;
struct drm_panel *panel_ops;
...
int (*drm_create_enc_conn)();
};

A drm_bridge object has drm_panel ops and drm_create_enc_conn
callback. And once the crtc driver calls register function of the
drm_bridge with drm_create_enc_conn callback pointer, a drm_crtc is
created, and once the panel driver calls the register function with
drm_panel ops, a encoder and a connector are created. At this time,
drm_fb_helper_initial_config() can be called appropriately to connect
crtc and connector, and to display framebuffer on lcd panel.

The above is just my opinion for the case, and we are testing this way
implementing it internally.

Thanks,
Inki Dae

> code.  I wish there were folks like the Sean and St?phane who cared
> enough to do this for omapdrm ;-)
>
> Other drivers have some modularity to separate code that is
> significantly different across genarations (but what form that takes
> differs depending on what how the hw differs across generations).
> This isn't a bad thing.  But you need to look at the end result.  For
> example how I split out the phy code for the mdp4 code in msm (hdmi
> and dsi phy differ between otherwise similar 28nm and 45nm parts, but
> the rest of the display controller blocks are basically the same).
>
> BR,
> -R
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Inki Dae]

2013/10/23 Sean Paul :
> On Wed, Oct 23, 2013 at 10:29 AM, Dave Airlie  wrote:
>>> As I mentioned earlier, display_ops is needed to have no any
>>> dependency of drm framework directly like below,
>>>
>>>   DRM Framework
>>>|
>>> Exynos DRM Framework
>>> /   |   \
>>>  Real device drivers
>>>
>>> In particular, in case of ARM based DRM drivers with separated
>>> devices, I still tend to think it's better design than that device
>>> drivers implement the connector callbacks directly, but I will try to
>>> consider what is the better way.
>>>
>>
>> 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.
>

Before that, I think we need to decide that drm driver should reuse
lcd class based drivers, or just should copy the lcd class based
drivers into drm framework.

1. in case that drm driver reuses the existing lcd class based drivers,
- we would need a common layer across between Linux framebuffer and
drm framework. In other words, the lcd class based drivers and drm
driver should be able to include a header file to the common layer,
and the header should provide some callbacks such as drm_panel
structure, and also some functions to create a connector and a
encoder. Actually, that is why Exynos drm framework has a wrapper to
drm connector and encoder, and display_ops also.

2. in case that drm driver uses duplicated lcd driver,
- we wouldn't need the common layer because a connector and a encoder
can be created directly in the lcd driver so in this case, I think the
wrapper to drm connector and display_ops wouldn't be needed anymore.

And shouldn't the device tree related works be discussed after we
select one of the above two cases? or should we consider all of the
above two cases?

> 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.
>>
>> Dave.
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Inki Dae]

2013/10/23 Sean Paul :
> On Wed, Oct 23, 2013 at 1:42 AM, Inki Dae  wrote:
>> 2013/10/23 Sean Paul :
>>> On Wed, Oct 23, 2013 at 12:48 AM, Inki Dae  wrote:
>>>> 2013/10/23 St?phane Marchesin :
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Oct 22, 2013 at 9:15 PM, Inki Dae  wrote:
>>>>>>
>>>>>> 2013/10/23 St?phane Marchesin :
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>> > On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae  
>>>>>> > wrote:
>>>>>> >>
>>>>>> >> 2013/10/23 St?phane Marchesin :
>>>>>> >> >
>>>>>> >> >
>>>>>> >> >
>>>>>> >> > On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae 
>>>>>> >> > wrote:
>>>>>> >> >>
>>>>>> >> >> 2013/10/22 Sean Paul :
>>>>>> >> >> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae >>>>> >> >> > samsung.com>
>>>>>> >> >> > wrote:
>>>>>> >> >> >>
>>>>>> >> >> >>
>>>>>> >> >> >>> -Original Message-
>>>>>> >> >> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>>>> >> >> >>> Sent: Tuesday, October 22, 2013 6:18 AM
>>>>>> >> >> >>> To: Inki Dae
>>>>>> >> >> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>>>>> >> >> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>>>>>> >> >> >>> manager/display/subdrv
>>>>>> >> >> >>>
>>>>>> >> >> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul
>>>>>> >> >> >>> 
>>>>>> >> >> >>> wrote:
>>>>>> >> >> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae
>>>>>> >> >> >>> > 
>>>>>> >> >> >>> > wrote:
>>>>>> >> >> >>> >>
>>>>>> >> >> >>> >>
>>>>>> >> >> >>> >>> -Original Message-
>>>>>> >> >> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>>>> >> >> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>>>>>> >> >> >>> >>> To: Inki Dae
>>>>>> >> >> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>>>>> >> >> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>>>>>> >> >> >>> >>> manager/display/subdrv
>>>>>> >> >> >>> >>>
>>>>>> >> >> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae
>>>>>> >> >> >>> >>> 
>>>>>> >> >> >> wrote:
>>>>>> >> >> >>> >>> >
>>>>>> >> >> >>> >>> >
>>>>>> >> >> >>> >>> >> -Original Message-
>>>>>> >> >> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>>>> >> >> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>>>>>> >> >> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at 
>>>>>> >> >> >>> >>> >> samsung.com
>>>>>> >> >> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
>>>>>> >> >> >>> >>> >> marcheu at chromium.org;
>>>>>> >> >> >>> Sean
>>>>>> >> >> >>> >>> >> Paul
>>>>>> >> >> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
>>>>>&

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

[Dave Airlie]

>

 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.

Dave.

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

[Dave Airlie]

On Wed, Oct 23, 2013 at 3:45 PM, Sean Paul  wrote:
> On Wed, Oct 23, 2013 at 10:29 AM, Dave Airlie  wrote:
>>> As I mentioned earlier, display_ops is needed to have no any
>>> dependency of drm framework directly like below,
>>>
>>>   DRM Framework
>>>|
>>> Exynos DRM Framework
>>> /   |   \
>>>  Real device drivers
>>>
>>> In particular, in case of ARM based DRM drivers with separated
>>> devices, I still tend to think it's better design than that device
>>> drivers implement the connector callbacks directly, but I will try to
>>> consider what is the better way.
>>>
>>
>> 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.

Dave.

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

[Dave Airlie]

> As I mentioned earlier, display_ops is needed to have no any
> dependency of drm framework directly like below,
>
>   DRM Framework
>|
> Exynos DRM Framework
> /   |   \
>  Real device drivers
>
> In particular, in case of ARM based DRM drivers with separated
> devices, I still tend to think it's better design than that device
> drivers implement the connector callbacks directly, but I will try to
> consider what is the better way.
>

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.

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.

Dave.

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

[Inki Dae]

2013/10/23 Sean Paul :
> On Wed, Oct 23, 2013 at 12:48 AM, Inki Dae  wrote:
>> 2013/10/23 St?phane Marchesin :
>>>
>>>
>>>
>>> On Tue, Oct 22, 2013 at 9:15 PM, Inki Dae  wrote:
>>>>
>>>> 2013/10/23 St?phane Marchesin :
>>>> >
>>>> >
>>>> >
>>>> > On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae  
>>>> > wrote:
>>>> >>
>>>> >> 2013/10/23 St?phane Marchesin :
>>>> >> >
>>>> >> >
>>>> >> >
>>>> >> > On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae 
>>>> >> > wrote:
>>>> >> >>
>>>> >> >> 2013/10/22 Sean Paul :
>>>> >> >> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae 
>>>> >> >> > wrote:
>>>> >> >> >>
>>>> >> >> >>
>>>> >> >> >>> -Original Message-
>>>> >> >> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>> >> >> >>> Sent: Tuesday, October 22, 2013 6:18 AM
>>>> >> >> >>> To: Inki Dae
>>>> >> >> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>>> >> >> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>>>> >> >> >>> manager/display/subdrv
>>>> >> >> >>>
>>>> >> >> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul
>>>> >> >> >>> 
>>>> >> >> >>> wrote:
>>>> >> >> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae
>>>> >> >> >>> > 
>>>> >> >> >>> > wrote:
>>>> >> >> >>> >>
>>>> >> >> >>> >>
>>>> >> >> >>> >>> -----Original Message-
>>>> >> >> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>> >> >> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>>>> >> >> >>> >>> To: Inki Dae
>>>> >> >> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>>> >> >> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>>>> >> >> >>> >>> manager/display/subdrv
>>>> >> >> >>> >>>
>>>> >> >> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae
>>>> >> >> >>> >>> 
>>>> >> >> >> wrote:
>>>> >> >> >>> >>> >
>>>> >> >> >>> >>> >
>>>> >> >> >>> >>> >> -Original Message-
>>>> >> >> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>> >> >> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>>>> >> >> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at 
>>>> >> >> >>> >>> >> samsung.com
>>>> >> >> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
>>>> >> >> >>> >>> >> marcheu at chromium.org;
>>>> >> >> >>> Sean
>>>> >> >> >>> >>> >> Paul
>>>> >> >> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
>>>> >> >> >>> >>> >> manager/display/subdrv
>>>> >> >> >>> >>> >>
>>>> >> >> >>> >>> >> This patch splits display and manager from subdrv. The
>>>> >> >> >>> >>> >> result
>>>> >> >> >>> >>> >> is
>>>> >> >> >>> that
>>>> >> >> >>> >>> >> crtc functions can directly call into manager callbacks
>>>> >> >> >>> >>&g

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

[Inki Dae]

2013/10/22 Inki Dae :
> 2013/10/17 Sean Paul :
>> This patch splits display and manager from subdrv. The result is that
>> crtc functions can directly call into manager callbacks and encoder
>> functions can directly call into display callbacks. This will allow
>> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
>> with common code.
>>
>> Signed-off-by: Sean Paul 
>> ---
>>
>> Changes in v2:
>> - Pass display into display_ops instead of context
>>
>>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++-
>>  drivers/gpu/drm/exynos/exynos_drm_core.c  | 181 ---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 115 +--
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.h  |  20 +-
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c   |  29 +-
>>  drivers/gpu/drm/exynos/exynos_drm_drv.h   | 106 +++---
>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258 ++-
>>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
>>  drivers/gpu/drm/exynos/exynos_drm_fb.c|   4 +-
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c  | 161 +
>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c  | 449 
>> --
>
> Build error. it seems that you missed vidi module. Can you consider
> vidi module also?
>

Sean, ping~~~

> Thanks,
> Inki Dae
>
>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h  |   2 +
>>  drivers/gpu/drm/exynos/exynos_drm_plane.c |  15 +-
>>  13 files changed, 466 insertions(+), 942 deletions(-)
>>  delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>>

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

[Inki Dae]

2013/10/23 St?phane Marchesin :
>
>
>
> On Tue, Oct 22, 2013 at 9:15 PM, Inki Dae  wrote:
>>
>> 2013/10/23 St?phane Marchesin :
>> >
>> >
>> >
>> > On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae  wrote:
>> >>
>> >> 2013/10/23 St?phane Marchesin :
>> >> >
>> >> >
>> >> >
>> >> > On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae 
>> >> > wrote:
>> >> >>
>> >> >> 2013/10/22 Sean Paul :
>> >> >> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae 
>> >> >> > wrote:
>> >> >> >>
>> >> >> >>
>> >> >> >>> -Original Message-
>> >> >> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >> >> >>> Sent: Tuesday, October 22, 2013 6:18 AM
>> >> >> >>> To: Inki Dae
>> >> >> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>> >> >> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>> >> >> >>> manager/display/subdrv
>> >> >> >>>
>> >> >> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul
>> >> >> >>> 
>> >> >> >>> wrote:
>> >> >> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae
>> >> >> >>> > 
>> >> >> >>> > wrote:
>> >> >> >>> >>
>> >> >> >>> >>
>> >> >> >>> >>> -Original Message-
>> >> >> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >> >> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>> >> >> >>> >>> To: Inki Dae
>> >> >> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>> >> >> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>> >> >> >>> >>> manager/display/subdrv
>> >> >> >>> >>>
>> >> >> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae
>> >> >> >>> >>> 
>> >> >> >> wrote:
>> >> >> >>> >>> >
>> >> >> >>> >>> >
>> >> >> >>> >>> >> -Original Message-
>> >> >> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >> >> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>> >> >> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at 
>> >> >> >>> >>> >> samsung.com
>> >> >> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
>> >> >> >>> >>> >> marcheu at chromium.org;
>> >> >> >>> Sean
>> >> >> >>> >>> >> Paul
>> >> >> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
>> >> >> >>> >>> >> manager/display/subdrv
>> >> >> >>> >>> >>
>> >> >> >>> >>> >> This patch splits display and manager from subdrv. The
>> >> >> >>> >>> >> result
>> >> >> >>> >>> >> is
>> >> >> >>> that
>> >> >> >>> >>> >> crtc functions can directly call into manager callbacks
>> >> >> >>> >>> >> and
>> >> >> >>> >>> >> encoder
>> >> >> >>> >>> >> functions can directly call into display callbacks. This
>> >> >> >>> >>> >> will
>> >> >> >>> >>> >> allow
>> >> >> >>> >>> >> us to remove the exynos_drm_hdmi shim and support
>> >> >> >>> >>> >> mixer/hdmi
>> >> >> >>> >>> >> &
>> >> >> >>> fimd/dp
>> >> >> >>> >>&

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

[Inki Dae]

2013/10/23 St?phane Marchesin :
>
>
>
> On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae  wrote:
>>
>> 2013/10/23 St?phane Marchesin :
>> >
>> >
>> >
>> > On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae  wrote:
>> >>
>> >> 2013/10/22 Sean Paul :
>> >> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae 
>> >> > wrote:
>> >> >>
>> >> >>
>> >> >>> -Original Message-
>> >> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >> >>> Sent: Tuesday, October 22, 2013 6:18 AM
>> >> >>> To: Inki Dae
>> >> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>> >> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>> >> >>> manager/display/subdrv
>> >> >>>
>> >> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul 
>> >> >>> wrote:
>> >> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae 
>> >> >>> > wrote:
>> >> >>> >>
>> >> >>> >>
>> >> >>> >>> -Original Message-
>> >> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>> >> >>> >>> To: Inki Dae
>> >> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>> >> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>> >> >>> >>> manager/display/subdrv
>> >> >>> >>>
>> >> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae
>> >> >>> >>> 
>> >> >> wrote:
>> >> >>> >>> >
>> >> >>> >>> >
>> >> >>> >>> >> -Original Message-
>> >> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>> >> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
>> >> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
>> >> >>> >>> >> marcheu at chromium.org;
>> >> >>> Sean
>> >> >>> >>> >> Paul
>> >> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
>> >> >>> >>> >> manager/display/subdrv
>> >> >>> >>> >>
>> >> >>> >>> >> This patch splits display and manager from subdrv. The
>> >> >>> >>> >> result
>> >> >>> >>> >> is
>> >> >>> that
>> >> >>> >>> >> crtc functions can directly call into manager callbacks and
>> >> >>> >>> >> encoder
>> >> >>> >>> >> functions can directly call into display callbacks. This
>> >> >>> >>> >> will
>> >> >>> >>> >> allow
>> >> >>> >>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi
>> >> >>> >>> >> &
>> >> >>> fimd/dp
>> >> >>> >>> >> with common code.
>> >> >>> >>> >>
>> >> >>> >>> >> Signed-off-by: Sean Paul 
>> >> >>> >>> >> ---
>> >> >>> >>> >>
>> >> >>> >>> >> Changes in v2:
>> >> >>> >>> >>   - Pass display into display_ops instead of context
>> >> >>> >>> >
>> >> >>> >>> > Sorry but it seems like more reasonable to pass device object
>> >> >>> >>> > into
>> >> >>> >>> > display_ops and manager_ops.
>> >> >>> >>> >
>> >> >>> >>>
>> >> >>> >>>
>> >> >>> >>> So you've changed your mind from when you said the following?
>> >> >>&

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

[Inki Dae]

2013/10/23 St?phane Marchesin :
>
>
>
> On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae  wrote:
>>
>> 2013/10/22 Sean Paul :
>> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae  wrote:
>> >>
>> >>
>> >>> -Original Message-
>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >>> Sent: Tuesday, October 22, 2013 6:18 AM
>> >>> To: Inki Dae
>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>> >>>
>> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul 
>> >>> wrote:
>> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae 
>> >>> > wrote:
>> >>> >>
>> >>> >>
>> >>> >>> -Original Message-
>> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>> >>> >>> To: Inki Dae
>> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>> >>> >>> manager/display/subdrv
>> >>> >>>
>> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae 
>> >> wrote:
>> >>> >>> >
>> >>> >>> >
>> >>> >>> >> -Original Message-
>> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
>> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
>> >>> >>> >> marcheu at chromium.org;
>> >>> Sean
>> >>> >>> >> Paul
>> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
>> >>> >>> >> manager/display/subdrv
>> >>> >>> >>
>> >>> >>> >> This patch splits display and manager from subdrv. The result
>> >>> >>> >> is
>> >>> that
>> >>> >>> >> crtc functions can directly call into manager callbacks and
>> >>> >>> >> encoder
>> >>> >>> >> functions can directly call into display callbacks. This will
>> >>> >>> >> allow
>> >>> >>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi &
>> >>> fimd/dp
>> >>> >>> >> with common code.
>> >>> >>> >>
>> >>> >>> >> Signed-off-by: Sean Paul 
>> >>> >>> >> ---
>> >>> >>> >>
>> >>> >>> >> Changes in v2:
>> >>> >>> >>   - Pass display into display_ops instead of context
>> >>> >>> >
>> >>> >>> > Sorry but it seems like more reasonable to pass device object
>> >>> >>> > into
>> >>> >>> > display_ops and manager_ops.
>> >>> >>> >
>> >>> >>>
>> >>> >>>
>> >>> >>> So you've changed your mind from when you said the following?
>> >>> >>>
>> >>> >>> >>> manager->ops->xxx(manager, ...);
>> >>> >>> >>> display->ops->xxx(display, ...);
>> >>> >>> >>>
>> >>> >>> >>> Agree.
>> >>> >>>
>> >>> >>
>> >>> >>
>> >>> >> True. Before that, My comment was to pass device object into
>> >>> display_ops and
>> >>> >> manager_ops, and then you said the good solution is to pass manager
>> >>> >> and
>> >>> >> display to each driver. At that time, I thought no matter how the
>> >>> callback
>> >>> >> is called if the framework doesn't call callbacks of each driver
>> >>> >> with
>> >>> ctx.
>> >>> >> So I agreed.
>> >>

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

[Sean Paul]

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.

Sean



> Dave.

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

[Inki Dae]

2013/10/22 Sean Paul :
> On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae  wrote:
>>
>>
>>> -Original Message-
>>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>> Sent: Tuesday, October 22, 2013 6:18 AM
>>> To: Inki Dae
>>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>>
>>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul  
>>> wrote:
>>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae  
>>> > wrote:
>>> >>
>>> >>
>>> >>> -Original Message-
>>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>>> >>> To: Inki Dae
>>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>> >>>
>>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae 
>> wrote:
>>> >>> >
>>> >>> >
>>> >>> >> -----Original Message-
>>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
>>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at 
>>> >>> >> chromium.org;
>>> Sean
>>> >>> >> Paul
>>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>> >>> >>
>>> >>> >> This patch splits display and manager from subdrv. The result is
>>> that
>>> >>> >> crtc functions can directly call into manager callbacks and encoder
>>> >>> >> functions can directly call into display callbacks. This will allow
>>> >>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi &
>>> fimd/dp
>>> >>> >> with common code.
>>> >>> >>
>>> >>> >> Signed-off-by: Sean Paul 
>>> >>> >> ---
>>> >>> >>
>>> >>> >> Changes in v2:
>>> >>> >>   - Pass display into display_ops instead of context
>>> >>> >
>>> >>> > Sorry but it seems like more reasonable to pass device object into
>>> >>> > display_ops and manager_ops.
>>> >>> >
>>> >>>
>>> >>>
>>> >>> So you've changed your mind from when you said the following?
>>> >>>
>>> >>> >>> manager->ops->xxx(manager, ...);
>>> >>> >>> display->ops->xxx(display, ...);
>>> >>> >>>
>>> >>> >>> Agree.
>>> >>>
>>> >>
>>> >>
>>> >> True. Before that, My comment was to pass device object into
>>> display_ops and
>>> >> manager_ops, and then you said the good solution is to pass manager and
>>> >> display to each driver. At that time, I thought no matter how the
>>> callback
>>> >> is called if the framework doesn't call callbacks of each driver with
>>> ctx.
>>> >> So I agreed.
>>> >>
>>> >>
>>> >>> It would have been nice if you had changed your mind *before* I
>>> >>> reworked everything. At any rate, I think it's still the right thing
>>> >>> to do.
>>> >>
>>> >> Really sorry about that. And I will add new patch for it so you don't
>>> need
>>> >> to concern about that.
>>> >>
>>> >>>
>>> >>>
>>> >>> > I'm not sure but display_ops could be implemented in other framework
>>> >>> based
>>> >>> > driver such as CDF based lcd panel driver. So if you pass display -
>>> it's
>>> >>> > specific to exynos drm framework - into display_ops, the other
>>> framework
>>> >>> > based driver should include specific exynos drm header.
>>> >>> >
>>> >>>
>>> >>> AFAIK, CDF will not land in its

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

[Sean Paul]

On Wed, Oct 23, 2013 at 11:27 AM, Sean Paul  wrote:
> On Wed, Oct 23, 2013 at 11:22 AM, Dave Airlie  wrote:
>> On Wed, Oct 23, 2013 at 3:45 PM, Sean Paul  wrote:
>>> On Wed, Oct 23, 2013 at 10:29 AM, Dave Airlie  wrote:
> As I mentioned earlier, display_ops is needed to have no any
> dependency of drm framework directly like below,
>
>   DRM Framework
>|
> Exynos DRM Framework
> /   |   \
>  Real device drivers
>
> In particular, in case of ARM based DRM drivers with separated
> devices, I still tend to think it's better design than that device
> drivers implement the connector callbacks directly, but I will try to
> consider what is the better way.
>

 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.
>

tl;dr: What Rob said

> Sean
>
>> Dave.

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

[Sean Paul]

On Wed, Oct 23, 2013 at 11:22 AM, Dave Airlie  wrote:
> On Wed, Oct 23, 2013 at 3:45 PM, Sean Paul  wrote:
>> On Wed, Oct 23, 2013 at 10:29 AM, Dave Airlie  wrote:
 As I mentioned earlier, display_ops is needed to have no any
 dependency of drm framework directly like below,

   DRM Framework
|
 Exynos DRM Framework
 /   |   \
  Real device drivers

 In particular, in case of ARM based DRM drivers with separated
 devices, I still tend to think it's better design than that device
 drivers implement the connector callbacks directly, but I will try to
 consider what is the better way.

>>>
>>> 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.

Sean

> Dave.

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

[Rob Clark]

On Wed, Oct 23, 2013 at 11:22 AM, Dave Airlie  wrote:
> On Wed, Oct 23, 2013 at 3:45 PM, Sean Paul  wrote:
>> On Wed, Oct 23, 2013 at 10:29 AM, Dave Airlie  wrote:
 As I mentioned earlier, display_ops is needed to have no any
 dependency of drm framework directly like below,

   DRM Framework
|
 Exynos DRM Framework
 /   |   \
  Real device drivers

 In particular, in case of ARM based DRM drivers with separated
 devices, I still tend to think it's better design than that device
 drivers implement the connector callbacks directly, but I will try to
 consider what is the better way.

>>>
>>> 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.

I think we do definitely need a way to group nodes so we know what
needs to be assembled into a drm driver.  I know folks seem to believe
that DT should be software agnostic, but maybe we just need some
grouper nodes that sit on the side and know how to map device DT nodes
to software.

(The real funny thing about needing that is..  well I've yet to see
someone be able to hot-unplug a block on a piece of silicon.)

BR,
-R

> Dave.
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rob Clark]

On Wed, Oct 23, 2013 at 9:18 AM, Inki Dae  wrote:
> Look at omapdrm, nouveau, and radeon drm drivers.


btw, please don't look at omapdrm as a "good" example in this regard.
The layering is really not a good idea and for a long time caused a
lot of problems, which we essentially solved by bypassing parts of the
layering.  I still think omapdss and omapdrm should be flattened into
a single drm driver, then net result would be a drop in # of lines of
code.  I wish there were folks like the Sean and St?phane who cared
enough to do this for omapdrm ;-)

Other drivers have some modularity to separate code that is
significantly different across genarations (but what form that takes
differs depending on what how the hw differs across generations).
This isn't a bad thing.  But you need to look at the end result.  For
example how I split out the phy code for the mdp4 code in msm (hdmi
and dsi phy differ between otherwise similar 28nm and 45nm parts, but
the rest of the display controller blocks are basically the same).

BR,
-R

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

[Sean Paul]

On Wed, Oct 23, 2013 at 10:29 AM, Dave Airlie  wrote:
>> As I mentioned earlier, display_ops is needed to have no any
>> dependency of drm framework directly like below,
>>
>>   DRM Framework
>>|
>> Exynos DRM Framework
>> /   |   \
>>  Real device drivers
>>
>> In particular, in case of ARM based DRM drivers with separated
>> devices, I still tend to think it's better design than that device
>> drivers implement the connector callbacks directly, but I will try to
>> consider what is the better way.
>>
>
> 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.
>
> Dave.

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

[Sean Paul]

On Wed, Oct 23, 2013 at 1:42 AM, Inki Dae  wrote:
> 2013/10/23 Sean Paul :
>> On Wed, Oct 23, 2013 at 12:48 AM, Inki Dae  wrote:
>>> 2013/10/23 St?phane Marchesin :
>>>>
>>>>
>>>>
>>>> On Tue, Oct 22, 2013 at 9:15 PM, Inki Dae  wrote:
>>>>>
>>>>> 2013/10/23 St?phane Marchesin :
>>>>> >
>>>>> >
>>>>> >
>>>>> > On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae  
>>>>> > wrote:
>>>>> >>
>>>>> >> 2013/10/23 St?phane Marchesin :
>>>>> >> >
>>>>> >> >
>>>>> >> >
>>>>> >> > On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae 
>>>>> >> > wrote:
>>>>> >> >>
>>>>> >> >> 2013/10/22 Sean Paul :
>>>>> >> >> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae >>>> >> >> > samsung.com>
>>>>> >> >> > wrote:
>>>>> >> >> >>
>>>>> >> >> >>
>>>>> >> >> >>> -Original Message-
>>>>> >> >> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>>> >> >> >>> Sent: Tuesday, October 22, 2013 6:18 AM
>>>>> >> >> >>> To: Inki Dae
>>>>> >> >> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>>>> >> >> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>>>>> >> >> >>> manager/display/subdrv
>>>>> >> >> >>>
>>>>> >> >> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul
>>>>> >> >> >>> 
>>>>> >> >> >>> wrote:
>>>>> >> >> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae
>>>>> >> >> >>> > 
>>>>> >> >> >>> > wrote:
>>>>> >> >> >>> >>
>>>>> >> >> >>> >>
>>>>> >> >> >>> >>> -Original Message-
>>>>> >> >> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>>> >> >> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>>>>> >> >> >>> >>> To: Inki Dae
>>>>> >> >> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>>>> >> >> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>>>>> >> >> >>> >>> manager/display/subdrv
>>>>> >> >> >>> >>>
>>>>> >> >> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae
>>>>> >> >> >>> >>> 
>>>>> >> >> >> wrote:
>>>>> >> >> >>> >>> >
>>>>> >> >> >>> >>> >
>>>>> >> >> >>> >>> >> -Original Message-
>>>>> >> >> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>>> >> >> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>>>>> >> >> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at 
>>>>> >> >> >>> >>> >> samsung.com
>>>>> >> >> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
>>>>> >> >> >>> >>> >> marcheu at chromium.org;
>>>>> >> >> >>> Sean
>>>>> >> >> >>> >>> >> Paul
>>>>> >> >> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
>>>>> >> >> >>> >>> >> manager/display/subdrv
>>>>> >> >> >>> >>> >>
>>>>> >> >> >>> >>> >> This patch splits display and manager from subdrv. The
>>>>

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

[Sean Paul]

On Wed, Oct 23, 2013 at 12:48 AM, Inki Dae  wrote:
> 2013/10/23 St?phane Marchesin :
>>
>>
>>
>> On Tue, Oct 22, 2013 at 9:15 PM, Inki Dae  wrote:
>>>
>>> 2013/10/23 St?phane Marchesin :
>>> >
>>> >
>>> >
>>> > On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae  wrote:
>>> >>
>>> >> 2013/10/23 St?phane Marchesin :
>>> >> >
>>> >> >
>>> >> >
>>> >> > On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae 
>>> >> > wrote:
>>> >> >>
>>> >> >> 2013/10/22 Sean Paul :
>>> >> >> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae 
>>> >> >> > wrote:
>>> >> >> >>
>>> >> >> >>
>>> >> >> >>> -Original Message-
>>> >> >> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>> >> >> >>> Sent: Tuesday, October 22, 2013 6:18 AM
>>> >> >> >>> To: Inki Dae
>>> >> >> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>> >> >> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>>> >> >> >>> manager/display/subdrv
>>> >> >> >>>
>>> >> >> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul
>>> >> >> >>> 
>>> >> >> >>> wrote:
>>> >> >> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae
>>> >> >> >>> > 
>>> >> >> >>> > wrote:
>>> >> >> >>> >>
>>> >> >> >>> >>
>>> >> >> >>> >>> -Original Message-
>>> >> >> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>> >> >> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>>> >> >> >>> >>> To: Inki Dae
>>> >> >> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>> >> >> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>>> >> >> >>> >>> manager/display/subdrv
>>> >> >> >>> >>>
>>> >> >> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae
>>> >> >> >>> >>> 
>>> >> >> >> wrote:
>>> >> >> >>> >>> >
>>> >> >> >>> >>> >
>>> >> >> >>> >>> >> -Original Message-
>>> >> >> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>>> >> >> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>>> >> >> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at 
>>> >> >> >>> >>> >> samsung.com
>>> >> >> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
>>> >> >> >>> >>> >> marcheu at chromium.org;
>>> >> >> >>> Sean
>>> >> >> >>> >>> >> Paul
>>> >> >> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
>>> >> >> >>> >>> >> manager/display/subdrv
>>> >> >> >>> >>> >>
>>> >> >> >>> >>> >> This patch splits display and manager from subdrv. The
>>> >> >> >>> >>> >> result
>>> >> >> >>> >>> >> is
>>> >> >> >>> that
>>> >> >> >>> >>> >> crtc functions can directly call into manager callbacks
>>> >> >> >>> >>> >> and
>>> >> >> >>> >>> >> encoder
>>> >> >> >>> >>> >> functions can directly call into display callbacks. This
>>> >> >> >>> >>> >> will
>>> >> >> >>> >>> >>

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

[Sean Paul]

On Tue, Oct 22, 2013 at 10:28 PM, Inki Dae  wrote:
> 2013/10/22 Sean Paul :
>> On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae  wrote:
>>>
>>>
>>>> -Original Message-
>>>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>> Sent: Tuesday, October 22, 2013 6:18 AM
>>>> To: Inki Dae
>>>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>>>
>>>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul  
>>>> wrote:
>>>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae  
>>>> > wrote:
>>>> >>
>>>> >>
>>>> >>> -Original Message-
>>>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>>>> >>> To: Inki Dae
>>>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>>> >>>
>>>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae 
>>> wrote:
>>>> >>> >
>>>> >>> >
>>>> >>> >> -Original Message-
>>>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>>>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>>>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
>>>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at 
>>>> >>> >> chromium.org;
>>>> Sean
>>>> >>> >> Paul
>>>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>>> >>> >>
>>>> >>> >> This patch splits display and manager from subdrv. The result is
>>>> that
>>>> >>> >> crtc functions can directly call into manager callbacks and encoder
>>>> >>> >> functions can directly call into display callbacks. This will allow
>>>> >>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi &
>>>> fimd/dp
>>>> >>> >> with common code.
>>>> >>> >>
>>>> >>> >> Signed-off-by: Sean Paul 
>>>> >>> >> ---
>>>> >>> >>
>>>> >>> >> Changes in v2:
>>>> >>> >>   - Pass display into display_ops instead of context
>>>> >>> >
>>>> >>> > Sorry but it seems like more reasonable to pass device object into
>>>> >>> > display_ops and manager_ops.
>>>> >>> >
>>>> >>>
>>>> >>>
>>>> >>> So you've changed your mind from when you said the following?
>>>> >>>
>>>> >>> >>> manager->ops->xxx(manager, ...);
>>>> >>> >>> display->ops->xxx(display, ...);
>>>> >>> >>>
>>>> >>> >>> Agree.
>>>> >>>
>>>> >>
>>>> >>
>>>> >> True. Before that, My comment was to pass device object into
>>>> display_ops and
>>>> >> manager_ops, and then you said the good solution is to pass manager and
>>>> >> display to each driver. At that time, I thought no matter how the
>>>> callback
>>>> >> is called if the framework doesn't call callbacks of each driver with
>>>> ctx.
>>>> >> So I agreed.
>>>> >>
>>>> >>
>>>> >>> It would have been nice if you had changed your mind *before* I
>>>> >>> reworked everything. At any rate, I think it's still the right thing
>>>> >>> to do.
>>>> >>
>>>> >> Really sorry about that. And I will add new patch for it so you don't
>>>> need
>>>> >> to concern about that.
>>>> >>
>>>> >>>
>>>> >>>
>>>> >>> > I'm not sure but display_ops could be implemented in other framework
>>>> >>> based
>>>> >>> > d

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

[Stéphane Marchesin]

On Tue, Oct 22, 2013 at 9:15 PM, Inki Dae  wrote:

> 2013/10/23 St?phane Marchesin :
> >
> >
> >
> > On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae  wrote:
> >>
> >> 2013/10/23 St?phane Marchesin :
> >> >
> >> >
> >> >
> >> > On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae 
> wrote:
> >> >>
> >> >> 2013/10/22 Sean Paul :
> >> >> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae 
> >> >> > wrote:
> >> >> >>
> >> >> >>
> >> >> >>> -Original Message-
> >> >> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
> >> >> >>> Sent: Tuesday, October 22, 2013 6:18 AM
> >> >> >>> To: Inki Dae
> >> >> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> >> >> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
> >> >> >>> manager/display/subdrv
> >> >> >>>
> >> >> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul <
> seanpaul at chromium.org>
> >> >> >>> wrote:
> >> >> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae <
> inki.dae at samsung.com>
> >> >> >>> > wrote:
> >> >> >>> >>
> >> >> >>> >>
> >> >> >>> >>> -Original Message-
> >> >> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
> >> >> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
> >> >> >>> >>> To: Inki Dae
> >> >> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> >> >> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
> >> >> >>> >>> manager/display/subdrv
> >> >> >>> >>>
> >> >> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae
> >> >> >>> >>> 
> >> >> >> wrote:
> >> >> >>> >>> >
> >> >> >>> >>> >
> >> >> >>> >>> >> -Original Message-
> >> >> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
> >> >> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
> >> >> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at 
> >> >> >>> >>> >> samsung.com
> >> >> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
> >> >> >>> >>> >> marcheu at chromium.org;
> >> >> >>> Sean
> >> >> >>> >>> >> Paul
> >> >> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
> >> >> >>> >>> >> manager/display/subdrv
> >> >> >>> >>> >>
> >> >> >>> >>> >> This patch splits display and manager from subdrv. The
> >> >> >>> >>> >> result
> >> >> >>> >>> >> is
> >> >> >>> that
> >> >> >>> >>> >> crtc functions can directly call into manager callbacks
> and
> >> >> >>> >>> >> encoder
> >> >> >>> >>> >> functions can directly call into display callbacks. This
> >> >> >>> >>> >> will
> >> >> >>> >>> >> allow
> >> >> >>> >>> >> us to remove the exynos_drm_hdmi shim and support
> mixer/hdmi
> >> >> >>> >>> >> &
> >> >> >>> fimd/dp
> >> >> >>> >>> >> with common code.
> >> >> >>> >>> >>
> >> >> >>> >>> >> Signed-off-by: Sean Paul 
> >> >> >>> >>> >> ---
> >> >> >>> >>> >>
> >> >> >>> >>> >> Changes in v2:
> >> >> >>> >>> >>   - Pass display into display_

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

[Stéphane Marchesin]

On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae  wrote:

> 2013/10/23 St?phane Marchesin :
> >
> >
> >
> > On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae  wrote:
> >>
> >> 2013/10/22 Sean Paul :
> >> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae 
> wrote:
> >> >>
> >> >>
> >> >>> -Original Message-
> >> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
> >> >>> Sent: Tuesday, October 22, 2013 6:18 AM
> >> >>> To: Inki Dae
> >> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> >> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
> manager/display/subdrv
> >> >>>
> >> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul 
> >> >>> wrote:
> >> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae 
> >> >>> > wrote:
> >> >>> >>
> >> >>> >>
> >> >>> >>> -Original Message-
> >> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
> >> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
> >> >>> >>> To: Inki Dae
> >> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> >> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
> >> >>> >>> manager/display/subdrv
> >> >>> >>>
> >> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae  >
> >> >> wrote:
> >> >>> >>> >
> >> >>> >>> >
> >> >>> >>> >> -Original Message-
> >> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
> >> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
> >> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
> >> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
> >> >>> >>> >> marcheu at chromium.org;
> >> >>> Sean
> >> >>> >>> >> Paul
> >> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
> >> >>> >>> >> manager/display/subdrv
> >> >>> >>> >>
> >> >>> >>> >> This patch splits display and manager from subdrv. The result
> >> >>> >>> >> is
> >> >>> that
> >> >>> >>> >> crtc functions can directly call into manager callbacks and
> >> >>> >>> >> encoder
> >> >>> >>> >> functions can directly call into display callbacks. This will
> >> >>> >>> >> allow
> >> >>> >>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi
> &
> >> >>> fimd/dp
> >> >>> >>> >> with common code.
> >> >>> >>> >>
> >> >>> >>> >> Signed-off-by: Sean Paul 
> >> >>> >>> >> ---
> >> >>> >>> >>
> >> >>> >>> >> Changes in v2:
> >> >>> >>> >>   - Pass display into display_ops instead of context
> >> >>> >>> >
> >> >>> >>> > Sorry but it seems like more reasonable to pass device object
> >> >>> >>> > into
> >> >>> >>> > display_ops and manager_ops.
> >> >>> >>> >
> >> >>> >>>
> >> >>> >>>
> >> >>> >>> So you've changed your mind from when you said the following?
> >> >>> >>>
> >> >>> >>> >>> manager->ops->xxx(manager, ...);
> >> >>> >>> >>> display->ops->xxx(display, ...);
> >> >>> >>> >>>
> >> >>> >>> >>> Agree.
> >> >>> >>>
> >> >>> >>
> >> >>> >>
> >> >>> >> True. Before that, My comment was to pass device object into
> >> >>> display_ops and
> >&

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

[Stéphane Marchesin]

On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae  wrote:

> 2013/10/22 Sean Paul :
> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae  wrote:
> >>
> >>
> >>> -Original Message-
> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
> >>> Sent: Tuesday, October 22, 2013 6:18 AM
> >>> To: Inki Dae
> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> >>>
> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul 
> wrote:
> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae 
> wrote:
> >>> >>
> >>> >>
> >>> >>> -Original Message-
> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
> >>> >>> To: Inki Dae
> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
> manager/display/subdrv
> >>> >>>
> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae 
> >> wrote:
> >>> >>> >
> >>> >>> >
> >>> >>> >> -Original Message-
> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
> marcheu at chromium.org;
> >>> Sean
> >>> >>> >> Paul
> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
> manager/display/subdrv
> >>> >>> >>
> >>> >>> >> This patch splits display and manager from subdrv. The result is
> >>> that
> >>> >>> >> crtc functions can directly call into manager callbacks and
> encoder
> >>> >>> >> functions can directly call into display callbacks. This will
> allow
> >>> >>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi &
> >>> fimd/dp
> >>> >>> >> with common code.
> >>> >>> >>
> >>> >>> >> Signed-off-by: Sean Paul 
> >>> >>> >> ---
> >>> >>> >>
> >>> >>> >> Changes in v2:
> >>> >>> >>   - Pass display into display_ops instead of context
> >>> >>> >
> >>> >>> > Sorry but it seems like more reasonable to pass device object
> into
> >>> >>> > display_ops and manager_ops.
> >>> >>> >
> >>> >>>
> >>> >>>
> >>> >>> So you've changed your mind from when you said the following?
> >>> >>>
> >>> >>> >>> manager->ops->xxx(manager, ...);
> >>> >>> >>> display->ops->xxx(display, ...);
> >>> >>> >>>
> >>> >>> >>> Agree.
> >>> >>>
> >>> >>
> >>> >>
> >>> >> True. Before that, My comment was to pass device object into
> >>> display_ops and
> >>> >> manager_ops, and then you said the good solution is to pass manager
> and
> >>> >> display to each driver. At that time, I thought no matter how the
> >>> callback
> >>> >> is called if the framework doesn't call callbacks of each driver
> with
> >>> ctx.
> >>> >> So I agreed.
> >>> >>
> >>> >>
> >>> >>> It would have been nice if you had changed your mind *before* I
> >>> >>> reworked everything. At any rate, I think it's still the right
> thing
> >>> >>> to do.
> >>> >>
> >>> >> Really sorry about that. And I will add new patch for it so you
> don't
> >>> need
> >>> >> to concern about that.
> >>> >>
> >>> >>>
> >>> >>>
> >>> >>> > I'm not sure but display_ops could be implemented in other
> framework
> 

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

[Inki Dae]

2013/10/17 Sean Paul :
> This patch splits display and manager from subdrv. The result is that
> crtc functions can directly call into manager callbacks and encoder
> functions can directly call into display callbacks. This will allow
> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
> with common code.
>
> Signed-off-by: Sean Paul 
> ---
>
> Changes in v2:
> - Pass display into display_ops instead of context
>
>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++-
>  drivers/gpu/drm/exynos/exynos_drm_core.c  | 181 ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 115 +--
>  drivers/gpu/drm/exynos/exynos_drm_crtc.h  |  20 +-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c   |  29 +-
>  drivers/gpu/drm/exynos/exynos_drm_drv.h   | 106 +++---
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258 ++-
>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
>  drivers/gpu/drm/exynos/exynos_drm_fb.c|   4 +-
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c  | 161 +
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c  | 449 
> --

Build error. it seems that you missed vidi module. Can you consider
vidi module also?

Thanks,
Inki Dae

>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h  |   2 +
>  drivers/gpu/drm/exynos/exynos_drm_plane.c |  15 +-
>  13 files changed, 466 insertions(+), 942 deletions(-)
>  delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>

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

[Inki Dae]

> -Original Message-
> From: Sean Paul [mailto:seanpaul at chromium.org]
> Sent: Tuesday, October 22, 2013 6:18 AM
> To: Inki Dae
> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> 
> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul  wrote:
> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae  wrote:
> >>
> >>
> >>> -Original Message-
> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
> >>> Sent: Thursday, October 17, 2013 11:37 PM
> >>> To: Inki Dae
> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> >>>
> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae 
wrote:
> >>> >
> >>> >
> >>> >> -Original Message-
> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at 
> >>> >> chromium.org;
> Sean
> >>> >> Paul
> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> >>> >>
> >>> >> This patch splits display and manager from subdrv. The result is
> that
> >>> >> crtc functions can directly call into manager callbacks and encoder
> >>> >> functions can directly call into display callbacks. This will allow
> >>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi &
> fimd/dp
> >>> >> with common code.
> >>> >>
> >>> >> Signed-off-by: Sean Paul 
> >>> >> ---
> >>> >>
> >>> >> Changes in v2:
> >>> >>   - Pass display into display_ops instead of context
> >>> >
> >>> > Sorry but it seems like more reasonable to pass device object into
> >>> > display_ops and manager_ops.
> >>> >
> >>>
> >>>
> >>> So you've changed your mind from when you said the following?
> >>>
> >>> >>> manager->ops->xxx(manager, ...);
> >>> >>> display->ops->xxx(display, ...);
> >>> >>>
> >>> >>> Agree.
> >>>
> >>
> >>
> >> True. Before that, My comment was to pass device object into
> display_ops and
> >> manager_ops, and then you said the good solution is to pass manager and
> >> display to each driver. At that time, I thought no matter how the
> callback
> >> is called if the framework doesn't call callbacks of each driver with
> ctx.
> >> So I agreed.
> >>
> >>
> >>> It would have been nice if you had changed your mind *before* I
> >>> reworked everything. At any rate, I think it's still the right thing
> >>> to do.
> >>
> >> Really sorry about that. And I will add new patch for it so you don't
> need
> >> to concern about that.
> >>
> >>>
> >>>
> >>> > I'm not sure but display_ops could be implemented in other framework
> >>> based
> >>> > driver such as CDF based lcd panel driver. So if you pass display -
> it's
> >>> > specific to exynos drm framework - into display_ops, the other
> framework
> >>> > based driver should include specific exynos drm header.
> >>> >
> >>>
> >>> AFAIK, CDF will not land in its current separate-from-drm form, we
> >>> don't need to worry about this. Furthermore, these ops should just go
> >>> away and become drm_crtc/drm_encoder/drm_connector funcs anyways.
> >>>
> >>
> >> Can you assure the display_ops never implemented in other framework
> based
> >> driver, not CDF? At any rate, I think all possibilities should be
> opened.
> >>
> >
> > I don't think we should let an RFC framework make the code more
> > complicated for unclear benefit. By removing manager/display entirely,
> > we can get rid of a *lot* of other code that is basically just
> > plumbing drm hooks (exynos_drm_connector is a good example).
> >
> 
> I hacked this up today to prove it out. Check out the top 5 commits i

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

[Inki Dae]

> -Original Message-
> From: Sean Paul [mailto:seanpaul at chromium.org]
> Sent: Monday, October 21, 2013 11:47 PM
> To: Inki Dae
> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> 
> On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae  wrote:
> >
> >
> >> -Original Message-
> >> From: Sean Paul [mailto:seanpaul at chromium.org]
> >> Sent: Thursday, October 17, 2013 11:37 PM
> >> To: Inki Dae
> >> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> >> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> >>
> >> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae  wrote:
> >> >
> >> >
> >> >> -Original Message-
> >> >> From: Sean Paul [mailto:seanpaul at chromium.org]
> >> >> Sent: Thursday, October 17, 2013 4:27 AM
> >> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
> >> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at 
> >> >> chromium.org;
> Sean
> >> >> Paul
> >> >> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> >> >>
> >> >> This patch splits display and manager from subdrv. The result is
> that
> >> >> crtc functions can directly call into manager callbacks and encoder
> >> >> functions can directly call into display callbacks. This will allow
> >> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi &
> fimd/dp
> >> >> with common code.
> >> >>
> >> >> Signed-off-by: Sean Paul 
> >> >> ---
> >> >>
> >> >> Changes in v2:
> >> >>   - Pass display into display_ops instead of context
> >> >
> >> > Sorry but it seems like more reasonable to pass device object into
> >> > display_ops and manager_ops.
> >> >
> >>
> >>
> >> So you've changed your mind from when you said the following?
> >>
> >> >>> manager->ops->xxx(manager, ...);
> >> >>> display->ops->xxx(display, ...);
> >> >>>
> >> >>> Agree.
> >>
> >
> >
> > True. Before that, My comment was to pass device object into display_ops
> and
> > manager_ops, and then you said the good solution is to pass manager and
> > display to each driver. At that time, I thought no matter how the
> callback
> > is called if the framework doesn't call callbacks of each driver with
> ctx.
> > So I agreed.
> >
> >
> >> It would have been nice if you had changed your mind *before* I
> >> reworked everything. At any rate, I think it's still the right thing
> >> to do.
> >
> > Really sorry about that. And I will add new patch for it so you don't
> need
> > to concern about that.
> >
> >>
> >>
> >> > I'm not sure but display_ops could be implemented in other framework
> >> based
> >> > driver such as CDF based lcd panel driver. So if you pass display -
> it's
> >> > specific to exynos drm framework - into display_ops, the other
> framework
> >> > based driver should include specific exynos drm header.
> >> >
> >>
> >> AFAIK, CDF will not land in its current separate-from-drm form, we
> >> don't need to worry about this. Furthermore, these ops should just go
> >> away and become drm_crtc/drm_encoder/drm_connector funcs anyways.
> >>
> >
> > Can you assure the display_ops never implemented in other framework
> based
> > driver, not CDF? At any rate, I think all possibilities should be
opened.
> >
> 
> I don't think we should let an RFC framework make the code more
> complicated for unclear benefit. By removing manager/display entirely,
> we can get rid of a *lot* of other code that is basically just
> plumbing drm hooks (exynos_drm_connector is a good example).
> 
> >>
> >> > And another one, the patch 6 passes manager object to manager_ops,
> and
> >> for
> >> > this, you made the manager object to be set to driver data;
> >> > platform_set_drvdata(pdev, &manager). That isn't reasonable.
> Generally,
> >> > driver_data would point to device driver's context object.
> >> >
> >>
> >> I'm not sure why this isn't reasonab

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

[Sean Paul]

On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae  wrote:
>
>
>> -Original Message-
>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> Sent: Tuesday, October 22, 2013 6:18 AM
>> To: Inki Dae
>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>
>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul  wrote:
>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae  wrote:
>> >>
>> >>
>> >>> -Original Message-
>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>> >>> To: Inki Dae
>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>> >>>
>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae 
> wrote:
>> >>> >
>> >>> >
>> >>> >> -Original Message-
>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at 
>> >>> >> chromium.org;
>> Sean
>> >>> >> Paul
>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>> >>> >>
>> >>> >> This patch splits display and manager from subdrv. The result is
>> that
>> >>> >> crtc functions can directly call into manager callbacks and encoder
>> >>> >> functions can directly call into display callbacks. This will allow
>> >>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi &
>> fimd/dp
>> >>> >> with common code.
>> >>> >>
>> >>> >> Signed-off-by: Sean Paul 
>> >>> >> ---
>> >>> >>
>> >>> >> Changes in v2:
>> >>> >>   - Pass display into display_ops instead of context
>> >>> >
>> >>> > Sorry but it seems like more reasonable to pass device object into
>> >>> > display_ops and manager_ops.
>> >>> >
>> >>>
>> >>>
>> >>> So you've changed your mind from when you said the following?
>> >>>
>> >>> >>> manager->ops->xxx(manager, ...);
>> >>> >>> display->ops->xxx(display, ...);
>> >>> >>>
>> >>> >>> Agree.
>> >>>
>> >>
>> >>
>> >> True. Before that, My comment was to pass device object into
>> display_ops and
>> >> manager_ops, and then you said the good solution is to pass manager and
>> >> display to each driver. At that time, I thought no matter how the
>> callback
>> >> is called if the framework doesn't call callbacks of each driver with
>> ctx.
>> >> So I agreed.
>> >>
>> >>
>> >>> It would have been nice if you had changed your mind *before* I
>> >>> reworked everything. At any rate, I think it's still the right thing
>> >>> to do.
>> >>
>> >> Really sorry about that. And I will add new patch for it so you don't
>> need
>> >> to concern about that.
>> >>
>> >>>
>> >>>
>> >>> > I'm not sure but display_ops could be implemented in other framework
>> >>> based
>> >>> > driver such as CDF based lcd panel driver. So if you pass display -
>> it's
>> >>> > specific to exynos drm framework - into display_ops, the other
>> framework
>> >>> > based driver should include specific exynos drm header.
>> >>> >
>> >>>
>> >>> AFAIK, CDF will not land in its current separate-from-drm form, we
>> >>> don't need to worry about this. Furthermore, these ops should just go
>> >>> away and become drm_crtc/drm_encoder/drm_connector funcs anyways.
>> >>>
>> >>
>> >> Can you assure the display_ops never implemented in other framework
>> based
>> >> driver, not CDF? At any rate, I think all possibi

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

[Sean Paul]

On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul  wrote:
> On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae  wrote:
>>
>>
>>> -Original Message-
>>> From: Sean Paul [mailto:seanpaul at chromium.org]
>>> Sent: Thursday, October 17, 2013 11:37 PM
>>> To: Inki Dae
>>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>>
>>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae  wrote:
>>> >
>>> >
>>> >> -Original Message-
>>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>>> >> Sent: Thursday, October 17, 2013 4:27 AM
>>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
>>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at 
>>> >> chromium.org; Sean
>>> >> Paul
>>> >> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>> >>
>>> >> This patch splits display and manager from subdrv. The result is that
>>> >> crtc functions can directly call into manager callbacks and encoder
>>> >> functions can directly call into display callbacks. This will allow
>>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
>>> >> with common code.
>>> >>
>>> >> Signed-off-by: Sean Paul 
>>> >> ---
>>> >>
>>> >> Changes in v2:
>>> >>   - Pass display into display_ops instead of context
>>> >
>>> > Sorry but it seems like more reasonable to pass device object into
>>> > display_ops and manager_ops.
>>> >
>>>
>>>
>>> So you've changed your mind from when you said the following?
>>>
>>> >>> manager->ops->xxx(manager, ...);
>>> >>> display->ops->xxx(display, ...);
>>> >>>
>>> >>> Agree.
>>>
>>
>>
>> True. Before that, My comment was to pass device object into display_ops and
>> manager_ops, and then you said the good solution is to pass manager and
>> display to each driver. At that time, I thought no matter how the callback
>> is called if the framework doesn't call callbacks of each driver with ctx.
>> So I agreed.
>>
>>
>>> It would have been nice if you had changed your mind *before* I
>>> reworked everything. At any rate, I think it's still the right thing
>>> to do.
>>
>> Really sorry about that. And I will add new patch for it so you don't need
>> to concern about that.
>>
>>>
>>>
>>> > I'm not sure but display_ops could be implemented in other framework
>>> based
>>> > driver such as CDF based lcd panel driver. So if you pass display - it's
>>> > specific to exynos drm framework - into display_ops, the other framework
>>> > based driver should include specific exynos drm header.
>>> >
>>>
>>> AFAIK, CDF will not land in its current separate-from-drm form, we
>>> don't need to worry about this. Furthermore, these ops should just go
>>> away and become drm_crtc/drm_encoder/drm_connector funcs anyways.
>>>
>>
>> Can you assure the display_ops never implemented in other framework based
>> driver, not CDF? At any rate, I think all possibilities should be opened.
>>
>
> I don't think we should let an RFC framework make the code more
> complicated for unclear benefit. By removing manager/display entirely,
> we can get rid of a *lot* of other code that is basically just
> plumbing drm hooks (exynos_drm_connector is a good example).
>

I hacked this up today to prove it out. Check out the top 5 commits in
https://github.com/crseanpaul/exynos-drm-next/commits/linux-next-exynos-staging.
It removes exynos_drm_connector in favor of just implementing
drm_connector directly. This same treatment should be done for
exynos_drm_encoder and exynos_drm_crtc, but I didn't get around to
doing this.

As you can see, it cuts out a lot of code and removes an entire
abstraction layer. Much nicer :)

Sean

>>>
>>> > And another one, the patch 6 passes manager object to manager_ops, and
>>> for
>>> > this, you made the manager object to be set to driver data;
>>> > platform_set_drvdata(pdev, &manager). That isn't reasonable. Generally,
>>> > driver_data would point to device driver's context object.
>>> >
&

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

[Sean Paul]

On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae  wrote:
>
>
>> -Original Message-
>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> Sent: Thursday, October 17, 2013 11:37 PM
>> To: Inki Dae
>> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>
>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae  wrote:
>> >
>> >
>> >> -Original Message-
>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >> Sent: Thursday, October 17, 2013 4:27 AM
>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at 
>> >> chromium.org; Sean
>> >> Paul
>> >> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>> >>
>> >> This patch splits display and manager from subdrv. The result is that
>> >> crtc functions can directly call into manager callbacks and encoder
>> >> functions can directly call into display callbacks. This will allow
>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
>> >> with common code.
>> >>
>> >> Signed-off-by: Sean Paul 
>> >> ---
>> >>
>> >> Changes in v2:
>> >>   - Pass display into display_ops instead of context
>> >
>> > Sorry but it seems like more reasonable to pass device object into
>> > display_ops and manager_ops.
>> >
>>
>>
>> So you've changed your mind from when you said the following?
>>
>> >>> manager->ops->xxx(manager, ...);
>> >>> display->ops->xxx(display, ...);
>> >>>
>> >>> Agree.
>>
>
>
> True. Before that, My comment was to pass device object into display_ops and
> manager_ops, and then you said the good solution is to pass manager and
> display to each driver. At that time, I thought no matter how the callback
> is called if the framework doesn't call callbacks of each driver with ctx.
> So I agreed.
>
>
>> It would have been nice if you had changed your mind *before* I
>> reworked everything. At any rate, I think it's still the right thing
>> to do.
>
> Really sorry about that. And I will add new patch for it so you don't need
> to concern about that.
>
>>
>>
>> > I'm not sure but display_ops could be implemented in other framework
>> based
>> > driver such as CDF based lcd panel driver. So if you pass display - it's
>> > specific to exynos drm framework - into display_ops, the other framework
>> > based driver should include specific exynos drm header.
>> >
>>
>> AFAIK, CDF will not land in its current separate-from-drm form, we
>> don't need to worry about this. Furthermore, these ops should just go
>> away and become drm_crtc/drm_encoder/drm_connector funcs anyways.
>>
>
> Can you assure the display_ops never implemented in other framework based
> driver, not CDF? At any rate, I think all possibilities should be opened.
>

I don't think we should let an RFC framework make the code more
complicated for unclear benefit. By removing manager/display entirely,
we can get rid of a *lot* of other code that is basically just
plumbing drm hooks (exynos_drm_connector is a good example).

>>
>> > And another one, the patch 6 passes manager object to manager_ops, and
>> for
>> > this, you made the manager object to be set to driver data;
>> > platform_set_drvdata(pdev, &manager). That isn't reasonable. Generally,
>> > driver_data would point to device driver's context object.
>> >
>>
>> I'm not sure why this isn't reasonable, but it's a moot point. The
>> driver data is only used up until we get rid of the pm ops, it needn't
>> be set at all once things go through dpms.
>>
>
> Generally, device drivers can call its own internal functions, and they will
> call that functions with ctx. However, if you set manager to driver_data
> then that functions should be called with manager object and also internally
> that functions should get ctx from the manager object. What is the purpose
> of manager? Do you think it's reasonable?
>

So, to avoid setting the manager as the drvdata, we could implement
something like fimd_dpms_ctx(ctx, mode) that takes ctx and the manager
callback calls it fimd_dpms(mgr, mode) { ctx = mgr->ctx;
fimd_dpms_ctx(ctx, mode); }. Alternatively, you can save a pointer to
mgr in ctx, but that creates a circular link between the two. IMO,
both of those solutions suck :)

I'd much rather just set drvdata to the manager and call the hook
directly. Like I said earlier, this is just a temporary step since we
remove these pm ops later in the patch series.

Sean


> Anyway, I'd like to say really sorry about inconvenient again. So I will fix
> it.
>
> Thanks,
> Inki Dae
>
>> Sean
>>
>>
>> > Thanks,
>> > Inki Dae
>> >
>

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

[Inki Dae]

> -Original Message-
> From: Sean Paul [mailto:seanpaul at chromium.org]
> Sent: Thursday, October 17, 2013 11:37 PM
> To: Inki Dae
> Cc: dri-devel; Dave Airlie; Tomasz Figa; St?phane Marchesin
> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> 
> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae  wrote:
> >
> >
> >> -Original Message-
> >> From: Sean Paul [mailto:seanpaul at chromium.org]
> >> Sent: Thursday, October 17, 2013 4:27 AM
> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at 
> >> chromium.org; Sean
> >> Paul
> >> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> >>
> >> This patch splits display and manager from subdrv. The result is that
> >> crtc functions can directly call into manager callbacks and encoder
> >> functions can directly call into display callbacks. This will allow
> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
> >> with common code.
> >>
> >> Signed-off-by: Sean Paul 
> >> ---
> >>
> >> Changes in v2:
> >>   - Pass display into display_ops instead of context
> >
> > Sorry but it seems like more reasonable to pass device object into
> > display_ops and manager_ops.
> >
> 
> 
> So you've changed your mind from when you said the following?
> 
> >>> manager->ops->xxx(manager, ...);
> >>> display->ops->xxx(display, ...);
> >>>
> >>> Agree.
> 


True. Before that, My comment was to pass device object into display_ops and
manager_ops, and then you said the good solution is to pass manager and
display to each driver. At that time, I thought no matter how the callback
is called if the framework doesn't call callbacks of each driver with ctx.
So I agreed.


> It would have been nice if you had changed your mind *before* I
> reworked everything. At any rate, I think it's still the right thing
> to do.

Really sorry about that. And I will add new patch for it so you don't need
to concern about that.

> 
> 
> > I'm not sure but display_ops could be implemented in other framework
> based
> > driver such as CDF based lcd panel driver. So if you pass display - it's
> > specific to exynos drm framework - into display_ops, the other framework
> > based driver should include specific exynos drm header.
> >
> 
> AFAIK, CDF will not land in its current separate-from-drm form, we
> don't need to worry about this. Furthermore, these ops should just go
> away and become drm_crtc/drm_encoder/drm_connector funcs anyways.
> 

Can you assure the display_ops never implemented in other framework based
driver, not CDF? At any rate, I think all possibilities should be opened.

> 
> > And another one, the patch 6 passes manager object to manager_ops, and
> for
> > this, you made the manager object to be set to driver data;
> > platform_set_drvdata(pdev, &manager). That isn't reasonable. Generally,
> > driver_data would point to device driver's context object.
> >
> 
> I'm not sure why this isn't reasonable, but it's a moot point. The
> driver data is only used up until we get rid of the pm ops, it needn't
> be set at all once things go through dpms.
> 

Generally, device drivers can call its own internal functions, and they will
call that functions with ctx. However, if you set manager to driver_data
then that functions should be called with manager object and also internally
that functions should get ctx from the manager object. What is the purpose
of manager? Do you think it's reasonable?

Anyway, I'd like to say really sorry about inconvenient again. So I will fix
it. 

Thanks,
Inki Dae

> Sean
> 
> 
> > Thanks,
> > Inki Dae
> >

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

[Inki Dae]

> -Original Message-
> From: Sean Paul [mailto:seanpaul at chromium.org]
> Sent: Thursday, October 17, 2013 4:27 AM
> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at chromium.org; 
> Sean
> Paul
> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
> 
> This patch splits display and manager from subdrv. The result is that
> crtc functions can directly call into manager callbacks and encoder
> functions can directly call into display callbacks. This will allow
> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
> with common code.
> 
> Signed-off-by: Sean Paul 
> ---
> 
> Changes in v2:
>   - Pass display into display_ops instead of context

Sorry but it seems like more reasonable to pass device object into
display_ops and manager_ops.

I'm not sure but display_ops could be implemented in other framework based
driver such as CDF based lcd panel driver. So if you pass display - it's
specific to exynos drm framework - into display_ops, the other framework
based driver should include specific exynos drm header.

And another one, the patch 6 passes manager object to manager_ops, and for
this, you made the manager object to be set to driver data;
platform_set_drvdata(pdev, &manager). That isn't reasonable. Generally,
driver_data would point to device driver's context object.

Thanks,
Inki Dae

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

[Sean Paul]

On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae  wrote:
>
>
>> -Original Message-
>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> Sent: Thursday, October 17, 2013 4:27 AM
>> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
>> Cc: airlied at linux.ie; tomasz.figa at gmail.com; marcheu at chromium.org; 
>> Sean
>> Paul
>> Subject: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>>
>> This patch splits display and manager from subdrv. The result is that
>> crtc functions can directly call into manager callbacks and encoder
>> functions can directly call into display callbacks. This will allow
>> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
>> with common code.
>>
>> Signed-off-by: Sean Paul 
>> ---
>>
>> Changes in v2:
>>   - Pass display into display_ops instead of context
>
> Sorry but it seems like more reasonable to pass device object into
> display_ops and manager_ops.
>


So you've changed your mind from when you said the following?

>>> manager->ops->xxx(manager, ...);
>>> display->ops->xxx(display, ...);
>>>
>>> Agree.

It would have been nice if you had changed your mind *before* I
reworked everything. At any rate, I think it's still the right thing
to do.


> I'm not sure but display_ops could be implemented in other framework based
> driver such as CDF based lcd panel driver. So if you pass display - it's
> specific to exynos drm framework - into display_ops, the other framework
> based driver should include specific exynos drm header.
>

AFAIK, CDF will not land in its current separate-from-drm form, we
don't need to worry about this. Furthermore, these ops should just go
away and become drm_crtc/drm_encoder/drm_connector funcs anyways.


> And another one, the patch 6 passes manager object to manager_ops, and for
> this, you made the manager object to be set to driver data;
> platform_set_drvdata(pdev, &manager). That isn't reasonable. Generally,
> driver_data would point to device driver's context object.
>

I'm not sure why this isn't reasonable, but it's a moot point. The
driver data is only used up until we get rid of the pm ops, it needn't
be set at all once things go through dpms.

Sean


> Thanks,
> Inki Dae
>

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

[Sean Paul]

This patch splits display and manager from subdrv. The result is that
crtc functions can directly call into manager callbacks and encoder
functions can directly call into display callbacks. This will allow
us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
with common code.

Signed-off-by: Sean Paul 
---

Changes in v2:
- Pass display into display_ops instead of context

 drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++-
 drivers/gpu/drm/exynos/exynos_drm_core.c  | 181 ---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 115 +--
 drivers/gpu/drm/exynos/exynos_drm_crtc.h  |  20 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c   |  29 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.h   | 106 +++---
 drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258 ++-
 drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
 drivers/gpu/drm/exynos/exynos_drm_fb.c|   4 +-
 drivers/gpu/drm/exynos/exynos_drm_fimd.c  | 161 +
 drivers/gpu/drm/exynos/exynos_drm_hdmi.c  | 449 --
 drivers/gpu/drm/exynos/exynos_drm_hdmi.h  |   2 +
 drivers/gpu/drm/exynos/exynos_drm_plane.c |  15 +-
 13 files changed, 466 insertions(+), 942 deletions(-)
 delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_hdmi.c

diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c 
b/drivers/gpu/drm/exynos/exynos_drm_connector.c
index ca270e2..9a16dbe 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_connector.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c
@@ -23,26 +23,20 @@
drm_connector)

 struct exynos_drm_connector {
-   struct drm_connectordrm_connector;
-   uint32_tencoder_id;
-   struct exynos_drm_manager *manager;
+   struct drm_connectordrm_connector;
+   uint32_tencoder_id;
+   struct exynos_drm_display   *display;
 };

 static int exynos_drm_connector_get_modes(struct drm_connector *connector)
 {
struct exynos_drm_connector *exynos_connector =
to_exynos_connector(connector);
-   struct exynos_drm_manager *manager = exynos_connector->manager;
-   struct exynos_drm_display_ops *display_ops = manager->display_ops;
+   struct exynos_drm_display *display = exynos_connector->display;
struct edid *edid = NULL;
unsigned int count = 0;
int ret;

-   if (!display_ops) {
-   DRM_DEBUG_KMS("display_ops is null.\n");
-   return 0;
-   }
-
/*
 * if get_edid() exists then get_edid() callback of hdmi side
 * is called to get edid data through i2c interface else
@@ -51,8 +45,8 @@ static int exynos_drm_connector_get_modes(struct 
drm_connector *connector)
 * P.S. in case of lcd panel, count is always 1 if success
 * because lcd panel has only one mode.
 */
-   if (display_ops->get_edid) {
-   edid = display_ops->get_edid(manager->dev, connector);
+   if (display->ops->get_edid) {
+   edid = display->ops->get_edid(display, connector);
if (IS_ERR_OR_NULL(edid)) {
ret = PTR_ERR(edid);
edid = NULL;
@@ -75,8 +69,8 @@ static int exynos_drm_connector_get_modes(struct 
drm_connector *connector)
return 0;
}

-   if (display_ops->get_panel)
-   panel = display_ops->get_panel(manager->dev);
+   if (display->ops->get_panel)
+   panel = display->ops->get_panel(display);
else {
drm_mode_destroy(connector->dev, mode);
return 0;
@@ -105,14 +99,13 @@ static int exynos_drm_connector_mode_valid(struct 
drm_connector *connector,
 {
struct exynos_drm_connector *exynos_connector =
to_exynos_connector(connector);
-   struct exynos_drm_manager *manager = exynos_connector->manager;
-   struct exynos_drm_display_ops *display_ops = manager->display_ops;
+   struct exynos_drm_display *display = exynos_connector->display;
int ret = MODE_BAD;

DRM_DEBUG_KMS("%s\n", __FILE__);

-   if (display_ops && display_ops->check_mode)
-   if (!display_ops->check_mode(manager->dev, mode))
+   if (display->ops->check_mode)
+   if (!display->ops->check_mode(display, mode))
ret = MODE_OK;

return ret;
@@ -151,8 +144,7 @@ static int exynos_drm_connector_fill_modes(struct 
drm_connector *connector,
 {
struct exynos_drm_connector *exynos_connector =
to_exynos_connector(connector);
-   struct exynos_drm_manager *manager = exynos_connector->manager;
-   struct exynos_drm_manager_ops *ops = manager->ops;
+   struct exynos_drm_display *display = exynos_connector->d