Patchwork Add i.MX IPUv3 base/KMS driver to the staging tree

login
register
mail settings
Submitter Sascha Hauer
Date Sept. 12, 2012, 10:31 a.m.
Message ID <1347445874-10779-1-git-send-email-s.hauer@pengutronix.de>
Download mbox
Permalink /patch/183316/
State New
Headers show

Pull-request

git://git.pengutronix.de/git/imx/linux-2.6.git tags/staging-drm-imx

Comments

Sascha Hauer - Sept. 12, 2012, 10:31 a.m.
Hi Greg et all,

The following adds the i.MX IPUv3 base and KMS driver to the staging
tree. It currently depends on two patches adding helper functions to
the DRM core. Dave Airlie already has signalled he is ok with these
helper functions. Apart from that, please let me know if there's anything
else blocking the IPU patches from going to staging. If not, I would
ping again once the helper patches show up in linux-next.

Thanks
 Sascha


The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:

  Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)

are available in the git repository at:

  git://git.pengutronix.de/git/imx/linux-2.6.git tags/staging-drm-imx

for you to fetch changes up to d344cf816ce270a74d5210d771646ad04c89a7cc:

  staging: drm/imx: Add TODO (2012-09-12 12:01:13 +0200)

----------------------------------------------------------------
This adds an ARM i.MX IPUv3 base and DRM driver to the staging
tree.

----------------------------------------------------------------
Philipp Zabel (1):
      staging: drm/imx: Add devicetree binding documentation

Sascha Hauer (5):
      staging: drm/imx: Add i.MX drm core support
      staging: drm/imx: Add parallel display support
      staging: drm/imx: add i.MX IPUv3 base driver
      staging: drm/imx: Add i.MX IPUv3 crtc support
      staging: drm/imx: Add TODO

 .../bindings/staging/imx-drm/fsl-imx-drm.txt       |   41 +
 drivers/staging/Kconfig                            |    2 +
 drivers/staging/Makefile                           |    1 +
 drivers/staging/imx-drm/Kconfig                    |   35 +
 drivers/staging/imx-drm/Makefile                   |    9 +
 drivers/staging/imx-drm/TODO                       |   22 +
 drivers/staging/imx-drm/imx-drm-core.c             |  882 +++++++++++++++
 drivers/staging/imx-drm/imx-drm.h                  |   58 +
 drivers/staging/imx-drm/imx-fb.c                   |   47 +
 drivers/staging/imx-drm/imx-fbdev.c                |   74 ++
 drivers/staging/imx-drm/ipu-v3/Makefile            |    3 +
 drivers/staging/imx-drm/ipu-v3/imx-ipu-v3.h        |  318 ++++++
 drivers/staging/imx-drm/ipu-v3/ipu-common.c        | 1143 ++++++++++++++++++++
 drivers/staging/imx-drm/ipu-v3/ipu-dc.c            |  379 +++++++
 drivers/staging/imx-drm/ipu-v3/ipu-di.c            |  700 ++++++++++++
 drivers/staging/imx-drm/ipu-v3/ipu-dmfc.c          |  408 +++++++
 drivers/staging/imx-drm/ipu-v3/ipu-dp.c            |  336 ++++++
 drivers/staging/imx-drm/ipu-v3/ipu-prv.h           |  206 ++++
 drivers/staging/imx-drm/ipuv3-crtc.c               |  579 ++++++++++
 drivers/staging/imx-drm/parallel-display.c         |  262 +++++
 20 files changed, 5505 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
 create mode 100644 drivers/staging/imx-drm/Kconfig
 create mode 100644 drivers/staging/imx-drm/Makefile
 create mode 100644 drivers/staging/imx-drm/TODO
 create mode 100644 drivers/staging/imx-drm/imx-drm-core.c
 create mode 100644 drivers/staging/imx-drm/imx-drm.h
 create mode 100644 drivers/staging/imx-drm/imx-fb.c
 create mode 100644 drivers/staging/imx-drm/imx-fbdev.c
 create mode 100644 drivers/staging/imx-drm/ipu-v3/Makefile
 create mode 100644 drivers/staging/imx-drm/ipu-v3/imx-ipu-v3.h
 create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-common.c
 create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-dc.c
 create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-di.c
 create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-dmfc.c
 create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-dp.c
 create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-prv.h
 create mode 100644 drivers/staging/imx-drm/ipuv3-crtc.c
 create mode 100644 drivers/staging/imx-drm/parallel-display.c
Greg KH - Sept. 12, 2012, 4:49 p.m.
On Wed, Sep 12, 2012 at 12:31:09PM +0200, Sascha Hauer wrote:
> This patch adds the i.MX glue stuff between i.MX and drm.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/staging/Kconfig                |    2 +
>  drivers/staging/Makefile               |    1 +

The patch fails at this point when applying it.

What tree did you make it against?  Please do so against linux-next or
hopefully, the staging-next branch, and resubmit these.

Also, will this cause build errors if I don't have the needed patchs
that are in the DRM tree in my tree?

thanks,

greg k-h
Sascha Hauer - Sept. 13, 2012, 6:56 a.m.
On Wed, Sep 12, 2012 at 09:49:29AM -0700, Greg Kroah-Hartman wrote:
> On Wed, Sep 12, 2012 at 12:31:09PM +0200, Sascha Hauer wrote:
> > This patch adds the i.MX glue stuff between i.MX and drm.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/staging/Kconfig                |    2 +
> >  drivers/staging/Makefile               |    1 +
> 
> The patch fails at this point when applying it.
> 
> What tree did you make it against?  Please do so against linux-next or
> hopefully, the staging-next branch, and resubmit these.
> 
> Also, will this cause build errors if I don't have the needed patchs
> that are in the DRM tree in my tree?

Yes, the driver won't build without the patches. I'll rebase the IPU
support on linux-next once the dependency patches are in place.

Thanks
  Sascha
Dirk Behme - Sept. 14, 2012, 9:29 a.m.
On 12.09.2012 12:31, Sascha Hauer wrote:
> The IPU is the Image Processing Unit found on i.MX51/53/6 SoCs. It
> features several units for image processing, this patch adds support
> for the units needed for Framebuffer support, namely:
> 
> - Display Controller (dc)
> - Display Interface (di)
> - Display Multi Fifo Controller (dmfc)
> - Display Processor (dp)
> - Image DMA Controller (idmac)
> 
> This patch is based on the Freescale driver, but follows a different
> approach. The Freescale code implements logical idmac channels and
> the handling of the subunits is hidden in common idmac code pathes
> in big switch/case statements. This patch instead just provides code
> and resource management for the different subunits. The user, in this
> case the framebuffer driver, decides how the different units play
> together.
> 
> The IPU has other units missing in this patch:
> 
> - CMOS Sensor Interface (csi)
> - Video Deinterlacer (vdi)
> - Sensor Multi FIFO Controler (smfc)
> - Image Converter (ic)
> - Image Rotator (irt)
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
....
> +static int ipu_reset(struct ipu_soc *ipu)
> +{
> +       unsigned long timeout;
> +
> +       ipu_cm_write(ipu, 0x807FFFFF, IPU_MEM_RST);
> +
> +       timeout = jiffies + msecs_to_jiffies(1000);
> +       while (ipu_cm_read(ipu, IPU_MEM_RST) & 0x80000000) {
> +               if (time_after(jiffies, timeout))
> +                       return -ETIME;
> +               cpu_relax();
> +       }
> +
> +       mdelay(300);
           ^^^^^^^^^^^^

> +       return 0;
> +}

While doing some boot time measurement with i.MX6, we found that the 
above mdelay(300) is hurting regarding boot time. On i.MX6 you have two 
IPU instances, so in the end you get 600ms additional delay.

Looking at the Freescale code, this function looks like

static int ipu_reset(struct ipu_soc *ipu)
{
int timeout = 1000;

ipu_cm_write(ipu, 0x807FFFFF, IPU_MEM_RST);

while (ipu_cm_read(ipu, IPU_MEM_RST) & 0x80000000) {
  if (!timeout--)
    return -ETIME;
  msleep(1);
}
return 0;
}

So there is a msleep() in the loop but no mdelay() outside. Any idea why 
the mdelay() is needed here? Or what could be done regarding boot time 
with this?

Note: This is just a question, this shouldn't block the staging process.

Best regards

Dirk
Sascha Hauer - Sept. 14, 2012, 9:38 a.m.
On Fri, Sep 14, 2012 at 11:29:06AM +0200, Dirk Behme wrote:
> On 12.09.2012 12:31, Sascha Hauer wrote:
> >+
> >+       timeout = jiffies + msecs_to_jiffies(1000);
> >+       while (ipu_cm_read(ipu, IPU_MEM_RST) & 0x80000000) {
> >+               if (time_after(jiffies, timeout))
> >+                       return -ETIME;
> >+               cpu_relax();
> >+       }
> >+
> >+       mdelay(300);
>           ^^^^^^^^^^^^
> 
> >+       return 0;
> >+}
> 
> While doing some boot time measurement with i.MX6, we found that the
> above mdelay(300) is hurting regarding boot time. On i.MX6 you have
> two IPU instances, so in the end you get 600ms additional delay.
> 
> Looking at the Freescale code, this function looks like
> 
> static int ipu_reset(struct ipu_soc *ipu)
> {
> int timeout = 1000;
> 
> ipu_cm_write(ipu, 0x807FFFFF, IPU_MEM_RST);
> 
> while (ipu_cm_read(ipu, IPU_MEM_RST) & 0x80000000) {
>  if (!timeout--)
>    return -ETIME;
>  msleep(1);
> }
> return 0;
> }
> 
> So there is a msleep() in the loop but no mdelay() outside. Any idea
> why the mdelay() is needed here? Or what could be done regarding
> boot time with this?

I remember we had issues on i.MX51 or 53 without it, but I would have to
recheck it.
In any way, I think this should be reworked. The reset takes quite a
long time and it's not nice to block the boot process for so long.
Some asynchronous reset would be nice here.

Sascha
Eric Nelson - Sept. 18, 2012, 10:06 p.m.
Hi Sascha,

On 09/12/2012 03:31 AM, Sascha Hauer wrote:
> From: Philipp Zabel<p.zabel@pengutronix.de>
>
> Signed-off-by: Philipp Zabel<p.zabel@pengutronix.de>
> Signed-off-by: Sascha Hauer<s.hauer@pengutronix.de>
> ---
>   .../bindings/staging/imx-drm/fsl-imx-drm.txt       |   41 ++++++++++++++++++++
>   1 file changed, 41 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
>
> diff --git a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
> new file mode 100644
> index 0000000..07654f0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
> @@ -0,0 +1,41 @@
> +Freescale i.MX IPUv3
> +====================
> +
> +Required properties:
> +- compatible: Should be "fsl,<chip>-ipu"
> +- reg: should be register base and length as documented in the
> +  datasheet
> +- interrupts: Should contain sync interrupt and error interrupt,
> +  in this order.
> +- #crtc-cells: 1, See below
> +
> +example:
> +
> +ipu: ipu@18000000 {
> +	#crtc-cells =<1>;
> +	compatible = "fsl,imx53-ipu";
> +	reg =<0x18000000 0x080000000>;
> +	interrupts =<11 10>;
> +};
> +
> +Parallel display support
> +========================
> +
> +Required properties:
> +- compatible: Should be "fsl,imx-parallel-display"
> +- crtc: the crtc this display is connected to, see below
> +Optional properties:
> +- interface_pix_fmt: How this display is connected to the
> +  crtc. Currently supported types: "rgb24", "rgb565"
> +- edid: verbatim EDID data block describing attached display.
> +- ddc: phandle describing the i2c bus handling the display data
> +  channel
> +
> +example:
> +
> +display@di0 {
> +	compatible = "fsl,imx-parallel-display";
> +	edid = [edid-data];
> +	crtc =<&ipu 0>;
> +	interface-pix-fmt = "rgb24";
> +};

Do you have a working sample of [edid-data] for a parallel/LVDS/HDMI
display or know a good way to produce one?

Thanks in advance,


Eric
Shawn Guo - Sept. 19, 2012, 5:53 a.m.
On Wed, Sep 12, 2012 at 12:31:14PM +0200, Sascha Hauer wrote:
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/staging/imx-drm/TODO |   22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 drivers/staging/imx-drm/TODO
> 
> diff --git a/drivers/staging/imx-drm/TODO b/drivers/staging/imx-drm/TODO
> new file mode 100644
> index 0000000..2075646
> --- /dev/null
> +++ b/drivers/staging/imx-drm/TODO
> @@ -0,0 +1,22 @@
> +TODO:
> +- get DRM Maintainer review for this code
> +- Factor out more code to common helper functions
> +- decide where to put the base driver. It is not specific to a subsystem
> +  and would be used by DRM/KMS and media/V4L2
> +- convert irq driver to irq_domain_add_linear
> +
> +Missing features (not necessarily for moving out out staging):

out of?

> +
> +- Add KMS plane support for CRTC driver
> +- Add LDB (LVDS Display Bridge) support
> +- Add i.MX6 HDMI support
> +- Add support for IC (Image converter)
> +- Add support for CSI (CMOS Sensor interface)
> +- Add support for VDIC (Video Deinterlacer)
> +
> +Many work-in-progress patches for the above features exist. Contact
> +Sascha Hauer <kernel@pengutronix.de> if you are interested in working
> +on a specific feature.
> +
> +Please send any patches to Greg Kroah-Hartman <greg@kroah.com> and

Greg Kroah-Hartman <gregkh@linuxfoundation.org>?

> +Sascha Hauer <kernel@pengutronix.de>
> -- 
> 1.7.10.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Sascha Hauer - Sept. 19, 2012, 6:52 a.m.
On Tue, Sep 18, 2012 at 03:06:36PM -0700, Eric Nelson wrote:
> Hi Sascha,
> 
> On 09/12/2012 03:31 AM, Sascha Hauer wrote:
> >From: Philipp Zabel<p.zabel@pengutronix.de>
> >
> >Signed-off-by: Philipp Zabel<p.zabel@pengutronix.de>
> >Signed-off-by: Sascha Hauer<s.hauer@pengutronix.de>
> >---
> >  .../bindings/staging/imx-drm/fsl-imx-drm.txt       |   41 ++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
> >
> >+
> >+example:
> >+
> >+display@di0 {
> >+	compatible = "fsl,imx-parallel-display";
> >+	edid = [edid-data];
> >+	crtc =<&ipu 0>;
> >+	interface-pix-fmt = "rgb24";
> >+};
> 
> Do you have a working sample of [edid-data] for a parallel/LVDS/HDMI
> display or know a good way to produce one?

No, we are using the of videomode helper, see

http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg18618.html

It is not included here to not add a dependency on it. We are trying to
mainline this separately.

Sascha
Sascha Hauer - Sept. 19, 2012, 7:18 a.m.
On Wed, Sep 19, 2012 at 01:53:25PM +0800, Shawn Guo wrote:
> On Wed, Sep 12, 2012 at 12:31:14PM +0200, Sascha Hauer wrote:
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/staging/imx-drm/TODO |   22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >  create mode 100644 drivers/staging/imx-drm/TODO
> > 
> > diff --git a/drivers/staging/imx-drm/TODO b/drivers/staging/imx-drm/TODO
> > new file mode 100644
> > index 0000000..2075646
> > --- /dev/null
> > +++ b/drivers/staging/imx-drm/TODO
> > @@ -0,0 +1,22 @@
> > +TODO:
> > +- get DRM Maintainer review for this code
> > +- Factor out more code to common helper functions
> > +- decide where to put the base driver. It is not specific to a subsystem
> > +  and would be used by DRM/KMS and media/V4L2
> > +- convert irq driver to irq_domain_add_linear
> > +
> > +Missing features (not necessarily for moving out out staging):
> 
> out of?

Will fix.

> 
> > +
> > +- Add KMS plane support for CRTC driver
> > +- Add LDB (LVDS Display Bridge) support
> > +- Add i.MX6 HDMI support
> > +- Add support for IC (Image converter)
> > +- Add support for CSI (CMOS Sensor interface)
> > +- Add support for VDIC (Video Deinterlacer)
> > +
> > +Many work-in-progress patches for the above features exist. Contact
> > +Sascha Hauer <kernel@pengutronix.de> if you are interested in working
> > +on a specific feature.
> > +
> > +Please send any patches to Greg Kroah-Hartman <greg@kroah.com> and
> 
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>?

Hm, right. I copied this from another TODO file.

# find drivers/staging/ -name TODO | xargs grep greg@kroah.com | wc -l
20

Maybe Gregs address should be dropped here, he will show up in
get-maintainers.pl anyway.

Greg?

Sascha
Greg KH - Sept. 19, 2012, 8:05 a.m.
On Wed, Sep 19, 2012 at 09:18:03AM +0200, Sascha Hauer wrote:
> On Wed, Sep 19, 2012 at 01:53:25PM +0800, Shawn Guo wrote:
> > On Wed, Sep 12, 2012 at 12:31:14PM +0200, Sascha Hauer wrote:
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/staging/imx-drm/TODO |   22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >  create mode 100644 drivers/staging/imx-drm/TODO
> > > 
> > > diff --git a/drivers/staging/imx-drm/TODO b/drivers/staging/imx-drm/TODO
> > > new file mode 100644
> > > index 0000000..2075646
> > > --- /dev/null
> > > +++ b/drivers/staging/imx-drm/TODO
> > > @@ -0,0 +1,22 @@
> > > +TODO:
> > > +- get DRM Maintainer review for this code
> > > +- Factor out more code to common helper functions
> > > +- decide where to put the base driver. It is not specific to a subsystem
> > > +  and would be used by DRM/KMS and media/V4L2
> > > +- convert irq driver to irq_domain_add_linear
> > > +
> > > +Missing features (not necessarily for moving out out staging):
> > 
> > out of?
> 
> Will fix.
> 
> > 
> > > +
> > > +- Add KMS plane support for CRTC driver
> > > +- Add LDB (LVDS Display Bridge) support
> > > +- Add i.MX6 HDMI support
> > > +- Add support for IC (Image converter)
> > > +- Add support for CSI (CMOS Sensor interface)
> > > +- Add support for VDIC (Video Deinterlacer)
> > > +
> > > +Many work-in-progress patches for the above features exist. Contact
> > > +Sascha Hauer <kernel@pengutronix.de> if you are interested in working
> > > +on a specific feature.
> > > +
> > > +Please send any patches to Greg Kroah-Hartman <greg@kroah.com> and
> > 
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org>?
> 
> Hm, right. I copied this from another TODO file.
> 
> # find drivers/staging/ -name TODO | xargs grep greg@kroah.com | wc -l
> 20
> 
> Maybe Gregs address should be dropped here, he will show up in
> get-maintainers.pl anyway.
> 
> Greg?

Both end up in the same place, so it's not a big deal to leave my
kroah.com address in there, but for new files, use linuxfoundation.org
please.

thanks,

greg k-h
Eric Nelson - Sept. 19, 2012, 1:43 p.m.
On 09/18/2012 11:52 PM, Sascha Hauer wrote:
> On Tue, Sep 18, 2012 at 03:06:36PM -0700, Eric Nelson wrote:
>> Hi Sascha,
>>
>> On 09/12/2012 03:31 AM, Sascha Hauer wrote:
>>> From: Philipp Zabel<p.zabel@pengutronix.de>
>>>
>>> Signed-off-by: Philipp Zabel<p.zabel@pengutronix.de>
>>> Signed-off-by: Sascha Hauer<s.hauer@pengutronix.de>
>>> ---
>>>   .../bindings/staging/imx-drm/fsl-imx-drm.txt       |   41 ++++++++++++++++++++
>>>   1 file changed, 41 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
>>>
>>> +
>>> +example:
>>> +
>>> +display@di0 {
>>> +	compatible = "fsl,imx-parallel-display";
>>> +	edid = [edid-data];
>>> +	crtc =<&ipu 0>;
>>> +	interface-pix-fmt = "rgb24";
>>> +};
>>
>> Do you have a working sample of [edid-data] for a parallel/LVDS/HDMI
>> display or know a good way to produce one?
>
> No, we are using the of videomode helper, see
>
> http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg18618.html
>
> It is not included here to not add a dependency on it. We are trying to
> mainline this separately.
>
> Sascha
>

Thanks Sascha.