mbox

[RFC] DRM helpers for embedded systems

Message ID 1334158428-23735-1-git-send-email-s.hauer@pengutronix.de
State New
Headers show

Pull-request

git://git.pengutronix.de/git/imx/linux-2.6.git gpu/sdrm

Message

Sascha Hauer April 11, 2012, 3:33 p.m. UTC
Hi All,

The following series adds support for a new set of drm helpers called
sdrm. It is targeted to ease the implementation of drivers for embedded
systems. The basic idea is that instead of handling a comlete drm device
in each driver we introduce helpers which take care of the drm device
and most of the interfacing with the drm core. Users of these helpers
can register crtcs, encoders and connectors as separate devices which
reflects the hardware of embedded systems better than a monolithic drm
device. The patches are mostly based on the exynos driver. While writing
a driver for my devices I realized that I had to duplicate the bulk of
this driver, mostly replacing the exynos_ prefixes with imx_ prefixes,
hence the idea of creating a common infrastructure for this.

As a testbed driver a i.MX LCDC driver and a Intel (Marvell) PXA2xx
driver is included. Both are very simple last-decade-embedded-lcd-controllers.
The drivers have been tested with the xf86 modesetting driver, some libdrm
tests and a custom kms testing tool. Currently only the base framebuffers
are supported, but KMS plane support is definitely on my todo list.  The
drivers posted here are mostly created for demonstration purpose and to
give a template for other drivers, but the motivation for creating this
layer was the i.MX5/6 IPU (Image Processing Unit) which has two crts (four
on i.MX6), several on-SoC encoders and overlay possibilities. So this layer
should not be limited to the real 'simple' cases.

The sdrm patches currently have some limitations, but they should be
enough for being useful and to present it to a wider audience. Currently
no custom ioctls are supported, basically because I didn't need them
yet.  Also the pitch for the framebuffers are hardcoded by the sdrm
layer, this should be done in the crtc drivers instead.  Another thing
is that I currently have no idea what might be needed to support IOMMUs,
but maybe Thierry can help me out with his Tegra patches ;)

For a more complete branch including board support for a PXA27x based
Phytec board see this branch:

git://git.pengutronix.de/git/imx/linux-2.6.git gpu/sdrm-full

Comments very appreciated.

Sascha



The following changes since commit 0034102808e0dbbf3a2394b82b1bb40b5778de9e:

  Linux 3.4-rc2 (2012-04-07 18:30:41 -0700)

are available in the git repository at:

  git://git.pengutronix.de/git/imx/linux-2.6.git gpu/sdrm

for you to fetch changes up to fc3d0ff4825de998f1fd902184f7df040248d0de:

  DRM: add PXA kms simple driver (2012-04-11 17:10:46 +0200)

----------------------------------------------------------------
Philipp Zabel (1):
      DRM: add PXA kms simple driver

Sascha Hauer (6):
      drm: remove legacy mode_group handling
      drm: make gamma_set optional
      DRM: add sdrm layer for general embedded system support
      DRM: Add sdrm 1:1 encoder - connector helper
      DRM: add i.MX kms simple driver
      ARM i.MX27 pcm038: Add sdrm support

 arch/arm/mach-imx/pcm970-baseboard.c      |   78 ++-
 arch/arm/mach-pxa/include/mach/regs-lcd.h |    2 +
 arch/arm/plat-mxc/include/mach/imxfb.h    |    8 +-
 drivers/gpu/drm/Kconfig                   |    6 +
 drivers/gpu/drm/Makefile                  |    3 +
 drivers/gpu/drm/drm_crtc.c                |  163 ++----
 drivers/gpu/drm/drm_pci.c                 |    8 -
 drivers/gpu/drm/drm_platform.c            |    8 -
 drivers/gpu/drm/drm_usb.c                 |    6 -
 drivers/gpu/drm/imx/Kconfig               |    8 +
 drivers/gpu/drm/imx/Makefile              |    1 +
 drivers/gpu/drm/imx/imx-lcdc-crtc.c       |  506 ++++++++++++++++
 drivers/gpu/drm/pxa/Kconfig               |   12 +
 drivers/gpu/drm/pxa/Makefile              |    1 +
 drivers/gpu/drm/pxa/pxa-lcdc-crtc.c       |  845 +++++++++++++++++++++++++++
 drivers/gpu/drm/sdrm/Kconfig              |   11 +
 drivers/gpu/drm/sdrm/Makefile             |    5 +
 drivers/gpu/drm/sdrm/sdrm.c               |  904 +++++++++++++++++++++++++++++
 drivers/gpu/drm/sdrm/sdrm.h               |   57 ++
 drivers/gpu/drm/sdrm/sdrm_encon.c         |  211 +++++++
 drivers/gpu/drm/sdrm/sdrm_encon_dummy.c   |  193 ++++++
 drivers/gpu/drm/sdrm/sdrm_fb.c            |  191 ++++++
 drivers/gpu/drm/sdrm/sdrm_fbdev.c         |  238 ++++++++
 drivers/gpu/drm/sdrm/sdrm_gem.c           |  342 +++++++++++
 include/drm/drmP.h                        |    1 -
 include/drm/drm_crtc.h                    |   24 +-
 include/drm/sdrm.h                        |  102 ++++
 include/drm/sdrm_encon.h                  |   69 +++
 28 files changed, 3831 insertions(+), 172 deletions(-)
 create mode 100644 drivers/gpu/drm/imx/Kconfig
 create mode 100644 drivers/gpu/drm/imx/Makefile
 create mode 100644 drivers/gpu/drm/imx/imx-lcdc-crtc.c
 create mode 100644 drivers/gpu/drm/pxa/Kconfig
 create mode 100644 drivers/gpu/drm/pxa/Makefile
 create mode 100644 drivers/gpu/drm/pxa/pxa-lcdc-crtc.c
 create mode 100644 drivers/gpu/drm/sdrm/Kconfig
 create mode 100644 drivers/gpu/drm/sdrm/Makefile
 create mode 100644 drivers/gpu/drm/sdrm/sdrm.c
 create mode 100644 drivers/gpu/drm/sdrm/sdrm.h
 create mode 100644 drivers/gpu/drm/sdrm/sdrm_encon.c
 create mode 100644 drivers/gpu/drm/sdrm/sdrm_encon_dummy.c
 create mode 100644 drivers/gpu/drm/sdrm/sdrm_fb.c
 create mode 100644 drivers/gpu/drm/sdrm/sdrm_fbdev.c
 create mode 100644 drivers/gpu/drm/sdrm/sdrm_gem.c
 create mode 100644 include/drm/sdrm.h
 create mode 100644 include/drm/sdrm_encon.h

Comments

Alan Cox April 11, 2012, 8:22 p.m. UTC | #1
> +static int sdrm_suspend(struct drm_device *drm, pm_message_t state)
> +{
> +	/* TODO */
> +
> +	return 0;
> +}
> +
> +static int sdrm_resume(struct drm_device *drm)
> +{
> +	/* TODO */
> +
> +	return 0;
> +}

These probably need to call into the sdrm device specific handling.


> +static int sdrm_get_irq(struct drm_device *dev)
> +{
> +	/*
> +	 * Return an arbitrary number to make the core happy.
> +	 * We can't return anything meaningful here since drm
> +	 * devices in general have multiple irqs
> +	 */
> +	return 1234;
> +}

If there isn't a meaningful IRQ then surely 0 should be returned.
Actually I'd suggest returning sdrm->irq or similar, because some simple
DRM type use cases will have a single IRQ (notably 2 on older PC hardware)

> + * sdrm_device_get - find or allocate sdrm device with unique name
> + *
> + * This function returns the sdrm device with the unique name 'name'
> + * If this already exists, return it, otherwise allocate a new
> + * object.

This naming is a bit confusing because the kernel mid layers etc tend to
use _get and _put for ref counting not lookup ?


> +	/*
> +	 * enable drm irq mode.
> +	 * - with irq_enabled = 1, we can use the vblank feature.
> +	 *
> +	 * P.S. note that we wouldn't use drm irq handler but
> +	 *      just spsdrmific driver own one instead bsdrmause
> +	 *      drm framework supports only one irq handler and
> +	 *      drivers can well take care of their interrupts
> +	 */
> +	drm->irq_enabled = 1;

We've got a couple of assumptions here I think I'd question for generality

1. That its a platform device
2. That it can't use the standard IRQ helpers in some cases.

Probably it should take a struct device and a struct of the bits you'd
fish out from platform or pci or other device type. And yes probably
there would be a platform_ version that wraps it.


> +static int sdrm_fb_dirty(struct drm_framebuffer *fb,
> +		struct drm_file *file_priv, unsigned flags,
> +		unsigned color, struct drm_clip_rect *clips,
> +		unsigned num_clips)
> +{
> +	/* TODO */
> +
> +	return 0;
> +}

Probably a helper method.

> +static struct fb_ops sdrm_fb_ops = {
> +	.owner		= THIS_MODULE,
> +	.fb_fillrect	= cfb_fillrect,
> +	.fb_copyarea	= cfb_copyarea,
> +	.fb_imageblit	= cfb_imageblit,
> +	.fb_check_var	= drm_fb_helper_check_var,
> +	.fb_set_par	= drm_fb_helper_set_par,
> +	.fb_blank	= drm_fb_helper_blank,
> +	.fb_pan_display	= drm_fb_helper_pan_display,
> +	.fb_setcmap	= drm_fb_helper_setcmap,
> +};

If you re assuming any kind of gtt then you should probably allow for gtt
based scrolling eventually, but thats an optimisation.


> +int sdrm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> +{
> +	struct drm_gem_object *obj = vma->vm_private_data;
> +	struct sdrm_gem_obj *sdrm_gem_obj = to_sdrm_gem_obj(obj);
> +	struct drm_device *dev = obj->dev;
> +	unsigned long pfn;
> +	pgoff_t page_offset;
> +	int ret;

For dumb hardware take a look how gma500 and some other bits do this -
you can premap the entire buffer when you take the first fault, which for
a dumb fb is a good bet.



Looking at it from the point of view of x86 legacy devices then the
things I see are

- Device is quite possibly PCI (but may be platform eg vesa)
- Memory will probably be allocated in the PCI space
- Mappings are probably write combining but not on all hardware

There's probably a case for pinning/unpinning scanout buffers according
to whether they are used. On some hardware the io mapping needed is a
precious resource. Also for stuff with a fixed fb space it means you can
combine it with invalidating the mmap mappings of an object and copying
objects in/out of the frame buffer to provide the expected interfaces to
allocate/release framebuffers.

Alan
Sascha Hauer April 12, 2012, 8:58 a.m. UTC | #2
On Wed, Apr 11, 2012 at 09:22:47PM +0100, Alan Cox wrote:
> > +static int sdrm_suspend(struct drm_device *drm, pm_message_t state)
> > +{
> > +	/* TODO */
> > +
> > +	return 0;
> > +}
> > +
> > +static int sdrm_resume(struct drm_device *drm)
> > +{
> > +	/* TODO */
> > +
> > +	return 0;
> > +}
> 
> These probably need to call into the sdrm device specific handling.
> 
> 
> > +static int sdrm_get_irq(struct drm_device *dev)
> > +{
> > +	/*
> > +	 * Return an arbitrary number to make the core happy.
> > +	 * We can't return anything meaningful here since drm
> > +	 * devices in general have multiple irqs
> > +	 */
> > +	return 1234;
> > +}
> 
> If there isn't a meaningful IRQ then surely 0 should be returned.
> Actually I'd suggest returning sdrm->irq or similar, because some simple
> DRM type use cases will have a single IRQ (notably 2 on older PC hardware)

Hm, At the moment I can't even trigger this function to be called. I can
simply return 0 here. Returning a real irq does not sound sane since I
want the interrupt handled internally. Noone else has any business using
it.

> 
> > + * sdrm_device_get - find or allocate sdrm device with unique name
> > + *
> > + * This function returns the sdrm device with the unique name 'name'
> > + * If this already exists, return it, otherwise allocate a new
> > + * object.
> 
> This naming is a bit confusing because the kernel mid layers etc tend to
> use _get and _put for ref counting not lookup ?

Ok, lookup sounds better. Will rename it.

> 
> 
> > +	/*
> > +	 * enable drm irq mode.
> > +	 * - with irq_enabled = 1, we can use the vblank feature.
> > +	 *
> > +	 * P.S. note that we wouldn't use drm irq handler but
> > +	 *      just spsdrmific driver own one instead bsdrmause
> > +	 *      drm framework supports only one irq handler and
> > +	 *      drivers can well take care of their interrupts
> > +	 */
> > +	drm->irq_enabled = 1;
> 
> We've got a couple of assumptions here I think I'd question for generality
> 
> 1. That its a platform device
> 2. That it can't use the standard IRQ helpers in some cases.
> 
> Probably it should take a struct device and a struct of the bits you'd
> fish out from platform or pci or other device type. And yes probably
> there would be a platform_ version that wraps it.

I had a look and it turned out that I don't need anything specific to a
platform_device, so I can simply pass in a regular struct device here.
Having a platform_device here seems to be a leftover from earlier
versions in which I used the drm_platform stubs.

> 
> 
> > +static int sdrm_fb_dirty(struct drm_framebuffer *fb,
> > +		struct drm_file *file_priv, unsigned flags,
> > +		unsigned color, struct drm_clip_rect *clips,
> > +		unsigned num_clips)
> > +{
> > +	/* TODO */
> > +
> > +	return 0;
> > +}
> 
> Probably a helper method.

Yes.

> 
> > +static struct fb_ops sdrm_fb_ops = {
> > +	.owner		= THIS_MODULE,
> > +	.fb_fillrect	= cfb_fillrect,
> > +	.fb_copyarea	= cfb_copyarea,
> > +	.fb_imageblit	= cfb_imageblit,
> > +	.fb_check_var	= drm_fb_helper_check_var,
> > +	.fb_set_par	= drm_fb_helper_set_par,
> > +	.fb_blank	= drm_fb_helper_blank,
> > +	.fb_pan_display	= drm_fb_helper_pan_display,
> > +	.fb_setcmap	= drm_fb_helper_setcmap,
> > +};
> 
> If you re assuming any kind of gtt then you should probably allow for gtt
> based scrolling eventually, but thats an optimisation.

I'll keep that for later.

> 
> 
> > +int sdrm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > +{
> > +	struct drm_gem_object *obj = vma->vm_private_data;
> > +	struct sdrm_gem_obj *sdrm_gem_obj = to_sdrm_gem_obj(obj);
> > +	struct drm_device *dev = obj->dev;
> > +	unsigned long pfn;
> > +	pgoff_t page_offset;
> > +	int ret;
> 
> For dumb hardware take a look how gma500 and some other bits do this -
> you can premap the entire buffer when you take the first fault, which for
> a dumb fb is a good bet.
> 
> 
> 
> Looking at it from the point of view of x86 legacy devices then the
> things I see are
> 
> - Device is quite possibly PCI (but may be platform eg vesa)
> - Memory will probably be allocated in the PCI space
> - Mappings are probably write combining but not on all hardware
> 
> There's probably a case for pinning/unpinning scanout buffers according
> to whether they are used. On some hardware the io mapping needed is a
> precious resource. Also for stuff with a fixed fb space it means you can
> combine it with invalidating the mmap mappings of an object and copying
> objects in/out of the frame buffer to provide the expected interfaces to
> allocate/release framebuffers.

I'll have a look. Unfortunately my knowledge of these things is quite
limited. I am hoping a bit for Thierry here since he has a iommu on
Tegra and maybe this helps making the GEM support more generic.

Sascha
Dave Airlie April 20, 2012, 10:02 a.m. UTC | #3
On Wed, Apr 11, 2012 at 4:33 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> This patch adds support for creating simple drm devices. The
> basic idea of this patch is that drm drivers using the sdrm layer
> no longer have to implement a full drm device but instead only
> register crtcs, encoders and connectors with the sdrm layer. The
> sdrm layer then registers a drm device with the drm core and
> takes care of the drm device.

I'm sorry to say I totally hate this on every level. I think I said to
you before that midlayers are not the answer, and this is a hella big
midlayer.

I understand the SoC architecture but I can't think this is the way forward.

The problems I see from a highlevel:

This layer is highly restrictive on its users, I get the feeling once
you get to implement 2-3 complete drivers or try and implement a
driver say on x86 that should work in a similar fashion you are going
to realise you made things overly generic and the driver can't change
it. Then the drivers will start to workaround the midlayer and we'll
end up worse off. I don't really want to pick on specifics but the
taking over of the fb ops is on example,

I think this should work as a set of helpers that might work in place
of the current set of helpers. The current helpers are very directed
towards desktop x86 experience, so a new set of these might be better.

I get the feeling the drm can just be a virtual platform device of
some sort, then it reads the device tree and binds all the information
on what crtc/encoders are available,

Also the mode group stuff isn't legacy, the render nodes stuff posted
is what is intended to use it for, again it may not be useful on ARM,
but on desktop it has a very useful use case.

I'm sorry to not provide the answer I would fine acceptable, maybe if
I had a week of time to write something I could figure it out, maybe
someone else can give advice on how this sort of thing might look,
Linearo/ARM guys can some of you guys look at this?

Dave.
Thierry Reding April 20, 2012, 12:38 p.m. UTC | #4
* Dave Airlie wrote:
> I get the feeling the drm can just be a virtual platform device of
> some sort, then it reads the device tree and binds all the information
> on what crtc/encoders are available,

That's pretty much what I've come up with in the second round of Tegra DRM
patches. Basically display controllers and outputs (RGB, HDMI, TVO, DSI) get
separate drivers and register themselves with the DRM driver which then looks
at the device tree to see which display controllers to register as CRTCs and
parses a list of connector nodes to create encoder/connector pairs that
define the physical connectors and their corresponding outputs.

I did take a brief look at the SDRM patches as well and they didn't quite
seem to fit what was needed for Tegra. But if time allows I'll take a closer
look.

Thierry
Sascha Hauer April 20, 2012, 1:10 p.m. UTC | #5
[Added some embedded graphics maintainers to Cc who might be interested
in this]

On Fri, Apr 20, 2012 at 11:02:02AM +0100, Dave Airlie wrote:
> On Wed, Apr 11, 2012 at 4:33 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > This patch adds support for creating simple drm devices. The
> > basic idea of this patch is that drm drivers using the sdrm layer
> > no longer have to implement a full drm device but instead only
> > register crtcs, encoders and connectors with the sdrm layer. The
> > sdrm layer then registers a drm device with the drm core and
> > takes care of the drm device.
> 
> I'm sorry to say I totally hate this on every level. I think I said to
> you before that midlayers are not the answer, and this is a hella big
> midlayer.
> 
> I understand the SoC architecture but I can't think this is the way forward.
> 
> The problems I see from a highlevel:
> 
> This layer is highly restrictive on its users, I get the feeling once
> you get to implement 2-3 complete drivers or try and implement a
> driver say on x86 that should work in a similar fashion you are going
> to realise you made things overly generic and the driver can't change
> it.

That's maybe where our philosophies clash. I'd say that drivers should just
have enough freedom to get their job done whereas you want to give
drivers the freedom to do anything they want. I come from an area where
we have dozens of drivers which mostly are quite similar, in the ARM
world we currently try to get the duplication out of the drivers because
otherwise we can't handle the sheer amount of devices we have.

> Then the drivers will start to workaround the midlayer and we'll
> end up worse off. I don't really want to pick on specifics but the
> taking over of the fb ops is on example,

The layer can be extended when we need it, no need to work around it. I
took over the fb ops because currently I don't need to adjust them. I
know that some devices have accelerated blit operations and this means
that we may add a way to overwrite the ops later. Same with the ioctls.
I didn't provide a way to add device specific ioctls now, partly because
currently I don't need them, partly to get this series out.

> 
> I think this should work as a set of helpers that might work in place
> of the current set of helpers. The current helpers are very directed
> towards desktop x86 experience, so a new set of these might be better.

Hm, this means duplicating the helpers. The KMS support is to a great
extend defined by the helpers. Duplicating them would mean more code
fragmentation and different behaviour from a users point of view. I'd
rather not go this way.

> 
> I get the feeling the drm can just be a virtual platform device of
> some sort, then it reads the device tree and binds all the information
> on what crtc/encoders are available,

We can do that. Currently I use a string matching mechanism to tie
together the different pieces of a device, but sooner or later it will
be devicetree anyway, so no problem to convert this sooner instead of
later. The only problem I see with this is that for example X86 will not
have devicetree, so I see a value in not binding whatever we come up
with too tight to devicetree.

> 
> Also the mode group stuff isn't legacy, the render nodes stuff posted
> is what is intended to use it for, again it may not be useful on ARM,
> but on desktop it has a very useful use case.

I didn't remove them because they are not useful, but because currently
I couldn't add an encoder/connector to a active drm device (see how the
exynos driver currently works around this, I doubt it will work this way
once the legacy_mode_group handling is actually used). The fact that this
is currently unused motivated me to remove it. That said, I can live without
it, no problem.

> 
> I'm sorry to not provide the answer I would fine acceptable, maybe if
> I had a week of time to write something I could figure it out, maybe
> someone else can give advice on how this sort of thing might look,
> Linearo/ARM guys can some of you guys look at this?

Take these patches as my try to show that something has to change to
make drm stuff more widely usable on embedded devices. As said, there
are dozens of different devices out there, many of them are dumb
framebuffer devices like the PXA and i.MX driver I posted, others are
more advanced like the exynos, tegra and the newer i.MX devices. Some of
the bigger players can effort to write (and maintain?) a 300KB driver,
others can not. Those who cannot currently stay in drivers/video, but
this has limitations when it comes to overlay, hot pluggable monitors,
multiple crtcs, etc.

I'd like to find a solution for this which makes us all happy. In the
end reducing the amount of code duplication also helps you as a
maintainer.

(BTW each driver in drm has this layer somewhere in it. If I had hidden
it in imx specific functions I probably wouldn't have raised any
questions, but I don't want to go that way)

Sascha
Sascha Hauer April 20, 2012, 1:20 p.m. UTC | #6
On Fri, Apr 20, 2012 at 02:38:43PM +0200, Thierry Reding wrote:
> * Dave Airlie wrote:
> > I get the feeling the drm can just be a virtual platform device of
> > some sort, then it reads the device tree and binds all the information
> > on what crtc/encoders are available,
> 
> That's pretty much what I've come up with in the second round of Tegra DRM
> patches. Basically display controllers and outputs (RGB, HDMI, TVO, DSI) get
> separate drivers and register themselves with the DRM driver which then looks
> at the device tree to see which display controllers to register as CRTCs and
> parses a list of connector nodes to create encoder/connector pairs that
> define the physical connectors and their corresponding outputs.
> 
> I did take a brief look at the SDRM patches as well and they didn't quite
> seem to fit what was needed for Tegra. But if time allows I'll take a closer
> look.

Can you elaborate which parts don't fit? I am very interested to
improve this situation, and I think having code to share will be a
benefit for us all.

I know that the patches I wrote are no one-solution-fits-for-all yet,
they are mainly something to show that drm drivers do not have to be
huge complex drivers.

Sascha
Daniel Vetter April 20, 2012, 1:33 p.m. UTC | #7
On Fri, Apr 20, 2012 at 03:10:05PM +0200, Sascha Hauer wrote:
> (BTW each driver in drm has this layer somewhere in it. If I had hidden
> it in imx specific functions I probably wouldn't have raised any
> questions, but I don't want to go that way)

That's _exactly_ what you should be doing. And once you have more than one
driver that works in a similar way, you can extract the common code as
helper functions to make life easier. Like Rob&Alan did with a few gem
helpers they needed in omapdrm/gma500.

For your case it sounds like a new set of modeset helper functions
tailored for the embedded use-case would be good (as Dave Airlie
suggested). Adding yet another middle-layer (like sdrm is) that forces
drivers to go through it usually ends up in tears. And drm core
unfortunately still has too much middle-layer heritage: For an awful lot
of setup and teardown issues it's the core of the problme - because
drivers can't control when drm sets up and tears down certain things, it's
done at the wrong time (for certain drivers at least). Same problem when
the abstraction doesn't quite fit.

Helper functions leave the driver in full control of what's going on, and
working around hw-specific madness with ease.

https://lwn.net/Articles/336262/ is the canonical reference for why a lot
of kernel people are allergic to anything that looks like a middle-layer.

Yours, Daniel
Mark Brown April 20, 2012, 2:25 p.m. UTC | #8
On Fri, Apr 20, 2012 at 02:38:43PM +0200, Thierry Reding wrote:
> * Dave Airlie wrote:
> > I get the feeling the drm can just be a virtual platform device of
> > some sort, then it reads the device tree and binds all the information
> > on what crtc/encoders are available,

> That's pretty much what I've come up with in the second round of Tegra DRM
> patches. Basically display controllers and outputs (RGB, HDMI, TVO, DSI) get
> separate drivers and register themselves with the DRM driver which then looks
> at the device tree to see which display controllers to register as CRTCs and
> parses a list of connector nodes to create encoder/connector pairs that
> define the physical connectors and their corresponding outputs.

> I did take a brief look at the SDRM patches as well and they didn't quite
> seem to fit what was needed for Tegra. But if time allows I'll take a closer
> look.

This sounds an awful lot like how ASoC hangs together...
Thierry Reding April 20, 2012, 2:49 p.m. UTC | #9
* Mark Brown wrote:
> On Fri, Apr 20, 2012 at 02:38:43PM +0200, Thierry Reding wrote:
> > * Dave Airlie wrote:
> > > I get the feeling the drm can just be a virtual platform device of
> > > some sort, then it reads the device tree and binds all the information
> > > on what crtc/encoders are available,
> 
> > That's pretty much what I've come up with in the second round of Tegra DRM
> > patches. Basically display controllers and outputs (RGB, HDMI, TVO, DSI) get
> > separate drivers and register themselves with the DRM driver which then looks
> > at the device tree to see which display controllers to register as CRTCs and
> > parses a list of connector nodes to create encoder/connector pairs that
> > define the physical connectors and their corresponding outputs.
> 
> > I did take a brief look at the SDRM patches as well and they didn't quite
> > seem to fit what was needed for Tegra. But if time allows I'll take a closer
> > look.
> 
> This sounds an awful lot like how ASoC hangs together...

What in particular sounds awful?

Thierry
Mark Brown April 20, 2012, 3:06 p.m. UTC | #10
On Fri, Apr 20, 2012 at 04:49:43PM +0200, Thierry Reding wrote:
> * Mark Brown wrote:

> > This sounds an awful lot like how ASoC hangs together...

> What in particular sounds awful?

Nothing - "an awful" is an English idiom for "very".
Thierry Reding April 20, 2012, 3:13 p.m. UTC | #11
* Mark Brown wrote:
> On Fri, Apr 20, 2012 at 04:49:43PM +0200, Thierry Reding wrote:
> > * Mark Brown wrote:
> 
> > > This sounds an awful lot like how ASoC hangs together...
> 
> > What in particular sounds awful?
> 
> Nothing - "an awful" is an English idiom for "very".

I know =) But it has a somewhat negative connotation, from which I deduced
that you somehow thought it wasn't a good solution.

Thierry
Sascha Hauer April 20, 2012, 3:15 p.m. UTC | #12
On Fri, Apr 20, 2012 at 03:25:58PM +0100, Mark Brown wrote:
> On Fri, Apr 20, 2012 at 02:38:43PM +0200, Thierry Reding wrote:
> > * Dave Airlie wrote:
> > > I get the feeling the drm can just be a virtual platform device of
> > > some sort, then it reads the device tree and binds all the information
> > > on what crtc/encoders are available,
> 
> > That's pretty much what I've come up with in the second round of Tegra DRM
> > patches. Basically display controllers and outputs (RGB, HDMI, TVO, DSI) get
> > separate drivers and register themselves with the DRM driver which then looks
> > at the device tree to see which display controllers to register as CRTCs and
> > parses a list of connector nodes to create encoder/connector pairs that
> > define the physical connectors and their corresponding outputs.
> 
> > I did take a brief look at the SDRM patches as well and they didn't quite
> > seem to fit what was needed for Tegra. But if time allows I'll take a closer
> > look.
> 
> This sounds an awful lot like how ASoC hangs together...

Very much, yes. In ASoC and DRM we both have several physical devices spread
around the SoC which form a logical device. I assume that before ASoC existed
also everyone had a single PCI device which could be used to collect the
information together.

Sascha
Mark Brown April 20, 2012, 3:20 p.m. UTC | #13
On Fri, Apr 20, 2012 at 05:15:18PM +0200, Sascha Hauer wrote:
> On Fri, Apr 20, 2012 at 03:25:58PM +0100, Mark Brown wrote:

> > This sounds an awful lot like how ASoC hangs together...

> Very much, yes. In ASoC and DRM we both have several physical devices spread
> around the SoC which form a logical device. I assume that before ASoC existed
> also everyone had a single PCI device which could be used to collect the
> information together.

Yeah, it's a similar issue - on PC hardware we tend to have a single
integrated device which does everything (at least from the point of view
of the outside world, physically things may sometimes be split).
Sascha Hauer April 21, 2012, 8:18 a.m. UTC | #14
On Fri, Apr 20, 2012 at 03:33:14PM +0200, Daniel Vetter wrote:
> On Fri, Apr 20, 2012 at 03:10:05PM +0200, Sascha Hauer wrote:
> > (BTW each driver in drm has this layer somewhere in it. If I had hidden
> > it in imx specific functions I probably wouldn't have raised any
> > questions, but I don't want to go that way)
> 
> That's _exactly_ what you should be doing. And once you have more than one
> driver that works in a similar way, you can extract the common code as
> helper functions to make life easier.

I already have three drivers working in a similar way from which I
posted two. I could easily throw in some more into the pot. Also the
code is based on the exynos driver, so I think it's suitable for this
aswell.

> 
> For your case it sounds like a new set of modeset helper functions
> tailored for the embedded use-case would be good (as Dave Airlie
> suggested).

One of my problems is that currently drm is based on the assumption that
there is a single device which offers all needed resources and
information to form a drm device. On embedded systems this is simply not
the case, we have our resources all around the SoC. I have physical
devices which are crtcs, encoders or connectors, but drm does not
provide a way to glue them together. You can find this aswell in the
exynos driver if you grep for exynos_drm_subdrv_register. If you follow
this function you'll see that a good bunch of the driver actually
handles the management of these subdevices. Do you have a suggestion
solving the involved code duplication with helper functions?

Also sooner or later it will happen that the same hdmi controller is
used on two otherwise different SoCs. Currently the driver can't be
shared between SoCs because each hdmi driver implements exynos or
nouveau specific callbacks. I guess the answer is to put the common hdmi
driver code into helper functions and to implement a middle layer in
each drm driver wishing to use it.

> Adding yet another middle-layer (like sdrm is) that forces
> drivers to go through it usually ends up in tears. And drm core
> unfortunately still has too much middle-layer heritage: For an awful lot
> of setup and teardown issues it's the core of the problme - because
> drivers can't control when drm sets up and tears down certain things, it's
> done at the wrong time (for certain drivers at least). Same problem when
> the abstraction doesn't quite fit.
> 
> Helper functions leave the driver in full control of what's going on, and
> working around hw-specific madness with ease.
> 
> https://lwn.net/Articles/336262/ is the canonical reference for why a lot
> of kernel people are allergic to anything that looks like a middle-layer.

I have read the article when it was featured on LWN. While I agree to
several things I have my problems with it. Take for example the MMC
core. A MMC driver mainly has to implement two callbacks, .request and
.set_ios. Noone has ever asked to get direct access from the driver to
the underlying block device and eventually pass control to MMC helper
functions. This makes the MMC core a middle layer sitting between the
blockdevice and the driver.  With drm instead it's normal that ioctls
fall straight through to the driver. This leads to such funny things
that the kernel itself cannot control the device to bring a console on
the screen without dedicated help from the driver.

Sascha