From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753859AbdA3SHf (ORCPT ); Mon, 30 Jan 2017 13:07:35 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:35984 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750948AbdA3SH2 (ORCPT ); Mon, 30 Jan 2017 13:07:28 -0500 Date: Mon, 30 Jan 2017 19:06:27 +0100 From: Daniel Vetter To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: Daniel Vetter , dri-devel@lists.freedesktop.org, thomas.petazzoni@free-electrons.com, "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2 0/2] drm: Add support for tiny LCD displays Message-ID: <20170130180627.4ay4zhyw7s54nekb@phenom.ffwll.local> Mail-Followup-To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= , dri-devel@lists.freedesktop.org, thomas.petazzoni@free-electrons.com, "linux-kernel@vger.kernel.org" References: <20170127195631.25265-1-noralf@tronnes.org> <20170130084424.wxdny2pzfmypvd4u@phenom.ffwll.local> <35663035-dd27-97b3-5a66-57bd1957a1a6@tronnes.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <35663035-dd27-97b3-5a66-57bd1957a1a6@tronnes.org> X-Operating-System: Linux phenom 4.8.0-1-amd64 User-Agent: NeoMutt/20161126 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 30, 2017 at 04:03:53PM +0100, Noralf Trønnes wrote: > > Den 30.01.2017 09.44, skrev Daniel Vetter: > > Hi Noralf, > > > > On Fri, Jan 27, 2017 at 08:56:29PM +0100, Noralf Trønnes wrote: > > > This is an attempt at providing a DRM version of drivers/staging/fbtft. > > > > > > The tinydrm library provides a very simplified view of DRM in particular > > > for tiny displays that has onboard video memory and is connected through > > > a slow bus like SPI/I2C. > > > > > > Only core patches this time. > > > > > > > > > Noralf. > > > > > > > > > Changes since version 1: > > > - Add tinydrm.rst > > > - Set tdev->fbdev_cma=NULL on unregister (lastclose is called after that). > > Hm, this sounds like a buglet in the drm framework ... how do we call > > lastclose when the driver is disappearing? I do see a drm_lastclose call > > at the beginning of drm_dev_unregister (which we might want to remove for > > KMS drivers, it doesn't make much sense imo), but that shouldn't result in > > troubles. > > Ah, I see, I'm tearing down fbdev before unregistering drm. > That should be reversed. > > static void tinydrm_unregister(struct tinydrm_device *tdev) > { > drm_crtc_force_disable_all(tdev->drm); > > if (tdev->fbdev_cma) { > drm_fbdev_cma_fini(tdev->fbdev_cma); > tdev->fbdev_cma = NULL; > } > > drm_dev_unregister(tdev->drm); > } > > > > - Remove some DRM_DEBUG*() > > > - Write-combined memory has uncached reads, so speed up by copying/buffering > > > one pixel line before conversion. > > Hm, why are you using write-combining memory? Or is that needed so that > > you can (if available) use hw spi engines? > > That comes with using the CMA helper: > drm_fbdev_cma_create_with_funcs() -> drm_gem_cma_create() -> dma_alloc_wc() > > Here's a comment I have added in the code for why I set the DMA mask on > the SPI device and why it will work: > > /* > * Even though it's not the SPI device that does DMA (the master does), > * the dma mask is necessary for the dma_alloc_wc() in > * drm_gem_cma_create(). The dma_addr returned will be a physical > * adddress which might be different from the bus address, but this is > * not a problem since the address will not be used. > * The virtual address is used in the transfer and the SPI core > * re-maps it on the SPI master device using the DMA streaming API > * (spi_map_buf()). > */ > > > Either way, I think this all looks good, pls submit a pull request to Dave > > with these two patches as soon as latest drm-misc has landed (I'll send a > > pull request for that later today). > > I'm confused, I thought you wanted the core patches through drm-misc > and the rest as a pull request to Dave. > > This is what you said: > > Looks all pretty. A bunch of ideas below, but all optional. For merging > I > think simplest to first get the core patches in through drm-misc, and > then > you can submit a pull request to Dave for tinydrm+backends (just needs > an > ack for the dt parts from dt maintainers), including MAINTAINERS entry. > Ack from my side. Ah, I thought we already have all the prep patches you need merged into drm-misc. Still the same plan. > > Another one: Do you want to maintain tinydrm as part of the drm-misc > > group, i.e. want commit rights there? That would also help a bit with > > pushing all your great drm refactoring patches through the machinery ... > > My goal is to port staging/fbtft to drm. Whether or not I will do work > in drm core (refactoring) besides the tinydrm drivers when that is > accomplished, I don't know. Working on drm as a hobbyist is very > difficult (for me at least) because it is very complex, it changes all > the time and on top of that it has a high volume mailinglist. > It takes effort to keep up. > > So I will probably end up doing only tinydrm maintanence. > As for how that is best done, I don't know. Once I have covered a 3-4 > controller types, a new driver is just a copy of a similar one with a > different register initialization. This means that it's easy to review > patches. They will all look the same, more or less. > So for me it's ofc best if I can review the patches and then commit > them myself. As for my own patches, if I need that tit for tat > reviewing to get them in, it will be difficult for me to review other > drivers because I have no idea how a GPU operates, and to keep up with > drm best pratices will be next to impossible for me. I think you're slightly understating your knowledge about the display side of drm here a bit :-) We're (for now) also only talking about getting small display drivers into drm-misc. > If the Device Tree guys allows me to add fbtft support to tinydrm, then > there won't be much activity once the fbtft drivers have been ported > over. The uncertainty stems from the fact that the fbtft drivers are > written as controller drivers, but they contain panel specific things > like register setup and how to do rotation. > So compatible = "fb_ili9341", supports a controller with a specific > panel, but it can support other panels through the 'init' DT property > which encodes register values and delays (which is a no-no). I guess getting minimal cross-review going on with panel drivers would still be great. I'm also trying to get Thierry's drm_panel tree into drm-misc, but that hasn't really happened. There's also the nice thing with once you have a group, it's much easier to load-balance with someone else when you're not around ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch