Patchwork [RFC] i.MX DRM support

login
register
mail settings
Submitter Sascha Hauer
Date June 14, 2012, 1:43 p.m.
Message ID <1339681407-28721-1-git-send-email-s.hauer@pengutronix.de>
Download mbox
Permalink /patch/164932/
State New
Headers show

Pull-request

git://git.pengutronix.de/git/imx/linux-2.6.git work/gpu/imx-drm-ipu

Comments

Sascha Hauer - June 14, 2012, 1:43 p.m.
Hi All,

The following is the state-of-the-art i.MX IPU (Image Processing Unit)
DRM support.

This code is around for quite some time now and has been posted several
times with different APIs, first with plain old framebuffer support, now
DRM, first platform device binding, now devicetree. Unfortunately there's
quite much code needed to get something useful out of the IPU, so these
patches haven't received a lot of attention from people not involved in
i.MX. I think we have now come to a point where this code needs more public
exposure and where it's easier to talk in incremental changes instead of
blobs. Therefore I request this to go to staging for some cycles. This
would allow us to have something in mainline as a base for further discussion
while still being able to change bigger amounts of the driver without breaking
officially supported features. What do you think about this?

The IPU driver generally works on i.MX51, i.MX53 and i.MX6. The latter has
two IPUs, the driver is able to handle them both.

This series only contains the IPU driver. There are some additional glue
code patches necessary. Also necessary are two drm CMA/GEM helper patches
currently waiting for review on dri-devel. For testing the driver please
use this branch:

git://git.pengutronix.de/git/imx/linux-2.6.git work/gpu/imx-drm-ipu-complete

The branch contains board support for the i.MX51 babbage (both digital and
analog DVI outputs are supported) and i.MX6q armadillo2 (HDMI). Compile
with imx_v6_v7_defconfig.

For discussion of the devicetree binding please refer to the seperate mail
I sent.

As usual, all comments/suggestions welcome

Sascha


The following changes since commit cfaf025112d3856637ff34a767ef785ef5cf2ca9:

  Linux 3.5-rc2 (2012-06-08 18:40:09 -0700)

are available in the git repository at:

  git://git.pengutronix.de/git/imx/linux-2.6.git work/gpu/imx-drm-ipu

for you to fetch changes up to 6c25e018285a93053daa69786faa8b3fe43597be:

  DRM i.MX: Add devicetree binding documentation (2012-06-14 14:30:52 +0200)

----------------------------------------------------------------
Philipp Zabel (1):
      DRM i.MX: Add devicetree binding documentation

Sascha Hauer (5):
      DRM: Add i.MX drm core support
      DRM i.MX: Add parallel display support
      DRM i.MX: Add LCDC support
      DRM: add i.MX IPUv3 base driver
      DRM: Add i.MX IPUv3 crtc support

 .../devicetree/bindings/drm/fsl-imx-drm.txt        |  126 +++
 drivers/gpu/drm/Kconfig                            |    2 +
 drivers/gpu/drm/Makefile                           |    1 +
 drivers/gpu/drm/imx/Kconfig                        |   40 +
 drivers/gpu/drm/imx/Makefile                       |   10 +
 drivers/gpu/drm/imx/imx-drm-core.c                 |  891 ++++++++++++++++
 drivers/gpu/drm/imx/imx-drm.h                      |   58 +
 drivers/gpu/drm/imx/imx-fb.c                       |   47 +
 drivers/gpu/drm/imx/imx-fbdev.c                    |   65 ++
 drivers/gpu/drm/imx/imx-ipuv3-crtc.c               |  579 ++++++++++
 drivers/gpu/drm/imx/imx-lcdc-crtc.c                |  523 +++++++++
 drivers/gpu/drm/imx/imx-parallel-display.c         |  255 +++++
 drivers/gpu/drm/imx/ipu-v3/Makefile                |    3 +
 drivers/gpu/drm/imx/ipu-v3/ipu-common.c            | 1111 ++++++++++++++++++++
 drivers/gpu/drm/imx/ipu-v3/ipu-dc.c                |  384 +++++++
 drivers/gpu/drm/imx/ipu-v3/ipu-di.c                |  695 ++++++++++++
 drivers/gpu/drm/imx/ipu-v3/ipu-dmfc.c              |  410 ++++++++
 drivers/gpu/drm/imx/ipu-v3/ipu-dp.c                |  336 ++++++
 drivers/gpu/drm/imx/ipu-v3/ipu-prv.h               |  211 ++++
 include/drm/imx-ipu-v3.h                           |  320 ++++++
 20 files changed, 6067 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/drm/fsl-imx-drm.txt
 create mode 100644 drivers/gpu/drm/imx/Kconfig
 create mode 100644 drivers/gpu/drm/imx/Makefile
 create mode 100644 drivers/gpu/drm/imx/imx-drm-core.c
 create mode 100644 drivers/gpu/drm/imx/imx-drm.h
 create mode 100644 drivers/gpu/drm/imx/imx-fb.c
 create mode 100644 drivers/gpu/drm/imx/imx-fbdev.c
 create mode 100644 drivers/gpu/drm/imx/imx-ipuv3-crtc.c
 create mode 100644 drivers/gpu/drm/imx/imx-lcdc-crtc.c
 create mode 100644 drivers/gpu/drm/imx/imx-parallel-display.c
 create mode 100644 drivers/gpu/drm/imx/ipu-v3/Makefile
 create mode 100644 drivers/gpu/drm/imx/ipu-v3/ipu-common.c
 create mode 100644 drivers/gpu/drm/imx/ipu-v3/ipu-dc.c
 create mode 100644 drivers/gpu/drm/imx/ipu-v3/ipu-di.c
 create mode 100644 drivers/gpu/drm/imx/ipu-v3/ipu-dmfc.c
 create mode 100644 drivers/gpu/drm/imx/ipu-v3/ipu-dp.c
 create mode 100644 drivers/gpu/drm/imx/ipu-v3/ipu-prv.h
 create mode 100644 include/drm/imx-ipu-v3.h
Fabio Estevam - June 19, 2012, 6:20 p.m.
On Thu, Jun 14, 2012 at 10:43 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Hi All,
>
> The following is the state-of-the-art i.MX IPU (Image Processing Unit)
> DRM support.
>
> This code is around for quite some time now and has been posted several
> times with different APIs, first with plain old framebuffer support, now
> DRM, first platform device binding, now devicetree. Unfortunately there's
> quite much code needed to get something useful out of the IPU, so these
> patches haven't received a lot of attention from people not involved in
> i.MX. I think we have now come to a point where this code needs more public
> exposure and where it's easier to talk in incremental changes instead of
> blobs. Therefore I request this to go to staging for some cycles. This
> would allow us to have something in mainline as a base for further discussion
> while still being able to change bigger amounts of the driver without breaking
> officially supported features. What do you think about this?

+1 on this.

Thanks for your hard work on this, Sascha.

Regards,

Fabio Estevam
Shawn Guo - June 21, 2012, 5:30 a.m.
On Thu, Jun 14, 2012 at 03:43:23PM +0200, Sascha Hauer wrote:
...
> +struct drm_device *imx_drm_device_get(void)
> +{
> +	struct imx_drm_device *imxdrm = __imx_drm_device();
> +	struct imx_drm_encoder *enc;
> +	struct imx_drm_connector *con;
> +	struct imx_drm_crtc *crtc;
> +
> +	mutex_lock(&imxdrm->mutex);
> +
> +	list_for_each_entry(enc, &imxdrm->encoder_list, list) {
> +		if (!try_module_get(enc->owner)) {
> +			dev_err(imxdrm->dev, "could not get module %s\n",
> +					module_name(enc->owner));
> +			goto unwind_enc;
> +		}
> +	}
> +
> +	list_for_each_entry(con, &imxdrm->connector_list, list) {
> +		if (!try_module_get(con->owner)) {
> +			dev_err(imxdrm->dev, "could not get module %s\n",
> +					module_name(con->owner));
> +			goto unwind_con;
> +		}
> +	}
> +
> +	list_for_each_entry(crtc, &imxdrm->crtc_list, list) {
> +		if (!try_module_get(crtc->owner)) {
> +			dev_err(imxdrm->dev, "could not get module %s\n",
> +					module_name(crtc->owner));
> +			goto unwind_crtc;
> +		}
> +	}
> +
> +	imxdrm->references++;
> +
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return imx_drm_device->drm;

Though I'm not quite sure about the point of retrieving the static
variable imx_drm_device from calling __imx_drm_device(), shouldn't
imxdrm be used here since you already retrieve it?

> +
> +unwind_crtc:
> +	list_for_each_entry_continue_reverse(crtc, &imxdrm->crtc_list, list)
> +		module_put(crtc->owner);
> +unwind_con:
> +	list_for_each_entry_continue_reverse(con, &imxdrm->connector_list, list)
> +		module_put(con->owner);
> +unwind_enc:
> +	list_for_each_entry_continue_reverse(enc, &imxdrm->encoder_list, list)
> +		module_put(enc->owner);
> +
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return NULL;
> +
> +}
> +EXPORT_SYMBOL_GPL(imx_drm_device_get);

...

> +/*
> + * register a connector to the drm core
> + */
> +static int imx_drm_connector_register(
> +		struct imx_drm_connector *imx_drm_connector)
> +{
> +	struct imx_drm_device *imxdrm = __imx_drm_device();
> +	int ret;
> +
> +	drm_connector_init(imxdrm->drm, imx_drm_connector->connector,
> +			imx_drm_connector->connector->funcs,
> +			imx_drm_connector->connector->connector_type);
> +	drm_mode_group_reinit(imxdrm->drm);
> +	ret = drm_sysfs_connector_add(imx_drm_connector->connector);
> +	if (ret)
> +		goto err;

Simply return ret to save the err path?

> +
> +	return 0;
> +err:
> +
> +	return ret;
> +}

...

> +/*
> + * imx_drm_add_encoder - add a new encoder
> + */
> +int imx_drm_add_encoder(struct drm_encoder *encoder,
> +		struct imx_drm_encoder **newenc, struct module *owner)
> +{
> +	struct imx_drm_device *imxdrm = __imx_drm_device();
> +	struct imx_drm_encoder *imx_drm_encoder;
> +	int ret;
> +
> +	mutex_lock(&imxdrm->mutex);
> +
> +	if (imxdrm->references) {
> +		ret = -EBUSY;
> +		goto err_busy;
> +	}
> +
> +	imx_drm_encoder = kzalloc(sizeof(struct imx_drm_encoder), GFP_KERNEL);

sizeof(*imx_drm_encoder)

> +	if (!imx_drm_encoder) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	imx_drm_encoder->encoder = encoder;
> +	imx_drm_encoder->owner = owner;
> +
> +	ret = imx_drm_encoder_register(imx_drm_encoder);
> +	if (ret) {
> +		kfree(imx_drm_encoder);
> +		ret = -ENOMEM;
> +		goto err_register;
> +	}
> +
> +	list_add_tail(&imx_drm_encoder->list, &imxdrm->encoder_list);
> +
> +	*newenc = imx_drm_encoder;
> +
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return 0;
> +
> +err_register:
> +	kfree(imx_drm_encoder);
> +err_alloc:
> +err_busy:
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(imx_drm_add_encoder);

...

> +/*
> + * imx_drm_add_connector - add a connector
> + */
> +int imx_drm_add_connector(struct drm_connector *connector,
> +		struct imx_drm_connector **new_con,
> +		struct module *owner)
> +{
> +	struct imx_drm_device *imxdrm = __imx_drm_device();
> +	struct imx_drm_connector *imx_drm_connector;
> +	int ret;
> +
> +	mutex_lock(&imxdrm->mutex);
> +
> +	if (imxdrm->references) {
> +		ret = -EBUSY;
> +		goto err_busy;
> +	}
> +
> +	imx_drm_connector = kzalloc(sizeof(struct imx_drm_connector),

sizeof(*imx_drm_connector)

> +			GFP_KERNEL);
> +	if (!imx_drm_connector) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	imx_drm_connector->connector = connector;
> +	imx_drm_connector->owner = owner;
> +
> +	ret = imx_drm_connector_register(imx_drm_connector);
> +	if (ret)
> +		goto err_register;
> +
> +	list_add_tail(&imx_drm_connector->list, &imxdrm->connector_list);
> +
> +	*new_con = imx_drm_connector;
> +
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return 0;
> +
> +err_register:
> +	kfree(imx_drm_connector);
> +err_alloc:
> +err_busy:
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(imx_drm_add_connector);

...

> +static int __init imx_drm_init(void)
> +{
> +	int ret;
> +
> +	imx_drm_device = kzalloc(sizeof(*imx_drm_device), GFP_KERNEL);
> +	if (!imx_drm_device)
> +		return -ENOMEM;
> +
> +	mutex_init(&imx_drm_device->mutex);
> +	INIT_LIST_HEAD(&imx_drm_device->crtc_list);
> +	INIT_LIST_HEAD(&imx_drm_device->connector_list);
> +	INIT_LIST_HEAD(&imx_drm_device->encoder_list);
> +
> +	imx_drm_pdev = platform_device_register_simple("imx-drm", -1, NULL, 0);
> +	if (!imx_drm_pdev) {
> +		ret = -EINVAL;
> +		goto err_pdev;
> +	}
> +
> +	imx_drm_pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32),
> +
> +	ret = platform_driver_register(&imx_drm_pdrv);
> +	if (ret)
> +		goto err_pdrv;
> +
> +	return 0;
> +
> +err_pdev:
> +	kfree(imx_drm_device);
> +err_pdrv:
> +	platform_device_unregister(imx_drm_pdev);

The order between these two blocks looks wrong.

> +
> +	return ret;
> +}

...

> +static int __init imx_fb_helper_init(void)
> +{
> +	struct drm_device *drm = imx_drm_device_get();
> +
> +	if (!drm)
> +		return -EINVAL;
> +
> +	fbdev_cma = drm_fbdev_cma_init(drm, PREFERRED_BPP,
> +			drm->mode_config.num_crtc, MAX_CONNECTOR);
> +
> +	imx_drm_fb_helper_set(fbdev_cma);
> +
> +	if (IS_ERR(fbdev_cma)) {
> +		imx_drm_device_put();
> +		return PTR_ERR(fbdev_cma);
> +	}

Shouldn't this error check be put right after drm_fbdev_cma_init call?

> +
> +	return 0;
> +}
Shawn Guo - June 21, 2012, 5:35 a.m.
On Thu, Jun 14, 2012 at 03:43:24PM +0200, Sascha Hauer wrote:
> +static const struct of_device_id imx_pd_dt_ids[] = {
> +	{ .compatible = "fsl,imx-parallel-display", .data = NULL, },

Can we use particular soc name to define the compatible string?  Also,
the .data initialization seems not needed.

> +	{ /* sentinel */ }
> +};
Shawn Guo - June 21, 2012, 6:07 a.m.
On Thu, Jun 21, 2012 at 01:35:56PM +0800, Shawn Guo wrote:
> On Thu, Jun 14, 2012 at 03:43:24PM +0200, Sascha Hauer wrote:
> > +static const struct of_device_id imx_pd_dt_ids[] = {
> > +	{ .compatible = "fsl,imx-parallel-display", .data = NULL, },
> 
> Can we use particular soc name to define the compatible string?

Just realized that it's actually not representing any hardware block.
If that's the case, I feel we should try to get it away from device
tree.

Regards,
Shawn

> Also,
> the .data initialization seems not needed.
> 
> > +	{ /* sentinel */ }
> > +};
Shawn Guo - June 21, 2012, 6:12 a.m.
On Thu, Jun 14, 2012 at 03:43:25PM +0200, Sascha Hauer wrote:
...
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <linux/fb.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <mach/hardware.h>

This looks suspicious.

> +#include <mach/imxfb.h>

We should probably copy those needed macros into the file to save this
<mach> inclusion?

> +#include <generated/mach-types.h>
> +#include <drm/drm_gem_cma_helper.h>
> +
> +#include "imx-drm.h"
> +
> +#define LCDC_SSA		0x00
> +#define LCDC_SIZE		0x04
> +#define LCDC_VPW		0x08
> +#define LCDC_CPOS		0x0C
> +#define LCDC_LCWHB		0x10
> +#define LCDC_LCHCC		0x14
> +#define LCDC_PCR		0x18
> +#define LCDC_HCR		0x1C
> +#define LCDC_VCR		0x20
> +#define LCDC_POS		0x24
> +#define LCDC_LSCR1		0x28
> +#define LCDC_PWMR		0x2C
> +#define LCDC_DMACR		0x30
> +#define LCDC_RMCR		0x34
> +#define LCDC_LCDICR		0x38
> +#define LCDC_LIER		0x3c
> +#define LCDC_LISR		0x40
> +
> +#define SIZE_XMAX(x)		((((x) >> 4) & 0x3f) << 20)
> +
> +#define YMAX_MASK		(cpu_is_mx1() ? 0x1ff : 0x3ff)

Ah, here it needs <mach/hardware.h>.  We may not want to use
cpu_is_mx1() any more.

> +#define SIZE_YMAX(y)		((y) & YMAX_MASK)
> +
> +#define VPW_VPW(x)		((x) & 0x3ff)
> +
> +#define HCR_H_WIDTH(x)		(((x) & 0x3f) << 26)
> +#define HCR_H_WAIT_1(x)		(((x) & 0xff) << 8)
> +#define HCR_H_WAIT_2(x)		((x) & 0xff)
> +
> +#define VCR_V_WIDTH(x)		(((x) & 0x3f) << 26)
> +#define VCR_V_WAIT_1(x)		(((x) & 0xff) << 8)
> +#define VCR_V_WAIT_2(x)		((x) & 0xff)
> +
> +#define RMCR_LCDC_EN_MX1	(1 << 1)
> +
> +#define RMCR_SELF_REF		(1 << 0)
> +
> +#define LIER_EOF		(1 << 1)
> +
> +struct imx_crtc {
> +	struct drm_crtc		base;
> +	struct imx_drm_crtc	*imx_drm_crtc;
> +	int			di_no;
> +	int			enabled;
> +	void __iomem		*regs;
> +	u32			pwmr;
> +	u32			lscr1;
> +	u32			dmacr;
> +	u32			pcr;
> +	struct clk		*clk;
> +	struct device		*dev;
> +	int			vblank_enable;
> +
> +	struct drm_pending_vblank_event *page_flip_event;
> +	struct drm_framebuffer	*newfb;
> +};
> +
> +#define to_imx_crtc(x) container_of(x, struct imx_crtc, base)
> +
> +static void imx_crtc_load_lut(struct drm_crtc *crtc)
> +{
> +}
> +
> +#define PCR_BPIX_8		(3 << 25)
> +#define PCR_BPIX_12		(4 << 25)
> +#define PCR_BPIX_16		(5 << 25)
> +#define PCR_BPIX_18		(6 << 25)
> +#define PCR_END_SEL		(1 << 18)
> +#define PCR_END_BYTE_SWAP	(1 << 17)
> +
> +static const char *fourcc_to_str(u32 fourcc)
> +{
> +	static char buf[5];
> +
> +	*(u32 *)buf = fourcc;
> +	buf[4] = 0;
> +
> +	return buf;
> +}
> +
> +static int imx_drm_crtc_set(struct drm_crtc *crtc,
> +		struct drm_display_mode *mode)
> +{
> +	struct imx_crtc *imx_crtc = to_imx_crtc(crtc);
> +	struct drm_framebuffer *fb = crtc->fb;
> +	int lower_margin = mode->vsync_start - mode->vdisplay;
> +	int upper_margin = mode->vtotal - mode->vsync_end;
> +	int vsync_len = mode->vsync_end - mode->vsync_start;
> +	int hsync_len = mode->hsync_end - mode->hsync_start;
> +	int right_margin = mode->hsync_start - mode->hdisplay;
> +	int left_margin = mode->htotal - mode->hsync_end;
> +	unsigned long lcd_clk;
> +	u32 pcr;
> +
> +	lcd_clk = clk_get_rate(imx_crtc->clk) / 1000;
> +
> +	if (!mode->clock)
> +		return -EINVAL;
> +
> +	pcr = DIV_ROUND_CLOSEST(lcd_clk, mode->clock);
> +	if (--pcr > 0x3f)
> +		pcr = 0x3f;
> +
> +	switch (fb->pixel_format) {
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ARGB8888:
> +		pcr |= PCR_BPIX_18;
> +		pcr |= PCR_END_SEL | PCR_END_BYTE_SWAP;
> +		break;
> +	case DRM_FORMAT_RGB565:
> +		if (cpu_is_mx1())

Ditto

> +			pcr |= PCR_BPIX_12;
> +		else
> +			pcr |= PCR_BPIX_16;
> +		break;
> +	case DRM_FORMAT_RGB332:
> +		pcr |= PCR_BPIX_8;
> +		break;
> +	default:
> +		dev_err(imx_crtc->dev, "unsupported pixel format %s\n",
> +				fourcc_to_str(fb->pixel_format));
> +		return -EINVAL;
> +	}
> +
> +	/* add sync polarities */
> +	pcr |= imx_crtc->pcr & ~(0x3f | (7 << 25));
> +
> +	dev_dbg(imx_crtc->dev,
> +			"xres=%d hsync_len=%d left_margin=%d right_margin=%d\n",
> +			mode->hdisplay, hsync_len,
> +			left_margin, right_margin);
> +	dev_dbg(imx_crtc->dev,
> +			"yres=%d vsync_len=%d upper_margin=%d lower_margin=%d\n",
> +			mode->vdisplay, vsync_len,
> +			upper_margin, lower_margin);
> +
> +	writel(VPW_VPW(mode->hdisplay * fb->bits_per_pixel / 8 / 4),
> +		imx_crtc->regs + LCDC_VPW);
> +
> +	writel(HCR_H_WIDTH(hsync_len - 1) |
> +		HCR_H_WAIT_1(right_margin - 1) |
> +		HCR_H_WAIT_2(left_margin - 3),
> +		imx_crtc->regs + LCDC_HCR);
> +
> +	writel(VCR_V_WIDTH(vsync_len) |
> +		VCR_V_WAIT_1(lower_margin) |
> +		VCR_V_WAIT_2(upper_margin),
> +		imx_crtc->regs + LCDC_VCR);
> +
> +	writel(SIZE_XMAX(mode->hdisplay) | SIZE_YMAX(mode->vdisplay),
> +			imx_crtc->regs + LCDC_SIZE);
> +
> +	writel(pcr, imx_crtc->regs + LCDC_PCR);
> +	writel(imx_crtc->pwmr, imx_crtc->regs + LCDC_PWMR);
> +	writel(imx_crtc->lscr1, imx_crtc->regs + LCDC_LSCR1);
> +	/* reset default */
> +	writel(0x00040060, imx_crtc->regs + LCDC_DMACR);
> +
> +	return 0;
> +}

...

> +static int __devinit imx_crtc_probe(struct platform_device *pdev)
> +{
> +	struct imx_crtc *imx_crtc;
> +	struct resource *res;
> +	int ret, irq;
> +	u32 pcr_value = 0xf00080c0;
> +	u32 lscr1_value = 0x00120300;
> +	u32 pwmr_value = 0x00a903ff;
> +
> +	imx_crtc = devm_kzalloc(&pdev->dev, sizeof(*imx_crtc), GFP_KERNEL);
> +	if (!imx_crtc)
> +		return -ENOMEM;
> +
> +	imx_crtc->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	res = devm_request_mem_region(&pdev->dev, res->start,
> +			resource_size(res), DRIVER_NAME);
> +	if (!res)
> +		return -EBUSY;
> +
> +	imx_crtc->regs = devm_ioremap(&pdev->dev, res->start,
> +			resource_size(res));
> +	if (!imx_crtc->regs) {
> +		dev_err(&pdev->dev, "Cannot map frame buffer registers\n");
> +		return -EBUSY;
> +	}

devm_request_and_ioremap() can help a little bit.

> +
> +	irq = platform_get_irq(pdev, 0);
> +	ret = devm_request_irq(&pdev->dev, irq, imx_irq_handler, 0, "imx_drm",
> +			imx_crtc);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "irq request failed with %d\n", ret);
> +		return ret;
> +	}
> +
> +	imx_crtc->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(imx_crtc->clk)) {
> +		ret = PTR_ERR(imx_crtc->clk);
> +		dev_err(&pdev->dev, "unable to get clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	clk_prepare_enable(imx_crtc->clk);
> +	imx_crtc->enabled = 1;
> +
> +	platform_set_drvdata(pdev, imx_crtc);
> +
> +	imx_crtc->pcr = pcr_value & PDATA_PCR;
> +
> +	if (imx_crtc->pcr != pcr_value)
> +		dev_err(&pdev->dev, "invalid bits set in pcr: 0x%08x\n",
> +				pcr_value & ~PDATA_PCR);
> +
> +	imx_crtc->lscr1 = lscr1_value;
> +	imx_crtc->pwmr = pwmr_value;
> +
> +	ret = imx_drm_add_crtc(&imx_crtc->base,
> +			&imx_crtc->imx_drm_crtc,
> +			&imx_imx_drm_helper, THIS_MODULE,
> +			pdev->dev.of_node, 0);
> +	if (ret)
> +		goto err_init;
> +
> +	dev_info(&pdev->dev, "probed\n");
> +
> +	return 0;
> +
> +err_init:
> +	clk_disable_unprepare(imx_crtc->clk);
> +	clk_put(imx_crtc->clk);
> +
> +	return ret;
> +}
> +
> +static int __devexit imx_crtc_remove(struct platform_device *pdev)
> +{
> +	struct imx_crtc *imx_crtc = platform_get_drvdata(pdev);
> +
> +	imx_drm_remove_crtc(imx_crtc->imx_drm_crtc);
> +
> +	writel(0, imx_crtc->regs + LCDC_LIER);
> +
> +	clk_disable_unprepare(imx_crtc->clk);
> +	clk_put(imx_crtc->clk);
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id imx_lcdc_dt_ids[] = {
> +	{ .compatible = "fsl,imx1-lcdc", .data = NULL, },
> +	{ .compatible = "fsl,imx21-lcdc", .data = NULL, },

So .data will be used to kill those cpu_is_xxx uses.

> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver imx_crtc_driver = {
> +	.remove		= __devexit_p(imx_crtc_remove),
> +	.probe		= imx_crtc_probe,
> +	.driver		= {
> +		.name	= DRIVER_NAME,
> +		.owner	= THIS_MODULE,
> +		.of_match_table = imx_lcdc_dt_ids,
> +	},
> +};
> +
> +static int __init imx_lcdc_init(void)
> +{
> +	return platform_driver_register(&imx_crtc_driver);
> +}
> +
> +static void __exit imx_lcdc_exit(void)
> +{
> +	platform_driver_unregister(&imx_crtc_driver);
> +}
> +
> +module_init(imx_lcdc_init);
> +module_exit(imx_lcdc_exit)

Can these simply be module_platform_driver(imx_crtc_driver)?

> +
> +MODULE_DESCRIPTION("Freescale i.MX framebuffer driver");
> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.10
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Sascha Hauer - June 21, 2012, 6:52 a.m.
On Thu, Jun 21, 2012 at 01:30:16PM +0800, Shawn Guo wrote:
> On Thu, Jun 14, 2012 at 03:43:23PM +0200, Sascha Hauer wrote:
> ...
> > +struct drm_device *imx_drm_device_get(void)
> > +{
> > +	struct imx_drm_device *imxdrm = __imx_drm_device();
> > +	struct imx_drm_encoder *enc;
> > +	struct imx_drm_connector *con;
> > +	struct imx_drm_crtc *crtc;
> > +
> > +	mutex_lock(&imxdrm->mutex);
> > +
> > +	list_for_each_entry(enc, &imxdrm->encoder_list, list) {
> > +		if (!try_module_get(enc->owner)) {
> > +			dev_err(imxdrm->dev, "could not get module %s\n",
> > +					module_name(enc->owner));
> > +			goto unwind_enc;
> > +		}
> > +	}
> > +
> > +	list_for_each_entry(con, &imxdrm->connector_list, list) {
> > +		if (!try_module_get(con->owner)) {
> > +			dev_err(imxdrm->dev, "could not get module %s\n",
> > +					module_name(con->owner));
> > +			goto unwind_con;
> > +		}
> > +	}
> > +
> > +	list_for_each_entry(crtc, &imxdrm->crtc_list, list) {
> > +		if (!try_module_get(crtc->owner)) {
> > +			dev_err(imxdrm->dev, "could not get module %s\n",
> > +					module_name(crtc->owner));
> > +			goto unwind_crtc;
> > +		}
> > +	}
> > +
> > +	imxdrm->references++;
> > +
> > +	mutex_unlock(&imxdrm->mutex);
> > +
> > +	return imx_drm_device->drm;
> 
> Though I'm not quite sure about the point of retrieving the static
> variable imx_drm_device from calling __imx_drm_device(), shouldn't
> imxdrm be used here since you already retrieve it?

The point of calling __imx_drm_device() instead of using imx_drm_device
directly is that I thought it might be easier to convert to a non global
variable later should we have (or want) to. I'm also unsure about the
value of having such a function, but you are right, it should be used
consistently if we have it.

I'll fix the remaining things you mentioned in the next round. Thanks
for looking at it.

Sascha
Sascha Hauer - June 21, 2012, 7:06 a.m.
On Thu, Jun 21, 2012 at 02:07:32PM +0800, Shawn Guo wrote:
> On Thu, Jun 21, 2012 at 01:35:56PM +0800, Shawn Guo wrote:
> > On Thu, Jun 14, 2012 at 03:43:24PM +0200, Sascha Hauer wrote:
> > > +static const struct of_device_id imx_pd_dt_ids[] = {
> > > +	{ .compatible = "fsl,imx-parallel-display", .data = NULL, },
> > 
> > Can we use particular soc name to define the compatible string?
> 
> Just realized that it's actually not representing any hardware block.
> If that's the case, I feel we should try to get it away from device
> tree.

It actually represents a hardware block, or what else should a display
be? You are right in the way that it does not correspond to some
register block, but still it's hardware.
The information about the connected display must definitely live in the
devicetree, I can't think off any other place.

(We could argue that the displays should be subnodes of the IPU. I have
chosen not to do so because other display connections such as the LVDS
unit are outside of the IPU, but still need a display description.)

Sascha
Sascha Hauer - June 21, 2012, 7:10 a.m.
On Thu, Jun 21, 2012 at 02:12:48PM +0800, Shawn Guo wrote:
> On Thu, Jun 14, 2012 at 03:43:25PM +0200, Sascha Hauer wrote:
> ...
> > +#include <linux/device.h>
> > +#include <linux/platform_device.h>
> > +#include <drm/drmP.h>
> > +#include <drm/drm_fb_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +#include <drm/drm_fb_cma_helper.h>
> > +#include <linux/fb.h>
> > +#include <linux/clk.h>
> > +#include <linux/module.h>
> > +#include <mach/hardware.h>
> 
> This looks suspicious.

Indeed ;)

> 
> > +#include <mach/imxfb.h>
> 
> We should probably copy those needed macros into the file to save this
> <mach> inclusion?

I don't think they are actually needed, I will have a look at it.

> 
> > +#include <generated/mach-types.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +
> > +#include "imx-drm.h"
> > +
> > +#define LCDC_SSA		0x00
> > +#define LCDC_SIZE		0x04
> > +#define LCDC_VPW		0x08
> > +#define LCDC_CPOS		0x0C
> > +#define LCDC_LCWHB		0x10
> > +#define LCDC_LCHCC		0x14
> > +#define LCDC_PCR		0x18
> > +#define LCDC_HCR		0x1C
> > +#define LCDC_VCR		0x20
> > +#define LCDC_POS		0x24
> > +#define LCDC_LSCR1		0x28
> > +#define LCDC_PWMR		0x2C
> > +#define LCDC_DMACR		0x30
> > +#define LCDC_RMCR		0x34
> > +#define LCDC_LCDICR		0x38
> > +#define LCDC_LIER		0x3c
> > +#define LCDC_LISR		0x40
> > +
> > +#define SIZE_XMAX(x)		((((x) >> 4) & 0x3f) << 20)
> > +
> > +#define YMAX_MASK		(cpu_is_mx1() ? 0x1ff : 0x3ff)
> 
> Ah, here it needs <mach/hardware.h>.  We may not want to use
> cpu_is_mx1() any more.

Will fix.

> > +
> > +static struct platform_driver imx_crtc_driver = {
> > +	.remove		= __devexit_p(imx_crtc_remove),
> > +	.probe		= imx_crtc_probe,
> > +	.driver		= {
> > +		.name	= DRIVER_NAME,
> > +		.owner	= THIS_MODULE,
> > +		.of_match_table = imx_lcdc_dt_ids,
> > +	},
> > +};
> > +
> > +static int __init imx_lcdc_init(void)
> > +{
> > +	return platform_driver_register(&imx_crtc_driver);
> > +}
> > +
> > +static void __exit imx_lcdc_exit(void)
> > +{
> > +	platform_driver_unregister(&imx_crtc_driver);
> > +}
> > +
> > +module_init(imx_lcdc_init);
> > +module_exit(imx_lcdc_exit)
> 
> Can these simply be module_platform_driver(imx_crtc_driver)?

Yes. I had to put it in an earlier initcall in an earlier version of
this driver, hence couldn't use module_platform_driver() back then. Will
fix.

Sascha
Shawn Guo - June 22, 2012, 5:29 a.m.
On Thu, Jun 14, 2012 at 03:43:24PM +0200, Sascha Hauer wrote:
> +static int __devinit imx_pd_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	const u8 *edidp;
> +	struct imx_parallel_display *imxpd;
> +	int ret;
> +	u32 crtcs[2];

It seems used nowhere.

> +	const char *fmt;
> +	struct device_node *crtc_node;

Ditto.

> +
> +	imxpd = devm_kzalloc(&pdev->dev, sizeof(*imxpd), GFP_KERNEL);
> +	if (!imxpd)
> +		return -ENOMEM;
> +
> +	edidp = of_get_property(np, "edid", &imxpd->edid_len);
> +	if (edidp)
> +		imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
> +
> +	ret = of_property_read_string(np, "interface_pix_fmt", &fmt);
> +	if (!ret) {
> +		if (!strcmp(fmt, "rgb24"))
> +			imxpd->interface_pix_fmt = V4L2_PIX_FMT_RGB24;
> +		else if (!strcmp(fmt, "rgb565"))
> +			imxpd->interface_pix_fmt = V4L2_PIX_FMT_RGB565;
> +	}
> +
> +	imxpd->dev = &pdev->dev;
> +
> +	ret = imx_pd_register(imxpd);
> +	if (ret)
> +		return ret;
> +
> +	ret = imx_drm_encoder_add_possible_crtcs(imxpd->imx_drm_encoder, np);
> +
> +	platform_set_drvdata(pdev, imxpd);
> +
> +	return 0;
> +}
Shawn Guo - June 22, 2012, 6:04 a.m.
On Thu, Jun 14, 2012 at 03:43:26PM +0200, Sascha Hauer wrote:
...
> +#include <linux/module.h>
> +#include <linux/export.h>
> +#include <linux/types.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +#include <linux/spinlock.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/list.h>
> +#include <linux/irq.h>
> +#include <mach/common.h>

This seems not needed at all.

> +#include <drm/imx-ipu-v3.h>
> +#include <linux/of_device.h>
> +#include <asm/mach/irq.h>

...

> +void ipu_ch_param_set_field(struct ipu_ch_param __iomem *base, u32 wbs, u32 v)

Rename it to ipu_ch_param_write_field ...

> +{
> +	u32 bit = (wbs >> 8) % 160;
> +	u32 size = wbs & 0xff;
> +	u32 word = (wbs >> 8) / 160;
> +	u32 i = bit / 32;
> +	u32 ofs = bit % 32;
> +	u32 mask = (1 << size) - 1;
> +	u32 val;
> +
> +	pr_debug("%s %d %d %d\n", __func__, word, bit , size);
> +
> +	val = readl(&base->word[word].data[i]);
> +	val &= ~(mask << ofs);
> +	val |= v << ofs;
> +	writel(val, &base->word[word].data[i]);
> +
> +	if ((bit + size - 1) / 32 > i) {
> +		val = readl(&base->word[word].data[i + 1]);
> +		val &= ~(mask >> (mask ? (32 - ofs) : 0));
> +		val |= v >> (ofs ? (32 - ofs) : 0);
> +		writel(val, &base->word[word].data[i + 1]);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(ipu_ch_param_set_field);
> +
> +u32 ipu_ch_param_read_field(struct ipu_ch_param __iomem *base, u32 wbs)

... or rename this to ipu_ch_param_get_field for a better couple?

> +{
> +	u32 bit = (wbs >> 8) % 160;
> +	u32 size = wbs & 0xff;
> +	u32 word = (wbs >> 8) / 160;
> +	u32 i = bit / 32;
> +	u32 ofs = bit % 32;
> +	u32 mask = (1 << size) - 1;
> +	u32 val = 0;
> +
> +	pr_debug("%s %d %d %d\n", __func__, word, bit , size);
> +
> +	val = (readl(&base->word[word].data[i]) >> ofs) & mask;
> +
> +	if ((bit + size - 1) / 32 > i) {
> +		u32 tmp;
> +		tmp = readl(&base->word[word].data[i + 1]);
> +		tmp &= mask >> (ofs ? (32 - ofs) : 0);
> +		val |= tmp << (ofs ? (32 - ofs) : 0);
> +	}
> +
> +	return val;
> +}
> +EXPORT_SYMBOL_GPL(ipu_ch_param_read_field);

...

> +static int ipu_reset(struct ipu_soc *ipu)
> +{
> +	int timeout = 10000;

We may want to use a better timeout mechanism.

> +
> +	ipu_cm_write(ipu, 0x807FFFFF, IPU_MEM_RST);
> +
> +	while (ipu_cm_read(ipu, IPU_MEM_RST) & 0x80000000) {
> +		if (!timeout--)
> +			return -ETIME;
> +		udelay(100);
> +	}
> +
> +	mdelay(300);
> +
> +	return 0;
> +}

...

> +static int ipu_irq_init(struct ipu_soc *ipu)
> +{
> +	int i;
> +
> +	ipu->irq_start = irq_alloc_descs(-1, 0, IPU_NUM_IRQS, 0);

We may want to give a try on linear irqdomain, so that irqdesc will
only be allocated for those in-use irqs, and we do not have to maintain
irq_base.

> +	if (ipu->irq_start < 0)
> +		return ipu->irq_start;
> +
> +	for (i = ipu->irq_start; i < ipu->irq_start + IPU_NUM_IRQS; i++) {
> +		irq_set_chip_and_handler(i, &ipu_irq_chip, handle_level_irq);
> +		set_irq_flags(i, IRQF_VALID);
> +		irq_set_chip_data(i, ipu);
> +	}
> +
> +	irq_set_chained_handler(ipu->irq_sync, ipu_irq_handler);
> +	irq_set_handler_data(ipu->irq_sync, ipu);
> +	irq_set_chained_handler(ipu->irq_err, ipu_err_irq_handler);
> +	irq_set_handler_data(ipu->irq_err, ipu);
> +
> +	return 0;
> +}

...

> +static int __devinit ipu_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *of_id =
> +			of_match_device(imx_ipu_dt_ids, &pdev->dev);
> +	struct ipu_soc *ipu;
> +	struct resource *res;
> +	unsigned long ipu_base;
> +	int i, ret, irq_sync, irq_err;
> +	struct ipu_devtype *devtype;
> +
> +	devtype = of_id->data;
> +
> +	dev_info(&pdev->dev, "Initializing %s\n", devtype->name);
> +
> +	irq_sync = platform_get_irq(pdev, 0);
> +	irq_err = platform_get_irq(pdev, 1);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	dev_info(&pdev->dev, "irq_sync: %d irq_err: %d\n",
> +			irq_sync, irq_err);
> +
> +	if (!res || irq_sync < 0 || irq_err < 0)
> +		return -ENODEV;
> +
> +	ipu_base = res->start;
> +
> +	ipu = devm_kzalloc(&pdev->dev, sizeof(*ipu), GFP_KERNEL);
> +	if (!ipu)
> +		return -ENODEV;
> +
> +	for (i = 0; i < 64; i++)
> +		ipu->channel[i].ipu = ipu;
> +	ipu->devtype = devtype;
> +	ipu->ipu_type = devtype->type;
> +
> +	spin_lock_init(&ipu->lock);
> +	mutex_init(&ipu->channel_lock);
> +
> +	dev_info(&pdev->dev, "cm_reg:   0x%08lx\n",
> +			ipu_base + devtype->cm_ofs);
> +	dev_info(&pdev->dev, "idmac:    0x%08lx\n",
> +			ipu_base + devtype->cm_ofs + IPU_CM_IDMAC_REG_OFS);
> +	dev_info(&pdev->dev, "cpmem:    0x%08lx\n",
> +			ipu_base + devtype->cpmem_ofs);
> +	dev_info(&pdev->dev, "disp0:    0x%08lx\n",
> +			ipu_base + devtype->disp0_ofs);
> +	dev_info(&pdev->dev, "disp1:    0x%08lx\n",
> +			ipu_base + devtype->disp1_ofs);
> +	dev_info(&pdev->dev, "srm:      0x%08lx\n",
> +			ipu_base + devtype->srm_ofs);
> +	dev_info(&pdev->dev, "tpm:      0x%08lx\n",
> +			ipu_base + devtype->tpm_ofs);
> +	dev_info(&pdev->dev, "dc:       0x%08lx\n",
> +			ipu_base + devtype->cm_ofs + IPU_CM_DC_REG_OFS);
> +	dev_info(&pdev->dev, "ic:       0x%08lx\n",
> +			ipu_base + devtype->cm_ofs + IPU_CM_IC_REG_OFS);
> +	dev_info(&pdev->dev, "dmfc:     0x%08lx\n",
> +			ipu_base + devtype->cm_ofs + IPU_CM_DMFC_REG_OFS);
> +
> +	ipu->cm_reg = devm_ioremap(&pdev->dev,
> +			ipu_base + devtype->cm_ofs, PAGE_SIZE);
> +	ipu->idmac_reg = devm_ioremap(&pdev->dev,
> +			ipu_base + devtype->cm_ofs + IPU_CM_IDMAC_REG_OFS,
> +			PAGE_SIZE);
> +	ipu->cpmem_base = devm_ioremap(&pdev->dev,
> +			ipu_base + devtype->cpmem_ofs, PAGE_SIZE);
> +
> +	if (!ipu->cm_reg || !ipu->idmac_reg || !ipu->cpmem_base) {
> +		ret = -ENOMEM;
> +		goto failed_ioremap;
> +	}
> +
> +	ipu->clk = devm_clk_get(&pdev->dev, "bus");
> +	if (IS_ERR(ipu->clk)) {
> +		ret = PTR_ERR(ipu->clk);
> +		dev_err(&pdev->dev, "clk_get failed with %d", ret);
> +		goto failed_clk_get;
> +	}
> +
> +	platform_set_drvdata(pdev, ipu);
> +
> +	clk_prepare_enable(ipu->clk);
> +
> +	ipu->dev = &pdev->dev;
> +	ipu->irq_sync = irq_sync;
> +	ipu->irq_err = irq_err;
> +
> +	ret = ipu_irq_init(ipu);
> +	if (ret)
> +		goto out_failed_irq;
> +
> +	ipu_reset(ipu);
> +
> +	/* Set MCU_T to divide MCU access window into 2 */
> +	ipu_cm_write(ipu, 0x00400000L | (IPU_MCU_T_DEFAULT << 18),
> +			IPU_DISP_GEN);
> +
> +	ret = ipu_submodules_init(ipu, pdev, ipu_base, ipu->clk);
> +	if (ret)
> +		goto failed_submodules_init;
> +
> +	ret = ipu_add_client_devices(ipu);
> +	if (ret) {
> +		dev_err(&pdev->dev, "adding client devices failed with %d\n",
> +				ret);
> +		goto failed_add_clients;
> +	}
> +
> +	return 0;
> +
> +failed_add_clients:
> +	ipu_submodules_exit(ipu);
> +failed_submodules_init:
> +	ipu_irq_exit(ipu);
> +out_failed_irq:

clk_disable_unprepare(ipu->clk);

> +failed_clk_get:
> +failed_ioremap:
> +	return ret;
> +}
Sascha Hauer - July 2, 2012, 10:05 a.m.
On Thu, Jun 14, 2012 at 03:43:22PM +0200, Sascha Hauer wrote:
> Hi All,
> 
> The following is the state-of-the-art i.MX IPU (Image Processing Unit)
> DRM support.
> 
> This code is around for quite some time now and has been posted several
> times with different APIs, first with plain old framebuffer support, now
> DRM, first platform device binding, now devicetree. Unfortunately there's
> quite much code needed to get something useful out of the IPU, so these
> patches haven't received a lot of attention from people not involved in
> i.MX. I think we have now come to a point where this code needs more public
> exposure and where it's easier to talk in incremental changes instead of
> blobs. Therefore I request this to go to staging for some cycles.

Dave, Greg,

Comments to this one? I addressed the comments I received so far and am
about to respin this series. Is it ok to put this to staging? If yes,
should I move the whole stuff into drivers/staging/ or should it stay
in drivers/gpu/drm with just a Kconfig dependency on STAGING?

Thanks
 Sascha
Greg KH - July 6, 2012, 10:58 p.m.
On Mon, Jul 02, 2012 at 12:05:06PM +0200, Sascha Hauer wrote:
> On Thu, Jun 14, 2012 at 03:43:22PM +0200, Sascha Hauer wrote:
> > Hi All,
> > 
> > The following is the state-of-the-art i.MX IPU (Image Processing Unit)
> > DRM support.
> > 
> > This code is around for quite some time now and has been posted several
> > times with different APIs, first with plain old framebuffer support, now
> > DRM, first platform device binding, now devicetree. Unfortunately there's
> > quite much code needed to get something useful out of the IPU, so these
> > patches haven't received a lot of attention from people not involved in
> > i.MX. I think we have now come to a point where this code needs more public
> > exposure and where it's easier to talk in incremental changes instead of
> > blobs. Therefore I request this to go to staging for some cycles.
> 
> Dave, Greg,
> 
> Comments to this one? I addressed the comments I received so far and am
> about to respin this series. Is it ok to put this to staging? If yes,
> should I move the whole stuff into drivers/staging/ or should it stay
> in drivers/gpu/drm with just a Kconfig dependency on STAGING?

That's up to the DRM subsystem maintainer to choose.

greg k-h
Dave Airlie - July 8, 2012, 7:35 a.m.
On Fri, Jul 6, 2012 at 11:58 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Jul 02, 2012 at 12:05:06PM +0200, Sascha Hauer wrote:
>> On Thu, Jun 14, 2012 at 03:43:22PM +0200, Sascha Hauer wrote:
>> > Hi All,
>> >
>> > The following is the state-of-the-art i.MX IPU (Image Processing Unit)
>> > DRM support.
>> >
>> > This code is around for quite some time now and has been posted several
>> > times with different APIs, first with plain old framebuffer support, now
>> > DRM, first platform device binding, now devicetree. Unfortunately there's
>> > quite much code needed to get something useful out of the IPU, so these
>> > patches haven't received a lot of attention from people not involved in
>> > i.MX. I think we have now come to a point where this code needs more public
>> > exposure and where it's easier to talk in incremental changes instead of
>> > blobs. Therefore I request this to go to staging for some cycles.
>>
>> Dave, Greg,
>>
>> Comments to this one? I addressed the comments I received so far and am
>> about to respin this series. Is it ok to put this to staging? If yes,
>> should I move the whole stuff into drivers/staging/ or should it stay
>> in drivers/gpu/drm with just a Kconfig dependency on STAGING?
>
> That's up to the DRM subsystem maintainer to choose.

Sorry guys been out of the loop on arm drivers due to other things,
but probably should go in staging proper for now, until I can at least
spend time reviewing it.

Dave.
Sascha Hauer - July 9, 2012, 8:42 a.m.
On Sun, Jul 08, 2012 at 08:35:55AM +0100, Dave Airlie wrote:
> On Fri, Jul 6, 2012 at 11:58 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Jul 02, 2012 at 12:05:06PM +0200, Sascha Hauer wrote:
> >> On Thu, Jun 14, 2012 at 03:43:22PM +0200, Sascha Hauer wrote:
> >> > Hi All,
> >> >
> >> > The following is the state-of-the-art i.MX IPU (Image Processing Unit)
> >> > DRM support.
> >> >
> >> > This code is around for quite some time now and has been posted several
> >> > times with different APIs, first with plain old framebuffer support, now
> >> > DRM, first platform device binding, now devicetree. Unfortunately there's
> >> > quite much code needed to get something useful out of the IPU, so these
> >> > patches haven't received a lot of attention from people not involved in
> >> > i.MX. I think we have now come to a point where this code needs more public
> >> > exposure and where it's easier to talk in incremental changes instead of
> >> > blobs. Therefore I request this to go to staging for some cycles.
> >>
> >> Dave, Greg,
> >>
> >> Comments to this one? I addressed the comments I received so far and am
> >> about to respin this series. Is it ok to put this to staging? If yes,
> >> should I move the whole stuff into drivers/staging/ or should it stay
> >> in drivers/gpu/drm with just a Kconfig dependency on STAGING?
> >
> > That's up to the DRM subsystem maintainer to choose.
> 
> Sorry guys been out of the loop on arm drivers due to other things,
> but probably should go in staging proper for now, until I can at least
> spend time reviewing it.

Right now we have dependencies on:

http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg24409.html
http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg24224.html

We could move these two helpers into staging, but they are needed by
other drivers from Laurent Pinchart and Lars-Peter Clausen aswell. So
it would be good to have them available for them aswell. Could you have
a look on these and consider applying?

Thanks,
  Sascha