mripard / sunxi-mali

GNU General Public License v2.0
100 stars 54 forks source link

Making it work with tinydrm #18

Closed sergey-suloev closed 6 years ago

sergey-suloev commented 6 years ago

Hello guys

trying to make my SPI 3.5 inch display work with tinydrm and mali. I already have my display working with tinydrm! Double buffering seems working too.

But I am getting memory issues with mali:

[ 126.000839] Session started [ 126.001490] Mali<5>: [ 126.001498] _mali_ukk_mem_bind, vaddr=0x10000000, size =0x4b000! [ 126.001530] Mali: ERR: /home/sergey/Projects/drivers/sunxi/orangepipc/sunxi-mali/r6p2/src/devicedrv/mali/common/mali_mem_validation.c [ 126.001534] mali_mem_validation_check() 62

[ 126.001538] MALI PHYSICAL RANGE VALIDATION ERROR: The range supplied was: phys_base=0x715A1000, size=0x0004B000

[ 126.001543] Mali<1>: [ 126.001545] Bind external buf failed [ 126.001551] Mali<1>: [ 126.001554] _mali_ukk_mem_bind, return ERROR! [ 126.002206] Mali<4>: [ 126.002213] _mali_ukk_mem_allocate, vaddr=0x10000000, size =0x40000!

Any ideas what this would mean ? I can see that Mali is trying to bind a buffer provided but it cannot. Any possible reason for that ?

giuliobenetti commented 6 years ago

Hi @sergey-suloev, can you pastebin your dts where you declare you cma?

And can you pastebin your complete dmesg? Because at a certain point it should call mali_mem_validation_add_range where it writes: "Memory Validator installed for Mali physical address base=0x%08X, size=0x%08X\n"

So we can check why it fails on mali_mem_validation_check.

Reading on code in mali_mem_validation.c, you pass first 2 if conditions on mali_mem_validation_check. But after I don't know what's your mali_mem_validator.phys_base and .size and I think that it fails there.

giuliobenetti commented 6 years ago

I sent a Pull Request with more verbosity on those data. You can try to recompile with those commits and check all addresses and sizes.

sergey-suloev commented 6 years ago

@giuliobenetti ok, I am going to try your change

mripard commented 6 years ago

This is likely because you haven't declared a shared memory pool between the display and the GPU. This is what the reserved-memory region and properties are about.

I'd say you need to put the reserved-memory property on the display device in the DT, but I'm not 100% sure about how tinydrm works exactly.

sergey-suloev commented 6 years ago

@mripard you're right, I don't have memory region declared in display node. I didn't even try it because sun4i DRM module works fine w/o it. Problem with tiny drm that it doesn't have any dedicated DT node like "tinydrm-display-engine".

mripard commented 6 years ago

How do you probe tinydrm then?

sergey-suloev commented 6 years ago

@mripard tinydrm is a set of core modules which are loaded along with your display driver module. all memory allocations are done inside tinydrm.

https://github.com/orpaltech/antenna-analyzer-armbian/blob/master/build/userpatches/kernel/sunxi-next/board_orangepipc/DRM-driver-for-Waveshare-3.5inch-display.patch

mripard commented 6 years ago

Right, but that's just the driver. You also need a matching device for the driver to probe, and judging from that driver, the node would have the compatible waveshare,waveshare35a.

sergey-suloev commented 6 years ago

@mripard it is here https://github.com/orpaltech/antenna-analyzer-armbian/blob/master/build/userpatches/kernel/sunxi-next/board_orangepipc/arch/arm/boot/dts/overlay/sun8i-h3-waveshare35a-drm.dts

mripard commented 6 years ago

Then add the reserved memory property to that node.

sergey-suloev commented 6 years ago

@mripard I tried that, but error was the same. I think tinydrm is ignoring that reference.

mripard commented 6 years ago

Can you try the following patch: http://code.bulix.org/ruh5or-242911?raw

sergey-suloev commented 6 years ago

@mripard ok thanks does it require putting memory region into my SPI driver ?

mripard commented 6 years ago

Yes, this is just so that it isn't ignored anymore.

sergey-suloev commented 6 years ago

drivers/gpu/drm/tinydrm/core/tinydrm-core.c: In function ‘tinydrm_init’: drivers/gpu/drm/tinydrm/core/tinydrm-core.c:148:2: error: implicit declaration of function ‘of_reserved_mem_device_init’; did you mean ‘resume_device$ of_reserved_mem_device_init(parent); ^~~~~~~ resume_device_irqs drivers/gpu/drm/tinydrm/core/tinydrm-core.c: In function ‘tinydrm_fini’: drivers/gpu/drm/tinydrm/core/tinydrm-core.c:178:2: error: implicit declaration of function ‘of_reserved_mem_device_release’ [-Werror=implicit-functio$ of_reserved_mem_device_release(parent); ^~~~~~~~~~

I think I forgot some include

mripard commented 6 years ago

of_reserved.h probably

sergey-suloev commented 6 years ago

yes, the following will fix it

include <linux/of_reserved_mem.h>

mripard commented 6 years ago

Sorry, it's not really clear, but did it solve your issue or is it compiling now (or both?)

Le lun. 18 déc. 2017 à 17:17, orpaltech notifications@github.com a écrit :

yes, the following will fix it

include <linux/of_reserved_mem.h>

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mripard/sunxi-mali/issues/18#issuecomment-352475431, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWen1zQA5Bp18j7XpWtRtsFE4irarhhks5tBpAVgaJpZM4Q8k4K .

sergey-suloev commented 6 years ago

@mripard it is compiling now but I have another issue unrelated to this one that makes my build fail as soon as it is resolved I'm gonna report about results

mripard commented 6 years ago

Ok, let me know

Le lun. 18 déc. 2017 à 18:02, orpaltech notifications@github.com a écrit :

@mripard https://github.com/mripard it is compiling now but I have another issue unrelated to this one that makes my build fail as soon as it is resolved I'm gonna report about results

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mripard/sunxi-mali/issues/18#issuecomment-352488472, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWenwM9sG9y8mbBmHLcNY4EOfxXrldWks5tBpo8gaJpZM4Q8k4K .

sergey-suloev commented 6 years ago

@mripard oops while loading the driver

sergey-suloev commented 6 years ago

@mripard well.. this is really strange. somteimes the driver is loading fine but sometimes it is oopsing

sergey-suloev commented 6 years ago

[ 9.329629] waveshare35a spi0.0: assigned reserved memory node linux,cma [ 9.330730] platform mali-utgard: assigned reserved memory node linux,cma

mripard commented 6 years ago

Can you provide your dt, and the whole logs when it is oopsing?

Le lun. 18 déc. 2017 à 20:41, orpaltech notifications@github.com a écrit :

[ 9.329629] waveshare35a spi0.0: assigned reserved memory node linux,cma [ 9.330730] platform mali-utgard: assigned reserved memory node linux,cma

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mripard/sunxi-mali/issues/18#issuecomment-352536315, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWen_5K2Yu1m09eWlNhEoQpk4vjind8ks5tBr_WgaJpZM4Q8k4K .

sergey-suloev commented 6 years ago

well.. it is oopsing only sometimes and therefore it seems like an SPI driver issue... here is my uart log

minicom_12_18_2017-2.log minicom_12_18_2017.log

sergey-suloev commented 6 years ago

https://github.com/orpaltech/antenna-analyzer-armbian/blob/master/build/userpatches/kernel/sunxi-next/board_orangepipc/Add-sun8i-h3-overlays.patch

overlay is named "waveshare35a-drm"

sergey-suloev commented 6 years ago

https://github.com/orpaltech/antenna-analyzer-armbian/blob/master/build/userpatches/kernel/sunxi-next/DRM-driver-for-Waveshare-3.5inch-display.patch

here is my driver it is actually a copy of this one https://github.com/notro/tinydrm/blob/master/piscreen.c

sergey-suloev commented 6 years ago

here is my dmesg dmesg_12_18_2017.txt

seems like the problem persists even after using cma

sergey-suloev commented 6 years ago

It has a lot of SPI timeouts during boot. That needs to be improved.

root@aapi-hn77laj:~# dmesg | grep "spi" [ 9.557451] waveshare35a spi0.0: assigned reserved memory node linux,cma [ 9.558139] [drm] Initialized waveshare35a 1.0.0 20171208 for spi0.0 on minor 0 [ 10.651437] spi_master spi0: spi0.0: timeout transferring 614400 bytes@16000000Hz for 630(614)ms [ 10.651710] waveshare35a spi0.0: SPI transfer failed: -110 [ 10.652326] spi_master spi0: failed to transfer one message from queue [ 10.652649] waveshare35a spi0.0: Failed to update display -110 [ 10.661872] waveshare35a spi0.0: fb0: frame buffer device [ 10.662199] ads7846 spi0.1: spi0.1 supply vcc not found, using dummy regulator [ 10.663783] ads7846 spi0.1: touchscreen, irq 59 [ 10.667299] input: ADS7846 Touchscreen as /devices/platform/soc/1c68000.spi/spi_master/spi0/spi0.1/input/input0 [ 10.992341] spi_master spi0: spi0.0: timeout transferring 307200 bytes@16000000Hz for 320(307)ms [ 10.992714] waveshare35a spi0.0: SPI transfer failed: -110 [ 10.993049] spi_master spi0: failed to transfer one message from queue [ 11.120277] spi_master spi0: spi0.0: timeout transferring 2 bytes@10000000Hz for 110(100)ms [ 11.120287] waveshare35a spi0.0: SPI transfer failed: -110 [ 11.120305] spi_master spi0: failed to transfer one message from queue [ 11.230246] spi_master spi0: spi0.0: timeout transferring 2 bytes@10000000Hz for 110(100)ms [ 11.230260] waveshare35a spi0.0: SPI transfer failed: -110 [ 11.230284] spi_master spi0: failed to transfer one message from queue [ 11.340243] spi_master spi0: spi0.0: timeout transferring 2 bytes@10000000Hz for 110(100)ms [ 11.340257] waveshare35a spi0.0: SPI transfer failed: -110 [ 11.340277] spi_master spi0: failed to transfer one message from queue

giuliobenetti commented 6 years ago

Is it ok to discuss hardware bound to drivers here? Next questions are useful to understand if it’s a hardware issue or driver- If it’s off topic can someone suggest a place to discuss such topic? Thanks

Can you try to lower frequency to 5Mhz? How long are wires? What is max frequency supported by controller? Did you put series resistor like 33ohm on spi lines? Can you check that sck line or other lines don’t drift their 0-level from ground with a scope?

Spi should fail only on reads. On write, controller shouldn’t care almost about anything about hardware. And in your driver it seems you only write on spi.

Last question, have you checked also h3 max spi speed?

Let me know

sergey-suloev commented 6 years ago

Hi @giuliobenetti why do you think it is a hardware issue ? I have a similar overlay which is using FBTFT engine from staging and there are no issues at all (on the same hardware). https://github.com/orpaltech/antenna-analyzer-armbian/blob/fa32e47905ffcd99584e2ff75aec6a112d5d0337/build/userpatches/kernel/sunxi-next/board_orangepipc/Add-sun8i-h3-overlays.patch#L960

Something make me think that it is still a software issue. I may be wrong.

sergey-suloev commented 6 years ago

@giuliobenetti I have checked the datasheet and there is nothing about speed. I guess it is pretty much typical for such type of hardware, i.e. 16Mhz should be acceptable.

sergey-suloev commented 6 years ago

See what notro has answered

https://github.com/notro/tinydrm/issues/7#issuecomment-352573500

giuliobenetti commented 6 years ago

@sergey-suloev i didn’t understand it was working with same hardware. So ok. It made me think about hardware because of randomness of the problem against a simple peripheral like spi, and I’ve experienced similar issues in the past regarding high speed clocks.

sergey-suloev commented 6 years ago

@giuliobenetti well, it is of course all related to hardware.. but my point is that the software is putting the hardware in a not operational mode

mripard commented 6 years ago

@sergey-suloev it seems like getting your SPI setup is a prerequisite.

And then, it seems that our current approach doesn't really match what tinydrm does with regard to memory allocations. And the switch to vmalloc @notro suggested won't help either.

God, fbdev is awful.

sergey-suloev commented 6 years ago

@mripard so what is your vision about ways to go ? is this possible at all to make tinydrm and gpu work together with affordable efforts ?

as for SPI yes, it should be tuned. as soon as FBTFT driver works then there is nothing wrong with hardware, IMHO.

mripard commented 6 years ago

I'm not sure that the GPU can cope with vmalloc'd buffers, so at the moment it's unclear to me if that can even work in the first place.

notro commented 6 years ago

Isn't this what PRIME is for? gpu driver creates a buffer for rendering and tinydrm imports it for scanout. Why the need for fbdev?

mripard commented 6 years ago

PRIME is supposed to cover that, however, that's based on the assumption that you're using GEM buffers.

However, when the Mali blob wants to use the fbdev interface, it will mmap the fbdev buffer and directly draws into it. This is not really smart, but that's the way it is.

There's some checks in place in the kernel driver to make sure that the userspace blob will not ask the GPU to render in a memory area that it's not supposed to, and this is what will be triggered in our case, since the fbdev buffer was not allocated within the memory region we told the mali driver was usable.

I guess one could disable these checks, but I don't really want to leave the whole memory accessible through the GPU.

BTW, how does tinydrm work? Using a mechanism similar to the fbdev deferred io, setting up a page fault handler on the buffers to know when it has been redrawn and can be sent over SPI/I2C?

notro commented 6 years ago

tinydrm relies on userspace to call the DRM_IOCTL_MODE_DIRTYFB ioctl to tell which region it has updated. The drm_framebuffer_funcs->dirty function then flushes this region over SPI to the display controller onboard ram. For fbdev emulation it uses fbdev deferred I/O to call into the dirty callback using drm_fb_helper_deferred_io().

Currently tinydrm uses the cma helper, but I'm about to move it to vmalloc buffers so it can import buffers that isn't physically continuous, making it work with all gpu drivers (at least those that can provide a virtual address).

sergey-suloev commented 6 years ago

I tried to remove memory check https://github.com/sergey-suloev/sunxi-mali/commit/d3876d695ccba60528c7604413b8e92cb61f5b46 , it worked with no errors but also without affecting the screen (no changes on the screen but cursor jump to the middle of the screen). Looks like tinydrm is still missing some functionality. dmesg_12_19_2017.txt

mripard commented 6 years ago

@notro Which device is it using for the CMA allocations then? I guess from your previous reply the SPI master?

notro commented 6 years ago

It's a hack really, it uses the SPI child:

int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
              struct gpio_desc *dc)
{
    struct device *dev = &spi->dev;
...
    /*
     * 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()).
     */
    if (!dev->coherent_dma_mask) {
        ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
        if (ret) {
            dev_warn(dev, "Failed to set dma mask %d\n", ret);
            return ret;
        }
    }

It should probably have used the spi master as you guessed, but anyhow it's going away in a few weeks time.

sergey-suloev commented 6 years ago

@mripard @notro To summarize all this. As far as I understand, even if we remove that memory address limitation tinydrm + mali is not going to work together, correct ? Can you explain, please, what is missing in tinydrm ? thanks

P.S. Below is my system info and dmesg while running test app

http://sprunge.us/RJjf

BTW, I made my SPI driver work stable by removing this line: https://github.com/notro/tinydrm/blob/0c9ad04e35422ac44596666c99bed298476eaa39/piscreen.c#L133

Also changed this value to 250, because it is used in the FBTFT's counterpart. https://github.com/notro/tinydrm/blob/0c9ad04e35422ac44596666c99bed298476eaa39/piscreen.c#L93

sergey-suloev commented 6 years ago

@notro @mripard Hi guys is this really impossible this time to push the thing and make it work somehow? Does it require tons
of coding ?

mripard commented 6 years ago

It's unclear the amount of work that would be needed. Nothing is ever impossible, but I have close to zero interest in doing that myself. Feel free to work on it though.

sergey-suloev commented 6 years ago

ok, let's close it then here. I may work on it on my own.

notro commented 6 years ago

However, when the Mali blob wants to use the fbdev interface, it will mmap the fbdev buffer and directly draws into it. This is not really smart, but that's the way it is.

If you're trying to do what Maxime outlines here, then I don't think that will ever work. Unless Mali is able to work with a vmalloc buffer, which I guess would require an IOMMU and someone to make it work.

However, if you go the DRM route and export a PRIME buffer from the mali DRM driver, then it should be possible to import that into tinydrm for scanout.

How to actually do that with regular apps is beyond me, but here's a guy that made it work with the vc4 driver: https://github.com/anholt/linux/issues/10#issuecomment-343576158

sergey-suloev commented 6 years ago

@notro so in both cases mali driver should be modified ??