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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
