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.