Re: [PATCH] omap2+: add drm device

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 7433 bytes --]

On Mon, 2012-03-05 at 10:54 -0600, Rob Clark wrote:
> From: Andy Gross <andy.gross@ti.com>
> 
> Register OMAP DRM/KMS platform device, and reserve a CMA region for
> the device to use for buffer allocation.  DMM is split into a
> separate device using hwmod.
> 
> Signed-off-by: Andy Gross <andy.gross@ti.com>
> Signed-off-by: Rob Clark <rob@ti.com>
> ---
>  arch/arm/plat-omap/Makefile           |    2 +-
>  arch/arm/plat-omap/common.c           |    3 +-
>  arch/arm/plat-omap/drm.c              |   83 +++++++++++++++++++++++++++++++++
>  arch/arm/plat-omap/include/plat/drm.h |   64 +++++++++++++++++++++++++
>  4 files changed, 150 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/plat-omap/drm.c
>  create mode 100644 arch/arm/plat-omap/include/plat/drm.h
> 
> diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
> index 9a58461..b86e6cb 100644
> --- a/arch/arm/plat-omap/Makefile
> +++ b/arch/arm/plat-omap/Makefile
> @@ -4,7 +4,7 @@
>  
>  # Common support
>  obj-y := common.o sram.o clock.o devices.o dma.o mux.o \
> -	 usb.o fb.o counter_32k.o
> +	 usb.o fb.o counter_32k.o drm.o
>  obj-m :=
>  obj-n :=
>  obj-  :=
> diff --git a/arch/arm/plat-omap/common.c b/arch/arm/plat-omap/common.c
> index 06383b5..0d87dab 100644
> --- a/arch/arm/plat-omap/common.c
> +++ b/arch/arm/plat-omap/common.c
> @@ -21,10 +21,10 @@
>  #include <plat/board.h>
>  #include <plat/vram.h>
>  #include <plat/dsp.h>
> +#include <plat/drm.h>
>  
>  #include <plat/omap-secure.h>
>  
> -
>  #define NO_LENGTH_CHECK 0xffffffff
>  
>  struct omap_board_config_kernel *omap_board_config __initdata;
> @@ -65,6 +65,7 @@ const void *__init omap_get_var_config(u16 tag, size_t *len)
>  
>  void __init omap_reserve(void)
>  {
> +	omapdrm_reserve_vram();
>  	omapfb_reserve_sdram_memblock();
>  	omap_vram_reserve_sdram_memblock();
>  	omap_dsp_reserve_sdram_memblock();
> diff --git a/arch/arm/plat-omap/drm.c b/arch/arm/plat-omap/drm.c

As Tony said, mach-omap2 is probably a better place. fb.c is in
plat-omap because it supports both OMAP1 and OMAP2+.

> new file mode 100644
> index 0000000..28279df
> --- /dev/null
> +++ b/arch/arm/plat-omap/drm.c
> @@ -0,0 +1,83 @@
> +/*
> + * DRM/KMS device registration for TI OMAP platforms
> + *
> + * Copyright (C) 2012 Texas Instruments
> + * Author: Rob Clark <rob.clark@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
> +#ifdef CONFIG_CMA
> +#  include <linux/dma-contiguous.h>
> +#endif
> +
> +#include <plat/omap_device.h>
> +#include <plat/omap_hwmod.h>
> +
> +#include <plat/drm.h>
> +
> +#if defined(CONFIG_DRM_OMAP) || (CONFIG_DRM_OMAP_MODULE)
> +
> +static struct omap_drm_platform_data omapdrm_platdata;
> +
> +static struct platform_device omap_drm_device = {
> +		.dev = {
> +			.coherent_dma_mask = DMA_BIT_MASK(32),
> +			.platform_data = &omapdrm_platdata,
> +		},
> +		.name = "omapdrm",
> +		.id = 0,

Can there be more than one omapdrm device? I guess not. If so, the id
should be -1.

> +};
> +
> +static int __init omap_init_gpu(void)
> +{
> +	struct omap_hwmod *oh = NULL;
> +	struct platform_device *pdev;
> +
> +	/* lookup and populate the DMM information, if present - OMAP4+ */
> +	oh = omap_hwmod_lookup("dmm");
> +
> +	if (oh) {
> +		pdev = omap_device_build(oh->name, -1, oh, NULL, 0, NULL, 0,
> +					false);
> +		WARN(IS_ERR(pdev), "Could not build omap_device for %s\n",
> +			oh->name);
> +	}
> +
> +	return platform_device_register(&omap_drm_device);
> +
> +}

The function is called omap_init_gpu(), but it doesn't do anything
related to the gpu... And aren't DMM and DRM totally separate things,
why are they bunched together like that?

> +arch_initcall(omap_init_gpu);
> +
> +void omapdrm_reserve_vram(void)
> +{
> +#ifdef CONFIG_CMA
> +	/*
> +	 * Create private 32MiB contiguous memory area for omapdrm.0 device
> +	 * TODO revisit size.. if uc/wc buffers are allocated from CMA pages
> +	 * then the amount of memory we need goes up..
> +	 */
> +	dma_declare_contiguous(&omap_drm_device.dev, 32 * SZ_1M, 0, 0);

What does this actually do? Does it reserve the memory, i.e. it's not
usable for others? If so, shouldn't there be a way for the user to
configure it?

> +#else
> +#  warning "CMA is not enabled, there may be limitations about scanout buffer allocations on OMAP3 and earlier"
> +#endif
> +}
> +
> +#endif
> diff --git a/arch/arm/plat-omap/include/plat/drm.h b/arch/arm/plat-omap/include/plat/drm.h
> new file mode 100644
> index 0000000..df9bc41
> --- /dev/null
> +++ b/arch/arm/plat-omap/include/plat/drm.h
> @@ -0,0 +1,64 @@
> +/*
> + * DRM/KMS device registration for TI OMAP platforms
> + *
> + * Copyright (C) 2012 Texas Instruments
> + * Author: Rob Clark <rob.clark@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __PLAT_OMAP_DRM_H__
> +#define __PLAT_OMAP_DRM_H__
> +
> +/*
> + * Optional platform data to configure the default configuration of which
> + * pipes/overlays/CRTCs are used.. if this is not provided, then instead the
> + * first CONFIG_DRM_OMAP_NUM_CRTCS are used, and they are each connected to
> + * one manager, with priority given to managers that are connected to
> + * detected devices.  Remaining overlays are used as video planes.  This
> + * should be a good default behavior for most cases, but yet there still
> + * might be times when you wish to do something different.
> + */
> +struct omap_kms_platform_data {
> +	/* overlays to use as CRTCs: */
> +	int ovl_cnt;
> +	const int *ovl_ids;
> +
> +	/* overlays to use as video planes: */
> +	int pln_cnt;
> +	const int *pln_ids;
> +
> +	int mgr_cnt;
> +	const int *mgr_ids;
> +
> +	int dev_cnt;
> +	const char **dev_names;
> +};
> +
> +struct omap_drm_platform_data {
> +	struct omap_kms_platform_data *kms_pdata;
> +};

I don't know if there's need to add these... With device tree the
plaform data will not be usable anyway.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]