Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754216AbbHMUoL (ORCPT ); Thu, 13 Aug 2015 16:44:11 -0400 Received: from gabe.freedesktop.org ([131.252.210.177]:34821 "EHLO gabe.freedesktop.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752958AbbHMUoJ (ORCPT ); Thu, 13 Aug 2015 16:44:09 -0400 From: Eric Anholt To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, Stephen Warren , Lee Jones , linux-kernel@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 3/7] drm/vc4: Add KMS support for Raspberry Pi. In-Reply-To: <20150813075141.GX17734@phenom.ffwll.local> References: <1439427380-2436-1-git-send-email-eric@anholt.net> <1439427380-2436-4-git-send-email-eric@anholt.net> <20150813075141.GX17734@phenom.ffwll.local> User-Agent: Notmuch/0.20.2 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Thu, 13 Aug 2015 13:44:03 -0700 Message-ID: <87d1yqq5po.fsf@eliezer.anholt.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10474 Lines: 283 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Daniel Vetter writes: > On Wed, Aug 12, 2015 at 05:56:16PM -0700, Eric Anholt wrote: >> This is the start of a full VC4 driver. Right now this just supports >> configuring the display using a pre-existing video mode (because >> changing the pixel clock isn't available yet, and doesn't work when it >> is). However, this is enough for fbcon and bringing up X using >> xf86-video-modesetting. >>=20 >> Signed-off-by: Eric Anholt >> --- >> drivers/gpu/drm/Kconfig | 2 + >> drivers/gpu/drm/Makefile | 1 + >> drivers/gpu/drm/vc4/Kconfig | 14 + >> drivers/gpu/drm/vc4/Makefile | 18 ++ >> drivers/gpu/drm/vc4/vc4_bo.c | 54 ++++ >> drivers/gpu/drm/vc4/vc4_crtc.c | 583 +++++++++++++++++++++++++++++++= +++ >> drivers/gpu/drm/vc4/vc4_debugfs.c | 38 +++ >> drivers/gpu/drm/vc4/vc4_drv.c | 249 +++++++++++++++ >> drivers/gpu/drm/vc4/vc4_drv.h | 123 +++++++ >> drivers/gpu/drm/vc4/vc4_hdmi.c | 651 +++++++++++++++++++++++++++++++= +++++++ >> drivers/gpu/drm/vc4/vc4_hvs.c | 172 ++++++++++ >> drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++ >> drivers/gpu/drm/vc4/vc4_plane.c | 320 +++++++++++++++++++ >> drivers/gpu/drm/vc4/vc4_regs.h | 562 ++++++++++++++++++++++++++++++++ >> 14 files changed, 2871 insertions(+) >> create mode 100644 drivers/gpu/drm/vc4/Kconfig >> create mode 100644 drivers/gpu/drm/vc4/Makefile >> create mode 100644 drivers/gpu/drm/vc4/vc4_bo.c >> create mode 100644 drivers/gpu/drm/vc4/vc4_crtc.c >> create mode 100644 drivers/gpu/drm/vc4/vc4_debugfs.c >> create mode 100644 drivers/gpu/drm/vc4/vc4_drv.c >> create mode 100644 drivers/gpu/drm/vc4/vc4_drv.h >> create mode 100644 drivers/gpu/drm/vc4/vc4_hdmi.c >> create mode 100644 drivers/gpu/drm/vc4/vc4_hvs.c >> create mode 100644 drivers/gpu/drm/vc4/vc4_kms.c >> create mode 100644 drivers/gpu/drm/vc4/vc4_plane.c >> create mode 100644 drivers/gpu/drm/vc4/vc4_regs.h > > Made a quick pass and found a few things to update to latest drm > developments. Of course didn't look at the hardware details since no clue, > but looks really nice overall. > -Daniel If you have anything about the hardware that you were curious about, I'd be interested in trying to explain them in the comments to the extent that I can. It's unfortunate that we haven't shipped docs for the display side of things, but had to do a lot of reading of the verilog just to get this far, anyway. >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >> index c46ca31..1730a76 100644 >> --- a/drivers/gpu/drm/Kconfig >> +++ b/drivers/gpu/drm/Kconfig >> @@ -240,3 +240,5 @@ source "drivers/gpu/drm/sti/Kconfig" >> source "drivers/gpu/drm/amd/amdkfd/Kconfig" >>=20=20 >> source "drivers/gpu/drm/imx/Kconfig" >> + >> +source "drivers/gpu/drm/vc4/Kconfig" >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile >> index 5713d05..b991ac5 100644 >> --- a/drivers/gpu/drm/Makefile >> +++ b/drivers/gpu/drm/Makefile >> @@ -42,6 +42,7 @@ obj-$(CONFIG_DRM_MGA) +=3D mga/ >> obj-$(CONFIG_DRM_I810) +=3D i810/ >> obj-$(CONFIG_DRM_I915) +=3D i915/ >> obj-$(CONFIG_DRM_MGAG200) +=3D mgag200/ >> +obj-$(CONFIG_DRM_VC4) +=3D vc4/ >> obj-$(CONFIG_DRM_CIRRUS_QEMU) +=3D cirrus/ >> obj-$(CONFIG_DRM_SIS) +=3D sis/ >> obj-$(CONFIG_DRM_SAVAGE)+=3D savage/ >> diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig >> new file mode 100644 >> index 0000000..130cc94 >> --- /dev/null >> +++ b/drivers/gpu/drm/vc4/Kconfig >> @@ -0,0 +1,14 @@ >> +config DRM_VC4 >> + tristate "Broadcom VC4 Graphics" >> + depends on ARCH_BCM2835 >> + depends on DRM >> + select DRM_KMS_HELPER >> + select DRM_KMS_FB_HELPER >> + select DRM_KMS_CMA_HELPER > > drm-misc/linux-next already has Archit's patches to enable/disable fbdev > in the core code, so you don't need to bother about these selects here any > more, it'll no-op out if drm fbdev emulation isn't enabled. Since you're > reusing cma fbdev helpers I don't think there's any need for other changes > because of this. It sounds like I should rebase on that, then? >> + help >> + Choose this option if you have a system that has a Broadcom >> + VC4 GPU, such as the Raspberry Pi or other BCM2708/BCM2835. >> + >> + This driver requires that "avoid_warnings=3D2" be present in >> + the config.txt for the firmware, to keep it from smashing >> + our display setup. >> diff --git a/drivers/gpu/drm/vc4/Makefile b/drivers/gpu/drm/vc4/Makefile >> new file mode 100644 >> index 0000000..4aa07ca >> --- /dev/null >> +++ b/drivers/gpu/drm/vc4/Makefile >> @@ -0,0 +1,18 @@ >> +ccflags-y :=3D -Iinclude/drm >> + >> +# Please keep these build lists sorted! >> + >> +# core driver code >> +vc4-y :=3D \ >> + vc4_bo.o \ >> + vc4_crtc.o \ >> + vc4_drv.o \ >> + vc4_kms.o \ >> + vc4_hdmi.o \ >> + vc4_hvs.o \ >> + vc4_plane.o \ >> + $() >> + >> +vc4-$(CONFIG_DEBUG_FS) +=3D vc4_debugfs.o >> + >> +obj-$(CONFIG_DRM_VC4) +=3D vc4.o >> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c >> new file mode 100644 >> index 0000000..fee8cac >> --- /dev/null >> +++ b/drivers/gpu/drm/vc4/vc4_bo.c >> @@ -0,0 +1,54 @@ >> +/* >> + * Copyright =C2=A9 2015 Broadcom >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +/* DOC: VC4 GEM BO management support. >> + * >> + * The VC4 GPU architecture (both scanout and rendering) has direct >> + * access to system memory with no MMU in between. To support it, we >> + * use the GEM CMA helper functions to allocate contiguous ranges of >> + * physical memory for our BOs. >> + */ > > Since you're doing kerneldoc considered pulling it all into a new vc4 > section in the drm docbook template? I hadn't found the docbook template. Interesting. I'll try to cook up some general vc4 docs for that. I think that could be a separate commit, though? >> + >> +#include "vc4_drv.h" >> + >> +struct vc4_bo *vc4_bo_create(struct drm_device *dev, size_t size) >> +{ >> + struct drm_gem_cma_object *cma_obj; >> + >> + cma_obj =3D drm_gem_cma_create(dev, size); >> + if (IS_ERR(cma_obj)) >> + return NULL; >> + else >> + return to_vc4_bo(&cma_obj->base); >> +} >> + >> +int vc4_dumb_create(struct drm_file *file_priv, >> + struct drm_device *dev, >> + struct drm_mode_create_dumb *args) >> +{ >> + int min_pitch =3D DIV_ROUND_UP(args->width * args->bpp, 8); >> + struct vc4_bo *bo =3D NULL; >> + int ret; >> + >> + if (args->pitch < min_pitch) >> + args->pitch =3D min_pitch; >> + >> + if (args->size < args->pitch * args->height) >> + args->size =3D args->pitch * args->height; >> + >> + mutex_lock(&dev->struct_mutex); >> + bo =3D vc4_bo_create(dev, roundup(args->size, PAGE_SIZE)); >> + mutex_unlock(&dev->struct_mutex); > > I'm on a struct_mutex crusade (trying to get rid of it in core and allow > drivers to live without it). On a quick look there doesn't seem to be > anything that needs struct_mutex here, so please just remove it. If there > is indeed something vc4-internal you want to protect, please use your own > driver-internal mutex (e.g. for drm_mm or command submission or whatever). > > btw the last bit in the drm core for modern drivers that needs > struct_mutex is mmap_offset gem object lookup. I plan to replace that with > kref_get_unless_zero trickery, which would make the core and a lot of > drivers struct_mutex free and so relegate it mostly to a legacy role (and > can be forgotten). Struct mutex is here because this code is from the V3D series, with the in-kernel BO cache ripped out (it turns out that the CMA allocator is slow, and you can't just userspace cache since we have to do allocations within the kernel to the tune of a couple per draw and that's too much). I'll pull the mutex calls out for now until the cache stuff is submitted. >> +static bool vc4_crtc_mode_fixup(struct drm_crtc *crtc, >> + const struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted_mode) >> +{ >> + return true; >> +} > > mode_fixup on crtcs is optional since 840bfe953384a and I just merged a > patch to make it optional for encoders too (when using atomic helpers > which you do). You can remove them both. Great! It felt like there was a *lot* of boilerplate when I was first writing this stuff, and things are way better than they used to be. >> +static int vc4_drm_load(struct drm_device *dev, unsigned long flags) >> +{ >> + struct vc4_dev *vc4; >> + int ret; >> + >> + vc4 =3D devm_kzalloc(dev->dev, sizeof(*vc4), GFP_KERNEL); >> + if (!vc4) >> + return -ENOMEM; >> + >> + dev_set_drvdata(dev->dev, dev); >> + vc4->dev =3D dev; >> + dev->dev_private =3D vc4; >> + >> + drm_mode_config_init(dev); >> + >> + ret =3D component_bind_all(dev->dev, dev); >> + if (ret) >> + return ret; >> + >> + vc4_kms_load(dev); >> + >> + return 0; >> +} > > ->load has backwards init ordering (we register public interfaces before > calling it, yay) because backwards compat. Hence deprecated, please use > drm_dev_alloc(); ... driver init including drm_dev_set_unique(); > drm_dev_register(); instead and drop ->load. Will do. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJVzQGUAAoJELXWKTbR/J7ojOEP/0/7h0tv8tyHDWe4tyDrNvax /oedU6OgxAd8u5uuU2JJ9OFEF8TN8o9Z9ncBJRfM7qrYFb0rElJGAJKLQ4ZFJ/z7 QZHbfc5ECUf1Yfhm3ASOw15vKKEE2+8NFd/7pdlbaSUqVQ03TAH3YjZFISLzYg6i bVY0paEET7rXntF9Sql/9N2XShFVf1z/a4iPNNRJBKweTjSMEMxdyD0dOpIFCJT/ 8Cpt0dH8eyBtkDVbIEVtKr6mmUH+k3jYHHEY92GKi8caeXQozWKRy382LneC0hdA fZgIdefqVk6ZqsTcukjIpGQiX4o87xYfRUGohv2t1e+0YE1N8dwhapdh0SPX7kdF cwYsLAsCEwxMBrKHshxz77AcKIus5dTRj6SKUQQatqQlhrN5yUFTO+9Hr2ranR9m wFadWQykKt/OMUhiozMj1+nbNx5ANHTvsuzDJ1c/K0CwutMQwkGD11YJ6AYzsA1n wNcaEIaiIKC4n/fpDVK3P2o5iu+fesKQj5mAG0Pf0gLR1S8KQXuBrVcs1+14lsm0 e3hqVSjFcfq5QbhZkPhI7wdtctgOn0owGbuAEbQ9qIRmK2fr8PuJSuIZSMyCC8i4 7Bhj+ZGbNqyYASiMGYqmZ+B2v1aKOQ5lVsWBvaqISUARLIubVpPoTuX0OFn6gwRf AN8dWnJA+U5IIxgG89CI =UKfA -----END PGP SIGNATURE----- --=-=-=-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/