Message-ID: <35663035-dd27-97b3-5a66-57bd1957a1a6@tronnes.org>
Date:   Mon, 30 Jan 2017 16:03:53 +0100
From:   Noralf Trønnes <noralf@...nnes.org>
To:     Daniel Vetter <daniel@...ll.ch>
Cc:     dri-devel@...ts.freedesktop.org,
        thomas.petazzoni@...e-electrons.com,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 0/2] drm: Add support for tiny LCD displays


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.

> 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.

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).

Noralf.

Powered by blists - more mailing lists