Re: [PATCH v2 0/2] drm: Add support for tiny LCD displays

[Daniel Vetter <daniel@ffwll.ch>]

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