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

[Daniel Vetter]

On Tue, Dec 10, 2019 at 04:06:53PM +0000, Emil Velikov wrote:
> 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?

Just noticed that the driver also seems to be missing fbdev emulation.
Given that that is a one-line (for a proper driver using all the right
helpers) it should be added. See drm_fbdev_generic_setup().
-Daniel

> 
> >  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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch