PineVideo ignores the GraphicsV instance

While looking at why installing a GraphicsV driver on a PineBook causes a hard-lock, I’ve spotted PineVideo has the GraphicsV instance check commented out in its GraphicsV driver:

GraphicsV_Handler
        Push    "lr"
;        LDRB    lr, instance         ; may need another if more than 1 graphics module
;        TEQ     lr, R4, LSR #24      ; our display?
;        Pull    "pc", NE

I’m sure that’s probably just an oversight when it was uploaded.

I don’t think PineVideo is registering itself correctly either – it just claims GraphicsV using OS_Claim, while other video drivers like BCMVideo also register using OS_ScreenMode 64 and OS_ScreenMode 65, as described here.

There’s a further issue with the jump table on the proceeding lines, which does not clear the Head/Overlay from bits 16-23 in r4 before using it for the jump address:

        BIC     r4, r4, #&ff000000   ; clear display number
        CMP     r4, #(GraphicsV_TableEnd - GraphicsV_Table) / 4
        ADDLO   pc, pc, r4, LSL #2

I wonder if it would be worth rewriting PineVideo from scratch. My understanding is that the there’s very little code that’s actually used and most features are actually unimplemented. As such, it feels reasonable to start again in C instead of continuing to work with the old one.

This is something I was looking at doing a while ago, but I don’t know if I have the right skills for it. For now though, I submitted a merge request a month ago to remove most of the unused code that was left over from other drivers, so hopefully that might be a bit easier to work with.

I wonder if it would be worth rewriting PineVideo from scratch

That’s probably not required as I don’t think many of the GraphicsV drivers are written in C. I suspect GraphicsV was retro-fitted into the existing drivers and as there’s no C template for a GraphicsV driver devs have just copy/pasted an existing driver instead of starting from scratch in C – which is understandable.

If you already have a merge request submitted, could you resolve the minor issues noted above as part of that?

I don’t think any of the GraphicsV drivers are written in C

Well except for OMAPVideo, OMAP4Video, NVidia (mixed C/assembler), GC320Video, UDLVideo, and OMAPHDMI, none of the GraphicsV drivers are written in C.

The only ones written in assembler are VIDC20Video because it was scraped from the pre-HAL kernel, and BCMVideo/PineVideo/IMXVideo which all appear to share a common initial author who presumably enjoys tracking down stack imbalances and manually allocating more registers than I have fingers. Each to their own.

If you already have a merge request submitted, could you resolve the minor issues noted above as part of that?

The merge request has grown quite large, so I’d rather keep it to just the dead code removal to make code review easier. I can have a go at sorting out those issues separately, though.

GC320Video

Slight tangent, but could you provide a quick summary of what features are supported by the GC320Video driver? e.g. does it have multiple overlays, what pixel formats does it support, is GV_Render implemented…

OK, I’ve opened a new merge request: https://gitlab.riscosopen.org/RiscOS/Sources/Video/HWSupport/PineVideo/-/merge_requests/2

I’ve opened a new merge request

Great. Should line 85 not be clearing the top 8 bits through? ie.

EOR     lr, R4, lr, LSL #24

Did you double-check code further on isn’t reliant on R4 having the top 16 bits cleared? I don’t expect it will be referenced beyond the jump table, but best to be sure.

Interesting feedback, and thanks. The PineVideo driver could indeed do with some improvement as indicated above. At the time of coding I hadn’t encountered the OS_ScreenMode calls. I’ll see what I can do in the next week or few ref that.
To ease testing do any of you have code snippets etc that specifically exercise these issues?
Thanks
John

Cameron. Just looked at your MR.. Again useful. I’m not personably enabled to merge formally. All I can do is as you… And get very frustrated when, for example, a short while after initially supplying code to ROOL for this port I needed to provide updated programmers etc in the HAL. These programmers enable the nightly builds to be readily installed, BUT… merge doesnt happen

Just looked at your MR.. Again useful. I’m not personably enabled to merge formally. All I can do is as you… And get very frustrated

From the review comments, it looks like the PineVideo change raised in this thread wouldn’t work. LSL versus LSR that Jon noted is still there.

a short while after initially supplying code to ROOL for this port I needed to provide updated programmers etc in the HAL. These programmers enable the nightly builds to be readily installed, BUT… merge doesnt happen

I spent an hour or so reviewing the original merge request, which was subsequently closed with 2 unresolved threads and the source project deleted. A similar one was opened 1h20m later, then closed, and finally the current one which has…the same code in that I commented on originally.

So the Git safeguards all seem to be working – ROOL aren’t merging unfinished/non working code.

To ease testing do any of you have code snippets etc that specifically exercise these issues?

No code snippets, but if you download ADFFS 2.83 and simply run it – the system will hard-lock if PineVideo isn’t checking the instance or installing itself correctly via OS_ScreenMode.

If you plan to implement overlays, ADFFS 2.84 beta uses overlays to upscale legacy Modes, instead of relying on the machine/monitor to support their resolutions as 2.83 does.

From the review comments, it looks like the PineVideo change raised in this thread wouldn’t work. LSL versus LSR that Jon noted is still there.

I’ve updated the MR to include a fix for that.

Unfortunately, both ADFFS 2.83 and 2.84 beta still lock up on startup even with these changes, which suggests that the issue might be something else. I get slightly different results depending on whether or not I’ve reselected the screen mode as described here, but the overall effect is the same.

PineVideo at present does not attempt to do overlays. Agreed it would be nice for the to me added.

@Sprow:

I spent an hour or so reviewing the original merge request, which was subsequently closed with 2 unresolved threads and the source project deleted. A similar one was opened 1h20m later, then closed, and finally the current one which has…the same code in that I commented on originally.

So the Git safeguards all seem to be working – ROOL aren’t merging unfinished/non working code.

These updates have been sanitised and resubmitted. The original MR has been deleted as I’m not familiar enough with the minutiae of git to be certain ‘you got what I intended’.

It would be helpful if the updates could be merged, resulting in ROOL’s autobuilder being able to produce output, including programmers, that enable the rom too be directly and easily installed.

Unfortunately, both ADFFS 2.83 and 2.84 beta still lock up on startup even with these changes, which suggests that the issue might be something else

If it’s any help, the ADFFS start-up code does the following:

1. Requests the current GraphicsV driver via OS_ScreenMode 11
2. Asks that existing GraphicsV driver for its Features via GraphicsV 8
3. Installs the ADFFS GraphicsV driver via OS_ScreenMode 64
4. Starts a 50Hz VSync timer via RTSupport
5. Initialises the ADFFS GraphicsV driver via OS_ScreenMode 65
6. Starts the ADFFS GraphicsV driver via OS_ScreenMode 11

At this point all GraphicsV calls are redirected to the existing GraphicsV driver, until a legacy mode is entered.

Do you have a minimal example that just does GraphicsV passthrough? It might help with ruling out certain possibilities.

Also, is this issue specific to the Pinebook and RK3399 ports or do other devices have similar issues?

Do you have a minimal example that just does GraphicsV passthrough?

It’s not particularily easy to rip the GraphicsV driver out of ADFFS as there are a lot of dependencies.

I have a Pinebook Pro on order to help progress this.

These updates have been sanitised and resubmitted. The original MR has been deleted as I’m not familiar enough with the minutiae of git to be certain ‘you got what I intended’.

I wouldn’t dismiss them as minutiae of git, it’s the basic operations used during a merge request, ROOL even produced a cheat sheet with illustrations to help (or without illustrations if you’re a command line masochist).

Following these steps keeps the discussion together, otherwise we get the situation as here where:

1. The original merge request was deleted with 2 unresolved threads
2. Its replacement opened with the same issue I’d previously queried still present, Jeffrey spent time reviewing, then it was deleted with 2 unresolved threads (but not the same 2 as before)
3. Then the latest incarnation I note still has the same thing I queried originally, plus some new issues accidentally undoing Cameron’s intervening changes

Making 1+1+4=6 unresolved threads spread across 3 merge requests, 2 of which the originating project is now gone – makes it harder than it needs to be following what’s going on.

I see Cameron submitted a C rewrite of PineVideo, which I feel is definitely progress in the right direction.

As there’s now three versions of this driver, a closed-source ASM for the PBP, an open-source ASM for the PB and now a C version for PB, some cooperation is probably needed to agree the way forward.

I would also consider renaming the Module to MaliVideo as this is a Mali driver and not specific to Pine’s except for the hardcoded resolutions, which can hopefully be removed if EDID support can be implemented.

I recently purchased a Pinebook Pro so I could look at enhancing this driver, in particular adding a hardware cursor and overlay support. Re-writing it in C was going to be my first step, so Cameron has done me a favour there. The problem however is the Mali documentation appears to be under NDA and where ARM have open-sourced older 32-bit drivers, they appear to rely on binary blobs for low-level.

I would also consider renaming the Module to MaliVideo as this is a Mali driver and not specific to Pine’s except for the hardcoded resolutions, which can hopefully be removed if EDID support can be implemented.

I don’t think it’s using the Mali GPU at all – my limited understanding is that the PineVideo driver exclusively uses the display controller on the Allwinner A64 SoC, which should be usable for implementing overlays. It’s described in the datasheets on the Pine64 website, and NetBSD has KMS/DRM drivers for it that could be useful as a reference.

As there’s now three versions of this driver, a closed-source ASM for the PBP, an open-source ASM for the PB and now a C version for PB, some cooperation is probably needed to agree the way forward.

I suspect that the Pinebook Pro video driver is unrelated to PineVideo since the RK3399 is likely to have its own display controller, and because boards like the ROCKPro64 and Rock Pi 4 probably require proper mode switching support. If both of them have the same name, it would be a good idea for the RK3399 driver to be renamed.

The C version should be able to replace the old one, but I’m not 100% sure how to approach converting the DMA code.

I don’t think it’s using the Mali GPU at all

I expect you’re correct. My confusion has come from the RK3399 documentation, which details Mali but doesn’t go into a lot of detail about any other display driver, except for a brief mention of tx_invid0 in an example of changing to 640×480. It refers to it as “VideoSampler” – I’m not sure if that’s analogous to VideoCore on the Pi, or a subsystem. The block diagram has VOPB and VOPL as the video sources, although there’s no mention of what they are outside of the diagram.

Looking at the PBP OS build, I can see it’s video driver is “rk3399video”. So the driver we’re talking about here “PineVideo”, is specific to the Pinebook. The code for bother however look close enough that they could probably be merged into one unified PineVideo that compiles to either the PB or PBP ROM builds.

I’m still working on getting a working PBP, so am not at the point I can look at the video driver. I need Partition Manager to build a working boot drive, ideally with a boot menu and dual-boot Linux, although I need to tweak the ROM so it will play nice with U-Boot. Once that’s done, I’ll move onto the video driver.

I appreciate I’ll be looking at the RK3399 video driver, where this is the Allwinner A64 video driver, but I suspect they’re not fundamentally that different. Once I find the relevant TRM’s, I’ll know more.

The code for bother however look close enough that they could probably be merged into one unified PineVideo that compiles to either the PB or PBP ROM builds.

I think the similarity is mainly due to the fact that PineVideo barely talks to the hardware at all. I suspect what is happening is that u-boot is doing all the hard work on setting up the display controller for the text console, and all that PineVideo is doing is providing the address of the framebuffer to RISC OS. It would explain why it doesn’t support any features that would require changing the framebuffer (such as resolution changes, other pixel formats and double-buffering).

I would be surprised if the A64 and the RK3399 were actually similar enough to justify having a single driver for both, given that both were made by different companies and have separate drivers on Linux and the BSDs.