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

[Linus Walleij]

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