Re: [PATCH RFC 2/8] drm/sprd: add Unisoc's drm kms master

[Emil Velikov]

Welcome to DRM Kevin,

On Tue, 10 Dec 2019 at 08:40, Kevin Tang <kevin3.tang@xxxxxxxxx> wrote:
>
> From: Kevin Tang <kevin.tang@xxxxxxxxxx>
>
> Adds drm support for the Unisoc's display subsystem.
>
> This is drm device and gem driver. This driver provides support for the
> Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>
Did you use XFree86 or Xorg to test this? The XFree86 codebase have been
missing for years.

Out of curiosity - did you try any Wayland, or bare-metal compositor?

>  source "drivers/gpu/drm/mcde/Kconfig"
>
> +source "drivers/gpu/drm/sprd/Kconfig"
> +
>  # Keep legacy drivers last
>
>  menuconfig DRM_LEGACY
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9f1c7c4..85ca211 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -122,3 +122,4 @@ obj-$(CONFIG_DRM_LIMA)  += lima/
>  obj-$(CONFIG_DRM_PANFROST) += panfrost/
>  obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/
>  obj-$(CONFIG_DRM_MCDE) += mcde/
> +obj-$(CONFIG_DRM_SPRD) += sprd/
> diff --git a/drivers/gpu/drm/sprd/Kconfig b/drivers/gpu/drm/sprd/Kconfig
> new file mode 100644
> index 0000000..79f286b
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/Kconfig
> @@ -0,0 +1,14 @@
> +config DRM_SPRD
> +       tristate "DRM Support for Unisoc SoCs Platform"
> +       depends on ARCH_SPRD
> +       depends on DRM && OF
> +       select DRM_KMS_HELPER
> +       select DRM_GEM_CMA_HELPER
> +       select DRM_KMS_CMA_HELPER
> +       select DRM_MIPI_DSI
> +       select DRM_PANEL
> +       select VIDEOMODE_HELPERS
> +       select BACKLIGHT_CLASS_DEVICE
> +       help
> +         Choose this option if you have a Unisoc chipsets.
s/chipsets/chipset/ ?


> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ccflags-y += -Iinclude/drm
> +
> +subdir-ccflags-y += -I$(src)
> +
I think we can drop the includes here, unless there's a specific error
message that you're setting.


> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_drm.c

> +#define DRIVER_NAME    "sprd"
> +#define DRIVER_DESC    "Spreadtrum SoCs' DRM Driver"
> +#define DRIVER_DATE    "20180501"
The date is mostly for cosmetic purposes. Yet we're nearly 2020 and
this reads 2018 - update?

<snip>

> +static struct drm_driver sprd_drm_drv = {
> +       .driver_features        = DRIVER_GEM | DRIVER_MODESET |
> +                                 DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
There is no modeset exposed by the driver, let alone an atomic one.

Thus I would drop the following code from this patch and add it with a
patch that uses it.
 - tokens - DRIVER_MODESET, DRIVER_ATOMIC)
 - no-op modeset/atomic functions just above, and
 - vblank/kms code (further down) in bind/unbind


<snip>

> +static int sprd_drm_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +
> +       ret = dma_set_mask_and_coherent(&pdev->dev, ~0);
> +       if (ret)
> +               DRM_ERROR("dma_set_mask_and_coherent failed (%d)\n", ret);
> +
Is the hardware going to work correctly the dma call fails? Should we
use "return ret;" here?

> +       return sprd_drm_component_probe(&pdev->dev, &sprd_drm_component_ops);
> +}
> +
> +static int sprd_drm_remove(struct platform_device *pdev)
> +{
> +       component_master_del(&pdev->dev, &sprd_drm_component_ops);
> +       return 0;
> +}
> +
> +static void sprd_drm_shutdown(struct platform_device *pdev)
> +{
> +       struct drm_device *drm = platform_get_drvdata(pdev);
> +
> +       if (!drm) {
Can this happen in reality?

<snip>

> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_drm.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019 Unisoc Inc.
> + */
> +
> +#ifndef _SPRD_DRM_H_
> +#define _SPRD_DRM_H_
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_print.h>
> +
> +struct sprd_drm {
> +       struct drm_device *drm;

> +       struct drm_atomic_state *state;
> +       struct device *dpu_dev;
> +       struct device *gsp_dev;
These three are unused. Please add alongside the code which is using them.


> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_gem.c

As Thomas pointed out, this is effectively a copy of drm_gem_cma_helper.c
Please drop this file and use the respective CMA functions, instead.


> diff --git a/drivers/gpu/drm/sprd/sprd_gem.h b/drivers/gpu/drm/sprd/sprd_gem.h
> new file mode 100644
> index 0000000..4c10d8a
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_gem.h
Just like the C file - this is effectively a copy of the existing CMA codebase.

HTH
Emil