Re: [PATCH v5] drm/pl111: Initial drm/kms driver for pl111

From: Linus Walleij
Date: Tue Apr 11 2017 - 05:18:05 EST


On Tue, Apr 11, 2017 at 3:18 AM, Eric Anholt <eric@xxxxxxxxxx> wrote:

> From: Tom Cooksey <tom.cooksey@xxxxxxx>

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@xxxxxxx>
> Signed-off-by: Eric Anholt <eric@xxxxxxxxxx>

Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

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