Messages in this thread | | | From | Linus Walleij <> | Date | Tue, 11 Apr 2017 11:17:51 +0200 | Subject | Re: [PATCH v5] drm/pl111: Initial drm/kms driver for pl111 |
| |
On Tue, Apr 11, 2017 at 3:18 AM, Eric Anholt <eric@anholt.net> wrote:
> From: Tom Cooksey <tom.cooksey@arm.com>
Well that can be debated at this point. I think it should have your Author: tag and just Tom in the Signed-off-by, then mention the history in the commit message.
> This is a modesetting driver for the pl111 CLCD display controller > found on various ARM platforms such as the Versatile Express. The > driver has only been tested on the bcm911360_entphn platform so far, > with PRIME-based buffer sharing between vc4 and clcd. > > It reuses the existing devicetree binding, while not using quite as > many of its properties as the fbdev driver does (those are left for > future work). > > v2: Nearly complete rewrite by anholt, cutting 2/3 of the code thanks > to DRM core's excellent new helpers. > v3: Don't match pl110 any more, don't attach if we don't have a DRM > panel, use DRM_GEM_CMA_FOPS, update MAINTAINERS, use the simple > display helper, use drm_gem_cma_dumb_create (same as our wrapper). > v4: Change the driver's .name to not clash with fbdev in sysfs, drop > platform alias, drop redundant "drm" in DRM driver name, hook up > .prepare_fb to the CMA helper so that DMA fences should work. > v5: Move register definitions inside the driver directory, fix build > in COMPILE_TEST and !AMBA mode. > > Signed-off-by: Tom Cooksey <tom.cooksey@arm.com> > Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
This is as good starting point as any. We need to get moving with this. Some minor things below that can just as well be fixed later.
> create mode 100644 drivers/gpu/drm/pl111/Kconfig
Maybe someone would want to place this under drivers/gpu/drm/arm since it is obviously an ARM block. But since ARM has pretty much dropped the ball on this IP it is probably best to keep it separate like this.
> +++ b/drivers/gpu/drm/pl111/Kconfig > @@ -0,0 +1,12 @@ > +config DRM_PL111 > + tristate "DRM Support for PL111 CLCD Controller" > + depends on DRM > + depends on ARM || ARM64 || COMPILE_TEST > + select DRM_KMS_HELPER > + select DRM_KMS_CMA_HELPER > + select DRM_GEM_CMA_HELPER > + select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE > + help > + Choose this option for DRM support for the PL111 CLCD controller. > + If M is selected the module will be called pl111_drm.
I must say the driver is *really* slim and readable with all the new helpers from DRM, good job all who refactored the DRM support for simple framebuffer systems.
> + panel = of_drm_find_panel(panel_node); > + of_node_put(panel_node);
Was meaning to ask whether the panel bindings used by the fbdev drivers are 100% compatible with what DRM is using and from your code it seems like yes, they are?
> +irqreturn_t pl111_irq(int irq, void *data) > +{ > + struct pl111_drm_dev_private *priv = data; > + u32 irq_stat; > + irqreturn_t status = IRQ_NONE; > + > + irq_stat = readl(priv->regs + CLCD_PL111_MIS); > + > + if (!irq_stat) > + return IRQ_NONE; > + > + if (irq_stat & CLCD_IRQ_NEXTBASE_UPDATE) { > + drm_crtc_handle_vblank(&priv->pipe.crtc); > + > + status = IRQ_HANDLED; > + } > + > + /* Clear the interrupt once done */ > + writel(irq_stat, priv->regs + CLCD_PL111_ICR); > + > + return status; > +}
And this is the first time in history that the vblank interrupt on PL11x is actually used for something, which in itself is a good reason to migrate to this driver.
Yours, Linus Walleij
| |