From: Emil Velikov <emil.l.velikov@gmail.com> To: Kevin Tang <kevin3.tang@gmail.com> Cc: David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, orsonzhai@gmail.com, ML dri-devel <dri-devel@lists.freedesktop.org>, "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>, zhang.lyra@gmail.com, baolin.wang@linaro.org Subject: Re: [PATCH RFC 2/8] drm/sprd: add Unisoc's drm kms master Date: Tue, 10 Dec 2019 16:06:53 +0000 [thread overview] Message-ID: <CACvgo50K=43OHW33i8ZsMG3QvuVxLZSo0iBMSt0Z-X4N2eTObw@mail.gmail.com> (raw) In-Reply-To: <1575966995-13757-3-git-send-email-kevin3.tang@gmail.com> Welcome to DRM Kevin, On Tue, 10 Dec 2019 at 08:40, Kevin Tang <kevin3.tang@gmail.com> wrote: > > From: Kevin Tang <kevin.tang@unisoc.com> > > 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
WARNING: multiple messages have this Message-ID (diff)
From: Emil Velikov <emil.l.velikov@gmail.com> To: Kevin Tang <kevin3.tang@gmail.com> Cc: baolin.wang@linaro.org, David Airlie <airlied@linux.ie>, zhang.lyra@gmail.com, "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>, ML dri-devel <dri-devel@lists.freedesktop.org>, orsonzhai@gmail.com Subject: Re: [PATCH RFC 2/8] drm/sprd: add Unisoc's drm kms master Date: Tue, 10 Dec 2019 16:06:53 +0000 [thread overview] Message-ID: <CACvgo50K=43OHW33i8ZsMG3QvuVxLZSo0iBMSt0Z-X4N2eTObw@mail.gmail.com> (raw) In-Reply-To: <1575966995-13757-3-git-send-email-kevin3.tang@gmail.com> Welcome to DRM Kevin, On Tue, 10 Dec 2019 at 08:40, Kevin Tang <kevin3.tang@gmail.com> wrote: > > From: Kevin Tang <kevin.tang@unisoc.com> > > 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-12-10 16:08 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-10 8:36 [PATCH RFC 0/8] Add Unisoc's drm kms module Kevin Tang 2019-12-10 8:36 ` Kevin Tang 2019-12-10 8:36 ` [PATCH RFC 1/8] dt-bindings: display: add Unisoc's drm master bindings Kevin Tang 2019-12-10 8:36 ` Kevin Tang 2019-12-13 9:42 ` Maxime Ripard 2019-12-13 9:42 ` Maxime Ripard 2019-12-10 8:36 ` [PATCH RFC 2/8] drm/sprd: add Unisoc's drm kms master Kevin Tang 2019-12-10 8:36 ` Kevin Tang 2019-12-10 10:32 ` Thomas Zimmermann 2019-12-10 10:32 ` Thomas Zimmermann 2019-12-10 12:38 ` tang pengchuan 2019-12-10 12:47 ` Thomas Zimmermann 2019-12-10 12:47 ` Thomas Zimmermann 2019-12-10 13:42 ` tang pengchuan 2019-12-11 3:00 ` tang pengchuan 2019-12-10 10:41 ` Daniel Vetter 2019-12-10 10:41 ` Daniel Vetter 2019-12-10 11:40 ` tang pengchuan 2019-12-10 16:06 ` Emil Velikov [this message] 2019-12-10 16:06 ` Emil Velikov 2019-12-10 22:01 ` Daniel Vetter 2019-12-10 22:01 ` Daniel Vetter 2019-12-10 8:36 ` [PATCH RFC 3/8] dt-bindings: display: add Unisoc's dpu bindings Kevin Tang 2019-12-10 8:36 ` Kevin Tang 2019-12-13 9:44 ` Maxime Ripard 2019-12-13 9:44 ` Maxime Ripard 2019-12-10 8:36 ` [PATCH RFC 4/8] drm/sprd: add Unisoc's drm display controller driver Kevin Tang 2019-12-10 8:36 ` Kevin Tang 2019-12-10 10:44 ` Thomas Zimmermann 2019-12-10 10:44 ` Thomas Zimmermann 2019-12-10 11:29 ` tang pengchuan 2019-12-10 11:32 ` tang pengchuan 2019-12-10 17:13 ` Emil Velikov 2019-12-10 17:13 ` Emil Velikov 2019-12-11 1:18 ` tang pengchuan 2019-12-11 11:46 ` Emil Velikov 2019-12-11 11:46 ` Emil Velikov 2019-12-10 8:36 ` [PATCH RFC 5/8] dt-bindings: display: add Unisoc's mipi dsi&dphy bindings Kevin Tang 2019-12-10 8:36 ` Kevin Tang 2019-12-13 9:46 ` Maxime Ripard 2019-12-13 9:46 ` Maxime Ripard 2019-12-10 8:36 ` [PATCH RFC 6/8] drm/sprd: add Unisoc's drm mipi dsi&dphy driver Kevin Tang 2019-12-10 8:36 ` Kevin Tang 2019-12-10 8:36 ` [PATCH RFC 7/8] dt-bindings: display: add Unisoc's generic mipi panel bindings Kevin Tang 2019-12-10 8:36 ` Kevin Tang 2019-12-13 9:48 ` Maxime Ripard 2019-12-13 9:48 ` Maxime Ripard 2019-12-10 8:36 ` [PATCH RFC 8/8] drm/sprd: add Unisoc's drm generic mipi panel driver Kevin Tang 2019-12-10 8:36 ` Kevin Tang 2019-12-10 18:54 ` [PATCH RFC 0/8] Add Unisoc's drm kms module Sam Ravnborg 2019-12-10 18:54 ` Sam Ravnborg 2019-12-12 14:29 ` tang pengchuan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CACvgo50K=43OHW33i8ZsMG3QvuVxLZSo0iBMSt0Z-X4N2eTObw@mail.gmail.com' \ --to=emil.l.velikov@gmail.com \ --cc=airlied@linux.ie \ --cc=baolin.wang@linaro.org \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=kevin3.tang@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=orsonzhai@gmail.com \ --cc=zhang.lyra@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.