| Message ID | 1339681407-28721-1-git-send-email-s.hauer@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
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
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; > +}
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 */ } > +};
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 */ } > > +};
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
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
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
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
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; > +}
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; > +}
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
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
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.
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
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