Re: [PATCH v4 1/5] drm/rockchip: Add basic drm driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Mark,

You'll find some comments inline.
Anyway, I wouldn't call it a review (your driver is using some concepts
I'm not used to, like IOMMUs) but rather a collection of nitpicks :-).

I haven't been through the whole driver yet, but I'll get back to it
soon ;-).

And remember this is a 2 way thing, I wait for your review too
(here is the last version of my driver [1]) :-)


On Mon, 22 Sep 2014 18:48:54 +0800
Mark yao <mark.yao@xxxxxxxxxxxxxx> wrote:

> This patch adds the basic structure of a DRM Driver for Rockchip Socs.
> 
> Signed-off-by: Mark yao <mark.yao@xxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - use the component framework to defer main drm driver probe
>   until all VOP devices have been probed.
> - use dma-mapping API with ARM_DMA_USE_IOMMU, create dma mapping by
>   master device and each vop device can shared the drm dma mapping.
> - use drm_crtc_init_with_planes and drm_universal_plane_init.
> - remove unnecessary middle layers.
> - add cursor set, move funcs to rockchip drm crtc.
> - use vop reset at first init
> - reference framebuffer when used and unreference when swap out vop
> 
> Changes in v3:
> - change "crtc->fb" to "crtc->primary-fb"
> Adviced by Daniel Vetter
> - init cursor plane with universal api, remove unnecessary cursor set,move 
> 
> Changes in v4:
> Adviced by David Herrmann
> - remove drm_platform_*() usage, use register drm device directly.
> Adviced by Rob Clark
> - remove special mmap ioctl, do userspace mmap with normal mmap() or mmap offset
> 
>  drivers/gpu/drm/Kconfig                       |    2 +
>  drivers/gpu/drm/Makefile                      |    1 +
>  drivers/gpu/drm/rockchip/Kconfig              |   19 +
>  drivers/gpu/drm/rockchip/Makefile             |   10 +
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  524 ++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |  120 +++
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |  201 ++++
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.h    |   28 +
>  drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c |  231 +++++
>  drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h |   20 +
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c   |  404 ++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.h   |   72 ++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 1372 +++++++++++++++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h   |  187 ++++
>  include/uapi/drm/rockchip_drm.h               |   75 ++
>  15 files changed, 3266 insertions(+)
>  create mode 100644 drivers/gpu/drm/rockchip/Kconfig
>  create mode 100644 drivers/gpu/drm/rockchip/Makefile
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_drv.h
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fb.h
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.h
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_gem.h
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>  create mode 100644 include/uapi/drm/rockchip_drm.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index b066bb3..7c4c3c6 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -171,6 +171,8 @@ config DRM_SAVAGE
>  
>  source "drivers/gpu/drm/exynos/Kconfig"
>  
> +source "drivers/gpu/drm/rockchip/Kconfig"
> +
>  source "drivers/gpu/drm/vmwgfx/Kconfig"
>  
>  source "drivers/gpu/drm/gma500/Kconfig"
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 4a55d59..d03387a 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_DRM_VMWGFX)+= vmwgfx/
>  obj-$(CONFIG_DRM_VIA)	+=via/
>  obj-$(CONFIG_DRM_NOUVEAU) +=nouveau/
>  obj-$(CONFIG_DRM_EXYNOS) +=exynos/
> +obj-$(CONFIG_DRM_ROCKCHIP) +=rockchip/
>  obj-$(CONFIG_DRM_GMA500) += gma500/
>  obj-$(CONFIG_DRM_UDL) += udl/
>  obj-$(CONFIG_DRM_AST) += ast/
> diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
> new file mode 100644
> index 0000000..7146c80
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/Kconfig
> @@ -0,0 +1,19 @@
> +config DRM_ROCKCHIP
> +	tristate "DRM Support for Rockchip"
> +	depends on DRM && ROCKCHIP_IOMMU
> +	select ARM_DMA_USE_IOMMU
> +	select IOMMU_API
> +	select DRM_KMS_HELPER
> +	select DRM_KMS_FB_HELPER
> +	select DRM_PANEL
> +	select FB_CFB_FILLRECT
> +	select FB_CFB_COPYAREA
> +	select FB_CFB_IMAGEBLIT
> +	select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
> +	select VIDEOMODE_HELPERS
> +	help
> +	  Choose this option if you have a Rockchip soc chipset.
> +	  This driver provides kernel mode setting and buffer
> +	  management to userspace. This driver does not provides
> +	  2D or 3D acceleration; acceleration is performed by other
> +	  IP found on the SoC.
> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> new file mode 100644
> index 0000000..6e6d468
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/Makefile
> @@ -0,0 +1,10 @@
> +#
> +# Makefile for the drm device driver.  This driver provides support for the
> +# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
> +
> +ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/rockchip

Do you really need those specific CFLAGS (AFAIK, these path are
already set, though you'll have to include <drm/xxx.h> instead of
"xxx.h" if you're referencing drm headers, but you're already
referencing the correct patch anyway) ?

> +
> +rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o rockchip_drm_fbdev.o \
> +		rockchip_drm_gem.o rockchip_drm_vop.o
> +
> +obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> new file mode 100644
> index 0000000..94926cb
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -0,0 +1,524 @@
> +/*
> + * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> + * Author:Mark Yao <mark.yao@xxxxxxxxxxxxxx>
> + *
> + * based on exynos_drm_drv.c
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +

[...]

> +
> +static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags)
> +{
> +	struct rockchip_drm_private *private;
> +	struct dma_iommu_mapping *mapping;
> +	struct device *dev = drm_dev->dev;
> +	int ret;
> +
> +	private = devm_kzalloc(drm_dev->dev, sizeof(*private), GFP_KERNEL);
> +	if (!private)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(drm_dev->dev, dev);
> +	drm_dev->dev_private = private;
> +
> +	drm_mode_config_init(drm_dev);
> +
> +	rockchip_drm_mode_config_init(drm_dev);
> +
> +	dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
> +				      GFP_KERNEL);
> +	if (!dev->dma_parms) {
> +		ret = -ENOMEM;
> +		goto err_config_cleanup;
> +	}
> +
> +	/* TODO(djkurtz): fetch the mapping start/size from somewhere */
> +	mapping = arm_iommu_create_mapping(&platform_bus_type, 0x10000000,
> +					   SZ_1G);
> +	if (IS_ERR(mapping)) {
> +		ret = PTR_ERR(mapping);
> +		goto err_config_cleanup;
> +	}
> +
> +	dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> +	dma_set_max_seg_size(dev, 0xffffffffu);
> +
> +	ret = arm_iommu_attach_device(dev, mapping);
> +	if (ret)
> +		goto err_release_mapping;
> +
> +	/* Try to bind all sub drivers. */
> +	ret = component_bind_all(dev, drm_dev);
> +	if (ret)
> +		goto err_detach_device;
> +
> +	/* init kms poll for handling hpd */
> +	drm_kms_helper_poll_init(drm_dev);
> +
> +	/*
> +	 * enable drm irq mode.
> +	 * - with irq_enabled = true, we can use the vblank feature.
> +	 */
> +	drm_dev->irq_enabled = true;
> +
> +	/*
> +	 * with vblank_disable_allowed = true, vblank interrupt will be disabled
> +	 * by drm timer once a current process gives up ownership of
> +	 * vblank event.(after drm_vblank_put function is called)
> +	 */
> +	drm_dev->vblank_disable_allowed = true;
> +
> +	ret = drm_vblank_init(drm_dev, ROCKCHIP_MAX_CRTC);
> +	if (ret)
> +		goto err_kms_helper_poll_fini;
> +
> +	rockchip_drm_fbdev_init(drm_dev);
> +
> +	/* force connectors detection */
> +	drm_helper_hpd_irq_event(drm_dev);
> +
> +	return 0;
> +
> +err_kms_helper_poll_fini:
> +	drm_kms_helper_poll_fini(drm_dev);
> +	component_unbind_all(dev, drm_dev);
> +err_detach_device:
> +	arm_iommu_detach_device(dev);
> +err_release_mapping:
> +	arm_iommu_release_mapping(dev->archdata.mapping);
> +err_config_cleanup:
> +	drm_mode_config_cleanup(drm_dev);
> +	drm_dev->dev_private = NULL;
> +	dev_set_drvdata(dev, NULL);


Not sure you need to set driver data to NULL.


> +	return ret;
> +}
> +

[...]

> +
> +#ifdef CONFIG_PM_SLEEP
> +static int rockchip_drm_sys_suspend(struct device *dev)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +	pm_message_t message;
> +
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
> +	message.event = PM_EVENT_SUSPEND;
> +
> +	return rockchip_drm_suspend(drm_dev, message);
> +}
> +
> +static int rockchip_drm_sys_resume(struct device *dev)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +
> +	if (pm_runtime_suspended(dev))

You meant

	if (!pm_runtime_suspended(dev))

right ?

BTW, I see the same mistake in exynos driver [2]


> +		return 0;
> +
> +	return rockchip_drm_resume(drm_dev);
> +}
> +#endif
> +
> +static const struct dev_pm_ops rockchip_drm_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(rockchip_drm_sys_suspend,
> +				rockchip_drm_sys_resume)
> +};
> +
> +int rockchip_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc,
> +			  struct device_node *np)
> +{
> +	struct rockchip_drm_private *priv = drm->dev_private;
> +	struct device_node *port;
> +	int pipe;
> +
> +	if (priv->num_pipe >= ROCKCHIP_MAX_CRTC)
> +		return -EINVAL;
> +
> +	port = of_get_child_by_name(np, "port");
> +	of_node_put(np);

Not sure you should call of_node_put on a node pointer passed as an
argument (unless you previously called of_node_get which is not the case
in this function)...


Best Regards,

Boris

[1]http://thread.gmane.org/gmane.comp.video.dri.devel/114064
[2]https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/exynos/exynos_drm_drv.c?id=refs/tags/next-20140922#n373
-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux